kou commented on code in PR #48939:
URL: https://github.com/apache/arrow/pull/48939#discussion_r2715748463
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -2802,7 +2802,7 @@ TEST(TestArrowReadWrite, ReadCoalescedColumnSubset) {
for (std::vector<int>& column_subset : column_subsets) {
std::shared_ptr<Table> result;
- ASSERT_OK(reader->ReadTable(column_subset, &result));
+ ASSERT_OK_AND_ASSIGN(result, reader->ReadTable(column_subset));
Review Comment:
Could you use `ASSERT_OK_AND_ASSIGN(auto result,
reader->ReadTable(column_subset));` here?
##########
cpp/src/parquet/arrow/arrow_reader_writer_test.cc:
##########
@@ -2840,7 +2840,7 @@ TEST(TestArrowReadWrite, ListLargeRecords) {
// Read everything
std::shared_ptr<Table> result;
- ASSERT_OK_NO_THROW(reader->ReadTable(&result));
+ ASSERT_OK_AND_ASSIGN(result, reader->ReadTable());
Review Comment:
Ditto. (There are more similar codes.)
##########
cpp/src/parquet/arrow/reader.cc:
##########
@@ -202,9 +202,12 @@ class FileReaderImpl : public FileReader {
std::shared_ptr<RowGroupReader> RowGroup(int row_group_index) override;
- Status ReadTable(const std::vector<int>& indices,
- std::shared_ptr<Table>* out) override {
- return ReadRowGroups(Iota(reader_->metadata()->num_row_groups()), indices,
out);
+ Result<std::shared_ptr<Table>> ReadTable(
+ const std::vector<int>& column_indices) override {
+ std::shared_ptr<Table> table;
+ RETURN_NOT_OK(ReadRowGroups(Iota(reader_->metadata()->num_row_groups()),
+ column_indices, &table));
Review Comment:
Oh... We need to add `arrow::Result` version of `ReadRowGroups()`...
Let's work on it as a separated task. Could you open an issue for it?
##########
cpp/examples/arrow/parquet_read_write.cc:
##########
@@ -37,8 +37,7 @@ arrow::Status ReadFullFile(std::string path_to_file) {
ARROW_ASSIGN_OR_RAISE(arrow_reader, parquet::arrow::OpenFile(input, pool));
// Read entire file as a single Arrow table
- std::shared_ptr<arrow::Table> table;
- ARROW_RETURN_NOT_OK(arrow_reader->ReadTable(&table));
+ ARROW_RETURN_NOT_OK(arrow_reader->ReadTable().status());
Review Comment:
```suggestion
std::shared_ptr<arrow::Table> table;
ARROW_ASSIGN_OR_RAISE(table, arrow_reader->ReadTable());
```
##########
cpp/src/arrow/filesystem/s3fs_benchmark.cc:
##########
@@ -315,8 +315,8 @@ static void ParquetRead(benchmark::State& st, S3FileSystem*
fs, const std::strin
ASSERT_OK(builder.properties(properties)->Build(&reader));
if (read_strategy == "ReadTable") {
- std::shared_ptr<Table> table;
- ASSERT_OK(reader->ReadTable(column_indices, &table));
+ auto table_result = reader->ReadTable(column_indices);
+ ASSERT_OK(table_result.status());
Review Comment:
```suggestion
ASSERT_OK_AND_ASSIGN(auto table, reader->ReadTable(column_indices));
```
If this reports the "variable is not used" warning, we can use
`ASSERT_OK(reader->ReadTable(column_indices).status());`
##########
cpp/examples/tutorial_examples/file_access_example.cc:
##########
@@ -185,9 +185,8 @@ arrow::Status RunMain() {
// (Doc section: Parquet OpenFile)
// (Doc section: Parquet Read)
- std::shared_ptr<arrow::Table> parquet_table;
// Read the table.
- PARQUET_THROW_NOT_OK(reader->ReadTable(&parquet_table));
+ PARQUET_ASSIGN_OR_THROW(auto parquet_table, reader->ReadTable());
Review Comment:
Could you keep using explicit type instead of `auto` for easy to understand?
--
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]
