GH-50703: [C++][Parquet] Reserve dictionary byte array data before decoding - #50710
GH-50703: [C++][Parquet] Reserve dictionary byte array data before decoding#50710Punisheroot wants to merge 5 commits into
Conversation
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
There was a problem hiding this comment.
Pull request overview
This PR optimizes Parquet dictionary-encoded BYTE_ARRAY decoding into Arrow binary/string builders by predecoding dictionary indices to compute the exact decoded payload size, reserving once, and then appending via unsafe fast paths. It also adds a regression test for invalid/truncated index streams and extends microbenchmarks to include null-density scenarios.
Changes:
- Predecode dictionary indices for Binary/String/LargeBinary/LargeString builders, validate bounds up front, compute exact decoded byte length, and reserve once before appending.
- Add a regression test ensuring dense dictionary decode rejects out-of-range and truncated index streams.
- Extend dictionary decode benchmarks with 10% and 50% null-density configurations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/parquet/decoder.cc | Predecode indices + exact reservation path for dictionary BYTE_ARRAY decoding into non-view Arrow builders. |
| cpp/src/parquet/encoding_test.cc | Adds regression coverage for invalid/truncated dictionary-index streams. |
| cpp/src/parquet/encoding_benchmark.cc | Adds benchmark variants exercising dense decode with configurable null density. |
| } else { | ||
| for (int64_t i = 0; i < length; ++i) { | ||
| helper->UnsafeAppendNull(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I benchmarked this exact change by replacing the per-value loop with helper->AppendNulls(length) in an ABBA comparison using the same Windows MSVC Release build. It improved the 10% randomly distributed null case by approximately 2.4–2.6%, but regressed the 50% null case by approximately 10–12%. The latter produces many short null runs, for which the bulk call is not consistently beneficial.
A separate microbenchmark showed clear benefits only for sufficiently long runs, so an adaptive run-length threshold may be worth exploring separately. For this PR, I’d prefer to retain the existing per-value path and keep the change focused on exact dictionary payload reservation.
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 74acebb. Watch https://buildkite.com/apache-arrow and https://conbench.arrow-dev.org for updates. A comment will be posted here when the runs are complete. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 74acebb. There were 15 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
Thanks for the Conbench report. I investigated all 15 possible regressions The follow-up fix described below is included in commit 59e8845. Root causeSix regressions affected
These regressions were real but indirect. The Plain decoding algorithm was not I confirmed this by comparing the generated GCC 14 code for the baseline, the Classification of the other resultsSeven alerts belong to separate Arrow benchmark executables: the two temporal The Boolean RLE and FLBA cases require a more cautious classification because
Paired GCC 14 controls did not reproduce either regression. With the complete
A second experiment using the same executable for every comparison and changing Follow-up fixThe follow-up adds a narrowly scoped overload for the known-size
This restores the intended generated code without increasing GCC's global Performance validationI could not reproduce the exact Apache c6a host locally, so these measurements For the six originally regressed Plain cases:
The dictionary optimization remains intact:
All 76 registered Correctness validationThe exact final source passed:
A disposable diagnostic build also forced the normally rare chunk rollover at Relation to the original PR benchmarksThe original Windows/MSVC measurements remain valid for the dictionary cases The follow-up therefore supplements the original measurements with the missing A new Conbench run on the Apache runners is still required for final validation. Thank you! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cpp/src/parquet/encoding_benchmark.cc:1335
DecodeArrowWithNullDenseBenchmarkregenerates inputs and re-encodes before the decode loop without pausing timing, so the benchmark measures setup+encode work in addition to decode. Pause timing around the reinitialization so results reflect decode cost (consistent with the other Decode* benchmarks which rely on SetUp()).
void DecodeArrowWithNullDenseBenchmark(benchmark::State& state) {
null_probability_ = static_cast<double>(state.range(1)) / 10000;
InitDataInputs();
DoEncodeArrow();
| if (valid) { | ||
| for (int64_t i = 0; i < length; ++i) { | ||
| const auto& val = dict_values[decoded_indices[pos_indices++]]; | ||
| RETURN_NOT_OK(helper->AppendValue( | ||
| val.ptr, static_cast<int32_t>(val.len), remaining_data_length)); | ||
| remaining_data_length -= val.len; | ||
| } | ||
| values_decoded += static_cast<int>(length); | ||
| } else { | ||
| for (int64_t i = 0; i < length; ++i) { | ||
| helper->UnsafeAppendNull(); | ||
| } | ||
| } | ||
| return Status::OK(); | ||
| })); | ||
| DCHECK_EQ(pos_indices, values_to_decode); | ||
| DCHECK_EQ(remaining_data_length, 0); | ||
| *out_num_values = values_decoded; | ||
| return Status::OK(); |
There was a problem hiding this comment.
The defensive validity-bitmap fix is included in commit d9e9f6e.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
cpp/src/parquet/decoder.cc:2497
- The validity-bitmap validation error doesn’t report the expected vs actual non-null counts, which makes diagnosing upstream bitmap/null_count mismatches harder. Including both counts would align with the more detailed index-stream validation message above.
if (ARROW_PREDICT_FALSE(actual_non_null != num_values - null_count)) {
throw ParquetException(
"Invalid validity bitmap: null_count does not match the number of non-null "
"values");
}
| RETURN_NOT_OK(PushChunk()); | ||
| RETURN_NOT_OK(ReserveInitialChunkData(estimated_remaining_data_length)); | ||
| chunk_space_remaining_ -= length; | ||
| --entries_remaining_; | ||
| builder_->UnsafeAppend(data, length); |
What changed
This changes dictionary-encoded
BYTE_ARRAYdecoding for Binary, String,LargeBinary, and LargeString builders to:
BinaryView and StringView builders keep the existing bounded streaming path,
since short view values are stored inline and do not benefit from reserving the
dictionary values' total byte length.
The patch also adds dictionary decode benchmarks with 10% and 50% null density
and a regression test for out-of-range and truncated dictionary-index streams.
Why
The dictionary path previously could not derive a useful data-size estimate
from page metadata, so every value used the checked append path and the value
buffer grew incrementally. Predecoding the indices provides the exact payload
size while hoisting the bounds checks out of the append loop.
The trade-off is one reusable
int32_tscratch entry per decoded non-null valueand an additional pass over the indices. The scratch allocation is retained for
reuse across decode calls.
This addresses the dictionary-path exact-reservation proposal in GH-50703.
Benchmarks
Windows MSVC Release build, 65,536 values, baseline/optimized/optimized/baseline
order, five randomized repetitions per run:
BinaryView is the unchanged-path control. These are local decoder
microbenchmarks rather than an end-to-end scan of the reporter's 6.5 GB file.
Validation
parquet-encoding-test.exe --gtest_filter=DictEncodingAdHoc.DenseDecodeRejectsInvalidOrTruncatedIndices: passedparquet-encoding-test.exesuite: 136/136 passedgit diff --check: passedAI assistance disclosure
AI assistance was used for code exploration, implementation drafting, and
test/benchmark iteration. I reviewed and validated the resulting changes and
can explain and take ownership of them.