fix(parquet): handle short input in GetBatchBools#946
Conversation
68137fa to
40ad2fa
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The byte-aligned bulk path is a real improvement and correctly returns io.ErrUnexpectedEOF when a full byte can't be read. But the fix is incomplete for counts that aren't a whole number of bytes — the sub-byte remainder still fabricates trailing values at EOF. Also note this overlaps #944 on bit_reader.go. Detail inline.
| buf := arrow.Uint32Traits.CastToBytes(b.unpackBuf[:]) | ||
| blen := buflen * 8 | ||
| for i < length { | ||
| for length-i >= 8 { |
There was a problem hiding this comment.
This full-byte loop is correct, but after it (length-i in 1..7) the sub-byte remainder falls through to b.fillbuffer() + the trailing for ; i < length { b.next(1) } loop, and fillbuffer zero-pads / suppresses EOF — so short input still fabricates false values instead of stopping.
Concrete: out := make([]bool, 9) over a 1-byte input decodes 8 real bits, then the trailing loop appends a 9th false and returns (9, nil) instead of (8, io.ErrUnexpectedEOF). Same class for 1–7 bools from empty input, or 9–15 from one byte.
Also: this PR and #944 both rewrite bit_reader.go (#944 adds validBits accounting to next/fillbuffer, which would actually close this trailing-path gap). They will conflict — worth coordinating the two.
40ad2fa to
414537e
Compare
zeroshade
left a comment
There was a problem hiding this comment.
Re-review: the sub-byte remainder now returns io.ErrUnexpectedEOF (ensureBoolBitAvailable + fillbuffer error propagation) instead of fabricating trailing bits, with tests for the 9-from-1-byte / empty-input edge cases. LGTM — thanks!
Rationale for this change
GetBatchBoolsrounded the remaining value count down to a whole byte inside a loop that also handled one-to-seven trailing values. That produced a zero-length read without advancing the loop. The bulk path also advanced by the requested bit count after a short read, while the buffered remainder could decode zero-padding past EOF as realfalsevalues.What changes are included in this PR?
Run the bulk path only while at least eight values remain, fill each requested byte chunk while detecting stalled readers, and preserve the number of complete decoded bytes when EOF occurs. Before decoding a sub-byte remainder, verify that its source byte exists; this applies to both aligned and already-buffered unaligned reads. Missing trailing bits return
io.ErrUnexpectedEOFwithout modifying the destination suffix.Are these changes tested?
Yes. Regression coverage includes lengths 0 through 17 from aligned and unaligned offsets, truncated full-byte input, 1–7 values from empty input, 9–17 values with only complete preceding bytes, an unaligned truncation, untouched output suffixes, and a reader that returns no progress. The full Parquet suite passes, along with native,
-tags noasm, race-enabled, repeated regression, and AMD64 compilation checks.Are there any user-facing changes?
Short boolean batches no longer loop indefinitely, and truncated input now returns its partial count and an error instead of fabricated success or zero-padded boolean values.
Related changes
This PR remains limited to
GetBatchBools. #944 adds generic scalar EOF accounting, while #943 handles truncated bulk integer unpacking. The current #944 and #946 heads merge cleanly; #944 reinforces the remainder check through shared valid-bit tracking without changing the boolean bulk-read behavior in this PR.