Repository navigation
Preserve field metadata through physical CASE expressions - #26158
Draft
osipovartem wants to merge 16 commits into
Draft
osipovartem wants to merge 16 commits into
osipovartem wants to merge 16 commits into
Conversation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #26158 +/- ##
==========================================
- Coverage 82.77% 82.76% -0.01%
==========================================
Files 1147 1147
Lines 450909 451585 +676
Branches 450909 451585 +676
==========================================
+ Hits 373252 373776 +524
- Misses 54934 55007 +73
- Partials 22723 22802 +79 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gafiatulin
pushed a commit
to gafiatulin/datafusion
that referenced
this pull request
Oct 30, 2026
…#24865) ## Which issue does this PR close? - Part of apache#24860. ## Rationale for this change `COALESCE`, `NVL`/`IFNULL`, and `NVL2` drop the field metadata of their arguments when computing their return field: `return_field_from_args` builds the output `Field` from the argument *data types* only. For columns carrying Arrow extension types (`ARROW:extension:name` / `ARROW:extension:metadata` — e.g. `arrow.uuid`, `geoarrow.wkb`), the planned schema of any expression containing these functions silently loses the extension-type identity, and metadata-aware UDFs that dispatch on their argument `Field`s fail to plan over them (see apache#24860 for a full reproducer). This is one of several independent metadata drop sites listed in apache#24860. It is the plan-time schema fix for the conditional functions. These functions simplify into `CASE`, whose logical and physical drop sites are addressed separately in apache#26145 and apache#26158. ## What changes are included in this PR? - `datafusion_common::metadata::coerced_fields_metadata`: computes the metadata for an expression whose result may come from any one of several inputs coerced to a common type. It lives next to the related metadata utilities so that `CASE` (in `datafusion-expr`) can share it: - Metadata is merged across the inputs whose data type is not `Null` (an untyped NULL literal carries no metadata and cannot contribute a typed value), with earlier inputs taking precedence for repeated keys. Unrelated keys are not required to agree, so metadata that arrived with the data (e.g. from an Arrow file) cannot cause an extension type to be dropped. - The extension type keys are handled as a unit, as in `Expr::Cast` output field derivation: differing values of `ARROW:extension:name` are a planning error, and `ARROW:extension:metadata` always comes from the input that supplied the extension name (byte-wise differences in it are tolerated, since extension parameters are often JSON). - `coalesce`: `return_field_from_args` attaches the merged metadata of its arguments. Return type and nullability are unchanged. `NVL`/`IFNULL` delegate to `coalesce` and are fixed by the same change. - `nvl2`: same, over the second and third arguments only — the first argument is only tested for NULL and never contributes a value, so its metadata is excluded. ## What is the testing strategy for this PR? - Unit tests for `coerced_fields_metadata` in `metadata.rs`: merge precedence, untyped NULL inputs skipped, empty result, error on differing extension names, and extension name/metadata kept paired. - Unit tests in `coalesce.rs` and `nvl2.rs` exercise `return_field_from_args`: metadata propagated, untyped NULL arguments not blocking propagation, conflicting extension types rejected, NVL2's test argument excluded, and nullability unchanged. There is deliberately no sqllogictest yet: `arrow_metadata()` reads its argument field at execution time, and by then these functions have been simplified into `CASE`, so end-to-end coverage depends on the `CASE` fixes (apache#26145, apache#26158). This ordering is safe: the optimizer's schema invariant ignores metadata (`assert_expected_schema` / `logically_equivalent_names_and_types`), so a plan whose `coalesce` carries metadata that the simplified `CASE` does not still plans. ## Are there any user-facing changes? - The planned schema (e.g. `DataFrame::schema()`) of expressions containing `COALESCE`/`NVL`/`IFNULL`/`NVL2` over metadata-bearing columns now preserves that metadata. - `COALESCE`/`NVL`/`IFNULL`/`NVL2` over arguments with *differing* `ARROW:extension:name` values (e.g. an `arrow.uuid` column and an `arrow.json` column) is now a planning error. Previously these planned and silently dropped the extension type. - New public function `datafusion_common::metadata::coerced_fields_metadata`. No other API changes.
osipovartem
force-pushed
the
upstream-case-physical-metadata
branch
from
October 30, 2026 17:23
57507b6 to
ef9bdc4
Compare
osipovartem
force-pushed
the
upstream-case-physical-metadata
branch
from
October 30, 2026 18:00
ef9bdc4 to
3fad8a2
Compare
osipovartem
marked this pull request as ready for review
October 30, 2026 18:00
osipovartem
force-pushed
the
upstream-case-physical-metadata
branch
from
October 30, 2026 21:01
3fad8a2 to
dbba94a
Compare
github-actions
Bot
added
logical-expr
osipovartem
force-pushed
the
upstream-case-physical-metadata
branch
from
October 30, 2026 21:52
dbba94a to
4693a34
Compare
(cherry picked from commit d05bc23)
osipovartem
force-pushed
the
upstream-case-physical-metadata
branch
from
October 30, 2026 22:09
4693a34 to
3018e53
Compare
|
Marked as draft after 14 days without activity: merge conflicts. Please address these items and mark Ready for review when ready. |
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
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?
Part of #24860. Stacked on #26145; merge the logical contract first. Rebased onto logical
bd6ab71dd9723494d1e0c47a1908d96ba9c2d84a, including its main merge. Physical-only comparison.Rationale for this change
Physical CASE drops field metadata, including for CASE-based NVL/IFNULL/NVL2 rewrites. Execution-time consumers such as
arrow_metadataread physical argument fields; physical-only inference also cannot reconstruct logical annotations discarded during lowering.What changes are included?
coerced_fields_metadata: earlier keys win, extension name/parameters stay paired, and incompatible extension names are rejected.What is the testing strategy?
End-to-end SQL cases are consolidated in
metadata.slt, including #26895's intended results, aliases, branch precedence, NULLs, extension conflicts and CASE-based function rewrites. This follow-up adds binary/NOT unmarked-branch checks and a nine-level metadata-bearing function case, and updates the planner assertion to require retained logical/physical metadata after alias lowering.The new deep-function requirement and stale planner assertion were each reproduced failing before correction. Focused validation at
dbba94af0: 70/70 CASE tests; default-feature physical-expression suite 1,764 passed, 2 existing ignored; bothmetadata.sltandcast_extension_type_metadata.sltpassed. Protobuf/JSON tests inspect decoded fields and values, not only projection schemas. Existing tests retain scalar, typed/untyped NULL, rewriting, borrowed-field and field-name scaling coverage.The final main-synced stack at
3018e535a, based on logicalbd6ab71dd, passed the documented extended-workspace run: 13,383 Rust tests (8 existing ignored) and 545/545 SQLLogicTest files. Strict all-target/all-feature workspace Clippy with-D warnings, rustfmt and diff checks passed on this stack. The earlier pre-main-merge run passed 13,375 Rust tests, including 1,878 proto/recursive-feature-enabled physical-expression tests with 1 ignored. These revision/feature-specific counts are not interchangeable denominators.Performance evidence and limits
The current cutoff removal delegates to cached function fields; it does not add row-wise evaluation. The inherited ordinary CASE kernels and fast paths remain covered. The final logical binary-path measurements, costs versus main and deep CASE/UDF limitation are disclosed in #26145; the structural worklist does not establish global planning linearity. The rebased stack was rerun with the logical binary-inference and NOT/NULL fixes, not only the earlier cutoff-removal revision.
The earlier physical screen used 31 fixtures, 20 Criterion samples, one-second warmup/measurement, Rust 1.99.0 and
ci-bench, comparing185efd08dto the subsequent physical implementation with the same logical dependency, not Apache main. Those are historical measurements, not a fresh timing run of this rebased revision:That screen disclosed approximately 6% wide-planning cost and an unresolved 180-to-218-ns scalar NULL fixture difference. A separate earlier three-control comparison against main
49c67c296measured main/candidate medians of 9.682/9.814 us (literal branches), 9.125/8.991 us (column branches), and 4.151/4.131 us (column/implicit NULL). These do not qualify current release-profile, end-to-end SQL, Iceberg or SPCS latency, and are not a zero-regression claim. API/performance decisions remain maintainer merge gates.Are there user-facing or API changes?
CASE-derived fields retain result metadata. New
PhysicalCaseNodefields are binary-protobuf wire-additive, and old encoded plans decode without a pin. Adding public generated Rust fields breaks exhaustive struct literals and needs maintainer/API-version coordination. Older binary readers can ignore and lose the annotation; older generated JSON readers may reject unknown fields. Mixed-version metadata preservation is not promised.The physical-only portion does not change
ExprSchema. End-to-end behavior requires #26145; this stack must not merge ahead of that dependency and the API/performance decisions above.