-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
ENH: Implement more efficient merge for arrow strings #54443
Conversation
): | ||
lk, _ = lk._values_for_factorize() | ||
|
||
# error: Item "ndarray" of "Union[Any, ndarray]" has no attribute | ||
# "_values_for_factorize" | ||
rk, _ = rk._values_for_factorize() # type: ignore[union-attr] | ||
elif isinstance(lk.dtype, ArrowDtype) and is_string_dtype(lk.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.
no kidding my reaction on reading this was to sigh and then say out loud "gross". let's take this opportunity to implement the appropriate EA method
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.
:) As discussed, let's do as a follow up. But I agree that this is needed
A whatsnew note would be good otherwise LGTM |
Thanks @phofl |
) | ||
if how == "right": | ||
return rlab, llab, count | ||
return llab, rlab, count |
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.
should the sort
keyword matter somewhere in here?
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.
Follow-up. This didn't cover sort. Wanted to get the other PR in first
* ENH: Implement more efficient merge for arrow strings * Fix typing * Update * ENH: Implement more efficient merge for arrow strings
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.lets see if this passes ci
Main: 1.4701979160308838
PR: 0.6503970623016357