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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
