From 67f716b2ad9c694e52157d5a7c5561cb5288cf3b Mon Sep 17 00:00:00 2001 From: Artsem Lebiadzevich Date: Sun, 25 Oct 2026 22:13:39 +0300 Subject: [PATCH 1/2] GH-45010: [C++] Error out in unique/value_counts when unified dictionary overflows the index type --- cpp/src/arrow/compute/kernels/vector_hash.cc | 13 +++++---- .../arrow/compute/kernels/vector_hash_test.cc | 29 +++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_hash.cc b/cpp/src/arrow/compute/kernels/vector_hash.cc index 9a29dcf333f9..9db2a9efdd2a 100644 --- a/cpp/src/arrow/compute/kernels/vector_hash.cc +++ b/cpp/src/arrow/compute/kernels/vector_hash.cc @@ -445,9 +445,11 @@ class NullHashKernel : public HashKernel { class DictionaryHashKernel : public HashKernel { public: - explicit DictionaryHashKernel(std::unique_ptr indices_kernel, - std::shared_ptr dictionary_value_type) + DictionaryHashKernel(std::unique_ptr indices_kernel, + std::shared_ptr index_type, + std::shared_ptr dictionary_value_type) : indices_kernel_(std::move(indices_kernel)), + index_type_(std::move(index_type)), dictionary_value_type_(std::move(dictionary_value_type)) {} Status Reset() override { return indices_kernel_->Reset(); } @@ -513,15 +515,16 @@ class DictionaryHashKernel : public HashKernel { return first_dictionary_; } - auto out_dict_type = first_dictionary_->type(); + // The output keeps the input index type, so the unified dictionary must fit in it std::shared_ptr out_dict; - RETURN_NOT_OK(dictionary_unifier_->GetResult(&out_dict_type, &out_dict)); + RETURN_NOT_OK(dictionary_unifier_->GetResultWithIndexType(index_type_, &out_dict)); return out_dict; } private: std::unique_ptr indices_kernel_; std::shared_ptr first_dictionary_; + std::shared_ptr index_type_; std::shared_ptr dictionary_value_type_; std::unique_ptr dictionary_unifier_; }; @@ -597,7 +600,7 @@ Result> DictionaryHashInit(KernelContext* ctx, ARROW_ASSIGN_OR_RAISE(auto indices_hasher, GetHashInit(dict_type.index_type()->id())(ctx, args)); return std::make_unique( - checked_pointer_cast(std::move(indices_hasher)), + checked_pointer_cast(std::move(indices_hasher)), dict_type.index_type(), dict_type.value_type()); } diff --git a/cpp/src/arrow/compute/kernels/vector_hash_test.cc b/cpp/src/arrow/compute/kernels/vector_hash_test.cc index b11e0a722a57..6ab0bb95c6a3 100644 --- a/cpp/src/arrow/compute/kernels/vector_hash_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_hash_test.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -764,6 +765,34 @@ TEST_F(TestHashKernel, DictionaryUniqueAndValueCounts) { } } +TEST_F(TestHashKernel, DictionaryUniqueAndValueCountsIndexOverflow) { + // GH-45010: unifying the chunk dictionaries can produce more entries than the + // index type can address + auto dict_ty = dictionary(int8(), int64()); + auto make_chunk = [&](int64_t first_value, int64_t dict_length) { + std::vector values(dict_length); + std::iota(values.begin(), values.end(), first_value); + std::shared_ptr dict; + ArrayFromVector(values, &dict); + auto indices = ArrayFromJSON(int8(), "[" + std::to_string(dict_length - 1) + "]"); + return std::make_shared(dict_ty, indices, dict); + }; + + // 64 + 63 = 127 unified entries still fit in int8 + auto fits = *ChunkedArray::Make({make_chunk(0, 64), make_chunk(100, 63)}, dict_ty); + ASSERT_OK_AND_ASSIGN(auto uniques, Unique(fits)); + ASSERT_OK(uniques->ValidateFull()); + ASSERT_OK_AND_ASSIGN(auto counts, ValueCounts(fits)); + ASSERT_OK(counts->ValidateFull()); + + // 64 + 65 = 129 unified entries do not + auto overflows = *ChunkedArray::Make({make_chunk(0, 64), make_chunk(100, 65)}, dict_ty); + EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("larger index type"), + Unique(overflows)); + EXPECT_RAISES_WITH_MESSAGE_THAT(Invalid, ::testing::HasSubstr("larger index type"), + ValueCounts(overflows)); +} + /* TODO(ARROW-4124): Determine if we want to do something that is reproducible with * floats. TEST_F(TestHashKernel, ValueCountsFloat) { From bfc4ac71177ca8217799b9a66f25a9d38d96d7ca Mon Sep 17 00:00:00 2001 From: Artsem Lebiadzevich Date: Mon, 26 Oct 2026 10:15:49 +0300 Subject: [PATCH 2/2] Check the highest index, not the dictionary length, against the index type A unified dictionary of 128 entries is addressable by int8 indices (0..127), but DictionaryUnifier::GetResultWithIndexType rejects it because it compares the dictionary length with the index type maximum. Check length - 1 instead and test the exact 128/129 boundary for int8. --- cpp/src/arrow/compute/kernels/vector_hash.cc | 13 +++++++++++-- cpp/src/arrow/compute/kernels/vector_hash_test.cc | 5 +++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/vector_hash.cc b/cpp/src/arrow/compute/kernels/vector_hash.cc index 9db2a9efdd2a..57061cf413ff 100644 --- a/cpp/src/arrow/compute/kernels/vector_hash.cc +++ b/cpp/src/arrow/compute/kernels/vector_hash.cc @@ -515,9 +515,18 @@ class DictionaryHashKernel : public HashKernel { return first_dictionary_; } - // The output keeps the input index type, so the unified dictionary must fit in it + std::shared_ptr out_dict_type; std::shared_ptr out_dict; - RETURN_NOT_OK(dictionary_unifier_->GetResultWithIndexType(index_type_, &out_dict)); + RETURN_NOT_OK(dictionary_unifier_->GetResult(&out_dict_type, &out_dict)); + // The output keeps the input index type, so the highest index into the unified + // dictionary (its length minus one) must fit in it + const Int64Scalar max_index(out_dict->length() - 1); + if (out_dict->length() > 0 && + !::arrow::internal::IntegersCanFit(max_index, *index_type_).ok()) { + return Status::Invalid( + "These dictionaries cannot be combined. The unified dictionary requires a " + "larger index type."); + } return out_dict; } diff --git a/cpp/src/arrow/compute/kernels/vector_hash_test.cc b/cpp/src/arrow/compute/kernels/vector_hash_test.cc index 6ab0bb95c6a3..8bd907679f4d 100644 --- a/cpp/src/arrow/compute/kernels/vector_hash_test.cc +++ b/cpp/src/arrow/compute/kernels/vector_hash_test.cc @@ -778,10 +778,11 @@ TEST_F(TestHashKernel, DictionaryUniqueAndValueCountsIndexOverflow) { return std::make_shared(dict_ty, indices, dict); }; - // 64 + 63 = 127 unified entries still fit in int8 - auto fits = *ChunkedArray::Make({make_chunk(0, 64), make_chunk(100, 63)}, dict_ty); + // 64 + 64 = 128 unified entries still fit in int8: the highest index is 127 + auto fits = *ChunkedArray::Make({make_chunk(0, 64), make_chunk(100, 64)}, dict_ty); ASSERT_OK_AND_ASSIGN(auto uniques, Unique(fits)); ASSERT_OK(uniques->ValidateFull()); + ASSERT_EQ(checked_cast(*uniques).dictionary()->length(), 128); ASSERT_OK_AND_ASSIGN(auto counts, ValueCounts(fits)); ASSERT_OK(counts->ValidateFull());