zeroshade commented on code in PR #2054:
URL: https://github.com/apache/iceberg-go/pull/2054#discussion_r4108439983
##########
table/row_delta.go:
##########
@@ -379,6 +400,49 @@ func (rd *RowDelta) validateRemovedDeletes(resolved,
replacedLive []iceberg.Data
return nil
}
+// findReplacedLiveDVs walks the current snapshot's delete manifests and
+// collects all live deletion vectors whose referenced data file this delta
+// adds a replacement for. This is used to enforce the v3 spec invariant
+// that at most one live DV can exist per data file: if any such live DVs
+// are found and they are not being explicitly removed, the commit must fail.
+func (rd *RowDelta) findReplacedLiveDVs(fs iceio.IO, meta *MetadataBuilder)
([]iceberg.DataFile, error) {
+ snap := rd.txn.planningSnapshot(meta)
+ if snap == nil {
+ // No existing snapshot means no live DVs to worry about.
+ return nil, nil
+ }
+
+ addedRefs := make(map[string]struct{}, len(rd.delFiles))
+ for _, f := range rd.delFiles {
Review Comment:
**Same-delta duplicate DVs are still accepted.** The existing
`RemoveDeletes` contract says, “The v3 spec allows at most one live deletion
vector per data file.” Here `addedRefs` collapses two new DVs referencing the
same data file into one key. If the data file has no live DV yet, the scan
returns no `replacedLive`; without `RemoveDeletes`, `validateRemovedDeletes`
(which checks duplicate additions) is never called. `AddDeletes(dvA, dvB)` can
therefore commit two live DVs and make scans reject the table. Please reject
duplicate added references on the fast-append path and test that case.
##########
table/row_delta.go:
##########
@@ -234,6 +234,27 @@ func (rd *RowDelta) Commit(ctx context.Context) error {
op := rd.Operation()
+ // Find live DVs that would be superseded by added DVs, regardless of
+ // whether removals are present. If any such live DVs exist and are not
+ // being removed, the commit must fail.
+ replacedLive, err := rd.findReplacedLiveDVs(fs, meta)
Review Comment:
**The check does not cover concurrent first-DV additions.** Two transactions
based on a snapshot with no DV for the same data file both pass this scan.
After one commits, the other's no-removal transaction is replayable: `doCommit`
rebuilds its manifest list against the refreshed parent but does not rerun this
check, and `RowDelta.validate` checks the referenced data file rather than DV
uniqueness. The retry can inherit the first DV and commit the second. Please
check against the refreshed parent during retry, or prevent replay of these
additions, and cover the two-writer case in a test. The existing
`RemoveDeletes` contract requires at most one live DV per data file.
--
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]
