Conversation
glitchy
changed the title
e8860f6 to
0b3158b
Compare
xanderbailey
left a comment
xanderbailey
left a comment
There was a problem hiding this comment.
Hey! This is exactly the PR I was about to implement but I was going to add the wiring for DataFusion also so you can do INSERT OVERWRITE I think it's not a lot of extra code to modify IcebergCommitExec to take InsertOp. I'm happy to add this is a follow-up PR if you were wanting to focus on the core iceberg part.
|
Gave it a go here glitchy#1 |
Thanks @xanderbailey. Yep I wanted to keep this PR focused on the core Iceberg transaction layer. A follow-up PR wiring it into DataFusion with |
Sounds good to me! |
timsaucer
left a comment
timsaucer
left a comment
There was a problem hiding this comment.
This is a nice PR!
I did run into a problem when I tried using it with my code. Specifically I was getting a panic when I would delete more files than I added during an overwrite. I wrote up a PR targeting your branch with a proposed solution: glitchy#2
|
@glitchy Ping on this. What do you think of my proposed solution for avoiding the panic I found? |
|
@glitchy is there anything I can do to help? Would love to get this over the line! |
|
Should we try to get https://github.com/apache/iceberg-rust/pull/2367/changes merged first and rebase this on it? |
## Summary Adds atomic overwrite snapshot support to RustyIceberg.jl, enabling callers to replace all (or a subset of) existing Parquet files with a new set in a single Iceberg `Operation::Overwrite` snapshot. **Depends on**: RelationalAI/iceberg-rust#76 (cherry-pick of upstream [apache/iceberg-rust#2185](apache/iceberg-rust#2185), which adds `OverwriteAction` to `iceberg-rust`). ## Changes ### FFI (`iceberg_rust_ffi/src/transaction.rs`) - `IcebergOverwriteAction` — accumulates added + deleted `DataFile` lists - `iceberg_overwrite_action_new` / `_free` - `iceberg_overwrite_action_add_data_files` — move new files into action - `iceberg_overwrite_action_delete_data_files` — move files-to-delete into action - `iceberg_overwrite_action_apply` — calls `Transaction::overwrite().apply()` - `iceberg_table_list_data_files` — async walk of manifest list to collect all live `DataFile` records from the current snapshot ### Julia bindings (`src/transaction.jl`) - `OverwriteAction` struct + constructor / `free_overwrite_action!` - `add_data_files(action, files)` / `delete_data_files(action, files)` - `apply(action, tx)` / `with_overwrite(f, tx)` convenience helper - `list_data_files(table) -> DataFiles` - All new symbols exported from `RustyIceberg` ### Tests (`test/overwrite_tests.jl`) Self-contained, no Docker — all tests use `mktempdir` + `catalog_create_memory`: - OverwriteAction lifecycle (new / free / double-free) - `list_data_files` on empty table - `list_data_files` after append - Overwrite replaces **all** existing files - Overwrite deletes only **explicitly listed** files; others survive intact - Overwrite add-only (no deletes) produces a new snapshot - Two sequential overwrites converge correctly - Error handling: freed action, null DataFiles, committed (consumed) transaction ## Usage ```julia # Replace all existing files atomically old_files = list_data_files(table) new_files = RustyIceberg.with_data_file_writer(table) do w write(w, new_data) end updated_table = with_transaction(table, catalog) do tx with_overwrite(tx) do action add_data_files(action, new_files) delete_data_files(action, old_files) end end ``` ## Test plan - [ ] `make run-containers && make test` passes (all overwrite testsets green) - [ ] Existing test suite unaffected (27875 pre-existing tests still pass) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Richard Gankema <richardgankema@gmail.com>
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
0b3158b to
7c1984b
Compare
|
My apologies for the delay @xanderbailey @timsaucer @c-thiel --life has kept me away. Continuing this push for full Copy-on-Write (CoW) and Merge-on-Read (MoR) support. Rebased onto current main and folded in the community fixes:
|
7c1984b to
3d0b57f
Compare
|
@timsaucer @xanderbailey updated and ready for review. |
|
Hey @glitchy, Full SCD1 upsert scenario: 3-row initial load, incoming batch with 2 updates + 1 new insert. All assertions passed, snapshot.operation = Overwrite, summary correctly reports added-data-files=1 / deleted-data-files=1. One thing worth to note: the Arrow schema passed to the Parquet writer must be derived via table.metadata().current_schema().as_ref().try_into() — a manually built ArrowSchema silently drops the field ID metadata and fails with DataInvalid => Field id N not found in struct array. Not a bug in this PR (existing writer behavior), just easy for downstream consumers to trip over. API shape works well for keyed upsert use cases. |
|
@malon64 thanks for the downstream validation as well as for the heads up on the parquet writer. I went to create a guard for it, but then realized it was causing regressions in existing write paths. |
timsaucer
left a comment
timsaucer
left a comment
There was a problem hiding this comment.
LGTM, but I don't have approval authority in this repo.
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>
3d0b57f to
5493b3a
Compare
|
@c-thiel --realizing I never engaged with your comment--so sorry. I took a look at #2367 and it seems to have stalled--it's still open with requested changes and hasn't been updated in about a month. I'd rather not block this indefinitely on it: the two PRs are complementary rather than overlapping. #2367 adds delete-manifest writing to the |
Import Manifest and DataFile at the top instead of using fully-qualified crate::spec:: paths inline in a signature and a test helper.
|
Hi @glitchy — review of the overwrite path on the latest branch, with regression tests and cross-validation against the Java SDK + spec. Follow-up PR with fixes: glitchy#3 Bugs1. 2. Already- |
|
@timsaucer @c-thiel — following up on my review above (#2185 (comment)): two bugs in the overwrite path, with fixes + regression tests at glitchy#3:
@glitchy has been inactive since June 30 (before I posted the review), so the fork PR has been sitting without a reviewer. How would the team prefer to proceed?
Happy to do (3) if there's agreement — this feature looks close, and it'd be a shame for it to stall on these two issues. (Separately, the branch now conflicts with main, so it needs a rebase in any case.) |
|
@blackmwk @CTTY — could you help with the ownership/process decision here? The original author has been inactive since June 30, the branch now conflicts with main, and the two correctness fixes with regression tests are ready in glitchy#3. Would you prefer that I open a successor PR preserving @glitchy’s commits/authorship and apply the fixes on top, or should we continue waiting for the original branch to be updated? @CTTY, since #2367 also touches SnapshotProducer delete processing, your view on whether the two should remain independent would be especially helpful. |
|
@u70b3 I'd like to take this up |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Really nice first cut at CoW overwrite — the OverwriteAction/OverwriteOperation split mirrors FastAppend cleanly, and the scope (explicit file-path deletes, no predicate overwrite yet) is the right place to start. I'd hold it before merging though, because two correctness issues here silently corrupt table state, and everything downstream — including @xanderbailey's INSERT OVERWRITE wiring — will build on top of this.
The one I'd fix first: a second overwrite over a manifest that already holds a Deleted entry resurrects that file as live data — the else branch re-adds it via add_existing_entry, which forces status back to Existing. Closely related, the rewritten manifest is stamped with the table's current schema and default partition spec rather than the original manifest's, so after any schema/partition evolution the Avro header carries the wrong schema-id/partition-spec-id and Java/PyIceberg will misread partitions. Both of these were caught independently across the review.
Things I'd like to settle in this PR before the follow-ups build on it:
- guard the rewrite loop on
is_alive()so prior tombstones survive (this needsadd_deleted_entryto preserve/stampsnapshot_id— worth pinning that contract down) - derive schema + partition spec from the manifest being rewritten, not the table default
- keep delete-only manifests instead of filtering them out (the #2148 fix FastAppend already made)
- straighten out the deletion summary counts: count what the rewrite actually removed, skip the truncate path for partial overwrites, and use saturating subtraction so the delete-heavy case @timsaucer hit doesn't underflow again
- route rewritten manifests through
metadata_location()+ the encryption manager, and name them with the commit uuid - regression tests for the two-overwrite (resurrection) path and a delete-only overwrite
One logistics note: as-is this doesn't build against main — it depends on the 6-arg SnapshotProducer::new / key_metadata that aren't in main yet, so I think there's a prerequisite PR to land first.
@xanderbailey's DataFusion wiring is a great follow-up and fine to keep out of scope here. Once the correctness items above are settled I'm happy to take another pass and approve.
| writer.add_deleted_entry(deleted)?; | ||
| } else { | ||
| let cloned: ManifestEntry = (**entry).clone(); | ||
| writer.add_existing_entry(cloned)?; |
There was a problem hiding this comment.
This else branch resurrects previously-deleted files. add_existing_entry forces status = Existing (writer.rs:380), and an entry that was already Deleted in a prior overwrite isn't a live match here, so it falls through and comes back as live data.
Concretely: an overwrite deletes A / adds C, so M1 is rewritten with A=Deleted, B=Existing. A later overwrite that touches B rewrites that manifest again — A isn't in the new delete set, hits this branch, and reappears as Existing.
Java's filterManifestWithDeletedFiles only ever re-adds liveEntries() (Added/Existing); already-Deleted entries are dropped. I'd guard on entry.is_alive() first — skip or re-add tombstones as Deleted, add_deleted_entry for paths in the delete set, add_existing_entry only for the rest. Re-adding tombstones needs the original snapshot_id preserved, which ties into the add_deleted_entry note below. wdyt?
| output_file, | ||
| Some(self.snapshot_id), | ||
| manifest_file.key_metadata.clone(), | ||
| table.metadata().current_schema().clone(), |
There was a problem hiding this comment.
We're stamping the rewritten manifest with the table's current schema and default partition spec, but the entries were written under the original manifest's schema/spec. After any schema or partition evolution these differ, and the Avro header ends up with the wrong schema-id (505) and partition-spec-id (507).
Readers trust those ids to interpret partition values, so pruning and pushdown silently run against the wrong mapping — this is also how Java/PyIceberg will misread the table.
I'd pull both from the manifest itself: manifest.metadata().schema.clone() and manifest.metadata().partition_spec.clone() (or partition_spec_by_id(manifest_file.partition_spec_id)). Java does exactly this via newManifestWriter(reader.spec()).
| return Ok(manifest_list | ||
| .entries() | ||
| .iter() | ||
| .filter(|entry| entry.has_added_files() || entry.has_existing_files()) |
There was a problem hiding this comment.
This filter drops any manifest whose only entries are Deleted-status. FastAppendOperation.existing_manifest() hit exactly this and added || entry.has_deleted_files() (append.rs:142, referencing #2148) — dropping delete-only manifests lets the removed files reappear as live.
So overwrite().delete(A).add(B) followed by overwrite().add(C) (no deletes) would drop the manifest that recorded A as Deleted, and A comes back.
I'd mirror the append fix and add || manifest_file.has_deleted_files() here — and note the same guard is repeated in the deletes-path loop just below (line 178).
| ); | ||
| } | ||
|
|
||
| for data_file in &self.deleted_data_files { |
There was a problem hiding this comment.
Two things tangle up around the deletion counts here.
remove_file runs for every user-supplied deleted file regardless of whether it was actually found live in a manifest, so double-deletes or never-committed paths inflate deleted-data-files / deleted-records. I'd count from what the rewrite pass actually marked Deleted, and error if a delete path matched nothing (the existence validation Java does).
Separately, produce_manifests still calls summary() with truncate_full_table = (operation == Overwrite), so for a partial overwrite the truncate path zeroes the totals and reports deleted-data-files as the previous total rather than the k you deleted. test_overwrite_with_deleted_files only passes because it deletes all files (k == N). I'd gate truncate behind an operation hook that OverwriteOperation returns false for — with one caveat: once truncate is off, update_totals needs saturating subtraction, otherwise the delete-heavy case @timsaucer hit underflows again (the June rebase only avoided it because truncate capped removed at the previous total).
| table.metadata().location(), | ||
| Uuid::now_v7(), | ||
| ); | ||
| let output_file = table.file_io().new_output(&new_manifest_path)?; |
There was a problem hiding this comment.
rewrite_manifest always writes through a plain output_file, but new_manifest_writer in snapshot.rs branches on table.encryption_manager() and wraps it with em.encrypt(...) when present. For an encrypted table the rewritten manifest goes out in plaintext, and readers using the copied key_metadata to decrypt it will fail or read garbage.
I'd mirror new_manifest_writer and route through the encryption manager when one is configured.
| } | ||
|
|
||
| /// Add a deleted manifest entry, preserving the original sequence numbers. | ||
| pub(crate) fn add_deleted_entry(&mut self, mut entry: ManifestEntry) -> Result<()> { |
There was a problem hiding this comment.
This sets status but leaves snapshot_id to the caller, and it sits right next to add_delete_entry (line 343) which stamps both status and snapshot_id = self.snapshot_id. The two differ only by tense, so it's easy to reach for the wrong one.
A tombstone written with the wrong snapshot_id attributes the delete to the wrong snapshot, and expireSnapshots then either drops the data too early or never. Once the resurrection fix starts re-adding prior tombstones through here, this contract matters. I'd either stamp snapshot_id here too, or rename (add_tombstone_entry) and document that the caller owns snapshot_id.
|
|
||
| impl SnapshotProduceOperation for OverwriteOperation { | ||
| fn operation(&self) -> Operation { | ||
| Operation::Overwrite |
There was a problem hiding this comment.
A delete-only overwrite (deletes, no adds) still reports Operation::Overwrite. Java's BaseOverwriteFiles returns DELETE when there are only deletes, APPEND when only adds, and OVERWRITE when both — tools keying off the summary operation will read a delete-only snapshot as an overwrite. Minor, but I'd match on (has_adds, has_deletes).
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_overwrite_with_deleted_files() { |
There was a problem hiding this comment.
This test overwrites a manifest whose entries are all freshly-appended (Added), so it never exercises rewriting a manifest that already holds a Deleted entry — which is exactly the resurrection path flagged above. As written it can't catch that bug.
I'd add a second overwrite: after this one leaves original1/original2 as Deleted, run another overwrite that rewrites the same manifest and assert those two stay Deleted rather than flipping back to Existing.
| // 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() |
There was a problem hiding this comment.
This branch is what enables a delete-only overwrite, but nothing tests that path — no overwrite with deletes and no adds. I'd add one, plus a case that deletes a path absent from every manifest (which is where the phantom-count issue above bites).
Small thing while we're here: the error message just below still only mentions added files/properties — worth adding deleted files so it matches the guard.
| 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()); |
There was a problem hiding this comment.
is_err() alone passes on any error, so if this ever starts failing earlier for an unrelated reason the test still goes green. I'd match on err.kind() == ErrorKind::PreconditionFailed so it actually pins the precondition.
Prompted by review comments on the upstream PR our OverwriteAction was originally built on top of (apache#2185). Checked each comment against our own implementation; three applied here too: - Resurrection bug (the serious one): rewrite_manifest's fallback branch called add_existing_entry for every entry not newly deleted this round, including entries that were already Deleted by a prior overwrite -- add_existing_entry unconditionally resets status to Existing, silently resurrecting previously-deleted files as live data the next time their manifest got rewritten for an unrelated reason. Fixed by routing already-non-alive entries through add_deleted_entry instead, preserving their original snapshot_id as a tombstone. Added test_second_overwrite_does_not_resurrect_deleted_file to lock this in. - Wrong schema-id on rewritten manifests: schema was taken from table.metadata().current_schema() (the table's *current* schema) rather than the manifest being rewritten's own schema. After any schema evolution between the original write and a later overwrite, this stamped the wrong schema-id on old entries, which schema-id-aware readers (Java, PyIceberg) would misinterpret. Fixed by deriving both schema and partition spec directly from the manifest's own ManifestMetadata (which already carries fully resolved objects, not just IDs) -- this is also simpler than the existing partition_spec_by_id table lookup it replaces, and doesn't depend on the table still listing that partition spec. - Manifest naming/location bypassed convention: rewritten manifests were written to a hardcoded `{location}/metadata/` path with a fresh random UUID per manifest, rather than `metadata_location()` (which respects a configured write.metadata.path table property, already used correctly elsewhere in this same file for the manifest list) and the commit's own UUID (shared across every manifest touched by one commit, matching SnapshotProducer::new_manifest_writer's own convention). Added a commit_uuid() getter on SnapshotProducer and an index parameter to rewrite_manifest to keep names unique when a commit rewrites more than one manifest. Also fixed, one level up in shared code: SnapshotProducer::summary() unconditionally applied "truncate full table" semantics (replace computed added/removed counts with the previous snapshot's totals) for any Operation::Overwrite, which is wrong for OverwriteAction's explicit-file-list partial overwrites -- it already knows exactly what was added/removed. Added a `truncate_full_table()` method to SnapshotProduceOperation (default false, only meaningful for an operation that genuinely replaces the whole table) and had OverwriteOperation opt out explicitly. Updated test_delete_only_overwrite_summary, which had documented the old (wrong) truncated counts as expected behavior, to assert the correct ones. Not applicable: the review's manifest-filtering concern (dropping delete-only manifests) is already handled correctly on our side -- both FastAppendAction and OverwriteAction's existing_manifest() were confirmed to already keep manifests with has_deleted_files(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
We need overwrite for a service we build on iceberg-rust with a Lakekeeper catalog, so I went through the review and did the fixes on top of your branch: glitchy#4. It merges main first (the |
This branch has not been deployed
10 participants
Which issue does this PR close?
Part of #2186
This is the first in a series of PRs working toward full Copy-on-Write (CoW) and Merge-on-Read (MoR) support. CoW comes first because it provides the foundation that MoR eventually depends on. This PR delivers complete CoW overwrite semantics. Subsequent PRs will add
RowDeltaActionfor writing position/equality delete files (MoR write path), scan-side delete file reconciliation (MoR read path), and compaction.Adds
OverwriteAction, a newTransactionActionthat produces snapshots withOperation::Overwritesemantics. It adds new data files and optionally removes existing data files by rewriting affected manifests with entries marked asManifestStatus::Deleted.Supporting changes:
ManifestWriter::add_deleted_entry()--the existingadd_entry()unconditionally sets status toAdded; there was no way to write deleted entriesSnapshotProducer::snapshot_id()getter--needed to stamp the snapshot ID on deleted manifest entriesFastAppendAction::existing_manifest()now preserves delete-only manifests so that deleted entries produced byOverwriteActionsurvive subsequent appendsAre these changes tested?
5 new unit tests in
transaction::overwrite::tests:test_empty_data_overwrite_action--error on empty file listtest_overwrite_snapshot_properties--custom properties flow to snapshot summarytest_overwrite_incompatible_partition_value--rejects mismatched partition typestest_overwrite_basic--verifies updates, requirements, operation type, manifest structure, sequence numberstest_overwrite_with_deleted_files--end-to-end: append via catalog, overwrite with deletes via catalog, verify original file isDeletedand replacement isAdded