fix(table): reject dvs added to tables below v3 - #2004
Conversation
Signed-off-by: badalprasadsingh <badal@datazip.io>
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
The version gate is exactly the right fix, and the v1/v2/v3 table-driven coverage is thorough. Nice work.
The one thing I'd want us to decide: RowDelta.Commit now guards the format version but still doesn't validate a DV's referenced_data_file/content_offset/content_size_in_bytes, while validateDeleteFilesToAdd does. The new version guard makes that asymmetry more conspicuous: a v3 caller can AddDeletes a ref-less DV and write a manifest other engines can't apply. I'd add the same checks here to bring the two paths to parity, but it's pre-existing and out of this PR's stated scope, so a tracked follow-up is fine too. I'd just not leave it silent.
The rest is minor: the version-check call splits the DV logic across two spots in validateDeleteFilesToAdd, one test case name oversells what it proves, the rows/deletes eager-vs-lazy split in the table test buys nothing, and the v3-accept test quietly skips the referenced-file existence check. All inline.
Anyway - this is good to me as is.
| ct, f.FilePath()) | ||
| } | ||
|
|
||
| if err := validateDeletionVectorFormatVersion(f, meta.formatVersion, "row delta"); err != nil { |
There was a problem hiding this 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?
| } | ||
| } | ||
|
|
||
| if err := validateDeletionVectorFormatVersion(df, meta.formatVersion, operation); err != nil { |
There was a problem hiding this 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 continues, 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.
| name string | ||
| formatVersion int | ||
| rows []iceberg.DataFile | ||
| deletes func(*testing.T) []iceberg.DataFile |
There was a problem hiding this 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.
| errContains: "requires table format version >= 3", | ||
| }, | ||
| { | ||
| name: "deletion vector without referenced data file on v2", |
There was a problem hiding this 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".
| dvPath := tbl.Location() + "/data/dv-001.puffin" | ||
|
|
||
| tx := tbl.NewTransaction() | ||
| require.NoError(t, tx.NewRowDelta(nil).AddDeletes(buildDVFile(t, dvPath, dataPath)).Commit(t.Context())) |
There was a problem hiding this 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.
|
thanks @laskoviymishka :), I agree on the validation gap, i will be tracking it as a follow-up and address the minor suggestions as well. |
…-up) (#2016) * fix: reject dvs below v3 Signed-off-by: badalprasadsingh <badal@datazip.io> * minor follow ups Signed-off-by: badalprasadsingh <badal@datazip.io> * minor Signed-off-by: badalprasadsingh <badal@datazip.io> * chore: minor Signed-off-by: badalprasadsingh <badal@datazip.io> * minor Signed-off-by: badalprasadsingh <badal@datazip.io> * minor Signed-off-by: badalprasadsingh <badal@datazip.io> --------- Signed-off-by: badalprasadsingh <badal@datazip.io>
2 participants
Fixes #2003, added a validating function (
validateAddedDeletionVector) and calling it at both paths:RowDelta.Commit, andvalidateDeleteFilesToAdd.Added the regression test cases for it.