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 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. A null value in the dictionary, referenced by a valid index, was not reported either.

What changes are included in this PR?

Added SetNullBitsFromDictionary, which calls is_null on the dictionary values, maps the result through the indices with take, and ORs the resulting bitmap into the output.
Extended IsNullExec to route a DICTIONARY type with floating-point values and nan_is_null=true to that helper.
The output bitmap already holds the inverted index validity bitmap when the helper runs, so slots with a null index need no extra handling.
take runs with bounds checking, so an out-of-range index returns an error instead of reading past the dictionary.

The path 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.

Are these changes tested?

Yes. IsNullDictionaryNullValues covers a null dictionary value and fails without the kernel change. IsNullDictionaryNanIsNull, IsNullDictionaryNanIsNullHalfFloat and IsNullDictionaryNanIsNullUnsignedIndices cover NaN entries for float64, float16 and a uint8 index type.

Are there any user-facing changes?

Yes. is_null(..., nan_is_null=true) now treats both NaN and null entries of a dictionary-encoded float array as null.

Written in conjunction with my pair programmer Claude.

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.

+    default:
+      SetNanBitsDictionary<int32_t, ValueType>(arr, dict_span, out_bitmap, out_offset);
+      break;


+  const IndexType* indices = arr.GetValues<IndexType>(1);
+  const ValueType* dict_values = dict_span.GetValues<ValueType>(1);
+  for (int64_t i = 0; i < arr.length; ++i) {
+    auto dict_index = indices[i];
+    bool is_nan;
+    if constexpr (std::is_same_v<ValueType, uint16_t>) {
+      is_nan = Float16::FromBits(dict_values[dict_index]).is_nan();
+    } else {
+      is_nan = std::isnan(dict_values[dict_index]);
+    }


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.

Fix verified RED->GREEN. is_null with nan_is_null=True ignores NaN in dictionary-encoded float arrays at scalar_validity.cc:124

…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.

…floats

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.

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

shoemoney · 2026-09-19T11:08:54Z

+  const auto& dict_type = checked_cast<const DictionaryType&>(*arr.type);
+  ARROW_ASSIGN_OR_RAISE(Datum dict_is_null,


scalar_validity.cc includes common_internal.h, which imports arrow::internal::checked_cast into namespace arrow with using internal::checked_cast (line 51). Both calls are inside namespaces nested in arrow, so unqualified lookup finds that declaration. On September 18, I verified the unchanged commit 8811b9e with a fresh compiler syntax check using the generated build flags and warnings as errors; it passes. The scalar utility suite also passes all 99 tests. No code change is needed for this finding.

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

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

Open

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 added 4 commits September 18, 2026 09:00

fix(arrow): handle nan_is_null for dictionary-encoded floats

e50a11f

Fix verified RED->GREEN. is_null with nan_is_null=True ignores NaN in dictionary-encoded float arrays at scalar_validity.cc:124

test(arrow): add is_null nan_is_null coverage for dictionary-encoded …

bb4099c

…floats

fix(arrow): map is_null through the dictionary to cover null values

1d2b266

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.

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

apacheGH-51127: [C++] Build is_null dictionary indices directly

8811b9e

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

Navigation Menu

GH-51127: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays - #51000

GH-51127: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays#51000
shoemoney wants to merge 5 commits into
apache:mainfrom
shoemoney:fix/arrow-isnull-dict

shoemoney commented Aug 25, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reranko05 left a 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!

github-actions Bot commented Sep 1, 2026

Uh oh!

shoemoney commented Sep 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

shoemoney Sep 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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!

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!

shoemoney Sep 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shoemoney commented Aug 25, 2026 •

edited

Loading

shoemoney commented Sep 1, 2026 •

edited

Loading