Skip to content

perf: extend string IN-list pruning benchmark coverage - #24836

Open
goutamadwant wants to merge 3 commits into
apache:mainfrom
goutamadwant:perf-24707-in-list-pruning-benchmarks
Open

perf: extend string IN-list pruning benchmark coverage#24836
goutamadwant wants to merge 3 commits into
apache:mainfrom
goutamadwant:perf-24707-in-list-pruning-benchmarks

Conversation

@goutamadwant

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

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?

  • Add domain sizes 1, 2, 4, 8, and 16 while retaining the existing boundary and large-domain cases.
  • Evaluate each domain at 16, 256, and 4,096 pruning containers.
  • Keep construction benchmarks independent of container count.
  • Preserve the existing 4,096-container Criterion benchmark IDs so historical baselines remain comparable.
  • Validate both representations produce the same pruning results for every evaluation matrix cell outside the timed loop.

The production pruning implementation and its current threshold are unchanged.

What is the testing strategy for this PR?

  • ./dev/rust_lint.sh

  • cargo 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_encryption
  • Full 72-case Criterion matrix:

    cargo bench --locked --profile release-nonlto -p datafusion-pruning --bench string_in_list_pruning -- --sample-size 10 --warm-up-time 0.1 --measurement-time 0.2 --noplot

Are there any user-facing changes?

No. This PR only extends benchmark coverage.

@codecov-commenter

codecov-commenter commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.61%. Comparing base (33028d5) to head (90c966e).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend the compact IN-list pruning benchmark

3 participants