GH-44810 [C++][Parquet] Added `arrow::Result` version of `parquet::arrow::FileReader::Make()` #44819

malinjawi · 2024-11-23T18:35:34Z

Rationale for this change

This PR refactors parquet::arrow::FileReader::Make() and related functions to return arrow::Result instead of ::arrow::Status. This modernizes the error handling and improves consistency across the codebase.

What changes are included in this PR?

Replaced ::arrow::Status with arrow::Result<std::unique_ptr<FileReader>> in the Make() functions under CPP Parquet.

Are these changes tested?

All unit tests pass.

Are there any user-facing changes?

No user-facing changes; this is an internal refactor.

GitHub Issue: [C++][Parquet] Add arrow::Result version of parquet::arrow::FileReader::Make() #44810

github-actions · 2024-11-23T18:36:01Z

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

malinjawi · 2024-11-23T18:36:12Z

@github-actions autotune

malinjawi · 2024-11-23T18:43:54Z

hey @wgtmac I am new to arrow and just noticed the failures appearing in the checks. I am not sure if my changes weren't covered in unit testing or not can you please advise?

kou

Could you also update codes that use parquet::arrow::FileReader::Make()?
CI failures tell us where we use it.

kou · 2024-11-24T01:13:25Z

cpp/src/parquet/arrow/reader.h

-  static ::arrow::Status Make(::arrow::MemoryPool* pool,
-                              std::unique_ptr<ParquetFileReader> reader,
-                              const ArrowReaderProperties& properties,
-                              std::unique_ptr<FileReader>* out);


Could you keep the arrow::Status version with deprecated mark so that users will be able to migrate to the arrow::Result version smoothly?
See also: #44785

Got it. Will add comments instead and address the comments. Thanks @kou

+1. We need to mark the existing ones as deprecated without deleting them.

kou · 2024-11-24T01:14:20Z

cpp/src/parquet/arrow/schema.h

 /// between arrow's Schema and parquet's Schema.
 struct PARQUET_EXPORT SchemaManifest {
-  static ::arrow::Status Make(
+  static ::arrow::Result<std::unique_ptr<FileReader>> Make(


Could you work on this as a separated task?

Alright, I have reverted this now.

kou · 2024-11-24T01:14:25Z

cpp/src/parquet/arrow/writer.h

+      MemoryPool* pool, std::unique_ptr<ParquetFileWriter> writer,
+      std::shared_ptr<::arrow::Schema> schema,
+      std::shared_ptr<ArrowWriterProperties> arrow_properties,
+      std::unique_ptr<FileWriter>* out);


Could you work on this as a separated task?

Alright, I have reverted this now

kou · 2024-11-24T01:14:30Z

cpp/src/parquet/bloom_filter.cc

 static constexpr uint32_t kBloomFilterHeaderSizeGuess = 256;

-static ::arrow::Status ValidateBloomFilterHeader(
+static ::arrow::Result<std::unique_ptr<FileReader>> ValidateBloomFilterHeader(


Could you work on this as a separated task?

Reverted. @kou Also, would be beneficial to have an issue with subtasks for this refactoring?

Yes. Could you create subissues?

…ake()" This reverts commit d4ee43f.

raulcd

Thanks for taking an effort!
There are also some linting failures, please do take a look on those once the other things have been reviewed!

raulcd · 2024-11-25T09:18:23Z

cpp/src/parquet/arrow/reader.cc

+Result<std::unique_ptr<FileReader>> Make(::arrow::MemoryPool* pool,
                        std::unique_ptr<ParquetFileReader> reader,
                        const ArrowReaderProperties& properties,
                        std::unique_ptr<FileReader>* out) {


std::unique_ptr<FileReader>* out should be removed as part of the signature of the function. The Result already contains the FileReader value so this is unnecessary. See this commit with a similar change for an example:
0cdbdac

Hey @raulcd updated this but I think i got something wrong on how to use the std::unique_ptr<FileReader>* out internally in the function.

raulcd · 2024-11-25T09:19:10Z

cpp/src/parquet/arrow/reader.cc

 }

-Status FileReader::Make(::arrow::MemoryPool* pool,
+Result<std::unique_ptr<FileReader>> Make(::arrow::MemoryPool* pool,


You still have to provide the implementations for Status FileReader because they are part of the headers file. Even though they are deprecated you can't remove them.

Done! keeping the implementations for Status FileReader

raulcd

There are failures on CI where some usages on other files should be updated to stop using the deprecated API, see for example arrow/cpp/src/arrow/dataset/file_parquet.cc as seen on the failure:

 FAILED: src/arrow/dataset/CMakeFiles/arrow_dataset_objlib.dir/file_parquet.cc.o 
/usr/bin/ccache /usr/lib/ccache/clang++-14 -DADDRESS_SANITIZER -DARROW_DS_EXPORTING -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_UBSAN -DJSON_DIAGNOSTICS=0 -DJSON_USE_IMPLICIT_CONVERSIONS=1 -I/build/cpp/src -I/arrow/cpp/src -I/arrow/cpp/src/generated -I/arrow/cpp/src/parquet -isystem /build/cpp/xsimd_ep/src/xsimd_ep-install/include -isystem /build/cpp/opentelemetry_ep-install/include -Qunused-arguments -fcolor-diagnostics  -Wall -Wextra -Wdocumentation -DARROW_WARN_DOCUMENTATION -Wshorten-64-to-32 -Wno-missing-braces -Wno-unused-parameter -Wno-constant-logical-operand -Wno-return-stack-address -Wdate-time -Wno-unknown-warning-option -Wno-pass-failed -msse4.2  -fsanitize=address -DADDRESS_SANITIZER -fsanitize=undefined -fno-sanitize=alignment,vptr,function,float-divide-by-zero -fno-sanitize-recover=all -fsanitize-coverage=pc-table,inline-8bit-counters,edge,no-prune,trace-cmp,trace-div,trace-gep -fsanitize-blacklist=/arrow/cpp/build-support/sanitizer-disallowed-entries.txt -g -Werror -O0 -ggdb -g1 -fPIC   -fsanitize-coverage=pc-table,inline-8bit-counters,edge,no-prune,trace-cmp,trace-div,trace-gep -std=c++17 -MD -MT src/arrow/dataset/CMakeFiles/arrow_dataset_objlib.dir/file_parquet.cc.o -MF src/arrow/dataset/CMakeFiles/arrow_dataset_objlib.dir/file_parquet.cc.o.d -o src/arrow/dataset/CMakeFiles/arrow_dataset_objlib.dir/file_parquet.cc.o -c /arrow/cpp/src/arrow/dataset/file_parquet.cc
/arrow/cpp/src/arrow/dataset/file_parquet.cc:507:45: error: 'Make' is deprecated: Deprecated in 19.0.0. Use arrow::Result version instead. [-Werror,-Wdeprecated-declarations]
  RETURN_NOT_OK(parquet::arrow::FileReader::Make(

kou · 2024-12-03T01:42:37Z

cpp/src/parquet/arrow/reader.cc

  *out = std::make_unique<FileReaderImpl>(pool, std::move(reader), properties);
  return static_cast<FileReaderImpl*>(out->get())->Init();


Can we use the Result version here?

Suggested change

*out = std::make_unique<FileReaderImpl>(pool, std::move(reader), properties);

return static_cast<FileReaderImpl*>(out->get())->Init();

return Make(pool, std::move(reader), properties)->Value(out);

kou · 2024-12-03T01:45:01Z

cpp/src/parquet/arrow/reader.h

+  // Factory function to create a FileReader from a ParquetFileReader and properties.
+  // \deprecated Deprecated in 19.0.0. Use arrow::Result version instead.


We should use /// not // for Doxygen: https://www.doxygen.nl/manual/docblocks.html

Suggested change

// Factory function to create a FileReader from a ParquetFileReader and properties.

// \deprecated Deprecated in 19.0.0. Use arrow::Result version instead.

/// Factory function to create a FileReader from a ParquetFileReader and properties.

/// \deprecated Deprecated in 19.0.0. Use arrow::Result version instead.

kou · 2024-12-03T01:46:48Z

cpp/src/parquet/arrow/reader.h


-  /// Factory function to create a FileReader from a ParquetFileReader
+  // Factory function to create a FileReader from a ParquetFileReader and properties
+  // Returns an arrow::Result containing a unique pointer to the FileReader.


Could you use \return Doxygen markup?
https://www.doxygen.nl/manual/commands.html#cmdreturn

wgtmac · 2024-12-04T05:18:48Z

Just want to echo the comment from @raulcd that we need to fix all source files not to call the deprecated functions. Perhaps we need to run a full C++ build to make sure they are all covered.

kou · 2025-11-17T02:25:23Z

@malinjawi Do you still want to complete this PR?

added arrow::Result version for parquet::arrow::FileReader::Make()

d4ee43f

malinjawi requested a review from wgtmac as a code owner November 23, 2024 18:35

github-actions bot added Component: Parquet Component: C++ labels Nov 23, 2024

github-actions bot added the awaiting review Awaiting review label Nov 23, 2024

kou reviewed Nov 24, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Nov 24, 2024

Revert "added arrow::Result version for parquet::arrow::FileReader::M…

06881ee

…ake()" This reverts commit d4ee43f.

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 24, 2024

Mohammad Linjawi added 2 commits November 24, 2024 18:01

updated result method on reader.cc

18a5b80

added deprecated flags to Make ::arrow::Status versions

b54b107

raulcd reviewed Nov 25, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Nov 25, 2024

Refactor FileReader::Make methods to align with Result-based API

4687dbe

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 2, 2024

malinjawi requested review from kou and raulcd December 2, 2024 15:56

raulcd reviewed Dec 2, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Dec 2, 2024

kou reviewed Dec 3, 2024

View reviewed changes

		*out = std::make_unique<FileReaderImpl>(pool, std::move(reader), properties);
		return static_cast<FileReaderImpl*>(out->get())->Init();

	*out = std::make_unique<FileReaderImpl>(pool, std::move(reader), properties);
	return static_cast<FileReaderImpl*>(out->get())->Init();
	return Make(pool, std::move(reader), properties)->Value(out);

		// Factory function to create a FileReader from a ParquetFileReader and properties.
		// \deprecated Deprecated in 19.0.0. Use arrow::Result version instead.

GH-44810 [C++][Parquet] Added arrow::Result version of parquet::arrow::FileReader::Make() #44819

Are you sure you want to change the base?

GH-44810 [C++][Parquet] Added arrow::Result version of parquet::arrow::FileReader::Make() #44819

Uh oh!

Conversation

malinjawi commented Nov 23, 2024 • edited by github-actions bot 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!

github-actions bot commented Nov 23, 2024

Uh oh!

malinjawi commented Nov 23, 2024

Uh oh!

malinjawi commented Nov 23, 2024

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!

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!

Choose a reason for hiding this comment

Uh oh!

raulcd 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!

raulcd 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!

wgtmac commented Dec 4, 2024

Uh oh!

kou commented Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-44810 [C++][Parquet] Added `arrow::Result` version of `parquet::arrow::FileReader::Make()` #44819

GH-44810 [C++][Parquet] Added `arrow::Result` version of `parquet::arrow::FileReader::Make()` #44819

malinjawi commented Nov 23, 2024 •

edited by github-actions bot

Loading