From 9d1c806037921863f419a5abfb5799a6255df017 Mon Sep 17 00:00:00 2001 From: Dhruv Arya Date: Tue, 1 Sep 2026 23:00:52 +0000 Subject: [PATCH 1/2] test(transaction): add round-trip tests for transaction APIs against MemoryCatalog Add end-to-end tests that commit transactions against an in-process MemoryCatalog and assert the persisted metadata, covering property updates, location updates, atomic multi-action commits, and rollback when one action in a chained transaction fails. --- crates/iceberg/src/transaction/mod.rs | 190 ++++++++++++++++++++++++++ 1 file changed, 190 insertions(+) diff --git a/crates/iceberg/src/transaction/mod.rs b/crates/iceberg/src/transaction/mod.rs index 3622471277..7a442a76ad 100644 --- a/crates/iceberg/src/transaction/mod.rs +++ b/crates/iceberg/src/transaction/mod.rs @@ -694,3 +694,193 @@ 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::memory::tests::new_memory_catalog; + use crate::transaction::tests::make_v3_minimal_table_in_catalog; + use crate::transaction::{ApplyTransactionAction, Transaction}; + use crate::{Catalog, ErrorKind}; + + #[tokio::test] + async fn test_update_properties_commit_round_trip() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + + // Sanity: the keys we are about to set are not already present. + assert!(!table.metadata().properties().contains_key("owner")); + assert!(!table.metadata().properties().contains_key("team")); + + 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 table returned from commit reflects the new properties. + assert_eq!( + committed + .metadata() + .properties() + .get("owner") + .map(String::as_str), + Some("iceberg-rust") + ); + assert_eq!( + committed + .metadata() + .properties() + .get("team") + .map(String::as_str), + Some("storage") + ); + + // A fresh load from the catalog also sees the properties, confirming the + // commit was persisted rather than only mutated in the returned table. + let reloaded = catalog.load_table(committed.identifier()).await.unwrap(); + assert_eq!( + reloaded + .metadata() + .properties() + .get("owner") + .map(String::as_str), + Some("iceberg-rust") + ); + assert_eq!( + reloaded + .metadata() + .properties() + .get("team") + .map(String::as_str), + Some("storage") + ); + + // The metadata pointer 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_v3_minimal_table_in_catalog(&catalog).await; + let new_location = format!("{}/relocated", table.metadata().location()); + + 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_commit_atomically() { + // A transaction can carry multiple actions. Committing once should apply + // all of them together and produce a single new metadata version. + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let initial_metadata_log_len = table.metadata().metadata_log().len(); + let new_location = format!("{}/relocated", table.metadata().location()); + + let tx = Transaction::new(&table); + let tx = tx + .update_table_properties() + .set("owner".to_string(), "iceberg-rust".to_string()) + .apply(tx) + .unwrap(); + let tx = tx + .update_location() + .set_location(new_location.clone()) + .apply(tx) + .unwrap(); + + let committed = tx.commit(&catalog).await.unwrap(); + let reloaded = catalog.load_table(committed.identifier()).await.unwrap(); + + assert_eq!( + reloaded + .metadata() + .properties() + .get("owner") + .map(String::as_str), + Some("iceberg-rust") + ); + assert_eq!(reloaded.metadata().location(), new_location); + // Both actions collapse into a single new metadata-log entry. + assert_eq!( + reloaded.metadata().metadata_log().len(), + initial_metadata_log_len + 1 + ); + } + + #[tokio::test] + async fn test_failing_chained_actions_do_not_commit() { + // If any action in the transaction fails to apply, the whole commit must + // be rejected and the catalog must be left untouched. + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let initial_location = table.metadata().location().to_string(); + let initial_metadata_location = table.metadata_location().map(str::to_string); + let initial_metadata_log_len = table.metadata().metadata_log().len(); + let new_location = format!("{initial_location}/relocated"); + + let tx = Transaction::new(&table); + let tx = tx + .update_table_properties() + .set("owner".to_string(), "iceberg-rust".to_string()) + .apply(tx) + .unwrap(); + let tx = tx + .update_location() + .set_location(new_location) + .apply(tx) + .unwrap(); + // Deleting a column that does not exist makes the schema update fail when + // the transaction is applied at commit time, aborting the whole commit. + let tx = tx + .update_schema() + .delete_column("nonexistent") + .apply(tx) + .unwrap(); + + let error = tx.commit(&catalog).await.unwrap_err(); + assert_eq!(error.kind(), ErrorKind::PreconditionFailed); + assert!(error.message().contains("nonexistent")); + + // None of the earlier actions in the transaction leaked into the catalog. + let reloaded = catalog.load_table(table.identifier()).await.unwrap(); + assert!(!reloaded.metadata().properties().contains_key("owner")); + assert_eq!(reloaded.metadata().location(), initial_location); + assert_eq!( + reloaded.metadata_location(), + initial_metadata_location.as_deref() + ); + assert_eq!( + reloaded.metadata().metadata_log().len(), + initial_metadata_log_len + ); + } +} From ac8735fd50a87d797eb8b6af537597c175274a97 Mon Sep 17 00:00:00 2001 From: Dhruv Arya Date: Tue, 1 Sep 2026 23:22:03 +0000 Subject: [PATCH 2/2] chore(transaction): trim redundant test comments Drop the module doc and what-style comments to match the terser comment density of the surrounding test modules, keeping only the non-obvious why notes. --- crates/iceberg/src/transaction/mod.rs | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-) diff --git a/crates/iceberg/src/transaction/mod.rs b/crates/iceberg/src/transaction/mod.rs index 7a442a76ad..dda0ccacb0 100644 --- a/crates/iceberg/src/transaction/mod.rs +++ b/crates/iceberg/src/transaction/mod.rs @@ -697,15 +697,6 @@ mod test_row_lineage { #[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::memory::tests::new_memory_catalog; use crate::transaction::tests::make_v3_minimal_table_in_catalog; use crate::transaction::{ApplyTransactionAction, Transaction}; @@ -716,7 +707,6 @@ mod test_commit_against_memory_catalog { let catalog = new_memory_catalog().await; let table = make_v3_minimal_table_in_catalog(&catalog).await; - // Sanity: the keys we are about to set are not already present. assert!(!table.metadata().properties().contains_key("owner")); assert!(!table.metadata().properties().contains_key("team")); @@ -730,7 +720,6 @@ mod test_commit_against_memory_catalog { let committed = tx.commit(&catalog).await.unwrap(); - // The table returned from commit reflects the new properties. assert_eq!( committed .metadata() @@ -748,8 +737,7 @@ mod test_commit_against_memory_catalog { Some("storage") ); - // A fresh load from the catalog also sees the properties, confirming the - // commit was persisted rather than only mutated in the returned table. + // A fresh load proves the commit was persisted, not just applied in-memory. let reloaded = catalog.load_table(committed.identifier()).await.unwrap(); assert_eq!( reloaded @@ -768,7 +756,6 @@ mod test_commit_against_memory_catalog { Some("storage") ); - // The metadata pointer must have advanced. assert_ne!( committed.metadata_location(), table.metadata_location(), @@ -798,8 +785,6 @@ mod test_commit_against_memory_catalog { #[tokio::test] async fn test_chained_actions_commit_atomically() { - // A transaction can carry multiple actions. Committing once should apply - // all of them together and produce a single new metadata version. let catalog = new_memory_catalog().await; let table = make_v3_minimal_table_in_catalog(&catalog).await; let initial_metadata_log_len = table.metadata().metadata_log().len(); @@ -838,8 +823,6 @@ mod test_commit_against_memory_catalog { #[tokio::test] async fn test_failing_chained_actions_do_not_commit() { - // If any action in the transaction fails to apply, the whole commit must - // be rejected and the catalog must be left untouched. let catalog = new_memory_catalog().await; let table = make_v3_minimal_table_in_catalog(&catalog).await; let initial_location = table.metadata().location().to_string(); @@ -858,8 +841,7 @@ mod test_commit_against_memory_catalog { .set_location(new_location) .apply(tx) .unwrap(); - // Deleting a column that does not exist makes the schema update fail when - // the transaction is applied at commit time, aborting the whole commit. + // Deleting a non-existent column makes the schema action fail at commit time. let tx = tx .update_schema() .delete_column("nonexistent")