wgtmac commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r3801787413
##########
cpp/src/parquet/statistics.h:
##########
@@ -226,15 +233,17 @@ class PARQUET_EXPORT Statistics {
/// \param[in] num_values total number of values
/// \param[in] null_count number of null values
/// \param[in] distinct_count number of distinct values
+ /// \param[in] nan_count number of NaN values
/// \param[in] has_min_max whether the min/max statistics are set
/// \param[in] has_null_count whether the null_count statistics are set
/// \param[in] has_distinct_count whether the distinct_count statistics are
set
+ /// \param[in] has_nan_count whether the nan_count statistics are set
/// \param[in] pool a memory pool to use for any memory allocations, optional
static std::shared_ptr<Statistics> Make(
const ColumnDescriptor* descr, const std::string& encoded_min,
const std::string& encoded_max, int64_t num_values, int64_t null_count,
- int64_t distinct_count, bool has_min_max, bool has_null_count,
- bool has_distinct_count,
+ int64_t distinct_count, int64_t nan_count, bool has_min_max, bool
has_null_count,
Review Comment:
This looks like a breaking change to me. Should we just add a
`std::optional<int64_t> nan_count = std::nullopt` to the end?
##########
docs/source/python/parquet.rst:
##########
@@ -302,7 +302,7 @@ such as the row groups and column chunk metadata and
statistics:
<pyarrow._parquet.RowGroupMetaData object at ...>
num_columns: 4
num_rows: 3
- total_byte_size: 290
+ total_byte_size: 272
Review Comment:
Why this file has been changed?
##########
cpp/src/parquet/arrow/reader_internal.cc:
##########
@@ -270,6 +270,15 @@ Status ByteArrayStatisticsAsScalars(const Statistics&
statistics,
return ExtractDecimalMinMaxFromBytes(statistics.EncodeMin(),
statistics.EncodeMax(),
*logical_type, min, max);
}
+ if (logical_type->type() == LogicalType::Type::FLOAT16) {
+ *min = std::make_shared<::arrow::HalfFloatScalar>(
Review Comment:
So this is an unrelated bug?
##########
cpp/src/parquet/statistics.h:
##########
@@ -245,17 +254,19 @@ class PARQUET_EXPORT Statistics {
/// \param[in] num_values total number of values
/// \param[in] null_count number of null values
/// \param[in] distinct_count number of distinct values
+ /// \param[in] nan_count number of NaN values
/// \param[in] has_min_max whether the min/max statistics are set
/// \param[in] has_null_count whether the null_count statistics are set
/// \param[in] has_distinct_count whether the distinct_count statistics are
set
+ /// \param[in] has_nan_count whether the nan_count statistics are set
/// \param[in] is_min_value_exact whether the min value is exact
/// \param[in] is_max_value_exact whether the max value is exact
/// \param[in] pool a memory pool to use for any memory allocations, optional
static std::shared_ptr<Statistics> Make(
const ColumnDescriptor* descr, const std::string& encoded_min,
const std::string& encoded_max, int64_t num_values, int64_t null_count,
- int64_t distinct_count, bool has_min_max, bool has_null_count,
- bool has_distinct_count, std::optional<bool> is_min_value_exact,
+ int64_t distinct_count, int64_t nan_count, bool has_min_max, bool
has_null_count,
Review Comment:
ditto
##########
cpp/src/parquet/statistics.cc:
##########
@@ -658,15 +832,30 @@ class TypedStatisticsImpl : public TypedStatistics<DType>
{
} else {
has_null_count_ = false;
}
+ if (has_nan_count) {
+ statistics_.nan_count = nan_count;
+ has_nan_count_ = true;
+ } else {
+ has_nan_count_ = false;
+ }
if (has_distinct_count) {
SetDistinctCount(distinct_count);
} else {
has_distinct_count_ = false;
}
if (has_min_max) {
- PlainDecode(encoded_min, &min_);
- PlainDecode(encoded_max, &max_);
+ if constexpr (std::same_as<DType, ByteArrayType> || std::same_as<DType,
FLBAType>) {
+ T decoded_min;
+ T decoded_max;
+ PlainDecode(encoded_min, &decoded_min);
+ PlainDecode(encoded_max, &decoded_max);
+ Copy(decoded_min, &min_, min_buffer_.get());
Review Comment:
Yet another unrelated bug?
##########
cpp/src/parquet/statistics.cc:
##########
@@ -870,18 +1112,85 @@ class TypedStatisticsImpl : public
TypedStatistics<DType> {
this->has_distinct_count_ = false;
// Null count calculation is cheap and enabled by default.
this->has_null_count_ = true;
+ // NaN counts are collected alongside floating-point bounds and enabled by
+ // default.
+ if constexpr (std::same_as<DType, FloatType> || std::same_as<DType,
DoubleType>) {
+ this->has_nan_count_ = true;
+ } else if constexpr (std::same_as<DType, FLBAType>) {
+ this->has_nan_count_ = is_half_float_;
+ } else {
+ this->has_nan_count_ = false;
+ }
+ }
+
+ template <ColumnOrder::type column_order, typename VisitValues>
+ void UpdateFloatingBoundsWithOrder(VisitValues&& visit_values, bool
update_nan_count) {
+ using ArrowFloat = decltype(ToArrowFloat(std::declval<T>()));
+
+ FloatingValueSummary<ArrowFloat, column_order> summary;
+ std::invoke(std::forward<VisitValues>(visit_values),
+ [&](const auto& value) { summary.Add(value); });
+ if (has_nan_count_ && update_nan_count) {
+ statistics_.nan_count += summary.nan_count();
+ }
+ const auto& bounds = summary.bounds();
+ if (bounds.has_value()) {
+ if constexpr (std::same_as<DType, FLBAType>) {
+ DCHECK(is_half_float_);
+ const auto min = bounds->first.ToLittleEndian();
+ const auto max = bounds->second.ToLittleEndian();
+ SetMinMaxPair({FLBA{min.data()}, FLBA{max.data()}});
+ } else {
+ SetMinMaxPair(bounds.value());
+ }
+ }
+ }
+
+ template <typename VisitValues>
+ void UpdateFloatingBounds(VisitValues&& visit_values, bool update_nan_count)
{
+ if (descr_->column_order().get_order() ==
ColumnOrder::IEEE_754_TOTAL_ORDER) {
+ UpdateFloatingBoundsWithOrder<ColumnOrder::IEEE_754_TOTAL_ORDER>(
+ std::forward<VisitValues>(visit_values), update_nan_count);
+ } else {
+ DCHECK(descr_->can_use_min_max());
+ UpdateFloatingBoundsWithOrder<ColumnOrder::TYPE_DEFINED_ORDER>(
+ std::forward<VisitValues>(visit_values), update_nan_count);
+ }
}
void SetMinMaxPair(std::pair<T, T> min_max) {
if (comparator_ == nullptr) return;
- // CleanStatistic can return a nullopt in case of erroneous values, e.g.
NaN
- auto maybe_min_max = CleanStatistic(min_max, logical_type_);
+ auto maybe_min_max =
+ descr_->column_order().get_order() == ColumnOrder::IEEE_754_TOTAL_ORDER
+ ? std::optional<std::pair<T, T>>(min_max)
+ : CleanStatistic(min_max, logical_type_);
if (!maybe_min_max) return;
auto min = maybe_min_max.value().first;
auto max = maybe_min_max.value().second;
- if (!has_min_max_) {
+ bool replace_all_nan_bounds = false;
+ if constexpr (std::same_as<DType, FloatType> || std::same_as<DType,
DoubleType> ||
+ std::same_as<DType, FLBAType>) {
+ if (descr_->column_order().get_order() ==
ColumnOrder::IEEE_754_TOTAL_ORDER) {
+ DCHECK((!std::same_as<DType, FLBAType>) || is_half_float_);
+
+ const bool min_is_nan = IsNaNValue(ToArrowFloat(min));
+ DCHECK_EQ(min_is_nan, IsNaNValue(ToArrowFloat(max)));
+ bool current_bounds_are_nan = false;
+ if (has_min_max_) {
+ const bool current_min_is_nan = IsNaNValue(ToArrowFloat(min_));
+ DCHECK_EQ(current_min_is_nan, IsNaNValue(ToArrowFloat(max_)));
+ current_bounds_are_nan = current_min_is_nan;
+ }
+ if (min_is_nan && has_min_max_ && !current_bounds_are_nan) {
+ return;
+ }
+ replace_all_nan_bounds = !min_is_nan && has_min_max_ &&
current_bounds_are_nan;
Review Comment:
```suggestion
if (has_min_max_ && min_is_nan != current_bounds_are_nan) {
if (min_is_nan) {
return;
}
replace_all_nan_bounds = true;
}
```
This might be easier to understand.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -937,6 +1253,20 @@ void TypedStatisticsImpl<DType>::Update(const T* values,
int64_t num_values,
IncrementNumValues(num_values);
if (num_values == 0 || comparator_ == nullptr) return;
+ if constexpr (std::same_as<DType, FloatType> || std::same_as<DType,
DoubleType> ||
+ std::same_as<DType, FLBAType>) {
+ const bool use_floating_bounds = !std::same_as<DType, FLBAType> ||
is_half_float_;
+ if (use_floating_bounds) {
+ UpdateFloatingBounds(
+ [&](auto&& visit) {
+ for (int64_t value_index = 0; value_index < num_values;
++value_index) {
+ visit(ToArrowFloat(SafeLoad(values + value_index)));
+ }
+ },
+ true);
Review Comment:
```suggestion
/*update_nan_count=*/true);
```
##########
cpp/src/parquet/metadata.cc:
##########
@@ -2101,16 +2063,22 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl {
metadata_->__set_version(file_version);
metadata_->__set_created_by(properties_->created_by());
- // Users cannot set the `ColumnOrder` since we do not have user defined
sort order
- // in the spec yet.
- // We always default to `TYPE_DEFINED_ORDER`. We can expose it in
- // the API once we have user defined sort orders in the Parquet format.
- // TypeDefinedOrder implies choose SortOrder based on
ConvertedType/PhysicalType
- format::TypeDefinedOrder type_defined_order;
- format::ColumnOrder column_order;
- column_order.__set_TYPE_ORDER(type_defined_order);
- column_order.__isset.TYPE_ORDER = true;
- metadata_->column_orders.resize(schema_->num_columns(), column_order);
+ metadata_->column_orders.reserve(schema_->num_columns());
+ for (int column_index = 0; column_index < schema_->num_columns();
++column_index) {
+ format::ColumnOrder column_order;
+ switch (schema_->Column(column_index)->column_order().get_order()) {
+ case ColumnOrder::TYPE_DEFINED_ORDER:
+ column_order.__set_TYPE_ORDER(format::TypeDefinedOrder{});
+ break;
+ case ColumnOrder::IEEE_754_TOTAL_ORDER:
+ column_order.__set_IEEE_754_TOTAL_ORDER(format::IEEE754TotalOrder{});
+ break;
+ case ColumnOrder::UNDEFINED:
Review Comment:
If I remember correctly, `ColumnOrder::UNDEFINED` is a valid value for
legacy Parquet files that written before `TYPE_DEFINED_ORDER` has been added?
##########
cpp/src/parquet/statistics.h:
##########
@@ -134,11 +134,13 @@ class PARQUET_EXPORT EncodedStatistics {
int64_t null_count = 0;
int64_t distinct_count = 0;
+ int64_t nan_count = 0;
Review Comment:
I still prefer to make `nan_count` right now and migrate `null_count` and
`distinct_count` in a followup PR if anyway they will be migrated in the future.
##########
cpp/src/parquet/schema_internal.h:
##########
@@ -50,5 +50,8 @@ std::unique_ptr<Node> Unflatten(const format::SchemaElement*
elements, int lengt
PARQUET_EXPORT
void ToParquet(const GroupNode* schema, std::vector<format::SchemaElement>*
out);
+PARQUET_EXPORT
+bool IsFloatingPoint(const ColumnDescriptor& descr);
Review Comment:
IsFloatingPointType ?
##########
cpp/src/parquet/statistics.cc:
##########
@@ -349,6 +351,45 @@ struct CompareHelper<Float16LogicalType,
/*is_signed=*/true> {
}
};
+float ToArrowFloat(float value) { return value; }
+
+double ToArrowFloat(double value) { return value; }
+
+Float16 ToArrowFloat(const FLBA& value) {
+ DCHECK_NE(value.ptr, nullptr);
+ return Float16::FromLittleEndian(value.ptr);
+}
+
+template <typename Int, typename T>
+std::strong_ordering TotalOrderCompareBits(T lhs, T rhs) {
+ //
https://parquet.apache.org/blog/2026/05/29/taming-floating-point-statistics-in-apache-parquet-ieee-754-total-order-and-nan-counts/
+ auto lhs_bits = SafeCopy<Int>(lhs);
+ auto rhs_bits = SafeCopy<Int>(rhs);
+ using UInt = std::make_unsigned_t<Int>;
+ constexpr int sign_shift = sizeof(Int) * 8 - 1;
+ lhs_bits ^= static_cast<Int>(static_cast<UInt>(lhs_bits >> sign_shift) >> 1);
+ rhs_bits ^= static_cast<Int>(static_cast<UInt>(rhs_bits >> sign_shift) >> 1);
+ return lhs_bits <=> rhs_bits;
+}
+
+std::strong_ordering TotalOrderCompare(float lhs, float rhs) {
+ static_assert(std::numeric_limits<float>::is_iec559);
+ // TODO: Use std::strong_order once all supported standard libraries
implement its
Review Comment:
It seems that we don't need this comment. We anyway need to maintain
TotalOrderCompareBits above because of float16.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -523,6 +564,66 @@ class TypedComparatorImpl
int type_length_;
};
+template <typename DType>
+class TotalOrderComparatorImpl
+ : public TypedComparator<typename RebindLogical<DType>::DType> {
+ public:
+ using T = typename RebindLogical<DType>::c_type;
+
+ bool Compare(const T& lhs, const T& rhs) const override {
+ return std::is_lt(TotalOrderCompare(ToArrowFloat(lhs), ToArrowFloat(rhs)));
+ }
+
+ std::pair<T, T> GetMinMax(const T* values, int64_t length) const override {
+ DCHECK_GT(length, 0);
+ T min = SafeLoad(values);
+ T max = min;
+ for (int64_t value_index = 1; value_index < length; ++value_index) {
+ const T value = SafeLoad(values + value_index);
Review Comment:
What if `value` is NaN? We need to eliminate any NaN value if there are
valid bounds.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -870,18 +1112,85 @@ class TypedStatisticsImpl : public
TypedStatistics<DType> {
this->has_distinct_count_ = false;
// Null count calculation is cheap and enabled by default.
this->has_null_count_ = true;
+ // NaN counts are collected alongside floating-point bounds and enabled by
+ // default.
+ if constexpr (std::same_as<DType, FloatType> || std::same_as<DType,
DoubleType>) {
+ this->has_nan_count_ = true;
+ } else if constexpr (std::same_as<DType, FLBAType>) {
+ this->has_nan_count_ = is_half_float_;
+ } else {
+ this->has_nan_count_ = false;
+ }
+ }
+
+ template <ColumnOrder::type column_order, typename VisitValues>
+ void UpdateFloatingBoundsWithOrder(VisitValues&& visit_values, bool
update_nan_count) {
+ using ArrowFloat = decltype(ToArrowFloat(std::declval<T>()));
+
+ FloatingValueSummary<ArrowFloat, column_order> summary;
+ std::invoke(std::forward<VisitValues>(visit_values),
+ [&](const auto& value) { summary.Add(value); });
+ if (has_nan_count_ && update_nan_count) {
+ statistics_.nan_count += summary.nan_count();
+ }
+ const auto& bounds = summary.bounds();
+ if (bounds.has_value()) {
+ if constexpr (std::same_as<DType, FLBAType>) {
+ DCHECK(is_half_float_);
+ const auto min = bounds->first.ToLittleEndian();
+ const auto max = bounds->second.ToLittleEndian();
+ SetMinMaxPair({FLBA{min.data()}, FLBA{max.data()}});
+ } else {
+ SetMinMaxPair(bounds.value());
+ }
+ }
+ }
+
+ template <typename VisitValues>
+ void UpdateFloatingBounds(VisitValues&& visit_values, bool update_nan_count)
{
+ if (descr_->column_order().get_order() ==
ColumnOrder::IEEE_754_TOTAL_ORDER) {
+ UpdateFloatingBoundsWithOrder<ColumnOrder::IEEE_754_TOTAL_ORDER>(
+ std::forward<VisitValues>(visit_values), update_nan_count);
+ } else {
+ DCHECK(descr_->can_use_min_max());
+ UpdateFloatingBoundsWithOrder<ColumnOrder::TYPE_DEFINED_ORDER>(
+ std::forward<VisitValues>(visit_values), update_nan_count);
+ }
}
void SetMinMaxPair(std::pair<T, T> min_max) {
if (comparator_ == nullptr) return;
- // CleanStatistic can return a nullopt in case of erroneous values, e.g.
NaN
- auto maybe_min_max = CleanStatistic(min_max, logical_type_);
+ auto maybe_min_max =
+ descr_->column_order().get_order() == ColumnOrder::IEEE_754_TOTAL_ORDER
+ ? std::optional<std::pair<T, T>>(min_max)
+ : CleanStatistic(min_max, logical_type_);
if (!maybe_min_max) return;
auto min = maybe_min_max.value().first;
auto max = maybe_min_max.value().second;
- if (!has_min_max_) {
+ bool replace_all_nan_bounds = false;
+ if constexpr (std::same_as<DType, FloatType> || std::same_as<DType,
DoubleType> ||
+ std::same_as<DType, FLBAType>) {
+ if (descr_->column_order().get_order() ==
ColumnOrder::IEEE_754_TOTAL_ORDER) {
+ DCHECK((!std::same_as<DType, FLBAType>) || is_half_float_);
+
+ const bool min_is_nan = IsNaNValue(ToArrowFloat(min));
+ DCHECK_EQ(min_is_nan, IsNaNValue(ToArrowFloat(max)));
Review Comment:
Let's be strict here to use exception when max is not NaN.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -523,6 +564,66 @@ class TypedComparatorImpl
int type_length_;
};
+template <typename DType>
+class TotalOrderComparatorImpl
+ : public TypedComparator<typename RebindLogical<DType>::DType> {
+ public:
+ using T = typename RebindLogical<DType>::c_type;
+
+ bool Compare(const T& lhs, const T& rhs) const override {
+ return std::is_lt(TotalOrderCompare(ToArrowFloat(lhs), ToArrowFloat(rhs)));
+ }
+
+ std::pair<T, T> GetMinMax(const T* values, int64_t length) const override {
+ DCHECK_GT(length, 0);
+ T min = SafeLoad(values);
+ T max = min;
+ for (int64_t value_index = 1; value_index < length; ++value_index) {
+ const T value = SafeLoad(values + value_index);
+ min = std::is_lt(TotalOrderCompare(ToArrowFloat(value),
ToArrowFloat(min))) ? value
+
: min;
+ max = std::is_lt(TotalOrderCompare(ToArrowFloat(max),
ToArrowFloat(value))) ? value
+
: max;
+ }
+ return {min, max};
+ }
+
+ std::pair<T, T> GetMinMaxSpaced(const T* values, int64_t length,
+ const uint8_t* valid_bits,
+ int64_t valid_bits_offset) const override {
+ DCHECK_GT(length, 0);
+ T min{};
+ T max{};
+ bool has_value = false;
+ ::arrow::internal::VisitSetBitRunsVoid(
+ valid_bits, valid_bits_offset, length, [&](int64_t position, int64_t
run_length) {
+ int64_t value_index = 0;
+ if (!has_value) {
+ const T value = SafeLoad(values + position);
+ min = value;
+ max = value;
+ has_value = true;
+ value_index = 1;
+ }
+ for (; value_index < run_length; ++value_index) {
+ const T value = SafeLoad(values + position + value_index);
Review Comment:
ditto
##########
cpp/src/parquet/statistics.cc:
##########
@@ -905,6 +1214,13 @@ inline bool TypedStatisticsImpl<FLBAType>::MinMaxEqual(
template <typename DType>
bool TypedStatisticsImpl<DType>::MinMaxEqual(
const TypedStatisticsImpl<DType>& other) const {
+ if constexpr (std::same_as<T, float> || std::same_as<T, double>) {
Review Comment:
It is better to throw if two stats are of different column orders. They
cannot be compared blindly.
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -83,6 +83,11 @@ namespace parquet {
namespace {
+bool CanWriteLegacyStatistics(const ColumnDescriptor& descr) {
+ return descr.column_order().get_order() == ColumnOrder::TYPE_DEFINED_ORDER &&
Review Comment:
Let's add a comment for this. It is a little bit hard to understand.
```
// Legacy min/max fields use signed comparison; only populate them for
TypeDefinedOrder
// with signed sort order, not for IEEE total order.
```
##########
cpp/src/parquet/statistics.cc:
##########
@@ -952,6 +1282,24 @@ void TypedStatisticsImpl<DType>::UpdateSpaced(const T*
values, const uint8_t* va
IncrementNumValues(num_values);
if (num_values == 0 || comparator_ == nullptr) return;
+ if constexpr (std::same_as<DType, FloatType> || std::same_as<DType,
DoubleType> ||
+ std::same_as<DType, FLBAType>) {
+ const bool use_floating_bounds = !std::same_as<DType, FLBAType> ||
is_half_float_;
+ if (use_floating_bounds) {
+ UpdateFloatingBounds(
+ [&](auto&& visit) {
+ ::arrow::internal::VisitSetBitRunsVoid(
+ valid_bits, valid_bits_offset, num_spaced_values,
+ [&](int64_t position, int64_t run_length) {
+ for (int64_t value_index = 0; value_index < run_length;
++value_index) {
+ visit(ToArrowFloat(SafeLoad(values + position +
value_index)));
+ }
+ });
+ },
+ true);
Review Comment:
```suggestion
/*update_nan_count=*/true);
```
--
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]
