Copilot commented on code in PR #51000:
URL: https://github.com/apache/arrow/pull/51000#discussion_r3857330502
##########
cpp/src/arrow/compute/kernels/scalar_validity.cc:
##########
@@ -101,6 +103,60 @@ static void SetNanBits(const ArraySpan& arr, uint8_t*
out_bitmap, int64_t out_of
}
}
+template <typename IndexType, typename ValueType>
+static void SetNanBitsDictionary(const ArraySpan& arr, const ArraySpan&
dict_span,
+ uint8_t* out_bitmap, int64_t out_offset) {
+ 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]);
+ }
+ if (is_nan) {
+ bit_util::SetBit(out_bitmap, i + out_offset);
+ }
+ }
+}
+
+template <typename ValueType>
+static void DispatchIndexType(const ArraySpan& arr, const ArraySpan& dict_span,
+ uint8_t* out_bitmap, int64_t out_offset) {
+ const auto& dict_type = checked_cast<const DictionaryType&>(*arr.type);
+ switch (dict_type.index_type()->id()) {
+ case Type::INT8:
+ SetNanBitsDictionary<int8_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::INT16:
+ SetNanBitsDictionary<int16_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::INT32:
+ SetNanBitsDictionary<int32_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::INT64:
+ SetNanBitsDictionary<int64_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::UINT8:
+ SetNanBitsDictionary<uint8_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::UINT16:
+ SetNanBitsDictionary<uint16_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::UINT32:
+ SetNanBitsDictionary<uint32_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ case Type::UINT64:
+ SetNanBitsDictionary<uint64_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
+ default:
+ SetNanBitsDictionary<int32_t, ValueType>(arr, dict_span, out_bitmap,
out_offset);
+ break;
Review Comment:
The default branch in DispatchIndexType falls back to treating the indices
buffer as int32, which would misinterpret memory (and potentially read
out-of-bounds) if an unexpected index type id ever reaches this code path.
Since all supported dictionary index types are explicitly handled, the default
should be unreachable and fail fast instead of silently choosing a width.
##########
cpp/src/arrow/compute/kernels/scalar_validity.cc:
##########
@@ -101,6 +103,60 @@ static void SetNanBits(const ArraySpan& arr, uint8_t*
out_bitmap, int64_t out_of
}
}
+template <typename IndexType, typename ValueType>
+static void SetNanBitsDictionary(const ArraySpan& arr, const ArraySpan&
dict_span,
+ uint8_t* out_bitmap, int64_t out_offset) {
+ 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]);
+ }
Review Comment:
SetNanBitsDictionary dereferences `dict_values[dict_index]` even when the
corresponding index slot is null (and the indices buffer can contain
uninitialized / arbitrary values for nulls). This can lead to out-of-bounds
reads or crashes when `arr` has nulls. Skip NaN checks for null index slots
(and ideally guard against negative / out-of-range indices) before indexing
into the dictionary.
--
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]
