adriangb opened a new pull request, #24831: URL: https://github.com/apache/datafusion/pull/24831
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/24721 ## Rationale for this change This is an alternative to https://github.com/apache/datafusion/pull/24670 by @gene-bordegaray, opened as a **draft** to show the diff and CI for the smaller shape rather than to compete with it. The projection commit here is his work, taken verbatim. Field metadata on a `ProjectionExec`'s output schema can silently disappear when the physical optimizer removes or rewrites projections, so query results lose declared field metadata and metadata-reading expressions return wrong answers. That is #24721, and @gene-bordegaray diagnosed it in three parts (identity-projection removal, collapsing across a metadata boundary, and `make_with_child` rederiving the schema). Fixing those exposes a second, pre-existing bug. 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 disagreement was masked because the optimizer rederives a projection's schema from its expressions, repairing the logical schema on the way through. Once projections preserve metadata faithfully, the repair stops and a cast to an extension type loses it: ```sql SELECT arrow_metadata(CAST(raw AS UUID), 'ARROW:extension:name') -- 'arrow.uuid' before, NULL after ``` #24670 currently addresses that by introducing a `CastTarget` enum (stacked from https://github.com/apache/datafusion/pull/24725), which changes the public `Cast.field`/`TryCast.field` types and fails the semver check, blocking a 55.1 backport. This PR takes the target's metadata when it carries any and otherwise inherits the source's, which resolves the same divergence with no API change. ## What changes are included in this PR? Two commits, each green on its own: 1. `preserve projection field metadata during physical optimization` — @gene-bordegaray's `physical-plan/src/projection.rs` from #24670, unmodified, applied to current `main`. 2. `use the cast target's metadata when it carries any` — 20 lines in `expr/src/expr_schema.rs`. No public types change, so `cargo-semver-checks` stays clean. The rule is deliberately narrower than the three-part sentinel (`name.is_empty() && is_nullable && metadata.is_empty()`) used by the physical cast and by https://github.com/apache/datafusion/pull/23169. That sentinel reclassifies a protobuf-roundtripped `CAST(x AS Utf8)` with `nullable = false` as an explicit target and drops its source metadata; an emptiness check cannot change behaviour for any cast whose target carries no metadata, which seems the safer property for a patch release. Deliberately **not** included, to keep this reviewable and backportable: - Stripping `ARROW:extension:name`/`:metadata` when a cast changes the storage type (https://github.com/apache/datafusion/issues/22079). Real bug, user-visible behaviour change, belongs on its own. - The protobuf gap where `to_proto` writes `field.metadata()` and `from_proto` rebuilds from `arrow_type` + `nullable` only (https://github.com/apache/datafusion/issues/24695), already tracked separately by @gene-bordegaray. - Any change to how the cast target is *represented*. Whether that should be a `CastTarget` enum, the existing sentinel, or dropping source-metadata inheritance entirely is a real discussion, but it is a design question for the next major rather than a blocker on a correctness fix. ## What is the testing strategy for this PR? The existing `datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt` is the regression test: with commit 1 alone it reproduces the CI failure @gabotechs reported on #24670, byte for byte and at the same line, and commit 2 makes it pass. ``` [Diff] (-expected|+actual) - 00010203040506070809000102030506 arrow.uuid + 00010203040506070809000102030506 NULL at datafusion/sqllogictest/test_files/cast_extension_type_metadata.slt:36 ``` Commit 2 also adds `expr_schema::tests::test_cast_output_field_metadata`, covering both directions of the rule for `Cast` and `TryCast`. Reverting the rule while keeping the test makes it fail, so it is load-bearing rather than decorative. Locally: the full sqllogictest suite (504/504 files) and the `datafusion-expr`, `-physical-expr`, `-physical-plan`, `-sql`, `-proto` and `-optimizer` unit tests (5495 passed, 0 failed) are green. ## Are there any user-facing changes? Yes, both are bug fixes to schemas that were already wrong: - Field metadata declared by a projection survives physical optimization. - A cast whose target field carries metadata reports that metadata on `to_field()`, matching what the physical plan already produces. A plain `CAST(expr AS type)` synthesizes a target with no metadata, so its behaviour is unchanged. No breaking changes to public APIs. -- 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]
