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 2 commits 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. |
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit 98f1769. 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. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
cpp/src/arrow/util/rle_encoding_internal.h:460
BitPackedRunDecoder::GetBatchnow invokes the optional validator unconditionally, even whenopts.batch_sizeis 0 (e.g. when called withbatch_size == 0or the decoder is exhausted). Validators are likely to assumesize > 0, so this can cause unexpected behavior or crashes. Early-return whenopts.batch_size == 0(and skip unpack/validation) to keep the validator contract to “only called when values were decoded.”
template <typename Func = RleDefault>
[[nodiscard]] rle_size_t GetBatch(value_type* out, rle_size_t batch_size,
rle_size_t value_bit_width, Func&& validator = {}) {
const int64_t bits_read = static_cast<int64_t>(values_read_) * value_bit_width;
const int64_t bytes_fully_read = bits_read / 8;
// The parser only creates runs whose full payload fits in max_read_bytes_ (see
// BitPackedRun), so the max_read_bytes difference below is in [0, max_read_bytes_]
// and fits an int. A negative (unbounded) max_read_bytes_ stays negative.
ARROW_DCHECK(max_read_bytes_ < 0 || bytes_fully_read <= max_read_bytes_);
const uint8_t* unread_data = data_ + bytes_fully_read;
const ::arrow::internal::UnpackOptions opts{
/* .batch_size= */ std::min(batch_size, remaining()),
/* .bit_width= */ value_bit_width,
/* .bit_offset= */ static_cast<int>(bits_read % 8),
/* .max_read_bytes= */ static_cast<int>(max_read_bytes_ - bytes_fully_read),
};
if constexpr (std::is_same_v<T, bool>) {
::arrow::internal::unpack(unread_data, out, opts);
} else {
::arrow::internal::unpack(
unread_data, reinterpret_cast<std::make_unsigned_t<value_type>*>(out), opts);
}
// Validate decoded output if given a validator
if constexpr (!std::is_same_v<std::decay_t<Func>, RleDefault>) {
validator(static_cast<const value_type*>(out), opts.batch_size);
}
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit 98f1769. There were 3 benchmark results indicating a performance regression:
The full Conbench report has more details. |
|
The updated benchmark numbers are inconclusive: some speedups, some slowdowns. Nothing really remarkable. |
|
Yeah, sounds reasonable: big RLE runs win, and probably bit packed loose a bit because FindMinMax is called on smaller arrays. I think we can close this and the original issue (also since it is based on 14.0). One interesting point is that |
|
|
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?