You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2024/09/06 16:26:54 UTC
[PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
wgtmac opened a new pull request, #43995:
URL: https://github.com/apache/arrow/pull/43995
### Rationale for this change
The current C++ parquet implementation interprets following parquet schema as `array<struct<array:array<int>>>, which is wrong:
```
optional group a (LIST) {
repeated group array (LIST) {
repeated int32 array;
}
}
```
### What changes are included in this PR?
According to the parquet spec, the above schema should be inferred as `array<array:array<int>>`.
### Are these changes tested?
Yes, a test case has been added to verify the fix.
### Are there any user-facing changes?
No.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1814615707
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,81 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
+ auto VerifyData = [](std::unique_ptr<ParquetFileReader> file_reader) {
+ // Expected Parquet schema of legacy two-level encoding
+ constexpr std::string_view kExpectedLegacyList =
+ "required group field_id=-1 a (List) {\n"
+ " repeated group field_id=-1 array (List) {\n"
+ " repeated int32 field_id=-1 array;\n"
+ " }\n"
+ "}\n";
+
+ // Expected Arrow schema and data
+ auto arrow_inner_list =
+ field("array", list(field("array", ::arrow::int32(), /*nullable=*/false)),
+ /*nullable=*/false);
+ auto arrow_outer_list = list(arrow_inner_list);
+ auto arrow_schema =
+ ::arrow::schema({field("a", arrow_outer_list, /*nullable=*/false)});
+ auto expected_table = TableFromJSON(arrow_schema, {R"([[[[1,2],[3,4]]]])"});
+
+ // Verify Parquet schema
+ auto root_group = file_reader->metadata()->schema()->group_node();
+ ASSERT_EQ(1, root_group->field_count());
+ std::stringstream nodeStr;
+ PrintSchema(root_group->field(0).get(), nodeStr);
+ ASSERT_EQ(kExpectedLegacyList, nodeStr.str());
+
+ // Verify Arrow schema and data
+ std::unique_ptr<FileReader> reader;
+ ASSERT_OK_NO_THROW(
+ FileReader::Make(default_memory_pool(), std::move(file_reader), &reader));
+ std::shared_ptr<Table> table;
+ ASSERT_OK(reader->ReadTable(&table));
+ AssertTablesEqual(*expected_table, *table);
+ };
+
+ // Round-trip test for Parquet C++ reader and writer
+ {
+ // Create Parquet schema of legacy two-level encoding
+ auto inner_list = GroupNode::Make("array", Repetition::REPEATED,
+ {schema::Int32("array", Repetition::REPEATED)},
+ LogicalType::List());
+ auto outer_list =
+ GroupNode::Make("a", Repetition::REQUIRED, {inner_list}, LogicalType::List());
+ auto schema_node = GroupNode::Make("schema", Repetition::REQUIRED, {outer_list});
+
+ // Create a Parquet writer to write values of nested list
+ auto sink = CreateOutputStream();
+ auto file_writer =
+ ParquetFileWriter::Open(sink, std::dynamic_pointer_cast<GroupNode>(schema_node));
+ auto row_group_writer = file_writer->AppendRowGroup();
+ auto int_writer = dynamic_cast<Int32Writer*>(row_group_writer->NextColumn());
+ ASSERT_TRUE(int_writer != nullptr);
+
+ // Directly write a single row of nested list: [[1, 2],[3, 4]]
+ constexpr int64_t kNumValues = 4;
+ constexpr std::array<int16_t, kNumValues> kRepLevels = {0, 2, 1, 2};
+ constexpr std::array<int16_t, kNumValues> kDefLevels = {2, 2, 2, 2};
+ constexpr std::array<int32_t, kNumValues> kValues = {1, 2, 3, 4};
+ int_writer->WriteBatch(kNumValues, kDefLevels.data(), kRepLevels.data(),
+ kValues.data());
+ file_writer->Close();
+ ASSERT_OK_AND_ASSIGN(auto buffer, sink->Finish());
+
+ // Read schema and verify it applies two-level encoding of list type
+ ASSERT_NO_FATAL_FAILURE(
+ VerifyData(ParquetFileReader::Open(std::make_shared<BufferReader>(buffer))));
+ }
+
+ // Interoperability test for Parquet file generated by parquet-java
Review Comment:
They have the same content so we can share the verify process in the same test case.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1823796150
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -651,45 +653,65 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
int16_t repeated_ancestor_def_level = current_levels.IncrementRepeated();
if (list_node.is_group()) {
- // Resolve 3-level encoding
- //
- // required/optional group name=whatever {
- // repeated group name=list {
- // required/optional TYPE item;
- // }
- // }
- //
- // yields list<item: TYPE ?nullable> ?nullable
- //
- // We distinguish the special case that we have
- //
- // required/optional group name=whatever {
- // repeated group name=array or $SOMETHING_tuple {
- // required/optional TYPE item;
- // }
- // }
- //
- // In this latter case, the inner type of the list should be a struct
- // rather than a primitive value
- //
- // yields list<item: struct<item: TYPE ?nullable> not null> ?nullable
const auto& list_group = static_cast<const GroupNode&>(list_node);
- // Special case mentioned in the format spec:
- // If the name is array or ends in _tuple, this should be a list of struct
- // even for single child elements.
- if (list_group.field_count() == 1 && !HasStructListName(list_group)) {
- // List of primitive type
- RETURN_NOT_OK(
- NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
- } else {
+ if (list_group.field_count() > 1) {
+ // The inner type of the list should be a struct when there are multiple fields
+ // in the repeated group
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1) {
+ const auto& repeated_field = list_group.field(0);
+ if (repeated_field->is_repeated()) {
Review Comment:
No, this does not work. It has two issues:
- MAP annotation will never happen. I have explained at https://github.com/apache/parquet-format/pull/466#issuecomment-2449039626. Same for the non-legacy three-level list.
- `ListToSchemaField()` will throw at https://github.com/apache/arrow/blob/3917b605ef1851ad036fb62856efcc83e32f8580/cpp/src/parquet/arrow/schema.cc#L635 due to the spec https://github.com/apache/parquet-format/blob/3478990aa74ffe1c76ab53e7ed8cf8f4f609c7a3/LogicalTypes.md?plain=1#L594, which is too strict. I have changed it to check for only non-legacy list.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2336589289
> I think we are just missing check of this line
This fixing itself LGTM, but I just think should we test and align more...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #43995:
URL: https://github.com/apache/arrow/pull/43995
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r2602460590
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -727,6 +780,60 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, IllegalParquetNestedSchema) {
Review Comment:
Could you please provide a simple demo to reproduce a file with such schema? It indeed looks like an invalid schema per the schema: https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists. I need to investigate if it is a missing piece.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "xhumanoid (via GitHub)" <gi...@apache.org>.
xhumanoid commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r2601296815
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -727,6 +780,60 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, IllegalParquetNestedSchema) {
Review Comment:
@wgtmac
just to clarify: what to do with old files with legacy encoding? after this patch we can't read it
now we have exact the same schema, because it's still default for parquet-java until now
https://github.com/apache/parquet-java/tree/master/parquet-avro#configuration-for-writing
`parquet.avro.write-old-list-structure`
```
Flag whether to write list structures in the old way (2 levels) or the new one (3 levels).
When writing at 2 levels no null values are available at the element level.
**The default value is true**
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1820127199
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,81 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
+ auto VerifyData = [](std::unique_ptr<ParquetFileReader> file_reader) {
+ // Expected Parquet schema of legacy two-level encoding
+ constexpr std::string_view kExpectedLegacyList =
+ "required group field_id=-1 a (List) {\n"
+ " repeated group field_id=-1 array (List) {\n"
+ " repeated int32 field_id=-1 array;\n"
+ " }\n"
+ "}\n";
+
+ // Expected Arrow schema and data
+ auto arrow_inner_list =
+ field("array", list(field("array", ::arrow::int32(), /*nullable=*/false)),
+ /*nullable=*/false);
+ auto arrow_outer_list = list(arrow_inner_list);
Review Comment:
should this be not-null as well?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1824020918
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -631,9 +633,12 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
if (group.field_count() != 1) {
return Status::Invalid("LIST-annotated groups must have a single child.");
}
- if (group.is_repeated()) {
- return Status::Invalid("LIST-annotated groups must not be repeated.");
- }
+
+ // The Parquet spec requires that LIST-annotated group cannot be repeated when
+ // it applies normal three-level encoding. We need to figure out legacy list
+ // structures and do not enforce this rule for them.
+ bool is_legacy_list_structure = true;
Review Comment:
Now I have changed the repetition check for only non-legacy list.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r2602467915
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -727,6 +780,60 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, IllegalParquetNestedSchema) {
Review Comment:
What confuses me above is that the legacy two-level list should not have `LIST` annotation but the schema above does. In addition, the above schema mixes `MAP` and `MAP_KEY_VALUE` annotation for map type.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r2602467915
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -727,6 +780,60 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, IllegalParquetNestedSchema) {
Review Comment:
What confused me is that the legacy two-level list should not have `LIST` annotation but the schema above does. In addition, the above schema mixes `MAP` and `MAP_KEY_VALUE` annotation for map type.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1823796150
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -651,45 +653,65 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
int16_t repeated_ancestor_def_level = current_levels.IncrementRepeated();
if (list_node.is_group()) {
- // Resolve 3-level encoding
- //
- // required/optional group name=whatever {
- // repeated group name=list {
- // required/optional TYPE item;
- // }
- // }
- //
- // yields list<item: TYPE ?nullable> ?nullable
- //
- // We distinguish the special case that we have
- //
- // required/optional group name=whatever {
- // repeated group name=array or $SOMETHING_tuple {
- // required/optional TYPE item;
- // }
- // }
- //
- // In this latter case, the inner type of the list should be a struct
- // rather than a primitive value
- //
- // yields list<item: struct<item: TYPE ?nullable> not null> ?nullable
const auto& list_group = static_cast<const GroupNode&>(list_node);
- // Special case mentioned in the format spec:
- // If the name is array or ends in _tuple, this should be a list of struct
- // even for single child elements.
- if (list_group.field_count() == 1 && !HasStructListName(list_group)) {
- // List of primitive type
- RETURN_NOT_OK(
- NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
- } else {
+ if (list_group.field_count() > 1) {
+ // The inner type of the list should be a struct when there are multiple fields
+ // in the repeated group
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1) {
+ const auto& repeated_field = list_group.field(0);
+ if (repeated_field->is_repeated()) {
Review Comment:
No, this does not work. It has two issues:
- MAP annotation will never happen. I have explained at https://github.com/apache/parquet-format/pull/466#issuecomment-2449039626.
- `ListToSchemaField()` will throw at https://github.com/apache/arrow/blob/3917b605ef1851ad036fb62856efcc83e32f8580/cpp/src/parquet/arrow/schema.cc#L635 due to the spec https://github.com/apache/parquet-format/blob/3478990aa74ffe1c76ab53e7ed8cf8f4f609c7a3/LogicalTypes.md?plain=1#L594
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1804308092
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,62 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
Review Comment:
Well, we can't invoke parquet-java from the C++ test suite, so that's a good reason for a file to reside in parquet-testing (also for other implementations). Is there any problem with that?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1821082820
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
The rule (4) says that `Otherwise, the repeated field's type is the element type with the repeated field's repetition.`
If I change my special schema to the one below (which does not use `array` as the element name):
```
optional group a (LIST) {
repeated group b (LIST) {
repeated int32 c;
}
}
```
Then rule 4 applies to the outer list and rule 1 applies to the inner list, which results in `list<list<int32>>` again.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1820982299
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
The rule (3) of backward-compatibility rules is that `If the repeated field is a group with one field and is named either array or uses the LIST-annotated group's name with _tuple appended then the repeated type is the element type and elements are required.`. It says that **the repeated type is the element type**.
```
optional group my_list (LIST) {
repeated group array {
required binary str (STRING);
};
}
```
So for the schema you've just mentioned above, its element type is `group array { required binary str (STRING); }` which perfectly resolves to `OneTuple<String>`.
```
optional group a (LIST) {
repeated group array (LIST) {
repeated int32 array;
}
}
```
However, for the schema I've mentioned in this issue, its element type is `group array (LIST) { repeated int32 array; }` and it perfectly resolves to `List<int32>` according to rule (1) which is `If the repeated field is not a group, then its type is the element type and elements are required.`.
The parquet-java implementation has interpreted this case in the same way: https://github.com/apache/parquet-java/blob/42cf31c0fbe4f000d4ddb1e1092c6634989ea3ca/parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java#L588
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1821098603
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
The rule 3 has following implications:
- The repeated field is a group with one field and is named array, whether or not LIST-annotated.
- The repeated field is a group with one field and uses the LIST-annotated group's name with _tuple appended.
Am I right? @emkornfield
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1824018428
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -727,6 +780,60 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, IllegalParquetNestedSchema) {
Review Comment:
This case verifies that three-level list and map cannot be nested in a legacy two-level list.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1823146507
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -651,45 +653,65 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
int16_t repeated_ancestor_def_level = current_levels.IncrementRepeated();
if (list_node.is_group()) {
- // Resolve 3-level encoding
- //
- // required/optional group name=whatever {
- // repeated group name=list {
- // required/optional TYPE item;
- // }
- // }
- //
- // yields list<item: TYPE ?nullable> ?nullable
- //
- // We distinguish the special case that we have
- //
- // required/optional group name=whatever {
- // repeated group name=array or $SOMETHING_tuple {
- // required/optional TYPE item;
- // }
- // }
- //
- // In this latter case, the inner type of the list should be a struct
- // rather than a primitive value
- //
- // yields list<item: struct<item: TYPE ?nullable> not null> ?nullable
const auto& list_group = static_cast<const GroupNode&>(list_node);
- // Special case mentioned in the format spec:
- // If the name is array or ends in _tuple, this should be a list of struct
- // even for single child elements.
- if (list_group.field_count() == 1 && !HasStructListName(list_group)) {
- // List of primitive type
- RETURN_NOT_OK(
- NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
- } else {
+ if (list_group.field_count() > 1) {
+ // The inner type of the list should be a struct when there are multiple fields
+ // in the repeated group
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1) {
+ const auto& repeated_field = list_group.field(0);
+ if (repeated_field->is_repeated()) {
Review Comment:
Thanks for the careful explanation on my last questions, after rereading I think I mostly agree that this is a bug that needs to be fixed. I think I missed the second (LIST) annotation. Which even though this check corresponds to the java, code, it seems an important factor here is that the list_group is in fact a (LIST and maybe even possibly a map), not that the inner element is repeated?
So I think the logic might make more sense as (pseudocode):
```
if (list_group.field_count() > 1) {
...
} else if (HasListElementName(list_group, group)) {
if (IsMap(list_group)) {
RETURN_NOT_OK(
ListToMapField(*list_group, current_levels, ctx, out, child_field));
} else if IsList(list_group)) {
RETURN_NOT_OK(
ListToSchemaField(*list_group, current_levels, ctx, out, child_field));
} else {
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
}
} else {
...
}
```
Does this formulation work?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2334419500
@emkornfield @pitrou @mapleFU Would you mind taking a look? Thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2341403849
> Where is this in the Parquet spec? I cannot find a [similar example](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists).
The wording of the spec is very ambigious:
> If the repeated field is not a group, then its type is the element type and elements are required.
If the repeated field is a group with multiple fields, then its type is the element type and elements are required.
If the repeated field is a group with one field and is named either array or uses the LIST-annotated group's name with _tuple appended then the repeated type is the element type and elements are required.
Otherwise, the repeated field's type is the element type with the repeated field's repetition.
I think this just following the rule(1): the repeated field is not a group, then its type is the element type and elements are required.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1820982299
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
The rule (3) of backward-compatibility rules is that `If the repeated field is a group with one field and is named either array or uses the LIST-annotated group's name with _tuple appended then the repeated type is the element type and elements are required.`. It says that **the repeated type is the element type**.
```
optional group my_list (LIST) {
repeated group array {
required binary str (STRING);
};
}
```
So for the schema you've just mentioned above, its element type is `group array { required binary str (STRING); }` which perfectly resolves to `OneTuple<String>`.
```
optional group a (LIST) {
repeated group array (LIST) {
repeated int32 array;
}
}
```
However, for the schema I've mentioned in this issue, its element type is `group array (LIST) { repeated int32 array; }` and it perfectly resolves to `List<String>` according to rule (1) which is `If the repeated field is not a group, then its type is the element type and elements are required.`.
The parquet-java implementation has interpreted this case in the same way: https://github.com/apache/parquet-java/blob/42cf31c0fbe4f000d4ddb1e1092c6634989ea3ca/parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java#L588
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1820128969
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
BTW, it looks like the HasStructListName is not correct for _tuple, as it only checks the name ends in `_tuple` and not that it is the top level list appended with `_tuple`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1856174932
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4085,6 +4085,81 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
+ auto VerifyData = [](std::unique_ptr<ParquetFileReader> file_reader) {
+ // Expected Parquet schema of legacy two-level encoding
+ constexpr std::string_view kExpectedLegacyList =
+ "required group field_id=-1 a (List) {\n"
+ " repeated group field_id=-1 array (List) {\n"
+ " repeated int32 field_id=-1 array;\n"
+ " }\n"
+ "}\n";
+
+ // Expected Arrow schema and data
+ auto arrow_inner_list =
+ field("array", list(field("array", ::arrow::int32(), /*nullable=*/false)),
+ /*nullable=*/false);
+ auto arrow_outer_list = list(arrow_inner_list);
+ auto arrow_schema =
+ ::arrow::schema({field("a", arrow_outer_list, /*nullable=*/false)});
+ auto expected_table = TableFromJSON(arrow_schema, {R"([[[[1,2],[3,4]]]])"});
+
+ // Verify Parquet schema
+ auto root_group = file_reader->metadata()->schema()->group_node();
+ ASSERT_EQ(1, root_group->field_count());
+ std::stringstream nodeStr;
+ PrintSchema(root_group->field(0).get(), nodeStr);
+ ASSERT_EQ(kExpectedLegacyList, nodeStr.str());
+
+ // Verify Arrow schema and data
+ std::unique_ptr<FileReader> reader;
+ ASSERT_OK_NO_THROW(
+ FileReader::Make(default_memory_pool(), std::move(file_reader), &reader));
+ std::shared_ptr<Table> table;
+ ASSERT_OK(reader->ReadTable(&table));
Review Comment:
Would have been nice to add a validation call here.
```c++
ASSERT_OK(table->ValidateFull());
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1814456332
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,81 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
+ auto VerifyData = [](std::unique_ptr<ParquetFileReader> file_reader) {
+ // Expected Parquet schema of legacy two-level encoding
+ constexpr std::string_view kExpectedLegacyList =
+ "required group field_id=-1 a (List) {\n"
+ " repeated group field_id=-1 array (List) {\n"
+ " repeated int32 field_id=-1 array;\n"
+ " }\n"
+ "}\n";
+
+ // Expected Arrow schema and data
+ auto arrow_inner_list =
+ field("array", list(field("array", ::arrow::int32(), /*nullable=*/false)),
+ /*nullable=*/false);
+ auto arrow_outer_list = list(arrow_inner_list);
+ auto arrow_schema =
+ ::arrow::schema({field("a", arrow_outer_list, /*nullable=*/false)});
+ auto expected_table = TableFromJSON(arrow_schema, {R"([[[[1,2],[3,4]]]])"});
+
+ // Verify Parquet schema
+ auto root_group = file_reader->metadata()->schema()->group_node();
+ ASSERT_EQ(1, root_group->field_count());
+ std::stringstream nodeStr;
+ PrintSchema(root_group->field(0).get(), nodeStr);
+ ASSERT_EQ(kExpectedLegacyList, nodeStr.str());
+
+ // Verify Arrow schema and data
+ std::unique_ptr<FileReader> reader;
+ ASSERT_OK_NO_THROW(
+ FileReader::Make(default_memory_pool(), std::move(file_reader), &reader));
+ std::shared_ptr<Table> table;
+ ASSERT_OK(reader->ReadTable(&table));
+ AssertTablesEqual(*expected_table, *table);
+ };
+
+ // Round-trip test for Parquet C++ reader and writer
+ {
+ // Create Parquet schema of legacy two-level encoding
+ auto inner_list = GroupNode::Make("array", Repetition::REPEATED,
+ {schema::Int32("array", Repetition::REPEATED)},
+ LogicalType::List());
+ auto outer_list =
+ GroupNode::Make("a", Repetition::REQUIRED, {inner_list}, LogicalType::List());
+ auto schema_node = GroupNode::Make("schema", Repetition::REQUIRED, {outer_list});
+
+ // Create a Parquet writer to write values of nested list
+ auto sink = CreateOutputStream();
+ auto file_writer =
+ ParquetFileWriter::Open(sink, std::dynamic_pointer_cast<GroupNode>(schema_node));
+ auto row_group_writer = file_writer->AppendRowGroup();
+ auto int_writer = dynamic_cast<Int32Writer*>(row_group_writer->NextColumn());
+ ASSERT_TRUE(int_writer != nullptr);
+
+ // Directly write a single row of nested list: [[1, 2],[3, 4]]
+ constexpr int64_t kNumValues = 4;
+ constexpr std::array<int16_t, kNumValues> kRepLevels = {0, 2, 1, 2};
+ constexpr std::array<int16_t, kNumValues> kDefLevels = {2, 2, 2, 2};
+ constexpr std::array<int32_t, kNumValues> kValues = {1, 2, 3, 4};
+ int_writer->WriteBatch(kNumValues, kDefLevels.data(), kRepLevels.data(),
+ kValues.data());
+ file_writer->Close();
+ ASSERT_OK_AND_ASSIGN(auto buffer, sink->Finish());
+
+ // Read schema and verify it applies two-level encoding of list type
+ ASSERT_NO_FATAL_FAILURE(
+ VerifyData(ParquetFileReader::Open(std::make_shared<BufferReader>(buffer))));
+ }
+
+ // Interoperability test for Parquet file generated by parquet-java
Review Comment:
I don't know should we use another test for this open file test. Both ok for me
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2342463156
Iām using Hive schema, so that's why it is `array<array<int>>`. The file could be easily produced by Spark Sql like below:
```
package org.example
import org.apache.spark.sql.SparkSession
object ParquetTwoLevelList {
def main(args: Array[String]): Unit = {
val spark = SparkSession.builder
.master("local[1]")
.appName("NestedListTest")
.config("spark.serializer", "org.apache.spark.serializer.KryoSerializer")
.config("spark.sql.catalog.spark_catalog", "org.apache.spark.sql.hudi.catalog.HoodieCatalog")
.config("spark.sql.extensions", "org.apache.spark.sql.hudi.HoodieSparkSessionExtension")
.config("spark.kryo.registrator", "org.apache.spark.HoodieSparkKryoRegistrar")
.getOrCreate()
spark.sql("CREATE TABLE nested_list_test (a array<array<int>>) USING HUDI")
spark.sql("INSERT INTO nested_list_test VALUES ( array(array(1,2), array(3,4)) )")
}
}
```
The parquet-cli prints the following metadata:
```
File path: /Users/gangwu/Projects/hudi-spark-generator/spark-warehouse/nested_list_test/f92ed4b5-c063-4b94-90a4-5ef997db1a6c-0_0-13-12_20240911093900996.parquet
Created by: parquet-mr version 1.12.3 (build f8dced182c4c1fbdec6ccb3185537b5a01e6ed6b)
Properties:
hoodie_bloom_filter_type_code: DYNAMIC_V0
org.apache.hudi.bloomfilter: ***
hoodie_min_record_key: 20240911093900996_0_0
parquet.avro.schema: {"type":"record","name":"nested_list_test_record","namespace":"hoodie.nested_list_test","fields":[{"name":"_hoodie_commit_time","type":["null","string"],"doc":"","default":null},{"name":"_hoodie_commit_seqno","type":["null","string"],"doc":"","default":null},{"name":"_hoodie_record_key","type":["null","string"],"doc":"","default":null},{"name":"_hoodie_partition_path","type":["null","string"],"doc":"","default":null},{"name":"_hoodie_file_name","type":["null","string"],"doc":"","default":null},{"name":"a","type":["null",{"type":"array","items":["null",{"type":"array","items":["null","int"]}]}],"default":null}]}
writer.model.name: avro
hoodie_max_record_key: 20240911093900996_0_0
Schema:
message hoodie.nested_list_test.nested_list_test_record {
optional binary _hoodie_commit_time (STRING);
optional binary _hoodie_commit_seqno (STRING);
optional binary _hoodie_record_key (STRING);
optional binary _hoodie_partition_path (STRING);
optional binary _hoodie_file_name (STRING);
optional group a (LIST) {
repeated group array (LIST) {
repeated int32 array;
}
}
}
Row group 0: count: 1 441.00 B records start: 4 total(compressed): 441 B total(uncompressed):349 B
--------------------------------------------------------------------------------
type encodings count avg size nulls min / max
_hoodie_commit_time BINARY G _ 1 68.00 B 0 "20240911093900996" / "20240911093900996"
_hoodie_commit_seqno BINARY G _ 1 72.00 B 0 "20240911093900996_0_0" / "20240911093900996_0_0"
_hoodie_record_key BINARY G _ 1 72.00 B 0 "20240911093900996_0_0" / "20240911093900996_0_0"
_hoodie_partition_path BINARY G _ 1 50.00 B 0 "" / ""
_hoodie_file_name BINARY G _ 1 116.00 B 0 "f92ed4b5-c063-4b94-90a4-5..." / "f92ed4b5-c063-4b94-90a4-5..."
a.array.array INT32 G _ 4 15.75 B 0 "1" / "4"
-------------
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1768734223
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,62 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
Review Comment:
This feature is supported by parquet-java via a writer flag `parquet.avro.write-old-list-structure`: https://github.com/apache/parquet-java/blob/08a4e7e6279f8c3b8558bd294fee4489a96d0db1/parquet-avro/src/main/java/org/apache/parquet/avro/AvroWriteSupport.java#L69 so I don't think we need to add a test file to parquet-testing repo.
Instead, I added a roundtrip test to create the file by C++ Parquet writer and then read it via the C++ Arrow Parquet reader. Does it look good to you? @pitrou @mapleFU
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2336585735
```
optional group a (LIST) {
repeated group array (LIST) {
repeated int32 array;
}
}
```
IMO, the root cause is that the current code recognizes the schema above as a three-level encoding. However, the inner-most field can only be `required` or `optional` in three-level encoding, but here the int32 field is `repeated`. We can decouple the nesting field into two lists as below:
```
outer_list:
optional group a (LIST) {
repeated group array (LIST) {}
}
inner_list:
repeated group array (LIST) {
repeated int32 array;
}
```
It is obvious that `inner_list` can simply apply backward-compatibility rule (1). For the `outer_list`, the current code applies rule (3). I think we need to apply rule (4) here by modifying the rule (3) to below:
> If the repeated field is a group with one **required or optional** field and is named either array or uses the LIST-annotated group's name with _tuple appended then the repeated type is the element type and elements are required.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2496614631
@emkornfield Thanks for your review!
I've rebased it and the test failure in `R / rhub/ubuntu-gcc12:latest` is unrelated (observed the same error from other PRs). I'll merge it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1821011195
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,81 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
+ auto VerifyData = [](std::unique_ptr<ParquetFileReader> file_reader) {
+ // Expected Parquet schema of legacy two-level encoding
+ constexpr std::string_view kExpectedLegacyList =
+ "required group field_id=-1 a (List) {\n"
+ " repeated group field_id=-1 array (List) {\n"
+ " repeated int32 field_id=-1 array;\n"
+ " }\n"
+ "}\n";
+
+ // Expected Arrow schema and data
+ auto arrow_inner_list =
+ field("array", list(field("array", ::arrow::int32(), /*nullable=*/false)),
+ /*nullable=*/false);
+ auto arrow_outer_list = list(arrow_inner_list);
Review Comment:
Indeed, `list()` cannot accept nullable parameter and it is set by `field("a", arrow_outer_list, /*nullable=*/false)` in the next line.
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,81 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
+ auto VerifyData = [](std::unique_ptr<ParquetFileReader> file_reader) {
+ // Expected Parquet schema of legacy two-level encoding
+ constexpr std::string_view kExpectedLegacyList =
+ "required group field_id=-1 a (List) {\n"
+ " repeated group field_id=-1 array (List) {\n"
+ " repeated int32 field_id=-1 array;\n"
+ " }\n"
+ "}\n";
+
+ // Expected Arrow schema and data
+ auto arrow_inner_list =
+ field("array", list(field("array", ::arrow::int32(), /*nullable=*/false)),
+ /*nullable=*/false);
+ auto arrow_outer_list = list(arrow_inner_list);
Review Comment:
Indeed. `list()` cannot accept nullable parameter and it is set by `field("a", arrow_outer_list, /*nullable=*/false)` in the next line.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2334642962
Parquet schema is too tricky for me, I'd try to take a look at https://github.com/apache/parquet-java/blob/aec7bc64dffa373db678ab2fc8b46565b4c011a5/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java#L220 tomorrow...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2336586648
> Our `ListToSchemaField` is like this part of the code https://github.com/apache/parquet-java/blob/aec7bc64dffa373db678ab2fc8b46565b4c011a5/parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java#L397-L421
>
> Should we port the impl and testings in that?
I think we are just missing check of this line: https://github.com/apache/parquet-java/blob/aec7bc64dffa373db678ab2fc8b46565b4c011a5/parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java#L588
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1823146507
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -651,45 +653,65 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
int16_t repeated_ancestor_def_level = current_levels.IncrementRepeated();
if (list_node.is_group()) {
- // Resolve 3-level encoding
- //
- // required/optional group name=whatever {
- // repeated group name=list {
- // required/optional TYPE item;
- // }
- // }
- //
- // yields list<item: TYPE ?nullable> ?nullable
- //
- // We distinguish the special case that we have
- //
- // required/optional group name=whatever {
- // repeated group name=array or $SOMETHING_tuple {
- // required/optional TYPE item;
- // }
- // }
- //
- // In this latter case, the inner type of the list should be a struct
- // rather than a primitive value
- //
- // yields list<item: struct<item: TYPE ?nullable> not null> ?nullable
const auto& list_group = static_cast<const GroupNode&>(list_node);
- // Special case mentioned in the format spec:
- // If the name is array or ends in _tuple, this should be a list of struct
- // even for single child elements.
- if (list_group.field_count() == 1 && !HasStructListName(list_group)) {
- // List of primitive type
- RETURN_NOT_OK(
- NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
- } else {
+ if (list_group.field_count() > 1) {
+ // The inner type of the list should be a struct when there are multiple fields
+ // in the repeated group
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1) {
+ const auto& repeated_field = list_group.field(0);
+ if (repeated_field->is_repeated()) {
Review Comment:
Thanks for the careful explanation on my last questions, after rereading I think I mostly agree that this is a bug that needs to be fixed. I think I missed the second (LIST) annotation. Which even though this check corresponds to the java, code, it seems an important factor here is that the list_group is in fact a (LIST and maybe even possibly a map), not that the inner element is repeated?
So I think the logic might make more sense as (pseudocode):
```
if (list_group.field_count() > 1) {
...
} else if (HasListElementName(list_group, group)) {
if (IsMap(list_group) || IsList(list_group)) {
RETURN_NOT_OK(
NodeToSchemaField(list_group, current_levels, ctx, out, child_field));
} else {
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
}
} else {
...
}
```
Does this formulation work?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1814425927
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,62 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
Review Comment:
Yes, I've just added an interop test case.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1826026018
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -1846,7 +1953,9 @@ TEST_F(TestLevels, ListErrors) {
{
::arrow::Status error = MaybeSetParquetSchema(GroupNode::Make(
"child_list", Repetition::REPEATED,
- {PrimitiveNode::Make("bool", Repetition::REPEATED, ParquetType::BOOLEAN)},
+ {GroupNode::Make("list", Repetition::REPEATED,
Review Comment:
Original case is a two-level list, which I switched to a three-level list because two-level list should allow repeated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1814377671
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4093,6 +4093,62 @@ TEST(TestArrowReaderAdHoc, OldDataPageV2) {
TryReadDataFile(path);
}
+TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
Review Comment:
Now it has been added?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1820136327
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
i'm not sure this is correct, or at least the comments above need to be updated to explain the logic further.
Specifially from the exapmles on logical type.md:
```
// List<OneTuple<String>> (nullable list, non-null elements)
optional group my_list (LIST) {
repeated group array {
required binary str (STRING);
};
}
```
This seems to imply that despite how the file was written with Avro bindings there should in fact be an intermediate struct and not a llist<list<>>, its not clear to me if this is a bug in the spec, or a bug in Avro java writer implementation.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1822262239
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
I have opened https://github.com/apache/parquet-format/pull/466 to clarify things.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2336587256
What about this line: https://github.com/apache/parquet-java/blob/aec7bc64dffa373db678ab2fc8b46565b4c011a5/parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java#L587 ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2336586198
Yes. It's so tricky, I think we can just copying the Java code directly, lol
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2335029956
I've check Java related code:
https://github.com/apache/parquet-java/blob/aec7bc64dffa373db678ab2fc8b46565b4c011a5/parquet-column/src/main/java/org/apache/parquet/schema/Type.java
https://github.com/apache/parquet-java/blob/aec7bc64dffa373db678ab2fc8b46565b4c011a5/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L145
I'll dive into it this after noon
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2342500627
@wgtmac Would you mind check testing file and add one if not exists in `parquet-testing`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2418655221
Gentle ping :) @emkornfield @pitrou @mapleFU
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2348132556
I will try to use parquet-java to create a minimal file and add it to parquet-testing. The file created by Hudi is too large due to a file-level bloom filter embedded in the file footer.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1822261145
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -513,11 +513,13 @@ Status PopulateLeaf(int column_index, const std::shared_ptr<Field>& field,
}
// Special case mentioned in the format spec:
-// If the name is array or ends in _tuple, this should be a list of struct
-// even for single child elements.
-bool HasStructListName(const GroupNode& node) {
+// If the name is array or uses the parent's name with `_tuple` appended,
+// this should be:
+// - a list of list or map type if the repeated group node is LIST- or MAP-annotated.
+// - otherwise, a list of struct even for single child elements.
+bool HasListElementName(const GroupNode& node, const GroupNode& parent) {
::std::string_view name{node.name()};
- return name == "array" || EndsWith(name, "_tuple");
+ return name == "array" || name == (parent.name() + "_tuple");
Review Comment:
@emkornfield Fix the matching of `_tuple` to follow the spec.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1821082820
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
The rule (4) says that `Otherwise, the repeated field's type is the element type with the repeated field's repetition.`
If I change my special schema to the one below (which does not use `array` as the element name):
```
optional group a (LIST) {
repeated group b (LIST) {
repeated int32 c;
}
}
```
Then rule 4 applies to the outer list and rule 1 applies to the inner list, which results in `list<list<int32>>` again.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "emkornfield (via GitHub)" <gi...@apache.org>.
emkornfield commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1823146507
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -651,45 +653,65 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
int16_t repeated_ancestor_def_level = current_levels.IncrementRepeated();
if (list_node.is_group()) {
- // Resolve 3-level encoding
- //
- // required/optional group name=whatever {
- // repeated group name=list {
- // required/optional TYPE item;
- // }
- // }
- //
- // yields list<item: TYPE ?nullable> ?nullable
- //
- // We distinguish the special case that we have
- //
- // required/optional group name=whatever {
- // repeated group name=array or $SOMETHING_tuple {
- // required/optional TYPE item;
- // }
- // }
- //
- // In this latter case, the inner type of the list should be a struct
- // rather than a primitive value
- //
- // yields list<item: struct<item: TYPE ?nullable> not null> ?nullable
const auto& list_group = static_cast<const GroupNode&>(list_node);
- // Special case mentioned in the format spec:
- // If the name is array or ends in _tuple, this should be a list of struct
- // even for single child elements.
- if (list_group.field_count() == 1 && !HasStructListName(list_group)) {
- // List of primitive type
- RETURN_NOT_OK(
- NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
- } else {
+ if (list_group.field_count() > 1) {
+ // The inner type of the list should be a struct when there are multiple fields
+ // in the repeated group
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1) {
+ const auto& repeated_field = list_group.field(0);
+ if (repeated_field->is_repeated()) {
Review Comment:
Thanks for the careful explanation on my last questions, after rereading I think I mostly agree that this is a bug that needs to be fixed. I think I missed the second (LIST) annotation. Which even though this check corresponds to the java, code, it seems an important factor here is that the list_group is in fact a (LIST and maybe even possibly a map), not that the inner element is repeated?
So I think the logic might make more sense as (pseudocode):
```
if (list_group.field_count() > 1) {
...
} else if (HasListElementName(list_group, group)) {
if (IsMap(list_group) || IsList(list_group)) {
RETURN_NOT_OK(
NodeToSchemaField(*repeated_field, current_levels, ctx, out, child_field));
} else {
RETURN_NOT_OK(GroupToStruct(list_group, current_levels, ctx, out, child_field));
}
} else {
...
}
```
Does this formulation work?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2334631531
https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists
Without legacy:
> The element field encodes the list's element type and repetition. Element repetition must be required or optional.
With backward capability:
Some existing data does not include the inner element layer. For backward-compatibility, the type of elements in LIST-annotated structures should always be determined by the following rules:
1. If the repeated field is not a group, then its type is the element type and elements are required.
2. If the repeated field is a group with multiple fields, then its type is the element type and elements are required.
3. If the repeated field is a group with one field and is named either array or uses the LIST-annotated group's name with _tuple appended then the repeated type is the element type and elements are required.
4. Otherwise, the repeated field's type is the element type with the repeated field's repetition.
So, seems this hit the (1)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2341375665
> The current C++ parquet implementation interprets following parquet schema as `array<struct<array:array>>, which is wrong:
What is "array"? Do you mean "list"? Can you fix the PR description?
> According to the parquet spec, the above schema should be inferred as `array<array<int>>`.
Where is this in the Parquet spec? I cannot find a [similar example](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists).
> I have seen an issue when reading a Parquet file created by Hudi.
1. Can we check with the Parquet ML whether this is really a legitimate schema structure?
2. If so, can we add a testing file in parquet-testing?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r1821098603
##########
cpp/src/parquet/arrow/schema.cc:
##########
@@ -681,6 +681,10 @@ Status ListToSchemaField(const GroupNode& group, LevelInfo current_levels,
// List of primitive type
RETURN_NOT_OK(
NodeToSchemaField(*list_group.field(0), current_levels, ctx, out, child_field));
+ } else if (list_group.field_count() == 1 && list_group.field(0)->is_repeated()) {
Review Comment:
The rule 3 has following implications:
- The repeated field is a group with one field and is named array, whether or not LIST-annotated.
- The repeated field is a group with one field and uses the LIST-annotated group's name with _tuple appended.
Am I right? @emkornfield
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #43995:
URL: https://github.com/apache/arrow/pull/43995#issuecomment-2497458448
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit a6fe59592c643d591ee719041dd2b5d8b9ada7d6.
There were 132 benchmark results with an error:
- Commit Run on `arm64-t4g-2xlarge-linux` at [2024-11-25 07:38:33Z](https://conbench.ursa.dev/compare/runs/1af47583560143fdb6f475165065643e...615b341ca34c45c38feb78aaaeb0ce20/)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1](https://conbench.ursa.dev/benchmark-results/06744378fb48799b80008d27dea3de17)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1](https://conbench.ursa.dev/benchmark-results/0674437b82337ea580003981212525c6)
- and 130 more (see the report linked below)
There were no benchmark performance regressions. š
The [full Conbench report](https://github.com/apache/arrow/runs/33467988364) has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-43994: [C++][Parquet] Fix schema conversion from two-level encoding nested list [arrow]
Posted by "xhumanoid (via GitHub)" <gi...@apache.org>.
xhumanoid commented on code in PR #43995:
URL: https://github.com/apache/arrow/pull/43995#discussion_r2601296815
##########
cpp/src/parquet/arrow/arrow_schema_test.cc:
##########
@@ -727,6 +780,60 @@ TEST_F(TestConvertParquetSchema, ParquetRepeatedNestedSchema) {
ASSERT_NO_FATAL_FAILURE(CheckFlatSchema(arrow_schema));
}
+TEST_F(TestConvertParquetSchema, IllegalParquetNestedSchema) {
Review Comment:
@wgtmac
just to clarify: what to do with old files with legacy encoding? after this patch we can't read it
now we have exact the same schema, because it's still default for parquet-java until now
https://github.com/apache/parquet-java/tree/master/parquet-avro#configuration-for-writing
`parquet.avro.write-old-list-structure`
```
Flag whether to write list structures in the old way (2 levels) or the new one (3 levels).
When writing at 2 levels no null values are available at the element level.
**The default value is true**
```
our schema after parquet-java
```
optional group xxx (LIST) {
repeated group array (MAP) {
repeated group key_value (MAP_KEY_VALUE) {
required binary key (STRING);
required binary value (STRING);
}
}
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org