adamreeve commented on code in PR #51401:
URL: https://github.com/apache/arrow/pull/51401#discussion_r4059134079
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -292,15 +298,17 @@ class FileReaderImpl : public FileReader {
END_PARQUET_CATCH_EXCEPTIONS
}
- Status ReadColumn(int i, const std::vector<int>& row_groups,
+ Status ReadColumn(int column_index, const std::vector<int>& row_groups,
std::shared_ptr<ChunkedArray>* out) {
std::unique_ptr<ColumnReader> flat_column_reader;
- RETURN_NOT_OK(GetColumn(i, SomeRowGroupsFactory(row_groups),
&flat_column_reader));
- return ReadColumn(i, row_groups, flat_column_reader.get(), out);
+ RETURN_NOT_OK(
+ GetColumn(column_index, SomeRowGroupsFactory(row_groups),
&flat_column_reader));
+ ARROW_ASSIGN_OR_RAISE(auto field_indices,
manifest_.GetFieldIndices({column_index}));
Review Comment:
I don't think this change is correct. See the documentation for
`FileReader::ReadColumn`
[here](https://github.com/apache/arrow/blob/70f5c269ae8d154f57f7a7ec6babe6e0685ed6ec/cpp/src/parquet/arrow/reader.h#L140-L153).
In this method, `i` (or now `column_index`) is already a top-level field
index. So there's no need to convert to a a field index using
`manifest_.GetFieldIndices`.
For clarity, it would make sense to rename `i` to `field_index` in this
method and the one below (and in the header too).
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -1374,14 +1382,16 @@ Future<std::shared_ptr<Table>>
FileReaderImpl::DecodeRowGroups(
std::vector<std::shared_ptr<ColumnReaderImpl>> readers;
std::shared_ptr<::arrow::Schema> result_schema;
RETURN_NOT_OK(GetFieldReaders(column_indices, row_groups, &readers,
&result_schema));
+ ARROW_ASSIGN_OR_RAISE(auto field_indices,
manifest_.GetFieldIndices(column_indices));
Review Comment:
This change being correct relies on an assumption that the order of field
indices returned by `manifest_.GetFieldIndices` is the same as the order the
readers are in. This is true, because of how `GetFieldReaders` is implemented,
but a bit fragile and could be broken by a future code change.
Can you instead refactor `GetFieldReaders` so it also returns the field
indices? It's a method on the internal `FileReaderImpl` class so it's OK to
change the method signature.
This would also avoid the overhead of creating the field indices vector
twice.
--
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]
