You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
The issue is that Decode takes fundamentally different arguments:
In the general case, an int16_t* array where the data type correspond to one value
In the bitmap case, a uint8_t*array + bit offset to find the exact pointed bit.
So we'd need to also dynamically dispatch the output type here, which IMHO brings extra complexity and performance hit (virtual polymorphism brings little since this is fully private and used in separate classes).
If this is about code reuse, I would suggest:
Make a de-pimpled templated LevelDecoder
Either:
Wrap it in a pimpl to make the existing LevelDecoder
OR split rle_encoding_internal.hpp into a rle_decoding_fwd_internal.hpp+rle_decoding_internal.hpp to have a light class definition (without function definitions and other headers) to include in column_reader.h.
The reason will be displayed to describe this comment to others. Learn more.
The issue is that Decode takes fundamentally different arguments:
* In the general case, an int16_t* array where the data type correspond to one value
* In the bitmap case, a uint8_t*array + bit offset to find the exact pointed bit.
Ideally the RecordReader would not see levels values anymore, so this discrepancy would disappear.
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to introduce more templating in these classes. As I said above, hopefully we should be able to decouple more by using an abstract interface for the level decoders.
The reason will be displayed to describe this comment to others. Learn more.
This is mostly tied to what we can achieve with the LevelDecoder, but I think the templating can help the performance here (and this is private so it is not an extra burden on the user).
The reason will be displayed to describe this comment to others. Learn more.
Looking at column_reader.cc.o in release mode it goes from 720K in main to 2.1M in this PR (but it's not the worst offender).
We could first remove the template inheritance in the array and binary and readers that is not as critical if we want to save some space.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors Parquet C++ RecordReader / column reading internals to enable a fast-path for “flat optional” columns by decoding definition levels directly into an Arrow validity bitmap instead of materializing int16_t level arrays.
Changes:
Introduce a new reusable PageLevelDecoder abstraction and rework the legacy LevelDecoder to use it.
Add a specialized FlatOptionalTypedRecordReader that bypasses level materialization and writes validity directly from encoded levels.
Update Arrow/Parquet readers and tests to accommodate readers that do not expose level buffers (e.g., def_levels() can be nullptr).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
File
Description
cpp/src/parquet/reader_test.cc
Updates assertions for flat-optional readers that no longer report written/positioned levels.
cpp/src/parquet/level_decoder.h
Adds the new generic per-page level decoder implementation used by multiple reader paths.
cpp/src/parquet/column_reader.h
Adjusts public reader/decoder interfaces (virtualized RecordReader accessors; updated LevelDecoder API).
cpp/src/parquet/column_reader.cc
Implements the refactor, adds validity-bitmap decoding path for flat optional, restructures record readers and sinks.
cpp/src/parquet/column_reader_test.cc
Updates tests to skip level checks when levels are intentionally not exposed.
cpp/src/parquet/arrow/reader_internal.cc
Fixes zero-copy transfer to read values_written() before releasing buffers (release resets counters).
cpp/src/arrow/util/rle_bitmap_test.cc
Extends bitmap decoder tests to cover new CountUpTo behavior.
cpp/src/arrow/util/rle_bitmap_internal.h
Adds CountUpTo support and refactors bitmap decoder iteration helpers to share traversal logic.
Suppressed comments (1)
cpp/src/parquet/column_reader.h:100
Typos in the LevelDecoder API docs ("int32_to" and "decoder levels") reduce clarity for users reading the public header.
/// Decode a batch of levels int32_to an array and returns the number of levels decoded.
int32_t Decode(int32_t batch_size, int16_t* levels);
/// Advance the decoder and throw away decoder levels.
int32_t Skip(int32_t batch_size);
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cpp/src/parquet/column_reader.h:70
Doc comment has a couple typos that make it ambiguous ("repetition of definition level") and grammatically incorrect ("take"). This should match the same wording used elsewhere ("repetition or definition level") and use correct grammar/capitalization.
/// Decoder for repetition of definition level.
///
/// This decoder is used with either a deprecated bit packed (`BIT_PACKED = 4`)
/// encoding or a mixed bit packed and RLE one (`RLE = 3`).
/// Because it take as input a single buffer, `SetData` and `Decode` are typically
cpp/src/parquet/column_reader.h:409
Spelling mistake in comment: "reagardless" -> "regardless".
/// Always return false when the leaf is required, reagardless of the class input.
cpp/src/parquet/column_reader.h:381
The values_written() doc still claims FLBA/BYTE_ARRAY readers "reset the values after each call", but with the new ValueSink abstraction the count appears to accumulate until Reset()/ReleaseValues(). This comment should be updated to avoid misleading API consumers.
/// nulls and null_count() will be 0. There is no read-ahead/buffering for values. For
/// FLBA and ByteArray types this value reflects the values written with the last
/// ReadRecords call since those readers will reset the values after each call.
/// This is reset by both Reset() and ReleaseValues(), so it must be read before
/// the values buffer is transferred to the caller.
cpp/src/parquet/level_decoder.h:42
Minor doc typo/grammar: "Because it take" -> "Because it takes" (and consider capitalizing Parquet consistently).
/// Because it take as input a single buffer, `SetData` and `Decode` are typically
/// used on each of the parquet `DataPage`.
cpp/src/parquet/column_reader.h:96
Typo in the LevelDecoder API doc: "int32_to" should be "into" (and the sentence can be simplified).
/// Decode a batch of levels int32_to an array and returns the number of levels decoded.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
cpp/src/parquet/level_decoder.h:21
PageLevelDecoder uses std::min (and other <algorithm> utilities) but this header doesn't include <algorithm>, which can fail to compile depending on transitive includes.
The reason will be displayed to describe this comment to others. Learn more.
At first I wanted I lazily compute it, then I realize it would not be possible because this is likely called repeatedly across row groups (so I would need to do it eagerly).
Since the existing class can still handle the flat optional case, I am suggesting we enable it behind a flag (default to false) in RecordReader::Make and take the API evolution separately.
The reason will be displayed to describe this comment to others. Learn more.
Since the existing class can still handle the flat optional case, I am suggesting we enable it behind a flag (default to false) in RecordReader::Make and take the API evolution separately.
Another option is to introduce a new interface and leave the RecordReader API untouched (there are already several inelegant options read_dictionary, read_dense_for_nullable).
I don't know how much work that would necessitate?
Document null level accessor contract for flat optional readers
cpp/src/parquet/column_reader.h:368
Flat optional readers now return nullptr for this accessor and report zero level counters, but the public RecordReader contract still only says “Decoded definition levels.” This is a breaking semantic change for callers that size or consume the buffer through levels_position(); if the non-materialized behavior is intentional, document the null-pointer contract (and that callers must not dereference it) here, along with the corresponding repetition-level behavior.
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
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.
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
Rationale for this change
WIP
What changes are included in this PR?
Are these changes tested?
Tests are expected to fail until we figure out what
int16_t* RecordReader::def_levels()should do.Are there any user-facing changes?
RecordReaderfor flat optional #50628