perf: extend string IN-list pruning benchmark coverage - #24836
perf: extend string IN-list pruning benchmark coverage#24836goutamadwant wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24836 +/- ##
==========================================
- Coverage 81.61% 81.61% -0.01%
==========================================
Files 1124 1124
Lines 412077 412077
Branches 412077 412077
==========================================
- Hits 336318 336298 -20
- Misses 55949 55961 +12
- Partials 19810 19818 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @goutamadwant for working on this.
Overall good! Just one blocking issue, plus resolve the conflicts.
| impl IntervalStatistics { | ||
| fn new(domain_size: usize) -> Self { | ||
| let min = StringViewArray::from_iter_values((0..CONTAINERS).map(|index| { | ||
| fn new(domain_size: usize, container_count: usize) -> Self { |
There was a problem hiding this comment.
The new container-count axis also changes the sampled domain distribution. For a 1,024-value domain, 16 containers exercise only the first 8 values, 256 exercise the first 128, while 4,096 exercise the entire domain twice. That changes binary-search positions, branch behavior, and string-comparison work along with the container count, so the resulting crossover cannot be attributed to container scaling alone. Could we generate evenly distributed positions across the full domain for each container count, then repeat them as needed?
There was a problem hiding this comment.
@kumarUjjawal thanks for checking! pushed a fix to address this. each hit/miss interval pair is now mapped to an evenly spaced position across the full domain, including both endpoints, and the distribution repeats only when the pair count exceeds the domain size. For the 1,024-value domain, 16 and 256 containers therefore sample 8 and 128 evenly distributed positions respectively, while 4,096 containers cover all 1,024 positions twice.
resolved the conflicts too. Let me know if you have any other comments. thanks!
Which issue does this PR close?
IN-list pruning benchmark #24707.Rationale for this change
The string IN-list pruning benchmark only covered domain sizes 20, 21, 256, and 1,024 with a fixed 4,096 pruning containers. This did not provide the small-list or container-scaling measurements needed by the follow-up work in #24709 and #24710.
This PR extends the benchmark before making any production threshold change, following the contributor guide's recommendation to isolate benchmark additions from the optimization they are intended to evaluate.
What changes are included in this PR?
The production pruning implementation and its current threshold are unchanged.
What is the testing strategy for this PR?
./dev/rust_lint.shcargo test --locked -p datafusion-pruning(95 tests passed)Extended workspace test suite:
RUST_BACKTRACE=1 cargo test --locked --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-cli --workspace --lib --tests --bins --features avro,json,backtrace,extended_tests,recursive_protection,parquet_encryptionFull 72-case Criterion matrix:
Are there any user-facing changes?
No. This PR only extends benchmark coverage.