ryanworl opened a new pull request, #1784: URL: https://github.com/apache/iceberg-go/pull/1784
## What Commits whose staged snapshot carries delete-file removals — the `deleteFilesToRemove` argument of `Transaction.ReplaceFiles`, and the deletion-vector supersession a v3 merge-on-read delete performs when the data file already carries a DV — no longer enter `doCommit`'s refresh-and-replay retry loop. On a CAS conflict they fail immediately with `ErrCommitFailed` (annotated so it's distinguishable from an exhausted retry budget), and the caller re-resolves the removals against the current snapshot in a new transaction. The mechanism is one flag set by the snapshot producer, one early return in the retry loop, docs, and regression tests. ## Why Removal identity is snapshot-relative: the removed entries were resolved against the snapshot the writer built on, so a replayed removal is semantically a different operation. Concretely: writer A supersedes DV₁ with its merged DV₂ (what any v3 MoR delete does on a file that already has a DV). Writer B supersedes DV₁ with DV₁′ and wins the CAS. A's replay inherits DV₁′ from the fresh base while A's stale removal of DV₁ replays as a no-op (`checkRemovedFiles` deliberately treats a superseded DV as absent — expunging by ref alone would discard the peer's newer deletes). The committed snapshot has two live DVs on one data file, which the v3 spec forbids. The same shape hits path-keyed `ReplaceFiles` removals today: a compaction replay can remove the data file a peer's freshly added delete file references, silently no-oping that delete and resurrecting its rows in the compacted replacement. Java does not have this bug because `SnapshotProducer.commit()` re-runs the whole apply-validate-commit cycle after `refresh()`, re-resolving removals against the fresh base. Go's replay patches the already-built updates instead. The correct long-term fix is Java-style re-apply on retry; until that lands, removal-carrying commits must fail fast rather than replay incorrectly (once re-apply exists, this flag simply becomes unnecessary and can be removed). ## Behavior changes Only for removal-carrying commits with `commit.retry.num-retries` > 0, which today retry incorrectly. Such commits now fail on the first conflict even when the peer's change is unrelated and a replay would have been harmless — the library cannot tell the cases apart, and failing loudly beats corrupting silently. Callers of the removal APIs (compaction/maintenance) already handle commit failure by rebuilding against a fresh table. Data-file-only removals are unaffected and keep replaying: they are path-keyed, and the retry rebuild already fails terminally when the path is gone from the fresh base. ## Tests - `TestMoRDeleteSupersedingDVFailsInsteadOfReplaying`: the two-live-DV shape through `Transaction.Delete` on a v3 MoR table — exactly one `CommitTable` attempt, `ErrCommitFailed`, exactly one live DV (the peer's) afterward, and the transaction stays retriable. - `TestReplaceFilesWithRemovalsFailsInsteadOfReplaying`: the delete-loss shape through `ReplaceFiles`, where the peer's new position delete is invisible to the serializable added-data-files validator. - `TestReplaceFilesDataOnlyStillReplays`: regression guard that a data-file-only replace still replays to success. Both removal tests fail on the pre-fix code — the corrupting replay commits successfully. `go test ./table/...` and `golangci-lint run` are clean. Made with [Cursor](https://cursor.com) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
