feat: adds efficient move support to Value::get<string>() (#980)

Fixes #422

Changes Value::get<T>() to efficiently move std::string values out
of the proto and directly to the caller. std::string is the only type
that can do this because it's the only type which is exactly the same in
the proto and outside of it. All other types require some type of
decoding, which necessarily requires some amount of copying from the
proto out to the caller.

Note: the unit tests verifying this behavior rely on unspecified behavior of std::string. Specifically that a moved-from large string ends up empty. This is not guaranteed, but it appears to the true in all cases that I've seen. So we rely on this behavior in the unit test. If this turns out to not be true on some platform, we'll have to adjust the unit tests. I've commented the tests about this.

google/cloud/spanner/value.cc

@@ -287,6 +287,15 @@ StatusOr<std::string> Value::GetValue(std::string const&,
   return pv.string_value();
 }
 
+StatusOr<std::string> Value::GetValue(std::string const&,
+                                      google::protobuf::Value&& pv,
+                                      google::spanner::v1::Type const&) {
+  if (pv.kind_case() != google::protobuf::Value::kStringValue) {
+    return Status(StatusCode::kUnknown, "missing STRING");
+  }
+  return std::move(*pv.mutable_string_value());
+}
+
 StatusOr<Bytes> Value::GetValue(Bytes const&, google::protobuf::Value const& pv,
                                 google::spanner::v1::Type const&) {
   if (pv.kind_case() != google::protobuf::Value::kStringValue) {

google/cloud/spanner/value.h

@@ -275,7 +275,7 @@ class Value {
    * @endcode
    */
   template <typename T>
-  StatusOr<T> get() const {
+  StatusOr<T> get() const& {
     // Ignores the name field because it is never set on the incoming `T`.
     if (!EqualTypeProtoIgnoringNames(type_, MakeTypeProto(T{})))
       return Status(StatusCode::kUnknown, "wrong type");
@@ -286,6 +286,19 @@ class Value {
     return GetValue(T{}, value_, type_);
   }
 
+  /// @copydoc get()
+  template <typename T>
+  StatusOr<T> get() && {
+    // Ignores the name field because it is never set on the incoming `T`.
+    if (!EqualTypeProtoIgnoringNames(type_, MakeTypeProto(T{})))
+      return Status(StatusCode::kUnknown, "wrong type");
+    if (value_.kind_case() == google::protobuf::Value::kNullValue) {
+      if (is_optional<T>::value) return T{};
+      return Status(StatusCode::kUnknown, "null value");
+    }
+    return GetValue(T{}, std::move(value_), type_);
+  }
+
   /**
    * Allows Google Test to print internal debugging information when test
    * assertions fail.
@@ -417,7 +430,7 @@ class Value {
   };
 
   // Tag-dispatch overloads to extract a C++ value from a `Value` protobuf. The
-  // first argument type is the tag, the first argument value is ignored.
+  // first argument type is the tag, its value is ignored.
   static StatusOr<bool> GetValue(bool, google::protobuf::Value const&,
                                  google::spanner::v1::Type const&);
   static StatusOr<std::int64_t> GetValue(std::int64_t,
@@ -428,63 +441,68 @@ class Value {
   static StatusOr<std::string> GetValue(std::string const&,
                                         google::protobuf::Value const&,
                                         google::spanner::v1::Type const&);
+  static StatusOr<std::string> GetValue(std::string const&,
+                                        google::protobuf::Value&&,
+                                        google::spanner::v1::Type const&);
   static StatusOr<Bytes> GetValue(Bytes const&, google::protobuf::Value const&,
                                   google::spanner::v1::Type const&);
   static StatusOr<Timestamp> GetValue(Timestamp, google::protobuf::Value const&,
                                       google::spanner::v1::Type const&);
   static StatusOr<Date> GetValue(Date, google::protobuf::Value const&,
                                  google::spanner::v1::Type const&);
-  template <typename T>
-  static StatusOr<optional<T>> GetValue(optional<T> const&,
-                                        google::protobuf::Value const& pv,
+  template <typename T, typename V>
+  static StatusOr<optional<T>> GetValue(optional<T> const&, V&& pv,
                                         google::spanner::v1::Type const& pt) {
     if (pv.kind_case() == google::protobuf::Value::kNullValue) {
       return optional<T>{};
     }
-    auto value = GetValue(T{}, pv, pt);
+    auto value = GetValue(T{}, std::forward<V>(pv), pt);
     if (!value) return std::move(value).status();
     return optional<T>{*std::move(value)};
   }
-  template <typename T>
+  template <typename T, typename V>
   static StatusOr<std::vector<T>> GetValue(
-      std::vector<T> const&, google::protobuf::Value const& pv,
-      google::spanner::v1::Type const& pt) {
+      std::vector<T> const&, V&& pv, google::spanner::v1::Type const& pt) {
     if (pv.kind_case() != google::protobuf::Value::kListValue) {
       return Status(StatusCode::kUnknown, "missing ARRAY");
     }
     std::vector<T> v;
-    for (auto const& e : pv.list_value().values()) {
-      auto value = GetValue(T{}, e, pt.array_element_type());
+    for (int i = 0; i < pv.list_value().values().size(); ++i) {
+      auto&& e = GetProtoListValueElement(std::forward<V>(pv), i);
+      using ET = decltype(e);
+      auto value = GetValue(T{}, std::forward<ET>(e), pt.array_element_type());
       if (!value) return std::move(value).status();
       v.push_back(*std::move(value));
     }
     return v;
   }
-  template <typename... Ts>
+  template <typename V, typename... Ts>
   static StatusOr<std::tuple<Ts...>> GetValue(
-      std::tuple<Ts...> const&, google::protobuf::Value const& pv,
-      google::spanner::v1::Type const& pt) {
+      std::tuple<Ts...> const&, V&& pv, google::spanner::v1::Type const& pt) {
     if (pv.kind_case() != google::protobuf::Value::kListValue) {
       return Status(StatusCode::kUnknown, "missing STRUCT");
     }
     std::tuple<Ts...> tup;
     Status status;  // OK
-    ExtractTupleValues f{status, 0, pv.list_value(), pt};
+    ExtractTupleValues<V> f{status, 0, std::forward<V>(pv), pt};
     internal::ForEach(tup, f);
     if (!status.ok()) return status;
     return tup;
   }
 
   // A functor to be used with internal::ForEach (see below) to extract C++
   // types from a ListValue proto and store then in a tuple.
+  template <typename V>
   struct ExtractTupleValues {
     Status& status;
     int i;
-    google::protobuf::ListValue const& list_value;
+    V&& pv;
     google::spanner::v1::Type const& type;
     template <typename T>
     void operator()(T& t) {
-      auto value = GetValue(T{}, list_value.values(i), type);
+      auto&& e = GetProtoListValueElement(std::forward<V>(pv), i);
+      using ET = decltype(e);
+      auto value = GetValue(T{}, std::forward<ET>(e), type);
       ++i;
       if (!value) {
         status = std::move(value).status();
@@ -495,7 +513,9 @@ class Value {
     template <typename T>
     void operator()(std::pair<std::string, T>& p) {
       p.first = type.struct_type().fields(i).name();
-      auto value = GetValue(T{}, list_value.values(i), type);
+      auto&& e = GetProtoListValueElement(std::forward<V>(pv), i);
+      using ET = decltype(e);
+      auto value = GetValue(T{}, std::forward<ET>(e), type);
       ++i;
       if (!value) {
         status = std::move(value).status();
@@ -505,6 +525,19 @@ class Value {
     }
   };
 
+  // Protocol buffers are not friendly to generic programming, because they use
+  // different syntax and different names for mutable and non-mutable
+  // functions. To make GetValue(vector<T>, ...) (above) work, we need split
+  // the different protobuf syntaxes into overloaded functions.
+  static google::protobuf::Value const& GetProtoListValueElement(
+      google::protobuf::Value const& pv, int pos) {
+    return pv.list_value().values(pos);
+  }
+  static google::protobuf::Value&& GetProtoListValueElement(
+      google::protobuf::Value&& pv, int pos) {
+    return std::move(*pv.mutable_list_value()->mutable_values(pos));
+  }
+
   static bool EqualTypeProtoIgnoringNames(google::spanner::v1::Type const& a,
                                           google::spanner::v1::Type const& b);
 

google/cloud/spanner/value_test.cc

@@ -165,6 +165,107 @@ TEST(Value, BasicSemantics) {
   }
 }
 
+// NOTE: This test relies on unspecified behavior about the moved-from state
+// of std::string. Specifically, this test relies on the fact that "large"
+// strings, when moved-from, end up empty. And we use this fact to verify that
+// spanner::Value::get<T>() correctly handles moves. If this test ever breaks
+// on some platform, we could probably delete this, unless we can think of a
+// better way to test move semantics.
+TEST(Value, RvalueGetString) {
+  using Type = std::string;
+  Type const data = std::string(128, 'x');
+  Value v(data);
+
+  auto s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(data, *s);
+
+  s = std::move(v).get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(data, *s);
+
+  // NOLINTNEXTLINE(bugprone-use-after-move)
+  s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ("", *s);
+}
+
+// NOTE: This test relies on unspecified behavior about the moved-from state
+// of std::string. Specifically, this test relies on the fact that "large"
+// strings, when moved-from, end up empty. And we use this fact to verify that
+// spanner::Value::get<T>() correctly handles moves. If this test ever breaks
+// on some platform, we could probably delete this, unless we can think of a
+// better way to test move semantics.
+TEST(Value, RvalueGetOptoinalString) {
+  using Type = optional<std::string>;
+  Type const data = std::string(128, 'x');
+  Value v(data);
+
+  auto s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(*data, **s);
+
+  s = std::move(v).get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(*data, **s);
+
+  // NOLINTNEXTLINE(bugprone-use-after-move)
+  s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ("", **s);
+}
+
+// NOTE: This test relies on unspecified behavior about the moved-from state
+// of std::string. Specifically, this test relies on the fact that "large"
+// strings, when moved-from, end up empty. And we use this fact to verify that
+// spanner::Value::get<T>() correctly handles moves. If this test ever breaks
+// on some platform, we could probably delete this, unless we can think of a
+// better way to test move semantics.
+TEST(Value, RvalueGetVectorString) {
+  using Type = std::vector<std::string>;
+  Type const data(128, std::string(128, 'x'));
+  Value v(data);
+
+  auto s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(data, *s);
+
+  s = std::move(v).get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(data, *s);
+
+  // NOLINTNEXTLINE(bugprone-use-after-move)
+  s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(Type(data.size(), ""), *s);
+}
+
+// NOTE: This test relies on unspecified behavior about the moved-from state
+// of std::string. Specifically, this test relies on the fact that "large"
+// strings, when moved-from, end up empty. And we use this fact to verify that
+// spanner::Value::get<T>() correctly handles moves. If this test ever breaks
+// on some platform, we could probably delete this, unless we can think of a
+// better way to test move semantics.
+TEST(Value, RvalueGetStructString) {
+  using Type = std::tuple<std::pair<std::string, std::string>, std::string>;
+  Type data{std::make_pair("name", std::string(128, 'x')),
+            std::string(128, 'x')};
+  Value v(data);
+
+  auto s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(data, *s);
+
+  s = std::move(v).get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(data, *s);
+
+  // NOLINTNEXTLINE(bugprone-use-after-move)
+  s = v.get<Type>();
+  EXPECT_STATUS_OK(s);
+  EXPECT_EQ(Type({"name", ""}, ""), *s);
+}
+
 TEST(Value, DoubleNaN) {
   double const nan = std::nan("NaN");
   Value v{nan};