Skip to content

GH-48949: [C++][Parquet] Add Result versions for parquet::arrow::FileReader::ReadRowGroup(s) #48982

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
Merged
kou merged 6 commits into from
Jan 27, 2026
Merged

GH-48949: [C++][Parquet] Add Result versions for parquet::arrow::FileReader::ReadRowGroup(s) #48982

kou merged 6 commits into from
Jan 27, 2026
+114 −90

Conversation

@fenfeng9
Copy link
Contributor

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

Rationale for this change

FileReader::ReadRowGroup(s) 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 APIs to allow clearer error propagation:

  • Add new Result-returning ReadRowGroup() / ReadRowGroups() methods
  • Deprecate the old Status/out-parameter overloads
  • Update C++ callers and R/Python/GLib bindings to use the new API

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.
Status versions of FileReader::ReadRowGroup(s) have been deprecated.

virtual ::arrow::Status ReadRowGroup(int i, const std::vector<int>& column_indices,
                                     std::shared_ptr<::arrow::Table>* out);
virtual ::arrow::Status ReadRowGroup(int i, std::shared_ptr<::arrow::Table>* out);

virtual ::arrow::Status ReadRowGroups(const std::vector<int>& row_groups,
                                      const std::vector<int>& column_indices,
                                      std::shared_ptr<::arrow::Table>* out);
virtual ::arrow::Status ReadRowGroups(const std::vector<int>& row_groups,
                                      std::shared_ptr<::arrow::Table>* out);

@github-actions
Copy link

github-actions bot commented Jan 26, 2026

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

@fenfeng9
Copy link
Contributor Author

fenfeng9 commented Jan 26, 2026

@kou, the PR is ready for review.

wgtmac
wgtmac reviewed Jan 27, 2026
View reviewed changes
cpp/src/parquet/arrow/arrow_reader_writer_test.cc
// Read everything
ASSERT_OK_NO_THROW(reader->ReadRowGroup(0, &r1));
ASSERT_OK_AND_ASSIGN(r1, reader->ReadRowGroup(0));
ASSERT_OK_NO_THROW(reader->RowGroup(1)->ReadTable(&r2));
Copy link
Member

@wgtmac wgtmac Jan 27, 2026

Choose a reason for hiding this comment

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

Do we have a Result version for this ReadTable call?

Copy link
Contributor Author

@fenfeng9 fenfeng9 Jan 27, 2026

Choose a reason for hiding this comment

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

This call is on a RowGroupReader, which still only has the Status + out-parameter API, so I left it unchanged.

Copy link
Member

@kou kou Jan 27, 2026

Choose a reason for hiding this comment

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

Let's work on it as a separated task.

cpp/src/parquet/arrow/arrow_reader_writer_test.cc Outdated
std::shared_ptr<Table> r1, r2, r3, r4;
// Read everything
ASSERT_OK_NO_THROW(reader->ReadRowGroup(0, &r1));
ASSERT_OK_AND_ASSIGN(r1, reader->ReadRowGroup(0));
Copy link
Member

@wgtmac wgtmac Jan 27, 2026

Choose a reason for hiding this comment

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

Suggested change
ASSERT_OK_AND_ASSIGN(r1, reader->ReadRowGroup(0));
ASSERT_OK_AND_ASSIGN(auto r1, reader->ReadRowGroup(0));

We have the chance to remove line 2454

Copy link
Contributor Author

@fenfeng9 fenfeng9 Jan 27, 2026

Choose a reason for hiding this comment

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

Updated: r1/r3/r4 use ASSERT_OK_AND_ASSIGN(auto ...), and r2 stays since RowGroupReader::ReadTable is still Status+out.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 27, 2026
@fenfeng9 fenfeng9 force-pushed the refactor/parquet-read-row-group-result branch from 300da57 to 3025c60 Compare January 27, 2026 07:18
@fenfeng9 fenfeng9 requested a review from wgtmac January 27, 2026 08:15
kou
kou approved these changes Jan 27, 2026
View reviewed changes
Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

cpp/src/parquet/arrow/arrow_reader_writer_test.cc
// Read everything
ASSERT_OK_NO_THROW(reader->ReadRowGroup(0, &r1));
ASSERT_OK_AND_ASSIGN(r1, reader->ReadRowGroup(0));
ASSERT_OK_NO_THROW(reader->RowGroup(1)->ReadTable(&r2));
Copy link
Member

@kou kou Jan 27, 2026

Choose a reason for hiding this comment

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

Let's work on it as a separated task.

@github-actions github-actions bot added awaiting merge Awaiting merge awaiting review Awaiting review and removed awaiting committer review Awaiting committer review labels Jan 27, 2026
@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review awaiting merge Awaiting merge labels Jan 27, 2026
@fenfeng9 @kou
Update cpp/src/parquet/arrow/fuzz_internal.cc
b250c0b 
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
@fenfeng9
Copy link
Contributor Author

fenfeng9 commented Jan 27, 2026

I’ve opened a follow‑up issue for RowGroupReader Result APIs: #49025.

@kou kou merged commit eb1525e into apache:main Jan 27, 2026
45 of 47 checks passed
@kou kou removed the awaiting committer review Awaiting committer review label Jan 27, 2026
@kou kou mentioned this pull request Jan 27, 2026
Closed
@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Jan 27, 2026

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit eb1525e.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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

Reviewers

@kou kou kou approved these changes

@wgtmac wgtmac wgtmac approved these changes

@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

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