You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "fenfeng9 (via GitHub)" <gi...@apache.org> on 2026/01/22 07:38:55 UTC
[PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
fenfeng9 opened a new pull request, #48939:
URL: https://github.com/apache/arrow/pull/48939
### Rationale for this change
`FileReader::ReadTable` previously returned `Status` and required callers to pass an `out` parameter.
### What changes are included in this PR?
Introduce `Result<std::shared_ptr<Table>>` returning API to allow clearer error propagation:
- Add new Result-returning `ReadTable()` methods
- Deprecate the old Status versions
- Migrate all callers to use the new API
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes.
Status version FileReader::ReadTable has been deprecated.
```cpp
virtual ::arrow::Status ReadTable(std::shared_ptr<::arrow::Table>* out);
virtual ::arrow::Status ReadTable(const std::vector<int>& column_indices,
std::shared_ptr<::arrow::Table>* 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3790282815
@kou CI passed, 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #48939:
URL: https://github.com/apache/arrow/pull/48939#discussion_r2716541836
##########
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:
https://github.com/apache/arrow/issues/48949
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3789196586
Yes. If you can't do it, I can help 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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3793519087
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 75ef03165dc7e54aca0f842f54598833ddbd0a01.
There was 1 benchmark result with an error:
- Commit Run on `arm64-t4g-2xlarge-linux` at [2026-01-24 00:25:27Z](https://conbench.arrow-dev.org/compare/runs/f87355be278d48dbb8eb657d3cb7eb40...a2372181fd9e43fe833500a3264ecb20/)
- [`dataset-read` (Python) with async=True, dataset=nyctaxi_multi_ipc_s3](https://conbench.arrow-dev.org/benchmark-results/069741166bea7b3180008e69b8ae2420)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/61338187395) has more details. It also includes information about 6 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3783894289
I've created #48949 to track add `arrow::Result` version of `ReadRowGroups()`...
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3789173179
Hi, @kou, I have a question: For R / Python / GLib bindings, can we switch directly to the new API?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #48939:
URL: https://github.com/apache/arrow/pull/48939#discussion_r2715791119
##########
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:
Sure, I'll open a new issue for the ReadRowGroups refactoring. I'll get to the code updates soon.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #48939:
URL: https://github.com/apache/arrow/pull/48939
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3782982134
:warning: GitHub issue #48394 **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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #48939:
URL: https://github.com/apache/arrow/pull/48939#discussion_r2716540664
##########
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:
updated.
##########
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:
updated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3782997351
Hi @kou, Could you please review it? Thanks!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #48939:
URL: https://github.com/apache/arrow/pull/48939#discussion_r2716534496
##########
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:
updated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on code in PR #48939:
URL: https://github.com/apache/arrow/pull/48939#discussion_r2716532766
##########
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:
updated.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() [arrow]
Posted by "fenfeng9 (via GitHub)" <gi...@apache.org>.
fenfeng9 commented on PR #48939:
URL: https://github.com/apache/arrow/pull/48939#issuecomment-3789211590
Thanks! I’ll try it first.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org