kosiew commented on code in PR #24790:
URL: https://github.com/apache/datafusion/pull/24790#discussion_r3912594407


##########
datafusion/datasource-parquet/src/schema_coercion.rs:
##########
@@ -728,4 +732,53 @@ mod tests {
 
         assert_eq!(result, expected_schema);
     }
+
+    #[test]
+    fn coerce_int96_to_resolution_preserves_field_metadata() {
+        // Spark and Delta Lake stamp a field id on every field. Coercion must
+        // carry that metadata across on nested fields as well as leaves:
+        // formats that identify a column by id rather than by name, such as
+        // Delta Lake column mapping, cannot resolve a column that loses it.
+        let spark_schema = "
+            message spark_schema {
+                REQUIRED INT64 c0 = 1;
+                OPTIONAL group c1 = 2 {
+                    OPTIONAL INT96 c2 = 3;
+                }
+                OPTIONAL group c3 (LIST) = 4 {
+                    REPEATED group list {
+                        OPTIONAL INT96 element = 5;
+                    }
+                }
+            }
+        ";
+
+        let schema = parse_message_type(spark_schema).expect("should parse 
schema");
+        let descr = SchemaDescriptor::new(Arc::new(schema));
+        let arrow_schema = parquet_to_arrow_schema(&descr, None).unwrap();
+
+        let result = Int96Coercer::new(&descr, &arrow_schema, 
&TimeUnit::Microsecond)
+            .coerce()
+            .unwrap();
+
+        for (original, coerced) in 
arrow_schema.fields().iter().zip(result.fields()) {

Review Comment:
   Could we make this regression test a little stronger by adding a map 
containing an INT96 field and recursively comparing the metadata of nested 
fields?
   
   Right now the assertion compares only the top-level fields, while the test 
comment says metadata should be preserved on nested fields as well. A recursive 
comparison covering struct, list, and map children would also exercise the 
rebuilt map and entry struct paths and protect arbitrary nested metadata, 
rather than only the outer Parquet field IDs.
   
   This is non-blocking, but I think it would make the regression coverage 
match the intent of the fix more closely.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to