From 86b33fca108d2abf2342cd4f381ba4dfe6465fc1 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Mon, 17 Nov 2025 19:54:35 +0000 Subject: [PATCH 01/12] test(bigtable): conform with ExecuteQuery_FailsOnTypeMismatch* tests --- .../ci/run_conformance_tests_proxy_bazel.sh | 2 +- .../internal/data_connection_impl_test.cc | 18 ++- .../partial_result_set_resume_test.cc | 18 ++- .../internal/partial_result_set_source.cc | 15 +- .../partial_result_set_source_test.cc | 128 +++++++++++------- 5 files changed, 114 insertions(+), 67 deletions(-) diff --git a/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh b/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh index fe5fb720a12eb..30c5655f493e8 100755 --- a/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh +++ b/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh @@ -51,7 +51,7 @@ exit_status=$? # CloseClient go test -v \ -run "TestExecuteQuery" \ - -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField|RetryTest_WithPlanRefresh|PlanRefresh" \ + -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|FailsOnStructMissingField|RetryTest_WithPlanRefresh|PlanRefresh" \ -proxy_addr=:9999 exit_status=$? diff --git a/google/cloud/bigtable/internal/data_connection_impl_test.cc b/google/cloud/bigtable/internal/data_connection_impl_test.cc index ece9d6ae0851d..d34bb12e864a0 100644 --- a/google/cloud/bigtable/internal/data_connection_impl_test.cc +++ b/google/cloud/bigtable/internal/data_connection_impl_test.cc @@ -226,23 +226,27 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, absl::optional resume_token, bool reset) { google::bigtable::v2::ProtoRows proto_rows; for (auto const& v : values) { - proto_rows.add_values()->set_string_value(v); + google::bigtable::v2::Value value; + value.set_string_value(v); + google::bigtable::v2::Type type; + *(*value.mutable_type()).mutable_string_type() = + std::move(google::bigtable::v2::Type_String{}); + *proto_rows.add_values() = value; } - std::string binary_batch_data = proto_rows.SerializeAsString(); auto text = absl::Substitute( R"pb( - proto_rows_batch: { - batch_data: "$0", - }, - $1 $2 + proto_rows_batch: {}, + $0 $1 estimated_batch_size: 31, batch_checksum: 123456 )pb", - binary_batch_data, resume_token ? absl::Substitute(R"(resume_token: "$0")", *resume_token) : "", reset ? "reset: true," : ""); ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(text, &response)); + std::string binary_batch_data = proto_rows.SerializeAsString(); + *(*response.mutable_proto_rows_batch()).mutable_batch_data() = + binary_batch_data; } // For tests that need to manually interact with the client/server metadata in a diff --git a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc index a3433029f2566..fa05e4fb79974 100644 --- a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc +++ b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc @@ -79,23 +79,27 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, absl::optional resume_token, bool reset) { google::bigtable::v2::ProtoRows proto_rows; for (auto const& v : values) { - proto_rows.add_values()->set_string_value(v); + google::bigtable::v2::Value value; + value.set_string_value(v); + google::bigtable::v2::Type type; + *(*value.mutable_type()).mutable_string_type() = + std::move(google::bigtable::v2::Type_String{}); + *proto_rows.add_values() = value; } - std::string binary_batch_data = proto_rows.SerializeAsString(); auto text = absl::Substitute( R"pb( - proto_rows_batch: { - batch_data: "$0", - }, - $1 $2 + proto_rows_batch: {}, + $0 $1 estimated_batch_size: 31, batch_checksum: 123456 )pb", - binary_batch_data, resume_token ? absl::Substitute(R"(resume_token: "$0")", *resume_token) : "", reset ? "reset: true," : ""); ASSERT_TRUE(TextFormat::ParseFromString(text, &response)); + std::string binary_batch_data = proto_rows.SerializeAsString(); + *(*response.mutable_proto_rows_batch()).mutable_batch_data() = + binary_batch_data; } // Helper function for MockPartialResultSetReader::Read to return true and diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index ef74c7155f1fc..7bb3e86b3b1f7 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -51,9 +51,8 @@ PartialResultSetSource::Create( // Do an initial read from the stream to determine the fate of the factory. auto status = source->ReadFromStream(); - // If the initial read finished the stream, and `Finish()` failed, then - // creating the `PartialResultSetSource` should fail with the same error. - if (source->state_ == State::kFinished && !status.ok()) return status; + // Any error during parsing will be returned. + if (!status.ok()) return status; return {std::move(source)}; } @@ -195,6 +194,12 @@ Status PartialResultSetSource::ProcessDataFromStream( return {}; // OK } +bool ColumnAndValueTypesMatch( + google::bigtable::v2::ColumnMetadata const& column, + google::bigtable::v2::Value value) { + return value.type().SerializeAsString() == column.type().SerializeAsString(); +} + Status PartialResultSetSource::BufferProtoRows() { if (metadata_.has_value()) { auto const& proto_schema = metadata_->proto_schema(); @@ -219,6 +224,10 @@ Status PartialResultSetSource::BufferProtoRows() { while (parsed_value != proto_values.end()) { for (auto const& column : proto_schema.columns()) { + if (!ColumnAndValueTypesMatch(column, *parsed_value)) { + return internal::InternalError("Metadata and Value not matching.", + GCP_ERROR_INFO()); + } auto value = FromProto(column.type(), *parsed_value); values.push_back(std::move(value)); ++parsed_value; diff --git a/google/cloud/bigtable/internal/partial_result_set_source_test.cc b/google/cloud/bigtable/internal/partial_result_set_source_test.cc index cbd8af4eca494..aa0c9eaeacf87 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source_test.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source_test.cc @@ -265,27 +265,33 @@ TEST(PartialResultSetSourceTest, SingleResponse) { google::bigtable::v2::ResultSetMetadata metadata; ASSERT_TRUE(TextFormat::ParseFromString(kResultMetadataText, &metadata)); auto constexpr kProtoRowsText = R"pb( - values { string_value: "r1" } - values { string_value: "f1" } - values { string_value: "q1" } + values { + string_value: "r1" + type { string_type {} } + } + values { + string_value: "f1" + type { string_type {} } + } + values { + string_value: "q1" + type { string_type {} } + } )pb"; google::bigtable::v2::ProtoRows proto_rows; ASSERT_TRUE(TextFormat::ParseFromString(kProtoRowsText, &proto_rows)); - std::string binary_batch_data = proto_rows.SerializeAsString(); - std::string partial_result_set_text = - absl::Substitute(R"pb( - proto_rows_batch: { - batch_data: "$0", - }, - resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", - reset: true, - estimated_batch_size: 31, - batch_checksum: 123456 - )pb", - binary_batch_data); + std::string partial_result_set_text = R"pb( + proto_rows_batch: {}, + resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", + reset: true, + estimated_batch_size: 31, + batch_checksum: 123456 + )pb"; google::bigtable::v2::PartialResultSet response; ASSERT_TRUE(TextFormat::ParseFromString(partial_result_set_text, &response)); + *(*response.mutable_proto_rows_batch()).mutable_batch_data() = + proto_rows.SerializeAsString(); auto grpc_reader = std::make_unique(); @@ -364,19 +370,46 @@ TEST(PartialResultSetSourceTest, MultipleResponses) { std::array proto_rows_text{{ R"pb( - values { string_value: "a1" } - values { string_value: "a2" } - values { string_value: "a3" } + values { + string_value: "a1" + type { string_type {} } + } + values { + string_value: "a2" + type { string_type {} } + } + values { + string_value: "a3" + type { string_type {} } + } )pb", R"pb( - values { string_value: "b1" } - values { string_value: "b2" } - values { string_value: "b3" } + values { + string_value: "b1" + type { string_type {} } + } + values { + string_value: "b2" + type { string_type {} } + } + values { + string_value: "b3" + type { string_type {} } + } )pb", R"pb( - values { string_value: "c1" } - values { string_value: "c2" } - values { string_value: "c3" } + values { + string_value: "c1" + type { string_type {} } + } + values { + string_value: "c2" + type { string_type {} } + } + values { + string_value: "c3" + type { string_type {} } + } )pb", }}; @@ -384,21 +417,18 @@ TEST(PartialResultSetSourceTest, MultipleResponses) { for (auto const* text : proto_rows_text) { google::bigtable::v2::ProtoRows proto_rows; ASSERT_TRUE(TextFormat::ParseFromString(text, &proto_rows)); - std::string binary_batch_data = proto_rows.SerializeAsString(); - std::string partial_result_set_text = absl::Substitute( - R"pb( - proto_rows_batch: { - batch_data: "$0", - }, - resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", - reset: true, - estimated_batch_size: 31, - batch_checksum: 123456 - )pb", - binary_batch_data); + std::string partial_result_set_text = R"pb( + proto_rows_batch: {}, + resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", + reset: true, + estimated_batch_size: 31, + batch_checksum: 123456 + )pb"; google::bigtable::v2::PartialResultSet response; ASSERT_TRUE( TextFormat::ParseFromString(partial_result_set_text, &response)); + *(*response.mutable_proto_rows_batch()).mutable_batch_data() = + proto_rows.SerializeAsString(); responses.push_back(std::move(response)); } @@ -495,7 +525,10 @@ TEST(PartialResultSetSourceTest, ResponseWithNoValues) { std::array proto_rows_text{{ R"pb( - values { string_value: "a1" } + values { + string_value: "a1" + type { string_type {} } + } )pb", R"pb( )pb", @@ -506,21 +539,18 @@ TEST(PartialResultSetSourceTest, ResponseWithNoValues) { for (auto const* text : proto_rows_text) { google::bigtable::v2::ProtoRows proto_rows; ASSERT_TRUE(TextFormat::ParseFromString(text, &proto_rows)); - std::string binary_batch_data = proto_rows.SerializeAsString(); - std::string partial_result_set_text = absl::Substitute( - R"pb( - proto_rows_batch: { - batch_data: "$0", - }, - resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", - reset: true, - estimated_batch_size: 31, - batch_checksum: 123456 - )pb", - binary_batch_data); + std::string partial_result_set_text = R"pb( + proto_rows_batch: {}, + resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", + reset: true, + estimated_batch_size: 31, + batch_checksum: 123456 + )pb"; google::bigtable::v2::PartialResultSet response; ASSERT_TRUE( TextFormat::ParseFromString(partial_result_set_text, &response)); + *(*response.mutable_proto_rows_batch()).mutable_batch_data() = + proto_rows.SerializeAsString(); responses.push_back(std::move(response)); } From ed28034510174b81f8d2b0811cca8dfeb676bf47 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Mon, 17 Nov 2025 19:58:00 +0000 Subject: [PATCH 02/12] chore: clang-tidy --- google/cloud/bigtable/internal/partial_result_set_source.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index 7bb3e86b3b1f7..fadb6f613c87b 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -196,7 +196,7 @@ Status PartialResultSetSource::ProcessDataFromStream( bool ColumnAndValueTypesMatch( google::bigtable::v2::ColumnMetadata const& column, - google::bigtable::v2::Value value) { + google::bigtable::v2::Value const& value) { return value.type().SerializeAsString() == column.type().SerializeAsString(); } From 08930aa666c788b6a6d2d7de79348bbcf4790fa4 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Mon, 17 Nov 2025 20:00:14 +0000 Subject: [PATCH 03/12] chore: remove unused var --- google/cloud/bigtable/internal/data_connection_impl_test.cc | 1 - google/cloud/bigtable/internal/partial_result_set_resume_test.cc | 1 - 2 files changed, 2 deletions(-) diff --git a/google/cloud/bigtable/internal/data_connection_impl_test.cc b/google/cloud/bigtable/internal/data_connection_impl_test.cc index d34bb12e864a0..af5e8baac646a 100644 --- a/google/cloud/bigtable/internal/data_connection_impl_test.cc +++ b/google/cloud/bigtable/internal/data_connection_impl_test.cc @@ -228,7 +228,6 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, for (auto const& v : values) { google::bigtable::v2::Value value; value.set_string_value(v); - google::bigtable::v2::Type type; *(*value.mutable_type()).mutable_string_type() = std::move(google::bigtable::v2::Type_String{}); *proto_rows.add_values() = value; diff --git a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc index fa05e4fb79974..21b6f5fde7fe3 100644 --- a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc +++ b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc @@ -81,7 +81,6 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, for (auto const& v : values) { google::bigtable::v2::Value value; value.set_string_value(v); - google::bigtable::v2::Type type; *(*value.mutable_type()).mutable_string_type() = std::move(google::bigtable::v2::Type_String{}); *proto_rows.add_values() = value; From 882f299cd6875ae3b2ed0ea5bbbfc3cc865eb1f8 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Mon, 17 Nov 2025 20:01:11 +0000 Subject: [PATCH 04/12] chore: move value --- google/cloud/bigtable/internal/data_connection_impl_test.cc | 2 +- .../cloud/bigtable/internal/partial_result_set_resume_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/google/cloud/bigtable/internal/data_connection_impl_test.cc b/google/cloud/bigtable/internal/data_connection_impl_test.cc index af5e8baac646a..7508431b96c6c 100644 --- a/google/cloud/bigtable/internal/data_connection_impl_test.cc +++ b/google/cloud/bigtable/internal/data_connection_impl_test.cc @@ -230,7 +230,7 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, value.set_string_value(v); *(*value.mutable_type()).mutable_string_type() = std::move(google::bigtable::v2::Type_String{}); - *proto_rows.add_values() = value; + *proto_rows.add_values() = std::move(value); } auto text = absl::Substitute( R"pb( diff --git a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc index 21b6f5fde7fe3..dbaf53cd9f7bb 100644 --- a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc +++ b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc @@ -83,7 +83,7 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, value.set_string_value(v); *(*value.mutable_type()).mutable_string_type() = std::move(google::bigtable::v2::Type_String{}); - *proto_rows.add_values() = value; + *proto_rows.add_values() = std::move(value); } auto text = absl::Substitute( R"pb( From eefa27bbcad6f4dbd71867e1e10d3d603b288b6a Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Mon, 17 Nov 2025 21:58:34 +0000 Subject: [PATCH 05/12] chore: check at value level --- .../internal/data_connection_impl_test.cc | 17 +-- .../partial_result_set_resume_test.cc | 17 +-- .../internal/partial_result_set_source.cc | 108 ++++++++++++++- .../partial_result_set_source_test.cc | 128 +++++++----------- google/cloud/bigtable/value.cc | 8 +- google/cloud/bigtable/value.h | 5 + 6 files changed, 172 insertions(+), 111 deletions(-) diff --git a/google/cloud/bigtable/internal/data_connection_impl_test.cc b/google/cloud/bigtable/internal/data_connection_impl_test.cc index 7508431b96c6c..ece9d6ae0851d 100644 --- a/google/cloud/bigtable/internal/data_connection_impl_test.cc +++ b/google/cloud/bigtable/internal/data_connection_impl_test.cc @@ -226,26 +226,23 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, absl::optional resume_token, bool reset) { google::bigtable::v2::ProtoRows proto_rows; for (auto const& v : values) { - google::bigtable::v2::Value value; - value.set_string_value(v); - *(*value.mutable_type()).mutable_string_type() = - std::move(google::bigtable::v2::Type_String{}); - *proto_rows.add_values() = std::move(value); + proto_rows.add_values()->set_string_value(v); } + std::string binary_batch_data = proto_rows.SerializeAsString(); auto text = absl::Substitute( R"pb( - proto_rows_batch: {}, - $0 $1 + proto_rows_batch: { + batch_data: "$0", + }, + $1 $2 estimated_batch_size: 31, batch_checksum: 123456 )pb", + binary_batch_data, resume_token ? absl::Substitute(R"(resume_token: "$0")", *resume_token) : "", reset ? "reset: true," : ""); ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(text, &response)); - std::string binary_batch_data = proto_rows.SerializeAsString(); - *(*response.mutable_proto_rows_batch()).mutable_batch_data() = - binary_batch_data; } // For tests that need to manually interact with the client/server metadata in a diff --git a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc index dbaf53cd9f7bb..a3433029f2566 100644 --- a/google/cloud/bigtable/internal/partial_result_set_resume_test.cc +++ b/google/cloud/bigtable/internal/partial_result_set_resume_test.cc @@ -79,26 +79,23 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response, absl::optional resume_token, bool reset) { google::bigtable::v2::ProtoRows proto_rows; for (auto const& v : values) { - google::bigtable::v2::Value value; - value.set_string_value(v); - *(*value.mutable_type()).mutable_string_type() = - std::move(google::bigtable::v2::Type_String{}); - *proto_rows.add_values() = std::move(value); + proto_rows.add_values()->set_string_value(v); } + std::string binary_batch_data = proto_rows.SerializeAsString(); auto text = absl::Substitute( R"pb( - proto_rows_batch: {}, - $0 $1 + proto_rows_batch: { + batch_data: "$0", + }, + $1 $2 estimated_batch_size: 31, batch_checksum: 123456 )pb", + binary_batch_data, resume_token ? absl::Substitute(R"(resume_token: "$0")", *resume_token) : "", reset ? "reset: true," : ""); ASSERT_TRUE(TextFormat::ParseFromString(text, &response)); - std::string binary_batch_data = proto_rows.SerializeAsString(); - *(*response.mutable_proto_rows_batch()).mutable_batch_data() = - binary_batch_data; } // Helper function for MockPartialResultSetReader::Read to return true and diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index fadb6f613c87b..30eb61f40d1d0 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -194,10 +194,108 @@ Status PartialResultSetSource::ProcessDataFromStream( return {}; // OK } -bool ColumnAndValueTypesMatch( - google::bigtable::v2::ColumnMetadata const& column, - google::bigtable::v2::Value const& value) { - return value.type().SerializeAsString() == column.type().SerializeAsString(); +/** + * Checks whether the declared type in column matches the value's contents. + * Since the received values may or may not have type() set, we check against + * the value contents themselves + */ +bool TypeAndValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { + using google::bigtable::v2::Type; + bool has_matching_value; + switch (type.kind_case()) { + case Type::kArrayType: { + if (!value.has_array_value()) { + has_matching_value = value.has_array_value(); + break; + } + has_matching_value = true; + for (auto const& val : value.array_value().values()) { + if (!TypeAndValuesMatch(type.array_type().element_type(), val)) { + has_matching_value = false; + break; + } + } + break; + } + case Type::kMapType: { + if (!value.has_array_value()) { + has_matching_value = value.has_array_value(); + break; + } + has_matching_value = true; + auto key_type = type.map_type().key_type(); + ; + auto value_type = type.map_type().value_type(); + ; + for (auto const& val : value.array_value().values()) { + if (!val.has_array_value() || val.array_value().values_size() != 2) { + has_matching_value = false; + break; + } + auto map_key = val.array_value().values(0); + auto map_value = val.array_value().values(1); + if (!TypeAndValuesMatch(key_type, map_key) || + !TypeAndValuesMatch(value_type, map_value)) { + has_matching_value = false; + break; + } + } + break; + } + case Type::kStructType: { + if (!value.has_array_value()) { + has_matching_value = value.has_array_value(); + break; + } + has_matching_value = true; + auto fields = type.struct_type().fields(); + auto values = value.array_value().values(); + if (fields.size() != values.size()) { + has_matching_value = value.has_array_value(); + break; + } + for (int i = 0; i < fields.size(); ++i) { + auto const& f1 = fields.Get(i); + auto const& v = values[i]; + if (!TypeAndValuesMatch(f1.type(), v)) { + has_matching_value = false; + break; + } + } + break; + } + case Type::kBoolType: + has_matching_value = value.has_bool_value(); + break; + case Type::kBytesType: + has_matching_value = value.has_bytes_value(); + break; + case Type::kDateType: + has_matching_value = value.has_date_value(); + break; + case Type::kEnumType: + has_matching_value = value.has_int_value(); + break; + case Type::kFloat32Type: + case Type::kFloat64Type: + has_matching_value = value.has_float_value(); + break; + case Type::kInt64Type: + has_matching_value = value.has_int_value(); + break; + case Type::kStringType: + has_matching_value = value.has_string_value(); + break; + case Type::kTimestampType: + has_matching_value = value.has_timestamp_value(); + break; + default: + has_matching_value = false; + break; + } + // Nulls are allowed; + return has_matching_value || bigtable::Value::IsNullValue(value); } Status PartialResultSetSource::BufferProtoRows() { @@ -224,7 +322,7 @@ Status PartialResultSetSource::BufferProtoRows() { while (parsed_value != proto_values.end()) { for (auto const& column : proto_schema.columns()) { - if (!ColumnAndValueTypesMatch(column, *parsed_value)) { + if (!TypeAndValuesMatch(column.type(), *parsed_value)) { return internal::InternalError("Metadata and Value not matching.", GCP_ERROR_INFO()); } diff --git a/google/cloud/bigtable/internal/partial_result_set_source_test.cc b/google/cloud/bigtable/internal/partial_result_set_source_test.cc index aa0c9eaeacf87..cbd8af4eca494 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source_test.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source_test.cc @@ -265,33 +265,27 @@ TEST(PartialResultSetSourceTest, SingleResponse) { google::bigtable::v2::ResultSetMetadata metadata; ASSERT_TRUE(TextFormat::ParseFromString(kResultMetadataText, &metadata)); auto constexpr kProtoRowsText = R"pb( - values { - string_value: "r1" - type { string_type {} } - } - values { - string_value: "f1" - type { string_type {} } - } - values { - string_value: "q1" - type { string_type {} } - } + values { string_value: "r1" } + values { string_value: "f1" } + values { string_value: "q1" } )pb"; google::bigtable::v2::ProtoRows proto_rows; ASSERT_TRUE(TextFormat::ParseFromString(kProtoRowsText, &proto_rows)); - std::string partial_result_set_text = R"pb( - proto_rows_batch: {}, - resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", - reset: true, - estimated_batch_size: 31, - batch_checksum: 123456 - )pb"; + std::string binary_batch_data = proto_rows.SerializeAsString(); + std::string partial_result_set_text = + absl::Substitute(R"pb( + proto_rows_batch: { + batch_data: "$0", + }, + resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", + reset: true, + estimated_batch_size: 31, + batch_checksum: 123456 + )pb", + binary_batch_data); google::bigtable::v2::PartialResultSet response; ASSERT_TRUE(TextFormat::ParseFromString(partial_result_set_text, &response)); - *(*response.mutable_proto_rows_batch()).mutable_batch_data() = - proto_rows.SerializeAsString(); auto grpc_reader = std::make_unique(); @@ -370,46 +364,19 @@ TEST(PartialResultSetSourceTest, MultipleResponses) { std::array proto_rows_text{{ R"pb( - values { - string_value: "a1" - type { string_type {} } - } - values { - string_value: "a2" - type { string_type {} } - } - values { - string_value: "a3" - type { string_type {} } - } + values { string_value: "a1" } + values { string_value: "a2" } + values { string_value: "a3" } )pb", R"pb( - values { - string_value: "b1" - type { string_type {} } - } - values { - string_value: "b2" - type { string_type {} } - } - values { - string_value: "b3" - type { string_type {} } - } + values { string_value: "b1" } + values { string_value: "b2" } + values { string_value: "b3" } )pb", R"pb( - values { - string_value: "c1" - type { string_type {} } - } - values { - string_value: "c2" - type { string_type {} } - } - values { - string_value: "c3" - type { string_type {} } - } + values { string_value: "c1" } + values { string_value: "c2" } + values { string_value: "c3" } )pb", }}; @@ -417,18 +384,21 @@ TEST(PartialResultSetSourceTest, MultipleResponses) { for (auto const* text : proto_rows_text) { google::bigtable::v2::ProtoRows proto_rows; ASSERT_TRUE(TextFormat::ParseFromString(text, &proto_rows)); - std::string partial_result_set_text = R"pb( - proto_rows_batch: {}, - resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", - reset: true, - estimated_batch_size: 31, - batch_checksum: 123456 - )pb"; + std::string binary_batch_data = proto_rows.SerializeAsString(); + std::string partial_result_set_text = absl::Substitute( + R"pb( + proto_rows_batch: { + batch_data: "$0", + }, + resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", + reset: true, + estimated_batch_size: 31, + batch_checksum: 123456 + )pb", + binary_batch_data); google::bigtable::v2::PartialResultSet response; ASSERT_TRUE( TextFormat::ParseFromString(partial_result_set_text, &response)); - *(*response.mutable_proto_rows_batch()).mutable_batch_data() = - proto_rows.SerializeAsString(); responses.push_back(std::move(response)); } @@ -525,10 +495,7 @@ TEST(PartialResultSetSourceTest, ResponseWithNoValues) { std::array proto_rows_text{{ R"pb( - values { - string_value: "a1" - type { string_type {} } - } + values { string_value: "a1" } )pb", R"pb( )pb", @@ -539,18 +506,21 @@ TEST(PartialResultSetSourceTest, ResponseWithNoValues) { for (auto const* text : proto_rows_text) { google::bigtable::v2::ProtoRows proto_rows; ASSERT_TRUE(TextFormat::ParseFromString(text, &proto_rows)); - std::string partial_result_set_text = R"pb( - proto_rows_batch: {}, - resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", - reset: true, - estimated_batch_size: 31, - batch_checksum: 123456 - )pb"; + std::string binary_batch_data = proto_rows.SerializeAsString(); + std::string partial_result_set_text = absl::Substitute( + R"pb( + proto_rows_batch: { + batch_data: "$0", + }, + resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=", + reset: true, + estimated_batch_size: 31, + batch_checksum: 123456 + )pb", + binary_batch_data); google::bigtable::v2::PartialResultSet response; ASSERT_TRUE( TextFormat::ParseFromString(partial_result_set_text, &response)); - *(*response.mutable_proto_rows_batch()).mutable_batch_data() = - proto_rows.SerializeAsString(); responses.push_back(std::move(response)); } diff --git a/google/cloud/bigtable/value.cc b/google/cloud/bigtable/value.cc index 47e88ddfc8441..4dc7b2996e675 100644 --- a/google/cloud/bigtable/value.cc +++ b/google/cloud/bigtable/value.cc @@ -207,12 +207,6 @@ bool MapEqual( // NOLINT(misc-no-recursion) comparison_function); } -// From the proto description, `NULL` values are represented by having a kind -// equal to KIND_NOT_SET -bool IsNullValue(google::bigtable::v2::Value const& value) { - return value.kind_case() == google::bigtable::v2::Value::KIND_NOT_SET; -} - // A helper to escape all double quotes in the given string `s`. For example, // if given `"foo"`, outputs `\"foo\"`. This is useful when a caller needs to // wrap `s` itself in double quotes. @@ -238,7 +232,7 @@ std::ostream& StreamHelper(std::ostream& os, // NOLINT(misc-no-recursion) google::bigtable::v2::Value const& v, google::bigtable::v2::Type const& t, StreamMode mode) { - if (IsNullValue(v)) { + if (Value::IsNullValue(v)) { return os << "NULL"; } diff --git a/google/cloud/bigtable/value.h b/google/cloud/bigtable/value.h index 4f986dc2dfae2..b097881785990 100644 --- a/google/cloud/bigtable/value.h +++ b/google/cloud/bigtable/value.h @@ -310,6 +310,11 @@ class Value { */ friend std::ostream& operator<<(std::ostream& os, Value const& v); + // `NULL` values are represented by having a kind equal to KIND_NOT_SET + static bool IsNullValue(google::bigtable::v2::Value const& value) { + return value.kind_case() == google::bigtable::v2::Value::KIND_NOT_SET; + } + private: // Metafunction that returns true if `T` is an `absl::optional` template From ec4f063dfeecd16b515bcb8d176a1974516367ba Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Mon, 17 Nov 2025 23:44:49 +0000 Subject: [PATCH 06/12] chore: move to value.cc, add tests --- .../internal/partial_result_set_source.cc | 107 +------ google/cloud/bigtable/value.cc | 107 +++++++ google/cloud/bigtable/value.h | 11 + google/cloud/bigtable/value_test.cc | 263 ++++++++++++++++++ 4 files changed, 383 insertions(+), 105 deletions(-) diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index 30eb61f40d1d0..3659aec7aa581 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -194,110 +194,6 @@ Status PartialResultSetSource::ProcessDataFromStream( return {}; // OK } -/** - * Checks whether the declared type in column matches the value's contents. - * Since the received values may or may not have type() set, we check against - * the value contents themselves - */ -bool TypeAndValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value) { - using google::bigtable::v2::Type; - bool has_matching_value; - switch (type.kind_case()) { - case Type::kArrayType: { - if (!value.has_array_value()) { - has_matching_value = value.has_array_value(); - break; - } - has_matching_value = true; - for (auto const& val : value.array_value().values()) { - if (!TypeAndValuesMatch(type.array_type().element_type(), val)) { - has_matching_value = false; - break; - } - } - break; - } - case Type::kMapType: { - if (!value.has_array_value()) { - has_matching_value = value.has_array_value(); - break; - } - has_matching_value = true; - auto key_type = type.map_type().key_type(); - ; - auto value_type = type.map_type().value_type(); - ; - for (auto const& val : value.array_value().values()) { - if (!val.has_array_value() || val.array_value().values_size() != 2) { - has_matching_value = false; - break; - } - auto map_key = val.array_value().values(0); - auto map_value = val.array_value().values(1); - if (!TypeAndValuesMatch(key_type, map_key) || - !TypeAndValuesMatch(value_type, map_value)) { - has_matching_value = false; - break; - } - } - break; - } - case Type::kStructType: { - if (!value.has_array_value()) { - has_matching_value = value.has_array_value(); - break; - } - has_matching_value = true; - auto fields = type.struct_type().fields(); - auto values = value.array_value().values(); - if (fields.size() != values.size()) { - has_matching_value = value.has_array_value(); - break; - } - for (int i = 0; i < fields.size(); ++i) { - auto const& f1 = fields.Get(i); - auto const& v = values[i]; - if (!TypeAndValuesMatch(f1.type(), v)) { - has_matching_value = false; - break; - } - } - break; - } - case Type::kBoolType: - has_matching_value = value.has_bool_value(); - break; - case Type::kBytesType: - has_matching_value = value.has_bytes_value(); - break; - case Type::kDateType: - has_matching_value = value.has_date_value(); - break; - case Type::kEnumType: - has_matching_value = value.has_int_value(); - break; - case Type::kFloat32Type: - case Type::kFloat64Type: - has_matching_value = value.has_float_value(); - break; - case Type::kInt64Type: - has_matching_value = value.has_int_value(); - break; - case Type::kStringType: - has_matching_value = value.has_string_value(); - break; - case Type::kTimestampType: - has_matching_value = value.has_timestamp_value(); - break; - default: - has_matching_value = false; - break; - } - // Nulls are allowed; - return has_matching_value || bigtable::Value::IsNullValue(value); -} - Status PartialResultSetSource::BufferProtoRows() { if (metadata_.has_value()) { auto const& proto_schema = metadata_->proto_schema(); @@ -322,7 +218,8 @@ Status PartialResultSetSource::BufferProtoRows() { while (parsed_value != proto_values.end()) { for (auto const& column : proto_schema.columns()) { - if (!TypeAndValuesMatch(column.type(), *parsed_value)) { + if (!bigtable::Value::TypeAndValuesMatch(column.type(), + *parsed_value)) { return internal::InternalError("Metadata and Value not matching.", GCP_ERROR_INFO()); } diff --git a/google/cloud/bigtable/value.cc b/google/cloud/bigtable/value.cc index 4dc7b2996e675..490a702f5dd7b 100644 --- a/google/cloud/bigtable/value.cc +++ b/google/cloud/bigtable/value.cc @@ -338,6 +338,113 @@ std::ostream& operator<<(std::ostream& os, Value const& v) { return StreamHelper(os, v.value_, v.type_, StreamMode::kScalar); } +// NOLINTNEXTLINE(misc-no-recursion) +bool Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { + if (!value.has_array_value()) { + return false; + } + for (auto const& val : value.array_value().values()) { + if (!TypeAndValuesMatch(type.array_type().element_type(), val)) { + return false; + } + } + return true; +} + +// NOLINTNEXTLINE(misc-no-recursion) +bool Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { + if (!value.has_array_value()) { + return false; + } + auto key_type = type.map_type().key_type(); + auto value_type = type.map_type().value_type(); + for (auto const& val : value.array_value().values()) { + if (!val.has_array_value() || val.array_value().values_size() != 2) { + return false; + } + auto map_key = val.array_value().values(0); + auto map_value = val.array_value().values(1); + if (!TypeAndValuesMatch(key_type, map_key) || + !TypeAndValuesMatch(value_type, map_value)) { + return false; + } + } + return true; +} + +// NOLINTNEXTLINE(misc-no-recursion) +bool Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { + if (!value.has_array_value()) { + return false; + } + auto fields = type.struct_type().fields(); + auto values = value.array_value().values(); + if (fields.size() != values.size()) { + return false; + } + for (int i = 0; i < fields.size(); ++i) { + auto const& f1 = fields.Get(i); + auto const& v = values[i]; + if (!TypeAndValuesMatch(f1.type(), v)) { + return false; + } + } + return true; +} + +/** + * Checks whether the declared type in column matches the value's contents. + * Since the received values may or may not have type() set, we check against + * the value contents themselves + */ +// NOLINTNEXTLINE(misc-no-recursion) +bool Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { + using google::bigtable::v2::Type; + bool has_matching_value; + switch (type.kind_case()) { + case Type::kArrayType: + return TypeAndArrayValuesMatch(type, value); + case Type::kMapType: + return TypeAndMapValuesMatch(type, value); + case Type::kStructType: + return TypeAndStructValuesMatch(type, value); + case Type::kBoolType: + has_matching_value = value.has_bool_value(); + break; + case Type::kBytesType: + has_matching_value = value.has_bytes_value(); + break; + case Type::kDateType: + has_matching_value = value.has_date_value(); + break; + case Type::kEnumType: + has_matching_value = value.has_int_value(); + break; + case Type::kFloat32Type: + case Type::kFloat64Type: + has_matching_value = value.has_float_value(); + break; + case Type::kInt64Type: + has_matching_value = value.has_int_value(); + break; + case Type::kStringType: + has_matching_value = value.has_string_value(); + break; + case Type::kTimestampType: + has_matching_value = value.has_timestamp_value(); + break; + default: + has_matching_value = false; + break; + } + // Nulls are allowed; + return has_matching_value || bigtable::Value::IsNullValue(value); +} + // // Value::TypeProtoIs // diff --git a/google/cloud/bigtable/value.h b/google/cloud/bigtable/value.h index b097881785990..63927ee401ef3 100644 --- a/google/cloud/bigtable/value.h +++ b/google/cloud/bigtable/value.h @@ -315,7 +315,18 @@ class Value { return value.kind_case() == google::bigtable::v2::Value::KIND_NOT_SET; } + static bool TypeAndValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value); + private: + static bool TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value); + static bool TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value); + static bool TypeAndStructValuesMatch( + google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value); + // Metafunction that returns true if `T` is an `absl::optional` template struct IsOptional : std::false_type {}; diff --git a/google/cloud/bigtable/value_test.cc b/google/cloud/bigtable/value_test.cc index 21d459a404ace..687485c4526d7 100644 --- a/google/cloud/bigtable/value_test.cc +++ b/google/cloud/bigtable/value_test.cc @@ -1747,6 +1747,269 @@ TEST(Value, OutputStreamMatchesT) { // std::unordered_map<...> // Not included, because a raw map cannot be streamed. } +void TestTypeAndValuesMatch(std::string const& type_text, + std::string const& value_text, bool expected) { + google::bigtable::v2::Type type; + ASSERT_TRUE(TextFormat::ParseFromString(type_text, &type)); + google::bigtable::v2::Value value; + ASSERT_TRUE(TextFormat::ParseFromString(value_text, &value)); + EXPECT_EQ(expected, Value::TypeAndValuesMatch(type, value)); +} + +TEST(Value, TypeAndValuesMatchScalar) { + TestTypeAndValuesMatch("int64_type {}", "int_value: 123", true); + TestTypeAndValuesMatch("string_type {}", "string_value: 'hello'", true); + TestTypeAndValuesMatch("bool_type {}", "bool_value: true", true); + TestTypeAndValuesMatch("float64_type {}", "float_value: 3.14", true); + TestTypeAndValuesMatch("float32_type {}", "float_value: 3.14", true); + TestTypeAndValuesMatch("bytes_type {}", "bytes_value: 'bytes'", true); + TestTypeAndValuesMatch("timestamp_type {}", + "timestamp_value: { seconds: 123 }", true); + TestTypeAndValuesMatch("date_type {}", + "date_value: { year: 2025, month: 1, day: 1 }", true); +} + +TEST(Value, TypeAndValuesMatchScalarMismatch) { + TestTypeAndValuesMatch("int64_type {}", "string_value: 'mismatch'", false); + TestTypeAndValuesMatch("string_type {}", "int_value: 123", false); +} + +TEST(Value, TypeAndValuesMatchNullScalar) { + TestTypeAndValuesMatch("int64_type {}", "", true); + TestTypeAndValuesMatch("string_type {}", "", true); +} + +TEST(Value, TypeAndValuesMatchArray) { + auto type = R"pb( + array_type { element_type { int64_type {} } } + )pb"; + auto matching_value = R"pb( + array_value { + values { int_value: 1 } + values { int_value: 2 } + } + )pb"; + TestTypeAndValuesMatch(type, matching_value, true); +} + +TEST(Value, TypeAndValuesMatchArrayMismatchElementType) { + auto type = R"pb( + array_type { element_type { int64_type {} } } + )pb"; + auto mismatched_value = R"pb( + array_value { + values { int_value: 1 } + values { string_value: "2" } + } + )pb"; + TestTypeAndValuesMatch(type, mismatched_value, false); +} + +TEST(Value, TypeAndValuesMatchArrayMismatchScalar) { + auto type = R"pb( + array_type { element_type { int64_type {} } } + )pb"; + TestTypeAndValuesMatch(type, "int_value: 123", false); +} + +TEST(Value, TypeAndValuesMatchArrayWithNull) { + auto type = R"pb( + array_type { element_type { int64_type {} } } + )pb"; + auto value_with_null = R"pb( + array_value { + values { int_value: 1 } + values {} # null + values { int_value: 3 } + } + )pb"; + TestTypeAndValuesMatch(type, value_with_null, true); +} + +TEST(Value, TypeAndValuesMatchStruct) { + auto type = R"pb( + struct_type { + fields { + field_name: "name" + type { string_type {} } + } + fields { + field_name: "age" + type { int64_type {} } + } + } + )pb"; + auto matching_value = R"pb( + array_value { + values { string_value: "John" } + values { int_value: 42 } + } + )pb"; + TestTypeAndValuesMatch(type, matching_value, true); +} + +TEST(Value, TypeAndValuesMatchStructMismatchFieldType) { + auto type = R"pb( + struct_type { + fields { + field_name: "name" + type { string_type {} } + } + fields { + field_name: "age" + type { int64_type {} } + } + } + )pb"; + auto mismatched_value = R"pb( + array_value { + values { string_value: "John" } + values { string_value: "42" } + } + )pb"; + TestTypeAndValuesMatch(type, mismatched_value, false); +} + +TEST(Value, TypeAndValuesMatchStructMismatchFieldCount) { + auto type = R"pb( + struct_type { + fields { type { string_type {} } } + fields { type { int64_type {} } } + } + )pb"; + auto mismatched_value = R"pb( + array_value { values { string_value: "John" } } + )pb"; + // The current implementation has a bug and will return true here. + // This test will fail until the bug is fixed. + TestTypeAndValuesMatch(type, mismatched_value, false); +} + +TEST(Value, TypeAndValuesMatchStructMismatchScalar) { + auto type = R"pb( + struct_type { fields { type { string_type {} } } } + )pb"; + TestTypeAndValuesMatch(type, "string_value: 'John'", false); +} + +TEST(Value, TypeAndValuesMatchStructWithNull) { + auto type = R"pb( + struct_type { + fields { type { string_type {} } } + fields { type { int64_type {} } } + } + )pb"; + auto value_with_null = R"pb( + array_value { + values { string_value: "John" } + values {} + } + )pb"; + TestTypeAndValuesMatch(type, value_with_null, true); +} + +TEST(Value, TypeAndValuesMatchMap) { + auto type = R"pb( + map_type { + key_type { string_type {} } + value_type { int64_type {} } + } + )pb"; + auto matching_value = R"pb( + array_value { + values { + array_value { + values { string_value: "key1" } + values { int_value: 1 } + } + } + } + )pb"; + TestTypeAndValuesMatch(type, matching_value, true); +} + +TEST(Value, TypeAndValuesMatchMapMismatchKeyType) { + auto type = R"pb( + map_type { + key_type { string_type {} } + value_type { int64_type {} } + } + )pb"; + auto mismatched_value = R"pb( + array_value { + values { + array_value { + values { int_value: 1 } + values { int_value: 1 } + } + } + } + )pb"; + TestTypeAndValuesMatch(type, mismatched_value, false); +} + +TEST(Value, TypeAndValuesMatchMapMismatchValueType) { + auto type = R"pb( + map_type { + key_type { string_type {} } + value_type { int64_type {} } + } + )pb"; + auto mismatched_value = R"pb( + array_value { + values { + array_value { + values { string_value: "key1" } + values { string_value: "1" } + } + } + } + )pb"; + TestTypeAndValuesMatch(type, mismatched_value, false); +} + +TEST(Value, TypeAndValuesMatchMapMismatchScalar) { + auto type = R"pb( + map_type { + key_type { string_type {} } + value_type { int64_type {} } + } + )pb"; + TestTypeAndValuesMatch(type, "string_value: 'foo'", false); +} + +TEST(Value, TypeAndValuesMatchMapMalformedEntry) { + auto type = R"pb( + map_type { + key_type { string_type {} } + value_type { int64_type {} } + } + )pb"; + auto malformed_value = R"pb( + array_value { values { array_value { values { string_value: "key1" } } } } + )pb"; + TestTypeAndValuesMatch(type, malformed_value, false); +} + +TEST(Value, TypeAndValuesMatchMapWithNullValue) { + auto type = R"pb( + map_type { + key_type { string_type {} } + value_type { int64_type {} } + } + )pb"; + auto value_with_null = R"pb( + array_value { + values { + array_value { + values { string_value: "key1" } + values {} + } + } + } + )pb"; + TestTypeAndValuesMatch(type, value_with_null, true); +} } // namespace GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END From 370eabd830dbbe2d0a3004927ee9e89fff31b433 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Tue, 18 Nov 2025 04:43:42 +0000 Subject: [PATCH 07/12] chore: clang-tidy fixes --- google/cloud/bigtable/value.cc | 24 ++++++------- google/cloud/bigtable/value_test.cc | 54 ++++++++++++++--------------- 2 files changed, 38 insertions(+), 40 deletions(-) diff --git a/google/cloud/bigtable/value.cc b/google/cloud/bigtable/value.cc index 490a702f5dd7b..3cd8a27d2c7ba 100644 --- a/google/cloud/bigtable/value.cc +++ b/google/cloud/bigtable/value.cc @@ -344,12 +344,11 @@ bool Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, if (!value.has_array_value()) { return false; } - for (auto const& val : value.array_value().values()) { - if (!TypeAndValuesMatch(type.array_type().element_type(), val)) { - return false; - } - } - return true; + auto const& vals = value.array_value().values(); + // NOLINTNEXTLINE(misc-no-recursion) + return std::all_of(vals.begin(), vals.end(), [&](auto const& val) -> bool { + return TypeAndValuesMatch(type.array_type().element_type(), val); + }); } // NOLINTNEXTLINE(misc-no-recursion) @@ -360,18 +359,17 @@ bool Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, } auto key_type = type.map_type().key_type(); auto value_type = type.map_type().value_type(); - for (auto const& val : value.array_value().values()) { + auto const& vals = value.array_value().values(); + // NOLINTNEXTLINE(misc-no-recursion) + return std::all_of(vals.begin(), vals.end(), [&](auto const& val) -> bool { if (!val.has_array_value() || val.array_value().values_size() != 2) { return false; } auto map_key = val.array_value().values(0); auto map_value = val.array_value().values(1); - if (!TypeAndValuesMatch(key_type, map_key) || - !TypeAndValuesMatch(value_type, map_value)) { - return false; - } - } - return true; + return TypeAndValuesMatch(key_type, map_key) && + TypeAndValuesMatch(value_type, map_value); + }); } // NOLINTNEXTLINE(misc-no-recursion) diff --git a/google/cloud/bigtable/value_test.cc b/google/cloud/bigtable/value_test.cc index 687485c4526d7..e17829eac1ef5 100644 --- a/google/cloud/bigtable/value_test.cc +++ b/google/cloud/bigtable/value_test.cc @@ -1780,10 +1780,10 @@ TEST(Value, TypeAndValuesMatchNullScalar) { } TEST(Value, TypeAndValuesMatchArray) { - auto type = R"pb( + auto const* type = R"pb( array_type { element_type { int64_type {} } } )pb"; - auto matching_value = R"pb( + auto const* matching_value = R"pb( array_value { values { int_value: 1 } values { int_value: 2 } @@ -1793,10 +1793,10 @@ TEST(Value, TypeAndValuesMatchArray) { } TEST(Value, TypeAndValuesMatchArrayMismatchElementType) { - auto type = R"pb( + auto const* type = R"pb( array_type { element_type { int64_type {} } } )pb"; - auto mismatched_value = R"pb( + auto const* mismatched_value = R"pb( array_value { values { int_value: 1 } values { string_value: "2" } @@ -1806,17 +1806,17 @@ TEST(Value, TypeAndValuesMatchArrayMismatchElementType) { } TEST(Value, TypeAndValuesMatchArrayMismatchScalar) { - auto type = R"pb( + auto const* type = R"pb( array_type { element_type { int64_type {} } } )pb"; TestTypeAndValuesMatch(type, "int_value: 123", false); } TEST(Value, TypeAndValuesMatchArrayWithNull) { - auto type = R"pb( + auto const* type = R"pb( array_type { element_type { int64_type {} } } )pb"; - auto value_with_null = R"pb( + auto const* value_with_null = R"pb( array_value { values { int_value: 1 } values {} # null @@ -1827,7 +1827,7 @@ TEST(Value, TypeAndValuesMatchArrayWithNull) { } TEST(Value, TypeAndValuesMatchStruct) { - auto type = R"pb( + auto const* type = R"pb( struct_type { fields { field_name: "name" @@ -1839,7 +1839,7 @@ TEST(Value, TypeAndValuesMatchStruct) { } } )pb"; - auto matching_value = R"pb( + auto const* matching_value = R"pb( array_value { values { string_value: "John" } values { int_value: 42 } @@ -1849,7 +1849,7 @@ TEST(Value, TypeAndValuesMatchStruct) { } TEST(Value, TypeAndValuesMatchStructMismatchFieldType) { - auto type = R"pb( + auto const* type = R"pb( struct_type { fields { field_name: "name" @@ -1861,7 +1861,7 @@ TEST(Value, TypeAndValuesMatchStructMismatchFieldType) { } } )pb"; - auto mismatched_value = R"pb( + auto const* mismatched_value = R"pb( array_value { values { string_value: "John" } values { string_value: "42" } @@ -1871,13 +1871,13 @@ TEST(Value, TypeAndValuesMatchStructMismatchFieldType) { } TEST(Value, TypeAndValuesMatchStructMismatchFieldCount) { - auto type = R"pb( + auto const* type = R"pb( struct_type { fields { type { string_type {} } } fields { type { int64_type {} } } } )pb"; - auto mismatched_value = R"pb( + auto const* mismatched_value = R"pb( array_value { values { string_value: "John" } } )pb"; // The current implementation has a bug and will return true here. @@ -1886,20 +1886,20 @@ TEST(Value, TypeAndValuesMatchStructMismatchFieldCount) { } TEST(Value, TypeAndValuesMatchStructMismatchScalar) { - auto type = R"pb( + auto const* type = R"pb( struct_type { fields { type { string_type {} } } } )pb"; TestTypeAndValuesMatch(type, "string_value: 'John'", false); } TEST(Value, TypeAndValuesMatchStructWithNull) { - auto type = R"pb( + auto const* type = R"pb( struct_type { fields { type { string_type {} } } fields { type { int64_type {} } } } )pb"; - auto value_with_null = R"pb( + auto const* value_with_null = R"pb( array_value { values { string_value: "John" } values {} @@ -1909,13 +1909,13 @@ TEST(Value, TypeAndValuesMatchStructWithNull) { } TEST(Value, TypeAndValuesMatchMap) { - auto type = R"pb( + auto const* type = R"pb( map_type { key_type { string_type {} } value_type { int64_type {} } } )pb"; - auto matching_value = R"pb( + auto const* matching_value = R"pb( array_value { values { array_value { @@ -1929,13 +1929,13 @@ TEST(Value, TypeAndValuesMatchMap) { } TEST(Value, TypeAndValuesMatchMapMismatchKeyType) { - auto type = R"pb( + auto const* type = R"pb( map_type { key_type { string_type {} } value_type { int64_type {} } } )pb"; - auto mismatched_value = R"pb( + auto const* mismatched_value = R"pb( array_value { values { array_value { @@ -1949,13 +1949,13 @@ TEST(Value, TypeAndValuesMatchMapMismatchKeyType) { } TEST(Value, TypeAndValuesMatchMapMismatchValueType) { - auto type = R"pb( + auto const* type = R"pb( map_type { key_type { string_type {} } value_type { int64_type {} } } )pb"; - auto mismatched_value = R"pb( + auto const* mismatched_value = R"pb( array_value { values { array_value { @@ -1969,7 +1969,7 @@ TEST(Value, TypeAndValuesMatchMapMismatchValueType) { } TEST(Value, TypeAndValuesMatchMapMismatchScalar) { - auto type = R"pb( + auto const* type = R"pb( map_type { key_type { string_type {} } value_type { int64_type {} } @@ -1979,26 +1979,26 @@ TEST(Value, TypeAndValuesMatchMapMismatchScalar) { } TEST(Value, TypeAndValuesMatchMapMalformedEntry) { - auto type = R"pb( + auto const* type = R"pb( map_type { key_type { string_type {} } value_type { int64_type {} } } )pb"; - auto malformed_value = R"pb( + auto const* malformed_value = R"pb( array_value { values { array_value { values { string_value: "key1" } } } } )pb"; TestTypeAndValuesMatch(type, malformed_value, false); } TEST(Value, TypeAndValuesMatchMapWithNullValue) { - auto type = R"pb( + auto const* type = R"pb( map_type { key_type { string_type {} } value_type { int64_type {} } } )pb"; - auto value_with_null = R"pb( + auto const* value_with_null = R"pb( array_value { values { array_value { From 59224c8f71eeea176c09a84a11b8318fd2e477c3 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Tue, 18 Nov 2025 06:24:12 +0000 Subject: [PATCH 08/12] chore: keep has_matching_value in flow --- google/cloud/bigtable/value.cc | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/google/cloud/bigtable/value.cc b/google/cloud/bigtable/value.cc index 3cd8a27d2c7ba..f5da28936048d 100644 --- a/google/cloud/bigtable/value.cc +++ b/google/cloud/bigtable/value.cc @@ -405,11 +405,14 @@ bool Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type, bool has_matching_value; switch (type.kind_case()) { case Type::kArrayType: - return TypeAndArrayValuesMatch(type, value); + has_matching_value = TypeAndArrayValuesMatch(type, value); + break; case Type::kMapType: - return TypeAndMapValuesMatch(type, value); + has_matching_value = TypeAndMapValuesMatch(type, value); + break; case Type::kStructType: - return TypeAndStructValuesMatch(type, value); + has_matching_value = TypeAndStructValuesMatch(type, value); + break; case Type::kBoolType: has_matching_value = value.has_bool_value(); break; @@ -440,7 +443,7 @@ bool Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type, break; } // Nulls are allowed; - return has_matching_value || bigtable::Value::IsNullValue(value); + return has_matching_value || IsNullValue(value); } // From 286c9d8a873ef5ec863ba07ff0135ff1b9e41c30 Mon Sep 17 00:00:00 2001 From: Diego Marquez Date: Tue, 18 Nov 2025 01:58:02 -0500 Subject: [PATCH 09/12] Modify skipped tests for ExecuteQuery in Bazel script Updated the list of skipped tests in run_conformance_tests_proxy_bazel.sh. --- google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh b/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh index dead77c664937..02aec44ef023f 100755 --- a/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh +++ b/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh @@ -51,8 +51,7 @@ exit_status=$? # CloseClient go test -v \ -run "TestExecuteQuery|TestExecuteQuery_PlanRefresh$|TestExecuteQuery_PlanRefresh_WithMetadataChange|TestExecuteQuery_PlanRefresh_Retries|TestExecuteQuery_PlanRefresh_RecoversAfterPermanentError" \ - -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|FailsOnStructMissingField|RetryTest_WithPlanRefresh|PlanRefresh" \ - + -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|ArraytFailsOnStructMissingField|TestExecuteQuery_PlanRefresh_AfterResumeTokenCausesError|TestExecuteQuery_RetryTest_WithPlanRefresh|TestExecuteQuery_PlanRefresh_RespectsDeadline" \ -proxy_addr=:9999 exit_status=$? From 48ead5426284b8b106b49e9256fd708082499cba Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Tue, 18 Nov 2025 17:09:58 +0000 Subject: [PATCH 10/12] chore: adapt to merge --- .../bigtable/ci/run_conformance_tests_proxy_bazel.sh | 12 ++++++------ .../cloud/bigtable/internal/data_connection_impl.cc | 9 +++++++++ .../bigtable/internal/partial_result_set_source.cc | 5 ++++- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh b/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh index 02aec44ef023f..cd1eb50c43418 100755 --- a/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh +++ b/google/cloud/bigtable/ci/run_conformance_tests_proxy_bazel.sh @@ -50,8 +50,8 @@ exit_status=$? # Run all the ExecuteQuery tests that either work or we plan to skip such as # CloseClient go test -v \ - -run "TestExecuteQuery|TestExecuteQuery_PlanRefresh$|TestExecuteQuery_PlanRefresh_WithMetadataChange|TestExecuteQuery_PlanRefresh_Retries|TestExecuteQuery_PlanRefresh_RecoversAfterPermanentError" \ - -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|ArraytFailsOnStructMissingField|TestExecuteQuery_PlanRefresh_AfterResumeTokenCausesError|TestExecuteQuery_RetryTest_WithPlanRefresh|TestExecuteQuery_PlanRefresh_RespectsDeadline" \ + -run "TestExecuteQuery|TestExecuteQuery_PlanRefresh$|TestExecuteQuery_PlanRefresh_WithMetadataChange|TestExecuteQuery_PlanRefresh_Retries|TestExecuteQuery_FailsOnSuccesfulStreamWithNoToken" \ + -skip "CloseClient|FailsOnEmptyMetadata|FailsOnExecuteQueryMetadata|FailsOnInvalidType|FailsOnNotEnoughData|FailsOnNotEnoughDataWithCompleteRows|FailsOnSuccesfulStreamWithNoToken|ChecksumMismatch|FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField|TestExecuteQuery_PlanRefresh_AfterResumeTokenCausesError|TestExecuteQuery_RetryTest_WithPlanRefresh|TestExecuteQuery_PlanRefresh_RespectsDeadline|TestExecuteQuery_PlanRefresh_RecoversAfterPermanentError" \ -proxy_addr=:9999 exit_status=$? @@ -72,10 +72,10 @@ exit_status=$? #exit_status=$? # Response/Metadata mismatches b/461233335 -#go test -v \ -# -run "FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField" \ -# -proxy_addr=:9999 -#exit_status=$? +go test -v \ + -run "FailsOnTypeMismatch|FailsOnTypeMismatchWithinMap|FailsOnTypeMismatchWithinArray|FailsOnTypeMismatchWithinStruct|FailsOnStructMissingField" \ + -proxy_addr=:9999 +exit_status=$? # QueryPlan refresh tests b/461233613 #go test -v \ diff --git a/google/cloud/bigtable/internal/data_connection_impl.cc b/google/cloud/bigtable/internal/data_connection_impl.cc index 7cdc49d753ded..c2678799c3264 100644 --- a/google/cloud/bigtable/internal/data_connection_impl.cc +++ b/google/cloud/bigtable/internal/data_connection_impl.cc @@ -111,6 +111,10 @@ bool IsStatusMetadataIndicatingRetryPolicyExhausted(Status const& status) { "retry-policy-exhausted")); } +bool IsStatusIndicatingInternalError(Status const& status) { + return status.code() == StatusCode::kInternal; +} + class DefaultPartialResultSetReader : public bigtable_internal::PartialResultSetReader { public: @@ -962,6 +966,11 @@ bigtable::RowStream DataConnectionImpl::ExecuteQuery( } last_status = source.status(); + if (IsStatusIndicatingInternalError(source.status())) { + return bigtable::RowStream(std::make_unique( + std::move(last_status))); + } + if (QueryPlanRefreshRetry::IsQueryPlanExpired(source.status())) { query_plan->Invalidate(source.status(), query_plan_data->prepared_query()); diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index 3659aec7aa581..6ac9fc05c6b51 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -52,7 +52,9 @@ PartialResultSetSource::Create( auto status = source->ReadFromStream(); // Any error during parsing will be returned. - if (!status.ok()) return status; + if (!status.ok()) { + return status; + } return {std::move(source)}; } @@ -220,6 +222,7 @@ Status PartialResultSetSource::BufferProtoRows() { for (auto const& column : proto_schema.columns()) { if (!bigtable::Value::TypeAndValuesMatch(column.type(), *parsed_value)) { + std::cout << "Metadata and Value not matching." << std::endl; return internal::InternalError("Metadata and Value not matching.", GCP_ERROR_INFO()); } From b87f64f9d415d4fecbacf7dd910858dbb51171ce Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Tue, 18 Nov 2025 17:45:56 +0000 Subject: [PATCH 11/12] chore: use custom error messages --- .../internal/partial_result_set_source.cc | 9 +- google/cloud/bigtable/value.cc | 111 ++++++++++++------ google/cloud/bigtable/value.h | 8 +- google/cloud/bigtable/value_test.cc | 8 +- 4 files changed, 89 insertions(+), 47 deletions(-) diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index 3c6db694e2820..9058100ad4cf1 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -228,11 +228,10 @@ Status PartialResultSetSource::BufferProtoRows() { while (parsed_value != proto_values.end()) { for (auto const& column : proto_schema.columns()) { - if (!bigtable::Value::TypeAndValuesMatch(column.type(), - *parsed_value)) { - std::cout << "Metadata and Value not matching." << std::endl; - return internal::InternalError("Metadata and Value not matching.", - GCP_ERROR_INFO()); + auto type_value_match_result = bigtable::Value::TypeAndValuesMatch(column.type(), + *parsed_value); + if (!type_value_match_result.ok()) { + return type_value_match_result; } auto value = FromProto(column.type(), *parsed_value); values.push_back(std::move(value)); diff --git a/google/cloud/bigtable/value.cc b/google/cloud/bigtable/value.cc index f5da28936048d..8b8eb787b6b07 100644 --- a/google/cloud/bigtable/value.cc +++ b/google/cloud/bigtable/value.cc @@ -17,6 +17,7 @@ #include "google/cloud/internal/throw_delegate.h" #include "absl/container/flat_hash_set.h" #include "absl/strings/cord.h" +#include "absl/strings/substitute.h" #include #include #include @@ -339,58 +340,71 @@ std::ostream& operator<<(std::ostream& os, Value const& v) { } // NOLINTNEXTLINE(misc-no-recursion) -bool Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, +Status Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value) { if (!value.has_array_value()) { - return false; + return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: MAP"); } auto const& vals = value.array_value().values(); - // NOLINTNEXTLINE(misc-no-recursion) - return std::all_of(vals.begin(), vals.end(), [&](auto const& val) -> bool { - return TypeAndValuesMatch(type.array_type().element_type(), val); - }); + for (auto const& val : vals) { + auto const element_match_result = TypeAndValuesMatch(type.array_type().element_type(), val); + if (!element_match_result.ok()) { + return element_match_result; + } + } + return Status{}; } // NOLINTNEXTLINE(misc-no-recursion) -bool Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, +Status Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value) { if (!value.has_array_value()) { - return false; + return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: MAP"); } auto key_type = type.map_type().key_type(); auto value_type = type.map_type().value_type(); auto const& vals = value.array_value().values(); - // NOLINTNEXTLINE(misc-no-recursion) - return std::all_of(vals.begin(), vals.end(), [&](auto const& val) -> bool { + for (auto const& val : vals) { if (!val.has_array_value() || val.array_value().values_size() != 2) { - return false; + return internal::InternalError("ARRAY_VALUE must contain entries of 2 values"); } auto map_key = val.array_value().values(0); auto map_value = val.array_value().values(1); - return TypeAndValuesMatch(key_type, map_key) && - TypeAndValuesMatch(value_type, map_value); - }); + // NOLINTNEXTLINE(misc-no-recursion) + auto key_match_result = TypeAndValuesMatch(key_type, map_key); + if (!key_match_result.ok()) { + return key_match_result; + } + // NOLINTNEXTLINE(misc-no-recursion) + auto value_match_result = TypeAndValuesMatch(value_type, map_value); + if (!value_match_result.ok()) { + return value_match_result; + } + } + return Status{}; } // NOLINTNEXTLINE(misc-no-recursion) -bool Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type, +Status Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value) { if (!value.has_array_value()) { - return false; + return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: STRUCT"); } auto fields = type.struct_type().fields(); auto values = value.array_value().values(); if (fields.size() != values.size()) { - return false; + auto const message = absl::Substitute("received Struct with $0 values, but metadata has $1 fields", values.size(), fields.size()); + return internal::InternalError(message); } for (int i = 0; i < fields.size(); ++i) { auto const& f1 = fields.Get(i); auto const& v = values[i]; - if (!TypeAndValuesMatch(f1.type(), v)) { - return false; + auto match_result = TypeAndValuesMatch(f1.type(), v); + if (!match_result.ok()) { + return match_result; } } - return true; + return Status{}; } /** @@ -399,51 +413,74 @@ bool Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type, * the value contents themselves */ // NOLINTNEXTLINE(misc-no-recursion) -bool Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type, +Status Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value) { using google::bigtable::v2::Type; - bool has_matching_value; + auto make_mismatch_metadata_status = [&](std::string const& value_kind, std::string const& type_name) { + auto const message = absl::Substitute("Value kind must be $0 for columns of type: $1", value_kind, type_name); + return internal::InternalError(message); + }; + // Null values are allowed by default + if (IsNullValue(value)) { + return Status{}; + } + Status result; switch (type.kind_case()) { case Type::kArrayType: - has_matching_value = TypeAndArrayValuesMatch(type, value); + result = TypeAndArrayValuesMatch(type, value); break; case Type::kMapType: - has_matching_value = TypeAndMapValuesMatch(type, value); + result = TypeAndMapValuesMatch(type, value); break; case Type::kStructType: - has_matching_value = TypeAndStructValuesMatch(type, value); + result = TypeAndStructValuesMatch(type, value); break; case Type::kBoolType: - has_matching_value = value.has_bool_value(); + if (!value.has_bool_value()) { + result = make_mismatch_metadata_status("BOOL_VALUE", "BOOL"); + } break; case Type::kBytesType: - has_matching_value = value.has_bytes_value(); + if (!value.has_bytes_value()) { + result = make_mismatch_metadata_status("BYTES_VALUE", "BYTES"); + } break; case Type::kDateType: - has_matching_value = value.has_date_value(); - break; - case Type::kEnumType: - has_matching_value = value.has_int_value(); + if (!value.has_date_value()) { + result = make_mismatch_metadata_status("DATE_VALUE", "DATE"); + } break; case Type::kFloat32Type: + if (!value.has_float_value()) { + result = make_mismatch_metadata_status("FLOAT_VALUE", "FLOAT32"); + } + break; case Type::kFloat64Type: - has_matching_value = value.has_float_value(); + if (!value.has_float_value()) { + result = make_mismatch_metadata_status("FLOAT_VALUE", "FLOAT64"); + } break; case Type::kInt64Type: - has_matching_value = value.has_int_value(); + if (!value.has_int_value()) { + result = make_mismatch_metadata_status("INT_VALUE", "INT64"); + } break; case Type::kStringType: - has_matching_value = value.has_string_value(); + if (!value.has_string_value()) { + result = make_mismatch_metadata_status("STRING_VALUE", "STRING"); + } break; case Type::kTimestampType: - has_matching_value = value.has_timestamp_value(); + if (!value.has_timestamp_value()) { + result = make_mismatch_metadata_status("TIMESTAMP_VALUE", "TIMESTAMP"); + } break; default: - has_matching_value = false; + result = internal::InternalError("Unsupported type"); break; } // Nulls are allowed; - return has_matching_value || IsNullValue(value); + return result; } // diff --git a/google/cloud/bigtable/value.h b/google/cloud/bigtable/value.h index 63927ee401ef3..d0c50bccc8372 100644 --- a/google/cloud/bigtable/value.h +++ b/google/cloud/bigtable/value.h @@ -315,15 +315,15 @@ class Value { return value.kind_case() == google::bigtable::v2::Value::KIND_NOT_SET; } - static bool TypeAndValuesMatch(google::bigtable::v2::Type const& type, + static Status TypeAndValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value); private: - static bool TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, + static Status TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value); - static bool TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, + static Status TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value); - static bool TypeAndStructValuesMatch( + static Status TypeAndStructValuesMatch( google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value); diff --git a/google/cloud/bigtable/value_test.cc b/google/cloud/bigtable/value_test.cc index e17829eac1ef5..6da6a52d510dd 100644 --- a/google/cloud/bigtable/value_test.cc +++ b/google/cloud/bigtable/value_test.cc @@ -1747,13 +1747,19 @@ TEST(Value, OutputStreamMatchesT) { // std::unordered_map<...> // Not included, because a raw map cannot be streamed. } + void TestTypeAndValuesMatch(std::string const& type_text, std::string const& value_text, bool expected) { google::bigtable::v2::Type type; ASSERT_TRUE(TextFormat::ParseFromString(type_text, &type)); google::bigtable::v2::Value value; ASSERT_TRUE(TextFormat::ParseFromString(value_text, &value)); - EXPECT_EQ(expected, Value::TypeAndValuesMatch(type, value)); + auto result = Value::TypeAndValuesMatch(type, value); + if (expected) { + EXPECT_STATUS_OK(result); + } else { + EXPECT_THAT(result, Not(IsOk())); + } } TEST(Value, TypeAndValuesMatchScalar) { From 39580cac0f9344e9069e2f0258e532e81d7c0703 Mon Sep 17 00:00:00 2001 From: diegomarquezp Date: Tue, 18 Nov 2025 17:54:22 +0000 Subject: [PATCH 12/12] chore: lint --- .../internal/partial_result_set_source.cc | 4 +- google/cloud/bigtable/value.cc | 39 ++++++++++++------- google/cloud/bigtable/value.h | 9 +++-- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/google/cloud/bigtable/internal/partial_result_set_source.cc b/google/cloud/bigtable/internal/partial_result_set_source.cc index d99c29428f6ab..991ef44a9407e 100644 --- a/google/cloud/bigtable/internal/partial_result_set_source.cc +++ b/google/cloud/bigtable/internal/partial_result_set_source.cc @@ -229,8 +229,8 @@ Status PartialResultSetSource::BufferProtoRows() { while (parsed_value != proto_values.end()) { for (auto const& column : proto_schema.columns()) { - auto type_value_match_result = bigtable::Value::TypeAndValuesMatch(column.type(), - *parsed_value); + auto type_value_match_result = + bigtable::Value::TypeAndValuesMatch(column.type(), *parsed_value); if (!type_value_match_result.ok()) { return type_value_match_result; } diff --git a/google/cloud/bigtable/value.cc b/google/cloud/bigtable/value.cc index 8b8eb787b6b07..9b1ddf95d914e 100644 --- a/google/cloud/bigtable/value.cc +++ b/google/cloud/bigtable/value.cc @@ -340,14 +340,17 @@ std::ostream& operator<<(std::ostream& os, Value const& v) { } // NOLINTNEXTLINE(misc-no-recursion) -Status Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value) { +Status Value::TypeAndArrayValuesMatch( + google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { if (!value.has_array_value()) { - return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: MAP"); + return internal::InternalError( + "Value kind must be ARRAY_VALUE for columns of type: MAP"); } auto const& vals = value.array_value().values(); for (auto const& val : vals) { - auto const element_match_result = TypeAndValuesMatch(type.array_type().element_type(), val); + auto const element_match_result = + TypeAndValuesMatch(type.array_type().element_type(), val); if (!element_match_result.ok()) { return element_match_result; } @@ -357,16 +360,18 @@ Status Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, // NOLINTNEXTLINE(misc-no-recursion) Status Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value) { + google::bigtable::v2::Value const& value) { if (!value.has_array_value()) { - return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: MAP"); + return internal::InternalError( + "Value kind must be ARRAY_VALUE for columns of type: MAP"); } auto key_type = type.map_type().key_type(); auto value_type = type.map_type().value_type(); auto const& vals = value.array_value().values(); for (auto const& val : vals) { if (!val.has_array_value() || val.array_value().values_size() != 2) { - return internal::InternalError("ARRAY_VALUE must contain entries of 2 values"); + return internal::InternalError( + "ARRAY_VALUE must contain entries of 2 values"); } auto map_key = val.array_value().values(0); auto map_value = val.array_value().values(1); @@ -385,15 +390,19 @@ Status Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, } // NOLINTNEXTLINE(misc-no-recursion) -Status Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value) { +Status Value::TypeAndStructValuesMatch( + google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value) { if (!value.has_array_value()) { - return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: STRUCT"); + return internal::InternalError( + "Value kind must be ARRAY_VALUE for columns of type: STRUCT"); } auto fields = type.struct_type().fields(); auto values = value.array_value().values(); if (fields.size() != values.size()) { - auto const message = absl::Substitute("received Struct with $0 values, but metadata has $1 fields", values.size(), fields.size()); + auto const message = absl::Substitute( + "received Struct with $0 values, but metadata has $1 fields", + values.size(), fields.size()); return internal::InternalError(message); } for (int i = 0; i < fields.size(); ++i) { @@ -414,10 +423,12 @@ Status Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type, */ // NOLINTNEXTLINE(misc-no-recursion) Status Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value) { + google::bigtable::v2::Value const& value) { using google::bigtable::v2::Type; - auto make_mismatch_metadata_status = [&](std::string const& value_kind, std::string const& type_name) { - auto const message = absl::Substitute("Value kind must be $0 for columns of type: $1", value_kind, type_name); + auto make_mismatch_metadata_status = [&](std::string const& value_kind, + std::string const& type_name) { + auto const message = absl::Substitute( + "Value kind must be $0 for columns of type: $1", value_kind, type_name); return internal::InternalError(message); }; // Null values are allowed by default diff --git a/google/cloud/bigtable/value.h b/google/cloud/bigtable/value.h index d0c50bccc8372..d7c16679645e7 100644 --- a/google/cloud/bigtable/value.h +++ b/google/cloud/bigtable/value.h @@ -316,13 +316,14 @@ class Value { } static Status TypeAndValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value); + google::bigtable::v2::Value const& value); private: - static Status TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value); + static Status TypeAndArrayValuesMatch( + google::bigtable::v2::Type const& type, + google::bigtable::v2::Value const& value); static Status TypeAndMapValuesMatch(google::bigtable::v2::Type const& type, - google::bigtable::v2::Value const& value); + google::bigtable::v2::Value const& value); static Status TypeAndStructValuesMatch( google::bigtable::v2::Type const& type, google::bigtable::v2::Value const& value);