fix(parquet/encoding): read ahead in the streaming value buffer#937
fix(parquet/encoding): read ahead in the streaming value buffer#937joechenrh wants to merge 3 commits into
Conversation
Fill read only `need` bytes per call, so a value source yielding small chunks got a Read per value (a 4-byte length then the value: ~2 Reads/value). On a 32 MiB page of ~9-byte values that is ~5M Read calls, making streaming decode 1.4-1.8x slower than the materialized path for small values -- a real cost for decode-bound readers (e.g. local files), even if it is hidden behind IO in cloud pipelines. Fill now reads toward the end of the current chunk (bounded by the value region) instead of stopping at `need`, so the reader is called ~once per chunk (one Read per decompressor block): ~5M Reads drop to ~260 on that page. To keep peak at one chunk despite reading ahead, Recycle compacts the unconsumed tail to the front only once the consumed prefix passes half the chunk -- rare and independent of batch size -- instead of rotating to a fresh chunk. Peak stays ~1 MiB (the DefaultBufferSize window), unchanged. Small-value decode overhead drops from ~1.4-1.8x to ~1.2x. The residual is the per-value Fill/Advance calls through the ValueBuffer vs the materialized path's inline slicing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d841275 to
5e79d11
Compare
…-discard - TestStreamBufferReadAheadStopsAtRegion: reader carries bytes past the value region; fails if the off+remaining read-ahead clamp is dropped - TestStreamBufferSkip: skip past the buffered read-ahead so the io.CopyN reader-discard branch stays covered - document the cur[off:n] read-ahead invariant on off/n Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zeroshade
left a comment
There was a problem hiding this comment.
Read-ahead/refill logic looks correct — region clamping, alias stability across rotate, and short/EOF handling all check out. One doc-accuracy nit inline.
| // Compact once the consumed prefix passes half the chunk, so read-ahead keeps room | ||
| // without a rotate (peak = one chunk) and the copy stays rare. |
There was a problem hiding this comment.
nit: peak = one chunk is a little stronger than what the code guarantees. Compaction only runs once the consumed prefix passes half the chunk, so a small consumed prefix followed by a larger value can still fall through to rotate, which keeps the old chunk in live while allocating the new one — a transient two-chunk peak. Suggest softening to something like keeps read-ahead room and makes rotations rare rather than implying a hard one-chunk ceiling.
There was a problem hiding this comment.
Updated in joechenrh@80b7fe3 . Reworded the comment to describe the compaction/rotation tradeoff without implying a hard one-chunk ceiling. Thanks for catching that!
Summary
The streaming value buffer's
Fillread onlyneedbytes per call, so the decoders callFill(Read) per value. And each call just copies a few bytes out of an already-decoded block. This makes streaming decoding 1.2–1.9× slower than the materialized path for small values.Now
Fillreads toward the end of the current chunk — so the reader is called once per chunk. AndRecyclemoves the unconsumed data to the front once the consumed data exceeds half the buffer, instead of allocating a fresh chunk on rotate.Correction
I need to correct — and apologize for — something I wrote while landing the streaming decode in #880:
That conclusion was wrong, and worse, it reverted something that had already been working. An earlier iteration of the value buffer did read ahead correctly; but dropped in the later refactor. The re-prototype behind the "≈0 improvement" claim was then broken. From testing, this adds a 1.2–1.9× small-value decode regression for zstd.
Sorry for removing the code that works and then drawing the wrong conclusion. This PR restores read-ahead with some changes and adds a test for that path.
Results
Decode-only,
ReadBatchInPage; single ByteArray column, 32 MiB pages, arrow-go's defaultBufferSize(16 KiB), median of 3.Ratio = materialized / streaming, so >1 means streaming is slower:
ReadBatchgives almost the same ratios.The remaining ~1.04–1.16× (zstd) is the streaming path's inherent overhead: the codec pulls its compressed input incrementally, so it issues many more reads than the materialized path's single bulk read — profiling attributes most of the gap to those read syscalls — plus a
Fill/Advancecall per value. Read-ahead removes what the value-side read count added; it does not remove those.gzip
gzip benefits too, but its residual is higher (~1.33× at 9 B vs zstd's ~1.16×). The extra gap is in flate's input reading. Both paths read the compressed input byte by byte; the difference is inlining:
*bytes.Reader, whoseReadByteinlines, so flate keeps its bit-buffer state in registers across the read loop (huffmanBytesReader).ReadByteis a real call per byte and the state spills around each call (huffmanBufioReader) — ~1.85× slower in that loop.Feeding flate a
*bytes.Readerunder streaming recovers it (~1.06×), but that means holding the whole compressed page in memory, which gives up the streaming memory bound.