Skip to content

GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() #48939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
fenfeng9 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

GH-48394: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::ReadTable() #48939

fenfeng9 wants to merge 4 commits into from
+81 −81

Conversation

@fenfeng9
Copy link

@fenfeng9 fenfeng9 commented Jan 22, 2026 •
edited by github-actions bot
Loading

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.

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);

@fenfeng9 fenfeng9 requested a review from wgtmac as a code owner January 22, 2026 07:38
@github-actions
Copy link

github-actions bot commented Jan 22, 2026

⚠️ GitHub issue #48394 has been automatically assigned in GitHub to PR creator.

@fenfeng9
Copy link
Author

fenfeng9 commented Jan 22, 2026

Hi @kou, Could you please review it? Thanks!

kou
kou reviewed Jan 22, 2026
View reviewed changes
cpp/examples/tutorial_examples/file_access_example.cc Outdated
Comment on lines 188 to 189
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());
Copy link
Member

@kou kou Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you keep using explicit type instead of auto for easy to understand?

Copy link
Author

@fenfeng9 fenfeng9 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

cpp/src/arrow/filesystem/s3fs_benchmark.cc Outdated
Comment on lines 318 to 319
auto table_result = reader->ReadTable(column_indices);
ASSERT_OK(table_result.status());
Copy link
Member

@kou kou Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto table_result = reader->ReadTable(column_indices);
ASSERT_OK(table_result.status());
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());

Copy link
Author

@fenfeng9 fenfeng9 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated
Comment on lines 2804 to 2805
std::shared_ptr<Table> result;
ASSERT_OK(reader->ReadTable(column_subset, &result));
ASSERT_OK_AND_ASSIGN(result, reader->ReadTable(column_subset));
Copy link
Member

@kou kou Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use ASSERT_OK_AND_ASSIGN(auto result, reader->ReadTable(column_subset)); here?

Copy link
Author

@fenfeng9 fenfeng9 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated
Comment on lines 2842 to 2843
std::shared_ptr<Table> result;
ASSERT_OK_NO_THROW(reader->ReadTable(&result));
ASSERT_OK_AND_ASSIGN(result, reader->ReadTable());
Copy link
Member

@kou kou Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. (There are more similar codes.)

Copy link
Author

@fenfeng9 fenfeng9 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

cpp/src/parquet/arrow/reader.cc
Comment on lines +208 to +209
RETURN_NOT_OK(ReadRowGroups(Iota(reader_->metadata()->num_row_groups()),
column_indices, &table));
Copy link
Member

@kou kou Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@fenfeng9 fenfeng9 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll open a new issue for the ReadRowGroups refactoring. I'll get to the code updates soon.

Copy link
Author

@fenfeng9 fenfeng9 Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#48949

@fenfeng9 fenfeng9 force-pushed the refactor/parquet-readtable-result branch from afe13a4 to cf88ec1 Compare January 22, 2026 10:57
@fenfeng9
Copy link
Author

fenfeng9 commented Jan 22, 2026

I've created #48949 to track add arrow::Result version of ReadRowGroups()...

@fenfeng9 fenfeng9 requested a review from kou January 22, 2026 11:38
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 23, 2026
@fenfeng9
Copy link
Author

fenfeng9 commented Jan 23, 2026

Hi, @kou, I have a question: For R / Python / GLib bindings, can we switch directly to the new API?

@kou
Copy link
Member

kou commented Jan 23, 2026

Yes. If you can't do it, I can help it.

@fenfeng9
Copy link
Author

fenfeng9 commented Jan 23, 2026

Thanks! I’ll try it first.

@fenfeng9 fenfeng9 force-pushed the refactor/parquet-readtable-result branch from cf88ec1 to 6ae9eb5 Compare January 23, 2026 12:28
@fenfeng9
Copy link
Author

fenfeng9 commented Jan 23, 2026

@kou CI passed, Please take a look when you get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wgtmac wgtmac wgtmac approved these changes

@kou kou Awaiting requested review from kou kou is a code owner

@jonkeane jonkeane Awaiting requested review from jonkeane jonkeane is a code owner

@thisisnic thisisnic Awaiting requested review from thisisnic thisisnic is a code owner

@AlenkaF AlenkaF Awaiting requested review from AlenkaF AlenkaF is a code owner

@raulcd raulcd Awaiting requested review from raulcd raulcd is a code owner

@rok rok Awaiting requested review from rok rok is a code owner

Assignees

No one assigned

Labels

awaiting committer review Awaiting committer review Component: C++ Component: GLib Component: Parquet Component: Python Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fenfeng9 @kou @wgtmac