ewoodbury commented on code in PR #3255:
URL: https://github.com/apache/iceberg-rust/pull/3255#discussion_r4235503150


##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -193,6 +195,284 @@ pub(crate) enum ColumnSource {
     // post-processing step by using the projection mask.
 }
 
+/// Reconciles a source array to the target type by matching nested fields on
+/// field id rather than position, mirroring iceberg-java's nested readers.
+///
+/// The plan is resolved once per file, when the `BatchTransform` is built, so
+/// applying it per batch is just index lookups and array assembly.
+#[derive(Debug)]
+pub(crate) enum PromotePlan {
+    PassThrough,
+    Cast(DataType),
+    Struct {
+        fields: Fields,
+        children: Vec<ChildPlan>,
+    },
+    List {
+        field: FieldRef,
+        element: Box<PromotePlan>,
+    },
+    Map {
+        field: FieldRef,
+        entry_fields: Fields,
+        entries: Vec<ChildPlan>,
+        sorted: bool,
+    },
+}
+
+#[derive(Debug)]
+pub(crate) enum ChildPlan {
+    FromSource {
+        source_index: usize,
+        plan: PromotePlan,
+    },
+    Add {
+        target_type: DataType,
+        value: Option<PrimitiveLiteral>,
+    },
+}
+
+impl PromotePlan {
+    fn build(
+        source: &DataType,
+        target: &DataType,
+        snapshot_schema: &IcebergSchema,
+        constant_fields: &HashMap<i32, ColumnConstant>,
+    ) -> Result<Self> {
+        if source == target {
+            return Ok(PromotePlan::PassThrough);
+        }
+        match (source, target) {
+            (DataType::Struct(source_fields), DataType::Struct(target_fields)) 
=> {
+                Ok(PromotePlan::Struct {
+                    children: Self::build_struct_children(
+                        source_fields,
+                        target_fields,
+                        snapshot_schema,
+                        constant_fields,
+                    )?,
+                    fields: target_fields.clone(),
+                })
+            }
+            // A file's Arrow schema hint can give either offset width; 
`apply` converts it.
+            // Table schemas are always `List` 
(`ToArrowSchemaConverter::list`).
+            (
+                DataType::List(source_field) | 
DataType::LargeList(source_field),
+                DataType::List(target_field),
+            ) => Ok(PromotePlan::List {
+                element: Box::new(Self::build(
+                    source_field.data_type(),
+                    target_field.data_type(),
+                    snapshot_schema,
+                    constant_fields,
+                )?),
+                field: target_field.clone(),
+            }),
+            (DataType::Map(source_entries, _), DataType::Map(target_entries, 
sorted)) => {
+                match (source_entries.data_type(), target_entries.data_type()) 
{
+                    (DataType::Struct(source_fields), 
DataType::Struct(target_fields)) => {
+                        Ok(PromotePlan::Map {
+                            entries: Self::build_struct_children(
+                                source_fields,
+                                target_fields,
+                                snapshot_schema,
+                                constant_fields,
+                            )?,
+                            entry_fields: target_fields.clone(),
+                            field: target_entries.clone(),
+                            sorted: *sorted,
+                        })
+                    }
+                    _ => Err(Error::new(
+                        ErrorKind::Unexpected,
+                        format!(
+                            "expected struct-typed map entries, got 
{source_entries:?} and {target_entries:?}"
+                        ),
+                    )),
+                }
+            }
+            (_, DataType::Struct(_) | DataType::List(_) | 
DataType::LargeList(_))
+            | (_, DataType::Map(_, _)) => {
+                Err(invalid_data!("cannot promote {source:?} to {target:?}"))
+            }
+            _ => Ok(PromotePlan::Cast(target.clone())),
+        }
+    }
+
+    fn build_struct_children(
+        source_fields: &Fields,
+        target_fields: &Fields,
+        snapshot_schema: &IcebergSchema,
+        constant_fields: &HashMap<i32, ColumnConstant>,
+    ) -> Result<Vec<ChildPlan>> {
+        let mut source_by_id = HashMap::with_capacity(source_fields.len());
+        for (idx, field) in source_fields.iter().enumerate() {
+            if let Some(id) = try_get_field_id_from_metadata(field)? {
+                source_by_id.insert(id, idx);
+            }
+        }
+        // Name mapping only assigns top-level ids (#1845). Fully id-less 
children
+        // match by position when the counts line up, and `build` checks each 
pair.
+        // A same-type reorder cannot be detected without ids. Any missing id
+        // errors instead of nulling that child.
+        if source_by_id.len() != source_fields.len() {
+            if source_by_id.is_empty() && source_fields.len() == 
target_fields.len() {
+                return source_fields
+                    .iter()
+                    .zip(target_fields.iter())
+                    .enumerate()
+                    .map(|(source_index, (source_field, target_field))| {
+                        Ok(ChildPlan::FromSource {
+                            source_index,
+                            plan: Self::build(
+                                source_field.data_type(),
+                                target_field.data_type(),
+                                snapshot_schema,
+                                constant_fields,
+                            )?,
+                        })
+                    })
+                    .collect();
+            }
+            return Err(invalid_data!(
+                "cannot reconcile struct fields by id: source fields do not 
all have field ids"
+            ));
+        }
+
+        target_fields
+            .iter()
+            .map(|target_field| {
+                let field_id = try_get_field_id_from_metadata(target_field)?;
+                match field_id.and_then(|id| source_by_id.get(&id).copied()) {
+                    Some(source_index) => Ok(ChildPlan::FromSource {
+                        plan: Self::build(
+                            source_fields[source_index].data_type(),
+                            target_field.data_type(),
+                            snapshot_schema,
+                            constant_fields,
+                        )?,
+                        source_index,
+                    }),
+                    None => {
+                        // Identity partition constants apply inside structs 
too. A missing
+                        // child uses that value before initial-default 
(#3261) or null.
+                        if let Some(ColumnConstant::Scalar(datum)) =
+                            field_id.and_then(|id| constant_fields.get(&id))
+                        {
+                            return Ok(ChildPlan::Add {
+                                target_type: target_field.data_type().clone(),
+                                value: Some(datum.literal().clone()),
+                            });
+                        }
+                        // Missing nested fields are null-filled. Applying an
+                        // initial-default here is not implemented yet (#3261).
+                        let iceberg_field =
+                            field_id.and_then(|id| 
snapshot_schema.field_by_id(id));
+                        if iceberg_field.is_some_and(|f| 
f.initial_default.is_some()) {

Review Comment:
   Agreed it's close. After going through it in detail, I'd rather close #3261 
completely than half-close it here. The issue also covers the non-null {} 
struct default case (which is referenced by #2635), and that will need a 
non-null struct built from each child's default at both the top level and 
nested levels, which is a more substantial change.
   
   I'd say better option is to open a focused follow-up that covers both, once 
this is in. So for now I left the FeatureUnsupported error and its #3261 TODO 
as is.



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