emkornfield commented on code in PR #38867:
URL: https://github.com/apache/arrow/pull/38867#discussion_r1405594492


##########
cpp/src/parquet/arrow/reader.h:
##########
@@ -187,6 +188,11 @@ class PARQUET_EXPORT FileReader {
       const std::vector<int>& row_group_indices, const std::vector<int>& 
column_indices,
       std::unique_ptr<::arrow::RecordBatchReader>* out) = 0;
 
+  virtual ::arrow::Status GetRecordBatchReader(
+      const std::vector<int>& row_group_indices, const std::vector<int>& 
column_indices,
+      const std::shared_ptr<std::map<int, RowRangesPtr>>& row_ranges_map,

Review Comment:
   Please document this in the method contract.  The way this is being passed 
(as a shared pointer and values being pointer seems a little strange to me) is 
there a reason how this contract was arrived at (I need to take a closer look 
at how Range was arrived at).
   
   I'm not sure if keeping consistency for a brand new method is worth it or if 
this should return arrow::Result<std::unique_ptr<RecordBatchReader>> at some 
point we should convert all the methods to return result.



-- 
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]

Reply via email to