[GitHub] [arrow] lidavidm commented on a change in pull request #12669: ARROW-15974: [C++] Migrate flight/types.h header definitions to use Result<>

[GitBox <gi...@apache.org>]

lidavidm commented on a change in pull request #12669:
URL: https://github.com/apache/arrow/pull/12669#discussion_r832544456



##########
File path: cpp/src/arrow/flight/sql/server_test.cc
##########
@@ -184,8 +183,7 @@ class TestFlightSqlServer : public ::testing::Test {
   std::mutex server_ready_m;
 
   void RunServer() {
-    arrow::flight::Location location;
-    ARROW_CHECK_OK(arrow::flight::Location::ForGrpcTcp("localhost", port, &location));
+    Location location = *Location::ForGrpcTcp("localhost", port);

Review comment:
       nit, but this could be `ARROW_CHECK_OK(Location::ForGrpcTcp(...).Value(&location));` still to preserve the previous behavior (that said, it should really be `ASSERT_OK`)

##########
File path: cpp/src/arrow/flight/transport/grpc/grpc_server.cc
##########
@@ -587,9 +589,9 @@ class GrpcServerTransport : public internal::ServerTransport {
     }
 
     if (scheme == kSchemeGrpcTls) {
-      RETURN_NOT_OK(Location::ForGrpcTls(uri.host(), port, &location_));
+      RETURN_NOT_OK(Location::ForGrpcTls(uri.host(), port).Value(&location_));

Review comment:
       `ARROW_ASSIGN_OR_RAISE(location_, ...);`

##########
File path: cpp/src/arrow/flight/sql/test_server_cli.cc
##########
@@ -31,9 +31,8 @@
 DEFINE_int32(port, 31337, "Server port to listen on");
 
 arrow::Status RunMain() {
-  arrow::flight::Location location;
-  ARROW_CHECK_OK(arrow::flight::Location::ForGrpcTcp("0.0.0.0", FLAGS_port, &location));
-  arrow::flight::FlightServerOptions options(location);
+  auto location = arrow::flight::Location::ForGrpcTcp("0.0.0.0", FLAGS_port);

Review comment:
       This function returns Status so this can be `ARROW_ASSIGN_OR_RAISE` (not sure why it was done like that before)

##########
File path: cpp/src/arrow/flight/transport/grpc/grpc_server.cc
##########
@@ -244,12 +244,12 @@ class GrpcServiceHandler final : public FlightService::Service {
     // Write flight info to stream until listing is exhausted
     while (true) {
       ProtoType pb_value;
-      std::unique_ptr<UserType> value;
-      GRPC_RETURN_NOT_OK(iterator->Next(&value));
-      if (!value) {
+      arrow::Result<std::unique_ptr<UserType>> value = iterator->Next();
+      GRPC_RETURN_NOT_OK(value.status());

Review comment:
       In general I'd rather we not declare any variables of type `Result<>` and instead stick closer to the prior style, e.g. `GRPC_RETURN_NOT_OK(iterator->Next().Value(&value));`

##########
File path: cpp/src/arrow/flight/test_definitions.cc
##########
@@ -142,10 +142,10 @@ void DataTest::CheckDoGet(
   ASSERT_OK(client_->GetFlightInfo(descr, &info));
   check_endpoints(info->endpoints());
 
-  std::shared_ptr<Schema> schema;
   ipc::DictionaryMemo dict_memo;
-  ASSERT_OK(info->GetSchema(&dict_memo, &schema));
-  AssertSchemaEqual(*expected_schema, *schema);
+  arrow::Result<std::shared_ptr<Schema>> schema = info->GetSchema(&dict_memo);

Review comment:
       use `ASSERT_OK_AND_ASSIGN`




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org