zeroshade commented on code in PR #1169:
URL: https://github.com/apache/arrow-go/pull/1169#discussion_r3798930572
##########
parquet/internal/encoding/delta_bit_packing.go:
##########
@@ -181,22 +181,22 @@ func (d *deltaBitPackDecoder[T]) unpackNextMini() error {
d.miniBlockValues = append(d.miniBlockValues,
T(d.lastVal))
}
} else {
- if cap(d.deltaBuf) < 1 {
- d.deltaBuf = make([]uint64, 1)
+ if cap(d.deltaBuf) < n {
+ d.deltaBuf = make([]uint64, n)
}
- d.deltaBuf = d.deltaBuf[:1]
+ d.deltaBuf = d.deltaBuf[:n]
minDelta := d.minDelta
- for j := 0; j < n; j++ {
- nread, err := d.bitdecoder.GetBatch(width, d.deltaBuf)
- if err != nil {
- return err
- }
- if nread != 1 {
- return errors.New("parquet: eof exception")
- }
+ nread, err := d.bitdecoder.GetBatch(width, d.deltaBuf)
+ if err != nil {
Review Comment:
**Blocking:** `GetBatch` can return a partial count with a bare `io.EOF`
when truncated input ends on a complete 32-value packed-group boundary.
Returning that error here bypasses the `nread != n` corruption check below.
This becomes silent data loss in `pqarrow`: `recordReader.Err()` treats an
exact `io.EOF` as normal end-of-stream. A truncation sweep of a valid
1,024-value delta page found multiple cuts where this branch returns bare
`io.EOF`; end-to-end, the reader returns no rows and `Err() == nil`.
A miniblock must always contain all `n` padded values, so please convert any
incomplete read—including bare `io.EOF`—to `io.ErrUnexpectedEOF` or another
corruption error before returning. A regression test should assert the error
kind, not only that an error occurred.
##########
parquet/internal/encoding/delta_bit_packing.go:
##########
@@ -181,22 +181,22 @@ func (d *deltaBitPackDecoder[T]) unpackNextMini() error {
d.miniBlockValues = append(d.miniBlockValues,
T(d.lastVal))
}
} else {
- if cap(d.deltaBuf) < 1 {
- d.deltaBuf = make([]uint64, 1)
+ if cap(d.deltaBuf) < n {
+ d.deltaBuf = make([]uint64, n)
Review Comment:
This allocation is sized directly from the file-controlled `valsPerMini`.
`miniBlockValues` already allocates `n*sizeof(T)` above; this new `[]uint64`
scratch doubles the malformed-header amplification.
For example, a tiny header declaring `valsPerMini = 1<<24` makes the int64
decoder allocate roughly 256 MiB before discovering that the packed data is
absent—about 128 MiB more than the merge base. The high-water allocation also
remains attached to a reused decoder.
Could this scratch remain bounded—for example, decode in the same
approximately 1,024-value chunks already used by `GetBatch`—and accumulate into
`miniBlockValues` incrementally?
--
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]
