Copilot commented on code in PR #50629:
URL: https://github.com/apache/arrow/pull/50629#discussion_r4217243891
##########
cpp/src/parquet/column_reader.h:
##########
@@ -280,20 +291,41 @@ namespace internal {
/// \since 1.3.0
class PARQUET_EXPORT RecordReader {
public:
+ /// \brief Parameters to create a record reader.
+ struct MakeParams {
+ /// Column descriptor
+ const ColumnDescriptor* descr = NULLPTR;
+ /// Level info, used to determine if a column is nullable or not
+ LevelInfo leaf_info;
+ /// Memory pool to use for buffering values and rep/def levels
+ ::arrow::MemoryPool* pool = ::arrow::default_memory_pool();
+ /// True if reading directly as Arrow dictionary-encoded
+ bool read_dictionary = false;
+ /// True if reading dense and not leaving space for null values
+ bool read_dense_for_nullable = false;
+ /// Which type to read this column as (optional). Currently only used for
byte
+ /// array columns (see BinaryRecordReader::GetBuilderChunks).
+ std::shared_ptr<::arrow::DataType> arrow_type = NULLPTR;
+ /// Allow optimizations prevent the computation of definition or
repetition levels.
+ ///
+ /// Attempting to access levels that are not computed will throw an
exception.
+ /// Currently this only enable an optimization for flat (non nested)
optional values
+ /// where definition levels are decoded directly into the validity bitmap.
+ /// Other optimization may be added in the future.
+ bool allow_levels_optimization = false;
+ };
+
+ /// \brief Creates a record reader.
+ static std::shared_ptr<RecordReader> Make(const MakeParams& params);
+
/// \brief Creates a record reader.
- /// @param descr Column descriptor
- /// @param leaf_info Level info, used to determine if a column is nullable
or not
- /// @param pool Memory pool to use for buffering values and rep/def levels
- /// @param read_dictionary True if reading directly as Arrow
dictionary-encoded
- /// @param read_dense_for_nullable True if reading dense and not leaving
space for null
- /// values
- /// @param arrow_type Which type to read this column as (optional). Currently
- /// only used for byte array columns (see
BinaryRecordReader::GetBuilderChunks).
+ /// \see DispatchParams for the parameters documentation.
static std::shared_ptr<RecordReader> Make(
const ColumnDescriptor* descr, LevelInfo leaf_info,
::arrow::MemoryPool* pool = ::arrow::default_memory_pool(),
bool read_dictionary = false, bool read_dense_for_nullable = false,
- const std::shared_ptr<::arrow::DataType>& arrow_type = NULLPTR);
+ const std::shared_ptr<::arrow::DataType>& arrow_type = NULLPTR,
+ bool allow_levels_optimization = false);
Review Comment:
Appending `allow_levels_optimization` to this exported function changes the
C++ mangled signature, so existing binaries that call the six-argument
`RecordReader::Make` will fail to link even though the new parameter has a
default. Please retain an exported six-argument overload (forwarding to the new
implementation) and add the seven-argument overload separately, preserving the
ABI for this public API.
##########
cpp/src/parquet/file_reader.cc:
##########
@@ -128,9 +128,14 @@ std::shared_ptr<internal::RecordReader>
RowGroupReader::RecordReader(
internal::LevelInfo level_info =
internal::LevelInfo::ComputeLevelInfo(descr);
- auto reader = internal::RecordReader::Make(
- descr, level_info, contents_->properties()->memory_pool(),
read_dictionary,
- contents_->properties()->read_dense_for_nullable());
+ auto reader = internal::RecordReader::Make({
+ .descr = descr,
+ .leaf_info = level_info,
+ .pool = contents_->properties()->memory_pool(),
+ .read_dictionary = read_dictionary,
+ .read_dense_for_nullable =
contents_->properties()->read_dense_for_nullable(),
+ .allow_levels_optimization = true,
Review Comment:
This unconditionally opts `RowGroupReader::RecordReader()` into the new
breaking mode, so existing callers of the low-level `def_levels()` accessor now
receive an exception for flat optional columns instead of the decoded levels.
The PR description leaves this contract unresolved; please preserve the old
behavior for this public entry point or add an explicit opt-in and
document/update its callers before enabling it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
