-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-44651: [Python] Allow from_buffers to work with StringView on Python #44701
Conversation
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.
Looks good!
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.
I'll be merging this as it was approved with a single minor comment about the error message formatting which has been updated
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit aa26f28. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 27 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Currently
from_buffers
is not working with StringView on Python because we validate against num_buffers. This only take into account the mandatory buffers but does not take into account the variadic_spec that can be present for both string_view and binary_viewWhat changes are included in this PR?
Take into account whether the type contains a variadic_spec for the non-mandatory buffers and only check lower_bound number of buffers.
Are these changes tested?
Yes, I've added a couple of tests.
Are there any user-facing changes?
We are exposing a new method on the Python DataType.
has_variadic_buffers
which tells us whether the number of buffers expected is only lower-bounded by num_buffers.