xanderbailey commented on code in PR #3236:
URL: https://github.com/apache/iceberg-rust/pull/3236#discussion_r4025559408
##########
crates/iceberg/src/encryption/io.rs:
##########
@@ -138,8 +153,8 @@ impl EncryptedOutputFile {
)))
}
- /// Write bytes to file (transparently encrypted).
- pub async fn write(&self, bs: Bytes) -> Result<()> {
+ /// Write bytes to the file and return its encrypted size.
+ pub async fn write(&self, bs: Bytes) -> Result<FileMetadata> {
Review Comment:
Played around with a couple of approaches.
My preference is `key_metadata(&self)` -> `key_metadata_with_length(&self,
length: u64)`, so you can't get key metadata without supplying a length —
`output.key_metadata().encode()` becomes impossible instead of silently
emitting `file_length: None`. Draft:
https://github.com/xanderbailey/iceberg-rust/pull/4/changes
Two honest limitations: it enforces that you supplied a length, not that
it's the length of what you just wrote; and it only guards the
`EncryptedOutputFile` route, so a bare `StandardKeyMetadata::encode()` is
unaffected. I think this is okay since `StandardKeyMetadata` is really a wire
type so I think it's reasonable to assume that people should be responsible for
putting the correct length in it.
It does pick up two things for free — hoisting the output file past
`close()` is the same restructure the `.expect()` in your other comment needed,
and it lets `WriterFuture` and the `key_metadata` field go away, so it's net a
little simpler. It also covers `PuffinWriter::new_from_encrypted`, which
retains no key metadata and so already relies on the caller holding the output
file.
On the two options you suggested: both target the one-shot `write()`, which
has no production callers — every one is a test. Production goes through
`writer() -> Box<dyn FileWrite>`. Most `FileWrite` impls are plain storage
backends and shouldn't become encryption-aware, so a `WrittenFile` from
`write()` doesn't reach the paths that matter.
That's also why the length is hard to *verify*: the only designs that
guarantee it are ones where the writer produces the metadata, which means it
stops being an opaque `Box<dyn FileWrite>`. Interior mutability keeps the trait
object by handing an `Arc<OnceLock<u64>>` to `AesGcmFileWrite` to post back
after close — but `key_metadata()` then returns a `Result`, it's a runtime
check, it needs the same lifetime restructure anyway, and it couples the stream
codec to the output-file abstraction. I find it harder to reason about and I'd
rather have the compile error.
I also tried forcing the length at encode time:
https://github.com/xanderbailey/iceberg-rust/pull/3/changes That's the broader
fix — `encode()` is the one choke point all key metadata passes through, so it
covers Parquet and any future write path. But it started as a mandatory
`encode(file_length: u64)` and had to soften to `Option<u64>`:
`bindings/python/tests/test_encryption.py` pins the wire format in both
directions including the null-union tags for absent optionals, and PyIceberg's
`key_metadata.py` declares `file_length: int | None`, so length-less encoding
has to stay expressible. Python work is very new (last few days) so we could
break that if we believe it's the right path forward.
So: https://github.com/xanderbailey/iceberg-rust/pull/4/ is smaller and
can't express omission at all,
https://github.com/xanderbailey/iceberg-rust/pull/3 covers more surface but is
larger and softer. I lean towards the first.
--
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]
