Re: [PR] (parquet-avro) Automatically detect list encodings in AvroReadSupport [parquet-java]

["wgtmac (via GitHub)" <gi...@apache.org>]

wgtmac commented on code in PR #3753:
URL: https://github.com/apache/parquet-java/pull/3753#discussion_r3921973251


##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroReadSupport.java:
##########
@@ -128,6 +136,18 @@ public ReadContext init(
     MessageType projection = fileSchema;
     Map<String, String> metadata = new LinkedHashMap<String, String>();
 
+    boolean autoDetectListStructure =
+        configuration.getBoolean(AUTO_DETECT_LIST_STRUCTURE, AUTO_DETECT_LIST_STRUCTURE_DEFAULT);
+
+    if (autoDetectListStructure
+        && configuration.get(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE) == null
+        && configuration.get(AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS) == null) {
+      if (writesNewListStructure(fileSchema)) {
+        configuration.setBoolean(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE, false);

Review Comment:
   This writes the inferred mode back into the shared Configuration. ParquetReader reuses that configuration across files, so after a 3-level file, a later 2-level file (especially with a projection) is still converted as 3-level. Could this stay per-file/read-context instead of mutating the caller's configuration?



##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroReadSupport.java:
##########
@@ -128,6 +136,18 @@ public ReadContext init(
     MessageType projection = fileSchema;
     Map<String, String> metadata = new LinkedHashMap<String, String>();
 
+    boolean autoDetectListStructure =
+        configuration.getBoolean(AUTO_DETECT_LIST_STRUCTURE, AUTO_DETECT_LIST_STRUCTURE_DEFAULT);
+
+    if (autoDetectListStructure
+        && configuration.get(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE) == null

Review Comment:
   Is it a good time to make a shift on the default value of `AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE`? It has caused a lot of troubles.



##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroReadSupport.java:
##########
@@ -230,4 +250,39 @@ private GenericData getDataModel(ParquetConfiguration conf, Schema schema) {
     return ReflectionUtils.newInstance(suppClass, ConfigurationUtil.createHadoopConfiguration(conf))
         .get();
   }
+
+  private static boolean writesNewListStructure(MessageType schema) {
+    return Boolean.TRUE.equals(allListStructuresAreThreeLevel(schema));
+  }
+
+  // Given a Parquet schema, return true only if the schema:
+  // - contains one or more List fields
+  // - encodes every List field using 3-level list structure
+  private static Boolean allListStructuresAreThreeLevel(Type type) {
+    if (type.isPrimitive()) {
+      return null;
+    }
+    GroupType group = type.asGroupType();
+    if (group.getLogicalTypeAnnotation() instanceof LogicalTypeAnnotation.ListLogicalTypeAnnotation) {
+      if (group.getFieldCount() != 1) {
+        return false;
+      }
+      Type repeated = group.getType(0);
+      return !repeated.isPrimitive()

Review Comment:
   I may be misremembering the LIST compatibility rules, so I wanted to check this edge case: could a legacy 2-level list also use the `list`/`element` names and therefore look like this to `allListStructuresAreThreeLevel`? If so, would auto-detect change its Avro shape unexpectedly, or is this case ruled out by the spec or writer assumptions?



-- 
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: issues-unsubscribe@parquet.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org