[format] Follow Parquet list backward-compatibility rules in schema clipping - #9550
[format] Follow Parquet list backward-compatibility rules in schema clipping#9550juntaozhang wants to merge 3 commits into
Conversation
| clipParquetType(elementReadType, parquetListElementType(arrayGroup)); | ||
|
|
||
| if (level == 3) { | ||
| if (isThreeLevelList(arrayGroup)) { |
There was a problem hiding this comment.
[P1] Preserve the original Rule 2 interpretation after clipping
Rule 2 is identified from the original two-field repeated group here, but after recursive projection the rebuilt requested schema may contain only one field. ParquetReaderUtil.getArrayElementColumn then reclassifies that clipped group as a Rule 5 wrapper and unwraps it. For LIST<STRUCT<x,y>> read as ARRAY<ROW>, reader construction pairs the RowType with a PrimitiveColumnIO and throws ClassCastException. Please carry the original list interpretation into field construction, or otherwise keep Rule 2 unambiguous after projection, and add a one-field projection read test.
There was a problem hiding this comment.
Thanks! Fixed by introduced a LayoutContext that records the list-layout verdict from the file schema keyed by field path, so reader construction no longer re-guesses from the reshaped requested schema. Covered by testReadStructElementWithProjection — writes a Rule 2 LIST<STRUCT<x,y>> file and reads it back as ARRAY<ROW>.
|
|
||
| // Rule 5: the repeated group is a wrapper containing exactly one non-repeated child. | ||
| if (repeatedGroup.getFieldCount() != 1 | ||
| || repeatedGroup.getType(0).getRepetition() == Type.Repetition.REPEATED) { |
There was a problem hiding this comment.
[P1] Complete Rule 3 handling beyond element selection
This correctly keeps the repeated group as the element, but downstream schema conversion still treats its repeated child as a scalar. With the Rule 3 shape added in the tests and two inner values [8, 9], the inferred type returns only 8; using the semantically correct nested array type instead fails during clipping. Please convert the nested repeated child as an array and align clipping and ColumnIO traversal, with an end-to-end multi-value read test.
There was a problem hiding this comment.
Thanks! Fixed: the unannotated nested repeated group is now inferred as a nested ARRAY, and clipping plus ColumnIO traversal both treat its repeated child as the nested list's element. Covered end-to-end by testReadNestedLegacyList
| "Parquet list type only have two level representation and three level representation."); | ||
| } | ||
|
|
||
| return listType.getType(0); |
There was a problem hiding this comment.
[P2] Preserve required element nullability for Rules 1-4
The returned element node still has REPEATED repetition, while convertToPaimonField only applies notNull() to REQUIRED nodes. As a result, Rules 1-4 are converted to nullable Paimon elements even though the Parquet compatibility rules define them as required. Please force the converted element DataType to non-null for non-Rule-5 layouts, without changing the physical repeated Type used by clipping, and add conversion-level nullability assertions.
There was a problem hiding this comment.
Thanks! Fixed: the inferred element type is forced non-null for Rules 1-4 (Rule 5 keeps the wrapper child's own nullability). Asserted at conversion level in ParquetSchemaConverterTest and end-to-end in ParquetLegacyListReadTest.
|
Overall assessment: HIGH RISK Requirement fit: SUPPORTED I consider this PR high risk in its current form. It changes persisted Parquet schema interpretation across schema conversion, requested-schema clipping, and reader construction. Focused reproductions exposed both silent data loss in Rule 3 handling and a deterministic reader-construction failure for a Rule 2 projection, plus incorrect element nullability for Rules 1-4. Helper-shape unit tests alone are not sufficient for this compatibility-sensitive path. Please address the inline findings and add end-to-end legacy LIST read coverage before merging. |
6ef8921 to
1b75798
Compare
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: CLEAN in this review.
The earlier Rule 2 projection failure, Rule 3 lost inner values, and Rules 1–4 element-nullability findings are addressed. LayoutContext captures the layout from the original file before clipping, and reader construction uses the physical field path instead of reclassifying the projected shape. This has direct value for reading existing Parquet files produced with legacy LIST encodings.
Validation: 52 tests passed: 19 legacy-layout/schema tests, 29 Variant read/pruning tests, and four additional actual-Parquet-file probes. The latter cover empty/null nested lists, all-null struct fields after projection, and Rule 2 projection beneath nested arrays. They also exercise small reader batches. I traced the canonical-only Variant pruning guard and did not find a new actionable regression.
These were JDK 8 runs of the exact-head changed classes with cached unchanged dependencies; a full Maven/Spark/Flink compatibility matrix was not run.
JingsongLi
left a comment
There was a problem hiding this comment.
Two additional P2 findings from reviewing the current head, detailed inline. Validation: 53 targeted tests passed under JDK 8 with the normal Maven checks; separate probes confirmed the missing-field reader regression and the MAP inference issue.
| boolean threeLevel = | ||
| listLayout.isThreeLevelList(requestedGroup, groupColumnIO.getFieldPath()); |
There was a problem hiding this comment.
[P2] Use the original LIST layout when inferring fallback fields too
Reading a newly added nested field can now fail during reader construction. For a file containing s.a: ARRAY<ROW<x INT, y INT>> encoded with Rule 2, reading only s.newField enters the all-fields-missing fallback. findCheapestGroupField retains a.element.x to preserve whether s is null, and the extra-field loop above infers that clipped schema as ARRAY<INT>. However, this lookup remembers that the original list element was a struct, so constructField receives a GroupColumnIO for the inferred primitive and throws ClassCastException.
I reproduced this with an actual three-row Parquet file, including a null parent and an empty list: the baseline reader implementation passes, while this head fails at ParquetReaderUtil.java:208. Please make fallback-field type inference use the original layout as well, or preserve its logical element type during pruning, and add an end-to-end missing-nested-field projection test.
| } else if (ParquetListLayoutResolver.isLegacyNestedList(groupType)) { | ||
| paimonDataType = | ||
| new ArrayType( | ||
| convertToPaimonField(parquetListElementType(groupType)).type()); | ||
| new ArrayType(convertToPaimonField(groupType.getType(0)).type().notNull()); |
There was a problem hiding this comment.
[P2] Preserve MAP annotations before applying legacy nested-list inference
isLegacyNestedList also matches a repeated MAP group containing its single repeated key_value child. Because this branch precedes the MAP branch, a legacy list with this shape is inferred as ARRAY<ARRAY<ROW<key, value>>> instead of ARRAY<MAP<STRING, INT>>:
optional group my_list (LIST) {
repeated group array (MAP) {
repeated group key_value {
required binary key (STRING);
optional int32 value;
}
}
}
This is produced by parquet-avro's old LIST encoding: it converts array elements with REPEATED repetition and preserves that repetition for MAP elements. A converter probe confirms that the MAP node itself converted to MapType on the baseline and now converts to ArrayType. Whole-list inference was already incorrect on the baseline, so this is a gap in the new compatibility handling, rather than a claim that these complete files previously inferred correctly.
Please handle MAP annotations before this fallback, or restrict the fallback to unannotated groups, and add an inference test for an array of maps using the old Avro LIST layout.
Purpose
Follow Parquet's backward-compatibility rules for the LIST logical type when clipping Parquet schemas, so that nested variant column pruning and schema conversion correctly distinguish wrapper groups from element types across all standard and legacy list encodings:
arrayor<list>_tupleis the element type.Previously the list-structure check did not fully apply these rules, which could misidentify legacy two-level encodings as three-level lists and return the wrong element type during nested variant pruning.
Tests
mvn -pl paimon-format -DwildcardSuites=none -Dtest=ParquetSchemaConverterTest test