jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2586103898
##########
arrow-pyarrow-integration-testing/tests/test_sql.py:
##########
@@ -120,28 +121,50 @@ def assert_pyarrow_leak():
# This defines that Arrow consumers should allow any object that has specific
"dunder"
# methods, `__arrow_c_*_`. These wrapper classes ensure that arrow-rs is able
to handle
# _any_ class, without pyarrow-specific handling.
-class SchemaWrapper:
- def __init__(self, schema):
+
+
+class ArrowSchemaExportable(Protocol):
+ def __arrow_c_schema__(self) -> object: ...
+
+
+class ArrowArrayExportable(Protocol):
+ def __arrow_c_array__(
+ self,
+ requested_schema: Union[object, None] = None
+ ) -> Tuple[object, object]:
+ ...
+
+
+class ArrowStreamExportable(Protocol):
+ def __arrow_c_stream__(
+ self,
+ requested_schema: Union[object, None] = None
+ ) -> object:
+ ...
+
+
+class SchemaWrapper(ArrowSchemaExportable):
+ def __init__(self, schema: ArrowSchemaExportable) -> None:
self.schema = schema
- def __arrow_c_schema__(self):
+ def __arrow_c_schema__(self) -> object:
return self.schema.__arrow_c_schema__()
-class ArrayWrapper:
- def __init__(self, array):
+class ArrayWrapper(ArrowArrayExportable):
Review Comment:
One doesn't *have* to subtype a `typing.Protocol`, yes (this is the whole
idea behind it, i.e. not doing static typing but structural typing, which can
allow consuming external objects that don't have to inherit directly from your
classes as long as they conform to a certain pattern).
But in cases where you anyways have strong control over your own classes, I
find it highly beneficial to always inherit directly from the Protocol if
possible. This has the advantage that it will move the detection of type
mismatches to the place where you *defined* your class, instead of requiring
you to make sure that you used all classes in business logic where objects
conforming to a certain protocol are expected. Also, I saw a bit over the years
that with very intricate Protocols, subtle type errors sometimes can be caught
a bit more reliably with existing Python type checkers when directly inheriting
from a Protocol, but this shouldn't be really relevant here I think.
Besides that, I sadly don't think that there anyways are type checks
actually running in the CI or so :sweat_smile: I think the only thing done here
is to compile the Python package and run the tests. There probably should be
another PR introducing some type checking with `mypy --strict` or so.
--
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]
