viirya commented on code in PR #3615:
URL: https://github.com/apache/iceberg-rust/pull/3615#discussion_r4459759232
##########
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:
Nit: deriving these from the fixture makes the `snapshot_log[3..]`
expectation easier to follow and keeps it robust to fixture changes:
```rust
let first_id = metadata.snapshot_log[0].snapshot_id;
let third_id = metadata.snapshot_log[2].snapshot_id;
```
##########
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:
`test_remove_snapshots_records_existing_ids_once` pins the recorded order to
request order. If that's intended as a guarantee, could the doc comment above
say so (e.g. "in the order they appear in `snapshot_ids`")? Otherwise the test
could compare the ids as a set.
##########
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:
The previous test already passes the unknown id `0`, so the only extra
coverage here is the empty-input case, and the change doesn't touch that path.
Consider dropping this test or folding `&[]` into the previous one.
--
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]
