adriangb opened a new pull request, #24835: URL: https://github.com/apache/datafusion/pull/24835
## Which issue does this PR close? - Part of https://github.com/apache/datafusion/issues/22079 - Part of https://github.com/apache/datafusion/issues/24724 ## Stacking This is **PR 3 of 3** decomposing https://github.com/apache/datafusion/pull/23169. - Stacked on https://github.com/apache/datafusion/pull/24833, which is itself stacked on https://github.com/apache/datafusion/pull/24831. **Please review those first.** - The only new commit here is `feat: give TryCastExpr a target field`. Everything below it in the diff belongs to the two PRs underneath. - PR 2 (https://github.com/apache/datafusion/pull/24834) also branches from #24833; it and this PR are independent of each other. Opened as a draft while the stack is under review. ## Rationale for this change `Expr::TryCast` holds a `FieldRef` target so a `TRY_CAST` can name a destination richer than a `DataType` — for example 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` refused to lower the expression at all: ``` 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 ``` That is odd on its face: the same query written with `CAST` has worked since https://github.com/apache/datafusion/pull/20836, which gave `CastExpr` a target field. The guard in the planner was the symptom; the missing field was the cause. ## What changes are included in this PR? `TryCastExpr` gains a `target_field`, mirroring `CastExpr`: - `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, exported alongside the existing `try_cast`. It elides the cast only when the cast would be a genuine no-op, exactly as `cast_with_target_field` does — a same-type `TRY_CAST` is still meaningful when it drops metadata. - `create_physical_expr` passes the logical target field straight through, and the planner guard is deleted. - `return_field` derives its result from the shared `cast_output_field`, so `TRY_CAST` and `CAST` report their output field by the same rule. ### Proto `datafusion/proto` does serialize both cast expressions, and both `PhysicalCastNode` and `PhysicalTryCastNode` carried only an `ArrowType`. A cast to an extension type therefore came back from serialization as a plain cast to the storage type, silently losing `ARROW:extension:name`. For `CastExpr` that is a pre-existing gap, present since it gained a target field; for `TryCastExpr` it 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: ```protobuf optional datafusion_common.Field target_field = 3; ``` 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 own `datafusion/proto-models/regen.sh`. ## What is the testing strategy for this PR? Full `sqllogictest` suite 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 --tests` green; `./ci/scripts/rust_clippy.sh` exits 0. New tests: - `cast_extension_type_metadata.slt`: `TRY_CAST(... AS UUID)` on a literal and on a column now returns `arrow.uuid` instead of failing to plan. These replace the `statement error` that pinned the old planner guard, and are the cases #23169's reference test file covers at its lines 49 and 66. A third case checks that a `TRY_CAST` naming 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: - routing only the data type through `create_physical_expr` instead of the target field (leaving the guard removed) fails the first new slt case at `cast_extension_type_metadata.slt:51` with `NULL` in place of `arrow.uuid`. Against `main` all three slt cases fail outright, with the planning error above. - forcing `target_field: None` on the encode side fails both `target_field_survives_a_proto_round_trip` tests, while both `a_type_only_target_field_is_not_encoded` tests 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. - A `CAST` or `TRY_CAST` to an extension type keeps that extension type across protobuf serialization. API changes are additive: `TryCastExpr::new_with_target_field`, `TryCastExpr::target_field`, `try_cast_with_target_field`, and the optional `target_field` on `PhysicalCastNode`/`PhysicalTryCastNode`. `TryCastExpr::new` and `try_cast` keep their signatures and behaviour for a type-only target. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
