perf: use compact pruning for large string NOT IN lists - #24781
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24781 +/- ##
==========================================
- Coverage 81.63% 81.59% -0.05%
==========================================
Files 1123 1123
Lines 409729 411131 +1402
Branches 409729 411131 +1402
==========================================
+ Hits 334496 335448 +952
- Misses 55576 55919 +343
- Partials 19657 19764 +107 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@sunchao would you be interested in reviewing @kumarUjjawal's work? |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on this. The compact NOT IN pruning path looks good overall, and I like the coverage around null semantics, fully matched row groups, page pruning, and mixed intervals. I have one non-blocking suggestion to strengthen the end-to-end coverage.
| // domain, which satisfies NOT IN. Only an interval | ||
| // pinned to one domain value rules out every row. | ||
| // Truncated Parquet bounds cannot fake that: min | ||
| // truncates downward and max upward, so equal bounds |
There was a problem hiding this comment.
Could we add an end-to-end NOT IN case with deliberately truncated Parquet statistics or page-index bounds? This arm relies on truncation producing a lower min and upper max. The current integration coverage uses exact short strings, while the unit test uses synthetic bounds. A small writer setup with a short truncate length would help lock in the conservative behavior and ensure we never incorrectly prune in this case.
sunchao
left a comment
There was a problem hiding this comment.
Thanks @kumarUjjawal. Reusing the sorted-domain machinery from #24526 makes sense, and I found no incorrect-result issue in the NULL, interval, dictionary, or inverse-predicate handling. The mixed-container benchmark shows a substantial improvement.
I found two pruning CPU regressions to address, detailed inline: losing an OR short circuit with one missing bound, and scanning long statistics bounds before ruling out domain membership. Both require a raised max_in_list_size; the default remains 20. The long-bound case additionally requires statistics longer than the default writer's 64-byte truncation.
Validation at 6241218359dc2e6df9c5cd35b7fda03f19cbfdbe: five independent review passes, 98 pruning unit tests, five focused Parquet integration tests, and 132,192 differential/safety checks per revision across six string/dictionary types. The base/head probes used identical sources, separate optimized builds, and serial repeated measurements pinned to one CPU. Timings below measure PruningPredicate::prune with prepared statistics, not whole-query runtime.
Nonblocking benchmark suggestion: add uniform singleton containers. With 21 literals and all 4,096 containers equal to the first excluded literal, evaluation went from 22 to 229 µs; construction plus one evaluation went from 83 to 236 µs. At 256 literals, construction savings outweigh the slower single evaluation, so this is a distribution tradeoff worth documenting and measuring rather than an unconditional regression claim.
| // neither arm. | ||
| (Some(min), None) | ||
| if self.values.last().is_some_and(|v| v.as_bytes() < min) => | ||
| if self.membership == SetMembership::In |
There was a problem hiding this comment.
[P2] Preserve the OR short circuit when only one bound is known
For NOT IN, these guards send every one-sided interval to None, including cases where the old comparison chain returned true. For example, with s_min = 'zzz', absent s_max, and no null rows, every comparison in s NOT IN ('a00', ..., 'a20') is already true on the known bound. Returning NULL still keeps the same containers, but an enclosing OR now has to evaluate its other branch. Modern Parquet min/max fields are independently optional, and this metadata passes the existing ordering/trust checks.
I reproduced this with cap 1,024 and 4,096 Utf8View containers: s NOT IN ('a00', ..., 'a20') OR n IN (0, 10, ..., 10230), with n_min = n_max = 10229, null counts 0 and row counts 128. Base pruning took 0.142 ms versus 5.46 ms here (about 38x); both retained every container. This is extra CPU, not incorrect data.
Could we add explicit NotIn arms that return Some(true) when the known bound is absent from the domain, keeping UNKNOWN when it is a domain member, and cover this composed-OR case?
| // compares for equality rather than order, so it does | ||
| // not rely on the bound ordering the IN arm needs. | ||
| SetMembership::NotIn => { | ||
| Some(min != max || !self.contains(min)) |
There was a problem hiding this comment.
[P2] Avoid scanning long bounds before checking domain membership
This new path first goes through min > max above, then evaluates min != max before looking in the domain. Both comparisons can scan the entire common prefix even when short list literals could reject min immediately. The previous per-literal equality kernels reject such values using their length/prefix.
With cap 21, literals a00000000 through a00000020, and 4,096 Utf8View containers whose bounds are "z".repeat(16384) + "a" and "z".repeat(16384) + "z", pruning took 0.229 ms on base versus 6.63 ms here (about 29x), with identical keep decisions. These are prepared-statistics pruning timings; actual Parquet statistics conversion adds work in both versions. The case is reachable with larger/untruncated statistics or custom statistics providers, while the default writer's 64-byte truncation limits exposure.
Could we handle NotIn before the ordering guard and use a domain-membership rejection before comparing the full bounds, with a benchmark for long common prefixes?
6241218 to
d8b9b0d
Compare
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the updates. I went through the follow-up changes and everything looks good to me.
The truncation concern is addressed by applying four-byte truncation to both row-group statistics and column-index bounds, with the Parquet tests confirming that the affected scans are conservatively retained.
The one-sided interval case is also covered now. The new NotIn handling preserves Some(true) when an available bound falls outside the literal domain, and the unit test exercises this as part of an enclosing OR with a large right-side IN arm.
The long-bound case looks good as well. Checking domain membership before comparing min != max avoids the problematic ordering, and the added benchmark distributions cover long common-prefix bounds.
I did not find any new correctness or performance issues in the follow-up diff. Thanks for addressing the review feedback!
sunchao
left a comment
There was a problem hiding this comment.
Thanks for the updates, @kumarUjjawal. I re-reviewed d8b9b0d66 with five independent passes focused on performance, design, and abstraction.
The missing-bound OR short circuit is restored, and the original comparison of long min/max bounds is removed. The new Parquet truncation test also exercises the intended conservative behavior. I found no incorrect-result issue.
The overall design remains appropriately small: the private membership enum extends the existing expression and shares normalization, domain ownership, and expression plumbing. I would keep that design. One long-string performance regression remains in the membership comparator, detailed inline; it can be addressed within the existing vector without adding a separate hash structure.
The intended large-list improvement remains substantial. On the committed mixed-container benchmark with 1,024 literals, base/head construction was 3.00/0.214 ms and pruning evaluation was 196/0.404 ms. I observed no material positive-IN regression. These are component benchmarks with prepared statistics, not whole-query timings.
Validation against merge-base 27e81e9a45317afeacd7d7dc2b77f10637ecd5d5: 99 pruning unit tests, six focused Parquet integration tests, and 132,192 differential/safety checks per revision across six string/dictionary types all passed. Performance probes used identical sources, separate optimized builds, and serial repeated measurements pinned to one CPU.
| /// Does the sorted, deduplicated domain hold `value`? | ||
| fn contains(&self, value: &[u8]) -> bool { | ||
| self.values | ||
| .binary_search_by(|candidate| candidate.as_bytes().cmp(value)) |
There was a problem hiding this comment.
[P2] Reject impossible string lengths before scanning shared prefixes
There is still a long-prefix regression when the list literals also share the statistics prefix. contains() performs lexicographic comparisons even when the bound and every candidate have different lengths. The former Arrow equality kernels reject these candidates by length without reading the string buffers.
For example, use cap 21, prefix = "z".repeat(16384), literals prefix + "domain00000000" through prefix + "domain00000020", and 4,096 Utf8View containers with min = max = prefix + "a". Identical base/head probes keep every container, but prepared-statistics pruning takes 0.230 ms on base versus 8.96 ms here (about 39x). The added long-prefix benchmark uses short literals, so it misses this case.
This is a CPU regression, not an incorrect-result issue. It requires a raised cap and long untruncated/custom statistics; the default cap of 20 avoids the path, and the control with 64-byte bounds improved when construction was included. Actual Parquet metadata conversion is outside these timings.
Could we sort and search the existing NOT IN domain by (byte length, bytes), using the same comparator in both places, while keeping positive IN lexicographically ordered for interval searches? That rejects absent lengths before scanning prefixes without adding another collection. Please also cover long literals sharing the bounds' prefix in the benchmark.
sunchao
left a comment
There was a problem hiding this comment.
Re-reviewed 53ec3966c650 with five independent passes focused on performance, design, abstraction, and correctness. I found no new defect.
The remaining shared-prefix issue is addressed: NOT IN sorting and binary search now share the same length-first comparator, while IN keeps the lexical order required by its interval search. This is a small, appropriate change within the existing representation; I do not see a need for another index or abstraction.
The previously reported 21-literal / 4,096-container singleton probe with a 16 KiB shared prefix now takes 0.075 ms, versus 0.228 ms on the merge-base and 8.96 ms on the previous revision, with identical keep decisions. The earlier missing-bound OR and long-bound fixes remain effective. Repeated positive-IN evaluation was within 1% of the baseline at 21 and 1,024 literals. The committed mixed-container NOT IN benchmark at 1,024 literals measured 196.6 ms on base versus 0.395 ms here. These are prepared-statistics pruning measurements, not whole-query timings.
One nonblocking coverage suggestion: extend the committed NOT IN equivalence test with mixed byte lengths, Unicode, duplicates, and singleton membership hits in a domain above the compact-path threshold. The current NOT IN test domains have equal-length values; the mixed-length Unicode test exercises IN. My independent mixed-length oracle passed, but retaining that coverage would protect the new ordering invariant.
Validation against merge-base 27e81e9a45317afeacd7d7dc2b77f10637ecd5d5: 99 pruning unit tests, six focused Parquet integration tests, and 132,192 differential/safety checks per revision across six string/dictionary types passed. Measurements used identical probes, separate optimized builds, and serial repeated runs pinned to one CPU.
|
Amazing to see this ready, thank you all!! |
Which issue does this PR close?
IN-list pruning excludesNOT INand NULL-containing lists #24711.Rationale for this change
Large string NOT IN lists create long expression chains for Parquet pruning. These expressions are expensive to build and evaluate.
What changes are included in this PR?
Use a compact sorted domain for large, non-null string NOT IN lists.
Preserve the existing behavior for lists containing NULL.
Add unit tests and Parquet row-group and page-pruning tests.
Add a mixed-container regression test.
Extend the existing benchmark and configuration documentation.
In a local benchmark with 1,024 values, construction was about 17 times faster and evaluation was about 27 times faster.
Are these changes tested?
Yes
Are there any user-facing changes?
No public API changes