ViderStar commented on code in PR #55253:
URL: https://github.com/apache/arrow/pull/55253#discussion_r4374927396
##########
cpp/src/arrow/compute/kernels/vector_hash.cc:
##########
@@ -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<Array> out_dict;
- RETURN_NOT_OK(dictionary_unifier_->GetResult(&out_dict_type, &out_dict));
+ RETURN_NOT_OK(dictionary_unifier_->GetResultWithIndexType(index_type_,
&out_dict));
Review Comment:
Yep, the helper compares the dictionary length rather than the highest
index, so exactly 128 entries were wrongly rejected for int8. Fixed in bfc4ac7:
the kernel now uses `GetResult()` again and checks that `length - 1 `fits in
the input index type. The test now covers the exact boundary (64 + 64 = 128
entries succeed for both unique and value_counts, 129 return Invalid).
Side note: `GetResultWithIndexType` itself has the same off-by-one, and it’s
also used by Concatenate and `UnifyChunkedArray`. I left it out to keep this PR
focused - happy to fix it here or in a follow-up, whichever you prefer =)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
