Merge pull request #90368 from mgartner/backport22.2-90266

release-22.2: opt: fix normalization of comparisons with constants

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

pkg/sql/opt/norm/comp_funcs.go

@@ -35,82 +35,50 @@ func (c *CustomFuncs) CommuteInequality(
 	return c.f.DynamicConstruct(op, right, left).(opt.ScalarExpr)
 }
 
-// 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
 	}
 	result, err := eval.BinaryOp(c.f.evalCtx, o.EvalOp, lDatum, rDatum)
 	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
 }
 

pkg/sql/opt/norm/rules/comp.opt

@@ -29,26 +29,25 @@
 # 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.
 [NormalizeCmpPlusConst, Normalize]
 (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