You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "wjones127 (via GitHub)" <gi...@apache.org> on 2023/08/30 03:21:35 UTC
[GitHub] [arrow-rs] wjones127 opened a new pull request, #4751: Implement PyArrowType for `Box`
wjones127 opened a new pull request, #4751:
URL: https://github.com/apache/arrow-rs/pull/4751
# 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?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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
[GitHub] [arrow-rs] wjones127 commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1310849337
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
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`.
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1310460013
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
I'm guessing this is necessary because RecordBatchReader is a foreign trait, and so we can't provide a blanket impl as `RecordBatchReader` could be implemeneted for `Vec` or something silly. As you say it ultimately makes no difference as we end up boxing anyway
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1311508557
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
Created https://github.com/apache/arrow-rs/pull/4757 which I think would remove the need for this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1310467474
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
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
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1311507218
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
Hmm... Perhaps we can relax those constraints :thinking:
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1310460013
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
I'm guessing this is necessary because RecordBatchReader is a foreign trait, and so we can't provide a blanket impl as `RecordBatchReader` could be implemeneted for `Vec` or something silly. As you say it ultimately makes no difference as we end up boxing anyway
--
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
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4751: Implement PyArrowType for `Box`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4751:
URL: https://github.com/apache/arrow-rs/pull/4751#discussion_r1310460013
##########
arrow/src/pyarrow.rs:
##########
@@ -277,10 +278,19 @@ impl FromPyArrow for ArrowArrayStreamReader {
}
}
-impl IntoPyArrow for ArrowArrayStreamReader {
+impl FromPyArrow for Box<dyn RecordBatchReader + Send> {
Review Comment:
I'm guessing this is necessary because RecordBatchReader is a foreign trait, and so we can't provide a blanket impl as `RecordBatchReader` could be implemeneted for `Vec` or something silly
--
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