perf: use compact pruning for large string NOT IN lists - #24781
perf: use compact pruning for large string NOT IN lists#24781kumarUjjawal wants to merge 1 commit into
Conversation
github-actions
Bot
added
documentation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24781 +/- ##
==========================================
- Coverage 81.53% 81.53% -0.01%
==========================================
Files 1123 1123
Lines 406041 406218 +177
Branches 406041 406218 +177
==========================================
+ Hits 331049 331192 +143
- Misses 55631 55652 +21
- Partials 19361 19374 +13 ☔ 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
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
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?
5 participants
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