xanderbailey commented on code in PR #2564:
URL: https://github.com/apache/iceberg-rust/pull/2564#discussion_r3354802152
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -421,26 +421,26 @@ impl<'a> SnapshotProducer<'a> {
let manifest_list_path = self.generate_manifest_list_file_path(0);
let next_seq_num = self.table.metadata().next_sequence_number();
let first_row_id = self.table.metadata().next_row_id();
+ let writer = self
+ .table
+ .file_io()
+ .new_output(manifest_list_path.clone())?
+ .writer()
+ .await?;
let mut manifest_list_writer = match
self.table.metadata().format_version() {
FormatVersion::V1 => ManifestListWriter::v1(
- self.table
- .file_io()
- .new_output(manifest_list_path.clone())?,
+ writer,
self.snapshot_id,
self.table.metadata().current_snapshot_id(),
),
FormatVersion::V2 => ManifestListWriter::v2(
- self.table
- .file_io()
- .new_output(manifest_list_path.clone())?,
+ writer,
self.snapshot_id,
self.table.metadata().current_snapshot_id(),
next_seq_num,
),
FormatVersion::V3 => ManifestListWriter::v3(
Review Comment:
Alternative to this would be adding a `v3_encrypted` constructor and leave
all other constructors alone and then `ManifestListWriter` internally stores a
pinned future to a file writer similar to the approach proposed in
https://github.com/apache/iceberg-rust/pull/2568 but honestly I think this API
break is acceptable and keeps constructors uniform. The worry with introducing
a new constructor here for `v3_encrypted` is it's easy for future contributors
to misuse the api and "forget" to encrypt things correctly.
--
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]
