laskoviymishka commented on code in PR #2025:
URL: https://github.com/apache/iceberg-go/pull/2025#discussion_r4063623031
##########
table/data_file_stats_ref.go:
##########
@@ -45,6 +45,16 @@ func dataFileCollections(file iceberg.DataFile) (
return internal.BorrowedDataFileCollections(file)
}
+func dataFileEqualityFieldIDs(file iceberg.DataFile) []int {
Review Comment:
This returns the concrete file's internal equality-IDs slice directly, but
the public EqualityFieldIDs() getter clones (slices.Clone), and the Borrowed*
contract on the siblings says these values must not escape the current
operation. Here the returned slice does escape: callers store it in
lazyEqualityDeleteFile.fieldIDs and deleteFileInfo.fieldIDs for the whole scan.
It's safe today only because the lazyEqualityDeleteFile keeps the *dataFile
alive, so the backing array can't be recycled. But the moment someone adds
pooling or a Reset() that reuses that backing array, every retained fieldIDs
across in-flight scans would alias recycled memory and delete-key comparisons
would go wrong, with nothing at the call site hinting the slice was borrowed.
I'd slices.Clone on the fast-path return here. It's one small alloc per
unique file, amortized by the dedup you just added. Failing that, a doc comment
spelling out the retain-with-DataFile contract (the siblings all have one, this
function has none). wdyt?
##########
table/equality_delete_reader_internal_test.go:
##########
@@ -265,6 +276,41 @@ 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.equalityFieldIDsCalls)
Review Comment:
This assertion only proves the fallback path. countingEqualityFieldDataFile
embeds iceberg.DataFile (the interface), and Go won't promote
DataFileCollectionsRef through an embedded interface, so
*countingEqualityFieldDataFile never satisfies it and dataFileEqualityFieldIDs
always takes the EqualityFieldIDs() branch here.
For a real manifest-entry data file it's the other way around:
DataFileCollectionsRef is satisfied, the borrow path is taken, and
EqualityFieldIDs() is never called, so this counter would read 0. So the "read
once per path" claim is verified only for the path production never uses, and a
bug in the DataFileCollectionsRef branch (wrong IDs, or the borrow called more
than once per path) wouldn't be caught.
Could we use a double that actually implements DataFileCollectionsRef (or a
concrete manifest-entry-sourced file) and count borrow invocations instead?
That's what exercises the hot path. wdyt?
##########
table/equality_delete_reader.go:
##########
@@ -390,13 +414,33 @@ func newLazyEqualityDeleteLoader(
}
}
- if len(loader.files) == 0 {
+ if firstFile == nil {
return nil, nil
}
+ if loader.files == nil {
+ firstFile.id = 0
+ loader.files = map[string]*lazyEqualityDeleteFile{firstPath:
firstFile}
+ }
return loader, nil
}
+func (l *lazyEqualityDeleteLoader) needsSchemaHistory() bool {
+ if l == nil {
+ return false
+ }
+
+ for _, file := range l.files {
+ for _, fieldID := range file.fieldIDs {
+ if _, found := l.tableSchema.FindColumnName(fieldID);
!found {
Review Comment:
Small one: the old check used FindFieldByID, and I'd keep it here. It's
equivalent today since both return false for an absent ID, but FindFieldByID is
the "does a field with this ID exist?" question this is actually asking, and it
matches schemaForEqualityFields and the rest of the file.
There's also a sharper edge: FindColumnName goes through IndexNameByID,
which panics on duplicate field names, where FindFieldByID's IndexByID just
overwrites. So a hand-crafted or corrupted schema with dup names that used to
degrade gracefully (treat IDs as absent, load history) would now crash the
scan. Switching back to FindFieldByID sidesteps that. wdyt?
##########
table/equality_delete_reader_internal_test.go:
##########
@@ -265,6 +276,41 @@ func
TestReadAllEqualityDeleteFilesRejectsEmptyEqualityFieldIDs(t *testing.T) {
require.ErrorContains(t, err, "empty-equality-fields.parquet")
}
+func TestEqualityDeleteMetadataIsReadOncePerPath(t *testing.T) {
Review Comment:
needsSchemaHistory() is the part of this change doing the real work, and I
don't think anything exercises its positive path. The pre-existing
TestLazyEqualityDeleteLoaderRejectsFieldAbsentFromSchemaHistory passes
tableSchemas in directly, so it skips the method entirely.
The failure mode that worries me: if needsSchemaHistory() regressed to
always returning false, every test here would still pass, but in production a
delete file referencing a dropped column would never load its historical schema
and would silently stop resolving.
Could we add a test that builds a loader with a field ID absent from the
current schema but present in a historical one, asserts needsSchemaHistory()
returns true, assigns the schemas, and loads the file cleanly, plus the
negative case where all IDs are present? That's the regression guard I'd want
on this before merge. wdyt?
##########
table/equality_delete_reader.go:
##########
@@ -363,25 +363,49 @@ func newLazyEqualityDeleteLoader(
tableSchema: tableSchema,
tableSchemas: tableSchemas,
nameMapping: nameMapping,
- files: make(map[string]*lazyEqualityDeleteFile),
}
+ var firstPath string
+ var firstFile *lazyEqualityDeleteFile
for _, task := range tasks {
for _, dataFile := range task.EqualityDeleteFiles {
if dataFile.ContentType() !=
iceberg.EntryContentEqDeletes {
continue
}
- fieldIDs := dataFile.EqualityFieldIDs()
- if len(fieldIDs) == 0 {
- return nil, fmt.Errorf("%w: equality delete
file %s", ErrEmptyEqualityFieldIDs, dataFile.FilePath())
- }
-
path := dataFile.FilePath()
+ if loader.files == nil {
+ if firstFile == nil {
+ fieldIDs :=
dataFileEqualityFieldIDs(dataFile)
+ if len(fieldIDs) == 0 {
+ return nil, fmt.Errorf("%w:
equality delete file %s", ErrEmptyEqualityFieldIDs, path)
+ }
+
+ firstPath = path
+ firstFile = &lazyEqualityDeleteFile{
+ dataFile: dataFile,
+ fieldIDs: fieldIDs,
+ }
+
+ continue
+ }
+ if path == firstPath {
+ continue
+ }
+
+ loader.files =
make(map[string]*lazyEqualityDeleteFile, 2)
+ firstFile.id = 0
Review Comment:
firstFile is built with id already at its zero value, so this firstFile.id =
0 (and the matching one at line 421) is a no-op. It makes a reader stop to
check whether id could have been something else. I'd drop both; if the "first
file is always id 0" invariant is worth stating, a one-line comment says it
more clearly than the self-assignment.
##########
table/equality_delete_reader.go:
##########
@@ -522,17 +566,21 @@ func readAllEqualityDeleteFiles(ctx context.Context, fs
iceio.IO, schema *iceber
continue
}
- if len(d.EqualityFieldIDs()) == 0 {
- return nil, fmt.Errorf("%w: equality delete
file %s", ErrEmptyEqualityFieldIDs, d.FilePath())
+ path := d.FilePath()
+ if _, ok := uniqueDeletes[path]; ok {
+ continue
+ }
+
+ fieldIDs := dataFileEqualityFieldIDs(d)
+ if len(fieldIDs) == 0 {
+ return nil, fmt.Errorf("%w: equality delete
file %s", ErrEmptyEqualityFieldIDs, path)
}
hasAny = true
Review Comment:
After the reorder, hasAny is set only once a new entry is about to be
inserted, so it's now exactly equivalent to len(uniqueDeletes) > 0. It reads as
extra state to trace across three continue paths, and if someone later slips a
continue between hasAny = true and the insert the invariant breaks silently.
I'd drop hasAny and gate the return on len(uniqueDeletes) == 0 instead.
--
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]
