rdblue commented on code in PR #4685:
URL: https://github.com/apache/iceberg/pull/4685#discussion_r868721508


##########
python/src/iceberg/schema.py:
##########
@@ -279,53 +326,54 @@ def visit(obj, visitor: SchemaVisitor[T]) -> T:
 @visit.register(Schema)
 def _(obj: Schema, visitor: SchemaVisitor[T]) -> T:
     """Visit a Schema with a concrete SchemaVisitor"""
-    return visitor.schema(obj, visit(obj.as_struct(), visitor))
+    return visit(obj.as_struct(), visitor)
 
 
 @visit.register(StructType)
 def _(obj: StructType, visitor: SchemaVisitor[T]) -> T:
     """Visit a StructType with a concrete SchemaVisitor"""
-    results = []
+
+    result = visitor.struct(obj)
+
     for field in obj.fields:
+        visitor.field(field)
         visitor.before_field(field)
-        try:
-            result = visit(field.type, visitor)
-        finally:
-            visitor.after_field(field)
+        visit(field.type, visitor)
+        visitor.after_field(field)
 
-        results.append(visitor.field(field, result))
-
-    return visitor.struct(obj, results)
+    return result
 
 
 @visit.register(ListType)
 def _(obj: ListType, visitor: SchemaVisitor[T]) -> T:
     """Visit a ListType with a concrete SchemaVisitor"""
+    result = visitor.list(obj)
+
+    visitor.field(obj.element)
+
     visitor.before_list_element(obj.element)
-    try:
-        result = visit(obj.element.type, visitor)
-    finally:
-        visitor.after_list_element(obj.element)
+    visit(obj.element.type, visitor)
+    visitor.after_list_element(obj.element)
 
-    return visitor.list(obj, result)
+    return result
 
 
 @visit.register(MapType)
 def _(obj: MapType, visitor: SchemaVisitor[T]) -> T:
     """Visit a MapType with a concrete SchemaVisitor"""
+    result = visitor.map(obj)
+
+    visitor.field(obj.key)

Review Comment:
   Changing the behavior of `field` is probably going to cause problems later 
when translating between Java and Python implementations. `field` isn't called 
for map key, map value, or list element in any of the Java visitors.



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