adriangb opened a new pull request, #24834: URL: https://github.com/apache/datafusion/pull/24834
## Which issue does this PR close? - Part of https://github.com/apache/datafusion/issues/22079 ## Stacking This is **PR 2 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**; this PR depends on both, and cannot work without #24833. - The only new commit here is `fix: arrow_cast must not elide a metadata-changing cast`. Everything below it in the diff belongs to the two PRs underneath. - PR 3 (`TryCastExpr` target field) branches from #24833, not from this one; the two are independent. Opened as a draft while the stack is under review. ## Rationale for this change With the strict cast-metadata rule from #24833 in place, this still returns `arrow.uuid`: ```sql SELECT arrow_metadata(arrow_cast(uuid_val, 'FixedSizeBinary(16)'), 'ARROW:extension:name'); -- arrow.uuid ``` That is an `arrow.uuid` value that escaped a cast back to its plain storage type, which is the failure mode in https://github.com/apache/datafusion/issues/22079. `ArrowCastFunc::simplify` short-circuits when the argument's data type already equals the requested one and returns the argument untouched, so no cast is built and no rule about cast metadata can apply. That was sound while a same-type cast could not change anything. It is not any more: a cast target's metadata is authoritative, and `arrow_cast` names a storage type and nothing else — its declared return field (`return_field_from_args`) never carries metadata. Casting an extension-typed value back to its own storage type therefore *is* meaningful, and the short circuit swallowed it. ## What changes are included in this PR? `ArrowCastFunc::simplify` now elides the cast only when the argument carries no metadata for the cast to strip. When it does carry metadata, a real `Expr::Cast` with a type-only target is built, and the rule from #24833 drops the metadata. The condition is "the argument carries any metadata", not "the argument carries `ARROW:extension:name`". That follows from the rule rather than from the symptom: the cast target carries no metadata at all, so *any* metadata on the argument is metadata this cast removes, and singling out one key would leave `arrow_cast` silently preserving the rest. This is also where the second commit of #24833 earns its keep. Building the cast is not enough on its own — the physical lowering used to drop a same-type cast with a type-only target, which would have put the metadata straight back. ## 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-functions -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 `cast_extension_type_metadata.slt` cases: - `arrow_cast(uuid_val, 'FixedSizeBinary(16)')` — same storage type — no longer reports `arrow.uuid`. This is the case #23169's reference test file covers at its line 80. - `arrow_cast(uuid_val, 'Binary')` — different storage type — likewise. - an `EXPLAIN` pinning that an `arrow_cast` whose argument has no metadata is still simplified away entirely, so the short circuit is narrowed rather than removed. Load-bearing check: reverting the condition to the old `source_type == target_type` fails the first new case (`cast_extension_type_metadata.slt:70`) with `arrow.uuid` instead of `NULL`, while the `EXPLAIN` case keeps passing — which is what confirms the two halves of the new condition are each doing something. ## Are there any user-facing changes? Yes. `arrow_cast(expr, '<type>')` no longer returns `expr` unchanged when `expr` already has that type but carries field metadata; it now produces a value with that type and no metadata, matching `arrow_cast`'s declared return field. No public API changes. -- 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]
