Skip to content

Avoid normalizing hidden values in sliced list set operations - #25300

Merged
neilconway merged 4 commits into
apache:mainfrom
yinli-systems:fix/sliced-set-op-normalization
Sep 17, 2026
Merged

neilconway merged 4 commits into
apache:mainfrom
yinli-systems:fix/sliced-set-op-normalization

Conversation

@yinli-systems

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

A sliced ListArray can retain a child values array that is much larger than its logical range. The floating-point set-operation paths currently call normalize_float_zero() on that full child array before slicing it to the visible offsets. When the backing array contains -0.0, this scans and allocates in proportion to hidden backing data rather than the values the query can observe.

This is particularly expensive for small slices of large arrays. In the added Criterion benchmark, array_distinct over two visible values backed by 1,048,576 Float64 values takes 184.46 us on the parent commit and 0.823 us with this change (about 224x faster). This is a component benchmark, not an end-to-end query speedup.

What changes are included in this PR?

  • Slice each list's visible child range before floating-point zero normalization in array_distinct, array_union, array_intersect, and array_except.
  • Keep row and output indices relative to the normalized slice, including the right-hand concatenation offset used by union and intersection.
  • Preserve the existing full-child path when the entire child array is visible.
  • Add a benchmark that independently varies backing-buffer size and visible-range size, plus an unsliced control.
  • Add regression coverage for non-zero offsets, List and LargeList, signed zero, repeated NaNs, null elements, null list rows, and an empty visible list.

Benchmark results on the same machine and parent commit (c149764), using Criterion's 100-sample estimates:

Input Parent This PR
2 visible values, 1,024 backing values 1.024 us 0.858 us
2 visible values, 1,048,576 backing values 184.46 us 0.823 us
2,048 visible values, 1,048,576 backing values 167.27 us 12.50 us

With two visible values, the new implementation remains approximately flat as the backing array grows from 1,024 to 1,048,576 values. Increasing the visible range still increases runtime as expected.

What is the testing strategy for this PR?

The new unit tests exercise all affected set operations and make the pre-slice and in-slice values intentionally different so incorrect absolute/local index conversion is observable.

Validation completed locally:

  • cargo test -p datafusion-functions-nested (127 tests and 2 doctests)
  • Extended workspace test command from AGENTS.md, including all 519 sqllogictest files
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • cargo bench -p datafusion-functions-nested --bench array_set_ops -- array_distinct_sliced_float

Are there any user-facing changes?

There are no API or result-semantics changes. Sliced floating-point list set operations avoid work and allocation for child values outside their logical range while retaining the existing signed-zero, NaN, and null behavior.

@github-actions github-actions Bot added the functions Changes to functions implementation label Sep 14, 2026
@codecov-commenter

codecov-commenter commented Sep 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.81119% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.33%. Comparing base (b300cea) to head (800a042).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/functions-nested/src/set_ops.rs 89.38% 3 Missing and 9 partials ⚠️
datafusion/functions-nested/src/except.rs 86.66% 1 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25300      +/-   ##
==========================================
+ Coverage   82.28%   82.33%   +0.04%     
==========================================
  Files        1137     1137              
  Lines      430211   432049    +1838     
  Branches   430211   432049    +1838     
==========================================
+ Hits       354018   355714    +1696     
- Misses      54771    54832      +61     
- Partials    21422    21503      +81     

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

@neilconway neilconway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yinli-systems ! The writeup describing the root cause and the proposed fix is very good. The fix itself seems right.

Minor, but I think the benchmark matrix is a bit overkill: they were probably useful when developing this PR, but we probably just need a single case to catch regressions here moving forward. e.g., create_sliced_float_array(1024 * 1024, 2) would probably be good enough.

Comment thread datafusion/functions-nested/src/set_ops.rs Outdated
Comment thread datafusion/functions-nested/src/set_ops.rs Outdated
@yinli-systems

Copy link
Copy Markdown
Contributor Author

Thanks @neilconway — addressed all three suggestions in 284a8cf:

  • restored the RowConverter / IEEE 754 totalOrder rationale
  • extracted normalize_visible_values and reused it across union/intersect/distinct/except
  • reduced the benchmark to the single create_sliced_float_array(1024 * 1024, 2) regression case

Validation: cargo fmt --all -- --check, full all-target/all-feature Clippy with warnings denied, the repository-required extended workspace test suite, and all 128 datafusion-functions-nested library tests pass. The focused benchmark runs at about 1.16 µs on this machine.

@neilconway neilconway left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Minor suggested cleanup, otherwise lgtm.

Comment thread datafusion/functions-nested/src/set_ops.rs Outdated
@neilconway
neilconway added this pull request to the merge queue Sep 17, 2026
Merged via the queue into apache:main with commit bf67e97 Sep 17, 2026
41 checks passed
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.

3 participants