feat: add BuildHasher variants for hash_utils#21820
Merged
Merged
Conversation
xudong963
marked this pull request as draft
April 24, 2026 04:36
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/hash-buildhasher (0823c8d) to fd093fb (merge-base) diff File an issue against this benchmark runner |
|
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
xudong963
changed the title
xudong963
marked this pull request as ready for review
April 24, 2026 06:16
Member
Author
|
@Dandandan, this PR resolves the regression for the #21429. It would be sweet to get your thoughts! |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Member
Author
|
run benchmark with_hashes |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/hash-buildhasher (ddc3035) to 95cda37 (merge-base) diff using: with_hashes File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagewith_hashes — base (merge-base)
with_hashes — branch
File an issue against this benchmark runner |
Contributor
|
Thanks @xudong963 for the work! Three things:
Question: what's the near-term consumer of |
Member
Author
|
@zhuqi-lucas Thanks! Current HEAD already has custom-hasher coverage for Dictionary and checks equivalence with the underlying Utf8 values. I’ll add coverage for Struct and long Utf8View values, including the external-buffer path. For parity, I can add scoped single-column leaf tests using the same RandomState. Full bit-for-bit parity is not currently a valid invariant: the optimized HashState path seeds primitive/view rehashing from the previous column hash, while the generic BuildHasher path uses combine_hashes. Multi-column inputs and multi-field structs can therefore differ even with the same builder. I’ll document that distinction; if full parity is expected, we should align on the API design first. Both null helpers currently use mul_col; I’ll rename both to multi_col. The near-term consumer is from Massive's latest stateful streaming system that hashes Arrow group-key columns with a fixed-seed XXH3 builder for stable key-group and checkpoint routing. (The project is still private). I guess if @AdamGS has some other use cases. |
Contributor
Got it, thanks for this info @xudong963 ! |
zhuqi-lucas
approved these changes
Jul 16, 2026
Member
Author
|
run benchmark with_hashes |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/hash-buildhasher (a1cbec8) to 18121a6 (merge-base) diff using: with_hashes File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagewith_hashes — base (merge-base)
with_hashes — branch
File an issue against this benchmark runner |
github-actions
Bot
removed
sql
Contributor
|
Shall we merge it? |
alamb
reviewed
Jul 26, 2026
| F: FnOnce(&[u64]) -> Result<R>, | ||
| S: BuildHasher, | ||
| { | ||
| build_hasher::with_hashes_with_hasher(arrays, hash_builder, callback) |
Contributor
There was a problem hiding this comment.
Is there some way to reduce the duplication here? For example, can we update with_hashes to call this method with the default hasher?
Member
Author
There was a problem hiding this comment.
with_hashes is an existing API and cannot directly delegate to with_hashes_with_hasher: its HashState path uses seeded rehashing for some multi-column values, so delegation would change existing hash results and bypass the optimized path. However, the thread-local buffer management is duplicated. I’ll extract that part into a small private helper while keeping the two hashing implementations separate.
Member
Author
There was a problem hiding this comment.
I tried extracting the thread-local buffer lifecycle into a shared helper so that with_hashes and with_hashes_with_hasher could reuse it. However, the benchmark showed consistent regressions: approximately 13–16% for struct_array and 13% for utf8: multiple, no nulls. After reverting the extraction, the follow-up benchmark returned those cases to baseline.
The likely cause is the additional generic closure/function boundary around this hot path, which changed inlining and code generation despite the helper being marked #[inline]. Since this code is performance-sensitive and the regression is material, I plan to retain this small amount of duplication to preserve the existing optimized call structure.
Member
Author
|
run benchmark with_hashes |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/hash-buildhasher (b33ccf6) to bb670fb (merge-base) diff using: with_hashes File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagewith_hashes — base (merge-base)
with_hashes — branch
File an issue against this benchmark runner |
xudong963
force-pushed
the
xudong963/hash-buildhasher
branch
from
July 27, 2026 03:46
b33ccf6 to
b1b6240
Compare
Member
Author
|
run benchmark with_hashes |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/hash-buildhasher (b1b6240) to bb670fb (merge-base) diff using: with_hashes File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagewith_hashes — base (merge-base)
with_hashes — branch
File an issue against this benchmark runner |
Member
Author
|
I plan to merge the PR, we can keep iterating if new comments get in in the future. |
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?
Rationale for this change
This PR adds
BuildHasher-based variants forhash_utilsso callers can compute row hashes with a caller-provided hash builder instead of always using DataFusion's defaultRandomState.The main constraint is performance:
with_hashesis a hot path, especially for string, dictionary, and nested array hashing. A previous version in #21429 caused measurable regressions in the defaultRandomStatepath, for examplelarge_utf8: single, no nullsregressed from roughly26.7usto36.3us, andlarge_utf8: multiple, no nullsfrom roughly112usto127us.This version keeps the default path performance-oriented by avoiding a fully generic
BuildHasherrewrite of the existing hot loops.What changes are included in this PR?
This PR adds:
with_hashes_with_hashercreate_hashes_with_hasherThe implementation intentionally uses a hybrid design:
RandomStateleaf hot paths remain specialized.BuildHasherleaf paths live separately inhash_utils/build_hasher.rs.The trade-off is that there is still some duplication for primitive/string/binary leaf loops. That duplication is intentional: those are the hottest loops, and keeping them separate prevents the existing
RandomStatepath from becoming generic overBuildHasheror being perturbed by the custom-hasher implementation.Are these changes tested?
Yes.