impl(bigtable): dedup map when creating Value type

Author: scotthart

google/cloud/bigtable/value.cc

@@ -15,6 +15,7 @@
 #include "google/cloud/bigtable/value.h"
 #include "google/cloud/bigtable/timestamp.h"
 #include "google/cloud/internal/throw_delegate.h"
+#include "absl/container/flat_hash_set.h"
 #include "absl/strings/cord.h"
 #include <google/bigtable/v2/types.pb.h>
 #include <google/protobuf/descriptor.h>
@@ -591,6 +592,33 @@ StatusOr<absl::CivilDay> Value::GetValue(absl::CivilDay const&,
 
 bool Value::is_null() const { return IsNullValue(value_); }
 
+// We're calling this function from a constructor which we prefer to always
+// complete. Any errors encountered will be deferred, and we will report them on
+// attempts at accessing the value.
+// Any duplicates keys found in the map will be deduped to use the last value
+// specified for the key, per:
+// https://github.com/googleapis/googleapis/blob/0eeb1be5b78a9c7e006ee57cde95349834ae9f3b/google/bigtable/v2/types.proto#L357
+void Value::DedupProtoMap() {
+  google::bigtable::v2::Value dedup_value;
+  absl::flat_hash_set<std::string> keys;
+  for (int i = value_.array_value().values_size() - 1; i >= 0; --i) {
+    auto map_value_proto = GetProtoValueArrayElement(value_, i);
+    if (!map_value_proto.has_array_value() ||
+        map_value_proto.array_value().values_size() != 2) {
+      continue;
+    }
+    auto const& key = map_value_proto.array_value().values(0);
+    // TODO(#15766): Find a better way to hash these protos.
+    if (keys.insert(key.SerializeAsString()).second) {
+      *dedup_value.mutable_array_value()->add_values() =
+          std::move(map_value_proto);
+    }
+  }
+  std::reverse(dedup_value.mutable_array_value()->mutable_values()->begin(),
+               dedup_value.mutable_array_value()->mutable_values()->end());
+  value_ = std::move(dedup_value);
+}
+
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace bigtable
 }  // namespace cloud

google/cloud/bigtable/value.h

@@ -629,13 +629,6 @@ class Value {
                                    pt.map_type().value_type());
       if (!key) return std::move(key).status();
       if (!value) return std::move(value).status();
-
-      // the documented behavior indicates that the last value will take
-      // precedence for a given key.
-      auto const& pos = m.find(key.value());
-      if (pos != m.end()) {
-        m.erase(pos);
-      }
       m.insert(std::make_pair(*std::move(key), *std::move(value)));
     }
     return m;
@@ -692,6 +685,8 @@ class Value {
     return std::move(*pv.mutable_array_value()->mutable_values(pos));
   }
 
+  void DedupProtoMap();
+
   // A private templated constructor that is called by all the public
   // constructors to set the type_ and value_ members. The `PrivateConstructor`
   // type is used so that this overload is never chosen for
@@ -701,10 +696,20 @@ class Value {
   struct PrivateConstructor {};
   template <typename T>
   Value(PrivateConstructor, T&& t)
-      : type_(MakeTypeProto(t)), value_(MakeValueProto(std::forward<T>(t))) {}
+      : type_(MakeTypeProto(t)), value_(MakeValueProto(std::forward<T>(t))) {
+    if (type_.has_map_type() && value_.has_array_value() &&
+        !value_.array_value().values().empty()) {
+      DedupProtoMap();
+    }
+  }
 
   Value(google::bigtable::v2::Type t, google::bigtable::v2::Value v)
-      : type_(std::move(t)), value_(std::move(v)) {}
+      : type_(std::move(t)), value_(std::move(v)) {
+    if (type_.has_map_type() && value_.has_array_value() &&
+        !value_.array_value().values().empty()) {
+      DedupProtoMap();
+    }
+  }
 
   friend struct bigtable_internal::ValueInternals;
   friend class Parameter;

google/cloud/bigtable/value_test.cc

@@ -18,6 +18,7 @@
 #include "google/cloud/testing_util/is_proto_equal.h"
 #include "google/cloud/testing_util/status_matchers.h"
 #include "absl/strings/cord.h"
+#include <google/protobuf/text_format.h>
 #include <google/type/date.pb.h>
 #include <gmock/gmock.h>
 #include <cmath>
@@ -39,6 +40,7 @@ namespace {
 using ::google::cloud::testing_util::IsOk;
 using ::google::cloud::testing_util::IsOkAndHolds;
 using ::google::cloud::testing_util::IsProtoEqual;
+using ::google::protobuf::TextFormat;
 using ::testing::Not;
 
 absl::Time MakeTime(std::int64_t sec, int nanos) {
@@ -169,8 +171,6 @@ TEST(Value, BasicSemantics) {
   for (auto ts : TestTimes()) {
     SCOPED_TRACE("Testing: google::cloud::bigtable::Timestamp " +
                  bigtable_internal::TimestampToRFC3339(ts));
-    std::cout << "Testing: google::cloud::bigtable::Timestamp "
-              << bigtable_internal::TimestampToRFC3339(ts) << std::endl;
     TestBasicSemantics(ts);
     std::vector<Timestamp> v(5, ts);
     TestBasicSemantics(v);
@@ -1004,6 +1004,56 @@ TEST(Value, ProtoConversionMap) {
   EXPECT_TRUE(p.first.has_map_type());
 }
 
+TEST(Value, ProtoMapWithDuplicateKeys) {
+  auto constexpr kTypeProto = R"""(
+map_type {
+  key_type {
+    bytes_type {
+    }
+  }
+  value_type {
+    string_type {
+    }
+  }
+}
+)""";
+  google::bigtable::v2::Type type_proto;
+  ASSERT_TRUE(TextFormat::ParseFromString(kTypeProto, &type_proto));
+
+  auto constexpr kValueProto = R"""(
+array_value {
+  values {
+    array_value {
+      values {
+        bytes_value: "foo"
+      }
+      values {
+        string_value: "foo"
+      }
+    }
+  }
+  values {
+    array_value {
+      values {
+        bytes_value: "foo"
+      }
+      values {
+        string_value: "bar"
+      }
+    }
+  }
+}
+)""";
+  google::bigtable::v2::Value value_proto;
+  ASSERT_TRUE(TextFormat::ParseFromString(kValueProto, &value_proto));
+
+  auto value = bigtable_internal::FromProto(type_proto, value_proto);
+  auto map = value.get<std::unordered_map<Bytes, std::string>>();
+  ASSERT_STATUS_OK(map);
+  EXPECT_THAT(*map, ::testing::UnorderedElementsAre(
+                        ::testing::Pair(Bytes("foo"), "bar")));
+}
+
 void SetNullProtoKind(Value& v) {
   auto p = bigtable_internal::ToProto(v);
   p.second.clear_kind();