Avoid normalizing hidden values in sliced list set operations - #25300
Open
yinli-systems wants to merge 1 commit into
Open
yinli-systems wants to merge 1 commit into
yinli-systems wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25300 +/- ##
========================================
Coverage 81.91% 81.91%
========================================
Files 1134 1134
Lines 425703 425827 +124
Branches 425703 425827 +124
========================================
+ Hits 348725 348832 +107
- Misses 56300 56308 +8
- Partials 20678 20687 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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?
Rationale for this change
A sliced
ListArraycan retain a child values array that is much larger than its logical range. The floating-point set-operation paths currently callnormalize_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_distinctover two visible values backed by 1,048,576Float64values 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?
array_distinct,array_union,array_intersect, andarray_except.ListandLargeList, 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: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)AGENTS.md, including all 519 sqllogictest filescargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkcargo bench -p datafusion-functions-nested --bench array_set_ops -- array_distinct_sliced_floatAre 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.