Skip to content

Commit 3aa7ade

Browse files
authored
fix: snapshot was producing empty summary (#1767)
## Which issue does this PR close? - Closes #. There was no open issue for this. I noticed this while trying out iceberg-rust. ## What changes are included in this PR? The first line in the method `SnapshotProducer::write_added_manifest` (`let added_data_files = std::mem::take(&mut self.added_data_files);`) sets the `self.added_data_files` to an empty vec. `SnapshotProducer::write_added_manifest` is called via the call chain `SnapshotProducer::commit` -> `SnapshotProducer::manifest_file` -> `SnapshotProducer::write_added_manifest`). Hence, if `SnapshotProducer::summary` is called after calling `SnapshotProducer::manifest_file`, the summary produced was empty due to empty `self.added_data_files`. This PR rearranges the code in `SnapshotProducer::commit` to make sure the produced summary is not empty. ## Are these changes tested? No new tests have been added for this as there were no previous tests.
1 parent b3b5afe commit 3aa7ade

File tree

1 file changed

+13
-10
lines changed

1 file changed

+13
-10
lines changed

crates/iceberg/src/transaction/snapshot.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -380,17 +380,8 @@ impl<'a> SnapshotProducer<'a> {
380380
snapshot_produce_operation: OP,
381381
process: MP,
382382
) -> Result<ActionCommit> {
383-
let new_manifests = self
384-
.manifest_file(&snapshot_produce_operation, &process)
385-
.await?;
386-
let next_seq_num = self.table.metadata().next_sequence_number();
387-
388-
let summary = self.summary(&snapshot_produce_operation).map_err(|err| {
389-
Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err)
390-
})?;
391-
392383
let manifest_list_path = self.generate_manifest_list_file_path(0);
393-
384+
let next_seq_num = self.table.metadata().next_sequence_number();
394385
let mut manifest_list_writer = match self.table.metadata().format_version() {
395386
FormatVersion::V1 => ManifestListWriter::v1(
396387
self.table
@@ -408,6 +399,18 @@ impl<'a> SnapshotProducer<'a> {
408399
next_seq_num,
409400
),
410401
};
402+
403+
// Calling self.summary() before self.manifest_file() is important because self.added_data_files
404+
// will be set to an empty vec after self.manifest_file() returns, resulting in an empty summary
405+
// being generated.
406+
let summary = self.summary(&snapshot_produce_operation).map_err(|err| {
407+
Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err)
408+
})?;
409+
410+
let new_manifests = self
411+
.manifest_file(&snapshot_produce_operation, &process)
412+
.await?;
413+
411414
manifest_list_writer.add_manifests(new_manifests.into_iter())?;
412415
manifest_list_writer.close().await?;
413416

0 commit comments

Comments
 (0)