Reuse hash values during Aggregate(partial) -> Repartition -> Aggregate(final) - #23720
Closed
gabotechs wants to merge 4 commits into
Closed
Reuse hash values during Aggregate(partial) -> Repartition -> Aggregate(final)#23720gabotechs wants to merge 4 commits into
gabotechs wants to merge 4 commits into
Conversation
github-actions
Bot
added
physical-expr
Contributor
Author
|
run benchmarks tpch10 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (45fa16b) to d0304b3 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23720 +/- ##
==========================================
+ Coverage 80.69% 80.71% +0.01%
==========================================
Files 1095 1096 +1
Lines 372623 373436 +813
Branches 372623 373436 +813
==========================================
+ Hits 300695 301425 +730
- Misses 53979 54021 +42
- Partials 17949 17990 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
gabotechs
changed the title
Contributor
Author
|
run benchmarks clickbench_partitioned |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (45fa16b) to d0304b3 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmarks tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (45fa16b) to d0304b3 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
gabotechs
force-pushed
the
gabotechs/reuse-hashes
branch
2 times, most recently
from
July 20, 2026 13:24
1158083 to
1c84f94
Compare
Contributor
|
At a highlevel, this change sounds good. It would be interesting to see if the partial aggregation skipping behavior can be disabled with no impact. I think this should do it. Since the hashes are re-used the partial aggregation should be less expensive. |
Contributor
Author
|
Yeah, I can imagine how you'd want to enable this based on the hashing expressions. For example, maybe hashing on a simple I still want to see if we can reach a situation where this enabled 100% of the times is worth it, that would simplify things a bit. |
Contributor
I think for primitives I found it can be a quite slower to store the hash rather than regenerate it inside a loop as hashing is just is a shift / multiply / xor (potentially SIMD) vs a potential cache miss / extra cache usage. For (long) strings it can of course be very beneficial. |
Contributor
Agree, what is worse: the internal hash column will be repartitioned and coalesced together with other normal columns, and
Seems strange that q33 and q34 not get improvement... |
Contributor
Author
|
run benchmarks tpch10 tpcds clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (7d64c1d) to d0304b3 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (7d64c1d) to d0304b3 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (7d64c1d) to d0304b3 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (43d60d9) to 050046a (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
gabotechs
force-pushed
the
gabotechs/reuse-hashes
branch
from
July 27, 2026 10:19
43d60d9 to
0a9d82c
Compare
Contributor
Author
|
run benchmarks tpch10 tpcds clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (842267c) to e8a65f2 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (842267c) to e8a65f2 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (842267c) to e8a65f2 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
gabotechs
force-pushed
the
gabotechs/reuse-hashes
branch
from
July 27, 2026 15:55
d1bd5b7 to
af01982
Compare
Contributor
Author
|
run benchmarks tpch10 tpcds clickbench_partitioned |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (af01982) to e8a65f2 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (af01982) to e8a65f2 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing gabotechs/reuse-hashes (af01982) to e8a65f2 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
gabotechs
force-pushed
the
gabotechs/reuse-hashes
branch
from
July 27, 2026 16:06
af01982 to
e43f13e
Compare
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
gabotechs
force-pushed
the
gabotechs/reuse-hashes
branch
from
July 27, 2026 16:23
e43f13e to
e2066ab
Compare
gabotechs
force-pushed
the
gabotechs/reuse-hashes
branch
from
July 28, 2026 09:43
e2066ab to
5283bc6
Compare
discord9
pushed a commit
to discord9/datafusion
that referenced
this pull request
Jul 28, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Precedes apache#23720. While displaying a plan with metrics, allow filtering them by name, not just by category or type. This is very useful when writing snapshot tests where only a specific metrics needs to be asserted, without other unrelated metrics polluting the snapshot assertion. Regardless of what happens with apache#23720, I think this PR is still worth it, as it allows creating some really nice `insta` tests asserting runtime properties reliably by just cherry picking the runtime metrics relevant for that specific test. This is relevant not only within DataFusion codebase, but also for other people's codebases using DataFusion and relying on `insta` for snapshot testing, See an example of this here: https://github.com/apache/datafusion/pull/23720/changes#diff-8281405e117428077c19c07afbbe59fed303a3460096e958f761d34f0899b618 ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> While displaying metrics, allows filtering them by name ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes, by a new small test. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> People using the metrics display API will be able to filter metrics by name.
Contributor
Author
|
After trying several things out, and a lot of profiling, I don't think it's worth pursuing this. Some things that I've tried:
The performance gains are so small that, even if they can be replicated locally, they fall within the typical noise of the benchmarks. Looking at profiles, the % of CPU samples we can save by reusing hashes looks something like this: TPCH-SF10: 1.3% -> 1.3% of CPU time hashing. Almost no saves, because hashes are mainly over primitive types, or because hashing the first time dominates massively, as Aggregate(partial) compresses a lot. So my conclusion is that, unless we come up with specific workloads where hashing demonstrates to be a bottleneck, I don't think the complexity is worth it. |
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
6 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.
Still in draft 🙈, expect heavy changes
Which issue does this PR close?
Rationale for this change
During partial + final aggregations, in order to accumulate + repartition data cross partitions, a hash is computed over the grouping expressions.
This hash is computed 3 times:
And depending on what's getting hashed, it can be an expensive operation (e.g., hashing arbitrary length strings).
This PR introduces a mechanism that allows reusing the hashes across different operators, by propagating them under an internal column in the schema:
What changes are included in this PR?
Introduces to core structs that aim to set the foundations of hash reuse across operators:
ExpressionHasher: handles hashing columns based on expressions, maintaining an internal buffer for reusing allocations, stamping the hashes into internal columns so that they can cross operator boundaries, and retrieving those hashes from the internal columns if present.GroupHashTracker: during groupings, it keeps track of the per-group hashes so that operators that aggregate can emit them as an additional internal column for reuse purposes.These two structs are used across all the aggregation implementations in the codebase for reusing hashes.
Additionally, some metrics for tracking how many hashes where computed VS how many hashes where reused are also added to operators that need to deal with hashes
Are these changes tested?
Yes, by existing and new tests.
Are there any user-facing changes?
No, this should be transparent to users.