GH-41692: [Python] Improve substrait extended expressions support #41693
Conversation
|
|
| @@ -56,7 +56,7 @@ Status ParseFromBufferImpl(const Buffer& buf, const std::string& full_name, | |||
| if (message->ParseFromZeroCopyStream(&buf_stream)) { | |||
| return Status::OK(); | |||
| } | |||
| return Status::IOError("ParseFromZeroCopyStream failed for ", full_name); | |||
| return Status::Invalid("ParseFromZeroCopyStream failed for ", full_name); | |||
There was a problem hiding this comment.
It seemed odd that it failed with an IOError given that the ArrayInputStream was built from already in-memory Buffer, so no IO was involved. If it fails, it actually means that the Substrait data is invalid
github-actions
bot
added
awaiting committer review
|
Marking as ready for review as the failures seem to be unrelated, @jorisvandenbossche would you mind reviewing this when you have the chance? |
There was a problem hiding this comment.
Even though my knowledge of this is limited I'll take some time to review tomorrow. Would you mind rebasing to see if the failing CI issues are solved (I haven't seen a bunch of those jobs failing for a while).
I plan to start 18.0.0 next Wednesday and would love to ensure this won't break CI.
github-actions
bot
added
awaiting changes
|
👍 rebased @raulcd |
|
@github-actions crossbow submit preview-docs |
|
Revision: d24f0f0 Submitted crossbow builds: ursacomputing/crossbow @ actions-1e09098ee5
|
There was a problem hiding this comment.
Even though my knowledge of this is limited I'll take some time to review tomorrow.
I've tried to review but to be honest my knowledge around this is pretty limited so in general the change LGTM but would let you / others decide whether to merge this.
CI and docs look good to me
github-actions
bot
added
awaiting changes
Co-authored-by: Raúl Cumplido <[email protected]>
github-actions
bot
added
awaiting change review
|
going to merge this as it shouldn't impact user base at the moment and it didn´t get further comments. We can revise if there are concerns as there is time before v19 |
|
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit e0ab40d. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Addresses some missing features and usability issues when using PyArrow with Substrait ExtendedExpressions
BoundExpressionsforScanner(columns=X)instead of a dict of expressions.BoundExpressionsforScanner(filter=X)so that user doesn't have to distinguish betweenExpressionandBoundExpressionsand can always just usepyarrow.substrait.deserialize_expressionspyarrow.BoundExpressionsdirectly fromprotobuf.Message, thus allowing to use substrait-python objects.memoryviewfrom methods encoding substrait, so that those can be directly passed to substrait-python (or more in general other python libraries) without a copy being involved.memoryviewso that the output of encoding functions can be sent back to dencoding functions.