zeroshade opened a new pull request, #2302: URL: https://github.com/apache/iceberg-go/pull/2302
### Problem Partial-progress `RewriteDataFiles` can bring back rows that a concurrent writer deleted. Groups are planned once, then committed batch by batch, and each batch starts from the table the previous batch committed (`recordCommittedRewriteBatch`). With `commit.retry.num-retries` > 0: 1. A peer commits a position delete P for data file F while batch 1 is committing. F belongs to batch 2. 2. Batch 1's first attempt fails on the stale branch head. The retry runs the rewrite validator for batch 1's files only, finds no conflict, and commits on top of the peer snapshot. 3. Batch 2's transaction starts from that table, so P is part of its base and its commit-time validator never examines it. 4. Batch 2 rewrites F from tasks planned before P existed, so the deleted rows are written into the new file. `CollectDeadPositionDeletes` then finds P pointing at a rewritten file and removes it as dead. The same applies to a deletion vector for F, which goes through the same validator branch. Equality deletes are not affected, because any concurrent equality delete fails the batch that would absorb it. Without retries, batch 1 fails on the stale head and so does every later batch, so nothing is lost. A non-partial rewrite validates the whole window and is rejected with `ErrConflictingDeleteFiles`. ### Fix Before rewriting a batch, `validateNoNewDeletesSincePlanning` runs `validateNoNewDeletesForRewrittenFiles` over every snapshot committed on the branch between the planning metadata and the current table. Together with the existing commit-time validator, which covers current to the catalog head, every snapshot since planning is now checked. A conflicting batch is handled like a failed commit: its groups go to `FailedGroups` with `ErrConflictingDeleteFiles`, it counts toward `MaxFailedCommits`, and the run continues. The check runs before the batch is executed, so nothing is written for it. The `MaxFailedCommits` doc now mentions these rejections. ### Testing `TestRewriteDataFiles_PartialProgressRejectsDeleteAbsorbedByEarlierBatch` uses a one-shot `beforeCommit` hook on `concurrentTestCatalog` to land a file-scoped position delete for a batch-2 file just before batch 1 commits. It checks that batch 1 completes, batch 2 fails with `ErrConflictingDeleteFiles`, the deleted row stays deleted, the delete stays live, and the rejected batch leaves no outputs. Without the fix, both groups complete and the deleted row comes back. `go test ./table/... ./cmd/iceberg/...` passes. -- 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]
