mbutrovich commented on code in PR #2822:
URL: https://github.com/apache/iceberg-rust/pull/2822#discussion_r3667422745
##########
crates/iceberg/src/puffin/reader.rs:
##########
@@ -19,40 +19,59 @@ use tokio::sync::OnceCell;
use super::validate_puffin_compression;
use crate::Result;
-use crate::io::InputFile;
+use crate::encryption::EncryptedInputFile;
+use crate::io::{FileRead, InputFile};
use crate::puffin::blob::Blob;
use crate::puffin::metadata::{BlobMetadata, FileMetadata};
/// Puffin reader
pub struct PuffinReader {
- input_file: InputFile,
+ file_read: Box<dyn FileRead>,
+ file_length: u64,
file_metadata: OnceCell<FileMetadata>,
}
impl PuffinReader {
- /// 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> {
Review 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.
##########
crates/iceberg/src/puffin/metadata.rs:
##########
@@ -401,9 +391,27 @@ mod tests {
java_zstd_compressed_metric_input_file,
uncompressed_metric_file_metadata,
zstd_compressed_metric_file_metadata,
};
+ use crate::{ErrorKind, Result};
const INVALID_MAGIC_VALUE: [u8; 4] = [80, 70, 65, 0];
+ /// Reads file metadata from an [`InputFile`], resolving its reader and
length.
+ 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(
Review 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.
--
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]
