colexec: add support for LShift and RShift operators

[tancredosouza]

Previously, there was no support for LShift (<<) and RShift (>>) in the vectorized engine.
This commit implements these operators.

Fixes #49467.
Fixes #49468.

Release note (sql change): Vectorized execution engine now supports binary shift (>> and <<) operators.

[cockroach-teamcity]

All committers have signed the CLA.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

• @yuzefovich (author of colexec: add support for RShift binary operator #49468, author of colexec: add support for LShift binary operator #49467)

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

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[yuzefovich]

Thanks for working on this! The code is looking very good, I just have a couple of nits and a suggestion for the full support.

Also, take a look at the message from our blathers bot - all three commits should be squashed into one, and we need to add a release note both to the commit and the PR description (something like
"Release note (sql change): Vectorized execution engine now supports binary shift (>> and <<) operators.").

Reviewed 4 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tancredosouza)

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 27 at r1 (raw file):

)

var binaryOpIntMethod = map[tree.BinaryOperator]string{

nit: since these values are the same as in execgen.BinaryOpName map, I think we could reuse that map and avoid declaring this one.

Update: I see that you updated it in the second commit, nice! Never mind then.

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 184 at r1 (raw file):

	for _, binOp := range []tree.BinaryOperator{tree.LShift, tree.RShift} {
		binOpOutputTypes[binOp] = make(map[typePair]*types.T)

nit: I think you can use populateBinOpIntOutputTypeOnIntArgs method here.

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 182 at r3 (raw file):

		for _, intWidth := range supportedWidthsByCanonicalTypeFamily[types.IntFamily] {
			binOpOutputTypes[binOp][typePair{types.IntFamily, intWidth, types.IntFamily, anyWidth}] = types.Int
		}

As you noticed previously, we also support shift operators when the left argument is either VarBit or INet. In order for datumIntCustomizer to pick up the support of shift operators, we need to register the output types. I think you could figure it out, but please let me know if you need some more specific guidance.

---

pkg/sql/logictest/testdata/logic_test/vectorize_overloads, line 337 at r3 (raw file):

query I rowsort
SELECT _int >> 63 FROM many_types

We should also add a few tests to make sure that VarBit and INet types on the left are supported.

---

pkg/sql/sem/tree/eval.go, line 59 at r1 (raw file):

	errSqrtOfNegNumber = pgerror.New(pgcode.InvalidArgumentForPowerFunction, "cannot take square root of a negative number")

	// ErrShiftArgOutOfRange is reported on a shift argument out of range.

nit: I'd slightly adjust the comment, "reported when a binary shift argument is out of range".

[yuzefovich]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tancredosouza)

---

pkg/sql/colexec/execgen/cmd/execgen/overloads_bin.go, line 182 at r3 (raw file):

Previously, yuzefovich wrote…

As you noticed previously, we also support shift operators when the left argument is either VarBit or INet. In order for datumIntCustomizer to pick up the support of shift operators, we need to register the output types. I think you could figure it out, but please let me know if you need some more specific guidance.

Actually, it might be a little tricky - I think we can easily add the support for VarBit but not for INet. The difficulty is that INet >> INet return bool which is not a datum-backed type, and we currently don't support such binary operations. So please take a stab at adding VarBit support.

[tancredosouza]

Thanks for reviewing my PR!
I've written some SQL Logic Tests with _varbit arguments. With the code as-is, the tests ran successfully.

query T
SELECT _varbit >> 1 FROM many_types
----
NULL
0
01

query T
SELECT _varbit << 1 FROM many_types
----
NULL
0
10

However, I was expecting some error message since I supposedly didn't register outputTypes for VarBit. Maybe I'm indeed not clear about how the datumIntCustomizer would pick up support the shift operation.

What do you think? Thanks again for your suggestions.

[yuzefovich]

What's happening is that the vectorized engine doesn't support shift operations on varbits and falls back to using old row-by-row execution engine. This occurs transparently for the user, so simply by running the query you don't see it. You need to use EXPLAIN (VEC) and then the fallback becomes clear:

query T
EXPLAIN (VEC) SELECT _varbit >> 1 FROM many_types
----
│
└ Node 1
  └ *rowexec.tableReader

That's the reason for having many EXPLAIN (VEC) queries in this file - making sure that we are actually using the vectorized engine.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tancredosouza)

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[tancredosouza]

Hello there. I have fixed the code per your suggestions, implemented support for VarBIt operator and added some tests.

[yuzefovich]

Thanks for your contribution!

I'll merge this once the build is green.

Reviewed 3 of 3 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained

[yuzefovich]

bors r=yuzefovich

[craig]

Build succeeded

• GitHub CI (Cockroach)

[SWDevAngel]

@tancredosouza Hi Tan. Thank you for contributing to CockroachDB this year. As a token of our appreciation we would like to send you a gift. Please DM me on our community Slack @lisa so I can send you a link. (If you don’t want a gift, we also have a charitable contribution choice.) All orders need to be in by December 13, so please send this asap. :)