feat: add BuildHasher variants for hash_utils#21429
Closed
xudong963 wants to merge 9 commits into
Closed
Conversation
(cherry picked from commit 02b972d)
Contributor
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (6a1cc02) to cdfade5 (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 |
Contributor
|
run benchmark with_hashes |
Contributor
These might be regressions, I am rerunning once more. |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (6a1cc02) to cdfade5 (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
marked this pull request as draft
April 8, 2026 07:30
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (1dec571) to cdfade5 (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 |
This reverts commit 1dec571.
Keep the default RandomState path concrete while adding dedicated BuildHasher entry points and tests. Also wire the parquet feature through object_store/tokio so datafusion-common all-features clippy builds cleanly.
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (c022fb4) to cdfade5 (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 |
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (501b223) to cdfade5 (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 |
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (87812d2) to cdfade5 (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 |
…om hot loop The #[inline(never)] hash_dictionary_inner carried a C: ChildHashing type parameter, causing different monomorphization for the RandomState path vs original code. This regressed dictionary_utf8_int32 by ~15%. Restructure: separate "hash dict values" (differs per hasher) from "scatter hashes to keys" (shared hot loop). The new hash_dictionary_scatter takes dict_hashes: &[u64] with no hasher parameter, so both paths call the exact same monomorphization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (3a3bfc5) to cdfade5 (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 |
…che locality The _with_hasher leaf functions (~600 lines) were interleaved between the hot RandomState functions, polluting the instruction cache for the default path. Move all BuildHasherHashValue trait/impls and _with_hasher leaf functions to the end of the file (after create_hashes_with_hasher, before tests) so the hot default path stays tightly packed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
run benchmark with_hashes |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing xudong963/upstream-pr46 (1ad2c2d) to cdfade5 (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 |
Member
Author
|
supressed by #21820 |
pull Bot
pushed a commit
to TCeason/arrow-datafusion
that referenced
this pull request
Jul 27, 2026
## Which issue does this PR close? - Closes apache#21428. ## Rationale for this change This PR adds `BuildHasher`-based variants for `hash_utils` so callers can compute row hashes with a caller-provided hash builder instead of always using DataFusion's default `RandomState`. The main constraint is performance: `with_hashes` is a hot path, especially for string, dictionary, and nested array hashing. A previous version in apache#21429 caused measurable regressions in the default `RandomState` path, for example `large_utf8: single, no nulls` regressed from roughly `26.7us` to `36.3us`, and `large_utf8: multiple, no nulls` from roughly `112us` to `127us`. This version keeps the default path performance-oriented by avoiding a fully generic `BuildHasher` rewrite of the existing hot loops. ## What changes are included in this PR? This PR adds: - `with_hashes_with_hasher` - `create_hashes_with_hasher` - custom-hasher implementations for primitive, string, binary, byte-view, dictionary, and nested arrays - tests covering custom hashers, multi-column hashing, and dictionary equivalence The implementation intentionally uses a hybrid design: - Default `RandomState` leaf hot paths remain specialized. - Custom `BuildHasher` leaf paths live separately in `hash_utils/build_hasher.rs`. - Nested/structural logic is shared through an internal child-hashing adapter, so struct/list/map/union/run/dictionary behavior does not need to be broadly duplicated. 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 `RandomState` path from becoming generic over `BuildHasher` or being perturbed by the custom-hasher implementation. ## Are these changes tested? Yes. --------- Co-authored-by: Dmitrii Blaginin <dmitrii@blaginin.me> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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
3 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
datafusion_common::hash_utilscurrently exposes hashing helpers only for DataFusion'sRandomState. This makes the code harder to reuse from callers that need a customBuildHasherfor integration or test scenarios.What changes are included in this PR?
with_hashes_with_hasherandcreate_hashes_with_hasherpublic APIs.RandomStatepath for the currentwith_hashesand
create_hashesAPIs.hash_utilstests to cover the new custom-hasher entry points.force_hash_collisionsso the feature-specific test configuration keeps passing.Are these changes tested?
Yes.
cargo test -p datafusion-common hash_utilscargo test -p datafusion-common --features force_hash_collisions hash_utilsAre there any user-facing changes?
Yes.
datafusion-commonnow exposeswith_hashes_with_hasherandcreate_hashes_with_hasherfor callers that need a customBuildHasher.