perf: adaptive direct mapped group values for dense primitive group by keys - #24313
Draft
Dandandan wants to merge 3 commits into
Draft
perf: adaptive direct mapped group values for dense primitive group by keys#24313Dandandan wants to merge 3 commits into
Dandandan wants to merge 3 commits into
Conversation
Single column primitive group by currently hashes every row. When the values are integers spanning a narrow range - ids, dates, small enums, dictionary codes - the group index can be looked up by indexing a vector with `value - min` instead, which removes hashing from the hot path. The direct mapped table is not chosen up front: the values are hashed as before while their range is tracked, and the table is only built once the groups fill at least 1/8 of the range they span (small tables are always worth it). Deciding from the first batch alone would be wrong in both directions, since a batch of a dense column looks sparse simply because it holds a fraction of the values. Values outside the range grow the table while it stays dense enough, otherwise the groups move back to the hash table for good. Slot lookup is a single wrapping subtraction on a bias-mapped u64, so values below the table are rejected by the same bounds check as values above it. Group values stay in `values` in group index order, so emitting is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYz9cxwBdfZLvcKmWcoiF9
Contributor
Author
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark tpch_mem |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch_memResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (900ca24) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch_memCPU Details (lscpu)Details
Resource Usagetpch_mem — base (merge-base)
tpch_mem — branch
File an issue against this benchmark runner |
Tracking the range of a batch can itself rule the values out, which is what happens on the first batch of a type that cannot be direct mapped at all, such as a float group by key. `try_migrate_to_dense` was called anyway and tripped its own debug assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYz9cxwBdfZLvcKmWcoiF9
Dandandan
changed the title
Contributor
Author
|
run benchmark vectorized_aggregation sql_aggregation |
Contributor
Author
|
run benchmark clickbench_extended |
Contributor
Author
|
run tpch10 tpch_mem10 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (12d6df5) to ad977b0 (merge-base) diff Run configurationrun benchmark clickbench_extendedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (12d6df5) to ad977b0 (merge-base) diff Run configurationrun benchmark sql_aggregationResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (12d6df5) to ad977b0 (merge-base) diff Run configurationrun benchmark vectorized_aggregationResults will be posted here when complete File an issue against this benchmark runner |
|
Benchmark for this request failed. Run configurationrun benchmark sql_aggregationLast 20 lines of output: Click to expandFile an issue against this benchmark runner |
|
Benchmark for this request failed. Run configurationrun benchmark vectorized_aggregationLast 20 lines of output: Click to expandFile an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (12d6df5) to ad977b0 (merge-base) diff Run configurationrun benchmark clickbench_extendedCPU Details (lscpu)Details
Resource Usageclickbench_extended — base (merge-base)
clickbench_extended — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark tpch10 tpch_mem10 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (12d6df5) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch10Results will be posted here when complete File an issue against this benchmark runner |
Review cleanups, no intended change in behaviour beyond the two fixes noted below: - Whether a type can be direct mapped is a compile time fact, so it is an associated const rather than a per value `Option`. Non integer types are now ruled out when the group values are created instead of by scanning a batch, which supersedes the earlier fix for float keys. - The range is tracked as the groups are created while hashing rather than by a second pass over every batch. The range of all the values equals the range of the distinct ones, so this is the same range for O(new groups) instead of O(rows), and removes the ~5% cost this used to add to columns that never become dense. - Slot lookup casts straight to `u64`: the bias cancels in the wrapping subtraction, so it only obscured that the join side maps keys the same way (see `ArrayMap`). - Growing and building the table were the same decision procedure twice; they are one function taking the range to cover. - `EmitTo::First` walks the groups rather than the slots, of which there can be many more. Fixes: values found not to be dense no longer keep hashing after the groups are drained, which matters because a stream is reused across spill cycles and the post spill keys arrive sorted. Slack added to a table is also part of the range it records, so groups created in that slack are still placed when the table is later rebuilt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYz9cxwBdfZLvcKmWcoiF9
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch_mem10Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (12d6df5) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch10CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch_mem10CPU Details (lscpu)Details
Resource Usagetpch_mem10 — base (merge-base)
tpch_mem10 — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmarks |
Contributor
Author
|
run benchmark tpch_mem |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark tpch_memResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing perf/primitive-group-values-no-hash-cache (2138753) to ad977b0 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — 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
2 participants
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?
Rationale for this change
Single column primitive
GROUP BYhashes every row. When the keys are integers over a narrow range — ids, dates, small enums, dictionary codes — the group index can be found by indexing a vector withvalue - min, with no hashing at all.Micro-benchmark,
UInt64keys, 8192 row batches, 3 runs alternated with the baseline to cancel machine drift (M-series laptop):The last row is sparse enough that the table is never built; the range is tracked as groups are created, so watching for density costs nothing per row. Memory is lower whenever the table is used: 4 bytes per slot against 16 bytes per hash table bucket.
What changes are included in this PR?
GroupValuesPrimitivegains a direct mapped store next to the existing hash table:u64, so values below the table are rejected by the same bounds check as values above it.valuesin group index order, soemitis unchanged.The hash path itself is untouched, so anything that does not qualify behaves exactly as before.
Two spill tests were retuned: they assert that a fixed memory limit forces a spill, and the direct mapped table reports less memory than the hash table it replaces.
Are these changes tested?
Yes — 14 unit tests in
primitive.rscovering direct mapped interning, growth upwards and downwards, rejection of wide ranges, sparse values staying hashed, a column that only looks sparse until enough values are seen, falling back mid-stream without renumbering groups, groups created in a grown table's slack surviving a rebuild, reconsidering density after the groups are drained, nulls, floats,EmitTo::Firstre-indexing andclear_shrink. Each asserts that every input row maps to a group holding that row's value, so it holds whichever store was used. Fulldatafusion-physical-plansuite passes (1693 tests), as doesaggregate_fuzzunder--features extended_tests.Are there any user-facing changes?
No API changes. Aggregations on integer keys report lower memory use, so a query near a memory limit may now succeed where it previously spilled.