fix: make a cast target's metadata authoritative - #24864
Draft
adriangb wants to merge 1 commit into
Draft
Conversation
A cast currently merges its source field's metadata into the output, minus the two extension-type keys. This makes the output field of a cast depend on both the target and whatever metadata the source happened to carry, which is hard to reason about and means a plain `CAST(expr AS type)` can produce a field nobody described. Take the target's metadata instead: a cast produces the field its target describes. A type-only target describes a field with no metadata, so a plain `CAST(expr AS type)` produces none. Because the target's metadata is authoritative, a same-type cast is meaningful whenever it clears metadata, so the three places that elide a cast when the types already match now elide it only when the source also carries no metadata. Union coercion is affected for the same reason: `coerce_exprs_for_schema` cast each branch to the destination's `DataType`, which would now drop the metadata the union's output schema advertises and leave the logical and physical schemas disagreeing. It coerces to the destination field instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24864 +/- ##
==========================================
- Coverage 81.61% 81.61% -0.01%
==========================================
Files 1123 1123
Lines 409392 409385 -7
Branches 409392 409385 -7
==========================================
- Hits 334139 334120 -19
- Misses 55635 55644 +9
- Partials 19618 19621 +3 ☔ 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
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?
Rationale for this change
#23169 made cast metadata consistent between the logical and physical layers, and stripped the two extension-type keys so that
CAST(uuid_col AS Utf8)no longer produces aUtf8field claiming to bearrow.uuid. It kept the long-standing rule that a cast otherwise merges its source field's metadata into the output.The alternative raised on that PR is that the target's metadata should simply be authoritative: a cast produces the field its target describes. A type-only target describes a field with no metadata, so a plain
CAST(expr AS type)produces none. That was the behaviour proposed in #22079 and in #23169 (comment).Merging leaves the output field dependent on both the target and whatever metadata the source happened to carry, and needs a per-key exception list to stay correct — currently two keys, and any future metadata with type semantics would need adding to it. Stamping needs no exception list, because metadata that describes a type cannot outlive the type it describes.
This PR exists so the cost of that change is a diff and a test run rather than an estimate.
What changes are included in this PR?
The rule, in the three places that derive a cast's output field: logical
cast_output_field,CastExpr::resolved_target_field, andTryCastExpr::return_field. Each loses its type-only branch and its extension-key removal.Two consequences that are not obvious from the rule itself:
Cast elision. A same-type cast used to be a metadata no-op, so three places elide it when the types already match. Once the target's metadata is authoritative, a same-type cast is meaningful whenever it clears metadata, so those guards elide only when the source also carries no metadata. Align metadata propagation through Physical and Logical casts #23169 already made this move for extension keys specifically; this widens the same condition. The three are
cast_with_target_field,try_cast_with_target_field, andArrowCastFunc::simplify.Union coercion.
coerce_exprs_for_schemacast each branch to the destination'sDataType. Under the new rule that cast drops the metadata the union's output schema advertises, and the logical and physical schemas stop agreeing (Internal error: Physical input schema should be the same as the one converted from logical input schema). It now coerces to the destination field, via a newcoerce_expr_to_field. This is a real consequence of the rule, and is the only part of the change that is not mechanical.What is the testing strategy for this PR?
The behaviour change is measured rather than described: it moves 7 assertions, all in
datafusion/sqllogictest/test_files/metadata.slt, regenerated with--complete(expected results only; no query text changed). Those 7 are the whole blast radius across the 504-file sqllogictest suite. All 7 were added in #21390, and that PR only madetry_castmatchcast, so half of the surface being changed has existed only since 54.Four unit tests that asserted the merge rule are updated to assert the stamp rule.
Verified on this branch with CI's own invocations:
cargo test --profile ci --workspace --lib --tests --bins --no-fail-fastwith CI's feature set: green./ci/scripts/rust_clippy.sh: exit 0RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace: cleanAre there any user-facing changes?
Yes, and it is a breaking behaviour change, which is why this is a draft against 56 rather than a backport.
CAST(expr AS type)andTRY_CAST(expr AS type)no longer carry the source column's field metadata into the result. A cast to an explicit target field is unchanged: it already used the target's metadata. Code that relies on metadata surviving a cast can attach it with an alias, or with an optimizer rule.No public API changes.
🤖 Generated with Claude Code