This change is 

Nice to close on this. What is the versioning story? Seems like we should at least bump the distsql version no?

Review status: 0 of 11 files reviewed at latest revision, all discussions resolved, some commit checks failed.

pkg/util/encoding/encoding.go, line 2189 at r1 (raw file):

func getTupleKeyLength(buf []byte) (int, error) {
	return 0, errors.Errorf("TODO(donotmerge)")

WIP?

Comments from Reviewable

Good point, this requires a version bump. Added.

Review status: 0 of 13 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

pkg/util/encoding/encoding.go, line 2189 at r1 (raw file):

Removed. Cruft code that wasn't used. Thanks.

Comments from Reviewable

Review status: 0 of 12 files reviewed at latest revision, all discussions resolved, some commit checks pending.

pkg/sql/sqlbase/table.go, line 1436 at r2 (raw file):

		return a.NewDOid(tree.MakeDOid(tree.DInt(i))), rkey, err
	default:
		if t, ok := valType.(types.TCollatedString); ok {

[nit] this block could be moved to a second switch t := valType.(type)

pkg/sql/sqlbase/testutils.go, line 119 at r2 (raw file):

	case ColumnType_TUPLE:
		// TODO(arjun)
		return tree.DNull

This violates nullOk, surprising it doesn't break something

Comments from Reviewable

I think this needs some testing. I would recommend adding tests to encoding.go, alongside the tests like TestValueEncodeDecodeBytes and so forth, following the pattern of encoding random tuples, decoding them, and asserting that the output's equal to the input.

Review status: 0 of 14 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

pkg/sql/sem/tree/format_test.go, line 197 at r4 (raw file):

		{`sqrt(123.0) + 456`, tree.FmtParsable,
			`sqrt(123.0:::DECIMAL) + 456:::DECIMAL`},
		{`row()`, tree.FmtParsable, `row()`},

this might need to be ROW() since we explicitly output that as the string.

pkg/sql/sqlbase/encoded_datum.go, line 142 at r4 (raw file):

		panic(fmt.Sprintf("invalid datum type given: %s, expected %s",
			d.ResolvedType(), ptyp))
	}

Why did you remove this?

pkg/sql/sqlbase/encoded_datum.go, line 202 at r4 (raw file):

	}
	if err != nil {
		return errors.Wrapf(err, "error decoding % bytes", len(ed.encoded))

This format string probably should be %d :)

pkg/sql/sqlbase/table.go, line 1449 at r4 (raw file):

			for i, tupleType := range tupleTypes.Types {
				var err error
				dtuple.D[i], rkey, err = DecodeTableKey(a, tupleType, rkey, dir)

I'm confused about how this works. You encode with EncodeTableValue but decode with DecodeTableKey. How is this possible?

Now that I think about it, is this code even used? We never encode tuple types with key encoding. I have a feeling this stanza is dead code. If not, I'd love to know why it isn't

pkg/sql/sqlbase/table.go, line 1678 at r4 (raw file):

			return decodeArray(a, typ.Typ, buf)
		case types.TTuple:
			return decodeTuple(a, typ, buf[1:])

why is this buf[1:]? what byte gets eaten? we don't seem to do that elsewhere.

Comments from Reviewable

Review status: 0 of 14 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/table.go, line 731 at r6 (raw file):

	case *tree.DTuple:
		scratch = scratch[0:0]
		// TODO(arjun): How do we eliminate this allocation?

Be kind to your future self and specify which allocation you're concerned about eliminating.

Seems like you're encoding a TupleValue as an encoded tuple that is then encoded again similarly to a bytes value. Did you consider the alternative of encoding a tuple value as a tag, a count of the number of items in the tuple and then the individual tuple values? I think with that encoding you'd be able to encode the tuple values directly into appendTo. Something like:

appendTo = encoding.EncodeValueTag(appendTo, uint32(colID), encoding.Tuple)
appendTo = encoding.EncodeNonsortingUvarint(appendTo, len(t.D))
for _, dd := range t.D {
  var err error
  appendTo, err = EncodeTableValue(appendTo, ColumnID(encoding.NoColumnID), dd, scratch)
  if err != nil {
    return nil, err
  }
}

Comments from Reviewable

Added some randomized testing, great idea @jordanlewis! Finding test failures, and debugging them now. Thank you.

Review status: 0 of 14 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

pkg/sql/sem/tree/format_test.go, line 197 at r4 (raw file):

Done.

pkg/sql/sqlbase/encoded_datum.go, line 142 at r4 (raw file):

It was making debugging difficult, so I temporarily removed it. Put back in.

pkg/sql/sqlbase/encoded_datum.go, line 202 at r4 (raw file):

Done.

pkg/sql/sqlbase/table.go, line 1436 at r2 (raw file):

Much cleaner! Thanks.

pkg/sql/sqlbase/table.go, line 1449 at r4 (raw file):

🙀 You are right! Will fix.

pkg/sql/sqlbase/table.go, line 1678 at r4 (raw file):

Removed, was bogus and wrong.

pkg/sql/sqlbase/table.go, line 731 at r6 (raw file):

Wonderful. Done, thank you!

pkg/sql/sqlbase/testutils.go, line 119 at r2 (raw file):

Fixed.

Comments from Reviewable

The new encoding LGTM.

Review status: 0 of 15 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/table.go, line 1559 at r7 (raw file):

	}

	result := tree.DTuple{

Do we need to add DatumAlloc.dtupleAlloc []tree.DTuple?

Comments from Reviewable

pkg/sql/sqlbase/table.go, line 1559 at r7 (raw file):

Sure, but the main allocation is the below line with make([]tree.Datum and that's hard to pre-allocate. Any thoughts on batching those allocations?

Comments from Reviewable

Review status: 0 of 15 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/table.go, line 1559 at r7 (raw file):

DatumAlloc.datumAlloc []tree.Datum?

Comments from Reviewable

Review status: 14 of 17 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

pkg/util/encoding/encoding.go, line 1060 at r9 (raw file):

I don't think the comments here does justice to the requirement: we can't change any of these values. I think it would be worthwhile to replace the use of iota with explicitly listing the values. That is:

const (
  Unknown Type = 0
  Null Type = 1
  NotNull Type = 2
...

Comments from Reviewable

pkg/util/encoding/encoding.go, line 1060 at r9 (raw file):

I agree; I will do so in this PR. And it needs to be moved to a proto in a subsequent PR. I've already lost hours.

Comments from Reviewable

Review status: 14 of 17 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/table.go, line 1099 at r9 (raw file):

Done.

pkg/sql/sqlbase/table.go, line 1576 at r9 (raw file):

I've removed that comment since it seems to only cause confusion.

pkg/sql/sqlbase/table.go, line 1582 at r9 (raw file):

So if you are given a DNull, that is an acceptable value when you're expecting any Tuple. If you are given (1, NULL, 'foo'), that is only an acceptable value when you're expecting a tuple of type (int, any, string). In this case we are only checking if we are given a DNull. In this case, we won't see a tuple encoding header. If we are in the latter scenario, we'd see a tuple encoding header and the normal decoding path would work.

Comments from Reviewable

Review status: 12 of 17 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/sql/sqlbase/table.go, line 1582 at r9 (raw file):

Recapping offline conversation:

This stanza exists to prevent null tuples from being incorrectly decoded. However, this isn't a problem in practice - null values are already decoded separately up in DecodeTableValue, which peels off the type byte from an encoding and makes a decision. By the time control is passed to decodeTuple, we're 100% positive we're not decoding a NULL tuple.

The tests can be converted to use EncodeTableValue and DecodeTableValue to get around this.

Comments from Reviewable

PTAL @jordanlewis

Review status: 12 of 19 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

pkg/sql/sqlbase/table.go, line 1582 at r9 (raw file):

Done.

Comments from Reviewable

Review status: 12 of 19 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.

Comments from Reviewable

bors r+

Review status: 12 of 19 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

Comments from Reviewable

Build failed (retrying...)

• GitHub CI (Cockroach)

Build failed

• GitHub CI (Cockroach)

Build failures seem to be due to newly added logic tests that require rowsort. They weren't caught until the rebase. Nice work, bors, caught what would have turned into maddeningly flaky tests.

Will fix and retry.

The bors is really paying its rent today, found #26402. Will rebase on top of that and retry once that merges.

bors r+

Build succeeded

• GitHub CI (Cockroach)