feat(encryption): [16/N] Enable encryption for purge table - #2918
feat(encryption): [16/N] Enable encryption for purge table#2918xanderbailey wants to merge 3 commits into
Conversation
| /// (the `manifest-list-v3-encrypted.avro` testdata, an encrypted empty list) | ||
| /// are seeded into the in-memory `FileIO` at that path so callers can read | ||
| /// the current snapshot's manifest list. | ||
| pub async fn make_encrypted_table() -> Table { |
There was a problem hiding this comment.
Moved from crates/iceberg/src/transaction/mod.rs
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr!
| 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(); |
There was a problem hiding this comment.
| let mut manifests_to_delete: HashSet<ManifestFile> = HashSet::new(); | |
| 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.
There was a problem hiding this comment.
Very nice.
There was a problem hiding this comment.
Not related to this issue, but we should add an #cfg[(test)] in lib.rs, it should not be public api.
There was a problem hiding this comment.
Happy to do this in a follow up, I thought it was odd that this was pub.
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this 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
There was a problem hiding this comment.
Is there harm in a targeted unit test here? It's a little harder to write good tests at the catalog level because we block commits today (link) but I'd be happy to create an issue to add these regression tests in crates/catalog/loader/tests if that works for you?
There was a problem hiding this comment.
I'm worrying about the maintainability. I prefer to create an issue to add the integration tests later.
There was a problem hiding this comment.
This is a utilit method shared by all catalogs, so I don't want to add ut for it.
There was a problem hiding this comment.
Would you prefer to add this unit test now or when we add the catalog level tests?
There was a problem hiding this comment.
I prefer to remove this ut for now.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Discussion with @blackmwk on the manifest-path-vs-ManifestFile keying and the test-coverage question already covers what I would have raised here. Thanks @blackmwk and @xanderbailey!
3 participants
Which issue does this PR close?
Working towards #2034
What changes are included in this PR?
We previously used
FileIOto read manifest files rather than going through theload_manifestmethod which handles decryption correctly.Are these changes tested?
Tested added to show we can purge encrypted tables.