perf(parquet): pre-size values in OffsetBuffer::extend_from_dictionary - #10690
Closed
AarryaSaraf wants to merge 3 commits into
Closed
perf(parquet): pre-size values in OffsetBuffer::extend_from_dictionary#10690AarryaSaraf wants to merge 3 commits into
AarryaSaraf wants to merge 3 commits into
Conversation
extend_from_dictionary reserves the offsets buffer but not values, so every gathered dictionary value lands in an unreserved Vec and amortized doubling re-copies roughly all gathered data one extra time. For large dictionary values (e.g. binary image columns, which common writers dictionary-encode because the dictionary-size limit is checked lazily) that extra copy dominates the decode. An exact-sum reserve was proposed and withdrawn in apache#5250: the second bounds-checked pass over the keys regresses small-value dictionaries by 10-15% on the crate's own benchmarks (reproduced at 8-18% on 59.1.0). This uses an O(1) estimate instead - keys.len() times the dictionary's average value length - which is exact for uniform value lengths and an ordinary reservation hint otherwise, with no per-key pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AarryaSaraf
force-pushed
the
dict-extend-reserve-values
branch
from
August 14, 2026 23:27
05fb795 to
6b4cc75
Compare
AarryaSaraf
marked this pull request as ready for review
August 15, 2026 00:19
Rich-T-kid
reviewed
Aug 18, 2026
Contributor
There was a problem hiding this comment.
nit: im not sure the test_offset_buffer_extend_from_dictionary_size_hint is needed. the arithmetic makes sense and it already accounts for dividing by zero. this is essentially optionally pre-allocating space in a vector, nothing logically has changed.
The reservation is a capacity hint and cannot change decode output, so a test asserting the output is unchanged carries no signal. Removed it. The guard it partly covered is folded into the arithmetic instead: `checked_div` returns None for both an empty dictionary and one with zero entries, replacing the separate `checked_sub(1).filter(|n| *n > 0)`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
Agreed, removed. |
etseidl
pushed a commit
that referenced
this pull request
Aug 19, 2026
# Which issue does this PR close? None. This is benchmark coverage split out of #10690, so that the change proposed there can be measured against a benchmark that already exists on `main`. It covers the shape reported in #10694. # Rationale for this change Every dictionary-encoded case in `arrow_reader` decodes ~20 byte values: `build_dictionary_encoded_string_page_iterator` builds `"Dictionary value {x}"` at 1% unique. Two properties of that shape keep the dictionary gather out of the measurement entirely — values are small enough that per-key overhead dominates (RLE index decoding, the per-key bounds check), and the dictionary is a couple of KiB, so it stays cached for the whole decode. Columns of large binary payloads invert both. A writer's dictionary size limit is checked lazily, so such a column is often dictionary encoded all the way to the end, with the dictionary page holding the entire column and each entry referenced exactly once. The gather then gets its data from a source far too large to cache, and it is the gather rather than the per-key work that dominates. No benchmark in the crate covers that, so changes to `OffsetBuffer::extend_from_dictionary` currently have nothing to be measured against. # What changes are included in this PR? One generator and one case in the existing `BinaryArray` group: - `build_dictionary_encoded_large_value_page_iterator` — 64 KiB values, all distinct, 128 per page, mandatory (no NULLs), otherwise the same row-group and page geometry as the existing generators. - `arrow_array_reader/BinaryArray/dictionary encoded, mandatory, no NULLs, large values` One iteration decodes 64 MiB of output from a 32 MiB dictionary. Measured on `main` (Apple M-series): ``` arrow_array_reader/BinaryArray/dictionary encoded, mandatory, no NULLs, large values time: [7.6857 ms 7.8982 ms 8.1303 ms] ``` # Are these changes tested? This is a benchmark. It asserts its decoded value count on each run, as the surrounding cases do. # Are there any user-facing changes? No. # AI disclosure This benchmark was drafted with AI assistance and reviewed by me.
Contributor
|
run benchmark arrow_reader |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing dict-extend-reserve-values (294292b) to f271113 (merge-base) diff Run configurationrun benchmark arrow_readerBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader File an issue against this benchmark runner |
Rich-T-kid
approved these changes
Aug 19, 2026
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing dict-extend-reserve-values (294292b) to f271113 (merge-base) diff Run configurationrun benchmark arrow_readerCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
run benchmark arrow_reader env:
BENCH_FILTER: large values |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing dict-extend-reserve-values (294292b) to f271113 (merge-base) diff Run configurationrun benchmark arrow_reader
env:
BENCH_FILTER: "large values"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing dict-extend-reserve-values (294292b) to f271113 (merge-base) diff Run configurationrun benchmark arrow_reader
env:
BENCH_FILTER: "large values"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
run benchmark arrow_reader env:
BENCH_FILTER: BinaryArr |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing dict-extend-reserve-values (294292b) to f271113 (merge-base) diff Run configurationrun benchmark arrow_reader
env:
BENCH_FILTER: "BinaryArr"BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench arrow_reader File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing dict-extend-reserve-values (294292b) to f271113 (merge-base) diff Run configurationrun benchmark arrow_reader
env:
BENCH_FILTER: "BinaryArr"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
On my x86 mac laptop: |
Contributor
|
I'll try on my linux WS tomorrow, but so far not seeing much of an improvement. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #10694, but does not close it. That issue reports a ~2x gap against
parquet-cpp on large distinct dictionary-encoded values; this addresses one
contributing cause and moves about a fifth of it. The residual stays open there.
Rationale for this change
OffsetBuffer::extend_from_dictionaryreservesoffsetsbut notvalues, soevery gathered dictionary value is appended to an unreserved
Vec<u8>andamortized doubling re-copies roughly all gathered data one extra time. For a
column of large dictionary-encoded values that cost is measurable; for the small
values the crate's benchmarks cover, it is not.
#5250 raised the same allocation profile and proposed an exact-sum reserve —
a second pass over the keys summing each referenced value's length. It was
withdrawn after regressing the small-string dictionary benchmarks by ~10-15%. We
reproduced that on 59.1.0: +8-18% across the three
arrow_array_reader/StringArray/dictionary encodedcases. At ~19 byte values thesecond bounds-checked pass over the keys costs more than the copy it saves.
This PR uses an O(1) estimate instead:
keys.len() × (dict_values.len() / dict_entry_count). No per-key pass, exact when value lengths are uniform, and anordinary reservation hint when they are not.
Note that
OffsetBuffer::with_capacitydeliberately does not pre-sizevalues("its size is unpredictable"). That remains true in general; this change is
scoped to the dictionary path, where the dictionary page gives a cheap size
signal.
What changes are included in this PR?
A reservation hint at the top of
extend_from_dictionary. No behavior change: itis skipped for an empty dictionary, and it only pre-sizes the same
Vecthe loopwas already growing.
The reservation uses
try_reserverather thanreserve. A skewed dictionary — afew very large entries with most keys selecting small ones — can inflate an
average-based estimate far above the true output size, and the dictionary comes
from an untrusted file, so a failed
reservewould abort the process. Withtry_reservethat case degrades to the current growth behavior instead.Are these changes tested?
No new test, per review. The reservation is a capacity hint that cannot alter
output, and the arithmetic is total by construction
Performance, Linux,
parquet59.1.0 read fromPython over the C data interface, against pyarrow 24.0.0. The file is 256 MiB,
1024 rows, a
binary()column of 256 KiB distinct values written with PyArrowdefaults, so dictionary encoded end to end. Wall time at the read operation,
3 repeats per arm:
pq.read_table)3 of 3 runs rank-matched not-worse; the honest band on the improvement is
−6% to −12%. Two caveats: n = 3 with overlapping spreads, and the parquet-cpp
baseline comes from a different run than the patched arm, because the in-run
baselines drifted structurally against each other. So this is a direction plus a
rough size, not a precise figure.
This recovers part of the gap against parquet-cpp on this shape and not all of
it — we had predicted the reserve would close it, and it moved about a fifth of
the way. The rest is tracked in #10694.
The three existing small-string dictionary cases measure at parity — the property
the exact-sum variant in #5250 failed. Patched vs baseline means: 267.2 vs
257.7 µs, 261.9 vs 260.6 µs, 248.4 vs 252.4 µs, i.e. scattered around zero, with
a null control on the same machine at −0.6% (p = 0.40).
The large-value benchmark added in #10691 does not separate the two on my
Apple hardware. Interleaving stock and patched runs against a common baseline,
7 stock and 5 patched:
The distributions overlap almost entirely, and a null control on the same box —
stock re-run against its own saved baseline, byte-identical code — reports
−9.3% (p = 0.00). A single-digit effect is not resolvable there, which is why the
table above comes from Linux and our own harness rather than from this benchmark.
That is the main thing I would value from review: a run of #10691's case on your
reference hardware. It is the measurement I cannot produce.
For the same reason we are not quoting macOS magnitudes for the large-value case
at all. An earlier null control there reported "+43.6% regressed, p = 0.00" on
byte-identical code, and one case read 24.6 / 37.1 / 30.9 / 35.4 ms across four
builds, two of which were provably identical. The small-string cases quoted above
are stable on that machine; the large-value ones are not.
Are there any user-facing changes?
No.
AI disclosure
The patch and this description were drafted with AI assistance and reviewed by me
line by line. The measurements are my own runs. I verified the reservation cannot
change decode output — it only affects
Veccapacity — and worked through theskewed-dictionary over-estimate case by hand, which is why
try_reserveis used.