Copilot commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r4053856943
##########
cpp/src/parquet/page_index.h:
##########
@@ -73,6 +74,12 @@ class PARQUET_EXPORT ColumnIndex {
/// available.
virtual const std::vector<int64_t>& null_counts() const = 0;
+ /// \brief The number of NaN values in each data page, if available.
+ ///
+ /// `std::nullopt` means the field is not set. When present, the span
contains
+ /// one value per data page.
+ virtual std::optional<std::span<const int64_t>> nan_counts() const = 0;
Review Comment:
The PR description advertises a `ColumnIndex::has_nan_counts()` presence
API, but this interface only adds an optional span and is inconsistent with the
existing `has_null_counts()`/`null_counts()` pair. Please add the presence
accessor and implement it from `__isset.nan_counts`, or update the public API
contract if the optional-return design is intentional.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -773,6 +975,41 @@ class TypedStatisticsImpl : public TypedStatistics<DType> {
}
if (comparator_ == nullptr) return;
+
+ if constexpr (IsOneOf<DType, FloatType, DoubleType, FLBAType>::value) {
+ auto visit_valid_indices = [&](auto&& visit) {
+ ::arrow::internal::VisitSetBitRunsVoid(
+ values.null_bitmap_data(), values.offset(), values.length(),
+ [&](int64_t position, int64_t run_length) {
+ for (int64_t value_index = 0; value_index < run_length;
++value_index) {
+ visit(position + value_index);
+ }
+ });
+ };
+ if constexpr (IsOneOf<DType, FloatType, DoubleType>::value) {
+ using ArrayType = typename ::arrow::CTypeTraits<T>::ArrayType;
+ const auto& array = checked_cast<const ArrayType&>(values);
+ UpdateFloatingBounds(
+ [&](auto&& visit) {
+ visit_valid_indices(
+ [&](int64_t value_index) { visit(array.Value(value_index));
});
+ },
+ update_counts);
Review Comment:
`update_counts` is false on the dictionary-array path (`column_writer.cc`
updates stats from the referenced, de-duplicated dictionary) and on paths where
null/num counts are supplied separately. Passing it through as
`update_nan_count` therefore clears `nan_count`; even changing it to true would
count each referenced dictionary value once rather than each data occurrence. A
dictionary page such as `[NaN, NaN, 1]` consequently loses or undercounts NaNs,
so the advertised row-group/page `nan_count` metadata is wrong. Compute NaN
counts from the actual values/indices with multiplicity, independently of the
null and value count flag.
--
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]
