This is an automated email from the ASF dual-hosted git repository. szehon-ho pushed a commit to branch codex/optimize-snapshot-removal in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit 70382fb3810abd9dd0455d7c98d5b56a829c25aa Author: Szehon Ho <[email protected]> AuthorDate: Tue Nov 3 11:36:02 2026 -0800 perf(spec): remove snapshots with direct map lookups --- crates/iceberg/src/spec/table_metadata_builder.rs | 66 ++++++++++++++++++++--- 1 file changed, 59 insertions(+), 7 deletions(-) diff --git a/crates/iceberg/src/spec/table_metadata_builder.rs b/crates/iceberg/src/spec/table_metadata_builder.rs index 6d78f77d4..81ef93137 100644 --- a/crates/iceberg/src/spec/table_metadata_builder.rs +++ b/crates/iceberg/src/spec/table_metadata_builder.rs @@ -458,14 +458,11 @@ impl TableMetadataBuilder { pub fn remove_snapshots(mut self, snapshot_ids: &[i64]) -> Self { let mut removed_snapshots = Vec::with_capacity(snapshot_ids.len()); - self.metadata.snapshots.retain(|k, _| { - if snapshot_ids.contains(k) { - removed_snapshots.push(*k); - false - } else { - true + for &snapshot_id in snapshot_ids { + if self.metadata.snapshots.remove(&snapshot_id).is_some() { + removed_snapshots.push(snapshot_id); } - }); + } if !removed_snapshots.is_empty() { self.changes.push(TableUpdate::RemoveSnapshots { @@ -1426,6 +1423,7 @@ mod tests { Transform, Type, UnboundPartitionField, }; use crate::table::Table; + use crate::test_utils::scan::TableTestFixture; use crate::test_utils::test_runtime; const TEST_LOCATION: &str = "s3://bucket/test/location"; @@ -2426,6 +2424,60 @@ mod tests { builder.add_snapshot(snapshot).unwrap_err(); } + #[test] + fn test_remove_snapshots_records_existing_ids_once() { + let fixture = TableTestFixture::new_with_deep_history(); + let metadata = fixture.table.metadata().clone(); + let first_id = 3051729675574597004; + let third_id = 3056729675574597004; + let mut expected_metadata = metadata.clone(); + expected_metadata.snapshots = [ + 3055729675574597004, + 3057729675574597004, + 3059729675574597004, + ] + .into_iter() + .map(|id| (id, metadata.snapshot_by_id(id).unwrap().clone())) + .collect(); + expected_metadata.snapshot_log = metadata.snapshot_log[3..].to_vec(); + let expected = TableMetadataBuildResult { + metadata: expected_metadata, + changes: vec![TableUpdate::RemoveSnapshots { + snapshot_ids: vec![third_id, first_id], + }], + expired_metadata_logs: vec![], + }; + let mut builder = metadata.into_builder(None); + builder.last_updated_ms = Some(expected.metadata.last_updated_ms); + + let result = builder + .remove_snapshots(&[third_id, 0, first_id, third_id, 0, first_id]) + .build() + .unwrap(); + + assert_eq!(result, expected); + } + + #[test] + fn test_remove_snapshots_ignores_empty_or_unknown_ids() { + let fixture = TableTestFixture::new_with_deep_history(); + let metadata = fixture.table.metadata(); + let expected = TableMetadataBuildResult { + metadata: metadata.clone(), + changes: vec![], + expired_metadata_logs: vec![], + }; + + for snapshot_ids in [&[][..], &[0, 0][..]] { + let mut builder = metadata.clone().into_builder(None); + builder.last_updated_ms = Some(metadata.last_updated_ms); + + let result = builder.remove_snapshots(snapshot_ids).build().unwrap(); + + assert_eq!(result, expected); + } + } + #[test] fn test_add_incompatible_current_schema_fails() { let builder = builder_without_changes(FormatVersion::V2);
