Merge pull request #2292 from cockroachdb/pmattis/sql-analyze-misc

Miscellaneous addendums to the {analyze,simplify}Expr() work.

sql/analyze.go

@@ -874,13 +874,15 @@ func simplifyOneOrExpr(left, right parser.Expr) (parser.Expr, parser.Expr) {
 }
 
 func simplifyOneOrInExpr(left, right *parser.ComparisonExpr) (parser.Expr, parser.Expr) {
+	if left.Operator != parser.In && right.Operator != parser.In {
+		panic(fmt.Sprintf("IN expression required: %s vs %s", left, right))
+	}
+
 	switch left.Operator {
 	case parser.EQ:
 		switch right.Operator {
 		case parser.In:
 			left, right = right, left
-		default:
-			panic(fmt.Sprintf("unexpected operator: %s", right.Operator))
 		}
 		fallthrough
 
@@ -905,10 +907,17 @@ func simplifyOneOrInExpr(left, right *parser.ComparisonExpr) (parser.Expr, parse
 			}
 		}
 
+		if !typeCheckTuple(left.Left, tuple2) {
+			return left, right
+		}
+
 		// We keep the tuples for an in expression in sorted order. So now we just
 		// merge the two sorted lists.
-		left.Right = mergeSorted(tuple, tuple2)
-		return left, nil
+		return &parser.ComparisonExpr{
+			Operator: parser.In,
+			Left:     left.Left,
+			Right:    mergeSorted(tuple, tuple2),
+		}, nil
 	}
 
 	return left, right
@@ -927,6 +936,9 @@ func simplifyComparisonExpr(n *parser.ComparisonExpr) parser.Expr {
 			if !ok {
 				break
 			}
+			if !typeCheckTuple(n.Left, tuple) {
+				break
+			}
 			sort.Sort(tuple)
 			tuple = uniqTuple(tuple)
 			n.Right = tuple
@@ -1013,6 +1025,29 @@ func mergeSorted(a, b parser.DTuple) parser.DTuple {
 	return r
 }
 
+// typeCheckTuple verifies that the types of all of the values in tuple are
+// either DNull or agree with the arg type.
+//
+// TODO(pmattis): I think we need a TypeCheckExpr which performs this type
+// check as well as others like the CASE type checks.
+func typeCheckTuple(arg parser.Expr, tuple parser.DTuple) bool {
+	qval, ok := arg.(*qvalue)
+	if !ok {
+		return false
+	}
+	argType := reflect.TypeOf(qval.Datum())
+	for _, d := range tuple {
+		if d == parser.DNull {
+			continue
+		}
+		dtype := reflect.TypeOf(d)
+		if argType != dtype {
+			return false
+		}
+	}
+	return true
+}
+
 func isVar(e parser.Expr) bool {
 	switch e.(type) {
 	case *qvalue:

sql/analyze_test.go

@@ -18,6 +18,7 @@
 package sql
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/sql/parser"
@@ -56,7 +57,7 @@ func parseAndNormalizeExpr(t *testing.T, sql string) (parser.Expr, qvalMap) {
 	return r, s.qvals
 }
 
-func checkEquivExpr(t *testing.T, a, b parser.Expr, qvals qvalMap) bool {
+func checkEquivExpr(a, b parser.Expr, qvals qvalMap) error {
 	// The expressions above only use the values 1 and 2. Verify that the
 	// simplified expressions evaluate to the same value as the original
 	// expression for interesting values.
@@ -66,20 +67,17 @@ func checkEquivExpr(t *testing.T, a, b parser.Expr, qvals qvalMap) bool {
 		}
 		da, err := parser.EvalExpr(a)
 		if err != nil {
-			t.Errorf("%s: %v", a, err)
-			return false
+			return fmt.Errorf("%s: %v", a, err)
 		}
 		db, err := parser.EvalExpr(b)
 		if err != nil {
-			t.Errorf("%s: %v", b, err)
-			return false
+			return fmt.Errorf("%s: %v", b, err)
 		}
 		if da != db {
-			t.Errorf("%s: %d: expected %s, but found %s", a, v, da, db)
-			return false
+			return fmt.Errorf("%s: %d: expected %s, but found %s", a, v, da, db)
 		}
 	}
-	return true
+	return nil
 }
 
 func TestSplitOrExpr(t *testing.T) {
@@ -147,6 +145,8 @@ func TestSimplifyExpr(t *testing.T) {
 
 		{`a IN (1, 1)`, `a IN (1)`},
 		{`a IN (2, 3, 1)`, `a IN (1, 2, 3)`},
+		{`a IN (1, true)`, `true`},
+		{`a IN (1, NULL)`, `a IN (NULL, 1)`}, // TODO(pmattis): `a IN (1)`
 
 		{`a LIKE '%foo'`, `true`},
 		{`a LIKE 'foo'`, `a = 'foo'`},
@@ -171,9 +171,9 @@ func TestSimplifyNotExpr(t *testing.T) {
 	defer leaktest.AfterTest(t)
 
 	testData := []struct {
-		expr     string
-		expected string
-		check    bool
+		expr       string
+		expected   string
+		checkEquiv bool
 	}{
 		{`NOT a = 1`, `a != 1`, true},
 		{`NOT a != 1`, `a = 1`, true},
@@ -196,8 +196,11 @@ func TestSimplifyNotExpr(t *testing.T) {
 		if s := expr2.String(); d.expected != s {
 			t.Errorf("%s: expected %s, but found %s", d.expr, d.expected, s)
 		}
-		if d.check && !checkEquivExpr(t, expr1, expr2, qvals) {
-			continue
+		if d.checkEquiv {
+			if err := checkEquivExpr(expr1, expr2, qvals); err != nil {
+				t.Error(err)
+				continue
+			}
 		}
 	}
 }
@@ -233,9 +236,9 @@ func TestSimplifyAndExprCheck(t *testing.T) {
 	defer leaktest.AfterTest(t)
 
 	testData := []struct {
-		expr     string
-		expected string
-		check    bool
+		expr       string
+		expected   string
+		checkEquiv bool
 	}{
 		{`a = 1 AND a = 1`, `a = 1`, true},
 		{`a = 1 AND a = 2`, `false`, true},
@@ -358,8 +361,11 @@ func TestSimplifyAndExprCheck(t *testing.T) {
 			t.Errorf("%s: expected %s, but found %s", d.expr, d.expected, s)
 		}
 
-		if d.check && !checkEquivExpr(t, expr1, expr2, qvals) {
-			continue
+		if d.checkEquiv {
+			if err := checkEquivExpr(expr1, expr2, qvals); err != nil {
+				t.Error(err)
+				continue
+			}
 		}
 
 		if _, ok := expr2.(*parser.AndExpr); !ok {
@@ -535,7 +541,8 @@ func TestSimplifyOrExprCheck(t *testing.T) {
 			t.Fatalf("%s: expected %s, but found %s", d.expr, d.expected, s)
 		}
 
-		if !checkEquivExpr(t, expr1, expr2, qvals) {
+		if err := checkEquivExpr(expr1, expr2, qvals); err != nil {
+			t.Error(err)
 			continue
 		}