pyarrow: Small code simplifications#9594
Conversation
- use `.call_method0(M)?` instead of `.getattr(M)?.call0()` - Use `.extract()` that allows more advanced features like directly extracting tuple elements - remove temporary variables just before returning - use &raw const and &raw mut pointers instead of casting and addr_of!
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Looks like a nice cleanup to me -- thank you @Tpt
FYI @kylebarron in case you would also like a chance to review
| let capsule = value.getattr("__arrow_c_schema__")?.call0()?; | ||
| let capsule = capsule.cast::<PyCapsule>()?; | ||
| validate_pycapsule(capsule, "arrow_schema")?; | ||
| let capsule = value.call_method0("__arrow_c_schema__")?.extract()?; |
There was a problem hiding this comment.
looks like the corret usecase: https://docs.rs/pyo3/latest/pyo3/prelude/trait.PyAnyMethods.html#tymethod.call_method0
kylebarron
left a comment
kylebarron
left a comment
There was a problem hiding this comment.
Seems fine. I didn't know about &raw mut and &raw const
| if !tuple.is_instance_of::<PyTuple>() { | ||
| return Err(PyTypeError::new_err( | ||
| "Expected __arrow_c_array__ to return a tuple.", | ||
| )); | ||
| } |
There was a problem hiding this comment.
fwiw previously there was a nice user-facing error here, while now the error generated from extract will be much more obtuse. Ideally this exception will never be raised except if the producer doesn't follow the spec correctly.
There was a problem hiding this comment.
I will make a follow on PR to keep the message
There was a problem hiding this comment.
|
Thanks again @Tpt and @kylebarron |
# Which issue does this PR close? - Follow on to #9594 # Rationale for this change @kylebarron says #9594 (comment): > fwiw previously there was a nice user-facing error here, while now the error generated from extract will be much more obtuse. Ideally this exception will never be raised except if the producer doesn't follow the spec correctly. # What changes are included in this PR? Restore the nice error # Are these changes tested? yes, added a test # 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 call them out. -->
3 participants
Rationale for this change
Makes the code simpler and more readable by relying on new PyO3 and Rust features. No behavior should have changed outside of an error message if
__arrow_c_array__does not return a tupleWhat changes are included in this PR?
.call_method0(M)?instead of.getattr(M)?.call0().extract()that allows more advanced features like directly extracting tuple elements