Skip to content

perf: Replace SipHash with foldhash in BoundedWindowAggExec - #23984

Merged
neilconway merged 2 commits into
apache:mainfrom
neilconway:neilc/perf-window-partition-foldhash
Jul 30, 2026
Merged

perf: Replace SipHash with foldhash in BoundedWindowAggExec#23984
neilconway merged 2 commits into
apache:mainfrom
neilconway:neilc/perf-window-partition-foldhash

Conversation

@neilconway

@neilconway neilconway commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

PartitionWindowAggStates and PartitionBatches were using IndexMap's default SipHash hasher. Switching to foldhash yields a small but measurable performance improvement.

Benchmark below. Note that the improvement is modest in part because linear mode is very inefficient when there are many partitions (#23982); I will send a PR for that separately.

Benchmarks:

  • linear, 10,000 partitions: 216.7 ms -> 204.1 ms (~6%)
  • linear, 100 partitions: 45.0 ms -> 45.4 ms (within drift)
  • sorted, 10,000 partitions: 35.0 ms -> 34.8 ms (control)

What changes are included in this PR?

  • Add new benchmark
  • Switch from using SipHash to foldhash

Are these changes tested?

Yes, covered by existing tests.

Are there any user-facing changes?

No functional change at all.

Covers Linear mode (all partitions stay live until input end, state maps
probed for every live partition on every batch) at two partition
cardinalities, plus Sorted mode as a control where finished partitions
are pruned eagerly.
PartitionWindowAggStates and PartitionBatches were using IndexMap's
default SipHash hasher. Switching to foldhash yields a small but
measurable performance improvement.

This improves performance in linear mode for 10k partitions by ~6%.
@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate labels Jul 29, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.75%. Comparing base (2f25454) to head (ef78b05).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #23984      +/-   ##
==========================================
- Coverage   80.75%   80.75%   -0.01%     
==========================================
  Files        1096     1096              
  Lines      373588   373588              
  Branches   373588   373588              
==========================================
- Hits       301687   301685       -2     
+ Misses      53898    53897       -1     
- Partials    18003    18006       +3     

☔ 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.

@neilconway

Copy link
Copy Markdown
Contributor Author

@viirya Let me know if you have a chance to take a look :)

@viirya viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Swapping the two IndexMap aliases (PartitionWindowAggStates, PartitionBatches) to foldhash RandomState is purely a hasher change — IndexMap preserves insertion order and partition-key equality is decided by PartitionKey's Eq, neither of which the hasher affects, so results are unchanged. Same idea as #23956.

One thing I checked since it's the easy way to break a hasher swap: the two construction sites in bounded_window_agg_exec.rs are correctly updated from IndexMap::new() (only valid for the default hasher) to IndexMap::default(), which works for any Default hasher and builds the same empty map. Good.

Benchmark is honest about the modest win and the linear-mode caveat (#23982). LGTM.

@neilconway

Copy link
Copy Markdown
Contributor Author

@viirya Thanks for the review!

@neilconway
neilconway added this pull request to the merge queue Jul 30, 2026
Merged via the queue into apache:main with commit 1ae6b87 Jul 30, 2026
42 checks passed
@neilconway
neilconway deleted the neilc/perf-window-partition-foldhash branch July 30, 2026 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize BoundedWindowAggExec to use foldhash instead of SipHash

3 participants