ZENOTME commented on code in PR #731:
URL: https://github.com/apache/iceberg-rust/pull/731#discussion_r1899344509


##########
crates/iceberg/src/spec/schema.rs:
##########
@@ -1132,6 +1132,213 @@ impl ReassignFieldIds {
     }
 }
 
+/// A post order schema visitor with partner.
+///
+/// For order of methods called, please refer to [`visit_schema_with_partner`].
+pub trait SchemaWithPartnerVisitor<P> {
+    /// Return type of this visitor.
+    type T;
+
+    /// Called before struct field.
+    fn before_struct_field(&mut self, _field: &NestedFieldRef, _partner: &P) 
-> Result<()> {
+        Ok(())
+    }
+    /// Called after struct field.
+    fn after_struct_field(&mut self, _field: &NestedFieldRef, _partner: &P) -> 
Result<()> {
+        Ok(())
+    }
+    /// Called before list field.
+    fn before_list_element(&mut self, _field: &NestedFieldRef, _partner: &P) 
-> Result<()> {
+        Ok(())
+    }
+    /// Called after list field.
+    fn after_list_element(&mut self, _field: &NestedFieldRef, _partner: &P) -> 
Result<()> {
+        Ok(())
+    }
+    /// Called before map key field.
+    fn before_map_key(&mut self, _field: &NestedFieldRef, _partner: &P) -> 
Result<()> {
+        Ok(())
+    }
+    /// Called after map key field.
+    fn after_map_key(&mut self, _field: &NestedFieldRef, _partner: &P) -> 
Result<()> {
+        Ok(())
+    }
+    /// Called before map value field.
+    fn before_map_value(&mut self, _field: &NestedFieldRef, _partner: &P) -> 
Result<()> {
+        Ok(())
+    }
+    /// Called after map value field.
+    fn after_map_value(&mut self, _field: &NestedFieldRef, _partner: &P) -> 
Result<()> {
+        Ok(())
+    }
+
+    /// Called before every type, if this function return `Some`, the 
following visiting will be skipped.
+    /// This function used to implement early return.
+    fn visit_type_before(&mut self, _ty: &Type, _partner: &P) -> 
Result<Option<Self::T>> {
+        return Ok(None);
+    }
+
+    /// Called after schema's type visited.
+    fn schema(&mut self, schema: &Schema, partner: &P, value: Self::T) -> 
Result<Self::T>;
+    /// Called after struct's field type visited.
+    fn field(&mut self, field: &NestedFieldRef, partner: &P, value: Self::T) 
-> Result<Self::T>;
+    /// Called after struct's fields visited.
+    fn r#struct(
+        &mut self,
+        r#struct: &StructType,
+        partner: &P,
+        results: Vec<Self::T>,
+    ) -> Result<Self::T>;
+    /// Called after list fields visited.
+    fn list(&mut self, list: &ListType, partner: &P, value: Vec<Self::T>) -> 
Result<Self::T>;
+    /// Called after map's key and value fields visited.
+    fn map(
+        &mut self,
+        map: &MapType,
+        partner: &P,
+        key_value: Vec<Self::T>,
+        value: Vec<Self::T>,
+    ) -> Result<Self::T>;
+    /// Called when see a primitive type.
+    fn primitive(&mut self, p: &PrimitiveType, partner: &P) -> Result<Self::T>;
+}
+
+/// Accessor used to get child partner from parent partner.
+pub trait PartnerAccessor<P> {
+    /// List partner iterator.
+    type L: ListPartnerIterator<P>;
+    /// Map partner iterator.
+    type M: MapPartnerIterator<P>;
+
+    /// Get the struct partner from schema partner.
+    fn struct_parner<'a>(&self, schema_partner: &'a P) -> Result<&'a P>;
+    /// Get the field partner from struct partner.
+    fn field_partner<'a>(&self, struct_partner: &'a P, field_id: i32, field: 
&str)
+        -> Result<&'a P>;
+    /// Get the list element partner from list partner.
+    fn list_element_partner<'a>(&self, list_partner: &'a P) -> Result<Self::L>;
+    /// Get the map key partner from map partner.
+    fn map_element_partner<'a>(&self, map_partner: &'a P) -> Result<Self::M>;
+}
+
+/// Iterator for list partner.
+pub trait ListPartnerIterator<P> {
+    /// Get the next partner.
+    fn next(&mut self) -> Option<P>;
+}
+
+/// Iterator for map partner.
+pub trait MapPartnerIterator<P> {
+    /// Get the next partner.
+    fn next(&mut self) -> Option<(P, P)>;
+}
+
+/// Visiting a type in post order.
+pub fn visit_type_with_partner<P, V: SchemaWithPartnerVisitor<P>, A: 
PartnerAccessor<P>>(
+    r#type: &Type,
+    partner: &P,
+    visitor: &mut V,
+    accessor: &A,
+) -> Result<V::T> {
+    if let Some(res) = visitor.visit_type_before(r#type, partner)? {
+        return Ok(res);
+    }
+    match r#type {
+        Type::Primitive(p) => visitor.primitive(p, partner),
+        Type::List(list) => {
+            let mut results = Vec::new();
+            let mut list_element_partner_iter = 
accessor.list_element_partner(partner)?;
+            if let Some(list_element_partner) = 
list_element_partner_iter.next() {

Review Comment:
   > This doesn't seem quite in efficient. It should be the actual visitor's 
reponsibility to do visit partner.
   
   For now, our visit pattern is that the control logic is in visit function. 
The visitor only take care of how to access data. If we let the actual 
visitor's reponsibility to do visit partner, which means that it need to know 
access which partner, e.g it need to know some index I think. 🤔 And the 
interface should looks like:
    ```
   fn before_list_element(&mut self, _field: &NestedFieldRef, _partner: &P, 
element_idx: usize) -> Result<()> {
           Ok(())
       }
   ```
   Is that what you means?



-- 
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-unsubscr...@iceberg.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to