avison9 opened a new pull request, #4317:
URL: https://github.com/apache/iceberg-python/pull/4317
Closes #4316
> **Draft: depends on #4315.** This branch is stacked on #4315, so the first
commit
> (`Assert every ref a transaction writes, not just the first`) belongs to
that PR. Only the last commit
> is new here. Without #4315, a conflict on a second branch never triggers a
retry, so the path fixed
> here can't be reached. I'll rebase onto `main` once #4315 merges.
# Rationale for this change
When a transaction writes more than one branch, concurrency validation for a
write on a later branch
was computed from the **first** write's branch. Both places that build a
`CommitWindow` resolved a
single window from `self._snapshot_producers[0]` and reused it for every
producer:
- `Transaction._rebuild_snapshot_updates` (the retry path, reported in
#4316);
- the empty-staged-output path in `Transaction.commit_transaction` (a delete
whose plan matched
nothing). Same root cause, found while fixing the first.
If the first branch didn't change concurrently, its window is empty, so a
serializable delete or
overwrite on the second branch skipped validation and committed over a
conflicting change it never saw.
```python
tx = t.transaction()
tx.append(pa.table({"x": [9]})) # main
tx.delete("x == 1", branch="b") # serializable delete on b
cat.load_table("ns.t").append(pa.table({"x": [1]}), branch="b") #
concurrent matching append to b
tx.commit_transaction()
# before: commits, conflict missed (the delete alone raises)
# after: ValidationException, same as the single-branch delete
```
This adds `Transaction._resolve_commit_windows()`, which resolves **one
window per target branch**,
based on the first producer that writes to that branch, and uses it in both
places.
Why not simply a window per producer? A producer's `_starting_snapshot_id`
is its branch head as the
transaction sees it. For a second write to the same branch, that's the
snapshot staged by the first
write, which the catalog has never seen, so resolving it fails with `Cannot
find starting snapshot`.
(`Transaction.delete` already pins its overwrite's start to the delete's for
the same reason.) The
first producer on each branch always starts from a committed snapshot.
Single-branch transactions behave exactly as before: there, the first
producer on the branch is
producer 0.
## Are these changes tested?
Yes. Three tests were added to `tests/table/test_commit_retry.py`, and each
runs against every catalog
fixture:
- `test_retry_validates_each_branch_against_its_own_commits`: the #4316
scenario (retry path) raises
`ValidationException` and leaves both branches untouched.
- `test_empty_plan_validation_checks_each_branch_against_its_own_commits`:
the same bug in the
empty-output path.
- `test_retry_with_two_writes_to_one_branch_succeeds`: a guard that a second
write to the same branch
still retries successfully. It fails with `Cannot find starting snapshot`
under a naive
per-producer window.
The first two fail before the change and the third passes. Unit suite
against the #4315 base:
4530 → 4539 passed, no new failures. `make lint` is clean.
## Are there any user-facing changes?
Yes, as a bug fix. Multi-branch transactions now validate each branch's
writes against that branch's
concurrent commits, so a conflicting serializable delete or overwrite raises
`ValidationException`
instead of committing. There is no API change.
--
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]
