laskoviymishka commented on code in PR #2025:
URL: https://github.com/apache/iceberg-go/pull/2025#discussion_r4149614992
##########
table/equality_delete_reader.go:
##########
@@ -303,6 +304,28 @@ func newEqualityDeleteFileSet(id int, deleteSet
*equalityDeleteSet) *equalityDel
}
}
+func validateEqualityDeleteMetadata(
+ dataFile iceberg.DataFile,
+ existingFile iceberg.DataFile,
+ existingFieldIDs []int,
+) error {
+ if dataFile != nil && reflect.TypeOf(dataFile).Comparable() && dataFile
== existingFile {
Review Comment:
The `dataFile != nil` check only guards the `reflect.TypeOf` call in this
fast path. If a nil interface ever reached here it short-circuits past the fast
path and then panics two lines down at `dataFileEqualityFieldIDsRef(dataFile)`
and `dataFile.FilePath()`, so the guard reads like it protects the whole
function when it only covers the `if`. Callers all check `ContentType` first so
it can't happen today, but I'd either drop the `!= nil` sub-expression or add a
real top-of-function nil check so the contract is honest.
While we're here: this identity fast path only fires when two tasks share
the exact same pointer, which is a test-only shape. Real manifest entries are
distinct allocations, so in production this always falls through to the
structural compare. Not wrong, but a one-line comment (or scoping the check to
`Kind() == reflect.Ptr`) would save the next reader the double-take.
##########
table/equality_delete_reader.go:
##########
@@ -303,6 +304,28 @@ func newEqualityDeleteFileSet(id int, deleteSet
*equalityDeleteSet) *equalityDel
}
}
+func validateEqualityDeleteMetadata(
+ dataFile iceberg.DataFile,
+ existingFile iceberg.DataFile,
+ existingFieldIDs []int,
+) error {
+ if dataFile != nil && reflect.TypeOf(dataFile).Comparable() && dataFile
== existingFile {
+ return nil
+ }
+
+ fieldIDs := dataFileEqualityFieldIDsRef(dataFile)
+ if len(fieldIDs) == 0 {
+ return fmt.Errorf("%w: equality delete file %s",
ErrEmptyEqualityFieldIDs, dataFile.FilePath())
+ }
+ if dataFile.FileFormat() == existingFile.FileFormat() &&
slices.Equal(fieldIDs, existingFieldIDs) {
+ return nil
+ }
+
+ return fmt.Errorf(
Review Comment:
`ErrEmptyEqualityFieldIDs` and `ErrAmbiguousEqualityColumn` are `%w`-wrapped
sentinels, but this conflict is a bare `fmt.Errorf`, so the test has to
`ErrorContains` on a substring and scan-layer callers can't `errors.Is` it
apart from an I/O or schema failure. I'd give it a sentinel and wrap it:
```go
var ErrConflictingEqualityDeleteMetadata = errors.New("conflicting equality
delete metadata")
```
then `%w` it here and switch the test to `require.ErrorIs`.
##########
table/equality_delete_reader_internal_test.go:
##########
@@ -265,6 +285,134 @@ func
TestReadAllEqualityDeleteFilesRejectsEmptyEqualityFieldIDs(t *testing.T) {
require.ErrorContains(t, err, "empty-equality-fields.parquet")
}
+func TestEqualityDeleteMetadataIsReadOncePerPath(t *testing.T) {
+ t.Parallel()
+
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ )
+
+ base := newEqualityDeleteSetAssemblyTestFile(t,
"mem://metadata-dedup/delete.parquet", []int{1})
+ deleteFile := &countingEqualityFieldDataFile{DataFile: base}
+ tasks := make([]FileScanTask, 100)
+ for i := range tasks {
+ tasks[i] = FileScanTask{EqualityDeleteFiles:
[]iceberg.DataFile{deleteFile}}
+ }
+
+ loader, err := newLazyEqualityDeleteLoader(iceio.NewMemFS(), schema,
nil, nil, tasks)
+ require.NoError(t, err)
+ assert.Len(t, loader.files, 1)
+ assert.Equal(t, 1, deleteFile.borrowedEqualityFieldIDsCalls)
+ assert.Zero(t, deleteFile.equalityFieldIDsCalls)
+
+ fs := iceio.NewMemFS()
+ path := "mem://metadata-dedup/eager-delete.parquet"
+ writeEqualityDeleteParquetToMemFS(t, fs, path, `[{"id": 1}]`)
+ base = newEqualityDeleteSetAssemblyTestFile(t, path, []int{1})
+ deleteFile = &countingEqualityFieldDataFile{DataFile: base}
+ tasks = make([]FileScanTask, 100)
+ for i := range tasks {
+ tasks[i] = FileScanTask{EqualityDeleteFiles:
[]iceberg.DataFile{deleteFile}}
+ }
+
+ perTask, err := readAllEqualityDeleteFiles(t.Context(), fs, schema,
nil, tasks, 1)
+ require.NoError(t, err)
+ assert.Len(t, perTask, len(tasks))
+ assert.Equal(t, 1, deleteFile.borrowedEqualityFieldIDsCalls)
+ assert.Zero(t, deleteFile.equalityFieldIDsCalls)
+}
+
+func TestEqualityDeleteMetadataRejectsConflictingSamePath(t *testing.T) {
+ t.Parallel()
+
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ iceberg.NestedField{ID: 2, Name: "data", Type:
iceberg.PrimitiveTypes.Int64, Required: true},
+ )
+ path := "mem://metadata-conflict/delete.parquet"
+ first := newEqualityDeleteSetAssemblyTestFile(t, path, []int{1, 2})
+
+ avroBuilder, err := iceberg.NewDataFileBuilder(
+ *iceberg.UnpartitionedSpec,
+ iceberg.EntryContentEqDeletes,
+ path,
+ iceberg.AvroFile,
+ nil,
+ nil,
+ nil,
+ 1,
+ 128,
+ )
+ require.NoError(t, err)
+
+ tests := []struct {
+ name string
+ second iceberg.DataFile
Review Comment:
Now that this is table-driven, I'd add a `wantErr` flag and one accept row:
two distinct DataFile objects at the same path, matching format and field IDs,
asserting the dedup succeeds.
```go
{
name: "accept: distinct objects, matching format and IDs",
second: newEqualityDeleteSetAssemblyTestFile(t, path, []int{1, 2}),
wantErr: false,
},
```
Right now the accept branch (`FileFormat` match plus `slices.Equal`
returning nil) has zero coverage: every test either reuses one pointer so the
identity shortcut fires, or expects the reject error. A future change that
inverts the condition or flips the `slices.Equal` args wouldn't be caught.
Related: `TestEqualityDeleteMetadataIsReadOncePerPath` reuses a single pointer,
so it's really asserting read-once-per-pointer, not per-path. The accept row is
what covers the per-path structural case, and it's exactly the branch
@zeroshade's thread is asking about.
##########
table/equality_delete_reader.go:
##########
@@ -303,6 +304,28 @@ func newEqualityDeleteFileSet(id int, deleteSet
*equalityDeleteSet) *equalityDel
}
}
+func validateEqualityDeleteMetadata(
+ dataFile iceberg.DataFile,
+ existingFile iceberg.DataFile,
+ existingFieldIDs []int,
+) error {
+ if dataFile != nil && reflect.TypeOf(dataFile).Comparable() && dataFile
== existingFile {
+ return nil
+ }
+
+ fieldIDs := dataFileEqualityFieldIDsRef(dataFile)
+ if len(fieldIDs) == 0 {
+ return fmt.Errorf("%w: equality delete file %s",
ErrEmptyEqualityFieldIDs, dataFile.FilePath())
+ }
+ if dataFile.FileFormat() == existingFile.FileFormat() &&
slices.Equal(fieldIDs, existingFieldIDs) {
Review Comment:
> REQUEST_CHANGES on the dedup identity
agreed this is the piece to settle. the one thing I'd add:
`slices.Equal(fieldIDs, existingFieldIDs)` is order-sensitive, so the same
physical path with `equality_ids = [1,2]` vs `[2,1]` is a hard conflict, and
the new subtest now locks that in as intended. that's stricter than the spec
and both other clients. equality-delete matching is a set predicate (a row
matches if all equality fields match), Java's `DeleteFileIndex` does no path
dedup or cross-entry check (first-wins), and PyIceberg dedups via `set()`, so a
table they read fine would error here.
root cause is our own key encoding: `readEqualityDeleteFile` builds
composite keys in `fieldIDs` slice order, so the strictness is an artifact of
the encoding, not the spec. the clean fix is to canonicalize (sort ascending by
field ID) when we build the key set and when we compare, so order stops
mattering and we're spec-correct; at minimum a comment on why we reject
permutations. the intent (avoid silent under-deletion) is right and the
practical risk is low, so I don't think this blocks on its own. since it's your
thread, your call on whether to canonicalize now or just document.
--
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]
