wgtmac commented on code in PR #37003:
URL: https://github.com/apache/arrow/pull/37003#discussion_r1289557178
##########
cpp/src/parquet/file_reader.h:
##########
@@ -58,6 +59,11 @@ class PARQUET_EXPORT RowGroupReader {
// column. Ownership is shared with the RowGroupReader.
std::shared_ptr<ColumnReader> Column(int i);
+ // Construct a RecordReader for the indicated row group-relative column i.
+ // Ownership is shared with the RowGroupReader.
+ std::shared_ptr<internal::RecordReader> RecordReader(
+ int i, bool read_dictionary = false, bool read_dense_for_nullable =
false);
Review Comment:
Can we wrap `read_dictionary` and `read_dense_for_nullable` into
ArrowReaderProperties / ReaderProperties? These properties are easily expanded
to make it difficult to maintain. Both ArrowReaderProperties and
ReaderProperties are available in the RowGroupReader, so we can simplify the
interface to `std::shared_ptr<RecordReader> RecordReader(int i)`.
##########
cpp/src/parquet/file_reader.h:
##########
@@ -24,6 +24,7 @@
#include "arrow/io/caching.h"
#include "arrow/util/type_fwd.h"
+#include "parquet/column_reader.h"
Review Comment:
Please use forward declaration like line 34 below instead of including the
header file.
##########
cpp/src/parquet/file_reader.h:
##########
@@ -58,6 +59,11 @@ class PARQUET_EXPORT RowGroupReader {
// column. Ownership is shared with the RowGroupReader.
std::shared_ptr<ColumnReader> Column(int i);
+ // Construct a RecordReader for the indicated row group-relative column i.
+ // Ownership is shared with the RowGroupReader.
+ std::shared_ptr<internal::RecordReader> RecordReader(
Review Comment:
IMHO, I'd prefer to move RecordReader out of internal namespace first.
Because we can know the consequence of namespace change before exposing it.
--
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]
