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: [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]
