emkornfield commented on a change in pull request #7704: URL: https://github.com/apache/arrow/pull/7704#discussion_r453149789
########## File path: cpp/src/parquet/arrow/reader.cc ########## @@ -301,62 +302,50 @@ class FileReaderImpl : public FileReader { class RowGroupRecordBatchReader : public ::arrow::RecordBatchReader { public: - RowGroupRecordBatchReader(std::vector<std::unique_ptr<ColumnReaderImpl>> field_readers, - std::shared_ptr<::arrow::Schema> schema, int64_t batch_size) - : field_readers_(std::move(field_readers)), - schema_(std::move(schema)), - batch_size_(batch_size) {} - ~RowGroupRecordBatchReader() override {} std::shared_ptr<::arrow::Schema> schema() const override { return schema_; } - static Status Make(const std::vector<int>& row_groups, - const std::vector<int>& column_indices, FileReaderImpl* reader, - int64_t batch_size, - std::unique_ptr<::arrow::RecordBatchReader>* out) { - std::vector<int> field_indices; - if (!reader->manifest_.GetFieldIndices(column_indices, &field_indices)) { - return Status::Invalid("Invalid column index"); - } + static ::arrow::Result<std::unique_ptr<RowGroupRecordBatchReader>> Make( + const std::vector<int>& row_group_indices, const std::vector<int>& column_indices, + int64_t batch_size, FileReaderImpl* reader) { + std::unique_ptr<RowGroupRecordBatchReader> out(new RowGroupRecordBatchReader); - std::vector<std::unique_ptr<ColumnReaderImpl>> field_readers(field_indices.size()); - std::vector<std::shared_ptr<Field>> fields; + RETURN_NOT_OK(FromParquetSchema( + reader->parquet_reader()->metadata()->schema(), default_arrow_reader_properties(), + /*key_value_metadata=*/nullptr, column_indices, &out->schema_)); - auto included_leaves = VectorToSharedSet(column_indices); - for (size_t i = 0; i < field_indices.size(); ++i) { - RETURN_NOT_OK(reader->GetFieldReader(field_indices[i], included_leaves, row_groups, - &field_readers[i])); - fields.push_back(field_readers[i]->field()); - } - out->reset(new RowGroupRecordBatchReader(std::move(field_readers), - ::arrow::schema(fields), batch_size)); - return Status::OK(); + using ::arrow::RecordBatchIterator; + + auto row_group_index_to_batch_iterator = + [=](const int* i) -> ::arrow::Result<RecordBatchIterator> { Review comment: this binding everything by value here intentional? If so could you add a comment why? ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org