-
Notifications
You must be signed in to change notification settings - Fork 27
feat: accept iterables in RecordBatchReader.from_batches #495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
454881a
4bd49b3
f715c1b
26d80ab
059fefb
799b6ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,13 @@ | ||
| use std::fmt::Display; | ||
| use std::sync::{Arc, Mutex}; | ||
|
|
||
| use arrow_array::{ArrayRef, RecordBatchIterator, RecordBatchReader, StructArray}; | ||
| use arrow_schema::{Field, SchemaRef}; | ||
| use arrow_array::{ArrayRef, RecordBatch, RecordBatchIterator, RecordBatchReader, StructArray}; | ||
| use arrow_schema::{ArrowError, Field, SchemaRef}; | ||
| use pyo3::exceptions::{PyIOError, PyStopIteration, PyValueError}; | ||
| use pyo3::intern; | ||
| use pyo3::prelude::*; | ||
| use pyo3::types::{PyCapsule, PyTuple, PyType}; | ||
| use pyo3::Borrowed; | ||
|
|
||
| use crate::error::PyArrowResult; | ||
| use crate::export::{Arro3RecordBatch, Arro3Schema, Arro3Table}; | ||
|
|
@@ -19,6 +20,59 @@ use crate::input::AnyRecordBatch; | |
| use crate::schema::display_schema; | ||
| use crate::{PyRecordBatch, PySchema, PyTable}; | ||
|
|
||
| /// Input for `from_batches`: either a sequence (extracted eagerly) or an | ||
| /// arbitrary iterable (consumed lazily via GIL-acquiring iterator). | ||
| enum RecordBatchInput { | ||
| Sequence(Vec<PyRecordBatch>), | ||
| Iterator(Py<PyAny>), | ||
|
Comment on lines
+26
to
+27
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to double-check here what pyo3 auto-converts to a sequence. It'll convert anything with an Perhaps it makes sense to make a one-off test function just to validate how it handles inputs. We don't want it automatically materializing iterators to a sequence. |
||
| } | ||
|
|
||
| impl<'a, 'py> FromPyObject<'a, 'py> for RecordBatchInput { | ||
| type Error = PyErr; | ||
|
|
||
| fn extract(ob: Borrowed<'a, 'py, PyAny>) -> PyResult<Self> { | ||
| if let Ok(vec) = ob.extract::<Vec<PyRecordBatch>>() { | ||
| Ok(Self::Sequence(vec)) | ||
| } else { | ||
| let iter = ob.call_method0(intern!(ob.py(), "__iter__"))?; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we should have a better error message. If
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By default the |
||
| Ok(Self::Iterator(iter.unbind())) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// A RecordBatchReader that lazily pulls batches from a Python iterator. | ||
| /// | ||
| /// Each call to `next()` acquires the GIL and calls `__next__` on the | ||
| /// underlying Python iterator. | ||
| struct PyIteratorRecordBatchReader { | ||
| schema: SchemaRef, | ||
| iter: Py<PyAny>, | ||
| } | ||
|
Comment on lines
+43
to
+50
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a performance regression for any existing case of a sequence of batches, because now it needs to acquire the GIL for every iteration
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a micro-benchmark, absolutely. In real world cases where record batches contain a meaningful amount of data, probably not. If you feel strongly, then I can probably special case Sequence from Iterable to win very microsecond back.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think acquiring the GIL can have real-world performance overhead and I'd rather not introduce it for the existing approach where it doesn't need it. You can define an enum like enum PyRecordBatchReaderInput {
Sequence(Vec<PyRecordBatch>)
Iterator(PyRecordBatchIterator)
}and then implement
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries. Done. |
||
|
|
||
| impl Iterator for PyIteratorRecordBatchReader { | ||
| type Item = Result<RecordBatch, ArrowError>; | ||
|
|
||
| fn next(&mut self) -> Option<Self::Item> { | ||
| Python::attach(|py| { | ||
| let iter = self.iter.bind(py); | ||
| match iter.call_method0(intern!(py, "__next__")) { | ||
| Ok(obj) => match obj.extract::<PyRecordBatch>() { | ||
| Ok(batch) => Some(Ok(batch.into_inner())), | ||
| Err(e) => Some(Err(ArrowError::ExternalError(Box::new(e)))), | ||
| }, | ||
| Err(e) if e.is_instance_of::<PyStopIteration>(py) => None, | ||
| Err(e) => Some(Err(ArrowError::ExternalError(Box::new(e)))), | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl RecordBatchReader for PyIteratorRecordBatchReader { | ||
| fn schema(&self) -> SchemaRef { | ||
| self.schema.clone() | ||
| } | ||
| } | ||
|
|
||
| /// A Python-facing Arrow record batch reader. | ||
| /// | ||
| /// This is a wrapper around a [RecordBatchReader]. | ||
|
|
@@ -217,15 +271,23 @@ impl PyRecordBatchReader { | |
| } | ||
|
|
||
| #[classmethod] | ||
| fn from_batches(_cls: &Bound<PyType>, schema: PySchema, batches: Vec<PyRecordBatch>) -> Self { | ||
| let batches = batches | ||
| .into_iter() | ||
| .map(|batch| batch.into_inner()) | ||
| .collect::<Vec<_>>(); | ||
| Self::new(Box::new(RecordBatchIterator::new( | ||
| batches.into_iter().map(Ok), | ||
| schema.into_inner(), | ||
| ))) | ||
| fn from_batches(_cls: &Bound<PyType>, schema: PySchema, batches: RecordBatchInput) -> Self { | ||
| let schema = schema.into_inner(); | ||
| match batches { | ||
| RecordBatchInput::Sequence(vec) => { | ||
| let batches = vec | ||
| .into_iter() | ||
| .map(|batch| batch.into_inner()) | ||
| .collect::<Vec<_>>(); | ||
| Self::new(Box::new(RecordBatchIterator::new( | ||
| batches.into_iter().map(Ok), | ||
| schema, | ||
| ))) | ||
| } | ||
| RecordBatchInput::Iterator(iter) => { | ||
| Self::new(Box::new(PyIteratorRecordBatchReader { schema, iter })) | ||
| } | ||
| } | ||
|
Comment on lines
+277
to
+290
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I'd move this onto a private method on Then this call site can stay clean |
||
| } | ||
|
|
||
| #[classmethod] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should clarify; iterators are probably also consumed lazily.