laskoviymishka commented on code in PR #1250:
URL: https://github.com/apache/iceberg-go/pull/1250#discussion_r3466834135
##########
table/arrow_scanner.go:
##########
@@ -358,38 +355,59 @@ func processPositionalDeletes(ctx context.Context,
deletes set[int64]) recProces
// (GC-friendly) and shared across every per-batch Boolean array; each
// array.NewBoolean / array.NewSlice pair is released after the batch.
//
-// rowCount bounds the mask to the data file's row count. The closure-captured
-// nextIdx tracks absolute position across batches, mirroring
-// processPositionalDeletes.
-func filterByDeletionVector(ctx context.Context, bitmap
*dv.RoaringPositionBitmap, rowCount int64) recProcessFn {
+// rowCount bounds the mask to the data file's row count. cursor maps each
batch
+// row to its original file position: when no row group was pruned the
positions
+// are contiguous and the mask is sliced zero-copy; when pruning skipped groups
+// the emitted rows are non-contiguous, so the keep bits are gathered per row
at
+// the cursor's positions.
+func filterByDeletionVector(ctx context.Context, bitmap
*dv.RoaringPositionBitmap, rowCount int64, cursor *rowPositionCursor)
recProcessFn {
nextIdx := int64(0)
keepBits := bitmap.KeepMaskBytes(rowCount)
buf := memory.NewBufferBytes(keepBits)
+ mem := compute.GetAllocator(ctx)
return func(r arrow.RecordBatch) (arrow.RecordBatch, error) {
defer r.Release()
- currentIdx := nextIdx
- nextIdx += r.NumRows()
+ nrows := r.NumRows()
+
+ if !cursor.src.pruned() {
+ currentIdx := nextIdx
+ nextIdx += nrows
+
+ // Wrap (and slice) the shared keep-mask buffer for
this batch.
+ // array.NewSlice on a Boolean array tracks the
bit-level offset,
+ // so we don't need byte-aligned slicing — currentIdx
can land
+ // anywhere within a byte.
+ full := array.NewBoolean(int(rowCount), buf, nil, 0)
+ defer full.Release()
+ sliced := array.NewSlice(full, currentIdx,
nextIdx).(*array.Boolean)
+ defer sliced.Release()
+
+ return compute.FilterRecordBatch(ctx, r, sliced,
compute.DefaultFilterOptions())
+ }
- // Wrap (and slice) the shared keep-mask buffer for this batch.
- // array.NewSlice on a Boolean array tracks the bit-level
offset,
- // so we don't need byte-aligned slicing — currentIdx can land
- // anywhere within a byte.
- full := array.NewBoolean(int(rowCount), buf, nil, 0)
- defer full.Release()
- sliced := array.NewSlice(full, currentIdx,
nextIdx).(*array.Boolean)
- defer sliced.Release()
+ bldr := array.NewBooleanBuilder(mem)
+ defer bldr.Release()
+ bldr.Reserve(int(nrows))
+ for range nrows {
+ pos := cursor.next()
+ // Test keep bit at absolute position pos: byte pos/8,
bit pos%8,
+ // LSB-first (the layout the fast path's
array.NewBoolean reads).
+ bldr.Append(keepBits[pos>>3]&(1<<(uint(pos)&7)) != 0)
Review Comment:
This is the one thing I'd want settled before merge.
`keepBits` is sized `⌈rowCount/8⌉` from the manifest's `File.Count()`, but
`pos` here comes from `cursor.next()` walking `RowGroupSpan`s drawn from
Parquet row-group metadata. If those two ever disagree — a writer bug, a
hand-built file, corruption — `pos` can reach or exceed `rowCount` and
`keepBits[pos>>3]` is an out-of-range index that panics. The fast path is
immune because `array.NewSlice` validates; this slow path has no guard.
It also contradicts the `next()` contract a few lines up ("reading past the
total row count degrades... rather than panicking") — that only holds for
callers that don't index a bounded slice, and this one does. I'd add the guard
and keep-by-default:
```go
if pos >= rowCount {
bldr.Append(true)
continue
}
```
If we'd rather treat an overshoot as a true invariant violation, the
alternative is to panic here with a message that names the position overflow —
but a silent OOB is the worst of both. 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]
