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 90.50992% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.59%. Comparing base (a274959) to head (0603df9).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...tafusion/physical-expr/src/expressions/try_cast.rs	87.87%	13 Missing and 11 partials ⚠️
datafusion/physical-plan/src/projection.rs	85.60%	8 Missing and 10 partials ⚠️
datafusion/functions/src/core/arrow_try_cast.rs	0.00%	11 Missing ⚠️
datafusion/physical-expr/src/expressions/cast.rs	95.21%	1 Missing and 8 partials ⚠️
datafusion/physical-expr/src/planner.rs	95.12%	0 Missing and 4 partials ⚠️
datafusion/functions/src/core/arrow_cast.rs	90.90%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24670      +/-   ##
==========================================
+ Coverage   81.58%   81.59%   +0.01%     
==========================================
  Files        1123     1123              
  Lines      406610   407226     +616     
  Branches   406610   407226     +616     
==========================================
+ Hits       331719   332281     +562     
- Misses      55453    55477      +24     
- Partials    19438    19468      +30

☔ 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

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

# Conflicts: # datafusion/physical-expr/src/expressions/cast.rs # datafusion/physical-expr/src/expressions/mod.rs # datafusion/physical-expr/src/planner.rs

…-metadata-cleanup

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 👍

Restore the two public signatures on `CastExpr` that this branch changed, so the metadata fixes can be backported to a patch release: * `CastExpr::new_with_target_field` takes `FieldRef` by value again * `CastExpr::target_field()` returns `&FieldRef` again Both are achieved by storing the target `FieldRef` as before and adding a private `explicit_target` flag that records whether the field was supplied by the caller or synthesized from a `DataType`. That flag carries exactly the information the `Option<HashMap<..>>` / `Option<bool>` pair carried, so `target_metadata()`, `target_nullable()`, `has_explicit_metadata()` and `has_explicit_nullability()` keep their meaning and are derived from it. `Hash`/`PartialEq` compare the same components as before, so the field name still does not participate. `TryCastExpr` is given the same shape for symmetry. Its metadata-aware API is new on this branch, so nothing there is a compatibility constraint. Cast semantics are unchanged: explicit target fields still supply their metadata and nullability verbatim, type-only casts still pass source metadata through with the extension type keys stripped, and the output field name still comes from the source expression. Comparing the public signatures against the merge base with main, the only remaining deltas are additions: + CastExpr::target_metadata / target_nullable + CastExpr::has_explicit_metadata / has_explicit_nullability + TryCastExpr::new_with_target_field / target_metadata / target_field + expressions::try_cast_with_target_field `cast_with_target_field` keeps its `&FieldRef` parameter: `mod cast` is private and it is only re-exported as `pub(crate)`, so it is not part of the public API. Verified: `cargo clippy --workspace --all-targets -- -D warnings` clean, 504/504 sqllogictest files pass, and the unit tests for physical-expr, physical-expr-adapter, physical-plan, pruning, expr, functions and the datafusion core lib all pass.

Keep cast metadata changes non-breaking

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 🙇

paleolimbot and others added 17 commits June 25, 2026 14:58

try the generated answer

c74dd77

don't call return_field for the source info rewrite

b4fdb49

comments

1d1608b

update the cast representation to remove is_default_target_field()

944cb03

maybe cleaner

83ae9eb

minimize the diff

6b076ec

align logical cast rules

7d9525d

typo

5e202a7

another typo

b8ddb43

implement try cast

0eb421f

fix build

bbeab9e

fix docs

2adb3cc

ensure try_cast semantics are the same

3aaa9ed

sql logic tests

fb98847

slightly better

efb95ce

better

7d1ceaa

Merge branch 'main' into cast-metadata-cleanup

5dff049

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

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

Open

fix: preserve cast target metadata #24725

Open

paleolimbot added 3 commits August 31, 2026 11:54

Merge remote-tracking branch 'upstream/main' into cast-metadata-cleanup

5fa6460

# Conflicts: # datafusion/physical-expr/src/expressions/cast.rs # datafusion/physical-expr/src/expressions/mod.rs # datafusion/physical-expr/src/planner.rs

Merge remote-tracking branch 'origin/cast-metadata-cleanup' into cast…

4ae5fe0

…-metadata-cleanup

fix cast target field borrowing

8a771c8

timsaucer reviewed Aug 31, 2026

View reviewed changes

timsaucer and others added 3 commits August 31, 2026 15:57

Merge pull request #2 from timsaucer/non-breaking-cast-metadata

007b8ef

Keep cast metadata changes non-breaking

add expr proto fix

e077100

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

Draft

fix: preserve projection metadata

0603df9

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

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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 Sep 1, 2026 •

edited

Loading

adriangb Sep 1, 2026 •

edited

Loading