pitrou commented on a change in pull request #11065:
URL: https://github.com/apache/arrow/pull/11065#discussion_r701060614
##########
File path: cpp/src/arrow/adapters/orc/adapter.h
##########
@@ -56,55 +66,114 @@ class ARROW_EXPORT ORCFileReader {
/// \brief Return the schema read from the ORC file
///
/// \param[out] out the returned Schema object
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status ReadSchema(std::shared_ptr<Schema>* out);
+ /// \brief Return the schema read from the ORC file
+ ///
+ /// \return the returned Schema object
+ Result<std::shared_ptr<Schema>> ReadSchema();
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(std::shared_ptr<Table>* out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read();
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[in] schema the Table schema
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(const std::shared_ptr<Schema>& schema, std::shared_ptr<Table>*
out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \param[in] schema the Table schema
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read(const std::shared_ptr<Schema>& schema);
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[in] include_indices the selected field indices to read
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(const std::vector<int>& include_indices, std::shared_ptr<Table>*
out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \param[in] include_indices the selected field indices to read
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read(std::vector<int>& include_indices);
Review comment:
Should `const std::vector<int>&`
##########
File path: cpp/src/arrow/adapters/orc/adapter.h
##########
@@ -56,55 +66,114 @@ class ARROW_EXPORT ORCFileReader {
/// \brief Return the schema read from the ORC file
///
/// \param[out] out the returned Schema object
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status ReadSchema(std::shared_ptr<Schema>* out);
+ /// \brief Return the schema read from the ORC file
+ ///
+ /// \return the returned Schema object
+ Result<std::shared_ptr<Schema>> ReadSchema();
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(std::shared_ptr<Table>* out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read();
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[in] schema the Table schema
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(const std::shared_ptr<Schema>& schema, std::shared_ptr<Table>*
out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \param[in] schema the Table schema
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read(const std::shared_ptr<Schema>& schema);
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[in] include_indices the selected field indices to read
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(const std::vector<int>& include_indices, std::shared_ptr<Table>*
out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \param[in] include_indices the selected field indices to read
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read(std::vector<int>& include_indices);
+
/// \brief Read the file as a Table
///
/// The table will be composed of one record batch per stripe.
///
/// \param[in] schema the Table schema
/// \param[in] include_indices the selected field indices to read
/// \param[out] out the returned Table
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status Read(const std::shared_ptr<Schema>& schema,
const std::vector<int>& include_indices, std::shared_ptr<Table>*
out);
+ /// \brief Read the file as a Table
+ ///
+ /// The table will be composed of one record batch per stripe.
+ ///
+ /// \param[in] schema the Table schema
+ /// \param[in] include_indices the selected field indices to read
+ /// \return the returned Table
+ Result<std::shared_ptr<Table>> Read(const std::shared_ptr<Schema>& schema,
+ const std::vector<int>& include_indices);
+
/// \brief Read a single stripe as a RecordBatch
///
/// \param[in] stripe the stripe index
/// \param[out] out the returned RecordBatch
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Status ReadStripe(int64_t stripe, std::shared_ptr<RecordBatch>* out);
+ /// \brief Read a single stripe as a RecordBatch
+ ///
+ /// \param[in] stripe the stripe index
+ /// \return the returned RecordBatch
+ Result<std::shared_ptr<RecordBatch>> ReadStripe(int stripe);
Review comment:
Is there a reason to pass the stripe index as `int` instead of
`int64_t`? If not, le's remain consistent.
##########
File path: cpp/src/arrow/adapters/orc/adapter.cc
##########
@@ -436,6 +436,13 @@ Status ORCFileReader::Open(const
std::shared_ptr<io::RandomAccessFile>& file,
return Status::OK();
}
+Result<std::unique_ptr<ORCFileReader>> ORCFileReader::Open(
+ const std::shared_ptr<io::RandomAccessFile>& file, MemoryPool* pool) {
+ auto result = std::unique_ptr<ORCFileReader>(new ORCFileReader());
+ RETURN_NOT_OK(result->impl_->Open(file, pool));
+ return std::move(result);
+}
+
Review comment:
Now, the deprecated overloads can be rewritten to call the
non-deprecated ones:
```c++
Status ORCFileReader::Open(const std::shared_ptr<io::RandomAccessFile>& file,
MemoryPool* pool, std::unique_ptr<ORCFileReader>*
reader) {
return Open(file, pool).Value(&reader);
}
```
##########
File path: cpp/src/arrow/adapters/orc/adapter.h
##########
@@ -56,55 +66,114 @@ class ARROW_EXPORT ORCFileReader {
/// \brief Return the schema read from the ORC file
///
/// \param[out] out the returned Schema object
+ ARROW_DEPRECATED("Deprecated in 6.0.0.")
Review comment:
This is good, but it would be better to make the message a bit more
informative. How about:
```c++
ARROW_DEPRECATED("Deprecated in 6.0.0. Use Result-returning overload
instead.")
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
