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-3580-896f9d6c50ff52fa9e43aec170d3f14bfb40b512 in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit 0ebe240a0f73a3efca2ad4a98326b06ccfd2e30e Author: Liang-Chi Hsieh <[email protected]> AuthorDate: Tue Nov 3 19:29:17 2026 +0000 fix(spec): record ref removals in TableMetadataBuilder::remove_snapshots (#3580) remove_snapshots dropped refs pointing at removed snapshots without recording RemoveSnapshotRef changes, and left current_snapshot_id set when main was one of them. Route the dangling refs through remove_ref, matching Java's TableMetadata.Builder. Co-authored-by: Isaac <[email protected]> --- crates/iceberg/src/spec/table_metadata_builder.rs | 155 +++++++++++++++++++++- 1 file changed, 153 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/spec/table_metadata_builder.rs b/crates/iceberg/src/spec/table_metadata_builder.rs index 6d78f77d4..2b63066ce 100644 --- a/crates/iceberg/src/spec/table_metadata_builder.rs +++ b/crates/iceberg/src/spec/table_metadata_builder.rs @@ -455,6 +455,7 @@ 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. + /// 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()); @@ -474,9 +475,18 @@ impl TableMetadataBuilder { } // Remove refs that are no longer valid - self.metadata + let mut dangling_refs: Vec<String> = self + .metadata .refs - .retain(|_, v| self.metadata.snapshots.contains_key(&v.snapshot_id)); + .iter() + .filter(|(_, v)| !self.metadata.snapshots.contains_key(&v.snapshot_id)) + .map(|(name, _)| name.clone()) + .collect(); + dangling_refs.sort(); + + for ref_name in dangling_refs { + self = self.remove_ref(&ref_name); + } self } @@ -2398,6 +2408,147 @@ mod tests { ]); } + /// Builds metadata with one snapshot per id and the given refs, as committed metadata + /// without pending changes. + fn metadata_with_snapshots_and_refs( + snapshot_ids: &[i64], + refs: &[(&str, SnapshotReference)], + ) -> TableMetadata { + let mut builder = builder_without_changes(FormatVersion::V2); + let timestamp_ms = builder.metadata.last_updated_ms + 1; + for (i, snapshot_id) in snapshot_ids.iter().enumerate() { + let snapshot = Snapshot::builder() + .with_snapshot_id(*snapshot_id) + .with_timestamp_ms(timestamp_ms + i as i64) + .with_sequence_number(i as i64 + 1) + .with_schema_id(0) + .with_manifest_list(format!("/snap-{snapshot_id}.avro")) + .with_summary(Summary { + operation: Operation::Append, + additional_properties: HashMap::new(), + }) + .build(); + builder = builder.add_snapshot(snapshot).unwrap(); + } + for (ref_name, reference) in refs { + builder = builder.set_ref(ref_name, reference.clone()).unwrap(); + } + builder.build().unwrap().metadata + } + + fn branch_ref(snapshot_id: i64) -> SnapshotReference { + SnapshotReference { + snapshot_id, + retention: SnapshotRetention::Branch { + min_snapshots_to_keep: None, + max_snapshot_age_ms: None, + max_ref_age_ms: None, + }, + } + } + + fn tag_ref(snapshot_id: i64) -> SnapshotReference { + SnapshotReference { + snapshot_id, + retention: SnapshotRetention::Tag { + max_ref_age_ms: None, + }, + } + } + + #[test] + fn test_remove_snapshots_records_removal_of_refs_to_removed_snapshots() { + let metadata = metadata_with_snapshots_and_refs(&[1, 2], &[ + (MAIN_BRANCH, branch_ref(2)), + ("branch-on-1", branch_ref(1)), + ("tag-on-1", tag_ref(1)), + ("branch-on-2", branch_ref(2)), + ]); + + let result = metadata + .into_builder(Some( + "s3://bucket/test/location/metadata/metadata2.json".to_string(), + )) + .remove_snapshots(&[1]) + .build() + .unwrap(); + + assert_eq!(result.changes, vec![ + TableUpdate::RemoveSnapshots { + snapshot_ids: vec![1] + }, + TableUpdate::RemoveSnapshotRef { + ref_name: "branch-on-1".to_string() + }, + TableUpdate::RemoveSnapshotRef { + ref_name: "tag-on-1".to_string() + }, + ]); + assert_eq!( + result.metadata.refs, + HashMap::from([ + (MAIN_BRANCH.to_string(), branch_ref(2)), + ("branch-on-2".to_string(), branch_ref(2)), + ]) + ); + assert_eq!(result.metadata.current_snapshot_id, Some(2)); + } + + #[test] + fn test_remove_snapshots_clears_current_snapshot_when_main_snapshot_removed() { + let metadata = metadata_with_snapshots_and_refs(&[1, 2], &[ + (MAIN_BRANCH, branch_ref(2)), + ("branch-on-1", branch_ref(1)), + ]); + + let result = metadata + .into_builder(Some( + "s3://bucket/test/location/metadata/metadata2.json".to_string(), + )) + .remove_snapshots(&[2]) + .build() + .unwrap(); + + assert_eq!(result.changes, vec![ + TableUpdate::RemoveSnapshots { + snapshot_ids: vec![2] + }, + TableUpdate::RemoveSnapshotRef { + ref_name: MAIN_BRANCH.to_string() + }, + ]); + assert_eq!(result.metadata.current_snapshot_id, None); + assert_eq!( + result.metadata.refs, + HashMap::from([("branch-on-1".to_string(), branch_ref(1))]) + ); + assert!(result.metadata.snapshot_log.is_empty()); + } + + #[test] + fn test_remove_snapshots_keeps_refs_to_retained_snapshots() { + let metadata = metadata_with_snapshots_and_refs(&[1, 2, 3], &[ + (MAIN_BRANCH, branch_ref(3)), + ("branch-on-2", branch_ref(2)), + ("tag-on-3", tag_ref(3)), + ]); + let refs_before = metadata.refs.clone(); + + let result = metadata + .into_builder(Some( + "s3://bucket/test/location/metadata/metadata2.json".to_string(), + )) + .remove_snapshots(&[1]) + .build() + .unwrap(); + + assert_eq!(result.changes, vec![TableUpdate::RemoveSnapshots { + snapshot_ids: vec![1] + }]); + assert_eq!(result.metadata.refs, refs_before); + assert_eq!(result.metadata.current_snapshot_id, Some(3)); + } + #[test] fn test_cannot_add_duplicate_snapshot_id() { let builder = builder_without_changes(FormatVersion::V2);
