Merge pull request #35 from pingcap/siddontang/cleanup-compare

cleanup compare

expression/expressions/binop.go

@@ -346,105 +346,6 @@ func (o *BinaryOperation) evalLogicOp(ctx context.Context, args map[interface{}]
 	}
 }
 
-func compareFloatString(a float64, s string) (int, error) {
-	// MySQL will convert string to a float point value
-	// MySQL use a very loose conversation, e.g, 123.abc -> 123
-	// we should do a trade off whether supporting this feature or using a strict mode
-	// now we use a strict mode
-	b, err := types.StrToFloat(s)
-	if err != nil {
-		return 0, err
-	}
-	return types.CompareFloat64(a, b), nil
-}
-
-func compareStringFloat(s string, a float64) (int, error) {
-	n, err := compareFloatString(a, s)
-	return -n, err
-}
-
-// See https://dev.mysql.com/doc/refman/5.7/en/type-conversion.html
-func evalCompare(a interface{}, b interface{}) (int, error) {
-	// TODO: support compare time type with other types
-	switch x := a.(type) {
-	case float64:
-		switch y := b.(type) {
-		case float64:
-			return types.CompareFloat64(x, y), nil
-		case string:
-			return compareFloatString(x, y)
-		}
-	case int64:
-		switch y := b.(type) {
-		case int64:
-			return types.CompareInt64(x, y), nil
-		case uint64:
-			return types.CompareInteger(x, y), nil
-		case string:
-			return compareFloatString(float64(x), y)
-		}
-	case uint64:
-		switch y := b.(type) {
-		case uint64:
-			return types.CompareUint64(x, y), nil
-		case int64:
-			return -types.CompareInteger(y, x), nil
-		case string:
-			return compareFloatString(float64(x), y)
-		}
-	case mysql.Decimal:
-		switch y := b.(type) {
-		case mysql.Decimal:
-			return x.Cmp(y), nil
-		case string:
-			f, err := mysql.ConvertToDecimal(y)
-			if err != nil {
-				return 0, err
-			}
-			return x.Cmp(f), nil
-		}
-	case string:
-		switch y := b.(type) {
-		case string:
-			return types.CompareString(x, y), nil
-		case int64:
-			return compareStringFloat(x, float64(y))
-		case uint64:
-			return compareStringFloat(x, float64(y))
-		case float64:
-			return compareStringFloat(x, y)
-		case mysql.Decimal:
-			f, err := mysql.ConvertToDecimal(x)
-			if err != nil {
-				return 0, err
-			}
-			return f.Cmp(y), nil
-		case mysql.Time:
-			n, err := y.CompareString(x)
-			return -n, err
-		case mysql.Duration:
-			n, err := y.CompareString(x)
-			return -n, err
-		}
-	case mysql.Time:
-		switch y := b.(type) {
-		case mysql.Time:
-			return x.Compare(y), nil
-		case string:
-			return x.CompareString(y)
-		}
-	case mysql.Duration:
-		switch y := b.(type) {
-		case mysql.Duration:
-			return x.Compare(y), nil
-		case string:
-			return x.CompareString(y)
-		}
-	}
-
-	return 0, errors.Errorf("invalid compare type %T cmp %T", a, b)
-}
-
 // operator: >=, >, <=, <, !=, <>, = <=>, etc.
 // see https://dev.mysql.com/doc/refman/5.7/en/comparison-operators.html
 func (o *BinaryOperation) evalComparisonOp(ctx context.Context, args map[interface{}]interface{}) (interface{}, error) {
@@ -459,7 +360,7 @@ func (o *BinaryOperation) evalComparisonOp(ctx context.Context, args map[interfa
 		return nil, nil
 	}
 
-	n, err := evalCompare(a, b)
+	n, err := types.Compare(a, b)
 	if err != nil {
 		return nil, o.traceErr(err)
 	}

expression/expressions/binop_test.go

@@ -15,7 +15,6 @@ package expressions
 
 import (
 	"errors"
-	"time"
 
 	. "github.com/pingcap/check"
 	"github.com/pingcap/tidb/expression"
@@ -106,44 +105,6 @@ func (s *testBinOpSuite) TestComparisonOp(c *C) {
 		c.Assert(v, IsNil)
 	}
 
-	// test evalCompare function
-	cmpTbl := []struct {
-		lhs interface{}
-		rhs interface{}
-		ret int // 0, 1, -1
-	}{
-		{float64(1), float64(1), 0},
-		{float64(1), "1", 0},
-		{int64(1), int64(1), 0},
-		{int64(-1), uint64(1), -1},
-		{int64(-1), "-1", 0},
-		{uint64(1), uint64(1), 0},
-		{uint64(1), int64(-1), 1},
-		{uint64(1), "1", 0},
-		{mysql.NewDecimalFromInt(1, 0), mysql.NewDecimalFromInt(1, 0), 0},
-		{mysql.NewDecimalFromInt(1, 0), "1", 0},
-		{"1", "1", 0},
-		{"1", int64(-1), 1},
-		{"1", float64(2), -1},
-		{"1", uint64(1), 0},
-		{"1", mysql.NewDecimalFromInt(1, 0), 0},
-		{"2011-01-01 11:11:11", mysql.Time{Time: time.Now(), Type: mysql.TypeDatetime, Fsp: 0}, -1},
-		{"12:00:00", mysql.ZeroDuration, 1},
-		{mysql.ZeroDuration, mysql.ZeroDuration, 0},
-		{mysql.Time{Time: time.Now().Add(time.Second * 10), Type: mysql.TypeDatetime, Fsp: 0},
-			mysql.Time{Time: time.Now(), Type: mysql.TypeDatetime, Fsp: 0}, 1},
-	}
-
-	for _, t := range cmpTbl {
-		ret, err := evalCompare(t.lhs, t.rhs)
-		c.Assert(err, IsNil)
-		c.Assert(ret, Equals, t.ret)
-
-		ret, err = evalCompare(t.rhs, t.lhs)
-		c.Assert(err, IsNil)
-		c.Assert(ret, Equals, -t.ret)
-	}
-
 	// test error
 	mock := mockExpr{
 		isStatic: false,
@@ -194,9 +155,6 @@ func (s *testBinOpSuite) TestComparisonOp(c *C) {
 	expr := BinaryOperation{Op: opcode.Plus, L: Value{1}, R: Value{1}}
 	_, err := expr.evalComparisonOp(nil, nil)
 	c.Assert(err, NotNil)
-
-	_, err = evalCompare(1, 1)
-	c.Assert(err, NotNil)
 }
 
 func (s *testBinOpSuite) TestIdentRelOp(c *C) {

expression/expressions/builtin_control.go

@@ -65,10 +65,7 @@ func builtinNullIf(args []interface{}, m map[interface{}]interface{}) (interface
 		return v1, nil
 	}
 
-	// coerce for later eval compare
-	x, y := types.Coerce(v1, v2)
-
-	if n, err := evalCompare(x, y); err != nil || n == 0 {
+	if n, err := types.Compare(v1, v2); err != nil || n == 0 {
 		return nil, err
 	}
 

expression/expressions/builtin_groupby.go

@@ -255,7 +255,11 @@ func builtinMax(args []interface{}, ctx map[interface{}]interface{}) (v interfac
 	if max == nil {
 		max = y
 	} else {
-		if types.Compare(max, y) < 0 {
+		n, err := types.Compare(max, y)
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+		if n < 0 {
 			max = y
 		}
 	}
@@ -288,7 +292,11 @@ func builtinMin(args []interface{}, ctx map[interface{}]interface{}) (v interfac
 	if min == nil {
 		min = y
 	} else {
-		if types.Compare(min, y) > 0 {
+		n, err := types.Compare(min, y)
+		if err != nil {
+			return nil, errors.Trace(err)
+		}
+		if n > 0 {
 			min = y
 		}
 	}

expression/expressions/unary_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/pingcap/tidb/expression"
 	mysql "github.com/pingcap/tidb/mysqldef"
 	"github.com/pingcap/tidb/parser/opcode"
+	"github.com/pingcap/tidb/util/types"
 )
 
 var _ = Suite(&testUnaryOperationSuite{})
@@ -111,7 +112,7 @@ func (s *testUnaryOperationSuite) TestUnaryOp(c *C) {
 		result, err := exprc.Eval(nil, nil)
 		c.Assert(err, IsNil)
 
-		ret, err := evalCompare(result, t.result)
+		ret, err := types.Compare(result, t.result)
 		c.Assert(err, IsNil)
 		c.Assert(ret, Equals, 0)
 	}

plan/plans/index.go

@@ -14,6 +14,8 @@
 package plans
 
 import (
+	"fmt"
+
 	"github.com/pingcap/tidb/column"
 	"github.com/pingcap/tidb/context"
 	"github.com/pingcap/tidb/expression"
@@ -151,7 +153,13 @@ func indexCompare(a interface{}, b interface{}) int {
 		return -1
 	}
 
-	return types.Compare(a, b)
+	n, err := types.Compare(a, b)
+	if err != nil {
+		// Old compare panics if err, so here we do the same thing now.
+		// TODO: return err instead of panic.
+		panic(fmt.Sprintf("should never happend %v", err))
+	}
+	return n
 }
 
 func (r *indexPlan) doSpan(ctx context.Context, txn kv.Transaction, span *indexSpan, f plan.RowIterFunc) error {

plan/plans/orderby.go

@@ -19,6 +19,7 @@ import (
 	"strings"
 
 	"github.com/juju/errors"
+	"github.com/ngaut/log"
 	"github.com/pingcap/tidb/context"
 	"github.com/pingcap/tidb/expression"
 	"github.com/pingcap/tidb/expression/expressions"
@@ -102,7 +103,13 @@ func (t *orderByTable) Less(i, j int) bool {
 		v1 := t.Rows[i].Key[index]
 		v2 := t.Rows[j].Key[index]
 
-		ret := types.Compare(v1, v2)
+		ret, err := types.Compare(v1, v2)
+		if err != nil {
+			// we just have to log this error and skip it.
+			// TODO: record this error and handle it out later.
+			log.Errorf("compare %v %v err %v", v1, v2, err)
+		}
+
 		if !asc {
 			ret = -ret
 		}

stmt/stmts/update.go

@@ -167,7 +167,12 @@ func updateRecord(ctx context.Context, h int64, data []interface{}, t table.Tabl
 			continue
 		}
 		od := oldData[i]
-		if types.Compare(d, od) != 0 {
+		n, err := types.Compare(d, od)
+		if err != nil {
+			return errors.Trace(err)
+		}
+
+		if n != 0 {
 			rowChanged = true
 			break
 		}