From b426d3d7f1f7fa2b3cbd62e9a9dcb00a1e1586a6 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 19 Oct 2022 14:24:00 -0400 Subject: [PATCH] opt: fix normalization of comparisons with constants A prior commit in #88199 attempted to fix a bug in the `NormalizeCmpPlusConst`, `NormalizeCmpMinusConst`, and `NormalizeCmpConstMinus` rules by checking for overflow/underflow in the addition/subtraction of constants in a comparison expression. This was insufficient to completely fix the bug because the transformation is invalid if the non-normalized expression would have overflowed. Consider an expression: t::TIME + '-11 hrs'::INTERVAL > '01:00'::TIME `NormalizeCmpPlusConst` would successively normalize it to this: t::TIME > '01:00'::TIME - '-11 hrs'::INTERVAL => t::TIME > '12:00'::TIME This expression is not semantically equivalent to the original expression. It yields different results when `t` is a value that would underflow when eleven hours is subtracted from it. For example, consider `t = '03:00'::TIME`: Original expression: t::TIME + '-11 hrs'::INTERVAL > '01:00'::TIME => '03:00'::TIME + '-11 hrs'::INTERVAL > '01:00'::TIME => '16:00'::TIME > '01:00'::TIME => true Normalized expression: t::TIME > '12:00'::TIME => '03:00'::TIME > '12:00'::TIME => false These normalization rules are only valid with types where overflow or underflow during addition and subtraction results in an error. This commit restricts these normalization rules to only operate on integers, floats, and decimals, which will error if there is underflow or overflow. Fixes #90053 Release note (bug fix): A bug has been fixed that caused incorrect evaluation of comparison expressions involving time and interval types, like `col::TIME + '10 hrs'::INTERVAL' > '01:00'::TIME`. --- pkg/sql/logictest/testdata/logic_test/time | 16 ++ pkg/sql/opt/norm/comp_funcs.go | 80 +++----- pkg/sql/opt/norm/rules/comp.opt | 29 +-- pkg/sql/opt/norm/testdata/rules/comp | 209 ++++----------------- pkg/sql/sem/tree/datum.go | 3 - 5 files changed, 93 insertions(+), 244 deletions(-) diff --git a/pkg/sql/logictest/testdata/logic_test/time b/pkg/sql/logictest/testdata/logic_test/time index c9e300d0a3db..31bdcc44a315 100644 --- a/pkg/sql/logictest/testdata/logic_test/time +++ b/pkg/sql/logictest/testdata/logic_test/time @@ -580,3 +580,19 @@ query B SELECT t + '-18:00:00'::INTERVAL < '07:00:00'::TIME FROM t88128 ---- true + +subtest regression_90053 + +# Regression tests for #90053. Do not normalize comparisons with constants when +# addition/subtraction of the types involved could overflow without an error. +query B +SELECT '00:01:40.01+09:00:00' < (col::TIMETZ + '-83 years -1 mons -38 days -10:32:23.707137') +FROM (VALUES ('03:16:01.252182+01:49:00')) v(col); +---- +true + +query B +SELECT t::TIME + '-11 hrs'::INTERVAL > '01:00'::TIME +FROM (VALUES ('03:00')) v(t) +---- +true diff --git a/pkg/sql/opt/norm/comp_funcs.go b/pkg/sql/opt/norm/comp_funcs.go index 91a8dfd7171e..64af17df0032 100644 --- a/pkg/sql/opt/norm/comp_funcs.go +++ b/pkg/sql/opt/norm/comp_funcs.go @@ -44,47 +44,43 @@ func (c *CustomFuncs) CommuteInequality( panic(errors.AssertionFailedf("called commuteInequality with operator %s", redact.Safe(op))) } -// FoldBinaryCheckOverflow attempts to evaluate a binary expression with +// ArithmeticErrorsOnOverflow returns true if addition or subtraction with the +// given types will cause an error when the value overflows or underflows. +func (c *CustomFuncs) ArithmeticErrorsOnOverflow(left, right *types.T) bool { + switch left.Family() { + case types.IntFamily, types.FloatFamily, types.DecimalFamily: + default: + return false + } + switch right.Family() { + case types.IntFamily, types.FloatFamily, types.DecimalFamily: + default: + return false + } + return true +} + +// FoldBinaryCheckNull attempts to evaluate a binary expression with // constant inputs. The only operations supported are plus and minus. It returns // a constant expression if all the following criteria are met: // -// 1. The right datum is an integer, float, decimal, or interval. This -// restriction can be lifted for any type that we can construct a zero value -// of. The zero value of the right type is required in order to check for -// overflow/underflow (see #5). -// 2. An overload function for the given operator and input types exists and +// 1. An overload function for the given operator and input types exists and // has an appropriate volatility. -// 3. The result type of the overload is equivalent to the type of left. This -// is required in order to check for overflow/underflow (see #5). -// 4. The evaluation causes no error. -// 5. The evaluation does not overflow or underflow. +// 2. There is no error when evaluating the binary expression. // // If any of these conditions are not met, it returns ok=false. -func (c *CustomFuncs) FoldBinaryCheckOverflow( +func (c *CustomFuncs) FoldBinaryCheckNull( op opt.Operator, left, right opt.ScalarExpr, ) (_ opt.ScalarExpr, ok bool) { - var zeroDatumForRightType tree.Datum - switch right.DataType().Family() { - case types.IntFamily, types.FloatFamily, types.DecimalFamily: - zeroDatumForRightType = tree.DZero - case types.IntervalFamily: - zeroDatumForRightType = tree.DZeroInterval - default: - // Any other type families of right are not supported. - return nil, false - } - o, ok := memo.FindBinaryOverload(op, left.DataType(), right.DataType()) if !ok || !c.CanFoldOperator(o.Volatility) { return nil, false } - if !o.ReturnType.Equivalent(left.DataType()) { - // We can only check for overflow or underflow when the result type - // matches the type of left. - return nil, false - } - lDatum, rDatum := memo.ExtractConstDatum(left), memo.ExtractConstDatum(right) + // TODO(mgartner): FoldBinaryCheckNull is similar to FoldBinary, except + // for this NULL check. The NULL check might not be necessary since we no + // longer use this function on TIME and INTERVAL types, so maybe we can + // reuse FoldBinary instead. if lDatum == tree.DNull || rDatum == tree.DNull { return nil, false } @@ -92,34 +88,6 @@ func (c *CustomFuncs) FoldBinaryCheckOverflow( if err != nil { return nil, false } - - cmpResLeft, err := result.CompareError(c.f.evalCtx, lDatum) - if err != nil { - return nil, false - } - - cmpRightZero, err := rDatum.CompareError(c.f.evalCtx, zeroDatumForRightType) - if err != nil { - return nil, false - } - - // If the operator is + and right is <0, check for underflow. - if op == opt.PlusOp && cmpRightZero < 0 && cmpResLeft > 0 { - return nil, false - } - // If the operator is + and right is >=0, check for overflow. - if op == opt.PlusOp && cmpRightZero >= 0 && cmpResLeft < 0 { - return nil, false - } - // If the operator is - and right is <0, check for overflow. - if op == opt.MinusOp && cmpRightZero < 0 && cmpResLeft < 0 { - return nil, false - } - // If the operator is - and right is >=0, check for underflow. - if op == opt.MinusOp && cmpRightZero >= 0 && cmpResLeft > 0 { - return nil, false - } - // The operation did not overflow or underflow. return c.f.ConstructConstVal(result, o.ReturnType), true } diff --git a/pkg/sql/opt/norm/rules/comp.opt b/pkg/sql/opt/norm/rules/comp.opt index 3fe3f4fb4d5b..3cde9f0ae864 100644 --- a/pkg/sql/opt/norm/rules/comp.opt +++ b/pkg/sql/opt/norm/rules/comp.opt @@ -29,16 +29,11 @@ # The rule can only perform this transformation if all of the following criteria # are met: # -# 1. $leftRight is an integer, float, decimal, or interval. This restriction -# can be lifted for any type that we can construct a zero value of. The -# zero value of the right type is required in order to check for -# overflow/underflow (see #5). +# 1. The generated Minus expression will error if there is an overflow (see +# ArithmeticErrorsOnOverflow). # 2. A Minus overload for the given input types exists and has an appropriate # volatility. -# 3. The result type of the overload is equivalent to the type of $right. This -# is required in order to check for overflow/underflow (see #5). -# 4. The evaluation of the Minus operator causes no error. -# 5. The evaluation of the Minus operator does not overflow or underflow. +# 2. There is no error when evaluating the new binary expression. # # NOTE: Ne is not part of the operator choices because it wasn't handled in # normalize.go either. We can add once we've proved it's OK to do so. @@ -46,9 +41,13 @@ (Eq | Ge | Gt | Le | Lt (Plus $leftLeft:^(ConstValue) $leftRight:(ConstValue)) $right:(ConstValue) & + (ArithmeticErrorsOnOverflow + (TypeOf $right) + (TypeOf $leftRight) + ) & (CanConstructBinary Minus $right $leftRight) & (Let - ($result $ok):(FoldBinaryCheckOverflow + ($result $ok):(FoldBinaryCheckNull Minus $right $leftRight @@ -72,9 +71,13 @@ (Eq | Ge | Gt | Le | Lt (Minus $leftLeft:^(ConstValue) $leftRight:(ConstValue)) $right:(ConstValue) & + (ArithmeticErrorsOnOverflow + (TypeOf $right) + (TypeOf $leftRight) + ) & (CanConstructBinary Plus $right $leftRight) & (Let - ($result $ok):(FoldBinaryCheckOverflow + ($result $ok):(FoldBinaryCheckNull Plus $right $leftRight @@ -98,9 +101,13 @@ (Eq | Ge | Gt | Le | Lt (Minus $leftLeft:(ConstValue) $leftRight:^(ConstValue)) $right:(ConstValue) & + (ArithmeticErrorsOnOverflow + (TypeOf $leftLeft) + (TypeOf $right) + ) & (CanConstructBinary Minus $leftLeft $right) & (Let - ($result $ok):(FoldBinaryCheckOverflow + ($result $ok):(FoldBinaryCheckNull Minus $leftLeft $right diff --git a/pkg/sql/opt/norm/testdata/rules/comp b/pkg/sql/opt/norm/testdata/rules/comp index b8280dc79a8a..aee34ed6b7d2 100644 --- a/pkg/sql/opt/norm/testdata/rules/comp +++ b/pkg/sql/opt/norm/testdata/rules/comp @@ -129,84 +129,6 @@ select └── filters └── (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 not -# an integer, decimal, float, or interval. -norm expect-not=NormalizeCmpPlusConst -SELECT * FROM a WHERE '2022-01-01'::date + s::time >= '2022-01-01 1:00:00'::timestamp ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── stable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - └── (s:4::TIME + '2022-01-01') >= '2022-01-01 01:00:00' [outer=(4), stable] - -# The rule should not apply if the result of the constructed Minus operator would be -# a different type than the RHS of the comparison, because it's impossible to -# check for underflow. -norm expect-not=NormalizeCmpPlusConst -SELECT * FROM a WHERE - 1::decimal + i >= length('foo') AND - '1:00:00'::time + i::interval >= '2:00:00'::time ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── immutable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - ├── (i:2 + 1) >= 3 [outer=(2), immutable] - └── (i:2::INTERVAL + '01:00:00') >= '02:00:00' [outer=(2), immutable] - -# The rule should apply if the constructed Minus operator would not underflow or -# overflow. -norm expect=NormalizeCmpPlusConst -SELECT * FROM a WHERE - '05:00:00'::interval + s::time < '06:00:00'::time AND - '-05:00:00'::interval + s::time < '12:00:00'::time ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── stable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - ├── s:4::TIME < '01:00:00' [outer=(4), stable] - └── s:4::TIME < '17:00:00' [outer=(4), stable] - -# The rule should not apply if the constructed Minus operator would underflow or -# overflow. -norm expect-not=NormalizeCmpPlusConst -SELECT * FROM a WHERE - '05:00:00'::interval + s::time < '01:00:00'::time AND - '-05:00:00'::interval + s::time < '23:00:00'::time ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── stable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - ├── (s:4::TIME + '05:00:00') < '01:00:00' [outer=(4), stable] - └── (s:4::TIME + '-05:00:00') < '23:00:00' [outer=(4), stable] - # Regression test for #89024 - don't attempt to evaluate op for NULL values. norm expect-not=(NormalizeCmpPlusConst,NormalizeCmpMinusConst,NormalizeCmpConstMinus) SELECT 1 @@ -218,6 +140,28 @@ values ├── key: () └── fd: ()-->(1) +norm expect-not=(NormalizeCmpPlusConst,NormalizeCmpMinusConst,NormalizeCmpConstMinus) +SELECT 1 WHERE 1 - 10 <= NULL::INT +---- +values + ├── columns: "?column?":1!null + ├── cardinality: [0 - 0] + ├── key: () + └── fd: ()-->(1) + +# Regression test for #90053. This rule should not apply when the generated Plus +# or Minus can overflow or underflow without error. +norm expect-not=(NormalizeCmpPlusConst,NormalizeCmpMinusConst,NormalizeCmpConstMinus) +SELECT '00:01:40.01+09:00:00' < (col::TIMETZ + '-83 years -1 mons -38 days -10:32:23.707137') +FROM (VALUES ('03:16:01.252182+01:49:00')) v(col); +---- +values + ├── columns: "?column?":2!null + ├── cardinality: [1 - 1] + ├── key: () + ├── fd: ()-->(2) + └── (true,) + # -------------------------------------------------- # NormalizeCmpMinusConst # -------------------------------------------------- @@ -262,83 +206,18 @@ select └── filters └── (s:4::JSONB - 1) = '[1]' [outer=(4), immutable] -# The rule should not apply if the type of RHS the constructed Plus operator is -# a not an integer, decimal, float, or interval. -norm expect-not=NormalizeCmpMinusConst -SELECT * FROM a WHERE i::date - '01:00:00'::time >= '2022-01-01 1:00:00'::timestamp ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── immutable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - └── (i:2::DATE - '01:00:00') >= '2022-01-01 01:00:00' [outer=(2), immutable] - -# The rule should not apply if the result of the constructed Plus operator is a -# different type than the RHS of the comparison, because it's impossible to -# check for underflow. -norm expect-not=NormalizeCmpMinusConst -SELECT * FROM a WHERE - i - 1::decimal >= length('foo') AND - d - '1w'::interval >= '2018-09-23'::date ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── immutable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - ├── (i:2 - 1) >= 3 [outer=(2), immutable] - └── (d:6 - '7 days') >= '2018-09-23' [outer=(6), immutable] - -# The rule should apply if the constructed Plus operator does not underflow or -# overflow. -norm expect=NormalizeCmpMinusConst -SELECT * FROM a WHERE - s::time - '05:00:00'::interval < '06:00:00'::time AND - s::time - '-05:00:00'::interval < '12:00:00'::time ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── stable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - ├── s:4::TIME < '11:00:00' [outer=(4), stable] - └── s:4::TIME < '07:00:00' [outer=(4), stable] - -# The rule should not apply if the constructed Plus operator would overflow or -# underflow. -norm expect-not=NormalizeCmpMinusConst -SELECT * FROM a WHERE - s::time - '05:00:00'::interval < '23:00:00'::time AND - s::time - '-05:00:00'::interval < '01:00:00'::time +# Regression test for #90053. This rule should not apply when the generated Plus +# or Minus can overflow or underflow without error. +norm expect-not=(NormalizeCmpPlusConst,NormalizeCmpMinusConst,NormalizeCmpConstMinus) +SELECT (col::TIMETZ - '83 years -1 mons -38 days -10:32:23.707137') > '00:01:40.01+09:00:00' +FROM (VALUES ('03:16:01.252182+01:49:00')) v(col); ---- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── stable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - ├── (s:4::TIME - '05:00:00') < '23:00:00' [outer=(4), stable] - └── (s:4::TIME - '-05:00:00') < '01:00:00' [outer=(4), stable] +values + ├── columns: "?column?":2!null + ├── cardinality: [1 - 1] + ├── key: () + ├── fd: ()-->(2) + └── (true,) # -------------------------------------------------- # NormalizeCmpConstMinus @@ -385,8 +264,8 @@ select └── filters └── ('[1, 2]' - i:2) = '[1]' [outer=(2), immutable] -# The rule should not apply if the type of RHS the constructed Minus operator is -# a not an integer, decimal, float, or interval. +# Regression test for #90053. This rule should not apply when the generated Plus +# or Minus can overflow or underflow without error. norm expect-not=NormalizeCmpConstMinus SELECT * FROM a WHERE '2022-01-01'::date - s::time >= '2022-01-01 1:00:00'::timestamp ---- @@ -402,24 +281,6 @@ select └── filters └── ('2022-01-01' - s:4::TIME) >= '2022-01-01 01:00:00' [outer=(4), stable] -# The rule should not apply if the result of the constructed Minus operator is a different -# type than the RHS of the comparison, because it's impossible to check for -# underflow. -norm expect-not=NormalizeCmpConstMinus -SELECT * FROM a WHERE '01:00:00'::time - i::interval <= '02:00:00'::time ----- -select - ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - ├── immutable - ├── key: (1) - ├── fd: (1)-->(2-6) - ├── scan a - │ ├── columns: k:1!null i:2 f:3 s:4 j:5 d:6 - │ ├── key: (1) - │ └── fd: (1)-->(2-6) - └── filters - └── ('01:00:00' - i:2::INTERVAL) <= '02:00:00' [outer=(2), immutable] - # -------------------------------------------------- # NormalizeTupleEquality # -------------------------------------------------- diff --git a/pkg/sql/sem/tree/datum.go b/pkg/sql/sem/tree/datum.go index bd3e26434e5e..8f39087c64aa 100644 --- a/pkg/sql/sem/tree/datum.go +++ b/pkg/sql/sem/tree/datum.go @@ -2901,9 +2901,6 @@ type DInterval struct { duration.Duration } -// DZeroInterval is the zero-valued DInterval. -var DZeroInterval = &DInterval{} - // AsDInterval attempts to retrieve a DInterval from an Expr, panicking if the // assertion fails. func AsDInterval(e Expr) (*DInterval, bool) {