fix: arrow_cast must not elide a metadata-changing cast - #24834
Closed
adriangb wants to merge 6 commits into
Closed
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.
`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.
github-actions
Bot
added
logical-expr
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.
`ArrowCastFunc::simplify` short-circuited whenever the argument's data type
already equalled the requested one and returned the argument untouched. That
was sound while a same-type cast could not change anything, but a cast
target's metadata is authoritative, and `arrow_cast` names a storage type and
nothing else - its declared return field (`return_field_from_args`) never
carries metadata.
So casting an extension-typed value back to its own storage type is not a
no-op, it strips the extension metadata, and the short circuit swallowed it:
SELECT arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'),
'ARROW:extension:name')
-- 'arrow.uuid' before, NULL after
which is an `arrow.uuid` value that escaped a cast back to plain
`FixedSizeBinary(16)` (apache#22079).
Elide the cast only when the argument carries no metadata for it to strip.
Checking metadata emptiness rather than just `ARROW:extension:name` follows
from the rule itself: the target field carries no metadata at all, so any
metadata on the argument is metadata the cast removes.
adriangb
force-pushed
the
adriangb/arrow-cast-strip-extension
branch
from
September 1, 2026 06:34
bd548b6 to
d939afc
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24834 +/- ##
========================================
Coverage 81.58% 81.58%
========================================
Files 1123 1123
Lines 406610 406889 +279
Branches 406610 406889 +279
========================================
+ Hits 331719 331949 +230
- Misses 55453 55470 +17
- Partials 19438 19470 +32 ☔ 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 2 of 3 decomposing #23169.
fix: arrow_cast must not elide a metadata-changing cast. Everything below it in the diff belongs to the two PRs underneath.TryCastExprtarget field) branches from fix: make a cast target's metadata authoritative #24833, not from this one; the two are independent.Opened as a draft while the stack is under review.
Rationale for this change
With the strict cast-metadata rule from #24833 in place, this still returns
arrow.uuid:That is an
arrow.uuidvalue that escaped a cast back to its plain storage type, which is the failure mode in #22079.ArrowCastFunc::simplifyshort-circuits when the argument's data type already equals the requested one and returns the argument untouched, so no cast is built and no rule about cast metadata can apply. That was sound while a same-type cast could not change anything. It is not any more: a cast target's metadata is authoritative, andarrow_castnames a storage type and nothing else — its declared return field (return_field_from_args) never carries metadata. Casting an extension-typed value back to its own storage type therefore is meaningful, and the short circuit swallowed it.What changes are included in this PR?
ArrowCastFunc::simplifynow elides the cast only when the argument carries no metadata for the cast to strip. When it does carry metadata, a realExpr::Castwith a type-only target is built, and the rule from #24833 drops the metadata.The condition is "the argument carries any metadata", not "the argument carries
ARROW:extension:name". That follows from the rule rather than from the symptom: the cast target carries no metadata at all, so any metadata on the argument is metadata this cast removes, and singling out one key would leavearrow_castsilently preserving the rest.This is also where the second commit of #24833 earns its keep. Building the cast is not enough on its own — the physical lowering used to drop a same-type cast with a type-only target, which would have put the metadata straight back.
What is the testing strategy for this PR?
Full
sqllogictestsuite green (504/504 files),cargo test -p datafusion-expr -p datafusion-expr-common -p datafusion-functions -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
cast_extension_type_metadata.sltcases:arrow_cast(uuid_val, 'FixedSizeBinary(16)')— same storage type — no longer reportsarrow.uuid. This is the case Align metadata propagation through Physical and Logical casts #23169's reference test file covers at its line 80.arrow_cast(uuid_val, 'Binary')— different storage type — likewise.EXPLAINpinning that anarrow_castwhose argument has no metadata is still simplified away entirely, so the short circuit is narrowed rather than removed.Load-bearing check: reverting the condition to the old
source_type == target_typefails the first new case (cast_extension_type_metadata.slt:70) witharrow.uuidinstead ofNULL, while theEXPLAINcase keeps passing — which is what confirms the two halves of the new condition are each doing something.Are there any user-facing changes?
Yes.
arrow_cast(expr, '<type>')no longer returnsexprunchanged whenexpralready has that type but carries field metadata; it now produces a value with that type and no metadata, matchingarrow_cast's declared return field. No public API changes.