blackmwk commented on code in PR #2701:
URL: https://github.com/apache/iceberg-rust/pull/2701#discussion_r3681691826
##########
crates/iceberg/src/encryption/key_metadata.rs:
##########
@@ -108,6 +111,22 @@ impl From<SecureKey> for StandardKeyMetadata {
}
}
+/// AAD prefix length in bytes.
+const AAD_PREFIX_LENGTH: usize = 16;
+
+/// Generate a [`StandardKeyMetadata`] with a fresh random DEK and AAD prefix,
+/// sized to `key_size`.
+pub(crate) fn generate_standard_key_metadata(key_size: AesKeySize) ->
StandardKeyMetadata {
Review Comment:
```suggestion
impl StandardKeyMetadata {
pub(crate) fn generate(key_size: AesKeySize) -> StandardKeyMetadata {
```
We could make it part of `StandardKeyMetadata`, align with
`SecureKey::generate` api.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -85,19 +90,34 @@ 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 {
+ ///
+ /// When `encryption.key-id` is set, records the DEK length.
+ pub fn from_table_properties(table_props: &TableProperties, schema:
SchemaRef) -> Result<Self> {
let cdc = table_props.cdc_enabled.then_some(CdcOptions {
min_chunk_size: table_props.cdc_min_chunk_size,
max_chunk_size: table_props.cdc_max_chunk_size,
norm_level: table_props.cdc_norm_level,
});
+
// TODO: translate the remaining write.parquet.* keys (e.g.
compression-codec,
// row-group-size-bytes, page-size-bytes).
// This constructor is intended to be the single place that maps them.
let props = WriterProperties::builder()
.set_content_defined_chunking(cdc)
.build();
- Self::new_with_match_mode(props, schema, FieldMatchMode::Id)
+
+ let data_encryption_key_size = table_props
Review Comment:
I prefer to make this part of `TableProperties`, e.g. add a
`data_entryption_key_size` method
##########
crates/integrations/datafusion/src/physical_plan/write.rs:
##########
@@ -601,4 +606,100 @@ mod tests {
Ok(())
}
+
+ #[tokio::test]
+ async fn test_iceberg_write_exec_encrypted() -> Result<()> {
Review Comment:
This only tested write, not read. I perfer to use sqllogictest to set table
properties, and test read and write encrypted parquet files.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -114,13 +134,18 @@ impl FileWriterBuilder for ParquetWriterBuilder {
type R = ParquetWriter;
async fn build(&self, output_file: OutputFile) -> Result<Self::R> {
+ let key_metadata = self
Review Comment:
I think this is incorrect. The keymetadata should be injected through build,
it need to be kept in metadata file.
--
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]
