From 1a292d65dcf17cd0cdf4b0c67db3a7385e40b7b7 Mon Sep 17 00:00:00 2001 From: Felipe Oliveira Carvalho Date: Fri, 12 Jul 2024 20:52:11 -0300 Subject: [PATCH] flight: Don't wrap a FlightInfo in an arrow::Result<> in FromProto To make it more consistent with the rest of serialization code. This reduces code size in ~4KiB. --- cpp/src/arrow/flight/flight_internals_test.cc | 5 +-- .../arrow/flight/serialization_internal.cc | 31 ++++++++++--------- cpp/src/arrow/flight/serialization_internal.h | 2 +- .../flight/transport/grpc/grpc_client.cc | 12 ++++--- cpp/src/arrow/flight/types.cc | 7 +++-- 5 files changed, 31 insertions(+), 26 deletions(-) diff --git a/cpp/src/arrow/flight/flight_internals_test.cc b/cpp/src/arrow/flight/flight_internals_test.cc index 23e032d834ba1..caab357ef8f4a 100644 --- a/cpp/src/arrow/flight/flight_internals_test.cc +++ b/cpp/src/arrow/flight/flight_internals_test.cc @@ -79,8 +79,9 @@ void TestRoundtrip(const std::vector& values, ASSERT_OK(internal::ToProto(values[i], &pb_value)); if constexpr (std::is_same_v) { - ASSERT_OK_AND_ASSIGN(FlightInfo value, internal::FromProto(pb_value)); - EXPECT_EQ(values[i], value); + FlightInfo::Data info_data; + ASSERT_OK(internal::FromProto(pb_value, &info_data)); + EXPECT_EQ(values[i], FlightInfo{std::move(info_data)}); } else if constexpr (std::is_same_v) { std::string data; ASSERT_OK(internal::FromProto(pb_value, &data)); diff --git a/cpp/src/arrow/flight/serialization_internal.cc b/cpp/src/arrow/flight/serialization_internal.cc index 10600d055b3a8..2f419462ed804 100644 --- a/cpp/src/arrow/flight/serialization_internal.cc +++ b/cpp/src/arrow/flight/serialization_internal.cc @@ -251,22 +251,21 @@ Status ToProto(const FlightDescriptor& descriptor, pb::FlightDescriptor* pb_desc // FlightInfo -arrow::Result FromProto(const pb::FlightInfo& pb_info) { - FlightInfo::Data info; - RETURN_NOT_OK(FromProto(pb_info.flight_descriptor(), &info.descriptor)); +Status FromProto(const pb::FlightInfo& pb_info, FlightInfo::Data* info) { + RETURN_NOT_OK(FromProto(pb_info.flight_descriptor(), &info->descriptor)); - info.schema = pb_info.schema(); + info->schema = pb_info.schema(); - info.endpoints.resize(pb_info.endpoint_size()); + info->endpoints.resize(pb_info.endpoint_size()); for (int i = 0; i < pb_info.endpoint_size(); ++i) { - RETURN_NOT_OK(FromProto(pb_info.endpoint(i), &info.endpoints[i])); + RETURN_NOT_OK(FromProto(pb_info.endpoint(i), &info->endpoints[i])); } - info.total_records = pb_info.total_records(); - info.total_bytes = pb_info.total_bytes(); - info.ordered = pb_info.ordered(); - info.app_metadata = pb_info.app_metadata(); - return FlightInfo(std::move(info)); + info->total_records = pb_info.total_records(); + info->total_bytes = pb_info.total_bytes(); + info->ordered = pb_info.ordered(); + info->app_metadata = pb_info.app_metadata(); + return Status::OK(); } Status FromProto(const pb::BasicAuth& pb_basic_auth, BasicAuth* basic_auth) { @@ -315,8 +314,9 @@ Status ToProto(const FlightInfo& info, pb::FlightInfo* pb_info) { Status FromProto(const pb::PollInfo& pb_info, PollInfo* info) { if (pb_info.has_info()) { - ARROW_ASSIGN_OR_RAISE(auto flight_info, FromProto(pb_info.info())); - info->info = std::make_unique(std::move(flight_info)); + FlightInfo::Data info_data; + RETURN_NOT_OK(FromProto(pb_info.info(), &info_data)); + info->info = std::make_unique(std::move(info_data)); } if (pb_info.has_flight_descriptor()) { FlightDescriptor descriptor; @@ -360,8 +360,9 @@ Status ToProto(const PollInfo& info, pb::PollInfo* pb_info) { Status FromProto(const pb::CancelFlightInfoRequest& pb_request, CancelFlightInfoRequest* request) { - ARROW_ASSIGN_OR_RAISE(FlightInfo info, FromProto(pb_request.info())); - request->info = std::make_unique(std::move(info)); + FlightInfo::Data info_data; + RETURN_NOT_OK(FromProto(pb_request.info(), &info_data)); + request->info = std::make_unique(std::move(info_data)); return Status::OK(); } diff --git a/cpp/src/arrow/flight/serialization_internal.h b/cpp/src/arrow/flight/serialization_internal.h index 90dde87d3a5eb..cdb8ca5cf7842 100644 --- a/cpp/src/arrow/flight/serialization_internal.h +++ b/cpp/src/arrow/flight/serialization_internal.h @@ -60,7 +60,7 @@ Status FromProto(const pb::FlightDescriptor& pb_descr, FlightDescriptor* descr); Status FromProto(const pb::FlightEndpoint& pb_endpoint, FlightEndpoint* endpoint); Status FromProto(const pb::RenewFlightEndpointRequest& pb_request, RenewFlightEndpointRequest* request); -arrow::Result FromProto(const pb::FlightInfo& pb_info); +Status FromProto(const pb::FlightInfo& pb_info, FlightInfo::Data* info); Status FromProto(const pb::PollInfo& pb_info, PollInfo* info); Status FromProto(const pb::CancelFlightInfoRequest& pb_request, CancelFlightInfoRequest* request); diff --git a/cpp/src/arrow/flight/transport/grpc/grpc_client.cc b/cpp/src/arrow/flight/transport/grpc/grpc_client.cc index f799ba761c40d..6d8d40c2ebcf8 100644 --- a/cpp/src/arrow/flight/transport/grpc/grpc_client.cc +++ b/cpp/src/arrow/flight/transport/grpc/grpc_client.cc @@ -648,10 +648,10 @@ class UnaryUnaryAsyncCall : public ::grpc::ClientUnaryReactor, public internal:: void OnDone(const ::grpc::Status& status) override { if (status.ok()) { - auto result = internal::FromProto(pb_response); - client_status = result.status(); + FlightInfo::Data info_data; + client_status = internal::FromProto(pb_response, &info_data); if (client_status.ok()) { - listener->OnNext(std::move(result).MoveValueUnsafe()); + listener->OnNext(FlightInfo{std::move(info_data)}); } } Finish(status); @@ -889,7 +889,8 @@ class GrpcClientImpl : public internal::ClientTransport { pb::FlightInfo pb_info; while (!options.stop_token.IsStopRequested() && stream->Read(&pb_info)) { - ARROW_ASSIGN_OR_RAISE(FlightInfo info_data, internal::FromProto(pb_info)); + FlightInfo::Data info_data; + RETURN_NOT_OK(internal::FromProto(pb_info, &info_data)); flights.emplace_back(std::move(info_data)); } if (options.stop_token.IsStopRequested()) rpc.context.TryCancel(); @@ -939,7 +940,8 @@ class GrpcClientImpl : public internal::ClientTransport { stub_->GetFlightInfo(&rpc.context, pb_descriptor, &pb_response), &rpc.context); RETURN_NOT_OK(s); - ARROW_ASSIGN_OR_RAISE(auto info_data, internal::FromProto(pb_response)); + FlightInfo::Data info_data; + RETURN_NOT_OK(internal::FromProto(pb_response, &info_data)); *info = std::make_unique(std::move(info_data)); return Status::OK(); } diff --git a/cpp/src/arrow/flight/types.cc b/cpp/src/arrow/flight/types.cc index d8a6cbd0da63e..5840389c72271 100644 --- a/cpp/src/arrow/flight/types.cc +++ b/cpp/src/arrow/flight/types.cc @@ -277,7 +277,7 @@ arrow::Result FlightInfo::Make(const Schema& schema, data.ordered = ordered; data.app_metadata = std::move(app_metadata); RETURN_NOT_OK(internal::SchemaToString(schema, &data.schema)); - return FlightInfo(data); + return FlightInfo(std::move(data)); } arrow::Result> FlightInfo::GetSchema( @@ -300,8 +300,9 @@ arrow::Result> FlightInfo::Deserialize( std::string_view serialized) { pb::FlightInfo pb_info; RETURN_NOT_OK(ParseFromString("FlightInfo", serialized, &pb_info)); - ARROW_ASSIGN_OR_RAISE(FlightInfo info, internal::FromProto(pb_info)); - return std::make_unique(std::move(info)); + FlightInfo::Data info_data; + RETURN_NOT_OK(internal::FromProto(pb_info, &info_data)); + return std::make_unique(std::move(info_data)); } std::string FlightInfo::ToString() const {