feat: give TryCastExpr a target field - #24835
Draft
adriangb wants to merge 6 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.
`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
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
adriangb
commented
Sep 1, 2026
| NULL NULL | ||
| 3 NULL | ||
|
|
||
| # Regression test: CAST with single-argument arrow_metadata (returns full map) |
Contributor
Author
There was a problem hiding this comment.
Does this need updating?
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.
`Expr::TryCast` holds a `FieldRef` target so that a `TRY_CAST` can name a
destination richer than a `DataType` - an extension type resolved by a
`TypePlanner`, whose `ARROW:extension:name` lives in the field's metadata. The
physical `TryCastExpr` stored only a `DataType`, so there was nowhere to put
that target, and `create_physical_expr` bailed out rather than lower it:
SELECT TRY_CAST(raw AS UUID) FROM ...;
Error during planning: TryCast from FixedSizeBinary(16) to
FixedSizeBinary(16)<{"ARROW:extension:name": "arrow.uuid"}> is not supported
which is odd on its face, since the same query with `CAST` has worked since
apache#20836.
Give `TryCastExpr` a `target_field`, mirroring `CastExpr`:
* `TryCastExpr::new_with_target_field` is the field-aware constructor;
`TryCastExpr::new` keeps working and synthesizes a type-only target
* `try_cast_with_target_field` is the field-aware builder, and elides the cast
only when it would be a genuine no-op, exactly as `cast_with_target_field`
does
* `create_physical_expr` passes the logical target field straight through, and
the planner guard is gone
Proto carried only the data type, for `PhysicalTryCastNode` and
`PhysicalCastNode` alike, so a cast to an extension type came back from
serialization as a plain cast to the storage type. Both messages gain an
optional `target_field`; it is written only when the target says more than a
data type, so plans that do not use one encode exactly as before, and a node
without it still decodes by falling back to `arrow_type`.
adriangb
force-pushed
the
adriangb/try-cast-target-field
branch
from
September 1, 2026 06:43
851fa03 to
330214f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24835 +/- ##
========================================
Coverage 81.58% 81.58%
========================================
Files 1123 1123
Lines 406610 407094 +484
Branches 406610 407094 +484
========================================
+ Hits 331719 332118 +399
- Misses 55453 55496 +43
- Partials 19438 19480 +42 ☔ 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 3 of 3 decomposing #23169.
feat: give TryCastExpr a target field. Everything below it in the diff belongs to the two PRs underneath.Opened as a draft while the stack is under review.
Rationale for this change
Expr::TryCastholds aFieldReftarget so aTRY_CASTcan name a destination richer than aDataType— for example an extension type resolved by aTypePlanner, whoseARROW:extension:namelives in the field's metadata. The physicalTryCastExprstored only aDataType, so there was nowhere to put that target, andcreate_physical_exprrefused to lower the expression at all:That is odd on its face: the same query written with
CASThas worked since #20836, which gaveCastExpra target field. The guard in the planner was the symptom; the missing field was the cause.What changes are included in this PR?
TryCastExprgains atarget_field, mirroringCastExpr:TryCastExpr::new_with_target_field(expr, target_field)is the new field-aware constructor.TryCastExpr::new(expr, cast_type)keeps working unchanged and synthesizes a type-only target, so this is purely additive.cast_type()now reads through the target field;target_field()exposes it.try_cast_with_target_field(expr, input_schema, target_field)is the field-aware builder. It ispub(crate), matching the visibility of its counterpartcast_with_target_field; both are used only bycreate_physical_expr. It elides the cast only when the cast would be a genuine no-op, exactly ascast_with_target_fielddoes — a same-typeTRY_CASTis still meaningful when it drops metadata.create_physical_exprpasses the logical target field straight through, and the planner guard is deleted.return_fieldderives its result from the sharedcast_output_field, soTRY_CASTandCASTreport their output field by the same rule.Proto
datafusion/protodoes serialize both cast expressions, and bothPhysicalCastNodeandPhysicalTryCastNodecarried only anArrowType. A cast to an extension type therefore came back from serialization as a plain cast to the storage type, silently losingARROW:extension:name. ForCastExprthat is a pre-existing gap, present since it gained a target field; forTryCastExprit would be a gap this PR introduces. Fixing only one of the two would leave a confusing asymmetry, so both messages gain the same optional field:It is written only when the target says more than a data type, so plans that do not use one encode byte for byte as they did before, and a node without it still decodes by falling back to
arrow_type. Generated code was refreshed with the repository's owndatafusion/proto-models/regen.sh.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-physical-expr -p datafusion-physical-plan -p datafusion-sql -p datafusion-proto -p datafusion-proto-models -p datafusion-optimizer -p datafusion-substrait --lib --testsgreen;./ci/scripts/rust_clippy.shexits 0.New tests:
cast_extension_type_metadata.slt:TRY_CAST(... AS UUID)on a literal and on a column now returnsarrow.uuidinstead of failing to plan. These replace thestatement errorthat pinned the old planner guard, and are the cases Align metadata propagation through Physical and Logical casts #23169's reference test file covers at its lines 49 and 66. A third case checks that aTRY_CASTnaming only a data type still drops the source's metadata.try_cast.rs:try_cast_with_target_field_carries_target_metadata,same_type_try_cast_is_only_elided_when_it_is_a_no_op,target_field_survives_a_proto_round_trip,a_type_only_target_field_is_not_encoded.cast.rs:target_field_survives_a_proto_round_trip,a_type_only_target_field_is_not_encoded.Load-bearing checks:
create_physical_exprinstead of the target field (leaving the guard removed) fails the first new slt case atcast_extension_type_metadata.slt:51withNULLin place ofarrow.uuid. Againstmainall three slt cases fail outright, with the planning error above.target_field: Noneon the encode side fails bothtarget_field_survives_a_proto_round_triptests, while botha_type_only_target_field_is_not_encodedtests keep passing — which is what confirms the "only encode an explicit target" condition is doing something rather than the field always being written.Are there any user-facing changes?
Yes, and they are all fixes:
TRY_CAST(expr AS <extension type>)plans and executes instead of failing, and reports the target's metadata.CASTorTRY_CASTto an extension type keeps that extension type across protobuf serialization.API changes are additive:
TryCastExpr::new_with_target_field,TryCastExpr::target_field, and the optionaltarget_fieldonPhysicalCastNode/PhysicalTryCastNode.try_cast_with_target_fieldis crate-internal and adds no public surface.TryCastExpr::newandtry_castkeep their signatures and behaviour for a type-only target.