laskoviymishka commented on code in PR #2004:
URL: https://github.com/apache/iceberg-go/pull/2004#discussion_r4024083982
##########
table/transaction.go:
##########
@@ -1209,6 +1217,10 @@ func (t *Transaction)
validateDeleteFilesToAdd(deleteFiles []rewriteDeleteFileAd
}
}
+ if err := validateDeletionVectorFormatVersion(df,
meta.formatVersion, operation); err != nil {
Review Comment:
Small structural thing: this lifts the version check above the
`!IsDeletionVector`/`IsDeletionVector` pair, so the DV logic is now split:
version check here, ref/offset/size still down in the `if IsDeletionVector(df)`
block at line 1241.
Two ways to regroup: move this call to the first line of that `if
IsDeletionVector(df)` block, or, since the `!IsDeletionVector` branch above
always `continue`s, collapse the now-redundant trailing `if
IsDeletionVector(df)` into an `else`. Either's fine, I'd just pick one so the
DV path reads as a single block.
##########
table/row_delta.go:
##########
@@ -196,6 +198,10 @@ func (rd *RowDelta) Commit(ctx context.Context) error {
ct, f.FilePath())
}
+ if err := validateDeletionVectorFormatVersion(f,
meta.formatVersion, "row delta"); err != nil {
Review Comment:
The version gate here is exactly right. What I'd think about while we're in
this loop: `validateDeleteFilesToAdd` also checks that a DV carries a non-empty
`referenced_data_file`, a non-nil `content_offset >= 0`, and
`content_size_in_bytes > 0`, but `RowDelta.Commit` still doesn't. So a caller
passing a Puffin pos-delete without a ref (or with a nil offset) through
`AddDeletes` on a v3 table gets no error and writes a manifest entry other
engines can't apply.
It's a pre-existing gap, not something this PR introduced, but adding the
explicit version guard makes the missing field checks more conspicuous. I'd add
the same ref/offset/size checks right after this call so the two add-delete
paths validate identically, or if you'd rather keep this PR tight, a tracked
follow-up is fine. wdyt?
##########
table/row_delta_test.go:
##########
@@ -1108,6 +1108,107 @@ func TestRowDeltaRemoveDeletesFailsInsteadOfReplaying(t
*testing.T) {
"the data file must carry exactly one live DV: the peer's")
}
+func buildPuffinPosDeleteWithoutRef(t *testing.T, path string)
iceberg.DataFile {
+ t.Helper()
+
+ b, err := iceberg.NewDataFileBuilder(*iceberg.UnpartitionedSpec,
iceberg.EntryContentPosDeletes,
+ path, iceberg.PuffinFile, nil, nil, nil, 2, 128)
+ require.NoError(t, err)
+
+ return b.Build()
+}
+
+func TestRowDeltaRejectsDeletionVectorBelowV3(t *testing.T) {
+ const (
+ dataPath = "s3://bucket/data/insert.parquet"
+ dvPath = "s3://bucket/data/dv-001.puffin"
+ )
+
+ tests := []struct {
+ name string
+ formatVersion int
+ rows []iceberg.DataFile
+ deletes func(*testing.T) []iceberg.DataFile
+ errContains string
+ }{
+ {
+ name: "deletion vector alone on v2",
+ formatVersion: 2,
+ deletes: func(t *testing.T) []iceberg.DataFile {
+ return []iceberg.DataFile{buildDVFile(t,
dvPath, dataPath)}
+ },
+ errContains: "requires table format version >= 3",
+ },
+ {
+ name: "deletion vector beside valid files on
v2",
+ formatVersion: 2,
+ rows: []iceberg.DataFile{buildDataFile(t,
dataPath)},
+ deletes: func(t *testing.T) []iceberg.DataFile {
+ return []iceberg.DataFile{
+ buildPosDeleteFile(t,
"s3://bucket/data/pos-del.parquet"),
+ buildEqDeleteFile(t,
"s3://bucket/data/eq-del.parquet", []int{1}),
+ buildDVFile(t, dvPath, dataPath),
+ }
+ },
+ errContains: "requires table format version >= 3",
+ },
+ {
+ name: "deletion vector without referenced data
file on v2",
Review Comment:
This case name says "without referenced data file," but the assertion is the
version error, so what it actually proves is that the format-version check
fires before any missing-ref check on v2. A future reader could take the name
to mean we validate the ref on v2, which we don't. I'd rename it to something
like "deletion vector missing ref still fails on format version for v2".
##########
table/row_delta_test.go:
##########
@@ -1108,6 +1108,107 @@ func TestRowDeltaRemoveDeletesFailsInsteadOfReplaying(t
*testing.T) {
"the data file must carry exactly one live DV: the peer's")
}
+func buildPuffinPosDeleteWithoutRef(t *testing.T, path string)
iceberg.DataFile {
+ t.Helper()
+
+ b, err := iceberg.NewDataFileBuilder(*iceberg.UnpartitionedSpec,
iceberg.EntryContentPosDeletes,
+ path, iceberg.PuffinFile, nil, nil, nil, 2, 128)
+ require.NoError(t, err)
+
+ return b.Build()
+}
+
+func TestRowDeltaRejectsDeletionVectorBelowV3(t *testing.T) {
+ const (
+ dataPath = "s3://bucket/data/insert.parquet"
+ dvPath = "s3://bucket/data/dv-001.puffin"
+ )
+
+ tests := []struct {
+ name string
+ formatVersion int
+ rows []iceberg.DataFile
+ deletes func(*testing.T) []iceberg.DataFile
+ errContains string
+ }{
+ {
+ name: "deletion vector alone on v2",
+ formatVersion: 2,
+ deletes: func(t *testing.T) []iceberg.DataFile {
+ return []iceberg.DataFile{buildDVFile(t,
dvPath, dataPath)}
+ },
+ errContains: "requires table format version >= 3",
+ },
+ {
+ name: "deletion vector beside valid files on
v2",
+ formatVersion: 2,
+ rows: []iceberg.DataFile{buildDataFile(t,
dataPath)},
+ deletes: func(t *testing.T) []iceberg.DataFile {
+ return []iceberg.DataFile{
+ buildPosDeleteFile(t,
"s3://bucket/data/pos-del.parquet"),
+ buildEqDeleteFile(t,
"s3://bucket/data/eq-del.parquet", []int{1}),
+ buildDVFile(t, dvPath, dataPath),
+ }
+ },
+ errContains: "requires table format version >= 3",
+ },
+ {
+ name: "deletion vector without referenced data
file on v2",
+ formatVersion: 2,
+ deletes: func(t *testing.T) []iceberg.DataFile {
+ return
[]iceberg.DataFile{buildPuffinPosDeleteWithoutRef(t, dvPath)}
+ },
+ errContains: "requires table format version >= 3",
+ },
+ {
+ // v1 rejects every delete file before the DV check is
reached.
+ name: "deletion vector on v1",
+ formatVersion: 1,
+ deletes: func(t *testing.T) []iceberg.DataFile {
+ return []iceberg.DataFile{buildDVFile(t,
dvPath, dataPath)}
+ },
+ errContains: "format version >= 2",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ tbl := newRowDeltaCommitTestTableVersion(t,
tt.formatVersion)
+
+ rd :=
tbl.NewTransaction().NewRowDelta(nil).AddRows(tt.rows...).AddDeletes(tt.deletes(t)...)
+
+ err := rd.Commit(t.Context())
+ require.Error(t, err)
+ assert.ErrorContains(t, err, tt.errContains)
+ assert.Empty(t, writtenManifests(t, tbl.Location()), "a
rejected row delta must not leave manifests behind")
+
+ validTx := tbl.NewTransaction()
+ require.NoError(t,
validTx.NewRowDelta(nil).AddRows(buildDataFile(t,
dataPath)).Commit(t.Context()))
+ assert.NotEmpty(t, writtenManifests(t, tbl.Location()),
"the same probe must observe the manifests a successful commit writes")
+ })
+ }
+}
+
+func TestRowDeltaAcceptsDeletionVectorOnV3(t *testing.T) {
+ tbl := newRowDeltaCommitTestTableVersion(t, 3)
+ dataPath := tbl.Location() + "/data/insert.parquet"
+ dvPath := tbl.Location() + "/data/dv-001.puffin"
+
+ tx := tbl.NewTransaction()
+ require.NoError(t, tx.NewRowDelta(nil).AddDeletes(buildDVFile(t,
dvPath, dataPath)).Commit(t.Context()))
Review Comment:
Worth a note here: this adds the DV to a freshly-created table whose main
branch doesn't exist yet, so `doCommit` skips the conflict validators (the
`SnapshotByName(branch) != nil` guard in table.go) and `validateDataFilesExist`
never runs. The test is correct for what it asserts (v3 acceptance), it just
isn't the full round-trip it reads like.
A one-line comment saying conflict validation is bypassed here would save
the next reader the trace, or commit an initial data file first if we want the
referenced-file check to actually fire. Your call.
##########
table/row_delta_test.go:
##########
@@ -1108,6 +1108,107 @@ func TestRowDeltaRemoveDeletesFailsInsteadOfReplaying(t
*testing.T) {
"the data file must carry exactly one live DV: the peer's")
}
+func buildPuffinPosDeleteWithoutRef(t *testing.T, path string)
iceberg.DataFile {
+ t.Helper()
+
+ b, err := iceberg.NewDataFileBuilder(*iceberg.UnpartitionedSpec,
iceberg.EntryContentPosDeletes,
+ path, iceberg.PuffinFile, nil, nil, nil, 2, 128)
+ require.NoError(t, err)
+
+ return b.Build()
+}
+
+func TestRowDeltaRejectsDeletionVectorBelowV3(t *testing.T) {
+ const (
+ dataPath = "s3://bucket/data/insert.parquet"
+ dvPath = "s3://bucket/data/dv-001.puffin"
+ )
+
+ tests := []struct {
+ name string
+ formatVersion int
+ rows []iceberg.DataFile
+ deletes func(*testing.T) []iceberg.DataFile
Review Comment:
`rows` is a plain slice built eagerly with the outer `t`, but `deletes` is a
`func(*testing.T)` called lazily inside each subtest. The builders are all
infallible (constructed from constants), so the closure doesn't buy us
anything, and the asymmetry means a `rows` builder failure would `FailNow` the
outer `t` and report at the struct literal, while a `deletes` failure reports
per-subtest.
I'd make `deletes` a plain `[]iceberg.DataFile` too and build both at
struct-init time. Matches how the other tests in this file call the builders.
--
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]
