[jopdorp]

Which issue does this PR close?

Part of #2186.

What changes are included in this PR?

Continues #2185 by @glitchy, with their commits kept as they were, and addresses @laskoviymishka's review on top:

• A manifest holding a deleted path is rewritten under its own schema and spec through SnapshotProducer::new_manifest_writer, carrying only live entries, like Java's ManifestFilterManager, so an earlier delete can't come back.
• Deletes must match a live data-file entry and its summary metrics; non-data inputs and missing paths are rejected. Delete-file manifests are carried unchanged. Added and deleted files are collapsed by path without cloning the path strings.
• The operation is delete, append or overwrite depending on what the commit does, and only the listed files are replaced, so the totals carry over.
• Merged main and regenerated public-api.txt.

It includes the producer changes from #3253; those drop out of this diff once that merges. It will move onto MergingSnapshotProducer when #3046 lands.

Are these changes tested?

Tests in transaction::overwrite::tests read the committed manifests back: overwrite with deletes, delete-only reports Delete, a second overwrite doesn't resurrect an earlier delete, schema and spec evolution, a missing delete is rejected, a duplicated delete path counts once, incorrect deletion metadata and delete-file targets are rejected, and V3 row lineage survives a rewrite. cargo test -p iceberg --lib, the public API check, fmt and clippy with -D warnings are clean.

AI Disclosure

The review fixes, their tests and the merges were drafted with an AI coding assistant, then reviewed, run and checked against the Java and pyiceberg implementations by us. Worth a close look: which operation a commit reports, and that rewritten manifests drop earlier tombstones like Java does.

[laskoviymishka]

This picks up cleanly from the #2185 round — the things I flagged there have landed: rewritten manifests now stamp the schema and partition-spec they were written with, delete-only manifests survive a later overwrite, the summary reports the files actually marked deleted (and we reject deleting a path the table doesn't have, matching BaseOverwriteFiles), and the rewrite routes through new_manifest_writer so the commit-uuid naming and encryption come along. Test coverage on all of that is solid.

On the two things you flagged: the operation mapping is right and lines up with BaseOverwriteFiles — adds+deletes → Overwrite, deletes-only → Delete, add-only → Append; the only wrinkle is a property-only overwrite falling through to Append, which is the existing #1548 workaround rather than anything new (noted inline). And the add_deleted_entry → add_tombstone_entry rename is fine by me — it's pub(crate), so nothing moves on the published surface; the only thing there is the doc pointing at dead code.

The one thing I'd want fixed before this merges is first_row_id on the rewritten V3 manifests: write_manifest_file() returns None and add_manifests then stamps a fresh id above the row IDs the surviving entries actually carry, so row-ID-range pruning — and Java/PyIceberg readers — can skip real rows. It's latent until MOR lands on top, but I'd rather not bake it in. Details inline.

Two more I'd sort out in the same pass — not correctness, but I wouldn't ship them: the produce-then-summarize reorder means write_added_manifest deep-clones the whole added-file vec on every commit, and that path is shared with FastAppendAction, so plain appends pay for it too; and the new deleted_data_files on SnapshotProducer is dead weight — cloned in only to answer is_empty(), and it reads like it drives the summary when it doesn't. Both trace back to the same reorder, so they're natural to fix together. Smaller stuff (intra-batch dedup, the "fast append" error string) is inline.

Carry the first_row_id through and tidy up the clone/field, and I'm happy to take another pass.

[jopdorp]

@laskoviymishka reworked on top of the reshaped #3253: one pass over the manifests, only live entries carried like Java, dedup in the producer. Could you take another look?

[laskoviymishka]

The round-1 cleanups all landed — write_added_manifest uses mem::take now, deleted_data_files feeds the summary instead of being dead weight, and the error-string / dead-doc / intra-batch-dedup nits are all sorted. Thanks for working through those.

On first_row_id: I went back through it and I think my round-1 call was wrong, so I'll name that directly. The per-entry data_file.first_row_id survives the rewrite — the reader materializes it for live entries, add_existing_entry carries it through, and _serde writes it back — and the fresh manifest-level first_row_id only fills null ids, which a rewrite of existing files never has. That lines up with how Java's V3 writer assigns first_row_id, so I'm withdrawing the corruption concern. The one piece still open from last round is a regression test: the behavior rests entirely on that reader→serde chain and nothing in the suite asserts row lineage, so I'd want a test that appends A/B/C on a V3 table, overwrite-deletes B, and checks A and C keep their original first_row_id with next_row_id advancing as expected.

Two things I didn't catch last round that I'd want before this merges, both inline. The deleted-file summary and the running totals are built from the DataFiles the caller passes, not the manifest entries we match — so a caller with the right path but a wrong record_count or size writes a snapshot whose summary and totals disagree with its own manifests, and the new update_totals guard mostly just hides the negative case. And delete_data_files never checks content type while existing_manifest walks Deletes manifests too, so a path that matches a delete file or DV gets tombstoned and silently stops applying its deletes — Java keeps those on separate paths for exactly this reason.

Fix those two and add the lineage test and I think we're there — this is close.

[jopdorp]

@laskoviymishka the deletion checks and V3 lineage test are in, along with the producer fixes. the library suite, fmt, clippy and public API check pass locally. could you take another look?

[jopdorp]

@laskoviymishka merged main now that #3253 is in, so the diff is only the overwrite action, and it's moved onto the new stateful TransactionAction trait. still passes the library suite, fmt, clippy and the public API check.

[laskoviymishka]

The round-2 asks are all in: non-data deletions are rejected and Deletes manifests pass through untouched, caller metadata is checked against the matched entry before the summary is built, and dedup borrows the paths. The V3 lineage test pins A and C to their original first_row_id, so that thread is closed.

One real thing left, and it's a panic. If an identity-partitioned int source was later promoted to long, overwrite-deleting a file written under the old spec hits unreachable!() in Datum's Display — the removed-file summary renders that partition with current_schema() and builds a (Long, Int) datum. The code is in main, but this PR is the first caller that sends old-spec files through it, so I'd fix it here; either option inline works, as long as there's a test that promotes the source before deleting.

The rest is small cleanup: document the strict-match contract on delete_data_files, name the mismatched field in its error, settle delete-plus-re-add of one path under check_duplicate, and add the positive Deletes-manifest test. Panic fix plus that cleanup and we're good to merge.

[jopdorp]

@laskoviymishka The partition panic is fixed. The deletion contract and errors are clearer, and the missing tests are in. Reusing a deleted path still requires disabling the duplicate check; that is now documented and tested. The library suite, formatting and clippy pass locally. Could you take another look?

[jopdorp]

@laskoviymishka the last piece of the 10-20 round is in too, deleted paths are excluded from the duplicate check in b106b0a. Ready for another look.

[laskoviymishka]

It's almost there. The round-3 panic fix and the check_duplicate exclusion are in, thanks. I said last round that would be enough to merge, but this pass turned up one new thing: a debug_assert_eq! panic when a deleted path is live in two Data manifests (inline on overwrite.rs). The rest are small doc and test nits. I'll do at least one more round, then I'd like @CTTY to look at the SnapshotProducer and FastAppend changes. Could the description also drop the #3253 paragraph now that it's merged?

[laskoviymishka]

Could we open an issue for this arm? With the scale gone, a dropped decimal source renders 123 for 1.23. That's harmless for changed-partition-count, but it lands in the partitions.* summary keys once write.summary.partition-limit is above 0. We settled on the Display arms last round, so this is only about tracking it.

[laskoviymishka]

Could the doc say which operation this reports and what it doesn't check? operation() returns Delete or Append for one-sided commits, and there's no row-filter mode or validation of concurrent commits. Java's default OverwriteFiles skips that validation too, so it's scope to state rather than a gap.

Suggested change

	/// 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`.
	/// Adds data files and removes listed data files by path.
	///
	/// The operation is `Overwrite` when files are both added and removed, otherwise `Delete` or
	/// `Append`. There is no row-filter mode, and files committed concurrently are not checked.

[laskoviymishka]

Could this error name the file and its content type? When several files go to delete_data_files, there's no telling which one was rejected. Also, crate::spec::DataContentType here and crate::spec::ManifestContentType at L185 could move into the use crate::spec::{…} import, though snapshot.rs writes the same path inline today.

[laskoviymishka]

Could these reads run concurrently, the way validate_duplicate_files does, or at least carry a TODO? Here every non-empty Data manifest is read one after another, with no pruning and no early exit once every delete is matched. With check_duplicate on, the same commit has already read each manifest once in that concurrent pass.

[laskoviymishka]

Could this check move into a small helper that returns Option<String>? The continue in the last else of the let initializer is easy to misread as falling through to the return Err, and the loop at L239 then filters the live entries a second time. The behavior is right as written. This is only about readability.

[laskoviymishka]

Could one of these tests run on a V2 table? Every test here uses make_v3_minimal_table_in_catalog, but V2 rewrites go through a different manifest builder. As I read it, nothing in the rewrite is V2-specific, so this is about coverage, not a suspected bug. The promotion test's catalog create at L610 shows how to pick the format version.

[laskoviymishka]

Could this assert the whole entry list? It also passes when a.parquet is missing entirely. As I read the rewrite loop, missing is the expected end state, since the second rewrite carries only live entries and drops a's tombstone. An assert_eq! against the expected list (no a.parquet, b.parquet Deleted, c and d live) would pin the delete and the tombstone drop together, like the new re-add test does.

[laskoviymishka]

Could both partition cases build the expected message from the two Structs, e.g. format!("partition: requested {:?}, table has {:?}", …)? This line hard-codes the derived Debug text, which changes on any rename in Struct or Literal. Meanwhile the Literal::string case at L752 only checks the partition: requested prefix.

[laskoviymishka]

Could we keep the ordering note main had? summary() still has to run before write_added_manifest, which takes added_data_files, and since the move nothing here says so.

Suggested change

	let summary = self.summary(snapshot_produce_operation).map_err(|err| {
	// Must run before `write_added_manifest`, which takes `added_data_files`.
	let summary = self.summary(snapshot_produce_operation).map_err(|err| {