adriangb opened a new pull request, #24833: URL: https://github.com/apache/datafusion/pull/24833
## 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 1 of 3** decomposing https://github.com/apache/datafusion/pull/23169. - Stacked on https://github.com/apache/datafusion/pull/24831 (`adriangb/minimal-cast-metadata-fix`), which contains the projection-metadata fix and the first version of the cast-metadata rule. **Please review that one first.** - The only new commit here is `fix: make a cast target's metadata authoritative`. Everything below it in the diff belongs to #24831. - PR 2 (`arrow_cast` must not elide a metadata-changing cast) and PR 3 (`TryCastExpr` target field) both branch from this one. Opened as a draft while the stack is under review. ## Rationale for this change Field metadata such as `ARROW:extension:name` describes how to interpret **one particular storage type**. A cast produces a different storage type, so inheriting the source's metadata mints a field that claims to be an extension type it no longer is: ```sql SELECT arrow_metadata(CAST(uuid_val AS BYTEA), 'ARROW:extension:name'); -- 'arrow.uuid' <- a Binary column claiming to be a UUID ``` That is the failure mode described in https://github.com/apache/datafusion/issues/22079. Underneath it sits a second problem. `Expr::Cast`/`Expr::TryCast` and the physical `CastExpr` each derive the output field of a cast, and each did it differently: - logical `cast_output_field()` inherited the source's metadata unless the target carried some; - physical `CastExpr::resolved_target_field()` used a non-synthesized target field verbatim, and otherwise inherited everything from the source. Two implementations of one question is how the layers drifted apart (https://github.com/apache/datafusion/issues/24724), and because `arrow_metadata(...)` in SQL observes the *physical* field, a logical-only change is invisible end to end. ## What changes are included in this PR? **One rule, in one place.** `datafusion_expr_common::casts::cast_output_field` is now the single definition of how a cast's source field and target field combine, and both layers call it: - 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 (`is_type_only_cast_target`), and from the source otherwise Callers: logical `Expr::Cast`/`Expr::TryCast` (`expr_schema.rs`), physical `CastExpr::resolved_target_field`, and physical `TryCastExpr::return_field`. `TryCastExpr` has no target field yet, so it passes a type-only stand-in; PR 3 gives it a real one. The behaviour change is the second bullet. A caller that wants metadata on the result of a cast now has to ask for it, by putting the metadata on the cast target. **Audit of the three same-type-cast elision sites.** Under the old rule a same-type cast was a metadata no-op; under the new one it is meaningful — it is how you spell "drop this metadata" — so every place that elides one changes semantics. 1. `Expr::cast_to` (`expr_schema.rs`) elides when the types already match. 2. `cast_with_target_field` (`physical-expr/expressions/cast.rs`) elides when the types match **and** the target is type-only. These two already agree: both are type-only coercion helpers, both elide only for a type-only target, and neither is reachable from a user-written `CAST`, which the SQL planner lowers straight to `Expr::Cast`. No change needed beyond renaming the physical predicate to the shared `is_type_only_cast_target`. 3. `ArrowCastFunc::simplify` short-circuits when source and target types are equal and returns the argument with no cast at all. That one is genuinely wrong under the new rule and is fixed in PR 2 — it is the only reason `arrow_cast(uuid_val, 'FixedSizeBinary(16)')` still keeps `arrow.uuid`. **UNION branch coercion.** This is the one site that did not survive the rule change. `coerce_exprs_for_schema` cast each branch to the destination's *data type*, so the cast carried a type-only target and 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. Differences: - field metadata at index 0 [name]: (physical) {} vs (logical) {"metadata_key": "the nonnull_name field"} ``` on all three of the metadata-preserving UNION regression queries in `metadata.slt`. The fix is to coerce to the destination **field** rather than just its data type (`cast_expr_to_field`), so a coerced branch ends up carrying exactly the metadata it was coerced to. The destination contributes only its data type and metadata; the name and nullability stay those of the expression being cast, which is what keeps the logical and physical fields identical. **Updated assertions.** Seven `metadata.slt` assertions added by #21390 pinned the old inheritance (`CAST`/`TRY_CAST` preserving source metadata). They now assert the new rule. ## What is the testing strategy for this PR? Full `sqllogictest` suite green (504/504 files), and `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-optimizer --lib --tests` green. `./ci/scripts/rust_clippy.sh` exits 0. New tests: - `datafusion/expr-common/src/casts.rs`: `type_only_cast_target_is_recognised`, `cast_output_field_does_not_inherit_source_metadata`, `cast_output_field_takes_an_explicit_target_verbatim`, `cast_output_field_force_nullable_is_for_try_cast` - `datafusion/physical-expr/src/expressions/cast.rs`: `type_only_cast_does_not_inherit_source_metadata` - `datafusion/physical-expr/src/expressions/try_cast.rs`: `try_cast_does_not_inherit_source_metadata` - `cast_extension_type_metadata.slt`: casting an `arrow.uuid` value to `BYTEA` drops the extension metadata - `metadata.slt`: a UNION branch coerced across types keeps the metadata the union's output schema advertises Each new test was checked to be load-bearing by temporarily reverting the fix it covers: - reverting the metadata rule to the old one fails all six new unit tests plus the new `cast_extension_type_metadata.slt` case, the seven updated `metadata.slt` assertions, and two `parquet_metadata_functions.slt` queries; - reverting `cast_expr_to_field` back to a data-type-only cast fails the new `metadata.slt` UNION case and reproduces the three `Internal error: Physical input schema should be the same...` failures at `metadata.slt:128`, `:158` and `:190`. ## Are there any user-facing changes? Yes. `CAST` and `TRY_CAST` no longer copy the source column's field metadata onto their result. To keep metadata across a cast, put it on the cast target (for example via a `TypePlanner` extension type). No public API is removed; `datafusion_expr_common::casts::cast_output_field` and `is_type_only_cast_target` are added. -- 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]
