dannycjones commented on code in PR #2367:
URL: https://github.com/apache/iceberg-rust/pull/2367#discussion_r3306382398
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -312,13 +325,69 @@ impl<'a> SnapshotProducer<'a> {
builder.build()
}
});
- let mut writer = self.new_manifest_writer(ManifestContentType::Data)?;
+ let mut writer = self.new_manifest_writer(
+ content_type,
+ self.table.metadata().default_partition_spec_id(),
+ )?;
for entry in manifest_entries {
writer.add_entry(entry)?;
}
writer.write_manifest_file().await
}
+ async fn write_deleted_manifest(
+ &mut self,
+ deleted_entries: Vec<ManifestEntry>,
+ ) -> Result<Vec<ManifestFile>> {
Review Comment:
Missing Rustdoc here
```suggestion
/// Write manifest files for deleted manifest entries.
///
/// Returns list of created manifest files.
async fn write_deleted_manifest(
&mut self,
deleted_entries: Vec<ManifestEntry>,
) -> Result<Vec<ManifestFile>> {
```
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
Review Comment:
Should we rename this trait function?
It was unclear to me what it was doing, and may be lingering with an old
name from when we only had simple appends.
Ultimately, it is building a list of manifest files that will be part of the
new snapshot / manifest list. Perhaps `build_manifest_file_list`?
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -511,3 +593,182 @@ impl<'a> SnapshotProducer<'a> {
Ok(ActionCommit::new(updates, requirements))
}
}
+
+#[cfg(test)]
+mod tests {
Review Comment:
Would it be useful to have a follow-up to provide simple coverage of other
manifest changes? i.e. added data files.
I'm happy to own this.
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -36,7 +36,7 @@ pub const MAIN_BRANCH: &str = "main";
/// Reference to [`Snapshot`].
pub type SnapshotRef = Arc<Snapshot>;
-#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Eq, Clone)]
+#[derive(Debug, Default, Serialize, Deserialize, PartialEq, Eq, Clone, Hash)]
Review Comment:
It might be a carry-over from the SnapshotValidator in
https://github.com/apache/iceberg-rust/pull/1606.
Perhaps we drop it from this PR for now.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
Review Comment:
Yep, looks like a gap here in the SnapshotProducer
--
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]
