feat: add BuildHasher variants for hash_utils#21820
Conversation
|
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 |
|
@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. |
|
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 |
|
Thanks @xudong963 for the work! Three things:
Question: what's the near-term consumer of |
|
@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. |
Got it, thanks for this info @xudong963 ! |
|
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 |
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.