Perseus commented on code in PR #3283:
URL: https://github.com/apache/iceberg-rust/pull/3283#discussion_r4537842762
##########
crates/iceberg/src/encryption/io.rs:
##########
@@ -74,8 +74,22 @@ impl EncryptedInputFile {
/// Creates a reader that transparently decrypts on each read.
pub async fn reader(&self) -> Result<Box<dyn FileRead>> {
+ // Identity just returns the file reader it was given.
+ self.reader_with_wrapper(std::convert::identity).await
+ }
+
+ /// Creates a decrypting reader, letting the caller wrap the file reader
first.
+ ///
+ /// Calls `wrap` once with the file reader and uses the result for
decryption.
+ /// This lets callers add behavior such as logging reads or counting bytes
+ /// without changing the decryption code.
+ pub(crate) async fn reader_with_wrapper<F, R>(&self, wrap: F) ->
Result<Box<dyn FileRead>>
+ where
+ F: FnOnce(Box<dyn FileRead>) -> R,
+ R: FileRead,
+ {
let encrypted_length = self.encrypted_length()?;
- let raw_reader = self.inner.reader().await?;
+ let raw_reader = Box::new(wrap(self.inner.reader().await?));
Review Comment:
This adds a new `Box<>` allocation for the normal `std::convert::identity`
reader path, when not using the `CountingFileRead`. I'm not sure how much of a
problem that is in practice; one possible way to avoid this is to return a
boxed reader from `CountingFileRead` and then remove all the `Box::new()`
wrapers here and in `caching_delete_file_loader.rs` . This would also mean that
wereturn `Box<dyn FileRead>` from the closure `F` again
--
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]
