blackmwk commented on code in PR #2701:
URL: https://github.com/apache/iceberg-rust/pull/2701#discussion_r3727678059
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -918,6 +1002,160 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn test_parquet_writer_encrypted_write_path() -> Result<()> {
Review Comment:
I think all cases covered by `test_parquet_writer_encrypted_roundtrip`, and
we can remove this one?
##########
crates/iceberg/src/encryption/manager.rs:
##########
@@ -429,6 +417,19 @@ impl EncryptionManager {
}
}
+/// An [`EncryptionManager`] backed by an in-memory KMS holding `table_key_id`.
+#[cfg(test)]
+pub(crate) fn test_encryption_manager(table_key_id: &str) ->
Arc<EncryptionManager> {
Review Comment:
Move this method to crate's test_util.rs mod,.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -85,19 +90,42 @@ impl ParquetWriterBuilder {
/// Currently translates the content-defined-chunking keys
/// (`write.parquet.content-defined-chunking.*`); other keys fall back to
/// parquet-rs defaults.
- pub fn from_table_properties(table_props: &TableProperties, schema:
SchemaRef) -> Self {
+ ///
+ /// Pass the table's [`EncryptionManager`] to write encrypted files; it
+ /// mints a DEK per file in [`FileWriterBuilder::build`]. Errors if
+ /// `encryption.key-id` is set and no manager is supplied, since the
+ /// resulting DEKs would be recorded in plain-text manifests.
+ pub fn from_table_properties(
+ table_props: &TableProperties,
+ schema: SchemaRef,
+ encryption_manager: Option<Arc<EncryptionManager>>,
Review Comment:
`encryption_manager` is optional, so I don't think you need to change the
public api, just add a new `with_encryption_manager` method
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -514,6 +557,41 @@ impl ParquetWriter {
}
}
+fn resolve_writer_properties(
+ writer_properties: &WriterProperties,
+ key_metadata: Option<&StandardKeyMetadata>,
+) -> Result<WriterProperties> {
+ let Some(key_metadata) = key_metadata else {
+ return Ok(writer_properties.clone());
+ };
+
+ if writer_properties.file_encryption_properties().is_some() {
+ return Err(Error::new(
+ ErrorKind::Unexpected,
+ "Parquet writer properties already have file encryption properties
set",
+ ));
+ }
+
+ let mut builder =
+
FileEncryptionProperties::builder(key_metadata.encryption_key().as_bytes().to_vec());
+ if let Some(aad) = key_metadata.aad_prefix() {
+ builder = builder.with_aad_prefix(aad.to_vec());
+ }
+ let file_encryption_properties = builder.build().map_err(|e| {
+ Error::new(
+ ErrorKind::Unexpected,
+ "Failed to build parquet file encryption properties",
+ )
+ .with_source(e)
+ })?;
+
+ Ok(writer_properties
+ .clone()
Review Comment:
nit: Do we really need this clone if we already have into_builder?
--
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]
