laskoviymishka commented on code in PR #2016:
URL: https://github.com/apache/iceberg-go/pull/2016#discussion_r4064433630
##########
table/transaction.go:
##########
@@ -1289,24 +1306,11 @@ func (t *Transaction)
validateDeleteFilesToAdd(deleteFiles []rewriteDeleteFileAd
}
if IsDeletionVector(df) {
Review Comment:
The self-guard landed inside `validateDeletionVectorToAdd`, so the helper is
safe to call unconditionally now, good. That does leave two guards for the same
predicate though: this outer `if IsDeletionVector(df)` is dead, since the
`!IsDeletionVector` block above already returns or continues on every non-DV
path, so we only reach here on a DV.
Not blocking, but I'd drop the outer wrapper and let the helper's own guard
carry it, then add a one-liner on `validateDeletionVectorToAdd` noting it's
safe on any DataFile and no-ops on non-DVs. Keeps the precondition visible and
drops a branch a reader would otherwise read as a real else. Happy either way.
##########
table/row_delta_test.go:
##########
@@ -1108,74 +1108,94 @@ 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) {
+func TestRowDeltaRejectsInvalidDeletionVector(t *testing.T) {
const (
dataPath = "s3://bucket/data/insert.parquet"
dvPath = "s3://bucket/data/dv-001.puffin"
)
+ offset, length := int64(4), int64(64)
+ negativeOffset, zeroLength := int64(-1), int64(0)
tests := []struct {
name string
formatVersion int
rows []iceberg.DataFile
- deletes func(*testing.T) []iceberg.DataFile
+ deletes []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",
+ deletes: []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),
- }
+ deletes: []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",
+ name: "deletion vector missing ref still fails
on format version for v2",
Review Comment:
Small wording thing, non-blocking: this DV is missing ref, offset, and size,
but the case name singles out the ref. What it actually proves is that the v2
format-version check fires before any field check, so something like `format
version check precedes field checks on v2` would read truer to intent.
##########
table/row_lineage_prune_delete_test.go:
##########
@@ -255,6 +252,28 @@ func buildTwoRowGroupV3Table(t *testing.T) *table.Table {
return tbl
}
+// commitLegacyPosDelete adds posDel while tbl is still v2, then upgrades it
to v3.
+// The no-op Delete creates a v3 snapshot, which gives the existing rows a
_row_id.
Review Comment:
Optional: the comment says what this does, but the load-bearing bit is
implicit. It leans on `Delete(AlwaysFalse)` still forcing a snapshot rewrite so
the upgraded rows pick up a `_row_id`. The `require.NotNil(FirstRowID)` below
would catch a regression, so it's not silently fragile, but a clause noting the
no-op Delete has to stay a rewrite-triggering op would save a future reader
from an empty-filter short-circuit quietly gutting this setup.
--
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]
