GH-50689: [C++][Parquet] Add IEEE-754 total order and nan count for floating types - #50807
GH-50689: [C++][Parquet] Add IEEE-754 total order and nan count for floating types#50807HuaHuaY wants to merge 1 commit into
Conversation
github-actions
Bot
added
awaiting review
9b4091a to
5812e60
Compare
| ARROW_SUPPRESS_DEPRECATION_WARNING= \ | ||
| ARROW_UNSUPPRESS_DEPRECATION_WARNING= \ | ||
| GANDIVA_EXPORT= \ | ||
| PARQUET_DEPRECATED(x)= \ |
There was a problem hiding this comment.
Add this because I added a PARQUET_DEPRECATED at cpp/src/parquet/statistics.h and ci failed. https://github.com/apache/arrow/actions/runs/30975844655/job/92209544482
I believe this is a long-standing issue. If someone would like me to submit a separate PR to fix it, I can certainly do so.
github-actions
Bot
added
awaiting committer review
bd13067 to
f51ffc1
Compare
a306ade to
2f24bb6
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Arrow C++’s Parquet implementation to support IEEE-754 total ordering for floating-point statistics and to record NaN counts in statistics and page indexes, aligning with parquet-format changes (apache/parquet-format#514) and improving correctness for pruning, encoding, and metadata round-trips involving NaNs and signed zeros.
Changes:
- Add
ColumnOrder::IEEE_754_TOTAL_ORDERand a writer property to control floating-point column ordering (defaulting to IEEE total order). - Track and serialize
nan_countin column/page statistics and expose per-pagenan_countsviaColumnIndex. - Update statistics computation, dictionary encoding, metadata/schema handling, and dataset pruning to be NaN-aware (including special handling for FLOAT16 pruning limitations).
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/source/python/parquet.rst | Update example metadata output values to match new behavior/encoding sizes. |
| cpp/src/parquet/types.h | Add IEEE_754_TOTAL_ORDER to ColumnOrder enum and declare static instance. |
| cpp/src/parquet/types.cc | Define ColumnOrder::ieee_754_total_order_. |
| cpp/src/parquet/thrift_internal.h | Serialize/deserialize nan_count; treat IEEE order as min/max-value stats field source. |
| cpp/src/parquet/statistics.h | Extend statistics APIs and encoded state to include optional nan_count; update comparator docs. |
| cpp/src/parquet/statistics.cc | Implement IEEE total-order comparisons, NaN counting, and IEEE-aware min/max handling and merging. |
| cpp/src/parquet/statistics_test.cc | Add/extend tests for NaN counts, IEEE total order, and float16 behaviors. |
| cpp/src/parquet/schema.h | Allow can_use_min_max() for IEEE order (see review note re: non-float types). |
| cpp/src/parquet/schema.cc | Add IsFloatingPointType helper for schema/metadata decisions. |
| cpp/src/parquet/schema_test.cc | Test that IEEE column order allows min/max usage. |
| cpp/src/parquet/schema_internal.h | Expose IsFloatingPointType as a non-public schema utility. |
| cpp/src/parquet/properties.h | Add writer property floating_point_column_order with validation and plumbing. |
| cpp/src/parquet/page_index.h | Add has_nan_counts() / nan_counts() to ColumnIndex API. |
| cpp/src/parquet/page_index.cc | Persist per-page nan_counts when available for floating columns; validate vector lengths. |
| cpp/src/parquet/page_index_test.cc | Add tests covering nan_counts propagation for IEEE total-order float columns. |
| cpp/src/parquet/metadata.cc | Plumb encoded stats into Statistics::Make; enforce column-order compatibility; parse/write IEEE column order in file metadata. |
| cpp/src/parquet/file_writer.cc | Build writer schema with per-float column orders from writer properties and stabilize type_length. |
| cpp/src/parquet/file_serialize_test.cc | Add round-trip test validating floating-point column order behavior and schema stability. |
| cpp/src/parquet/encoding_test.cc | Add tests ensuring dictionary encoding preserves float bit patterns and avoids NaN hash collisions. |
| cpp/src/parquet/encoder.cc | Change float/double dictionary memoization keys to use bitwise representations for NaN payload stability. |
| cpp/src/parquet/column_writer.cc | Gate legacy min/max field population based on effective ordering; enable stats collection via can_use_min_max(). |
| cpp/src/parquet/arrow/reader_internal.cc | Decode FLOAT16 min/max statistics into HalfFloat scalars for Arrow conversion. |
| cpp/src/parquet/arrow/index_test.cc | Add nan_counts to column index round-trip expectations; add test parquet file coverage for mixed orders/nan counts. |
| cpp/src/parquet/arrow/arrow_reader_writer_test.cc | Add end-to-end test ensuring float dictionary round-trips preserve NaN payloads and signed zeros with IEEE order and page index. |
| cpp/src/arrow/dataset/file_parquet.cc | Make dataset pruning NaN-aware using nan_count; skip FLOAT16 numeric pruning until kernels exist. |
| cpp/src/arrow/dataset/file_parquet_test.cc | Add tests validating pruning expressions with/without nan_count and for FLOAT16 behavior. |
| cpp/apidoc/Doxyfile | Teach Doxygen about PARQUET_DEPRECATED macro for API docs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/src/parquet/statistics.cc:27
std::invokeis used inUpdateFloatingBoundsWithOrder, but this file doesn’t include<functional>, which is the standard header that declaresstd::invoke. This can cause build failures depending on transitive includes; add the missing include explicitly.
#include <algorithm>
#include <cmath>
#include <compare>
#include <concepts>
#include <cstring>
#include <limits>
#include <type_traits>
#include <utility>
|
@wgtmac Please take a look again. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/src/parquet/schema.cc:635
schema::IsFloatingPointTypedereferencesdescr.logical_type()without a null check.ColumnDescriptor::logical_type()can be empty (e.g. FLBA columns created viaPrimitiveNode::Makewith only physical/converted type), which would crash when this helper is called (e.g. from page index / metadata code).
bool IsFloatingPointType(const ColumnDescriptor& descr) {
return descr.physical_type() == Type::FLOAT || descr.physical_type() == Type::DOUBLE ||
descr.logical_type()->type() == LogicalType::Type::FLOAT16;
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments! Looks good to me now. I still have two comments with open questions.
| /// \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( |
There was a problem hiding this comment.
I think that's a different concern. Here I mean we don't have to add yet another overload for adding an optional nan_count because we can add it to the end of an existing one with a default value. We don't need a breaking change this time. The issue you've mentioned is to replace has_xxx and xxx with a single std::optional<int64_t> which has to be a breaking change.
| Repetition::type rep) { | ||
| return PrimitiveNode::Make(name, rep, LogicalType::Float16(), | ||
| Type::FIXED_LEN_BYTE_ARRAY, 2); | ||
| auto node = PrimitiveNode::Make(name, rep, LogicalType::Float16(), |
There was a problem hiding this comment.
nit: should we expand PrimitiveNode::Make to accept column order as well? Currently we cannot create a primitive node of floating type with type-defined-order in a single shot.
|
@pitrou Do you want to take a look? |
|
Yes, I'll take a look. Sorry! |
|
|
||
| if (!may_have_null) { | ||
| return single_value; | ||
| if (is_floating_point && (!nan_count.has_value() || *nan_count != 0)) { |
There was a problem hiding this comment.
Nit: suggest creating a bool may_have_nan to make these if clauses clearer.
There was a problem hiding this comment.
Or perhaps even a with_nan lambda just like with_null.
| ->build(); | ||
| auto arrow_writer_properties = | ||
| ArrowWriterProperties::Builder() | ||
| .floating_point_column_order(ColumnOrder::TYPE_DEFINED_ORDER) |
There was a problem hiding this comment.
Why not also test IEEE_754_TOTAL_ORDER here?
| } | ||
|
|
||
| template <typename DType, typename UInt> | ||
| void TestFloatingDictionaryBits(std::span<const UInt> bits, |
There was a problem hiding this comment.
Can you add comments explaining what this does in its different steps?
| *logical_type, min, max); | ||
| } | ||
| if (logical_type->type() == LogicalType::Type::FLOAT16) { | ||
| *min = std::make_shared<::arrow::HalfFloatScalar>( |
There was a problem hiding this comment.
If this is a new feature, is it tested somewhere?
| /// \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; | ||
|
|
There was a problem hiding this comment.
We can keep this API for consistency, or choose something more idiomatic such as:
virtual std::optional<std::span<const uint64_t>> nan_counts() const = 0;What do you think?
| 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); |
There was a problem hiding this comment.
Nit: it might be nice to add an overload TypedStatistics<T>::Update(std::span<const T> values, int64_t null_count).
| TYPED_TEST(TestFloatStatistics, Infinities) { this->TestInfinities(); } | ||
|
|
||
| template <typename DType, typename UInt> | ||
| void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) { |
There was a problem hiding this comment.
Make it clear this is about FP values?
| void TestNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) { | |
| void TestFloatNativeTotalOrder(UInt negative_nan_bits, UInt positive_nan_bits) { |
|
|
||
| TEST(TestFloatStatistics, TotalOrderFloat16) { | ||
| // -qNaN(payload=1), +qNaN(payload=1). | ||
| BufferedFloat16 negative_nan(Float16::FromBits(0xfe01)); |
There was a problem hiding this comment.
Unrelated, but I wonder if at some point TypedStatistics<FixedLenByteArray> could have a Update that doesn't involve an array of FLBAs, e.g.:
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;There was a problem hiding this comment.
That could perhaps help us get rid of the BufferedFloat16 mess.
There was a problem hiding this comment.
And/or add a Float16Statistics class deriving from TypedStatistics<FixedLenByteArray>, with overloads that take/return Float16 values?
| return lhs_bits <=> rhs_bits; | ||
| } | ||
|
|
||
| std::strong_ordering TotalOrderCompare(float lhs, float rhs) { |
There was a problem hiding this comment.
Can we expose these functions as internal APIs somewhere (parquet/statistics_internal.h perhaps?) and unit-test them directly?
|
|
||
| if (comparator_ == nullptr) return; | ||
|
|
||
| if constexpr (IsOneOf<DType, FloatType, DoubleType, FLBAType>::value) { |
There was a problem hiding this 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?
Rationale for this change
Implement IEEE 754 total order and NaN counts from apache/parquet-format#514.
What changes are included in this PR?
nan_countto statistics andnan_countsto PageIndex.Are these changes tested?
Yes.
Are there any user-facing changes?
cpp/src/parquet/types.h: AddsColumnOrder::IEEE_754_TOTAL_ORDER.cpp/src/parquet/properties.h: Adds the floating-point column-order writer property.cpp/src/parquet/schema.h: Allows column descriptors to use IEEE-ordered min/max statistics.cpp/src/parquet/page_index.h: Exposeshas_nan_counts()andnan_counts().cpp/src/parquet/statistics.h: Adds NaN fields and presence APIs toEncodedStatisticsandStatistics, and extends encoded-stateStatistics::Make/MakeStatisticsoverloads withnan_countandhas_nan_count.