You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ap...@apache.org on 2023/02/01 15:04:46 UTC
[arrow] branch master updated: GH-33954: [C++][Parquet] Preserve field-id for nested type (#33955)
This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 984248e654 GH-33954: [C++][Parquet] Preserve field-id for nested type (#33955)
984248e654 is described below
commit 984248e6543fa7d05723c02a61a50447440ebd05
Author: Gang Wu <us...@gmail.com>
AuthorDate: Wed Feb 1 23:04:39 2023 +0800
GH-33954: [C++][Parquet] Preserve field-id for nested type (#33955)
### Rationale for this change
Function below converts arrow schema to parquet schema and preserves field-id for primitive types but loses that information for nested types.
```cpp
Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
const WriterProperties& properties,
const ArrowWriterProperties& arrow_properties, NodePtr* out);
```
In the Apache Iceberg, field-ids are assigned to all column types. We need to keep field-id of all columns.
### What changes are included in this PR?
Fix `FieldToNode` to preserve field-id of STRUCT/MAP/LIST types.
### Are these changes tested?
Added a test case in the `cpp/src/parquet/arrow/arrow_schema_test.cc`.
### Are there any user-facing changes?
No.
* Closes: #33954
Authored-by: Gang Wu <us...@gmail.com>
Signed-off-by: Antoine Pitrou <an...@python.org>
---
cpp/src/parquet/arrow/arrow_schema_test.cc | 48 ++++++++++++++++++++++++++++++
cpp/src/parquet/arrow/schema.cc | 25 ++++++++--------
2 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/cpp/src/parquet/arrow/arrow_schema_test.cc b/cpp/src/parquet/arrow/arrow_schema_test.cc
index 703b234044..3ceadc4ebc 100644
--- a/cpp/src/parquet/arrow/arrow_schema_test.cc
+++ b/cpp/src/parquet/arrow/arrow_schema_test.cc
@@ -1327,6 +1327,54 @@ TEST_F(TestConvertRoundTrip, FieldIdPreserveExisting) {
ASSERT_EQ(thrift_field_ids, expected_field_ids);
}
+TEST_F(TestConvertRoundTrip, FieldIdPreserveAllColumnTypes) {
+ std::vector<std::shared_ptr<Field>> arrow_fields;
+ arrow_fields.push_back(::arrow::field("c1", INT32, true, FieldIdMetadata(2)));
+ arrow_fields.push_back(::arrow::field("c2", DOUBLE, true, FieldIdMetadata(4)));
+ arrow_fields.push_back(::arrow::field("c3", DECIMAL_8_4, true, FieldIdMetadata(6)));
+ arrow_fields.push_back(::arrow::field("c4", UTF8, true, FieldIdMetadata(8)));
+
+ auto inner_struct =
+ ::arrow::struct_({::arrow::field("c5_1_1", UTF8, true, FieldIdMetadata(14))});
+ arrow_fields.push_back(::arrow::field(
+ "c5",
+ ::arrow::struct_({::arrow::field("c5_1", inner_struct, true, FieldIdMetadata(12)),
+ ::arrow::field("c5_2", INT64, true, FieldIdMetadata(16))}),
+ true, FieldIdMetadata(10)));
+
+ auto list_element = ::arrow::field("c6_1", UTF8, true, FieldIdMetadata(20));
+ arrow_fields.push_back(::arrow::field("c6", ::arrow::list(std::move(list_element)),
+ true, FieldIdMetadata(18)));
+
+ auto map_key = ::arrow::field("c7_1", UTF8, false, FieldIdMetadata(24));
+ auto map_value = ::arrow::field("c7_2", UTF8, true, FieldIdMetadata(26));
+ arrow_fields.push_back(::arrow::field(
+ "c7", std::make_shared<::arrow::MapType>(std::move(map_key), std::move(map_value)),
+ true, FieldIdMetadata(22)));
+
+ ASSERT_OK(RoundTripSchema(arrow_fields));
+ auto field_ids = GetFieldIdsDfs(result_schema_->fields());
+ // c7 field is an arrow map type which is in fact list<struct<key_field, value_field>>
+ // and the middle struct type does not have field-id.
+ auto expected_field_ids =
+ std::vector<int>{2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, -1, 24, 26};
+ ASSERT_EQ(field_ids, expected_field_ids);
+
+ // Parquet has a field id for the schema itself.
+ // c6 field is a three-level list type where the middle level does not have field-id
+ // c7 field is a map type which in turn is a three-level list type, too.
+ expected_field_ids =
+ std::vector<int>{-1, 2, 4, 6, 8, 10, 12, 14, 16, 18, -1, 20, 22, -1, 24, 26};
+ auto parquet_ids = GetParquetFieldIds(parquet_schema_);
+ ASSERT_EQ(parquet_ids, expected_field_ids);
+
+ // In our unit test a "not set" thrift field has a value of 0
+ expected_field_ids =
+ std::vector<int>{0, 2, 4, 6, 8, 10, 12, 14, 16, 18, 0, 20, 22, 0, 24, 26};
+ auto thrift_field_ids = GetThriftFieldIds(parquet_format_schema_);
+ ASSERT_EQ(thrift_field_ids, expected_field_ids);
+}
+
TEST(InvalidSchema, ParquetNegativeDecimalScale) {
const auto& type = ::arrow::decimal(23, -2);
const auto& field = ::arrow::field("f0", type);
diff --git a/cpp/src/parquet/arrow/schema.cc b/cpp/src/parquet/arrow/schema.cc
index 07d481ba8e..ac827bed20 100644
--- a/cpp/src/parquet/arrow/schema.cc
+++ b/cpp/src/parquet/arrow/schema.cc
@@ -81,7 +81,7 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
const ArrowWriterProperties& arrow_properties, NodePtr* out);
Status ListToNode(const std::shared_ptr<::arrow::BaseListType>& type,
- const std::string& name, bool nullable,
+ const std::string& name, bool nullable, int field_id,
const WriterProperties& properties,
const ArrowWriterProperties& arrow_properties, NodePtr* out) {
NodePtr element;
@@ -92,12 +92,12 @@ Status ListToNode(const std::shared_ptr<::arrow::BaseListType>& type,
NodePtr list = GroupNode::Make("list", Repetition::REPEATED, {element});
*out = GroupNode::Make(name, RepetitionFromNullable(nullable), {list},
- LogicalType::List());
+ LogicalType::List(), field_id);
return Status::OK();
}
Status MapToNode(const std::shared_ptr<::arrow::MapType>& type, const std::string& name,
- bool nullable, const WriterProperties& properties,
+ bool nullable, int field_id, const WriterProperties& properties,
const ArrowWriterProperties& arrow_properties, NodePtr* out) {
// TODO: Should we offer a non-compliant mode that forwards the type names?
NodePtr key_node;
@@ -111,12 +111,12 @@ Status MapToNode(const std::shared_ptr<::arrow::MapType>& type, const std::strin
NodePtr key_value =
GroupNode::Make("key_value", Repetition::REPEATED, {key_node, value_node});
*out = GroupNode::Make(name, RepetitionFromNullable(nullable), {key_value},
- LogicalType::Map());
+ LogicalType::Map(), field_id);
return Status::OK();
}
Status StructToNode(const std::shared_ptr<::arrow::StructType>& type,
- const std::string& name, bool nullable,
+ const std::string& name, bool nullable, int field_id,
const WriterProperties& properties,
const ArrowWriterProperties& arrow_properties, NodePtr* out) {
std::vector<NodePtr> children(type->num_fields());
@@ -134,7 +134,8 @@ Status StructToNode(const std::shared_ptr<::arrow::StructType>& type,
"Consider adding a dummy child field.");
}
- *out = GroupNode::Make(name, RepetitionFromNullable(nullable), std::move(children));
+ *out = GroupNode::Make(name, RepetitionFromNullable(nullable), std::move(children),
+ nullptr, field_id);
return Status::OK();
}
@@ -281,6 +282,7 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
std::shared_ptr<const LogicalType> logical_type = LogicalType::None();
ParquetType::type type;
Repetition::type repetition = RepetitionFromNullable(field->nullable());
+ int field_id = FieldIdFromMetadata(field->metadata());
int length = -1;
int precision = -1;
@@ -399,15 +401,15 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
break;
case ArrowTypeId::STRUCT: {
auto struct_type = std::static_pointer_cast<::arrow::StructType>(field->type());
- return StructToNode(struct_type, name, field->nullable(), properties,
+ return StructToNode(struct_type, name, field->nullable(), field_id, properties,
arrow_properties, out);
}
case ArrowTypeId::FIXED_SIZE_LIST:
case ArrowTypeId::LARGE_LIST:
case ArrowTypeId::LIST: {
auto list_type = std::static_pointer_cast<::arrow::BaseListType>(field->type());
- return ListToNode(list_type, name, field->nullable(), properties, arrow_properties,
- out);
+ return ListToNode(list_type, name, field->nullable(), field_id, properties,
+ arrow_properties, out);
}
case ArrowTypeId::DICTIONARY: {
// Parquet has no Dictionary type, dictionary-encoded is handled on
@@ -426,8 +428,8 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
case ArrowTypeId::MAP: {
auto map_type = std::static_pointer_cast<::arrow::MapType>(field->type());
- return MapToNode(map_type, name, field->nullable(), properties, arrow_properties,
- out);
+ return MapToNode(map_type, name, field->nullable(), field_id, properties,
+ arrow_properties, out);
}
default: {
@@ -438,7 +440,6 @@ Status FieldToNode(const std::string& name, const std::shared_ptr<Field>& field,
}
}
- int field_id = FieldIdFromMetadata(field->metadata());
PARQUET_CATCH_NOT_OK(*out = PrimitiveNode::Make(name, repetition, logical_type, type,
length, field_id));