shoemoney commented on code in PR #51000:
URL: https://github.com/apache/arrow/pull/51000#discussion_r4121128512
##########
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:
NoBoundsCheck is intentional in ca9631491, following the [maintainer
request](https://github.com/apache/arrow/pull/51000#discussion_r4120695707).
Compute functions assume valid Arrow arrays. Dictionary validation checks
indices against the dictionary length in array/validate.cc. The malformed-array
tests and the earlier promise to reject out-of-range indices were removed from
this PR.
##########
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:
The separate unsigned-index test was removed in ca9631491 following the
[maintainer
request](https://github.com/apache/arrow/pull/51000#discussion_r4120772372).
The helper preserves dict_type.index_type() and delegates index handling to
Take, whose existing dispatch handles signed and unsigned widths. The
consolidated test covers float16, float32 and float64; the current PR
description records the removed unsigned-specific test.
--
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]
