MINOR: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays - #51000
MINOR: [C++] Fix is_null nan_is_null for dictionary-encoded float arrays#51000shoemoney wants to merge 2 commits into
Conversation
Fix verified RED->GREEN. is_null with nan_is_null=True ignores NaN in dictionary-encoded float arrays at scalar_validity.cc:124
|
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? or After updating the title, you can mark the pull request as ready for review. See also: |
There was a problem hiding this comment.
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_nullwhennan_is_nullis 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
left a comment
Reranko05
left a comment
There was a problem hiding this comment.
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?
…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.
shoemoney
changed the title
|
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. |
Rationale for this change
is_null(..., nan_is_null=true)did not detect NaN values in dictionary-encoded floating-point arrays.IsNullExeconly 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.What changes are included in this PR?
SetNanBitsDictionary, a templated helper that iterates dictionary indices and checks the referenced dictionary entry for NaN, for float, double, and half-float value types.DispatchIndexTypeto dispatch over all standard integer dictionary index types (int8/16/32/64, uint8/16/32/64), failing fast on an unrecognized index type instead of assuming int32.IsNullExecto detect a DICTIONARY type with floating-point values andnan_is_null=true, and dispatch to the new NaN detection path.SetNanBitsDictionaryskips null index slots before dereferencing the dictionary, since the indices buffer is not guaranteed initialized at null positions.Are these changes tested?
Yes, with a test that fails against the prior behavior and passes with this fix.
Are there any user-facing changes?
Yes.
is_null(..., nan_is_null=true)now correctly treats NaN values in dictionary-encoded float arrays as null.