This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3615-f533cd6175bb42b5788537f589cd9450a3054d27 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit fedc8e84655b8b93147b7416e1719d89e0ec3238 Author: Szehon Ho <[email protected]> AuthorDate: Thu Nov 5 20:10:22 2026 +0000 perf(spec): remove snapshots with direct map lookups (#3615) * perf(spec): remove snapshots with direct map lookups * chore(spec): address snapshot removal review feedback --------- Co-authored-by: Andrei Tserakhau <[email protected]> --- crates/iceberg/src/spec/table_metadata_builder.rs | 49 +++++++++++++++++++---- 1 file changed, 41 insertions(+), 8 deletions(-) diff --git a/crates/iceberg/src/spec/table_metadata_builder.rs b/crates/iceberg/src/spec/table_metadata_builder.rs index 2b63066ce..07ceae2c5 100644 --- a/crates/iceberg/src/spec/table_metadata_builder.rs +++ b/crates/iceberg/src/spec/table_metadata_builder.rs @@ -454,19 +454,17 @@ impl TableMetadataBuilder { /// Remove snapshots by its ids from the table metadata. /// Does nothing if a snapshot id is not present. - /// Keeps as changes only the snapshots that were actually removed. + /// Records removed snapshot IDs once each, in the order they first appear + /// in `snapshot_ids`. /// Refs pointing to a removed snapshot are removed as well, see [`Self::remove_ref`]. 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 { @@ -1436,6 +1434,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"; @@ -2577,6 +2576,40 @@ 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 = metadata.snapshot_log[0].snapshot_id; + let third_id = metadata.snapshot_log[2].snapshot_id; + 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_add_incompatible_current_schema_fails() { let builder = builder_without_changes(FormatVersion::V2);
