laskoviymishka commented on code in PR #2157:
URL: https://github.com/apache/iceberg-go/pull/2157#discussion_r4232288074
##########
table/equality_delete_reader_internal_test.go:
##########
@@ -864,6 +864,84 @@ func
TestProcessEqualityDeletesReturnsOriginalBatchWhenNoRowsMatch(t *testing.T)
result.Release()
}
+func TestProcessEqualityDeletesSingleInt64FastPathHandlesNullAndNegative(t
*testing.T) {
+ mem := memory.NewCheckedAllocator(memory.DefaultAllocator)
+ defer mem.AssertSize(t, 0)
+ ctx := compute.WithAllocator(t.Context(), mem)
+
+ builder := array.NewInt64Builder(mem)
+ builder.Append(1)
+ builder.AppendNull()
+ builder.Append(-7)
+ builder.Append(9)
+ values := builder.NewArray()
+ builder.Release()
+
+ schema := arrow.NewSchema([]arrow.Field{{
+ Name: "id", Type: arrow.PrimitiveTypes.Int64,
+ }}, nil)
+ record := array.NewRecordBatch(schema, []arrow.Array{values}, 4)
+ values.Release()
+
+ deleteSet := int64EqualityDeleteSet(1, -7)
+ deleteSet.keys[string([]byte{0})] = struct{}{}
+
+ fileSchema := iceberg.NewSchema(0,
+ iceberg.NestedField{ID: 1, Name: "id", Type:
iceberg.PrimitiveTypes.Int64},
+ )
+ process, err := processEqualityDeletesColumnarForFile(ctx,
+ []*equalityDeleteSet{deleteSet}, fileSchema, "data.parquet")
+ require.NoError(t, err)
+
+ result, err := process(record)
+ require.NoError(t, err)
+ require.Equal(t, int64(1), result.NumRows())
Review Comment:
This hand-asserts 4 rows, which pins the result but doesn't guard against
the fast path drifting from the generic one. I'd run the same batch twice —
once with `fileSchema == nil` to force the generic path, once with the schema —
and compare outputs, covering `math.MinInt64`/`math.MaxInt64`/`-1` for the full
8-byte round-trip, nulls kept when `hasNull` is false, and a sliced array with
a non-zero offset. That's the standard guard for a fast path and it's cheap to
add here.
##########
table/equality_delete_reader.go:
##########
@@ -1331,10 +1392,72 @@ func processEqualityDeletesColumnarForFile(ctx
context.Context, eqDeleteSets []*
var maskBuf *memory.Buffer
var maskBytes []byte
Review Comment:
The bool return is a bit of a trap: each of the three call sites has to pair
`if ensureMask()` with its own `defer maskBuf.Release()`, so forgetting the
pairing leaks and deferring unconditionally double-releases. I'd have
`ensureMask` own the cleanup — drop a `defer func() { if maskBuf != nil {
maskBuf.Release() } }()` next to the `maskBuf` declaration and make
`ensureMask` return nothing. That removes the trap and lets each loop drop a
branch.
##########
table/equality_delete_reader_internal_test.go:
##########
@@ -864,6 +864,84 @@ func
TestProcessEqualityDeletesReturnsOriginalBatchWhenNoRowsMatch(t *testing.T)
result.Release()
}
Review Comment:
We don't have a test that applies a fast-path set and a generic set to the
same batch, which is exactly where the lazily-created mask is shared across
both paths. I'd add one with two sets in one call — both orderings
(fast→generic and generic→fast), partial overlap so the second set has to skip
rows the first already cleared — under the `CheckedAllocator`. The `maskBytes
!= nil` skip and the single release are the parts of this refactor most likely
to break.
##########
table/equality_delete_reader.go:
##########
@@ -1288,6 +1291,55 @@ func makeColEncoder(arr arrow.Array) colEncoder {
}
}
+type singleInt64EqualityDeleteSet struct {
+ keys set[uint64]
+ hasNull bool
+}
+
+func (e *equalityDeleteSet) singleInt64Set(fileSchema *iceberg.Schema)
*singleInt64EqualityDeleteSet {
+ if fileSchema == nil || len(e.fieldIDs) != 1 {
+ return nil
+ }
+
+ field, ok := fileSchema.FindFieldByID(e.fieldIDs[0])
+ if !ok {
+ return nil
+ }
+ if _, ok := field.Type.(iceberg.Int64Type); !ok {
+ return nil
+ }
+
+ e.singleInt64Once.Do(func() {
+ result := &singleInt64EqualityDeleteSet{
+ keys: make(set[uint64], len(e.keys)),
+ }
+ for key := range e.keys {
+ if len(key) == 1 && key[0] == 0 {
+ result.hasNull = true
+
+ continue
+ }
Review Comment:
Worth a one-line comment that an unexpected key shape is deliberately left
to fall back to the generic path, plus a test that feeds a malformed key (short
key or wrong tag byte) and asserts the generic path still deletes the matches.
It's unreachable today since keys always come from `bufPutUint64`, but if the
key format ever changes — a version byte, a different null sentinel — this
stops firing silently and the perf win just disappears with nothing failing.
Decoding through a shared helper next to `bufPutUint64` would be even better,
so the encode/decode format has one source of truth.
##########
table/equality_delete_reader.go:
##########
@@ -1331,10 +1392,72 @@ func processEqualityDeletesColumnarForFile(ctx
context.Context, eqDeleteSets []*
var maskBuf *memory.Buffer
var maskBytes []byte
+ ensureMask := func() bool {
+ if maskBuf != nil {
+ return false
+ }
+
+ maskBuf = memory.NewResizableBuffer(mem)
+
maskBuf.Resize(int(bitutil.BytesForBits(int64(numRows))))
+ maskBytes = maskBuf.Bytes()
+
+ for i := range maskBytes {
+ maskBytes[i] = 0xFF
+ }
+
+ return true
+ }
var keyBuf bytes.Buffer
for setIdx, eqDel := range eqDeleteSets {
+ if int64Set := singleInt64Sets[setIdx]; int64Set != nil
{
+ column, parents, err := arrowArraysAtField(
+ r, fieldRefs[setIdx][0],
eqDel.fieldIDs[0], eqDel.colNames[0], dataFilePath)
+ if err != nil {
+ return nil, err
+ }
+ if len(parents) == 0 {
+ if values, ok := column.(*array.Int64);
ok {
+ rawValues :=
values.Int64Values()
+ if values.NullN() == 0 {
+ for row := range
numRows {
+ if maskBytes !=
nil && !bitutil.BitIsSet(maskBytes, row) {
+ continue
+ }
+ if _, deleted
:= int64Set.keys[uint64(rawValues[row])]; deleted {
+ if
ensureMask() {
+
defer maskBuf.Release()
+ }
+
bitutil.ClearBit(maskBytes, row)
+ }
+ }
+ } else {
+ for row := range
numRows {
+ if maskBytes !=
nil && !bitutil.BitIsSet(maskBytes, row) {
+ continue
Review Comment:
`deleted := false` is overwritten in both branches, so the initial
assignment is dead and `ineffassign`/`wastedassign` may flag it. A
continue-style match of the generic loop reads cleaner:
```go
if values.IsNull(row) {
if !int64Set.hasNull {
continue
}
} else if _, ok := int64Set.keys[uint64(rawValues[row])]; !ok {
continue
}
```
##########
table/equality_delete_reader.go:
##########
@@ -1288,6 +1291,55 @@ func makeColEncoder(arr arrow.Array) colEncoder {
}
}
+type singleInt64EqualityDeleteSet struct {
+ keys set[uint64]
+ hasNull bool
+}
+
+func (e *equalityDeleteSet) singleInt64Set(fileSchema *iceberg.Schema)
*singleInt64EqualityDeleteSet {
+ if fileSchema == nil || len(e.fieldIDs) != 1 {
+ return nil
+ }
+
+ field, ok := fileSchema.FindFieldByID(e.fieldIDs[0])
+ if !ok {
+ return nil
+ }
+ if _, ok := field.Type.(iceberg.Int64Type); !ok {
+ return nil
+ }
+
+ e.singleInt64Once.Do(func() {
+ result := &singleInt64EqualityDeleteSet{
Review Comment:
The typed index is kept alongside the string `set` for the life of the
shared set — roughly 8B plus map overhead per key on top of the existing string
keys. Fine as a deliberate trade-off, but worth a line in the struct doc so
it's not a surprise for large delete files. While we're here, `sync.Once` makes
`equalityDeleteSet` no-copy, so a quick `go vet -tags integration ./...` to
confirm nothing copies it by value (plain vet skips the build-tagged files).
##########
table/equality_delete_reader_bench_test.go:
##########
@@ -528,6 +528,50 @@ func buildBenchDeleteSetStringNoMatch(numDeletes int)
*equalityDeleteSet {
}
}
+func buildBenchDeleteSetSingleInt64(numDeletes int) *equalityDeleteSet {
+ keys := make(set[string], numDeletes)
+ var buf bytes.Buffer
+
+ for i := range numDeletes {
+ buf.Reset()
+ buf.WriteByte(1)
+ bufPutUint64(&buf, uint64(int64(i)*3))
+ keys[buf.String()] = struct{}{}
+ }
+
+ return &equalityDeleteSet{
+ keys: keys,
+ fieldIDs: []int{1},
+ colNames: []string{"id"},
+ }
+}
+
+func buildBenchDeleteSetSingleInt64NoMatch(numDeletes int) *equalityDeleteSet {
+ keys := make(set[string], numDeletes)
+ var buf bytes.Buffer
+
+ for i := range numDeletes {
+ buf.Reset()
+ buf.WriteByte(1)
+ bufPutUint64(&buf, uint64(-int64(i)-1))
+ keys[buf.String()] = struct{}{}
+ }
+
+ return &equalityDeleteSet{
+ keys: keys,
+ fieldIDs: []int{1},
+ colNames: []string{"id"},
+ }
+}
+
Review Comment:
Both benchmarks feed no-null data, so the null-aware loop never gets
measured — which is also the evidence for whether that second loop earns its
place. A null-heavy variant would close that. Also worth stating in the PR
numbers that the "before" figures came from the same builder run against main,
since these benchmark functions don't exist there.
--
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]
