You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "twuebi (via GitHub)" <gi...@apache.org> on 2026/06/22 14:21:43 UTC
[PR] fix(v3): re-enable row-group pruning with correct row lineage tracking [iceberg-go]
twuebi opened a new pull request, #1250:
URL: https://github.com/apache/iceberg-go/pull/1250
Follow-up to #1246 and #1237
Re-enable row-group pruning while correctly tracking row lineage & add adequate tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(v3): re-enable row-group pruning with correct row lineage tracking [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(v3): re-enable row-group pruning with correct row lineage tracking [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1250:
URL: https://github.com/apache/iceberg-go/pull/1250
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(v3): re-enable row-group pruning with correct row lineage tracking [iceberg-go]
Posted by "twuebi (via GitHub)" <gi...@apache.org>.
twuebi commented on code in PR #1250:
URL: https://github.com/apache/iceberg-go/pull/1250#discussion_r3467188213
##########
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:
added the guard, thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org