fix(transaction): prevent committing to a read-only table

[mkroll-db]

Which issue does this PR close?

• Closes Transaction::commit succeeds on a read-only table #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 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, 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.

[laskoviymishka]

Guard placement looks right to me. apply only stages actions and each action's commit runs inside do_commit, so checking ahead of it means nothing gets written and the catalog is never touched for a read-only table.

The empty case is where this departs from Java. commitSimpleTransaction returns early when base == current, so an empty transaction on a static table there never reaches StaticTableOperations.commit; here it now errors. I think failing regardless of action count is the better contract, and the test pins it. But it's a deliberate divergence and the part most likely to break callers that commit unconditionally, so I'd spell it out in the CHANGELOG and on commit's rustdoc (left suggestions inline).