BUG: Fix Series.str.contains with compiled regex on Arrow string dtype #61946
Conversation
Aniketsy
force-pushed
the
fix-arrow-contains-regex-v2
branch
from
0b16375 to
838b1c5
Compare
|
Hi @mroeschke I'd appreciate it if you could take a look and share your feedback. Thanks! |
|
Thankyou for the feedback! |
|
Additionally, if this is something that is not implemented by pyarrow, we should not raise a NotImplementedError, but fall back on the python object implementation (you can see a similar pattern in some other str methods, like |
jorisvandenbossche
added
Bug
Strings
|
@jorisvandenbossche Thank you for the feedback! I will update the PR accordingly. Would you mind letting me know the reason behind the one failing check (pre-commit.ci)? |
ruff is failing, which is used for auto formatting. I would recommend to install the pre-commit locally to avoid having this fail on CI: https://pandas.pydata.org/docs/dev/development/contributing_codebase.html#pre-commit |
|
hi @jorisvandenbossche Thankyou! |
pandas/core/arrays/string_arrow.py
Outdated
| @@ -344,6 +344,9 @@ def _str_contains( | |||
| na=lib.no_default, | |||
| regex: bool = True, | |||
| ): | |||
| if isinstance(pat, re.Pattern) and regex: | |||
There was a problem hiding this comment.
Can you combine this case with the one below?
pandas-dev#61942) and add whatsnew note
pandas-dev#61942) and add whatsnew note
pandas-dev#61942) and add whatsnew note
pandas-dev#61942) and add whatsnew note
| if any_string_dtype == "string[pyarrow]": | ||
| pytest.importorskip("pyarrow") |
There was a problem hiding this comment.
| if any_string_dtype == "string[pyarrow]": | |
| pytest.importorskip("pyarrow") |
That should already happen via the fixture, I think?
| "str": bool, | ||
| }.get(any_string_dtype, object) | ||
| expected = Series([False, True, True], dtype=expected_dtype) | ||
| assert str(result.dtype) == str(expected.dtype) |
There was a problem hiding this comment.
| assert str(result.dtype) == str(expected.dtype) |
This check will also be done by the assert_series_equal called on the next line
There was a problem hiding this comment.
For now , I have applied changes as per your suggestion .
|
Can you try to run the test you added locally? Then you can make sure to get it working correctly. Right now it is still failing according to CI |
|
Sure, I will try to run tests locally and update this PR . |
…type expectations
…type expectations
| pat = re.compile("ba.") | ||
| result = ser.str.contains(pat) | ||
|
|
||
| expected = Series([False, True, True], dtype=bool) |
There was a problem hiding this comment.
| expected = Series([False, True, True], dtype=bool) | |
| expected_dtype = ( | |
| np.bool_ if is_object_or_nan_string_dtype(any_string_dtype) else "boolean" | |
| ) | |
| expected = Series([False, True, True], dtype=expected_dtype) |
There was a problem hiding this comment.
Thanks for suggesting these changes! I tried running the tests locally to verify, but couldn’t get them working on my setup.
There was a problem hiding this comment.
The tests are passing now on CI!
…type expectations
jorisvandenbossche
dismissed
mroeschke’s stale review
test and whatsnew have been added
jorisvandenbossche
changed the title
|
Thanks @Aniketsy |
|
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
|
@jorisvandenbossche |
pandas-dev#61946) Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>
|
@Aniketsy you're welcome! |
|
Manual backport -> #62116 |
closes #61942
This PR fixes an issue in
Series.str.contains()where passing a compiled regex object failed when the underlying string data is backed by PyArrow.Please, provide feedback if my approach is not correct , I would love to improve and contribute in this.