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 6 commits into
Conversation
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.
bd13067 to
f51ffc1
Compare
| num_columns: 4 | ||
| num_rows: 3 | ||
| total_byte_size: 290 | ||
| total_byte_size: 272 |
There was a problem hiding this comment.
Legacy min/max field will not be written when using IEEE-754 order(IEEE-754 order is used by default now). So the byte size is changed.
| 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, |
There was a problem hiding this 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?
There was a problem hiding this comment.
I prefer to keep the API code style.
There was a problem hiding this comment.
This is not a style thing. It is a breaking change which we need to take it seriously.
There was a problem hiding this comment.
I checked that this code was written 7 years ago. It's true that many users may rely on it. If we think this breaking change is unaccepted in a new arrow release version, I'd prefer to add an overload, similar to #46992 (comment).
There was a problem hiding this comment.
I'd prefer to add an overload, similar to #46992 (comment).
I have push a new commit to do this.
| case ColumnOrder::IEEE_754_TOTAL_ORDER: | ||
| column_order.__set_IEEE_754_TOTAL_ORDER(format::IEEE754TotalOrder{}); | ||
| break; | ||
| case ColumnOrder::UNDEFINED: |
There was a problem hiding this comment.
If I remember correctly, ColumnOrder::UNDEFINED is a valid value for legacy Parquet files that written before TYPE_DEFINED_ORDER has been added?
There was a problem hiding this comment.
Here is writer-only code.
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.
| case ColumnOrder::IEEE_754_TOTAL_ORDER: | ||
| return true; |
There was a problem hiding this comment.
Although this is a public function, we have already checked the type before reading and writing files, so I don't think we need to do any extra checks here.
|
Run |
| int16_t max_repetition_level = 0, | ||
| const std::vector<PageLevelHistogram>& page_levels = {}) { | ||
| const bool build_size_stats = !page_levels.empty(); | ||
| const bool has_nan_counts = std::all_of( |
There was a problem hiding this comment.
nit: I guess that you don't want to change the signature here but currently has_nan_counts and has_null_counts are handled differently. It would be more readable to use consistent approach to pass or compute both of them.
| /// \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.
Why not just directly extending the above Make function? We already have several overloads now.
| virtual int64_t distinct_count() const = 0; | ||
|
|
||
| /// \brief Return true if the count of NaN values is set | ||
| virtual bool HasNanCount() const = 0; |
There was a problem hiding this comment.
Why not combine HasNanCount and nan_count just like std::optional<bool> is_min_value_exact() does?
| } | ||
| } | ||
|
|
||
| TEST(TestDictionaryEncoding, FloatingPointBits) { |
There was a problem hiding this comment.
Do we need to test float16 here?
| dictionary->WriteDict(buffer->mutable_data()); | ||
| const UInt* encoded = reinterpret_cast<const UInt*>(buffer->data()); | ||
| for (int value_index = 0; value_index < num_entries; ++value_index) { | ||
| EXPECT_EQ(bits[value_index], encoded[value_index]); |
There was a problem hiding this comment.
This is a little bit fraigle since it enforces that all distinct values should appear in the beginning of bits. Should we remove num_entries and add a const std::array<UInt, NumValues>& expected_bits to the input parameter instead?
| std::transform(bits.begin(), bits.end(), values.begin(), | ||
| [](UInt value) { return ::arrow::util::SafeCopy<T>(value); }); | ||
|
|
||
| auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, true); |
There was a problem hiding this comment.
| auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, true); | |
| auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, /*use_dictionary=*/true); |
Same apply to others
| assert_orders(&input_schema, ColumnOrder::TYPE_DEFINED_ORDER); | ||
| assert_orders(type_writer->schema(), ColumnOrder::TYPE_DEFINED_ORDER); | ||
| assert_orders(type_file->schema(), ColumnOrder::TYPE_DEFINED_ORDER); | ||
| EXPECT_THROW(ieee_file->AppendRowGroups(*type_file), ParquetException); |
| assert_orders(&input_schema, ColumnOrder::TYPE_DEFINED_ORDER); | ||
|
|
||
| auto [ieee_writer, ieee_file] = write_orders(ColumnOrder::IEEE_754_TOTAL_ORDER); | ||
| assert_orders(&input_schema, ColumnOrder::TYPE_DEFINED_ORDER); |
There was a problem hiding this comment.
Here we test that input schema has not been altered?
|
|
||
| namespace { | ||
|
|
||
| std::shared_ptr<GroupNode> MakeWriterSchema(const GroupNode& input_schema, |
There was a problem hiding this comment.
Should we remove this function? The Open function below just trust input std::shared_ptr<GroupNode> schema with all type_length and column_order. This provides the flexbility of low-level api to enable users to mix ieee754 and type_defined order for different floating point types.
Instead, we can move floating_point_column_order() to ArrowWriterProperties so we are converting Arrow schema to parquet GroupNode with expected column order at all once.
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.