From 2641d626b213ef36e0d65403ddc405c92fecdc29 Mon Sep 17 00:00:00 2001 From: glitchy Date: Fri, 12 Jun 2026 22:09:59 -0400 Subject: [PATCH 01/35] feat(transaction): add OverwriteAction with CoW delete support Adds an OverwriteAction to Transaction for atomic replacement of data files via an Operation::Overwrite snapshot. add_data_files() stages new files; delete_data_files() rewrites affected manifests with matching entries marked ManifestStatus::Deleted (copy-on-write). Manifests are loaded through the Table::manifest_list_reader abstraction. --- crates/iceberg/src/spec/manifest/writer.rs | 8 + crates/iceberg/src/transaction/append.rs | 1 + crates/iceberg/src/transaction/mod.rs | 7 + crates/iceberg/src/transaction/overwrite.rs | 521 ++++++++++++++++++++ crates/iceberg/src/transaction/snapshot.rs | 15 + 5 files changed, 552 insertions(+) create mode 100644 crates/iceberg/src/transaction/overwrite.rs diff --git a/crates/iceberg/src/spec/manifest/writer.rs b/crates/iceberg/src/spec/manifest/writer.rs index 88da1a396c..3eae375022 100644 --- a/crates/iceberg/src/spec/manifest/writer.rs +++ b/crates/iceberg/src/spec/manifest/writer.rs @@ -382,6 +382,14 @@ impl ManifestWriter { Ok(()) } + /// Add a deleted manifest entry, preserving the original sequence numbers. + pub(crate) fn add_deleted_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { + self.check_data_file(&entry.data_file)?; + entry.status = ManifestStatus::Deleted; + self.add_entry_inner(entry)?; + Ok(()) + } + /// Add an file as existing manifest entry. The original data and file sequence numbers, snapshot ID, /// which were assigned at commit, must be preserved when adding an existing entry. pub fn add_existing_file( diff --git a/crates/iceberg/src/transaction/append.rs b/crates/iceberg/src/transaction/append.rs index 546e1a2d64..ace32418d6 100644 --- a/crates/iceberg/src/transaction/append.rs +++ b/crates/iceberg/src/transaction/append.rs @@ -90,6 +90,7 @@ impl TransactionAction for FastAppendAction { self.key_metadata.clone(), self.snapshot_properties.clone(), self.added_data_files.clone(), + vec![], ); // validate added files diff --git a/crates/iceberg/src/transaction/mod.rs b/crates/iceberg/src/transaction/mod.rs index d78f41cd42..7493521e6f 100644 --- a/crates/iceberg/src/transaction/mod.rs +++ b/crates/iceberg/src/transaction/mod.rs @@ -55,6 +55,7 @@ mod action; pub use action::*; mod append; mod expire_snapshots; +mod overwrite; mod snapshot; mod sort_order; mod update_location; @@ -75,6 +76,7 @@ use crate::table::Table; use crate::transaction::action::BoxedTransactionAction; use crate::transaction::append::FastAppendAction; use crate::transaction::expire_snapshots::ExpireSnapshotsAction; +use crate::transaction::overwrite::OverwriteAction; use crate::transaction::sort_order::ReplaceSortOrderAction; use crate::transaction::update_location::UpdateLocationAction; use crate::transaction::update_properties::UpdatePropertiesAction; @@ -151,6 +153,11 @@ impl Transaction { FastAppendAction::new() } + /// Creates an overwrite action. + pub fn overwrite(&self) -> OverwriteAction { + OverwriteAction::new() + } + /// Creates replace sort order action. pub fn replace_sort_order(&self) -> ReplaceSortOrderAction { ReplaceSortOrderAction::new() diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs new file mode 100644 index 0000000000..013162c259 --- /dev/null +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -0,0 +1,521 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; + +use async_trait::async_trait; +use uuid::Uuid; + +use crate::error::Result; +use crate::spec::{ + DataFile, FormatVersion, ManifestContentType, ManifestEntry, ManifestFile, + ManifestWriterBuilder, Operation, +}; +use crate::table::Table; +use crate::transaction::snapshot::{ + DefaultManifestProcess, SnapshotProduceOperation, SnapshotProducer, +}; +use crate::transaction::{ActionCommit, TransactionAction}; + +/// OverwriteAction is a transaction action for overwriting data files in the table. +/// +/// Creates a snapshot with `Operation::Overwrite` semantics — adds new data files and +/// optionally removes existing data files by rewriting affected manifests with those +/// entries marked as `ManifestStatus::Deleted`. +pub struct OverwriteAction { + check_duplicate: bool, + commit_uuid: Option, + key_metadata: Option>, + snapshot_properties: HashMap, + added_data_files: Vec, + deleted_data_files: Vec, +} + +impl OverwriteAction { + pub(crate) fn new() -> Self { + Self { + check_duplicate: true, + commit_uuid: None, + key_metadata: None, + snapshot_properties: HashMap::default(), + added_data_files: vec![], + deleted_data_files: vec![], + } + } + + /// Set whether to check duplicate files. + pub fn with_check_duplicate(mut self, v: bool) -> Self { + self.check_duplicate = v; + self + } + + /// Add data files to the snapshot. + pub fn add_data_files(mut self, data_files: impl IntoIterator) -> Self { + self.added_data_files.extend(data_files); + self + } + + /// Specify data files to be removed from the table in this overwrite. + pub fn delete_data_files(mut self, data_files: impl IntoIterator) -> Self { + self.deleted_data_files.extend(data_files); + self + } + + /// Set commit UUID for the snapshot. + pub fn set_commit_uuid(mut self, commit_uuid: Uuid) -> Self { + self.commit_uuid = Some(commit_uuid); + self + } + + /// Set key metadata for manifest files. + pub fn set_key_metadata(mut self, key_metadata: Vec) -> Self { + self.key_metadata = Some(key_metadata); + self + } + + /// Set snapshot summary properties. + pub fn set_snapshot_properties(mut self, snapshot_properties: HashMap) -> Self { + self.snapshot_properties = snapshot_properties; + self + } +} + +#[async_trait] +impl TransactionAction for OverwriteAction { + async fn commit(self: Arc, table: &Table) -> Result { + let snapshot_producer = SnapshotProducer::new( + table, + self.commit_uuid.unwrap_or_else(Uuid::now_v7), + self.key_metadata.clone(), + self.snapshot_properties.clone(), + self.added_data_files.clone(), + self.deleted_data_files.clone(), + ); + + snapshot_producer.validate_added_data_files()?; + + if self.check_duplicate { + snapshot_producer.validate_duplicate_files().await?; + } + + let deleted_file_paths: HashSet = self + .deleted_data_files + .iter() + .map(|f| f.file_path.clone()) + .collect(); + + let snapshot_id = snapshot_producer.snapshot_id(); + snapshot_producer + .commit( + OverwriteOperation { + deleted_file_paths, + snapshot_id, + }, + DefaultManifestProcess, + ) + .await + } +} + +struct OverwriteOperation { + deleted_file_paths: HashSet, + snapshot_id: i64, +} + +impl SnapshotProduceOperation for OverwriteOperation { + fn operation(&self) -> Operation { + Operation::Overwrite + } + + async fn delete_entries( + &self, + _snapshot_produce: &SnapshotProducer<'_>, + ) -> Result> { + Ok(vec![]) + } + + async fn existing_manifest( + &self, + snapshot_produce: &SnapshotProducer<'_>, + ) -> Result> { + let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { + return Ok(vec![]); + }; + + let manifest_list = snapshot_produce + .table + .manifest_list_reader(snapshot) + .load() + .await?; + + if self.deleted_file_paths.is_empty() { + return Ok(manifest_list + .entries() + .iter() + .filter(|entry| entry.has_added_files() || entry.has_existing_files()) + .cloned() + .collect()); + } + + let mut result = Vec::new(); + + for manifest_file in manifest_list.entries() { + if !manifest_file.has_added_files() && !manifest_file.has_existing_files() { + continue; + } + + let manifest = manifest_file + .load_manifest(snapshot_produce.table.file_io()) + .await?; + + let has_deletes = manifest.entries().iter().any(|entry| { + entry.is_alive() && self.deleted_file_paths.contains(entry.file_path()) + }); + + if has_deletes { + let rewritten = self + .rewrite_manifest(snapshot_produce, manifest_file, &manifest) + .await?; + result.push(rewritten); + } else { + result.push(manifest_file.clone()); + } + } + + Ok(result) + } +} + +impl OverwriteOperation { + /// Rewrite a manifest, marking entries whose file paths are in `deleted_file_paths` + /// as `ManifestStatus::Deleted`. + async fn rewrite_manifest( + &self, + snapshot_produce: &SnapshotProducer<'_>, + manifest_file: &ManifestFile, + manifest: &crate::spec::Manifest, + ) -> Result { + let table = snapshot_produce.table; + + let new_manifest_path = format!( + "{}/metadata/{}-m-overwrite.avro", + table.metadata().location(), + Uuid::now_v7(), + ); + let output_file = table.file_io().new_output(&new_manifest_path)?; + let builder = ManifestWriterBuilder::new( + output_file, + Some(self.snapshot_id), + manifest_file.key_metadata.clone(), + table.metadata().current_schema().clone(), + table.metadata().default_partition_spec().as_ref().clone(), + ); + + let mut writer = match table.metadata().format_version() { + FormatVersion::V1 => builder.build_v1(), + FormatVersion::V2 => match manifest_file.content { + ManifestContentType::Data => builder.build_v2_data(), + ManifestContentType::Deletes => builder.build_v2_deletes(), + }, + FormatVersion::V3 => match manifest_file.content { + ManifestContentType::Data => builder.build_v3_data(), + ManifestContentType::Deletes => builder.build_v3_deletes(), + }, + }; + + for entry in manifest.entries() { + if entry.is_alive() && self.deleted_file_paths.contains(entry.file_path()) { + let mut deleted: ManifestEntry = (**entry).clone(); + deleted.snapshot_id = Some(self.snapshot_id); + writer.add_deleted_entry(deleted)?; + } else { + let cloned: ManifestEntry = (**entry).clone(); + writer.add_existing_entry(cloned)?; + } + } + + writer.write_manifest_file().await + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::sync::Arc; + + use crate::spec::{ + DataContentType, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, ManifestStatus, + Operation, SnapshotRef, Struct, + }; + use crate::transaction::tests::make_v2_minimal_table; + use crate::transaction::{Transaction, TransactionAction}; + use crate::{TableRequirement, TableUpdate}; + + fn test_data_file(path: &str, partition_spec_id: i32) -> crate::spec::DataFile { + DataFileBuilder::default() + .content(DataContentType::Data) + .file_path(path.to_string()) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(1) + .partition_spec_id(partition_spec_id) + .partition(Struct::from_iter([Some(Literal::long(300))])) + .build() + .unwrap() + } + + #[tokio::test] + async fn test_empty_data_overwrite_action() { + let table = make_v2_minimal_table(); + let tx = Transaction::new(&table); + let action = tx.overwrite().add_data_files(vec![]); + assert!(Arc::new(action).commit(&table).await.is_err()); + } + + #[tokio::test] + async fn test_overwrite_snapshot_properties() { + let table = make_v2_minimal_table(); + let tx = Transaction::new(&table); + + let mut snapshot_properties = HashMap::new(); + snapshot_properties.insert("key".to_string(), "val".to_string()); + + let data_file = test_data_file( + "test/1.parquet", + table.metadata().default_partition_spec_id(), + ); + + let action = tx + .overwrite() + .set_snapshot_properties(snapshot_properties) + .add_data_files(vec![data_file]); + let mut action_commit = Arc::new(action).commit(&table).await.unwrap(); + let updates = action_commit.take_updates(); + + let new_snapshot = if let TableUpdate::AddSnapshot { snapshot } = &updates[0] { + snapshot + } else { + unreachable!() + }; + assert_eq!( + new_snapshot + .summary() + .additional_properties + .get("key") + .unwrap(), + "val" + ); + } + + #[tokio::test] + async fn test_overwrite_incompatible_partition_value() { + let table = make_v2_minimal_table(); + let tx = Transaction::new(&table); + + let data_file = DataFileBuilder::default() + .content(DataContentType::Data) + .file_path("test/3.parquet".to_string()) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(1) + .partition_spec_id(table.metadata().default_partition_spec_id()) + .partition(Struct::from_iter([Some(Literal::string("test"))])) + .build() + .unwrap(); + + let action = tx.overwrite().add_data_files(vec![data_file]); + assert!(Arc::new(action).commit(&table).await.is_err()); + } + + #[tokio::test] + async fn test_overwrite_basic() { + let table = make_v2_minimal_table(); + let tx = Transaction::new(&table); + + let data_file = test_data_file( + "test/3.parquet", + table.metadata().default_partition_spec_id(), + ); + + let action = tx.overwrite().add_data_files(vec![data_file.clone()]); + let mut action_commit = Arc::new(action).commit(&table).await.unwrap(); + let updates = action_commit.take_updates(); + let requirements = action_commit.take_requirements(); + + assert!( + matches!((&updates[0],&updates[1]), (TableUpdate::AddSnapshot { snapshot },TableUpdate::SetSnapshotRef { reference,ref_name }) if snapshot.snapshot_id() == reference.snapshot_id && ref_name == MAIN_BRANCH) + ); + + assert_eq!( + vec![ + TableRequirement::UuidMatch { + uuid: table.metadata().uuid() + }, + TableRequirement::RefSnapshotIdMatch { + r#ref: MAIN_BRANCH.to_string(), + snapshot_id: table.metadata().current_snapshot_id + } + ], + requirements + ); + + let new_snapshot: SnapshotRef = if let TableUpdate::AddSnapshot { snapshot } = &updates[0] { + SnapshotRef::new(snapshot.clone()) + } else { + unreachable!() + }; + assert_eq!(new_snapshot.summary().operation, Operation::Overwrite); + + let manifest_list = table + .manifest_list_reader(&new_snapshot) + .load() + .await + .unwrap(); + assert_eq!(1, manifest_list.entries().len()); + assert_eq!( + manifest_list.entries()[0].sequence_number, + new_snapshot.sequence_number() + ); + + let manifest = manifest_list.entries()[0] + .load_manifest(table.file_io()) + .await + .unwrap(); + assert_eq!(1, manifest.entries().len()); + assert_eq!( + new_snapshot.sequence_number(), + manifest.entries()[0] + .sequence_number() + .expect("Inherit sequence number by load manifest") + ); + assert_eq!( + new_snapshot.snapshot_id(), + manifest.entries()[0].snapshot_id().unwrap() + ); + assert_eq!(data_file, *manifest.entries()[0].data_file()); + } + + #[tokio::test] + async fn test_overwrite_with_deleted_files() { + use crate::memory::tests::new_memory_catalog; + use crate::transaction::ApplyTransactionAction; + use crate::transaction::tests::make_v3_minimal_table_in_catalog; + + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let original_file = test_data_file("test/original.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx.fast_append().add_data_files(vec![original_file.clone()]); + let tx = action.apply(tx).unwrap(); + let table = tx.commit(&catalog).await.unwrap(); + + let snapshot = table.metadata().current_snapshot().unwrap(); + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + assert_eq!(1, manifest_list.entries().len()); + + let replacement_file = test_data_file("test/replacement.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![replacement_file.clone()]) + .delete_data_files(vec![original_file.clone()]); + let tx = action.apply(tx).unwrap(); + let table = tx.commit(&catalog).await.unwrap(); + + let snapshot = table.metadata().current_snapshot().unwrap(); + assert_eq!(snapshot.summary().operation, Operation::Overwrite); + + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + + assert_eq!(2, manifest_list.entries().len()); + + let mut all_entries = vec![]; + for manifest_file in manifest_list.entries() { + let manifest = manifest_file.load_manifest(table.file_io()).await.unwrap(); + for entry in manifest.entries() { + all_entries.push((entry.status(), entry.file_path().to_string())); + } + } + + assert!( + all_entries + .iter() + .any(|(status, path)| *status == ManifestStatus::Deleted + && path == "test/original.parquet"), + "Original file should be marked as Deleted, entries: {all_entries:?}", + ); + + assert!( + all_entries + .iter() + .any(|(status, path)| *status == ManifestStatus::Added + && path == "test/replacement.parquet"), + "Replacement file should be marked as Added, entries: {all_entries:?}", + ); + + // Verify snapshot summary reports the deleted file. + assert_eq!( + snapshot + .summary() + .additional_properties + .get("deleted-data-files") + .map(|s| s.as_str()), + Some("1") + ); + assert_eq!( + snapshot + .summary() + .additional_properties + .get("deleted-records") + .map(|s| s.as_str()), + Some("1") + ); + + // Step 3: Fast append after overwrite — delete-only manifest must survive. + let appended_file = test_data_file("test/appended.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx.fast_append().add_data_files(vec![appended_file.clone()]); + let tx = action.apply(tx).unwrap(); + let table = tx.commit(&catalog).await.unwrap(); + + let snapshot = table.metadata().current_snapshot().unwrap(); + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + + // 3 manifests: rewritten (deleted entry), overwrite added, fast_append added. + assert_eq!(3, manifest_list.entries().len()); + + let mut all_entries = vec![]; + for manifest_file in manifest_list.entries() { + let manifest = manifest_file.load_manifest(table.file_io()).await.unwrap(); + for entry in manifest.entries() { + all_entries.push((entry.status(), entry.file_path().to_string())); + } + } + + // The deleted entry must still be present after fast_append. + assert!( + all_entries + .iter() + .any(|(status, path)| *status == ManifestStatus::Deleted + && path == "test/original.parquet"), + "Deleted entry should survive fast_append, entries: {all_entries:?}", + ); + } +} diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index d4a3a7fdee..72bb4673fe 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -116,6 +116,7 @@ pub(crate) struct SnapshotProducer<'a> { key_metadata: Option>, snapshot_properties: HashMap, added_data_files: Vec, + deleted_data_files: Vec, // 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). @@ -123,12 +124,17 @@ pub(crate) struct SnapshotProducer<'a> { } impl<'a> SnapshotProducer<'a> { + pub(crate) fn snapshot_id(&self) -> i64 { + self.snapshot_id + } + pub(crate) fn new( table: &'a Table, commit_uuid: Uuid, key_metadata: Option>, snapshot_properties: HashMap, added_data_files: Vec, + deleted_data_files: Vec, ) -> Self { Self { table, @@ -137,6 +143,7 @@ impl<'a> SnapshotProducer<'a> { key_metadata, snapshot_properties, added_data_files, + deleted_data_files, manifest_counter: (0..), } } @@ -402,6 +409,14 @@ impl<'a> SnapshotProducer<'a> { ); } + for data_file in &self.deleted_data_files { + summary_collector.remove_file( + data_file, + table_metadata.current_schema().clone(), + table_metadata.default_partition_spec().clone(), + ); + } + let previous_snapshot = table_metadata.current_snapshot(); // User-supplied snapshot properties are applied first, then the computed From 4999d6935b646dc8da8113d71401501b8ded1574 Mon Sep 17 00:00:00 2001 From: Tim Saucer Date: Wed, 11 Mar 2026 18:36:01 +0100 Subject: [PATCH 02/35] Get the previous snapshot directly from the table_metadata and add update unit test --- crates/iceberg/src/transaction/overwrite.rs | 43 ++++++++++++--------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 013162c259..b1d34b298b 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -420,9 +420,10 @@ mod tests { let table = make_v3_minimal_table_in_catalog(&catalog).await; let spec_id = table.metadata().default_partition_spec_id(); - let original_file = test_data_file("test/original.parquet", spec_id); + let original_file1 = test_data_file("test/original1.parquet", spec_id); + let original_file2 = test_data_file("test/original2.parquet", spec_id); let tx = Transaction::new(&table); - let action = tx.fast_append().add_data_files(vec![original_file.clone()]); + let action = tx.fast_append().add_data_files(vec![original_file1.clone(), original_file2.clone()]); let tx = action.apply(tx).unwrap(); let table = tx.commit(&catalog).await.unwrap(); @@ -435,7 +436,7 @@ mod tests { let action = tx .overwrite() .add_data_files(vec![replacement_file.clone()]) - .delete_data_files(vec![original_file.clone()]); + .delete_data_files(vec![original_file1.clone(), original_file2.clone()]); let tx = action.apply(tx).unwrap(); let table = tx.commit(&catalog).await.unwrap(); @@ -454,13 +455,15 @@ mod tests { } } - assert!( - all_entries - .iter() - .any(|(status, path)| *status == ManifestStatus::Deleted - && path == "test/original.parquet"), - "Original file should be marked as Deleted, entries: {all_entries:?}", - ); + for original_file in ["test/original1.parquet", "test/original2.parquet"] { + assert!( + all_entries + .iter() + .any(|(status, path)| *status == ManifestStatus::Deleted + && path == original_file), + "Original file {original_file} should be marked as Deleted, entries: {all_entries:?}", + ); + } assert!( all_entries @@ -477,7 +480,7 @@ mod tests { .additional_properties .get("deleted-data-files") .map(|s| s.as_str()), - Some("1") + Some("2") ); assert_eq!( snapshot @@ -485,7 +488,7 @@ mod tests { .additional_properties .get("deleted-records") .map(|s| s.as_str()), - Some("1") + Some("2") ); // Step 3: Fast append after overwrite — delete-only manifest must survive. @@ -510,12 +513,14 @@ mod tests { } // The deleted entry must still be present after fast_append. - assert!( - all_entries - .iter() - .any(|(status, path)| *status == ManifestStatus::Deleted - && path == "test/original.parquet"), - "Deleted entry should survive fast_append, entries: {all_entries:?}", - ); + for original_file in ["test/original1.parquet", "test/original2.parquet"] { + assert!( + all_entries + .iter() + .any(|(status, path)| *status == ManifestStatus::Deleted + && path == original_file), + "Deleted entry should survive fast_append, entries: {all_entries:?}", + ); + } } } From a11e3b5a8b313f281405071d32934aebaea741a9 Mon Sep 17 00:00:00 2001 From: Gerald Berger Date: Thu, 28 May 2026 08:56:27 +0200 Subject: [PATCH 03/35] allow delete-only overwrite when no data files are added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SnapshotProducer precondition check blocked snapshots with only deleted files and no added files or snapshot properties. A delete-only Overwrite snapshot is valid per the Iceberg spec — the existing manifests are rewritten with the target entries marked as ManifestStatus::Deleted. Relax the check to also allow the case when deleted_data_files is non-empty, so callers can clear a table without simultaneously adding new files. Co-Authored-By: Claude Sonnet 4.6 --- crates/iceberg/src/transaction/snapshot.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 72bb4673fe..61a90c8f6b 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -355,7 +355,13 @@ impl<'a> SnapshotProducer<'a> { // TODO: Allowing snapshot property setup with no added data files is a workaround. // We should clean it up after all necessary actions are supported. // For details, please refer to https://github.com/apache/iceberg-rust/issues/1548 - if self.added_data_files.is_empty() && self.snapshot_properties.is_empty() { + // + // A delete-only overwrite (no added files, no properties, but with deleted files) is + // valid per the Iceberg spec — the existing manifests are rewritten with deleted entries. + if self.added_data_files.is_empty() + && self.snapshot_properties.is_empty() + && self.deleted_data_files.is_empty() + { return Err(Error::new( ErrorKind::PreconditionFailed, "No added data files or added snapshot properties found when write a manifest file", From 5493b3a5ec61de6aa9bf4ac7f61cb514125be76a Mon Sep 17 00:00:00 2001 From: glitchy Date: Fri, 12 Jun 2026 22:19:24 -0400 Subject: [PATCH 04/35] chore(transaction): rustfmt and refresh public API snapshot for OverwriteAction --- crates/iceberg/public-api.txt | 1 + crates/iceberg/src/transaction/overwrite.rs | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 653649e6cf..6311d6b6a9 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -3081,6 +3081,7 @@ pub async fn iceberg::transaction::Transaction::commit(self, catalog: &dyn icebe pub fn iceberg::transaction::Transaction::expire_snapshots(&self) -> iceberg::transaction::expire_snapshots::ExpireSnapshotsAction pub fn iceberg::transaction::Transaction::fast_append(&self) -> iceberg::transaction::append::FastAppendAction pub fn iceberg::transaction::Transaction::new(table: &iceberg::table::Table) -> Self +pub fn iceberg::transaction::Transaction::overwrite(&self) -> iceberg::transaction::overwrite::OverwriteAction pub fn iceberg::transaction::Transaction::replace_sort_order(&self) -> iceberg::transaction::sort_order::ReplaceSortOrderAction pub fn iceberg::transaction::Transaction::update_location(&self) -> iceberg::transaction::update_location::UpdateLocationAction pub fn iceberg::transaction::Transaction::update_schema(&self) -> iceberg::transaction::update_schema::UpdateSchemaAction diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index b1d34b298b..818637866a 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -423,7 +423,9 @@ mod tests { let original_file1 = test_data_file("test/original1.parquet", spec_id); let original_file2 = test_data_file("test/original2.parquet", spec_id); let tx = Transaction::new(&table); - let action = tx.fast_append().add_data_files(vec![original_file1.clone(), original_file2.clone()]); + let action = tx + .fast_append() + .add_data_files(vec![original_file1.clone(), original_file2.clone()]); let tx = action.apply(tx).unwrap(); let table = tx.commit(&catalog).await.unwrap(); From 2b175f2c53dce4ed8f81848c9e612ca356f5bb4f Mon Sep 17 00:00:00 2001 From: glitchy Date: Mon, 29 Jun 2026 23:33:56 -0400 Subject: [PATCH 05/35] refactor(transaction): hoist inline spec imports in overwrite Import Manifest and DataFile at the top instead of using fully-qualified crate::spec:: paths inline in a signature and a test helper. --- crates/iceberg/src/transaction/overwrite.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 818637866a..961c3e43f8 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -23,7 +23,7 @@ use uuid::Uuid; use crate::error::Result; use crate::spec::{ - DataFile, FormatVersion, ManifestContentType, ManifestEntry, ManifestFile, + DataFile, FormatVersion, Manifest, ManifestContentType, ManifestEntry, ManifestFile, ManifestWriterBuilder, Operation, }; use crate::table::Table; @@ -208,7 +208,7 @@ impl OverwriteOperation { &self, snapshot_produce: &SnapshotProducer<'_>, manifest_file: &ManifestFile, - manifest: &crate::spec::Manifest, + manifest: &Manifest, ) -> Result { let table = snapshot_produce.table; @@ -259,14 +259,14 @@ mod tests { use std::sync::Arc; use crate::spec::{ - DataContentType, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, ManifestStatus, - Operation, SnapshotRef, Struct, + DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, + ManifestStatus, Operation, SnapshotRef, Struct, }; use crate::transaction::tests::make_v2_minimal_table; use crate::transaction::{Transaction, TransactionAction}; use crate::{TableRequirement, TableUpdate}; - fn test_data_file(path: &str, partition_spec_id: i32) -> crate::spec::DataFile { + fn test_data_file(path: &str, partition_spec_id: i32) -> DataFile { DataFileBuilder::default() .content(DataContentType::Data) .file_path(path.to_string()) From ede30bd7724844d12ba3406e66be7bd5deebaf2f Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:16:49 +0200 Subject: [PATCH 06/35] Adapt to current SnapshotProducer and manifest reader APIs --- crates/iceberg/src/transaction/overwrite.rs | 25 ++++++++------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 961c3e43f8..8c9fc4db5a 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -40,7 +40,6 @@ use crate::transaction::{ActionCommit, TransactionAction}; pub struct OverwriteAction { check_duplicate: bool, commit_uuid: Option, - key_metadata: Option>, snapshot_properties: HashMap, added_data_files: Vec, deleted_data_files: Vec, @@ -51,7 +50,6 @@ impl OverwriteAction { Self { check_duplicate: true, commit_uuid: None, - key_metadata: None, snapshot_properties: HashMap::default(), added_data_files: vec![], deleted_data_files: vec![], @@ -82,12 +80,6 @@ impl OverwriteAction { self } - /// Set key metadata for manifest files. - pub fn set_key_metadata(mut self, key_metadata: Vec) -> Self { - self.key_metadata = Some(key_metadata); - self - } - /// Set snapshot summary properties. pub fn set_snapshot_properties(mut self, snapshot_properties: HashMap) -> Self { self.snapshot_properties = snapshot_properties; @@ -101,7 +93,6 @@ impl TransactionAction for OverwriteAction { let snapshot_producer = SnapshotProducer::new( table, self.commit_uuid.unwrap_or_else(Uuid::now_v7), - self.key_metadata.clone(), self.snapshot_properties.clone(), self.added_data_files.clone(), self.deleted_data_files.clone(), @@ -179,8 +170,10 @@ impl SnapshotProduceOperation for OverwriteOperation { continue; } - let manifest = manifest_file - .load_manifest(snapshot_produce.table.file_io()) + let manifest = snapshot_produce + .table + .manifest_reader() + .read(manifest_file) .await?; let has_deletes = manifest.entries().iter().any(|entry| { @@ -221,7 +214,6 @@ impl OverwriteOperation { let builder = ManifestWriterBuilder::new( output_file, Some(self.snapshot_id), - manifest_file.key_metadata.clone(), table.metadata().current_schema().clone(), table.metadata().default_partition_spec().as_ref().clone(), ); @@ -392,8 +384,9 @@ mod tests { new_snapshot.sequence_number() ); - let manifest = manifest_list.entries()[0] - .load_manifest(table.file_io()) + let manifest = table + .manifest_reader() + .read(&manifest_list.entries()[0]) .await .unwrap(); assert_eq!(1, manifest.entries().len()); @@ -451,7 +444,7 @@ mod tests { let mut all_entries = vec![]; for manifest_file in manifest_list.entries() { - let manifest = manifest_file.load_manifest(table.file_io()).await.unwrap(); + let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); for entry in manifest.entries() { all_entries.push((entry.status(), entry.file_path().to_string())); } @@ -508,7 +501,7 @@ mod tests { let mut all_entries = vec![]; for manifest_file in manifest_list.entries() { - let manifest = manifest_file.load_manifest(table.file_io()).await.unwrap(); + let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); for entry in manifest.entries() { all_entries.push((entry.status(), entry.file_path().to_string())); } From 26645050628d2a205ddbafc1bff75b0d1a2b716b Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:25:49 +0200 Subject: [PATCH 07/35] Keep deleted entries deleted when rewriting a manifest A manifest rewritten by a second overwrite fell through to add_existing_entry for entries that were already Deleted, forcing their status back to Existing and resurrecting the files as live data. Guard on is_alive() first and re-add prior tombstones as Deleted with their original snapshot id, matching Java's filterManifestWithDeletedFiles, which only re-adds live entries. --- crates/iceberg/src/transaction/overwrite.rs | 104 +++++++++++++++++--- 1 file changed, 93 insertions(+), 11 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 8c9fc4db5a..093f26ca68 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -231,13 +231,17 @@ impl OverwriteOperation { }; for entry in manifest.entries() { - if entry.is_alive() && self.deleted_file_paths.contains(entry.file_path()) { - let mut deleted: ManifestEntry = (**entry).clone(); + let entry: ManifestEntry = (**entry).clone(); + if !entry.is_alive() { + // A tombstone keeps its original snapshot id so the delete stays attributed + // to the snapshot that made it. + writer.add_deleted_entry(entry)?; + } else if self.deleted_file_paths.contains(entry.file_path()) { + let mut deleted = entry; deleted.snapshot_id = Some(self.snapshot_id); writer.add_deleted_entry(deleted)?; } else { - let cloned: ManifestEntry = (**entry).clone(); - writer.add_existing_entry(cloned)?; + writer.add_existing_entry(entry)?; } } @@ -250,12 +254,14 @@ mod tests { use std::collections::HashMap; use std::sync::Arc; + use crate::memory::tests::new_memory_catalog; use crate::spec::{ DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, - ManifestStatus, Operation, SnapshotRef, Struct, + ManifestEntryRef, ManifestStatus, Operation, SnapshotRef, Struct, }; - use crate::transaction::tests::make_v2_minimal_table; - use crate::transaction::{Transaction, TransactionAction}; + use crate::table::Table; + use crate::transaction::tests::{make_v2_minimal_table, make_v3_minimal_table_in_catalog}; + use crate::transaction::{ApplyTransactionAction, Transaction, TransactionAction}; use crate::{TableRequirement, TableUpdate}; fn test_data_file(path: &str, partition_spec_id: i32) -> DataFile { @@ -271,6 +277,33 @@ mod tests { .unwrap() } + async fn current_manifest_entries(table: &Table) -> Vec<(ManifestStatus, String)> { + let snapshot = table.metadata().current_snapshot().unwrap(); + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + + let mut entries = vec![]; + for manifest_file in manifest_list.entries() { + let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); + for entry in manifest.entries() { + entries.push((entry.status(), entry.file_path().to_string())); + } + } + entries + } + + async fn current_manifest_entry(table: &Table, path: &str) -> ManifestEntryRef { + let snapshot = table.metadata().current_snapshot().unwrap(); + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + + for manifest_file in manifest_list.entries() { + let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); + if let Some(entry) = manifest.entries().iter().find(|e| e.file_path() == path) { + return entry.clone(); + } + } + panic!("no manifest entry for {path}"); + } + #[tokio::test] async fn test_empty_data_overwrite_action() { let table = make_v2_minimal_table(); @@ -405,10 +438,6 @@ mod tests { #[tokio::test] async fn test_overwrite_with_deleted_files() { - use crate::memory::tests::new_memory_catalog; - use crate::transaction::ApplyTransactionAction; - use crate::transaction::tests::make_v3_minimal_table_in_catalog; - let catalog = new_memory_catalog().await; let table = make_v3_minimal_table_in_catalog(&catalog).await; let spec_id = table.metadata().default_partition_spec_id(); @@ -518,4 +547,57 @@ mod tests { ); } } + + #[tokio::test] + async fn test_second_overwrite_keeps_prior_deleted_entries_deleted() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let file_a = test_data_file("test/a.parquet", spec_id); + let file_b = test_data_file("test/b.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx + .fast_append() + .add_data_files(vec![file_a.clone(), file_b.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file("test/c.parquet", spec_id)]) + .delete_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + let tombstone = current_manifest_entry(&table, "test/a.parquet").await; + assert_eq!(ManifestStatus::Deleted, tombstone.status()); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file("test/d.parquet", spec_id)]) + .delete_data_files(vec![file_b.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let carried = current_manifest_entry(&table, "test/a.parquet").await; + assert_eq!( + ManifestStatus::Deleted, + carried.status(), + "the first overwrite's tombstone must stay Deleted" + ); + assert_eq!( + tombstone.snapshot_id, carried.snapshot_id, + "the tombstone must keep the snapshot that deleted the file" + ); + assert_eq!( + (tombstone.sequence_number, tombstone.file_sequence_number), + (carried.sequence_number, carried.file_sequence_number), + "the tombstone must keep its original sequence numbers" + ); + + let entries = current_manifest_entries(&table).await; + assert!( + entries.contains(&(ManifestStatus::Deleted, "test/b.parquet".to_string())), + "the second overwrite's own delete must be a tombstone, entries: {entries:?}" + ); + } } From ca9bad14d2b17f33d1b76507b49b422a342356b8 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:28:32 +0200 Subject: [PATCH 08/35] Stamp a rewritten manifest with the schema and spec it was written under The rewrite used the table's current schema and default partition spec, so after a schema or partition evolution the Avro header carried the wrong schema-id and partition-spec-id and readers interpreted the partition values against the wrong mapping. Take both from the manifest being rewritten, as Java does via newManifestWriter(reader.spec()). --- crates/iceberg/src/transaction/overwrite.rs | 90 ++++++++++++++++++++- 1 file changed, 86 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 093f26ca68..9bd6bb526e 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -214,8 +214,8 @@ impl OverwriteOperation { let builder = ManifestWriterBuilder::new( output_file, Some(self.snapshot_id), - table.metadata().current_schema().clone(), - table.metadata().default_partition_spec().as_ref().clone(), + manifest.metadata().schema.clone(), + manifest.metadata().partition_spec.clone(), ); let mut writer = match table.metadata().format_version() { @@ -257,11 +257,12 @@ mod tests { use crate::memory::tests::new_memory_catalog; use crate::spec::{ DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, - ManifestEntryRef, ManifestStatus, Operation, SnapshotRef, Struct, + ManifestEntryRef, ManifestStatus, Operation, PrimitiveType, SnapshotRef, Struct, Transform, + Type, UnboundPartitionSpec, }; use crate::table::Table; use crate::transaction::tests::{make_v2_minimal_table, make_v3_minimal_table_in_catalog}; - use crate::transaction::{ApplyTransactionAction, Transaction, TransactionAction}; + use crate::transaction::{AddColumn, ApplyTransactionAction, Transaction, TransactionAction}; use crate::{TableRequirement, TableUpdate}; fn test_data_file(path: &str, partition_spec_id: i32) -> DataFile { @@ -600,4 +601,85 @@ mod tests { "the second overwrite's own delete must be a tombstone, entries: {entries:?}" ); } + + #[tokio::test] + async fn test_rewritten_manifest_keeps_the_schema_and_spec_it_was_written_with() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let original_spec_id = table.metadata().default_partition_spec_id(); + let original_schema_id = table.metadata().current_schema_id(); + + let file_a = test_data_file("test/a.parquet", original_spec_id); + let tx = Transaction::new(&table); + let action = tx.fast_append().add_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx.update_schema().add_column(AddColumn::optional( + "extra", + Type::Primitive(PrimitiveType::Int), + )); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + // There is no transaction action for partition evolution yet, so the spec is replaced + // on the metadata directly. + let evolved = table + .metadata() + .clone() + .into_builder(None) + .add_default_partition_spec( + UnboundPartitionSpec::builder() + .add_partition_field(2, "y", Transform::Identity) + .unwrap() + .build(), + ) + .unwrap() + .build() + .unwrap() + .metadata; + let table = table.with_metadata(Arc::new(evolved)); + assert_ne!(original_schema_id, table.metadata().current_schema_id()); + assert_ne!( + original_spec_id, + table.metadata().default_partition_spec_id() + ); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file( + "test/b.parquet", + table.metadata().default_partition_spec_id(), + )]) + .delete_data_files(vec![file_a.clone()]); + let mut action_commit = Arc::new(action).commit(&table).await.unwrap(); + let updates = action_commit.take_updates(); + let new_snapshot: SnapshotRef = if let TableUpdate::AddSnapshot { snapshot } = &updates[0] { + SnapshotRef::new(snapshot.clone()) + } else { + unreachable!() + }; + + let manifest_list = table + .manifest_list_reader(&new_snapshot) + .load() + .await + .unwrap(); + let mut rewritten = None; + for manifest_file in manifest_list.entries() { + let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); + if manifest + .entries() + .iter() + .any(|entry| entry.status() == ManifestStatus::Deleted) + { + rewritten = Some((manifest_file.clone(), manifest)); + } + } + let (rewritten_file, rewritten_manifest) = + rewritten.expect("the manifest holding test/a.parquet must have been rewritten"); + + assert_eq!(original_schema_id, rewritten_manifest.metadata().schema_id); + assert_eq!(original_spec_id, rewritten_file.partition_spec_id); + } } From d782adc6755e058b3a7adc9125069b719be87969 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:30:45 +0200 Subject: [PATCH 09/35] Keep delete-only manifests when overwriting Both manifest filters dropped a manifest whose entries were all Deleted, which lets the removed files reappear as live data. This is the same fix FastAppend took in #2545 for #2148, applied to the no-deletes fast path and to the rewrite loop. --- crates/iceberg/src/transaction/overwrite.rs | 44 ++++++++++++++++++++- 1 file changed, 42 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 9bd6bb526e..8a8f4766e4 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -158,7 +158,13 @@ impl SnapshotProduceOperation for OverwriteOperation { return Ok(manifest_list .entries() .iter() - .filter(|entry| entry.has_added_files() || entry.has_existing_files()) + .filter(|entry| { + // Delete-only manifests record which files were removed and must survive + // until `expire_snapshots` cleans them up (see #2148). + entry.has_added_files() + || entry.has_existing_files() + || entry.has_deleted_files() + }) .cloned() .collect()); } @@ -166,7 +172,10 @@ impl SnapshotProduceOperation for OverwriteOperation { let mut result = Vec::new(); for manifest_file in manifest_list.entries() { - if !manifest_file.has_added_files() && !manifest_file.has_existing_files() { + if !manifest_file.has_added_files() + && !manifest_file.has_existing_files() + && !manifest_file.has_deleted_files() + { continue; } @@ -682,4 +691,35 @@ mod tests { assert_eq!(original_schema_id, rewritten_manifest.metadata().schema_id); assert_eq!(original_spec_id, rewritten_file.partition_spec_id); } + + #[tokio::test] + async fn test_overwrite_preserves_delete_only_manifest() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let file_a = test_data_file("test/a.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx.fast_append().add_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file("test/b.parquet", spec_id)]) + .delete_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file("test/c.parquet", spec_id)]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let entries = current_manifest_entries(&table).await; + assert!( + entries.contains(&(ManifestStatus::Deleted, "test/a.parquet".to_string())), + "the delete-only manifest must survive a later overwrite, entries: {entries:?}" + ); + } } From cbae2456161d79f7d0b68e0de1e8301761d2cd61 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:36:29 +0200 Subject: [PATCH 10/35] Count the deletions the overwrite actually made The summary counted every file the caller passed to delete_data_files, so a path that was never committed or was passed twice inflated deleted-data-files and deleted-records. The rewrite pass now records the entries it marked Deleted, with the schema and partition spec of the manifest each was recorded in, and the summary counts those. Two changes follow from it. The summary is built after the manifests rather than before, since the counts are only known once the rewrite has run. And the full-table truncate path is now gated on an operation hook that a partial overwrite answers false, instead of on the operation being Overwrite; with truncate off, update_totals needs a saturating subtraction so an overwrite that deletes more than the previous total does not underflow. --- crates/iceberg/src/spec/snapshot_summary.rs | 2 +- crates/iceberg/src/transaction/append.rs | 2 +- crates/iceberg/src/transaction/overwrite.rs | 54 ++++++++++++++++++-- crates/iceberg/src/transaction/snapshot.rs | 55 +++++++++++++-------- 4 files changed, 87 insertions(+), 26 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot_summary.rs b/crates/iceberg/src/spec/snapshot_summary.rs index c9fd022724..568dc54dba 100644 --- a/crates/iceberg/src/spec/snapshot_summary.rs +++ b/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); summary .additional_properties .insert(total_property.to_string(), new_total.to_string()); diff --git a/crates/iceberg/src/transaction/append.rs b/crates/iceberg/src/transaction/append.rs index e510d4a4b8..a3dc6a31f1 100644 --- a/crates/iceberg/src/transaction/append.rs +++ b/crates/iceberg/src/transaction/append.rs @@ -125,7 +125,7 @@ impl SnapshotProduceOperation for FastAppendOperation { } async fn existing_manifest( - &self, + &mut self, snapshot_produce: &SnapshotProducer<'_>, ) -> Result> { let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 8a8f4766e4..1d57844ebe 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -24,7 +24,7 @@ use uuid::Uuid; use crate::error::Result; use crate::spec::{ DataFile, FormatVersion, Manifest, ManifestContentType, ManifestEntry, ManifestFile, - ManifestWriterBuilder, Operation, + ManifestWriterBuilder, Operation, PartitionSpecRef, SchemaRef, }; use crate::table::Table; use crate::transaction::snapshot::{ @@ -116,6 +116,7 @@ impl TransactionAction for OverwriteAction { OverwriteOperation { deleted_file_paths, snapshot_id, + removed_data_files: vec![], }, DefaultManifestProcess, ) @@ -126,6 +127,7 @@ impl TransactionAction for OverwriteAction { struct OverwriteOperation { deleted_file_paths: HashSet, snapshot_id: i64, + removed_data_files: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, } impl SnapshotProduceOperation for OverwriteOperation { @@ -140,8 +142,12 @@ impl SnapshotProduceOperation for OverwriteOperation { Ok(vec![]) } + fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)] { + &self.removed_data_files + } + async fn existing_manifest( - &self, + &mut self, snapshot_produce: &SnapshotProducer<'_>, ) -> Result> { let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { @@ -207,7 +213,7 @@ impl OverwriteOperation { /// Rewrite a manifest, marking entries whose file paths are in `deleted_file_paths` /// as `ManifestStatus::Deleted`. async fn rewrite_manifest( - &self, + &mut self, snapshot_produce: &SnapshotProducer<'_>, manifest_file: &ManifestFile, manifest: &Manifest, @@ -220,6 +226,7 @@ impl OverwriteOperation { Uuid::now_v7(), ); let output_file = table.file_io().new_output(&new_manifest_path)?; + let partition_spec: PartitionSpecRef = Arc::new(manifest.metadata().partition_spec.clone()); let builder = ManifestWriterBuilder::new( output_file, Some(self.snapshot_id), @@ -248,6 +255,11 @@ impl OverwriteOperation { } else if self.deleted_file_paths.contains(entry.file_path()) { let mut deleted = entry; deleted.snapshot_id = Some(self.snapshot_id); + self.removed_data_files.push(( + deleted.data_file().clone(), + manifest.metadata().schema.clone(), + partition_spec.clone(), + )); writer.add_deleted_entry(deleted)?; } else { writer.add_existing_entry(entry)?; @@ -722,4 +734,40 @@ mod tests { "the delete-only manifest must survive a later overwrite, entries: {entries:?}" ); } + + #[tokio::test] + async fn test_overwrite_counts_a_duplicated_delete_path_once() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let file_a = test_data_file("test/a.parquet", spec_id); + let file_b = test_data_file("test/b.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx + .fast_append() + .add_data_files(vec![file_a.clone(), file_b.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file("test/c.parquet", spec_id)]) + .delete_data_files(vec![file_a.clone(), file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let summary = table.metadata().current_snapshot().unwrap().summary(); + assert_eq!( + summary.additional_properties.get("deleted-data-files"), + Some(&"1".to_string()) + ); + assert_eq!( + summary.additional_properties.get("deleted-records"), + Some(&"1".to_string()) + ); + assert_eq!( + summary.additional_properties.get("total-data-files"), + Some(&"2".to_string()) + ); + } } diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index aa9cf3818a..1ea66bd202 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -26,9 +26,10 @@ use uuid::Uuid; use crate::error::Result; use crate::spec::{ DataFile, DataFileFormat, FormatVersion, MAIN_BRANCH, ManifestContentType, ManifestEntry, - ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, Snapshot, - SnapshotReference, SnapshotRetention, SnapshotSummaryCollector, Struct, StructType, Summary, - TableProperties, update_snapshot_summaries, + ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, + PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, + SnapshotSummaryCollector, Struct, StructType, Summary, TableProperties, + update_snapshot_summaries, }; use crate::table::Table; use crate::transaction::ActionCommit; @@ -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>> + 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 { + false + } } pub(crate) struct DefaultManifestProcess; @@ -315,7 +331,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 { - 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(); if added_data_files.is_empty() { return Err(Error::new( ErrorKind::PreconditionFailed, @@ -348,7 +366,7 @@ impl<'a> SnapshotProducer<'a> { /// and collects all of the manifests to be included in the new snapshot as [ManifestFile] entries. async fn produce_manifests( &mut self, - snapshot_produce_operation: &OP, + snapshot_produce_operation: &mut OP, manifest_process: &MP, ) -> Result> { // Assert current snapshot producer contains new content to add to new snapshot. @@ -416,12 +434,8 @@ impl<'a> SnapshotProducer<'a> { ); } - for data_file in &self.deleted_data_files { - summary_collector.remove_file( - data_file, - table_metadata.current_schema().clone(), - table_metadata.default_partition_spec().clone(), - ); + for (data_file, schema, partition_spec) in snapshot_produce_operation.removed_data_files() { + summary_collector.remove_file(data_file, schema.clone(), partition_spec.clone()); } let previous_snapshot = table_metadata.current_snapshot(); @@ -442,7 +456,7 @@ impl<'a> SnapshotProducer<'a> { update_snapshot_summaries( summary, previous_snapshot.map(|s| s.summary()), - snapshot_produce_operation.operation() == Operation::Overwrite, + snapshot_produce_operation.truncate_full_table(), ) } @@ -460,7 +474,7 @@ impl<'a> SnapshotProducer<'a> { /// Finished building the action and return the [`ActionCommit`] to the transaction. pub(crate) async fn commit( mut self, - snapshot_produce_operation: OP, + mut snapshot_produce_operation: OP, process: MP, ) -> Result { let manifest_list_path = self.generate_manifest_list_file_path(0)?; @@ -500,17 +514,16 @@ impl<'a> SnapshotProducer<'a> { ), }; - // Calling self.summary() before self.produce_manifests() is important because self.added_data_files - // will be set to an empty vec after self.produce_manifests() returns, resulting in an empty summary - // being generated. + // The manifests are produced first so the summary can count the entries the operation + // actually marked Deleted. + let new_manifests = self + .produce_manifests(&mut snapshot_produce_operation, &process) + .await?; + let summary = self.summary(&snapshot_produce_operation).map_err(|err| { Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err) })?; - let new_manifests = self - .produce_manifests(&snapshot_produce_operation, &process) - .await?; - manifest_list_writer.add_manifests(new_manifests.into_iter())?; let writer_next_row_id = manifest_list_writer.next_row_id(); manifest_list_writer.close().await?; From 7f4f3ed56221ccdd9dc3c2371e1f73a3ed5bcd00 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:41:44 +0200 Subject: [PATCH 11/35] Name a rewritten manifest after the commit uuid The rewrite hardcoded {location}/metadata and a fresh Uuid::now_v7, so a table with a custom write.metadata.path got its rewritten manifests in the wrong prefix, and orphan cleanup could not find them after a failed commit because the name was not tied to the commit. Reuse the path SnapshotProducer already builds from metadata_location(), the commit uuid and the manifest counter; the counter becomes an AtomicU64 because the rewrite runs behind a shared reference. --- crates/iceberg/src/transaction/overwrite.rs | 51 ++++++++++++++++++--- crates/iceberg/src/transaction/snapshot.rs | 20 ++++---- 2 files changed, 57 insertions(+), 14 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 1d57844ebe..49bb9c5bbf 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -220,12 +220,9 @@ impl OverwriteOperation { ) -> Result { let table = snapshot_produce.table; - let new_manifest_path = format!( - "{}/metadata/{}-m-overwrite.avro", - table.metadata().location(), - Uuid::now_v7(), - ); - let output_file = table.file_io().new_output(&new_manifest_path)?; + let output_file = table + .file_io() + .new_output(snapshot_produce.new_manifest_path()?)?; let partition_spec: PartitionSpecRef = Arc::new(manifest.metadata().partition_spec.clone()); let builder = ManifestWriterBuilder::new( output_file, @@ -275,6 +272,8 @@ mod tests { use std::collections::HashMap; use std::sync::Arc; + use uuid::Uuid; + use crate::memory::tests::new_memory_catalog; use crate::spec::{ DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, @@ -770,4 +769,44 @@ mod tests { Some(&"2".to_string()) ); } + + #[tokio::test] + async fn test_rewritten_manifest_is_named_after_the_commit_uuid() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let file_a = test_data_file("test/a.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx.fast_append().add_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let commit_uuid = Uuid::now_v7(); + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .set_commit_uuid(commit_uuid) + .add_data_files(vec![test_data_file("test/b.parquet", spec_id)]) + .delete_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let prefix = format!( + "{}/{}-m", + table.metadata().metadata_location().unwrap(), + commit_uuid + ); + let snapshot = table.metadata().current_snapshot().unwrap(); + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + assert_eq!(2, manifest_list.entries().len()); + for manifest_file in manifest_list.entries() { + let path = &manifest_file.manifest_path; + let counter = path + .strip_prefix(&prefix) + .and_then(|rest| rest.strip_suffix(".avro")); + assert!( + counter.is_some_and(|c| !c.is_empty() && c.chars().all(|c| c.is_ascii_digit())), + "manifest path must be {prefix}.avro, got {path}" + ); + } + } } diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 1ea66bd202..2e4a68f135 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -17,7 +17,7 @@ use std::collections::{HashMap, HashSet}; use std::future::Future; -use std::ops::RangeFrom; +use std::sync::atomic::{AtomicU64, Ordering}; use futures::TryStreamExt; use futures::stream::FuturesUnordered; @@ -133,7 +133,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, + manifest_counter: AtomicU64, } impl<'a> SnapshotProducer<'a> { @@ -155,7 +155,7 @@ impl<'a> SnapshotProducer<'a> { snapshot_properties, added_data_files, deleted_data_files, - manifest_counter: (0..), + manifest_counter: AtomicU64::new(0), } } @@ -257,15 +257,19 @@ impl<'a> SnapshotProducer<'a> { snapshot_id } - fn new_manifest_writer(&mut self, content: ManifestContentType) -> Result { - let new_manifest_path = format!( + /// Returns the path for the next manifest file of this commit. + pub(crate) fn new_manifest_path(&self) -> Result { + 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)?; + )) + } + + fn new_manifest_writer(&mut self, content: ManifestContentType) -> Result { + let output_file = self.table.file_io().new_output(self.new_manifest_path()?)?; let partition_spec = self .table .metadata() From 774f62ab4609e4d5e17921c8fdceb94df94b30d2 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:44:13 +0200 Subject: [PATCH 12/35] Write rewritten manifests through the encryption manager rewrite_manifest built its own plain output file, so on an encrypted table the rewritten manifest went out in plaintext while its key metadata said otherwise. It now goes through SnapshotProducer::new_manifest_writer, which already branches on the table's encryption manager; the writer takes the schema and partition spec as arguments so the rewrite can keep the ones the manifest was written under. That leaves new_manifest_path with no caller outside this module, so it becomes private again. --- crates/iceberg/src/transaction/overwrite.rs | 29 ++++---------------- crates/iceberg/src/transaction/snapshot.rs | 30 +++++++++++++-------- 2 files changed, 24 insertions(+), 35 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 49bb9c5bbf..5eae6914b2 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -23,8 +23,7 @@ use uuid::Uuid; use crate::error::Result; use crate::spec::{ - DataFile, FormatVersion, Manifest, ManifestContentType, ManifestEntry, ManifestFile, - ManifestWriterBuilder, Operation, PartitionSpecRef, SchemaRef, + DataFile, Manifest, ManifestEntry, ManifestFile, Operation, PartitionSpecRef, SchemaRef, }; use crate::table::Table; use crate::transaction::snapshot::{ @@ -218,30 +217,12 @@ impl OverwriteOperation { manifest_file: &ManifestFile, manifest: &Manifest, ) -> Result { - let table = snapshot_produce.table; - - let output_file = table - .file_io() - .new_output(snapshot_produce.new_manifest_path()?)?; let partition_spec: PartitionSpecRef = Arc::new(manifest.metadata().partition_spec.clone()); - let builder = ManifestWriterBuilder::new( - output_file, - Some(self.snapshot_id), + let mut writer = snapshot_produce.new_manifest_writer( + manifest_file.content, manifest.metadata().schema.clone(), - manifest.metadata().partition_spec.clone(), - ); - - let mut writer = match table.metadata().format_version() { - FormatVersion::V1 => builder.build_v1(), - FormatVersion::V2 => match manifest_file.content { - ManifestContentType::Data => builder.build_v2_data(), - ManifestContentType::Deletes => builder.build_v2_deletes(), - }, - FormatVersion::V3 => match manifest_file.content { - ManifestContentType::Data => builder.build_v3_data(), - ManifestContentType::Deletes => builder.build_v3_deletes(), - }, - }; + partition_spec.as_ref().clone(), + )?; for entry in manifest.entries() { let entry: ManifestEntry = (**entry).clone(); diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 2e4a68f135..7f50cd9ac5 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -27,7 +27,7 @@ use crate::error::Result; use crate::spec::{ DataFile, DataFileFormat, FormatVersion, MAIN_BRANCH, ManifestContentType, ManifestEntry, ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, - PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, + PartitionSpec, PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, SnapshotSummaryCollector, Struct, StructType, Summary, TableProperties, update_snapshot_summaries, }; @@ -258,7 +258,7 @@ impl<'a> SnapshotProducer<'a> { } /// Returns the path for the next manifest file of this commit. - pub(crate) fn new_manifest_path(&self) -> Result { + fn new_manifest_path(&self) -> Result { Ok(format!( "{}/{}-m{}.{}", self.table.metadata().metadata_location()?, @@ -268,15 +268,15 @@ impl<'a> SnapshotProducer<'a> { )) } - fn new_manifest_writer(&mut self, content: ManifestContentType) -> Result { + /// 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, + ) -> Result { let output_file = self.table.file_io().new_output(self.new_manifest_path()?)?; - let partition_spec = self - .table - .metadata() - .default_partition_spec() - .as_ref() - .clone(); - let schema = self.table.metadata().current_schema().clone(); let builder = if let Some(em) = self.table.encryption_manager() { ManifestWriterBuilder::new_from_encrypted( @@ -359,7 +359,15 @@ impl<'a> SnapshotProducer<'a> { builder.build() } }); - let mut writer = self.new_manifest_writer(ManifestContentType::Data)?; + let mut writer = self.new_manifest_writer( + ManifestContentType::Data, + self.table.metadata().current_schema().clone(), + self.table + .metadata() + .default_partition_spec() + .as_ref() + .clone(), + )?; for entry in manifest_entries { writer.add_entry(entry)?; } From bd931fded5e5abf62ffe14d978df122534394060 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:46:16 +0200 Subject: [PATCH 13/35] Rename add_deleted_entry to add_tombstone_entry add_deleted_entry set the status but left snapshot_id to the caller, while add_delete_entry right above it stamps both; the two differed only by tense and a tombstone written with the wrong snapshot_id misattributes the delete, which expire_snapshots then acts on. Taking the reviewer's second option: rename rather than stamp, because carrying an earlier tombstone forward has to keep its original snapshot_id, and document that the caller owns the field. Named add_tombstone_entry rather than something closer to main's add_delete_entry (#2367) precisely so the two cannot be confused. --- crates/iceberg/src/spec/manifest/writer.rs | 9 +++++++-- crates/iceberg/src/transaction/overwrite.rs | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/spec/manifest/writer.rs b/crates/iceberg/src/spec/manifest/writer.rs index e0044945a9..b17dee5617 100644 --- a/crates/iceberg/src/spec/manifest/writer.rs +++ b/crates/iceberg/src/spec/manifest/writer.rs @@ -382,8 +382,13 @@ impl ManifestWriter { Ok(()) } - /// Add a deleted manifest entry, preserving the original sequence numbers. - pub(crate) fn add_deleted_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { + /// Add a manifest entry as a tombstone, preserving its original sequence numbers and + /// snapshot id. + /// + /// The caller owns `snapshot_id`: set it to the deleting snapshot for a file this commit + /// removes, and leave it untouched when carrying an earlier tombstone forward. Use + /// `add_delete_entry` instead to have this manifest's snapshot id stamped on the entry. + pub(crate) fn add_tombstone_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { self.check_data_file(&entry.data_file)?; entry.status = ManifestStatus::Deleted; self.add_entry_inner(entry)?; diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 5eae6914b2..f3a813e65a 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -229,7 +229,7 @@ impl OverwriteOperation { if !entry.is_alive() { // A tombstone keeps its original snapshot id so the delete stays attributed // to the snapshot that made it. - writer.add_deleted_entry(entry)?; + writer.add_tombstone_entry(entry)?; } else if self.deleted_file_paths.contains(entry.file_path()) { let mut deleted = entry; deleted.snapshot_id = Some(self.snapshot_id); @@ -238,7 +238,7 @@ impl OverwriteOperation { manifest.metadata().schema.clone(), partition_spec.clone(), )); - writer.add_deleted_entry(deleted)?; + writer.add_tombstone_entry(deleted)?; } else { writer.add_existing_entry(entry)?; } From abad9a5c62db1d23182a9b8f5ff8bcb9ade02080 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:49:17 +0200 Subject: [PATCH 14/35] Report the operation an overwrite actually performed A delete-only overwrite reported Operation::Overwrite, so tools keying off the summary read it as a replacement. Match Java's BaseOverwriteFiles: DELETE when there are only deletes, APPEND when there are only adds, OVERWRITE when there are both. test_overwrite_basic adds without deleting, so its expected operation becomes APPEND. Also names deleted data files in the precondition error, which has allowed a delete-only commit since the guard was relaxed. --- crates/iceberg/src/transaction/overwrite.rs | 52 ++++++++++++++++++++- crates/iceberg/src/transaction/snapshot.rs | 2 +- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index f3a813e65a..84612142ad 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -114,6 +114,7 @@ impl TransactionAction for OverwriteAction { .commit( OverwriteOperation { deleted_file_paths, + has_added_data_files: !self.added_data_files.is_empty(), snapshot_id, removed_data_files: vec![], }, @@ -125,13 +126,21 @@ impl TransactionAction for OverwriteAction { struct OverwriteOperation { deleted_file_paths: HashSet, + has_added_data_files: bool, snapshot_id: i64, removed_data_files: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, } impl SnapshotProduceOperation for OverwriteOperation { fn operation(&self) -> Operation { - Operation::Overwrite + match ( + self.has_added_data_files, + !self.deleted_file_paths.is_empty(), + ) { + (true, true) => Operation::Overwrite, + (false, true) => Operation::Delete, + _ => Operation::Append, + } } async fn delete_entries( @@ -406,7 +415,7 @@ mod tests { } else { unreachable!() }; - assert_eq!(new_snapshot.summary().operation, Operation::Overwrite); + assert_eq!(new_snapshot.summary().operation, Operation::Append); let manifest_list = table .manifest_list_reader(&new_snapshot) @@ -790,4 +799,43 @@ mod tests { ); } } + + #[tokio::test] + async fn test_delete_only_overwrite_reports_a_delete_operation() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let file_a = test_data_file("test/a.parquet", spec_id); + let file_b = test_data_file("test/b.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx + .fast_append() + .add_data_files(vec![file_a.clone(), file_b.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx.overwrite().delete_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let snapshot = table.metadata().current_snapshot().unwrap(); + assert_eq!(snapshot.summary().operation, Operation::Delete); + assert_eq!( + snapshot + .summary() + .additional_properties + .get("deleted-data-files"), + Some(&"1".to_string()) + ); + + let entries = current_manifest_entries(&table).await; + assert!( + entries.contains(&(ManifestStatus::Deleted, "test/a.parquet".to_string())), + "the deleted file must be a tombstone, entries: {entries:?}" + ); + assert!( + entries.contains(&(ManifestStatus::Existing, "test/b.parquet".to_string())), + "the untouched file must stay live, entries: {entries:?}" + ); + } } diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 7f50cd9ac5..93dec9b44a 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -395,7 +395,7 @@ impl<'a> SnapshotProducer<'a> { { return Err(Error::new( ErrorKind::PreconditionFailed, - "No added data files or added snapshot properties found when write a manifest file", + "No added data files, deleted data files, or added snapshot properties found when write a manifest file", )); } From c20752e88cf6468999494485aa689b3834e6da40 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 17:51:46 +0200 Subject: [PATCH 15/35] Pin the precondition the empty overwrite test is about is_err() alone passes on any error, so the test would stay green if the commit started failing earlier for an unrelated reason. Assert the kind and the message instead. --- crates/iceberg/src/transaction/overwrite.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 84612142ad..43d6725726 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -273,7 +273,7 @@ mod tests { use crate::table::Table; use crate::transaction::tests::{make_v2_minimal_table, make_v3_minimal_table_in_catalog}; use crate::transaction::{AddColumn, ApplyTransactionAction, Transaction, TransactionAction}; - use crate::{TableRequirement, TableUpdate}; + use crate::{ErrorKind, TableRequirement, TableUpdate}; fn test_data_file(path: &str, partition_spec_id: i32) -> DataFile { DataFileBuilder::default() @@ -320,7 +320,13 @@ mod tests { let table = make_v2_minimal_table(); let tx = Transaction::new(&table); let action = tx.overwrite().add_data_files(vec![]); - assert!(Arc::new(action).commit(&table).await.is_err()); + let err = Arc::new(action).commit(&table).await.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::PreconditionFailed); + assert!( + err.to_string() + .contains("No added data files, deleted data files, or added snapshot properties"), + "{err}" + ); } #[tokio::test] From 0369a6208ea4e56fe3f42d7c7bc07f7e58c877db Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 18:25:06 +0200 Subject: [PATCH 16/35] Reject deletes of files the table does not have An overwrite silently ignored a delete path that was not live in any manifest, so a typo or an already-deleted file committed as a no-op delete. Java's BaseOverwriteFiles validates the same thing and fails with "Missing required files to delete"; the rewrite pass already records what it marked Deleted, so the check is a comparison against the requested set. --- crates/iceberg/src/transaction/overwrite.rs | 58 +++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 43d6725726..28ff313d84 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -30,6 +30,7 @@ use crate::transaction::snapshot::{ DefaultManifestProcess, SnapshotProduceOperation, SnapshotProducer, }; use crate::transaction::{ActionCommit, TransactionAction}; +use crate::{Error, ErrorKind}; /// OverwriteAction is a transaction action for overwriting data files in the table. /// @@ -159,6 +160,7 @@ impl SnapshotProduceOperation for OverwriteOperation { snapshot_produce: &SnapshotProducer<'_>, ) -> Result> { let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { + self.validate_deletes_matched()?; return Ok(vec![]); }; @@ -213,11 +215,39 @@ impl SnapshotProduceOperation for OverwriteOperation { } } + self.validate_deletes_matched()?; + Ok(result) } } impl OverwriteOperation { + /// Fails when a requested delete path was not live in any manifest, matching the existence + /// check Java's `BaseOverwriteFiles` performs before committing. + fn validate_deletes_matched(&self) -> Result<()> { + let removed: HashSet<&str> = self + .removed_data_files + .iter() + .map(|(data_file, _, _)| data_file.file_path.as_str()) + .collect(); + let mut missing: Vec<&str> = self + .deleted_file_paths + .iter() + .map(String::as_str) + .filter(|path| !removed.contains(path)) + .collect(); + + if missing.is_empty() { + return Ok(()); + } + + missing.sort_unstable(); + Err(Error::new( + ErrorKind::DataInvalid, + format!("Missing required files to delete: {}", missing.join(", ")), + )) + } + /// Rewrite a manifest, marking entries whose file paths are in `deleted_file_paths` /// as `ManifestStatus::Deleted`. async fn rewrite_manifest( @@ -844,4 +874,32 @@ mod tests { "the untouched file must stay live, entries: {entries:?}" ); } + + #[tokio::test] + async fn test_overwrite_rejects_deleting_a_file_the_table_does_not_have() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + + let file_a = test_data_file("test/a.parquet", spec_id); + let tx = Transaction::new(&table); + let action = tx.fast_append().add_data_files(vec![file_a.clone()]); + let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); + + let tx = Transaction::new(&table); + let action = tx + .overwrite() + .add_data_files(vec![test_data_file("test/b.parquet", spec_id)]) + .delete_data_files(vec![ + file_a.clone(), + test_data_file("test/never-committed.parquet", spec_id), + ]); + let err = Arc::new(action).commit(&table).await.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::DataInvalid); + assert!( + err.to_string() + .contains("Missing required files to delete: test/never-committed.parquet"), + "{err}" + ); + } } From 807a26897afbbbd5dd01558c74e1b7e786857fd3 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 18:09:24 +0200 Subject: [PATCH 17/35] Refresh the public API snapshot for OverwriteAction Regenerated with make generate-public-api (cargo-public-api 0.51.0); the snapshot was missing OverwriteAction and its builder methods. --- crates/iceberg/public-api.txt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index 6786a80001..31cec20ec8 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -3251,6 +3251,13 @@ pub fn iceberg::transaction::FastAppendAction::add_data_files(self, data_files: pub fn iceberg::transaction::FastAppendAction::set_commit_uuid(self, commit_uuid: uuid::Uuid) -> Self pub fn iceberg::transaction::FastAppendAction::set_snapshot_properties(self, snapshot_properties: std::collections::hash::map::HashMap) -> Self pub fn iceberg::transaction::FastAppendAction::with_check_duplicate(self, v: bool) -> Self +pub struct iceberg::transaction::OverwriteAction +impl iceberg::transaction::OverwriteAction +pub fn iceberg::transaction::OverwriteAction::add_data_files(self, data_files: impl core::iter::traits::collect::IntoIterator) -> Self +pub fn iceberg::transaction::OverwriteAction::delete_data_files(self, data_files: impl core::iter::traits::collect::IntoIterator) -> Self +pub fn iceberg::transaction::OverwriteAction::set_commit_uuid(self, commit_uuid: uuid::Uuid) -> Self +pub fn iceberg::transaction::OverwriteAction::set_snapshot_properties(self, snapshot_properties: std::collections::hash::map::HashMap) -> Self +pub fn iceberg::transaction::OverwriteAction::with_check_duplicate(self, v: bool) -> Self pub struct iceberg::transaction::ReplaceSortOrderAction impl iceberg::transaction::ReplaceSortOrderAction pub fn iceberg::transaction::ReplaceSortOrderAction::asc(self, name: &str, null_order: iceberg::spec::NullOrder) -> Self From fed895fde70456a11c4b17c1ce244ab79d215326 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 19:38:03 +0200 Subject: [PATCH 18/35] Let a snapshot operation report the files it removed The snapshot summary could only count what the caller asked to remove, so an operation that skips a path it never had, or is handed the same path twice, inflated deleted-data-files and deleted-records. SnapshotProduceOperation now reports the entries it actually marked Deleted, each with the schema and partition spec of the manifest that recorded it, and the summary counts those. Two changes follow from it. The manifests are produced before the summary, since the counts are only known once the operation has run, which means the added data files are cloned rather than taken. And the full-table truncate that drops the previous totals is gated on a new operation hook instead of on the operation being Overwrite: a partial overwrite keeps the totals, and with truncate off update_totals needs a saturating subtraction so removing more than the previous total does not underflow. --- crates/iceberg/src/spec/snapshot_summary.rs | 40 +++++- crates/iceberg/src/transaction/append.rs | 2 +- crates/iceberg/src/transaction/snapshot.rs | 135 +++++++++++++++++--- 3 files changed, 160 insertions(+), 17 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot_summary.rs b/crates/iceberg/src/spec/snapshot_summary.rs index c9fd022724..105952957f 100644 --- a/crates/iceberg/src/spec/snapshot_summary.rs +++ b/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); summary .additional_properties .insert(total_property.to_string(), new_total.to_string()); @@ -1274,4 +1274,42 @@ mod tests { assert_eq!(props.get(TOTAL_POSITION_DELETES).unwrap(), "2"); assert_eq!(props.get(TOTAL_EQUALITY_DELETES).unwrap(), "1"); } + + #[test] + fn test_update_totals_saturates_when_removed_exceeds_previous_total() { + let prev_props: HashMap = [ + (TOTAL_DATA_FILES, "2"), + (TOTAL_RECORDS, "10"), + (TOTAL_FILE_SIZE, "100"), + ] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + let previous_summary = Summary { + operation: Operation::Append, + additional_properties: prev_props, + }; + + let new_props: HashMap = [ + (DELETED_DATA_FILES, "3"), + (DELETED_RECORDS, "25"), + (REMOVED_FILE_SIZE, "250"), + ] + .into_iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(); + + let summary = Summary { + operation: Operation::Delete, + additional_properties: new_props, + }; + + let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); + let props = &updated.additional_properties; + + assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "0"); + assert_eq!(props.get(TOTAL_RECORDS).unwrap(), "0"); + assert_eq!(props.get(TOTAL_FILE_SIZE).unwrap(), "0"); + } } diff --git a/crates/iceberg/src/transaction/append.rs b/crates/iceberg/src/transaction/append.rs index 872a1d89c1..3533c62d27 100644 --- a/crates/iceberg/src/transaction/append.rs +++ b/crates/iceberg/src/transaction/append.rs @@ -124,7 +124,7 @@ impl SnapshotProduceOperation for FastAppendOperation { } async fn existing_manifest( - &self, + &mut self, snapshot_produce: &SnapshotProducer<'_>, ) -> Result> { let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 1f09a79a08..62ef9c6d60 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -26,9 +26,10 @@ use uuid::Uuid; use crate::error::Result; use crate::spec::{ DataFile, DataFileFormat, FormatVersion, MAIN_BRANCH, ManifestContentType, ManifestEntry, - ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, Snapshot, - SnapshotReference, SnapshotRetention, SnapshotSummaryCollector, Struct, StructType, Summary, - TableProperties, update_snapshot_summaries, + ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, + PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, + SnapshotSummaryCollector, Struct, StructType, Summary, TableProperties, + update_snapshot_summaries, }; use crate::table::Table; use crate::transaction::ActionCommit; @@ -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>> + 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 { + false + } } pub(crate) struct DefaultManifestProcess; @@ -308,7 +324,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 { - 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(); if added_data_files.is_empty() { return Err(Error::new( ErrorKind::PreconditionFailed, @@ -341,7 +359,7 @@ impl<'a> SnapshotProducer<'a> { /// and collects all of the manifests to be included in the new snapshot as [ManifestFile] entries. async fn produce_manifests( &mut self, - snapshot_produce_operation: &OP, + snapshot_produce_operation: &mut OP, manifest_process: &MP, ) -> Result> { // Assert current snapshot producer contains new content to add to new snapshot. @@ -403,6 +421,10 @@ impl<'a> SnapshotProducer<'a> { ); } + for (data_file, schema, partition_spec) in snapshot_produce_operation.removed_data_files() { + summary_collector.remove_file(data_file, schema.clone(), partition_spec.clone()); + } + let previous_snapshot = table_metadata.current_snapshot(); // User-supplied snapshot properties are applied first, then the computed @@ -421,7 +443,7 @@ impl<'a> SnapshotProducer<'a> { update_snapshot_summaries( summary, previous_snapshot.map(|s| s.summary()), - snapshot_produce_operation.operation() == Operation::Overwrite, + snapshot_produce_operation.truncate_full_table(), ) } @@ -439,7 +461,7 @@ impl<'a> SnapshotProducer<'a> { /// Finished building the action and return the [`ActionCommit`] to the transaction. pub(crate) async fn commit( mut self, - snapshot_produce_operation: OP, + mut snapshot_produce_operation: OP, process: MP, ) -> Result { let manifest_list_path = self.generate_manifest_list_file_path(0)?; @@ -479,17 +501,16 @@ impl<'a> SnapshotProducer<'a> { ), }; - // Calling self.summary() before self.produce_manifests() is important because self.added_data_files - // will be set to an empty vec after self.produce_manifests() returns, resulting in an empty summary - // being generated. + // The manifests are produced first so the summary can count the entries the operation + // actually marked Deleted. + let new_manifests = self + .produce_manifests(&mut snapshot_produce_operation, &process) + .await?; + let summary = self.summary(&snapshot_produce_operation).map_err(|err| { Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err) })?; - let new_manifests = self - .produce_manifests(&snapshot_produce_operation, &process) - .await?; - manifest_list_writer.add_manifests(new_manifests.into_iter())?; let writer_next_row_id = manifest_list_writer.next_row_id(); manifest_list_writer.close().await?; @@ -556,3 +577,87 @@ impl<'a> SnapshotProducer<'a> { Ok(ActionCommit::new(updates, requirements)) } } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use uuid::Uuid; + + use super::*; + use crate::spec::{DataContentType, DataFileBuilder, Literal}; + use crate::transaction::tests::make_v2_minimal_table; + + /// An operation that adds no existing manifests and reports one removed data file. + struct RemoveOneFileOperation { + removed: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, + } + + impl SnapshotProduceOperation for RemoveOneFileOperation { + fn operation(&self) -> Operation { + Operation::Delete + } + + async fn delete_entries(&self, _: &SnapshotProducer<'_>) -> Result> { + Ok(vec![]) + } + + async fn existing_manifest( + &mut self, + _: &SnapshotProducer<'_>, + ) -> Result> { + Ok(vec![]) + } + + fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)] { + &self.removed + } + } + + fn data_file(table: &Table, path: &str, record_count: u64) -> DataFile { + DataFileBuilder::default() + .content(DataContentType::Data) + .file_path(path.to_string()) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(record_count) + .partition_spec_id(table.metadata().default_partition_spec_id()) + .partition(Struct::from_iter([Some(Literal::long(300))])) + .build() + .unwrap() + } + + #[tokio::test] + async fn test_summary_counts_files_the_operation_reports_as_removed() { + let table = make_v2_minimal_table(); + let added = data_file(&table, "test/added.parquet", 3); + let removed = data_file(&table, "test/removed.parquet", 2); + + let operation = RemoveOneFileOperation { + removed: vec![( + removed, + table.metadata().current_schema().clone(), + table.metadata().default_partition_spec().clone(), + )], + }; + + let mut action_commit = + SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![added]) + .commit(operation, DefaultManifestProcess) + .await + .unwrap(); + + let updates = action_commit.take_updates(); + let TableUpdate::AddSnapshot { snapshot } = &updates[0] else { + unreachable!() + }; + let properties = &snapshot.summary().additional_properties; + + assert_eq!(properties.get("deleted-data-files").unwrap(), "1"); + assert_eq!(properties.get("deleted-records").unwrap(), "2"); + assert_eq!(properties.get("added-data-files").unwrap(), "1"); + assert_eq!(properties.get("added-records").unwrap(), "3"); + assert_eq!(properties.get("total-data-files").unwrap(), "0"); + assert_eq!(properties.get("total-records").unwrap(), "1"); + } +} From 3fc4e80621568c9f5601cb9e7b7003fe6d2eb053 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 19:38:47 +0200 Subject: [PATCH 19/35] Keep manifest names unique across a commit The manifest name is built inline in new_manifest_writer from the metadata location, the commit uuid and a counter, so anything else that needs to write a manifest for the same commit has to reinvent it, and a name invented elsewhere is neither unique within the commit nor findable by orphan cleanup after a failed one. Pull the name out into new_manifest_path, and make the counter an AtomicU64 so it can be bumped behind a shared reference. --- crates/iceberg/src/transaction/snapshot.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 62ef9c6d60..0347257290 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -17,7 +17,7 @@ use std::collections::{HashMap, HashSet}; use std::future::Future; -use std::ops::RangeFrom; +use std::sync::atomic::{AtomicU64, Ordering}; use futures::TryStreamExt; use futures::stream::FuturesUnordered; @@ -132,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, + manifest_counter: AtomicU64, } impl<'a> SnapshotProducer<'a> { @@ -148,7 +148,7 @@ impl<'a> SnapshotProducer<'a> { commit_uuid, snapshot_properties, added_data_files, - manifest_counter: (0..), + manifest_counter: AtomicU64::new(0), } } @@ -250,15 +250,19 @@ impl<'a> SnapshotProducer<'a> { snapshot_id } - fn new_manifest_writer(&mut self, content: ManifestContentType) -> Result { - let new_manifest_path = format!( + /// Returns the path for the next manifest file of this commit. + fn new_manifest_path(&self) -> Result { + 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)?; + )) + } + + fn new_manifest_writer(&mut self, content: ManifestContentType) -> Result { + let output_file = self.table.file_io().new_output(self.new_manifest_path()?)?; let partition_spec = self .table .metadata() From 6325d2270424c92fb2f9734721de4ff91708c86e Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 18 Sep 2026 19:41:28 +0200 Subject: [PATCH 20/35] Write a manifest under the schema and spec it was written with new_manifest_writer hardcoded the table's current schema and default partition spec, so an operation that rewrites an existing manifest could not keep the schema and spec that manifest's entries were recorded under, and building a writer by hand instead means losing the encryption manager the method already branches on: on an encrypted table the manifest would go out in plaintext while its key metadata said otherwise. The schema and partition spec become arguments, and the added-files path passes the table's current ones. --- crates/iceberg/src/transaction/snapshot.rs | 89 +++++++++++++++++++--- 1 file changed, 78 insertions(+), 11 deletions(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 0347257290..1770e12f6c 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -27,7 +27,7 @@ use crate::error::Result; use crate::spec::{ DataFile, DataFileFormat, FormatVersion, MAIN_BRANCH, ManifestContentType, ManifestEntry, ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, - PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, + PartitionSpec, PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, SnapshotSummaryCollector, Struct, StructType, Summary, TableProperties, update_snapshot_summaries, }; @@ -261,15 +261,15 @@ impl<'a> SnapshotProducer<'a> { )) } - fn new_manifest_writer(&mut self, content: ManifestContentType) -> Result { + /// 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, + ) -> Result { let output_file = self.table.file_io().new_output(self.new_manifest_path()?)?; - let partition_spec = self - .table - .metadata() - .default_partition_spec() - .as_ref() - .clone(); - let schema = self.table.metadata().current_schema().clone(); let builder = if let Some(em) = self.table.encryption_manager() { ManifestWriterBuilder::new_from_encrypted( @@ -352,7 +352,15 @@ impl<'a> SnapshotProducer<'a> { builder.build() } }); - let mut writer = self.new_manifest_writer(ManifestContentType::Data)?; + let mut writer = self.new_manifest_writer( + ManifestContentType::Data, + self.table.metadata().current_schema().clone(), + self.table + .metadata() + .default_partition_spec() + .as_ref() + .clone(), + )?; for entry in manifest_entries { writer.add_entry(entry)?; } @@ -585,11 +593,15 @@ impl<'a> SnapshotProducer<'a> { #[cfg(test)] mod tests { use std::collections::HashMap; + use std::sync::Arc; use uuid::Uuid; use super::*; - use crate::spec::{DataContentType, DataFileBuilder, Literal}; + use crate::spec::{ + DataContentType, DataFileBuilder, Literal, Manifest, NestedField, PartitionSpec, + PrimitiveType, Schema, Transform, Type, + }; use crate::transaction::tests::make_v2_minimal_table; /// An operation that adds no existing manifests and reports one removed data file. @@ -664,4 +676,59 @@ mod tests { assert_eq!(properties.get("total-data-files").unwrap(), "0"); assert_eq!(properties.get("total-records").unwrap(), "1"); } + + #[tokio::test] + async fn test_new_manifest_writer_uses_the_given_schema_and_spec() { + let table = make_v2_minimal_table(); + let schema = Arc::new( + Schema::builder() + .with_schema_id(7) + .with_fields(vec![ + NestedField::required(1, "a", Type::Primitive(PrimitiveType::Long)).into(), + ]) + .build() + .unwrap(), + ); + let partition_spec = PartitionSpec::builder(schema.clone()) + .with_spec_id(3) + .add_partition_field("a", "a", Transform::Identity) + .unwrap() + .build() + .unwrap(); + + let producer = SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![]); + let first = producer + .new_manifest_writer( + ManifestContentType::Data, + schema.clone(), + partition_spec.clone(), + ) + .unwrap() + .write_manifest_file() + .await + .unwrap(); + let second = producer + .new_manifest_writer(ManifestContentType::Data, schema.clone(), partition_spec) + .unwrap() + .write_manifest_file() + .await + .unwrap(); + + assert_ne!(first.manifest_path, second.manifest_path); + + let bytes = table + .file_io() + .new_input(&first.manifest_path) + .unwrap() + .read() + .await + .unwrap(); + let metadata = Manifest::parse_avro(&bytes).unwrap().into_parts().1; + + // Not the table's defaults, which are schema 0 and spec 0. + assert_eq!(metadata.schema_id(), 7); + assert_eq!(metadata.schema().as_ref(), schema.as_ref()); + assert_eq!(metadata.partition_spec().spec_id(), 3); + assert_eq!(first.partition_spec_id, 3); + } } From 1cad7313308ad4bc7168d4111b12aa921a6379fc Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Tue, 29 Sep 2026 21:55:37 +0200 Subject: [PATCH 21/35] test(transaction): cover partial overwrite and per-file spec in snapshot summary Co-authored-by: Marco Kroll <216147982+mkroll-db@users.noreply.github.com> --- crates/iceberg/src/transaction/snapshot.rs | 200 ++++++++++++++++++++- 1 file changed, 198 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 1770e12f6c..c1b4feb40b 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -599,10 +599,11 @@ mod tests { use super::*; use crate::spec::{ - DataContentType, DataFileBuilder, Literal, Manifest, NestedField, PartitionSpec, - PrimitiveType, Schema, Transform, Type, + DataContentType, DataFileBuilder, Literal, Manifest, ManifestStatus, NestedField, + PartitionSpec, PrimitiveType, Schema, Transform, Type, }; use crate::transaction::tests::make_v2_minimal_table; + use crate::transaction::{Transaction, TransactionAction}; /// An operation that adds no existing manifests and reports one removed data file. struct RemoveOneFileOperation { @@ -630,6 +631,75 @@ mod tests { } } + struct RemoveExistingFileOperation { + file_path: String, + removed: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, + } + + impl SnapshotProduceOperation for RemoveExistingFileOperation { + fn operation(&self) -> Operation { + Operation::Overwrite + } + + async fn delete_entries(&self, _: &SnapshotProducer<'_>) -> Result> { + Ok(vec![]) + } + + async fn existing_manifest( + &mut self, + producer: &SnapshotProducer<'_>, + ) -> Result> { + let snapshot = producer.table.metadata().current_snapshot().unwrap(); + let manifest_list = producer.table.manifest_list_reader(snapshot).load().await?; + // The initial append writes both files into one manifest. + assert_eq!(manifest_list.entries().len(), 1); + + let manifest = producer + .table + .manifest_reader() + .read(&manifest_list.entries()[0]) + .await?; + let schema = manifest.metadata().schema().clone(); + let partition_spec = manifest.metadata().partition_spec().clone(); + let mut writer = producer.new_manifest_writer( + ManifestContentType::Data, + schema.clone(), + partition_spec.clone(), + )?; + + let mut removed = Vec::new(); + for entry in manifest.entries() { + if entry.file_path() == self.file_path && entry.is_alive() { + writer.add_delete_entry(entry.as_ref().clone())?; + removed.push(( + entry.data_file().clone(), + schema.clone(), + Arc::new(partition_spec.clone()), + )); + } else if entry.is_alive() { + writer.add_existing_entry(entry.as_ref().clone())?; + } + } + assert_eq!(removed.len(), 1); + + let rewritten_manifest = writer.write_manifest_file().await?; + self.removed.extend(removed); + Ok(vec![rewritten_manifest]) + } + + fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)] { + &self.removed + } + } + + fn apply_commit(table: Table, mut commit: ActionCommit) -> Table { + let mut builder = table.metadata().clone().into_builder(None); + for update in commit.take_updates() { + builder = update.apply(builder).unwrap(); + } + table.with_metadata(Arc::new(builder.build().unwrap().metadata)) + } + fn data_file(table: &Table, path: &str, record_count: u64) -> DataFile { DataFileBuilder::default() .content(DataContentType::Data) @@ -677,6 +747,80 @@ mod tests { assert_eq!(properties.get("total-records").unwrap(), "1"); } + #[tokio::test] + async fn test_partial_overwrite_reports_actual_removed_file() { + let table = make_v2_minimal_table(); + let removed = data_file(&table, "test/removed.parquet", 2); + let kept = data_file(&table, "test/kept.parquet", 5); + let append = Transaction::new(&table) + .fast_append() + .add_data_files(vec![removed.clone(), kept]); + let initial_commit = Arc::new(append).commit(&table).await.unwrap(); + let table = apply_commit(table, initial_commit); + + let initial_summary = &table + .metadata() + .current_snapshot() + .unwrap() + .summary() + .additional_properties; + assert_eq!(initial_summary.get("total-data-files").unwrap(), "2"); + assert_eq!(initial_summary.get("total-records").unwrap(), "7"); + + let mut replacement = data_file(&table, "test/replacement.parquet", 3); + replacement.file_size_in_bytes = 300; + let operation = RemoveExistingFileOperation { + file_path: removed.file_path.clone(), + removed: vec![], + }; + let commit = + SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![replacement]) + .commit(operation, DefaultManifestProcess) + .await + .unwrap(); + let table = apply_commit(table, commit); + let snapshot = table.metadata().current_snapshot().unwrap(); + let properties = &snapshot.summary().additional_properties; + + assert_eq!(snapshot.summary().operation, Operation::Overwrite); + assert_eq!(properties.get("deleted-data-files").unwrap(), "1"); + assert_eq!(properties.get("deleted-records").unwrap(), "2"); + assert_eq!(properties.get("added-data-files").unwrap(), "1"); + assert_eq!(properties.get("added-records").unwrap(), "3"); + assert_eq!(properties.get("removed-files-size").unwrap(), "100"); + assert_eq!(properties.get("added-files-size").unwrap(), "300"); + assert_eq!(properties.get("total-data-files").unwrap(), "2"); + assert_eq!(properties.get("total-records").unwrap(), "8"); + assert_eq!(properties.get("total-files-size").unwrap(), "400"); + + let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); + assert_eq!(manifest_list.entries().len(), 2); + let mut statuses = HashMap::new(); + for manifest_file in manifest_list.entries() { + let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); + for entry in manifest.entries() { + assert!( + statuses + .insert(entry.file_path().to_string(), entry.status()) + .is_none() + ); + } + } + assert_eq!(statuses.len(), 3); + assert_eq!( + statuses.get("test/removed.parquet"), + Some(&ManifestStatus::Deleted) + ); + assert_eq!( + statuses.get("test/kept.parquet"), + Some(&ManifestStatus::Existing) + ); + assert_eq!( + statuses.get("test/replacement.parquet"), + Some(&ManifestStatus::Added) + ); + } + #[tokio::test] async fn test_new_manifest_writer_uses_the_given_schema_and_spec() { let table = make_v2_minimal_table(); @@ -731,4 +875,56 @@ mod tests { assert_eq!(metadata.partition_spec().spec_id(), 3); assert_eq!(first.partition_spec_id, 3); } + + #[tokio::test] + async fn test_removed_file_summary_uses_the_reported_partition_spec() { + let table = make_v2_minimal_table(); + let metadata = table + .metadata() + .clone() + .into_builder(None) + .set_properties(HashMap::from([( + TableProperties::PROPERTY_WRITE_PARTITION_SUMMARY_LIMIT.to_string(), + "10".to_string(), + )])) + .unwrap() + .build() + .unwrap() + .metadata; + let table = table.with_metadata(Arc::new(metadata)); + + let reported_schema = table.metadata().current_schema().clone(); + let reported_spec = Arc::new( + PartitionSpec::builder(reported_schema.clone()) + .with_spec_id(1) + .add_partition_field("y", "y", Transform::Identity) + .unwrap() + .build() + .unwrap(), + ); + + let added = data_file(&table, "test/added.parquet", 3); + let removed = data_file(&table, "test/removed.parquet", 2); + let operation = RemoveOneFileOperation { + removed: vec![(removed, reported_schema, reported_spec)], + }; + + let mut action_commit = + SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![added]) + .commit(operation, DefaultManifestProcess) + .await + .unwrap(); + + let updates = action_commit.take_updates(); + let TableUpdate::AddSnapshot { snapshot } = &updates[0] else { + unreachable!() + }; + let properties = &snapshot.summary().additional_properties; + + let removed_partition = properties + .get("partitions.y=300") + .expect("removal must be summarized under the reported spec's partition path"); + assert!(removed_partition.contains("deleted-data-files=1")); + assert!(properties.contains_key("partitions.x=300")); + } } From 7320fa4a03268b414ed8ab2fe3322afb4e8f9295 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Tue, 29 Sep 2026 21:57:29 +0200 Subject: [PATCH 22/35] Adapt the evolved-spec test to the UnboundPartitionField builder --- crates/iceberg/src/transaction/overwrite.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 28ff313d84..8597c65a7a 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -298,7 +298,7 @@ mod tests { use crate::spec::{ DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, ManifestEntryRef, ManifestStatus, Operation, PrimitiveType, SnapshotRef, Struct, Transform, - Type, UnboundPartitionSpec, + Type, UnboundPartitionField, UnboundPartitionSpec, }; use crate::table::Table; use crate::transaction::tests::{make_v2_minimal_table, make_v3_minimal_table_in_catalog}; @@ -675,7 +675,14 @@ mod tests { .into_builder(None) .add_default_partition_spec( UnboundPartitionSpec::builder() - .add_partition_field(2, "y", Transform::Identity) + .add_partition_field( + UnboundPartitionField::builder() + .source_ids(vec![2]) + .name("y") + .transform(Transform::Identity) + .build() + .unwrap(), + ) .unwrap() .build(), ) From d5623e3c29b10db073fe60b48a39da32dd44423b Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:20:33 +0200 Subject: [PATCH 23/35] Drop the full-table truncation from the snapshot summary Java and PyIceberg derive totals from the files an operation marks deleted, with no truncation path. --- crates/iceberg/src/spec/snapshot_summary.rs | 283 +------------------- crates/iceberg/src/transaction/snapshot.rs | 12 +- 2 files changed, 8 insertions(+), 287 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot_summary.rs b/crates/iceberg/src/spec/snapshot_summary.rs index 105952957f..b13b35bf2a 100644 --- a/crates/iceberg/src/spec/snapshot_summary.rs +++ b/crates/iceberg/src/spec/snapshot_summary.rs @@ -330,9 +330,8 @@ where T: PartialOrd + Default + ToString { } pub(crate) fn update_snapshot_summaries( - summary: Summary, + mut summary: Summary, previous_summary: Option<&Summary>, - truncate_full_table: bool, ) -> Result { // Validate that the operation is supported if summary.operation != Operation::Append @@ -345,15 +344,6 @@ pub(crate) fn update_snapshot_summaries( )); } - let mut summary = match previous_summary { - Some(prev_summary) if truncate_full_table && summary.operation == Operation::Overwrite => { - truncate_table_summary(summary, prev_summary).map_err(|err| { - Error::new(ErrorKind::Unexpected, "Failed to truncate table summary.") - .with_source(err) - })? - } - _ => summary, - }; update_totals( &mut summary, @@ -405,77 +395,6 @@ pub(crate) fn update_snapshot_summaries( Ok(summary) } -fn get_prop(previous_summary: &Summary, prop: &str) -> Result { - let value_str = previous_summary - .additional_properties - .get(prop) - .map(String::as_str) - .unwrap_or("0"); - value_str.parse::().map_err(|err| { - Error::new( - ErrorKind::Unexpected, - format!("Failed to parse summary property '{prop}' value '{value_str}' as u64."), - ) - .with_source(err) - }) -} - -fn truncate_table_summary(mut summary: Summary, previous_summary: &Summary) -> Result { - for prop in [ - TOTAL_DATA_FILES, - TOTAL_DELETE_FILES, - TOTAL_RECORDS, - TOTAL_FILE_SIZE, - TOTAL_POSITION_DELETES, - TOTAL_EQUALITY_DELETES, - ] { - summary - .additional_properties - .insert(prop.to_string(), "0".to_string()); - } - - let value = get_prop(previous_summary, TOTAL_DATA_FILES)?; - if value != 0 { - summary - .additional_properties - .insert(DELETED_DATA_FILES.to_string(), value.to_string()); - } - let value = get_prop(previous_summary, TOTAL_DELETE_FILES)?; - if value != 0 { - summary - .additional_properties - .insert(REMOVED_DELETE_FILES.to_string(), value.to_string()); - } - let value = get_prop(previous_summary, TOTAL_RECORDS)?; - if value != 0 { - summary - .additional_properties - .insert(DELETED_RECORDS.to_string(), value.to_string()); - } - let value = get_prop(previous_summary, TOTAL_FILE_SIZE)?; - if value != 0 { - summary - .additional_properties - .insert(REMOVED_FILE_SIZE.to_string(), value.to_string()); - } - - let value = get_prop(previous_summary, TOTAL_POSITION_DELETES)?; - if value != 0 { - summary - .additional_properties - .insert(REMOVED_POSITION_DELETES.to_string(), value.to_string()); - } - - let value = get_prop(previous_summary, TOTAL_EQUALITY_DELETES)?; - if value != 0 { - summary - .additional_properties - .insert(REMOVED_EQUALITY_DELETES.to_string(), value.to_string()); - } - - Ok(summary) -} - fn update_totals( summary: &mut Summary, previous_summary: Option<&Summary>, @@ -589,7 +508,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); assert_eq!( updated.additional_properties.get(TOTAL_DATA_FILES).unwrap(), @@ -626,194 +545,6 @@ mod tests { ); } - #[test] - fn test_truncate_table_summary() { - let prev_props: HashMap = [ - (TOTAL_DATA_FILES.to_string(), "10".to_string()), - (TOTAL_DELETE_FILES.to_string(), "5".to_string()), - (TOTAL_RECORDS.to_string(), "100".to_string()), - (TOTAL_FILE_SIZE.to_string(), "1000".to_string()), - (TOTAL_POSITION_DELETES.to_string(), "3".to_string()), - (TOTAL_EQUALITY_DELETES.to_string(), "2".to_string()), - ] - .into_iter() - .collect(); - - let previous_summary = Summary { - operation: Operation::Overwrite, - additional_properties: prev_props, - }; - - let mut new_props = HashMap::new(); - new_props.insert("dummy".to_string(), "value".to_string()); - let summary = Summary { - operation: Operation::Overwrite, - additional_properties: new_props, - }; - - let truncated = truncate_table_summary(summary, &previous_summary).unwrap(); - - assert_eq!( - truncated - .additional_properties - .get(TOTAL_DATA_FILES) - .unwrap(), - "0" - ); - assert_eq!( - truncated - .additional_properties - .get(TOTAL_DELETE_FILES) - .unwrap(), - "0" - ); - assert_eq!( - truncated.additional_properties.get(TOTAL_RECORDS).unwrap(), - "0" - ); - assert_eq!( - truncated - .additional_properties - .get(TOTAL_FILE_SIZE) - .unwrap(), - "0" - ); - assert_eq!( - truncated - .additional_properties - .get(TOTAL_POSITION_DELETES) - .unwrap(), - "0" - ); - assert_eq!( - truncated - .additional_properties - .get(TOTAL_EQUALITY_DELETES) - .unwrap(), - "0" - ); - - assert_eq!( - truncated - .additional_properties - .get(DELETED_DATA_FILES) - .unwrap(), - "10" - ); - assert_eq!( - truncated - .additional_properties - .get(REMOVED_DELETE_FILES) - .unwrap(), - "5" - ); - assert_eq!( - truncated - .additional_properties - .get(DELETED_RECORDS) - .unwrap(), - "100" - ); - assert_eq!( - truncated - .additional_properties - .get(REMOVED_FILE_SIZE) - .unwrap(), - "1000" - ); - assert_eq!( - truncated - .additional_properties - .get(REMOVED_POSITION_DELETES) - .unwrap(), - "3" - ); - assert_eq!( - truncated - .additional_properties - .get(REMOVED_EQUALITY_DELETES) - .unwrap(), - "2" - ); - } - - #[test] - fn test_update_snapshot_summaries_overwrite_truncate_handles_totals_above_i32_max() { - // A table can legitimately accumulate more than i32::MAX rows or files - // over its lifetime. Truncating such a table on overwrite must succeed - // and surface the previous totals into the deleted-* counters. - let big = (i32::MAX as u64 + 1).to_string(); // 2_147_483_648 - let prev_props: HashMap = [ - (TOTAL_DATA_FILES.to_string(), big.clone()), - (TOTAL_DELETE_FILES.to_string(), "0".to_string()), - (TOTAL_RECORDS.to_string(), big.clone()), - (TOTAL_FILE_SIZE.to_string(), "0".to_string()), - (TOTAL_POSITION_DELETES.to_string(), "0".to_string()), - (TOTAL_EQUALITY_DELETES.to_string(), "0".to_string()), - ] - .into_iter() - .collect(); - - let previous_summary = Summary { - operation: Operation::Overwrite, - additional_properties: prev_props, - }; - - let summary = Summary { - operation: Operation::Overwrite, - additional_properties: HashMap::new(), - }; - - let updated = update_snapshot_summaries(summary, Some(&previous_summary), true) - .expect("overwrite truncation should accept totals above i32::MAX"); - assert_eq!( - updated - .additional_properties - .get(DELETED_DATA_FILES) - .unwrap(), - &big - ); - assert_eq!( - updated.additional_properties.get(DELETED_RECORDS).unwrap(), - &big - ); - } - - #[test] - fn test_update_snapshot_summaries_overwrite_truncate_returns_err_on_malformed_total() { - // Non-numeric values in the previous summary (corruption, manual edits, - // a foreign implementation) must surface as a recoverable Err - not - // crash the process. - let prev_props: HashMap = [ - (TOTAL_DATA_FILES.to_string(), "not_a_number".to_string()), - (TOTAL_DELETE_FILES.to_string(), "0".to_string()), - (TOTAL_RECORDS.to_string(), "0".to_string()), - (TOTAL_FILE_SIZE.to_string(), "0".to_string()), - (TOTAL_POSITION_DELETES.to_string(), "0".to_string()), - (TOTAL_EQUALITY_DELETES.to_string(), "0".to_string()), - ] - .into_iter() - .collect(); - - let previous_summary = Summary { - operation: Operation::Overwrite, - additional_properties: prev_props, - }; - - let summary = Summary { - operation: Operation::Overwrite, - additional_properties: HashMap::new(), - }; - - let err = update_snapshot_summaries(summary, Some(&previous_summary), true) - .expect_err("malformed previous summary must produce an Err, not a panic"); - assert!( - err.message().contains("truncate table summary"), - "expected wrapped 'Failed to truncate table summary' context, got: {}", - err.message() - ); - } - #[test] fn test_snapshot_summary_collector_build() { let schema = Arc::new( @@ -1146,7 +877,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "12"); @@ -1192,7 +923,7 @@ mod tests { }; // Must not panic. - let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); let props = &updated.additional_properties; // The total whose added delta was unparsable is skipped... @@ -1220,7 +951,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, None, false).unwrap(); + let updated = update_snapshot_summaries(summary, None).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "4"); @@ -1264,7 +995,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "8"); @@ -1305,7 +1036,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "0"); diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index c1b4feb40b..3aeb2af39a 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -95,12 +95,6 @@ pub(crate) trait SnapshotProduceOperation: Send + Sync { 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 { - false - } } pub(crate) struct DefaultManifestProcess; @@ -452,11 +446,7 @@ impl<'a> SnapshotProducer<'a> { additional_properties, }; - update_snapshot_summaries( - summary, - previous_snapshot.map(|s| s.summary()), - snapshot_produce_operation.truncate_full_table(), - ) + update_snapshot_summaries(summary, previous_snapshot.map(|s| s.summary())) } fn generate_manifest_list_file_path(&self, attempt: i64) -> Result { From 296532e6ba26caf89334ce1bcbacd0872674abc3 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:20:41 +0200 Subject: [PATCH 24/35] Omit a summary total that would go negative, as Java does --- crates/iceberg/src/spec/snapshot_summary.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot_summary.rs b/crates/iceberg/src/spec/snapshot_summary.rs index b13b35bf2a..89af2081ba 100644 --- a/crates/iceberg/src/spec/snapshot_summary.rs +++ b/crates/iceberg/src/spec/snapshot_summary.rs @@ -451,7 +451,10 @@ fn update_totals( return; }; - let new_total = (previous_total + added).saturating_sub(removed); + // Like Java, a total that would go negative is omitted rather than written as a false basis. + let Some(new_total) = previous_total.saturating_add(added).checked_sub(removed) else { + return; + }; summary .additional_properties .insert(total_property.to_string(), new_total.to_string()); @@ -1007,7 +1010,7 @@ mod tests { } #[test] - fn test_update_totals_saturates_when_removed_exceeds_previous_total() { + fn test_update_totals_omits_total_when_removed_exceeds_previous_total() { let prev_props: HashMap = [ (TOTAL_DATA_FILES, "2"), (TOTAL_RECORDS, "10"), @@ -1039,8 +1042,8 @@ mod tests { let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); let props = &updated.additional_properties; - assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "0"); - assert_eq!(props.get(TOTAL_RECORDS).unwrap(), "0"); - assert_eq!(props.get(TOTAL_FILE_SIZE).unwrap(), "0"); + assert!(!props.contains_key(TOTAL_DATA_FILES)); + assert!(!props.contains_key(TOTAL_RECORDS)); + assert!(!props.contains_key(TOTAL_FILE_SIZE)); } } From eeff802555161e76e8a8bfb7b2027307f3d005a1 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:21:11 +0200 Subject: [PATCH 25/35] Name the removed-file fields and take the manifest spec by ref --- crates/iceberg/src/transaction/snapshot.rs | 82 +++++++++++++--------- 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 3aeb2af39a..43345fdeb2 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -27,7 +27,7 @@ use crate::error::Result; use crate::spec::{ DataFile, DataFileFormat, FormatVersion, MAIN_BRANCH, ManifestContentType, ManifestEntry, ManifestFile, ManifestListWriter, ManifestWriter, ManifestWriterBuilder, Operation, - PartitionSpec, PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, + PartitionSpecRef, SchemaRef, Snapshot, SnapshotReference, SnapshotRetention, SnapshotSummaryCollector, Struct, StructType, Summary, TableProperties, update_snapshot_summaries, }; @@ -87,16 +87,23 @@ pub(crate) trait SnapshotProduceOperation: Send + Sync { snapshot_produce: &SnapshotProducer<'_>, ) -> impl Future>> + Send; - /// Returns the data files this operation actually removed, each with the schema and - /// partition spec of the manifest that recorded it. + /// Returns the data files this operation actually removed. /// /// 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)] { + fn removed_data_files(&self) -> &[RemovedDataFile] { &[] } } +/// A data file an operation removed, with the schema and partition spec of the manifest that +/// recorded it. +pub(crate) struct RemovedDataFile { + pub(crate) data_file: DataFile, + pub(crate) schema: SchemaRef, + pub(crate) partition_spec: PartitionSpecRef, +} + pub(crate) struct DefaultManifestProcess; impl ManifestProcess for DefaultManifestProcess { @@ -261,9 +268,10 @@ impl<'a> SnapshotProducer<'a> { &self, content: ManifestContentType, schema: SchemaRef, - partition_spec: PartitionSpec, + partition_spec: PartitionSpecRef, ) -> Result { let output_file = self.table.file_io().new_output(self.new_manifest_path()?)?; + let partition_spec = partition_spec.as_ref().clone(); let builder = if let Some(em) = self.table.encryption_manager() { ManifestWriterBuilder::new_from_encrypted( @@ -349,11 +357,7 @@ impl<'a> SnapshotProducer<'a> { let mut writer = self.new_manifest_writer( ManifestContentType::Data, self.table.metadata().current_schema().clone(), - self.table - .metadata() - .default_partition_spec() - .as_ref() - .clone(), + self.table.metadata().default_partition_spec().clone(), )?; for entry in manifest_entries { writer.add_entry(entry)?; @@ -427,8 +431,12 @@ impl<'a> SnapshotProducer<'a> { ); } - for (data_file, schema, partition_spec) in snapshot_produce_operation.removed_data_files() { - summary_collector.remove_file(data_file, schema.clone(), partition_spec.clone()); + for removed in snapshot_produce_operation.removed_data_files() { + summary_collector.remove_file( + &removed.data_file, + removed.schema.clone(), + removed.partition_spec.clone(), + ); } let previous_snapshot = table_metadata.current_snapshot(); @@ -597,7 +605,7 @@ mod tests { /// An operation that adds no existing manifests and reports one removed data file. struct RemoveOneFileOperation { - removed: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, + removed: Vec, } impl SnapshotProduceOperation for RemoveOneFileOperation { @@ -616,14 +624,14 @@ mod tests { Ok(vec![]) } - fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)] { + fn removed_data_files(&self) -> &[RemovedDataFile] { &self.removed } } struct RemoveExistingFileOperation { file_path: String, - removed: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, + removed: Vec, } impl SnapshotProduceOperation for RemoveExistingFileOperation { @@ -650,7 +658,7 @@ mod tests { .read(&manifest_list.entries()[0]) .await?; let schema = manifest.metadata().schema().clone(); - let partition_spec = manifest.metadata().partition_spec().clone(); + let partition_spec = Arc::new(manifest.metadata().partition_spec().clone()); let mut writer = producer.new_manifest_writer( ManifestContentType::Data, schema.clone(), @@ -661,11 +669,11 @@ mod tests { for entry in manifest.entries() { if entry.file_path() == self.file_path && entry.is_alive() { writer.add_delete_entry(entry.as_ref().clone())?; - removed.push(( - entry.data_file().clone(), - schema.clone(), - Arc::new(partition_spec.clone()), - )); + removed.push(RemovedDataFile { + data_file: entry.data_file().clone(), + schema: schema.clone(), + partition_spec: partition_spec.clone(), + }); } else if entry.is_alive() { writer.add_existing_entry(entry.as_ref().clone())?; } @@ -677,7 +685,7 @@ mod tests { Ok(vec![rewritten_manifest]) } - fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)] { + fn removed_data_files(&self) -> &[RemovedDataFile] { &self.removed } } @@ -710,11 +718,11 @@ mod tests { let removed = data_file(&table, "test/removed.parquet", 2); let operation = RemoveOneFileOperation { - removed: vec![( - removed, - table.metadata().current_schema().clone(), - table.metadata().default_partition_spec().clone(), - )], + removed: vec![RemovedDataFile { + data_file: removed, + schema: table.metadata().current_schema().clone(), + partition_spec: table.metadata().default_partition_spec().clone(), + }], }; let mut action_commit = @@ -823,12 +831,14 @@ mod tests { .build() .unwrap(), ); - let partition_spec = PartitionSpec::builder(schema.clone()) - .with_spec_id(3) - .add_partition_field("a", "a", Transform::Identity) - .unwrap() - .build() - .unwrap(); + let partition_spec = Arc::new( + PartitionSpec::builder(schema.clone()) + .with_spec_id(3) + .add_partition_field("a", "a", Transform::Identity) + .unwrap() + .build() + .unwrap(), + ); let producer = SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![]); let first = producer @@ -896,7 +906,11 @@ mod tests { let added = data_file(&table, "test/added.parquet", 3); let removed = data_file(&table, "test/removed.parquet", 2); let operation = RemoveOneFileOperation { - removed: vec![(removed, reported_schema, reported_spec)], + removed: vec![RemovedDataFile { + data_file: removed, + schema: reported_schema, + partition_spec: reported_spec, + }], }; let mut action_commit = From 495f3adc4e8bd2a349e59a6866a14b55b8ad8fdd Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:21:32 +0200 Subject: [PATCH 26/35] Take the added files again instead of cloning them for the summary --- crates/iceberg/src/transaction/snapshot.rs | 35 ++++++++++++++-------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 43345fdeb2..5c335a5b9d 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -330,9 +330,7 @@ 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 { - // 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(); + let added_data_files = std::mem::take(&mut self.added_data_files); if added_data_files.is_empty() { return Err(Error::new( ErrorKind::PreconditionFailed, @@ -400,11 +398,8 @@ impl<'a> SnapshotProducer<'a> { Ok(manifest_files) } - // Returns a `Summary` of the current snapshot - fn summary( - &self, - snapshot_produce_operation: &OP, - ) -> Result { + // Collects the added files, before `produce_manifests` takes them. + fn added_files_summary(&self) -> SnapshotSummaryCollector { let mut summary_collector = SnapshotSummaryCollector::default(); let table_metadata = self.table.metadata_ref(); @@ -430,6 +425,16 @@ impl<'a> SnapshotProducer<'a> { table_metadata.default_partition_spec().clone(), ); } + summary_collector + } + + // Returns a `Summary` of the current snapshot + fn summary( + &self, + mut summary_collector: SnapshotSummaryCollector, + snapshot_produce_operation: &OP, + ) -> Result { + let table_metadata = self.table.metadata_ref(); for removed in snapshot_produce_operation.removed_data_files() { summary_collector.remove_file( @@ -511,15 +516,19 @@ impl<'a> SnapshotProducer<'a> { ), }; - // The manifests are produced first so the summary can count the entries the operation - // actually marked Deleted. + // The manifests are produced before the summary is finished so it can count the entries + // the operation actually marked Deleted. + let summary_collector = self.added_files_summary(); let new_manifests = self .produce_manifests(&mut snapshot_produce_operation, &process) .await?; - let summary = self.summary(&snapshot_produce_operation).map_err(|err| { - Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err) - })?; + let summary = self + .summary(summary_collector, &snapshot_produce_operation) + .map_err(|err| { + Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.") + .with_source(err) + })?; manifest_list_writer.add_manifests(new_manifests.into_iter())?; let writer_next_row_id = manifest_list_writer.next_row_id(); From 96da179920b3bf82c1fed8ff149b53b43c7ee3ed Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:23:03 +0200 Subject: [PATCH 27/35] Assert an operation reports exactly the files it marked deleted --- crates/iceberg/src/spec/snapshot_summary.rs | 1 - crates/iceberg/src/transaction/snapshot.rs | 31 +++++++++++++++++---- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot_summary.rs b/crates/iceberg/src/spec/snapshot_summary.rs index 89af2081ba..ccdad4589a 100644 --- a/crates/iceberg/src/spec/snapshot_summary.rs +++ b/crates/iceberg/src/spec/snapshot_summary.rs @@ -344,7 +344,6 @@ pub(crate) fn update_snapshot_summaries( )); } - update_totals( &mut summary, previous_summary, diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 5c335a5b9d..5c6aca8c79 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -130,9 +130,7 @@ pub(crate) struct SnapshotProducer<'a> { commit_uuid: Uuid, snapshot_properties: HashMap, added_data_files: Vec, - // 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). + // Numbers this commit's manifest files. Atomic so `new_manifest_writer` can take `&self`. manifest_counter: AtomicU64, } @@ -394,6 +392,17 @@ impl<'a> SnapshotProducer<'a> { // # TODO // Support process delete entries. + debug_assert_eq!( + manifest_files + .iter() + .filter(|m| m.added_snapshot_id == self.snapshot_id + && m.content == ManifestContentType::Data) + .filter_map(|m| m.deleted_files_count) + .sum::() as usize, + snapshot_produce_operation.removed_data_files().len(), + "an operation must report exactly the data files it marked deleted" + ); + let manifest_files = manifest_process.process_manifests(self, manifest_files); Ok(manifest_files) } @@ -612,7 +621,7 @@ mod tests { use crate::transaction::tests::make_v2_minimal_table; use crate::transaction::{Transaction, TransactionAction}; - /// An operation that adds no existing manifests and reports one removed data file. + /// An operation that writes each of its removed files as a deleted entry in its own manifest. struct RemoveOneFileOperation { removed: Vec, } @@ -628,9 +637,19 @@ mod tests { async fn existing_manifest( &mut self, - _: &SnapshotProducer<'_>, + producer: &SnapshotProducer<'_>, ) -> Result> { - Ok(vec![]) + let mut manifests = vec![]; + for removed in &self.removed { + let mut writer = producer.new_manifest_writer( + ManifestContentType::Data, + removed.schema.clone(), + removed.partition_spec.clone(), + )?; + writer.add_delete_file(removed.data_file.clone(), 0, Some(0))?; + manifests.push(writer.write_manifest_file().await?); + } + Ok(manifests) } fn removed_data_files(&self) -> &[RemovedDataFile] { From a5653bceda2812696483842a1cc032a164ab8546 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:23:53 +0200 Subject: [PATCH 28/35] Say in the removed_data_files doc what it must match --- crates/iceberg/src/transaction/snapshot.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 5c6aca8c79..3c0c3c2f86 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -87,7 +87,7 @@ pub(crate) trait SnapshotProduceOperation: Send + Sync { snapshot_produce: &SnapshotProducer<'_>, ) -> impl Future>> + Send; - /// Returns the data files this operation actually removed. + /// Returns the data files this operation marked deleted in the manifests it wrote. /// /// 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. From 29772614f6e37c8338e46ed7c682297ad032da2d Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:29:21 +0200 Subject: [PATCH 29/35] Summarize deleted files from the producer's list, as pyiceberg does Drops the removed_data_files hook and the manifest-first reorder, and truncation defaults back to every overwrite. --- crates/iceberg/src/spec/snapshot_summary.rs | 315 +++++++++++++-- crates/iceberg/src/transaction/append.rs | 3 +- crates/iceberg/src/transaction/snapshot.rs | 403 ++++---------------- 3 files changed, 356 insertions(+), 365 deletions(-) diff --git a/crates/iceberg/src/spec/snapshot_summary.rs b/crates/iceberg/src/spec/snapshot_summary.rs index 7a639aa08e..d68a49d149 100644 --- a/crates/iceberg/src/spec/snapshot_summary.rs +++ b/crates/iceberg/src/spec/snapshot_summary.rs @@ -331,8 +331,9 @@ where T: PartialOrd + Default + ToString { } pub(crate) fn update_snapshot_summaries( - mut summary: Summary, + summary: Summary, previous_summary: Option<&Summary>, + truncate_full_table: bool, ) -> Result { // Validate that the operation is supported if summary.operation != Operation::Append @@ -342,6 +343,16 @@ pub(crate) fn update_snapshot_summaries( return Err(invalid_data!("Operation is not supported.")); } + let mut summary = match previous_summary { + Some(prev_summary) if truncate_full_table && summary.operation == Operation::Overwrite => { + truncate_table_summary(summary, prev_summary).map_err(|err| { + Error::new(ErrorKind::Unexpected, "Failed to truncate table summary.") + .with_source(err) + })? + } + _ => summary, + }; + update_totals( &mut summary, previous_summary, @@ -392,6 +403,77 @@ pub(crate) fn update_snapshot_summaries( Ok(summary) } +fn get_prop(previous_summary: &Summary, prop: &str) -> Result { + let value_str = previous_summary + .additional_properties + .get(prop) + .map(String::as_str) + .unwrap_or("0"); + value_str.parse::().map_err(|err| { + Error::new( + ErrorKind::Unexpected, + format!("Failed to parse summary property '{prop}' value '{value_str}' as u64."), + ) + .with_source(err) + }) +} + +fn truncate_table_summary(mut summary: Summary, previous_summary: &Summary) -> Result { + for prop in [ + TOTAL_DATA_FILES, + TOTAL_DELETE_FILES, + TOTAL_RECORDS, + TOTAL_FILE_SIZE, + TOTAL_POSITION_DELETES, + TOTAL_EQUALITY_DELETES, + ] { + summary + .additional_properties + .insert(prop.to_string(), "0".to_string()); + } + + let value = get_prop(previous_summary, TOTAL_DATA_FILES)?; + if value != 0 { + summary + .additional_properties + .insert(DELETED_DATA_FILES.to_string(), value.to_string()); + } + let value = get_prop(previous_summary, TOTAL_DELETE_FILES)?; + if value != 0 { + summary + .additional_properties + .insert(REMOVED_DELETE_FILES.to_string(), value.to_string()); + } + let value = get_prop(previous_summary, TOTAL_RECORDS)?; + if value != 0 { + summary + .additional_properties + .insert(DELETED_RECORDS.to_string(), value.to_string()); + } + let value = get_prop(previous_summary, TOTAL_FILE_SIZE)?; + if value != 0 { + summary + .additional_properties + .insert(REMOVED_FILE_SIZE.to_string(), value.to_string()); + } + + let value = get_prop(previous_summary, TOTAL_POSITION_DELETES)?; + if value != 0 { + summary + .additional_properties + .insert(REMOVED_POSITION_DELETES.to_string(), value.to_string()); + } + + let value = get_prop(previous_summary, TOTAL_EQUALITY_DELETES)?; + if value != 0 { + summary + .additional_properties + .insert(REMOVED_EQUALITY_DELETES.to_string(), value.to_string()); + } + + Ok(summary) +} + fn update_totals( summary: &mut Summary, previous_summary: Option<&Summary>, @@ -508,7 +590,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); assert_eq!( updated.additional_properties.get(TOTAL_DATA_FILES).unwrap(), @@ -545,6 +627,194 @@ mod tests { ); } + #[test] + fn test_truncate_table_summary() { + let prev_props: HashMap = [ + (TOTAL_DATA_FILES.to_string(), "10".to_string()), + (TOTAL_DELETE_FILES.to_string(), "5".to_string()), + (TOTAL_RECORDS.to_string(), "100".to_string()), + (TOTAL_FILE_SIZE.to_string(), "1000".to_string()), + (TOTAL_POSITION_DELETES.to_string(), "3".to_string()), + (TOTAL_EQUALITY_DELETES.to_string(), "2".to_string()), + ] + .into_iter() + .collect(); + + let previous_summary = Summary { + operation: Operation::Overwrite, + additional_properties: prev_props, + }; + + let mut new_props = HashMap::new(); + new_props.insert("dummy".to_string(), "value".to_string()); + let summary = Summary { + operation: Operation::Overwrite, + additional_properties: new_props, + }; + + let truncated = truncate_table_summary(summary, &previous_summary).unwrap(); + + assert_eq!( + truncated + .additional_properties + .get(TOTAL_DATA_FILES) + .unwrap(), + "0" + ); + assert_eq!( + truncated + .additional_properties + .get(TOTAL_DELETE_FILES) + .unwrap(), + "0" + ); + assert_eq!( + truncated.additional_properties.get(TOTAL_RECORDS).unwrap(), + "0" + ); + assert_eq!( + truncated + .additional_properties + .get(TOTAL_FILE_SIZE) + .unwrap(), + "0" + ); + assert_eq!( + truncated + .additional_properties + .get(TOTAL_POSITION_DELETES) + .unwrap(), + "0" + ); + assert_eq!( + truncated + .additional_properties + .get(TOTAL_EQUALITY_DELETES) + .unwrap(), + "0" + ); + + assert_eq!( + truncated + .additional_properties + .get(DELETED_DATA_FILES) + .unwrap(), + "10" + ); + assert_eq!( + truncated + .additional_properties + .get(REMOVED_DELETE_FILES) + .unwrap(), + "5" + ); + assert_eq!( + truncated + .additional_properties + .get(DELETED_RECORDS) + .unwrap(), + "100" + ); + assert_eq!( + truncated + .additional_properties + .get(REMOVED_FILE_SIZE) + .unwrap(), + "1000" + ); + assert_eq!( + truncated + .additional_properties + .get(REMOVED_POSITION_DELETES) + .unwrap(), + "3" + ); + assert_eq!( + truncated + .additional_properties + .get(REMOVED_EQUALITY_DELETES) + .unwrap(), + "2" + ); + } + + #[test] + fn test_update_snapshot_summaries_overwrite_truncate_handles_totals_above_i32_max() { + // A table can legitimately accumulate more than i32::MAX rows or files + // over its lifetime. Truncating such a table on overwrite must succeed + // and surface the previous totals into the deleted-* counters. + let big = (i32::MAX as u64 + 1).to_string(); // 2_147_483_648 + let prev_props: HashMap = [ + (TOTAL_DATA_FILES.to_string(), big.clone()), + (TOTAL_DELETE_FILES.to_string(), "0".to_string()), + (TOTAL_RECORDS.to_string(), big.clone()), + (TOTAL_FILE_SIZE.to_string(), "0".to_string()), + (TOTAL_POSITION_DELETES.to_string(), "0".to_string()), + (TOTAL_EQUALITY_DELETES.to_string(), "0".to_string()), + ] + .into_iter() + .collect(); + + let previous_summary = Summary { + operation: Operation::Overwrite, + additional_properties: prev_props, + }; + + let summary = Summary { + operation: Operation::Overwrite, + additional_properties: HashMap::new(), + }; + + let updated = update_snapshot_summaries(summary, Some(&previous_summary), true) + .expect("overwrite truncation should accept totals above i32::MAX"); + assert_eq!( + updated + .additional_properties + .get(DELETED_DATA_FILES) + .unwrap(), + &big + ); + assert_eq!( + updated.additional_properties.get(DELETED_RECORDS).unwrap(), + &big + ); + } + + #[test] + fn test_update_snapshot_summaries_overwrite_truncate_returns_err_on_malformed_total() { + // Non-numeric values in the previous summary (corruption, manual edits, + // a foreign implementation) must surface as a recoverable Err - not + // crash the process. + let prev_props: HashMap = [ + (TOTAL_DATA_FILES.to_string(), "not_a_number".to_string()), + (TOTAL_DELETE_FILES.to_string(), "0".to_string()), + (TOTAL_RECORDS.to_string(), "0".to_string()), + (TOTAL_FILE_SIZE.to_string(), "0".to_string()), + (TOTAL_POSITION_DELETES.to_string(), "0".to_string()), + (TOTAL_EQUALITY_DELETES.to_string(), "0".to_string()), + ] + .into_iter() + .collect(); + + let previous_summary = Summary { + operation: Operation::Overwrite, + additional_properties: prev_props, + }; + + let summary = Summary { + operation: Operation::Overwrite, + additional_properties: HashMap::new(), + }; + + let err = update_snapshot_summaries(summary, Some(&previous_summary), true) + .expect_err("malformed previous summary must produce an Err, not a panic"); + assert!( + err.message().contains("truncate table summary"), + "expected wrapped 'Failed to truncate table summary' context, got: {}", + err.message() + ); + } + #[test] fn test_snapshot_summary_collector_build() { let schema = Arc::new( @@ -879,7 +1149,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "12"); @@ -925,7 +1195,7 @@ mod tests { }; // Must not panic. - let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); let props = &updated.additional_properties; // The total whose added delta was unparsable is skipped... @@ -953,7 +1223,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, None).unwrap(); + let updated = update_snapshot_summaries(summary, None, false).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "4"); @@ -997,7 +1267,7 @@ mod tests { additional_properties: new_props, }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); + let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); let props = &updated.additional_properties; assert_eq!(props.get(TOTAL_DATA_FILES).unwrap(), "8"); @@ -1010,39 +1280,20 @@ mod tests { #[test] fn test_update_totals_omits_total_when_removed_exceeds_previous_total() { - let prev_props: HashMap = [ - (TOTAL_DATA_FILES, "2"), - (TOTAL_RECORDS, "10"), - (TOTAL_FILE_SIZE, "100"), - ] - .into_iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(); - let previous_summary = Summary { operation: Operation::Append, - additional_properties: prev_props, + additional_properties: HashMap::from([(TOTAL_DATA_FILES.to_string(), "2".to_string())]), }; - - let new_props: HashMap = [ - (DELETED_DATA_FILES, "3"), - (DELETED_RECORDS, "25"), - (REMOVED_FILE_SIZE, "250"), - ] - .into_iter() - .map(|(k, v)| (k.to_string(), v.to_string())) - .collect(); - let summary = Summary { operation: Operation::Delete, - additional_properties: new_props, + additional_properties: HashMap::from([( + DELETED_DATA_FILES.to_string(), + "3".to_string(), + )]), }; - let updated = update_snapshot_summaries(summary, Some(&previous_summary)).unwrap(); - let props = &updated.additional_properties; + let updated = update_snapshot_summaries(summary, Some(&previous_summary), false).unwrap(); - assert!(!props.contains_key(TOTAL_DATA_FILES)); - assert!(!props.contains_key(TOTAL_RECORDS)); - assert!(!props.contains_key(TOTAL_FILE_SIZE)); + assert!(!updated.additional_properties.contains_key(TOTAL_DATA_FILES)); } } diff --git a/crates/iceberg/src/transaction/append.rs b/crates/iceberg/src/transaction/append.rs index 5701be3d13..881bd5d752 100644 --- a/crates/iceberg/src/transaction/append.rs +++ b/crates/iceberg/src/transaction/append.rs @@ -93,6 +93,7 @@ impl TransactionAction for FastAppendAction { self.commit_uuid.unwrap_or_else(Uuid::now_v7), self.snapshot_properties.clone(), self.dedupe_added_files(), + vec![], ); // validate added files @@ -124,7 +125,7 @@ impl SnapshotProduceOperation for FastAppendOperation { } async fn existing_manifest( - &mut self, + &self, snapshot_produce: &SnapshotProducer<'_>, ) -> Result> { let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index a3b6e70dbd..0a64594422 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -83,27 +83,17 @@ 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( - &mut self, + &self, snapshot_produce: &SnapshotProducer<'_>, ) -> impl Future>> + Send; - /// Returns the data files this operation marked deleted in the manifests it wrote. - /// - /// 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) -> &[RemovedDataFile] { - &[] + /// Returns whether the snapshot summary resets the table totals. Defaults to every + /// overwrite; an operation that overwrites only some files returns `false`. + fn truncate_full_table(&self) -> bool { + self.operation() == Operation::Overwrite } } -/// A data file an operation removed, with the schema and partition spec of the manifest that -/// recorded it. -pub(crate) struct RemovedDataFile { - pub(crate) data_file: DataFile, - pub(crate) schema: SchemaRef, - pub(crate) partition_spec: PartitionSpecRef, -} - pub(crate) struct DefaultManifestProcess; impl ManifestProcess for DefaultManifestProcess { @@ -130,6 +120,7 @@ pub(crate) struct SnapshotProducer<'a> { commit_uuid: Uuid, snapshot_properties: HashMap, added_data_files: Vec, + deleted_data_files: Vec, // Numbers this commit's manifest files. Atomic so `new_manifest_writer` can take `&self`. manifest_counter: AtomicU64, } @@ -140,6 +131,7 @@ impl<'a> SnapshotProducer<'a> { commit_uuid: Uuid, snapshot_properties: HashMap, added_data_files: Vec, + deleted_data_files: Vec, ) -> Self { Self { table, @@ -147,6 +139,7 @@ impl<'a> SnapshotProducer<'a> { commit_uuid, snapshot_properties, added_data_files, + deleted_data_files, manifest_counter: AtomicU64::new(0), } } @@ -244,26 +237,22 @@ impl<'a> SnapshotProducer<'a> { snapshot_id } - /// Returns the path for the next manifest file of this commit. - fn new_manifest_path(&self) -> Result { - Ok(format!( - "{}/{}-m{}.{}", - self.table.metadata().metadata_location()?, - self.commit_uuid, - self.manifest_counter.fetch_add(1, Ordering::Relaxed), - DataFileFormat::Avro - )) - } - - /// Returns a writer for the next manifest file of this commit, routed through the table's - /// encryption manager when one is configured. + /// Returns a writer for the next manifest file of this commit, written under the given + /// schema and partition spec. pub(crate) fn new_manifest_writer( &self, content: ManifestContentType, schema: SchemaRef, partition_spec: PartitionSpecRef, ) -> Result { - let output_file = self.table.file_io().new_output(self.new_manifest_path()?)?; + let new_manifest_path = format!( + "{}/{}-m{}.{}", + self.table.metadata().metadata_location()?, + self.commit_uuid, + 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 = partition_spec.as_ref().clone(); let builder = if let Some(em) = self.table.encryption_manager() { @@ -358,7 +347,7 @@ impl<'a> SnapshotProducer<'a> { /// and collects all of the manifests to be included in the new snapshot as [ManifestFile] entries. async fn produce_manifests( &mut self, - snapshot_produce_operation: &mut OP, + snapshot_produce_operation: &OP, manifest_process: &MP, ) -> Result> { // Assert current snapshot producer contains new content to add to new snapshot. @@ -366,10 +355,13 @@ impl<'a> SnapshotProducer<'a> { // TODO: Allowing snapshot property setup with no added data files is a workaround. // We should clean it up after all necessary actions are supported. // For details, please refer to https://github.com/apache/iceberg-rust/issues/1548 - if self.added_data_files.is_empty() && self.snapshot_properties.is_empty() { + if self.added_data_files.is_empty() + && self.deleted_data_files.is_empty() + && self.snapshot_properties.is_empty() + { return Err(Error::new( ErrorKind::PreconditionFailed, - "No added data files or added snapshot properties found when write a manifest file", + "No added or deleted data files or added snapshot properties found when write a manifest file", )); } @@ -385,23 +377,15 @@ impl<'a> SnapshotProducer<'a> { // # TODO // Support process delete entries. - debug_assert_eq!( - manifest_files - .iter() - .filter(|m| m.added_snapshot_id == self.snapshot_id - && m.content == ManifestContentType::Data) - .filter_map(|m| m.deleted_files_count) - .sum::() as usize, - snapshot_produce_operation.removed_data_files().len(), - "an operation must report exactly the data files it marked deleted" - ); - let manifest_files = manifest_process.process_manifests(self, manifest_files); Ok(manifest_files) } - // Collects the added files, before `produce_manifests` takes them. - fn added_files_summary(&self) -> SnapshotSummaryCollector { + // Returns a `Summary` of the current snapshot + fn summary( + &self, + snapshot_produce_operation: &OP, + ) -> Result { let mut summary_collector = SnapshotSummaryCollector::default(); let table_metadata = self.table.metadata_ref(); @@ -427,22 +411,17 @@ impl<'a> SnapshotProducer<'a> { table_metadata.default_partition_spec().clone(), ); } - summary_collector - } - - // Returns a `Summary` of the current snapshot - fn summary( - &self, - mut summary_collector: SnapshotSummaryCollector, - snapshot_produce_operation: &OP, - ) -> Result { - let table_metadata = self.table.metadata_ref(); - for removed in snapshot_produce_operation.removed_data_files() { + for data_file in &self.deleted_data_files { + let partition_spec = table_metadata + .partition_spec_by_id(data_file.partition_spec_id) + .ok_or_else(|| { + invalid_data!("Unknown partition spec id {}", data_file.partition_spec_id) + })?; summary_collector.remove_file( - &removed.data_file, - removed.schema.clone(), - removed.partition_spec.clone(), + data_file, + table_metadata.current_schema().clone(), + partition_spec.clone(), ); } @@ -461,7 +440,11 @@ impl<'a> SnapshotProducer<'a> { additional_properties, }; - update_snapshot_summaries(summary, previous_snapshot.map(|s| s.summary())) + update_snapshot_summaries( + summary, + previous_snapshot.map(|s| s.summary()), + snapshot_produce_operation.truncate_full_table(), + ) } fn generate_manifest_list_file_path(&self, attempt: i64) -> Result { @@ -478,7 +461,7 @@ impl<'a> SnapshotProducer<'a> { /// Finished building the action and return the [`ActionCommit`] to the transaction. pub(crate) async fn commit( mut self, - mut snapshot_produce_operation: OP, + snapshot_produce_operation: OP, process: MP, ) -> Result { let manifest_list_path = self.generate_manifest_list_file_path(0)?; @@ -518,20 +501,17 @@ impl<'a> SnapshotProducer<'a> { ), }; - // The manifests are produced before the summary is finished so it can count the entries - // the operation actually marked Deleted. - let summary_collector = self.added_files_summary(); + // Calling self.summary() before self.produce_manifests() is important because self.added_data_files + // will be set to an empty vec after self.produce_manifests() returns, resulting in an empty summary + // being generated. + let summary = self.summary(&snapshot_produce_operation).map_err(|err| { + Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err) + })?; + let new_manifests = self - .produce_manifests(&mut snapshot_produce_operation, &process) + .produce_manifests(&snapshot_produce_operation, &process) .await?; - let summary = self - .summary(summary_collector, &snapshot_produce_operation) - .map_err(|err| { - Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.") - .with_source(err) - })?; - manifest_list_writer.add_manifests(new_manifests.into_iter())?; let writer_next_row_id = manifest_list_writer.next_row_id(); let file_metadata = manifest_list_writer.close().await?; @@ -617,18 +597,14 @@ mod tests { use super::*; use crate::spec::{ - DataContentType, DataFileBuilder, Literal, Manifest, ManifestStatus, NestedField, - PartitionSpec, PrimitiveType, Schema, Transform, Type, + DataContentType, DataFileBuilder, Literal, Manifest, NestedField, PartitionSpec, + PrimitiveType, Schema, Transform, Type, }; use crate::transaction::tests::make_v2_minimal_table; - use crate::transaction::{Transaction, TransactionAction}; - /// An operation that writes each of its removed files as a deleted entry in its own manifest. - struct RemoveOneFileOperation { - removed: Vec, - } + struct DeleteOperation; - impl SnapshotProduceOperation for RemoveOneFileOperation { + impl SnapshotProduceOperation for DeleteOperation { fn operation(&self) -> Operation { Operation::Delete } @@ -637,216 +613,34 @@ mod tests { Ok(vec![]) } - async fn existing_manifest( - &mut self, - producer: &SnapshotProducer<'_>, - ) -> Result> { - let mut manifests = vec![]; - for removed in &self.removed { - let mut writer = producer.new_manifest_writer( - ManifestContentType::Data, - removed.schema.clone(), - removed.partition_spec.clone(), - )?; - writer.add_delete_file(removed.data_file.clone(), 0, Some(0))?; - manifests.push(writer.write_manifest_file().await?); - } - Ok(manifests) - } - - fn removed_data_files(&self) -> &[RemovedDataFile] { - &self.removed - } - } - - struct RemoveExistingFileOperation { - file_path: String, - removed: Vec, - } - - impl SnapshotProduceOperation for RemoveExistingFileOperation { - fn operation(&self) -> Operation { - Operation::Overwrite - } - - async fn delete_entries(&self, _: &SnapshotProducer<'_>) -> Result> { + async fn existing_manifest(&self, _: &SnapshotProducer<'_>) -> Result> { Ok(vec![]) } - - async fn existing_manifest( - &mut self, - producer: &SnapshotProducer<'_>, - ) -> Result> { - let snapshot = producer.table.metadata().current_snapshot().unwrap(); - let manifest_list = producer.table.manifest_list_reader(snapshot).load().await?; - // The initial append writes both files into one manifest. - assert_eq!(manifest_list.entries().len(), 1); - - let manifest = producer - .table - .manifest_reader() - .read(&manifest_list.entries()[0]) - .await?; - let schema = manifest.metadata().schema().clone(); - let partition_spec = Arc::new(manifest.metadata().partition_spec().clone()); - let mut writer = producer.new_manifest_writer( - ManifestContentType::Data, - schema.clone(), - partition_spec.clone(), - )?; - - let mut removed = Vec::new(); - for entry in manifest.entries() { - if entry.file_path() == self.file_path && entry.is_alive() { - writer.add_delete_entry(entry.as_ref().clone())?; - removed.push(RemovedDataFile { - data_file: entry.data_file().clone(), - schema: schema.clone(), - partition_spec: partition_spec.clone(), - }); - } else if entry.is_alive() { - writer.add_existing_entry(entry.as_ref().clone())?; - } - } - assert_eq!(removed.len(), 1); - - let rewritten_manifest = writer.write_manifest_file().await?; - self.removed.extend(removed); - Ok(vec![rewritten_manifest]) - } - - fn removed_data_files(&self) -> &[RemovedDataFile] { - &self.removed - } } - fn apply_commit(table: Table, mut commit: ActionCommit) -> Table { - let mut builder = table.metadata().clone().into_builder(None); - for update in commit.take_updates() { - builder = update.apply(builder).unwrap(); - } - table.with_metadata(Arc::new(builder.build().unwrap().metadata)) - } - - fn data_file(table: &Table, path: &str, record_count: u64) -> DataFile { - DataFileBuilder::default() + #[test] + fn test_summary_counts_deleted_data_files() { + let table = make_v2_minimal_table(); + let deleted = DataFileBuilder::default() .content(DataContentType::Data) - .file_path(path.to_string()) + .file_path("test/deleted.parquet".to_string()) .file_format(DataFileFormat::Parquet) .file_size_in_bytes(100) - .record_count(record_count) + .record_count(2) .partition_spec_id(table.metadata().default_partition_spec_id()) .partition(Struct::from_iter([Some(Literal::long(300))])) .build() - .unwrap() - } - - #[tokio::test] - async fn test_summary_counts_files_the_operation_reports_as_removed() { - let table = make_v2_minimal_table(); - let added = data_file(&table, "test/added.parquet", 3); - let removed = data_file(&table, "test/removed.parquet", 2); - - let operation = RemoveOneFileOperation { - removed: vec![RemovedDataFile { - data_file: removed, - schema: table.metadata().current_schema().clone(), - partition_spec: table.metadata().default_partition_spec().clone(), - }], - }; - - let mut action_commit = - SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![added]) - .commit(operation, DefaultManifestProcess) - .await - .unwrap(); + .unwrap(); - let updates = action_commit.take_updates(); - let TableUpdate::AddSnapshot { snapshot } = &updates[0] else { - unreachable!() - }; - let properties = &snapshot.summary().additional_properties; + let producer = SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![], vec![ + deleted, + ]); + let summary = producer.summary(&DeleteOperation).unwrap(); + let properties = &summary.additional_properties; assert_eq!(properties.get("deleted-data-files").unwrap(), "1"); assert_eq!(properties.get("deleted-records").unwrap(), "2"); - assert_eq!(properties.get("added-data-files").unwrap(), "1"); - assert_eq!(properties.get("added-records").unwrap(), "3"); - assert_eq!(properties.get("total-data-files").unwrap(), "0"); - assert_eq!(properties.get("total-records").unwrap(), "1"); - } - - #[tokio::test] - async fn test_partial_overwrite_reports_actual_removed_file() { - let table = make_v2_minimal_table(); - let removed = data_file(&table, "test/removed.parquet", 2); - let kept = data_file(&table, "test/kept.parquet", 5); - let append = Transaction::new(&table) - .fast_append() - .add_data_files(vec![removed.clone(), kept]); - let initial_commit = Arc::new(append).commit(&table).await.unwrap(); - let table = apply_commit(table, initial_commit); - - let initial_summary = &table - .metadata() - .current_snapshot() - .unwrap() - .summary() - .additional_properties; - assert_eq!(initial_summary.get("total-data-files").unwrap(), "2"); - assert_eq!(initial_summary.get("total-records").unwrap(), "7"); - - let mut replacement = data_file(&table, "test/replacement.parquet", 3); - replacement.file_size_in_bytes = 300; - let operation = RemoveExistingFileOperation { - file_path: removed.file_path.clone(), - removed: vec![], - }; - let commit = - SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![replacement]) - .commit(operation, DefaultManifestProcess) - .await - .unwrap(); - let table = apply_commit(table, commit); - let snapshot = table.metadata().current_snapshot().unwrap(); - let properties = &snapshot.summary().additional_properties; - - assert_eq!(snapshot.summary().operation, Operation::Overwrite); - assert_eq!(properties.get("deleted-data-files").unwrap(), "1"); - assert_eq!(properties.get("deleted-records").unwrap(), "2"); - assert_eq!(properties.get("added-data-files").unwrap(), "1"); - assert_eq!(properties.get("added-records").unwrap(), "3"); assert_eq!(properties.get("removed-files-size").unwrap(), "100"); - assert_eq!(properties.get("added-files-size").unwrap(), "300"); - assert_eq!(properties.get("total-data-files").unwrap(), "2"); - assert_eq!(properties.get("total-records").unwrap(), "8"); - assert_eq!(properties.get("total-files-size").unwrap(), "400"); - - let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); - assert_eq!(manifest_list.entries().len(), 2); - let mut statuses = HashMap::new(); - for manifest_file in manifest_list.entries() { - let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); - for entry in manifest.entries() { - assert!( - statuses - .insert(entry.file_path().to_string(), entry.status()) - .is_none() - ); - } - } - assert_eq!(statuses.len(), 3); - assert_eq!( - statuses.get("test/removed.parquet"), - Some(&ManifestStatus::Deleted) - ); - assert_eq!( - statuses.get("test/kept.parquet"), - Some(&ManifestStatus::Existing) - ); - assert_eq!( - statuses.get("test/replacement.parquet"), - Some(&ManifestStatus::Added) - ); } #[tokio::test] @@ -870,7 +664,8 @@ mod tests { .unwrap(), ); - let producer = SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![]); + let producer = + SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![], vec![]); let first = producer .new_manifest_writer( ManifestContentType::Data, @@ -905,60 +700,4 @@ mod tests { assert_eq!(metadata.partition_spec().spec_id(), 3); assert_eq!(first.partition_spec_id, 3); } - - #[tokio::test] - async fn test_removed_file_summary_uses_the_reported_partition_spec() { - let table = make_v2_minimal_table(); - let metadata = table - .metadata() - .clone() - .into_builder(None) - .set_properties(HashMap::from([( - TableProperties::PROPERTY_WRITE_PARTITION_SUMMARY_LIMIT.to_string(), - "10".to_string(), - )])) - .unwrap() - .build() - .unwrap() - .metadata; - let table = table.with_metadata(Arc::new(metadata)); - - let reported_schema = table.metadata().current_schema().clone(); - let reported_spec = Arc::new( - PartitionSpec::builder(reported_schema.clone()) - .with_spec_id(1) - .add_partition_field("y", "y", Transform::Identity) - .unwrap() - .build() - .unwrap(), - ); - - let added = data_file(&table, "test/added.parquet", 3); - let removed = data_file(&table, "test/removed.parquet", 2); - let operation = RemoveOneFileOperation { - removed: vec![RemovedDataFile { - data_file: removed, - schema: reported_schema, - partition_spec: reported_spec, - }], - }; - - let mut action_commit = - SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![added]) - .commit(operation, DefaultManifestProcess) - .await - .unwrap(); - - let updates = action_commit.take_updates(); - let TableUpdate::AddSnapshot { snapshot } = &updates[0] else { - unreachable!() - }; - let properties = &snapshot.summary().additional_properties; - - let removed_partition = properties - .get("partitions.y=300") - .expect("removal must be summarized under the reported spec's partition path"); - assert!(removed_partition.contains("deleted-data-files=1")); - assert!(properties.contains_key("partitions.x=300")); - } } From 1bb452b97ae4c491ae79ea9c5d8fdc97b74def1b Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:39:12 +0200 Subject: [PATCH 30/35] Collapse added and deleted files by path in the producer Overwrite gets the dedup fast append already had, and the shared error no longer says fast append. --- crates/iceberg/src/transaction/append.rs | 16 ++-------------- crates/iceberg/src/transaction/snapshot.rs | 14 +++++++++++--- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/crates/iceberg/src/transaction/append.rs b/crates/iceberg/src/transaction/append.rs index 881bd5d752..459f63811c 100644 --- a/crates/iceberg/src/transaction/append.rs +++ b/crates/iceberg/src/transaction/append.rs @@ -15,7 +15,7 @@ // specific language governing permissions and limitations // under the License. -use std::collections::{HashMap, HashSet}; +use std::collections::HashMap; use std::sync::Arc; use async_trait::async_trait; @@ -71,18 +71,6 @@ impl FastAppendAction { self.snapshot_properties = snapshot_properties; self } - - /// Collapse files sharing a path to their first occurrence, so a single - /// manifest never references the same file twice. Always runs (unlike the - /// `check_duplicate`-gated cross-snapshot check) since it is in-memory only. - fn dedupe_added_files(&self) -> Vec { - let mut seen = HashSet::with_capacity(self.added_data_files.len()); - self.added_data_files - .iter() - .filter(|data_file| seen.insert(data_file.file_path.as_str())) - .cloned() - .collect() - } } #[async_trait] @@ -92,7 +80,7 @@ impl TransactionAction for FastAppendAction { table, self.commit_uuid.unwrap_or_else(Uuid::now_v7), self.snapshot_properties.clone(), - self.dedupe_added_files(), + self.added_data_files.clone(), vec![], ); diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 0a64594422..d9aa05f90c 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -21,6 +21,7 @@ use std::sync::atomic::{AtomicU64, Ordering}; use futures::TryStreamExt; use futures::stream::FuturesUnordered; +use itertools::Itertools; use uuid::Uuid; use crate::error::{Result, invalid_data}; @@ -138,8 +139,15 @@ impl<'a> SnapshotProducer<'a> { snapshot_id: Self::generate_unique_snapshot_id(table), commit_uuid, snapshot_properties, - added_data_files, - deleted_data_files, + // Collapsed to the first file per path, so a manifest never references a file twice. + added_data_files: added_data_files + .into_iter() + .unique_by(|f| f.file_path.clone()) + .collect(), + deleted_data_files: deleted_data_files + .into_iter() + .unique_by(|f| f.file_path.clone()) + .collect(), manifest_counter: AtomicU64::new(0), } } @@ -148,7 +156,7 @@ impl<'a> SnapshotProducer<'a> { for data_file in &self.added_data_files { if data_file.content_type() != crate::spec::DataContentType::Data { return Err(invalid_data!( - "Only data content type is allowed for fast append" + "Only data content type is allowed" )); } // Check if the data file partition spec id matches the table default partition spec id. From 633142772c9697c80056cd14d865102c621ff45d Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:39:12 +0200 Subject: [PATCH 31/35] Rewrite only live entries and count deletes from the producer, as Java and pyiceberg do Drops add_tombstone_entry, the removed-files bookkeeping and the duplicate tests. --- crates/iceberg/src/spec/manifest/writer.rs | 21 - crates/iceberg/src/transaction/overwrite.rs | 568 ++++---------------- 2 files changed, 95 insertions(+), 494 deletions(-) diff --git a/crates/iceberg/src/spec/manifest/writer.rs b/crates/iceberg/src/spec/manifest/writer.rs index 874b2ee356..106b462627 100644 --- a/crates/iceberg/src/spec/manifest/writer.rs +++ b/crates/iceberg/src/spec/manifest/writer.rs @@ -315,10 +315,6 @@ impl ManifestWriter { /// Add a delete manifest entry. This method will update following status of the entry: /// - Update the entry status to `Deleted` /// - Set the snapshot id to the current snapshot id - /// - /// # TODO - /// Remove this allow later - #[allow(dead_code)] pub(crate) fn add_delete_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { self.check_data_file(&entry.data_file)?; entry.status = ManifestStatus::Deleted; @@ -350,10 +346,6 @@ impl ManifestWriter { /// Add an existing manifest entry. This method will update following status of the entry: /// - Update the entry status to `Existing` - /// - /// # TODO - /// Remove this allow later - #[allow(dead_code)] pub(crate) fn add_existing_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { self.check_data_file(&entry.data_file)?; entry.status = ManifestStatus::Existing; @@ -361,19 +353,6 @@ impl ManifestWriter { Ok(()) } - /// Add a manifest entry as a tombstone, preserving its original sequence numbers and - /// snapshot id. - /// - /// The caller owns `snapshot_id`: set it to the deleting snapshot for a file this commit - /// removes, and leave it untouched when carrying an earlier tombstone forward. Use - /// `add_delete_entry` instead to have this manifest's snapshot id stamped on the entry. - pub(crate) fn add_tombstone_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { - self.check_data_file(&entry.data_file)?; - entry.status = ManifestStatus::Deleted; - self.add_entry_inner(entry)?; - Ok(()) - } - /// Add an file as existing manifest entry. The original data and file sequence numbers, snapshot ID, /// which were assigned at commit, must be preserved when adding an existing entry. pub fn add_existing_file( diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 8597c65a7a..64e4d6f284 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -21,16 +21,13 @@ use std::sync::Arc; use async_trait::async_trait; use uuid::Uuid; -use crate::error::Result; -use crate::spec::{ - DataFile, Manifest, ManifestEntry, ManifestFile, Operation, PartitionSpecRef, SchemaRef, -}; +use crate::error::{Result, invalid_data}; +use crate::spec::{DataFile, ManifestEntry, ManifestFile, Operation}; use crate::table::Table; use crate::transaction::snapshot::{ DefaultManifestProcess, SnapshotProduceOperation, SnapshotProducer, }; use crate::transaction::{ActionCommit, TransactionAction}; -use crate::{Error, ErrorKind}; /// OverwriteAction is a transaction action for overwriting data files in the table. /// @@ -104,32 +101,23 @@ impl TransactionAction for OverwriteAction { snapshot_producer.validate_duplicate_files().await?; } - let deleted_file_paths: HashSet = self - .deleted_data_files - .iter() - .map(|f| f.file_path.clone()) - .collect(); - - let snapshot_id = snapshot_producer.snapshot_id(); + let operation = OverwriteOperation { + has_added_data_files: !self.added_data_files.is_empty(), + deleted_file_paths: self + .deleted_data_files + .iter() + .map(|f| f.file_path.clone()) + .collect(), + }; snapshot_producer - .commit( - OverwriteOperation { - deleted_file_paths, - has_added_data_files: !self.added_data_files.is_empty(), - snapshot_id, - removed_data_files: vec![], - }, - DefaultManifestProcess, - ) + .commit(operation, DefaultManifestProcess) .await } } struct OverwriteOperation { - deleted_file_paths: HashSet, has_added_data_files: bool, - snapshot_id: i64, - removed_data_files: Vec<(DataFile, SchemaRef, PartitionSpecRef)>, + deleted_file_paths: HashSet, } impl SnapshotProduceOperation for OverwriteOperation { @@ -140,10 +128,16 @@ impl SnapshotProduceOperation for OverwriteOperation { ) { (true, true) => Operation::Overwrite, (false, true) => Operation::Delete, + // Also a properties-only commit, the workaround from #1548. _ => Operation::Append, } } + // Only the listed files are replaced, so the table totals carry over. + fn truncate_full_table(&self) -> bool { + false + } + async fn delete_entries( &self, _snapshot_produce: &SnapshotProducer<'_>, @@ -151,159 +145,93 @@ impl SnapshotProduceOperation for OverwriteOperation { Ok(vec![]) } - fn removed_data_files(&self) -> &[(DataFile, SchemaRef, PartitionSpecRef)] { - &self.removed_data_files - } - async fn existing_manifest( - &mut self, + &self, snapshot_produce: &SnapshotProducer<'_>, ) -> Result> { - let Some(snapshot) = snapshot_produce.table.metadata().current_snapshot() else { - self.validate_deletes_matched()?; - return Ok(vec![]); - }; - - let manifest_list = snapshot_produce - .table - .manifest_list_reader(snapshot) - .load() - .await?; - - if self.deleted_file_paths.is_empty() { - return Ok(manifest_list - .entries() - .iter() - .filter(|entry| { - // Delete-only manifests record which files were removed and must survive - // until `expire_snapshots` cleans them up (see #2148). - entry.has_added_files() - || entry.has_existing_files() - || entry.has_deleted_files() - }) - .cloned() - .collect()); - } - - let mut result = Vec::new(); + let table = snapshot_produce.table; + let mut manifests = vec![]; + let mut matched: HashSet<&str> = HashSet::new(); + + if let Some(snapshot) = table.metadata().current_snapshot() { + let manifest_list = table.manifest_list_reader(snapshot).load().await?; + for manifest_file in manifest_list.entries() { + // Delete-only manifests record which files were removed and must survive + // until `expire_snapshots` cleans them up (see #2148). + if !manifest_file.has_added_files() + && !manifest_file.has_existing_files() + && !manifest_file.has_deleted_files() + { + continue; + } + if self.deleted_file_paths.is_empty() { + manifests.push(manifest_file.clone()); + continue; + } - for manifest_file in manifest_list.entries() { - if !manifest_file.has_added_files() - && !manifest_file.has_existing_files() - && !manifest_file.has_deleted_files() - { - continue; - } + let manifest = table.manifest_reader().read(manifest_file).await?; + let deletes: Vec<&str> = manifest + .entries() + .iter() + .filter(|entry| entry.is_alive()) + .filter_map(|entry| self.deleted_file_paths.get(entry.file_path())) + .map(String::as_str) + .collect(); + if deletes.is_empty() { + manifests.push(manifest_file.clone()); + continue; + } + matched.extend(deletes); - let manifest = snapshot_produce - .table - .manifest_reader() - .read(manifest_file) - .await?; - - let has_deletes = manifest.entries().iter().any(|entry| { - entry.is_alive() && self.deleted_file_paths.contains(entry.file_path()) - }); - - if has_deletes { - let rewritten = self - .rewrite_manifest(snapshot_produce, manifest_file, &manifest) - .await?; - result.push(rewritten); - } else { - result.push(manifest_file.clone()); + let mut writer = snapshot_produce.new_manifest_writer( + manifest_file.content, + manifest.metadata().schema.clone(), + Arc::new(manifest.metadata().partition_spec.clone()), + )?; + // Like Java, only live entries are carried into the rewrite. + for entry in manifest.entries().iter().filter(|entry| entry.is_alive()) { + if self.deleted_file_paths.contains(entry.file_path()) { + writer.add_delete_entry(entry.as_ref().clone())?; + } else { + writer.add_existing_entry(entry.as_ref().clone())?; + } + } + manifests.push(writer.write_manifest_file().await?); } } - self.validate_deletes_matched()?; - - Ok(result) - } -} - -impl OverwriteOperation { - /// Fails when a requested delete path was not live in any manifest, matching the existence - /// check Java's `BaseOverwriteFiles` performs before committing. - fn validate_deletes_matched(&self) -> Result<()> { - let removed: HashSet<&str> = self - .removed_data_files - .iter() - .map(|(data_file, _, _)| data_file.file_path.as_str()) - .collect(); let mut missing: Vec<&str> = self .deleted_file_paths .iter() .map(String::as_str) - .filter(|path| !removed.contains(path)) + .filter(|path| !matched.contains(path)) .collect(); - - if missing.is_empty() { - return Ok(()); + if !missing.is_empty() { + missing.sort_unstable(); + return Err(invalid_data!( + "Missing required files to delete: {}", + missing.join(", ") + )); } - missing.sort_unstable(); - Err(Error::new( - ErrorKind::DataInvalid, - format!("Missing required files to delete: {}", missing.join(", ")), - )) - } - - /// Rewrite a manifest, marking entries whose file paths are in `deleted_file_paths` - /// as `ManifestStatus::Deleted`. - async fn rewrite_manifest( - &mut self, - snapshot_produce: &SnapshotProducer<'_>, - manifest_file: &ManifestFile, - manifest: &Manifest, - ) -> Result { - let partition_spec: PartitionSpecRef = Arc::new(manifest.metadata().partition_spec.clone()); - let mut writer = snapshot_produce.new_manifest_writer( - manifest_file.content, - manifest.metadata().schema.clone(), - partition_spec.as_ref().clone(), - )?; - - for entry in manifest.entries() { - let entry: ManifestEntry = (**entry).clone(); - if !entry.is_alive() { - // A tombstone keeps its original snapshot id so the delete stays attributed - // to the snapshot that made it. - writer.add_tombstone_entry(entry)?; - } else if self.deleted_file_paths.contains(entry.file_path()) { - let mut deleted = entry; - deleted.snapshot_id = Some(self.snapshot_id); - self.removed_data_files.push(( - deleted.data_file().clone(), - manifest.metadata().schema.clone(), - partition_spec.clone(), - )); - writer.add_tombstone_entry(deleted)?; - } else { - writer.add_existing_entry(entry)?; - } - } - - writer.write_manifest_file().await + Ok(manifests) } } #[cfg(test)] mod tests { - use std::collections::HashMap; use std::sync::Arc; - use uuid::Uuid; - use crate::memory::tests::new_memory_catalog; use crate::spec::{ - DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, MAIN_BRANCH, - ManifestEntryRef, ManifestStatus, Operation, PrimitiveType, SnapshotRef, Struct, Transform, - Type, UnboundPartitionField, UnboundPartitionSpec, + DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, ManifestStatus, + Operation, PrimitiveType, SnapshotRef, Struct, Transform, Type, UnboundPartitionField, + UnboundPartitionSpec, }; use crate::table::Table; - use crate::transaction::tests::{make_v2_minimal_table, make_v3_minimal_table_in_catalog}; + use crate::transaction::tests::make_v3_minimal_table_in_catalog; use crate::transaction::{AddColumn, ApplyTransactionAction, Transaction, TransactionAction}; - use crate::{ErrorKind, TableRequirement, TableUpdate}; + use crate::{ErrorKind, TableUpdate}; fn test_data_file(path: &str, partition_spec_id: i32) -> DataFile { DataFileBuilder::default() @@ -332,157 +260,6 @@ mod tests { entries } - async fn current_manifest_entry(table: &Table, path: &str) -> ManifestEntryRef { - let snapshot = table.metadata().current_snapshot().unwrap(); - let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); - - for manifest_file in manifest_list.entries() { - let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); - if let Some(entry) = manifest.entries().iter().find(|e| e.file_path() == path) { - return entry.clone(); - } - } - panic!("no manifest entry for {path}"); - } - - #[tokio::test] - async fn test_empty_data_overwrite_action() { - let table = make_v2_minimal_table(); - let tx = Transaction::new(&table); - let action = tx.overwrite().add_data_files(vec![]); - let err = Arc::new(action).commit(&table).await.err().unwrap(); - assert_eq!(err.kind(), ErrorKind::PreconditionFailed); - assert!( - err.to_string() - .contains("No added data files, deleted data files, or added snapshot properties"), - "{err}" - ); - } - - #[tokio::test] - async fn test_overwrite_snapshot_properties() { - let table = make_v2_minimal_table(); - let tx = Transaction::new(&table); - - let mut snapshot_properties = HashMap::new(); - snapshot_properties.insert("key".to_string(), "val".to_string()); - - let data_file = test_data_file( - "test/1.parquet", - table.metadata().default_partition_spec_id(), - ); - - let action = tx - .overwrite() - .set_snapshot_properties(snapshot_properties) - .add_data_files(vec![data_file]); - let mut action_commit = Arc::new(action).commit(&table).await.unwrap(); - let updates = action_commit.take_updates(); - - let new_snapshot = if let TableUpdate::AddSnapshot { snapshot } = &updates[0] { - snapshot - } else { - unreachable!() - }; - assert_eq!( - new_snapshot - .summary() - .additional_properties - .get("key") - .unwrap(), - "val" - ); - } - - #[tokio::test] - async fn test_overwrite_incompatible_partition_value() { - let table = make_v2_minimal_table(); - let tx = Transaction::new(&table); - - let data_file = DataFileBuilder::default() - .content(DataContentType::Data) - .file_path("test/3.parquet".to_string()) - .file_format(DataFileFormat::Parquet) - .file_size_in_bytes(100) - .record_count(1) - .partition_spec_id(table.metadata().default_partition_spec_id()) - .partition(Struct::from_iter([Some(Literal::string("test"))])) - .build() - .unwrap(); - - let action = tx.overwrite().add_data_files(vec![data_file]); - assert!(Arc::new(action).commit(&table).await.is_err()); - } - - #[tokio::test] - async fn test_overwrite_basic() { - let table = make_v2_minimal_table(); - let tx = Transaction::new(&table); - - let data_file = test_data_file( - "test/3.parquet", - table.metadata().default_partition_spec_id(), - ); - - let action = tx.overwrite().add_data_files(vec![data_file.clone()]); - let mut action_commit = Arc::new(action).commit(&table).await.unwrap(); - let updates = action_commit.take_updates(); - let requirements = action_commit.take_requirements(); - - assert!( - matches!((&updates[0],&updates[1]), (TableUpdate::AddSnapshot { snapshot },TableUpdate::SetSnapshotRef { reference,ref_name }) if snapshot.snapshot_id() == reference.snapshot_id && ref_name == MAIN_BRANCH) - ); - - assert_eq!( - vec![ - TableRequirement::UuidMatch { - uuid: table.metadata().uuid() - }, - TableRequirement::RefSnapshotIdMatch { - r#ref: MAIN_BRANCH.to_string(), - snapshot_id: table.metadata().current_snapshot_id - } - ], - requirements - ); - - let new_snapshot: SnapshotRef = if let TableUpdate::AddSnapshot { snapshot } = &updates[0] { - SnapshotRef::new(snapshot.clone()) - } else { - unreachable!() - }; - assert_eq!(new_snapshot.summary().operation, Operation::Append); - - let manifest_list = table - .manifest_list_reader(&new_snapshot) - .load() - .await - .unwrap(); - assert_eq!(1, manifest_list.entries().len()); - assert_eq!( - manifest_list.entries()[0].sequence_number, - new_snapshot.sequence_number() - ); - - let manifest = table - .manifest_reader() - .read(&manifest_list.entries()[0]) - .await - .unwrap(); - assert_eq!(1, manifest.entries().len()); - assert_eq!( - new_snapshot.sequence_number(), - manifest.entries()[0] - .sequence_number() - .expect("Inherit sequence number by load manifest") - ); - assert_eq!( - new_snapshot.snapshot_id(), - manifest.entries()[0].snapshot_id().unwrap() - ); - assert_eq!(data_file, *manifest.entries()[0].data_file()); - } - #[tokio::test] async fn test_overwrite_with_deleted_files() { let catalog = new_memory_catalog().await; @@ -498,10 +275,6 @@ mod tests { let tx = action.apply(tx).unwrap(); let table = tx.commit(&catalog).await.unwrap(); - let snapshot = table.metadata().current_snapshot().unwrap(); - let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); - assert_eq!(1, manifest_list.entries().len()); - let replacement_file = test_data_file("test/replacement.parquet", spec_id); let tx = Transaction::new(&table); let action = tx @@ -513,90 +286,26 @@ mod tests { let snapshot = table.metadata().current_snapshot().unwrap(); assert_eq!(snapshot.summary().operation, Operation::Overwrite); + let summary = &snapshot.summary().additional_properties; + assert_eq!(summary.get("deleted-data-files").unwrap(), "2"); + assert_eq!(summary.get("deleted-records").unwrap(), "2"); - let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); - - assert_eq!(2, manifest_list.entries().len()); - - let mut all_entries = vec![]; - for manifest_file in manifest_list.entries() { - let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); - for entry in manifest.entries() { - all_entries.push((entry.status(), entry.file_path().to_string())); - } - } - - for original_file in ["test/original1.parquet", "test/original2.parquet"] { - assert!( - all_entries - .iter() - .any(|(status, path)| *status == ManifestStatus::Deleted - && path == original_file), - "Original file {original_file} should be marked as Deleted, entries: {all_entries:?}", - ); - } - - assert!( - all_entries - .iter() - .any(|(status, path)| *status == ManifestStatus::Added - && path == "test/replacement.parquet"), - "Replacement file should be marked as Added, entries: {all_entries:?}", - ); - - // Verify snapshot summary reports the deleted file. - assert_eq!( - snapshot - .summary() - .additional_properties - .get("deleted-data-files") - .map(|s| s.as_str()), - Some("2") - ); - assert_eq!( - snapshot - .summary() - .additional_properties - .get("deleted-records") - .map(|s| s.as_str()), - Some("2") - ); - - // Step 3: Fast append after overwrite — delete-only manifest must survive. - let appended_file = test_data_file("test/appended.parquet", spec_id); - let tx = Transaction::new(&table); - let action = tx.fast_append().add_data_files(vec![appended_file.clone()]); - let tx = action.apply(tx).unwrap(); - let table = tx.commit(&catalog).await.unwrap(); - - let snapshot = table.metadata().current_snapshot().unwrap(); - let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); - - // 3 manifests: rewritten (deleted entry), overwrite added, fast_append added. - assert_eq!(3, manifest_list.entries().len()); - - let mut all_entries = vec![]; - for manifest_file in manifest_list.entries() { - let manifest = table.manifest_reader().read(manifest_file).await.unwrap(); - for entry in manifest.entries() { - all_entries.push((entry.status(), entry.file_path().to_string())); - } - } - - // The deleted entry must still be present after fast_append. - for original_file in ["test/original1.parquet", "test/original2.parquet"] { + let entries = current_manifest_entries(&table).await; + assert_eq!(3, entries.len(), "{entries:?}"); + for entry in [ + (ManifestStatus::Deleted, "test/original1.parquet"), + (ManifestStatus::Deleted, "test/original2.parquet"), + (ManifestStatus::Added, "test/replacement.parquet"), + ] { assert!( - all_entries - .iter() - .any(|(status, path)| *status == ManifestStatus::Deleted - && path == original_file), - "Deleted entry should survive fast_append, entries: {all_entries:?}", + entries.contains(&(entry.0, entry.1.to_string())), + "{entries:?}" ); } } #[tokio::test] - async fn test_second_overwrite_keeps_prior_deleted_entries_deleted() { + async fn test_second_overwrite_does_not_resurrect_a_deleted_file() { let catalog = new_memory_catalog().await; let table = make_v3_minimal_table_in_catalog(&catalog).await; let spec_id = table.metadata().default_partition_spec_id(); @@ -615,8 +324,6 @@ mod tests { .add_data_files(vec![test_data_file("test/c.parquet", spec_id)]) .delete_data_files(vec![file_a.clone()]); let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - let tombstone = current_manifest_entry(&table, "test/a.parquet").await; - assert_eq!(ManifestStatus::Deleted, tombstone.status()); let tx = Transaction::new(&table); let action = tx @@ -625,26 +332,12 @@ mod tests { .delete_data_files(vec![file_b.clone()]); let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - let carried = current_manifest_entry(&table, "test/a.parquet").await; - assert_eq!( - ManifestStatus::Deleted, - carried.status(), - "the first overwrite's tombstone must stay Deleted" - ); - assert_eq!( - tombstone.snapshot_id, carried.snapshot_id, - "the tombstone must keep the snapshot that deleted the file" - ); - assert_eq!( - (tombstone.sequence_number, tombstone.file_sequence_number), - (carried.sequence_number, carried.file_sequence_number), - "the tombstone must keep its original sequence numbers" - ); - let entries = current_manifest_entries(&table).await; assert!( - entries.contains(&(ManifestStatus::Deleted, "test/b.parquet".to_string())), - "the second overwrite's own delete must be a tombstone, entries: {entries:?}" + !entries.iter().any( + |(status, path)| path == "test/a.parquet" && *status != ManifestStatus::Deleted + ), + "the first overwrite's delete must not come back, entries: {entries:?}" ); } @@ -736,37 +429,6 @@ mod tests { assert_eq!(original_spec_id, rewritten_file.partition_spec_id); } - #[tokio::test] - async fn test_overwrite_preserves_delete_only_manifest() { - let catalog = new_memory_catalog().await; - let table = make_v3_minimal_table_in_catalog(&catalog).await; - let spec_id = table.metadata().default_partition_spec_id(); - - let file_a = test_data_file("test/a.parquet", spec_id); - let tx = Transaction::new(&table); - let action = tx.fast_append().add_data_files(vec![file_a.clone()]); - let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - - let tx = Transaction::new(&table); - let action = tx - .overwrite() - .add_data_files(vec![test_data_file("test/b.parquet", spec_id)]) - .delete_data_files(vec![file_a.clone()]); - let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - - let tx = Transaction::new(&table); - let action = tx - .overwrite() - .add_data_files(vec![test_data_file("test/c.parquet", spec_id)]); - let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - - let entries = current_manifest_entries(&table).await; - assert!( - entries.contains(&(ManifestStatus::Deleted, "test/a.parquet".to_string())), - "the delete-only manifest must survive a later overwrite, entries: {entries:?}" - ); - } - #[tokio::test] async fn test_overwrite_counts_a_duplicated_delete_path_once() { let catalog = new_memory_catalog().await; @@ -803,46 +465,6 @@ mod tests { ); } - #[tokio::test] - async fn test_rewritten_manifest_is_named_after_the_commit_uuid() { - let catalog = new_memory_catalog().await; - let table = make_v3_minimal_table_in_catalog(&catalog).await; - let spec_id = table.metadata().default_partition_spec_id(); - - let file_a = test_data_file("test/a.parquet", spec_id); - let tx = Transaction::new(&table); - let action = tx.fast_append().add_data_files(vec![file_a.clone()]); - let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - - let commit_uuid = Uuid::now_v7(); - let tx = Transaction::new(&table); - let action = tx - .overwrite() - .set_commit_uuid(commit_uuid) - .add_data_files(vec![test_data_file("test/b.parquet", spec_id)]) - .delete_data_files(vec![file_a.clone()]); - let table = action.apply(tx).unwrap().commit(&catalog).await.unwrap(); - - let prefix = format!( - "{}/{}-m", - table.metadata().metadata_location().unwrap(), - commit_uuid - ); - let snapshot = table.metadata().current_snapshot().unwrap(); - let manifest_list = table.manifest_list_reader(snapshot).load().await.unwrap(); - assert_eq!(2, manifest_list.entries().len()); - for manifest_file in manifest_list.entries() { - let path = &manifest_file.manifest_path; - let counter = path - .strip_prefix(&prefix) - .and_then(|rest| rest.strip_suffix(".avro")); - assert!( - counter.is_some_and(|c| !c.is_empty() && c.chars().all(|c| c.is_ascii_digit())), - "manifest path must be {prefix}.avro, got {path}" - ); - } - } - #[tokio::test] async fn test_delete_only_overwrite_reports_a_delete_operation() { let catalog = new_memory_catalog().await; From 181f83417d5982ba87781b5e964f17889972eee2 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Fri, 2 Oct 2026 20:39:39 +0200 Subject: [PATCH 32/35] Fit the shared error on one line --- crates/iceberg/src/transaction/snapshot.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index d9aa05f90c..1c144fe936 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -155,9 +155,7 @@ impl<'a> SnapshotProducer<'a> { pub(crate) fn validate_added_data_files(&self) -> Result<()> { for data_file in &self.added_data_files { if data_file.content_type() != crate::spec::DataContentType::Data { - return Err(invalid_data!( - "Only data content type is allowed" - )); + return Err(invalid_data!("Only data content type is allowed")); } // Check if the data file partition spec id matches the table default partition spec id. if self.table.metadata().default_partition_spec_id() != data_file.partition_spec_id { From 683fae4fd1b4c43d44e699ca13cb872bf10a63d1 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Tue, 13 Oct 2026 14:07:32 +0200 Subject: [PATCH 33/35] Check that deletion summaries match manifest entries --- crates/iceberg/src/transaction/snapshot.rs | 36 ++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 0a64594422..c301750012 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -120,6 +120,7 @@ pub(crate) struct SnapshotProducer<'a> { commit_uuid: Uuid, snapshot_properties: HashMap, added_data_files: Vec, + /// Must match the data files marked Deleted by this commit, including their metrics. deleted_data_files: Vec, // Numbers this commit's manifest files. Atomic so `new_manifest_writer` can take `&self`. manifest_counter: AtomicU64, @@ -377,6 +378,18 @@ impl<'a> SnapshotProducer<'a> { // # TODO // Support process delete entries. + debug_assert_eq!( + manifest_files + .iter() + .filter(|m| m.added_snapshot_id == self.snapshot_id + && m.content == ManifestContentType::Data) + .filter_map(|m| m.deleted_files_count) + .map(u64::from) + .sum::(), + self.deleted_data_files.len() as u64, + "deleted files must match this commit's manifest entries" + ); + let manifest_files = manifest_process.process_manifests(self, manifest_files); Ok(manifest_files) } @@ -643,6 +656,29 @@ mod tests { assert_eq!(properties.get("removed-files-size").unwrap(), "100"); } + #[cfg(debug_assertions)] + #[tokio::test] + #[should_panic(expected = "deleted files must match this commit's manifest entries")] + async fn test_commit_rejects_unrecorded_deletions() { + let table = make_v2_minimal_table(); + let deleted = DataFileBuilder::default() + .content(DataContentType::Data) + .file_path("test/deleted.parquet".to_string()) + .file_format(DataFileFormat::Parquet) + .file_size_in_bytes(100) + .record_count(2) + .partition_spec_id(table.metadata().default_partition_spec_id()) + .partition(Struct::from_iter([Some(Literal::long(300))])) + .build() + .unwrap(); + SnapshotProducer::new(&table, Uuid::now_v7(), HashMap::new(), vec![], vec![ + deleted, + ]) + .commit(DeleteOperation, DefaultManifestProcess) + .await + .unwrap(); + } + #[tokio::test] async fn test_new_manifest_writer_uses_the_given_schema_and_spec() { let table = make_v2_minimal_table(); From 5f3693808baeab1f04e4cdc295963a5326f76e28 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Tue, 13 Oct 2026 14:07:56 +0200 Subject: [PATCH 34/35] Format dropped-source integer partition values --- crates/iceberg/src/spec/partition.rs | 47 +++++++++++++++++++++++++ crates/iceberg/src/spec/values/datum.rs | 8 +++-- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/spec/partition.rs b/crates/iceberg/src/spec/partition.rs index 17aa3de968..fa8de99a59 100644 --- a/crates/iceberg/src/spec/partition.rs +++ b/crates/iceberg/src/spec/partition.rs @@ -1913,6 +1913,53 @@ mod tests { assert!(!spec.has_sequential_ids()); } + #[test] + fn test_partition_to_path_with_dropped_source_column() { + for (source_type, transform, value, expected) in [ + ( + PrimitiveType::Int, + Transform::Identity, + Literal::int(1), + "p=1", + ), + ( + PrimitiveType::Timestamp, + Transform::Identity, + Literal::long(1), + "p=1", + ), + ( + PrimitiveType::Int, + Transform::Truncate(10), + Literal::int(10), + "p=10", + ), + ] { + let schema = Schema::builder() + .with_fields(vec![ + NestedField::required(1, "kept", PrimitiveType::Int.into()).into(), + NestedField::required(2, "dropped", source_type.into()).into(), + ]) + .build() + .unwrap(); + let spec = PartitionSpec::builder(schema) + .add_partition_field("dropped", "p", transform) + .unwrap() + .build() + .unwrap(); + let current_schema = Schema::builder() + .with_fields(vec![ + NestedField::required(1, "kept", PrimitiveType::Int.into()).into(), + ]) + .build() + .unwrap(); + assert_eq!( + spec.partition_to_path(&Struct::from_iter([Some(value)]), current_schema.into()), + expected + ); + } + } + #[test] fn test_partition_to_path() { let schema = Schema::builder() diff --git a/crates/iceberg/src/spec/values/datum.rs b/crates/iceberg/src/spec/values/datum.rs index 57d9675736..7fa54954c8 100644 --- a/crates/iceberg/src/spec/values/datum.rs +++ b/crates/iceberg/src/spec/values/datum.rs @@ -294,8 +294,12 @@ impl Display for Datum { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match (&self.r#type, &self.literal) { (_, PrimitiveLiteral::Boolean(val)) => write!(f, "{val}"), - (PrimitiveType::Int, PrimitiveLiteral::Int(val)) => write!(f, "{val}"), - (PrimitiveType::Long, PrimitiveLiteral::Long(val)) => write!(f, "{val}"), + (PrimitiveType::Int | PrimitiveType::Unknown, PrimitiveLiteral::Int(val)) => { + write!(f, "{val}") + } + (PrimitiveType::Long | PrimitiveType::Unknown, PrimitiveLiteral::Long(val)) => { + write!(f, "{val}") + } (_, PrimitiveLiteral::Float(val)) => write!(f, "{val}"), (_, PrimitiveLiteral::Double(val)) => write!(f, "{val}"), (PrimitiveType::Date, PrimitiveLiteral::Int(val)) => { From 543660f0d42c210ce7d44488cc2f146680139128 Mon Sep 17 00:00:00 2001 From: Jegor van Opdorp Date: Tue, 13 Oct 2026 14:09:32 +0200 Subject: [PATCH 35/35] Validate overwrite deletions before building their summary --- crates/iceberg/src/transaction/overwrite.rs | 278 ++++++++++++++++++-- crates/iceberg/src/transaction/snapshot.rs | 41 +-- 2 files changed, 272 insertions(+), 47 deletions(-) diff --git a/crates/iceberg/src/transaction/overwrite.rs b/crates/iceberg/src/transaction/overwrite.rs index 64e4d6f284..0577eb81ae 100644 --- a/crates/iceberg/src/transaction/overwrite.rs +++ b/crates/iceberg/src/transaction/overwrite.rs @@ -87,6 +87,13 @@ impl OverwriteAction { #[async_trait] impl TransactionAction for OverwriteAction { async fn commit(self: Arc, table: &Table) -> Result { + let mut deleted_files = HashMap::new(); + for file in &self.deleted_data_files { + if file.content_type() != crate::spec::DataContentType::Data { + return Err(invalid_data!("Only data content type is allowed")); + } + deleted_files.entry(file.file_path.as_str()).or_insert(file); + } let snapshot_producer = SnapshotProducer::new( table, self.commit_uuid.unwrap_or_else(Uuid::now_v7), @@ -103,11 +110,7 @@ impl TransactionAction for OverwriteAction { let operation = OverwriteOperation { has_added_data_files: !self.added_data_files.is_empty(), - deleted_file_paths: self - .deleted_data_files - .iter() - .map(|f| f.file_path.clone()) - .collect(), + deleted_files, }; snapshot_producer .commit(operation, DefaultManifestProcess) @@ -115,17 +118,14 @@ impl TransactionAction for OverwriteAction { } } -struct OverwriteOperation { +struct OverwriteOperation<'a> { has_added_data_files: bool, - deleted_file_paths: HashSet, + deleted_files: HashMap<&'a str, &'a DataFile>, } -impl SnapshotProduceOperation for OverwriteOperation { +impl SnapshotProduceOperation for OverwriteOperation<'_> { fn operation(&self) -> Operation { - match ( - self.has_added_data_files, - !self.deleted_file_paths.is_empty(), - ) { + match (self.has_added_data_files, !self.deleted_files.is_empty()) { (true, true) => Operation::Overwrite, (false, true) => Operation::Delete, // Also a properties-only commit, the workaround from #1548. @@ -164,19 +164,31 @@ impl SnapshotProduceOperation for OverwriteOperation { { continue; } - if self.deleted_file_paths.is_empty() { + if manifest_file.content == crate::spec::ManifestContentType::Deletes + || self.deleted_files.is_empty() + { manifests.push(manifest_file.clone()); continue; } let manifest = table.manifest_reader().read(manifest_file).await?; - let deletes: Vec<&str> = manifest - .entries() - .iter() - .filter(|entry| entry.is_alive()) - .filter_map(|entry| self.deleted_file_paths.get(entry.file_path())) - .map(String::as_str) - .collect(); + let mut deletes = vec![]; + for entry in manifest.entries().iter().filter(|entry| entry.is_alive()) { + if let Some(requested) = self.deleted_files.get(entry.file_path()) { + let actual = entry.data_file(); + if requested.record_count != actual.record_count + || requested.file_size_in_bytes != actual.file_size_in_bytes + || requested.partition_spec_id != actual.partition_spec_id + || requested.partition != actual.partition + { + return Err(invalid_data!( + "Delete file metadata does not match the table: {}", + entry.file_path() + )); + } + deletes.push(requested.file_path.as_str()); + } + } if deletes.is_empty() { manifests.push(manifest_file.clone()); continue; @@ -190,7 +202,7 @@ impl SnapshotProduceOperation for OverwriteOperation { )?; // Like Java, only live entries are carried into the rewrite. for entry in manifest.entries().iter().filter(|entry| entry.is_alive()) { - if self.deleted_file_paths.contains(entry.file_path()) { + if self.deleted_files.contains_key(entry.file_path()) { writer.add_delete_entry(entry.as_ref().clone())?; } else { writer.add_existing_entry(entry.as_ref().clone())?; @@ -201,9 +213,9 @@ impl SnapshotProduceOperation for OverwriteOperation { } let mut missing: Vec<&str> = self - .deleted_file_paths - .iter() - .map(String::as_str) + .deleted_files + .keys() + .copied() .filter(|path| !matched.contains(path)) .collect(); if !missing.is_empty() { @@ -224,9 +236,9 @@ mod tests { use crate::memory::tests::new_memory_catalog; use crate::spec::{ - DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, ManifestStatus, - Operation, PrimitiveType, SnapshotRef, Struct, Transform, Type, UnboundPartitionField, - UnboundPartitionSpec, + DataContentType, DataFile, DataFileBuilder, DataFileFormat, Literal, ManifestListWriter, + ManifestStatus, ManifestWriterBuilder, Operation, PrimitiveType, SnapshotRef, Struct, + Transform, Type, UnboundPartitionField, UnboundPartitionSpec, }; use crate::table::Table; use crate::transaction::tests::make_v3_minimal_table_in_catalog; @@ -531,4 +543,216 @@ mod tests { "{err}" ); } + + #[tokio::test] + async fn test_overwrite_rejects_incorrect_delete_metrics() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + let file = test_data_file("test/a.parquet", spec_id); + let tx = Transaction::new(&table); + let table = tx + .fast_append() + .add_data_files(vec![file.clone()]) + .apply(tx) + .unwrap() + .commit(&catalog) + .await + .unwrap(); + for incorrect in [ + DataFile { + record_count: 99, + ..file.clone() + }, + DataFile { + file_size_in_bytes: 999, + ..file.clone() + }, + DataFile { + partition: Struct::from_iter([Some(Literal::long(999))]), + ..file.clone() + }, + DataFile { + partition_spec_id: 99, + ..file.clone() + }, + DataFile { + partition: Struct::from_iter([Some(Literal::string("invalid"))]), + ..file.clone() + }, + ] { + let action = Transaction::new(&table) + .overwrite() + .delete_data_files(vec![incorrect]); + let err = Arc::new(action).commit(&table).await.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::DataInvalid); + assert!( + err.message() + .contains("Delete file metadata does not match") + ); + } + } + + #[tokio::test] + async fn test_overwrite_rejects_non_data_deletions() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + for content in [ + DataContentType::PositionDeletes, + DataContentType::EqualityDeletes, + ] { + let file = DataFile { + content, + ..test_data_file("test/deletes.parquet", spec_id) + }; + let action = Transaction::new(&table) + .overwrite() + .delete_data_files(vec![file]); + let err = Arc::new(action).commit(&table).await.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::DataInvalid); + assert!(err.message().contains("Only data content type is allowed")); + } + } + + #[tokio::test] + async fn test_overwrite_preserves_v3_row_lineage() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + let a = test_data_file("test/a.parquet", spec_id); + let b = test_data_file("test/b.parquet", spec_id); + let c = test_data_file("test/c.parquet", spec_id); + let tx = Transaction::new(&table); + let table = tx + .fast_append() + .add_data_files(vec![a, b.clone(), c]) + .apply(tx) + .unwrap() + .commit(&catalog) + .await + .unwrap(); + let snapshot = table.metadata().current_snapshot().unwrap(); + let manifests = table.manifest_list_reader(snapshot).load().await.unwrap(); + let manifest = table + .manifest_reader() + .read(&manifests.entries()[0]) + .await + .unwrap(); + let original_ids: Vec<_> = manifest + .entries() + .iter() + .map(|e| (e.file_path().to_string(), e.data_file().first_row_id)) + .collect(); + assert!(original_ids.iter().all(|(_, id)| id.is_some())); + let next_row_id = table.metadata().next_row_id(); + let replacement = test_data_file("test/d.parquet", spec_id); + let tx = Transaction::new(&table); + let table = tx + .overwrite() + .delete_data_files(vec![b]) + .add_data_files(vec![replacement]) + .apply(tx) + .unwrap() + .commit(&catalog) + .await + .unwrap(); + let manifests = table + .manifest_list_reader(table.metadata().current_snapshot().unwrap()) + .load() + .await + .unwrap(); + let mut actual_ids = vec![]; + for file in manifests.entries() { + let manifest = table.manifest_reader().read(file).await.unwrap(); + actual_ids.extend( + manifest + .entries() + .iter() + .filter(|e| e.is_alive()) + .map(|e| (e.file_path().to_string(), e.data_file().first_row_id)), + ); + } + for (path, id) in original_ids.iter().filter(|(p, _)| p != "test/b.parquet") { + assert!(actual_ids.contains(&(path.clone(), *id))); + } + // The rewritten manifest reserves IDs for its existing rows; their explicit IDs stay unchanged. + assert!(actual_ids.contains(&( + "test/d.parquet".to_string(), + Some((next_row_id + 2).try_into().unwrap()) + ))); + assert_eq!(table.metadata().next_row_id(), next_row_id + 3); + } + + #[tokio::test] + async fn test_overwrite_rejects_a_path_in_a_delete_manifest() { + let catalog = new_memory_catalog().await; + let table = make_v3_minimal_table_in_catalog(&catalog).await; + let spec_id = table.metadata().default_partition_spec_id(); + let tx = Transaction::new(&table); + let table = tx + .fast_append() + .add_data_files(vec![test_data_file("test/a.parquet", spec_id)]) + .apply(tx) + .unwrap() + .commit(&catalog) + .await + .unwrap(); + let snapshot = table.metadata().current_snapshot().unwrap(); + let file = test_data_file("test/deletes.parquet", spec_id); + let output = table + .file_io() + .new_output("test/delete-manifest.avro") + .unwrap(); + let mut writer = ManifestWriterBuilder::new( + output, + Some(snapshot.snapshot_id()), + table.metadata().current_schema().clone(), + table.metadata().default_partition_spec().as_ref().clone(), + ) + .build_v3_deletes(); + writer + .add_file( + DataFile { + content: DataContentType::PositionDeletes, + ..file.clone() + }, + snapshot.sequence_number(), + ) + .unwrap(); + let delete_manifest = writer.write_manifest_file().await.unwrap(); + let mut manifests: Vec<_> = table + .manifest_list_reader(snapshot) + .load() + .await + .unwrap() + .consume_entries() + .into_iter() + .collect(); + manifests.push(delete_manifest); + let mut writer = ManifestListWriter::v3( + table + .file_io() + .new_output(snapshot.manifest_list()) + .unwrap() + .writer() + .await + .unwrap(), + snapshot.snapshot_id(), + snapshot.parent_snapshot_id(), + snapshot.sequence_number(), + Some(table.metadata().next_row_id()), + ); + writer.add_manifests(manifests.into_iter()).unwrap(); + writer.close().await.unwrap(); + let action = Transaction::new(&table) + .overwrite() + .delete_data_files(vec![file]); + let err = Arc::new(action).commit(&table).await.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::DataInvalid); + assert!( + err.message() + .contains("Missing required files to delete: test/deletes.parquet") + ); + } } diff --git a/crates/iceberg/src/transaction/snapshot.rs b/crates/iceberg/src/transaction/snapshot.rs index 1c144fe936..1c69f07e34 100644 --- a/crates/iceberg/src/transaction/snapshot.rs +++ b/crates/iceberg/src/transaction/snapshot.rs @@ -21,7 +21,6 @@ use std::sync::atomic::{AtomicU64, Ordering}; use futures::TryStreamExt; use futures::stream::FuturesUnordered; -use itertools::Itertools; use uuid::Uuid; use crate::error::{Result, invalid_data}; @@ -139,19 +138,25 @@ impl<'a> SnapshotProducer<'a> { snapshot_id: Self::generate_unique_snapshot_id(table), commit_uuid, snapshot_properties, - // Collapsed to the first file per path, so a manifest never references a file twice. - added_data_files: added_data_files - .into_iter() - .unique_by(|f| f.file_path.clone()) - .collect(), - deleted_data_files: deleted_data_files - .into_iter() - .unique_by(|f| f.file_path.clone()) - .collect(), + added_data_files: Self::dedupe_data_files(added_data_files), + deleted_data_files: Self::dedupe_data_files(deleted_data_files), manifest_counter: AtomicU64::new(0), } } + fn dedupe_data_files(files: Vec) -> Vec { + let mut seen = HashSet::with_capacity(files.len()); + let keep: Vec = files + .iter() + .map(|f| seen.insert(f.file_path.as_str())) + .collect(); + files + .into_iter() + .zip(keep) + .filter_map(|(file, keep)| keep.then_some(file)) + .collect() + } + pub(crate) fn validate_added_data_files(&self) -> Result<()> { for data_file in &self.added_data_files { if data_file.content_type() != crate::spec::DataContentType::Data { @@ -355,7 +360,7 @@ impl<'a> SnapshotProducer<'a> { &mut self, snapshot_produce_operation: &OP, manifest_process: &MP, - ) -> Result> { + ) -> Result<(Vec, Summary)> { // Assert current snapshot producer contains new content to add to new snapshot. // // TODO: Allowing snapshot property setup with no added data files is a workaround. @@ -372,6 +377,9 @@ impl<'a> SnapshotProducer<'a> { } let existing_manifests = snapshot_produce_operation.existing_manifest(self).await?; + let summary = self.summary(snapshot_produce_operation).map_err(|err| { + Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err) + })?; let mut manifest_files = existing_manifests; // Process added entries. @@ -384,7 +392,7 @@ impl<'a> SnapshotProducer<'a> { // Support process delete entries. let manifest_files = manifest_process.process_manifests(self, manifest_files); - Ok(manifest_files) + Ok((manifest_files, summary)) } // Returns a `Summary` of the current snapshot @@ -507,14 +515,7 @@ impl<'a> SnapshotProducer<'a> { ), }; - // Calling self.summary() before self.produce_manifests() is important because self.added_data_files - // will be set to an empty vec after self.produce_manifests() returns, resulting in an empty summary - // being generated. - let summary = self.summary(&snapshot_produce_operation).map_err(|err| { - Error::new(ErrorKind::Unexpected, "Failed to create snapshot summary.").with_source(err) - })?; - - let new_manifests = self + let (new_manifests, summary) = self .produce_manifests(&snapshot_produce_operation, &process) .await?;