Kurtiscwright commented on code in PR #2453:
URL: https://github.com/apache/iceberg-rust/pull/2453#discussion_r3320786648
##########
crates/iceberg/src/spec/table_properties.rs:
##########
@@ -128,6 +128,11 @@ pub struct TableProperties {
pub cdc_max_chunk_size: usize,
/// Content-defined chunking normalization level (gearhash bit adjustment).
pub cdc_norm_level: i32,
+ /// The master key id used to encrypt this table's manifest list and data
+ /// files. `None` if `encryption.key-id` is not set.
+ pub encryption_key_id: Option<String>,
Review Comment:
Code style nit pick: we could utilize Rust's `match` and type system if we
had a `TableProperties` without this and a separate struct for
`EncryptedTableProperties`. The encrypted struct would look like
```rust
struct EncryptedTableProperties {
/// The master key id used to encrypt this table's manifest list and data
/// files. `None` if `encryption.key-id` is not set.
pub encryption_key_id: Option<String>,
...TableProperties,
}
```
Then when you need to check to use encrypted vs non-encrypted code paths you
would just utilize `match` and check the type of the table's properties.
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -199,8 +200,22 @@ impl Snapshot {
&self,
file_io: &FileIO,
table_metadata: &TableMetadata,
+ encryption_manager: Option<&EncryptionManager>,
Review Comment:
+1 it would be nice to encapsulate these public apis now rather than
scramble when Iceberg-Rust eventually preps for its 1.0 release. @blackmwk do
you feel strongly about adding a `manifest_list_loader` method in this PR or a
future one?
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -199,8 +200,22 @@ impl Snapshot {
&self,
file_io: &FileIO,
table_metadata: &TableMetadata,
+ encryption_manager: Option<&EncryptionManager>,
) -> Result<ManifestList> {
- let manifest_list_content =
file_io.new_input(&self.manifest_list)?.read().await?;
+ let manifest_list_content = match (&self.encryption_key_id,
encryption_manager) {
+ (Some(_), None) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Snapshot has encryption_key_id but no EncryptionManager
configured on Table",
+ ));
+ }
+ (Some(key_id), Some(em)) => {
+ let key_metadata =
em.decrypt_manifest_list_key_metadata(key_id).await?;
+ let input = file_io.new_input(&self.manifest_list)?;
+ EncryptedInputFile::new(input, key_metadata).read().await?
+ }
+ (None, _) => file_io.new_input(&self.manifest_list)?.read().await?,
Review Comment:
This would be nice small place to improve on what Java does, even if its in
a follow up comment only PR.
--
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]
