-
Notifications
You must be signed in to change notification settings - Fork 849
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
Implement PyArrowType for Box<dyn RecordBatchReader + Send>
#4751
Conversation
arrow/src/pyarrow.rs
Outdated
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader { | |||
} | |||
} | |||
|
|||
impl IntoPyArrow for ArrowArrayStreamReader { | |||
impl FromPyArrow for Box<dyn RecordBatchReader + Send> { |
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.
What is the advantage of this over just using ArrowArrayStreamReader directly?
As an aside we should probably document this method better, it isn't immediately clear it can be passed a pyarrow RecordBatchReader
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.
TBH I wouldn't have implemented FromPyArrow
if it weren't required by PyArrowType
. But you need to implement both IntoPyArrow
and FromPyArrow
in order to get PyArrowType
.
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.
Hmm... Perhaps we can relax those constraints 🤔
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.
Created #4757 which I think would remove the need for this
8675826
to
656ac57
Compare
Which issue does this PR close?
Closes #4730.
I couldn't get it to work for a generic RBR, but it needs to be boxed anyways.
Rationale for this change
Saves the user the step of creating an
ArrowArrayStreamReader
when exporting a record batch reader.What changes are included in this PR?
Are there any user-facing changes?