[zeroshade]

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.

[nssalian]

nit: this still says "groups whose catalog commits failed", but groups rejected by the new pre-check never reach the catalog. Maybe "groups whose batch failed to commit or was rejected because a delete for one of its files was committed after planning"

[zeroshade]

Updated in 3491bfe: FailedGroups now covers batches that failed to commit or were rejected because a delete for one of their files was committed after planning.

[nssalian]

Nothing pins this increment. If I replace it with _ = failedCommits, all the TestRewrite|Compaction tests still pass. The only MaxFailedCommits test goes through cat.failOn. Since the doc now says these rejections count toward the bound, could you add a case with MaxFailedCommits: 1 where two batches get rejected, and assert the "maximum failed commits reached" error?

[zeroshade]

Added TestRewriteDataFiles_PartialProgressRejectedBatchesCountTowardMaxFailedCommits in 3491bfe: three batches, peer deletes absorbed into batches 2 and 3, MaxFailedCommits: 1. It asserts the "maximum failed commits reached" error wrapping ErrConflictingDeleteFiles. Replacing the increment with _ = failedCommits makes it fail.

[nssalian]

I ran the new test against the merge-base rewrite_data_files.go and it fails at :868 with both groups completing. On base I can also see the actual symptom: the deleted row comes back (4 rows instead of 3) and the peer's delete is dropped as dead. With the fix it stays at 3 rows and the delete stays live. The same holds for a three-batch run where the peer delete targets the third batch. A peer delete on a file outside the rewrite doesn't trigger a false rejection. This lines up with Java's validateFromSnapshot(startingSnapshotId) in RewriteDataFilesCommitManager, just checked before the batch writes anything. It needs a rebase. On current main the new test's tx.AddFiles is AddDataFilesByPath after #2308, and concurrentTestCatalog conflicts with the new restConflicts field. Two small things I left comments for

[zeroshade]

Thanks @nssalian, and for checking it against base and the three-batch case. Rebased onto main: AddFiles is now AddDataFilesByPath, and concurrentTestCatalog keeps both restConflicts and beforeCommit. Both nits are addressed in 3491bfe; the absorbed-delete setup is now a shared fixture so the new MaxFailedCommits test can reuse it.