impl(bigtable): first group of query conformance tests (#15768)

* impl(bigtable): dedup map when creating Value type

* impl(bigtable): first group of query conformance tests

Author: scotthart

.typos.toml

@@ -60,6 +60,8 @@ extend-exclude = [
     "docfx/doxygen2toc_test.cc",
     # If we're using a post-download patch file for googleapis.
     "external/googleapis.patch",
+    # Mispelled test names in Bigtable conformance tests
+    "google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh",
 ]
 
 [default]

google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh

@@ -46,6 +46,43 @@ go test -v \
   -skip "Generic_CloseClient|Generic_DeadlineExceeded|NoRetry_OutOfOrderError_Reverse|Retry_LastScannedRow_Reverse|Retry_WithRetryInfo_OverallDedaline|TestExecuteQuery" \
   -proxy_addr=:9999
 exit_status=$?
+
+# Run all the ExecuteQuery tests that either work or we plan to skip such as
+# CloseClient
+go test -v \
+  -run "TestExecuteQuery" \
+  -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField|RetryTest_WithPlanRefresh|PlanRefresh" \
+  -proxy_addr=:9999
+exit_status=$?
+
+# These next four go test commands group the currently failing ExecuteQuery
+# tests into groups that exercise similar functionality and should be worked on
+# together.
+
+# Metadata tests b/461232934
+#go test -v \
+#  -run "FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType" \
+#  -proxy_addr=:9999
+#exit_status=$?
+
+# Stream reading tests b/461232110
+#go test -v \
+#  -run "FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch" \
+#  -proxy_addr=:9999
+#exit_status=$?
+
+# Response/Metadata mismatches b/461233335
+#go test -v \
+#  -run "FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField" \
+#  -proxy_addr=:9999
+#exit_status=$?
+
+# QueryPlan refresh tests b/461233613
+#go test -v \
+#  -run "RetryTest_WithPlanRefresh|PlanRefresh" \
+#  -proxy_addr=:9999
+#exit_status=$?
+
 # Remove the entire module cache, including unpacked source code of versioned
 # dependencies.
 go clean -modcache

google/cloud/bigtable/test_proxy/cbt_test_proxy.cc

@@ -14,6 +14,7 @@
 
 #include "google/cloud/bigtable/test_proxy/cbt_test_proxy.h"
 #include "google/cloud/bigtable/cell.h"
+#include "google/cloud/bigtable/client.h"
 #include "google/cloud/bigtable/idempotent_mutation_policy.h"
 #include "google/cloud/bigtable/mutations.h"
 #include "google/cloud/bigtable/table.h"
@@ -360,6 +361,70 @@ grpc::Status CbtTestProxy::ReadModifyWriteRow(
   return grpc::Status();
 }
 
+grpc::Status CbtTestProxy::ExecuteQuery(
+    grpc::ServerContext*,
+    google::bigtable::testproxy::ExecuteQueryRequest const* request,
+    google::bigtable::testproxy::ExecuteQueryResult* response) {
+  // Retrieve connection
+  auto const& conn = GetConnection(request->client_id());
+  if (!conn.ok()) return ToGrpcStatus(std::move(conn).status());
+  auto client = bigtable::Client(*conn);
+  auto const& request_proto = request->request();
+
+  // Call prepare query
+  auto instance = MakeInstanceResource(request_proto.instance_name());
+  // NOLINTNEXTLINE(deprecated-declarations)
+  bigtable::SqlStatement sql_statement{request_proto.query()};
+  auto prepared_query =
+      client.PrepareQuery(*std::move(instance), sql_statement);
+  if (!prepared_query.ok()) {
+    *response->mutable_status() = ToRpcStatus(prepared_query.status());
+    return ::grpc::Status();
+  }
+
+  // Bind parameters
+  std::unordered_map<std::string, Value> params;
+  for (auto const& param : request_proto.params()) {
+    auto value =
+        bigtable_internal::FromProto(param.second.type(), param.second);
+    params.insert(std::make_pair(param.first, std::move(value)));
+  }
+  auto bound_query = prepared_query->BindParameters(params);
+  auto bound_query_metadata = bound_query.response()->metadata();
+
+  RowStream result = client.ExecuteQuery(std::move(bound_query), {});
+
+  Status status;
+  std::vector<google::bigtable::testproxy::SqlRow> proxy_rows;
+  for (auto& row : result) {
+    if (!row.ok()) {
+      status = row.status();
+      break;
+    }
+    google::bigtable::testproxy::SqlRow proxy_row;
+    for (auto const& v : row->values()) {
+      *proxy_row.add_values() = bigtable_internal::ToProto(v).second;
+    }
+    proxy_rows.push_back(std::move(proxy_row));
+  }
+
+  if (status.ok()) {
+    for (auto& p : proxy_rows) {
+      *response->add_rows() = std::move(p);
+    }
+  }
+
+  // populate metadata
+  google::bigtable::testproxy::ResultSetMetadata metadata;
+  for (auto const& column : bound_query_metadata.proto_schema().columns()) {
+    *metadata.add_columns() = column;
+  }
+  *response->mutable_metadata() = metadata;
+
+  *response->mutable_status() = ToRpcStatus(status);
+  return grpc::Status();
+}
+
 StatusOr<std::shared_ptr<DataConnection>> CbtTestProxy::GetConnection(
     std::string const& client_id) {
   std::lock_guard<std::mutex> lk(mu_);

google/cloud/bigtable/test_proxy/cbt_test_proxy.h

@@ -91,6 +91,11 @@ class CbtTestProxy final
       ::google::bigtable::testproxy::ReadModifyWriteRowRequest const* request,
       ::google::bigtable::testproxy::RowResult* response) override;
 
+  grpc::Status ExecuteQuery(
+      ::grpc::ServerContext* context,
+      ::google::bigtable::testproxy::ExecuteQueryRequest const* request,
+      ::google::bigtable::testproxy::ExecuteQueryResult* response) override;
+
  private:
   StatusOr<std::shared_ptr<DataConnection>> GetConnection(
       std::string const& client_id);

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();

protos/google/cloud/bigtable/test_proxy/BUILD.bazel

@@ -32,6 +32,7 @@ proto_library(
         "@com_google_googleapis//google/api:client_proto",
         "@com_google_googleapis//google/bigtable/v2:bigtable_proto",
         "@com_google_googleapis//google/rpc:status_proto",
+        "@com_google_protobuf//:descriptor_proto",
         "@com_google_protobuf//:duration_proto",
     ],
 )