Skip to content

GH-34283 [Python] Add types_mapper support to index for to_pandas #34445

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 9, 2023
Merged

GH-34283 [Python] Add types_mapper support to index for to_pandas #34445

merged 3 commits into from
Mar 9, 2023

Conversation

phofl
Copy link
Contributor

@phofl phofl commented Mar 3, 2023 •
edited
Loading

Rationale for this change

What changes are included in this PR?

Only respects types_mapper for indexes as well

Are these changes tested?

Yes

Are there any user-facing changes?

Technically this breaks the API in a way that we would now respect the types_mapper for the index.

cc @jorisvandenbossche

@phofl phofl requested a review from AlenkaF as a code owner March 3, 2023 22:15
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

⚠️ GitHub issue #34283 has been automatically assigned in GitHub to PR creator.

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

⚠️ GitHub issue #34283 has been automatically assigned in GitHub to PR creator.

1 similar comment
@github-actions
Copy link

github-actions bot commented Mar 3, 2023

⚠️ GitHub issue #34283 has been automatically assigned in GitHub to PR creator.

AlenkaF
AlenkaF reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

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

Thank you for another fix @phofl! 🤩
The solution is much nicer than I thought it would be. And I only have one small nit question in the tests.

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Mar 6, 2023
jorisvandenbossche
jorisvandenbossche approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche 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!

python/pyarrow/tests/test_pandas.py Outdated
@@ -647,6 +647,23 @@ def test_mismatch_metadata_schema(self):
result = new_table.to_pandas()
tm.assert_frame_equal(result, expected)

@pytest.mark.parametrize("index", ["a", ["a", "b"]])
def test_to_pandas_types_mapper_index(self, index):
Copy link
Member

Choose a reason for hiding this comment

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

The other types_mapper related tests are lower in the file (not in one of the classes), so I would move it there (eg just below test_roundtrip_empty_table_with_extension_dtype_index)

(Sidenote, seeing that test_roundtrip_empty_table_with_extension_dtype_index test, that seems to be something that should be fixed as well, preserving interval index)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Mar 6, 2023
@jorisvandenbossche jorisvandenbossche merged commit b679a96 into apache:main Mar 9, 2023
@jorisvandenbossche jorisvandenbossche mentioned this pull request Mar 9, 2023
Closed
@jorisvandenbossche
Copy link
Member

Thanks @phofl !

@ursabot
Copy link

ursabot commented Mar 9, 2023

Benchmark runs are scheduled for baseline = 17f416f and contender = b679a96. b679a96 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.94% ⬆️0.03%] test-mac-arm
[Finished ⬇️1.02% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.13% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] b679a96d ec2-t3-xlarge-us-east-2
[Failed] b679a96d test-mac-arm
[Finished] b679a96d ursa-i9-9960x
[Finished] b679a96d ursa-thinkcentre-m75q
[Finished] 17f416f8 ec2-t3-xlarge-us-east-2
[Finished] 17f416f8 test-mac-arm
[Finished] 17f416f8 ursa-i9-9960x
[Finished] 17f416f8 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@phofl phofl deleted the 34283 branch March 24, 2023 23:46
@phofl phofl mentioned this pull request Mar 24, 2023
Merged
3 tasks
@j-bennet j-bennet mentioned this pull request Mar 25, 2023
Merged
2 tasks
@mroeschke mroeschke mentioned this pull request May 2, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

@AlenkaF AlenkaF AlenkaF approved these changes

Assignees
No one assigned
Labels
awaiting merge Awaiting merge Component: Python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Python] Table.to_pandas fails to convert index dtype with a custom type mapper
4 participants
@phofl @jorisvandenbossche @ursabot @AlenkaF