zeroshade commented on code in PR #2025:
URL: https://github.com/apache/iceberg-go/pull/2025#discussion_r4158574875
##########
table/equality_delete_reader.go:
##########
@@ -303,6 +307,35 @@ func newEqualityDeleteFileSet(id int, deleteSet
*equalityDeleteSet) *equalityDel
}
}
+func validateEqualityDeleteMetadata(
+ dataFile iceberg.DataFile,
+ existingFile iceberg.DataFile,
+ existingFieldIDs []int,
+) error {
+ // Callers have already inspected ContentType, so both files must be
non-nil.
+ // Only skip validation for the same immutable file pointer: a
comparable
+ // struct may still contain an interface holding a non-comparable value.
+ if reflect.TypeOf(dataFile).Kind() == reflect.Ptr && dataFile ==
existingFile {
Review Comment:
Blocker: `reflect.Ptr` fails the govet `inline` check (`Constant reflect.Ptr
should be inlined`). That is the only failure in all four red lint-and-test
jobs, and because lint runs before `make test-race`, CI ran no tests on this
commit. `go test -race ./table` passes locally. Use `reflect.Pointer`.
Keep the same-pointer shortcut itself. @laskoviymishka, r4149614992 calls
this a test-only shape, but real scans take it constantly: planning hands every
matching task the same `*dataFile` for a delete entry
(`appendEqualityDeletesAfter` appends `entry.entry.DataFile()`, and
`dataFilesWithoutColumnStats` memoizes one projected copy per original). This
is the common path, not the fallback.
##########
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
+ }{
+ {
+ name: "equality field IDs",
+ second: newEqualityDeleteSetAssemblyTestFile(t, path,
[]int{2, 1}),
+ },
+ {
+ name: "file format",
+ second: avroBuilder.EqualityFieldIDs([]int{1,
2}).Build(),
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ tasks := []FileScanTask{
+ {EqualityDeleteFiles:
[]iceberg.DataFile{first}},
+ {EqualityDeleteFiles:
[]iceberg.DataFile{tt.second}},
+ }
+
+ _, err := newLazyEqualityDeleteLoader(iceio.NewMemFS(),
schema, nil, nil, tasks)
+ require.ErrorContains(t, err, "conflicting equality
delete metadata")
+ require.ErrorContains(t, err, path)
+
+ _, err = readAllEqualityDeleteFiles(t.Context(),
iceio.NewMemFS(), schema, nil, tasks, 1)
+ require.ErrorContains(t, err, "conflicting equality
delete metadata")
+ require.ErrorContains(t, err, path)
+ })
+ }
+}
Review Comment:
This is fully covered by `TestEqualityDeleteMetadataConflictErrors`
(equality_delete_metadata_validation_test.go:127-167): the same reordered-IDs
and Avro rows, plus `ErrorIs`, the different- and empty-ID rows, and a zero-I/O
assertion. This one still matches on the error string, which r4149615008 asked
to replace. Please delete it. Also consider moving the new 167-line file into
this one, where the rest of the equality-delete loader tests live.
##########
table/equality_delete_reader.go:
##########
@@ -303,6 +307,35 @@ func newEqualityDeleteFileSet(id int, deleteSet
*equalityDeleteSet) *equalityDel
}
}
+func validateEqualityDeleteMetadata(
+ dataFile iceberg.DataFile,
+ existingFile iceberg.DataFile,
+ existingFieldIDs []int,
+) error {
+ // Callers have already inspected ContentType, so both files must be
non-nil.
+ // Only skip validation for the same immutable file pointer: a
comparable
+ // struct may still contain an interface holding a non-comparable value.
+ if reflect.TypeOf(dataFile).Kind() == reflect.Ptr && dataFile ==
existingFile {
+ return nil
+ }
+
+ fieldIDs := dataFileEqualityFieldIDsRef(dataFile)
+ if len(fieldIDs) == 0 {
+ return fmt.Errorf("%w: equality delete file %s",
ErrEmptyEqualityFieldIDs, dataFile.FilePath())
+ }
+ // This intentionally rejects reordered IDs, although equality matching
itself
+ // is order-independent. Keep this dedup change aligned with the
existing
+ // order-sensitive key encoding/grouping; canonicalization is a
separate change.
+ if dataFile.FileFormat() == existingFile.FileFormat() &&
slices.Equal(fieldIDs, existingFieldIDs) {
Review Comment:
@laskoviymishka, this is my ruling on r4149615004: compare same-path IDs as
sets, not with `slices.Equal`.
Each path gets one file set, and the `fieldIDs` of that set drive both the
delete-file read (`loadFile`, :500 and :508) and row matching
(`processEqualityDeletesColumnarForFile`, :1294). Tasks resolve the set by path
(:543), so nothing after construction reads the order of a later entry. The
same path with `[1,2]` and then `[2,1]` therefore scans correctly on main
today, and this check turns it into `ErrConflictingEqualityDeleteMetadata`.
Java groups equality deletes by `Sets.newHashSet(delete.equalityFieldIds())`
for the same reason.
Fix: require the same length and that every ID in each slice appears in the
other (a nested loop over a few IDs, no allocation). `[1]` vs `[2]` must still
be rejected. Update the comment above, and flip the "reordered IDs" row in
`TestEqualityDeleteMetadataConflictErrors`
(equality_delete_metadata_validation_test.go:143) to expect success; the other
reordered row goes away with the duplicate test. Normalizing the key encoding
across different paths stays out of scope.
##########
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)
+}
Review Comment:
Nit: both loaders here see one shared pointer, so after the first reference
this counts the same-pointer shortcut in `validateEqualityDeleteMetadata`, not
dedup by path. It will break on harmless changes to that shortcut.
`TestEqualityDeleteMetadataAcceptsDistinctSamePathFiles` already covers the
behavior through I/O counts, and its "borrowed metadata" rows exercise the
`DataFileCollectionsRef` path with distinct objects. @laskoviymishka, this is
the double you asked for in r4063623041; I am fine with dropping or keeping it.
--
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]
