Copilot commented on code in PR #3580:
URL: https://github.com/apache/iceberg-rust/pull/3580#discussion_r4419105465
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -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());
Review Comment:
This assertion makes the test brittle and ties it to a specific
`snapshot_log` cleanup strategy rather than the behavior under test (ref
removal + `current_snapshot_id` clearing). Consider asserting a more direct
invariant, e.g., that `snapshot_log` contains no entries for the removed
snapshot ID (or that it is consistent with `current_snapshot_id == None`), so
the test won't fail if `update_snapshot_log` behavior is adjusted while still
remaining correct.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -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();
Review Comment:
This removes (and records `RemoveSnapshotRef` changes for) *any* ref
pointing to a snapshot missing from `metadata.snapshots`, including refs that
may have already been dangling before this `remove_snapshots` call. If the
intent is to only remove refs that point to snapshots actually removed by this
call (as described), filter by the set of snapshot IDs that were removed (e.g.,
`removed_snapshots`) rather than by `contains_key` on the post-removal snapshot
map.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -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();
Review Comment:
Since the order doesn't need to be stable, `sort_unstable()` is typically
faster and uses less memory than `sort()` for simple string sorting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
