From 1aa4e56f44a74c7f7cb99283a7a0b68aa9e479c6 Mon Sep 17 00:00:00 2001
From: Justin Jaffray <justin@cockroachlabs.com>
Date: Mon, 11 Mar 2019 15:34:11 -0400
Subject: [PATCH] opt,sql: don't fold "- 0" for JSON

Fixes #35612.

Prior to this commit, we would unconditionally fold x - 0 to x. This was
invalid in the case where x was JSON, since `- 0` in that case means
"delete the 0th entry in the array".

This bug existed in both the HP and CBO.

This fix slightly abuses the existing "types.IsAdditiveType" function to
determine whether or not the identity applies.

Release note (bug fix): Subtracting 0 from a JSON array now correctly
removes its first element.
---
 pkg/sql/logictest/testdata/logic_test/json |  5 +++++
 pkg/sql/opt/norm/custom_funcs.go           |  9 +++++++++
 pkg/sql/opt/norm/rules/numeric.opt         |  6 ++++--
 pkg/sql/opt/norm/testdata/rules/numeric    | 11 +++++++++++
 pkg/sql/sem/tree/normalize.go              |  2 +-
 5 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/pkg/sql/logictest/testdata/logic_test/json b/pkg/sql/logictest/testdata/logic_test/json
index 60bda282806d..556b1c80fdd5 100644
--- a/pkg/sql/logictest/testdata/logic_test/json
+++ b/pkg/sql/logictest/testdata/logic_test/json
@@ -498,6 +498,11 @@ SELECT '{"a": 1}'::JSONB - 'b'
 {"a": 1}
 
 # `-` is one of the very few cases that PG errors in a JSON type mismatch with operators.
+query T
+SELECT '[1,2,3]'::JSONB - 0
+----
+[2, 3]
+
 query T
 SELECT '[1,2,3]'::JSONB - 1
 ----
diff --git a/pkg/sql/opt/norm/custom_funcs.go b/pkg/sql/opt/norm/custom_funcs.go
index cc2e91b91a21..967c2b04eb34 100644
--- a/pkg/sql/opt/norm/custom_funcs.go
+++ b/pkg/sql/opt/norm/custom_funcs.go
@@ -1292,6 +1292,15 @@ func (c *CustomFuncs) MakeLimited(sub *memo.SubqueryPrivate) *memo.SubqueryPriva
 //
 // ----------------------------------------------------------------------
 
+// IsAdditive returns true if the type of the expression supports addition and
+// subtraction in the natural way. This differs from "has a +/- Numeric
+// implementation" because JSON has an implementation for "- INT" which doesn't
+// obey x - 0 = x. Additive types include all numeric types as well as
+// timestamps and dates.
+func (c *CustomFuncs) IsAdditive(e opt.ScalarExpr) bool {
+	return types.IsAdditiveType(e.DataType())
+}
+
 // EqualsNumber returns true if the given numeric value (decimal, float, or
 // integer) is equal to the given integer value.
 func (c *CustomFuncs) EqualsNumber(datum tree.Datum, value int64) bool {
diff --git a/pkg/sql/opt/norm/rules/numeric.opt b/pkg/sql/opt/norm/rules/numeric.opt
index 7b149b500b75..7765aa1891a7 100644
--- a/pkg/sql/opt/norm/rules/numeric.opt
+++ b/pkg/sql/opt/norm/rules/numeric.opt
@@ -18,9 +18,11 @@
 [FoldZeroPlus, Normalize]
 (Plus $left:(Const 0) $right:*) => (Cast $right (BinaryColType Plus $left $right))
 
-# FoldMinusZero folds $left - 0 for numeric types.
+# FoldMinusZero folds $left - 0 for numeric types. This rule requires a check
+# that $left is numeric because JSON - INT is valid and is not a no-op with a
+# zero value.
 [FoldMinusZero, Normalize]
-(Minus $left:* $right:(Const 0)) => (Cast $left (BinaryColType Minus $left $right))
+(Minus $left:(IsAdditive $left) $right:(Const 0)) => (Cast $left (BinaryColType Minus $left $right))
 
 # FoldMultOne folds $left * 1 for numeric types.
 [FoldMultOne, Normalize]
diff --git a/pkg/sql/opt/norm/testdata/rules/numeric b/pkg/sql/opt/norm/testdata/rules/numeric
index f7445c71d053..c23a08bc0ceb 100644
--- a/pkg/sql/opt/norm/testdata/rules/numeric
+++ b/pkg/sql/opt/norm/testdata/rules/numeric
@@ -88,6 +88,17 @@ values
  ├── fd: ()-->(1)
  └── (0,) [type=tuple{decimal}]
 
+# Regression test for #35612.
+opt expect-not=FoldMinusZero
+SELECT '[123]'::jsonb - 0
+----
+values
+ ├── columns: "?column?":1(jsonb)
+ ├── cardinality: [1 - 1]
+ ├── key: ()
+ ├── fd: ()-->(1)
+ └── ('[]',) [type=tuple{jsonb}]
+
 # --------------------------------------------------
 # FoldMultOne, FoldOneMult
 # --------------------------------------------------
diff --git a/pkg/sql/sem/tree/normalize.go b/pkg/sql/sem/tree/normalize.go
index 3df53518c01f..af53c99cb378 100644
--- a/pkg/sql/sem/tree/normalize.go
+++ b/pkg/sql/sem/tree/normalize.go
@@ -140,7 +140,7 @@ func (expr *BinaryExpr) normalize(v *NormalizeVisitor) TypedExpr {
 			break
 		}
 	case Minus:
-		if v.isNumericZero(right) {
+		if types.IsAdditiveType(left.ResolvedType()) && v.isNumericZero(right) {
 			final, v.err = ReType(left, expectedType)
 			break
 		}