Skip to content

fix: propagate field metadata through coalesce, nvl, and nvl2 - #24865

Open
james-willis wants to merge 2 commits into
apache:mainfrom
james-willis:coalesce-metadata
Open

fix: propagate field metadata through coalesce, nvl, and nvl2#24865
james-willis wants to merge 2 commits into
apache:mainfrom
james-willis:coalesce-metadata

Conversation

@james-willis

Copy link
Copy Markdown

Which issue does this PR close?

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 Fields fail to plan over them (see #24860 for a full reproducer).

This is one of several independent metadata drop sites listed in #24860. It is the plan-time schema fix for the conditional functions; it does not by itself change execution-time behavior, because these functions simplify into CASE, which has its own drop sites (also #24860) that I intend to address in follow-up PRs.

What changes are included in this PR?

  • A small unanimous_metadata helper in datafusion/functions/src/utils.rs: returns the metadata shared by every argument field that can contribute a value to the result. Fields with a Null data type (untyped NULL literals) are ignored; if the remaining fields disagree, the result carries no metadata rather than claiming a type identity that only some inputs have.
  • coalesce: return_field_from_args now attaches the unanimous metadata of its arguments. Return type and nullability computation 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 in coalesce.rs and nvl2.rs directly exercise return_field_from_args: metadata propagated when the value arguments agree, dropped when they disagree, untyped NULL arguments not blocking propagation, NVL2's test argument excluded, and nullability unchanged.

There is deliberately no sqllogictest: arrow_metadata() reads its argument field at execution time, and at execution these functions have been simplified into CASE, whose own metadata drops (#24860) still lose the metadata en route. End-to-end SLT coverage lands with the CASE fix. (This ordering is safe: the optimizer's schema invariant intentionally ignores metadata — assert_expected_schema / logically_equivalent_names_and_types — so a plan whose coalesce carries metadata that the simplified CASE does not does not error; it just degrades to today's behavior until the CASE fix lands.)

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. No API changes.


cc @paleolimbot — this is the first slice of #24860, in the same family as your #22112; you'd mentioned on #21984 you could help shepherd this class of metadata fixes. This is my first DataFusion PR, so CI will need a committer to trigger it.

@github-actions github-actions Bot added the functions Changes to functions implementation label Sep 1, 2026
@james-willis

james-willis commented Sep 1, 2026

Copy link
Copy Markdown
Author

In this PR, if the branches of the coalesce, etc don't agree on the metadata, we drop extension metadata defensively.

My opinion is that this should actually be a planning time error unless all rows will hit only one branch but wanted to take the more conservative approach for now. I want to solicit opinions from the reviewers on what the desried behavior is here.

These functions built their return Field from argument data types only,
dropping field metadata (e.g. Arrow extension types) from the planned
schema. Attach the metadata shared by every value-contributing argument
instead. Part of apache#24860.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.62%. Comparing base (fefc227) to head (b48d6ba).

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #24865    +/-   ##
========================================
  Coverage   81.61%   81.62%            
========================================
  Files        1123     1123            
  Lines      409392   409506   +114     
  Branches   409392   409506   +114     
========================================
+ Hits       334143   334245   +102     
- Misses      55630    55637     +7     
- Partials    19619    19624     +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@james-willis
james-willis marked this pull request as ready for review September 1, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants