Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

API (string dtype): implement hierarchy (NA > NaN, pyarrow > python) for consistent comparisons between different string dtypes#61138

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbosschejorisvandenbossche commentedMar 17, 2025
edited
Loading

Closes#60639

This does not yet handle the case of comparison to object dtype.

  • Tests added and passed if fixing a bug or adding a new feature
  • Added an entry in the latestdoc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

…for consistent comparisons between different string dtypes
@jorisvandenbosschejorisvandenbossche added the StringsString extension data type and string data labelMar 17, 2025
@jorisvandenbosschejorisvandenbossche added this to the2.3 milestoneMar 17, 2025
@jorisvandenbosschejorisvandenbossche marked this pull request as ready for reviewMarch 19, 2025 16:07
expected = pd.array([None, None, None], dtype=expected_dtype)
tm.assert_extension_array_equal(result, expected)
# # with list
# other = [None, None, "c"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Did you want to implement testing this in this PR?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes, this was already implemented, just need to add this case back to the test. The original "array" test was actually testing with a list. I updated the test to now actually use an array (parametrized with all the different dtypes, to get all combinations of dtypes in both operands), and added a separate test with just the list.

@jorisvandenbosschejorisvandenbosscheforce-pushed thestring-dtype-comparison-methods-priority branch from9a0c382 to4ebd93bCompareMarch 19, 2025 18:31
@rhshadrachrhshadrach added the Numeric OperationsArithmetic, Comparison, and Logical operations labelMar 23, 2025
Comment on lines -350 to -364
result = getattr(a, op_name)(pd.NA)
expected = pd.array([None, None, None], dtype=expected_dtype)
tm.assert_extension_array_equal(result, expected)
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

For this case of comparing with NA, we already have a dedicated test just above, so removing it here

rhshadrach reacted with thumbs up emoji
Copy link
Member

@rhshadrachrhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Needs a whatsnew?

@rhshadrach
Copy link
Member

@jorisvandenbossche - I've merged main and pushed a commit here. If you have any objections, I can pull it off.

  • Adds whatsnew to 2.3.
  • Simplifies conditionals in a test.
  • Fixes behavior of ArrowExtensionArray and adds tests for it.

For the last one, previously ArrowExtensionArray vs Nan-Python was giving back NumPy bool. This was the only case where ArrowExtensionArray was not resulting in ArrowExtensionArray.

This does not yet handle the case of comparison to object dtype.

object dtype looks correct to me here.

@rhshadrachrhshadrach added the API - ConsistencyInternal Consistency of API/Behavior labelMay 10, 2025
Copy link
Member

@rhshadrachrhshadrach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

lgtm,@mroeschke can you have a look.

Copy link
MemberAuthor

@jorisvandenbosschejorisvandenbossche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@rhshadrach thanks for updating this!

object dtype looks correct to me here.

Hmm, not entirely sure anymore what I meant with that object dtype was not yet covered. I thought maybe the case where the object dtype does not contain just strings, but also that seems to work fine


in determining the result dtype when there are different string dtypes compared. Some examples:

- When ``pd.StringDtype("pyarrow", na_value=pd.NA)`` is compared against any other string dtype, the result will always be ``boolean[pyarrow]``.
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think this is not correct, it returns just the nullableboolean dtype? (i.e.pd.BooleanDtype()) Whereboolean[pyarrow] is an alias forpd.ArrowDtype(pa.boolean())

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Hmm, I see that it is actually the behaviour with this PR as well, but I thought I would have "fixed" that while making things consistent

Copy link
MemberAuthor

@jorisvandenbosschejorisvandenbosscheMay 14, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

And I also see that I coded explicitly myself this expected dtype in the tests ...

Copy link
Member

@rhshadrachrhshadrachMay 18, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Given the ordering

object < (python, NaN) < (pyarrow, NaN) < (python, NA) < (pyarrow, NA)

when we compare(pyarrow, NA) with anything we want the result to be as if we compared(pyarrow, NA) with itself, which should result inboolean[pyarrow].

@jorisvandenbossche
Copy link
MemberAuthor

object dtype looks correct to me here.

Hmm, not entirely sure anymore what I meant with that object dtype was not yet covered. I thought maybe the case where the object dtype does not contain just strings, but also that seems to work fine

One case related to object dtype that is still failing is comparing with an object series that has mixed types:

In [3]:ser1=pd.Series(["a",None,"b"],dtype=pd.StringDtype("pyarrow",na_value=np.nan))In [4]:ser2=pd.Series(["a",None,2],dtype=object)In [5]:ser1==ser2...File~/scipy/repos/pandas/pandas/core/arrays/arrow/array.py:517,inArrowExtensionArray._box_pa_array(cls,value,pa_type,copy)514pa_array=pa.array(value,type=pa_type,from_pandas=True)515except (pa.ArrowInvalid,pa.ArrowTypeError):516# GH50430: let pyarrow infer type, then cast-->517pa_array=pa.array(value,from_pandas=True)519ifpa_typeisNoneandpa.types.is_duration(pa_array.type):520# Workaround https://github.com/apache/arrow/issues/37291521frompandas.core.tools.timedeltasimportto_timedelta...ArrowTypeError:Expectedbytes,gota'int'objectIn [6]:ser1=pd.Series(["a",None,"b"],dtype=pd.StringDtype("python",na_value=np.nan))In [7]:ser1==ser2Out[7]:0True1False2False

So with just object dtype, such a comparison works. And it also works with the python-backed string dtype. But fails with the pyarrow-backed string dtype, because in this case the comparison defers to the ArrowExtensionArray implementation, which tries to convert the other side to a pyarrow array, which is not supported for mixed types. While we generally (although in many cases definitely not best practice) mixed-types object dtype in pandas.

(but let's consider this for a separate issue/PR)

rhshadrach reacted with thumbs up emoji

@mroeschkemroeschke merged commit6177e22 intopandas-dev:mainMay 19, 2025
44 checks passed
@mroeschke
Copy link
Member

Thank@jorisvandenbossche and@rhshadrach

@lumberbot-appLumberbot (App)
Copy link

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 2.3.xgit pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 6177e2233974643d17d1560a4d61804800534900
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #61138: API (string dtype): implement hierarchy (NA > NaN, pyarrow > python) for consistent comparisons between different string dtypes'
  1. Push to a named branch:
git push YOURFORK 2.3.x:auto-backport-of-pr-61138-on-2.3.x
  1. Create a PR against branch 2.3.x, I would have named this PR:

"Backport PR#61138 on branch 2.3.x (API (string dtype): implement hierarchy (NA > NaN, pyarrow > python) for consistent comparisons between different string dtypes)"

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 theStill Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free tosuggest an improvement.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@rhshadrachrhshadrachrhshadrach approved these changes

@mroeschkemroeschkemroeschke approved these changes

Assignees
No one assigned
Labels
API - ConsistencyInternal Consistency of API/BehaviorNumeric OperationsArithmetic, Comparison, and Logical operationsStill Needs Manual BackportStringsString extension data type and string data
Projects
None yet
Milestone
2.3
Development

Successfully merging this pull request may close these issues.

API (string dtype): comparisons between different string classes
3 participants
@jorisvandenbossche@rhshadrach@mroeschke

[8]ページ先頭

©2009-2025 Movatter.jp