Skip to content

Validate replaced data files during commit retries - #3811

Open
kevinjqliu wants to merge 6 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/codex-validate-concurrent-file-overwrite
Open

Validate replaced data files during commit retries#3811
kevinjqliu wants to merge 6 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/codex-validate-concurrent-file-overwrite

Conversation

@kevinjqliu

@kevinjqliu kevinjqliu commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • validate that explicitly replaced data files still exist when an overwrite retries
  • add regression coverage for conflicting and unrelated concurrent file deletions

Follow-up to #3780.

Why this validation is needed

Suppose transaction A is replacing data file F with a newly written file F2. Before A commits, transaction B deletes F and commits. A then refreshes the table and retries its commit.

The retry must fail. If A commits F2, it puts data derived from F back into the table and can undo B's deletion.

The existing checks do not catch this case:

  • delete_data_file(F) records a specific file, not a row predicate, so predicate-based deleted-file validation has nothing to match.
  • _validate_no_new_deletes_for_data_files checks newly added position or equality delete files. Transaction B removed a data file instead.

_validate_data_files_exist provides the missing check: before retrying, verify that every file being replaced is still present. The scan is narrowed by spec and partition, but only an exact file match causes a conflict.

Scenarios tested

Concurrent deletion Expected result
Exact file being replaced Fail
Different file in the same partition Succeed
File in a different partition Succeed

All three cases are covered by test_file_overwrite_validates_concurrent_file_delete and run against all three catalog fixtures.

Java alignment

Iceberg Java handles the same concerns separately:

Java also tests unrelated concurrent deletion succeeding and target-file deletion failing.

Tests

  • 9 regression-test executions: 3 scenarios × 3 catalog fixtures
  • all commit hooks passed

@kevinjqliu kevinjqliu changed the title Validate replaced files during commit retries Validate replaced data files during commit retries Aug 19, 2026
@kevinjqliu
kevinjqliu marked this pull request as ready for review August 19, 2026 06:32
@kevinjqliu
kevinjqliu requested review from Fokko and geruh and a lite review from Copilot August 19, 2026 06:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds commit-retry validation to prevent an overwrite that explicitly replaces a data file from succeeding if the replaced (target) file was concurrently deleted by another transaction, avoiding resurrecting data from a removed file.

Changes:

  • Add _validate_data_files_exist to detect concurrent deletions of explicitly targeted/replaced data files during the retry validation window.
  • Wire the new validation into snapshot concurrency validation when a commit has specific deleted/replaced data files tracked.
  • Add a regression test covering conflicting and non-conflicting concurrent deletions (same file, different file same partition, different partition) across all catalog fixtures.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
pyiceberg/table/update/validate.py Adds validation to detect concurrent deletion of specific required/replaced data files.
pyiceberg/table/update/snapshot.py Invokes the new validation during concurrency checks when explicit deleted/replaced files are present.
tests/table/test_commit_retry.py Adds regression coverage for overwrite retries with concurrent data-file deletions.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants