rdblue commented on code in PR #4685:
URL: https://github.com/apache/iceberg/pull/4685#discussion_r868271279
##########
python/src/iceberg/schema.py:
##########
@@ -279,53 +319,74 @@ 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)
+ visit(field.type, visitor)
+ except Exception as e:
+ logger.exception(f"Unable to visit the field: {field}")
+ raise e
finally:
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)
+ visit(obj.element.type, visitor)
+ except Exception as e:
+ logger.exception(f"Unable to visit the type: {obj}")
+ raise e
finally:
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)
visitor.before_map_key(obj.key)
try:
- key_result = visit(obj.key.type, visitor)
+ visit(obj.key.type, visitor)
+ except Exception as e:
+ logger.exception(f"Unable to visit the map ket type: {obj.key}")
+ raise e
finally:
visitor.after_map_key(obj.key)
+ visitor.field(obj.value)
visitor.before_map_value(obj.value)
try:
- value_result = visit(obj.value.type, visitor)
+ visit(obj.value.type, visitor)
+ except Exception as e:
+ logger.exception(f"Unable to visit the map value type: {obj.value}")
Review Comment:
Do we need to log exceptions at every level? Why not propagate like normal
to avoid too many logs?
A heavily nested object with an exception would result in _a lot_ of logs,
plus the exception getting handled and logged at a higher level.
--
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]
