exec: Add framework for vectorized casts.

[rohany]

This PR adds in a framework for performing casts
within the vectorized engine, along with a few
casts implemented already. Additionally, it implements
a randomized testing framework for the cast operators.

Fixes #39372

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rohany, and @solongordon)

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 144 at r1 (raw file):

}

type castOverload struct {

this struct could be declared right after the overload struct

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 154 at r1 (raw file):

}

type castAssignFunc func(to, from string) string

nit: declare this type after where compareFunc is defined

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 163 at r1 (raw file):

	convStr := `
    %[1]s = *apd.New(int64(%[2]s), 0)
  `

i think castIdentity and intToDecimal may fit in better in a different file. e.g. look at pkg/sql/exec/float.go. maybe there should be a cast.go or something

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 167 at r1 (raw file):

}

var castOverloads map[types.T][]castOverload

i think this should be declared near to comparisonOpOverloads and those other similar maps. also, could use a comment.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 296 at r1 (raw file):

				case types.Decimal:
				case types.Int8:
				case types.Int16:

i think it would be better to just not put all the ones we don't support yet here. as it is now, we would be adding castOverloads that. have a nil AssignFunc

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 410 at r1 (raw file):

				castOverloads[from] = append(castOverloads[from], ov)
			}
		case types.Float64:

i see what you mean about not really liking this hahah...

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rohany, and @solongordon)

---

pkg/sql/distsqlrun/column_exec_setup.go, line 770 at r1 (raw file):

		return planProjectionExpr(ctx, t.Operator, t.ResolvedType(), t.TypedLeft(), t.TypedRight(), columnTypes, input)
	case *tree.CastExpr:
		op, resultIdx, ct, memUsed, err = planProjectionOperators(ctx, t.Expr.(tree.TypedExpr), columnTypes, input)

i didn't follow this part -- why is there a projection here?

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)

---

pkg/sql/distsqlrun/column_exec_setup.go, line 770 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i didn't follow this part -- why is there a projection here?

The cast expression holds onto an expression inside of it that needs to be evaluated, then casted.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 144 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this struct could be declared right after the overload struct

I thought about this, but I wanted to keep it and associated types separate, as it was a little different than the overload type.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 154 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: declare this type after where compareFunc is defined

ditto^

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 163 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think castIdentity and intToDecimal may fit in better in a different file. e.g. look at pkg/sql/exec/float.go. maybe there should be a cast.go or something

Thats a good idea -- we could consolidate all the casting functions in there, rather than having them laying around here.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 167 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this should be declared near to comparisonOpOverloads and those other similar maps. also, could use a comment.

ok.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 296 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think it would be better to just not put all the ones we don't support yet here. as it is now, we would be adding castOverloads that. have a nil AssignFunc

We don't emit a cast operator for a type if the AssignFunc is nil -- I prefer this way because then its clear what cast operations we have and don't have supported.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 410 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i see what you mean about not really liking this hahah...

:(

[rohany]

Friendly ping!

[jordanlewis]

Nice work on this very detail oriented issue. We need to figure out a way to test this systematically before we can merge it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @rohany, and @solongordon)

---

pkg/sql/distsqlrun/column_exec_setup.go, line 770 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

The cast expression holds onto an expression inside of it that needs to be evaluated, then casted.

Yeah, I think our use of the word "projection" here is mildly confusing, but a "projection" in vectorized-land is just the result of an evaluated expression. It's in opposition to a selection, which is the special case of a projection in a filter that we deal with differently via the selection vector.

---

pkg/sql/exec/cast_operator_tmpl.go, line 126 at r2 (raw file):

		if sel := batch.Selection(); sel != nil {
			for _, i := range sel {
				if vecNulls.NullAt(i) {

Don't we already have a function to copy all of the nulls from one vector to another? We should do that unconditionally at the top, and then unconditionally perform all of the casts. It will be much faster, because that way there won't be any conditionals in the loop which usually is a major slowdown for things like this.

That way you could also collapse this giant if/else tree by one, I think - at the top you'd just set all the nulls in a batch if MaybeHasNulls was true.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 270 at r2 (raw file):

	// Build cast overloads.
	castOverloads = make(map[types.T][]castOverload)

I think

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 274 at r2 (raw file):

	for _, from := range inputTypes {
		switch from {
		// TODO (rohany): How do we handle casting between different widths of the same type?

I think currently nobody will generate such a cast expression... it's a limitation of the "outer" type system.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 315 at r2 (raw file):

			// that a bytes type can implemented, but we don't know each of the
			// things is contained here. Additionally, we don't really know
			// what to do even if it is a bytes to bytes operation here.

Yeah, this is problematic. Eventually I think we need to delete our types package and roll all the relevant information into the "main" types package, so that functions like this one that need to actually know the semantic type (and not just the "physical type") can do the right thing.

But I think your idea is good for now - to error in every case :)

[rohany]

Do we do any template generated tests? Though in a sense that is only testing the things we want to test.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, and @solongordon)

---

pkg/sql/exec/cast_operator_tmpl.go, line 126 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Don't we already have a function to copy all of the nulls from one vector to another? We should do that unconditionally at the top, and then unconditionally perform all of the casts. It will be much faster, because that way there won't be any conditionals in the loop which usually is a major slowdown for things like this.

That way you could also collapse this giant if/else tree by one, I think - at the top you'd just set all the nulls in a batch if MaybeHasNulls was true.

I think so, the projections do something like this. However I still think we want to case on nulls -- we don't want to unconditionally cast something if a null is there, because that position in the vector could be full of garbage and lead to an error when doing the cast (like to an apd.Decimal)

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 274 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think currently nobody will generate such a cast expression... it's a limitation of the "outer" type system.

Alright, then we don't have to worry about it.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 315 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Yeah, this is problematic. Eventually I think we need to delete our types package and roll all the relevant information into the "main" types package, so that functions like this one that need to actually know the semantic type (and not just the "physical type") can do the right thing.

But I think your idea is good for now - to error in every case :)

I guess that is probably something better done after 19.2, to avoid introducing alot of bugs.

[rohany]

@jordanlewis LogicTests with force vectorize is probably the best way to test this right?

[rohany]

PTAL -- I added randomized testing for the case operators that I'm more confident about.

[yuzefovich]

Very nice work! Should we add a few casts into vectorize logic test file?

Reviewed 1 of 6 files at r2, 8 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, and @solongordon)

---

Makefile, line 791 at r3 (raw file):

  pkg/sql/exec/any_not_null_agg.eg.go \
  pkg/sql/exec/avg_agg.eg.go \
  pkg/sql/exec/cast_operator.eg.go \

[nit]: I think word "operator" is unnecessary in the name of the file.

---

pkg/sql/distsqlrun/column_exec_setup.go, line 770 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Yeah, I think our use of the word "projection" here is mildly confusing, but a "projection" in vectorized-land is just the result of an evaluated expression. It's in opposition to a selection, which is the special case of a projection in a filter that we deal with differently via the selection vector.

Maybe we should rename the method to planExpressionEvaluators then or something along those lines?

---

pkg/sql/distsqlrun/column_exec_setup.go, line 772 at r3 (raw file):

		outputIdx := len(ct)
		op, err = exec.GetCastOperator(op, resultIdx, outputIdx, t.Expr.(tree.TypedExpr).ResolvedType(), t.Type)
		if err != nil {

[super nit]: no need for this nil check.

---

pkg/sql/exec/cast_operator_test.go, line 56 at r3 (raw file):

		{types.Bool, datumAsBool, types.Float, datumAsFloat, false},
		// decimal -> t tests
		{types.Decimal, datumAsDecimal, types.Bool, datumAsBool, false},

It seems like two test cases are missing here.

---

pkg/sql/exec/cast_operator_test.go, line 63 at r3 (raw file):

		// float -> t tests
		{types.Float, datumAsFloat, types.Bool, datumAsBool, false},
		{types.Float, datumAsFloat, types.Int, datumAsInt, true},

[nit]: it might be worth explicitly calling out why we want to retry in this particular case.

---

pkg/sql/exec/cast_operator_test.go, line 68 at r3 (raw file):

	evalCtx := tree.NewTestingEvalContext(cluster.MakeTestingClusterSettings())
	rng := rand.New(rand.NewSource(timeutil.Now().Unix()))

[nit]: I think we tend to use either NewPseudoRand or randomly generate a seed and then use it when calling NewSource.

---

pkg/sql/exec/cast_operator_test.go, line 77 at r3 (raw file):

			output := tuples{}
			for i := 0; i < n; i++ {
				fromDatum := sqlbase.RandDatum(rng, c.fromTyp, false)

[nit]: it'd be nice to mention what false here is argument for. I think it's whether a null can be generated, so if we're disabling it, please leave a comment why.

---

pkg/sql/exec/cast_operator_test.go, line 99 at r3 (raw file):

			runTests(t, []tuples{input}, output, orderedVerifier, []int{1},
				func(input []Operator) (Operator, error) {
					return GetCastOperator(input[0], 0, 1, c.fromTyp, c.toTyp)

[super nit]: same thing for argument 0 and 1.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 296 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

We don't emit a cast operator for a type if the AssignFunc is nil -- I prefer this way because then its clear what cast operations we have and don't have supported.

I agree with Rafi here, to me it's more confusing to enumerate all the types here. Intuitively, I'd think if I see a type mentioned here, I'd assume such a cast is supported. In order to see whether it is supported or not, one would need to actually read the code, and the convention of having a nil assign function for unsupported types can lead to the confusion as well.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 189 at r3 (raw file):

		convStr := `
			if math.IsNaN(float64(%[2]s)) || %[2]s <= float%[4]d(math.MinInt%[3]d) || %[2]s >= float%[4]d(math.MaxInt%[3]d) {
				panic("Cannot float to int conversion out of range")

Use execerror.NonVectorizedPanic (and adjust the error message).

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 207 at r3 (raw file):

	// TODO (rohany): How do we propagate this error -- is panicing ok?
	convStr := `
		var _dd apd.Decimal

Yeah, panicking is the way to propagate the error.

[nit]: consider s/_dd/tmpDec/ or something like that.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 319 at r3 (raw file):

	// Build cast overloads.
	castOverloads = make(map[coltypes.T][]castOverload)
	_ = castOverloads

Why do we need this line?

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 328 at r3 (raw file):

				case coltypes.Bool:
					ov.AssignFunc = castIdentity
				case coltypes.Bytes:

[nit]: I think we can just omit cases that we do not support (with a comment).

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 331 at r3 (raw file):

				case coltypes.Decimal:
				case coltypes.Int8:
					fallthrough

[nit]: instead of falling through, I'd use a case with multiple options, i.e. case coltypes.Int8, coltypes.Int16, ...

---

pkg/sql/sem/tree/datum.go, line 906 at r3 (raw file):

		return *t
	}
	panic(errors.AssertionFailedf("expected *DFloat, found %T", e))

s/DFloat/DDecimal/.

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/distsqlrun/column_exec_setup.go, line 772 at r3 (raw file):

Previously, yuzefovich wrote…

[super nit]: no need for this nil check.

Done.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 296 at r1 (raw file):

Previously, yuzefovich wrote…

I agree with Rafi here, to me it's more confusing to enumerate all the types here. Intuitively, I'd think if I see a type mentioned here, I'd assume such a cast is supported. In order to see whether it is supported or not, one would need to actually read the code, and the convention of having a nil assign function for unsupported types can lead to the confusion as well.

Ok. I removed all cases that don't have a cast supported.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 189 at r3 (raw file):

Previously, yuzefovich wrote…

Use execerror.NonVectorizedPanic (and adjust the error message).

Done.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 207 at r3 (raw file):

Previously, yuzefovich wrote…

Yeah, panicking is the way to propagate the error.

[nit]: consider s/_dd/tmpDec/ or something like that.

Done.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 319 at r3 (raw file):

Previously, yuzefovich wrote…

Why do we need this line?

good question.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 328 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: I think we can just omit cases that we do not support (with a comment).

ok.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 331 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: instead of falling through, I'd use a case with multiple options, i.e. case coltypes.Int8, coltypes.Int16, ...

oh cool -- i didn't know that go supported this.

---

pkg/sql/sem/tree/datum.go, line 906 at r3 (raw file):

Previously, yuzefovich wrote…

s/DFloat/DDecimal/.

Done.

---

pkg/sql/exec/cast_operator_test.go, line 56 at r3 (raw file):

Previously, yuzefovich wrote…

It seems like two test cases are missing here.

These casts are not implement right now.

---

pkg/sql/exec/cast_operator_test.go, line 63 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: it might be worth explicitly calling out why we want to retry in this particular case.

Done.

---

pkg/sql/exec/cast_operator_test.go, line 68 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: I think we tend to use either NewPseudoRand or randomly generate a seed and then use it when calling NewSource.

Done.

---

pkg/sql/exec/cast_operator_test.go, line 77 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: it'd be nice to mention what false here is argument for. I think it's whether a null can be generated, so if we're disabling it, please leave a comment why.

Done.

---

pkg/sql/exec/cast_operator_test.go, line 99 at r3 (raw file):

Previously, yuzefovich wrote…

[super nit]: same thing for argument 0 and 1.

Done.

[rohany]

Thanks for the review yahor! I updated the PR, and added a logic test. I'm not sure how to force casts to get planned.

[rohany]

PTAL

[yuzefovich]

Thanks! It but I'd like for Rafi to take a look as well before merging this.

I'm not sure how to force the casts, maybe something like this SELECT 1::INT2 < 2::INT2.

Reviewed 10 of 10 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/exec/cast_operator_test.go, line 56 at r3 (raw file):

Previously, rohany (Rohan Yadav) wrote…

These casts are not implement right now.

I see.

---

pkg/sql/exec/cast_operator_test.go, line 99 at r3 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Done.

I meant:
return GetCastOperator(input[0], 0 /* inputIdx */, 1 /* outputIdx */, c.fromTyp, c.toTyp).
The rule of thumb that I'm trying to follow is that if GoLand shows you the name of the argument, then that name should be added as an inlined comment.

[rohany]

Oh i see -- I'm too used to GoLand filling in the name there.

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/exec/cast_operator_test.go, line 99 at r3 (raw file):

Previously, yuzefovich wrote…

I meant:
return GetCastOperator(input[0], 0 /* inputIdx */, 1 /* outputIdx */, c.fromTyp, c.toTyp).
The rule of thumb that I'm trying to follow is that if GoLand shows you the name of the argument, then that name should be added as an inlined comment.

I think you slightly missed the arguments to comment on :)

---

pkg/sql/exec/cast_tmpl.go, line 20 at r5 (raw file):

// */}}

package exec

[nit]: s/cast_operator/cast/ (above).

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/exec/cast_operator_test.go, line 99 at r3 (raw file):

Previously, yuzefovich wrote…

I think you slightly missed the arguments to comment on :)

The go formatter is out to get me ... it reordered the comments and the comma

---

pkg/sql/exec/cast_tmpl.go, line 20 at r5 (raw file):

Previously, yuzefovich wrote…

[nit]: s/cast_operator/cast/ (above).

Done.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 176 at r6 (raw file):

}

func intToFloat(floatSize int) func(string, string) string {

have you thought of a way for these functions to be used in a similar way to the typeCustomizers used by the other overloads?

i think the idea would be that below, instead of ranging over the input types and having a case statement for each type, the "cast type customizers" could be registered into some static map, and the loop below could look up in the map. if it seems like this would be an annoying refactor to do after getting everything to work how it is now, i'm fine with the "type customizer" approach not happening. i could take a stab at it later.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 189 at r6 (raw file):

errors.New("Cannot perform float to int conversion: out of range")

i think the error here should be execerror.NonVectorizedPanic(tree.ErrIntOutOfRange) (to remain consistent with the error currently produced by the non-vec engine)

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 193 at r6 (raw file):

			%[1]s = int%[3]d(%[2]s)
		`
		return fmt.Sprintf(convStr, to, from, intSize, floatSize)

instead of Sprintf, this might be a bit more readable with the inline template approach, which is used for some of the binary op overloads in this file. your call if you think it's worth changing.

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 176 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

have you thought of a way for these functions to be used in a similar way to the typeCustomizers used by the other overloads?

i think the idea would be that below, instead of ranging over the input types and having a case statement for each type, the "cast type customizers" could be registered into some static map, and the loop below could look up in the map. if it seems like this would be an annoying refactor to do after getting everything to work how it is now, i'm fine with the "type customizer" approach not happening. i could take a stab at it later.

I thought about it a little bit, but I didn't think it would save that much on readability, because each one of the customizers would have to do a full case inside anyway. But it might be a good idea.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 189 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

errors.New("Cannot perform float to int conversion: out of range")

i think the error here should be execerror.NonVectorizedPanic(tree.ErrIntOutOfRange) (to remain consistent with the error currently produced by the non-vec engine)

Ok, let me fix this -- good catch.

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 193 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

instead of Sprintf, this might be a bit more readable with the inline template approach, which is used for some of the binary op overloads in this file. your call if you think it's worth changing.

I didn't think it was that worth using the templates due to the pretty short code size of these snippets. I imagine for the more complicated cast operations we should use the templates.

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @jordanlewis, @rafiss, @rohany, @solongordon, and @yuzefovich)

---

pkg/sql/exec/execgen/cmd/execgen/overloads.go, line 206 at r7 (raw file):

func floatToDecimal(to, from string) string {
	convStr := `
		var tmpDec apd.Decimal

to be extra safe, what i've been doing is introducing a new scope if a new variable in one of these code-generated blocks. that way there won't be an issue with name collisions. it's not super necessary though, so i'm fine with merging. (e.g. look at floatDecimalCustomizer)

[rohany]

Oh thats a great idea -- I'll do that first.

[rohany]

TFTR!

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)