From 8bb2406c8575547499b61e00b1f4424ac5f6661a Mon Sep 17 00:00:00 2001 From: Leonid Ryzhyk Date: Sun, 30 Aug 2026 00:03:00 -0700 Subject: [PATCH 1/2] Preserve field metadata when coercing INT96 timestamps Fixes #24786 `Int96Coercer` rebuilds every struct, list and map field with `Field::new_struct`, `Field::new_list` and `Field::new`, which start from empty metadata. Leaf fields go through `field_with_new_type`, which clones the field, so only container fields are affected, and only when the file holds an INT96 column: the same file read without `coerce_int96` keeps its metadata. Carry `current_field.metadata()` across in the three constructors. Signed-off-by: Leonid Ryzhyk --- .../datasource-parquet/src/schema_coercion.rs | 59 ++++++++++++++++++- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/datafusion/datasource-parquet/src/schema_coercion.rs b/datafusion/datasource-parquet/src/schema_coercion.rs index 30cd5d7e65948..3ac375f787c8f 100644 --- a/datafusion/datasource-parquet/src/schema_coercion.rs +++ b/datafusion/datasource-parquet/src/schema_coercion.rs @@ -328,7 +328,8 @@ fn coerce_int96_to_resolution_impl( current_field.name(), processed_children.as_slice(), current_field.is_nullable(), - ); + ) + .with_metadata(current_field.metadata().clone()); parent_fields.borrow_mut().push(Arc::new(processed_struct)); } (DataType::List(unprocessed_child), None) => { @@ -361,7 +362,8 @@ fn coerce_int96_to_resolution_impl( current_field.name(), Arc::clone(&processed_children[0]), current_field.is_nullable(), - ); + ) + .with_metadata(current_field.metadata().clone()); parent_fields.borrow_mut().push(Arc::new(processed_list)); } (DataType::Map(unprocessed_child, _), None) => { @@ -391,7 +393,8 @@ fn coerce_int96_to_resolution_impl( current_field.name(), DataType::Map(Arc::clone(&processed_children[0]), *sorted), current_field.is_nullable(), - ); + ) + .with_metadata(current_field.metadata().clone()); parent_fields.borrow_mut().push(Arc::new(processed_map)); } (DataType::Timestamp(TimeUnit::Nanosecond, None), None) @@ -460,6 +463,7 @@ pub fn transform_binary_to_string(schema: &Schema) -> Schema { } #[cfg(test)] mod tests { + use parquet::arrow::PARQUET_FIELD_ID_META_KEY; use parquet::arrow::parquet_to_arrow_schema; use super::*; @@ -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()) { + assert_eq!( + original.metadata(), + coerced.metadata(), + "field {} lost its metadata", + original.name(), + ); + } + + // The struct and the list are the fields the coercion rebuilds, so + // spell out that they kept their ids rather than leaving it to the loop. + assert_eq!( + result.field_with_name("c1").unwrap().metadata()[PARQUET_FIELD_ID_META_KEY], + "2", + ); + assert_eq!( + result.field_with_name("c3").unwrap().metadata()[PARQUET_FIELD_ID_META_KEY], + "4", + ); + } } From 9cc03a6f37dd5cc01ea4b8f5a7ed63f4862c151d Mon Sep 17 00:00:00 2001 From: Leonid Ryzhyk Date: Sat, 5 Sep 2026 23:12:32 -0700 Subject: [PATCH 2/2] Strengthen the metadata regression test to cover nested fields Review feedback on #24790: the test asserted metadata on top-level fields only, while the fix is about the fields the coercion rebuilds, which are nested. Add a map with an INT96 value to the schema, and compare metadata recursively so struct children, list elements, map entry structs and map key/value fields are all checked. Signed-off-by: Leonid Ryzhyk --- .../datasource-parquet/src/schema_coercion.rs | 117 ++++++++++++++++-- 1 file changed, 104 insertions(+), 13 deletions(-) diff --git a/datafusion/datasource-parquet/src/schema_coercion.rs b/datafusion/datasource-parquet/src/schema_coercion.rs index 3ac375f787c8f..825b7e27ff20d 100644 --- a/datafusion/datasource-parquet/src/schema_coercion.rs +++ b/datafusion/datasource-parquet/src/schema_coercion.rs @@ -750,35 +750,126 @@ mod tests { OPTIONAL INT96 element = 5; } } + OPTIONAL group c4 (MAP) = 6 { + REPEATED group key_value { + REQUIRED BYTE_ARRAY key (STRING) = 7; + OPTIONAL INT96 value = 8; + } + } } "; 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(); + // Arrow derives a field id for every field but a map's entry struct, and + // metadata holds more than ids, so stamp each field with a key of its + // own: coercion has to preserve arbitrary metadata at every level. + let arrow_schema = + stamp_every_field(&parquet_to_arrow_schema(&descr, None).unwrap()); let result = Int96Coercer::new(&descr, &arrow_schema, &TimeUnit::Microsecond) .coerce() .unwrap(); + assert_eq!(result.fields().len(), arrow_schema.fields().len()); for (original, coerced) in arrow_schema.fields().iter().zip(result.fields()) { + assert_field_metadata_preserved(original, coerced, original.name()); + } + + // The containers are the fields the coercion rebuilds, so spell out that + // they kept their ids: the recursion above compares the two schemas + // against each other and would also pass on a schema carrying no ids. + for (name, field_id) in [("c1", "2"), ("c3", "4"), ("c4", "6")] { assert_eq!( - original.metadata(), - coerced.metadata(), - "field {} lost its metadata", - original.name(), + result.field_with_name(name).unwrap().metadata() + [PARQUET_FIELD_ID_META_KEY], + field_id, ); } + } + + /// Metadata key stamped by [`stamp_every_field`], holding the field's path. + const TEST_METADATA_KEY: &str = "test.field_path"; + + /// Add [`TEST_METADATA_KEY`] to every field of `schema`, nested fields + /// included, keeping the metadata each field already has. + fn stamp_every_field(schema: &Schema) -> Schema { + fn stamp(field: &FieldRef, path: &str) -> FieldRef { + let child_path = |child: &FieldRef| format!("{path}.{}", child.name()); + let data_type = match field.data_type() { + DataType::Struct(children) => DataType::Struct( + children + .iter() + .map(|child| stamp(child, &child_path(child))) + .collect(), + ), + DataType::List(child) => DataType::List(stamp(child, &child_path(child))), + DataType::Map(child, sorted) => { + DataType::Map(stamp(child, &child_path(child)), *sorted) + } + other => other.clone(), + }; + let mut metadata = field.metadata().clone(); + metadata.insert(TEST_METADATA_KEY.to_string(), path.to_string()); + Arc::new( + field + .as_ref() + .clone() + .with_data_type(data_type) + .with_metadata(metadata), + ) + } - // The struct and the list are the fields the coercion rebuilds, so - // spell out that they kept their ids rather than leaving it to the loop. - assert_eq!( - result.field_with_name("c1").unwrap().metadata()[PARQUET_FIELD_ID_META_KEY], - "2", - ); + Schema::new_with_metadata( + schema + .fields() + .iter() + .map(|field| stamp(field, field.name())) + .collect::>(), + schema.metadata.clone(), + ) + } + + /// Assert `coerced` carries the metadata of `original`, and so does every + /// one of its children: struct fields, list elements, map entry structs. + fn assert_field_metadata_preserved( + original: &FieldRef, + coerced: &FieldRef, + path: &str, + ) { assert_eq!( - result.field_with_name("c3").unwrap().metadata()[PARQUET_FIELD_ID_META_KEY], - "4", + original.metadata(), + coerced.metadata(), + "field {path} lost its metadata", ); + + match (original.data_type(), coerced.data_type()) { + (DataType::Struct(original_children), DataType::Struct(coerced_children)) => { + assert_eq!(original_children.len(), coerced_children.len()); + for (original_child, coerced_child) in + original_children.iter().zip(coerced_children) + { + assert_field_metadata_preserved( + original_child, + coerced_child, + &format!("{path}.{}", original_child.name()), + ); + } + } + (DataType::List(original_child), DataType::List(coerced_child)) + | (DataType::Map(original_child, _), DataType::Map(coerced_child, _)) => { + assert_field_metadata_preserved( + original_child, + coerced_child, + &format!("{path}.{}", original_child.name()), + ); + } + // Only the arms above recurse, so a nested type here means the + // coercion changed the field's shape and children went unchecked. + (original_type, coerced_type) => assert!( + !original_type.is_nested(), + "field {path} changed shape: {original_type} -> {coerced_type}", + ), + } } }