qzyu999 commented on issue #3812:
URL:
https://github.com/apache/iceberg-python/issues/3812#issuecomment-5347604756
## Notes on `inspect.py` and `transforms.py`
Adding some analysis on the two "TBD" items to guide discussion.
### `table/inspect.py`
Every method in `InspectTable` follows the same pattern: iterate over
metadata objects, build a list of Python dicts, define a `pa.schema(...)`, and
call `pa.Table.from_pylist(data, schema=schema)`.
Absorption would mean moving the schema definitions and `from_pylist` calls
into helper functions in `io/pyarrow.py`:
```python
# io/pyarrow.py
def _build_snapshots_table(snapshots: list[dict[str, Any]]) -> pa.Table:
schema = pa.schema([
pa.field("committed_at", pa.timestamp(unit="ms"), nullable=False),
pa.field("snapshot_id", pa.int64(), nullable=False),
...
])
return pa.Table.from_pylist(snapshots, schema=schema)
```
Then `inspect.py` would only gather dicts and delegate:
```python
# table/inspect.py
from pyiceberg.io.pyarrow import _build_snapshots_table
def snapshots(self) -> "pa.Table":
rows = [{"committed_at": ..., ...} for s in self.tbl.metadata.snapshots]
return _build_snapshots_table(rows)
```
**My read:** This is technically doable but adds ~10 trivial wrapper
functions whose only job is `pa.Table.from_pylist(data, schema=X)`. There's no
compute here that benefits from engine substitution. A DataFusion engine would
not build metadata inspection tables any differently. This is output
formatting, not a substitution target
Leaning toward **leave**, but open to feedback.
### `transforms.py`
Each transform subclass defines a `pyarrow_transform()` method returning a
`Callable[[pa.Array], pa.Array]`. The shared `_pyiceberg_transform_wrapper`
dispatches on `pa.Array` vs `pa.ChunkedArray`.
Absorption would mean moving the implementations out of the classes and into
a dispatch function in `io/pyarrow.py`:
```python
# io/pyarrow.py
def _get_pyarrow_transform(transform: Transform, source: IcebergType) ->
Callable[[pa.Array], pa.Array]:
if isinstance(transform, BucketTransform):
return _pyiceberg_transform_wrapper(core.bucket,
transform._num_buckets)
elif isinstance(transform, YearTransform):
return _pyiceberg_transform_wrapper(core.year,
expected_type=pa.int32())
...
```
**My read:** This fights the existing OO design. The `pyarrow_transform()`
methods are polymorphic for a reason: each transform subclass owns its own
implementation. Pulling them into a central `isinstance` dispatch:
- Mirrors the class hierarchy in a fragile way (adding a new transform
requires editing two places)
- Doesn't improve substitutability. If DataFusion came in, you'd add a
parallel `datafusion_transform()` method or a generic `transform(engine)`
dispatch alongside the existing methods, not replace them.
The methods are already scoped by name (`pyarrow_transform`) to signal
they're the PyArrow-specific path. They *are* the extension point.
Leaning toward **leave**.
### Summary
| File | Absorption approach | Recommendation |
|------|-------------------|----------------|
| `inspect.py` | Move schema defs + `from_pylist` into `io/pyarrow.py`
helpers | Leave. No compute to substitute, just output formatting |
| `transforms.py` | Central dispatch function replacing polymorphic methods
| Leave. Breaks OO design, methods already serve as the named extension point |
Happy to reconsider if there's a use case I'm missing.
--
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]
