-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
REF/PERF: ArrowExtensionArray.__setitem__ #50632
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
Conversation
Not for this PR but because I'm cranky: the fact that we have to implement a kludgy |
This was a (failed?) attempt to make it a little less kludgy :). Agreed its still kludgy. Part of the complexity is supporting older versions of pyarrow where the pyarrow.compute functions have bugs we need to work around. We could remove the fast paths if simplicity is preferred. I'm not familiar with the buffers but could take a look. |
Definitely don't put time into this on my account. I've got a bug in my bonnet about preserving views (xref #45419) that becomes less important with CoW. Plus as mentioned, im not confident that the buffers() approach is even feasible. |
@@ -172,7 +172,6 @@ def test_setitem(multiple_chunks, key, value, expected): | |||
|
|||
result[key] = value | |||
tm.assert_equal(result, expected) | |||
assert result._data.num_chunks == expected._data.num_chunks |
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.
So this no longer holds?
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.
The existing implementation iterated through the chunks and set values chunk by chunk. This implementation passes the entire ChunkedArray
to pyarrow's compute functions. At the moment it looks like pyarrow.compute.if_else
combines chunks (but still returns a ChunkedArray with one chunk) whereas pyarrow.compute.replace_with_mask
maintains the chunking layout of the input. I'm not sure if that behavior applies for all cases or based on inputs. Let me know if you think pandas should ensure chunking layout remains unchanged.
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.
Generally I think we should try to maintain the chunking layout as much as possible, but it's more of an implementation detail and if the pyarrow compute functions don't necessarily maintain the chunking layout I suppose this is fine
|
||
if isinstance(data, pa.Array): | ||
data = pa.chunked_array([data]) | ||
self._data = data |
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.
Are we guaranteed here that self._data.type
matches self.dtype.pyarrow_dtype
?
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.
Yes, I believe so. I addressed the TODO in ArrowExtensionArray._maybe_convert_setitem_value
so that it now boxes the setitem values will raise if the replacement values cannot be cast to the original self._data.type.
Thanks @lukemanley |
doc/source/whatsnew/v2.0.0.rst
file if fixing a bug or adding a new feature.Simplifies and improves performance of
ArrowExtensionArray.__setitem__
via more use of pyarrow compute functions and less back and forth through numpy.ASVs: