Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
conbench-apache-arrow[bot] commented on PR #48285: URL: https://github.com/apache/arrow/pull/48285#issuecomment-3649551196 After merging your PR, Conbench analyzed the 2 benchmarking runs that have been run so far on merge-commit 7a36fcc8b7456bea52911f9601b26be51d16265a. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The [full Conbench report](https://github.com/apache/arrow/runs/57975887229) has more details. -- 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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou merged PR #48285: URL: https://github.com/apache/arrow/pull/48285 -- 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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2608816893
##
cpp/src/parquet/arrow/reader.cc:
##
@@ -1343,14 +1343,33 @@ Status FileReader::Make(::arrow::MemoryPool* pool,
std::unique_ptr reader,
const ArrowReaderProperties& properties,
std::unique_ptr* out) {
- *out = std::make_unique(pool, std::move(reader), properties);
- return static_cast(out->get())->Init();
+ ARROW_ASSIGN_OR_RAISE(*out, Make(pool, std::move(reader), properties));
+ return Status::OK();
}
Status FileReader::Make(::arrow::MemoryPool* pool,
std::unique_ptr reader,
std::unique_ptr* out) {
- return Make(pool, std::move(reader), default_arrow_reader_properties(), out);
+ ARROW_ASSIGN_OR_RAISE(*out,
+Make(pool, std::move(reader),
default_arrow_reader_properties()));
+ return Status::OK();
+}
+
+Result> FileReader::Make(
+::arrow::MemoryPool* pool, std::unique_ptr
parquet_reader,
+const ArrowReaderProperties& properties) {
+ std::unique_ptr reader =
+ std::make_unique(pool, std::move(parquet_reader),
properties);
+ RETURN_NOT_OK(static_cast(reader.get())->Init());
+ return reader;
+}
+
+Result> FileReader::Make(
+::arrow::MemoryPool* pool, std::unique_ptr
parquet_reader) {
+ std::unique_ptr reader = std::make_unique(
+ pool, std::move(parquet_reader), default_arrow_reader_properties());
+ RETURN_NOT_OK(static_cast(reader.get())->Init());
+ return reader;
Review Comment:
```suggestion
return Make(pool, std::move(parquet_reader),
default_arrow_reader_properties());
```
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on PR #48285: URL: https://github.com/apache/arrow/pull/48285#issuecomment-3631717368 @kou Thank you for your review. I add `auto` parameter. Please take a look when you get a chance. -- 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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2597616453
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4259,13 +4260,15 @@ TEST(TestArrowReaderAdHoc,
LARGE_MEMORY_TEST(LargeStringColumn)) {
auto reader =
ParquetFileReader::Open(std::make_shared(tables_buffer));
std::unique_ptr arrow_reader;
- ASSERT_OK(FileReader::Make(default_memory_pool(), std::move(reader),
&arrow_reader));
+ ASSERT_OK_AND_ASSIGN(arrow_reader,
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(auto arrow_reader,
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4494,8 +4497,8 @@ TEST(TestArrowReaderAdHoc, ReadFloat16Files) {
ARROW_SCOPED_TRACE("path = ", path);
std::unique_ptr reader;
-ASSERT_OK_NO_THROW(
-FileReader::Make(pool, ParquetFileReader::OpenFile(path, false),
&reader));
+ASSERT_OK_AND_ASSIGN(
+reader, FileReader::Make(pool, ParquetFileReader::OpenFile(path,
false)));
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(
auto reader, FileReader::Make(pool,
ParquetFileReader::OpenFile(path, false)));
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4617,8 +4620,8 @@ TEST(TestArrowColumnReader, NextBatchEmptyInput) {
auto reader =
ParquetFileReader::Open(std::make_shared(buffer));
std::unique_ptr file_reader;
- ASSERT_OK(
- FileReader::Make(::arrow::default_memory_pool(), std::move(reader),
&file_reader));
+ ASSERT_OK_AND_ASSIGN(
+ file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(
auto file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -5729,7 +5733,8 @@ TEST(TestArrowReadWrite, WriteAndReadRecordBatch) {
read_properties.set_batch_size(record_batch->num_rows());
auto reader =
ParquetFileReader::Open(std::make_shared(buffer));
std::unique_ptr arrow_reader;
- ASSERT_OK(FileReader::Make(pool, std::move(reader), read_properties,
&arrow_reader));
+ ASSERT_OK_AND_ASSIGN(arrow_reader,
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(auto arrow_reader,
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4567,8 +4570,8 @@ TEST(TestArrowFileReader, RecordBatchReaderEmptyInput) {
auto reader =
ParquetFileReader::Open(std::make_shared(buffer));
std::unique_ptr file_reader;
- ASSERT_OK(
- FileReader::Make(::arrow::default_memory_pool(), std::move(reader),
&file_reader));
+ ASSERT_OK_AND_ASSIGN(
+ file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(
auto file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4539,8 +4542,8 @@ TEST(TestArrowFileReader,
RecordBatchReaderEmptyRowGroups) {
auto reader =
ParquetFileReader::Open(std::make_shared(buffer));
std::unique_ptr file_reader;
- ASSERT_OK(
- FileReader::Make(::arrow::default_memory_pool(), std::move(reader),
&file_reader));
+ ASSERT_OK_AND_ASSIGN(
+ file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(
auto file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4362,8 +4365,8 @@ TEST(TestArrowReaderAdHoc, LegacyTwoLevelList) {
// Verify Arrow schema and data
std::unique_ptr reader;
-ASSERT_OK_NO_THROW(
-FileReader::Make(default_memory_pool(), std::move(file_reader),
&reader));
+ASSERT_OK_AND_ASSIGN(reader,
+ FileReader::Make(default_memory_pool(),
std::move(file_reader)));
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(auto reader,
FileReader::Make(default_memory_pool(),
std::move(file_reader)));
```
##
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##
@@ -4591,8 +4594,8 @@ TEST(TestArrowColumnReader, NextBatchZeroBatchSize) {
auto reader =
ParquetFileReader::Open(std::make_shared(buffer));
std::unique_ptr file_reader;
- ASSERT_OK(
- FileReader::Make(::arrow::default_memory_pool(), std::move(reader),
&file_reader));
+ ASSERT_OK_AND_ASSIGN(
+ file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(
auto file_reader, FileReader::Make(::arrow::default_memory_pool(),
std::move(reader)));
```
##
cpp/src/parquet/arrow/arrow_reader_writer_tes
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on PR #48285: URL: https://github.com/apache/arrow/pull/48285#issuecomment-3624578380 @kou Could you check this PR when you get a chance? -- 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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2579826936
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -736,8 +737,9 @@ static void BM_ReadIndividualRowGroups(::benchmark::State&
state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr arrow_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &arrow_reader));
+auto reader_result =
Review Comment:
Thanks. Change to `EXIT_NOT_OK`.
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2579638308
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -795,8 +799,8 @@ static void
BM_ReadMultipleRowGroupsGenerator(::benchmark::State& state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr unique_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &unique_reader));
+ASSIGN_OR_ABORT(unique_reader,
Review Comment:
Thanks. Fixed. I used `EXIT_NOT_OK` again.
`ASSIGN_OR_ABORT` used in the following line.
The two macros do not seem to behave the same on failure.
```
ASSIGN_OR_ABORT(auto generator,
arrow_reader->GetRecordBatchGenerator(arrow_reader, rgs,
{0}));
```
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2579399239
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -795,8 +799,8 @@ static void
BM_ReadMultipleRowGroupsGenerator(::benchmark::State& state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr unique_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &unique_reader));
+ASSIGN_OR_ABORT(unique_reader,
Review Comment:
No.
If you need a convenient macro, define it in this file.
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2579392265
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -795,8 +799,8 @@ static void
BM_ReadMultipleRowGroupsGenerator(::benchmark::State& state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr unique_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &unique_reader));
+ASSIGN_OR_ABORT(unique_reader,
Review Comment:
Can we change to `ASSIGN_OR_ABORT` instead of `EXIT_NOT_OK`?
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285: URL: https://github.com/apache/arrow/pull/48285#discussion_r2579390132 ## cpp/src/parquet/arrow/reader_writer_benchmark.cc: ## @@ -297,8 +297,9 @@ static void BenchmarkReadTable(::benchmark::State& state, const Table& table, auto reader = ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer)); std::unique_ptr arrow_reader; -EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(), std::move(reader), - &arrow_reader)); +auto reader_result = +FileReader::Make(::arrow::default_memory_pool(), std::move(reader)); +EXIT_NOT_OK(reader_result.status()); Review Comment: Thanks! fixed -- 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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou commented on code in PR #48285: URL: https://github.com/apache/arrow/pull/48285#discussion_r2579297355 ## cpp/src/parquet/arrow/reader_writer_benchmark.cc: ## @@ -297,8 +297,9 @@ static void BenchmarkReadTable(::benchmark::State& state, const Table& table, auto reader = ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer)); std::unique_ptr arrow_reader; -EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(), std::move(reader), - &arrow_reader)); +auto reader_result = +FileReader::Make(::arrow::default_memory_pool(), std::move(reader)); +EXIT_NOT_OK(reader_result.status()); Review Comment: Yes. -- 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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2579287461
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -736,8 +737,9 @@ static void BM_ReadIndividualRowGroups(::benchmark::State&
state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr arrow_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &arrow_reader));
+auto reader_result =
+FileReader::Make(::arrow::default_memory_pool(), std::move(reader));
+EXIT_NOT_OK(reader_result.status());
Review Comment:
ditto
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285: URL: https://github.com/apache/arrow/pull/48285#discussion_r2579286890 ## cpp/src/parquet/arrow/reader_writer_benchmark.cc: ## @@ -297,8 +297,9 @@ static void BenchmarkReadTable(::benchmark::State& state, const Table& table, auto reader = ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer)); std::unique_ptr arrow_reader; -EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(), std::move(reader), - &arrow_reader)); +auto reader_result = +FileReader::Make(::arrow::default_memory_pool(), std::move(reader)); +EXIT_NOT_OK(reader_result.status()); Review Comment: Thanks. It seems that `arrow_reader` doesn't use after declaration. ``` arrow/arrow/cpp/src/parquet/arrow/reader_writer_benchmark.cc:302:10: error: call to implicitly-deleted copy constructor of 'std::unique_ptr' 302 | auto arrow_reader = *arrow_reader_result; | ^ ``` Do we need the following instead of `auto arrow_reader = *arrow_reader_result`? ``` std::shared_ptr arrow_reader = std::move(*arrow_reader_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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2577082647
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -736,8 +737,9 @@ static void BM_ReadIndividualRowGroups(::benchmark::State&
state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr arrow_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &arrow_reader));
+auto reader_result =
Review Comment:
No. `ASSERT_OK_AND_ASSIGN()` is for Google Test. Benchmarks uses Google
Benchmark not Google Test.
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -297,8 +297,9 @@ static void BenchmarkReadTable(::benchmark::State& state,
const Table& table,
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr arrow_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &arrow_reader));
+auto reader_result =
+FileReader::Make(::arrow::default_memory_pool(), std::move(reader));
+EXIT_NOT_OK(reader_result.status());
Review Comment:
We need to assign to `arrow_reader`:
```suggestion
auto arrow_reader_result =
FileReader::Make(::arrow::default_memory_pool(), std::move(reader));
EXIT_NOT_OK(arrow_reader_result.status());
auto arrow_reader = *arrow_reader_result;
```
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -736,8 +737,9 @@ static void BM_ReadIndividualRowGroups(::benchmark::State&
state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr arrow_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &arrow_reader));
+auto reader_result =
+FileReader::Make(::arrow::default_memory_pool(), std::move(reader));
+EXIT_NOT_OK(reader_result.status());
Review Comment:
```suggestion
auto arrow_reader_result =
FileReader::Make(::arrow::default_memory_pool(), std::move(reader));
EXIT_NOT_OK(reader_result.status());
auto arrow_reader = *arrow_reader_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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2577009993
##
cpp/src/parquet/arrow/reader_writer_benchmark.cc:
##
@@ -736,8 +737,9 @@ static void BM_ReadIndividualRowGroups(::benchmark::State&
state) {
auto reader =
ParquetFileReader::Open(std::make_shared<::arrow::io::BufferReader>(buffer));
std::unique_ptr arrow_reader;
-EXIT_NOT_OK(FileReader::Make(::arrow::default_memory_pool(),
std::move(reader),
- &arrow_reader));
+auto reader_result =
Review Comment:
Can we use `ASSERT_OK_AND_ASSIGN` here?
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2573033495
##
cpp/src/parquet/arrow/reader.h:
##
@@ -126,6 +126,14 @@ class PARQUET_EXPORT FileReader {
std::unique_ptr reader,
std::unique_ptr* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr reader,
+ const ArrowReaderProperties& properties);
Review Comment:
Thanks! Add `ARROW_DEPRECATED` and fix some codes.
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
kou commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2572722146
##
cpp/src/parquet/arrow/reader.h:
##
@@ -126,6 +126,14 @@ class PARQUET_EXPORT FileReader {
std::unique_ptr reader,
std::unique_ptr* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr reader,
+ const ArrowReaderProperties& properties);
Review Comment:
"23.0.0". Our next release uses "23.0.0".
> Should I fix this?
Yes.
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato commented on code in PR #48285:
URL: https://github.com/apache/arrow/pull/48285#discussion_r2571558685
##
cpp/src/parquet/arrow/reader.h:
##
@@ -126,6 +126,14 @@ class PARQUET_EXPORT FileReader {
std::unique_ptr reader,
std::unique_ptr* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr reader,
+ const ArrowReaderProperties& properties);
Review Comment:
`23.0.0` or `24.0.0` in this case?
```diff
/Users/hsato/OpenProjects/arrow/arrow/cpp/src/parquet/arrow/reader.h:120:3:
note: 'Make' has been explicitly marked deprecated hediff --git
a/cpp/src/parquet/arrow/reader.h b/cpp/src/parquet/arrow/reader.h
index 3bdd7f18ca..996cda1aaf 100644
--- a/cpp/src/parquet/arrow/reader.h
+++ b/cpp/src/parquet/arrow/reader.h
@@ -116,12 +116,16 @@ class RowGroupReader;
class PARQUET_EXPORT FileReader {
public:
/// Factory function to create a FileReader from a ParquetFileReader and
properties
+ /// \deprecated Deprecated in 23.0.0. Use arrow::Result version instead.
+ ARROW_DEPRECATED("Deprecated in 23.0.0. Use arrow::Result version
instead.")
static ::arrow::Status Make(::arrow::MemoryPool* pool,
std::unique_ptr reader,
const ArrowReaderProperties& properties,
std::unique_ptr* out);
/// Factory function to create a FileReader from a ParquetFileReader
+ /// \deprecated Deprecated in 23.0.0. Use arrow::Result version instead.
+ ARROW_DEPRECATED("Deprecated in 23.0.0. Use arrow::Result version
instead.")
static ::arrow::Status Make(::arrow::MemoryPool* pool,
std::unique_ptr reader,
std::unique_ptr* out);
```
When I add deprecated, an error occurs in this area. Should I fix this? (Or
add `ARROW_DEPRECATED` in another PR?)
```
/path/toarrow/arrow/cpp/src/parquet/arrow/reader.cc:1407:22: error: 'Make'
is deprecated: Deprecated in 23.0.0. Use arrow::Result version instead.
[-Werror,-Wdeprecated-declarations]
1407 | return FileReader::Make(pool_, std::move(raw_reader_),
properties_, out);
| ^
```
--
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]
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
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 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 reader,
std::unique_ptr* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr reader,
+ const ArrowReaderProperties& properties);
+
+ /// Factory function to create a FileReader from a ParquetFileReader
+ static ::arrow::Result> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr reader);
Review Comment:
```suggestion
::arrow::MemoryPool* pool, std::unique_ptr reader);
```
##
cpp/src/parquet/arrow/reader.h:
##
@@ -126,6 +126,14 @@ class PARQUET_EXPORT FileReader {
std::unique_ptr reader,
std::unique_ptr* out);
+ /// Factory function to create a FileReader from a ParquetFileReader and
properties
+ static ::arrow::Result> Make(
+ ::arrow::MemoryPool* pool, std::unique_ptr 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 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 reader,
const ArrowReaderProperties& properties,
std::unique_ptr* out) {
- *out = std::make_unique(pool, std::move(reader), properties);
- return static_cast(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 reader,
const ArrowReaderProperties& properties,
std::unique_ptr* out) {
- *out = std::make_unique(pool, std::move(reader), properties);
- return static_cast(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 reader,
std::unique_ptr* 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 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.
T
Re: [PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
github-actions[bot] commented on PR #48285: URL: https://github.com/apache/arrow/pull/48285#issuecomment-3589010941 :warning: GitHub issue #44810 **has been automatically assigned in GitHub** to PR creator. -- 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]
[PR] GH-44810: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() [arrow]
hiroyuki-sato opened a new pull request, #48285: URL: https://github.com/apache/arrow/pull/48285 ### Rationale for this change `FileReader::Make` previously returned `Status` and required callers to pass an `out` parameter. ### What changes are included in this PR? Introduce a `Result>` returning API to allow clearer error propagation ### Are these changes tested? Yes. ### Are there any user-facing changes? No. -- 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]
