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

GH-39914: [pyarrow] Reorder to_pandas extension dtype mapping #44720

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bretttully
Copy link

@bretttully bretttully commented Nov 14, 2024

Rationale for this change

This is a long standing pandas ticket with some fairly horrible workarounds, where complex arrow types do not serialise well to pandas as the pandas metadata string is not parseable. However, types_mapper always had highest priority as it overrode what was set before.

What changes are included in this PR?

By switching the logical ordering, it means that we don't need to call _pandas_api.pandas_dtype(dtype) when using the pyarrow backend, thus resolving the issue of complex dtype with list or struct. It will likely still fail if the numpy backend is used, but at least this gives a working solution rather than an inability to load files at all.

Are these changes tested?

Existing tests should stay unchanged and a new test for the complex type has been added

Are there any user-facing changes?

This PR contains a "Critical Fix".
This makes pd.read_parquet(..., dtype_backend="pyarrow") work with complex data types where the metadata added by pyarrow during pd.to_parquet is not serialisable and currently throwing an exception. This issue currently prevents the use of pyarrow as the default backend for pandas.

Addresses pandas-dev/pandas#53011

`types_mapper` always had highest priority as it overrode what was set before. However, switching the logical ordering, it means that we don't need to call `_pandas_api.pandas_dtype(dtype)` when using the pyarrow backend. Resolving the issue of complex `dtype` with `list` or `struct`
Copy link

❌ GitHub issue #53011 could not be retrieved.

@github-actions github-actions bot added the awaiting review Awaiting review label Nov 14, 2024
@bretttully bretttully changed the title GH-53011: [pyarrow] Reorder to_pandas extension dtype mapping GH-39914: [pyarrow] Reorder to_pandas extension dtype mapping Nov 14, 2024
Copy link

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

@jorisvandenbossche
Copy link
Member

By switching the logical ordering, it means that we don't need to call _pandas_api.pandas_dtype(dtype) when using the pyarrow backend,

And because you added a name not in ext_columns to the subsequent methods to fill ext_columns, this should preserve the priority of the different methods to determine the pandas dtype? (metadata < pyarrow extension type < types_mapper)

Comment on lines +4432 to +4436
# Round trip df0 into df1
with io.BytesIO() as stream:
df0.to_parquet(stream, schema=schema)
stream.seek(0)
df1 = pd.read_parquet(stream, dtype_backend="pyarrow")
Copy link
Member

Choose a reason for hiding this comment

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

You might not need the roundtrip to parquet, but a table = pa.table(df); result = table.to_pandas(types_mapper=pd.ArrowDtype) should be sufficient to test this?

I know this doesn't test exactly pd.read_parquet in its entirety, but it should test the relevant part on the pyarrow side, and an actual pd.read_parquet test can still be added to pandas

Copy link
Author

Choose a reason for hiding this comment

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

The error only gets thrown once the pandas metadata is added to the table. That's why I have used a round-trip test. Is there another way to generate that metadata and set it on the table before calling to_pandas?

Copy link
Member

Choose a reason for hiding this comment

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

The metadata gets added on the pyarrow side, so table = pa.table(df) will do that

Copy link
Member

Choose a reason for hiding this comment

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

The pandas to_parquet method essentially just does a table = pa.Table.from_pandas(df) and then writes that to parquet (and pa.table(df) is a shorter less explicit version of that, but you can also use Table.from_pandas)

@bretttully
Copy link
Author

By switching the logical ordering, it means that we don't need to call _pandas_api.pandas_dtype(dtype) when using the pyarrow backend,

And because you added a name not in ext_columns to the subsequent methods to fill ext_columns, this should preserve the priority of the different methods to determine the pandas dtype? (metadata < pyarrow extension type < types_mapper)

Yes, exactly. Priority remains the same, but functions are skipped if the field already has a type, meaning that the code causing the error is no longer called if types_mapper is provided.

@jorisvandenbossche
Copy link
Member

The test_dlpack failure in the tests you can ignore (#44728)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review Awaiting review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants