JingsongLi commented on code in PR #9550:
URL: https://github.com/apache/paimon/pull/9550#discussion_r3921451153


##########
paimon-format/src/main/java/org/apache/paimon/format/parquet/ParquetSchemaConverter.java:
##########
@@ -490,20 +493,107 @@ public static DataField convertToPaimonField(Type 
parquetType) {
         return new DataField(parquetType.getId().intValue(), 
parquetType.getName(), paimonDataType);
     }
 
+    /** Returns true if the given group is annotated as a Parquet LIST logical 
type. */
+    public static boolean isList(GroupType listType) {
+        return listType.getLogicalTypeAnnotation()
+                instanceof LogicalTypeAnnotation.ListLogicalTypeAnnotation;
+    }
+
+    /**
+     * Returns true if the given group is a three-level Parquet list.
+     *
+     * <p>In a three-level list the immediate repeated child is a wrapper 
group whose single
+     * non-repeated child is the actual element type. This covers the 
canonical layout ({@code list
+     * -> element}) as well as legacy wrappers such as Hive's {@code bag} 
layout.
+     *
+     * <p>This corresponds to the Parquet spec's backward-compatibility 
<b>Rule 5</b>: a repeated
+     * group that contains exactly one non-repeated child is a wrapper, unless 
it matches one of
+     * Rules 1-4.
+     *
+     * <p>The compatibility encodings that are <em>not</em> three-level are:
+     *
+     * <ul>
+     *   <li><b>Rule 1</b>: the repeated field is a primitive and is itself 
the element type.
+     *   <li><b>Rule 2</b>: the repeated field is a group with multiple fields 
and is itself the
+     *       element type.
+     *   <li><b>Rule 3</b>: the repeated field is a group whose single child 
is also repeated; the
+     *       group itself is the element type.
+     *   <li><b>Rule 4</b>: the repeated field is a group named {@code 
"array"} or {@code
+     *       "<list>_tuple"} with a single child; the group itself is the 
element type.
+     * </ul>
+     *
+     * <p>See the Parquet spec: <a
+     * 
href="https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#backward-compatibility-rules";>LogicalTypes#Backward-compatibility-rules</a>
+     */
+    public static boolean isThreeLevelList(GroupType listType) {
+        if (!isList(listType)) {
+            return false;
+        }
+
+        // A list must have exactly one repeated child (the middle level).
+        if (listType.getFieldCount() != 1) {
+            return false;
+        }
+        Type middle = listType.getType(0);
+        if (middle.isPrimitive() || middle.getRepetition() != 
Type.Repetition.REPEATED) {
+            return false;
+        }
+        GroupType repeatedGroup = middle.asGroupType();
+
+        // 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) {

Review 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.



##########
paimon-format/src/main/java/org/apache/paimon/format/parquet/ParquetSchemaConverter.java:
##########
@@ -490,20 +493,107 @@ public static DataField convertToPaimonField(Type 
parquetType) {
         return new DataField(parquetType.getId().intValue(), 
parquetType.getName(), paimonDataType);
     }
 
+    /** Returns true if the given group is annotated as a Parquet LIST logical 
type. */
+    public static boolean isList(GroupType listType) {
+        return listType.getLogicalTypeAnnotation()
+                instanceof LogicalTypeAnnotation.ListLogicalTypeAnnotation;
+    }
+
+    /**
+     * Returns true if the given group is a three-level Parquet list.
+     *
+     * <p>In a three-level list the immediate repeated child is a wrapper 
group whose single
+     * non-repeated child is the actual element type. This covers the 
canonical layout ({@code list
+     * -> element}) as well as legacy wrappers such as Hive's {@code bag} 
layout.
+     *
+     * <p>This corresponds to the Parquet spec's backward-compatibility 
<b>Rule 5</b>: a repeated
+     * group that contains exactly one non-repeated child is a wrapper, unless 
it matches one of
+     * Rules 1-4.
+     *
+     * <p>The compatibility encodings that are <em>not</em> three-level are:
+     *
+     * <ul>
+     *   <li><b>Rule 1</b>: the repeated field is a primitive and is itself 
the element type.
+     *   <li><b>Rule 2</b>: the repeated field is a group with multiple fields 
and is itself the
+     *       element type.
+     *   <li><b>Rule 3</b>: the repeated field is a group whose single child 
is also repeated; the
+     *       group itself is the element type.
+     *   <li><b>Rule 4</b>: the repeated field is a group named {@code 
"array"} or {@code
+     *       "<list>_tuple"} with a single child; the group itself is the 
element type.
+     * </ul>
+     *
+     * <p>See the Parquet spec: <a
+     * 
href="https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#backward-compatibility-rules";>LogicalTypes#Backward-compatibility-rules</a>
+     */
+    public static boolean isThreeLevelList(GroupType listType) {
+        if (!isList(listType)) {
+            return false;
+        }
+
+        // A list must have exactly one repeated child (the middle level).
+        if (listType.getFieldCount() != 1) {
+            return false;
+        }
+        Type middle = listType.getType(0);
+        if (middle.isPrimitive() || middle.getRepetition() != 
Type.Repetition.REPEATED) {
+            return false;
+        }
+        GroupType repeatedGroup = middle.asGroupType();
+
+        // 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) {
+            return false;
+        }
+
+        // Rule 4: legacy "array" and "<list>_tuple" encodings are not 
wrappers; the repeated
+        // group itself is the element type.
+        return !LEGACY_LIST_ARRAY_NAME.equals(repeatedGroup.getName())
+                && !(listType.getName() + 
"_tuple").equals(repeatedGroup.getName());
+    }
+
+    /**
+     * Returns true if the given group follows the canonical three-level 
Parquet list layout ({@code
+     * list -> element}).
+     *
+     * <p>The canonical layout is described in the Parquet spec: <a
+     * 
href="https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#lists";>LogicalTypes#Lists</a>
+     */
+    public static boolean isCanonicalList(Type type) {
+        if (type.isPrimitive()) {
+            return false;
+        }
+
+        GroupType listGroup = type.asGroupType();
+        if (!isThreeLevelList(listGroup)) {
+            return false;
+        }
+
+        Type middle = listGroup.getType(0);
+        Type element = middle.asGroupType().getType(0);
+        return LIST_WRAPPER_NAME.equals(middle.getName())
+                && LIST_ELEMENT_NAME.equals(element.getName());
+    }
+
+    /**
+     * Returns the element type of the given LIST-annotated group according to 
the Parquet spec's
+     * backward-compatibility rules for lists.
+     *
+     * <p>For a three-level list (Rule 5) the returned type is the single 
child of the repeated
+     * wrapper. For Rules 1-4 the repeated field itself is returned because it 
is the element type.
+     */
     public static Type parquetListElementType(GroupType listType) {
-        int level = listType.getType(0) instanceof GroupType ? 3 : 2;
-        if (level == 3) {
-            // Level 3 representation of list type.
-            // List type should only have one middle group type, which is 
repeated, and one element
-            // type, which is optional.
+        checkArgument(
+                listType.getLogicalTypeAnnotation()
+                        instanceof 
LogicalTypeAnnotation.ListLogicalTypeAnnotation,
+                "Expected LIST-annotated group but got: %s",
+                listType);
+
+        if (isThreeLevelList(listType)) {
             return listType.getType(0).asGroupType().getType(0);
-        } else if (level == 2) {
-            // Level 2 representation of list type
-            return listType.getType(0);
-        } else {
-            throw new UnsupportedOperationException(
-                    "Parquet list type only have two level representation and 
three level representation.");
         }
+
+        return listType.getType(0);

Review 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.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to