Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Adds support for PyArrow Parquet content-defined chunking (CDC) in the PyIceberg write path, exposing it via new table properties and guarding usage by the minimum supported PyArrow version.
Changes:
- Add new table properties for enabling/configuring Parquet CDC (enabled/min/max/norm-level).
- Wire CDC properties through
_get_parquet_writer_kwargs, including a shared_require_pyarrow_versionguard. - Add unit + integration-style tests and document the new properties.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/io/test_pyarrow.py | Adds tests for CDC kwargs generation, version gating, and end-to-end wiring into pq.ParquetWriter. |
| pyiceberg/table/init.py | Introduces new CDC-related TableProperties constants and defaults. |
| pyiceberg/io/pyarrow.py | Adds _require_pyarrow_version helper, reuses it for Azure FS guard, and forwards CDC options to PyArrow writer kwargs. |
| mkdocs/docs/configuration.md | Documents new CDC table properties and their defaults/requirements. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2a93c6d to
d7f61aa
Compare
PyArrow's ParquetWriter has supported content-defined chunking natively since 21.0.0, producing stable page boundaries across appends for content-addressable storage. Wire this through as write.parquet.content-defined-chunking.* table properties, mirroring the property names and defaults already used by iceberg-rust. PyArrow validates the chunk sizes itself and raises a clear error, so pyiceberg doesn't duplicate those checks. Requesting CDC on an older PyArrow raises an ImportError from a shared _require_pyarrow_version helper, which also replaces the existing Azure filesystem guard.
The CDC tests raised ImportError on pyarrow 18-20, which pyproject still declares as supported.
d7f61aa to
390e404
Compare
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
Whoops, I guess I did a review on this earlier and never sent it out. Apologies for that!
| return self | ||
|
|
||
|
|
||
| def _require_pyarrow_version(min_version: str, feature: str) -> None: |
There was a problem hiding this comment.
Idea (this is out-of-scope): What if we had a require_pyarrow_with_version method that checks if pyarrow exists and optionally checks the version?
We do pyarrow checks across the codebase.
There was a problem hiding this comment.
Sounds like a good idea, creating a follow-up PR for this.
| f"pyarrow version >= {MIN_PYARROW_VERSION_SUPPORTING_AZURE_FS} required for AzureFileSystem support, " | ||
| f"but found version {pyarrow.__version__}." | ||
| ) | ||
| _require_pyarrow_version("20.0.0", "AzureFileSystem support") |
Ohh, no worries :) |
|
@rambleraptor could you please take another look? |
rambleraptor
left a comment
rambleraptor
left a comment
There was a problem hiding this comment.
I think this looks great! Thanks for doing this
| | `write.parquet.page-row-limit` | Number of rows | 20000 | Set a target threshold for the maximum number of rows within a column chunk | | ||
| | `write.parquet.dict-size-bytes` | Size in bytes | 2MB | Set the dictionary page size limit per row group | | ||
| | `write.parquet.content-defined-chunking.enabled` | Boolean | False | Enables content-defined chunking (CDC) for the Parquet writer, which produces stable page boundaries across appends. Requires `pyarrow>=21.0.0`; raises at write time on older versions. | | ||
| | `write.parquet.content-defined-chunking.min-chunk-size` | Size in bytes | 256KiB | The minimum chunk size used for content-defined chunking | |
There was a problem hiding this comment.
nit: Can you write this as a number without unit? 256KiB isn't actually a valid value.
There was a problem hiding this comment.
updated, not that the older page-size-bytes and dict-size-bytes rows in the same table still show 1MB and 2MB. I didn't change them because they're outside this PR.
Reopens #3608, which was closed by the stale bot. Rebased on current
main; GitHub refused to reopen the original PR after the branch was updated.Rationale for this change
PyArrow's
ParquetWriterhas natively supported content-defined chunking (CDC) since 21.0.0, producing stable page boundaries across appends (useful for content-addressable storage / dedup). PyIceberg's write path already funnels through a single kwargs builder (_get_parquet_writer_kwargs), so this wires CDC through aswrite.parquet.content-defined-chunking.*table properties, mirroring the property names and defaults iceberg-rust already uses for cross-engine consistency. Apyarrow>=21.0.0version guard raises a clearImportErrorif CDC is requested on an older PyArrow (extracted into a shared_require_pyarrow_versionhelper, reused by the existing Azure-filesystem version guard).Are these changes tested?
Yes: unit tests for
_get_parquet_writer_kwargs(disabled by default, enabled with defaults, enabled with custom values, unsupported PyArrow version) and an integration-style test that writes a table with CDC enabled end-to-end and reads it back.Are there any user-facing changes?
Yes: four new table properties (
write.parquet.content-defined-chunking.enabled,.min-chunk-size,.max-chunk-size,.norm-level), documented inconfiguration.md.