bench: add window aggregate filter benchmarks - #24589
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24589 +/- ##
==========================================
+ Coverage 81.31% 81.43% +0.12%
==========================================
Files 1117 1118 +1
Lines 395911 399550 +3639
Branches 395911 399550 +3639
==========================================
+ Hits 321918 325376 +3458
+ Misses 55177 55172 -5
- Partials 18816 19002 +186 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kumarUjjawal
left a comment
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thank you @lyne7-sc for working on this.
Overall looks good, I had two questions which I mentioned. Please take a look.
| argument_kinds: &[ArgumentKind], | ||
| filter_percents: &[usize], | ||
| ) { | ||
| let mut group = c.benchmark_group(format!("window_aggregate_filter/{name}")); |
There was a problem hiding this comment.
bounded_window sets sample_size(10) on its group, should we set it here too, per group so the microsecond-scale whole-partition cases can keep a higher count?
There was a problem hiding this comment.
Thanks. I've reduced the sample size to 10.
There was a problem hiding this comment.
What I had in mind was setting this per group: 10 for the millisecond-scale bounded frames, and leaving whole-partition at the default.
There was a problem hiding this comment.
Updated. the bounded groups use a sample size of 10, while the whole-partition group uses default of 100.
| &[ArgumentKind::Column, ArgumentKind::Power], | ||
| &[30], | ||
| ); | ||
| benchmark_window_case( |
There was a problem hiding this comment.
A whole-partition frame is constant in partition and does a single update_batch, while the cumulative and sliding frames walk row by row and spend their time in range calculation rather than argument evaluation. Would it be worth giving this group all three argument kinds and all three selectivities and trimming the cumulative group back?
There was a problem hiding this comment.
Thanks for the suggestion. I think this is a reasonable balance. I've updated the benchmark coverage.
| argument_kinds: &[ArgumentKind], | ||
| filter_percents: &[usize], | ||
| ) { | ||
| let mut group = c.benchmark_group(format!("window_aggregate_filter/{name}")); |
There was a problem hiding this comment.
What I had in mind was setting this per group: 10 for the millisecond-scale bounded frames, and leaving whole-partition at the default.
| &[ArgumentKind::Column, ArgumentKind::Power], | ||
| &[30], |
There was a problem hiding this comment.
Could we restore divide here, and the 10 and 50 percent points with it?
3 participants
Which issue does this PR close?
Rationale for this change
Add benchmark coverage for evaluating the performance impact of the window aggregate filter argument evaluation changes in #24508.
What changes are included in this PR?
Adds a Criterion benchmark for window aggregates with
FILTERover pre-ordered input.It covers:
BoundedWindowAggExec.WindowAggExec.powerarguments.FILTERselectivities.This PR only adds benchmark coverage and does not change behavior.
Are these changes tested?
Yes.
Are there any user-facing changes?
No.