Copilot commented on code in PR #45001:
URL: https://github.com/apache/arrow/pull/45001#discussion_r3644228810
##########
cpp/src/arrow/compute/key_hash_internal.cc:
##########
@@ -299,10 +299,11 @@ void Hashing32::HashBit(bool combine_hashes, int64_t
bit_offset, uint32_t num_ke
template <bool T_COMBINE_HASHES, typename T>
void Hashing32::HashIntImp(uint32_t num_keys, const T* keys, uint32_t* hashes)
{
- constexpr uint64_t multiplier = 11400714785074694791ULL;
+ constexpr uint64_t kMultiplier = 11400714785074694791ULL;
+ constexpr uint64_t kAddend = 9756277977048271785ULL;
for (uint32_t ikey = 0; ikey < num_keys; ++ikey) {
- uint64_t x = static_cast<uint64_t>(keys[ikey]);
- uint32_t hash = static_cast<uint32_t>(BYTESWAP(x * multiplier));
+ uint64_t x = static_cast<uint64_t>(keys[ikey]) + kAddend;
+ uint32_t hash = static_cast<uint32_t>(BYTESWAP(x * kMultiplier));
Review Comment:
Adding a fixed `kAddend` into `HashIntImp` changes the integer hash function
globally for all users of `Hashing32/Hashing64` (e.g., group-by / joins), not
just the new `hash32/hash64` scalar kernels. If the goal is only to reserve 0
as a null sentinel for the new public scalar hashes, it would be lower-risk to
keep the existing key-hash algorithm unchanged and remap/avoid 0 only in the
scalar hash kernel output path (or explicitly document why changing the shared
hash is acceptable).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
