refactor(encryption) use StandardKeyMetadata from EncryptedOutputFile in ManifestWriterBuilder#2628
Conversation
| pub fn iceberg::spec::ManifestWriterBuilder::build_v3_deletes(self) -> iceberg::spec::ManifestWriter | ||
| pub fn iceberg::spec::ManifestWriterBuilder::new(output: iceberg::io::OutputFile, snapshot_id: core::option::Option<i64>, key_metadata: core::option::Option<alloc::vec::Vec<u8>>, schema: iceberg::spec::SchemaRef, partition_spec: iceberg::spec::PartitionSpec) -> Self | ||
| pub fn iceberg::spec::ManifestWriterBuilder::new_from_encrypted(encrypted_output: iceberg::encryption::EncryptedOutputFile, snapshot_id: core::option::Option<i64>, key_metadata: core::option::Option<alloc::vec::Vec<u8>>, schema: iceberg::spec::SchemaRef, partition_spec: iceberg::spec::PartitionSpec) -> Self | ||
| pub fn iceberg::spec::ManifestWriterBuilder::new_from_encrypted(encrypted_output: iceberg::encryption::EncryptedOutputFile, snapshot_id: core::option::Option<i64>, schema: iceberg::spec::SchemaRef, partition_spec: iceberg::spec::PartitionSpec) -> iceberg::Result<Self> |
There was a problem hiding this comment.
This isn't a break to users since this hasn't been released or used within the crate
| pub fn new( | ||
| output: OutputFile, | ||
| snapshot_id: Option<i64>, | ||
| key_metadata: Option<Vec<u8>>, |
There was a problem hiding this comment.
I didn't realise this at the time but it's likely incorrect that we have key_metadata in this constructor at all. This should never be present with a plaintext OutputFile. But in #2568 we discussed a desire to not break this API but on closer look, I think the current API here is error-prone.
There was a problem hiding this comment.
@blackmwk very interested to know your thoughts here.
There was a problem hiding this comment.
Yes, we should remove it.
There was a problem hiding this comment.
Opened a new PR here #2666, wanted to keep this one targeted. Hope that's okay.
|
failing CI is fixed in #2627 |
xanderbailey
force-pushed
the
xb/refactor_manifest_list_writer_2
branch
from
584f31f to
da189b6
Compare
2 participants
Which issue does this PR close?
In #2568 we introduced
new_from_encryptedbut the api isn't very ergonomic becauseEncryptedOutputFilehasStandardKeyMetadataon it already so we don't need to pass inkey_metadataalso.working towards #2034
What changes are included in this PR?
Are these changes tested?