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.

…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.

A NullType field has no validity bitmap at all, so HashStructArray's per-field BitmapAnd silently skipped it and left the row valid even though every NullType row is null.

Prevents the compiler from eliding HashMultiColumn calls whose output is otherwise never read back within the benchmark loop.

Replace the bit-by-bit GenerateBitsUnrolled pass over the output validity bitmap with a CopyBitmap/CountSetBits pair, matching how validity is already copied elsewhere in this file. Also lowercase mid-sentence "hash functions" and hyphenate "run-end encoded"/"view-encoded" in the compute docs.

HashableMatcher only inspected the top-level type id (after unwrapping extension/dictionary), so an unsupported type nested inside a supported one -- list<binary_view>, struct<..., binary_view>, map<.., REE> -- passed dispatch and then failed deep inside ToColumnArray with a raw TypeError instead of a clean NotImplemented. Matches() now recurses into child fields, the same fix already applied for an extension's storage type.

A struct's non-nested children went straight to ToColumnArray, bypassing HashArray's dedicated zero-width branch, so struct<fixed_size_binary(0)> reintroduced the nonexistent-bit read already fixed for the plain type: rows holding the same empty value hashed differently. NeedsRecursiveHash now takes the DataType rather than just its id, so it can claim zero-width fixed_size_binary for the recursive path.

initialize.cc calls RegisterScalarHash unconditionally, but scalar_hash.cc was only listed in CMake, so Meson builds would compile the caller without the definition and fail to link. Wires up all four new sources to match CMake: scalar_hash.cc into the compute lib, scalar_hash_test.cc into arrow-compute-scalar-utility-test, and the scalar_hash and key_hash benchmarks.

A null list/map element had its hash canonicalized to 0 before the fold, dropping its validity. A valid integer 0 also hashes to 0, as does HashMultiColumn's substitution for a null slot, so [null] and [0] hashed alike -- and so did a null struct field, where the element itself is present: map<utf8, int32> entries {"a": null} and {"a": 0}. Any other constant would only narrow the collision, so fold nulls into a second accumulator instead: a valid element folds its hash, a null one folds its position, and the two mix at the end. Nothing there can be mistaken for a value hash, positions keep [null, x] and [x, null] apart, and a row without nulls folds nothing extra, so only real nulls cost anything -- Hash64ListInt64 goes from 89.9us to 103.2us. The validity is the one HashChild propagates, so a struct row with a null field counts as a null element, per the documented semantics.

A map is stored as list<struct<key, item>>, and the struct rule that a null field nullifies the row marked an entry with a null item absent, so its key was never folded: [["a", null]] and [["b", null]] hashed alike, and every map with null items collapsed whatever its keys. Arrow requires non-null keys and allows null items (MapArray::ValidateChildData), so that rule must not apply to a map's entries. Fold the keys and the items as two list folds over the map's own offsets instead, which keeps every key contributing and encodes a null item just as a null list element is. It recurses like any other nested type, so a map's key or item may itself be a map, to any depth. Hashing a map costs about 28% more as a result -- two passes over the entries rather than one fused pass over both columns -- while lists and primitives are unchanged.

- BinaryLike: replace an exact-duplicate CheckBinary call with a case covering a repeated value across rows - ZeroValueIsValid: add float16, the only fixed-width HashIntImp type the test's own header comment claimed to cover but omitted - CheckHashQuality: hoist the null-collapse explanation above both hash32/hash64 branches and fix it mispointing readers to a hash64 branch 'below' when it is actually above - UnsupportedNestedChildType: add a struct nesting an unsupported-value- type dictionary, since UnsupportedDictionaryValueType only exercises that case at the top level - RandomPrimitive: add decimal32/decimal64, which RandomArrayGenerator already supports alongside decimal128/decimal256

scalar_hash.cc uses std::vector/std::string/std::shared_ptr, and the two hash benchmarks use std::shared_ptr/std::unique_ptr, without including their headers directly and relying on transitive includes.

The executor promotes an all-scalar span to length-1 arrays before Exec, so the kernel only sees arrays; pin that down with a test that a scalar hashes as its array row does.

The docs described only array input, though a scalar argument returns a scalar and a chunked one returns chunked. Cover the chunked shape with a test too, which nothing exercised before.

These strings surface through the bindings' function help, so they should carry the same contract the C++ API docs do rather than a subset.

Copilot

🟡 Changes recommended

A critical issue remains where oversized execution spans can wrap to uint32_t and leave hashes unwritten.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 20/20 changed files
Comments generated: 1
Review effort level: Lite

It narrows the count to a uint32, and nothing upstream caps what reaches the kernel: the executor does not split spans by default, and a list's values child can be longer than its parent. Past UINT32_MAX the count wrapped and the tail of the output was left unwritten.

Copilot

🟡 Changes recommended

Fix the critical null dereference in variable-length hashing and add regressions for empty/all-null inputs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

cpp/src/arrow/compute/kernels/scalar_hash.cc:381

The kernel accepts fixed-size binary and documents that null inputs produce null outputs, but scalar promotion calls ArraySpan::FillFromScalar for an invalid FixedSizeBinaryScalar, whose value is null; that code unconditionally dereferences scalar.value before this kernel runs. Thus hash32/hash64 on MakeNullScalar(fixed_size_binary(...)) can crash instead of returning a null scalar. Please handle invalid fixed-size-binary scalars in promotion or add an equivalent pre-kernel short-circuit, with a regression test.

    } else if (!NeedsRecursiveHash(*array.type)) {
      ARROW_ASSIGN_OR_RAISE(auto column, ToColumnArray(array));
      std::vector<KeyColumnArray> columns{column.Slice(array.offset, array.length)};
      HashMultiColumnChunked(columns, hash_ctx, out);
      // A plain column's own validity is the whole story, and HashMultiColumn has
      // already folded it into the hash values via ToColumnArray's buffer.
      WriteOwnValidity(array, out_validity);

cpp/src/arrow/compute/kernels/scalar_hash.cc:291

A null FixedSizeListScalar reaches this path with a zero-length child span (ArraySpan::FillFromScalar), but rel_end - rel_start is still list_size. HashChild therefore widens that span and the primitive hash reads past its zero-length buffers (for example, hash32(MakeNullScalar(fixed_size_list(int32(), 8)))), causing undefined behavior/ASAN failures; the same applies when such a null fixed-size-list is nested in another value. Avoid reading a missing child range for invalid rows, or materialize a list_size-sized child during scalar promotion.

    ARROW_ASSIGN_OR_RAISE(auto value_hashes,
                          HashChild(values, values.offset + rel_start,
                                    rel_end - rel_start, hash_ctx, exec_ctx));

Files reviewed: 20/20 changed files
Comments generated: 1
Review effort level: Lite

kszucs · 2026-09-14T12:07:14Z

+    if (array.GetBuffer(2) != nullptr) {
+      var_length_buffer = array.GetBuffer(2)->data();
+    }
+  } else if (is_large_binary_like(type_id)) {
+    metadata = KeyColumnMetadata(false, sizeof(uint64_t));
+    if (array.GetBuffer(2) != nullptr) {
+      var_length_buffer = array.GetBuffer(2)->data();


The null pointer is real but never dereferenced — no crash to fix. Tests added in 8ac2f3a.

ToColumnArray does leave var_length_buffer null, but not from an all-null/empty array — those always carry a non-null (possibly zero-size) values buffer, and an ArrayData built with a genuinely null one is rejected by ValidateFull as Value data buffer is null. The shape that really produces it is the invalid BinaryScalar/StringScalar you named: ArraySpan::FillFromScalar sets buffers[2].data only if (scalar.is_valid).

No values buffer means all-equal offsets, so every row is zero-length and both implementations reach zero rows before reading a byte — num_rows_safe and num_rows_to_process each run to 0, and the scalar tail's ProcessFullStripes iterates istripe < num_stripes - 1, zero times for the num_stripes == 1 that a zero-length row yields (the memcpy is guarded on key_length > 0, and ProcessLastStripe reads the stack-local last_stripe_copy under an all-zero mask, not key). The pointer only ever appears as concatenated_keys + 0, which is well-defined.

NullValuesBufferHashesWithoutCrashing covers all four binary-like types and both kernels: null scalars, all-null/empty/empty-string arrays, and the same shapes as list and struct children. ScalarInput only covered int32, so this path had no coverage at all; my machine is arm64, so the new cases also buy AVX2 coverage on CI.

ToColumnArray leaves var_length_buffer null when a binary-like input carries no values buffer, and that pointer reaches Hashing{32,64}::HashVarLen. A null BinaryScalar/StringScalar is the shape that really produces it, since ArraySpan::FillFromScalar leaves buffers[2].data unset when the scalar is invalid. Every row of such a column is necessarily zero-length, so no byte is ever read -- pin that down rather than leave it to chance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

A null list-like scalar may carry no child array at all: BaseListScalar and FixedSizeListScalar guard their invariant checks on `if (this->value)`, and ArraySpan::FillFromScalar stands in a zero-length child for one. FIXED_SIZE_LIST takes its element range from list_size rather than from offsets, so it asked HashChild for list_size elements of a child that has none, reading past the 16-byte static FillZeroLengthArray points those buffers at -- a segfault for a wide enough list or a var-length value type. Clamp the range to what the child actually holds, in the fold as well as in the HashChild call. Neither clamp binds for a real array, whose child always spans list_size * (offset + length). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

kszucs · 2026-09-14T12:57:30Z

Picking up the two suppressed comments from the last review — the second is a real bug, fixed in 7b1d556.

fixed_size_binary null scalar — not reachable. MakeNullScalar(fixed_size_binary(4)) carries a non-null value, and a null-valued one cannot be constructed at all: FixedSizeBinaryScalar's constructor dereferences this->value->size() unguarded, so it segfaults there, before any kernel runs.

fixed_size_list null scalar — real, though not via the suggested repro (MakeNullScalar(fixed_size_list(int32(), 8)) also carries a properly sized child). FixedSizeListScalar(nullptr, type, /*is_valid=*/false) is legal — it and BaseListScalar both guard their invariant checks on if (this->value), and FillFromScalar's comment reads "when the scalar is null, scalar.value can also be null". FillFromScalar then substitutes a zero-length child via FillZeroLengthArray, whose buffers point at a static 16-byte kZeros with size == 0, while HashListArray still derives its range from list_size. Instrumented, that asked for 8 elems of child length 0 (child buf1 size=0); the new test segfaults without the fix.

Fixed by clamping the range to what the child actually holds, in the fold as well as in the HashChild call. To confirm the clamp can't bite on real data I instrumented it to report every time it changes the range and ran the full suite: it fires only on child.length == 0, never once on a real array across all 66 suites (Acero included), all green.

NullFixedSizeListScalarWithoutChild covers int32/int64/utf8/fixed_size_binary(3) children at list sizes 0/1/8/33, a nested-in-struct case, and the sibling list/large_list/map/list-of-fsl scalars — those take their range from offsets, which a null scalar fills with {0, 0}, so they were already safe.

Findings from a full review pass over the PR: - Reject a dictionary whose value type is nested. HashArray decodes a dictionary with Cast, which has no kernel producing a nested type, so dictionary<values=list<int32>> passed dispatch and then surfaced a cast_list error. Both statuses are NotImplemented, so the test asserts the message: dispatch rejects it rather than leaking a cast internal. - Name the FunctionDoc argument `values`, as 43 other kernels do, rather than `hash_input`. It becomes the Python keyword and is frozen once released. - Cover the public Hash32/Hash64 wrappers, which nothing called or tested, so a typo in either function name would have shipped unnoticed. - Link arrow-compute-key-hash-benchmark against arrow_compute_dep under Meson, guarded by needs_compute. HashMultiColumn and ColumnArrayFromArrayData live only in libarrow_compute, which arrow_benchmark_dep does not provide. - Document hash32/hash64 in the Python API reference, where they were absent. - Use the file's sentence-case heading convention and carry the "not suitable for cryptographic purposes" caveat from the FunctionDoc into compute.rst. - Drop a no-op refactor left in strategies.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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