Skip to content
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

BUG: __from_arrow__ doesn't accept pyarrow null arrays for numeric ma… #52223

Merged
merged 6 commits into from Apr 7, 2023

Conversation

lithomas1
Copy link
Member

…sked types

  • 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.

@lithomas1 lithomas1 added Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays Arrow pyarrow functionality labels Mar 26, 2023
@lithomas1 lithomas1 requested a review from phofl March 26, 2023 16:52
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Looks good. Just a merge conflict

@lithomas1 lithomas1 requested a review from mroeschke April 3, 2023 21:44
@lithomas1 lithomas1 added this to the 2.1 milestone Apr 3, 2023
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

LGTM. Merge when ready @phofl

Copy link
Member

@phofl phofl left a comment

Choose a reason for hiding this comment

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

small comments

doc/source/whatsnew/v2.1.0.rst Outdated Show resolved Hide resolved
else:
# pyarrow.ChunkedArray
chunks = array.chunks
length = array.length

if pyarrow.types.is_null(array.type):
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this up before the isinstance checks

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the isinstance checks are necessary since the way we find length depends on the type of pyarrow array(regular array uses len, while chunkedarray needs length).

I could be missing something again, though.

Copy link
Member

Choose a reason for hiding this comment

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

I assumed that array would have length as well, but that's not the case. So looks good

pandas/tests/arrays/masked/test_arrow_compat.py Outdated Show resolved Hide resolved
@mroeschke
Copy link
Member

Looks like a merge conflict otherwise LGTM

@lithomas1
Copy link
Member Author

Thanks for the reviews all, going to merge this to unblock myself.

@lithomas1 lithomas1 merged commit efccff8 into pandas-dev:main Apr 7, 2023
37 of 38 checks passed
@lithomas1 lithomas1 deleted the fix-arrow-numeric-nulls branch April 7, 2023 21:05
@lithomas1
Copy link
Member Author

Ugh, I'm going to need to backport this as well to backport #52087.

Ok to do this, @phofl?

@phofl
Copy link
Member

phofl commented May 24, 2023

I'd rather not create chains that we have to backport. If fixes aren't totally trivial we should probably hold off?

@lithomas1
Copy link
Member Author

Seems reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arrow pyarrow functionality Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants