GH-17211: [C++] Add `hash32` and `hash64` scalar compute functions by kszucs · Pull Request #45001 · apache/arrow

kszucs · 2024-12-11T12:29:11Z

Rationale for this change

Support for calculating elementwise hashes.

The PR adds two scalar functions hash32() and hash64() using the existing internal hashing machinery.

What changes are included in this PR?

Continuation of #39836 with the following changes:

Use column oriented hash-combine rather than flattening nested elements
Support arbitrary nesting levels with an optimization that only hash child arrays if they are also nested
Carry nullness in the output validity bitmap rather than reserving a hash value as a null sentinel: a null input row produces a null output row. A null struct field at any depth makes the whole row null; a null list or map element does not, since only the row's own validity matters there.
Hash dictionaries by their decoded values, so arrays encoding the same logical values via different dictionaries agree, and a valid index pointing at a null dictionary entry produces null.

Are these changes tested?

Yes. scalar_hash_test.cc covers the supported types, slicing of nested and independently-offset children, null propagation through nesting, and the unsupported-type errors. test_compute.py adds hypothesis tests asserting the null contract and that hashing a slice equals slicing the hash. Also verified under ASAN.

Are there any user-facing changes?

There are two new compute kernels, hash32 and hash64, available, documented in compute.rst. Null input rows produce null output rows.

GitHub Issue: [C++][Compute] Add scalar_hash function #17211

kszucs · 2024-12-11T13:16:39Z

Seems like we generate the same hash for both NULL and 0 which is not ideal.

In [1]: import pyarrow as pa

In [2]: import pyarrow.compute as pc

In [3]: pc.hash_64([None])
Out[3]:
<pyarrow.lib.UInt64Array object at 0x124247be0>
[
  0
]

In [4]: pc.hash_64([0])
Out[4]:
<pyarrow.lib.UInt64Array object at 0x1033027a0>
[
  0
]

zanmato1984

Some first glance comments. I'll look into more details later.

- Move ToColumnArray out of the FastHashScalar template (not dependent on the template args) and drop its unused ctx parameter - Include registry_internal.h in scalar_hash.cc so RegisterScalarHash has a visible prior declaration (-Werror=missing-declarations) - Simplify key_hash_benchmark registration to use add_arrow_compute_benchmark, dropping the manual EXTRA_LINK_LIBS block - Remove dead commented-out DoNotOptimize lines in key_hash_benchmark.cc - Fix out-of-order include in hashing_benchmark.cc - Update \since tags to 26.0.0 to match current dev version - Fix docs typo and note that null values hash to a sentinel, not null

…arks - scalar_hash_test.cc: the length>=4 slicing check was an "else if" after a length>=1 check, making it permanently unreachable; split into independent checks so both slicing cases actually run - hashing_benchmark.cc: remove unused kSeed/hashing_rng and the now dead arrow/testing/random.h include (this file generates its own random data via MakeIntegers/MakeStrings, never used the RNG)

- List-like types (list, large_list, fixed_size_list, map): hashing child values reused the parent's element offsets directly as byte offsets into the hashed-child buffer, without accounting for the hashed code's width (4 bytes for hash32, 8 for hash64). This corrupted results in a way that depended on row position, so two occurrences of the same nested value at different rows could hash differently. Fixed by folding each row's child hashes directly (reusing Hashing32/64::CombineHashes, now exposed publicly for this) instead of routing through Hasher::HashMultiColumn's byte-oriented var-length path. - Struct: a null struct row hashed based on whatever data happened to be in its child fields, since HashMultiColumn only tracks each child column's own validity, never the struct's own. Now forced to the documented 0 sentinel explicitly, matching every other type. - Added a benchmark for list hashing and extensive test coverage: duplicate/distinct content, null vs. empty distinctness, null hashing to 0 across all types (including a hand-built case where a null row's offset range is non-empty, per the columnar format spec).

Now that HashArray's is_list_like branch handles list/large_list/ fixed_size_list/map directly (folding child hashes locally instead of routing through Hasher::HashMultiColumn), ToColumnArray is never called with a nested array type or a non-null list_values_buffer: every caller passes either a plain field, or an already-reduced UInt32/UInt64 array. Drop the unreachable branches and the now-unused parameter.

HashChild attached the *parent* array's validity buffer and null_count to the ArrayData wrapping a nested (list-like or struct) child field's hashes, instead of the child's own. A field that is independently null while its parent struct row is valid was therefore invisible to the outer ToColumnArray/HashMultiColumn call: the field's already-correctly- zeroed hash data got re-hashed via HashFixed as if it were ordinary data, producing a non-zero, non-deterministic-looking result instead of the documented 0 sentinel. Fixed by using the child's own buffer(0)/null_count. Verified safe: a struct's child fields are always stored unsliced (offset 0), matching the offset=0 that ArrayData::Make defaults to here, so no misalignment is introduced. HashChild no longer needs the parent ArraySpan at all.

Replace the hardcoded 3 * sizeof(int32_t) * kMiniBatchLength formula (used identically for both the Hashing32 and Hashing64 benchmarks) with each hasher's own kHashBatchTempStackUsage constant, so the stack size actually tracks what HashMultiColumn needs instead of a duplicated magic number. Also fixes a stale comment in KeyHashIntegers64 that referred to Hashing32.

ColumnArrayFromArrayData was being rebuilt on every benchmark iteration, adding conversion and ASSERT overhead to the timed section and measuring more than just HashMultiColumn. Build it once before the loop for both KeyHashIntegers32 and KeyHashIntegers64.

for (const std::string& func : {"hash32", "hash64"}) binds the loop variable to a temporary constructed from each const char* literal, which GCC's -Wrange-loop-construct flags (and CI treats as an error on the AVX2 and ARM64 C++ jobs). Take the loop variable by value instead, matching the compiler's own suggested fix. Verified with g++-15 against the exact warning flag.

array.GetBuffer(2) was dereferenced unconditionally for binary_like/ large_binary_like types, unlike buffer(0)/(1) just above which are already null-checked. Per the columnar format spec, a zero-length array's values buffer may legitimately be omitted (null); match the existing defensive pattern instead of assuming it's always present.

…functions

… struct offset and empty-struct bugs in hash32/hash64 ArrayData::Slice() doesn't slice child_data, so a small slice of a large list/map array used to hash the entire unsliced child values array. Now only the referenced range is hashed (~1600x faster for heavily-sliced arrays, per the new Hash64ListInt64HeavilySliced/Hash64StringHeavilySliced benchmarks; binary-like arrays were already fine). Also fixes two real correctness bugs found while working on the above: - A struct field's own pre-existing offset (independent of the struct's own offset) was silently ignored when slicing that field's column, producing wrong hashes for structs built from already-offset fields. - Hashing a zero-field struct (struct<>) segfaulted: HashMultiColumn was called with an empty columns vector and read cols[0] unconditionally. Found via pyarrow hypothesis fuzzing, confirmed with a minimal repro. HashArray is now split into per-shape routines (HashStructArray, HashListArray) with HashArray acting as a router.

Property-based check that hashing a slice matches slicing the hash of the unsliced array, across the full hash_types strategy (primitives, lists, structs, dictionaries, maps, and nested combinations). Caught a real segfault on zero-field structs during fuzzing, fixed separately in scalar_hash.cc.

…to hash32/hash64 instead Adding kAddend to Hashing32/Hashing64::HashIntImp changed the hash for every user of the shared engine (hash-join, group-by), not just the new scalar hash32/hash64 kernels, to solve a problem specific to those two kernels (a valid 0-valued row colliding with the null-is-0 sentinel). Restore HashIntImp to its original form and instead remap a valid row's hash away from 0 in scalar_hash.cc's own output, after calling the shared, unmodified HashMultiColumn. Updated the test's HashPrimitive reference helper to apply the same remap so it stays a valid independent cross-check.

…s reject cleanly HashableMatcher only checked the top-level type id, so an extension type wrapping a union/view/run-end-encoded type passed dispatch and only failed later with a raw TypeError from ToColumnArray, instead of the NotImplemented a plain (non-extension) instance of the same unsupported type produces. Unwrap extension types (recursively, matching HashArray's own recursive unwrap) before checking.

…D_SIZE_LIST slicing - ZeroValueDoesNotCollideWithNull: the existing NullHashIsZero test only covered int8/int32, but the fix affects every fixed-width type whose byte width is a power of 2 up to 8 (ints, floats, dates, times, timestamps, durations). Check all of them explicitly rather than relying on RandomPrimitive happening to generate an exact zero. - FixedSizeListSliceOfLargerArrayMatchesIndependentArray: the existing slice-correctness test only covered LIST; FIXED_SIZE_LIST computes its referenced range via arithmetic instead of reading an offsets buffer, a genuinely different code path that wasn't covered on its own.

HashChild built its returned ArrayData with offset 0, but reused the child's raw validity buffer as-is. That buffer requires bit index child.offset + i to read logical row i, while every caller (via ToColumnArray, which never applies ArrayData::offset itself) reads buffer bit 0 as row 0. If a struct's nested field is itself an offset slice of a larger array, this misread validity by child.offset bits, misclassifying valid rows as null (or vice versa) and producing wrong combined hashes. Confirmed with a repro: a struct wrapping a list field sliced to offset=3 hashed row 0 (valid) as 0, colliding with null, while an equivalent independently-built struct hashed it correctly. Fixed by repacking the validity bitmap into a fresh, 0-based buffer in HashChild, so it's self-consistent with the already-0-based hash values buffer built alongside it.

…tirely HashArray already zeroes out[i] for every genuinely-null row of `sliced` (via ZeroNulls or the valid-0 remap, both of which correctly use sliced's own offset), so null-ness is already fully encoded in the hash values themselves. A validity buffer is therefore unnecessary -- and reusing the child's raw one would need rebasing anyway, since it's unshifted while the returned ArrayData has offset 0. Simpler and cheaper than repacking a copy.

The rest of compute.rst uses a single blank line between sections; the Hash Functions insertion had picked up an extra one on each side.

…os in hot benchmark loops StructArray::Slice() doesn't reslice child_data, so a struct's nested (list/struct) field was being hashed in full (child.length rows) even when only a small slice of the struct was requested -- the same class of bug fixed for list/map child data earlier, just for struct fields. Hash only the referenced range instead (~580x faster for a heavily sliced struct with a nested list field, per the new Hash64StructWithNestedListHeavilySliced benchmark). Also switch scalar_hash_benchmark.cc's hot loops from ASSERT_OK_AND_ASSIGN to CallFunction(...).ValueOrDie(), since the gtest assertion machinery isn't meant for and adds needless overhead inside a benchmarked loop.

{input_keycol} constructed a fresh std::vector on every iteration, adding allocation overhead that distorted the measurement, especially for small inputs.

They claimed the result is always an Array and referenced a "NestedArray" type that doesn't exist in Arrow. Clarify that the result matches the input's shape (Array/ChunkedArray), that nested types (struct, list, map, etc.) combine child values per row recursively, and mention the null sentinel behavior and lack of cross-version hash stability.

For LIST/LARGE_LIST/FIXED_SIZE_LIST/MAP, rel_start was computed as offsets[0] - values.offset and then HashChild was called with values.offset + rel_start, which algebraically cancels to just offsets[0] -- so values.offset was never actually applied. This produced incorrect hashes whenever the values/items child itself carried a pre-existing nonzero offset independent of the parent array (e.g. a list built via FromArrays with an already-sliced values array). Fix: define rel_start/rel_end as pure logical indices into `values` (relative to values.offset, matching how offsets buffers and GetValues<T> already work), and correspondingly adjust CombineOffsetRows's bias and the FIXED_SIZE_LIST per-row start formula so they no longer assume the old (buggy) rel_start definition.

…tinel Copilot review flagged that HashStructArray (and, by the same pattern, HashListArray) fed field/element hashes into HashMultiColumn/CombineRange without remapping a 0 result the way the leaf path already does -- so a struct whose fields are all valid could still legitimately combine to the same 0 used for a null struct row. Confirmed with a repro: struct{f0: 0} (int64) hashes to exactly 0 for both hash32/hash64, indistinguishable from a null struct. Fix reuses the leaf path's remap, but struct fields need an extra exclusion: a field independently null within an otherwise-valid struct row is documented to hash to 0 too (apacheGH-17211), including transitively through nested structs, so the remap must skip any row where a direct or nested child is null -- otherwise it would incorrectly overwrite that legitimate 0 with a nonzero sentinel. Also strengthens the hash32/hash64 hypothesis tests, which previously only checked determinism, to assert the null-sentinel and no-collision invariants against arbitrarily-shaped generated arrays.

…G variance MinGW CI failed TestScalarHash.RandomPrimitive: hash_set.size() was 48 vs a required 48.02 (tolerance 0.98). This isn't a hashing bug -- the test generates its arrays via RandomArrayGenerator, which uses std::uniform_int_distribution directly; that distribution's algorithm is implementation-defined, not just seed-defined, so the same seed can legitimately produce a different sequence (and occasionally a duplicate value, hence a correctly-duplicate hash) on a different platform/standard library. Loosen the tolerance to 0.9, enough to absorb an incidental duplicate or two without masking a real hash-quality regression. HashQuality already covers hash quality rigorously with inputs that are unique by construction, unaffected by this.

HashStructArray tracked any_child_null correctly but only skipped the null-sentinel remap for those rows, relying on HashMultiColumn to have already produced a literal 0. That only holds for column 0's null rows; a null in any later column instead combines with the running hash of earlier columns (the behavior HashMultiColumn's other caller, hashing independent group-by/join key columns, needs). Force the struct-level invariant explicitly instead. Extends the existing regression test with a multi-field case, since the single-field case couldn't catch this.

The three functions were identical except for the string-length range passed to MakeStructArray. Collapse into one Hash64StructWithStrings, parameterized via benchmark::State::range() and registered with ->Args() per size bucket.

The kernels declared OUTPUT_NOT_NULL and encoded a null row as the hash value 0, which forced remapping any valid row that legitimately hashed to 0 and made every nested combine step preserve that reserved value. Nullness now lives in a real output validity bitmap: HashArray and friends thread an out_validity parameter, so a valid row may hash to anything, and ZeroNulls and RemapValidZeroHashes are gone. The rules themselves are unchanged: a null row is null, a struct row with an independently-null field at any depth is null, and a list/map row's own validity is all that matters for it. Also: decode dictionaries to their logical values rather than hashing raw indices, so different dictionaries encoding the same values agree and a valid index into a null dictionary entry is null; canonicalize a null child's hash value before a parent folds it in, or list<struct<f0:int32>> rows [{f0: 7}] and [null] (whose f0 slot also holds 7) collide; reject unsupported dictionary value types at dispatch instead of deep inside Cast; and speed up validity handling via CopyBitmap/BitmapAnd and by not deep-copying ArraySpan per field (hash64 over int64 2.2x, over list<int64> 1.5x). Docs and the Python tests asserted the old contract and are updated.

fixed_size_binary(0) carries no data, so every value is the same empty string, yet rows hashed differently and an array disagreed with its own slice. ToColumnArray can only describe the type as a fixed-width column of length 0, exactly how a bit-packed boolean is encoded too, so HashMultiColumn called HashBit and took each row's hash from a bit that doesn't exist -- uninitialized memory, varying with the row's bit offset. Give every row one fixed hash in HashArray instead. Broken for the plain type all along, and reachable as dictionary(_, fixed_size_binary(0)) once dictionaries began being decoded; found by the pyarrow hypothesis tests. No behavior change otherwise: zero a null element's hash only in HashListArray, whose CombineRange folds values without consulting validity, and inline that helper into its one caller -- struct fields need none of it, since HashMultiColumn receives their validity and already fixes each null row's contribution. Drop single-use CombineOffsetRows so both row-folding branches read alike, and tighten scoping and comments.

github-actions Bot added Component: C++ Component: Documentation awaiting committer review Awaiting committer review labels Dec 11, 2024