bench(functions-aggregate): benchmark coalescing peer first_value into a struct (#23682) - #24559
Open
zhuqi-lucas wants to merge 2 commits into
Open
bench(functions-aggregate): benchmark coalescing peer first_value into a struct (#23682)#24559zhuqi-lucas wants to merge 2 commits into
zhuqi-lucas wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a benchmark comparing separate primitive first_value accumulators with a coalesced struct accumulator.
Changes:
- Adds coalesce-peers benchmark cases.
- Covers
i64,utf8, andf64inputs. - Registers the benchmark in the aggregate suite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24559 +/- ##
==========================================
- Coverage 81.34% 81.34% -0.01%
==========================================
Files 1117 1117
Lines 397528 397528
Branches 397528 397528
==========================================
- Hits 323385 323372 -13
- Misses 55225 55236 +11
- Partials 18918 18920 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
run benchmark first_last |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing bench/first-last-coalesce-peers (58c0117) to 5610e58 (merge-base) diff Run configurationrun benchmark first_lastResults will be posted here when complete File an issue against this benchmark runner |
zhuqi-lucas
changed the title
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing bench/first-last-coalesce-peers (58c0117) to 5610e58 (merge-base) diff Run configurationrun benchmark first_lastCPU Details (lscpu)Details
Resource Usagefirst_last — base (merge-base)
first_last — branch
File an issue against this benchmark runner |
neilconway
reviewed
Aug 21, 2026
| #[expect(clippy::unit_arg)] | ||
| black_box( | ||
| acc.update_batch( | ||
| &[Arc::clone(values), Arc::clone(&ord)], |
Contributor
There was a problem hiding this comment.
Feeding the same ord array in to every iteration is a little unrealistic, no? That will measure comparison cost, but doesn't capture workloads where the winner changes over time and the aggregate's running value needs to be updated.
Contributor
Author
There was a problem hiding this comment.
Thanks @neilconway for the review — good catch. You're right: reusing one ord meant iters 2..100 only hit compare-and-reject, so the update path wasn't measured.
Pushed a (winner changes) variant that feeds a strictly-decreasing ord per iteration, so every row becomes a new winner and the running value is replaced+copied every time — this exercises the update path (where the struct plan copies one wider row vs N narrow ones) that the reused-array version skipped. Kept the original as (winner stable) so both the compare-reject and compare-replace paths are covered. Will post fresh numbers once the bot reruns.
Adds coalesce_comparison_bench to the first_last benchmark: N independent primitive first_value accumulators (the pre-rewrite plan) vs one struct-valued accumulator carrying the same N columns (the post-rewrite plan produced by the CoalesceFirstLast optimizer rule). The struct path uses the native nested GroupsAccumulator from apache#23628. Runs as 'first_value coalesce_peers(i64,utf8,f64) separate x3' vs '... coalesced struct' so the head-to-head win is visible in the same run. Part of apache#23600.
The existing coalesce-peers head-to-head reused one `ord` array every iteration, so after the first iteration it only exercised compare-and-reject (the pure ordering-comparison path). Add a `(winner changes)` variant that feeds a strictly-decreasing `ord` per iteration, forcing the running value to be replaced+copied on every row, and label the original `(winner stable)`. This covers both the compare-reject and compare-replace paths, so the struct plan's wider running-value copy is measured too, per review feedback.
zhuqi-lucas
force-pushed
the
bench/first-last-coalesce-peers
branch
from
August 21, 2026 14:57
58c0117 to
7b438cb
Compare
Contributor
Author
|
run benchmark first_last |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing bench/first-last-coalesce-peers (7b438cb) to 5f0ba13 (merge-base) diff Run configurationrun benchmark first_lastResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing bench/first-last-coalesce-peers (7b438cb) to 5f0ba13 (merge-base) diff Run configurationrun benchmark first_lastCPU Details (lscpu)Details
Resource Usagefirst_last — base (merge-base)
first_last — branch
File an issue against this benchmark runner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
first_value/last_valuecoalesce epic).Rationale for this change
The
CoalesceFirstLastoptimizer rule (#23682) rewrites N peerfirst_value(col ORDER BY o)expressions that share oneORDER BYinto a singlefirst_value(named_struct(...) ORDER BY o). The win is at the accumulator level: N independent argmax passes collapse into one struct-valued argmax — one per-row ordering compare instead of N, one slot per group instead of N.This PR adds the benchmark that quantifies that win, so #23682 can cite concrete numbers as its performance justification. Nothing in the suite measured this head-to-head before.
What changes are included in this PR?
Adds
coalesce_comparison_benchtofunctions-aggregate/benches/first_last.rs, producing two cases in the same run:first_value coalesce_peers(i64,utf8,f64) separate x3— three independent primitivefirst_valueGroupsAccumulators (the pre-rewrite plan)first_value coalesce_peers(i64,utf8,f64) coalesced struct— one struct-valued GroupsAccumulator carrying the same three columns (the post-rewrite plan)The struct path exercises the native nested
GroupsAccumulatormerged in #23628. Reuses the existingprepare_typed_groups_accumulator/create_struct_arrayhelpers; no new dependencies or imports.Are these changes tested?
Benchmark-only; compiles under
cargo check --benches. Triggeredrun benchmark first_laston this PR for the head-to-head numbers — posted in a comment below.