Re: [PR] perf(table): deduplicate equality delete metadata setup [iceberg-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org