Skip to content

Commit ab3995a

Browse files
committed
Fix HashChild reusing a nested field's unshifted validity buffer
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.
1 parent 6053fa0 commit ab3995a

2 files changed

Lines changed: 60 additions & 1 deletion

File tree

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "arrow/compute/registry_internal.h"
2727
#include "arrow/compute/util.h"
2828
#include "arrow/result.h"
29+
#include "arrow/util/bitmap_ops.h"
2930

3031
namespace arrow {
3132
namespace compute {
@@ -136,8 +137,21 @@ struct FastHashScalar {
136137
ARROW_ASSIGN_OR_RAISE(auto buffer, AllocateBuffer(buffer_size, memory_pool));
137138
ARROW_RETURN_NOT_OK(
138139
HashArray(sliced, hash_ctx, memory_pool, buffer->mutable_data_as<c_type>()));
140+
141+
std::shared_ptr<Buffer> validity;
142+
if (sliced.GetBuffer(0) != nullptr) {
143+
// sliced.GetBuffer(0) is unshifted: reading logical row i requires bit
144+
// `sliced.offset + i`. But the returned ArrayData has offset 0, and callers
145+
// build a KeyColumnArray directly from its buffers without applying that
146+
// offset themselves (see ToColumnArray, which always treats bit/byte 0 of a
147+
// buffer as row 0). Repack into a fresh, 0-based bitmap so it's self-consistent
148+
// with the hash values buffer, which is already 0-based.
149+
ARROW_ASSIGN_OR_RAISE(validity, ::arrow::internal::CopyBitmap(
150+
memory_pool, sliced.GetBuffer(0)->data(),
151+
sliced.offset, sliced.length));
152+
}
139153
return ArrayData::Make(arrow_type, sliced.length,
140-
{sliced.GetBuffer(0), std::move(buffer)}, sliced.null_count);
154+
{std::move(validity), std::move(buffer)}, sliced.null_count);
141155
}
142156

143157
static Status HashStructArray(const ArraySpan& array, LightContext* hash_ctx,

cpp/src/arrow/compute/kernels/scalar_hash_test.cc

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -804,6 +804,51 @@ TEST_F(TestScalarHash, NestedNullFieldWithinValidStructHashesToZero) {
804804
}
805805
}
806806

807+
// Guards against HashChild reusing a nested field's raw (unshifted) validity buffer
808+
// without rebasing it: the buffer requires bit `child.offset + i` to read logical row
809+
// i, but the returned ArrayData has offset 0 and its buffer is read directly (bit 0 =
810+
// row 0) once wrapped in a KeyColumnArray. If a struct's nested field is itself an
811+
// offset slice of a larger array (e.g. GH-17211), this misreads validity by
812+
// `child.offset` bits -- here, a valid row would be misread as null (or vice versa)
813+
// unless the buffer is rebased to be self-consistent with the fresh hash values.
814+
TEST_F(TestScalarHash, NestedFieldWithOwnOffsetHashesCorrectly) {
815+
ListBuilder list_builder(default_memory_pool(), std::make_shared<Int32Builder>());
816+
auto* values = checked_cast<Int32Builder*>(list_builder.value_builder());
817+
ASSERT_OK(list_builder.AppendNull());
818+
for (int32_t row = 1; row < 10; row++) {
819+
ASSERT_OK(list_builder.Append());
820+
ASSERT_OK(values->Append(row));
821+
ASSERT_OK(values->Append(row + 1));
822+
}
823+
ASSERT_OK_AND_ASSIGN(auto base, list_builder.Finish());
824+
auto sliced_field = base->Slice(3, 5); // offset=3, length=5; logical row 0 = valid
825+
826+
ASSERT_OK_AND_ASSIGN(auto struct_with_offset_field,
827+
StructArray::Make({sliced_field}, {field("f0", list(int32()))}));
828+
829+
ListBuilder independent_builder(default_memory_pool(),
830+
std::make_shared<Int32Builder>());
831+
auto* independent_values =
832+
checked_cast<Int32Builder*>(independent_builder.value_builder());
833+
for (int32_t row = 3; row < 8; row++) {
834+
ASSERT_OK(independent_builder.Append());
835+
ASSERT_OK(independent_values->Append(row));
836+
ASSERT_OK(independent_values->Append(row + 1));
837+
}
838+
ASSERT_OK_AND_ASSIGN(auto independent_field, independent_builder.Finish());
839+
ASSERT_OK_AND_ASSIGN(
840+
auto independent_struct,
841+
StructArray::Make({independent_field}, {field("f0", list(int32()))}));
842+
843+
for (const std::string func : {"hash32", "hash64"}) {
844+
ASSERT_OK_AND_ASSIGN(Datum offset_result,
845+
CallFunction(func, {struct_with_offset_field}));
846+
ASSERT_OK_AND_ASSIGN(Datum independent_result,
847+
CallFunction(func, {independent_struct}));
848+
AssertDatumsEqual(offset_result, independent_result);
849+
}
850+
}
851+
807852
// The EXTENSION unwrapping at the top of HashArray should compose with the
808853
// is_list_like recursion; this combination was otherwise untested (ExtensionType
809854
// above only wraps a primitive).

0 commit comments

Comments
 (0)