adamreeve commented on code in PR #51362:
URL: https://github.com/apache/arrow/pull/51362#discussion_r4031330179
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -1379,11 +1379,18 @@ Future<std::shared_ptr<Table>>
FileReaderImpl::DecodeRowGroups(
// OptionalParallelForAsync requires an executor
if (!cpu_executor) cpu_executor = ::arrow::internal::GetCpuThreadPool();
- auto read_column = [row_groups, self, this](size_t i,
-
std::shared_ptr<ColumnReaderImpl> reader)
+ // `readers` only holds the requested columns, so its index `i` is a
position within
+ // the selection, not the column's actual index in the row group. Map back
to the
+ // real column index (as GetFieldReaders does internally) before calling
ReadColumn,
+ // which indexes RowGroupMetaData::ColumnChunk() by the latter.
+ ARROW_ASSIGN_OR_RAISE(std::vector<int> field_indices,
+ manifest_.GetFieldIndices(column_indices));
+
+ auto read_column = [row_groups, field_indices, self, this](
+ size_t i, std::shared_ptr<ColumnReaderImpl> reader)
-> ::arrow::Result<std::shared_ptr<::arrow::ChunkedArray>> {
std::shared_ptr<::arrow::ChunkedArray> column;
- RETURN_NOT_OK(ReadColumn(static_cast<int>(i), row_groups, reader.get(),
&column));
+ RETURN_NOT_OK(ReadColumn(field_indices[i], row_groups, reader.get(),
&column));
Review Comment:
It looks like this should be using `column_indices` to get the Parquet leaf
column index.
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -271,13 +271,13 @@ class FileReaderImpl : public FileReader {
Status ReadColumn(int i, const std::vector<int>& row_groups, ColumnReader*
reader,
std::shared_ptr<ChunkedArray>* out) {
BEGIN_PARQUET_CATCH_EXCEPTIONS
- // TODO(wesm): This calculation doesn't make much sense when we have
repeated
- // schema nodes
+ // NextBatch()'s size is a number of records (rows), not leaf values, so
use the
+ // row group's own row count directly rather than some column's
num_values() (which
+ // (a) requires picking a column, a prior source of index-confusion bugs,
and
+ // (b) over-counts for repeated schema nodes, counting elements rather
than rows).
Review Comment:
```suggestion
// row group's row count directly.
```
It's pretty common for AI to generate comments like this that compare the
new code to the old way it was done, but I don't think these are helpful to
leave in the code. The first part of the comment is enough to explain why
num_rows is used, and any explanation of the need for the change can go in the
PR description.
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -271,13 +271,13 @@ class FileReaderImpl : public FileReader {
Status ReadColumn(int i, const std::vector<int>& row_groups, ColumnReader*
reader,
std::shared_ptr<ChunkedArray>* out) {
BEGIN_PARQUET_CATCH_EXCEPTIONS
- // TODO(wesm): This calculation doesn't make much sense when we have
repeated
- // schema nodes
+ // NextBatch()'s size is a number of records (rows), not leaf values, so
use the
+ // row group's own row count directly rather than some column's
num_values() (which
+ // (a) requires picking a column, a prior source of index-confusion bugs,
and
+ // (b) over-counts for repeated schema nodes, counting elements rather
than rows).
int64_t records_to_read = 0;
for (auto row_group : row_groups) {
- // Can throw exception
- records_to_read +=
-
reader_->metadata()->RowGroup(row_group)->ColumnChunk(i)->num_values();
+ records_to_read += reader_->metadata()->RowGroup(row_group)->num_rows();
Review Comment:
> NextBatch()'s size is a number of records (rows), not leaf values
I think this is correct after looking through the code, but this is not very
clear. Maybe `NextBatch` and `LoadBatch` methods could have better
documentation comments. The number of records ends up being passed through to
`RecordReader::ReadRecords` which is better documented.
--
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]
