Copilot commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r4053874639
##########
cpp/src/parquet/page_index_test.cc:
##########
@@ -18,10 +18,13 @@
#include "parquet/page_index.h"
#include <gtest/gtest.h>
+#include <algorithm>
Review Comment:
`std::popcount` is used by this test below, but this translation unit does
not include `<bit>`; the standard library does not guarantee that the
declaration arrives through the current transitive includes, so this can fail
to compile. Add the direct standard header.
##########
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) {
+ auto visit_valid_indices = [&](auto&& visit) {
+ ::arrow::internal::VisitSetBitRunsVoid(
+ values.null_bitmap_data(), values.offset(), values.length(),
+ [&](int64_t position, int64_t run_length) {
+ for (int64_t value_index = 0; value_index < run_length;
++value_index) {
+ visit(position + value_index);
+ }
+ });
+ };
Review Comment:
`VisitSetBitRunsVoid` reports positions in the underlying bitmap coordinate
system when a bitmap exists, including `values.offset()`, while
`NumericArray::Value()` is relative to the sliced array. This passes the offset
twice for sliced arrays with a validity bitmap, so floating statistics can read
the wrong elements (or go out of bounds). Normalize the run position before
invoking `visit`, while retaining the helper's position-0 behavior for a null
bitmap.
##########
cpp/src/parquet/metadata.cc:
##########
@@ -2121,16 +2077,23 @@ class FileMetaDataBuilder::FileMetaDataBuilderImpl {
metadata_->__set_version(file_version);
metadata_->__set_created_by(properties_->created_by());
- // Users cannot set the `ColumnOrder` since we do not have user defined
sort order
- // in the spec yet.
- // We always default to `TYPE_DEFINED_ORDER`. We can expose it in
- // the API once we have user defined sort orders in the Parquet format.
- // TypeDefinedOrder implies choose SortOrder based on
ConvertedType/PhysicalType
- format::TypeDefinedOrder type_defined_order;
- format::ColumnOrder column_order;
- column_order.__set_TYPE_ORDER(type_defined_order);
- column_order.__isset.TYPE_ORDER = true;
- metadata_->column_orders.resize(schema_->num_columns(), column_order);
+ metadata_->column_orders.reserve(schema_->num_columns());
+ for (int column_index = 0; column_index < schema_->num_columns();
++column_index) {
+ format::ColumnOrder column_order;
+ switch (schema_->Column(column_index)->column_order().get_order()) {
+ case ColumnOrder::TYPE_DEFINED_ORDER:
+ column_order.__set_TYPE_ORDER(format::TypeDefinedOrder{});
+ break;
+ case ColumnOrder::IEEE_754_TOTAL_ORDER:
+ column_order.__set_IEEE_754_TOTAL_ORDER(format::IEEE754TotalOrder{});
+ break;
Review Comment:
`IEEE_754_TOTAL_ORDER` is only valid for FLOAT, DOUBLE, and FLOAT16, but
this branch serializes it for any schema whose order was manually set.
`SetColumnOrder` is public, so an INT32 schema can produce a file that readers
map back to `UNKNOWN` (and statistics construction can throw). Validate
`schema::IsFloatingPointType(*schema_->Column(column_index))` before emitting
the IEEE union, or reject this order when it is assigned.
##########
cpp/src/parquet/arrow/index_test.cc:
##########
@@ -663,4 +694,67 @@ TEST_F(ParquetBloomFilterRoundTripTest, ThrowForBoolean) {
::testing::HasSubstr("BloomFilterBuilder does not support
boolean type"));
}
+TEST(ParquetPageIndex, FloatingPointOrdersInterop) {
+ auto reader = ParquetFileReader::OpenFile(
+ test::get_data_file("floating_orders_nan_count.parquet"));
Review Comment:
This new test unconditionally opens `floating_orders_nan_count.parquet` from
`PARQUET_TEST_DATA`, but no corresponding fixture or parquet-testing submodule
update is included in the proposed changes. On the normal C++ test setup,
`get_data_file` resolves only that external fixture directory, so this test
will fail with a missing-file error unless the fixture is added and the
submodule revision is updated.
--
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]
