Uh oh!
There was an error while loading. Please reload this page.
MINOR: [Python][Parquet] Clarify code around restoring pandas metadata in ParquetDataset.read() - #51151
Conversation
…a in ParquetDataset.read()
| # if use_pandas_metadata, restore the pandas metadata (which gets | ||
| # lost if doing a specific `columns` selection in to_table) | ||
| if use_pandas_metadata: | ||
| if metadata and b"pandas" in metadata: |
There was a problem hiding this comment.
If metadata would come from metadata = self.schema.metadata, it means it already had a "pandas" key, and there is no need to add it back here (to_table() does not (or no longer) loose the metadata)
There was a problem hiding this comment.
🟢 Approval recommended
The change is a small, localized refactor that preserves intended behavior while making the metadata source/restore path more explicit.
Pull request overview
This PR refactors ParquetDataset.read() to make the pandas-metadata restoration logic clearer, specifically distinguishing the case where pandas metadata is sourced from _common_metadata / _metadata rather than from the dataset schema itself.
Changes:
- Introduces
common_metadatato explicitly track when pandas metadata is obtained from common metadata files for backwards compatibility. - Narrows the schema-metadata restoration step to only run when pandas metadata was sourced from common metadata (so it won’t already be present on the resulting table’s schema metadata).
File summaries
| File | Description |
|---|---|
| python/pyarrow/parquet/core.py | Clarifies pandas metadata handling by tracking and conditionally re-attaching pandas schema metadata only when it was obtained from common metadata files. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Rationale for this change
This might not strictly speaking be a valid "minor" issue since it changes code, but it should keep the logic the same and (for me) clarify the intent of it.
Are these changes tested?
By existing tests
Are there any user-facing changes?
No