GH-46869: [C++][Parquet] Deprecate `arrow::Status parquet::arrow::FileReadeder::GetRecordBatchReader()` by Ziy1-Tan · Pull Request #46932 · apache/arrow

Ziy1-Tan · 2025-06-29T08:44:28Z

Rationale for this change

We're migrating arrow::Status + output variable API to arrow::Result API.

What changes are included in this PR?

Deprecate arrow::Status parquet::arrow::FileReadeder::GetRecordBatchReader()
Users can convert the unique_ptr to shared_ptr if they want

Are these changes tested?

Yes.

Are there any user-facing changes?

No. But arrow::Status parquet::arrow::FileReadeder::GetRecordBatchReader() are deprecated.

GitHub Issue: [C++][Parquet] Deprecate Status versions of parquet:arrow::FileReader::GetRecordBatchReader() #46869

…r::GetRecordBatchReader() Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>

wgtmac · 2025-06-29T14:12:37Z

It would be better to change all test cases to use new non-deprecated apis.

Ziy1-Tan · 2025-06-29T15:08:31Z

It would be better to change all test cases to use new non-deprecated apis.

It is only used by example before:

arrow/cpp/examples/arrow/parquet_read_write.cc

Lines 69 to 70 in ed13ced

 std::shared_ptr<::arrow::RecordBatchReader> rb_reader; 
ARROW_ASSIGN_OR_RAISE(rb_reader, arrow_reader->GetRecordBatchReader()); 

wgtmac

cc @pitrou @mapleFU

wgtmac · 2025-06-30T01:18:11Z

 /// \returns error Status if either row_group_indices or column_indices
 /// contains an invalid index
+ /// \deprecated Deprecated in future release. Use arrow::Result version instead.
+ ARROW_DEPRECATED("Deprecated in future release. Use arrow::Result version instead.")


Can we specify a clear deprecated version? It should be 21.0.0 for now. (Note that there will be a feature freeze on July 1st)

wgtmac · 2025-06-30T01:18:51Z

+ /// \returns error Result if either row_group_indices or column_indices
+ /// contains an invalid index
+ virtual ::arrow::Result<std::shared_ptr<::arrow::RecordBatchReader>>
+ GetRecordBatchReaderSharedPtr(const std::vector<int>& row_group_indices,


Suggested change
GetRecordBatchReaderSharedPtr(conststd::vector<int>&row_group_indices,
GetRecordBatchReader(conststd::vector<int>&row_group_indices,
Can we avoid adding SharedPtr to be consistent?

GetRecordBatchReader is used by Result<std::unique_ptr<RecordBatchReader>> GetRecordBatchReader now. @wgtmac, Do you have better nameing of this? Or remove shared_ptr version completely.

Ah, then we don't need this new method as users can convert the unique_ptr to shared_ptr if they want.

I agree with @pitrou that these new functions are essentially duplicate.

kou

Could you update the PR description?

kou · 2025-07-02T01:02:22Z

Could you keep them? Users who still use old API use them.

Add these back again.

wgtmac · 2025-07-03T09:39:30Z

We don't need to change this, right?

wgtmac · 2025-07-03T09:40:11Z

It should be 21.0.0 (if we can merge this before the code freeze; otherwise 22.0.0)

Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>

kou

+1

mapleFU · 2025-07-04T01:43:20Z

I have same issue here: #46932 (comment) . would this in 21.0 or be 22.0?

kou · 2025-07-04T01:57:59Z

We don't have a release branch for 21.0.0 yet. So let's merge this for 21.0.0.

conbench-apache-arrow · 2025-07-04T07:48:38Z

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

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

[C++][Parquet] Add arrow::Result version of parquet::arrow::FileReade…
5a77784
…r::GetRecordBatchReader() Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>

Ziy1-Tan requested a review from wgtmac as a code owner June 29, 2025 08:44

Ziy1-Tan changed the title ~~GH-46869: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::GetRecordBatchReader()~~GH-46869: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::GetRecordBatchReader()Jun 29, 2025

github-actionsBot added the awaiting review Awaiting review label Jun 29, 2025

wgtmac requested changes Jun 30, 2025
View reviewed changes

github-actionsBot added awaiting committer review Awaiting committer review awaiting review Awaiting review Component: Parquet Component: C++ awaiting changes Awaiting changes and removed awaiting review Awaiting review awaiting committer review Awaiting committer review labels Jun 30, 2025

kou reviewed Jul 2, 2025
View reviewed changes

Ziy1-Tan force-pushed the main branch from b1692ad to 1891160Compare July 2, 2025 12:54

Ziy1-Tan changed the title ~~GH-46869: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::GetRecordBatchReader()~~GH-46869: [C++][Parquet] Deprecate arrow::Status parquet::arrow::FileReadeder::GetRecordBatchReader()Jul 2, 2025

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 2, 2025

Ziy1-Tan changed the title ~~GH-46869: [C++][Parquet] Deprecate arrow::Status parquet::arrow::FileReadeder::GetRecordBatchReader()~~GH-46869: [C++][Parquet] Deprecate arrow::Status parquet::arrow::FileReadeder::GetRecordBatchReader()Jul 2, 2025

wgtmac reviewed Jul 3, 2025
View reviewed changes

Deprecate shared_ptr version
f285b94
Signed-off-by: Ziy1-Tan <ajb459684460@gmail.com>

Ziy1-Tan force-pushed the main branch from 1891160 to f285b94Compare July 3, 2025 14:18

kou approved these changes Jul 3, 2025
View reviewed changes

github-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jul 3, 2025

wgtmac approved these changes Jul 4, 2025
View reviewed changes

kou merged commit f6ea922 into apache:mainJul 4, 2025

kou removed the awaiting merge Awaiting merge label Jul 4, 2025

kou mentioned this pull request Jul 4, 2025
[C++][Parquet] Deprecate Status versions of parquet:arrow::FileReader::GetRecordBatchReader()#46869
Closed

HuaHuaY mentioned this pull request Jul 31, 2026
[C++][Parquet] Remove code marked as deprecated except flight in versions 23.0.0 and earlier #50750
Closed

	GetRecordBatchReaderSharedPtr(conststd::vector<int>&row_group_indices,
	GetRecordBatchReader(conststd::vector<int>&row_group_indices,

Uh oh!

Conversation

Ziy1-Tan commented Jun 29, 2025• edited by kou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

wgtmac commented Jun 29, 2025

Uh oh!

Ziy1-Tan commented Jun 29, 2025

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ziy1-TanJul 1, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

mapleFU commented Jul 4, 2025

Uh oh!

kou commented Jul 4, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conbench-apache-arrowBot commented Jul 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ziy1-Tan commented Jun 29, 2025•
edited by kou
Loading

Ziy1-TanJul 1, 2025•
edited
Loading

kou commented Jul 4, 2025•
edited
Loading