avison9 opened a new pull request, #4315:
URL: https://github.com/apache/iceberg-python/pull/4315
Closes #4183
# Rationale for this change
A transaction that writes two refs only guarded the first one against
concurrent changes.
`Transaction._stage` de-duplicated requirements by type alone, so after the
first write staged
`AssertRefSnapshotId(ref="b")`, the second write's
`AssertRefSnapshotId(ref="main")` was dropped and
the commit reached the catalog without an assertion on `main`.
The type-only de-duplication was correct when it was added (#569), because a
transaction could only
ever assert `main`. Writing to branches (#941) made `AssertRefSnapshotId`
per-ref, but the
de-duplication was never updated.
This changes the key to `(type, ref)` for `AssertRefSnapshotId`. Each ref
keeps its first (base)
assertion, matching Java's `UpdateRequirements`. Other requirement types are
unchanged. Retries go
through `_stage` too, so they now keep every ref's assertion as well.
```python
cat.load_table("ns.t").manage_snapshots().rollback_to_snapshot(s1).commit()
# another writer
with t.transaction() as tx:
tx.append(row(3), branch="b")
tx.append(row(3))
cat.load_table("ns.t").scan() # before: [1, 2, 3] rollback silently undone
# after: [1, 3] conflict detected,
commit retried
```
Measured on a transaction that writes branch `b` and then `main`:
| Concurrent change to `main` | Format | Before | After |
|---|---|---|---|
| rollback | v1 / v2 | rollback undone | retried, `[1, 3]` |
| append | v1 | appended row lost | retried, `[1, 2, 3, 4]` |
| append | v2 | non-retryable `ValueError` (sequence number) | retried, `[1,
2, 3, 4]` |
## Are these changes tested?
Yes. Two tests were added to `tests/table/test_commit_retry.py`, and each
runs against every catalog
fixture:
-
`test_transaction_writing_two_branches_detects_concurrent_change_to_either`
covers the issue's
scenario on format v1 and v2, with both a concurrent rollback and a
concurrent append.
- `test_transaction_stages_one_ref_assertion_per_ref` checks that writing
`b` and then `main` twice
stages exactly one assertion per ref, each holding the base snapshot id.
Both fail when the `_stage` change is reverted. Unit suite: 4524 → 4530
passed, no new failures.
`make lint` is clean. `make test-integration` passes (1362 passed, 0 failed).
## Are there any user-facing changes?
Yes, as a bug fix. Multi-ref transactions now detect concurrent changes to
every ref they write, and
retry or fail with `CommitFailedException` instead of overwriting them.
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]
