You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/08/11 17:28:14 UTC
[PR] perf(parquet): batch delta binary packed decoding [arrow-go]
fallintoplace opened a new pull request, #1169:
URL: https://github.com/apache/arrow-go/pull/1169
### Rationale for this change
DELTA_BINARY_PACKED decoding called `BitReader.GetBatch` once for every value in a miniblock. The miniblock is already the unit decoded by this code, so this missed the existing batch unpackers.
### What changes are included in this PR?
- decode each miniblock into reusable scratch with one `GetBatch` call
- keep 33 to 64 bit values on the generic uint64 path instead of the 32 bit SIMD unpackers
- read SIMD input directly into pooled scratch so batched calls do not add allocations
- add int32 and int64 benchmarks with nonzero packed widths
- add coverage for batched values from 33 through 64 bits
### Are these changes tested?
Yes. I ran the full Parquet test tree with the parquet-testing fixtures, the internal utility and encoding tests with `noasm`, and a linux/amd64 cross-build.
Apple M1 Pro results for 65,536 values with `GOMAXPROCS=1` and 10 samples:
| Workload | Time |
| --- | ---: |
| int32 small deltas | -58.86% |
| int32 alternating wide deltas | -66.04% |
| int64 timestamp-like deltas | -59.72% |
| int64 deltas wider than 32 bits | -33.89% |
Allocations stay at 2 allocs/op in all four benchmarks.
--
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
Re: [PR] perf(parquet): batch delta binary packed decoding [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(parquet): batch delta binary packed decoding [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1169:
URL: https://github.com/apache/arrow-go/pull/1169
--
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
Re: [PR] perf(parquet): batch delta binary packed decoding [arrow-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #1169:
URL: https://github.com/apache/arrow-go/pull/1169#issuecomment-5319230851
Since this is a draft, i'll hold off on further review until the conflicts are resolved and it's marked ready
--
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