opt: prevent overflow when normalizing comparison with constants

[mgartner]

opt: prevent overflow when normalizing comparison with constants

Previously, the NormalizeCmpPlusConst, NormalizeCmpMinusConst, and
NormalizeCmpMinusConst would normalize expressions in the form
var +/- const1 ? const2 and const1 - var ? const2, where const1
and const2 were const constants and ? is any comparison operator, to
attempt to isolate the variable on one side of the comparison. Below are
some examples of the transformations these rules made:

a+1 > 10 =>  a > 10-1
a-1 > 10 =>  a > 10+1
1-a > 10 =>  1-10 > a

However, these transformations were invalid when the newly created
addition or subtraction operators overflowed. In the case of an
expression involving TIME and INTERVAL types, this could cause incorrect
query results. For example:

t-'2 hr'::INTERVAL < '23:00:00'::TIME
=>
t < '23:00:00'::TIME+'2 hr'::INTERVAL
=>
t < '01:00:00'::TIME

The first expression and last expression have different semantic meaning
because the addition of the constant TIME and constant INTERVAL
overflowed.

This commit prevents the rules from transforming expressions if they
would cause an overflow/underflow. In order to detect overflow, there
are new restrictions that prevent these rules from firing in some cases.
Notably, the datum on the RHS of the newly constructed +/- operator must
be an integer, float, decimal, or interval. Also, the result type of the
newly constructed +/- operator must be equivalent to the LHS of the
operator. Both of these restrictions are required in order to detect
overflow/underflow.

Informs #88128

Release note (bug fix): A bug has been fixed that caused incorrect
evaluation of expressions in the form col +/- const1 ? const2, where
const1 and const2 are constant values and ? is any comparison
operator. The bug was caused by operator overflow when the optimizer
attempted to simplify these expressions to have a single constant value.

normalize: remove invalid normalizations

It was recently discovered that normalization rules in the optimizer
were invalid (see #88128). These rules were adapted from the heuristic
optimizer several years ago, and they still remain in the normalize
package even though the heuristic optimizer no longer exists. The
normalize package is still used to normalize expressions during
backfilling, so the invalid rules can cause incorrect computed column
values and corrupt indexes. This commit removes these rules entirely.

Informs #88128

Release note: None

[cockroach-teamcity]

This change is 

[msirek]

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

---

pkg/sql/logictest/testdata/logic_test/time line 566 at r1 (raw file):

SELECT t + '02:00:00'::INTERVAL < '01:00:00'::TIME FROM t88128
----
false

We may need to check for overflow and underflow for both + and -, for example this test still shows incorrect results:

query B
SELECT t + '-18:00:00'::INTERVAL < '07:00:00'::TIME FROM t88128
----
true

[DrewKimball]

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

---

pkg/sql/opt/norm/comp_funcs.go line 44 at r1 (raw file):

//  1. An overload function for the given operator and input types exists and
//     has an appropriate volatility.
//  2. The result type of the overload is equivalent to the type of left. This

Why is this condition necessary? I don't think the datum comparison functions require the same type, at least in general.

[mgartner]

pkg/sql/opt/norm/comp_funcs.go line 44 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Why is this condition necessary? I don't think the datum comparison functions require the same type, at least in general.

They must be comparable in order to check for overflow. For example, consider:

CREATE TABLE t (i INTERVAL);
SELECT '1:00:00'::TIME + i >= '2:00:00'::TIME;

With this tranformation, we try to compute '2:00:00'::TIME - '1:00:00'::TIME, which results in an INTERVAL, not a TIME. Because an INTERVAL cannot be compared with a TIME, the overflow detection won't work.

Perhaps when a subtraction yields a different type than the operands, it is guaranteed there is no overflow? I didn't spend too much time thinking through if this is true generally, but if you have any insight, let me know. I thought for now I'd err on the side of overly restrictive (I even considered removing these rules entirely since I don't believe it's common to write expressions in this form and I'm weary of other bugs lurking here). I could leave a TODO mentioning that this restriction might be able to be lifted, if you'd like.

[DrewKimball]

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

---

pkg/sql/opt/norm/comp_funcs.go line 44 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

They must be comparable in order to check for overflow. For example, consider:

CREATE TABLE t (i INTERVAL);
SELECT '1:00:00'::TIME + i >= '2:00:00'::TIME;

With this tranformation, we try to compute '2:00:00'::TIME - '1:00:00'::TIME, which results in an INTERVAL, not a TIME. Because an INTERVAL cannot be compared with a TIME, the overflow detection won't work.

Perhaps when a subtraction yields a different type than the operands, it is guaranteed there is no overflow? I didn't spend too much time thinking through if this is true generally, but if you have any insight, let me know. I thought for now I'd err on the side of overly restrictive (I even considered removing these rules entirely since I don't believe it's common to write expressions in this form and I'm weary of other bugs lurking here). I could leave a TODO mentioning that this restriction might be able to be lifted, if you'd like.

Oh, I didn't realize INTERVAL and TIME are incomparable. I think it's fine to leave as is, in that case. Thanks for explaining.

[mgartner]

I added a new commit which fixes the same bug in the normalize package (a remnant of the old heuristic optimizer that is still used during backfills). PTAL!

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

---

pkg/sql/logictest/testdata/logic_test/time line 566 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

We may need to check for overflow and underflow for both + and -, for example this test still shows incorrect results:

query B
SELECT t + '-18:00:00'::INTERVAL < '07:00:00'::TIME FROM t88128
----
true

Woof, good catch!

I think the only way to detect underflow and overflow in these cases is to know if the RHS of the new +/- expression is > 0 or < 0. I've added in logic to detect overflow and underflow for all three rules, but I had to add the additional restriction that the RHS of the new +/- expression is an integer, float, decimal, or interval - because its easy to create zero values for those types. It may be possible to expand this set of valid types in the future, but I think this is good enough for now.

[DrewKimball]

Reviewed 2 of 7 files at r1, 6 of 6 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @msirek)

[msirek]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @msirek)

---

pkg/sql/opt/norm/testdata/rules/comp line 132 at r2 (raw file):

      └── (s:4::DATE + '02:00:00') = '2000-01-01 02:00:00' [outer=(4), stable]

# The rule should not apply if the type of RHS the created Minus operator is a

nit: 'is a' -> 'is'

[mgartner]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball, @michae2, and @msirek)

---

pkg/sql/opt/norm/comp_funcs.go line 44 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

Oh, I didn't realize INTERVAL and TIME are incomparable. I think it's fine to leave as is, in that case. Thanks for explaining.

Done.

---

pkg/sql/opt/norm/testdata/rules/comp line 132 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

nit: 'is a' -> 'is'

Done

[DrewKimball]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @michae2, and @msirek)

[mgartner]

TFTRs!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 22a967e to blathers/backport-release-21.2-88199: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

---

error creating merge commit from 22a967e to blathers/backport-release-22.1-88199: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

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