zeroshade commented on code in PR #864:
URL: https://github.com/apache/arrow-go/pull/864#discussion_r3554372419
##########
parquet/metadata/bloom_filter_test.go:
##########
@@ -721,3 +723,23 @@ func TestGetSpacedHashesFromBitmap(t *testing.T) {
assert.Equal(t, int(numValid), trueCount+falseCount, "all
hashes should be true or false")
})
}
+
+func TestRecycleEncryptedPathPool(t *testing.T) {
+ pool := &sync.Pool{New: func() any { return
memory.NewResizableBuffer(memory.NewGoAllocator()) }}
+ r := &RowGroupBloomFilterReader{bufferPool: pool}
+
+ bitset := make([]byte, 1024)
+ bf := &blockSplitBloomFilter{
+ data: memory.NewBufferBytes(bitset),
+ bitset32: arrow.Uint32Traits.CastFromBytes(bitset),
+ }
+
+ r.recycle(bf)
+
+ got := pool.Get().(*memory.Buffer)
+ if got.Mutable() {
Review Comment:
Non-blocking: this guard is non-deterministic. `sync.Pool.Get()` may hand
back a freshly `New()`-ed (mutable) buffer even if `recycle` had wrongly pooled
the nil-allocator one, in which case this early-`return`s and the test passes
without ever hitting the `ResizeNoShrink` panic path. With the current
(correct) code it always passes, so as a regression guard it's weak. Consider
asserting deterministically that `recycle` did not enqueue the encrypted buffer
— e.g. recycle into a freshly-created empty pool and assert the buffer it holds
is the allocator-backed one (`Mutable()`), or compare the specific pointer.
##########
parquet/metadata/bloom_filter.go:
##########
@@ -549,6 +551,46 @@ func (r *RowGroupBloomFilterReader) GetColumnBloomFilter(i
int) (BloomFilter, er
return bf, nil
}
+// VisitColumnBloomFilter invokes fn for the BloomFilter of the column at
index i.
+//
+// Lifetime contract: The BloomFilter passed to fn (and its backing bitset)
+// is recycled immediately after fn returns. Callers must not retain, store,
+// or use the BloomFilter or its data outside the scope of the fn function.
+func (r *RowGroupBloomFilterReader) VisitColumnBloomFilter(i int, fn
func(BloomFilter) error) error {
+ bf, err := r.GetColumnBloomFilter(i)
+ if err != nil || bf == nil {
+ return err
+ }
+
+ defer r.recycle(bf)
+
+ return fn(bf)
+}
+
+func (r *RowGroupBloomFilterReader) recycle(bf BloomFilter) {
+ if bf == nil {
+ return
+ }
+
+ b, ok := bf.(*blockSplitBloomFilter)
+ if !ok {
+ return
+ }
+
+ b.bitset32 = nil
+
+ if r.bufferPool != nil && b.cancelCleanup != nil {
+ // Stop the GC cleanup so it can't return b.data to the pool a
second time.
+ b.cancelCleanup()
Review Comment:
This resolves the double-`Put` blocker from my last round — verified it
unregisters in both build variants (`c.Stop` on go1.24,
`runtime.SetFinalizer(bf, nil)` on ≤1.23), so the buffer can't be returned to
the pool a second time.
Non-blocking: there's still no dedicated regression test for this path.
`TestRecycleEncryptedPathPool` only exercises the encrypted
(nil-`cancelCleanup`) branch, not the pool-backed one. A test that
reads/recycles a *pool-backed* filter, forces `runtime.GC()`, and asserts the
same buffer isn't enqueued twice (e.g. two `pool.Get()`s return distinct
pointers) would lock this in — the original bug was CI-invisible since it only
fires after a GC and the suite doesn't run this under `-race`.
##########
parquet/metadata/bloom_filter.go:
##########
@@ -549,6 +551,46 @@ func (r *RowGroupBloomFilterReader) GetColumnBloomFilter(i
int) (BloomFilter, er
return bf, nil
}
+// VisitColumnBloomFilter invokes fn for the BloomFilter of the column at
index i.
+//
+// Lifetime contract: The BloomFilter passed to fn (and its backing bitset)
+// is recycled immediately after fn returns. Callers must not retain, store,
+// or use the BloomFilter or its data outside the scope of the fn function.
+func (r *RowGroupBloomFilterReader) VisitColumnBloomFilter(i int, fn
func(BloomFilter) error) error {
Review Comment:
Nice scoped accessor — the lifetime contract is clear and `recycle` nils
`bitset32`/`data` so misuse fails fast. Two non-blocking follow-up notes:
1. Nothing in the library calls `VisitColumnBloomFilter` yet, so the
prompt-return win only lands for callers who opt in; `GetColumnBloomFilter`
users still rely on GC recycling. Fine if that's the intent.
2. The bloom reader still shares `file.Reader.bufferPool` with the
column/record readers, so MB-sized bitsets and small page buffers churn the
same pool. A dedicated per-reader bloom pool would resolve that pollution —
good as a follow-up, not needed here.
--
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]
