wgtmac commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r3873139470
##########
cpp/src/parquet/metadata.cc:
##########
@@ -932,6 +878,12 @@ class FileMetaData::FileMetaDataImpl {
auto msg = "AppendRowGroups requires equal schemas.\n" +
diff_output.str();
throw ParquetException(msg);
}
+ for (int column_index = 0; column_index < schema()->num_columns();
++column_index) {
Review Comment:
Shouldn't we check this in the above `if
(!schema()->Equals(*other->schema(), &diff_output))`?
##########
cpp/src/parquet/metadata.cc:
##########
@@ -2152,6 +2121,7 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl {
auto file_meta_data = std::unique_ptr<FileMetaData>(new FileMetaData());
file_meta_data->impl_->metadata_ = std::move(metadata_);
file_meta_data->impl_->InitSchema();
+ file_meta_data->impl_->InitColumnOrders();
Review Comment:
Does it mean that parquet-cpp has never emitted column orders?
##########
cpp/src/parquet/statistics.cc:
##########
@@ -905,6 +1218,14 @@ inline bool TypedStatisticsImpl<FLBAType>::MinMaxEqual(
template <typename DType>
bool TypedStatisticsImpl<DType>::MinMaxEqual(
const TypedStatisticsImpl<DType>& other) const {
+ if constexpr (IsOneOf<T, float, double>::value) {
+ if (descr_->column_order().get_order() ==
ColumnOrder::IEEE_754_TOTAL_ORDER) {
Review Comment:
It seems that above `TypedStatisticsImpl<FLBAType>::MinMaxEqual` uses
bit-wise comparison for float16 regardless of column order. I think this is
acceptable.
##########
cpp/src/parquet/schema.h:
##########
@@ -387,6 +387,8 @@ class PARQUET_EXPORT ColumnDescriptor {
switch (column_order().get_order()) {
case ColumnOrder::TYPE_DEFINED_ORDER:
return sort_order() != SortOrder::UNKNOWN;
+ case ColumnOrder::IEEE_754_TOTAL_ORDER:
+ return true;
Review Comment:
Should we call `IsFloatingPointType` here?
##########
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:
Fair enough, `SetMinMaxPair` will not be called publicly so this should be
fine.
##########
cpp/src/parquet/statistics.cc:
##########
@@ -812,11 +1051,15 @@ class TypedStatisticsImpl : public
TypedStatistics<DType> {
if (HasDistinctCount()) {
s.set_distinct_count(this->distinct_count());
}
+ if (HasNanCount()) {
+ s.set_nan_count(this->nan_count());
+ }
return s;
}
int64_t null_count() const override { return statistics_.null_count; }
int64_t distinct_count() const override { return statistics_.distinct_count;
}
+ int64_t nan_count() const override { return
statistics_.nan_count.value_or(0); }
Review Comment:
I still think we should just return `std::optional<int64_t>` for this. If
you insist to return int64_t, return -1 if missing seems more appropriate.
##########
cpp/src/parquet/statistics.h:
##########
@@ -89,6 +93,11 @@ class TypedComparator : public Comparator {
/// elements with accompanying bitmap indicating which elements are
/// included (bit set) and excluded (bit not set)
///
+ /// For floating-point types with ColumnOrder::TYPE_DEFINED_ORDER, NaNs are
Review Comment:
`std::pair<T, T> GetMinMax(const ::arrow::Array& values)` does not have this
comment. Is it because we have not implemented this new order to it? Should we
be explicit about this?
##########
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:
Rename it to indicate it is an interoperability test.
--
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]
