laskoviymishka commented on code in PR #1902:
URL: https://github.com/apache/iceberg-go/pull/1902#discussion_r3874734366
##########
table/equality_delete_reader.go:
##########
@@ -183,10 +183,19 @@ func makeArrowFieldEncoder(record arrow.RecordBatch, ref
arrowFieldRef, fieldID
}
type equalityDeleteFileSet struct {
- id int
+ id int
+ groupKey string
*equalityDeleteSet
}
+func newEqualityDeleteFileSet(id int, deleteSet *equalityDeleteSet)
*equalityDeleteFileSet {
+ return &equalityDeleteFileSet{
+ id: id,
+ groupKey: fmt.Sprint(deleteSet.fieldIDs),
Review Comment:
Caching the key here so it's computed once per unique delete file is the
right call.
One thing worth flagging while this is the single place the key gets built:
`fmt.Sprint(fieldIDs)` is order-sensitive, so `[1 2]` and `[2 1]` land in
separate groups, whereas Java keys on a `Set<Integer>` (via
`TypeUtil.selectInIdOrder`) and treats any permutation of the same IDs as one
group. Two delete files covering the same equality columns but storing
`equality_ids` in different orders would merge under Java and split here. It's
pre-existing, not something this PR introduces, but since the key is now
canonicalized to one spot, sorting `fieldIDs` ascending before the `Sprint`
(and before `colNames`) would close the gap cheaply and match Java. At minimum
a comment that the format has to stay collision-free for `[]int` would help.
wdyt?
##########
table/equality_delete_reader.go:
##########
@@ -298,16 +304,45 @@ func buildEqualityDeleteSetsPerTask(
continue
}
- // Group delete files by their field IDs key.
- groups := make(map[string][]*equalityDeleteFileSet)
+ var (
+ groupKey string
+ groupFiles []*equalityDeleteFileSet
+ groups map[string][]*equalityDeleteFileSet
+ )
+
for _, d := range t.EqualityDeleteFiles {
dk, ok := perFile[d.FilePath()]
if !ok {
continue
}
- groupKey := fmt.Sprint(dk.fieldIDs)
- groups[groupKey] = append(groups[groupKey], dk)
+ if groups == nil {
+ if len(groupFiles) == 0 {
+ groupKey = dk.groupKey
+ } else if dk.groupKey != groupKey {
+ groups =
make(map[string][]*equalityDeleteFileSet, 2)
+ groups[groupKey] = groupFiles
+ }
+ }
+
+ if groups == nil {
+ groupFiles = append(groupFiles, dk)
+ } else {
+ groups[dk.groupKey] =
append(groups[dk.groupKey], dk)
Review Comment:
This path, a file with the original key arriving after the map's been
promoted, has no test behind it. Neither existing test builds a task where
`groupFiles` holds more than one file at the moment of promotion, or where a
later file lands back on the already-migrated key.
I'd add two cases to
`TestBuildEqualityDeleteSetsPerTaskKeepsFieldGroupsSeparate`: `[A=[1], B=[1],
C=[2]]` (two group-1 files accumulate, then `groups["[1]"] = groupFiles`
assigns a 2-element slice), and `[A=[1], B=[2], C=[1]]` (promotion fires on B,
then C appends back through this line into the migrated `[1]` group). Assert
the `[1]` group ends up with both A and C in the second case.
That second layout is the one I care about most. It's the only path where
the migrated key takes a further append, and it's exactly what a future edit to
the promotion block could silently break.
##########
table/equality_delete_reader.go:
##########
@@ -298,16 +304,45 @@ func buildEqualityDeleteSetsPerTask(
continue
}
- // Group delete files by their field IDs key.
- groups := make(map[string][]*equalityDeleteFileSet)
+ var (
+ groupKey string
+ groupFiles []*equalityDeleteFileSet
+ groups map[string][]*equalityDeleteFileSet
+ )
+
for _, d := range t.EqualityDeleteFiles {
dk, ok := perFile[d.FilePath()]
if !ok {
continue
}
- groupKey := fmt.Sprint(dk.fieldIDs)
- groups[groupKey] = append(groups[groupKey], dk)
+ if groups == nil {
+ if len(groupFiles) == 0 {
+ groupKey = dk.groupKey
+ } else if dk.groupKey != groupKey {
+ groups =
make(map[string][]*equalityDeleteFileSet, 2)
+ groups[groupKey] = groupFiles
+ }
+ }
+
+ if groups == nil {
+ groupFiles = append(groupFiles, dk)
+ } else {
+ groups[dk.groupKey] =
append(groups[dk.groupKey], dk)
+ }
+ }
+
+ if groups == nil {
+ if len(groupFiles) == 0 {
+ continue
Review Comment:
This new early-`continue` (a task whose delete files are all absent from
`perFile`) isn't hit by either existing test, since both always match. A
one-line case with a task whose delete paths aren't keys in `perFile`,
asserting `perTask` has no entry for that index, would lock it in. Fine as a
follow-up.
##########
table/equality_delete_reader.go:
##########
@@ -298,16 +304,45 @@ func buildEqualityDeleteSetsPerTask(
continue
}
- // Group delete files by their field IDs key.
- groups := make(map[string][]*equalityDeleteFileSet)
+ var (
+ groupKey string
+ groupFiles []*equalityDeleteFileSet
+ groups map[string][]*equalityDeleteFileSet
+ )
+
for _, d := range t.EqualityDeleteFiles {
dk, ok := perFile[d.FilePath()]
if !ok {
continue
}
- groupKey := fmt.Sprint(dk.fieldIDs)
- groups[groupKey] = append(groups[groupKey], dk)
+ if groups == nil {
Review Comment:
These two consecutive `if groups == nil` blocks read like independent
guards, but they aren't. The first can flip `groups` from nil to a live map,
and the second observes that flip to decide where the current file goes. The
transition branch deliberately doesn't append `dk` itself; it relies on
fall-through to the second check to do it, which is the hardest part to see,
and a stray `continue` or a hoisted block would silently break the invariant
that each file is added exactly once.
An `if/else if/else` makes the three per-iteration paths explicit, and lets
us null out `groupFiles` right after it's handed to the map so nothing can
later append into the shared backing array:
```go
if groups != nil {
groups[dk.groupKey] = append(groups[dk.groupKey], dk)
} else if len(groupFiles) == 0 {
groupKey = dk.groupKey
groupFiles = append(groupFiles, dk)
} else if dk.groupKey != groupKey {
groups = make(map[string][]*equalityDeleteFileSet, 2)
groups[groupKey] = groupFiles
groupFiles = nil
groups[dk.groupKey] = append(groups[dk.groupKey], dk)
} else {
groupFiles = append(groupFiles, dk)
}
```
wdyt?
--
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]
