impl(spanner): add uuid type

[scotthart]

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 98.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.90%. Comparing base (518d46a) to head (5fd97bc).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
google/cloud/spanner/uuid.cc	95.83%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           main   #15023       +/-   ##
=========================================
+ Coverage      0   92.90%   +92.90%     
=========================================
  Files         0     2354     +2354     
  Lines         0   210355   +210355     
=========================================
+ Hits          0   195435   +195435     
- Misses        0    14920    +14920     

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

🚀 New features to boost your workflow:

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

[scotthart]

Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @cuiy0006)

[scotthart]

/gcbrun

[devbww]

It looks like this could/should be done without copying the string.

[scotthart]

It's necessary to copy the original string for error message contents.

[devbww]

No, copying the string isn't necessary. Nothing here mutates the characters of the argument string_view.

[scotthart]

A few of the operations performed do mutate the string_view. I added additional expectation criteria to the tests that verified that.

[devbww]

You appear to be confusing "mutate the string_view" with "mutate the characters of the string_view". Nothing does the latter. Indeed, string_view prohibits it. So, there is no need to copy the string.

[scotthart]

Copying the string_view instead.

[devbww]

"hyphen after braces" implies that this only applies to a hyphen after a brace. So, perhaps "hyphen after stripping any surrounding braces" would sound better?

[scotthart]

Done.

[devbww]

has_braces == true seems wrong for something we only know starts with a brace. Given that the variable is only used once, I'd just get rid of it. For example, ...

  if (absl::ConsumePrefix(&str, "{")) {
    if (!absl::ConsumeSuffix(&str, "}") {
      return internal::InvalidArgumentError(...);
    }
  }

[scotthart]

Done.

[devbww]

Consider a string of 31 hex digits, each separated by a hyphen, for a total length of 61 chars. Complaining that the string must be at least 32 chars long, when it is, seems wrong. Something about "not enough hex digits" would be more appropriate.

[scotthart]

Updated this error message and some others to specify hex digits as appropriate.

[devbww]

s/uint64_t/std::uint64_t/ et seq.

[scotthart]

Done.

[devbww]

It would probably be clearer to move this into the if (it == char_to_hex->end() block, if only to highlight that we're just special-casing the error message for a particular invalid character.

[scotthart]

Done.

[devbww]

If you're going to generalize using kHyphenPos, you should also compute the start/end_index pairs using it.

[scotthart]

So I began computing start/end pairs per call to to_hex, looked at the results and thought "I keep recomputing the values of start/end, this could be a loop". Refactored it into a loop.

[devbww]

Consider the operator std::string() snippet above, which has a single loop, and doesn't do any index computations.

[scotthart]

Created #15043 for future improvements as I'm out of time budget to work on this.

[devbww]

Remove redundant parentheses?

[scotthart]

Done.

[devbww]

This test seems best omitted, and then handled by the actual failure to find enough hex digits.

[scotthart]

Moved this check as recommended, and also added a guard for an empty string.

[scotthart]

Thanks for the review, Brad. I appreciate you donating your time.

Reviewable status: 0 of 6 files reviewed, 10 unresolved discussions (waiting on @cuiy0006 and @devbww)

[scotthart]

Done.

[scotthart]

Done.

[scotthart]

Updated this error message and some others to specify hex digits as appropriate.

[scotthart]

Done.

[scotthart]

So I began computing start/end pairs per call to to_hex, looked at the results and thought "I keep recomputing the values of start/end, this could be a loop". Refactored it into a loop.

[scotthart]

Moved this check as recommended, and also added a guard for an empty string.

[scotthart]

It's necessary to copy the original string for error message contents.

[scotthart]

Done.

[scotthart]

Done.

[devbww]

[0-f] is not good definition of a character class.

[scotthart]

Expanded definition. If this still isn't a "good" definition, please suggest one.

[devbww]

I'm not sure what "packed" is supposed to convey.

[scotthart]

reworded

[devbww]

OK, although we now have "128 bit unsigned" and "unsigned 64 bit" in consecutive comments. Consistency would be nice. And "N-bit" should be hyphenated when used as an adjective.

[scotthart]

Reworded across usages for consistency.

[devbww]

I'd get rid of this constructor. If the caller has two std::uint64_ts, they can build the absl::uint128 by themselves. It shouldn't be part of this class (just like there shouldn't be an accessor that returns a pair of std::uint64_ts).

[scotthart]

I'd prefer to give users a options that uses std integer types.

[devbww]

Then, there should be an accessor that returns a pair of std::uint64_ts.

And, you need to #include <cstdint>.

[scotthart]

Done.

[devbww]

"using formatted:" doesn't parse.

[scotthart]

reworded

[devbww]

s/ using//

[scotthart]

Done.

[devbww]

output.size() - kUuidStringLen is zero. Why obfuscate that?

[scotthart]

De-obfuscated

[devbww]

const_casting isn't good either. Why not just ...

  char* target = &output[0];
  char* const last = &output[output.size()];

[scotthart]

&output[0] turns into a clang-tidy diagnostic about using .data() instead which until c++17 only returns a const char*. I have multiple non-ideal choices.

[devbww]

Similarly, I would suggest not introducing two 64-bit values here. (Otherwise, why not four 32-bit values, for example?) A single 128-bit value should suffice.

[scotthart]

Lacking a std::uint128_t and absl::uint128 not behaving exactly like a std::uint128_t, std::uint64_t is the largest integer currently available.

[devbww]

Again, I don't know what absl::uint128 behavior you think is lacking. For example,

#include <cctype>
#include <cstring>

constexpr char kHexDigits[] = "0123456789abcdef";

StatusOr<Uuid> MakeUuid(absl::string_view str) {
  absl::uint128 uuid = 0;
  auto const original_str = str;

  if (absl::ConsumePrefix(&str, "{")) {
    if (!absl::ConsumeSuffix(&str, "}")) {
      return internal::InvalidArgumentError(
          absl::StrFormat("UUID missing closing '}': %s", original_str),
          GCP_ERROR_INFO());
    }
  }
  if (absl::StartsWith(str, "-")) {
    return internal::InvalidArgumentError(
        absl::StrFormat("UUID cannot begin with '-': %s", original_str),
        GCP_ERROR_INFO());
  }

  constexpr int kUuidNumberOfHexDigits = 32;
  for (int j = 0; j != kUuidNumberOfHexDigits; ++j) {
    absl::ConsumePrefix(&str, "-");
    if (str.empty()) {
      return internal::InvalidArgumentError(
          absl::StrFormat("UUID must contain %d hexadecimal digits: %s",
                          kUuidNumberOfHexDigits, original_str),
          GCP_ERROR_INFO());
    }
    auto const* dp = std::strchr(
        kHexDigits, std::tolower(static_cast<unsigned char>(str[0])));
    if (dp == nullptr) {
      if (str[0] == '-') {
        return internal::InvalidArgumentError(
            absl::StrFormat("UUID cannot contain consecutive hyphens: %s",
                            original_str),
            GCP_ERROR_INFO());
      }
      return internal::InvalidArgumentError(
          absl::StrFormat("UUID contains invalid character (%c): %s", str[0],
                          original_str),
          GCP_ERROR_INFO());
    }
    str.remove_prefix(1);
    uuid <<= 4;
    uuid += dp - kHexDigits;
  }

  if (!str.empty()) {
    return internal::InvalidArgumentError(
        absl::StrFormat("Extra characters found after parsing UUID: %s", str),
        GCP_ERROR_INFO());
  }
  return Uuid{uuid};
}

No "blocks" (or hash maps). Just a simple per-nibble loop.

[scotthart]

Created #15043 for future improvements as I'm out of time budget to work on this.

[devbww]

Why not provide the actual extra characters rather than the number of them?

[scotthart]

Done.

[devbww]

Moved this check as recommended, and also added a guard for an empty string.

I wouldn't make a special case of the empty string either. Just change the

  if (str[0] == '-') {

test to

  if (absl::StartsWith(str, "-")) {

and the empty string will be treated like any other illegal argument.

[scotthart]

I'd prefer the early exit and specific error message.

[devbww]

"Early exit" is only a preferred strategy when it makes the following code simpler. Here it remains the same.

Making "UUID cannot be empty" a special case over "UUID must contain 32 hexadecimal digits" is not worth the redundancy. It makes the code more cluttered and more difficult to understand. And why shouldn't MakeUuid("{}") behave the same way?

[scotthart]

Removed this check and added a test for "{}". Both now result in a satisfactory error message.

[devbww]

Why isn't this localized to ParseHexBlock() just like kMaxUuidBlockLength is?

[scotthart]

Done.

[devbww]

Given that == and < are simple operations on the representation, I don't know why we wouldn't choose to inline them.

[scotthart]

Done.

[scotthart]

Reviewable status: 0 of 6 files reviewed, 21 unresolved discussions (waiting on @cuiy0006 and @devbww)

[scotthart]

reworded

[scotthart]

I'd prefer to give users a options that uses std integer types.

[scotthart]

Done.

[scotthart]

reworded

[scotthart]

Expanded definition. If this still isn't a "good" definition, please suggest one.

[scotthart]

De-obfuscated

[scotthart]

absl::uint128 isn't a drop in replacement in the existing code (I tried). When we get a std::uint128_t I'm happy to revisit.

[scotthart]

I'd prefer the early exit and specific error message.

[scotthart]

Lacking a std::uint128_t and absl::uint128 not behaving exactly like a std::uint128_t, std::uint64_t is the largest integer currently available.

[scotthart]

Done.

[devbww]

You're welcome.

Aside: "switch from absl::flat_hash_map to std::unordered_map for 32 bit compatibility" sounds wacky.

Reviewed 3 of 6 files at r1, 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @cuiy0006 and @scotthart)

---

google/cloud/spanner/uuid.cc line 31 at r6 (raw file):

StatusOr<std::uint64_t> ParseHexBlock(absl::string_view& str,
                                      absl::string_view original_str) {
  constexpr int kMaxUuidNumberOfHexDigits = 32;

In what sense is this a "Max" value? That implies that we could have fewer hex digits, but that is not the case.

---

google/cloud/spanner/uuid.cc line 33 at r6 (raw file):

  constexpr int kMaxUuidNumberOfHexDigits = 32;
  constexpr int kMaxUuidBlockLength = 16;
  static auto const* char_to_hex = new std::unordered_map<char, std::uint8_t>(

#include <unordered_map>

[devbww]

OK, although we now have "128 bit unsigned" and "unsigned 64 bit" in consecutive comments. Consistency would be nice. And "N-bit" should be hyphenated when used as an adjective.

[devbww]

Then, there should be an accessor that returns a pair of std::uint64_ts.

And, you need to #include <cstdint>.

[devbww]

s/ using//

[devbww]

Consider the operator std::string() snippet above, which has a single loop, and doesn't do any index computations.

[devbww]

You appear to be confusing "mutate the string_view" with "mutate the characters of the string_view". Nothing does the latter. Indeed, string_view prohibits it. So, there is no need to copy the string.

[devbww]

const_casting isn't good either. Why not just ...

  char* target = &output[0];
  char* const last = &output[output.size()];

[devbww]

I don't know what you tried, but if absl::uint128 didn't work as an integral type then that would be an Abseil bug.

In any case, I'll assert that it does work.

constexpr char kHexDigits[] = "0123456789abcdef";

Uuid::operator std::string() const {
  constexpr char kTemplate[] = "00000000-0000-0000-0000-000000000000";
  char buf[sizeof kTemplate];
  auto uuid = uuid_;
  for (auto j = sizeof buf; j-- != 0;) {
    if (kTemplate[j] != '0') {
      buf[j] = kTemplate[j];
    } else {
      buf[j] = kHexDigits[static_cast<int>(uuid & 0xf)];
      uuid >>= 4;
    }
  }
  return buf;
}

[devbww]

"Early exit" is only a preferred strategy when it makes the following code simpler. Here it remains the same.

Making "UUID cannot be empty" a special case over "UUID must contain 32 hexadecimal digits" is not worth the redundancy. It makes the code more cluttered and more difficult to understand. And why shouldn't MakeUuid("{}") behave the same way?

[devbww]

Again, I don't know what absl::uint128 behavior you think is lacking. For example,

#include <cctype>
#include <cstring>

constexpr char kHexDigits[] = "0123456789abcdef";

StatusOr<Uuid> MakeUuid(absl::string_view str) {
  absl::uint128 uuid = 0;
  auto const original_str = str;

  if (absl::ConsumePrefix(&str, "{")) {
    if (!absl::ConsumeSuffix(&str, "}")) {
      return internal::InvalidArgumentError(
          absl::StrFormat("UUID missing closing '}': %s", original_str),
          GCP_ERROR_INFO());
    }
  }
  if (absl::StartsWith(str, "-")) {
    return internal::InvalidArgumentError(
        absl::StrFormat("UUID cannot begin with '-': %s", original_str),
        GCP_ERROR_INFO());
  }

  constexpr int kUuidNumberOfHexDigits = 32;
  for (int j = 0; j != kUuidNumberOfHexDigits; ++j) {
    absl::ConsumePrefix(&str, "-");
    if (str.empty()) {
      return internal::InvalidArgumentError(
          absl::StrFormat("UUID must contain %d hexadecimal digits: %s",
                          kUuidNumberOfHexDigits, original_str),
          GCP_ERROR_INFO());
    }
    auto const* dp = std::strchr(
        kHexDigits, std::tolower(static_cast<unsigned char>(str[0])));
    if (dp == nullptr) {
      if (str[0] == '-') {
        return internal::InvalidArgumentError(
            absl::StrFormat("UUID cannot contain consecutive hyphens: %s",
                            original_str),
            GCP_ERROR_INFO());
      }
      return internal::InvalidArgumentError(
          absl::StrFormat("UUID contains invalid character (%c): %s", str[0],
                          original_str),
          GCP_ERROR_INFO());
    }
    str.remove_prefix(1);
    uuid <<= 4;
    uuid += dp - kHexDigits;
  }

  if (!str.empty()) {
    return internal::InvalidArgumentError(
        absl::StrFormat("Extra characters found after parsing UUID: %s", str),
        GCP_ERROR_INFO());
  }
  return Uuid{uuid};
}

No "blocks" (or hash maps). Just a simple per-nibble loop.

[scotthart]

So the m32-pr build was failing with an assert from absl::flat_hash_map about the constructed from value not being the same as the key type. As inefficient as it is, std::unordered_map did not have the same issue.

Reviewable status: 3 of 6 files reviewed, 12 unresolved discussions (waiting on @cuiy0006 and @devbww)

---

google/cloud/spanner/uuid.cc line 31 at r6 (raw file):

Previously, devbww (Bradley White) wrote…

In what sense is this a "Max" value? That implies that we could have fewer hex digits, but that is not the case.

Renamed

---

google/cloud/spanner/uuid.cc line 33 at r6 (raw file):

Previously, devbww (Bradley White) wrote…

#include <unordered_map>

Added.

[scotthart]

Reworded across usages for consistency.

[scotthart]

Done.

[scotthart]

Done.

[scotthart]

Created #15043 for future improvements as I'm out of time budget to work on this.

[scotthart]

Copying the string_view instead.

[scotthart]

&output[0] turns into a clang-tidy diagnostic about using .data() instead which until c++17 only returns a const char*. I have multiple non-ideal choices.

[scotthart]

I didn't try much. I changed to_hex first parameter to an absl::uint128 and got compile errors trying to index into kHexChar
Created #15043 for future improvements as I'm out of time budget to work on this.

[scotthart]

Removed this check and added a test for "{}". Both now result in a satisfactory error message.

[scotthart]

Created #15043 for future improvements as I'm out of time budget to work on this.

[devbww]

It would be better to understand what "constructed value does not match the lookup key" means rather than switching to a container that doesn't make a similar check.

Reviewed 3 of 3 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cuiy0006)