ASRodrigo1 opened a new pull request, #4303: URL: https://github.com/apache/iceberg-python/pull/4303
Closes #4294 # Rationale for this change ## The problem [`update_table_metadata`](https://github.com/apache/iceberg-python/blob/1c800eb33bce585c73eb7056fb9acece8550ce6d/pyiceberg/table/update/__init__.py#L766) ends in `new_metadata.model_copy(deep=True)`. Every call therefore copies the entire table history: each snapshot with its summary, each snapshot log and metadata log entry, and each schema, partition spec and sort order. The cost of one call grows linearly with the number of snapshots the table keeps, regardless of how small the update is. The function sits on the hot path of every write: - [`Transaction.table_metadata`](https://github.com/apache/iceberg-python/blob/1c800eb33bce585c73eb7056fb9acece8550ce6d/pyiceberg/table/__init__.py#L276-L278) calls it on every read. One `append` reads the property 22 times. - [`Catalog._update_and_stage_table`](https://github.com/apache/iceberg-python/blob/1c800eb33bce585c73eb7056fb9acece8550ce6d/pyiceberg/catalog/__init__.py#L1103-L1113) calls it once more on every commit. It is used by the SQL, Hive and Glue catalogs. Each commit also adds a snapshot, so a table that receives frequent small commits pays a little more on each one. #4294 calls this quadratic in total. ## How we ran into it On a production table with a few thousand snapshots retained, a single `fast_append` commit took close to 10 seconds. Profiling the commit with cProfile and py-spy attributed 71% of its time to `update_table_metadata` → `model_copy(deep=True)` → `copy.deepcopy`, with about 4 million `deepcopy` calls per commit. The cost tracks retention: in a reproduction with a larger history, the same commit took 18 s with 10,000 snapshots and 68 s with 40,000. A writer that commits frequently cannot keep up at that depth. ## Why the deep copy exists, and why a shallow container copy is enough The deep copy was introduced in #182 so that the metadata returned by `update_table_metadata` cannot change the base metadata. `test_metadata_isolation_from_illegal_updates` pins that guarantee. It appends to `new_metadata.partition_specs` and asserts that `base_metadata.partition_specs` is unchanged. As the test's own comment notes, Pydantic's `frozen` config cannot prevent mutations such as a list append. That guarantee only requires the list and dict fields of `TableMetadata` to be distinct objects. The models they hold (`Snapshot`, `Schema`, `PartitionSpec`, `SortOrder`, `SnapshotRef`, the log entries) are themselves frozen, so they can be shared safely. This is also how the Java implementation works. [`TableMetadata.Builder`](https://github.com/apache/iceberg/blob/cd4c24c9adb73bf9324d8e4835e73c97c3a1491a/core/src/main/java/org/apache/iceberg/TableMetadata.java#L987-L1009) copies each collection (`Lists.newArrayList(base.snapshots())`, `Maps.newHashMap(base.refs)`, and so on) and shares the immutable elements. The handlers behind `_apply_table_update` were already required not to mutate their input in place: the first handler receives the base metadata itself, and the isolation test checks that the base is unchanged after legal updates. We audited all 23 handlers on `main` and found that each one builds new collections or returns its input unchanged. This change adds no new requirement on them. ## The change The final `model_copy(deep=True)` is replaced by `_copy_containers`, which copies every list and dict field and shares everything else: ```python def _copy_containers(metadata: TableMetadata) -> TableMetadata: return metadata.model_copy(update={name: value.copy() for name, value in metadata if isinstance(value, (list, dict))}) ``` There is no cache and no change to the API, and the `enforce_validation` path is untouched. Fixing the function rather than its callers makes every path cheap: reads with or without staged updates, and the commit inside each catalog. ## Relation to earlier work | | Approach | Status | What it leaves | | --- | --- | --- | --- | | #2674 | Do not copy the metadata for each data file in the snapshot summary (fixes #2673) | Merged | The other reads still deep-copy | | #3301 | Hoist repeated reads of `table_metadata` in `snapshot.py` | Merged, in 0.12.0 | About 20 reads per append remain, each a full deep copy | | #3302, #3784 | Cache `Transaction.table_metadata` between reads | Both closed by the stale bot. On #3302, reviewers raised the complexity of a cache. | The catalog commit still deep-copies, and so does every new transaction state | | #4302 | Return the table's metadata when nothing is staged, as suggested in the #3302 review | Open | Reads while updates are staged (`CreateTableTransaction`, `set_properties` before an append), and the catalog commit | | This PR | Remove the cost from `update_table_metadata` itself | | Parsing and serializing the metadata JSON, which every commit needs anyway | This PR does not conflict with #4302. With both applied, #4302 adds a small further gain (table below). ## Measurements One append of one row to a table registered in a `SqlCatalog` on SQLite, with a local warehouse and no network. Each time is the median of 3 appends. To set the depth, the metadata was rewritten to keep N snapshots that point at the table's real manifest list. All four variants ran on the same machine against the same code: `main` restores the deep copy, and `#4302` applies its change to `Transaction.table_metadata`. | Snapshots | Metadata JSON | `main` | #4302 | This PR | Both | | ---: | ---: | ---: | ---: | ---: | ---: | | 1,000 | 0.6 MB | 470 ms | 97 ms | 27 ms | 26 ms | | 3,500 | 2.3 MB | 1,930 ms | 204 ms | 74 ms | 70 ms | | 10,000 | 6.5 MB | 6,591 ms | 748 ms | 182 ms | 188 ms | | 40,000 | 25.9 MB | 30,589 ms | 4,081 ms | 1,230 ms | 1,052 ms | `update_table_metadata` alone, adding one snapshot (median of 5): | Snapshots | Before | After | | ---: | ---: | ---: | | 1,000 | 16.9 ms | 0.13 ms | | 3,500 | 62.6 ms | 0.36 ms | | 10,000 | 182.7 ms | 0.91 ms | | 40,000 | 1,151.7 ms | 10.5 ms | What remains at 40,000 snapshots is mostly reading and writing the 26 MB metadata file, which no change to this function can avoid. ## Are these changes tested? Yes. - **New test.** `test_update_metadata_shares_unchanged_models` checks three things after an update: the list and dict fields of the result are new objects, the snapshots and schemas are the same objects as in the base, and the base properties are unchanged. It fails on `main` and passes with this change. - **Existing isolation test.** `test_metadata_isolation_from_illegal_updates` passes unchanged. - **Unit suite.** `pytest tests/ -m unit --ignore=tests/integration` on Python 3.13: 4,521 passed, 5 skipped, 4 failed. The 4 failures are the Kerberos tests in `tests/catalog/test_hive.py`, which fail with `No module named 'puresasl'` because the `hive-kerberos` extra was not installed. They fail the same way on `main` (1c800eb3) without this change. - **Lint.** `ruff check` and `ruff format` are clean. mypy reports no new errors in the changed module. ## Are there any user-facing changes? The public API does not change, and commits produce the same metadata as before. One behavior changes. The models inside the returned metadata are now shared with the base metadata, so mutating one of them in place also changes the base. An example is writing into a snapshot's summary with `metadata.snapshots[0].summary[...] = ...`. These models are frozen and are not meant to be mutated after construction. Appending to or assigning into the metadata's own lists and dicts remains isolated, as before. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
