You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/09/07 06:17:23 UTC
(datafusion) branch main updated: Preserve field metadata when coercing INT96 timestamps (#24790)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new cc29ea12ae Preserve field metadata when coercing INT96 timestamps (#24790)
cc29ea12ae is described below
commit cc29ea12ae9c19640d98e43f5d3676aa3b334431
Author: Leonid Ryzhyk <ry...@gmail.com>
AuthorDate: Mon Sep 7 06:07:53 2026 +0000
Preserve field metadata when coercing INT96 timestamps (#24790)
## Which issue does this PR close?
- Closes #24786.
## Rationale for this change
`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.
## What changes are included in this PR?
Carry `current_field.metadata()` across in the three constructors.
## Are these changes tested?
yes
## Are there any user-facing changes?
---------
Signed-off-by: Leonid Ryzhyk <ry...@gmail.com>
---
.../datasource-parquet/src/schema_coercion.rs | 150 ++++++++++++++++++++-
1 file changed, 147 insertions(+), 3 deletions(-)
diff --git a/datafusion/datasource-parquet/src/schema_coercion.rs b/datafusion/datasource-parquet/src/schema_coercion.rs
index 30cd5d7e65..825b7e27ff 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,144 @@ 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;
+ }
+ }
+ 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));
+ // 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!(
+ 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),
+ )
+ }
+
+ Schema::new_with_metadata(
+ schema
+ .fields()
+ .iter()
+ .map(|field| stamp(field, field.name()))
+ .collect::<Vec<_>>(),
+ 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!(
+ 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}",
+ ),
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org