wgtmac commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r3877629146
##########
cpp/src/parquet/statistics.cc:
##########
@@ -610,14 +783,16 @@ class TypedStatisticsImpl : public TypedStatistics<DType>
{
pool_(pool),
min_buffer_(AllocateBuffer(pool_, 0)),
max_buffer_(AllocateBuffer(pool_, 0)),
- logical_type_(LogicalTypeId(descr_)) {
- if (descr->sort_order() != SortOrder::UNKNOWN) {
+ logical_type_(LogicalTypeId(descr_)),
+ is_half_float_(logical_type_ == LogicalType::Type::FLOAT16) {
+ if (descr->can_use_min_max()) {
comparator_ = MakeComparator<DType>(descr);
}
TypedStatisticsImpl::Reset();
}
- // Create stats from provided values.
+ // Only used by the deprecated MakeStatistics overload. Remove it after that
Review Comment:
Mark it as deprecated?
##########
cpp/src/parquet/statistics.cc:
##########
@@ -610,14 +783,16 @@ class TypedStatisticsImpl : public TypedStatistics<DType>
{
pool_(pool),
min_buffer_(AllocateBuffer(pool_, 0)),
max_buffer_(AllocateBuffer(pool_, 0)),
- logical_type_(LogicalTypeId(descr_)) {
- if (descr->sort_order() != SortOrder::UNKNOWN) {
+ logical_type_(LogicalTypeId(descr_)),
+ is_half_float_(logical_type_ == LogicalType::Type::FLOAT16) {
Review Comment:
Can we remove `is_half_float_` by just calling `logical_type_ ==
LogicalType::Type::FLOAT16` where it is called? It looks weird because this
variable does not apply to all cases.
##########
cpp/src/arrow/dataset/file_parquet_test.cc:
##########
@@ -974,6 +975,95 @@ TEST(TestParquetStatistics, NoNullCount) {
}
}
+template <typename T>
+void TestNaNCount(const std::shared_ptr<DataType>& type,
+ const ::parquet::schema::NodePtr& parquet_node) {
+ auto field = ::arrow::field("x", type);
+ auto dataset_schema = ::arrow::schema({field});
+ ::parquet::ColumnDescriptor descr(parquet_node, 0, 0);
Review Comment:
These test cases still use type_defined_order and do not cover the new
ieee754 total order and do not actually test the filtering logic. Does it make
sense to directly use `floating_orders_nan_count.parquet` from parquet-testing
for a real e2e interoperability test?
##########
cpp/src/parquet/statistics.cc:
##########
@@ -870,18 +1115,86 @@ 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, FLBAType>) {
Review Comment:
Why can't we call reset in all cases?
##########
cpp/src/parquet/statistics_test.cc:
##########
@@ -1670,6 +1698,82 @@ 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 negative_zero = -T{0};
+ const T positive_zero = T{0};
+ std::array<T, 4> mixed{negative_nan, positive_zero, negative_zero,
positive_nan};
Review Comment:
It might be better if we just have only positive_zero or negative_zero but
not both to verify that no adjustment is done for zero signedness. Same for
float16 test below.
##########
cpp/src/parquet/arrow/index_test.cc:
##########
@@ -663,4 +675,67 @@ TEST_F(ParquetBloomFilterRoundTripTest, ThrowForBoolean) {
::testing::HasSubstr("BloomFilterBuilder does not support
boolean type"));
}
+TEST(ParquetPageIndex, FloatingPointOrders) {
Review Comment:
BTW, I think this test case is a little bit weak. This file was added by
https://github.com/apache/parquet-testing/pull/104 and it contains following
attributes:
```
ยป parquet-cli meta
target/parquet-testing/data/floating_orders_nan_count.parquet
File path: data/floating_orders_nan_count.parquet
Created by: parquet-mr version 1.18.0-SNAPSHOT (build
c5dcd8ca5bad5fde9c797b876a16b5bf3b9206c0)
Properties:
original.created.by: parquet-mr version 1.18.0-SNAPSHOT (build
c5dcd8ca5bad5fde9c797b876a16b5bf3b9206c0)
writer.model.name: example
Schema:
message msg {
required float float_ieee754;
required float float_typedef;
required double double_ieee754;
required double double_typedef;
required fixed_len_byte_array(2) float16_ieee754 (FLOAT16);
required fixed_len_byte_array(2) float16_typedef (FLOAT16);
}
Row group 0: count: 10 42.20 B records start: 4 total(compressed): 422 B
total(uncompressed):422 B
--------------------------------------------------------------------------------
type encodings count avg size nulls min / max
float_ieee754 FLOAT _ _ 10 6.30 B 0 "-2.0" /
"5.0"
float_typedef FLOAT _ _ 10 6.30 B 0 "-2.0" /
"5.0"
double_ieee754 DOUBLE _ _ 10 10.50 B 0 "-2.0" /
"5.0"
double_typedef DOUBLE _ _ 10 10.50 B 0 "-2.0" /
"5.0"
float16_ieee754 FIXED[2] _ _ 10 4.30 B 0 "-2.0" / "5.0"
float16_typedef FIXED[2] _ _ 10 4.30 B 0 "-2.0" / "5.0"
Row group 1: count: 10 42.20 B records start: 426 total(compressed): 422
B total(uncompressed):422 B
--------------------------------------------------------------------------------
type encodings count avg size nulls min / max
float_ieee754 FLOAT _ _ 10 6.30 B 0 "-2.0" /
"3.0"
float_typedef FLOAT _ _ 10 6.30 B 0
double_ieee754 DOUBLE _ _ 10 10.50 B 0 "-2.0" /
"3.0"
double_typedef DOUBLE _ _ 10 10.50 B 0
float16_ieee754 FIXED[2] _ _ 10 4.30 B 0 "-2.0" / "3.0"
float16_typedef FIXED[2] _ _ 10 4.30 B 0
Row group 2: count: 10 42.20 B records start: 848 total(compressed): 422
B total(uncompressed):422 B
--------------------------------------------------------------------------------
type encodings count avg size nulls min / max
float_ieee754 FLOAT _ _ 10 6.30 B 0 "NaN" /
"NaN"
float_typedef FLOAT _ _ 10 6.30 B 0
double_ieee754 DOUBLE _ _ 10 10.50 B 0 "NaN" /
"NaN"
double_typedef DOUBLE _ _ 10 10.50 B 0
float16_ieee754 FIXED[2] _ _ 10 4.30 B 0 "NaN" / "NaN"
float16_typedef FIXED[2] _ _ 10 4.30 B 0
Row group 3: count: 10 42.20 B records start: 1270 total(compressed):
422 B total(uncompressed):422 B
--------------------------------------------------------------------------------
type encodings count avg size nulls min / max
float_ieee754 FLOAT _ _ 10 6.30 B 0 "0.0" /
"5.0"
float_typedef FLOAT _ _ 10 6.30 B 0 "-0.0" /
"5.0"
double_ieee754 DOUBLE _ _ 10 10.50 B 0 "0.0" /
"5.0"
double_typedef DOUBLE _ _ 10 10.50 B 0 "-0.0" /
"5.0"
float16_ieee754 FIXED[2] _ _ 10 4.30 B 0 "0.0" / "5.0"
float16_typedef FIXED[2] _ _ 10 4.30 B 0 "-0.0" / "5.0"
Row group 4: count: 10 42.20 B records start: 1692 total(compressed):
422 B total(uncompressed):422 B
--------------------------------------------------------------------------------
type encodings count avg size nulls min / max
float_ieee754 FLOAT _ _ 10 6.30 B 0 "-5.0" /
"-0.0"
float_typedef FLOAT _ _ 10 6.30 B 0 "-5.0" /
"0.0"
double_ieee754 DOUBLE _ _ 10 10.50 B 0 "-5.0" /
"-0.0"
double_typedef DOUBLE _ _ 10 10.50 B 0 "-5.0" /
"0.0"
float16_ieee754 FIXED[2] _ _ 10 4.30 B 0 "-5.0" /
"-0.0"
float16_typedef FIXED[2] _ _ 10 4.30 B 0 "-5.0" / "0.0"
```
It would be better to verify column chunk stats and page index of every row
group and column are correctly parsed.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -870,18 +1115,86 @@ 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, FLBAType>) {
+ if (!is_half_float_) {
+ this->statistics_.nan_count.reset();
+ }
+ } else if constexpr (!IsOneOf<DType, FloatType, DoubleType>::value) {
+ this->statistics_.nan_count.reset();
+ }
+ }
+
+ 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 (HasNanCount() && update_nan_count) {
Review Comment:
Should we clear it if `update_nan_count` is false?
--
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]
