Skip to content

fix: preserve field metadata for simple CASE branches - #26145

Draft
osipovartem wants to merge 24 commits into
apache:mainfrom
Embucket:upstream-case-field-metadata
Draft

osipovartem wants to merge 24 commits into
apache:mainfrom
Embucket:upstream-case-field-metadata

Conversation

@osipovartem

@osipovartem osipovartem commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR address?

Part of #24860. This covers logical CASE field metadata, complementary to #24865.

What changes?

  • Preserve field metadata from supported CASE result branches, including untyped/typed NULL, casts, aliases, nested CASE, and shallow metadata-bearing functions.
  • Align with the proposed fix: propagate field metadata through coalesce, nvl, and nvl2 #24865 merge policy: merge unrelated keys with earlier-branch precedence; keep Arrow extension name and parameters paired; reject conflicting extension names with a planning error. Missing metadata in one branch does not erase metadata from another.
  • Scan simple column/literal branches once for metadata and nullability. A new defaulted ExprSchema::field_and_nullable method preserves custom nullability overrides; DFSchema resolves both from one field lookup. Once a THEN may be nullable, defer subsequent nullability decisions to the existing reachability-aware logic.
  • Bound exact delegation through nested expressions at depth 8; unsupported or deeper branches retain conservative empty metadata. Physical CaseExpr metadata and constant-false simplification remain separate follow-ups.

Validation

  • Current head: datafusion-common --lib passes 644/644 tests; datafusion-expr --lib passes 301/301; datafusion-optimizer --lib passes 954/954, including custom-nullability/error and short-circuit regression cases. All-targets Clippy for both packages, rustfmt, and staged diff check pass.
  • A Criterion benchmark covers 10/100/1000 plain literal, plain column, and late-marked CASE branches. Both revisions were built in separate Cargo target directories using the same optimized ci-bench profile and host; expression construction is outside the timed loop. In this single short run, 1,000 distinct plain columns took ~1.420 ms before this PR and ~1.466 ms now: an observed +3.2% (~46 us) for a synthetic wide-schema planning case. Smaller plain-column cases were mixed; 1,000 plain literals were ~72.5 vs ~72.8 us. This does not establish end-to-end query performance or a statistically robust regression. The late-marked case is not a like-for-like performance comparison: the old code incorrectly omitted its metadata.
  • Earlier revisions passed optimizer and SQL integration tests. The latest changes have not yet been exercised by SQL integration tests. The local host volume has 246 GiB free, below our 250 GiB guard for a full DataFusion build.
  • Independent read-only review approved the draft update, not merge readiness. Before marking ready: run the SQL integration test and representative end-to-end planning measurements, then align with the shared helper if fix: propagate field metadata through coalesce, nvl, and nvl2 #24865 lands. Existing Codecov comments measure older commits.

@codecov-commenter

codecov-commenter commented Oct 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.40395% with 137 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.76%. Comparing base (4e6e564) to head (7cc7ac7).

Files with missing lines Patch % Lines
datafusion/expr/src/expr_schema.rs 73.93% 39 Missing and 89 partials ⚠️
datafusion/optimizer/src/analyzer/type_coercion.rs 86.36% 2 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26145      +/-   ##
==========================================
- Coverage   82.77%   82.76%   -0.02%     
==========================================
  Files        1148     1148              
  Lines      451075   451632     +557     
  Branches   451075   451632     +557     
==========================================
+ Hits       373397   373810     +413     
- Misses      54930    54976      +46     
- Partials    22748    22846      +98     

☔ 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.

@osipovartem
osipovartem marked this pull request as draft October 8, 2026 23:00
@osipovartem osipovartem changed the title fix: preserve common field metadata through CASE expressions fix: preserve field metadata for simple CASE branches Oct 8, 2026
@osipovartem
osipovartem marked this pull request as ready for review October 9, 2026 08:58
@james-willis

Copy link
Copy Markdown

PTAL at #24865

I originally had this strict metadata policy like you do here but changed it to be much more permissive on the basis of @paleolimbot 's comments.

@osipovartem

Copy link
Copy Markdown
Contributor Author

Thanks for pointing me to #24865. I agree CASE should use the same policy: merge unrelated metadata keys permissively, keep extension name/parameters paired, and reject conflicting extension names. The current strict full-map equality in this PR is inconsistent with that direction. I will align it with the shared helper and add CASE-specific tests for missing/unrelated metadata and conflicting extension names before this is merged.

…-metadata

# Conflicts:
#	datafusion/expr/src/expr_schema.rs
@osipovartem
osipovartem marked this pull request as draft October 28, 2026 00:29
@github-actions github-actions Bot added the common Related to common crate label Oct 28, 2026
@paleolimbot

paleolimbot commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

While I don't necessarily mind the approach here, I think it needs some justification for why it's needed over the simpler fix. This is a good workaround for when you don't control the ScalarUDF source code, but the actual bug here is that return_field() for case when and friends are dropping metadata.

I put a note on the other PR as well, but I recently discovered intersect_metadata_for_union(). While I think it is also not perfect, it might be better to use it here to reduce the number of times we define this logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants