lidavidm commented on a change in pull request #12669:
URL: https://github.com/apache/arrow/pull/12669#discussion_r835269041
##########
File path: cpp/src/arrow/flight/client.h
##########
@@ -253,13 +264,22 @@ class ARROW_FLIGHT_EXPORT FlightClient {
/// \param[in] options Per-RPC options
/// \param[in] descriptor the dataset request, whether a named dataset or
/// command
- /// \param[out] schema_result the SchemaResult describing the dataset schema
- /// \return Status
+ /// \return Arrow result with the SchemaResult describing the dataset schema
+ arrow::Result<std::unique_ptr<SchemaResult>> GetSchema(
Review comment:
FYI, it would have been fine to delay these changes for another PR, but
I appreciate it!
##########
File path: cpp/src/arrow/flight/server.h
##########
@@ -237,11 +244,13 @@ class ARROW_FLIGHT_EXPORT FlightServerBase {
/// \brief Retrieve the schema for the indicated descriptor
/// \param[in] context The call context.
/// \param[in] request may be null
- /// \param[out] schema the returned flight schema provider
- /// \return Status
- virtual Status GetSchema(const ServerCallContext& context,
- const FlightDescriptor& request,
- std::unique_ptr<SchemaResult>* schema);
+ /// \return Arrow result with the returned flight schema provider
+ virtual arrow::Result<std::unique_ptr<SchemaResult>> GetSchema(
Review comment:
Why are we changing RPC handler signatures? IMO, let's leave these as-is
for now. I'm not sure there's any real benefit in migrating them.
##########
File path: cpp/src/arrow/flight/transport/grpc/grpc_client.cc
##########
@@ -816,8 +817,7 @@ class GrpcClientImpl : public internal::ClientTransport {
std::string str;
RETURN_NOT_OK(internal::FromProto(pb_response, &str));
- schema_result->reset(new SchemaResult(str));
- return Status::OK();
+ return arrow::internal::make_unique<SchemaResult>(str);
Review comment:
nit: `std::move(str)` though I guess that wasn't done originally either
##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -1367,7 +1362,7 @@ void CudaDataTest::TestDoExchange() {
ASSERT_OK(writer->WriteRecordBatch(*cuda_batch));
FlightStreamChunk chunk;
- ASSERT_OK(reader->Next(&chunk));
+ ASSERT_OK_AND_ASSIGN(chunk, reader->Next());
Review comment:
nit, but if we're going to change these, then just combine the
declaration and assignment
--
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]
