Copilot commented on code in PR #51000:
URL: https://github.com/apache/arrow/pull/51000#discussion_r4120941573
##########
cpp/src/arrow/compute/kernels/scalar_validity.cc:
##########
@@ -80,10 +83,43 @@ static void SetNanBits(const ArraySpan& arr, uint8_t*
out_bitmap, int64_t out_of
}
}
+// Maps `is_null` over dictionary values and then through the indices, so
+// both NaN and null dictionary entries are reported, whatever the index type.
+static Status SetNanBitsFromDictionary(KernelContext* ctx, const ArraySpan&
arr,
+ uint8_t* out_bitmap, int64_t
out_offset) {
+ if (arr.length == 0) {
+ return Status::OK();
+ }
+ if (arr.GetNullCount() > 0) {
+ InvertBitmap(arr.buffers[0].data, arr.offset, arr.length, out_bitmap,
out_offset);
+ } else {
+ bit_util::SetBitsTo(out_bitmap, out_offset, arr.length, false);
+ }
+ NullOptions nan_is_null_options(/*nan_is_null=*/true);
+ ARROW_ASSIGN_OR_RAISE(Datum dict_is_null,
+ CallFunction("is_null",
{arr.dictionary().ToArrayData()},
+ &nan_is_null_options,
ctx->exec_context()));
+
+ const auto& dict_type = checked_cast<const DictionaryType&>(*arr.type);
+ auto indices = ArrayData::Make(dict_type.index_type(), arr.length,
+ {arr.GetBuffer(0), arr.GetBuffer(1)},
arr.GetNullCount(),
+ arr.offset);
+ ARROW_ASSIGN_OR_RAISE(Datum taken,
+ Take(dict_is_null, Datum(std::move(indices)),
+ TakeOptions::NoBoundsCheck(),
ctx->exec_context()));
Review Comment:
`Take` is called with `TakeOptions::NoBoundsCheck()`, which disables bounds
checking (see `TakeOptions` in `compute/api_vector.h`). That contradicts the
stated intent of erroring on out-of-range indices and could allow invalid
dictionary indices to read out of bounds; use bounds checking here.
##########
cpp/src/arrow/compute/kernels/scalar_validity_test.cc:
##########
@@ -208,6 +208,33 @@ TEST(TestValidityKernels, IsNullSetsZeroNullCount) {
ASSERT_EQ(out.array()->null_count, 0);
}
+TEST(TestValidityKernels, IsNullDictionaryNanIsNull) {
+ NullOptions default_options;
+ NullOptions nan_is_null_options(/*nan_is_null=*/true);
+
+ for (const auto& value_type : {float16(), float32(), float64()}) {
+ SCOPED_TRACE(value_type->ToString());
+ auto dict_ty = dictionary(int32(), value_type);
+ auto arr =
+ DictArrayFromJSON(dict_ty, "[0, 1, 2, 3, null, 1]", "[1.5, NaN, -0.0,
null]");
+
+ // Null dictionary values and null indices are always null.
+ CheckScalarUnary(
+ "is_null", arr,
+ ArrayFromJSON(boolean(), "[false, false, false, true, true, false]"));
+ CheckScalarUnary("is_null", arr,
+ ArrayFromJSON(boolean(), "[false, false, false, true,
true, false]"),
+ &default_options);
+ CheckScalarUnary("is_null", arr,
+ ArrayFromJSON(boolean(), "[false, true, false, true,
true, true]"),
+ &nan_is_null_options);
+
+ auto empty = DictArrayFromJSON(dict_ty, "[]", "[]");
+ CheckScalarUnary("is_null", empty, ArrayFromJSON(boolean(), "[]"),
+ &nan_is_null_options);
+ }
Review Comment:
This test only exercises `int32` dictionary indices, but the fix is intended
to work for any index type (and the PR description mentions unsigned indices).
Consider adding an unsigned index type (e.g. `uint8()`) here to prevent
regressions.
--
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]
