pitrou commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r3912193723
##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -393,10 +419,10 @@ std::optional<compute::Expression>
ParquetFileFragment::EvaluateStatisticsAsExpr
if (min->Equals(*max)) {
auto single_value = compute::equal(field_expr,
compute::literal(std::move(min)));
- if (!may_have_null) {
- return single_value;
+ if (is_floating_point && (!nan_count.has_value() || *nan_count != 0)) {
Review Comment:
Nit: suggest creating a `bool may_have_nan` to make these `if` clauses
clearer.
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -466,6 +468,76 @@ TEST(TestDictionaryEncoding, CannotDictDecodeBoolean) {
ASSERT_THROW(MakeDictDecoder<BooleanType>(nullptr), ParquetException);
}
+template <typename DType, typename UInt>
+void TestFloatingDictionaryBits(std::span<const UInt> bits,
Review Comment:
Can you add comments explaining what this does in its different steps?
##########
cpp/src/parquet/arrow/index_test.cc:
##########
@@ -373,18 +383,23 @@ TEST_F(ParquetPageIndexRoundTripTest, MultiplePages) {
/*min_values=*/{encode_int64(1), encode_int64(3),
encode_int64(6), ""},
/*max_values=*/{encode_int64(2), encode_int64(4),
encode_int64(6), ""},
BoundaryOrder::Ascending,
- /*null_counts=*/{0, 0, 1, 2}},
+ /*null_counts=*/{0, 0, 1, 2},
+ /*nan_counts=*/{}},
ColumnIndexObject{/*null_pages=*/{false, false, false, true},
/*min_values=*/{"a", "c", "f", ""},
/*max_values=*/{"b", "d", "f", ""},
BoundaryOrder::Ascending,
- /*null_counts=*/{0, 0, 1, 2}}));
+ /*null_counts=*/{0, 0, 1, 2}, /*nan_counts=*/{}}));
}
TEST_F(ParquetPageIndexRoundTripTest, DoubleWithNaNs) {
auto writer_properties = WriterProperties::Builder()
.enable_write_page_index()
->max_row_group_length(3) /* 3 rows per row
group */
->build();
+ auto arrow_writer_properties =
+ ArrowWriterProperties::Builder()
+ .floating_point_column_order(ColumnOrder::TYPE_DEFINED_ORDER)
Review Comment:
Why not also test `IEEE_754_TOTAL_ORDER` here?
##########
cpp/src/parquet/page_index.h:
##########
@@ -73,6 +73,15 @@ class PARQUET_EXPORT ColumnIndex {
/// available.
virtual const std::vector<int64_t>& null_counts() const = 0;
+ /// \brief Whether per-page NaN count information is available.
+ virtual bool has_nan_counts() const = 0;
+
+ /// \brief An optional vector with the number of NaN values in each data
page.
+ ///
+ /// `has_nan_counts` should be called first to determine if this information
is
+ /// available.
+ virtual const std::vector<int64_t>& nan_counts() const = 0;
+
Review Comment:
We can keep this API for consistency, or choose something more idiomatic
such as:
```c++
virtual std::optional<std::span<const uint64_t>> nan_counts() const = 0;
```
What do you think?
##########
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:
If this is a new feature, is it tested somewhere?
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1509,13 +1539,19 @@ void TestFloatStatistics<Float16LogicalType>::Init() {
template <typename T>
NodePtr TestFloatStatistics<T>::MakeNode(const std::string& name,
Repetition::type rep) {
Review Comment:
Why not optionally take a `ColumnOrder` argument here? This might make tests
easier to write.
##########
cpp/src/parquet/statistics.h:
##########
@@ -173,7 +183,7 @@ class PARQUET_EXPORT EncodedStatistics {
}
bool is_set() const {
- return has_min || has_max || has_null_count || has_distinct_count;
+ return has_min || has_max || has_null_count || has_distinct_count ||
nan_count;
Review Comment:
Nit: make it more explicit?
```suggestion
return has_min || has_max || has_null_count || has_distinct_count ||
nan_count.has_value();
```
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1670,10 +1706,85 @@ TYPED_TEST(TestFloatStatistics, NegativeZeros) {
this->TestNegativeZeroes(); }
TYPED_TEST(TestFloatStatistics, NaNs) { this->TestNaNs(); }
TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); }
+template <typename DType, typename UInt>
+void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) {
+ using T = typename DType::c_type;
+ auto node = schema::PrimitiveNode::Make("f", Repetition::REQUIRED,
DType::type_num);
+ std::static_pointer_cast<schema::PrimitiveNode>(node)->SetColumnOrder(
+ ColumnOrder::ieee_754_total_order_);
+ ColumnDescriptor descr(node, 0, 0);
+
+ const T negative_nan = SafeCopy<T>(negative_nan_bits);
+ const T positive_nan = SafeCopy<T>(positive_nan_bits);
+ const T positive_zero = T{0};
+ std::array<T, 3> mixed{negative_nan, positive_zero, positive_nan};
+ auto stats = MakeStatistics<DType>(&descr);
+ stats->Update(mixed.data(), mixed.size(), 0);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_TRUE(stats->HasMinMax());
+ ASSERT_FALSE(std::signbit(stats->min()));
+ ASSERT_FALSE(std::signbit(stats->max()));
+
+ std::array<T, 2> all_nan{positive_nan, negative_nan};
+ stats->Reset();
+ stats->Update(all_nan.data(), all_nan.size(), 0);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_EQ(negative_nan_bits, SafeCopy<UInt>(stats->min()));
+ ASSERT_EQ(positive_nan_bits, SafeCopy<UInt>(stats->max()));
+
+ auto same = MakeStatistics<DType>(&descr);
+ same->Update(all_nan.data(), all_nan.size(), 0);
+ ASSERT_TRUE(stats->Equals(*same));
+
+ auto numeric = MakeStatistics<DType>(&descr);
+ std::array<T, 1> values{T{1}};
+ numeric->Update(values.data(), values.size(), 0);
+ stats->Merge(*numeric);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_EQ(T{1}, stats->min());
+ ASSERT_EQ(T{1}, stats->max());
+}
+
+TEST(TestFloatStatistics, TotalOrder) {
+ // -qNaN(payload=1), +qNaN(payload=1).
+ TestNativeTotalOrder<FloatType>(uint32_t{0xffc00001}, uint32_t{0x7fc00001});
+ // -qNaN(payload=1), +qNaN(payload=1).
+ TestNativeTotalOrder<DoubleType>(uint64_t{0xfff8000000000001},
+ uint64_t{0x7ff8000000000001});
+}
+
+TEST(TestFloatStatistics, TotalOrderFloat16) {
+ // -qNaN(payload=1), +qNaN(payload=1).
+ BufferedFloat16 negative_nan(Float16::FromBits(0xfe01));
Review Comment:
Unrelated, but I wonder if at some point
`TypedStatistics<FixedLenByteArray>` could have a `Update` that doesn't involve
an array of `FLBA`s, e.g.:
```c++
template <int kByteLength>
virtual void Update(std::span<const std::array<uint8_t, kByteLength>
values, int64_t null_count) = 0 {
UpdateFromLinearBuffer(reinterpret_cast<const uint8_t*>(values.data()),
kByteLength, values.size(), null_count);
}
template <typename Value>
virtual std::enable_if_t<std::is_arithmetic_v<Value>> Update(
std::span<const Value> values, int64_t null_count) = 0 {
UpdateFromLinearBuffer(
reinterpret_cast<const uint8_t*>(values.data()),
static_cast<int>(sizeof(Value)), values.size(), null_count);
}
protected:
virtual void UpdateFromLinearBuffer(
const uint8_t* valyes, int value_length, int64_t num_values, int64_t
null_count) = 0;
```
##########
cpp/src/parquet/statistics.cc:
##########
@@ -349,6 +355,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) {
Review Comment:
Can we expose these functions as internal APIs somewhere
(`parquet/statistics_internal.h` perhaps?) and unit-test them directly?
##########
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) {
Review Comment:
It's... the amount of bespoke code that we have to add in this file for such
a conceptually simple addition is scary. This is going to become difficult to
maintain.
Perhaps the whole internal organization here has become impossible to work
with and we should think of something else?
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1670,10 +1706,85 @@ TYPED_TEST(TestFloatStatistics, NegativeZeros) {
this->TestNegativeZeroes(); }
TYPED_TEST(TestFloatStatistics, NaNs) { this->TestNaNs(); }
TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); }
+template <typename DType, typename UInt>
+void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) {
+ using T = typename DType::c_type;
+ auto node = schema::PrimitiveNode::Make("f", Repetition::REQUIRED,
DType::type_num);
+ std::static_pointer_cast<schema::PrimitiveNode>(node)->SetColumnOrder(
+ ColumnOrder::ieee_754_total_order_);
+ ColumnDescriptor descr(node, 0, 0);
+
+ const T negative_nan = SafeCopy<T>(negative_nan_bits);
+ const T positive_nan = SafeCopy<T>(positive_nan_bits);
+ const T positive_zero = T{0};
+ std::array<T, 3> mixed{negative_nan, positive_zero, positive_nan};
+ auto stats = MakeStatistics<DType>(&descr);
+ stats->Update(mixed.data(), mixed.size(), 0);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_TRUE(stats->HasMinMax());
+ ASSERT_FALSE(std::signbit(stats->min()));
+ ASSERT_FALSE(std::signbit(stats->max()));
+
+ std::array<T, 2> all_nan{positive_nan, negative_nan};
+ stats->Reset();
+ stats->Update(all_nan.data(), all_nan.size(), 0);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_EQ(negative_nan_bits, SafeCopy<UInt>(stats->min()));
+ ASSERT_EQ(positive_nan_bits, SafeCopy<UInt>(stats->max()));
+
+ auto same = MakeStatistics<DType>(&descr);
+ same->Update(all_nan.data(), all_nan.size(), 0);
+ ASSERT_TRUE(stats->Equals(*same));
+
+ auto numeric = MakeStatistics<DType>(&descr);
+ std::array<T, 1> values{T{1}};
+ numeric->Update(values.data(), values.size(), 0);
+ stats->Merge(*numeric);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_EQ(T{1}, stats->min());
+ ASSERT_EQ(T{1}, stats->max());
+}
+
+TEST(TestFloatStatistics, TotalOrder) {
+ // -qNaN(payload=1), +qNaN(payload=1).
+ TestNativeTotalOrder<FloatType>(uint32_t{0xffc00001}, uint32_t{0x7fc00001});
+ // -qNaN(payload=1), +qNaN(payload=1).
+ TestNativeTotalOrder<DoubleType>(uint64_t{0xfff8000000000001},
+ uint64_t{0x7ff8000000000001});
+}
+
+TEST(TestFloatStatistics, TotalOrderFloat16) {
+ // -qNaN(payload=1), +qNaN(payload=1).
+ BufferedFloat16 negative_nan(Float16::FromBits(0xfe01));
Review Comment:
And/or add a `Float16Statistics` class deriving from
`TypedStatistics<FixedLenByteArray>`, with overloads that take/return `Float16`
values?
##########
cpp/src/parquet/schema.cc:
##########
@@ -620,6 +630,11 @@ void ToParquet(const GroupNode* schema,
std::vector<format::SchemaElement>* out)
schema->VisitConst(&visitor);
}
+bool IsFloatingPointType(const ColumnDescriptor& descr) {
Review Comment:
Is this function used somewhere?
##########
cpp/src/parquet/statistics.h:
##########
@@ -78,6 +77,11 @@ class TypedComparator : public Comparator {
/// \brief Compute maximum and minimum elements in a batch of
/// elements without any nulls
+ ///
+ /// For floating-point types with ColumnOrder::TYPE_DEFINED_ORDER, NaNs are
+ /// ignored. With ColumnOrder::IEEE_754_TOTAL_ORDER, NaNs participate in the
+ /// result. IEEE total-order bounds must not be written directly as Parquet
+ /// statistics, which exclude NaNs when valid bounds exist.
Review Comment:
I don't understand the "must not be written", is it part of this API's
contract?
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1467,21 +1471,47 @@ class TestFloatStatistics : public ::testing::Test {
auto some_nan_stats = MakeStatistics<ParquetType>(descr);
// Ingesting only nans should not yield valid min max
AssertUnsetMinMax(some_nan_stats, all_nans);
+ ASSERT_EQ(std::make_optional(static_cast<int64_t>(all_nans.size())),
+ some_nan_stats->nan_count());
// Ingesting a mix of NaNs and non-NaNs should yield a valid min max.
AssertMinMaxAre(some_nan_stats, some_nans, min, max);
+ ASSERT_EQ(std::make_optional(static_cast<int64_t>(all_nans.size() + 3)),
+ some_nan_stats->nan_count());
// Ingesting only nans after a valid min/max, should have no effect
AssertMinMaxAre(some_nan_stats, all_nans, min, max);
+ ASSERT_EQ(std::make_optional(static_cast<int64_t>(all_nans.size() * 2 +
3)),
+ some_nan_stats->nan_count());
some_nan_stats = MakeStatistics<ParquetType>(descr);
AssertUnsetMinMax(some_nan_stats, all_nans, &valid_bitmap);
+ ASSERT_EQ(std::make_optional<int64_t>(7), some_nan_stats->nan_count());
Review Comment:
Why 7? We used `all_nans.size()` above.
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1467,21 +1471,47 @@ class TestFloatStatistics : public ::testing::Test {
auto some_nan_stats = MakeStatistics<ParquetType>(descr);
// Ingesting only nans should not yield valid min max
AssertUnsetMinMax(some_nan_stats, all_nans);
+ ASSERT_EQ(std::make_optional(static_cast<int64_t>(all_nans.size())),
Review Comment:
For the record, `std::make_optional` might not be required because
`std::optional<T>` can compare with arbitrary `U` if `T` can compare with `U`:
https://en.cppreference.com/cpp/utility/optional/operator_cmp
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1670,10 +1706,85 @@ TYPED_TEST(TestFloatStatistics, NegativeZeros) {
this->TestNegativeZeroes(); }
TYPED_TEST(TestFloatStatistics, NaNs) { this->TestNaNs(); }
TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); }
+template <typename DType, typename UInt>
+void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) {
+ using T = typename DType::c_type;
+ auto node = schema::PrimitiveNode::Make("f", Repetition::REQUIRED,
DType::type_num);
+ std::static_pointer_cast<schema::PrimitiveNode>(node)->SetColumnOrder(
+ ColumnOrder::ieee_754_total_order_);
+ ColumnDescriptor descr(node, 0, 0);
+
+ const T negative_nan = SafeCopy<T>(negative_nan_bits);
+ const T positive_nan = SafeCopy<T>(positive_nan_bits);
+ const T positive_zero = T{0};
+ std::array<T, 3> mixed{negative_nan, positive_zero, positive_nan};
+ auto stats = MakeStatistics<DType>(&descr);
+ stats->Update(mixed.data(), mixed.size(), 0);
Review Comment:
Nit: it might be nice to add an overload
`TypedStatistics<T>::Update(std::span<const T> values, int64_t null_count)`.
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1670,10 +1706,85 @@ TYPED_TEST(TestFloatStatistics, NegativeZeros) {
this->TestNegativeZeroes(); }
TYPED_TEST(TestFloatStatistics, NaNs) { this->TestNaNs(); }
TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); }
+template <typename DType, typename UInt>
+void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) {
Review Comment:
Make it clear this is about FP values?
```suggestion
void TestFloatNativeTotalOrder(UInt negative_nan_bits, UInt
positive_nan_bits) {
```
##########
cpp/src/arrow/dataset/file_parquet.cc:
##########
@@ -393,10 +419,10 @@ std::optional<compute::Expression>
ParquetFileFragment::EvaluateStatisticsAsExpr
if (min->Equals(*max)) {
auto single_value = compute::equal(field_expr,
compute::literal(std::move(min)));
- if (!may_have_null) {
- return single_value;
+ if (is_floating_point && (!nan_count.has_value() || *nan_count != 0)) {
Review Comment:
Or perhaps even a `with_nan` lambda just like `with_null`.
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1670,10 +1706,85 @@ TYPED_TEST(TestFloatStatistics, NegativeZeros) {
this->TestNegativeZeroes(); }
TYPED_TEST(TestFloatStatistics, NaNs) { this->TestNaNs(); }
TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); }
+template <typename DType, typename UInt>
+void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) {
+ using T = typename DType::c_type;
+ auto node = schema::PrimitiveNode::Make("f", Repetition::REQUIRED,
DType::type_num);
+ std::static_pointer_cast<schema::PrimitiveNode>(node)->SetColumnOrder(
+ ColumnOrder::ieee_754_total_order_);
+ ColumnDescriptor descr(node, 0, 0);
+
+ const T negative_nan = SafeCopy<T>(negative_nan_bits);
+ const T positive_nan = SafeCopy<T>(positive_nan_bits);
+ const T positive_zero = T{0};
+ std::array<T, 3> mixed{negative_nan, positive_zero, positive_nan};
+ auto stats = MakeStatistics<DType>(&descr);
+ stats->Update(mixed.data(), mixed.size(), 0);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_TRUE(stats->HasMinMax());
+ ASSERT_FALSE(std::signbit(stats->min()));
+ ASSERT_FALSE(std::signbit(stats->max()));
+
+ std::array<T, 2> all_nan{positive_nan, negative_nan};
+ stats->Reset();
+ stats->Update(all_nan.data(), all_nan.size(), 0);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_EQ(negative_nan_bits, SafeCopy<UInt>(stats->min()));
+ ASSERT_EQ(positive_nan_bits, SafeCopy<UInt>(stats->max()));
+
+ auto same = MakeStatistics<DType>(&descr);
+ same->Update(all_nan.data(), all_nan.size(), 0);
+ ASSERT_TRUE(stats->Equals(*same));
+
+ auto numeric = MakeStatistics<DType>(&descr);
+ std::array<T, 1> values{T{1}};
+ numeric->Update(values.data(), values.size(), 0);
+ stats->Merge(*numeric);
+ ASSERT_EQ(std::make_optional<int64_t>(2), stats->nan_count());
+ ASSERT_EQ(T{1}, stats->min());
+ ASSERT_EQ(T{1}, stats->max());
+}
+
+TEST(TestFloatStatistics, TotalOrder) {
+ // -qNaN(payload=1), +qNaN(payload=1).
+ TestNativeTotalOrder<FloatType>(uint32_t{0xffc00001}, uint32_t{0x7fc00001});
+ // -qNaN(payload=1), +qNaN(payload=1).
+ TestNativeTotalOrder<DoubleType>(uint64_t{0xfff8000000000001},
+ uint64_t{0x7ff8000000000001});
+}
+
+TEST(TestFloatStatistics, TotalOrderFloat16) {
+ // -qNaN(payload=1), +qNaN(payload=1).
+ BufferedFloat16 negative_nan(Float16::FromBits(0xfe01));
Review Comment:
That could perhaps help us get rid of the `BufferedFloat16` mess.
--
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]
