feat(bigtable): add support for STRUCT

[diegomarquezp]

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 94.75655% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.08%. Comparing base (fef647a) to head (55d79e2).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
google/cloud/bigtable/value_test.cc	95.86%	6 Missing ⚠️
google/cloud/bigtable/value.h	92.75%	5 Missing ⚠️
google/cloud/bigtable/value.cc	94.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15568      +/-   ##
==========================================
- Coverage   93.08%   93.08%   -0.01%     
==========================================
  Files        2418     2418              
  Lines      221886   222131     +245     
==========================================
+ Hits       206553   206769     +216     
- Misses      15333    15362      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[diegomarquezp]

This can likely be addressed with template functions. Will address cognitive penalty error.:

Step #2: FAILED: google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/value.cc.o 
Step #2: /usr/bin/cmake -E __run_co_compile --launcher=/usr/local/bin/sccache --tidy="/usr/local/bin/clang-tidy-wrapper;--extra-arg-before=--driver-mode=g++" --source=/workspace/google/cloud/bigtable/value.cc -- /usr/bin/clang++ -DGOOGLE_CLOUD_CPP_BIGTABLE_WITH_OTEL_METRICS -DGOOGLE_CLOUD_CPP_HAVE_OPENSSL -DGOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY -DHAVE_ABSEIL -DOPENTELEMETRY_ABI_VERSION_NO=1 -DOPENTELEMETRY_STL_VERSION=2014 -DPROTOBUF_USE_DLLS -I/workspace -isystem /workspace/cmake-out -isystem /workspace/cmake-out/google/cloud/bigtable -isystem /workspace/cmake-out/external/googleapis -isystem /workspace/cmake-out/google/cloud/monitoring -isystem /workspace/cmake-out/google/cloud/trace -fclang-abi-compat=17 -Wall -Wextra -Wconversion -Wno-sign-conversion -Werror -Wno-error=deprecated-declarations -pedantic -MD -MT google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/value.cc.o -MF google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/value.cc.o.d -o google/cloud/bigtable/CMakeFiles/google_cloud_cpp_bigtable.dir/value.cc.o -c /workspace/google/cloud/bigtable/value.cc
Step #2: /workspace/google/cloud/bigtable/value.cc:55:6: error: function 'Equal' has cognitive complexity of 36 (threshold 25) [readability-function-cognitive-complexity,-warnings-as-errors]
Step #2:    55 | bool Equal(google::bigtable::v2::Type const& pt1,  // NOLINT(misc-no-recursion)
Step #2:       |      ^
Step #2: /workspace/google/cloud/bigtable/value.cc:59:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    59 |   if (pt1.kind_case() != pt2.kind_case()) return false;
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:60:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    60 |   if (pv1.kind_case() != pv2.kind_case()) return false;
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:61:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    61 |   if (pt1.has_bool_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:64:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    64 |   if (pt1.has_int64_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:67:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    67 |   if (pt1.has_float32_type() || pt1.has_float64_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:67:30: note: +1
Step #2:    67 |   if (pt1.has_float32_type() || pt1.has_float64_type()) {
Step #2:       |                              ^
Step #2: /workspace/google/cloud/bigtable/value.cc:70:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    70 |   if (pt1.has_string_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:73:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    73 |   if (pt1.has_bytes_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:76:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    76 |   if (pt1.has_timestamp_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:77:79: note: +1
Step #2:    77 |     return pv1.timestamp_value().seconds() == pv2.timestamp_value().seconds() &&
Step #2:       |                                                                               ^
Step #2: /workspace/google/cloud/bigtable/value.cc:80:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    80 |   if (pt1.has_date_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:82:65: note: +1
Step #2:    82 |            pv1.date_value().month() == pv2.date_value().month() &&
Step #2:       |                                                                 ^
Step #2: /workspace/google/cloud/bigtable/value.cc:85:3: note: +1, including nesting penalty of 0, nesting level increased to 1
Step #2:    85 |   if (pt1.has_array_type()) {
Step #2:       |   ^
Step #2: /workspace/google/cloud/bigtable/value.cc:88:5: note: +2, including nesting penalty of 1, nesting level increased to 2
Step #2:    88 |     if (vec1.size() != vec2.size()) {

[scotthart]

I would just try factoring out the more complicated Equals checks into separate functions.

[diegomarquezp]

Done

[scotthart]

Because of google::bigtable::v2::Value::array_value is used to represent arrays, structs, and maps, let's add some tests to make sure testing for equality between a struct and array is false and attempting to convert one type into the other fail with correct error messages.

@scotthart reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 6 unresolved discussions

---

google/cloud/bigtable/value.h line 504 at r2 (raw file):

  static StatusOr<std::tuple<Ts...>> GetValue(
      std::tuple<Ts...> const&, V&& pv, google::bigtable::v2::Type const& pt) {
    if (!pv.has_array_value()) {

Per https://github.com/googleapis/googleapis/blob/374928c41762db87a25977ea3de13b62225bddb8/google/bigtable/v2/data.proto#L150, do we also need to check the type on the outermost value, in order to verify it is a struct?

---

google/cloud/bigtable/value.h line 516 at r2 (raw file):

  // A functor to be used with internal::ForEach to extract C++ types from a
  // ListValue proto and store then in a tuple.

ListValue is a specific "well known" protobuf type, that I don't think Bigtable is using (Spanner does however).

---

google/cloud/bigtable/value.h line 525 at r2 (raw file):

    template <typename T>
    void operator()(T& t) {
      auto&& e = GetProtoListValueElement(std::forward<V>(pv), i);

Either we're using the wrong function here, or we need to rename it to not specify "ListValue".

---

google/cloud/bigtable/value.cc line 112 at r2 (raw file):

}

// Compares two sets of Type and Value protos that represent a STRUCT for

s/STRUCT/ARRAY ?

---

google/cloud/bigtable/value.cc line 113 at r2 (raw file):

// Compares two sets of Type and Value protos that represent a STRUCT for
// equality. This method calls Equals() recursively to compare subtypes and

Can probably omit the recursive comment.

---

google/cloud/bigtable/value.cc line 115 at r2 (raw file):

// equality. This method calls Equals() recursively to compare subtypes and
// subvalues.
bool ArrayEqual(  // NOLINT(misc-no-recursion)

Is this linter annotation necessary?