GH-51127: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays by shoemoney · Pull Request #51000 · apache/arrow

shoemoney · 2026-08-25T20:53:50Z

Rationale for this change

is_null(..., nan_is_null=true) did not detect NaN values stored in a dictionary's values, so NaN entries in dictionary-encoded float/double/half-float arrays were reported as valid instead of null. Null dictionary values must also keep being reported as null with both default options and nan_is_null=true.

What changes are included in this PR?

Adds NaN and null detection to is_null for dictionary-encoded floating-point arrays. For dictionaries of floating types, the output bitmap is built by calling is_null on the dictionary values (which also catches NaN when nan_is_null=true), mapping that through the indices with take, and OR-ing it into the existing null-index bitmap. take runs without bounds checking, matching how other compute kernels treat dictionary indices as already valid. Every other is_null path is unchanged.

Are these changes tested?

Yes. A single parameterized test in scalar_validity_test.cc covers float16, float32, and float64 dictionaries: NaN values, null dictionary entries, null indices, default options, and empty arrays.

Are there any user-facing changes?

Yes. is_null(..., nan_is_null=true) now reports NaN entries in dictionary-encoded floating-point arrays as null. Null dictionary values were already reported as null and remain so.

GitHub Issue: [C++] is_null(nan_is_null=true) ignores NaN and null values in dictionary-encoded float arrays #51127

github-actions · 2026-08-25T20:54:19Z

Thanks for opening a pull request!

This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format.

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

Pull request overview

This PR fixes is_null(..., nan_is_null=true) for dictionary-encoded floating-point arrays by detecting NaN values in the dictionary and marking corresponding slots as null in the output.

Changes:

Added dictionary-aware NaN detection for is_null when nan_is_null is enabled.
Implemented templated helpers to dispatch over dictionary index integer types and float value types (float/double/half-float).
Added includes needed for dictionary type inspection and safe casting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Reranko05

Could you use the Arrow PR title template:-
GH-<Issue Number>: [<Component>] <Title>

If there isn't an existed issue, you can create a new one.

And could you use Arrow PR template for description

Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?

shoemoney · 2026-08-27T20:29:43Z

Updated the title and description to match the template, and fixed both issues from the review: null index slots are now skipped before dereferencing the dictionary, and the default case in the index-type dispatch fails fast instead of assuming int32.

pitrou · 2026-09-01T15:25:09Z

@shoemoney Hi, thanks for submitting this. Some high-level comments after taking a quick look:

This is not a MINOR change, can you open a separate issue for the bug?
Dictionary values (not only indices) can be null too, does your PR handle that?
A potentially simpler solution is to call is_null on the dictionary values:
a. dict_is_null = is_null(dict_array.dictionary)
b. temp_is_null = take(dict_is_null, dict_array.indices)
c. final_is_null = BitmapOrNot(temp_is_null.values, temp_is_null.validity_bitmap)

@zanmato1984 Thoughts?

shoemoney · 2026-09-01T21:28:33Z

Thanks for the look. On (1), opened #51127 and retitled against it. On (2), no: the current patch only skips null index slots, so a null dictionary value referenced by a valid index still gets dereferenced, which is the gap you're pointing at. Your is_null(dictionary) → take → BitmapOrNot approach handles both cases in one pass and is simpler than what I have, so I'd rather switch to it than extend the current loop — I'll do that in this PR unless you or @zanmato1984 would prefer it start fresh.

github-actions · 2026-09-01T21:28:59Z

⚠️ GitHub issue #51127 has been automatically assigned in GitHub to PR creator.

shoemoney · 2026-09-18T14:02:10Z

Switched to your approach in (3). The NaN loop and its per-index-type dispatch are gone, replaced by is_null on the dictionary values, take through the indices, and an OR into the output bitmap. The output already holds the inverted index validity bitmap at that point, so the take result's values bitmap goes in as is, no masking needed. take runs with bounds checking, so an out-of-range index now errors instead of reading past the dictionary.

That also answers (2): a null dictionary value behind a valid index is reported as null now. New test IsNullDictionaryNullValues fails at index 1 on the previous commit and passes on this one. I added IsNullDictionaryNanIsNullUnsignedIndices too, since the explicit index-type dispatch that used to be covered is gone.

The path still only runs for nan_is_null=true with a floating-point value type, so is_null with default options is unchanged for every dictionary type. Tell me if you would rather null dictionary values be reported for all dictionary types, that is a one-line widening of the condition.

Rebased onto main, commit 1d2b266. Local run of arrow-compute-scalar-utility-test: 99 tests from 41 suites passed.

Copilot

🟢 Approval recommended

The functional fix and new coverage align with the issue description, with only a minor performance refinement suggested in review comments.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

Copilot

🟡 Changes recommended

The new code uses checked_cast unqualified in two places, which should be ::arrow::internal::checked_cast (or imported), and will otherwise fail to compile.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

cpp/src/arrow/compute/kernels/scalar_validity.cc:171

Unqualified checked_cast here should be ::arrow::internal::checked_cast (or add a using internal::checked_cast; in scope); as written this is a compile error.

    const auto& dict_type = checked_cast<const DictionaryType&>(*arr.type);

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

…float arrays is_null(..., nan_is_null=true) did not detect NaN values in dictionary-encoded floating-point arrays. IsNullExec only checked the top-level type id for floating point, so dictionary-encoded arrays (type id DICTIONARY) skipped the NaN path entirely and NaN values in the dictionary were never marked null. Adds SetNanBitsFromDictionary, which calls is_null on the dictionary values with nan_is_null=true, maps the result through the indices with take, and ORs the resulting bitmap into the output. Hooks it into SetLogicalNullBits's DICTIONARY branch, alongside the existing dict_util::SetLogicalNullBits call that already reports null indices and null dictionary entries. Rebased onto the SetLogicalNullBits refactor that landed on main after this branch was opened. dict_util::SetLogicalNullBits now reports a null dictionary entry as a logical null unconditionally, not only under nan_is_null, so the IsNullDictionaryNullValues test's default_options expectation is updated to match (index 1 is now null even without nan_is_null).

Copilot

Copilot review overview

🟢 Approval recommended

The change is narrowly scoped to the nan_is_null dictionary+floating path and is backed by focused regression tests.

Review effort: Lite
Findings: None

Resolved since last review (3)

checked_cast is defined in arrow::internal, but it's used here unqualified; this won't compile…
SetNanBitsDictionary dereferences dict_values[dict_index] even when the corresponding index slot…
The default branch in DispatchIndexType falls back to treating the indices buffer as int32, which…

shoemoney · 2026-09-25T05:00:41Z

Fixed the malformed-index crash in 61bef91. For floating dictionaries with nan_is_null=true, checked take now runs before the dictionary validity lookup. Null-index payloads remain ignored, and other validity paths are unchanged.

The expanded bounds regression terminated with SIGSEGV (exit 139) before this correction and passes afterward. Locally, arrow-compute-scalar-utility-test --gtest_brief=1 passes all 102 tests from 41 suites on macOS arm64 Debug. clang-format 18.1.8 and cpplint 1.6.1 also pass. Full Arrow CI was not run locally.

Copilot

Copilot review overview

🟢 Approval recommended

The logic change is well-scoped, matches the reported issue, and is backed by comprehensive new tests; only minor naming/comment nits remain.

Review effort: Lite
Findings: 1

Open (1)

Rename function to match null-bit behavior · New

Previously missed (1)

In code that hasn't changed since last review

Fix typo in comment: invove to involve

cpp/src/arrow/compute/kernels/scalar_validity.cc:307

Typo in comment: "invove" should be "involve".

pitrou · 2026-09-28T09:45:01Z

+                                 arr.offset);
+  ARROW_ASSIGN_OR_RAISE(Datum taken,
+                        Take(dict_is_null, Datum(std::move(indices)),
+                             TakeOptions::BoundsCheck(), ctx->exec_context()));


Bounds checking is not necessary since the dictionary array is presumed valid, and therefore dict indices are within bounds already.

Changed TakeOptions::BoundsCheck() to NoBoundsCheck() and removed the checked-indices claim from the helper comment. In ca96314.

pitrou · 2026-09-28T09:51:31Z

+                   &nan_is_null_options);
+}
+
+TEST(TestValidityKernels, IsNullDictionaryNullValues) {


Can you merge this test with the previous one?

Merged dictionary-null-value assertions into IsNullDictionaryNanIsNull. In ca96314.

pitrou · 2026-09-28T09:52:05Z

+                   &nan_is_null_options);
+}
+
+TEST(TestValidityKernels, IsNullDictionaryNanIsNullUnsignedIndices) {


I don't think this test is useful, there is nothing in the implementation that justifies exercising this separately.

Removed the separate unsigned-index test. In ca96314.

pitrou · 2026-09-28T09:52:37Z

+                   &nan_is_null_options);
+}
+
+TEST(TestValidityKernels, IsNullDictionaryNanIsNullBounds) {


This test is creating an invalid Arrow array. Compute functions are only specified for valid Arrow arrays. Let's please remove it.

Removed the invalid bounds-array test; retained its valid empty-array case in the main test. In ca96314.

pitrou · 2026-09-28T09:53:11Z

+  CheckScalarUnary("is_null", empty, ArrayFromJSON(boolean(), "[]"), &options);
+}
+
+TEST(TestValidityKernels, IsNullDictionaryNanIsNullSkipsNullIndex) {


I don't think this test makes sense either.

Removed the special physical null-index payload test; ordinary null indices remain in the main test. In ca96314.

pitrou · 2026-09-28T09:53:36Z

+                   &options);
+}
+
+TEST(TestValidityKernels, IsNullDictionaryNanIsNullHalfFloat) {


Instead of a separate test for half-float, can you loop over all floating-point types in the main test?

Replaced the separate half-float test with one loop over float16(), float32(), and float64(). In ca96314. The consolidated regression passes for all three types, and the rebuilt scalar utility suite passes all 97 tests on macOS arm64 Debug. clang-format and cpplint also pass.

Copilot

Copilot review overview

🟡 Changes recommended

The new helper currently disables bounds checking in take, which risks out-of-bounds access and contradicts the intended safety behavior.

Review effort: Lite
Findings: 1 · 1

Open (2)

Use bounds-checked Take for dictionary indices · New
Test should cover unsigned dictionary index types · New

Resolved since last review (1)

Rename function to match null-bit behavior

pitrou · 2026-09-28T10:30:42Z

@shoemoney Can you please update the PR description to summarize changes at a high level? Also please write the PR description yourself, don't use AI.

shoemoney · 2026-09-28T11:06:35Z

The ARM64 macOS C++ job failed because arrow-s3fs-test hit the 300-second timeout on all three CTest attempts. The other 109 tests passed, including arrow-compute-scalar-utility-test. The fork run of ca9631491 passed all 110 tests on ARM64 macOS, including S3. This PR changes no filesystem or CI code. The timeout cause is still unconfirmed, so I have left the code unchanged.

shoemoney · 2026-09-28T11:59:24Z

Updated the description with a high-level summary.

pitrou · 2026-09-28T13:05:13Z

+  // Null index slots are already set from the input validity bitmap, so the
+  // values bitmap can be OR'ed in without masking null slots out first.
+  const ArrayData& result = *taken.array();
+  ::arrow::internal::BitmapOr(out_bitmap, out_offset, result.buffers[1]->data(),


Oops, I forgot one point here. You're calling Invert then BitmapOr, but BitmapOrNot would do faster in just one step.

Also, if arr.GetNullCount() == 0, no logical operation is required and we can just copy the bitmap returned by Take.

Changed in a78401dca: BitmapOrNot combines the taken bits with inverted index validity in one pass; arrays without null indices use CopyBitmap. The float16/32/64 test now also covers non-null indices pointing to null/NaN values. Rebuilt scalar-utility suite: 97 tests passed (macOS arm64 Debug); clang-format 18.1.8 and cpplint 1.6.1 passed. AI-assisted implementation and validation.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

pitrou

Thank you for the update, LGTM now

shoemoney requested a review from pitrou as a code owner August 25, 2026 20:53

Copilot AI lite review requested due to automatic review settings August 25, 2026 20:53

Copilot started reviewing on behalf of shoemoney August 25, 2026 20:54

View session

github-actions Bot marked this pull request as draft August 25, 2026 20:54

github-actions Bot added Component: C++ awaiting review Awaiting review labels Aug 25, 2026

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc Outdated

Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc Outdated

Reranko05 reviewed Aug 26, 2026

View reviewed changes

shoemoney changed the title ~~fix(arrow): handle nan_is_null for dictionary-encoded floats~~ MINOR: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays Aug 27, 2026

shoemoney mentioned this pull request Sep 1, 2026

[C++] is_null(nan_is_null=true) ignores NaN and null values in dictionary-encoded float arrays #51127

Closed

shoemoney changed the title ~~MINOR: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays~~ GH-51127: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays Sep 1, 2026

shoemoney force-pushed the fix/arrow-isnull-dict branch from 2ff410b to 1d2b266 Compare September 18, 2026 14:01

shoemoney marked this pull request as ready for review September 18, 2026 14:02

Copilot AI review requested due to automatic review settings September 18, 2026 14:02

Copilot started reviewing on behalf of shoemoney September 18, 2026 14:02

View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc Outdated

Copilot AI review requested due to automatic review settings September 18, 2026 14:28

Copilot started reviewing on behalf of shoemoney September 18, 2026 14:29

View session

Copilot AI reviewed Sep 18, 2026

View reviewed changes

Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc Outdated

Copilot AI review requested due to automatic review settings September 25, 2026 04:32

shoemoney force-pushed the fix/arrow-isnull-dict branch from 8811b9e to 59ca9af Compare September 25, 2026 04:32

Copilot started reviewing on behalf of shoemoney September 25, 2026 04:32

View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

apacheGH-51127: [C++] Check dictionary indices before validity lookup

61bef91

Copilot AI review requested due to automatic review settings September 25, 2026 04:59

Copilot started reviewing on behalf of shoemoney September 25, 2026 05:00

View session

Copilot AI reviewed Sep 25, 2026

View reviewed changes

Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc Outdated

pitrou requested changes Sep 28, 2026

View reviewed changes

apacheGH-51127: [C++] Simplify dictionary NaN null checks

ca96314

Copilot AI review requested due to automatic review settings September 28, 2026 10:06

Copilot started reviewing on behalf of shoemoney September 28, 2026 10:06

View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Comment thread cpp/src/arrow/compute/kernels/scalar_validity.cc

Comment thread cpp/src/arrow/compute/kernels/scalar_validity_test.cc

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 28, 2026

pitrou approved these changes Sep 28, 2026

View reviewed changes

pitrou reviewed Sep 28, 2026

View reviewed changes

apacheGH-51127: Combine dictionary null bits in one bitmap pass

a78401d

Copilot AI review requested due to automatic review settings September 28, 2026 13:35

Copilot AI reviewed Sep 28, 2026

pitrou approved these changes Sep 28, 2026

View reviewed changes

pitrou merged commit c6620b4 into apache:main Sep 28, 2026
62 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Sep 28, 2026

Conversation

shoemoney commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reranko05 left a comment

Choose a reason for hiding this comment

Uh oh!

shoemoney commented Aug 27, 2026

Uh oh!

pitrou commented Sep 1, 2026

Uh oh!

shoemoney commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

shoemoney commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

shoemoney commented Sep 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

shoemoney commented Aug 25, 2026 •

edited

Loading

shoemoney commented Sep 1, 2026 •

edited

Loading