kou commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2571431259
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4165,10 +4165,11 @@ void TryReadDataFile(const std::string& path,
auto pool = ::arrow::default_memory_pool();
std::unique_ptr<FileReader> arrow_reader;
Review Comment:
Could you remove this?
```suggestion
```
##########
cpp/src/parquet/arrow/reader.h:
##########
@@ -126,6 +126,14 @@ class PARQUET_EXPORT FileReader {
std::unique_ptr<ParquetFileReader> reader,
std::unique_ptr<FileReader>* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result<std::unique_ptr<FileReader>> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr<ParquetFileReader> reader,
+ const ArrowReaderProperties& properties);
+
+ /// Factory function to create a FileReader from a ParquetFileReader
+ static ::arrow::Result<std::unique_ptr<FileReader>> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr<ParquetFileReader> reader);
Review Comment:
```suggestion
::arrow::MemoryPool* pool, std::unique_ptr<ParquetFileReader> reader);
```
##########
cpp/src/parquet/arrow/reader.h:
##########
@@ -126,6 +126,14 @@ class PARQUET_EXPORT FileReader {
std::unique_ptr<ParquetFileReader> reader,
std::unique_ptr<FileReader>* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result<std::unique_ptr<FileReader>> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr<ParquetFileReader> reader,
+ const ArrowReaderProperties& properties);
Review Comment:
Could you add `ARROW_DEPRECATED()` to `Status` versions like
https://github.com/apache/arrow/blob/d16ba00a07424ce28eff0c1a4c313c516f5c31bb/cpp/src/parquet/arrow/reader.h#L194-L207
?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4165,10 +4165,11 @@ void TryReadDataFile(const std::string& path,
auto pool = ::arrow::default_memory_pool();
std::unique_ptr<FileReader> arrow_reader;
- Status s =
- FileReader::Make(pool, ParquetFileReader::OpenFile(path, false),
&arrow_reader);
- if (s.ok()) {
+ Status s;
+ Result result = FileReader::Make(pool, ParquetFileReader::OpenFile(path,
false));
+ if (result.ok()) {
std::shared_ptr<::arrow::Table> table;
+ auto arrow_reader = result->get();
s = arrow_reader->ReadTable(&table);
Review Comment:
```suggestion
s = (*reader_result)->ReadTable(&table);
```
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -1343,14 +1343,35 @@ Status FileReader::Make(::arrow::MemoryPool* pool,
std::unique_ptr<ParquetFileReader> reader,
const ArrowReaderProperties& properties,
std::unique_ptr<FileReader>* out) {
- *out = std::make_unique<FileReaderImpl>(pool, std::move(reader), properties);
- return static_cast<FileReaderImpl*>(out->get())->Init();
+ ARROW_ASSIGN_OR_RAISE(auto result, Make(pool, std::move(reader),
properties));
+ *out = std::move(result);
Review Comment:
Does this work?
```suggestion
ARROW_ASSIGN_OR_RAISE(*out, Make(pool, std::move(reader), properties));
```
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -1343,14 +1343,35 @@ Status FileReader::Make(::arrow::MemoryPool* pool,
std::unique_ptr<ParquetFileReader> reader,
const ArrowReaderProperties& properties,
std::unique_ptr<FileReader>* out) {
- *out = std::make_unique<FileReaderImpl>(pool, std::move(reader), properties);
- return static_cast<FileReaderImpl*>(out->get())->Init();
+ ARROW_ASSIGN_OR_RAISE(auto result, Make(pool, std::move(reader),
properties));
+ *out = std::move(result);
+ return Status::OK();
}
Status FileReader::Make(::arrow::MemoryPool* pool,
std::unique_ptr<ParquetFileReader> reader,
std::unique_ptr<FileReader>* out) {
- return Make(pool, std::move(reader), default_arrow_reader_properties(), out);
+ ARROW_ASSIGN_OR_RAISE(auto result,
+ Make(pool, std::move(reader),
default_arrow_reader_properties()));
+ *out = std::move(result);
Review Comment:
```suggestion
ARROW_ASSIGN_OR_RAISE(*out,
Make(pool, std::move(reader),
default_arrow_reader_properties()));
```
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -4165,10 +4165,11 @@ void TryReadDataFile(const std::string& path,
auto pool = ::arrow::default_memory_pool();
std::unique_ptr<FileReader> arrow_reader;
- Status s =
- FileReader::Make(pool, ParquetFileReader::OpenFile(path, false),
&arrow_reader);
- if (s.ok()) {
+ Status s;
+ Result result = FileReader::Make(pool, ParquetFileReader::OpenFile(path,
false));
+ if (result.ok()) {
Review Comment:
```suggestion
auto reader_result = FileReader::Make(pool,
ParquetFileReader::OpenFile(path, false));
if (reader_result.ok()) {
```
(I like `XXX_result` but others use `maybe_XXX` or something.)
--
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]
