szehon-ho commented on code in PR #3615:
URL: https://github.com/apache/iceberg-rust/pull/3615#discussion_r4461481913
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -458,14 +458,11 @@ impl TableMetadataBuilder {
pub fn remove_snapshots(mut self, snapshot_ids: &[i64]) -> Self {
Review Comment:
Documented that removed IDs are recorded once each, in first-occurrence
request order.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -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;
Review Comment:
Updated both IDs to come from `metadata.snapshot_log`.
##########
crates/iceberg/src/spec/table_metadata_builder.rs:
##########
@@ -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() {
Review Comment:
Dropped this test; the preceding test already covers unknown IDs.
--
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]
