Repository navigation
perf: Faster grouped first_value / last_value with a single primitive ORDER BY key - #27303
Open
freakyzoidberg wants to merge 6 commits into
Open
freakyzoidberg wants to merge 6 commits into
freakyzoidberg wants to merge 6 commits into
Conversation
…imitive ORDER BY key With a single ORDER BY expression of primitive type, grouped first_value and last_value now keep each group's ordering as a native value and compare it directly in one pass per batch, instead of ScalarValue orderings. The value state gets a batched update_rows. Results are unchanged: ties keep the first row, pre-ordered input keeps the first or last row, and the partial state is [value, ordering, is_set]. Tests compare the typed path with the generic one (ORDER BY k vs ORDER BY k, u, and on tied keys) and keep two-column twins of the tests it takes over. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A fresh accumulator grew it by doubling during its first batch. The freed blocks then scattered the per-group nested values allocated right after, which made evaluate for nested values 5-10% slower in benches/first_last.rs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Single-key queries there now run on the typed accumulator. The generic accumulator keeps its unit tests, and the clustered memory test orders by two columns to stay on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…omparison Keep Int8, UInt64, Float16, Float64, Decimal128, Decimal256, a timezone-aware Timestamp and Interval(MonthDayNano), and drop the array_agg check of the generated data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
sqllogictest
Contributor
|
run benchmark first_last |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/first-last-primitive-key (edf2a3c) to 9e0a08f (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 perf/first-last-primitive-key (edf2a3c) to 9e0a08f (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 |
Member
Author
|
Thanks, Looking mostly good, with 2x to 5x accross, digging a bit more in this one: the other "regression" appear to be noise (evaluate_bench with microsecs) |
Contributor
|
Probably this one is also worth a look |
When most rows beat their group's winner, recording every winning row and de-duplicating afterwards cost as much as it saved. Keep one entry per touched group instead, which also removes keep_last_update_of_each_group and ValueState::overwrite_is_cheap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
Thanks, that one and the coalesce_peers (winner changes) should be fixed now. Before the per-batch scratch buffer was sized to the batch input row count (1 MB) and freed inside the timed closure (evaluate_bench improvement), now its capped to the number of groups (updates on the same group key are done in place in a scratch, then merge in the state at the end of batch - (coalesce_peers improvement)) Could you re-run run benchmark first_last? |
jayzhan211
requested changes
Nov 11, 2026
jayzhan211
left a comment
jayzhan211
left a comment
Contributor
There was a problem hiding this comment.
@freakyzoidberg one test change requested inline. I also benchmarked head against the merge-base on an M-series laptop: both regressions flagged above are gone (coalesce_peers separate x3 (winner changes) 54.8 → 42.4 ms, evaluate_bench ... first(2) 11.8 → 0.59 µs), and the other cases I ran are 60–80% faster.
Contributor
|
run benchmark first_last |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/first-last-primitive-key (4a70a2e) to 9e0a08f (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 perf/first-last-primitive-key (4a70a2e) to 9e0a08f (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 |
gabotechs
approved these changes
Nov 12, 2026
gabotechs
left a comment
gabotechs
left a comment
Contributor
There was a problem hiding this comment.
+1 to @jayzhan211's comment about increasing test coverage in clustered_partial_stream.rs, but otherwise this looks good!
FIRST_VALUE(v ORDER BY v) now runs the single primitive key accumulator, so ORDER BY v, k alone left its shrink_to_fit untested. Run both orderings, so each case fails if its own accumulator stops releasing its scratch. Suggested by @jayzhan211.
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_valueordered by a single primitive column #27301.Rationale for this change
Grouped
first_value/last_valuewithORDER BYkeep each group's ordering as aVec<ScalarValue>. Every batch winner is converted toScalarValues and compared withcompare_rows, and every batch scans a scoreboard sized to all groups. With many groups, this dominates the query.What changes are included in this PR?
FirstLastPrimitiveKeyGroupsAccumulator<K>, used when there is exactly one ORDER BY expression of primitive type. It is picked withdowncast_primitive!, likeGroupValuesPrimitivefor GROUP BY keys. It keeps the orderings in aVec<K::Native>plus a null bitmap and compares them directly, in one pass per batch.ValueState::update_rows, so the value state is updated once per batch.[value, ordering, is_set]) and the rows picked, ties included, are the same as with the existing accumulator, which still handles every other ordering.What is the testing strategy for this PR?
first_last_primitive_key.sltcomparesORDER BY kwithORDER BY k, u.uis unique, so the existing accumulator is the oracle. It covers:first_last_ordered.slt,group_by.slt,aggregate.slt) now run on the new path unchanged.Benchmarks
cargo benchon an M4 Max,mainvs this PR:first_value update_bench nulls=0%, filter=falselast_value update_bench nulls=0%, filter=truelast_value merge_bench nulls=0%, filter=falsefirst_value update_bench struct(i64,utf8,f64) nulls=0%first_value merge_bench map<utf8,i64> nulls=0%first_value coalesce_peers(i64,utf8,f64) separate x3 (winner changes)first_value evaluate_bench nulls=0%, filter=false, first(2)Are there any user-facing changes?
No.
AI usage
Written with the help of AI tools
I've reviewed and understand the code.