laskoviymishka commented on code in PR #2016:
URL: https://github.com/apache/iceberg-go/pull/2016#discussion_r4045049148
##########
table/row_delta.go:
##########
@@ -198,8 +198,10 @@ func (rd *RowDelta) Commit(ctx context.Context) error {
ct, f.FilePath())
}
- if err := validateDeletionVectorFormatVersion(f,
meta.formatVersion, "row delta"); err != nil {
- return err
+ if IsDeletionVector(f) {
Review Comment:
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?
##########
table/transaction.go:
##########
@@ -1191,9 +1191,27 @@ type deleteFilesToAddSet struct {
dvsByRef map[string]rewriteDeleteFileAddition
}
-func validateDeletionVectorFormatVersion(df iceberg.DataFile, formatVersion
int, operation string) error {
- if IsDeletionVector(df) && formatVersion < 3 {
- return fmt.Errorf("deletion vector %s requires table format
version >= 3 for %s", df.FilePath(), operation)
+func validateDeletionVectorToAdd(df iceberg.DataFile, formatVersion int,
operation string) error {
Review Comment:
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?
##########
table/transaction.go:
##########
@@ -1289,24 +1303,10 @@ func (t *Transaction)
validateDeleteFilesToAdd(deleteFiles []rewriteDeleteFileAd
}
if IsDeletionVector(df) {
- ref := df.ReferencedDataFile()
- if ref == nil || *ref == "" {
- return nil, fmt.Errorf("deletion vector to add
is missing referenced_data_file for %s", operation)
- }
- offset := df.ContentOffset()
- if offset == nil {
- return nil, fmt.Errorf("deletion vector %s is
missing content_offset for %s", path, operation)
- }
- if *offset < 0 {
- return nil, fmt.Errorf("deletion vector %s has
invalid content_offset %d for %s", path, *offset, operation)
- }
- length := df.ContentSizeInBytes()
- if length == nil {
- return nil, fmt.Errorf("deletion vector %s is
missing content_size_in_bytes for %s", path, operation)
- }
- if *length <= 0 {
- return nil, fmt.Errorf("deletion vector %s has
invalid content_size_in_bytes %d for %s", path, *length, operation)
+ if err := validateDeletionVectorToAdd(df,
meta.formatVersion, operation); err != nil {
+ return nil, err
}
+ ref, offset, length := df.ReferencedDataFile(),
df.ContentOffset(), df.ContentSizeInBytes()
Review Comment:
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.
##########
table/row_delta_test.go:
##########
@@ -1190,22 +1204,15 @@ func TestRowDeltaRejectsDeletionVectorBelowV3(t
*testing.T) {
}
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()))
- tbl, err := tx.Commit(t.Context())
- require.NoError(t, err)
+ tbl, _, dataPath, dv := newTableWithLiveDV(t)
Review Comment:
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.
##########
table/transaction.go:
##########
@@ -1191,9 +1191,27 @@ type deleteFilesToAddSet struct {
dvsByRef map[string]rewriteDeleteFileAddition
}
-func validateDeletionVectorFormatVersion(df iceberg.DataFile, formatVersion
int, operation string) error {
- if IsDeletionVector(df) && formatVersion < 3 {
- return fmt.Errorf("deletion vector %s requires table format
version >= 3 for %s", df.FilePath(), operation)
+func validateDeletionVectorToAdd(df iceberg.DataFile, formatVersion int,
operation string) error {
+ path := df.FilePath()
+ if formatVersion < 3 {
+ return fmt.Errorf("deletion vector %s requires table format
version >= 3 for %s", path, operation)
+ }
+ if ref := df.ReferencedDataFile(); ref == nil || *ref == "" {
+ return fmt.Errorf("deletion vector %s is missing
referenced_data_file for %s", path, operation)
Review Comment:
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.
--
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]
