Skip to content

GH-17211: [C++] Add hash32 and hash64 scalar compute functions#45001

Open
kszucs wants to merge 69 commits into
apache:mainfrom
kszucs:scalar-hash
Open

GH-17211: [C++] Add hash32 and hash64 scalar compute functions#45001
kszucs wants to merge 69 commits into
apache:mainfrom
kszucs:scalar-hash

Conversation

@kszucs

@kszucs kszucs commented Dec 11, 2024

Copy link
Copy Markdown
Member

Rationale for this change

Support for calculating elementwise hashes.

The PR adds to 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

Are these changes tested?

Partially, working on a proper testing suite.

Are there any user-facing changes?

There are two new compute kernels, hash32 and hash64, available.

Comment thread cpp/src/arrow/compute/light_array_internal.h Outdated
Comment thread cpp/src/arrow/compute/light_array_internal.h Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Dec 11, 2024
Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc Outdated
@kszucs

kszucs commented Dec 11, 2024

Copy link
Copy Markdown
Member Author

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
]

Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc Outdated
@github-actions github-actions Bot added Component: Python awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 11, 2024
Comment thread python/pyarrow/tests/test_compute.py Outdated
@kszucs
kszucs marked this pull request as ready for review December 11, 2024 17:41
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 11, 2024
@kszucs kszucs changed the title GH-17211: [C++] Add hash_64 scalar compute function GH-17211: [C++] Add hash_64 scalar compute function Dec 11, 2024
@github-actions github-actions Bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Dec 11, 2024

@zanmato1984 zanmato1984 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread cpp/src/arrow/compute/api_scalar.h Outdated
Comment thread cpp/src/arrow/compute/kernels/CMakeLists.txt Outdated
Comment thread cpp/src/arrow/compute/kernels/CMakeLists.txt Outdated
Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc Outdated
Comment thread docs/source/cpp/compute.rst Outdated
Comment thread cpp/src/arrow/compute/api_scalar.cc Outdated
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 13, 2024
@kszucs kszucs changed the title GH-17211: [C++] Add hash_64 scalar compute function GH-17211: [C++] Add hash32 and hash64 scalar compute functions Dec 13, 2024
@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 13, 2024
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Comment thread r/tests/testthat/test-compute-aggregate.R
Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • In the STRUCT branch, nested children are hashed via HashChild(child), which produces a hash buffer aligned to the sliced child span, but the result is then sliced again with column.Slice(array.offset, array.length). Because ArraySpan::SetSlice (used by ExecSpan iteration) does not propagate to child_data, child often has full length and offset 0 even when the parent struct is sliced. This combination can (a) hash the wrong range, and (b) apply the parent offset twice for nested children, leading to out-of-bounds reads / incorrect hashes for sliced struct arrays with nested fields.
        columns[i] = column.Slice(array.offset, array.length);

kszucs added 2 commits July 24, 2026 10:53
… 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

python/pyarrow/tests/strategies.py:370

  • arrays() now rejects timezone-naive timestamp types via h.assume(ty.tz is not None), which both reduces coverage (no naive timestamps can be generated) and increases Hypothesis rejection rate since timestamp_types explicitly includes tz=None. Consider generating naive datetimes when ty.tz is None instead of assuming it away.
    elif pa.types.is_timestamp(ty):
        if zoneinfo is None:
            pytest.skip('no module named zoneinfo (or tzdata on Windows)')
        h.assume(ty.tz is not None)
        min_int64 = -(2**63)
        max_int64 = 2**63 - 1
        min_datetime = datetime.datetime.fromtimestamp(
            min_int64 // 10**9) + datetime.timedelta(hours=12)
        max_datetime = datetime.datetime.fromtimestamp(
            max_int64 // 10**9) - datetime.timedelta(hours=12)
        try:
            offset_hours, offset_min = ty.tz.split(":")
            sign = -1 if offset_hours.startswith("-") else 1
            offset = datetime.timedelta(
                hours=abs(int(offset_hours)), minutes=int(offset_min))
            tz = datetime.timezone(sign * offset)
        except ValueError:
            tz = zoneinfo.ZoneInfo(ty.tz)
        value = st.datetimes(timezones=st.just(tz), min_value=min_datetime,
                             max_value=max_datetime)

Comment thread cpp/src/arrow/compute/key_hash_internal.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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

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

  • HashChild() builds an ArrayData for the hashed child with offset=0 but reuses the input child's validity buffer (sliced.GetBuffer(0)). If the child slice offset is non-zero, the bitmap bits are still aligned to the original offset, so downstream consumers (notably HashStructArray -> ToColumnArray -> HashMultiColumn) will see nulls/validity shifted and can produce incorrect hashes (e.g. for sliced struct fields of nested types where null propagation is expected to yield the 0 sentinel). Consider materializing a new validity bitmap for the sliced range (CopyBitmap/AllocateEmptyBitmap) or otherwise preserving the bit offset when constructing the hashed child view.
  // Hashes the [offset, offset + length) slice of `child`.
  static Result<std::shared_ptr<ArrayData>> HashChild(const ArraySpan& child,
                                                      int64_t offset, int64_t length,
                                                      LightContext* hash_ctx,
                                                      MemoryPool* memory_pool) {
    auto sliced = child;
    sliced.SetSlice(offset, length);
    auto arrow_type = TypeTraits<ArrowType>::type_singleton();
    auto buffer_size = sliced.length * sizeof(c_type);
    ARROW_ASSIGN_OR_RAISE(auto buffer, AllocateBuffer(buffer_size, memory_pool));
    ARROW_RETURN_NOT_OK(
        HashArray(sliced, hash_ctx, memory_pool, buffer->mutable_data_as<c_type>()));
    return ArrayData::Make(arrow_type, sliced.length,
                           {sliced.GetBuffer(0), std::move(buffer)}, sliced.null_count);
  }

Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc
…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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

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

  • ToColumnArray doesn’t handle DICTIONARY (and will also fail for EXTENSION when used inside STRUCT via HashStructArray’s non-nested branch). This makes hash32/hash64 error at runtime for dictionary arrays (and potentially for structs containing extension fields), even though the kernels are otherwise intended to accept these inputs.

Consider mirroring ColumnMetadataFromDataType (cpp/src/arrow/compute/light_array_internal.cc:117-149) by unwrapping extension types and treating dictionary as fixed-width indices.

  auto type = array.type;
  auto type_id = type->id();
  if (type_id == Type::NA) {
    metadata = KeyColumnMetadata(true, 0, true);
  } else if (type_id == Type::BOOL) {

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

  • HashChild returns an ArrayData whose validity buffer is sliced.GetBuffer(0) but whose offset is implicitly 0. If the input child has a non-zero offset (e.g. a struct field that is itself a slice), the returned validity bitmap won’t be aligned with the newly-produced hash values, and subsequent ToColumnArray(...).Slice(...) calls can treat the wrong rows as null/valid.

This is especially relevant in HashStructArray’s nested-child path, where HashChild(child, child.offset, child.length, ...) is used specifically to handle independently-sliced fields, but the validity alignment is lost in the constructed ArrayData.

    return ArrayData::Make(arrow_type, sliced.length,
                           {sliced.GetBuffer(0), std::move(buffer)}, sliced.null_count);
  }

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comment on lines +127 to +141
// Hashes the [offset, offset + length) slice of `child`.
static Result<std::shared_ptr<ArrayData>> HashChild(const ArraySpan& child,
int64_t offset, int64_t length,
LightContext* hash_ctx,
MemoryPool* memory_pool) {
auto sliced = child;
sliced.SetSlice(offset, length);
auto arrow_type = TypeTraits<ArrowType>::type_singleton();
auto buffer_size = sliced.length * sizeof(c_type);
ARROW_ASSIGN_OR_RAISE(auto buffer, AllocateBuffer(buffer_size, memory_pool));
ARROW_RETURN_NOT_OK(
HashArray(sliced, hash_ctx, memory_pool, buffer->mutable_data_as<c_type>()));
return ArrayData::Make(arrow_type, sliced.length,
{sliced.GetBuffer(0), std::move(buffer)}, sliced.null_count);
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed with a repro — a struct wrapping a list field sliced to offset=3 hashed a valid row as 0 (colliding with null) instead of matching an equivalent independently-built struct. Fixed by repacking the validity bitmap into a fresh, 0-based buffer in HashChild, so it's self-consistent with the offset-0 ArrayData it returns (and with the already-0-based hash values buffer built alongside it), rather than reusing the child's raw buffer which needed child.offset + i to read correctly. Added NestedFieldWithOwnOffsetHashesCorrectly as a regression test.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified further — turns out no validity buffer is needed at all here (not even a view): the recursive HashArray call above already zeroes out[i] for every genuinely-null row (via ZeroNulls/the valid-0 remap, both using sliced's own correct offset), so null-ness is already fully encoded in the hash values themselves. Passing nullptr for validity is both simpler and cheaper than copying — verified with the same repro plus the full test suite.

kszucs added 2 commits July 24, 2026 13:13
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

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

  • ToColumnArray treats all fixed-width types the same, which includes Type::DICTIONARY (it’s a FixedWidthType). This means hash32/hash64 hash dictionary indices rather than the dictionary values, so semantically equal values can hash differently across different dictionary encodings (and especially across ChunkedArray chunks with different dictionaries). If hash32/hash64 are intended as value hashes, consider special-casing dictionaries by hashing the dictionary values once and then gathering by indices (or otherwise unifying/transposing dictionaries).
    metadata = KeyColumnMetadata(true, type->bit_width() / 8);
  } else if (is_binary_like(type_id)) {
    metadata = KeyColumnMetadata(false, sizeof(uint32_t));

Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc Outdated
Comment thread cpp/src/arrow/compute/api_scalar.h Outdated
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

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

  • ToColumnArray() rejects Type::DICTIONARY, but the rest of this PR (C++ tests + Python strategies) treats dictionary arrays as supported inputs for hash32/hash64. As-is, hashing a dictionary array will fail at runtime with a TypeError from here.

Add an explicit DICTIONARY case (treating it as a fixed-width indices buffer, consistent with ColumnMetadataFromDataType() in compute/light_array_internal.cc).

  auto type = array.type;
  auto type_id = type->id();
  if (type_id == Type::NA) {
    metadata = KeyColumnMetadata(true, 0, true);
  } else if (type_id == Type::BOOL) {
    metadata = KeyColumnMetadata(true, 0);
  } else if (is_fixed_width(type_id)) {
    metadata = KeyColumnMetadata(true, type->bit_width() / 8);
  } else if (is_binary_like(type_id)) {
    metadata = KeyColumnMetadata(false, sizeof(uint32_t));
    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();
    }
  } else {
    return Status::TypeError("Unsupported column data type ", type->name(),
                             " used with hash32/hash64 compute kernel");
  }

cpp/src/arrow/compute/api_scalar.h:1817

  • The API docs refer to a "NestedArray" type, but Arrow doesn't have a concrete type by that name (and it’s not used elsewhere in the C++ API). Consider rewording to "nested types" (e.g. struct/list/map) to avoid confusing users.
/// The result is an Array of length equal to the length of the input; however, the output
/// shall be a UInt32Array, with each element being a hash constructed from each row of
/// the input. If the input Array is a NestedArray, this means that each "attribute" or
/// "field" of the input NestedArray corresponding to the same "row" will collectively
/// produce a single uint32_t hash. At the moment, this function does not take options,

Comment thread cpp/src/arrow/compute/kernels/scalar_hash_benchmark.cc

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

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

  • HashStructArray currently hashes nested child arrays over the entire child length (HashChild(child, child.offset, child.length)) even when the struct itself is a small slice. Since ArrayData::Slice() doesn’t slice child_data, hashing a small slice of a large struct can become O(full child length) for nested fields. Hash only the slice range used by the struct (child.offset + array.offset, length = array.length) and then slice from 0.
      int64_t slice_offset = array.offset;
      if (is_nested(child.type->id())) {
        ARROW_ASSIGN_OR_RAISE(
            child_hashes[i],
            HashChild(child, child.offset, child.length, hash_ctx, memory_pool));

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

  • HashableMatcher::Matches only checks the top-level (after unwrapping EXTENSION) but the comment says this handles nesting. As written, types like list<binary_view> / struct<run_end_encoded> will pass dispatch and then fail later in HashArray/ToColumnArray with a TypeError instead of a clean NotImplemented from dispatch. Consider recursively rejecting unsupported types anywhere in the physical type tree (unwrapping EXTENSION at each node).
  bool Matches(const DataType& type) const override {
    // Unwrap extension types (recursively, in case of nesting) so a union/view/REE
    // storage type is rejected here too, rather than passing dispatch and failing
    // later with a raw TypeError from HashArray/ToColumnArray.
    const DataType* physical_type = &type;

Comment thread cpp/src/arrow/compute/key_hash_benchmark.cc
kszucs added 3 commits July 24, 2026 13:59
…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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • HashableMatcher only rejects unsupported types at the top level (and only unwraps EXTENSION wrappers). Nested unsupported types (e.g. struct<binary_view>, list<run_end_encoded<…>>, map<utf8, dense_union<…>>) will still dispatch, then fail later with a TypeError from HashArray/ToColumnArray instead of a consistent NotImplemented error. This is especially confusing given the docs explicitly call out union/view/run-end-encoded types as unsupported.
    return !(is_union(*physical_type) || is_binary_view_like(*physical_type) ||
             is_list_view(*physical_type) ||
             physical_type->id() == Type::RUN_END_ENCODED);

…hashing

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

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

  • HashListArray zeroes null rows, but it doesn’t remap non-null rows whose folded hash happens to be 0. Because the output has no validity bitmap, a valid list/map row hashing to 0 would be indistinguishable from a null row. Consider applying the same valid-0 remap used in HashArray’s non-nested path after ZeroNulls().
    // Required: a null row's offset range isn't guaranteed empty, and even an empty
    // one would collide with CombineRange's seed for a real empty list.
    ZeroNulls(array, out);
    return Status::OK();

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

  • HashableMatcher only checks the top-level (after unwrapping EXTENSION). Nested unsupported types (e.g. list<binary_view>, struct<run_end_encoded<…>>) can still pass dispatch and then fail later in HashArray/ToColumnArray with a TypeError instead of a clean NotImplemented. Consider making Matches() recurse into child fields (and unwrap extension storage types) so dispatch reliably rejects unsupported types anywhere in the type tree.
    return !(is_union(*physical_type) || is_binary_view_like(*physical_type) ||
             is_list_view(*physical_type) ||
             physical_type->id() == Type::RUN_END_ENCODED);
  }

Comment thread cpp/src/arrow/compute/kernels/scalar_hash.cc
…he null hash sentinel

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • HashableMatcher only checks the top-level physical type (after unwrapping EXTENSION). If a supported top-level type contains an unsupported nested type (e.g. list<utf8_view> or struct with a binary_view field), dispatch will succeed and the kernel will later fail with a TypeError from HashArray/ToColumnArray instead of cleanly reporting NotImplemented. Since the docs and tests treat view/union/REE as unsupported, it’s better to reject any type that contains these anywhere in its nesting so error reporting is consistent.
  bool Matches(const DataType& type) const override {
    // Unwrap extension types (recursively, in case of nesting) so a union/view/REE
    // storage type is rejected here too, rather than passing dispatch and failing
    // later with a raw TypeError from HashArray/ToColumnArray.
    const DataType* physical_type = &type;

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

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

  • HashableMatcher only checks the top-level (physical) type. If an input contains an unsupported type nested inside a supported container (e.g. struct<binary_view> or list<run_end_encoded<...>>), dispatch will still select this kernel and then fail later with a TypeError from HashArray/ToColumnArray rather than a clean NotImplemented at dispatch time. Consider recursively validating child/value/field types here so unsupported nested types are rejected consistently.
  bool Matches(const DataType& type) const override {
    // Unwrap extension types (recursively, in case of nesting) so a union/view/REE
    // storage type is rejected here too, rather than passing dispatch and failing
    // later with a raw TypeError from HashArray/ToColumnArray.
    const DataType* physical_type = &type;
    while (physical_type->id() == Type::EXTENSION) {
      physical_type =
          checked_cast<const ExtensionType&>(*physical_type).storage_type().get();
    }
    return !(is_union(*physical_type) || is_binary_view_like(*physical_type) ||
             is_list_view(*physical_type) ||
             physical_type->id() == Type::RUN_END_ENCODED);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants