mkroll-db opened a new pull request, #3463: URL: https://github.com/apache/iceberg-rust/pull/3463
## Which issue does this PR close? - Closes #3462. ## What changes are included in this PR? `Transaction::commit` now rejects read-only tables instead of attempting a catalog commit, with a `CHANGELOG.md` entry for the behaviour change. `StaticTable::into_table` already documented that its table "can't be used to perform modifications on the table", but nothing enforced it: `commit` ran through `do_commit` and issued `load_table` / `update_table`, committing for real whenever the caller's catalog resolved the table's identifier. `commit` now returns `ErrorKind::PreconditionFailed` with the message "Cannot modify a read-only table" when `Table::readonly()` is true, and attaches the table identifier as error context. The kind matches its own documentation, "the operation was rejected because the system is not in a state required for the operation's execution": committing is supported, this table instance is simply not in a state that permits it. Iceberg Java refuses at the same point with a different mechanism, [`StaticTableOperations.commit`](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/core/src/main/java/org/apache/iceberg/StaticTableOperations.java#L78-L81) throws `UnsupportedOperationException("Cannot modify a static table")`. The guard sits at the top of `commit`, ahead of the `actions.is_empty()` early return and before `do_commit` reaches the `SnapshotProducer::commit` that writes manifests. So the outcome never depends on how many actions were applied, and a refused commit leaves nothing behind in storage. ### Behaviour change Committing against a read-only table previously succeeded, and an empty transaction returned `Ok(table)` through the `actions.is_empty()` short circuit. Both now return an error. This affects tables from `StaticTable::into_table` and from the public `TableBuilder::readonly(true)`, and `CHANGELOG.md` lists it under Breaking Changes. ### What this does not cover The guard is at the catalog commit, the single choke point for metadata changes. It does not prevent bytes reaching object storage beforehand: `Table::file_io()` is public and `cow_rewrite` writes data files before it reaches `commit`. Iceberg Java has the same gap for caller-written data files, though it also refuses in [`StaticTableOperations.metadataFileLocation`](https://github.com/apache/iceberg/blob/apache-iceberg-1.12.0/core/src/main/java/org/apache/iceberg/StaticTableOperations.java#L88-L91), which its own metadata writes route through. That second guard is unnecessary here because this check runs before any action is applied, and therefore before any manifest is written. ## Are these changes tested? Three unit tests in `crates/iceberg/src/transaction/mod.rs`, plus a `make_v2_minimal_readonly_table` helper that builds the table through `StaticTable::from_metadata` rather than setting the flag by hand: - `test_commit_readonly_table_fails`: empty transaction, asserts the error kind, message, and non-retryability. Empty is deliberate, it is what fails if the guard ever moves below the `actions.is_empty()` early return. - `test_commit_readonly_table_with_actions_fails`: the realistic case, a transaction carrying an `update_table_properties` action, asserting the kind only so a message change breaks one test rather than two. - `test_commit_empty_transaction_is_noop`: an empty transaction against a writable table still returns `Ok` with unchanged metadata. That path had no test before. All three pass a bare `MockCatalog::new()` with no expectations, so mockall's panic on any unexpected call doubles as proof that `commit` never reaches the catalog. Verified locally: - `cargo test -p iceberg --lib`: 1942 passed, 0 failed, 1 ignored - `cargo clippy -p iceberg --all-targets`: no warnings - `cargo fmt -p iceberg -- --check`: clean ## AI Disclosure Code was written by a human with the guidance of AI (claude) on where to place the check and to refine the test cases. Pull description is fully AI written. -- 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]
