blackmwk commented on code in PR #2918:
URL: https://github.com/apache/iceberg-rust/pull/2918#discussion_r3664314939
##########
crates/iceberg/src/test_utils.rs:
##########
Review Comment:
Not related to this issue, but we should add an `#cfg[(test)]` in `lib.rs`,
it should not be public api.
##########
crates/iceberg/src/catalog/utils.rs:
##########
@@ -120,3 +122,93 @@ async fn delete_data_files(io: &FileIO, manifest_paths:
&HashSet<String>) -> Res
})
.await
}
+
+#[cfg(test)]
+mod tests {
Review Comment:
I'm hesitating to add this ut. The preferred test should be purging
encrypted table in the catalog ut, see crates/catalog/loader/tests
##########
crates/iceberg/src/catalog/utils.rs:
##########
@@ -38,7 +39,7 @@ const DELETE_CONCURRENCY: usize = 10;
/// may share the same data files.
pub async fn drop_table_data(table_info: &Table) -> Result<()> {
let mut manifest_lists_to_delete: HashSet<String> = HashSet::new();
- let mut manifests_to_delete: HashSet<String> = HashSet::new();
+ let mut manifests_to_delete: HashSet<ManifestFile> = HashSet::new();
Review Comment:
```suggestion
let mut manifests_to_delete: HashMap<String, ManifestFile> =
HashSet::new();
```
To use `ManifestFile` as key, it requires a lot including equality and
hashing, using path as key is more safe.
--
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]
