Skip to content

Commit b87f64f

Browse files
diegomarquezpdiegomarquezp
committed
chore: use custom error messages
1 parent 6ed9f32 commit b87f64f

File tree

4 files changed

+89
-47
lines changed

4 files changed

+89
-47
lines changed

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -228,11 +228,10 @@ Status PartialResultSetSource::BufferProtoRows() {
228228

229229
while (parsed_value != proto_values.end()) {
230230
for (auto const& column : proto_schema.columns()) {
231-
if (!bigtable::Value::TypeAndValuesMatch(column.type(),
232-
*parsed_value)) {
233-
std::cout << "Metadata and Value not matching." << std::endl;
234-
return internal::InternalError("Metadata and Value not matching.",
235-
GCP_ERROR_INFO());
231+
auto type_value_match_result = bigtable::Value::TypeAndValuesMatch(column.type(),
232+
*parsed_value);
233+
if (!type_value_match_result.ok()) {
234+
return type_value_match_result;
236235
}
237236
auto value = FromProto(column.type(), *parsed_value);
238237
values.push_back(std::move(value));

‎google/cloud/bigtable/value.cc‎

Lines changed: 74 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "google/cloud/internal/throw_delegate.h"
1818
#include "absl/container/flat_hash_set.h"
1919
#include "absl/strings/cord.h"
20+
#include "absl/strings/substitute.h"
2021
#include <google/bigtable/v2/types.pb.h>
2122
#include <google/protobuf/descriptor.h>
2223
#include <google/protobuf/message.h>
@@ -339,58 +340,71 @@ std::ostream& operator<<(std::ostream& os, Value const& v) {
339340
}
340341

341342
// NOLINTNEXTLINE(misc-no-recursion)
342-
bool Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type,
343+
Status Value::TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type,
343344
google::bigtable::v2::Value const& value) {
344345
if (!value.has_array_value()) {
345-
return false;
346+
return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: MAP");
346347
}
347348
auto const& vals = value.array_value().values();
348-
// NOLINTNEXTLINE(misc-no-recursion)
349-
return std::all_of(vals.begin(), vals.end(), [&](auto const& val) -> bool {
350-
return TypeAndValuesMatch(type.array_type().element_type(), val);
351-
});
349+
for (auto const& val : vals) {
350+
auto const element_match_result = TypeAndValuesMatch(type.array_type().element_type(), val);
351+
if (!element_match_result.ok()) {
352+
return element_match_result;
353+
}
354+
}
355+
return Status{};
352356
}
353357

354358
// NOLINTNEXTLINE(misc-no-recursion)
355-
bool Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type,
359+
Status Value::TypeAndMapValuesMatch(google::bigtable::v2::Type const& type,
356360
google::bigtable::v2::Value const& value) {
357361
if (!value.has_array_value()) {
358-
return false;
362+
return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: MAP");
359363
}
360364
auto key_type = type.map_type().key_type();
361365
auto value_type = type.map_type().value_type();
362366
auto const& vals = value.array_value().values();
363-
// NOLINTNEXTLINE(misc-no-recursion)
364-
return std::all_of(vals.begin(), vals.end(), [&](auto const& val) -> bool {
367+
for (auto const& val : vals) {
365368
if (!val.has_array_value() || val.array_value().values_size() != 2) {
366-
return false;
369+
return internal::InternalError("ARRAY_VALUE must contain entries of 2 values");
367370
}
368371
auto map_key = val.array_value().values(0);
369372
auto map_value = val.array_value().values(1);
370-
return TypeAndValuesMatch(key_type, map_key) &&
371-
TypeAndValuesMatch(value_type, map_value);
372-
});
373+
// NOLINTNEXTLINE(misc-no-recursion)
374+
auto key_match_result = TypeAndValuesMatch(key_type, map_key);
375+
if (!key_match_result.ok()) {
376+
return key_match_result;
377+
}
378+
// NOLINTNEXTLINE(misc-no-recursion)
379+
auto value_match_result = TypeAndValuesMatch(value_type, map_value);
380+
if (!value_match_result.ok()) {
381+
return value_match_result;
382+
}
383+
}
384+
return Status{};
373385
}
374386

375387
// NOLINTNEXTLINE(misc-no-recursion)
376-
bool Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type,
388+
Status Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type,
377389
google::bigtable::v2::Value const& value) {
378390
if (!value.has_array_value()) {
379-
return false;
391+
return internal::InternalError("Value kind must be ARRAY_VALUE for columns of type: STRUCT");
380392
}
381393
auto fields = type.struct_type().fields();
382394
auto values = value.array_value().values();
383395
if (fields.size() != values.size()) {
384-
return false;
396+
auto const message = absl::Substitute("received Struct with $0 values, but metadata has $1 fields", values.size(), fields.size());
397+
return internal::InternalError(message);
385398
}
386399
for (int i = 0; i < fields.size(); ++i) {
387400
auto const& f1 = fields.Get(i);
388401
auto const& v = values[i];
389-
if (!TypeAndValuesMatch(f1.type(), v)) {
390-
return false;
402+
auto match_result = TypeAndValuesMatch(f1.type(), v);
403+
if (!match_result.ok()) {
404+
return match_result;
391405
}
392406
}
393-
return true;
407+
return Status{};
394408
}
395409

396410
/**
@@ -399,51 +413,74 @@ bool Value::TypeAndStructValuesMatch(google::bigtable::v2::Type const& type,
399413
* the value contents themselves
400414
*/
401415
// NOLINTNEXTLINE(misc-no-recursion)
402-
bool Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type,
416+
Status Value::TypeAndValuesMatch(google::bigtable::v2::Type const& type,
403417
google::bigtable::v2::Value const& value) {
404418
using google::bigtable::v2::Type;
405-
bool has_matching_value;
419+
auto make_mismatch_metadata_status = [&](std::string const& value_kind, std::string const& type_name) {
420+
auto const message = absl::Substitute("Value kind must be $0 for columns of type: $1", value_kind, type_name);
421+
return internal::InternalError(message);
422+
};
423+
// Null values are allowed by default
424+
if (IsNullValue(value)) {
425+
return Status{};
426+
}
427+
Status result;
406428
switch (type.kind_case()) {
407429
case Type::kArrayType:
408-
has_matching_value = TypeAndArrayValuesMatch(type, value);
430+
result = TypeAndArrayValuesMatch(type, value);
409431
break;
410432
case Type::kMapType:
411-
has_matching_value = TypeAndMapValuesMatch(type, value);
433+
result = TypeAndMapValuesMatch(type, value);
412434
break;
413435
case Type::kStructType:
414-
has_matching_value = TypeAndStructValuesMatch(type, value);
436+
result = TypeAndStructValuesMatch(type, value);
415437
break;
416438
case Type::kBoolType:
417-
has_matching_value = value.has_bool_value();
439+
if (!value.has_bool_value()) {
440+
result = make_mismatch_metadata_status("BOOL_VALUE", "BOOL");
441+
}
418442
break;
419443
case Type::kBytesType:
420-
has_matching_value = value.has_bytes_value();
444+
if (!value.has_bytes_value()) {
445+
result = make_mismatch_metadata_status("BYTES_VALUE", "BYTES");
446+
}
421447
break;
422448
case Type::kDateType:
423-
has_matching_value = value.has_date_value();
424-
break;
425-
case Type::kEnumType:
426-
has_matching_value = value.has_int_value();
449+
if (!value.has_date_value()) {
450+
result = make_mismatch_metadata_status("DATE_VALUE", "DATE");
451+
}
427452
break;
428453
case Type::kFloat32Type:
454+
if (!value.has_float_value()) {
455+
result = make_mismatch_metadata_status("FLOAT_VALUE", "FLOAT32");
456+
}
457+
break;
429458
case Type::kFloat64Type:
430-
has_matching_value = value.has_float_value();
459+
if (!value.has_float_value()) {
460+
result = make_mismatch_metadata_status("FLOAT_VALUE", "FLOAT64");
461+
}
431462
break;
432463
case Type::kInt64Type:
433-
has_matching_value = value.has_int_value();
464+
if (!value.has_int_value()) {
465+
result = make_mismatch_metadata_status("INT_VALUE", "INT64");
466+
}
434467
break;
435468
case Type::kStringType:
436-
has_matching_value = value.has_string_value();
469+
if (!value.has_string_value()) {
470+
result = make_mismatch_metadata_status("STRING_VALUE", "STRING");
471+
}
437472
break;
438473
case Type::kTimestampType:
439-
has_matching_value = value.has_timestamp_value();
474+
if (!value.has_timestamp_value()) {
475+
result = make_mismatch_metadata_status("TIMESTAMP_VALUE", "TIMESTAMP");
476+
}
440477
break;
441478
default:
442-
has_matching_value = false;
479+
result = internal::InternalError("Unsupported type");
443480
break;
444481
}
445482
// Nulls are allowed;
446-
return has_matching_value || IsNullValue(value);
483+
return result;
447484
}
448485

449486
//

‎google/cloud/bigtable/value.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -315,15 +315,15 @@ class Value {
315315
return value.kind_case() == google::bigtable::v2::Value::KIND_NOT_SET;
316316
}
317317

318-
static bool TypeAndValuesMatch(google::bigtable::v2::Type const& type,
318+
static Status TypeAndValuesMatch(google::bigtable::v2::Type const& type,
319319
google::bigtable::v2::Value const& value);
320320

321321
private:
322-
static bool TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type,
322+
static Status TypeAndArrayValuesMatch(google::bigtable::v2::Type const& type,
323323
google::bigtable::v2::Value const& value);
324-
static bool TypeAndMapValuesMatch(google::bigtable::v2::Type const& type,
324+
static Status TypeAndMapValuesMatch(google::bigtable::v2::Type const& type,
325325
google::bigtable::v2::Value const& value);
326-
static bool TypeAndStructValuesMatch(
326+
static Status TypeAndStructValuesMatch(
327327
google::bigtable::v2::Type const& type,
328328
google::bigtable::v2::Value const& value);
329329

‎google/cloud/bigtable/value_test.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1747,13 +1747,19 @@ TEST(Value, OutputStreamMatchesT) {
17471747
// std::unordered_map<...>
17481748
// Not included, because a raw map cannot be streamed.
17491749
}
1750+
17501751
void TestTypeAndValuesMatch(std::string const& type_text,
17511752
std::string const& value_text, bool expected) {
17521753
google::bigtable::v2::Type type;
17531754
ASSERT_TRUE(TextFormat::ParseFromString(type_text, &type));
17541755
google::bigtable::v2::Value value;
17551756
ASSERT_TRUE(TextFormat::ParseFromString(value_text, &value));
1756-
EXPECT_EQ(expected, Value::TypeAndValuesMatch(type, value));
1757+
auto result = Value::TypeAndValuesMatch(type, value);
1758+
if (expected) {
1759+
EXPECT_STATUS_OK(result);
1760+
} else {
1761+
EXPECT_THAT(result, Not(IsOk()));
1762+
}
17571763
}
17581764

17591765
TEST(Value, TypeAndValuesMatchScalar) {

0 commit comments

Comments
 (0)