blackmwk commented on code in PR #2453:
URL: https://github.com/apache/iceberg-rust/pull/2453#discussion_r3263376615
##########
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:
We had some discussion before, and I think we should add sth like
`ManifestListLoader` to avoid such api changes.
##########
crates/iceberg/src/table.rs:
##########
@@ -356,9 +392,58 @@ impl StaticTable {
}
}
+/// If the table metadata sets the `encryption.key-id` property, build an
+/// [`EncryptionManager`] for the table.
+///
+/// Returns `Ok(None)` if the property is not set. Returns an error if the
+/// property is set but no [`KeyManagementClient`] was provided.
+fn maybe_configure_encryption(
+ kms_client: Option<&Arc<dyn KeyManagementClient>>,
+ metadata: &TableMetadataRef,
+) -> Result<Option<Arc<EncryptionManager>>> {
+ let table_properties = metadata.table_properties()?;
+ let Some(table_key_id) = table_properties.encryption_key_id else {
+ return Ok(None);
+ };
+
+ // Encryption is a v3 feature: `encryption-keys` table metadata and the
+ // snapshot `key-id` field are introduced in format version 3.
+ if metadata.format_version() < FormatVersion::V3 {
+ return Err(Error::new(
+ ErrorKind::PreconditionFailed,
+ format!(
+ "Table encryption requires format version 3, found {}",
+ metadata.format_version()
+ ),
+ ));
+ }
+
+ let kms_client = kms_client.ok_or_else(|| {
Review Comment:
Is this correct? Why user must provide a kms_client for table property?
##########
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:
The api should be as following:
```rust
impl Table {
pub fn manifest_list_loader(&self, s: &Snapshot) ->
Result<ManifestListLoader> {
...
}
}
```
With this approach, we could hide all table related constructs like
`Runtime`, `Cache`, `FileIO` to avoid api changes in future.
--
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]
