DevChrisCross commented on code in PR #1498:
URL: https://github.com/apache/iceberg-python/pull/1498#discussion_r1911913704
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1003,6 +1000,20 @@ def _(obj: pa.DictionaryType, visitor:
PyArrowSchemaVisitor[T]) -> T:
return visit_pyarrow(obj.value_type, visitor)
+@visit_pyarrow.register(pa.Field)
Review Comment:
If that's the case that it is only used in `StructType`, I do agree with
you. But I guess for me, and also for the sake of being explicit, I prefer
retaining the separate `@visit_pyarrow.register(pa.Field)`. Because initially
at first when I'm trying to understand the code, I'm also wondering why there's
no separate `visit_pyarrow` declared for `pa.Field`. At least in that case we
know it's only being used by `StructType` but still can be utilized immediately
to accommodate future changes :)
But do let me know if you still wish to proceed with removing the
`@visit_pyarrow.register(pa.Field)`
##########
pyiceberg/io/pyarrow.py:
##########
@@ -1003,6 +1000,20 @@ def _(obj: pa.DictionaryType, visitor:
PyArrowSchemaVisitor[T]) -> T:
return visit_pyarrow(obj.value_type, visitor)
+@visit_pyarrow.register(pa.Field)
Review Comment:
If that's the case that it is only used in `StructType`, I do agree with
you. But I guess for me, and also for the sake of being explicit, I prefer
retaining the separate `@visit_pyarrow.register(pa.Field)`. Because initially
at first when I'm trying to understand the code, I'm also wondering why there's
no separate `visit_pyarrow` declared for `pa.Field`. At least in that case we
know it's only being used by `StructType` but still can be utilized immediately
to accommodate future changes :)
But do let me know if you still wish to proceed with removing the
`@visit_pyarrow.register(pa.Field)` :)
--
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]
