sem/tree: add support for producing vectorized data from strings

[cucaroach]

tree.ValueHandler exposes raw machine type hooks that are used by
vec_handler to build coldata.Vec's.

Epic: CRDB-18892
Informs: #91831
Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[cucaroach]

Reviewer note: This code is notably devoid of tests, there are pretty exhaustive tests in the coming copy changes built on this so I thought it was fine, its pretty straightforward and I'm trying to break all the changes into bit sized pieces for review purposes.

[rytaft]

Reviewed 4 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @cucaroach)

---

pkg/col/coldataext/vec_handler.go line 34 at r1 (raw file):

	col coldata.Vec
	row int
}

nit: add an assertion that this implements tree.ValueHandler, e.g.:

var _ tree.ValueHandler = (*vecHandler)(nil)

---

pkg/col/coldataext/vec_handler.go line 48 at r1 (raw file):

// Decimal implements tree.ValueHandler interface. It returns a pointer into the
// vec to avoid copying.
func (v *vecHandler) Decimal() *apd.Decimal {

It's a bit strange that this is the only method that gets a value, while all the others seem to set the value, and it's not immediately clear by looking at the signature. Can you maybe prefix all the setter functions with Set? e.g., SetNull, SetString....

Also, out of curiosity, why is Decimal the only getter needed?

Edit: I see it's due to the implementation in ParseAndRequireStringEx. But why not just do the setting inside this function and make it return an error?

---

pkg/col/coldataext/vec_handler.go line 1 at r2 (raw file):

// Copyright 2022 The Cockroach Authors.

shouldn't this be 2023?

---

pkg/sql/sem/tree/datum.go line 2913 at r1 (raw file):

//
// Parts of this function are inlined into ParseAndRequireEx, if this changes materially
// ParseAndRequireEx may need to change too.

I don't see that function in this commit. Should this comment go into a different PR?

---

pkg/sql/sem/tree/parse_string.go line 161 at r1 (raw file):

}

// ValueHandler is an interface to allow raw types to extracted from strings.

nit: to -> to be

---

pkg/sql/sem/tree/parse_string.go line 182 at r1 (raw file):

// vector engine directly to a ValueHandler. Other types are handled by
// ParseAndRequireString.
func ParseAndRequireStringEx(t *types.T, s string, ctx ParseContext, vh ValueHandler, ph *pgdate.ParseHelper) (err error) {

What does Ex mean here?

---

pkg/sql/sem/tree/parse_string.go line 206 at r1 (raw file):

		if err = setDecimalString(s, dec); err != nil {
			// Erase any invalid results.
			*dec = apd.Decimal{}

Is there any way to do this inside the vh function so Decimal can also be a setter function?

[cucaroach]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

---

pkg/col/coldataext/vec_handler.go line 34 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: add an assertion that this implements tree.ValueHandler, e.g.:

var _ tree.ValueHandler = (*vecHandler)(nil)

👍

---

pkg/col/coldataext/vec_handler.go line 48 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

It's a bit strange that this is the only method that gets a value, while all the others seem to set the value, and it's not immediately clear by looking at the signature. Can you maybe prefix all the setter functions with Set? e.g., SetNull, SetString....

Also, out of curiosity, why is Decimal the only getter needed?

Edit: I see it's due to the implementation in ParseAndRequireStringEx. But why not just do the setting inside this function and make it return an error?

So its a setter not a getter its just returning a pointer into the vec so we can do an in place construct of the the decimal and avoid copy penalty and any heap allocations.

---

pkg/col/coldataext/vec_handler.go line 1 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

shouldn't this be 2023?

I wrote it in December, should I change it to 23?

---

pkg/sql/sem/tree/datum.go line 2913 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I don't see that function in this commit. Should this comment go into a different PR?

This was a warning that the body of ParseDTimestamp is basically duplicated in the case types.TimestampFamily: case arm of ParseAndRequireEx. Maybe its not worth mentioning or maybe I should go further and trying to squeeze the TimestmpFamily case arms down to eliminate the code duplication? Open to suggestions, maybe an additional TODO to refactor things better in ParseAndRequireEx and clarify this comment?

---

pkg/sql/sem/tree/parse_string.go line 182 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

What does Ex mean here?

Extra? I copied it from elsewhere, maybe ParseAndRequireStringHandler would be better?

---

pkg/sql/sem/tree/parse_string.go line 206 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Is there any way to do this inside the vh function so Decimal can also be a setter function?

This was the only way I could construct it that avoided copies and unnecessary heap allocations for temporaries. I'll add a clarifying comment here too.

[rytaft]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @cucaroach)

---

pkg/col/coldataext/vec_handler.go line 48 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

So its a setter not a getter its just returning a pointer into the vec so we can do an in place construct of the the decimal and avoid copy penalty and any heap allocations.

ok! I think you could make the comment more explicit (and I'd still change the other function names to Set...)

---

pkg/col/coldataext/vec_handler.go line 1 at r2 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

I wrote it in December, should I change it to 23?

I'd say 23 since the commit is dated 2023

---

pkg/sql/sem/tree/datum.go line 2913 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

This was a warning that the body of ParseDTimestamp is basically duplicated in the case types.TimestampFamily: case arm of ParseAndRequireEx. Maybe its not worth mentioning or maybe I should go further and trying to squeeze the TimestmpFamily case arms down to eliminate the code duplication? Open to suggestions, maybe an additional TODO to refactor things better in ParseAndRequireEx and clarify this comment?

Oh ok -- did you mean ParseAndRequireStringEx? That's what the new function is called.

Anyway, up to you if you want to leave this comment, but it seems a bit confusing to me since it's not really duplicated (that function also calls checkTimeBounds).

---

pkg/sql/sem/tree/parse_string.go line 182 at r1 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Extra? I copied it from elsewhere, maybe ParseAndRequireStringHandler would be better?

yea I like ParseAndRequireStringHandler better

[rytaft]

Sorry just noticed that you pushed at the same time as I wrote my comments.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @cucaroach)

---

pkg/sql/sem/tree/datum.go line 2913 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Oh ok -- did you mean ParseAndRequireStringEx? That's what the new function is called.

Anyway, up to you if you want to leave this comment, but it seems a bit confusing to me since it's not really duplicated (that function also calls checkTimeBounds).

(looks like the function name is fixed now)

[cucaroach]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rytaft)

---

pkg/col/coldataext/vec_handler.go line 48 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

ok! I think you could make the comment more explicit (and I'd still change the other function names to Set...)

Done.

---

pkg/col/coldataext/vec_handler.go line 1 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I'd say 23 since the commit is dated 2023

Done.

---

pkg/sql/sem/tree/datum.go line 2913 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

(looks like the function name is fixed now)

If you're happy with it I'll remove the comments and the TODO, I think when I originally wrote it I hadn't pulled out checkTimeBounds yet and it was worse.

[rytaft]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

[cucaroach]

TFTRs!
bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[yuzefovich]

Reviewed 1 of 4 files at r1, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @cucaroach)

---

pkg/col/coldataext/vec_handler.go line 31 at r4 (raw file):

type vecHandler struct {
	col coldata.Vec

Is this struct performance-sensitive? If so, it'd be better to unwrap coldata.Vec into a concrete slice in order to avoid an interface conversion on each setter method invocation. We could set up these fields to be auto-generated - take a look at TypedVecs in coldata/vec_tmpl.go. Then we'll only incur a type switch in MakeVecHandler and increase the size of vecHandler struct, but it seems like an acceptable tradeoff.

---

pkg/sql/sem/tree/parse_string.go line 122 at r4 (raw file):

}

func truncateString(s string, t *types.T) string {

nit: maybe s/truncateString/maybeTruncateString?

---

pkg/sql/sem/tree/parse_string.go line 189 at r4 (raw file):

	case types.BoolFamily:
		var b bool
		if b, err = ParseBool(strings.TrimSpace(s)); err == nil {

nit: I think we tend to short-circuit in err != nil case with a return so that the "success" branch is not indented. Up to you though.

---

pkg/sql/sem/tree/parse_string.go line 221 at r4 (raw file):

	case types.IntFamily:
		var i int64
		if i, err = strconv.ParseInt(s, 0, 64); err == nil {

Do we always parse ints as int64, regardless of t.Width()?

---

pkg/sql/sem/tree/parse_string.go line 274 at r4 (raw file):

			err = MakeParseError(s, types.Uuid, err)
		}
	default:

We also now support EnumFamily natively.

---

pkg/sql/sem/tree/parse_string.go line 275 at r4 (raw file):

		}
	default:
		var d Datum

nit: consider adding a test-only assertion here that typeconv.TypeFamilyToCanonicalTypeFamily(t.Family()) returns typeconv.DatumVecCanonicalTypeFamily. Perhaps some randomized unit test of ParseAndRequireStringHandler would also be good.

[cucaroach]

bors r-

[craig]

Canceled.

[cucaroach]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @yuzefovich)

---

pkg/col/coldataext/vec_handler.go line 31 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Is this struct performance-sensitive? If so, it'd be better to unwrap coldata.Vec into a concrete slice in order to avoid an interface conversion on each setter method invocation. We could set up these fields to be auto-generated - take a look at TypedVecs in coldata/vec_tmpl.go. Then we'll only incur a type switch in MakeVecHandler and increase the size of vecHandler struct, but it seems like an acceptable tradeoff.

Yes and my prototype that didn't use coldata.Vec was %15 faster during the string->data phase. I looked at TypedVec but didn't put 2 and 2 together, this is a great idea, thanks!

---

pkg/sql/sem/tree/parse_string.go line 189 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: I think we tend to short-circuit in err != nil case with a return so that the "success" branch is not indented. Up to you though.

Yeah I went back and forth on this, I wanted it to be consistent but some helper return a well formed parse error and some need to be massaged and wrapped so I went with avoiding as many else clauses as I could. I think its a wash which is better/worse for readability, when a tie go for brevity say I!

---

pkg/sql/sem/tree/parse_string.go line 221 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Do we always parse ints as int64, regardless of t.Width()?

Yeah, that's what ParseAndRequireString does, do you think its worth carving off int16 and int32 and using packed slices for those? I suppose if coldata already supports it why not? I'll probably throw on the TODO queue.

---

pkg/sql/sem/tree/parse_string.go line 274 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

We also now support EnumFamily natively.

Thanks!

---

pkg/sql/sem/tree/parse_string.go line 275 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: consider adding a test-only assertion here that typeconv.TypeFamilyToCanonicalTypeFamily(t.Family()) returns typeconv.DatumVecCanonicalTypeFamily. Perhaps some randomized unit test of ParseAndRequireStringHandler would also be good.

Okay. I have copy tests that hit all the types but I was feeling guilty about pushing this w/o tests, I'll add some basic ones.

[yuzefovich]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @cucaroach)

---

pkg/sql/sem/tree/parse_string.go line 221 at r4 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Yeah, that's what ParseAndRequireString does, do you think its worth carving off int16 and int32 and using packed slices for those? I suppose if coldata already supports it why not? I'll probably throw on the TODO queue.

Yeah, doesn't seem urgent, a TODO sgtm. I guess I'm surprised that we don't consult the schema of the table we're copying into nor default_int_size session variable for the width with which to parse integers.

[cucaroach]

RFAL, added some benchmarks and some testing code and incorporated Yahor's suggestions.

[yuzefovich]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @cucaroach)

---

pkg/sql/sem/tree/parse_string.go line 274 at r4 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

Thanks!

I still don't see the enums :)

---

pkg/sql/sem/tree/parse_string_test.go line 39 at r5 (raw file):

	case types.BoolFamily:
		return v.Bool()[i]
	case types.BytesFamily, types.StringFamily, types.UuidFamily:

nit: StringFamily and UuidFamily are not "canonical", so they will never match in this switch. Ditto for DateFamily and TimestampFamily.

---

pkg/sql/sem/tree/parse_string_test.go line 72 at r5 (raw file):

		d := randgen.RandDatum(rng, typ, false)
		s := d.String()
		if s[0] == '\'' {

nit: add a quick comment for why we need to do this.

---

pkg/sql/sem/tree/parse_string_test.go line 107 at r5 (raw file):

		{"423136fs", types.Date},
		{"", types.Decimal},
		{"not a decimnal", types.Decimal},

nit: s/decimnal/decimal/.

---

pkg/sql/sem/tree/parse_string_test.go line 181 at r5 (raw file):

	b.Run("datum", func(b *testing.B) {
		for i := 0; i < b.N; i++ {
			for _, tc := range benchCases {

Does the speed vary significantly for different types? Perhaps each bench case should be a separate benchmark run?

[cucaroach]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @rytaft, and @yuzefovich)

---

pkg/sql/sem/tree/parse_string.go line 274 at r4 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

I still don't see the enums :)

Done. Although my tests will come later in the copy testing patch, here's what they look like:

exec-ddl
CREATE TYPE testenum AS ENUM('cat','dog','bear');
CREATE TABLE tenum (i INT PRIMARY KEY,c1 testenum)
----

copy
COPY tenum FROM STDIN
0	cat
1	dog
2	bear
----
3

copy-kvtrace
COPY tenum FROM STDIN
0	cat
1	dog
2	bear
----
CPut /Table/<>/1/0/0 -> /TUPLE/2:2:Bytes/@
CPut /Table/<>/1/1/0 -> /TUPLE/2:2:Bytes/0x80
CPut /Table/<>/1/2/0 -> /TUPLE/2:2:Bytes/0xc0

exec-ddl
CREATE TABLE tenum2 (i INT PRIMARY KEY, c1 testenum, INDEX(c1))
----

copy
COPY tenum2 FROM STDIN
0	cat
----
1

copy-kvtrace
COPY tenum2 FROM STDIN
0	cat
----
CPut /Table/<>/1/0/0 -> /TUPLE/2:2:Bytes/@
InitPut /Table/<>/2/"@"/0/0 -> /BYTES/

exec-ddl
CREATE TABLE tenum3 (i INT PRIMARY KEY, c1 testenum, UNIQUE INDEX(c1))
----

copy
COPY tenum3 FROM STDIN
0	cat
----
1

copy-kvtrace
COPY tenum3 FROM STDIN
0	cat
----
CPut /Table/<>/1/0/0 -> /TUPLE/2:2:Bytes/@
InitPut /Table/<>/2/"@"/0 -> /BYTES/0x88

---

pkg/sql/sem/tree/parse_string_test.go line 181 at r5 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Does the speed vary significantly for different types? Perhaps each bench case should be a separate benchmark run?

Good idea! Output:

BenchmarkParseString
BenchmarkParseString/date
BenchmarkParseString/date-10             5820627               197.3 ns/op            16 B/op          1 allocs/op
BenchmarkParseString/bool
BenchmarkParseString/bool-10            65012752                18.58 ns/op            0 B/op          0 allocs/op
BenchmarkParseString/decimal
BenchmarkParseString/decimal-10         11046240               107.0 ns/op            40 B/op          2 allocs/op
BenchmarkParseString/float
BenchmarkParseString/float-10           39869371                29.47 ns/op            8 B/op          1 allocs/op
BenchmarkParseString/int
BenchmarkParseString/int-10             39210826                30.18 ns/op            8 B/op          1 allocs/op
BenchmarkParseString/interval
BenchmarkParseString/interval-10        14225242                81.95 ns/op           48 B/op          2 allocs/op
BenchmarkParseString/jsonb
BenchmarkParseString/jsonb-10            3849688               311.1 ns/op           400 B/op         11 allocs/op
BenchmarkParseString/string
BenchmarkParseString/string-10          60582729                17.36 ns/op           16 B/op          1 allocs/op
BenchmarkParseString/timestamptz
BenchmarkParseString/timestamptz-10      2409697               524.2 ns/op           944 B/op          3 allocs/op
BenchmarkParseString/vec/date
BenchmarkParseString/vec/date-10         6493965               182.3 ns/op             0 B/op          0 allocs/op
BenchmarkParseString/vec/bool
BenchmarkParseString/vec/bool-10        63239580                19.07 ns/op            0 B/op          0 allocs/op
BenchmarkParseString/vec/decimal
BenchmarkParseString/vec/decimal-10     13964929                84.75 ns/op            8 B/op          1 allocs/op
BenchmarkParseString/vec/float
BenchmarkParseString/vec/float-10       61221234                19.72 ns/op            0 B/op          0 allocs/op
BenchmarkParseString/vec/int
BenchmarkParseString/vec/int-10         81526585                14.66 ns/op            0 B/op          0 allocs/op
BenchmarkParseString/vec/interval
BenchmarkParseString/vec/interval-10    21017079                57.38 ns/op            0 B/op          0 allocs/op
BenchmarkParseString/vec/jsonb
BenchmarkParseString/vec/jsonb-10        3604507               333.7 ns/op           384 B/op         10 allocs/op
BenchmarkParseString/vec/string
BenchmarkParseString/vec/string-10      141814524                8.481 ns/op           0 B/op          0 allocs/op
BenchmarkParseString/vec/timestamptz
BenchmarkParseString/vec/timestamptz-10                  2721753               441.5 ns/op           896 B/op          1 allocs/op

[yuzefovich]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

[cucaroach]

Okay because ParseStringHandler isn't used anywhere yet I had to add a #keep to the BUILD.bazel, should pass CI now.

[cucaroach]

bors r+
TFTRs!

[craig]

This PR was included in a batch that was canceled, it will be automatically retried

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)