Avoid normalizing hidden values in sliced list set operations - #25300
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
neilconway
left a comment
There was a problem hiding this comment.
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.
|
Thanks @neilconway — addressed all three suggestions in 284a8cf:
Validation: |
neilconway
left a comment
There was a problem hiding this comment.
Great! Minor suggested cleanup, otherwise lgtm.
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.