wgtmac commented on code in PR #50807:
URL: https://github.com/apache/arrow/pull/50807#discussion_r3869837838
##########
cpp/src/parquet/statistics.h:
##########
@@ -259,6 +274,29 @@ class PARQUET_EXPORT Statistics {
std::optional<bool> is_max_value_exact,
::arrow::MemoryPool* pool = ::arrow::default_memory_pool());
+ /// \brief Create a new statistics instance given a column schema
+ /// definition and preexisting state
+ /// \param[in] descr the column schema
+ /// \param[in] encoded_min the encoded minimum value
+ /// \param[in] encoded_max the encoded maximum value
+ /// \param[in] num_values total number of values
+ /// \param[in] null_count number of null values
+ /// \param[in] distinct_count number of distinct values
+ /// \param[in] nan_count number of NaN values, if available
+ /// \param[in] has_min_max whether the min/max statistics are set
+ /// \param[in] has_null_count whether the null_count statistics are set
+ /// \param[in] has_distinct_count whether the distinct_count statistics are
set
+ /// \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(
Review Comment:
Why not just directly extending the above Make function? We already have
several overloads now.
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -466,6 +469,70 @@ TEST(TestDictionaryEncoding, CannotDictDecodeBoolean) {
ASSERT_THROW(MakeDictDecoder<BooleanType>(nullptr), ParquetException);
}
+template <typename DType, typename UInt, size_t NumValues>
+void TestFloatingDictionaryBits(const std::array<UInt, NumValues>& bits,
+ int num_entries) {
+ using T = typename DType::c_type;
+ std::array<T, NumValues> values;
+ std::transform(bits.begin(), bits.end(), values.begin(),
+ [](UInt value) { return ::arrow::util::SafeCopy<T>(value); });
+
+ auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, true);
Review Comment:
```suggestion
auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN,
/*use_dictionary=*/true);
```
Same apply to others
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -466,6 +469,70 @@ TEST(TestDictionaryEncoding, CannotDictDecodeBoolean) {
ASSERT_THROW(MakeDictDecoder<BooleanType>(nullptr), ParquetException);
}
+template <typename DType, typename UInt, size_t NumValues>
+void TestFloatingDictionaryBits(const std::array<UInt, NumValues>& bits,
+ int num_entries) {
+ using T = typename DType::c_type;
+ std::array<T, NumValues> values;
+ std::transform(bits.begin(), bits.end(), values.begin(),
+ [](UInt value) { return ::arrow::util::SafeCopy<T>(value); });
+
+ auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, true);
+ auto dictionary = dynamic_cast<DictEncoder<DType>*>(encoder.get());
+ ASSERT_NE(nullptr, dictionary);
+ encoder->Put(values.data(), values.size());
+ ASSERT_EQ(num_entries, dictionary->num_entries());
+
+ auto buffer = AllocateBuffer(default_memory_pool(),
dictionary->dict_encoded_size());
+ 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]);
Review 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?
##########
cpp/src/parquet/statistics.h:
##########
@@ -279,6 +317,12 @@ class PARQUET_EXPORT Statistics {
/// \brief The number of distinct values, may not be set
virtual int64_t distinct_count() const = 0;
+ /// \brief Return true if the count of NaN values is set
+ virtual bool HasNanCount() const = 0;
Review Comment:
Why not combine `HasNanCount` and `nan_count` just like `std::optional<bool>
is_min_value_exact()` does?
##########
cpp/src/parquet/file_serialize_test.cc:
##########
@@ -480,6 +482,67 @@ TEST(ParquetRoundtrip, AllNulls) {
EXPECT_THAT(def_levels, ElementsAre(0, 0, 0));
}
+TEST(TestFileWriter, FloatingPointColumnOrder) {
+ schema::NodeVector fields{
+ schema::Float("float", Repetition::REQUIRED),
+ schema::Double("double", Repetition::REQUIRED),
+ schema::PrimitiveNode::Make("float16", Repetition::REQUIRED,
LogicalType::Float16(),
+ Type::FIXED_LEN_BYTE_ARRAY, 2),
+ schema::Int32("int", Repetition::REQUIRED)};
+
+ auto schema = std::static_pointer_cast<GroupNode>(
+ GroupNode::Make("schema", Repetition::REQUIRED, fields));
+
+ auto assert_type_lengths = [](const SchemaDescriptor* schema) {
+ ASSERT_EQ(-1, schema->Column(0)->type_length());
+ ASSERT_EQ(-1, schema->Column(1)->type_length());
+ ASSERT_EQ(2, schema->Column(2)->type_length());
+ ASSERT_EQ(-1, schema->Column(3)->type_length());
+ };
+
+ auto write_orders = [&](ColumnOrder::type order) {
+ auto properties =
+
WriterProperties::Builder().floating_point_column_order(order)->build();
+ auto sink = CreateOutputStream();
+ auto writer = ParquetFileWriter::Open(sink, schema, properties);
+ assert_type_lengths(writer->schema());
+ writer->Close();
+ auto writer_metadata = writer->metadata();
+ PARQUET_ASSIGN_OR_THROW(auto buffer, sink->Finish());
+ auto file_metadata =
+ ParquetFileReader::Open(
+ std::make_shared<::arrow::io::BufferReader>(std::move(buffer)))
+ ->metadata();
+ return std::pair{std::move(writer_metadata), std::move(file_metadata)};
+ };
+
+ auto assert_orders = [](const SchemaDescriptor* schema,
+ ColumnOrder::type floating_point_order) {
+ for (int column_index = 0; column_index < 3; ++column_index) {
+ ASSERT_EQ(floating_point_order,
+ schema->Column(column_index)->column_order().get_order());
+ }
+ ASSERT_EQ(ColumnOrder::TYPE_DEFINED_ORDER,
+ schema->Column(3)->column_order().get_order());
+ };
+
+ SchemaDescriptor input_schema;
+ input_schema.Init(schema);
+ assert_type_lengths(&input_schema);
+ 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);
+ assert_orders(ieee_writer->schema(), ColumnOrder::IEEE_754_TOTAL_ORDER);
+ assert_orders(ieee_file->schema(), ColumnOrder::IEEE_754_TOTAL_ORDER);
+
+ auto [type_writer, type_file] =
write_orders(ColumnOrder::TYPE_DEFINED_ORDER);
+ 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);
Review Comment:
nit: check error message
##########
cpp/src/parquet/file_writer.cc:
##########
@@ -335,15 +338,46 @@ class RowGroupSerializer : public
RowGroupWriter::Contents {
// An implementation of ParquetFileWriter::Contents that deals with the Parquet
// file structure, Thrift serialization, and other internal matters
+namespace {
+
+std::shared_ptr<GroupNode> MakeWriterSchema(const GroupNode& input_schema,
Review 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.
##########
cpp/src/parquet/page_index_test.cc:
##########
@@ -506,6 +509,9 @@ void TestWriteTypedColumnIndex(schema::NodePtr node,
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(
Review 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.
##########
cpp/src/parquet/encoding_test.cc:
##########
@@ -466,6 +469,70 @@ TEST(TestDictionaryEncoding, CannotDictDecodeBoolean) {
ASSERT_THROW(MakeDictDecoder<BooleanType>(nullptr), ParquetException);
}
+template <typename DType, typename UInt, size_t NumValues>
+void TestFloatingDictionaryBits(const std::array<UInt, NumValues>& bits,
+ int num_entries) {
+ using T = typename DType::c_type;
+ std::array<T, NumValues> values;
+ std::transform(bits.begin(), bits.end(), values.begin(),
+ [](UInt value) { return ::arrow::util::SafeCopy<T>(value); });
+
+ auto encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, true);
+ auto dictionary = dynamic_cast<DictEncoder<DType>*>(encoder.get());
+ ASSERT_NE(nullptr, dictionary);
+ encoder->Put(values.data(), values.size());
+ ASSERT_EQ(num_entries, dictionary->num_entries());
+
+ auto buffer = AllocateBuffer(default_memory_pool(),
dictionary->dict_encoded_size());
+ 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]);
+ }
+
+ using ArrowType = std::conditional_t<std::is_same_v<DType, FloatType>,
+ ::arrow::FloatType,
::arrow::DoubleType>;
+ typename ::arrow::TypeTraits<ArrowType>::BuilderType builder;
+ ASSERT_OK(
+ builder.AppendValues(std::vector<T>(values.begin(), values.begin() +
num_entries)));
+ std::shared_ptr<::arrow::Array> values_array;
+ ASSERT_OK(builder.Finish(&values_array));
+ auto direct_encoder = MakeTypedEncoder<DType>(Encoding::PLAIN, true);
+ auto direct_dictionary =
dynamic_cast<DictEncoder<DType>*>(direct_encoder.get());
+ ASSERT_NE(nullptr, direct_dictionary);
+ direct_dictionary->PutDictionary(*values_array);
+ ASSERT_EQ(num_entries, direct_dictionary->num_entries());
+ auto direct_buffer =
+ AllocateBuffer(default_memory_pool(),
direct_dictionary->dict_encoded_size());
+ direct_dictionary->WriteDict(direct_buffer->mutable_data());
+ const UInt* direct_encoded = reinterpret_cast<const
UInt*>(direct_buffer->data());
+ for (int value_index = 0; value_index < num_entries; ++value_index) {
+ EXPECT_EQ(bits[value_index], direct_encoded[value_index]);
+ }
+}
+
+TEST(TestDictionaryEncoding, FloatingPointBits) {
Review Comment:
Do we need to test float16 here?
##########
cpp/src/parquet/file_serialize_test.cc:
##########
@@ -480,6 +482,67 @@ TEST(ParquetRoundtrip, AllNulls) {
EXPECT_THAT(def_levels, ElementsAre(0, 0, 0));
}
+TEST(TestFileWriter, FloatingPointColumnOrder) {
+ schema::NodeVector fields{
+ schema::Float("float", Repetition::REQUIRED),
+ schema::Double("double", Repetition::REQUIRED),
+ schema::PrimitiveNode::Make("float16", Repetition::REQUIRED,
LogicalType::Float16(),
+ Type::FIXED_LEN_BYTE_ARRAY, 2),
+ schema::Int32("int", Repetition::REQUIRED)};
+
+ auto schema = std::static_pointer_cast<GroupNode>(
+ GroupNode::Make("schema", Repetition::REQUIRED, fields));
+
+ auto assert_type_lengths = [](const SchemaDescriptor* schema) {
+ ASSERT_EQ(-1, schema->Column(0)->type_length());
+ ASSERT_EQ(-1, schema->Column(1)->type_length());
+ ASSERT_EQ(2, schema->Column(2)->type_length());
+ ASSERT_EQ(-1, schema->Column(3)->type_length());
+ };
+
+ auto write_orders = [&](ColumnOrder::type order) {
+ auto properties =
+
WriterProperties::Builder().floating_point_column_order(order)->build();
+ auto sink = CreateOutputStream();
+ auto writer = ParquetFileWriter::Open(sink, schema, properties);
+ assert_type_lengths(writer->schema());
+ writer->Close();
+ auto writer_metadata = writer->metadata();
+ PARQUET_ASSIGN_OR_THROW(auto buffer, sink->Finish());
+ auto file_metadata =
+ ParquetFileReader::Open(
+ std::make_shared<::arrow::io::BufferReader>(std::move(buffer)))
+ ->metadata();
+ return std::pair{std::move(writer_metadata), std::move(file_metadata)};
+ };
+
+ auto assert_orders = [](const SchemaDescriptor* schema,
+ ColumnOrder::type floating_point_order) {
+ for (int column_index = 0; column_index < 3; ++column_index) {
+ ASSERT_EQ(floating_point_order,
+ schema->Column(column_index)->column_order().get_order());
+ }
+ ASSERT_EQ(ColumnOrder::TYPE_DEFINED_ORDER,
+ schema->Column(3)->column_order().get_order());
+ };
+
+ SchemaDescriptor input_schema;
+ input_schema.Init(schema);
+ assert_type_lengths(&input_schema);
+ 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);
Review Comment:
Here we test that input schema has not been altered?
--
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]
