Skip to content

Commit eefa27b

Browse files
diegomarquezpdiegomarquezp
committed
chore: check at value level
1 parent 445d3e9 commit eefa27b

File tree

6 files changed

+172
-111
lines changed

6 files changed

+172
-111
lines changed

‎google/cloud/bigtable/internal/data_connection_impl_test.cc‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -226,26 +226,23 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response,
226226
absl::optional<std::string> resume_token, bool reset) {
227227
google::bigtable::v2::ProtoRows proto_rows;
228228
for (auto const& v : values) {
229-
google::bigtable::v2::Value value;
230-
value.set_string_value(v);
231-
*(*value.mutable_type()).mutable_string_type() =
232-
std::move(google::bigtable::v2::Type_String{});
233-
*proto_rows.add_values() = std::move(value);
229+
proto_rows.add_values()->set_string_value(v);
234230
}
231+
std::string binary_batch_data = proto_rows.SerializeAsString();
235232
auto text = absl::Substitute(
236233
R"pb(
237-
proto_rows_batch: {},
238-
$0 $1
234+
proto_rows_batch: {
235+
batch_data: "$0",
236+
},
237+
$1 $2
239238
estimated_batch_size: 31,
240239
batch_checksum: 123456
241240
)pb",
241+
binary_batch_data,
242242
resume_token ? absl::Substitute(R"(resume_token: "$0")", *resume_token)
243243
: "",
244244
reset ? "reset: true," : "");
245245
ASSERT_TRUE(google::protobuf::TextFormat::ParseFromString(text, &response));
246-
std::string binary_batch_data = proto_rows.SerializeAsString();
247-
*(*response.mutable_proto_rows_batch()).mutable_batch_data() =
248-
binary_batch_data;
249246
}
250247

251248
// For tests that need to manually interact with the client/server metadata in a

‎google/cloud/bigtable/internal/partial_result_set_resume_test.cc‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -79,26 +79,23 @@ void MakeResponse(google::bigtable::v2::PartialResultSet& response,
7979
absl::optional<std::string> resume_token, bool reset) {
8080
google::bigtable::v2::ProtoRows proto_rows;
8181
for (auto const& v : values) {
82-
google::bigtable::v2::Value value;
83-
value.set_string_value(v);
84-
*(*value.mutable_type()).mutable_string_type() =
85-
std::move(google::bigtable::v2::Type_String{});
86-
*proto_rows.add_values() = std::move(value);
82+
proto_rows.add_values()->set_string_value(v);
8783
}
84+
std::string binary_batch_data = proto_rows.SerializeAsString();
8885
auto text = absl::Substitute(
8986
R"pb(
90-
proto_rows_batch: {},
91-
$0 $1
87+
proto_rows_batch: {
88+
batch_data: "$0",
89+
},
90+
$1 $2
9291
estimated_batch_size: 31,
9392
batch_checksum: 123456
9493
)pb",
94+
binary_batch_data,
9595
resume_token ? absl::Substitute(R"(resume_token: "$0")", *resume_token)
9696
: "",
9797
reset ? "reset: true," : "");
9898
ASSERT_TRUE(TextFormat::ParseFromString(text, &response));
99-
std::string binary_batch_data = proto_rows.SerializeAsString();
100-
*(*response.mutable_proto_rows_batch()).mutable_batch_data() =
101-
binary_batch_data;
10299
}
103100

104101
// Helper function for MockPartialResultSetReader::Read to return true and

‎google/cloud/bigtable/internal/partial_result_set_source.cc‎

Lines changed: 103 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -194,10 +194,108 @@ Status PartialResultSetSource::ProcessDataFromStream(
194194
return {}; // OK
195195
}
196196

197-
bool ColumnAndValueTypesMatch(
198-
google::bigtable::v2::ColumnMetadata const& column,
199-
google::bigtable::v2::Value const& value) {
200-
return value.type().SerializeAsString() == column.type().SerializeAsString();
197+
/**
198+
* Checks whether the declared type in column matches the value's contents.
199+
* Since the received values may or may not have type() set, we check against
200+
* the value contents themselves
201+
*/
202+
bool TypeAndValuesMatch(google::bigtable::v2::Type const& type,
203+
google::bigtable::v2::Value const& value) {
204+
using google::bigtable::v2::Type;
205+
bool has_matching_value;
206+
switch (type.kind_case()) {
207+
case Type::kArrayType: {
208+
if (!value.has_array_value()) {
209+
has_matching_value = value.has_array_value();
210+
break;
211+
}
212+
has_matching_value = true;
213+
for (auto const& val : value.array_value().values()) {
214+
if (!TypeAndValuesMatch(type.array_type().element_type(), val)) {
215+
has_matching_value = false;
216+
break;
217+
}
218+
}
219+
break;
220+
}
221+
case Type::kMapType: {
222+
if (!value.has_array_value()) {
223+
has_matching_value = value.has_array_value();
224+
break;
225+
}
226+
has_matching_value = true;
227+
auto key_type = type.map_type().key_type();
228+
;
229+
auto value_type = type.map_type().value_type();
230+
;
231+
for (auto const& val : value.array_value().values()) {
232+
if (!val.has_array_value() || val.array_value().values_size() != 2) {
233+
has_matching_value = false;
234+
break;
235+
}
236+
auto map_key = val.array_value().values(0);
237+
auto map_value = val.array_value().values(1);
238+
if (!TypeAndValuesMatch(key_type, map_key) ||
239+
!TypeAndValuesMatch(value_type, map_value)) {
240+
has_matching_value = false;
241+
break;
242+
}
243+
}
244+
break;
245+
}
246+
case Type::kStructType: {
247+
if (!value.has_array_value()) {
248+
has_matching_value = value.has_array_value();
249+
break;
250+
}
251+
has_matching_value = true;
252+
auto fields = type.struct_type().fields();
253+
auto values = value.array_value().values();
254+
if (fields.size() != values.size()) {
255+
has_matching_value = value.has_array_value();
256+
break;
257+
}
258+
for (int i = 0; i < fields.size(); ++i) {
259+
auto const& f1 = fields.Get(i);
260+
auto const& v = values[i];
261+
if (!TypeAndValuesMatch(f1.type(), v)) {
262+
has_matching_value = false;
263+
break;
264+
}
265+
}
266+
break;
267+
}
268+
case Type::kBoolType:
269+
has_matching_value = value.has_bool_value();
270+
break;
271+
case Type::kBytesType:
272+
has_matching_value = value.has_bytes_value();
273+
break;
274+
case Type::kDateType:
275+
has_matching_value = value.has_date_value();
276+
break;
277+
case Type::kEnumType:
278+
has_matching_value = value.has_int_value();
279+
break;
280+
case Type::kFloat32Type:
281+
case Type::kFloat64Type:
282+
has_matching_value = value.has_float_value();
283+
break;
284+
case Type::kInt64Type:
285+
has_matching_value = value.has_int_value();
286+
break;
287+
case Type::kStringType:
288+
has_matching_value = value.has_string_value();
289+
break;
290+
case Type::kTimestampType:
291+
has_matching_value = value.has_timestamp_value();
292+
break;
293+
default:
294+
has_matching_value = false;
295+
break;
296+
}
297+
// Nulls are allowed;
298+
return has_matching_value || bigtable::Value::IsNullValue(value);
201299
}
202300

203301
Status PartialResultSetSource::BufferProtoRows() {
@@ -224,7 +322,7 @@ Status PartialResultSetSource::BufferProtoRows() {
224322

225323
while (parsed_value != proto_values.end()) {
226324
for (auto const& column : proto_schema.columns()) {
227-
if (!ColumnAndValueTypesMatch(column, *parsed_value)) {
325+
if (!TypeAndValuesMatch(column.type(), *parsed_value)) {
228326
return internal::InternalError("Metadata and Value not matching.",
229327
GCP_ERROR_INFO());
230328
}

‎google/cloud/bigtable/internal/partial_result_set_source_test.cc‎

Lines changed: 49 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -265,33 +265,27 @@ TEST(PartialResultSetSourceTest, SingleResponse) {
265265
google::bigtable::v2::ResultSetMetadata metadata;
266266
ASSERT_TRUE(TextFormat::ParseFromString(kResultMetadataText, &metadata));
267267
auto constexpr kProtoRowsText = R"pb(
268-
values {
269-
string_value: "r1"
270-
type { string_type {} }
271-
}
272-
values {
273-
string_value: "f1"
274-
type { string_type {} }
275-
}
276-
values {
277-
string_value: "q1"
278-
type { string_type {} }
279-
}
268+
values { string_value: "r1" }
269+
values { string_value: "f1" }
270+
values { string_value: "q1" }
280271
)pb";
281272
google::bigtable::v2::ProtoRows proto_rows;
282273
ASSERT_TRUE(TextFormat::ParseFromString(kProtoRowsText, &proto_rows));
283274

284-
std::string partial_result_set_text = R"pb(
285-
proto_rows_batch: {},
286-
resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=",
287-
reset: true,
288-
estimated_batch_size: 31,
289-
batch_checksum: 123456
290-
)pb";
275+
std::string binary_batch_data = proto_rows.SerializeAsString();
276+
std::string partial_result_set_text =
277+
absl::Substitute(R"pb(
278+
proto_rows_batch: {
279+
batch_data: "$0",
280+
},
281+
resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=",
282+
reset: true,
283+
estimated_batch_size: 31,
284+
batch_checksum: 123456
285+
)pb",
286+
binary_batch_data);
291287
google::bigtable::v2::PartialResultSet response;
292288
ASSERT_TRUE(TextFormat::ParseFromString(partial_result_set_text, &response));
293-
*(*response.mutable_proto_rows_batch()).mutable_batch_data() =
294-
proto_rows.SerializeAsString();
295289

296290
auto grpc_reader =
297291
std::make_unique<bigtable_testing::MockPartialResultSetReader>();
@@ -370,65 +364,41 @@ TEST(PartialResultSetSourceTest, MultipleResponses) {
370364

371365
std::array<char const*, 3> proto_rows_text{{
372366
R"pb(
373-
values {
374-
string_value: "a1"
375-
type { string_type {} }
376-
}
377-
values {
378-
string_value: "a2"
379-
type { string_type {} }
380-
}
381-
values {
382-
string_value: "a3"
383-
type { string_type {} }
384-
}
367+
values { string_value: "a1" }
368+
values { string_value: "a2" }
369+
values { string_value: "a3" }
385370
)pb",
386371
R"pb(
387-
values {
388-
string_value: "b1"
389-
type { string_type {} }
390-
}
391-
values {
392-
string_value: "b2"
393-
type { string_type {} }
394-
}
395-
values {
396-
string_value: "b3"
397-
type { string_type {} }
398-
}
372+
values { string_value: "b1" }
373+
values { string_value: "b2" }
374+
values { string_value: "b3" }
399375
)pb",
400376
R"pb(
401-
values {
402-
string_value: "c1"
403-
type { string_type {} }
404-
}
405-
values {
406-
string_value: "c2"
407-
type { string_type {} }
408-
}
409-
values {
410-
string_value: "c3"
411-
type { string_type {} }
412-
}
377+
values { string_value: "c1" }
378+
values { string_value: "c2" }
379+
values { string_value: "c3" }
413380
)pb",
414381
}};
415382

416383
std::vector<google::bigtable::v2::PartialResultSet> responses;
417384
for (auto const* text : proto_rows_text) {
418385
google::bigtable::v2::ProtoRows proto_rows;
419386
ASSERT_TRUE(TextFormat::ParseFromString(text, &proto_rows));
420-
std::string partial_result_set_text = R"pb(
421-
proto_rows_batch: {},
422-
resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=",
423-
reset: true,
424-
estimated_batch_size: 31,
425-
batch_checksum: 123456
426-
)pb";
387+
std::string binary_batch_data = proto_rows.SerializeAsString();
388+
std::string partial_result_set_text = absl::Substitute(
389+
R"pb(
390+
proto_rows_batch: {
391+
batch_data: "$0",
392+
},
393+
resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=",
394+
reset: true,
395+
estimated_batch_size: 31,
396+
batch_checksum: 123456
397+
)pb",
398+
binary_batch_data);
427399
google::bigtable::v2::PartialResultSet response;
428400
ASSERT_TRUE(
429401
TextFormat::ParseFromString(partial_result_set_text, &response));
430-
*(*response.mutable_proto_rows_batch()).mutable_batch_data() =
431-
proto_rows.SerializeAsString();
432402
responses.push_back(std::move(response));
433403
}
434404

@@ -525,10 +495,7 @@ TEST(PartialResultSetSourceTest, ResponseWithNoValues) {
525495

526496
std::array<char const*, 3> proto_rows_text{{
527497
R"pb(
528-
values {
529-
string_value: "a1"
530-
type { string_type {} }
531-
}
498+
values { string_value: "a1" }
532499
)pb",
533500
R"pb(
534501
)pb",
@@ -539,18 +506,21 @@ TEST(PartialResultSetSourceTest, ResponseWithNoValues) {
539506
for (auto const* text : proto_rows_text) {
540507
google::bigtable::v2::ProtoRows proto_rows;
541508
ASSERT_TRUE(TextFormat::ParseFromString(text, &proto_rows));
542-
std::string partial_result_set_text = R"pb(
543-
proto_rows_batch: {},
544-
resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=",
545-
reset: true,
546-
estimated_batch_size: 31,
547-
batch_checksum: 123456
548-
)pb";
509+
std::string binary_batch_data = proto_rows.SerializeAsString();
510+
std::string partial_result_set_text = absl::Substitute(
511+
R"pb(
512+
proto_rows_batch: {
513+
batch_data: "$0",
514+
},
515+
resume_token: "AAAAAWVyZXN1bWVfdG9rZW4=",
516+
reset: true,
517+
estimated_batch_size: 31,
518+
batch_checksum: 123456
519+
)pb",
520+
binary_batch_data);
549521
google::bigtable::v2::PartialResultSet response;
550522
ASSERT_TRUE(
551523
TextFormat::ParseFromString(partial_result_set_text, &response));
552-
*(*response.mutable_proto_rows_batch()).mutable_batch_data() =
553-
proto_rows.SerializeAsString();
554524
responses.push_back(std::move(response));
555525
}
556526

‎google/cloud/bigtable/value.cc‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -207,12 +207,6 @@ bool MapEqual( // NOLINT(misc-no-recursion)
207207
comparison_function);
208208
}
209209

210-
// From the proto description, `NULL` values are represented by having a kind
211-
// equal to KIND_NOT_SET
212-
bool IsNullValue(google::bigtable::v2::Value const& value) {
213-
return value.kind_case() == google::bigtable::v2::Value::KIND_NOT_SET;
214-
}
215-
216210
// A helper to escape all double quotes in the given string `s`. For example,
217211
// if given `"foo"`, outputs `\"foo\"`. This is useful when a caller needs to
218212
// wrap `s` itself in double quotes.
@@ -238,7 +232,7 @@ std::ostream& StreamHelper(std::ostream& os, // NOLINT(misc-no-recursion)
238232
google::bigtable::v2::Value const& v,
239233
google::bigtable::v2::Type const& t,
240234
StreamMode mode) {
241-
if (IsNullValue(v)) {
235+
if (Value::IsNullValue(v)) {
242236
return os << "NULL";
243237
}
244238

0 commit comments

Comments
 (0)