kylebarron commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4134776637
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -357,6 +378,16 @@ impl FromPyArrow for ArrayData {
}
}
+impl FromPyArrow for ArrayData {
+ type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
Review Comment:
The real type supported as input here is much broader than just
`pyarrow.Array`, because IIRC it supports any Arrow PyCapsule object as input.
Therefore the supported input type is actually
```py
from typing import Tuple, Protocol
class ArrowArrayExportable(Protocol):
def __arrow_c_array__(
self,
requested_schema: object | None = None
) -> Tuple[object, object]:
...
```
[see reference
here](https://arrow.apache.org/docs/format/CDataInterface/PyCapsuleInterface.html#protocol-typehints).
I'm not sure how you would define that inside pyo3 though... I write all my
type hints on the python side.
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -132,6 +132,29 @@ const ARRAY_STREAM_INPUT_TYPE: PyStaticExpr =
type_hint_union!(
type_hint_identifier!("pyarrow", "Table")
);
+/// Trait for converting Python objects to arrow-rs types without validation.
+///
+/// Prefer [`FromPyArrow`] where possible, as it validates the result before
returning.
+pub trait FromPyArrowUnchecked: Sized {
+ /// Convert a Python object to an arrow-rs type without validating the
result.
+ ///
+ /// # Safety
+ ///
+ /// The data exported by the Python object must satisfy all Arrow spec
invariants
+ /// (valid offsets, buffer sizes, UTF-8, etc.) that
[`ArrayData::validate_full`] checks.
+ unsafe fn from_pyarrow_bound_unchecked(value: &Bound<PyAny>) ->
PyResult<Self>;
+}
+
+impl<T: FromPyArrowUnchecked> FromPyArrowUnchecked for Vec<T> {
+ unsafe fn from_pyarrow_bound_unchecked(value: &Bound<PyAny>) ->
PyResult<Self> {
+ let mut v = Vec::with_capacity(value.len().unwrap_or(0));
+ for item in value.try_iter()? {
+ v.push(unsafe { T::from_pyarrow_bound_unchecked(&item?)? });
+ }
+ Ok(v)
+ }
+}
Review Comment:
I don't love this:
- This implicitly assumes that you can iterate over some Python object
(presumably a chunked array in this case) and get Arrow-native types. It thus
over-indexes on pyarrow, specifically, to the detriment of other Python Arrow
libraries.
- A pyarrow schema is also iterable, so a user could use this to import a
schema as a `Vec<Field>`, which would lose any schema metadata
- In general arrow-rs has [preferred APIs that don't materialize an entire
stream](https://github.com/apache/arrow-rs/issues/5295), so an API to convert
to `Vec<T>` would be an anti-pattern.
I would much prefer an API using lazy array readers (see
https://github.com/apache/arrow-rs/issues/6586 and
https://github.com/apache/arrow-rs/pull/11161)
Although I see there's [already an implementation of
this](https://docs.rs/arrow-pyarrow/60.0.0/arrow_pyarrow/trait.FromPyArrow.html#impl-FromPyArrow-for-Vec%3CT%3E),
so it's not a reason to ❌ this PR.
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -357,6 +378,16 @@ impl FromPyArrow for ArrayData {
}
}
+impl FromPyArrow for ArrayData {
+ type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+
+ fn from_pyarrow_bound(value: &Bound<PyAny>) -> PyResult<Self> {
+ let data = unsafe { ArrayData::from_pyarrow_bound_unchecked(value)? };
+ data.validate_full().map_err(to_py_err)?;
Review Comment:
oh I see this is not a new method; it already exists on `ArrayData`
--
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]
