Skip to content

Commit 6053fa0

Browse files
committed
Add test coverage for zero/null collision across byte widths and FIXED_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.
1 parent 07d0973 commit 6053fa0

1 file changed

Lines changed: 81 additions & 0 deletions

File tree

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

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,48 @@ TEST_F(TestScalarHash, NullHashIsZero) {
256256
ASSERT_NE(buf2[1], buf2[2]);
257257
}
258258

259+
// HashIntImp (used for any fixed-width type whose byte width is a power of 2 up to 8:
260+
// ints, floats, dates, times, timestamps, durations) doesn't special-case an
261+
// all-zero-bits key, so a legitimately valid "zero" value would otherwise hash to the
262+
// same 0 scalar_hash.cc uses as the null sentinel. Checked across every affected byte
263+
// width, not just int8/int32 (see NullHashIsZero).
264+
TEST_F(TestScalarHash, ZeroValueDoesNotCollideWithNull) {
265+
std::vector<std::pair<std::shared_ptr<DataType>, std::string>> cases{
266+
{int8(), R"([null, 0, 1])"},
267+
{int16(), R"([null, 0, 1])"},
268+
{int32(), R"([null, 0, 1])"},
269+
{int64(), R"([null, 0, 1])"},
270+
{uint8(), R"([null, 0, 1])"},
271+
{uint16(), R"([null, 0, 1])"},
272+
{uint32(), R"([null, 0, 1])"},
273+
{uint64(), R"([null, 0, 1])"},
274+
{float32(), R"([null, 0.0, 1.0])"},
275+
{float64(), R"([null, 0.0, 1.0])"},
276+
{date32(), R"([null, 0, 1])"},
277+
{date64(), R"([null, 0, 86400000])"},
278+
{time32(TimeUnit::SECOND), R"([null, 0, 1])"},
279+
{time64(TimeUnit::NANO), R"([null, 0, 1])"},
280+
{timestamp(TimeUnit::SECOND), R"([null, 0, 1])"},
281+
{duration(TimeUnit::MILLI), R"([null, 0, 1])"},
282+
};
283+
for (const std::string func : {"hash32", "hash64"}) {
284+
auto zero = func == "hash32" ? MakeScalar(uint32_t{0}) : MakeScalar(uint64_t{0});
285+
for (const auto& type_and_json : cases) {
286+
auto arr = ArrayFromJSON(type_and_json.first, type_and_json.second);
287+
ASSERT_OK_AND_ASSIGN(Datum result, CallFunction(func, {arr}));
288+
auto hashes = result.make_array();
289+
ASSERT_OK_AND_ASSIGN(auto null_hash, hashes->GetScalar(0));
290+
ASSERT_OK_AND_ASSIGN(auto zero_hash, hashes->GetScalar(1));
291+
ASSERT_OK_AND_ASSIGN(auto one_hash, hashes->GetScalar(2));
292+
ASSERT_TRUE(null_hash->Equals(*zero)) << type_and_json.first->ToString();
293+
ASSERT_FALSE(zero_hash->Equals(*zero))
294+
<< "valid zero-valued " << type_and_json.first->ToString()
295+
<< " should not collide with the null sentinel";
296+
ASSERT_FALSE(zero_hash->Equals(*one_hash)) << type_and_json.first->ToString();
297+
}
298+
}
299+
}
300+
259301
TEST_F(TestScalarHash, Boolean) {
260302
Datum result;
261303
std::shared_ptr<Array> array;
@@ -562,6 +604,45 @@ TEST_F(TestScalarHash, ListLikeSliceOfLargerArrayMatchesIndependentArray) {
562604
}
563605
}
564606

607+
// Same as ListLikeSliceOfLargerArrayMatchesIndependentArray, but for FIXED_SIZE_LIST,
608+
// which computes its referenced range via arithmetic (offset * list_size) rather than
609+
// reading an offsets buffer, so it's a genuinely different code path worth covering
610+
// on its own.
611+
TEST_F(TestScalarHash, FixedSizeListSliceOfLargerArrayMatchesIndependentArray) {
612+
constexpr int64_t kTotalRows = 1000;
613+
constexpr int64_t kSliceOffset = 137;
614+
constexpr int64_t kSliceLength = 10;
615+
constexpr int32_t kListSize = 2;
616+
617+
FixedSizeListBuilder list_builder(default_memory_pool(),
618+
std::make_shared<Int32Builder>(), kListSize);
619+
auto* values = checked_cast<Int32Builder*>(list_builder.value_builder());
620+
for (int64_t row = 0; row < kTotalRows; row++) {
621+
ASSERT_OK(list_builder.Append());
622+
ASSERT_OK(values->Append(static_cast<int32_t>(row)));
623+
ASSERT_OK(values->Append(static_cast<int32_t>(row + 1)));
624+
}
625+
ASSERT_OK_AND_ASSIGN(auto large_arr, list_builder.Finish());
626+
auto sliced = large_arr->Slice(kSliceOffset, kSliceLength);
627+
628+
FixedSizeListBuilder independent_builder(default_memory_pool(),
629+
std::make_shared<Int32Builder>(), kListSize);
630+
auto* independent_values =
631+
checked_cast<Int32Builder*>(independent_builder.value_builder());
632+
for (int64_t row = kSliceOffset; row < kSliceOffset + kSliceLength; row++) {
633+
ASSERT_OK(independent_builder.Append());
634+
ASSERT_OK(independent_values->Append(static_cast<int32_t>(row)));
635+
ASSERT_OK(independent_values->Append(static_cast<int32_t>(row + 1)));
636+
}
637+
ASSERT_OK_AND_ASSIGN(auto independent_arr, independent_builder.Finish());
638+
639+
for (const std::string func : {"hash32", "hash64"}) {
640+
ASSERT_OK_AND_ASSIGN(Datum sliced_result, CallFunction(func, {sliced}));
641+
ASSERT_OK_AND_ASSIGN(Datum independent_result, CallFunction(func, {independent_arr}));
642+
AssertDatumsEqual(sliced_result, independent_result);
643+
}
644+
}
645+
565646
void CheckRowsHashDifferently(const std::string& func, const std::shared_ptr<Array>& arr,
566647
int64_t row_a, int64_t row_b) {
567648
ASSERT_OK_AND_ASSIGN(Datum result, CallFunction(func, {arr}));

0 commit comments

Comments
 (0)