fix(parquet): reject truncated bit-packed batches#943
Conversation
20ffbd7 to
0ac0aa5
Compare
0ac0aa5 to
07d700a
Compare
b425985 to
ea7eec0
Compare
zeroshade
left a comment
There was a problem hiding this comment.
The BitReader-level hardening looks correct — distinguishing io.EOF at a clean 32-value group boundary from io.ErrUnexpectedEOF for a partial packed group, and the required-byte math (bitWidth*4 per 32-value group) checks out. The remaining gap: the newly-detected truncation isn't propagated by the RLE/dictionary decode entry points, so a truncated bit-packed literal batch is still silently accepted. Detail inline.
| numUnpacked, unpackErr := unpack32(b.reader, (*(*[]uint32)(unsafe.Pointer(&out)))[i:], int(bits)) | ||
| i += numUnpacked | ||
| b.byteoffset += int64(numUnpacked * int(bits) / 8) | ||
| if unpackErr != nil { |
There was a problem hiding this comment.
Good — unpack32 now surfaces truncation via unpackErr, and GetBatchIndex/GetBatch return it. But the RLE/dict callers drop it, so the hardening never reaches the column decoders:
parquet/internal/utils/typed_rle_dict.go:166—n, _ := r.r.GetBatchIndex(...)thenif n != litbatch { return read, nil }, so a truncated literal batch returns a short read with a nil error.- Same pattern at
parquet/internal/utils/rle.go:223,parquet/internal/encoding/levels.go:271(Encoding_BIT_PACKED levels), andtyped_rle_dict.go:94(spaced literals).
Concrete case: an RLE literal run claiming 64 one-bit indices but only 7 payload bytes now yields (32, io.ErrUnexpectedEOF) from GetBatchIndex, but GetBatchWithDict discards it and returns (read, nil). Please propagate the error through these callers so truncated input is actually rejected.
ea7eec0 to
52c5dd1
Compare
zeroshade
left a comment
There was a problem hiding this comment.
Re-review: the truncation error is now propagated end-to-end through the RLE/dictionary/level decoders (typed_rle_dict spaced+unspaced, rle.go, levels.go RLE+BIT_PACKED, boolean_decoder, and the column_reader/record_reader level-decode signatures), with 6 new tests including the truncated-literal case. CI is green (the one macOS -race parquet/file failure was flaky and passed on re-run). LGTM — thanks!
Rationale for this change
The generated and SIMD unpackers ignored short-read failures. SIMD code could access an empty buffer or decode stale pooled bytes, while the generated path could report a complete batch filled partly from zero values. RLE, dictionary, and level decoders also discarded the truncation errors once detected.
What changes are included in this PR?
Require each packed group to be read completely before decoding, return unpacking errors to
BitReader, and avoid invoking SIMD routines without at least one complete group. If truncation occurs after complete 32-value groups, both SIMD and pure-Go paths return the same decoded count and distinguish a missing group (io.EOF) from a partially present group (io.ErrUnexpectedEOF).Propagate those errors through RLE batches, dictionary decoding (including spaced values and index decoding), level decoding, boolean decoding, and column/record readers. Incomplete groups never modify the destination suffix or get reported as a successful batch.
Are these changes tested?
Yes. Every bit width from 1 through 32 is tested for values and dictionary indexes with partially present groups and exact group-boundary EOF. Additional tests cover RLE literals, spaced and unspaced typed dictionary decoding, the actual dictionary decoder, and level decoding. The full Parquet suite, native SIMD,
-tags noasm, race-enabled focused packages, repeated regressions, and AMD64 cross-compilation pass.Are there any user-facing changes?
Truncated packed values now return a consistent error and partial count instead of producing fabricated output, being silently accepted, or panicking.
Related changes
This PR covers bulk 32-value unpacking and propagation of its errors. #944 covers scalar and trailing buffered reads, while #946 fixes short boolean-batch input.