Preserve logical cast field semantics during physical lowering with field-aware CastExpr - #20836
Merged
Merged
Conversation
kosiew
changed the title
kosiew
force-pushed
the
cast-02-20164
branch
2 times, most recently
from
March 10, 2026 07:22
1058b83 to
df5486c
Compare
kosiew
changed the title
kosiew
marked this pull request as ready for review
March 10, 2026 09:10
kosiew
changed the title
Contributor
Author
|
@adriangb |
Contributor
|
Interestingly I was just poking around here myself: #21390 |
kosiew
marked this pull request as draft
April 7, 2026 03:26
Contributor
Author
Refactor logical Expr::Cast to use field-aware CastExpr, ensuring target FieldRef metadata is preserved. Enhance tests to confirm metadata retention, validate that same-type casts aren't elided for fields with semantics, and ensure existing TryCast rejection for extension types remains effective.
Expose shared cast_with_target_field helper to validate and build field-aware CastExprs in one place. Update planner to directly call this helper, removing the need for temporary type-only casts. Add regression tests to cover standard casts, metadata-bearing casts, and same-type semantic-preserving casts.
Consolidate default-target-field predicate and success construction path in cast_with_target_field to reduce duplicate code in cast.rs. Simplify tests in planner.rs by implementing shared setup helpers and caching return_field() results for standard casts.
Narrow cast_with_target_field from a public re-export to a crate-only re-export in datafusion/physical-expr/src/expressions/mod.rs. This change allows the planner to still utilize it while reducing the public expressions API surface.
Use as_planner_cast(...) helper in planner tests to eliminate repeated downcasting. Update cast_with_target_field documentation to clarify that default synthesized fields are elided while explicit field semantics are preserved.
kosiew
changed the title
- Refactored the handling of `Expr::Cast` to remove unnecessary line breaks and improve readability in the `create_physical_expr` function. - Modified the test for cast lowering to maintain consistent formatting while preserving target field metadata.
Collapse the nested if statements in datafusion/physical-expr/src/expressions/cast.rs to satisfy Clippy's collapsible_if lint. This change does not alter any existing behavior.
kosiew
marked this pull request as ready for review
April 7, 2026 14:48
Contributor
Author
|
@adriangb |
adriangb
approved these changes
Apr 7, 2026
adriangb
left a comment
adriangb
left a comment
Contributor
There was a problem hiding this comment.
Nice work! Can we add SLT tests that would reflect these changes?
Introduce TypePlanner hook in test_context.rs for file-specific SLT to handle UUID target-field metadata. Added cast_extension_type_metadata.slt to cover new test cases for CAST and TRY_CAST with FixedSizeBinary. Ensure target metadata is preserved and acknowledge unchanged rejection path for TRY_CAST.
Eliminate the test_try_cast_to_extension_type_is_rejected from planner.rs as the new SLT now directly covers this case. This cleanup ensures better maintainability and reduces test duplication.
Rich-T-kid
pushed a commit
to Rich-T-kid/datafusion
that referenced
this pull request
Apr 21, 2026
…ield-aware CastExpr (apache#20836) ## Which issue does this PR close? * Part of apache#20164 --- ## Rationale for this change The current physical planning path for `Expr::Cast` discards logical field information (name, nullability, and metadata) by lowering casts using only the target `DataType`. This results in a loss of semantic fidelity between logical and physical plans, particularly for metadata-bearing fields and same-type casts with explicit field intent. Additionally, the planner previously rejected casts with metadata due to limitations of the type-only casting API, creating inconsistencies with other parts of the system (e.g. adapter-generated expressions). This change introduces a field-aware casting path that preserves logical intent throughout physical lowering, ensuring consistent semantics across planner and adapter outputs. --- ## What changes are included in this PR? * Introduced `cast_with_target_field` to construct `CastExpr` using full `FieldRef` semantics (name, nullability, metadata). * Refactored existing `cast_with_options` to delegate to the new field-aware helper. * Moved `is_default_target_field` to a shared helper function for reuse. * Updated planner (`planner.rs`) to use `cast_with_target_field` instead of type-only casting. * Removed metadata rejection logic during cast lowering. * Ensured same-type casts preserve explicit field semantics unless the target field is default. * Adjusted cast construction to validate compatibility before building expressions. * Exported `cast_with_target_field` for internal planner use. --- ## Are these changes tested? Yes. Added planner-focused unit tests to validate: * Preservation of target field metadata during cast lowering * Correct propagation of nullability semantics * Proper handling of same-type casts with explicit field overrides * No regression for standard type-only casts * Rejection behavior for unsupported extension type casts via `TryCast` These tests ensure both backward compatibility and correctness of the new semantics. --- ## Are there any user-facing changes? Yes, behaviorally (but not API-breaking): * Cast expressions now preserve logical field metadata and nullability in physical plans. * Previously rejected metadata-bearing casts are now supported. * Same-type casts may now produce a `CastExpr` when explicit field semantics are provided. There are no breaking changes to public APIs, but downstream consumers that relied on previous planner behavior (e.g. metadata stripping or cast elision) may observe differences. --- ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.
adriangb
added a commit
to pydantic/datafusion
that referenced
this pull request
Sep 1, 2026
`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`.
barbarj
pushed a commit
to barbarj/datafusion
that referenced
this pull request
Sep 1, 2026
…#23169) ## Which issue does this PR close? - Closes apache#22079 - Closes apache#24724 ## Rationale for this change The logical `Expr::Cast` and `Expr::TryCast` have a `FieldRef` target that was added in apache#18136 so that logical casts can express a cast to an extension type. In combination with a SQL type planner ( apache#20676 ) and an optimizer rule, this enabled casting to/from extension types with custom semantics to actually occur. The ability to do this was reverted by apache#20836 (which removed the original test) and I am not sure that ability ever made it into a release. When investigating this issue, it became clear the logical and physical cast behaviour had diverged with respect to the target field. ## What changes are included in this PR? This PR strips specific metadata keys (extension name and extension metadata) when propagating metadata from the source of a cast to the target (because doing so may result in an invalid destination field that consumers could reject), and propagates all metadata from the (logical) cast target field (e.g., so that a cast to an extension type represented by the cast target field will have a `to_field()` that communicates the extension type). For the physical cast, this behaviour is replicated exactly (I hope). Note that actually casting to an extension type can be implemented with an optimizer rule, planner, or by the mechanism I have in the works in apache#21071 . ## Are these changes tested? Yes ## Are there any user-facing changes? It was in practice not common to create a `Expr::Cast` with field metadata internally and thus I don't think users will see metadata changes from the inclusion of metadata from the target field. I would be surprised if stripping the extension name/metadata from the source was disruptive (it was more likely to have caused errors). Superceeds an earlier but similar attempt ( apache#22162 ). --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Tim Saucer <timsaucer@gmail.com>
timsaucer
added a commit
to timsaucer/datafusion
that referenced
this pull request
Sep 2, 2026
…#23169) - Closes apache#22079 - Closes apache#24724 The logical `Expr::Cast` and `Expr::TryCast` have a `FieldRef` target that was added in apache#18136 so that logical casts can express a cast to an extension type. In combination with a SQL type planner ( apache#20676 ) and an optimizer rule, this enabled casting to/from extension types with custom semantics to actually occur. The ability to do this was reverted by apache#20836 (which removed the original test) and I am not sure that ability ever made it into a release. When investigating this issue, it became clear the logical and physical cast behaviour had diverged with respect to the target field. This PR strips specific metadata keys (extension name and extension metadata) when propagating metadata from the source of a cast to the target (because doing so may result in an invalid destination field that consumers could reject), and propagates all metadata from the (logical) cast target field (e.g., so that a cast to an extension type represented by the cast target field will have a `to_field()` that communicates the extension type). For the physical cast, this behaviour is replicated exactly (I hope). Note that actually casting to an extension type can be implemented with an optimizer rule, planner, or by the mechanism I have in the works in apache#21071 . Yes It was in practice not common to create a `Expr::Cast` with field metadata internally and thus I don't think users will see metadata changes from the inclusion of metadata from the target field. I would be surprised if stripping the extension name/metadata from the source was disruptive (it was more likely to have caused errors). Superceeds an earlier but similar attempt ( apache#22162 ). --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Tim Saucer <timsaucer@gmail.com> (cherry picked from commit 124291e)
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?
Rationale for this change
The current physical planning path for
Expr::Castdiscards logical field information (name, nullability, and metadata) by lowering casts using only the targetDataType. This results in a loss of semantic fidelity between logical and physical plans, particularly for metadata-bearing fields and same-type casts with explicit field intent.Additionally, the planner previously rejected casts with metadata due to limitations of the type-only casting API, creating inconsistencies with other parts of the system (e.g. adapter-generated expressions).
This change introduces a field-aware casting path that preserves logical intent throughout physical lowering, ensuring consistent semantics across planner and adapter outputs.
What changes are included in this PR?
cast_with_target_fieldto constructCastExprusing fullFieldRefsemantics (name, nullability, metadata).cast_with_optionsto delegate to the new field-aware helper.is_default_target_fieldto a shared helper function for reuse.planner.rs) to usecast_with_target_fieldinstead of type-only casting.cast_with_target_fieldfor internal planner use.Are these changes tested?
Yes.
Added planner-focused unit tests to validate:
TryCastThese tests ensure both backward compatibility and correctness of the new semantics.
Are there any user-facing changes?
Yes, behaviorally (but not API-breaking):
CastExprwhen explicit field semantics are provided.There are no breaking changes to public APIs, but downstream consumers that relied on previous planner behavior (e.g. metadata stripping or cast elision) may observe differences.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.