This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
The following commit(s) were added to refs/heads/main by this push:
new 0ebe240a0 fix(spec): record ref removals in
TableMetadataBuilder::remove_snapshots (#3580)
0ebe240a0 is described below
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);
