sql/opt: handle error when casting to anonymous tuple

[rafiss]

v22.1 will include more support for tuple casts, so it's no longer
correct to skip this cast in the optimizer. This fixes a bug when casting to
an anonymous record type.

To fully support this, the type-checking logic for same-typed tuple
expressions had to be made a little smarter.

No release note since the bug was never released.

refs ab420f0

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

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

---

-- commits, line 15 at r3:
[nit] commit msg needs fixing

---

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

	typedSubExprs, retType, err := TypeCheckSameTypedExprs(ctx, semaCtx, desired, expr.Exprs...)
	if err != nil {
		if expr.Name == "COALESCE" {

remove this

---

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

	// Handle tuples, which will in turn call into this function recursively for each element.
	//if _, ok := exprs[0].(*Tuple); ok {

needs cleanup

---

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

	first := exprs[0].(*Tuple)
	if err := checkAllExprsAreTuplesOrNulls(ctx, semaCtx, typedExprs[1:]); err != nil {
		//if err := checkAllExprsAreTuplesOrNulls(ctx, semaCtx, exprs[1:]); err != nil {

needs cleanup

---

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

	firstLen := len(first.Exprs)
	if err := checkAllTuplesHaveLength(typedExprs[1:], firstLen); err != nil {
		//if err := checkAllTuplesHaveLength(exprs[1:], firstLen); err != nil {

needs cleanup

---

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

		resTypes.TupleContents()[elemIdx] = resType
	}
	//for tupleIdx, expr := range exprs {

needs cleanup

---

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

// checkAllExprsAreTuplesOrNulls checks that all expressions in exprs are
// either tuples or nulls.
/*

needs cleanup

---

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

}
*/
func checkAllExprsAreTuplesOrNulls(

[nit] needs a comment

---

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

	for _, expr := range exprs {
		_, isTuple := expr.(*Tuple)
		isTuple = isTuple || expr.ResolvedType().Family() == types.TupleFamily

[nit] can't we always useResolvedType()?

---

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

		length = len(tuple.Exprs)
	} else {
		length = len(expr.ResolvedType().TupleContents())

[nit] Why can't we always use the ResolvedType() ?

[rafiss]

Sorry, I didn't intend for this to be reviewed yet. I removed the reviewers since I'm still hacking around to understand which parts of the type-checking logic we still need. Thanks for taking a look!

[rafiss]

This is ready for another look.

[mgartner]

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, 2 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rafiss)

---

pkg/sql/logictest/testdata/logic_test/builtin_function, line 2583 at r5 (raw file):

----

# Adding a cast shouldn't affect the type-checking.

Would logictest/tuple or logictest/cast be a better home for these tests?

---

pkg/sql/opt/exec/execbuilder/scalar.go, line 114 at r5 (raw file):

}

func (b *Builder) buildNull(ctx *buildScalarCtx, scalar opt.ScalarExpr) (tree.TypedExpr, error) {

nit: function signature can be simplified by removing error.

[rafiss]

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

---

pkg/sql/logictest/testdata/logic_test/builtin_function, line 2583 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Would logictest/tuple or logictest/cast be a better home for these tests?

yeah, probably. i initially had it here since all these tests do require a builtin and they are based on the test right before these ones

---

pkg/sql/opt/exec/execbuilder/scalar.go, line 114 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: function signature can be simplified by removing error.

i tried to make this change, but it's not possible since this function is used in scalarBuildFuncMap, which requires functions of type buildFunc

[mgartner]

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

---

pkg/sql/opt/exec/execbuilder/scalar.go, line 114 at r5 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i tried to make this change, but it's not possible since this function is used in scalarBuildFuncMap, which requires functions of type buildFunc

👍

[rafiss]

tftr!

bors r=mgartner

[craig]

Build succeeded:

• GitHub CI (Cockroach)