Re: [PR] fix(parquet): stop scalar reads past available input bits [arrow-go]

["zeroshade (via GitHub)" <gi...@apache.org>]

zeroshade commented on code in PR #944:
URL: https://github.com/apache/arrow-go/pull/944#discussion_r3598633465


##########
parquet/internal/utils/bit_reader.go:
##########
@@ -250,18 +249,47 @@ func (b *BitReader) fillbuffer() error {
 
 // next reads an integral value from the next bits in the buffer
 func (b *BitReader) next(bits uint) (v uint64, err error) {
-	v = trailingBits(b.buffer, b.bitoffset+bits) >> b.bitoffset
-	b.bitoffset += bits
-	// if we need more bits to get what was requested then refill the buffer
-	if b.bitoffset >= 64 {
+	if bits == 0 {
+		return 0, nil
+	}
+
+	if b.bitoffset == 64 {
 		b.byteoffset += 8
-		b.bitoffset -= 64
+		b.bitoffset = 0
 		if err = b.fillbuffer(); err != nil {
 			return 0, err
 		}
-		v |= trailingBits(b.buffer, b.bitoffset) << (bits - b.bitoffset)
 	}
-	return
+
+	end := b.bitoffset + bits
+	if end <= 64 {
+		if end > b.validBits {

Review Comment:
   This per-value guard is the source of the ~4.8% `PlainDecodingBoolean` regression: boolean plain-decode calls `next(1)` once per value, so it hits this check on every bit. The check is correct — the way to recover the throughput is to keep booleans off the per-value path (decode them via the bulk `GetBatchBools`, hardened in #946 with `ensureBoolBitAvailable`) rather than looping `next(1)`. Fixed-width/bulk paths (`GetBatch`/`unpack32`) don't pay this and are unaffected.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org