You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Rich-T-kid (via GitHub)" <gi...@apache.org> on 2026/08/27 18:09:35 UTC
[PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Rich-T-kid opened a new pull request, #10894:
URL: https://github.com/apache/arrow-rs/pull/10894
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
-->
- Closes #3672.
# Rationale for this change
like #3672 mentions, `ArrayData::from_pyarrow` does not validate its arguments and its filled with unsafe code. this can introduce invalidate data into arrow-rs that is very hard to trace.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
split `ArrayData::from_pyarrow` into two separate functions. one safe + one unsafe. the safe function validate that data produced from the unsafe function and returns an error if invalid.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are these changes tested?
I don't think this warrants a test.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
If this PR claims a performance improvement, please include evidence such as benchmark results.
-->
# Are there any user-facing changes?
yes, new unsafe api. the safe API now is truly safe.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3914461392
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +314,51 @@ impl ToPyArrow for Schema {
}
}
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
yea in https://github.com/apache/arrow-rs/blob/027b45f2a982832c93d159de6718d69d0e8b60e3/arrow-pyarrow/src/lib.rs#L399
its used here
https://github.com/apache/arrow-rs/blob/027b45f2a982832c93d159de6718d69d0e8b60e3/arrow-pyarrow/src/lib.rs#L444
or were you referring to the new public method? I could add an example to the doc comment
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-5871857498
Maybe @kylebarron can briefly review this change too as I think he has significant python experience and maintains a bunch of python bindings for arrow
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4074505398
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -463,6 +494,59 @@ impl FromPyArrow for RecordBatch {
}
}
+impl FromPyArrowUnchecked for RecordBatch {
+ unsafe fn from_pyarrow_bound_unchecked(value: &Bound<PyAny>) -> PyResult<Self> {
Review Comment:
good point
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4107145564
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -403,63 +434,73 @@ impl<T: ToPyArrow> ToPyArrow for Vec<T> {
}
}
+fn record_batch_from_pyarrow_bound_impl(
+ value: &Bound<PyAny>,
+ array_data_from_pyarrow: impl Fn(&Bound<PyAny>) -> PyResult<ArrayData>,
+) -> PyResult<RecordBatch> {
+ // Newer versions of PyArrow as well as other libraries with Arrow data implement this
+ // method, so prefer it over _export_to_c.
+ // See https://arrow.apache.org/docs/format/CDataInterface/PyCapsuleInterface.html
+ if let Some((schema_capsule, array_capsule)) = call_arrow_c_array_method_if_exists(value)? {
+ let schema_ptr = extract_capsule(&schema_capsule, c"arrow_schema", "__arrow_c_array__")?;
+ let array_ptr = extract_capsule(&array_capsule, c"arrow_array", "__arrow_c_array__")?;
+ let ffi_array = unsafe { FFI_ArrowArray::from_raw(array_ptr.as_ptr()) };
+ let array_data =
Review Comment:
good idea, in https://github.com/apache/arrow-rs/pull/10894/commits/7a27a18a6d6b76b5a76760a51801d0f83fc977eb we always call the unsafe method and then validate depending on the flag
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4057278834
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
https://github.com/apache/arrow-rs/pull/10894/commits/061a8724a943f3c23e58b8e1756fc35681095746 should address this 🚀
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3965267873
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
+1 The function name looks much more general than what the function actually do
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-5470107395
> i wonder if this has implications for performance, since this validation cannot be opted out of 🤔
>
> though i dont have much context on these pyarrow bindings either
@Jefffrey true but I think correctness is more important the performance for this case. we could also make the unsafe method public.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3975169012
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
> An option might be to introduce a new FromPyArrowUnchecked trait and implement it on ArrayData, RecordBatch
I'll work on this and share an update.
>one issue with this is its limited to only directly calling this for one array data; for example we cant use this for a vec of arraydata naturally, or within a recordbatch
are there currently use cases like this? from the two examples we've listed above it seems to be a single invocation, not in a loop or something of that nature
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3914461392
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +314,51 @@ impl ToPyArrow for Schema {
}
}
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
yea in https://github.com/apache/arrow-rs/blob/027b45f2a982832c93d159de6718d69d0e8b60e3/arrow-pyarrow/src/lib.rs#L399
its used here
https://github.com/apache/arrow-rs/blob/027b45f2a982832c93d159de6718d69d0e8b60e3/arrow-pyarrow/src/lib.rs#L444
or were you referring to the new public method?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3911954427
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +314,51 @@ impl ToPyArrow for Schema {
}
}
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
do we maybe have an example of how this could be used by users? for example in one of the pyarrow testing crates
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4073744251
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -463,6 +494,59 @@ impl FromPyArrow for RecordBatch {
}
}
+impl FromPyArrowUnchecked for RecordBatch {
+ unsafe fn from_pyarrow_bound_unchecked(value: &Bound<PyAny>) -> PyResult<Self> {
Review Comment:
nit: it's quite sad to duplicate all the logic. What about sharing it in a utility function that would take a `array_data_from_py_arrow: impl Fn(&Bound<PyAny>) -> PyResult<ArrayData>` argument?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3975169012
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
> An option might be to introduce a new FromPyArrowUnchecked trait and implement it on ArrayData, RecordBatch
I'll work on this and share an update.
>one issue with this is its limited to only directly calling this for one array data; for example we cant use this for a vec of arraydata naturally, or within a recordbatch
>one issue with this is its limited to only directly calling this for one array data; for example we cant use this for a vec of arraydata naturally, or within a recordbatch
are there currently use cases like this? from the two examples we've listed above it seems to be a single invocation, not in a loop or something of that nature
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3958224040
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
one issue with this is its limited to only directly calling this for one array data; for example we cant use this for a vec of arraydata naturally, or within a recordbatch
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3965267873
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
+1 The function name looks much more general than what the function actually do
An option might be to introduce a new `FromPyArrowUnchecked` trait and implement it on `ArrayData`, `RecordBatch`...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4106734251
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -403,63 +434,73 @@ impl<T: ToPyArrow> ToPyArrow for Vec<T> {
}
}
+fn record_batch_from_pyarrow_bound_impl(
+ value: &Bound<PyAny>,
+ array_data_from_pyarrow: impl Fn(&Bound<PyAny>) -> PyResult<ArrayData>,
+) -> PyResult<RecordBatch> {
+ // Newer versions of PyArrow as well as other libraries with Arrow data implement this
+ // method, so prefer it over _export_to_c.
+ // See https://arrow.apache.org/docs/format/CDataInterface/PyCapsuleInterface.html
+ if let Some((schema_capsule, array_capsule)) = call_arrow_c_array_method_if_exists(value)? {
+ let schema_ptr = extract_capsule(&schema_capsule, c"arrow_schema", "__arrow_c_array__")?;
+ let array_ptr = extract_capsule(&array_capsule, c"arrow_array", "__arrow_c_array__")?;
+ let ffi_array = unsafe { FFI_ArrowArray::from_raw(array_ptr.as_ptr()) };
+ let array_data =
Review Comment:
If we want this to have a safe path, then I think after this `ffi::from_ffi` we will need a `array_data.validate_full()`. We would probably need to pass in something like `validate:bool` for the function arguments to enable the check.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-6002110920
Thank you everyone.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3890018384
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +314,51 @@ impl ToPyArrow for Schema {
}
}
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
maybe
```suggestion
pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r3967859077
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -314,39 +329,79 @@ impl ToPyArrow for Schema {
}
}
-impl FromPyArrow for ArrayData {
- type_hint!(INPUT_TYPE = type_hint_identifier!("pyarrow", "Array"));
+/// Convert a Python object to [`ArrayData`] without validating the result.
+///
+/// # Safety
+///
+/// The caller must ensure the data produced by the Python object upholds the invariants
+/// required by [`ArrayData`]. Prefer [`FromPyArrow::from_pyarrow_bound`] for `ArrayData`, which
+/// validates the result before returning it.
+///
+/// # Example
+///
+/// ```ignore
+/// use arrow_array::{Array, Int32Array, make_array};
+/// use arrow_pyarrow::from_pyarrow_bound_unsafe;
+/// use pyo3::prelude::*;
+///
+/// /// Sums an int32 pyarrow array without paying the cost of full validation.
+/// ///
+/// /// # Safety
+/// ///
+/// /// `ob` must be a valid `pyarrow.Array` of type `int32` produced by a trusted source.
+/// /// Passing an array with out-of-bounds offsets or mismatched buffers is undefined behavior.
+/// #[pyfunction]
+/// unsafe fn fast_sum(ob: &Bound<'_, PyAny>) -> PyResult<i64> {
+/// // Skip full validation — we trust the source (e.g. data we produced ourselves).
+/// // Use `ArrayData::from_pyarrow_bound` instead if the data comes from an untrusted caller.
+/// let data = unsafe { from_pyarrow_bound_unsafe(ob)? };
+/// let array = make_array(data);
+/// let int_array = array
+/// .as_any()
+/// .downcast_ref::<Int32Array>()
+/// .ok_or_else(|| pyo3::exceptions::PyTypeError::new_err("expected int32 array"))?;
+///
+/// Ok(int_array.iter().flatten().map(i64::from).sum())
+/// }
+/// ```
+pub unsafe fn from_pyarrow_bound_unsafe(value: &Bound<PyAny>) -> PyResult<ArrayData> {
Review Comment:
I know it increases the scope of this PR, but @Tpt 's suggestion of a new trait is IMO the cleanest final solution. That way if we know for sure we don't need checking we can go strait to that version and if we need safety we can keep the existing method.
I'll try to think about how to do some performance testing on this and what the impacts might be for things like datafusion-python where we use these interfaces.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-6002325949
nice 🚀
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Rich-T-kid (via GitHub)" <gi...@apache.org>.
Rich-T-kid commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-5949362675
@kylebarron could you take a second look
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4134886969
##########
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 block this PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "Tpt (via GitHub)" <gi...@apache.org>.
Tpt commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4136200847
##########
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:
PyO3 does not support writing protocols inline inside of type hints yet. It's something I am currently investigating
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-5954402694
Others might feel differently about wanting a `Vec<T>` implementation, but lgtm!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#discussion_r4123253757
##########
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> {
Review Comment:
this also looks good to me from a backwards compatibility perspective
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feature: add safe version of `ArrayData::from_pyarrow` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #10894:
URL: https://github.com/apache/arrow-rs/pull/10894#issuecomment-5961709218
@Tpt do you have any thoughts on this change?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org