adamreeve commented on code in PR #51362:
URL: https://github.com/apache/arrow/pull/51362#discussion_r4035149479
##########
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:
OK yes I took another look and you're right, `column_indices` isn't correct
either. I saw that `FileReaderImpl::ReadColumn` uses this index (`i`) to index
into the leaf-level columns of the Parquet schema so had thought that
`column_indices` is the correct thing to use here. But there is only one
`FileReaderImpl` per top-level column. You've explained this problem in the new
#51370 issue. I'm not sure what the right solution is there, and I agree that
fixing this is a separate issue.
--
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]
