fix: make a cast target's metadata authoritative - #24833
Draft
adriangb wants to merge 5 commits into
Draft
Conversation
Field metadata on a ProjectionExec's output schema could silently disappear when the physical optimizer removed or rewrote projections: 1. A metadata-only identity projection was treated as removable, because the check only compared column indices, aliases, and counts. 2. Collapsing a projection across a metadata boundary substituted the outer expression through the inner projection, so metadata-reading expressions saw the scan field instead of the projected field. 3. `make_with_child` rebuilt the projection with `try_new`, rederiving the output schema and dropping the original metadata. This commit is taken verbatim from @gene-bordegaray's work in apache#24670. Co-Authored-By: Gene Bordegaray <gene.bordegaray@datadoghq.com>
The logical `Expr::Cast`/`Expr::TryCast` carry a `FieldRef` target so a cast
can express a destination that is more than a `DataType` (for example an
extension type produced by a `TypePlanner`). `cast_output_field` ignored that
field's metadata entirely and always inherited the source's, so
`Expr::to_field()` disagreed with the physical `CastExpr`, which already treats
a non-synthesized target field as authoritative.
The divergence was masked because the physical optimizer rederives a
projection's schema from its expressions, repairing the logical schema on the
way through. Once projections preserve their metadata faithfully (previous
commit) the underlying bug surfaces, and a cast to an extension type loses it:
SELECT arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name')
-- 'arrow.uuid' before, NULL after
Take the target's metadata when it carries any, and otherwise inherit the
source's. A plain `CAST(expr AS type)` synthesizes a target with no metadata,
so its long-standing behaviour is unchanged.
`Expr::Cast`/`Expr::TryCast` and the physical `CastExpr` each derive the output field of a cast, and each did it differently: the logical side inherited the source's metadata unless the target carried some, while the physical side used a non-synthesized target field verbatim. Two rules for one question is how the layers drifted apart in apache#24724. Give them one rule, in one place - `datafusion_expr_common::casts::cast_output_field`: * the data type always comes from the target * the metadata always comes from the target, *including* when it is empty * the name and nullability come from the target when it says more than a data type, and from the source otherwise The behaviour change is the second point. Metadata such as `ARROW:extension:name` describes how to read one particular storage type; a cast produces a different one, so inheriting the source's metadata mints a field claiming to be an extension type it no longer is (apache#22079): SELECT arrow_metadata(CAST(uuid_val AS BYTEA), 'ARROW:extension:name') -- 'arrow.uuid' before, NULL after A caller that wants metadata on the result now has to ask for it, by putting it on the cast target. That makes a same-type cast meaningful - it is how you spell "drop this metadata" - so the places that elide one had to be checked. The logical `Expr::cast_to` and the physical `cast()`/`cast_with_target_field` already agree: both elide only when the target is type-only, and neither is reachable from a user-written `CAST`, which the SQL planner lowers directly. The one place that did not survive is UNION branch coercion. `coerce_exprs_for_schema` cast each branch to the destination's *data type*, so the cast target carried no metadata and the coerced branch dropped the metadata the union's output schema still advertised - leaving the physical plan inconsistent with the logical one: Internal error: Physical input schema should be the same as the one converted from logical input schema. - field metadata at index 0 [name]: (physical) {} vs (logical) {"metadata_key": "the nonnull_name field"} Coerce to the destination *field* instead, so the branch ends up carrying exactly the metadata it was coerced to. The `metadata.slt` assertions that pinned the old inheritance are updated to the new rule.
github-actions
Bot
added
logical-expr
`cast_with_target_field` dropped the cast whenever the data types already matched and the target field was the synthesized type-only one. That was sound while a type-only cast could not change metadata; now that the target's metadata is authoritative, such a cast is exactly how you spell "drop this metadata", and eliding it leaves the physical plan reporting metadata the logical plan has already dropped. Elide only when the cast would produce the field the child already has, which `cast_output_field` answers directly. This is not observable end to end yet: the one query that reaches it, `arrow_cast(uuid_val, 'FixedSizeBinary(16)')`, is short-circuited earlier by `ArrowCastFunc::simplify`, which never builds the cast in the first place. That is fixed in the next PR of this stack, which relies on this one.
This was referenced Sep 1, 2026
An explicit target field fully determines a cast's output field, so there is no need to resolve the child expression to derive it. Resolving it anyway breaks `rewrite_file_row_index_expr`, which deliberately wraps a `Column` whose index lies outside the schema the cast is later asked about, and which relied on the previous short-circuit for explicit targets.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24833 +/- ##
========================================
Coverage 81.58% 81.58%
========================================
Files 1123 1123
Lines 406610 406886 +276
Branches 406610 406886 +276
========================================
+ Hits 331719 331947 +228
- Misses 55453 55470 +17
- Partials 19438 19469 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Stacking
This is PR 1 of 3 decomposing #23169.
adriangb/minimal-cast-metadata-fix), which contains the projection-metadata fix and the first version of the cast-metadata rule. Please review that one first.fix: make a cast target's metadata authoritativeandfix: only elide a cast that is a genuine no-op. Everything below them in the diff belongs to fix: preserve projection and cast target field metadata #24831.arrow_castmust not elide a metadata-changing cast) and PR 3 (TryCastExprtarget field) both branch from this one.Opened as a draft while the stack is under review.
Rationale for this change
Field metadata such as
ARROW:extension:namedescribes how to interpret one particular storage type. A cast produces a different storage type, so inheriting the source's metadata mints a field that claims to be an extension type it no longer is:That is the failure mode described in #22079.
Underneath it sits a second problem.
Expr::Cast/Expr::TryCastand the physicalCastExpreach derive the output field of a cast, and each did it differently:cast_output_field()inherited the source's metadata unless the target carried some;CastExpr::resolved_target_field()used a non-synthesized target field verbatim, and otherwise inherited everything from the source.Two implementations of one question is how the layers drifted apart (#24724), and because
arrow_metadata(...)in SQL observes the physical field, a logical-only change is invisible end to end.What changes are included in this PR?
One rule, in one place.
datafusion_expr_common::casts::cast_output_fieldis now the single definition of how a cast's source field and target field combine, and both layers call it:is_type_only_cast_target), and from the source otherwiseCallers: logical
Expr::Cast/Expr::TryCast(expr_schema.rs), physicalCastExpr::resolved_target_field, and physicalTryCastExpr::return_field.TryCastExprhas no target field yet, so it passes a type-only stand-in; PR 3 gives it a real one.The behaviour change is the second bullet. A caller that wants metadata on the result of a cast now has to ask for it, by putting the metadata on the cast target.
Audit of the three same-type-cast elision sites. Under the old rule a same-type cast was a metadata no-op; under the new one it is meaningful — it is how you spell "drop this metadata" — so every place that elides one changes semantics.
Expr::cast_to(expr_schema.rs) elides when the types already match.cast_with_target_field(physical-expr/expressions/cast.rs) elided when the types match and the target is type-only.Site 1 is fine as it stands.
Expr::cast_tois a type-only coercion helper whose contract is "give this expression this type"; when it declines to build a cast, no cast exists in the logical plan and none is lowered, so the two layers agree. It is not reachable from a user-writtenCAST, which the SQL planner lowers straight toExpr::Cast.Site 2 is not. A logical
Expr::Castwith matching types and a type-only target does something under the new rule — it strips metadata — but the physical lowering dropped it, so the physical plan kept metadata the logical plan had already discarded. The second commit changes the condition to "elide only when the cast would produce the field the child already has", whichcast_output_fieldanswers directly. It is not observable end to end in this PR because the only query that reaches it is short-circuited earlier by site 3; PR 2 relies on it.ArrowCastFunc::simplifyshort-circuits when source and target types are equal and returns the argument with no cast at all. That one is genuinely wrong under the new rule and is fixed in PR 2 — it is whyarrow_cast(uuid_val, 'FixedSizeBinary(16)')still keepsarrow.uuid.UNION branch coercion. This is the one site that did not survive the rule change.
coerce_exprs_for_schemacast each branch to the destination's data type, so the cast carried a type-only target and dropped the metadata the union's output schema still advertised, leaving the physical plan inconsistent with the logical one:on all three of the metadata-preserving UNION regression queries in
metadata.slt. The fix is to coerce to the destination field rather than just its data type (cast_expr_to_field), so a coerced branch ends up carrying exactly the metadata it was coerced to. The destination contributes only its data type and metadata; the name and nullability stay those of the expression being cast, which is what keeps the logical and physical fields identical.Updated assertions. Seven
metadata.sltassertions added by #21390 pinned the old inheritance (CAST/TRY_CASTpreserving source metadata). They now assert the new rule.What is the testing strategy for this PR?
Full
sqllogictestsuite green (504/504 files), andcargo test -p datafusion-expr -p datafusion-expr-common -p datafusion-physical-expr -p datafusion-physical-plan -p datafusion-sql -p datafusion-proto -p datafusion-optimizer --lib --testsgreen../ci/scripts/rust_clippy.shexits 0.New tests:
datafusion/expr-common/src/casts.rs:type_only_cast_target_is_recognised,cast_output_field_does_not_inherit_source_metadata,cast_output_field_takes_an_explicit_target_verbatim,cast_output_field_force_nullable_is_for_try_castdatafusion/physical-expr/src/expressions/cast.rs:type_only_cast_does_not_inherit_source_metadata,same_type_cast_is_only_elided_when_it_is_a_no_opdatafusion/physical-expr/src/expressions/try_cast.rs:try_cast_does_not_inherit_source_metadatacast_extension_type_metadata.slt: casting anarrow.uuidvalue toBYTEAdrops the extension metadatametadata.slt: a UNION branch coerced across types keeps the metadata the union's output schema advertisesEach new test was checked to be load-bearing by temporarily reverting the fix it covers:
cast_extension_type_metadata.sltcase, the seven updatedmetadata.sltassertions, and twoparquet_metadata_functions.sltqueries;cast_expr_to_fieldback to a data-type-only cast fails the newmetadata.sltUNION case and reproduces the threeInternal error: Physical input schema should be the same...failures atmetadata.slt:128,:158and:190;same_type_cast_is_only_elided_when_it_is_a_no_op.Are there any user-facing changes?
Yes.
CASTandTRY_CASTno longer copy the source column's field metadata onto their result. To keep metadata across a cast, put it on the cast target (for example via aTypePlannerextension type). No public API is removed;datafusion_expr_common::casts::cast_output_fieldandis_type_only_cast_targetare added.