mzzz-zzm commented on issue #977:
URL: https://github.com/apache/iceberg-go/issues/977#issuecomment-4369969986
## Suggested fix
When `newConflictContext` is called with a `base` that has no snapshot on
the target branch (`base.SnapshotByName(branch) == nil`), the function returned
an empty `concurrent` list. The fix below uses `AncestorsOf` to populate
`concurrent` with all snapshots currently on the branch — those are all commits
that happened after the writer loaded the (then-empty) table, and they must be
inspected by validators.
This change touches **one function in one file** and is independent of the
manifest list rebuild fix and the RowDelta partition filter fix.
## File changed
`table/conflict_validation.go` — `newConflictContext()`
## Diff
```diff
baseHead := base.SnapshotByName(branch)
if baseHead == nil {
- // Writer has no view of this branch yet (e.g. creating it) —
- // by definition there are no concurrent commits to validate
- // against.
- return &conflictContext{current: current, branch: branch, fs: fs,
caseSensitive: caseSensitive}, nil
+ // Writer started from a table that had no snapshot on this branch
+ // (empty table, or branch not yet created by the writer). All
+ // snapshots now present on the branch were committed concurrently
+ // and must be examined by validators that care about concurrent
+ // writes (e.g. RowDelta with equality deletes under SERIALIZABLE).
+ // Fast-append validators safely short-circuit on non-empty
+ // concurrent lists, so this does not regress the common path.
+ allConcurrent := AncestorsOf(currentHead.SnapshotID,
current.SnapshotByID)
+ return &conflictContext{
+ current: current,
+ branch: branch,
+ fs: fs,
+ caseSensitive: caseSensitive,
+ concurrent: allConcurrent,
+ }, nil
}
```
## Why this is safe for fast-append
Fast-append and merge-append producers do not register a validator (their
`producerImpl.validate` is a no-op). A non-empty `concurrent` list is walked
only when a validator calls `forEachAddedEntry` or
`validateNoConflictingDataFiles`. Writers that do not register validators pay
only the cost of the `AncestorsOf` walk (proportional to the number of
snapshots on the branch) with no change in commit acceptance behaviour.
## Existing test that encodes the old (wrong) behaviour
`TestNewConflictContext_WriterHasNoBranchView` in
`table/conflict_validation_test.go` currently asserts `assert.Empty(t,
ctx.concurrent)`. That assertion must be updated to reflect the correct
behaviour:
```diff
-assert.Empty(t, ctx.concurrent)
+assert.Len(t, ctx.concurrent, 1)
+assert.Equal(t, int64(7), ctx.concurrent[0].SnapshotID)
```
## Test
Run the reproducer from the bug report:
```
go test ./table/ -run TestBugRepro_EmptyTableConflictDetectionBypassed -v
```
Expected after this fix:
```
--- PASS: TestBugRepro_EmptyTableConflictDetectionBypassed (0.00s)
```
--
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]
