Repository navigation
Conversation
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.
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 <noreply@anthropic.com>
Import Manifest and DataFile at the top instead of using fully-qualified crate::spec:: paths inline in a signature and a test helper.
# Conflicts: # crates/iceberg/public-api.txt # crates/iceberg/src/transaction/append.rs # crates/iceberg/src/transaction/mod.rs
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.
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()).
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 apache#2545 for apache#2148, applied to the no-deletes fast path and to the rewrite loop.
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.
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.
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.
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 (apache#2367) precisely so the two cannot be confused.
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.
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.
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.
Regenerated with make generate-public-api (cargo-public-api 0.51.0); the snapshot was missing OverwriteAction and its builder methods.
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.
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.
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.
…hot summary Co-authored-by: Marco Kroll <216147982+mkroll-db@users.noreply.github.com>
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
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.
Java and PyIceberg derive totals from the files an operation marks deleted, with no truncation path.
Drops the removed_data_files hook and the manifest-first reorder, and truncation defaults back to every overwrite.
Overwrite gets the dedup fast append already had, and the shared error no longer says fast append.
…a and pyiceberg do Drops add_tombstone_entry, the removed-files bookkeeping and the duplicate tests.
|
@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
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
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.
|
@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? |
# Conflicts: # crates/iceberg/public-api.txt # crates/iceberg/src/transaction/append.rs # crates/iceberg/src/transaction/snapshot.rs
|
@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. |
This branch has not been deployed
5 participants
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:
SnapshotProducer::new_manifest_writer, carrying only live entries, like Java'sManifestFilterManager, so an earlier delete can't come back.public-api.txt.It includes the producer changes from #3253; those drop out of this diff once that merges. It will move onto
MergingSnapshotProducerwhen #3046 lands.Are these changes tested?
Tests in
transaction::overwrite::testsread 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 warningsare 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.