Re: [PR] fix(transaction): prevent committing to a read-only table [iceberg-rust]

["laskoviymishka (via GitHub)" <gi...@apache.org>]

laskoviymishka commented on code in PR #3463:
URL: https://github.com/apache/iceberg-rust/pull/3463#discussion_r4282487088


##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -173,6 +173,14 @@ impl Transaction {
 
     /// Commit transaction.

Review Comment:
   The rustdoc still just says "Commit transaction." Callers can now hit a new error here, even with no actions, so it should be documented where they'll look:
   
   ```suggestion
       /// Commit transaction.
       ///
       /// Returns `ErrorKind::PreconditionFailed` if the table is read-only
       /// (`Table::readonly()`), even when the transaction has no actions.
   ```



##########
CHANGELOG.md:
##########
@@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/).
 * `EncryptedInputFile::metadata()` is now synchronous and derives the plaintext size from key metadata without a storage stat. Remove `.await` from calls to this method.
 * AGS1 readers now require `StandardKeyMetadata::file_length` and reject missing or invalid lengths without falling back to a storage stat. AGS1-encrypted manifests, manifest lists, and Puffin files written by earlier development builds without this field must be rewritten using a build that can still read them before upgrading. This matches the Java client's read contract.
 * `iceberg_catalog_loader::BoxedCatalogBuilder` has a new required method, `with_runtime`. Types implementing `CatalogBuilder` get it through the blanket impl; types implementing `BoxedCatalogBuilder` directly (for example, a wrapper around another `Box<dyn BoxedCatalogBuilder>`) must implement it, typically by forwarding the runtime to the builder they wrap.
+* `Transaction::commit` now fails with `ErrorKind::PreconditionFailed` for read-only tables instead of committing them. Handle the error in callers that commit to a table obtained from `StaticTable::into_table`.

Review Comment:
   "instead of committing them" is wrong for the empty case: an empty transaction never committed anything, it just returned `Ok(table)`. That's also the case most likely to surprise people. The change also isn't limited to `StaticTable`. I'd reword:
   
   ```suggestion
   * `Transaction::commit` now returns `ErrorKind::PreconditionFailed` for read-only tables (`Table::readonly()`, e.g. from `StaticTable::into_table` or `TableBuilder::readonly(true)`), including empty transactions, which previously returned the table unchanged. Java treats an empty transaction on a static table as a no-op.
   ```



##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -322,6 +330,15 @@ mod tests {
             .unwrap()
     }
 
+    async fn make_v2_minimal_readonly_table() -> Table {
+        let metadata = make_v2_minimal_table().metadata().clone();
+        let ident = TableIdent::from_strs(["ns1", "test1"]).unwrap();

Review Comment:
   `make_v2_minimal_table()` already carries `ns1.test1`, so I'd take the identifier from it. That way the two helpers can't drift apart:
   
   ```rust
   let table = make_v2_minimal_table();
   StaticTable::from_metadata(table.metadata().clone(), table.identifier().clone(), FileIO::new_with_memory())
   ```



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org