GH-50421: [C++][Parquet] Add LevelDecoder Skip and Count#50422
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR extends Parquet C++ decoding by moving more “skip/count” functionality into LevelDecoder and underlying RLE/bit-packed utilities, with the goal of simplifying skip logic in column/record readers.
Changes:
- Refactors
LevelDecoderto a singlestd::variant-backed implementation and addsSkip/CountUpTo. - Reworks
TypedColumnReaderImpl::Skipto skip within a page by advancing rep/def levels and skipping only the required physical values. - Adds/updates Arrow RLE/bit-packed decoder internals and tests for
Advance/CountUpTo.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| cpp/src/parquet/column_reader.h | Updates LevelDecoder public API/docs to add Skip and CountUpTo and hide implementation details behind a pimpl. |
| cpp/src/parquet/column_reader.cc | Implements new LevelDecoder internals, introduces SkippableTypedDecoder, and simplifies TypedColumnReaderImpl::Skip. |
| cpp/src/arrow/util/rle_encoding_internal.h | Adds Advance / CountUpTo plumbing, introduces BitPackedDecoder, and refactors run processing into ProcessValues. |
| cpp/src/arrow/util/rle_encoding_test.cc | Adds targeted tests for CountUpTo and Advance behavior. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@ursabot please benchmark lang=C++ |
|
Benchmark runs are scheduled for commit b7bd198. 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. |
|
@pitrou this is ready again. |
pitrou
left a comment
There was a problem hiding this comment.
A couple minor comments, otherwise LGTM
|
Thanks for your patience. Conbench analyzed the 4 benchmarking runs that have been run so far on PR commit b7bd198. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
|
@pitrou this is looking good |
| // Advance the definition levels, counting how many correspond to present | ||
| // (non-null) values that must be skipped in the data decoder. | ||
| int64_t non_null_values_to_skip = batch_size; | ||
| if (this->max_def_level() > 0) { | ||
| const auto count = | ||
| this->definition_level_decoder_.CountUpTo(this->max_def_level(), batch_size); | ||
| non_null_values_to_skip = count.matching_count; | ||
| ARROW_DCHECK_EQ(count.processed_count, batch_size); | ||
| } | ||
| // Advance the repetition levels; their values are not needed. | ||
| if (this->max_rep_level() > 0) { | ||
| this->repetition_level_decoder_.Skip(batch_size); | ||
| } | ||
| // Skip the corresponding data values. | ||
| this->current_decoder_.Skip(non_null_values_to_skip); | ||
|
|
| /// Advance and count the number of occurrence of a values. | ||
| /// | ||
| /// The count is limited to at most the next `batch_size` items. | ||
| /// @return The matching value count and number of of element that were processed. |
|
@github-actions crossbow submit -g cpp |
pitrou
left a comment
There was a problem hiding this comment.
+1, this is ready to merge now :)
|
Revision: 6e5c7a2 Submitted crossbow builds: ursacomputing/crossbow @ actions-2073a4b27b |
Rationale for this change
Adding new methods to the
LevelDecoderto start reducing the complexity inColumnReaderImplBase,TypedColumnReaderImpl, andTypedRecordReader.What changes are included in this PR?
LevelDecodermembers based on a single variantLevelDecoder::SkipLevelDecoder::CountUpToSkipimplementation inTypedColumnReaderImplSkippableTypedDecoderto wrapTypedDecoderwith the remaining usage of thescratch_for_skip_bufferAre these changes tested?
Yes.
Are there any user-facing changes?
No.