GH-50548: [Parquet][C++] Check min max level inside decoder - #50682
GH-50548: [Parquet][C++] Check min max level inside decoder#50682AntoinePrv wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes Parquet (C++) level decoding by validating decoded levels during decoding (via a per-value/per-run validator) instead of performing a separate post-decode FindMinMax pass, aiming to reduce CPU overhead and improve locality.
Changes:
- Add a validator-capable
GetBatchpath toLevelDecoderto validate decoded levels inline. - Extend Arrow’s internal RLE / bit-packed decoders to optionally accept a validator functor during
Get/GetBatch.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cpp/src/parquet/column_reader.cc | Switch level validation from post-decode min/max scan to an inline validator passed into the decoder. |
| cpp/src/arrow/util/rle_encoding_internal.h | Add optional validator support throughout RLE and bit-packed decoding APIs to enable inline validation. |
|
@ursabot please benchmark |
|
Benchmark runs are scheduled for commit 283f56b. 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. |
283f56b to
a9ee56d
Compare
| const int num_values = std::min(num_values_remaining_, batch_size); | ||
| const int num_decoded = impl_->GetBatch(levels, num_values); | ||
| if (num_decoded > 0) { | ||
| internal::MinMax min_max = internal::FindMinMax(levels, num_decoded); |
There was a problem hiding this comment.
If this is the only place where FindMinMax is used, we can also remove it?
github-actions
Bot
added
awaiting committer review
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit a9ee56d. 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. |
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 283f56b. There were 136 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
@AntoinePrv Unfortunately, there are many large regressions on the benchmarks, and few improvements. |
|
Interesting, I can try to also enable the simd version. |
|
Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit a9ee56d. There were 99 benchmark results indicating a performance regression:
The full Conbench report has more details. |
Rationale for this change
Simpler check for Rle runs and better data locality on bit packed runs.
What changes are included in this PR?
Add validator on decoder and use it inside the LevelDecoder.
Simd accelerated
FindMinMaxis unused now. If this is enough, we can delete it, otherwise, we can tweak the validator to accept a range and use the SIMD version there.Are these changes tested?
With current tests
Are there any user-facing changes?