From e50a11f95eef9f20e6e3df36147fe80ebf6f1c72 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Tue, 25 Aug 2026 15:53:40 -0500 Subject: [PATCH 1/5] fix(arrow): handle nan_is_null for dictionary-encoded floats Fix verified RED->GREEN. is_null with nan_is_null=True ignores NaN in dictionary-encoded float arrays at scalar_validity.cc:124 --- .../arrow/compute/kernels/scalar_validity.cc | 74 +++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/scalar_validity.cc b/cpp/src/arrow/compute/kernels/scalar_validity.cc index 5913b756f1c0..e710efbeb712 100644 --- a/cpp/src/arrow/compute/kernels/scalar_validity.cc +++ b/cpp/src/arrow/compute/kernels/scalar_validity.cc @@ -21,8 +21,10 @@ #include "arrow/compute/kernels/common_internal.h" #include "arrow/compute/registry_internal.h" +#include "arrow/type.h" #include "arrow/util/bit_util.h" #include "arrow/util/bitmap_ops.h" +#include "arrow/util/checked_cast.h" #include "arrow/util/float16.h" #include "arrow/util/logging_internal.h" @@ -101,6 +103,60 @@ static void SetNanBits(const ArraySpan& arr, uint8_t* out_bitmap, int64_t out_of } } +template +static void SetNanBitsDictionary(const ArraySpan& arr, const ArraySpan& dict_span, + uint8_t* out_bitmap, int64_t out_offset) { + const IndexType* indices = arr.GetValues(1); + const ValueType* dict_values = dict_span.GetValues(1); + for (int64_t i = 0; i < arr.length; ++i) { + auto dict_index = indices[i]; + bool is_nan; + if constexpr (std::is_same_v) { + is_nan = Float16::FromBits(dict_values[dict_index]).is_nan(); + } else { + is_nan = std::isnan(dict_values[dict_index]); + } + if (is_nan) { + bit_util::SetBit(out_bitmap, i + out_offset); + } + } +} + +template +static void DispatchIndexType(const ArraySpan& arr, const ArraySpan& dict_span, + uint8_t* out_bitmap, int64_t out_offset) { + const auto& dict_type = checked_cast(*arr.type); + switch (dict_type.index_type()->id()) { + case Type::INT8: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::INT16: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::INT32: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::INT64: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::UINT8: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::UINT16: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::UINT32: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + case Type::UINT64: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + default: + SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + break; + } +} + Status IsNullExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) { const ArraySpan& arr = batch[0].array; ArraySpan* out_span = out->array_span_mutable(); @@ -136,6 +192,24 @@ Status IsNullExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) { return Status::NotImplemented("NaN detection not implemented for type ", arr.type->ToString()); } + } else if (arr.type->id() == Type::DICTIONARY && options.nan_is_null) { + const auto& dict_type = checked_cast(*arr.type); + if (is_floating(dict_type.value_type()->id())) { + const ArraySpan& dict_span = arr.dictionary(); + switch (dict_type.value_type()->id()) { + case Type::FLOAT: + DispatchIndexType(arr, dict_span, out_bitmap, out_span->offset); + break; + case Type::DOUBLE: + DispatchIndexType(arr, dict_span, out_bitmap, out_span->offset); + break; + case Type::HALF_FLOAT: + DispatchIndexType(arr, dict_span, out_bitmap, out_span->offset); + break; + default: + break; + } + } } return Status::OK(); } From 40798fde3dd7d288c1e329abec927d183e793a23 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Thu, 27 Aug 2026 15:25:47 -0500 Subject: [PATCH 2/5] fix(arrow): guard dictionary NaN detection against nulls and unknown index types Skip null index slots in SetNanBitsDictionary before dereferencing the dictionary to avoid out-of-bounds reads on arrays with nulls. Make the default case in DispatchIndexType fail fast instead of silently treating unknown index types as int32. --- cpp/src/arrow/compute/kernels/scalar_validity.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_validity.cc b/cpp/src/arrow/compute/kernels/scalar_validity.cc index e710efbeb712..1b6eac717da0 100644 --- a/cpp/src/arrow/compute/kernels/scalar_validity.cc +++ b/cpp/src/arrow/compute/kernels/scalar_validity.cc @@ -109,6 +109,9 @@ static void SetNanBitsDictionary(const ArraySpan& arr, const ArraySpan& dict_spa const IndexType* indices = arr.GetValues(1); const ValueType* dict_values = dict_span.GetValues(1); for (int64_t i = 0; i < arr.length; ++i) { + if (arr.IsNull(i)) { + continue; + } auto dict_index = indices[i]; bool is_nan; if constexpr (std::is_same_v) { @@ -152,7 +155,8 @@ static void DispatchIndexType(const ArraySpan& arr, const ArraySpan& dict_span, SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); break; default: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); + DCHECK(false) << "unreachable: unsupported dictionary index type " + << dict_type.index_type()->ToString(); break; } } From bb4099cc7588ceac06b6b1b039aaed16a26cc40c Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Thu, 27 Aug 2026 20:11:04 -0500 Subject: [PATCH 3/5] test(arrow): add is_null nan_is_null coverage for dictionary-encoded floats --- .../compute/kernels/scalar_validity_test.cc | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/cpp/src/arrow/compute/kernels/scalar_validity_test.cc b/cpp/src/arrow/compute/kernels/scalar_validity_test.cc index 4613176b48ca..f2e5bc874c1c 100644 --- a/cpp/src/arrow/compute/kernels/scalar_validity_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_validity_test.cc @@ -152,6 +152,37 @@ 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); + + auto dict_ty = dictionary(int32(), float64()); + auto arr = DictArrayFromJSON(dict_ty, "[0, 1, 2, null, 1]", "[1.5, NaN, -0.0]"); + + // Without nan_is_null, dictionary-encoded NaNs are not treated as null. + CheckScalarUnary("is_null", arr, + ArrayFromJSON(boolean(), "[false, false, false, true, false]")); + CheckScalarUnary("is_null", arr, + ArrayFromJSON(boolean(), "[false, false, false, true, false]"), + &default_options); + + // With nan_is_null, the dictionary entry backing index 1 is NaN, so every + // slot referencing it is null; the pre-existing null index stays null. + CheckScalarUnary("is_null", arr, + ArrayFromJSON(boolean(), "[false, true, false, true, true]"), + &nan_is_null_options); +} + +TEST(TestValidityKernels, IsNullDictionaryNanIsNullHalfFloat) { + NullOptions nan_is_null_options(/*nan_is_null=*/true); + + auto dict_ty = dictionary(int8(), float16()); + auto arr = DictArrayFromJSON(dict_ty, "[0, 1]", "[1.5, NaN]"); + + CheckScalarUnary("is_null", arr, ArrayFromJSON(boolean(), "[false, true]"), + &nan_is_null_options); +} + template class TestFloatingPointValidityKernels : public TestValidityKernels { public: From 1d2b26649c445b4debdfd03593a834c6a57b6b00 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Fri, 18 Sep 2026 09:00:53 -0500 Subject: [PATCH 4/5] fix(arrow): map is_null through the dictionary to cover null values Replace the per-index-type NaN loop with is_null over the dictionary values followed by take through the indices, so null dictionary entries are reported as well as NaN ones. --- .../arrow/compute/kernels/scalar_validity.cc | 95 ++++++------------- .../compute/kernels/scalar_validity_test.cc | 26 +++++ 2 files changed, 53 insertions(+), 68 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_validity.cc b/cpp/src/arrow/compute/kernels/scalar_validity.cc index 1b6eac717da0..312d4a47a3de 100644 --- a/cpp/src/arrow/compute/kernels/scalar_validity.cc +++ b/cpp/src/arrow/compute/kernels/scalar_validity.cc @@ -21,6 +21,7 @@ #include "arrow/compute/kernels/common_internal.h" #include "arrow/compute/registry_internal.h" +#include "arrow/compute/api_vector.h" #include "arrow/type.h" #include "arrow/util/bit_util.h" #include "arrow/util/bitmap_ops.h" @@ -103,62 +104,32 @@ static void SetNanBits(const ArraySpan& arr, uint8_t* out_bitmap, int64_t out_of } } -template -static void SetNanBitsDictionary(const ArraySpan& arr, const ArraySpan& dict_span, - uint8_t* out_bitmap, int64_t out_offset) { - const IndexType* indices = arr.GetValues(1); - const ValueType* dict_values = dict_span.GetValues(1); - for (int64_t i = 0; i < arr.length; ++i) { - if (arr.IsNull(i)) { - continue; - } - auto dict_index = indices[i]; - bool is_nan; - if constexpr (std::is_same_v) { - is_nan = Float16::FromBits(dict_values[dict_index]).is_nan(); - } else { - is_nan = std::isnan(dict_values[dict_index]); - } - if (is_nan) { - bit_util::SetBit(out_bitmap, i + out_offset); - } +// Maps `is_null` over the dictionary values and then through the indices, so that +// both NaN and null dictionary entries are reported, whatever the index type. +static Status SetNullBitsFromDictionary(KernelContext* ctx, const ArraySpan& arr, + const NullOptions& options, uint8_t* out_bitmap, + int64_t out_offset) { + if (arr.length == 0) { + return Status::OK(); } -} - -template -static void DispatchIndexType(const ArraySpan& arr, const ArraySpan& dict_span, - uint8_t* out_bitmap, int64_t out_offset) { const auto& dict_type = checked_cast(*arr.type); - switch (dict_type.index_type()->id()) { - case Type::INT8: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::INT16: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::INT32: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::INT64: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::UINT8: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::UINT16: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::UINT32: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - case Type::UINT64: - SetNanBitsDictionary(arr, dict_span, out_bitmap, out_offset); - break; - default: - DCHECK(false) << "unreachable: unsupported dictionary index type " - << dict_type.index_type()->ToString(); - break; - } + ARROW_ASSIGN_OR_RAISE(Datum dict_is_null, + CallFunction("is_null", {arr.dictionary().ToArrayData()}, + &options, ctx->exec_context())); + + std::shared_ptr indices = arr.ToArrayData(); + indices->type = dict_type.index_type(); + indices->dictionary = nullptr; + ARROW_ASSIGN_OR_RAISE(Datum taken, + Take(dict_is_null, Datum(std::move(indices)), + TakeOptions::BoundsCheck(), ctx->exec_context())); + + // Slots with a null index are already set from the input validity bitmap, so the + // values bitmap can be OR'ed in without masking the nulls out of it first. + const ArrayData& result = *taken.array(); + ::arrow::internal::BitmapOr(out_bitmap, out_offset, result.buffers[1]->data(), + result.offset, arr.length, out_offset, out_bitmap); + return Status::OK(); } Status IsNullExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) { @@ -199,20 +170,8 @@ Status IsNullExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* out) { } else if (arr.type->id() == Type::DICTIONARY && options.nan_is_null) { const auto& dict_type = checked_cast(*arr.type); if (is_floating(dict_type.value_type()->id())) { - const ArraySpan& dict_span = arr.dictionary(); - switch (dict_type.value_type()->id()) { - case Type::FLOAT: - DispatchIndexType(arr, dict_span, out_bitmap, out_span->offset); - break; - case Type::DOUBLE: - DispatchIndexType(arr, dict_span, out_bitmap, out_span->offset); - break; - case Type::HALF_FLOAT: - DispatchIndexType(arr, dict_span, out_bitmap, out_span->offset); - break; - default: - break; - } + RETURN_NOT_OK( + SetNullBitsFromDictionary(ctx, arr, options, out_bitmap, out_span->offset)); } } return Status::OK(); diff --git a/cpp/src/arrow/compute/kernels/scalar_validity_test.cc b/cpp/src/arrow/compute/kernels/scalar_validity_test.cc index f2e5bc874c1c..a08727037737 100644 --- a/cpp/src/arrow/compute/kernels/scalar_validity_test.cc +++ b/cpp/src/arrow/compute/kernels/scalar_validity_test.cc @@ -173,6 +173,32 @@ TEST(TestValidityKernels, IsNullDictionaryNanIsNull) { &nan_is_null_options); } +TEST(TestValidityKernels, IsNullDictionaryNullValues) { + NullOptions default_options; + NullOptions nan_is_null_options(/*nan_is_null=*/true); + + auto dict_ty = dictionary(int32(), float64()); + auto arr = DictArrayFromJSON(dict_ty, "[0, 1, 2, null]", "[1.5, null, NaN]"); + + // A null dictionary value is only reported through the nan_is_null path, which is + // where the dictionary is inspected at all. + CheckScalarUnary("is_null", arr, + ArrayFromJSON(boolean(), "[false, false, false, true]"), + &default_options); + CheckScalarUnary("is_null", arr, ArrayFromJSON(boolean(), "[false, true, true, true]"), + &nan_is_null_options); +} + +TEST(TestValidityKernels, IsNullDictionaryNanIsNullUnsignedIndices) { + NullOptions nan_is_null_options(/*nan_is_null=*/true); + + auto dict_ty = dictionary(uint8(), float32()); + auto arr = DictArrayFromJSON(dict_ty, "[2, 0, 1]", "[1.5, NaN, 2.5]"); + + CheckScalarUnary("is_null", arr, ArrayFromJSON(boolean(), "[false, false, true]"), + &nan_is_null_options); +} + TEST(TestValidityKernels, IsNullDictionaryNanIsNullHalfFloat) { NullOptions nan_is_null_options(/*nan_is_null=*/true); From 8811b9ea7ac687bada98cec98227e357bbfefcc5 Mon Sep 17 00:00:00 2001 From: Jeremy Schoemaker Date: Fri, 18 Sep 2026 09:25:44 -0500 Subject: [PATCH 5/5] GH-51127: [C++] Build is_null dictionary indices directly --- cpp/src/arrow/compute/kernels/scalar_validity.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/src/arrow/compute/kernels/scalar_validity.cc b/cpp/src/arrow/compute/kernels/scalar_validity.cc index 312d4a47a3de..9abc6af258b8 100644 --- a/cpp/src/arrow/compute/kernels/scalar_validity.cc +++ b/cpp/src/arrow/compute/kernels/scalar_validity.cc @@ -117,9 +117,9 @@ static Status SetNullBitsFromDictionary(KernelContext* ctx, const ArraySpan& arr CallFunction("is_null", {arr.dictionary().ToArrayData()}, &options, ctx->exec_context())); - std::shared_ptr indices = arr.ToArrayData(); - indices->type = dict_type.index_type(); - indices->dictionary = nullptr; + 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::BoundsCheck(), ctx->exec_context()));