sql: Clean up code within the new typing system

This commit cleans up a number of uses of the new `TypedExpr` interface,
improves the type checking system, added new test cases, and adds in a few
TODOs for future work.

sql/expr_filter.go

@@ -240,7 +240,9 @@ func splitBoolExpr(expr parser.Expr, conv varConvertFunc, weaker bool) (restrict
 //  - the implementation is best-effort (it tries to get as much of the expression into RES as
 //    possible, and make REM as small as possible).
 //  - the original expression is modified in-place and should not be used again.
-func splitFilter(expr parser.TypedExpr, conv varConvertFunc) (restricted, remainder parser.TypedExpr) {
+func splitFilter(
+	expr parser.TypedExpr, conv varConvertFunc,
+) (restricted, remainder parser.TypedExpr) {
 	if expr == nil {
 		return nil, nil
 	}

sql/group.go

@@ -101,8 +101,9 @@ func (p *planner) groupBy(n *parser.SelectClause, s *selectNode) (*groupNode, *r
 		if err != nil {
 			return nil, roachpb.NewError(err)
 		}
-		if retType := typedHaving.ReturnType(); !(retType.TypeEqual(parser.DummyBool) || retType == parser.DNull) {
-			return nil, roachpb.NewUErrorf("argument of HAVING must be type %s, not type %s", parser.DummyBool.Type(), retType.Type())
+		if typ := typedHaving.ReturnType(); !(typ.TypeEqual(parser.DummyBool) || typ == parser.DNull) {
+			return nil, roachpb.NewUErrorf("argument of HAVING must be type %s, not type %s",
+				parser.DummyBool.Type(), typ.Type())
 		}
 
 		typedHaving, err = p.parser.NormalizeExpr(p.evalCtx, typedHaving)

sql/parser/builtins.go

@@ -474,7 +474,7 @@ var Builtins = map[string][]Builtin{
 	"greatest": {
 		Builtin{
 			Types:      AnyType{},
-			ReturnType: nil, // No return type because AnyType parameters.
+			ReturnType: nil, // No explicit return type because AnyType parameters.
 			fn: func(ctx EvalContext, args DTuple) (Datum, error) {
 				return pickFromTuple(ctx, true /* greatest */, args)
 			},
@@ -484,7 +484,7 @@ var Builtins = map[string][]Builtin{
 	"least": {
 		Builtin{
 			Types:      AnyType{},
-			ReturnType: nil, // No return type because AnyType parameters.
+			ReturnType: nil, // No explicit return type because AnyType parameters.
 			fn: func(ctx EvalContext, args DTuple) (Datum, error) {
 				return pickFromTuple(ctx, false /* !greatest */, args)
 			},

sql/parser/constant.go

@@ -1,4 +1,4 @@
-// Copyright 2015 The Cockroach Authors.
+// Copyright 2016 The Cockroach Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -30,18 +30,22 @@ import (
 // Constant is an constant literal expression which may be resolved to more than one type.
 type Constant interface {
 	Expr
+	// AvailableTypes returns the ordered set of types that the Constant is able to
+	// be resolved into. The order of the type slice provides a notion of precedence,
+	// with the first element in the ordering being the Constant's "natural type".
 	AvailableTypes() []Datum
-	ResolveAsType(Datum) (TypedExpr, error)
+	// ResolveAsType resolves the Constant as the Datum type specified, or returns an
+	// error if the Constant could not be resolved as that type. The method should only
+	// be passed a type returned from AvailableTypes.
+	ResolveAsType(Datum) (Datum, error)
 }
 
 var _ Constant = &NumVal{}
 var _ Constant = &StrVal{}
 
 func isNumericConstant(expr Expr) bool {
-	if _, ok := expr.(*NumVal); ok {
-		return true
-	}
-	return false
+	_, ok := expr.(*NumVal)
+	return ok
 }
 
 func typeCheckConstant(c Constant, desired Datum) (TypedExpr, error) {
@@ -62,6 +66,8 @@ func naturalConstantType(c Constant) Datum {
 	return c.AvailableTypes()[0]
 }
 
+// canConstantBecome returns whether the provided Constant can become resolved
+// as the provided type.
 func canConstantBecome(c Constant, typ Datum) bool {
 	avail := c.AvailableTypes()
 	for _, availTyp := range avail {
@@ -72,6 +78,13 @@ func canConstantBecome(c Constant, typ Datum) bool {
 	return false
 }
 
+// shouldConstantBecome returns whether the provided Constant should or
+// should not become the provided type. The function is meant to be differentiated
+// from canConstantBecome in that it will exclude certain (Constant, Type) resolution
+// pairs that are possible, but not desirable.
+//
+// An example of this is resolving a floating point numeric constant without a value
+// past the decimal point as an DInt. This is possible, but it is not desirable.
 func shouldConstantBecome(c Constant, typ Datum) bool {
 	if num, ok := c.(*NumVal); ok {
 		if typ.TypeEqual(DummyInt) && num.Kind() == constant.Float {
@@ -85,7 +98,7 @@ func shouldConstantBecome(c Constant, typ Datum) bool {
 type NumVal struct {
 	constant.Value
 
-	// We preserve the "original" string representation (before folding and normalization).
+	// We preserve the "original" string representation (before folding).
 	OrigString string
 }
 
@@ -133,7 +146,7 @@ func (expr *NumVal) asInt64() (int64, error) {
 }
 
 // asConstantInt returns the value as an constant.Int if possible, along
-// with if the conversion was possible.
+// with a flag indicating whether the conversion was possible.
 func (expr *NumVal) asConstantInt() (constant.Value, bool) {
 	intVal := constant.ToInt(expr.Value)
 	if intVal.Kind() == constant.Int {
@@ -144,11 +157,14 @@ func (expr *NumVal) asConstantInt() (constant.Value, bool) {
 
 var numValAvailIntFloatDec = []Datum{DummyInt, DummyFloat, DummyDecimal}
 var numValAvailFloatIntDec = []Datum{DummyFloat, DummyInt, DummyDecimal}
-var numValAvailFloatDec = numValAvailIntFloatDec[1:]
+var numValAvailFloatDec = []Datum{DummyFloat, DummyDecimal}
 
-// var numValAvailDec = numValAvailIntFloatDec[2:]
+// var numValAvailDec = []Datum{DummyDecimal}
 
 // AvailableTypes implements the Constant interface.
+//
+// TODO(nvanbenschoten) is there a limit past which we should only allow (or prefer)
+// Decimal and not Float? See issue #6369.
 func (expr *NumVal) AvailableTypes() []Datum {
 	switch {
 	case expr.canBeInt64():
@@ -162,7 +178,7 @@ func (expr *NumVal) AvailableTypes() []Datum {
 }
 
 // ResolveAsType implements the Constant interface.
-func (expr *NumVal) ResolveAsType(typ Datum) (TypedExpr, error) {
+func (expr *NumVal) ResolveAsType(typ Datum) (Datum, error) {
 	switch typ {
 	case DummyInt:
 		i, exact := constant.Int64Val(constant.ToInt(expr.Value))
@@ -181,11 +197,13 @@ func (expr *NumVal) ResolveAsType(typ Datum) (TypedExpr, error) {
 			// like 6/7. If only we could call big.Rat.FloatString() on it...
 			num, den := s[:idx], s[idx+1:]
 			if _, ok := dd.SetString(num); !ok {
-				return nil, fmt.Errorf("could not evaluate numerator of %v as Datum type DDecimal from string %q", expr, num)
+				return nil, fmt.Errorf("could not evaluate numerator of %v as Datum type DDecimal "+
+					"from string %q", expr, num)
 			}
 			denDec := new(inf.Dec)
 			if _, ok := denDec.SetString(den); !ok {
-				return nil, fmt.Errorf("could not evaluate denominator %v as Datum type DDecimal from string %q", expr, den)
+				return nil, fmt.Errorf("could not evaluate denominator %v as Datum type DDecimal "+
+					"from string %q", expr, den)
 			}
 			dd.QuoRound(&dd.Dec, denDec, decimal.Precision, inf.RoundHalfUp)
 
@@ -202,12 +220,14 @@ func (expr *NumVal) ResolveAsType(typ Datum) (TypedExpr, error) {
 					break
 				}
 				if _, ok := dd.SetString(s); !ok {
-					return nil, fmt.Errorf("could not evaluate %v as Datum type DDecimal from string %q", expr, s)
+					return nil, fmt.Errorf("could not evaluate %v as Datum type DDecimal from "+
+						"string %q", expr, s)
 				}
 			}
 		} else {
 			if _, ok := dd.SetString(s); !ok {
-				return nil, fmt.Errorf("could not evaluate %v as Datum type DDecimal from string %q", expr, s)
+				return nil, fmt.Errorf("could not evaluate %v as Datum type DDecimal from "+
+					"string %q", expr, s)
 			}
 		}
 		return dd, nil
@@ -218,7 +238,9 @@ func (expr *NumVal) ResolveAsType(typ Datum) (TypedExpr, error) {
 
 var numValTypePriority = []Datum{DummyInt, DummyFloat, DummyDecimal}
 
-// commonNumericConstantType returns the best constant type...
+// commonNumericConstantType returns the best constant type which is shared
+// between a set of provided numeric constants. Here, "best" is defined as
+// the smallest numeric data type which will not lose information.
 func commonNumericConstantType(vals ...*NumVal) Datum {
 	bestType := 0
 	for _, c := range vals {
@@ -252,7 +274,7 @@ func (expr *StrVal) String() string {
 
 var strValAvailStringBytes = []Datum{DummyString, DummyBytes}
 var strValAvailBytesString = []Datum{DummyBytes, DummyString}
-var strValAvailBytes = strValAvailBytesString[:1]
+var strValAvailBytes = []Datum{DummyBytes}
 
 // AvailableTypes implements the Constant interface.
 func (expr *StrVal) AvailableTypes() []Datum {
@@ -266,7 +288,7 @@ func (expr *StrVal) AvailableTypes() []Datum {
 }
 
 // ResolveAsType implements the Constant interface.
-func (expr *StrVal) ResolveAsType(typ Datum) (TypedExpr, error) {
+func (expr *StrVal) ResolveAsType(typ Datum) (Datum, error) {
 	switch typ {
 	case DummyString:
 		return NewDString(expr.s), nil
@@ -319,8 +341,10 @@ var comparisonOpToToken = map[ComparisonOperator]token.Token{
 
 func (constantFolderVisitor) VisitPost(expr Expr) (retExpr Expr) {
 	defer func() {
-		// go/constant operations can panic for a number of reasons, but it's difficult
-		// to preemptively detect when they will. It's safest to just recover here.
+		// go/constant operations can panic for a number of reasons (like division
+		// by zero), but it's difficult to preemptively detect when they will. It's
+		// safest to just recover here without folding the expression and let
+		// normalization or evaluation deal with error handling.
 		if r := recover(); r != nil {
 			retExpr = expr
 		}
@@ -378,6 +402,7 @@ func (constantFolderVisitor) VisitPost(expr Expr) (retExpr Expr) {
 // foldNumericConstants folds all numeric constants using exact arithmetic.
 //
 // TODO(nvanbenschoten) Can this visitor be preallocated (like normalizeVisitor)?
+// TODO(nvanbenschoten) Do we also want to fold string-like constants?
 // TODO(nvanbenschoten) Investigate normalizing associative operations to group
 //     constants together and permit further numeric constant folding.
 func foldNumericConstants(expr Expr) (Expr, error) {
@@ -406,8 +431,8 @@ func (constantTypeVisitor) VisitPre(expr Expr) (recurse bool, newExpr Expr) {
 
 func (constantTypeVisitor) VisitPost(expr Expr) (retExpr Expr) { return expr }
 
-// TypeConstants type checks all Constant literal expressions, causing
-// them to become Datum representations of their values. While doing so,
+// TypeConstants type checks all Constant literal expressions, resolving
+// them as the Datum representations of their values. Before doing so,
 // it first folds all numeric constants.
 //
 // This means that Constants will become TypedExprs so that they can be
@@ -416,9 +441,9 @@ func (constantTypeVisitor) VisitPost(expr Expr) (retExpr Expr) { return expr }
 // expressions where full type checking is not desired.
 //
 // TODO(nvanbenschoten) Can this visitor be preallocated (like normalizeVisitor)?
-func TypeConstants(expr Expr) (Expr, error) {
+func TypeConstants(expr Expr) (TypedExpr, error) {
 	v := constantTypeVisitor{}
 	expr, _ = WalkExpr(v.cfv, expr)
 	expr, _ = WalkExpr(v, expr)
-	return expr, nil
+	return expr.(TypedExpr), nil
 }

sql/parser/constant_test.go

@@ -1,4 +1,4 @@
-// Copyright 2015 The Cockroach Authors.
+// Copyright 2016 The Cockroach Authors.
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -17,11 +17,17 @@
 package parser
 
 import (
+	"fmt"
 	"go/constant"
 	"go/token"
 	"reflect"
+	"strconv"
 	"strings"
 	"testing"
+
+	"github.com/cockroachdb/cockroach/util/decimal"
+
+	"gopkg.in/inf.v0"
 )
 
 func TestNumericConstantAvailableTypes(t *testing.T) {
@@ -39,14 +45,18 @@ func TestNumericConstantAvailableTypes(t *testing.T) {
 		{"9223372036854775807", wantInt},
 		{"1.0", wantFloatButCanBeInt},
 		{"-1234.0000", wantFloatButCanBeInt},
+		{"1e10", wantFloatButCanBeInt},
+		{"1E10", wantFloatButCanBeInt},
 		{"1.1", wantFloat},
+		{"1e-10", wantFloat},
+		{"1E-10", wantFloat},
 		{"-1231.131", wantFloat},
 		{"876543234567898765436787654321", wantFloat},
 	}
 
 	for i, test := range testCases {
 		tok := token.INT
-		if strings.Contains(test.str, ".") {
+		if strings.ContainsAny(test.str, ".eE") {
 			tok = token.FLOAT
 		}
 		val := constant.MakeFromLiteral(test.str, tok, 0)
@@ -58,13 +68,63 @@ func TestNumericConstantAvailableTypes(t *testing.T) {
 		c := &NumVal{Value: val}
 		avail := c.AvailableTypes()
 		if !reflect.DeepEqual(avail, test.avail) {
-			t.Errorf("%d: expected the available type set %v for %v, found %v", i, test.avail, c.Value.ExactString(), avail)
+			t.Errorf("%d: expected the available type set %v for %v, found %v",
+				i, test.avail, c.Value.ExactString(), avail)
 		}
 
 		// Make sure it can be resolved as each of those types.
 		for _, availType := range avail {
-			if _, err := c.ResolveAsType(availType); err != nil {
-				t.Errorf("%d: expected resolving %v as available type %s would succeed, found %v", i, c.Value.ExactString(), availType.Type(), err)
+			if res, err := c.ResolveAsType(availType); err != nil {
+				t.Errorf("%d: expected resolving %v as available type %s would succeed, found %v",
+					i, c.Value.ExactString(), availType.Type(), err)
+			} else {
+				resErr := func(parsed, resolved interface{}) {
+					t.Errorf("%d: expected resolving %v as available type %s would produce a Datum with the value %v, found %v",
+						i, c, availType.Type(), parsed, resolved)
+				}
+				switch typ := res.(type) {
+				case *DInt:
+					var i int64
+					var err error
+					if tok == token.INT {
+						if i, err = strconv.ParseInt(test.str, 10, 64); err != nil {
+							t.Fatal(err)
+						}
+					} else {
+						var f float64
+						if f, err = strconv.ParseFloat(test.str, 64); err != nil {
+							t.Fatal(err)
+						}
+						i = int64(f)
+					}
+					if resI := int64(*typ); i != resI {
+						resErr(i, resI)
+					}
+				case *DFloat:
+					f, err := strconv.ParseFloat(test.str, 64)
+					if err != nil {
+						t.Fatal(err)
+					}
+					if resF := float64(*typ); f != resF {
+						resErr(f, resF)
+					}
+				case *DDecimal:
+					d := new(inf.Dec)
+					if !strings.ContainsAny(test.str, "eE") {
+						if _, ok := d.SetString(test.str); !ok {
+							t.Fatal(fmt.Sprintf("could not set %q on decimal", test.str))
+						}
+					} else {
+						f, err := strconv.ParseFloat(test.str, 64)
+						if err != nil {
+							t.Fatal(err)
+						}
+						decimal.SetFromFloat(d, f)
+					}
+					if resD := &typ.Dec; d.Cmp(resD) != 0 {
+						resErr(d, resD)
+					}
+				}
 			}
 		}
 	}
@@ -88,13 +148,29 @@ func TestStringConstantAvailableTypes(t *testing.T) {
 		// Check available types.
 		avail := test.c.AvailableTypes()
 		if !reflect.DeepEqual(avail, test.avail) {
-			t.Errorf("%d: expected the available type set %v for %+v, found %v", i, test.avail, test.c, avail)
+			t.Errorf("%d: expected the available type set %v for %+v, found %v",
+				i, test.avail, test.c, avail)
 		}
 
 		// Make sure it can be resolved as each of those types.
 		for _, availType := range avail {
-			if _, err := test.c.ResolveAsType(availType); err != nil {
-				t.Errorf("%d: expected resolving %v as available type %s would succeed, found %v", i, test.c, availType.Type(), err)
+			if res, err := test.c.ResolveAsType(availType); err != nil {
+				t.Errorf("%d: expected resolving %v as available type %s would succeed, found %v",
+					i, test.c, availType.Type(), err)
+			} else {
+				var resString string
+				switch typ := res.(type) {
+				case *DString:
+					resString = string(*typ)
+				case *DBytes:
+					resString = string(*typ)
+				default:
+					panic("unreachable")
+				}
+				if resString != test.c.s {
+					t.Errorf("%d: expected resolving %v as available type %s would produce a Datum with the same value, found %s",
+						i, test.c, availType.Type(), resString)
+				}
 			}
 		}
 	}