feat(encryption) [15/N] write encrypted parquet data files#2701
feat(encryption) [15/N] write encrypted parquet data files#2701aarushigupta132 wants to merge 7 commits into
Conversation
aarushigupta132
changed the title
xanderbailey
left a comment
xanderbailey
left a comment
There was a problem hiding this comment.
Thanks carrying the flame on the encryption work. Have made a couple of comments to get us started.
aarushigupta132
changed the title
58df8f8 to
0783948
Compare
0783948 to
b80ebbf
Compare
9c618e6 to
6ca9626
Compare
6ca9626 to
12fa115
Compare
cc5fca7 to
74c54db
Compare
| type R = ParquetWriter; | ||
|
|
||
| async fn build(&self, output_file: OutputFile) -> Result<Self::R> { | ||
| let key_metadata = self |
There was a problem hiding this comment.
Confirmed that in rolling_writer as well, we build this again with new dek for each new parquet file
| /// When writing into an existing Iceberg table, prefer | ||
| /// [`Self::from_table_properties`], which derives `WriterProperties` from | ||
| /// the table's `write.parquet.*` properties. |
There was a problem hiding this comment.
curious to know if we should deprecate new and new_with_match_mode in favor of from_table_properties. Or update the comment to prefer the latter in all cases?
There was a problem hiding this comment.
I would add a comment in the doc here that encrypted writing is not supported with this constructor. I would honestly be in favour of removing this constructor since it's not used in this crate and nor should it be. I don't think we should do that in this PR but it would be a good follow-up IMO.
…lve_writer_properties param
…uet-data-files # Conflicts: # crates/iceberg/src/encryption/key_metadata.rs
xanderbailey
left a comment
xanderbailey
left a comment
There was a problem hiding this comment.
Few more comments and a test for the writer would be great. Maybe also something that shows datafusion writer works but that can be a follow up
| /// sized to `key_size`. | ||
| pub(crate) fn generate_standard_key_metadata(key_size: AesKeySize) -> StandardKeyMetadata { | ||
| let dek = SecureKey::generate(key_size); | ||
| StandardKeyMetadata::new(dek.as_bytes()).with_aad_prefix(&generate_aad_prefix()) |
| /// When writing into an existing Iceberg table, prefer | ||
| /// [`Self::from_table_properties`], which derives `WriterProperties` from | ||
| /// the table's `write.parquet.*` properties. |
There was a problem hiding this comment.
I would add a comment in the doc here that encrypted writing is not supported with this constructor. I would honestly be in favour of removing this constructor since it's not used in this crate and nor should it be. I don't think we should do that in this PR but it would be a good follow-up IMO.
| } | ||
|
|
||
| fn resolve_writer_properties( | ||
| writer_properties: &WriterProperties, |
There was a problem hiding this comment.
I would error if these WriterProperties already have encryption properties set.
…write-path tests
2 participants
Which issue does this PR close?
Working towards #2034.
What changes are included in this PR?
Adds support for writing Iceberg data files with Parquet Modular Encryption.
When a table is configured for encryption,
ParquetWriterBuilder::from_table_propertiesderives the data-encryption key size from the table properties.ParquetWriterthen mints a fresh per-file DEK, writes the file encrypted, and records the encoded key metadata on the resultingDataFileso readers can decrypt.Are these changes tested?
Existing
ParquetWritertests pass; encryption round-trip coverage lands later in the series.