qzyu999 commented on issue #3737: URL: https://github.com/apache/iceberg-python/issues/3737#issuecomment-5347164236
> I think the idea of having pluggable implementations is really intriguing! The first thing that would be most useful is to ensure that there's no PyArrow logic that's occurring outside of pyarrow.py. After that, I'd love to help work through what the compute protocols would be. Ideally, we would help build those in tangent with building out the modules. Hi @rambleraptor, thanks for the feedback. I agree that consolidating PyArrow logic should come before decomposing the monolith. I did an audit of the `pyiceberg/` source (excluding `io/pyarrow.py` itself and tests) by grepping for: - Top-level `import pyarrow` / `from pyarrow import` statements - Inline runtime `import pyarrow as pa` inside function bodies - `TYPE_CHECKING`-only imports (these don't count as leakage since they're erased at runtime) Here's what I found: ### Runtime PyArrow usage outside `io/pyarrow.py` | Location | What it does | Notes | |----------|-------------|-------| | `table/upsert_util.py` | Top-level `import pyarrow as pa` + `from pyarrow import compute as pc`. Does joins, group_by, take, cast, slice directly on `pa.Table` | Clearest case for absorption. Pure compute logic that should route through the pyarrow module | | `table/inspect.py` | Runtime `import pyarrow as pa` in every method. Builds `pa.schema()`/`pa.Table.from_pylist()` for metadata tables (snapshots, entries, refs, partitions, manifests, etc.) | Gray area. This is output formatting, not compute that would benefit from engine substitution | | `transforms.py` | Runtime `import pyarrow as pa` in `_pyiceberg_transform_wrapper` and `VoidTransform.pyarrow_transform()`. Dispatches on `pa.Array` vs `pa.ChunkedArray` | Already explicitly scoped as pyarrow extension points on each transform class | | `table/__init__.py` | Runtime imports in `append()`, `overwrite()`, `dynamic_partition_overwrite()`, `upsert()`. Checks `isinstance(df, pa.Table)` then delegates to `io.pyarrow` | Intentional API surface. The heavy lifting already goes through `io.pyarrow` | | `table/deletion_vector.py` | Single `pa.chunked_array()` call in `_bitmaps_to_chunked_array` | Trivial | | `catalog/__init__.py` | `_convert_schema_if_needed()` with runtime import, but it delegates to `visit_pyarrow`/`_ConvertToIcebergWithoutIDs` from `io.pyarrow` | Already correctly structured | ### TYPE_CHECKING only (not leakage) `schema.py`, `table/puffin.py`, `table/update/schema.py`, `catalog/sql.py`, `catalog/rest/__init__.py`, `catalog/noop.py`, `catalog/hive.py`, `catalog/glue.py`, `catalog/dynamodb.py`, `catalog/bigquery_metastore.py` all import `pyarrow` under `TYPE_CHECKING` for type annotations only. No runtime dependency. ### Proposed plan I'd like to open a tracking issue for this and submit one PR per location, absorbing the PyArrow logic into `io/pyarrow.py` so that callers import helpers from `pyiceberg.io.pyarrow` rather than calling `pa.*` directly. Each PR is a pure refactor with no behavior change. My read on what to absorb vs. leave: - **Absorb:** `table/upsert_util.py` (joins, dedup, row comparison are all compute operations that should live behind the pyarrow module boundary) - **Discuss:** `table/inspect.py` (metadata table construction), `transforms.py` (pyarrow_transform dispatch) - **Leave:** `table/__init__.py` (user-facing API), `catalog/__init__.py` (already delegates correctly), `deletion_vector.py` (trivial) The goal is that once containment is done, every runtime PyArrow call routes through `io/pyarrow.py`. Then the decomposition (#3738) splits that one module into focused submodules, and the protocol/substitution work has clean seam points. Does this match what you had in mind? Happy to hear if any of the "leave" or "discuss" items should be treated differently. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
