andybradshaw commented on code in PR #2392:
URL: https://github.com/apache/iceberg-rust/pull/2392#discussion_r3531259816
##########
crates/iceberg/src/transaction/mod.rs:
##########
@@ -645,3 +678,131 @@ mod test_row_lineage {
assert_eq!(manifest_file.first_row_id, Some(30));
}
}
+
+#[cfg(test)]
+mod test_commit_against_memory_catalog {
+ //! End-to-end tests for transaction APIs against an in-process
`MemoryCatalog`.
+ //!
+ //! Each test constructs a table inside a fresh memory catalog, builds a
+ //! transaction, commits it, and then asserts that the catalog's view of
the
+ //! table reflects the action's intended metadata change. This complements
+ //! the action-level unit tests in the per-action modules (which only
inspect
+ //! the produced `ActionCommit`) and the mock-catalog tests in this file
+ //! (which exercise the retry loop).
+
+ use crate::Catalog;
+ use crate::memory::tests::new_memory_catalog;
+ use crate::transaction::tests::make_v2_minimal_table_in_catalog;
+ use crate::transaction::{ApplyTransactionAction, Transaction};
+
+ #[tokio::test]
+ async fn test_update_properties_commit_round_trip() {
+ let catalog = new_memory_catalog().await;
+ let table = make_v2_minimal_table_in_catalog(&catalog).await;
+
+ // Sanity: the keys we are about to set are not already present.
+ assert!(table.metadata().properties().get("owner").is_none());
+ assert!(table.metadata().properties().get("team").is_none());
+
+ let tx = Transaction::new(&table);
+ let tx = tx
+ .update_table_properties()
+ .set("owner".to_string(), "iceberg-rust".to_string())
+ .set("team".to_string(), "storage".to_string())
+ .apply(tx)
+ .unwrap();
+
+ let committed = tx.commit(&catalog).await.unwrap();
+
+ // The returned table reflects the commit.
+ assert_eq!(
+ committed.metadata().properties().get("owner"),
+ Some(&"iceberg-rust".to_string())
+ );
+ assert_eq!(
+ committed.metadata().properties().get("team"),
+ Some(&"storage".to_string())
+ );
+
+ // A fresh load from the catalog also sees the properties, confirming
+ // the commit was persisted rather than only mutated in-memory.
+ let reloaded =
catalog.load_table(committed.identifier()).await.unwrap();
+ assert_eq!(
+ reloaded.metadata().properties().get("owner"),
+ Some(&"iceberg-rust".to_string())
+ );
+ assert_eq!(
+ reloaded.metadata().properties().get("team"),
+ Some(&"storage".to_string())
+ );
+
+ // The metadata location must have advanced.
+ assert_ne!(
+ committed.metadata_location(),
+ table.metadata_location(),
+ "commit should advance the metadata pointer"
+ );
+ }
+
+ #[tokio::test]
+ async fn test_update_location_commit_round_trip() {
+ let catalog = new_memory_catalog().await;
+ let table = make_v2_minimal_table_in_catalog(&catalog).await;
+
+ let original_location = table.metadata().location().to_string();
+ let new_location = format!("{original_location}/relocated");
+
+ let tx = Transaction::new(&table);
+ let tx = tx
+ .update_location()
+ .set_location(new_location.clone())
+ .apply(tx)
+ .unwrap();
+
+ let committed = tx.commit(&catalog).await.unwrap();
+
+ assert_eq!(committed.metadata().location(), new_location);
+
+ let reloaded =
catalog.load_table(committed.identifier()).await.unwrap();
+ assert_eq!(reloaded.metadata().location(), new_location);
+ }
+
+ #[tokio::test]
+ async fn test_chained_actions_single_commit() {
Review Comment:
I think the assertions here are insufficient to say that this happened in a
"single commit" vs. two independent commits (i.e. this test but rewritten to
commit on each tx action)... seems like not the most important behavior to
test, but could add something like the following to assert it more directly:
```rust
assert_eq!(reloaded.metadata().metadata_log().len(), 1);
```
but it also feels like we should be testing the case where chained actions
cause a single commit to abort. Something like:
```rust
#[tokio::test]
async fn test_failing_chained_actions_does_not_commit() {
// A failed transaction with multiple actions should apply none of the
changes.
// ... same as other tests ...
let tx = tx
.update_schema()
.delete_column("nonexistent".to_string())
.apply(tx)
.unwrap();
// The commit fails and the catalog was not updated.
assert!(tx.commit(&catalog).await.is_err());
let reloaded = catalog.load_table(table.identifier()).await.unwrap();
assert_eq!(reloaded.metadata().properties().get("owner"), None);
assert_eq!(reloaded.metadata().location(), original_location);
assert_eq!(reloaded.metadata().metadata_log().len(), 0);
}
```
Apologies for the drive-by review, trying to familiarize myself with the
repo and contribute in some way.
--
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]
