fix: preserve projection metadata during optimization by gene-bordegaray · Pull Request #24670 · apache/datafusion

gene-bordegaray · 2026-08-25T14:08:21Z

Closes Projection field metadata is lost during physical plan optimization #24721

There were four ways metadata could disappear.

1. Removing a metadata-only identity projection

Consider:

ProjectionExec: i@0 AS i
  output field metadata = {"event_field": "true"}

  DataSourceExec: i
    field metadata = {}

The check to remove the projection asked:

Is every expression a column?
Does column 0 remain column 0?
Does the alias match the column name?
Does the number of columns match?

All answers yes so optimizer removed projection:

DataSourceExec: i
 metadata = {}

Metadata lost.

2 Collapsing across a metadata boundary

Consider:

ProjectionExec: arrow_metadata(i, 'event_field') AS metadata

  ProjectionExec: i@0 AS i
    output metadata = {"event_field": "true"}

    DataSourceExec: i
      metadata = {}

The correct result is true.

The previous projection colapse logic would substitute the outer expression through the inner projection:

ProjectionExec: arrow_metadata(i, 'event_field') AS metadata

 DataSourceExec: i
   metadata = {}

Now the func sees the scan field instead of the inner projection field giving use result as NULL now.

3 Rebuilding a projection with a new child

Some optimizer paths replace the child of a projection:

Old:
ProjectionExec(metadata={"key": "value"})

 OldChild

---

New:
ProjectionExec(...?)

 NewChild

The previous make_with_child implementation did this:

ProjectionExec::try_new(projection.expr().to_vec(), new_child)

where try_new derives the output schema from the expressions and new child so it woudlnt retain metadata from the original projection.

4 Cast target metadata lost before optimization

This one was a little confusing because main passed the UUID metadata test, but the first version of this PR did not (@gabotechs this is what you called out)

Basically a cast can have an explicit target field with metadata. For example, the UUID type planner produces:

FixedSizeBinary(16)
  metadata = {"ARROW:extension:name": "arrow.uuid"}

But logical cast schema only used the target data type when deriving and kept th source metadata:

Source field:
  raw: FixedSizeBinary(16)
  metadata: {}

Cast target:
  FixedSizeBinary(16)
  metadata: {"ARROW:extension:name": "arrow.uuid"}

Derived cast output:
  FixedSizeBinary(16)
  metadata: {}

This appeared in CI when common sub-expr elimination extracts a repeated cast into
its own projection:

ProjectionExec: arrow_metadata(__common_expr_1, 'ARROW:extension:name')

 ProjectionExec: CAST(raw AS UUID) AS __common_expr_1

The inner projection was initially created with incorrect empty metadata, so the physical optimizer rebuilt that projection and rederived its schema so isthe was accidentally repairing the logical schema bug.

Once this PR started preserving projection metadata correctly had this pop up this other bug.

So then I solve the optimizer bugs in this PR

gabotechs

Good catch @gene-bordegaray! just to give more context, we were bitten by this in our system while upgrading.

Just left a suggestion for relaxing the requirements, but otherwise LGTM.

gabotechs · 2026-08-25T14:26:07Z

    }) && exprs.len() == projection.input().schema().fields().len()
+        && projection.schema() == projection.input().schema()


This might be putting more restrictions than just metadata equality. It might be fine, but if we want to play it safe it could be better to just do && projection.schema().metadata() == projection.input().schema().metadata()

We need to check full schema because even if the schema metadata is equal things like the field metadata might not be thus we nee to check this as well.

I don't see anything in the schema which would be overestricting this. I may be missing something though

Pretty much the order of columns. I bet that's why the current checks are like they are right now.

I think it's fine though, if this becomes too restrictive it will start popping up in tests

gabotechs · 2026-08-25T15:11:03Z

🤔 there seems to be a CI failure:

1. query result mismatch:
[SQL] SELECT
    CAST(raw AS UUID),
    arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name')
FROM (
    VALUES (
        arrow_cast(X'00010203040506070809000102030506', 'FixedSizeBinary(16)')
    )
) AS uuids(raw);
[Diff] (-expected|+actual)
-   00010203040506070809000102030506 arrow.uuid
+   00010203040506070809000102030506 NULL
at /home/runner/work/datafusion/datafusion/datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:36

Do you think it's related to this change?

gene-bordegaray · 2026-08-25T15:18:42Z

Do you think it's related to this change?

looking into

gene-bordegaray · 2026-08-25T15:47:12Z

Do you think it's related to this change?

Found issues, this is a bit more involved than I was hoping. Will the variants with the fix

gene-bordegaray · 2026-08-26T01:01:58Z

@gabotechs ok I figured out what was going on and documented it in the PR description. There is also another bug in the codec / serialization where we need to serialize metadata. I am not solving that in this PR to keep scoped / tracked. I will crete issue for this tmrw or you can if you would like 👍

gene-bordegaray · 2026-08-26T01:06:15Z

this is also a correctenss issue / regression in 55 so I can note this in the minor version bump

codecov-commenter · 2026-08-26T01:45:59Z

Codecov Report

❌ Patch coverage is 85.60000% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.61%. Comparing base (124291e) to head (d112760).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/physical-plan/src/projection.rs	85.60%	8 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24670      +/-   ##
==========================================
- Coverage   81.61%   81.61%   -0.01%     
==========================================
  Files        1123     1123              
  Lines      409392   409514     +122     
  Branches   409392   409514     +122     
==========================================
+ Hits       334134   334232      +98     
- Misses      55637    55650      +13     
- Partials    19621    19632      +11

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gene-bordegaray · 2026-08-26T13:23:27Z

created codec / serialization follow up here: #24695

gabotechs · 2026-08-26T17:29:05Z

+    // `Cast::new` and `TryCast::new` use this field when only a target
+    // type is known. In that case, retain the source field's metadata.
+    let type_only_target = target_field.name().is_empty()
+        && target_field.is_nullable()
+        && target_field.metadata().is_empty();
+    let metadata = if type_only_target {


Why is it not sufficient to do something simpler like:

Does target_field have metadata? if yes, then use that metadata, if not, then use the source_field metadata.

Ya I thought this then AI actually ciaght this and I validated.

There can be the case where we are giving an target field with empty metadata and target type of a FiexedSizeBinary, then the source say has metadata that is marking something as a arrow UUID. In this case you would expect the result to be corrctly casted to FixedSizeBinary and empty metadata.

But what would happen is the target metadata is empty so then it would try to use the source. But the source metadata is saying to treated the FixedSizeBinary as a arrow UUID thus wouldnt cast correctly.

Now this check prevents that by checking if its a type only target.

There may be a clarner way to represent this though. I will return with thoughts

mine can also be wrong. its kinda implicit. I think this difference should be properly marked but might be a larger change like with an enum:

enum CastTarget { DataType, Field }

gabotechs · 2026-08-26T17:31:43Z

+///
+/// Such a projection is an execution boundary: a parent expression such as
+/// `arrow_metadata` can observe its output field metadata.
+fn projection_overrides_metadata(projection: &ProjectionExec) -> Result<bool> {


Nit: a ProjectionExec::overrides_metadata method rather than a standalone function sounds very slightly more elegant.

gabotechs · 2026-08-26T17:36:30Z

+    if projection_overrides_metadata(outer)? {
+        return Ok(None);
+    }
+
    let mut current_exprs: Vec<ProjectionExpr> = outer.expr().to_vec();
    let mut current_input: Arc<dyn ExecutionPlan> = Arc::clone(outer.input());
    let mut column_ref_map: HashMap<Column, usize> = HashMap::new();
    let mut collapsed_any = false;

    'outer: while let Some(inner_proj) = current_input.downcast_ref::<ProjectionExec>() {
+        if projection_overrides_metadata(inner_proj)? {
+            break;
+        }
+
        // Collect the column references usage in the outer projection.


Imagine this situation:

ProjectionExec: <- does not override metadata ProjectionExec: <- overrides metadata

This is collapsible right? but the current code will omit collapsing it.

This is sometimes collapsable because take the example I put in the PR description. You might have this:

ProjectionExec: arrow_metadata(i, 'event_field') AS metadata ProjectionExec: i@0 AS i output metadata = {"event_field": "true"} DataSourceExec: i metadata = {}

The outer query does not have override metadata but the inner does. The correct result is true.

The previous projection logic would collapse these, but the outer projection expr reads the metadata. So it would make:

ProjectionExec: arrow_metadata(i, 'event_field') AS metadata DataSourceExec: i metadata = {}

Giving use result as NULL now.

This is kinda ocnservative as we could probably just chekc if everything in the outer projection is just referencing columns but I am just trying to get to correctness first. Then could do the optimixation. What you think?

Damn, this is tricky indeed... I think it's good then 👍

Still working on better fix it, so approach might change

gene-bordegaray · 2026-08-27T10:07:27Z

ok I pushed a change that introduces an enum to differentiation between data type and explicit field casts. It is a larger and public api change but it is what I see as properly tracking this information, not an ad hoc check

timsaucer

I've just been looking at this as it pertains to #24462 and it looks like it has a couple of API changes that would make it ineligible for a patch release.

timsaucer · 2026-08-31T17:42:25Z

-    /// The `DataType` the expression will yield
-    pub field: FieldRef,
+    /// The target type and metadata policy.
+    pub field: CastTarget,


This is a breaking change of a public type, so if we go down this route this PR will not be eligible for a backport to 55. https://datafusion.apache.org/contributor-guide/release_management.html#backport-criteria

timsaucer · 2026-08-31T17:43:54Z

+    #[prost(message, optional, tag = "5")]
+    pub target_field: ::core::option::Option<super::datafusion_common::Field>,


This and other places in this file are adding pub fields also making this a breaking change.

gene-bordegaray · 2026-08-31T18:04:52Z

hey @timsaucer yes, this PR was originally meant to be stacked on #24725 but because of the breaking changes we are gong to take #23169 approach which avoids this for now. Then I will rebase this on that PR and will not hve these breaking change 👍

adriangb · 2026-08-31T22:15:16Z

+/// Derives the output field for a cast expression from the source field, using
+/// explicit target metadata when supplied.
 /// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.
 fn cast_output_field(
    source_field: &FieldRef,
-    target_type: &DataType,
+    target: &CastTarget,
    force_nullable: bool,
 ) -> Arc<Field> {
+    let metadata = target
+        .metadata()
+        .cloned()
+        .unwrap_or_else(|| source_field.metadata().clone());
    let mut f = source_field
        .as_ref()
        .clone()
-        .with_data_type(target_type.clone())
-        .with_metadata(source_field.metadata().clone());
+        .with_data_type(target.data_type().clone())
+        .with_metadata(metadata);


This is the only change I needed on top of projection.rs to make the UUID test pass (counterfactual confirmed: reverting just this reproduces the CI failure).

Note your callsite changes below — cast_output_field(&src, field, false) / (&src, field, true) — work unchanged with this, since Cast.field would go back to being a FieldRef. So this suggestion plus dropping CastTarget is the whole edit; the other ~20 files in the stack come out with it.

Suggested change

/// Derives the output field for a cast expression from the source field, using

/// explicit target metadata when supplied.

/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.

fn cast_output_field(

source_field: &FieldRef,

target_type: &DataType,

target: &CastTarget,

force_nullable: bool,

) -> Arc<Field> {

let metadata = target

.metadata()

.cloned()

.unwrap_or_else(|| source_field.metadata().clone());

let mut f = source_field

.as_ref()

.clone()

.with_data_type(target_type.clone())

.with_metadata(source_field.metadata().clone());

.with_data_type(target.data_type().clone())

.with_metadata(metadata);

/// Derives the output field for a cast expression from the source field.

///

/// The cast target's metadata is authoritative when it carries any; otherwise the

/// source's metadata is inherited. This mirrors the physical `CastExpr`, whose

/// target field is already authoritative when it is not the synthesized type-only

/// field.

///

/// For `TryCast`, `force_nullable` is `true` since a failed cast returns NULL.

fn cast_output_field(

source_field: &FieldRef,

target_field: &FieldRef,

force_nullable: bool,

) -> Arc<Field> {

let metadata = if target_field.metadata().is_empty() {

source_field.metadata().clone()

} else {

target_field.metadata().clone()

};

let mut f = source_field

.as_ref()

.clone()

.with_data_type(target_field.data_type().clone())

.with_metadata(metadata);

yes sorry about the confusion I should've temporarily made this a draft, the diff for this PR is much smaller. Allthat the CastTarget and protobuf changes are mixed into this PR because I had stacked it on #24725 but they shouldn’t be reviewed as part of the projection optimizer fix. I’m going to rebase this onto #23169 after some discussion to go with that approach for the minor relese.

as far as this particular comment. I originally thought this too and @gabotechs also asked about this, its a subtle one. Let me know if that clarifies 👍

I will rebase now and the PR should clean up after #23169 merges

gene-bordegaray · 2026-09-01T00:53:52Z

+00010203040506070809000102030506 NULL
+
+# arrow_cast to a different type strips extension metadata (type-only cast semantics)
+query ?T


this test is the one that would fail in #24831 @adriangb , it was added in #23169

That's a new test from #23169 right? Isn't that a test for a different bug than #24721, i.e. we don't need to couple solving them?

Yes, it is for a different bug but when i rooginally made the minimal fix (my top commit now on this brnach) it fails some sqllogictests because of #23169 not being merged. Specifically:

SELECT CAST(raw AS UUID), arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name');

Expected on main:

00010203040506070809000102030506 arrow.uuid

But with the projection fix alone the projection starts to actually preserver logical schema then ignores the UUID target metadata, so the result:

- arrow.uuid + NULL

#24831 handles this with this check:

let metadata = if target_field.metadata().is_empty() { source_field.metadata().clone() } else { target_field.metadata().clone() };

as prposed in your brnach #24831 but it has the issue that I talk about here which is hwy I opted into stacking on #23169 to handle that case while not allowing another edge case to creep in.

So this is getting a bit tricky to handle. That particular test is from another PR, but solving just the bug at at the surface level unveils more underlying issues with casting and metadata that this relies on. I would think that getting this in the minor patch with #23169 would be the good short term solution, Then I read your comment and I think this could be a viable breaking change after some more discussion regarding how we want these semantics to behave.

Thanks for taking time to investigate all this @adriangb 🙇

it has the issue that I talk about #24670 (comment) which is hwy I opted into stacking on #23169 to handle that case while not allowing another edge case to creep in

The discussion you linked is suggesting that we go with:

Does target_field have metadata? if yes, then use that metadata, if not, then use the source_field metadata.

You propose that breaks with: arrow_cast(uuid_col, 'FixedSizeBinary') because the result is FixedSizeBinary but with the UUID metadata (invalid).

Under the stricter proposal in #23169 (comment) this would be resolved: we'd ignore the source field metadata.

I think the larger question is if we back port the behavior change to 55.

yes I think we are in aggreance, long term there is most likely a better solution than what is proposed. This PR is just trying to fix a bug that we saw pop up after the df55 upgrade and then created this cascading effect of more bugs being unveiled.

If we are ok with leaving that known incorrect behavior in the minor patch we can do the check:

Does target_field have metadata? if yes, then use that metadata, if not, then use the source_field metadata.

Seems like it will be sorted in the meeting tmrw 🙇

Since this PR is fixing 4 issues, can we merge a fix for 1-3 without 4?

yes we could and have CI pass, but there will be the undelying bug that the other PR is talking about and the check prposed elides. If we are aware of that and ok with it then more than happy to just have the top commit and use the check:

let metadata = if target_field.metadata().is_empty() { source_field.metadata().clone() } else { target_field.metadata().clone() };

This is my first participation in a patch release so some guidance would be great. Thank you again 🙇

EDIT: #23169 properly handles this so if we backpoirt this with it should fix all cases for now until we discuss long term semantics

This is my first participation in a patch release so some guidance would be great. Thank you again 🙇

You're doing great! The main thing is trying to minimize the amount of code and behavior change that ships with a patch. And in general keeping PRs decoupled (e.g. splitting a bit PR into two smaller ones so that at least one of them is unblocked) is good. Those were my suggestions. Sorry if they didn't make sense here.

ok cool, thanks! I think the current state after the prerequisite prs got merged is more along the lines of what we are lookiing for in this

github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 25, 2026

gabotechs previously approved these changes Aug 25, 2026

View reviewed changes

gene-bordegaray force-pushed the gene.bordegaray/2026/08/preserve-projection-schema-metadata branch from 42888f7 to 822b3f9 Compare August 26, 2026 01:05

gene-bordegaray force-pushed the gene.bordegaray/2026/08/preserve-projection-schema-metadata branch from 822b3f9 to f64100d Compare August 26, 2026 01:21

github-actions Bot added the logical-expr Logical plan and expressions label Aug 26, 2026

gene-bordegaray mentioned this pull request Aug 26, 2026

Physical-plan serialization drops ProjectionExec metadata #24695

Open

gabotechs reviewed Aug 26, 2026

View reviewed changes

gene-bordegaray mentioned this pull request Aug 27, 2026

Release DataFusion 55.1.0 (minor/patch) Release (Sep 2026) #24462

Open

23 tasks

gene-bordegaray force-pushed the gene.bordegaray/2026/08/preserve-projection-schema-metadata branch from f64100d to beed4cd Compare August 27, 2026 10:05

github-actions Bot added sql SQL Planner physical-expr Changes to the physical-expr crates substrait Changes to the substrait crate proto Related to proto crate functions Changes to functions implementation labels Aug 27, 2026

gene-bordegaray force-pushed the gene.bordegaray/2026/08/preserve-projection-schema-metadata branch from beed4cd to c30f5be Compare August 27, 2026 10:20

github-actions Bot added the auto detected api change Auto detected API change label Aug 27, 2026

alamb mentioned this pull request Aug 27, 2026

Projection field metadata is lost during physical plan optimization #24721

Open

This was referenced Aug 27, 2026

Cast expressions cannot distinguish inherited metadata from an explicitly empty target #24724

Closed

fix: preserve cast target metadata #24725

Draft

timsaucer reviewed Aug 31, 2026

View reviewed changes

adriangb reviewed Aug 31, 2026

View reviewed changes

adriangb mentioned this pull request Aug 31, 2026

fix: preserve projection and cast target field metadata #24831

Closed

gene-bordegaray force-pushed the gene.bordegaray/2026/08/preserve-projection-schema-metadata branch from c30f5be to 0603df9 Compare September 1, 2026 00:51

github-actions Bot added sqllogictest SQL Logic Tests (.slt) and removed sql SQL Planner substrait Changes to the substrait crate labels Sep 1, 2026

gene-bordegaray commented Sep 1, 2026

View reviewed changes

github-actions Bot removed the auto detected api change Auto detected API change label Sep 1, 2026

adriangb mentioned this pull request Sep 1, 2026

Align metadata propagation through Physical and Logical casts #23169

Merged

fix: preserve projection metadata

d112760

gene-bordegaray force-pushed the gene.bordegaray/2026/08/preserve-projection-schema-metadata branch from 0603df9 to d112760 Compare September 1, 2026 19:59

github-actions Bot removed logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) proto Related to proto crate functions Changes to functions implementation labels Sep 1, 2026

adriangb mentioned this pull request Sep 1, 2026

test: SLT coverage and pushdown characterization for projection metadata gene-bordegaray/datafusion#7

Open

		}) && exprs.len() == projection.input().schema().fields().len()
		&& projection.schema() == projection.input().schema()

		#[prost(message, optional, tag = "5")]
		pub target_field: ::core::option::Option<super::datafusion_common::Field>,

Uh oh!

Conversation

gene-bordegaray commented Aug 25, 2026 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Removing a metadata-only identity projection

2 Collapsing across a metadata boundary

3 Rebuilding a projection with a new child

4 Cast target metadata lost before optimization

Uh oh!

gabotechs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabotechs commented Aug 25, 2026

Uh oh!

gene-bordegaray commented Aug 25, 2026

Uh oh!

gene-bordegaray commented Aug 25, 2026

Uh oh!

gene-bordegaray commented Aug 26, 2026

Uh oh!

gene-bordegaray commented Aug 26, 2026

Uh oh!

codecov-commenter commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gene-bordegaray commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray commented Aug 27, 2026

Uh oh!

timsaucer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray commented Aug 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gene-bordegaray commented Aug 25, 2026 •

edited by alamb

Loading

codecov-commenter commented Aug 26, 2026 •

edited

Loading

gene-bordegaray commented Aug 26, 2026 •

edited

Loading

gene-bordegaray Aug 26, 2026 •

edited

Loading

gene-bordegaray Sep 1, 2026 •

edited

Loading

adriangb Sep 1, 2026 •

edited

Loading

gene-bordegaray Sep 1, 2026 •

edited

Loading