REF (string): de-duplicate ArrowStringArray methods #59555

jbrockmendel · 2024-08-19T21:38:16Z

closes #xxxx (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

jorisvandenbossche · 2024-08-20T19:33:45Z

Could we do it the other way around? i.e. have the method in ArrowExtensionArray point to the one defined in ArrowStringArray?
Personally, I feel it better belongs in string_arrow.py, to keep the string specific functionality in there (the ArrowExtensionArray definition is already huge ..)

jbrockmendel · 2024-08-20T20:36:21Z

Could we do it the other way around? i.e. have the method in ArrowExtensionArray point to the one defined in ArrowStringArray?

I think the thing to do is put it in the ArrowStringArrayMixin. I'm vaguely planning to move all the relevant stuff up there after the current crop of branches (some still local) are done.

mroeschke · 2024-08-21T17:38:00Z

If we're doing this moving of methods away from ArrowExtensionArray, using pandas.ArrowDtype(pyarrow.string()) will continue to use NA semantics correct?

jbrockmendel · 2024-08-21T18:01:22Z

This REF is behavior-neutral

…

On Wed, Aug 21, 2024 at 10:38 AM Matthew Roeschke ***@***.***> wrote: If we're doing this moving of methods away from ArrowExtensionArray, using pandas.ArrowDtype(pyarrow.string()) will continue to use NA semantics correct? — Reply to this email directly, view it on GitHub <#59555 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5UM6BAA2VILEGLUIJ6H33ZSTGA5AVCNFSM6AAAAABMYUEVH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBSGYZDIMBUGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

jorisvandenbossche · 2024-08-21T19:40:01Z

I'm vaguely planning to move all the relevant stuff up there after the current crop of branches (some still local) are done.

If that's the plan, my preference would be do to it at once, instead of spread over multiple PRs. I know we generally want smaller more granular PRs, but given that this will need to be backported, doing this in one go instead of multiple steps will make that easier.

jorisvandenbossche · 2024-08-21T20:16:08Z

I think the thing to do is put it in the ArrowStringArrayMixin.

Also a question on the mixin. What's the reason we need the mixin? (compared to keeping things in ArrowStringArray itself)

Right now ArrowStringArray doesn't actually inherit from the mixin, but uses explicit _str_method = ArrowStringArrayMixin._str_method (well, that's actually only done in ArrowStringArrayNumpySemantics, I see, and not in ArrowStringArray, that's another question).
So using that pattern, we could also keep things in ArrowStringArray, and add the methods to ArrowExtensionArray by doing _str_method = ArrowStringArray._str_method in that class.

Or is the idea that ArrowStringArray(NumpySemantics) eventually actually inherits from ArrowStringArrayMixin?

jbrockmendel · 2024-08-22T16:33:43Z

Also a question on the mixin. What's the reason we need the mixin? (compared to keeping things in ArrowStringArray itself)

It is not really needed. ArrowEA subclasses the mixin and ArrowStringArray subclasses ArrowEA. So all the methods could go directly on ArrowEA, its just that class is already really bulky. Putting the methods directly on ArrowStringArray and reversing the inheritance would surely be possible but that would be more invasive.

jorisvandenbossche · 2024-08-22T16:56:49Z

Ah, yes, I forgot about ArrowStringArray subclassing ArrowExtensionArray and therefore also subclassing the mixin.
But the problem with that inheritance is that currently it's not actually used in ArrowStringArray, right? Because ArrowStringArray also subclasses ObjectStringArrayMixin which will get chosen first as fall-back for a method that doesn't exist on ArrowStringArray

That means we should either add the methods explicitly with _str_method = ArrowStringArrayMixin._str_method, or let ArrowStringArray subclass ArrowStringArrayMixin directly with the proper order. However, I think the latter does not work? ("Cannot create a consistent method resolution order (MRO)")

jbrockmendel · 2024-08-22T17:00:54Z

That means we should either add the methods explicitly with _str_method = ArrowStringArrayMixin._str_method

Yes, one of my local de-dup branches does this for the relevant methods.

jorisvandenbossche · 2024-08-27T09:45:50Z

pandas/core/arrays/string_arrow.py

+    _str_strip = ArrowExtensionArray._str_strip
+    _str_lstrip = ArrowExtensionArray._str_lstrip
+    _str_rstrip = ArrowExtensionArray._str_rstrip
+    _str_removesuffix = ArrowStringArrayMixin._str_removesuffix


_str_removesuffix is also assigned in ArrowStringArrayNumpySemantics below. That can then be removed there?

good catch, will update

jorisvandenbossche · 2024-08-27T09:49:20Z

If we deduplicate those methods, my preference would be to directly move those methods from ArrowEA to the mixin, and get them from there in ArrowStringArray. Or is there a reason we do not do that here, but use that pattern (shared methods in the mixin instead of in ArrowEA) for other methods?

jbrockmendel · 2024-08-27T14:49:15Z

my preference would be to directly move those methods from ArrowEA to the mixin, and get them from there in ArrowStringArray.

Sure, will update. Will wait until after #59615, #59568, #59562 and then rebase on top of those to minimize headaches.

jorisvandenbossche

Looks good! Some small comments

jorisvandenbossche · 2024-09-09T15:03:03Z

pandas/core/arrays/string_arrow.py

-        else:
-            result = pc.utf8_rtrim(self._pa_array, characters=to_strip)
-        return type(self)(result)
+        return ArrowExtensionArray._str_slice(self, start=start, stop=stop, step=step)


Suggested change

return ArrowExtensionArray._str_slice(self, start=start, stop=stop, step=step)

return ArrowStringArrayMixin._str_slice(self, start=start, stop=stop, step=step)

?

I see you didn't actually move _str_slice so the above wouldn't work. But could you move that as well? (most other methods you are touching here are now in the mixin class I think)

Could also leave out slice because you are fixing that in a separate PR

sure, will update after the slice PR goes in

jorisvandenbossche · 2024-09-09T15:03:49Z

pandas/core/arrays/string_arrow.py

-            removed = pc.utf8_slice_codeunits(self._pa_array, len(prefix))
-            result = pc.if_else(starts_with, removed, self._pa_array)
-            return type(self)(result)
+            return ArrowExtensionArray._str_removeprefix(self, prefix)


Suggested change

return ArrowExtensionArray._str_removeprefix(self, prefix)

return ArrowStringArrayMixin._str_removeprefix(self, prefix)

Just for consistency, because for the direct assignments we point to ArrowStringArrayMixin

sure, will update

jorisvandenbossche · 2024-09-09T15:07:21Z

pandas/core/arrays/string_arrow.py

+    _str_slice_replace = ArrowStringArrayMixin._str_slice_replace
+    _str_len = ArrowStringArrayMixin._str_len
+
+    _rank = ArrowExtensionArray._rank


This one is strictly speaking not needed? (because it is only otherwise defined on the base EA class, so ArrowEA comes first in the method resolution order)

sure, will remove

jorisvandenbossche · 2024-09-10T19:51:33Z

There are a couple of failures after the latest update

jorisvandenbossche · 2024-09-11T19:39:54Z

Those failures are happening on other PRs as well, I think, so merging.

jbrockmendel mentioned this pull request Aug 19, 2024

REF (string): de-duplicate ArrowStringArray methods (2) #59556

Closed

jbrockmendel changed the title ~~REF: de-duplicate ArrowStringArray methods~~ REF (string): de-duplicate ArrowStringArray methods Aug 20, 2024

mroeschke added Refactor Internal refactoring of code Strings String extension data type and string data Arrow pyarrow functionality labels Aug 21, 2024

jorisvandenbossche mentioned this pull request Aug 21, 2024

BUG (string): ArrowStringArray.find corner cases #59562

Merged

5 tasks

jorisvandenbossche reviewed Aug 27, 2024

View reviewed changes

jbrockmendel force-pushed the ref-dedup branch from b32e828 to 3d9366c Compare August 27, 2024 14:51

mroeschke added this to the 2.3 milestone Aug 28, 2024

jbrockmendel force-pushed the ref-dedup branch 3 times, most recently from 839f8d2 to 2bbf515 Compare September 6, 2024 16:11

jorisvandenbossche reviewed Sep 9, 2024

View reviewed changes

jbrockmendel added 6 commits September 9, 2024 09:00

REF: de-duplicate arrow string methods

5464b03

REF: de-duplicate ArrowStringArray methods

284b889

CLN: remove redundant override

5e0679e

REF: re-use ArrowEA methods in ArrowStringArray

0bd75c0

REF: move implementations to mixing class

7afa160

re-order

fb17fb4

suggested edits

feb30a4

mroeschke requested a review from jorisvandenbossche September 9, 2024 17:39

Merge branch 'main' into ref-dedup

f970e3f

jbrockmendel force-pushed the ref-dedup branch from 7f891c7 to f970e3f Compare September 10, 2024 14:13

REF: move implementations to mixin

8f430f9

jorisvandenbossche approved these changes Sep 11, 2024

View reviewed changes

jorisvandenbossche merged commit 4444e52 into pandas-dev:main Sep 11, 2024
42 of 47 checks passed

jbrockmendel deleted the ref-dedup branch September 18, 2024 18:48

jorisvandenbossche added the backported label Oct 10, 2024

jorisvandenbossche pushed a commit to jorisvandenbossche/pandas that referenced this pull request Oct 10, 2024

REF (string): de-duplicate ArrowStringArray methods (pandas-dev#59555)

c8eadfd

jorisvandenbossche pushed a commit that referenced this pull request Oct 10, 2024

REF (string): de-duplicate ArrowStringArray methods (#59555)

2f1caf5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

REF (string): de-duplicate ArrowStringArray methods #59555

REF (string): de-duplicate ArrowStringArray methods #59555

jbrockmendel commented Aug 19, 2024

jorisvandenbossche commented Aug 20, 2024

jbrockmendel commented Aug 20, 2024

mroeschke commented Aug 21, 2024

jbrockmendel commented Aug 21, 2024 via email

jorisvandenbossche commented Aug 21, 2024

jorisvandenbossche commented Aug 21, 2024

jbrockmendel commented Aug 22, 2024

jorisvandenbossche commented Aug 22, 2024

jbrockmendel commented Aug 22, 2024

jorisvandenbossche Aug 27, 2024

jbrockmendel Aug 27, 2024

jorisvandenbossche commented Aug 27, 2024

jbrockmendel commented Aug 27, 2024

jorisvandenbossche left a comment

jorisvandenbossche Sep 9, 2024

jorisvandenbossche Sep 9, 2024

jbrockmendel Sep 9, 2024

jorisvandenbossche Sep 9, 2024

jbrockmendel Sep 9, 2024

jorisvandenbossche Sep 9, 2024

jbrockmendel Sep 9, 2024

jorisvandenbossche commented Sep 10, 2024

jorisvandenbossche commented Sep 11, 2024

	return ArrowExtensionArray._str_slice(self, start=start, stop=stop, step=step)
	return ArrowStringArrayMixin._str_slice(self, start=start, stop=stop, step=step)

	return ArrowExtensionArray._str_removeprefix(self, prefix)
	return ArrowStringArrayMixin._str_removeprefix(self, prefix)

REF (string): de-duplicate ArrowStringArray methods #59555

REF (string): de-duplicate ArrowStringArray methods #59555

Conversation

jbrockmendel commented Aug 19, 2024

jorisvandenbossche commented Aug 20, 2024

jbrockmendel commented Aug 20, 2024

mroeschke commented Aug 21, 2024

jbrockmendel commented Aug 21, 2024 via email

jorisvandenbossche commented Aug 21, 2024

jorisvandenbossche commented Aug 21, 2024

jbrockmendel commented Aug 22, 2024

jorisvandenbossche commented Aug 22, 2024

jbrockmendel commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Aug 27, 2024

jbrockmendel commented Aug 27, 2024

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Sep 10, 2024

jorisvandenbossche commented Sep 11, 2024