zeroshade commented on code in PR #1992:
URL: https://github.com/apache/iceberg-go/pull/1992#discussion_r3960486467
##########
table/replace_files_test.go:
##########
@@ -373,17 +373,118 @@ func
TestReplaceFilesWithDeleteFilesValidatesDeletionVectorIdentity(t *testing.T
require.Len(t, tasks, 2)
sharedPath := sharedTbl.Location() + "/data/shared.puffin"
+ vectorA := newRewriteDeletionVector(t, sharedPath,
tasks[0].File.FilePath(), &offsetA, &length)
+ vectorB := newRewriteDeletionVector(t, sharedPath,
tasks[1].File.FilePath(), &offsetB, &length)
tx = sharedTbl.NewTransaction()
err = tx.ReplaceFilesWithDeleteFiles(t.Context(), nil, nil,
[]iceberg.DataFile{sourceDelete},
[]table.DeleteFileAddition{
- {File: newRewriteDeletionVector(t, sharedPath,
tasks[0].File.FilePath(), &offsetA, &length), DataSequenceNumber: sequence},
- {File: newRewriteDeletionVector(t, sharedPath,
tasks[1].File.FilePath(), &offsetB, &length), DataSequenceNumber: sequence},
+ {File: vectorA, DataSequenceNumber: sequence},
+ {File: vectorB, DataSequenceNumber: sequence},
}, nil)
require.NoError(t, err, "distinct DV blobs in one Puffin
container must be accepted")
+ sharedTbl, err = tx.Commit(t.Context())
+ require.NoError(t, err)
+
+ offsetC := offsetB + length
+ tx = sharedTbl.NewTransaction()
+ err = tx.ReplaceFilesWithDeleteFiles(t.Context(), nil, nil,
+ []iceberg.DataFile{vectorA},
+ []table.DeleteFileAddition{{
+ File: newRewriteDeletionVector(t,
sharedPath, tasks[0].File.FilePath(), &offsetC, &length),
+ DataSequenceNumber: sequence,
+ }}, nil)
+ require.NoError(t, err, "a new DV blob may reuse an existing
container path while a sibling survives")
})
}
+func TestReplaceFilesWithDeleteFilesRejectsDuplicatePaths(t *testing.T) {
+ const path = "shared-delete-file"
+ offset, length := int64(8), int64(16)
+ pos := newPosDeleteFile(t, path)
+ eq := newEqDeleteFile(t, path)
+ vector := newRewriteDeletionVector(t, path, "data.parquet", &offset,
&length)
+ conflictError := "delete file path " + path +
+ " cannot identify both a deletion vector container and a
regular delete file for ReplaceFiles"
+
+ for _, tt := range []struct {
+ name string
+ version int
Review Comment:
**nit** — Duplicate-path subtests run against a snapshot-less table, so they
pin the checks only via validation ordering
TestReplaceFilesWithDeleteFilesRejectsDuplicatePaths builds tables via
newReplaceFilesTestTableVersion without committing a snapshot. When a
uniqueness/conflict check is removed, the observed error is 'invalid operation
value: cannot replace files in a table without an existing snapshot' rather
than a successful call. The subtests are non-vacuous today (mutation proves
they go RED), but they depend on the add-side validation running before the
snapshot precondition; reordering those would silently weaken them.
TestReplaceFilesWithDeleteFilesValidatesExistingPaths, which commits a real
snapshot and asserts 'An error is expected but got nil', is the more robust
pattern. Not worth blocking on.
--
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]
