feat(encryption) [14/N] Read / Write Encrypted puffin files - #2822
feat(encryption) [14/N] Read / Write Encrypted puffin files#2822xanderbailey wants to merge 3 commits into
Conversation
xanderbailey
changed the title
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
First pass, thanks @xanderbailey!
| /// Returns a new Puffin reader | ||
| pub fn new(input_file: InputFile) -> Self { | ||
| /// Returns a new Puffin reader for an unencrypted file. | ||
| pub async fn new(input_file: InputFile) -> Result<Self> { |
There was a problem hiding this comment.
PuffinReader::new() moves from sync + infallible to async + fallible. That's a public API break for the plaintext path, and it isn't strictly required by the encrypted case. EncryptedInputFile is genuinely async, but the plaintext branch could stay lazy, either matching the existing OnceCell<FileMetadata> pattern already used in this struct, or the deferred-future approach ManifestWriterBuilder::new_from_encrypted uses (crates/iceberg/src/spec/manifest/writer.rs:80-96) to keep its own constructor sync while EncryptedOutputFile::writer() is async underneath.
Nothing in the repo calls PuffinReader::new() yet, so this is free to go either way. Was eager resolution deliberate (one reader()/metadata() call up front instead of one per blob() call, fail-fast on a missing file), or mostly a side effect of sharing from_parts with the encrypted constructor? Worth a line either way so it's clear this was a considered trade-off and not just fallout from the refactor.
There was a problem hiding this comment.
Yes I actually tried to achieve this with #2882 with the idea that we could do the same as ManifestWriterBuilder, let me know what you think of that approach?
There was a problem hiding this comment.
Okay I gave this a shot and to make it lazy we'd need some fairly awkward gymnastics. Unlike ManifestWriterBuilder, which consumes its writer_future exactly once in build, PuffinReader reads through &self in both file_metadata() and blob(), the future is consumed on poll, so a stored BoxFuture can't back repeated reads without wrapping it in something like Mutex<Option<..>> + a OnceCell to resolve-once-and-cache. The cleaner lazy shape is an enum over the two source types (InputFile / EncryptedInputFile, neither of which is Clone) resolved into a cached (Box<dyn FileRead>, u64) — but that's still an extra source enum + OnceCell purely to preserve a sync signature on an API nothing in the crate calls yet. The eager version also gives us fail-fast on a missing file at construction. I don't think it's worth the complexity, WDYT?
| async fn read_file_metadata(input_file: &InputFile) -> Result<FileMetadata> { | ||
| let file_read = input_file.reader().await?; | ||
| let file_length = input_file.metadata().await?.size; | ||
| FileMetadata::read(file_read.as_ref(), file_length).await | ||
| } | ||
|
|
||
| /// Reads file metadata with a prefetch hint from an [`InputFile`]. | ||
| async fn read_file_metadata_with_prefetch( |
There was a problem hiding this comment.
Also: crates/iceberg/src/puffin/writer.rs:217 (async fn read_file_metadata)
Now that FileMetadata::read/read_with_prefetch take (&dyn FileRead, u64) instead of &InputFile, both test modules had to grow a small input_file.reader() + .metadata() + FileMetadata::read(...) helper to bridge back to the old &InputFile-based test call sites. The metadata.rs and writer.rs versions are near-identical (one returns Result<FileMetadata>, the other unwraps). Both files already pull shared fixtures from crate::puffin::test_utils, so it's worth moving these two helpers there instead of keeping two copies in sync.
3947c5a to
8c4e603
Compare
f27aec8 to
3af2cc8
Compare
2 participants
Which issue does this PR close?
What changes are included in this PR?
Puffin files aren't yet used in the repo but we should support encryption nevertheless, similar to #2568, we add a
new_from_encryptedconstructor for the reader and writer.Are these changes tested?
Yes - roundtrip test