zeroshade commented on code in PR #2025:
URL: https://github.com/apache/iceberg-go/pull/2025#discussion_r4065519330
##########
table/equality_delete_reader.go:
##########
@@ -514,30 +556,32 @@ func readAllEqualityDeleteFiles(ctx context.Context, fs
iceio.IO, schema *iceber
}
uniqueDeletes := make(map[string]deleteFileInfo)
Review Comment:
Whatever identity you settle on above needs to carry through here and
through the per-task lookup, since both resolve entries by path today. Worth
one regression with the same path and differing ordered equality IDs that
covers the lazy and eager paths together.
##########
table/equality_delete_reader.go:
##########
@@ -363,25 +363,48 @@ 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
+ }
Review Comment:
The dedup identity is the file path alone, and the skip happens before this
entry's equality IDs are ever read:
```go
if path == firstPath {
continue
}
```
Two equality-delete entries at the same path carrying different ordered
equality field IDs — say `[1]` and `[2]` — collapse to the first.
`dataFileEqualityFieldIDs` never observes field 2, so `needsSchemaHistory` and
`addFieldIDs` omit it, and a task referencing the second entry resolves through
the same path-keyed map and applies the first entry's delete set. That is a
wrong result, not a missed optimization. A focused regression expecting field 2
to trigger schema-history loading fails at this head.
Key the map on an end-to-end metadata identity — the path plus the ordered
equality field IDs, plus anything else with read semantics such as format — or
explicitly reject same-path entries whose metadata conflicts.
--
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]
