You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jonded94 (via GitHub)" <gi...@apache.org> on 2025/11/05 21:29:32 UTC
[PR] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
jonded94 opened a new pull request, #8790:
URL: https://github.com/apache/arrow-rs/pull/8790
# Rationale for this change
When dealing with Parquet files that have an exceedingly large amount of Binary or UTF8 data in one row group, there can be issues when returning a single RecordBatch because of index overflows (https://github.com/apache/arrow-rs/issues/7973).
In `pyarrow` this is usually solved by representing data as a `pyarrow.Table` object whose columns are `ChunkedArray`s, which basically are just lists of Arrow Arrays, or alternatively, the `pyarrow.Table` is just a representation of a list of `RecordBatch`es.
I'd like to build a function in PyO3 that returns a `pyarrow.Table`, very similar to [pyarrow's read_row_group method](https://arrow.apache.org/docs/python/generated/pyarrow.parquet.ParquetFile.html#pyarrow.parquet.ParquetFile.read_row_group).
Currently, AFAIS, there is no way in `arrow-pyarrow` to export a `pyarrow.Table` directly. Especially convenience methods from `Vec<RecordBatch>` seem to be missing. This PR tries to implement a convenience wrapper that allows directly exporting `pyarrow.Table`.
# What changes are included in this PR?
A new struct `Table` in the crate `arrow-pyarrow` is added which can be constructed from `Vec<RecordBatch>` or from `ArrowArrayStreamReader`.
It implements `FromPyArrow` and `IntoPyArrow`.
`FromPyArrow` will support anything that either implements the ArrowStreamReader protocol or is a RecordBatchReader, or has a `to_reader()` method which does that. `pyarrow.Table` does both of these things.
`IntoPyArrow` will result int a `pyarrow.Table` on the Python side, constructed through `pyarrow.Table.from_batches(...)`.
# Are these changes tested?
No, not yet. Please let me know whether you are in general fine with this PR, then I'll work on tests. So far I only tested it locally with very easy PyO3 dummy functions basically doing a round-trip, and with them everything worked:
```
#[pyfunction]
pub fn roundtrip_table(table: PyArrowType<Table>) -> PyArrowType<Table> {
table
}
#[pyfunction]
pub fn build_table(record_batches: Vec<PyArrowType<RecordBatch>>) -> PyArrowType<Table> {
PyArrowType(Table::try_new(record_batches.into_iter().map(|rb| rb.0).collect()).unwrap())
}
```
=>
```
>>> import pyo3parquet
>>> import pyarrow
>>> table = pyarrow.Table.from_pylist([{"foo": 1}])
>>> pyo3parquet.roundtrip_table(table)
pyarrow.Table
foo: int64
----
foo: [[1]]
>>> pyo3parquet.build_table(table.to_batches())
pyarrow.Table
foo: int64
----
foo: [[1]]
```
The real tests of course would be much more sophisticated than just this.
# Are there any user-facing changes?
A new `Table` convience wrapper is added!
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2505372002
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
Review Comment:
Reprashed it like your suggestion
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2510675748
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,120 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+ ) -> Result<Self, ArrowError> {
+ /// This function was copied from `pyo3_arrow/utils.rs` for now. I don't understand yet why
+ /// this is required instead of a "normal" `schema == record_batch.schema()` check.
+ ///
+ /// TODO: Either remove this check, replace it with something already existing in `arrow-rs`
+ /// or move it to a central `utils` location.
+ fn schema_equals(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields
Review Comment:
In principle, instead of using this schema check method at all, I'd much rather have the underlying issue solved by understanding why either the ArrowStreamReader PyCapsule interface of `pyarrow.Table` or the Rust `Box<dyn RecordBatchReader>` part seems to swallow up `RecordBatch` metadata.
If that issue would be fixed, then this function can be left out again and a normal `schema == recordbatch.schema()` test could be used. This function would only have relevance if it's *expected* that the `RecordBatch` coming from a stream reader somehow doesn't have metadata anymore, because then we would have to do this custom schema equality check.
But in general your comment would be also relevant for @kylebarron as he is using this function as-is in his crate `pyo3-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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3504923170
@kylebarron for now I stole the `schema_equals` function from `pyo3_arrow`, and everything seems to work again. I don't quite understand why the Record Batches are metadata less when read from `Box<dyn RecordBatchReader>` though.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3536535648
Okay, I think the error was in `impl Iterator for ArrowArrayStreamReader`. That actually did not correctly exposed RecordBatches that have metadata attached, but was just directly constructing `RecordBatch` from `StructArray` (which is metadata less by definition).
With that, consuming `pyarrow.Table` through the PyCapsule interface (and not with its custom `Table.to_batches()` method) seems to work now.
@alamb @kylebarron let me know whether the change of `impl Iterator for ArrowArrayStreamReader` in `arrow-array/src/ffi_stream.rs` is okay.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2582588902
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
>> What is current on main is pretty elegant
>
> `RecordBatch::from(StructArray::from(data)))`
I personally wouldn't call this elegant, as it's actually *currently* leading to unsoundless problems which I already stated a few times in this discussion :grimacing:
1. Since `StructArray` is metadata-less, the `RecordBatch` constructed from it is metadata-less by definition too. This means, the returned `RecordBatch` can't ever have the schema advertised by `ArrowArrayStreamReader`, at least if it's supposed to have metadata.
2. Much more importantly: There currently are no checks whether the `RecordBatch` constructed from `StructArray` and returned by the stream reader even corresponds to the schema which `ArrowArrayStreamReader` advertises. They are just returned as-is, and in prinicple it's possible to make this interface return an entirely different `RecordBatch`. This was the cause of this PR, namely that it's currently returning unsound `RecordBatch` without advertising this as an error somewhere.
The current state with `unsafe` in this PR more or less leaves problem 2 intact (with arguably introducing a chance of an additional invariant incorrectness in the `RecordBatch` itself), but at least fixes problem 1.
Nevertheless, I will proceed with introducing schema checking now and remove `unsafe`, which would also fix problem 2 in general, with a tiny additional compute cost.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3539448929
In [this](https://github.com/apache/arrow-rs/pull/8790/commits/b61aae006b9adca254aeee687de7122bb31b11b2) commit I also augmented the tests in `ffi_stream.rs` to actually check for exported/imported metadata, instead of dealing with a metadata-less schema.
Please let me know whether I did this right.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2505373258
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! convenience wrapper [Table] (which internally holds `Vec<RecordBatch>`), this is more meant for
+//! use cases where you already have `Vec<RecordBatch>` on the Rust side and want to export that in
+//! bulk as a `pyarrow.Table`. In general, it is recommended to use streaming approaches instead of
Review Comment:
I was a bit more condensed in the documentation and collapsed it now to one paragraph
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2505379281
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
Review Comment:
Previously I trusted that the schemas of all `RecordBatch`es from `ArrowArrayStreamReader` are correct, that's why I used the `unsafe` `new_unchecked` in the `TryFrom<ArrowArrayStreamReader> for Table` implementation for a slight performance boost.
Now there is only `try_new`.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3589218787
@alamb @kylebarron could I get a review on this PR again? :pleading_face:
Functionally I would consider it complete. I'm just unsure whether the ArrowArrayStreamReader can always be trusted to produce RecordBatches with the correct schema, at least data/column-wise, already mentioned in my comment [here](https://github.com/apache/arrow-rs/pull/8790#discussion_r2532232970).
This informs whether the `unsafe` logic I do there is justified or whether a potentially a bit costly check on schema validity on every RecordBatch would have to be introduced (which we didn't do before).
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3495859644
Thanks @kylebarron for your very quick review! :heart:
> Historically the attitude of this crate has been to avoid "Table" constructs to push users towards streaming approaches.
>
> I don't know what the stance of maintainers is towards including a `Table` construct for python integration.
Yes, I'm also not too sure about it, that's why I just sketched out a rough implementation without tests so far. A reason why I think this potentially could be nice to have in `arrow-pyarrow` is that the [documentation](https://arrow.apache.org/rust/arrow_pyarrow/index.html) even mentions that there is no equivalent concept to `pyarrow.Table` in `arrow-pyarrow` and that one has to do slight workarounds to use them:
> PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn’t have these same concepts. A chunked table is instead represented with Vec<RecordBatch>. A pyarrow.Table can be imported to Rust by calling [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader) and then importing the reader as a [ArrowArrayStreamReader].
At least I personally think having such a wrapper could be nice, since it simplifies stuff a bit when you anyways already have `Vec<RecordBatch>` on the Rust side somewhere or need to handle a `pyarrow.Table` on the Python side and want to have an easy method to generate such a thing from Rust. One still could mention in the documentation that generally, streaming approaches are highly preferred, and that the `pyarrow.Table` convenience wrapper shall only be used in cases where users know what they're doing.
### Slightly nicer Python workflow
In our very specific example, we have a Python class with a function such as this one:
```Python
class ParquetFile:
def read_row_group(self, index: int) -> pyarrow.RecordBatch: ...
```
In the [issue](https://github.com/apache/arrow-rs/issues/7973) I linked this unfortunately breaks down for a specific parquet file since a particular row group isn't expressable as a single `RecordBatch` without changing types somewhere. Either you'd have to change the underlying Arrow types from `String` to `LargeString` or `StringView`, or you change the returned type from `pyarrow.RecordBatch` to `Iterator[pyarrow.RecordBatch]` for example (or `RecordBatchReader` or any other streaming-capable object).
The latter comes with a bit of syntactic shortcomings in contexts where you want to apply `.to_pylist()` on whatever `read_row_group(...)` returns:
```Python
rg: pyarrow.RecordBatch | Iterator[pyarrow.RecordBatch] = ParquetFile(...).read_row_group(0)
python_objs: list[dict[str, Any]]
if isinstance(rg, pyarrow.RecordBatch):
python_objs = rg.to_pylist()
else:
python_objs = list(itertools.chain.from_iterable(batch.to_pylist() for batch in rg))
```
With `pyarrow.Table`, there already exists a thing which simplifies this a lot on the Python side:
```Python
rg: pyarrow.RecordBatch | pyarrow.Table = ParquetFile(...).read_row_group(0)
python_objs: list[dict[str, Any]] = rg.to_pylist()
```
And just for clarity, we unfortunately *need* to have the entire Row group deserialized as Python objects because our data ingestion pipelines that consume this are expecting to have access to the entire row group in bulk, so streaming approaches are sadly not usable.
> FWIW if you wanted to look at external crates, [`PyTable` exists](https://docs.rs/pyo3-arrow/latest/pyo3_arrow/struct.PyTable.html) that probably does what you want. (disclosure it's my project). That alternatively might give you ideas for how to handle the `Table` here if you still want to do that. (It's a separate crate for [these reasons](https://docs.rs/pyo3-arrow/latest/pyo3_arrow/#why-not-use-arrow-rss-python-integration))
Yes, in general, I much prefer the approach of `arro3` to be totally `pyarrow` agnostic. In our case unfortunately, we're right now still pretty hardcoded against `pyarrow` specifics and just use `arrow-rs` as a means to reduce memory load compared to reading & writing parquet datasets with `pyarrow` directly.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3607248280
Done, this PR is `unsafe` less and only adds the `pyarrow.Table` convience wrapper.
The fix for the `ArrowArrayStreamReader` can be found in this PR: https://github.com/apache/arrow-rs/pull/8944
As soon as both PRs are merged, I'll augment the tests in `arrow-pyarrow-integration-tests` to include more in-depth metadata checks.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2505381464
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
+
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: Option<SchemaRef>,
+ ) -> Result<Self, ArrowError> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ record_batches
+ .get(0)
+ .ok_or_else(|| ArrowError::SchemaError(
+ "If no schema is supplied explicitly, there must be at least one RecordBatch!".to_owned()
+ ))?
+ .schema()
+ .clone()
+ }
+ };
+ for record_batch in &record_batches {
+ if schema != record_batch.schema() {
+ return Err(ArrowError::SchemaError(
+ "All record batches must have the same schema.".to_owned(),
+ ));
+ }
+ }
+ Ok(Self {
+ record_batches,
+ schema,
+ })
+ }
+
+ pub fn record_batches(&self) -> &[RecordBatch] {
+ &self.record_batches
+ }
+
+ pub fn schema(&self) -> SchemaRef {
+ self.schema.clone()
+ }
+
+ pub fn into_inner(self) -> (Vec<RecordBatch>, SchemaRef) {
+ (self.record_batches, self.schema)
+ }
+}
+
+impl TryFrom<ArrowArrayStreamReader> for Table {
+ type Error = ArrowError;
+
+ fn try_from(value: ArrowArrayStreamReader) -> Result<Self, ArrowError> {
+ let schema = value.schema();
+ let batches = value.collect::<Result<Vec<_>, _>>()?;
+ // We assume all batches have the same schema here.
+ unsafe { Ok(Self::new_unchecked(batches, schema)) }
+ }
+}
+
+impl FromPyArrow for Table {
+ fn from_pyarrow_bound(ob: &Bound<PyAny>) -> PyResult<Self> {
+ let array_stream_reader: PyResult<ArrowArrayStreamReader> = {
Review Comment:
Applied the suggestion, the function is now much simpler
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2510684516
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,120 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+ ) -> Result<Self, ArrowError> {
+ /// This function was copied from `pyo3_arrow/utils.rs` for now. I don't understand yet why
+ /// this is required instead of a "normal" `schema == record_batch.schema()` check.
+ ///
+ /// TODO: Either remove this check, replace it with something already existing in `arrow-rs`
+ /// or move it to a central `utils` location.
+ fn schema_equals(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields
+ .iter()
+ .zip(right.fields.iter())
+ .all(|(left_field, right_field)| {
+ left_field.name() == right_field.name()
+ && left_field
+ .data_type()
+ .equals_datatype(right_field.data_type())
+ })
+ }
+
+ for record_batch in &record_batches {
+ if !schema_equals(&schema, &record_batch.schema()) {
+ return Err(ArrowError::SchemaError(
+ //"All record batches must have the same schema.".to_owned(),
Review Comment:
I only have the more verbose error message here right now to understand what's going on in the schema mismatch. This is currently commented out to signal that this is not intended to be merged as-is, but the schema mismatch issue shall be understood first.
In general I'm opinionless about how verbose the error message shall be, I'd happily to eventually remove whatever variant you dislike.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2510675748
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,120 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+ ) -> Result<Self, ArrowError> {
+ /// This function was copied from `pyo3_arrow/utils.rs` for now. I don't understand yet why
+ /// this is required instead of a "normal" `schema == record_batch.schema()` check.
+ ///
+ /// TODO: Either remove this check, replace it with something already existing in `arrow-rs`
+ /// or move it to a central `utils` location.
+ fn schema_equals(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields
Review Comment:
In principle, instead of using this schema check method at all, I'd much rather have the underlying issue solved by understanding why either the ArrowStreamReader PyCapsule interface of `pyarrow.Table` or the Rust `Box<dyn RecordBatchReader>` part seems to swallow up `RecordBatch` metadata.
If that issue would be fixed, then this function can be left out again and a normal `schema == recordbatch.schema()` test could be used. This function would only have relevance if it's *expected* that the `RecordBatch` coming from a stream reader somehow doesn't have metadata anymore.
But in general your comment would be also relevant for @kylebarron as he is using this function as-is in his crate `pyo3-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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2503876977
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +484,102 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// PyarrowType(Table::try_new(batches).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>) -> Self {
+ Self { record_batches }
+ }
+
+ pub fn try_new(record_batches: Vec<RecordBatch>) -> Result<Self, ArrowError> {
+ let schema = record_batches[0].schema();
Review Comment:
I overhauled `Table` to conform a little bit more to what you have in `pyo3-arrow`, namely a combination of `Vec<RecordBatch>` and `SchemaRef`. This also allows exporting an empty `pyarrow.Table`, which I'm also testing for in the integration testing PyO3 crate.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2529906108
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,120 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+ ) -> Result<Self, ArrowError> {
+ /// This function was copied from `pyo3_arrow/utils.rs` for now. I don't understand yet why
+ /// this is required instead of a "normal" `schema == record_batch.schema()` check.
+ ///
+ /// TODO: Either remove this check, replace it with something already existing in `arrow-rs`
+ /// or move it to a central `utils` location.
+ fn schema_equals(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields
Review Comment:
This function is no longer required since I found a way to receive RecordBatches with preserved metadata. But as I said, still potentially relevant for the `pyo3-arrow` crate.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2582255380
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -452,7 +462,19 @@ mod tests {
let array = unsafe { from_ffi(ffi_array, &ffi_schema) }.unwrap();
- let record_batch = RecordBatch::from(StructArray::from(array));
+ let record_batch = {
Review Comment:
same here -- unless there is some compelling reason we shouldn't be using `unsafe`, even thought this is test code
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
> I'm not sure whether there is another more elegant way to construct a RecordBatch with corresponding metadata from ArrayData. Right now I'm going through StructArray because the previous interface did that too. If there is another more elegant way, please let me know.
What is current on main is pretty elegant
```rust
RecordBatch::from(StructArray::from(data)))
```
It is very important that this crate doesn't introduce unsoundness bugs, so in general we try and avoid `unsafe` unless there is a compelling justification, as described here
https://github.com/apache/arrow-rs/blob/main/arrow/README.md#safety
So at the least this code should have a justification (as a comment) about why `unsafe` is ok (aka why the invariants are known to be true) rather than using the safe alternate
I suggest:
1. Remove the use of unsafe in this PR
2. Make a follow on PR that proposes converting this to `unsafe` where we can discuss the merits in a focused discussion
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2531912775
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
Can you please leave a comment here explaining:
1. The rationale for this form rather than just converting StructArray to RecordBatch
2. A `SAFETY` comment explaining why the `unsafe` is ok (aka how the invariants required for RecordBatch::new_unchecked are satisfied)
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,100 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
Review Comment:
I am not familiar enough with how the python interface works to know if this is reasonable or not. Perhaps @kylebarron can help review this part
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2510450735
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,120 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+ ) -> Result<Self, ArrowError> {
+ /// This function was copied from `pyo3_arrow/utils.rs` for now. I don't understand yet why
+ /// this is required instead of a "normal" `schema == record_batch.schema()` check.
+ ///
+ /// TODO: Either remove this check, replace it with something already existing in `arrow-rs`
+ /// or move it to a central `utils` location.
+ fn schema_equals(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields
+ .iter()
+ .zip(right.fields.iter())
+ .all(|(left_field, right_field)| {
+ left_field.name() == right_field.name()
+ && left_field
+ .data_type()
+ .equals_datatype(right_field.data_type())
+ })
+ }
+
+ for record_batch in &record_batches {
+ if !schema_equals(&schema, &record_batch.schema()) {
+ return Err(ArrowError::SchemaError(
+ //"All record batches must have the same schema.".to_owned(),
Review Comment:
```suggestion
```
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers [Table], a convenience wrapper for [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! that internally holds `Vec<RecordBatch>`, it is meant primarily for use cases where you already
+//! have `Vec<RecordBatch>` on the Rust side and want to export that in bulk as a `pyarrow.Table`.
+//! In general, it is recommended to use streaming approaches instead of dealing with data in bulk.
+//! For example, a `pyarrow.Table` (or any other object that implements the ArrayStream PyCapsule
+//! interface) can be imported to Rust through `PyArrowType<ArrowArrayStreamReader>>` instead of
Review Comment:
```suggestion
//! interface) can be imported to Rust through `PyArrowType<ArrowArrayStreamReader>` instead of
```
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,120 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+ ) -> Result<Self, ArrowError> {
+ /// This function was copied from `pyo3_arrow/utils.rs` for now. I don't understand yet why
+ /// this is required instead of a "normal" `schema == record_batch.schema()` check.
+ ///
+ /// TODO: Either remove this check, replace it with something already existing in `arrow-rs`
+ /// or move it to a central `utils` location.
+ fn schema_equals(left: &SchemaRef, right: &SchemaRef) -> bool {
+ left.fields
Review Comment:
This impl seems incorrect - the zip() operation does not check that the iterators have the same number of items. It actually checks that left is a subset of right or right is a subset of left. So, if `right` has one field more than `left` this function will still return `true`.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=95c113900129b392365cdfb3b4c2b4e6
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2504312554
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
Review Comment:
Wording might be slightly better as
```
Although arrow-rs offers Table, a convenience wrapper for `pyarrow.Table`, this is ...
```
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
+
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: Option<SchemaRef>,
Review Comment:
I think this should be
```suggestion
schema: SchemaRef,
```
to better mirror other APIs in arrow-rs
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! convenience wrapper [Table] (which internally holds `Vec<RecordBatch>`), this is more meant for
+//! use cases where you already have `Vec<RecordBatch>` on the Rust side and want to export that in
+//! bulk as a `pyarrow.Table`. In general, it is recommended to use streaming approaches instead of
+//! dealing with bulk data.
+//! For example, a `pyarrow.Table` can be imported to Rust through `PyArrowType<ArrowArrayStreamReader>`
+//! instead (since `pyarrow.Table` implements the ArrayStream PyCapsule interface).
Review Comment:
Well actually slight correction, assuming PyCapsule Interface input, both `Table` and `ArrowArrayStreamReader` will work with both table and stream input out of the box, the difference is just whether the Rust code materializes the data.
This is why I have this table in the pyo3-arrow docs:
<img width="801" height="267" alt="Image" src="https://github.com/user-attachments/assets/38d0e89d-c3bc-4e3b-a5ed-fcb11c9e7a78" />
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
+
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: Option<SchemaRef>,
+ ) -> Result<Self, ArrowError> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ record_batches
+ .get(0)
+ .ok_or_else(|| ArrowError::SchemaError(
+ "If no schema is supplied explicitly, there must be at least one RecordBatch!".to_owned()
+ ))?
+ .schema()
+ .clone()
+ }
+ };
+ for record_batch in &record_batches {
+ if schema != record_batch.schema() {
+ return Err(ArrowError::SchemaError(
+ "All record batches must have the same schema.".to_owned(),
+ ));
+ }
+ }
+ Ok(Self {
+ record_batches,
+ schema,
+ })
+ }
+
+ pub fn record_batches(&self) -> &[RecordBatch] {
+ &self.record_batches
+ }
+
+ pub fn schema(&self) -> SchemaRef {
+ self.schema.clone()
+ }
+
+ pub fn into_inner(self) -> (Vec<RecordBatch>, SchemaRef) {
+ (self.record_batches, self.schema)
+ }
+}
+
+impl TryFrom<ArrowArrayStreamReader> for Table {
Review Comment:
I think this would be better to implement on `Box<dyn RecordBatchReader>` or `T: RecordBatchReader`
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! convenience wrapper [Table] (which internally holds `Vec<RecordBatch>`), this is more meant for
+//! use cases where you already have `Vec<RecordBatch>` on the Rust side and want to export that in
+//! bulk as a `pyarrow.Table`. In general, it is recommended to use streaming approaches instead of
+//! dealing with bulk data.
+//! For example, a `pyarrow.Table` can be imported to Rust through `PyArrowType<ArrowArrayStreamReader>`
+//! instead (since `pyarrow.Table` implements the ArrayStream PyCapsule interface).
Review Comment:
Also reading through the docs again, I'd suggest making a reference to `Box<dyn RecordBatchReader>` rather than `ArrowArrayStreamReader`. The former is a higher level API and much easier to use.
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
+
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: Option<SchemaRef>,
+ ) -> Result<Self, ArrowError> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ record_batches
+ .get(0)
+ .ok_or_else(|| ArrowError::SchemaError(
+ "If no schema is supplied explicitly, there must be at least one RecordBatch!".to_owned()
+ ))?
+ .schema()
+ .clone()
+ }
+ };
+ for record_batch in &record_batches {
+ if schema != record_batch.schema() {
+ return Err(ArrowError::SchemaError(
+ "All record batches must have the same schema.".to_owned(),
+ ));
+ }
+ }
+ Ok(Self {
+ record_batches,
+ schema,
+ })
+ }
+
+ pub fn record_batches(&self) -> &[RecordBatch] {
+ &self.record_batches
+ }
+
+ pub fn schema(&self) -> SchemaRef {
+ self.schema.clone()
+ }
+
+ pub fn into_inner(self) -> (Vec<RecordBatch>, SchemaRef) {
+ (self.record_batches, self.schema)
+ }
+}
+
+impl TryFrom<ArrowArrayStreamReader> for Table {
+ type Error = ArrowError;
+
+ fn try_from(value: ArrowArrayStreamReader) -> Result<Self, ArrowError> {
+ let schema = value.schema();
+ let batches = value.collect::<Result<Vec<_>, _>>()?;
+ // We assume all batches have the same schema here.
+ unsafe { Ok(Self::new_unchecked(batches, schema)) }
+ }
+}
+
+impl FromPyArrow for Table {
+ fn from_pyarrow_bound(ob: &Bound<PyAny>) -> PyResult<Self> {
+ let array_stream_reader: PyResult<ArrowArrayStreamReader> = {
Review Comment:
Can we just do
```rs
let reader: Box<dyn RecordBatchReader> = FromPyArrow::from_pyarrow_bound(ob)?;
Self::try_from(reader)
```
We could have a discussion about what versions of pyarrow we need to support, but given that every pyarrow version of the last two years supports the PyCapsule Interface, I don't think we need to check for `to_reader` as well.
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
Review Comment:
I'd prefer removing `new_unchecked` and only having `try_new`. Do you have a need for `new_unchecked`? The validation to check schemas should be pretty minimal
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! convenience wrapper [Table] (which internally holds `Vec<RecordBatch>`), this is more meant for
+//! use cases where you already have `Vec<RecordBatch>` on the Rust side and want to export that in
+//! bulk as a `pyarrow.Table`. In general, it is recommended to use streaming approaches instead of
Review Comment:
```suggestion
//! bulk as a `pyarrow.Table`.
//!
//! In general, it is recommended to use streaming approaches instead of
```
(If this is meant to be a bulleted list, that might have to be indented)
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! convenience wrapper [Table] (which internally holds `Vec<RecordBatch>`), this is more meant for
+//! use cases where you already have `Vec<RecordBatch>` on the Rust side and want to export that in
+//! bulk as a `pyarrow.Table`. In general, it is recommended to use streaming approaches instead of
+//! dealing with bulk data.
+//! For example, a `pyarrow.Table` can be imported to Rust through `PyArrowType<ArrowArrayStreamReader>`
+//! instead (since `pyarrow.Table` implements the ArrayStream PyCapsule interface).
Review Comment:
I think it would be good to note here that another advantage of using `ArrowArrayStreamReader` is that it works with tables _and_ stream input out of the box. It doesn't matter which type the user passes in.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2582588902
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
>> What is current on main is pretty elegant
>
> `RecordBatch::from(StructArray::from(data)))`
I personally wouldn't call this elegant, as it's actually *currently* leading to unsoundness problems which I already stated a few times in this discussion :grimacing:
1. Since `StructArray` is metadata-less, the `RecordBatch` constructed from it is metadata-less by definition too. This means, the returned `RecordBatch` can't ever have the schema advertised by `ArrowArrayStreamReader`, at least if it's supposed to have metadata.
2. Much more importantly: There currently are no checks whether the `RecordBatch` constructed from `StructArray` and returned by the stream reader even corresponds to the schema which `ArrowArrayStreamReader` advertises. As far as I can see they are returned as-is, and in prinicple it's possible to make this interface return an different `RecordBatch`. This was the cause of this PR, namely that it's currently returning unsound `RecordBatch` without advertising this as an error somewhere.
The current state with `unsafe` in this PR more or less leaves problem 2 intact (with arguably introducing a chance of an additional invariant incorrectness in the `RecordBatch` itself), but at least fixes problem 1.
Nevertheless, I will proceed with introducing schema checking now and remove `unsafe`, which would also fix problem 2 in general, with a tiny additional compute cost.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2505370704
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -44,17 +44,20 @@
//! | `pyarrow.Array` | [ArrayData] |
//! | `pyarrow.RecordBatch` | [RecordBatch] |
//! | `pyarrow.RecordBatchReader` | [ArrowArrayStreamReader] / `Box<dyn RecordBatchReader + Send>` (1) |
+//! | `pyarrow.Table` | [Table] (2) |
//!
//! (1) `pyarrow.RecordBatchReader` can be imported as [ArrowArrayStreamReader]. Either
//! [ArrowArrayStreamReader] or `Box<dyn RecordBatchReader + Send>` can be exported
//! as `pyarrow.RecordBatchReader`. (`Box<dyn RecordBatchReader + Send>` is typically
//! easier to create.)
//!
-//! PyArrow has the notion of chunked arrays and tables, but arrow-rs doesn't
-//! have these same concepts. A chunked table is instead represented with
-//! `Vec<RecordBatch>`. A `pyarrow.Table` can be imported to Rust by calling
-//! [pyarrow.Table.to_reader()](https://arrow.apache.org/docs/python/generated/pyarrow.Table.html#pyarrow.Table.to_reader)
-//! and then importing the reader as a [ArrowArrayStreamReader].
+//! (2) Although arrow-rs offers a [pyarrow.Table](https://arrow.apache.org/docs/python/generated/pyarrow.Table)
+//! convenience wrapper [Table] (which internally holds `Vec<RecordBatch>`), this is more meant for
+//! use cases where you already have `Vec<RecordBatch>` on the Rust side and want to export that in
+//! bulk as a `pyarrow.Table`. In general, it is recommended to use streaming approaches instead of
+//! dealing with bulk data.
+//! For example, a `pyarrow.Table` can be imported to Rust through `PyArrowType<ArrowArrayStreamReader>`
+//! instead (since `pyarrow.Table` implements the ArrayStream PyCapsule interface).
Review Comment:
> I think it would be good to note here that another advantage of using ArrowArrayStreamReader is that it works with tables and stream input out of the box.
I added that in the docs.
> Also reading through the docs again, I'd suggest making a reference to Box<dyn RecordBatchReader> rather than ArrowArrayStreamReader. The former is a higher level API and much easier to use.
I'm not exactly sure what you mean here. `Box<dyn RecordBatchReader>` only implements *IntoPyArrow*, but not *FromPyArrow*. So in the example I state in the new documentation, that for *consuming* a `pyarrow.Table` in Rust, also a streaming approach could be used, the `Box<dyn RecordBatchReader>` isn't helping sadly. One has to use `ArrowArrayStreamReader`, since that properly implements `FromPyArrow`.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3504851078
Hey @kylebarron, thanks for the review! I implemented everything as you suggested.
As you can see, now the CI is broken, because of a suttle problem that was uncovered. Maybe you can help me, as I'm not too familiar with the FFI logic and all my sanity checks did not help me:
In the `test_table_roundtrip` Python test in `arrow-pyarrow-integration-test`, we're simply handing a `pyarrow.Table` to Rust and letting it roundtrip through the conversion layers back to a `pyarrow.Table`. Unfortunately, the conversion from `PyArrowType<Table>` -> `ArrowArrayStreamReader` -> `Table` is now failing, specifically the last part.
The `PyArrowType<Table>` -> `ArrowArrayStreamReader` conversion works, but as soon as `RecordBatch`es are read from the stream reader in the `try_new` [function](https://github.com/apache/arrow-rs/pull/8790/files#diff-2cc622072ff5fa80cf1a32a161da31ac058336ebedfeadbc8532fa52ea4224faR514) of `Table`, they lose their metadata. This fails then, because the `try_new` function validates that the schema of all record batches corresponds to the explicitly given schema.
The schema itself from the `ArrowArrayStreamReader` still has the metadata `{"key1": "value1"}` attached, but not the individual `RecordBatch`es. I left a somewhat verbose error message in the Rust error:
```
ValueError: Schema error: All record batches must have the same schema. Expected schema: Schema { fields: [Field { name: "ints", data_type: List(Field { data_type: Int32, nullable: true }), nullable: true }], metadata: {"key1": "value1"} }, got schema: Schema { fields: [Field { name: "ints", data_type: List(Field { data_type: Int32, nullable: true }), nullable: true }], metadata: {}
```
This previously worked because I used an `unsafe` interface for building a `Table` before which didn't check for schema validity.
Sanity checks:
- All other roundtrips work without problem, the metadata seems to be handed through all layers otherwise
- In the failing Python test `test_table_roundtrip`, I asserted that the `pyarrow.Table` definitly *has* the metadata still attached, and especially all RecordBatches from it
- Just in the conversion to Rust `RecordBatch` through the `Box<dyn ArrowArrayStreamReader>` they somehow seem to lose their metadata. Is this something which the FFI interface doesn't guarantee?
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3498377722
> I think that's outdated for Python -> Rust. I haven't tried but you should be able to pass a pyarrow.Table directly into an ArrowArrayStreamReader on the Rust side
Yes, exactly, that's what I even mentioned here in this PR (https://github.com/apache/arrow-rs/pull/8790/files#diff-2cc622072ff5fa80cf1a32a161da31ac058336ebedfeadbc8532fa52ea4224faR491-R492):
```
/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead)
```
As you said, the opposite, namely easily returning a `Vec<RecordBatch>` as a `pyarrow.Table` to Python is what's really missing here and what this PR mainly is about.
> I'm not sure I totally get your example. Seems bad to be returning a union of multiple types to Python.
My example wasn't entirely complete for simplicitly (and still isn't), it would be more something like this:
```Python
class ParquetFile:
@overload
def read_row_group(self, index: int, as_table: Literal[True]) -> pyarrow.Table: ...
@overload
def read_row_group(self, index: int, as_table: Literal[False]) -> pyarrow.RecordBatch: ...
def read_row_group(self, index: int, as_table: bool = False) -> pyarrow.RecordBatch | pyarrow.Table: ...
```
The advantage of that would be that both `pyrrow.RecordBatch` and `pyarrow.Table` implement `.to_list() -> list[dict[str, Any]]`. This is the important bit here, as we later just want to be able to call `to_pylist()` on whatever singular object `read_row_group(...)` returns and be guaranteed that the entire row group is deserialized as Python objects in this list. So it also could be expressed in our very specific example as:
```Python
class ToListCapable(Protocol):
def to_pylist(self) -> list[dict[str, Any]]: ...
class ParquetFile:
def read_row_group(self, index: int, as_table: bool = False) -> ToListCapable: ...
```
> The alternative is to return a stream and have the user either iterate over it lazily or choose to materialize it with pa.table(ParquetFile.read_row_group(...)).
&
> Well there's nothing stopping you from materializing the stream by passing it to pa.table(). You don't have to use the stream as a stream.
Yes, sure!. We also do that in other places, or have entirely streamable pipelines elsewhere that use the PyCapsule ArrowStream interface. It's just that for this very specific use case, a `Vec<RecordBatch>` -> `pyarrow.Table` convenience wrapper perfectly maps to what we need with no required changes in any consuming code, and I would be interested in whether maintainers of `arrow-pyarrow` find that useful for very specific niche use cases, as I said.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2585910539
##########
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:
I don't think you actually have to subclass from the prototype; the type checker will automatically check for structural type equality
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3499295595
I am not a python expert nor have I fully understood all the discussion on this ticket,
> At least I personally think having such a wrapper could be nice, since it simplifies stuff a bit when you anyways already have Vec<RecordBatch> on the Rust side somewhere or need to handle a pyarrow.Table on the Python side and want to have an easy method to generate such a thing from Rust. One still could mention in the documentation that generally, streaming approaches are highly preferred, and that the pyarrow.Table convenience wrapper shall only be used in cases where users know what they're doing.
This would be my preferred approach -- make it easy to go from Rust <> Python, while trying to encourage good practices (e.g. streaming). There is no reason to be pedantic and force someone through hoops to make PyTable if that is what they want
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2496363534
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +484,102 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// PyarrowType(Table::try_new(batches).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>) -> Self {
+ Self { record_batches }
+ }
+
+ pub fn try_new(record_batches: Vec<RecordBatch>) -> Result<Self, ArrowError> {
+ let schema = record_batches[0].schema();
Review Comment:
An Arrow table can be empty with no batches. It would probably be more reliable to store both batches and a standalone schema.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2532232970
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
> The rationale for this form rather than just converting StructArray to RecordBatch
Basically what I explained here (https://github.com/apache/arrow-rs/pull/8790#issuecomment-3536535648) => `StructArray` alone by definition is metadata-less, in turn leading to the problem that the resulting `RecordBatch` won't have any metadata attached if you just return it as-is.
I'm not sure whether there is another more elegant way to construct a `RecordBatch` with corresponding metadata from `ArrayData`. Right now I'm going through `StructArray` because the previous interface did that too. If there is another more elegant way, please let me know.
Other ways to attach metadata to an existing `RecordBatch` would be, as far as I can see, to call `with_schema()` (which will incure some "is subschema test" costs) or somehow through `schema_metadata_mut()`, but the interface feels a bit clunky for this specific task IMHO.
> A SAFETY comment explaining why the unsafe is ok (aka how the invariants required for RecordBatch::new_unchecked are satisfied)
One reason for the `unsafe` here is that I did not want to introduce performance penalties in comparison to what the interface did before (it just returned `RecordBatch` without checking whether it's actually corresponding to the schema of `ArrowArrayStreamReader`; and the schemas actually mismatched before my change, at least metadata-wise).
In principle `Iterator` of `ArrowArrayStreamReader` returns `Result`, so we can make this fallible through `RecordBatch::try_new(...)`. This would incur some costs though, such as checking each column for correct nullability, equal and correct row count, type checks, etc..
I would have guessed that at least data-wise the interface can be trusted and therefore the checks can be omitted? :sweat_smile: I'm really not the expert here, I would have assumed that someone from the `arrow-rs` team could have some opinion here :grimacing:
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3603348365
I've been at a conference or on vacation the last three weeks and am working through my backlog now. I'll try to review this today or tomorrow.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3502932264
I now added a bunch of tests in `arrow-pyarrow-integration-testing` and overhauled `Table` to allow exporting empty `pyarrow.Table`s.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2505380122
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
+
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: Option<SchemaRef>,
Review Comment:
Applied the suggestion
##########
arrow-pyarrow/src/lib.rs:
##########
@@ -484,6 +487,137 @@ impl IntoPyArrow for ArrowArrayStreamReader {
}
}
+/// This is a convenience wrapper around `Vec<RecordBatch>` that tries to simplify conversion from
+/// and to `pyarrow.Table`.
+///
+/// This could be used in circumstances where you either want to consume a `pyarrow.Table` directly
+/// (although technically, since `pyarrow.Table` implements the ArrayStreamReader PyCapsule
+/// interface, one could also consume a `PyArrowType<ArrowArrayStreamReader>` instead) or, more
+/// importantly, where one wants to export a `pyarrow.Table` from a `Vec<RecordBatch>` from the Rust
+/// side.
+///
+/// ```ignore
+/// #[pyfunction]
+/// fn return_table(...) -> PyResult<PyArrowType<Table>> {
+/// let batches: Vec<RecordBatch>;
+/// let schema: SchemaRef;
+/// PyArrowType(Table::try_new(batches, schema).map_err(|err| err.into_py_err(py))?)
+/// }
+/// ```
+#[derive(Clone)]
+pub struct Table {
+ record_batches: Vec<RecordBatch>,
+ schema: SchemaRef,
+}
+
+impl Table {
+ pub unsafe fn new_unchecked(record_batches: Vec<RecordBatch>, schema: SchemaRef) -> Self {
+ Self {
+ record_batches,
+ schema,
+ }
+ }
+
+ pub fn try_new(
+ record_batches: Vec<RecordBatch>,
+ schema: Option<SchemaRef>,
+ ) -> Result<Self, ArrowError> {
+ let schema = match schema {
+ Some(s) => s,
+ None => {
+ record_batches
+ .get(0)
+ .ok_or_else(|| ArrowError::SchemaError(
+ "If no schema is supplied explicitly, there must be at least one RecordBatch!".to_owned()
+ ))?
+ .schema()
+ .clone()
+ }
+ };
+ for record_batch in &record_batches {
+ if schema != record_batch.schema() {
+ return Err(ArrowError::SchemaError(
+ "All record batches must have the same schema.".to_owned(),
+ ));
+ }
+ }
+ Ok(Self {
+ record_batches,
+ schema,
+ })
+ }
+
+ pub fn record_batches(&self) -> &[RecordBatch] {
+ &self.record_batches
+ }
+
+ pub fn schema(&self) -> SchemaRef {
+ self.schema.clone()
+ }
+
+ pub fn into_inner(self) -> (Vec<RecordBatch>, SchemaRef) {
+ (self.record_batches, self.schema)
+ }
+}
+
+impl TryFrom<ArrowArrayStreamReader> for Table {
Review Comment:
Applied the suggestion
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3536513427
Hey, I pushed a version that actually does not use the PyCapsule ArrayStream interface for converting a `pyarrow.Table` to `Table`, if the given python object has a `to_batches()` method (which the `pyarrow.Table` does). This is not necessarily intended to stay that way, but this is helpful for diagnosing where RecordBatch metadata is dropped.
`pyarrow.Table.to_batches()` returns a `list[pyarrow.RecordBatch]` which I explicitly convert to `Vec<RecordBatch>` in the `from_pyarrow_bound` function of `impl FromPyArrow for Table`. This basically is the equivalent of what I'm doing in the corresponding `impl IntoPyArrow for Table`, as I'm not using the PyCapsule interface there, but just immediately construct a `pyarrow.Table` out of `Vec<RecordBatch>` through `pyarrow.Table.from_batches(...)`.
With that, I got RecordBatches with preserved metadata from `pyarrow.Table`, in turn allowing me to drop the `schema_equals` function but instead do a full `schema == record_batch.schema()` check.
Since I also checked on the Python side with a `pyarrow.RecordBatchReader.from_stream` of a `StreamWrapper` around a `pyarrow.Table` that RecordBatches from a ArrayStream PyCapsule interface of a `pyarrow.Table` definitely still have their metadata, the error has to be on the Rust side somewhere in the `Box<dyn RecordBatchReader>` / `impl FromPyArrow for ArrowArrayStreamReader` method.
Potentially there is a slight misuse of the PyCapsule interface somewhere, as this definitely seems to return RecordBatches without metadata. I'm not too familiar with the low-level stuff there, but I'll try to investigate; help is appreciated!
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "jonded94 (via GitHub)" <gi...@apache.org>.
jonded94 commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2582588902
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
>> What is current on main is pretty elegant
>
> `RecordBatch::from(StructArray::from(data)))`
I personally wouldn't call this elegant, as it's actually *currently* leading to unsoundness problems which I already stated a few times in this discussion :grimacing:
1. Since `StructArray` is metadata-less, the `RecordBatch` constructed from it is metadata-less by definition too. This means, the returned `RecordBatch` can't ever have the schema advertised by `ArrowArrayStreamReader`, at least if it's supposed to have metadata.
2. Much more importantly: There currently are no checks whether the `RecordBatch` constructed from `StructArray` and returned by the stream reader even corresponds to the schema which `ArrowArrayStreamReader` advertises. They are just returned as-is, and in prinicple it's possible to make this interface return an entirely different `RecordBatch`. This was the cause of this PR, namely that it's currently returning unsound `RecordBatch` without advertising this as an error somewhere.
The current state with `unsafe` in this PR more or less leaves problem 2 intact (with arguably introducing a chance of an additional invariant incorrectness in the `RecordBatch` itself), but at least fixes problem 1.
Nevertheless, I will proceed with introducing schema checking now and remove `unsafe`, which would also fix problem 2 in general, with a tiny additional compute cost.
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#discussion_r2582768750
##########
arrow-array/src/ffi_stream.rs:
##########
@@ -364,7 +364,13 @@ impl Iterator for ArrowArrayStreamReader {
let result = unsafe {
from_ffi_and_data_type(array, DataType::Struct(self.schema().fields().clone()))
};
- Some(result.map(|data| RecordBatch::from(StructArray::from(data))))
+ Some(result.map(|data| {
+ let struct_array = StructArray::from(data);
Review Comment:
> Basically what I explained here ([#8790 (comment)](https://github.com/apache/arrow-rs/pull/8790#issuecomment-3536535648)) => `StructArray` alone by definition is metadata-less, in turn leading to the problem that the resulting `RecordBatch` won't have any metadata attached if you just return it as-is.
>
> I'm not sure whether there is another more elegant way to construct a `RecordBatch` with corresponding metadata from `ArrayData`. Right now I'm going through `StructArray` because the previous interface did that too. If there is another more elegant way, please let me know.
I've complained about this before (though I can't find in what issue), and is one of [the reasons I document](https://docs.rs/pyo3-arrow/0.15.0/pyo3_arrow/#why-not-use-arrow-rss-python-integration) for why I created pyo3-arrow. It's currently impossible (I believe) in arrow-rs to persist extension metadata through the FFI interface.
I think we need a broader PR to handle this though; it shouldn't be shoehorned into this PR that is focused on the PyArrow Table handling
--
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] Implement a `Vec` wrapper for `pyarrow.Table` convenience [arrow-rs]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on PR #8790:
URL: https://github.com/apache/arrow-rs/pull/8790#issuecomment-3497654509
> one has to do slight workarounds to use them:
I think that's outdated for Python -> Rust. I haven't tried but you should be able to pass a `pyarrow.Table` directly into an `ArrowArrayStreamReader` on the Rust side, because it just looks for the `__arrow_c_stream__` method that exists either on the `Table` or the `pyarrow.RecordBatchReader`.
But I assume there's no way today to easily return a `Table` from Rust to Python.
> At least I personally think having such a wrapper could be nice, since it simplifies stuff a bit when you anyways already have `Vec<RecordBatch>` on the Rust side somewhere or need to handle a `pyarrow.Table` on the Python side and want to have an easy method to generate such a thing from Rust.
I'm fine with that; and I think other maintainers would probably be fine with that too, since it's only a concept that exists in the Python integration.
I'm not sure I totally get your example. Seems bad to be returning a union of multiple types to Python. But seems reasonable to return a `Table` there. The alternative is to return a stream and have the user either iterate over it lazily or choose to materialize it with `pa.table(ParquetFile.read_row_group(...))`.
> And just for clarity, we unfortunately _need_ to have the entire Row group deserialized as Python objects because our data ingestion pipelines that consume this are expecting to have access to the entire row group in bulk, so streaming approaches are sadly not usable.
Well there's nothing stopping you from materializing the stream by passing it to `pa.table()`. You don't have to use the stream as a stream.
> Yes, in general, I much prefer the approach of `arro3` to be totally `pyarrow` agnostic. In our case unfortunately, we're right now still pretty hardcoded against `pyarrow` specifics and just use `arrow-rs` as a means to reduce memory load compared to reading & writing parquet datasets with `pyarrow` directly.
You can use `pyo3-arrow` with `pyarrow` as well, but I'm not opposed to adding this functionality to arrow-rs as well.
--
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