-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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-39651: [Python] Basic pyarrow bindings for Binary/StringView classes #39652
GH-39651: [Python] Basic pyarrow bindings for Binary/StringView classes #39652
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.
Overall LGTM!
cdef class StringViewArray(Array): | ||
""" | ||
Concrete class for Arrow arrays of string (or utf8) view data type. | ||
""" |
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.
Is it worth adding TODOs or linking future GH issues?
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.
It's unclear if we need to add any specific method here (at least it's not needed for the current TODO items in the parent issue), so going to leave this as is for now.
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 787afa1. There were 2 benchmark results indicating a performance regression:
The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them. |
@jorisvandenbossche It seems that AppVeyor started failing by this commit. Could you take a look at this? |
Ah I assumed it was an unrelated failure .. Indeed seems to have started with this PR, but not directly any idea what could cause this |
… classes (apache#39652) ### Rationale for this change First step for apache#39633: exposing the Array, DataType and Scalar classes for BinaryView and StringView, such that those can already be represented in pyarrow. (I exposed a variant of StringBuilder as well, just for now to be able to create test data) * Closes: apache#39651 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>
… classes (apache#39652) ### Rationale for this change First step for apache#39633: exposing the Array, DataType and Scalar classes for BinaryView and StringView, such that those can already be represented in pyarrow. (I exposed a variant of StringBuilder as well, just for now to be able to create test data) * Closes: apache#39651 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>
… classes (apache#39652) ### Rationale for this change First step for apache#39633: exposing the Array, DataType and Scalar classes for BinaryView and StringView, such that those can already be represented in pyarrow. (I exposed a variant of StringBuilder as well, just for now to be able to create test data) * Closes: apache#39651 Authored-by: Joris Van den Bossche <[email protected]> Signed-off-by: Joris Van den Bossche <[email protected]>
Rationale for this change
First step for #39633: exposing the Array, DataType and Scalar classes for BinaryView and StringView, such that those can already be represented in pyarrow.
(I exposed a variant of StringBuilder as well, just for now to be able to create test data)