devseunggwan opened a new pull request, #3784: URL: https://github.com/apache/iceberg-python/pull/3784
# Rationale for this change `Transaction.table_metadata` replays every staged update through `update_table_metadata`, and that function ends in `model_copy(deep=True)`. A single read therefore deep-copies the whole metadata object — snapshot list included — so its cost scales with table history rather than with the work being done. Callers read the property many times per operation. #2674 and #3301 hoisted repeated accesses out of loops in `snapshot.py`. This addresses the cost at its source instead: repeated reads of an unchanged transaction state recompute once. **Carries forward #3302 by @rynewang**, which had an approving review and was then closed by the stale bot for inactivity (the bot's own comment notes this is not a judgement on merit). Its head branch lives on a fork with `maintainerCanModify=false`, so it cannot be revived from outside; this PR reproduces the change with authorship preserved via `Co-authored-by`. ## Measured A serverless writer on 1–6 row payloads, upgraded from 0.9.1 to 0.11.1, saw its mean duration go from **2495 ms to 10302 ms** — the tables it appends to have accumulated snapshot histories, and the payload size never changed. Applying this cache brought it back to **2856 ms**. Both figures are 10-minute-bucket means over matched time windows, n = 41–75 per bucket. Isolated against a local `SqlCatalog`, 20 timed appends per depth, 3 runs with the arms alternated, median: | snapshots | without | with | | |---|---|---|---| | 0 | 7.7 ms | 4.3 ms | 1.8× | | 100 | 42.4 ms | 13.2 ms | 3.2× | | 300 | 104.4 ms | 24.3 ms | 4.3× | | 500 | 173.4 ms | 36.2 ms | 4.8× | `update_table_metadata` calls per append go 22 → 2 at every depth, and the resulting table is identical (row count, snapshot count, schema). The multiplier grows with depth not because the cache works better on deep histories, but because the per-recompute deep-copy cost grows with the snapshot list. Note that the curve does not flatten — 4.3 ms at depth 0 versus 36.2 ms at depth 500. The two remaining recomputes still deep-copy, so the slope is reduced rather than removed. ## On the simpler alternative raised in #3302 @geruh suggested `if not self._updates: return self._table.metadata` instead of a cache. That covers a bare append, but not the expensive case: `CreateTableTransaction._initial_changes()` seeds `_updates` with ~10 entries before any write, so `_updates` is never empty for the snapshot producer's lifetime. `test_transaction_table_metadata_cached_with_updates_already_staged` pins that case. # Are these changes tested? Two new tests in `tests/table/test_init.py`. Both fail without the cache — 11 reads produce 11 recomputes, and the identity assertion fails. Reverting the property and re-running was used to confirm they discriminate rather than pass vacuously. One existing test changed. `test_snapshot_producer_bounded_metadata_access` (added by #3301) asserts `_MergeAppendFiles.__init__` makes **exactly one** more `update_table_metadata` call than its superclass. The cache absorbs that call too, so the count is 0 and the equality fails. It is relaxed to `<= 1`. That relaxation costs something, and the comment in the test says so: the assertion can no longer catch an un-hoisting of that constructor on its own, because repeated reads of an unchanged state are free either way. If you would rather keep the guard sharp, I can rework it to stage an update between the two constructions so each one misses the cache — happy to do that if preferred. Local runs: Pre-commit verified: `uv run python -m pytest tests/ -m "(unmarked or parametrize) and not integration"` → 3931 passed, 3 skipped · `uv run ruff check` → All checks passed · `mypy 1.18.2 --config=pyproject.toml` → diagnostics on `pyiceberg/table/__init__.py` byte-identical to `main` (no new errors). `ruff format` reports a pre-existing drift at an unrelated lambda in the same file, present on `main` too, so it is left untouched. Caller chain verified: `grep -rn '\.table_metadata\b' pyiceberg/` → 143 read sites across 12 modules (68 in `table/__init__.py`, 35 in `table/update/snapshot.py`, 19 in `table/update/spec.py`). The hot path for this change is the snapshot producer, which reads it repeatedly within a single commit. # Are there any user-facing changes? One observable difference: `last_updated_ms` on the returned metadata is now stable across repeated reads of the same logical state, instead of being re-stamped on each access. The timestamp written at commit time is unaffected. -- 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]
