keys: support splitting Ranges on tenant-id prefixed keys

[nvanbenschoten]

Fixes #48122.
Relates to #47903.
Relates to #48123.

This PR contains a series of small commits that work towards the introduction of tenant-id prefixed keyspaces and begin the removal of some keys.TODOSQLCodec instances. This should be the only time we need to touch C++ throughout this work.

[cockroach-teamcity]

This change is 

[tbg]

Reviewed 2 of 2 files at r1, 9 of 9 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten)

---

c-deps/libroach/encoding.cc, line 289 at r2 (raw file):

  buf->remove_prefix(1);
  uint64_t tid;
  return DecodeUvarint64(buf, &tid);

Oh, so this mutates the Slice*? Interesting, wouldn't have guessed.

---

c-deps/libroach/encoding_test.cc, line 80 at r2 (raw file):

}

TEST(Libroach, DecodeTenantAndTablePrefix) {

This mirrors go tests, right? Mind throwing in a comment about which one.

---

pkg/keys/keys.go, line 696 at r1 (raw file):

	// Strip tenant ID prefix from key.
	tenKey, _, err := DecodeTenantPrefix(key)

nit: you could avoid having two keys float around by changing the code as below:

if key2, _, err := DecodeTenantPrefix(key)
  if err != nil {
    return 0, errors.Errorf("%s: not a valid table key", key)
  }
  if encoding.PeekType(key2) != encoding.Int {
	// Not a table key, so the row prefix is the entire key.
	return n, nil
  }
}
// Rest of the code only looks at the end of `key`, so we don't have to use key2`
// and also we don't have two lengths floating around.

This makes it impossible to use the wrong key below. (Besides, tenKey suggests to me that it's a key with tenant prefix, but here's it's the opposite, so the name is a little odd))

[nvanbenschoten]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nvanbenschoten and @tbg)

---

c-deps/libroach/encoding_test.cc, line 80 at r2 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This mirrors go tests, right? Mind throwing in a comment about which one.

No, it's not directly mirroring anything. I just realized we had no direct test coverage for these functions.

---

pkg/keys/keys.go, line 696 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

nit: you could avoid having two keys float around by changing the code as below:

if key2, _, err := DecodeTenantPrefix(key)
  if err != nil {
    return 0, errors.Errorf("%s: not a valid table key", key)
  }
  if encoding.PeekType(key2) != encoding.Int {
	// Not a table key, so the row prefix is the entire key.
	return n, nil
  }
}
// Rest of the code only looks at the end of `key`, so we don't have to use key2`
// and also we don't have two lengths floating around.

This makes it impossible to use the wrong key below. (Besides, tenKey suggests to me that it's a key with tenant prefix, but here's it's the opposite, so the name is a little odd))

Unfortunately, we need the length of this stripped key down below when performing the colIDLen > uint64(tenN-1) check. I rewrote this a bit though based on your suggestion:

1. renamed tenKey/tenN to sqlKey/sqlN.
2. renamed colIDLen/colIDLenByte to colFamIDLen/colFamIDLenByte
3. updated the comments to discuss how this works with column family 0
4. added tests using real SQL keys

[nvanbenschoten]

bors r+

[craig]

Build failed (retrying...)

• GitHub CI (Cockroach)

[craig]

Build succeeded

• GitHub CI (Cockroach)