jopdorp commented on code in PR #3253:
URL: https://github.com/apache/iceberg-rust/pull/3253#discussion_r4168723300
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -82,9 +83,24 @@ pub(crate) trait SnapshotProduceOperation: Send + Sync {
/// - **Overwrite operations**: May exclude manifests for partitions being
overwritten
/// - **Delete operations**: May exclude manifests for partitions being
deleted
fn existing_manifest(
- &self,
+ &mut self,
snapshot_produce: &SnapshotProducer<'_>,
) -> impl Future<Output = Result<Vec<ManifestFile>>> + Send;
+
+ /// Returns the data files this operation actually removed, each with the
schema and
+ /// partition spec of the manifest that recorded it.
+ ///
+ /// Only populated once [`Self::existing_manifest`] has run, so the
snapshot summary counts
+ /// what was really removed rather than what the caller asked to remove.
+ fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)]
{
+ &[]
+ }
+
+ /// Returns whether this operation replaces the whole table, dropping the
previous totals
+ /// from the snapshot summary. A partial overwrite must return `false`.
+ fn truncate_full_table(&self) -> bool {
Review Comment:
We went the other way here: the default is back to main's (every overwrite
resets the totals), so a full-table overwrite gets it without doing anything,
and the overwrite in #3300 opts out because it only replaces the files it
lists. Happy to tie it to the operation type instead if you prefer.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -403,6 +433,10 @@ impl<'a> SnapshotProducer<'a> {
);
}
+ for (data_file, schema, partition_spec) in
snapshot_produce_operation.removed_data_files() {
Review Comment:
We dropped `removed_data_files()` altogether. The producer now takes the
deleted files like the added ones and the summary counts those, which is what
pyiceberg's `_SnapshotProducer._summary` does. The overwrite in #3300 rejects
any path it can't find live, so each listed file ends up as exactly one Deleted
entry. That also got rid of the fake operation in the tests.
##########
crates/iceberg/src/spec/snapshot_summary.rs:
##########
@@ -532,7 +532,7 @@ fn update_totals(
return;
};
- let new_total = previous_total + added - removed;
+ let new_total = (previous_total + added).saturating_sub(removed);
Review Comment:
Done: `saturating_add` on the left, and a total that would go negative is
now left out like Java's `updateTotal`. The test asserts the key is absent.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -234,22 +250,26 @@ impl<'a> SnapshotProducer<'a> {
snapshot_id
}
- fn new_manifest_writer(&mut self, content: ManifestContentType) ->
Result<ManifestWriter> {
- let new_manifest_path = format!(
+ /// Returns the path for the next manifest file of this commit.
+ fn new_manifest_path(&self) -> Result<String> {
+ Ok(format!(
"{}/{}-m{}.{}",
self.table.metadata().metadata_location()?,
self.commit_uuid,
- self.manifest_counter.next().unwrap(),
+ self.manifest_counter.fetch_add(1, Ordering::Relaxed),
DataFileFormat::Avro
- );
- let output_file = self.table.file_io().new_output(new_manifest_path)?;
- let partition_spec = self
- .table
- .metadata()
- .default_partition_spec()
- .as_ref()
- .clone();
- let schema = self.table.metadata().current_schema().clone();
+ ))
+ }
+
+ /// Returns a writer for the next manifest file of this commit, routed
through the table's
+ /// encryption manager when one is configured.
+ pub(crate) fn new_manifest_writer(
+ &self,
+ content: ManifestContentType,
+ schema: SchemaRef,
+ partition_spec: PartitionSpec,
Review Comment:
Takes `PartitionSpecRef` now, cloned once inside for the builder.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -308,7 +328,9 @@ impl<'a> SnapshotProducer<'a> {
// Write manifest file for added data files and return the ManifestFile
for ManifestList.
async fn write_added_manifest(&mut self) -> Result<ManifestFile> {
- let added_data_files = std::mem::take(&mut self.added_data_files);
+ // Cloned rather than taken: the summary is built after the manifests,
so the added
+ // files must still be here.
+ let added_data_files = self.added_data_files.clone();
Review Comment:
Gone with the reshape: the summary is built before the manifests again, from
the producer's own lists, so `mem::take` is back.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -82,9 +83,24 @@ pub(crate) trait SnapshotProduceOperation: Send + Sync {
/// - **Overwrite operations**: May exclude manifests for partitions being
overwritten
/// - **Delete operations**: May exclude manifests for partitions being
deleted
fn existing_manifest(
- &self,
+ &mut self,
snapshot_produce: &SnapshotProducer<'_>,
) -> impl Future<Output = Result<Vec<ManifestFile>>> + Send;
+
+ /// Returns the data files this operation actually removed, each with the
schema and
+ /// partition spec of the manifest that recorded it.
+ ///
+ /// Only populated once [`Self::existing_manifest`] has run, so the
snapshot summary counts
+ /// what was really removed rather than what the caller asked to remove.
+ fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)]
{
Review Comment:
The hook is gone, so there's no tuple left to name.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -116,7 +132,7 @@ pub(crate) struct SnapshotProducer<'a> {
// A counter used to generate unique manifest file names.
// It starts from 0 and increments for each new manifest file.
// Note: This counter is limited to the range of (0..u64::MAX).
- manifest_counter: RangeFrom<u64>,
+ manifest_counter: AtomicU64,
Review Comment:
Reworded, it now just says it numbers this commit's manifests and is atomic
so `new_manifest_writer` can take `&self`.
--
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]
