colexec: fix type management in execplan

[yuzefovich]

This commit fixes the column type management in execplan.go by
requiring that we allocate new []types.T slice whenever a projecting
operator wants to append a column to the passed-in type schema. Previous
behavior could result in "type schema corruption" (when it was captured
by batchSchemaPrefixEnforcer and possibly in other places), and this
is now fixed. The requirement is enforced by a linter rule that
prohibits code lines that have yps = append( or ypes = append(
inside (this is not perfect, but it should be good enough).

Fixes: #47889.

Release note (bug fix): Previously, CockroachDB could return an internal
error when performing a query with CASE, AND, OR operators in some cases
when it was executed via the vectorized engine, and this has been fixed.

[cockroach-teamcity]

This change is 

[yuzefovich]

My guess is that there have been some recent changes in the optimizer so that a new plan is now generated which exposed this problem (I didn't fully investigate this). The problem itself was introduced when we added batchSchemaPrefixEnforcer.

[yuzefovich]

cc @jordanlewis

[yuzefovich]

Updated the code as we discussed.

[asubiotto]

Reviewed 2 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @yuzefovich)

---

pkg/sql/colexec/execplan.go, line 218 at r1 (raw file):

				return false, errors.Newf("aggregates with arguments not supported")
			}
			inputTypes := make([]types.T, len(agg.ColIdx))

I think we should keep cases like this untouched. I would say we only pass in a copy if those types are referenced for later use? That's maybe not as clear though.

---

pkg/sql/colexec/execplan.go, line 523 at r1 (raw file):

// described below.
//
// Without explicit new allocations, tt is possible that planSelectionOperators

nit: s/tt/it

[yuzefovich]

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

---

pkg/sql/colexec/execplan.go, line 218 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I think we should keep cases like this untouched. I would say we only pass in a copy if those types are referenced for later use? That's maybe not as clear though.

It's quite unclear - this would imply that the caller (execplan.go) would need to know whether each callee (constructors of operators) stores the passed-in types and how those are used. I think it could be error-prone, and the situation would be pretty much the same as before this PR.

I agree it is a little unfortunate that cases like this need to be modified, but I'd rather lean on the safe side and enforce the linter rule throughout this file rather than trying to decide whether a copy is necessary or not - always make a copy and don't think about it. The performance implications should be very minor in the grand scheme of things.

[asubiotto]

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

[yuzefovich]

TFTR!

bors r+

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

---

pkg/sql/colexec/execplan.go, line 523 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

nit: s/tt/it

Done.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 7253fa45.

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

[yuzefovich]

CI flake.

bors r+

[yuzefovich]

bors r-

[yuzefovich]

bors r+

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 01860973.

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

[craig]

Build succeeded

• GitHub CI (Cockroach)