zeroshade commented on code in PR #1772:
URL: https://github.com/apache/iceberg-go/pull/1772#discussion_r3833440482
##########
table/arrow_scanner.go:
##########
@@ -522,16 +522,36 @@ type set[T comparable] map[T]struct{}
// batch handed to compute.Take, not into the whole file. Without batch-local
// indices, the second and later batches of a file would pass indices >= the
batch
// length and compute.Take would fail with "index error: N out of bounds".
+//
+// A nil result means that no row in the batch was deleted. The index builder
is
+// allocated lazily after the first deleted row so clean batches can pass
through
+// without reconstructing their columns.
func combinePositionalDeletes(mem memory.Allocator, deletes set[int64], cursor
*rowPositionCursor, nrows int64) arrow.Array {
- bldr := array.NewInt64Builder(mem)
- defer bldr.Release()
+ var bldr *array.Int64Builder
for i := range nrows {
- if _, ok := deletes[cursor.next()]; !ok {
+ if _, deleted := deletes[cursor.next()]; deleted {
+ if bldr == nil {
+ bldr = array.NewInt64Builder(mem)
+ bldr.Reserve(int(nrows))
Review Comment:
Reserving `nrows` regresses delete-heavy batches because the output index
only contains survivors. In a 64K-row all-deleted probe, `main` used ~2.75
KB/op at ~294–299 µs/op, while this branch used ~1.078 MB/op at ~357–364 µs/op.
Reserving only the prefix that must be backfilled (`int(i)`) restored ~2.9
KB/op and ~291–303 µs/op while preserving the lazy-builder optimization. Please
avoid reserving the full batch here.
##########
table/arrow_scanner_posdelete_bench_test.go:
##########
@@ -209,3 +210,52 @@ func benchmarkChunkBoundaries(length, numChunks int) []int
{
return boundaries
}
+
+func BenchmarkProcessPositionalDeletes(b *testing.B) {
+ const numRows = 64 * 1024
+
+ for _, tc := range []struct {
+ name string
+ deletes set[int64]
+ }{
+ {name: "clean", deletes: set[int64]{numRows: {}}},
+ {name: "partial", deletes: set[int64]{numRows / 2: {}}},
Review Comment:
This `partial` case deletes only one row, so it does not expose the
full-batch allocation above. Please add a delete-heavy/all-deleted case so the
benchmark protects both the clean-batch improvement and performance when few
survivor indices are needed.
--
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]
