fix(table): validate deletion vector fields in RowDelta (#2004 follow-up)

[badalprasadsingh]

Follow up to #2004 addressing review.

All the required DV level validations for Commit in RowDelta are in now. Similar to ReplaceFiles via one shared helper.

Changelog Note: RowDelta now rejects Parquet pos-deletes on v3 iceberg tables.

[laskoviymishka]

The shared validateDeletionVectorToAdd helper is the right move — one place for the DV field checks instead of them drifting between RowDelta and ReplaceFiles, and the new v3 test cases line up nicely with the ReplaceFiles ones.

I'd hold this before merging though. The PR reads as bringing RowDelta to full field-level parity with ReplaceFiles, but one asymmetry is still open and it's the one that matters: RowDelta.Commit only runs the DV checks when IsDeletionVector(f) is true, so a plain Parquet position-delete on a v3 table skips validation entirely and commits. ReplaceFiles rejects that case, and Java gates both paths through the same check, so RowDelta is currently the one door that lets a non-DV pos-delete onto v3. It's a correctness gap, not just a style one — a later DV merge for the same data file won't find the stray pos-delete to fold in, so deleted rows can reappear.

The rest is smaller and I've left it inline: the extracted helper dropped its internal IsDeletionVector guard so the precondition is now invisible, one test stopped exercising the commit path it's named for, and a couple of wording/nil-safety notes.

Things I'd want before merge:

• the symmetric "v3 pos-delete must be a DV" guard in RowDelta.Commit
• either re-guard validateDeletionVectorToAdd on IsDeletionVector or document the precondition
• keep an explicit accept-path assertion in TestRowDeltaAcceptsDeletionVectorOnV3

Once those are in, happy to take another pass and approve.

[laskoviymishka]

This branch is where the parity claim breaks. It only validates when IsDeletionVector(f) is true, so a plain Parquet EntryContentPosDeletes file on a v3 table skips every check and commits fine.

ReplaceFiles rejects exactly this a few lines up in validateDeleteFilesToAdd (position delete file %s must be a deletion vector for v%d table), and Java gates both RowDelta and RewriteFiles through the same validateDeleteFileForVersion — so today RowDelta is the one path that lets a non-DV pos-delete onto v3. It's not cosmetic: if an engine later writes a DV for the same data file, the merge logic expects at most one DV per data file and won't find the stray pos-delete to fold in, so deleted rows can come back.

I'd add the symmetric guard here, something like if !IsDeletionVector(f) && ct == iceberg.EntryContentPosDeletes && meta.formatVersion >= 3 returning the same "must be a deletion vector" error. wdyt?

[laskoviymishka]

The old helper self-guarded with IsDeletionVector(df) &&; this one drops that and opens straight into the format-version check. Both call sites happen to wrap it in if IsDeletionVector, so it's correct today, but the contract is now invisible — a future caller passing an equality delete (nil ContentOffset) would get a misleading "missing content_offset".

I'd either make it the first line here (if !IsDeletionVector(df) { return nil }) or drop a one-line doc comment stating the precondition. wdyt?

[laskoviymishka]

This quietly changes the referenced_data_file message from "deletion vector to add is missing referenced_data_file for %s" to include the path and drop "to add", and it hits the ReplaceFiles path too. Our tests are substring-only so they still pass. Fine by me either way — just flagging it's an intentional contract change so anyone matching on the old string knows.

[laskoviymishka]

We re-fetch ref/offset/length here and deref them just below without a nil check — safe only because validateDeletionVectorToAdd just guaranteed them non-nil, but that coupling is invisible at this line. Either return the validated values from the helper, or a short // safe: validateDeletionVectorToAdd guarantees non-nil would make it obvious.

[laskoviymishka]

This test no longer exercises the RowDelta commit it's named for. newTableWithLiveDV does the commit and hands back an already-committed table, so the body only reads CurrentSnapshot() — the "RowDelta accepts a valid DV without erroring" assertion has effectively moved into the helper.

I'd keep an explicit require.NoError on a fresh RowDelta commit with a valid DV here, or rename the test to say it's checking snapshot structure. Otherwise if the helper changes, we lose the accept-path coverage without noticing.

[badalprasadsingh]

There was a problem as RowDelta now rejects Parquet pos-deletes on v3 tables, which broke four tests that committed one:

• TestRowDeltaRemoveDeletesRejectsNonDV, TestScanPruningWithPositionalDeletes, TestScanRowLineagePreservedThroughPositionalDeletes: these need the delete on a v3 table. The only real way to get there is to commit it on v2 and then upgrade, so the tests now do exactly that.
• The two scan tests also commit a no-op Delete. That creates the first v3 snapshot, which gives the existing rows a _row_id.
• TestDataFileFromMetadata_EndToEndRowDelta: nothing v3-specific, so it now runs on v2.

Happy to change this if there's a better approach :)

[zeroshade]

APPROVE. The prior requested changes are addressed: table/row_delta.go:201-208 now rejects new non-DV position deletes on v3+, validateDeletionVectorToAdd self-guards, the validated dereference is documented, and TestRowDeltaAcceptsDeletionVectorOnV3 exercises the commit path explicitly. The required DV metadata checks match the v3 rules for referenced_data_file, content_offset, and content_size_in_bytes. The legacy positional-delete test changes preserve their assertions by committing on v2 and upgrading before exercising v3 reads, so the -87 lines did not drop coverage.

@laskoviymishka your CHANGES_REQUESTED is the remaining gate, and all five threads are still mechanically unresolved — I went through them and found no substantive item still open. Re-review when you can; I'll leave the merge to you.

[laskoviymishka]

This closes the gap I held on last round. RowDelta.Commit now rejects a plain Parquet pos-delete on v3 the same way ReplaceFiles and Java do, so a stray non-DV pos-delete can't slip onto a v3 table and get stranded when a later DV merges. All three things I asked for are in:

• the symmetric "v3 pos-delete must be a DV" guard in RowDelta.Commit
• the IsDeletionVector precondition is back, now as a self-guard inside validateDeletionVectorToAdd
• TestRowDeltaAcceptsDeletionVectorOnV3 keeps its explicit accept-path assertion

Approving. What I left inline is optional follow-up, not a hold: with the self-guard back in the helper, the outer if IsDeletionVector(df) in validateDeleteFilesToAdd is now dead code, so I'd drop it and let the helper's guard carry the contract, plus two small test-comment nits. Fold them into this PR or a later one, your call.

[laskoviymishka]

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.

[laskoviymishka]

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.

[laskoviymishka]

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.