From ffb8fddf871910dff2ec7a68c6ee22a92b01e96c Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 14:40:24 +0800
Subject: [PATCH 1/7] expression: move builtin to builtin package, cannot run

---
 expression/{ => builtin}/builtin.go              | 0
 expression/{ => builtin}/builtin_control.go      | 0
 expression/{ => builtin}/builtin_control_test.go | 0
 expression/{ => builtin}/builtin_groupby.go      | 0
 expression/{ => builtin}/builtin_groupby_test.go | 0
 expression/{ => builtin}/builtin_info.go         | 0
 expression/{ => builtin}/builtin_info_test.go    | 0
 expression/{ => builtin}/builtin_math.go         | 0
 expression/{ => builtin}/builtin_math_test.go    | 0
 expression/{ => builtin}/builtin_string.go       | 0
 expression/{ => builtin}/builtin_string_test.go  | 0
 expression/{ => builtin}/builtin_test.go         | 0
 expression/{ => builtin}/builtin_time.go         | 0
 expression/{ => builtin}/builtin_time_test.go    | 0
 14 files changed, 0 insertions(+), 0 deletions(-)
 rename expression/{ => builtin}/builtin.go (100%)
 rename expression/{ => builtin}/builtin_control.go (100%)
 rename expression/{ => builtin}/builtin_control_test.go (100%)
 rename expression/{ => builtin}/builtin_groupby.go (100%)
 rename expression/{ => builtin}/builtin_groupby_test.go (100%)
 rename expression/{ => builtin}/builtin_info.go (100%)
 rename expression/{ => builtin}/builtin_info_test.go (100%)
 rename expression/{ => builtin}/builtin_math.go (100%)
 rename expression/{ => builtin}/builtin_math_test.go (100%)
 rename expression/{ => builtin}/builtin_string.go (100%)
 rename expression/{ => builtin}/builtin_string_test.go (100%)
 rename expression/{ => builtin}/builtin_test.go (100%)
 rename expression/{ => builtin}/builtin_time.go (100%)
 rename expression/{ => builtin}/builtin_time_test.go (100%)

diff --git a/expression/builtin.go b/expression/builtin/builtin.go
similarity index 100%
rename from expression/builtin.go
rename to expression/builtin/builtin.go
diff --git a/expression/builtin_control.go b/expression/builtin/builtin_control.go
similarity index 100%
rename from expression/builtin_control.go
rename to expression/builtin/builtin_control.go
diff --git a/expression/builtin_control_test.go b/expression/builtin/builtin_control_test.go
similarity index 100%
rename from expression/builtin_control_test.go
rename to expression/builtin/builtin_control_test.go
diff --git a/expression/builtin_groupby.go b/expression/builtin/builtin_groupby.go
similarity index 100%
rename from expression/builtin_groupby.go
rename to expression/builtin/builtin_groupby.go
diff --git a/expression/builtin_groupby_test.go b/expression/builtin/builtin_groupby_test.go
similarity index 100%
rename from expression/builtin_groupby_test.go
rename to expression/builtin/builtin_groupby_test.go
diff --git a/expression/builtin_info.go b/expression/builtin/builtin_info.go
similarity index 100%
rename from expression/builtin_info.go
rename to expression/builtin/builtin_info.go
diff --git a/expression/builtin_info_test.go b/expression/builtin/builtin_info_test.go
similarity index 100%
rename from expression/builtin_info_test.go
rename to expression/builtin/builtin_info_test.go
diff --git a/expression/builtin_math.go b/expression/builtin/builtin_math.go
similarity index 100%
rename from expression/builtin_math.go
rename to expression/builtin/builtin_math.go
diff --git a/expression/builtin_math_test.go b/expression/builtin/builtin_math_test.go
similarity index 100%
rename from expression/builtin_math_test.go
rename to expression/builtin/builtin_math_test.go
diff --git a/expression/builtin_string.go b/expression/builtin/builtin_string.go
similarity index 100%
rename from expression/builtin_string.go
rename to expression/builtin/builtin_string.go
diff --git a/expression/builtin_string_test.go b/expression/builtin/builtin_string_test.go
similarity index 100%
rename from expression/builtin_string_test.go
rename to expression/builtin/builtin_string_test.go
diff --git a/expression/builtin_test.go b/expression/builtin/builtin_test.go
similarity index 100%
rename from expression/builtin_test.go
rename to expression/builtin/builtin_test.go
diff --git a/expression/builtin_time.go b/expression/builtin/builtin_time.go
similarity index 100%
rename from expression/builtin_time.go
rename to expression/builtin/builtin_time.go
diff --git a/expression/builtin_time_test.go b/expression/builtin/builtin_time_test.go
similarity index 100%
rename from expression/builtin_time_test.go
rename to expression/builtin/builtin_time_test.go

From 1a0998b07a201d910cc1f9664fffa67cbd37b22d Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 16:12:26 +0800
Subject: [PATCH 2/7] expression: move case when test from builtin to
 expression

---
 expression/case_test.go | 66 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 expression/case_test.go

diff --git a/expression/case_test.go b/expression/case_test.go
new file mode 100644
index 0000000000000..1a8ef525c325c
--- /dev/null
+++ b/expression/case_test.go
@@ -0,0 +1,66 @@
+// Copyright 2015 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package expression
+
+import (
+	. "github.com/pingcap/check"
+)
+
+var _ = Suite(&testCaseSuite{})
+
+type testCaseSuite struct {
+}
+
+func (s *testCaseSuite) TestCaseWhen(c *C) {
+	when0 := &WhenClause{mockExpr{val: 1}, mockExpr{val: "str1"}}
+	when1 := &WhenClause{mockExpr{val: 2}, mockExpr{val: "str2"}}
+	whens0 := []*WhenClause{when0, when1}
+
+	tbl := []struct {
+		value      Expression
+		whens      []*WhenClause
+		elseClause Expression
+		ret        interface{}
+	}{
+		{mockExpr{val: 1}, whens0, nil, "str1"},
+		{mockExpr{val: 2}, whens0, nil, "str2"},
+		{mockExpr{val: 3}, whens0, nil, nil},
+		{mockExpr{val: 3}, whens0, mockExpr{val: "str3"}, "str3"},
+	}
+
+	for _, t := range tbl {
+		f := FunctionCase{t.value, t.whens, t.elseClause}
+		v, err := f.Eval(nil, nil)
+		c.Assert(err, IsNil)
+		c.Assert(v, DeepEquals, t.ret)
+	}
+
+	f := FunctionCase{mockExpr{val: 4}, whens0, mockExpr{val: "else"}}
+	cf := f.Clone()
+	c.Assert(cf, NotNil)
+	v, err := f.Eval(nil, nil)
+	c.Assert(err, IsNil)
+	cv, err := cf.Eval(nil, nil)
+	c.Assert(err, IsNil)
+	c.Assert(v, DeepEquals, cv)
+	c.Assert(f.IsStatic(), IsFalse)
+
+	str := f.String()
+	c.Assert(len(str), Greater, 0)
+
+	when2 := &WhenClause{mockExpr{val: 2, isStatic: true}, mockExpr{val: "str2", isStatic: true}}
+	whens1 := []*WhenClause{when2}
+	f = FunctionCase{mockExpr{val: 4, isStatic: true}, whens1, mockExpr{val: "else", isStatic: true}}
+	c.Assert(f.IsStatic(), IsTrue)
+}

From 76d1a49ab69d931879c15d0b18aba140fa7c931d Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 16:14:04 +0800
Subject: [PATCH 3/7] *: rename package name and do some export

---
 expression/builtin/builtin.go              | 54 ++++++++++++----------
 expression/builtin/builtin_control.go      |  2 +-
 expression/builtin/builtin_control_test.go | 45 +-----------------
 expression/builtin/builtin_groupby.go      | 48 ++++++++-----------
 expression/builtin/builtin_groupby_test.go | 37 +++++++--------
 expression/builtin/builtin_info.go         |  2 +-
 expression/builtin/builtin_info_test.go    | 11 +++--
 expression/builtin/builtin_math.go         |  2 +-
 expression/builtin/builtin_math_test.go    |  2 +-
 expression/builtin/builtin_string.go       |  2 +-
 expression/builtin/builtin_string_test.go  |  8 ++--
 expression/builtin/builtin_test.go         | 16 +++----
 expression/builtin/builtin_time.go         |  2 +-
 expression/builtin/builtin_time_test.go    |  2 +-
 expression/call.go                         | 45 ++++++++++++++----
 expression/call_test.go                    |  8 ++++
 expression/helper.go                       |  5 +-
 17 files changed, 139 insertions(+), 152 deletions(-)

diff --git a/expression/builtin/builtin.go b/expression/builtin/builtin.go
index 7ca97638c3894..a76688e9767fd 100644
--- a/expression/builtin/builtin.go
+++ b/expression/builtin/builtin.go
@@ -15,22 +15,39 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
-import (
-	"fmt"
-	"strings"
+import "github.com/juju/errors"
 
-	"github.com/juju/errors"
+const (
+	// ExprEvalFn is the key saving Call expression.
+	ExprEvalFn = "$fn"
+	// ExprEvalArgCtx is the key saving Context for a Call expression.
+	ExprEvalArgCtx = "$ctx"
+	// ExprAggDone is the key indicating that aggregate function is done.
+	ExprAggDone = "$aggDone"
+	// ExprEvalArgAggEmpty is the key to evaluate the aggregate function for empty table.
+	ExprEvalArgAggEmpty = "$agg0"
+	// ExprAggDistinct is the key saving a distinct aggregate.
+	ExprAggDistinct = "$aggDistinct"
 )
 
-var builtin = map[string]struct {
-	f           func([]interface{}, map[interface{}]interface{}) (interface{}, error)
-	minArgs     int
-	maxArgs     int
-	isStatic    bool
-	isAggregate bool
-}{
+// Func is for a builtin function.
+type Func struct {
+	// F is the specific calling function.
+	F func([]interface{}, map[interface{}]interface{}) (interface{}, error)
+	// MinArgs is the minimal arguments needed,
+	MinArgs int
+	// MaxArgs is the maximal arguments needed, -1 for infinity.
+	MaxArgs int
+	// IsStatic shows whether this function can be called statically.
+	IsStatic bool
+	// IsAggregate represents whether this function is an aggregate function or not.
+	IsAggregate bool
+}
+
+// Funcs holds all registered builtin functions.
+var Funcs = map[string]Func{
 	// common functions
 	"coalesce": {builtinCoalesce, 1, -1, true, false},
 
@@ -83,19 +100,6 @@ var builtin = map[string]struct {
 	"user":         {builtinUser, 0, 0, false, false},
 }
 
-func badNArgs(min int, s string, args []interface{}) error {
-	a := []string{}
-	for _, v := range args {
-		a = append(a, fmt.Sprintf("%v", v))
-	}
-	switch len(args) < min {
-	case true:
-		return errors.Errorf("missing argument to %s(%s)", s, strings.Join(a, ", "))
-	default: //case false:
-		return errors.Errorf("too many arguments to %s(%s)", s, strings.Join(a, ", "))
-	}
-}
-
 func invArg(arg interface{}, s string) error {
 	return errors.Errorf("invalid argument %v (type %T) for %s", arg, arg, s)
 }
diff --git a/expression/builtin/builtin_control.go b/expression/builtin/builtin_control.go
index e1a39088bd605..ecc41acca4571 100644
--- a/expression/builtin/builtin_control.go
+++ b/expression/builtin/builtin_control.go
@@ -11,7 +11,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import "github.com/pingcap/tidb/util/types"
 
diff --git a/expression/builtin/builtin_control_test.go b/expression/builtin/builtin_control_test.go
index 70989e3bb4756..cf4e32ecd609b 100644
--- a/expression/builtin/builtin_control_test.go
+++ b/expression/builtin/builtin_control_test.go
@@ -11,7 +11,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"errors"
@@ -80,46 +80,3 @@ func (s *testBuiltinSuite) TestNullIf(c *C) {
 	_, err := builtinNullIf([]interface{}{errors.New("must error"), 1}, nil)
 	c.Assert(err, NotNil)
 }
-
-func (s *testBuiltinSuite) TestCaseWhen(c *C) {
-	when0 := &WhenClause{mockExpr{val: 1}, mockExpr{val: "str1"}}
-	when1 := &WhenClause{mockExpr{val: 2}, mockExpr{val: "str2"}}
-	whens0 := []*WhenClause{when0, when1}
-
-	tbl := []struct {
-		value      Expression
-		whens      []*WhenClause
-		elseClause Expression
-		ret        interface{}
-	}{
-		{mockExpr{val: 1}, whens0, nil, "str1"},
-		{mockExpr{val: 2}, whens0, nil, "str2"},
-		{mockExpr{val: 3}, whens0, nil, nil},
-		{mockExpr{val: 3}, whens0, mockExpr{val: "str3"}, "str3"},
-	}
-
-	for _, t := range tbl {
-		f := FunctionCase{t.value, t.whens, t.elseClause}
-		v, err := f.Eval(nil, nil)
-		c.Assert(err, IsNil)
-		c.Assert(v, DeepEquals, t.ret)
-	}
-
-	f := FunctionCase{mockExpr{val: 4}, whens0, mockExpr{val: "else"}}
-	cf := f.Clone()
-	c.Assert(cf, NotNil)
-	v, err := f.Eval(nil, nil)
-	c.Assert(err, IsNil)
-	cv, err := cf.Eval(nil, nil)
-	c.Assert(err, IsNil)
-	c.Assert(v, DeepEquals, cv)
-	c.Assert(f.IsStatic(), IsFalse)
-
-	str := f.String()
-	c.Assert(len(str), Greater, 0)
-
-	when2 := &WhenClause{mockExpr{val: 2, isStatic: true}, mockExpr{val: "str2", isStatic: true}}
-	whens1 := []*WhenClause{when2}
-	f = FunctionCase{mockExpr{val: 4, isStatic: true}, whens1, mockExpr{val: "else", isStatic: true}}
-	c.Assert(f.IsStatic(), IsTrue)
-}
diff --git a/expression/builtin/builtin_groupby.go b/expression/builtin/builtin_groupby.go
index b37544c4b0645..2cb6e0fc89024 100644
--- a/expression/builtin/builtin_groupby.go
+++ b/expression/builtin/builtin_groupby.go
@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"bytes"
@@ -30,18 +30,21 @@ import (
 
 // see https://dev.mysql.com/doc/refman/5.7/en/group-by-functions.html
 
-type aggregateDistinct struct {
-	// now we have to use memkv Temp, later may be use map directly
+// AggregateDistinct handles distinct data for aggregate function: count, sum, avg, and group_concat.
+type AggregateDistinct struct {
+	// Distinct is a memory key-value map.
+	// Now we have to use memkv Temp, later may be use map directly
 	Distinct memkv.Temp
 }
 
-func (c *Call) createDistinct() *aggregateDistinct {
-	a := new(aggregateDistinct)
+// CreateAggregateDistinct creates a distinct for function f.
+func CreateAggregateDistinct(f string, distinct bool) *AggregateDistinct {
+	a := &AggregateDistinct{}
 
-	switch strings.ToLower(c.F) {
+	switch strings.ToLower(f) {
 	case "count", "sum", "avg", "group_concat":
 		// only these aggregate functions support distinct
-		if c.Distinct {
+		if distinct {
 			a.Distinct, _ = memkv.CreateTemp(true)
 		}
 	}
@@ -50,7 +53,7 @@ func (c *Call) createDistinct() *aggregateDistinct {
 }
 
 // check whether v is distinct or not, return true for distinct
-func (a *aggregateDistinct) isDistinct(v ...interface{}) (bool, error) {
+func (a *AggregateDistinct) isDistinct(v ...interface{}) (bool, error) {
 	// no distinct flag
 	if a.Distinct == nil {
 		return true, nil
@@ -74,7 +77,7 @@ func (a *aggregateDistinct) isDistinct(v ...interface{}) (bool, error) {
 	return true, nil
 }
 
-func (a *aggregateDistinct) clear() {
+func (a *AggregateDistinct) clear() {
 	if a.Distinct == nil {
 		return
 	}
@@ -86,30 +89,15 @@ func (a *aggregateDistinct) clear() {
 	a.Distinct, _ = memkv.CreateTemp(true)
 }
 
-func getDistinct(ctx map[interface{}]interface{}, fn interface{}) *aggregateDistinct {
-	c, ok := fn.(*Call)
+func getDistinct(ctx map[interface{}]interface{}, fn interface{}) *AggregateDistinct {
+	v, ok := ctx[ExprAggDistinct]
 	if !ok {
-		// if fn is not a Call, maybe error
-		// but now we just return a dummpy aggregate distinct
-		return new(aggregateDistinct)
+		// here maybe an error, but now we just return a dummpy aggregate distinct
+		return new(AggregateDistinct)
 	}
 
-	// we may have multi aggregate function in one query
-	// e.g, select sum(c1) + count(*) from t
-	// so here we use a map to keep all aggregate disctinct
-	m := map[interface{}]interface{}{}
-	if v, ok := ctx[ExprAggDistinct]; ok {
-		m = v.(map[interface{}]interface{})
-	}
-
-	if d, ok := m[c]; ok {
-		return d.(*aggregateDistinct)
-	}
-
-	d := c.createDistinct()
-	m[c] = d
-
-	ctx[ExprAggDistinct] = m
+	// must be AggregateDistinct
+	d := v.(*AggregateDistinct)
 	return d
 }
 
diff --git a/expression/builtin/builtin_groupby_test.go b/expression/builtin/builtin_groupby_test.go
index 0baf4ffe257fb..a6116bd968134 100644
--- a/expression/builtin/builtin_groupby_test.go
+++ b/expression/builtin/builtin_groupby_test.go
@@ -11,11 +11,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	. "github.com/pingcap/check"
-
 	mysql "github.com/pingcap/tidb/mysqldef"
 	"github.com/pingcap/tidb/util/types"
 )
@@ -54,22 +53,23 @@ func (s *testBuiltinSuite) TestGroupBy(c *C) {
 	}
 
 	for _, t := range tbl {
-		// create a call and use dummy args.
-		e, err := NewCall(t.F, []Expression{Value{nil}}, t.Distinct)
-		c.Assert(err, IsNil)
-
-		call, ok := e.(*Call)
+		f, ok := Funcs[t.F]
 		c.Assert(ok, IsTrue)
 
 		m := map[interface{}]interface{}{}
+
+		m[ExprEvalFn] = new(Func)
+		m[ExprAggDistinct] = CreateAggregateDistinct(t.F, t.Distinct)
+
 		for _, arg := range t.RoundArgs {
-			call.Args = []Expression{Value{arg}}
+			args := []interface{}{arg}
 
-			_, err = call.Eval(nil, m)
+			_, err := f.F(args, m)
+			c.Assert(err, IsNil)
 		}
 
 		m[ExprAggDone] = struct{}{}
-		v, err := e.Eval(nil, m)
+		v, err := f.F(nil, m)
 		c.Assert(err, IsNil)
 		switch v.(type) {
 		case nil:
@@ -98,23 +98,24 @@ func (s *testBuiltinSuite) TestGroupConcat(c *C) {
 		{[]interface{}{1, nil, 1}, true, "1"},
 	}
 
+	f := builtinGroupConcat
+
 	for _, t := range tbl {
 		// create a call and use dummy args.
-		e, err := NewCall("group_concat", []Expression{Value{nil}}, t.Distinct)
-		c.Assert(err, IsNil)
-
-		call, ok := e.(*Call)
-		c.Assert(ok, IsTrue)
 
 		m := map[interface{}]interface{}{}
+		m[ExprEvalFn] = new(Func)
+		m[ExprAggDistinct] = CreateAggregateDistinct("group_concat", t.Distinct)
+
 		for _, arg := range t.RoundArgs {
-			call.Args = []Expression{Value{arg}}
+			args := []interface{}{arg}
 
-			_, err = call.Eval(nil, m)
+			_, err := f(args, m)
+			c.Assert(err, IsNil)
 		}
 
 		m[ExprAggDone] = struct{}{}
-		v, err := e.Eval(nil, m)
+		v, err := f(nil, m)
 		c.Assert(err, IsNil)
 		c.Assert(v, DeepEquals, t.Ret)
 	}
diff --git a/expression/builtin/builtin_info.go b/expression/builtin/builtin_info.go
index b9b85c61314ca..738922c408504 100644
--- a/expression/builtin/builtin_info.go
+++ b/expression/builtin/builtin_info.go
@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"github.com/juju/errors"
diff --git a/expression/builtin/builtin_info_test.go b/expression/builtin/builtin_info_test.go
index 5a1c9e42a5960..7b0e8d5866223 100644
--- a/expression/builtin/builtin_info_test.go
+++ b/expression/builtin/builtin_info_test.go
@@ -11,12 +11,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	. "github.com/pingcap/check"
 	"github.com/pingcap/tidb/sessionctx/db"
 	"github.com/pingcap/tidb/sessionctx/variable"
+	"github.com/pingcap/tidb/util/mock"
 )
 
 var _ = Suite(&testBuiltinInfoSuite{})
@@ -25,7 +26,7 @@ type testBuiltinInfoSuite struct {
 }
 
 func (s *testBuiltinSuite) TestDatabase(c *C) {
-	ctx := newMockCtx()
+	ctx := mock.NewContext()
 	m := map[interface{}]interface{}{}
 	v, err := builtinDatabase(nil, m)
 	c.Assert(err, NotNil)
@@ -42,7 +43,7 @@ func (s *testBuiltinSuite) TestDatabase(c *C) {
 }
 
 func (s *testBuiltinSuite) TestFoundRows(c *C) {
-	ctx := newMockCtx()
+	ctx := mock.NewContext()
 	m := map[interface{}]interface{}{}
 	v, err := builtinFoundRows(nil, m)
 	c.Assert(err, NotNil)
@@ -56,7 +57,7 @@ func (s *testBuiltinSuite) TestFoundRows(c *C) {
 }
 
 func (s *testBuiltinSuite) TestUser(c *C) {
-	ctx := newMockCtx()
+	ctx := mock.NewContext()
 	m := map[interface{}]interface{}{}
 	variable.BindSessionVars(ctx)
 	sessionVars := variable.GetSessionVars(ctx)
@@ -69,7 +70,7 @@ func (s *testBuiltinSuite) TestUser(c *C) {
 }
 
 func (s *testBuiltinSuite) TestCurrentUser(c *C) {
-	ctx := newMockCtx()
+	ctx := mock.NewContext()
 	m := map[interface{}]interface{}{}
 	variable.BindSessionVars(ctx)
 	sessionVars := variable.GetSessionVars(ctx)
diff --git a/expression/builtin/builtin_math.go b/expression/builtin/builtin_math.go
index ad9efce6ab83f..d9440ab9207e2 100644
--- a/expression/builtin/builtin_math.go
+++ b/expression/builtin/builtin_math.go
@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"math"
diff --git a/expression/builtin/builtin_math_test.go b/expression/builtin/builtin_math_test.go
index 8e253f2850e52..2846daa9df1e9 100644
--- a/expression/builtin/builtin_math_test.go
+++ b/expression/builtin/builtin_math_test.go
@@ -11,7 +11,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	. "github.com/pingcap/check"
diff --git a/expression/builtin/builtin_string.go b/expression/builtin/builtin_string.go
index b55c297d3a4d6..6f6cd2bf6f853 100644
--- a/expression/builtin/builtin_string.go
+++ b/expression/builtin/builtin_string.go
@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"fmt"
diff --git a/expression/builtin/builtin_string_test.go b/expression/builtin/builtin_string_test.go
index b6863d15afc16..923e803b16ce8 100644
--- a/expression/builtin/builtin_string_test.go
+++ b/expression/builtin/builtin_string_test.go
@@ -11,9 +11,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
+	"errors"
+
 	. "github.com/pingcap/check"
 )
 
@@ -47,7 +49,7 @@ func (s *testBuiltinSuite) TestConcat(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(v, IsNil)
 
-	args = []interface{}{mockExpr{}}
+	args = []interface{}{errors.New("must error")}
 	_, err = builtinConcat(args, nil)
 	c.Assert(err, NotNil)
 }
@@ -64,7 +66,7 @@ func (s *testBuiltinSuite) TestConcatWS(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(v, Equals, "a|b|c")
 
-	args = []interface{}{mockExpr{}}
+	args = []interface{}{errors.New("must error")}
 	_, err = builtinConcatWS(args, nil)
 	c.Assert(err, NotNil)
 }
diff --git a/expression/builtin/builtin_test.go b/expression/builtin/builtin_test.go
index da68377e245a4..498e2efb34dec 100644
--- a/expression/builtin/builtin_test.go
+++ b/expression/builtin/builtin_test.go
@@ -11,23 +11,21 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
+	"testing"
+
 	. "github.com/pingcap/check"
 )
 
-var _ = Suite(&testBuiltinSuite{})
-
-type testBuiltinSuite struct {
+func TestT(t *testing.T) {
+	TestingT(t)
 }
 
-func (s *testBuiltinSuite) TestBadNArgs(c *C) {
-	err := badNArgs(1, "", nil)
-	c.Assert(err, NotNil)
+var _ = Suite(&testBuiltinSuite{})
 
-	err = badNArgs(0, "", []interface{}{1})
-	c.Assert(err, NotNil)
+type testBuiltinSuite struct {
 }
 
 func (s *testBuiltinSuite) TestCoalesce(c *C) {
diff --git a/expression/builtin/builtin_time.go b/expression/builtin/builtin_time.go
index f274d236eae01..c71e2775197d7 100644
--- a/expression/builtin/builtin_time.go
+++ b/expression/builtin/builtin_time.go
@@ -15,7 +15,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"time"
diff --git a/expression/builtin/builtin_time_test.go b/expression/builtin/builtin_time_test.go
index 4ad100d14e4a8..42677d9791c0f 100644
--- a/expression/builtin/builtin_time_test.go
+++ b/expression/builtin/builtin_time_test.go
@@ -11,7 +11,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package expression
+package builtin
 
 import (
 	"strings"
diff --git a/expression/call.go b/expression/call.go
index b9d6337c0816b..3c42080981b1a 100644
--- a/expression/call.go
+++ b/expression/call.go
@@ -23,6 +23,7 @@ import (
 
 	"github.com/juju/errors"
 	"github.com/pingcap/tidb/context"
+	"github.com/pingcap/tidb/expression/builtin"
 )
 
 var (
@@ -38,17 +39,19 @@ type Call struct {
 	// Distinct only affetcts sum, avg, count, group_concat,
 	// so we can ignore it in other functions
 	Distinct bool
+
+	d *builtin.AggregateDistinct
 }
 
 // NewCall creates a Call expression with function name f, function args arg and
 // a distinct flag whether this function supports distinct or not.
 func NewCall(f string, args []Expression, distinct bool) (v Expression, err error) {
-	x := builtin[strings.ToLower(f)]
-	if x.f == nil {
+	x := builtin.Funcs[strings.ToLower(f)]
+	if x.F == nil {
 		return nil, errors.Errorf("undefined: %s", f)
 	}
 
-	if g, min, max := len(args), x.minArgs, x.maxArgs; g < min || (max != -1 && g > max) {
+	if g, min, max := len(args), x.MinArgs, x.MaxArgs; g < min || (max != -1 && g > max) {
 		a := []interface{}{}
 		for _, v := range args {
 			a = append(a, v)
@@ -82,8 +85,8 @@ func (c *Call) Clone() Expression {
 
 // IsStatic implements the Expression IsStatic interface.
 func (c *Call) IsStatic() bool {
-	v := builtin[strings.ToLower(c.F)]
-	if v.f == nil || !v.isStatic {
+	v := builtin.Funcs[strings.ToLower(c.F)]
+	if v.F == nil || !v.IsStatic {
 		return false
 	}
 
@@ -110,7 +113,7 @@ func (c *Call) String() string {
 
 // Eval implements the Expression Eval interface.
 func (c *Call) Eval(ctx context.Context, args map[interface{}]interface{}) (v interface{}, err error) {
-	f, ok := builtin[strings.ToLower(c.F)]
+	f, ok := builtin.Funcs[strings.ToLower(c.F)]
 	if !ok {
 		return nil, errors.Errorf("unknown function %s", c.F)
 	}
@@ -123,14 +126,38 @@ func (c *Call) Eval(ctx context.Context, args map[interface{}]interface{}) (v in
 		a[i] = v
 	}
 
+	c.createDistinct()
+
 	if args != nil {
-		args[ExprEvalFn] = c
-		args[ExprEvalArgCtx] = ctx
+		args[builtin.ExprEvalFn] = c
+		args[builtin.ExprEvalArgCtx] = ctx
+		args[builtin.ExprAggDistinct] = c.d
 	}
-	return f.f(a, args)
+	return f.F(a, args)
 }
 
 // Accept implements Expression Accept interface.
 func (c *Call) Accept(v Visitor) (Expression, error) {
 	return v.VisitCall(c)
 }
+
+func (c *Call) createDistinct() {
+	if c.d != nil {
+		return
+	}
+
+	c.d = builtin.CreateAggregateDistinct(c.F, c.Distinct)
+}
+
+func badNArgs(min int, s string, args []interface{}) error {
+	a := []string{}
+	for _, v := range args {
+		a = append(a, fmt.Sprintf("%v", v))
+	}
+	switch len(args) < min {
+	case true:
+		return errors.Errorf("missing argument to %s(%s)", s, strings.Join(a, ", "))
+	default: //case false:
+		return errors.Errorf("too many arguments to %s(%s)", s, strings.Join(a, ", "))
+	}
+}
diff --git a/expression/call_test.go b/expression/call_test.go
index faa06beb2eec1..86da8b4891429 100644
--- a/expression/call_test.go
+++ b/expression/call_test.go
@@ -74,3 +74,11 @@ func (s *testCallSuite) TestCall(c *C) {
 	_, err = f.Eval(nil, nil)
 	c.Assert(err, NotNil)
 }
+
+func (s *testCallSuite) TestBadNArgs(c *C) {
+	err := badNArgs(1, "", nil)
+	c.Assert(err, NotNil)
+
+	err = badNArgs(0, "", []interface{}{1})
+	c.Assert(err, NotNil)
+}
diff --git a/expression/helper.go b/expression/helper.go
index f5377086f2211..6e4710f68e6e0 100644
--- a/expression/helper.go
+++ b/expression/helper.go
@@ -30,6 +30,7 @@ import (
 	"github.com/ngaut/log"
 	"github.com/pingcap/tidb/context"
 
+	"github.com/pingcap/tidb/expression/builtin"
 	"github.com/pingcap/tidb/model"
 	mysql "github.com/pingcap/tidb/mysqldef"
 	"github.com/pingcap/tidb/parser/opcode"
@@ -146,13 +147,13 @@ func mentionedAggregateFuncs(e Expression, m *[]Expression) {
 		mentionedAggregateFuncs(x.L, m)
 		mentionedAggregateFuncs(x.R, m)
 	case *Call:
-		f, ok := builtin[strings.ToLower(x.F)]
+		f, ok := builtin.Funcs[strings.ToLower(x.F)]
 		if !ok {
 			log.Errorf("unknown function %s", x.F)
 			return
 		}
 
-		if f.isAggregate {
+		if f.IsAggregate {
 			// if f is aggregate function, we don't need check the arguments,
 			// because using an aggregate function in the aggregate arg like count(max(c1)) is invalid
 			// TODO: check whether argument contains an aggregate function and return error.

From e825e7c663efb04081b3be64414ac5b68788f25a Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 16:19:51 +0800
Subject: [PATCH 4/7] *: use builtin eval key instead

---
 expression/between_test.go |  3 ++-
 expression/binop_test.go   |  3 ++-
 expression/helper.go       | 10 ----------
 expression/ident.go        |  3 ++-
 expression/ident_test.go   |  5 +++--
 plan/plans/groupby.go      |  5 +++--
 6 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/expression/between_test.go b/expression/between_test.go
index ad0f729996177..b2f45c216c131 100644
--- a/expression/between_test.go
+++ b/expression/between_test.go
@@ -18,6 +18,7 @@ import (
 
 	. "github.com/pingcap/check"
 
+	"github.com/pingcap/tidb/expression/builtin"
 	"github.com/pingcap/tidb/util/types"
 )
 
@@ -47,7 +48,7 @@ func (s *testBetweenSuite) TestBetween(c *C) {
 	}
 
 	m := map[interface{}]interface{}{
-		ExprEvalArgAggEmpty: struct{}{},
+		builtin.ExprEvalArgAggEmpty: struct{}{},
 	}
 
 	for _, t := range table {
diff --git a/expression/binop_test.go b/expression/binop_test.go
index 442eac2a4e451..a82f50661b70d 100644
--- a/expression/binop_test.go
+++ b/expression/binop_test.go
@@ -19,6 +19,7 @@ import (
 
 	. "github.com/pingcap/check"
 
+	"github.com/pingcap/tidb/expression/builtin"
 	"github.com/pingcap/tidb/model"
 	mysql "github.com/pingcap/tidb/mysqldef"
 	"github.com/pingcap/tidb/parser/opcode"
@@ -171,7 +172,7 @@ func (s *testBinOpSuite) TestIdentRelOp(c *C) {
 	}
 
 	m := map[interface{}]interface{}{
-		ExprEvalArgAggEmpty: struct{}{},
+		builtin.ExprEvalArgAggEmpty: struct{}{},
 	}
 
 	for _, t := range tbl {
diff --git a/expression/helper.go b/expression/helper.go
index 6e4710f68e6e0..f6f9490a0cc12 100644
--- a/expression/helper.go
+++ b/expression/helper.go
@@ -39,22 +39,12 @@ import (
 )
 
 const (
-	// ExprEvalFn is the key saving Call expression.
-	ExprEvalFn = "$fn"
-	// ExprEvalArgCtx is the key saving Context for a Call expression.
-	ExprEvalArgCtx = "$ctx"
-	// ExprAggDone is the key indicating that aggregate function is done.
-	ExprAggDone = "$aggDone"
-	// ExprEvalArgAggEmpty is the key to evaluate the aggregate function for empty table.
-	ExprEvalArgAggEmpty = "$agg0"
 	// ExprEvalDefaultName is the key saving default column name for Default expression.
 	ExprEvalDefaultName = "$defaultName"
 	// ExprEvalIdentFunc is the key saving a function to retrieve value for identifier name.
 	ExprEvalIdentFunc = "$identFunc"
 	// ExprEvalPositionFunc is the key saving a Position expresion.
 	ExprEvalPositionFunc = "$positionFunc"
-	// ExprAggDistinct is the key saving a distinct aggregate.
-	ExprAggDistinct = "$aggDistinct"
 	// ExprEvalValuesFunc is the key saving a function to retrieve value for column name.
 	ExprEvalValuesFunc = "$valuesFunc"
 )
diff --git a/expression/ident.go b/expression/ident.go
index 6e268987862d1..a2ddff3d2378c 100644
--- a/expression/ident.go
+++ b/expression/ident.go
@@ -20,6 +20,7 @@ package expression
 import (
 	"github.com/juju/errors"
 	"github.com/pingcap/tidb/context"
+	"github.com/pingcap/tidb/expression/builtin"
 
 	"github.com/pingcap/tidb/model"
 )
@@ -57,7 +58,7 @@ func (i *Ident) Equal(x *Ident) bool {
 
 // Eval implements the Expression Eval interface.
 func (i *Ident) Eval(ctx context.Context, args map[interface{}]interface{}) (v interface{}, err error) {
-	if _, ok := args[ExprEvalArgAggEmpty]; ok {
+	if _, ok := args[builtin.ExprEvalArgAggEmpty]; ok {
 		// select c1, max(c1) from t where c1 = null, must get "NULL", "NULL" for empty table
 		return nil, nil
 	}
diff --git a/expression/ident_test.go b/expression/ident_test.go
index 5fe75f9c1ea0a..f71289be6c656 100644
--- a/expression/ident_test.go
+++ b/expression/ident_test.go
@@ -15,6 +15,7 @@ package expression
 
 import (
 	. "github.com/pingcap/check"
+	"github.com/pingcap/tidb/expression/builtin"
 	"github.com/pingcap/tidb/model"
 )
 
@@ -41,12 +42,12 @@ func (s *testIdentSuite) TestIdent(c *C) {
 	v, err := e.Eval(nil, m)
 	c.Assert(err, NotNil)
 
-	m[ExprEvalArgAggEmpty] = struct{}{}
+	m[builtin.ExprEvalArgAggEmpty] = struct{}{}
 	v, err = e.Eval(nil, m)
 	c.Assert(err, IsNil)
 	c.Assert(v, IsNil)
 
-	delete(m, ExprEvalArgAggEmpty)
+	delete(m, builtin.ExprEvalArgAggEmpty)
 	m[ExprEvalIdentFunc] = func(string) (interface{}, error) {
 		return 1, nil
 	}
diff --git a/plan/plans/groupby.go b/plan/plans/groupby.go
index f8460421a8f57..2d9d60b3522b0 100644
--- a/plan/plans/groupby.go
+++ b/plan/plans/groupby.go
@@ -23,6 +23,7 @@ import (
 	"github.com/juju/errors"
 	"github.com/pingcap/tidb/context"
 	"github.com/pingcap/tidb/expression"
+	"github.com/pingcap/tidb/expression/builtin"
 	"github.com/pingcap/tidb/field"
 	"github.com/pingcap/tidb/kv/memkv"
 	"github.com/pingcap/tidb/plan"
@@ -212,7 +213,7 @@ func (r *GroupByDefaultPlan) evalAggFields(ctx context.Context, out []interface{
 
 func (r *GroupByDefaultPlan) evalAggDone(ctx context.Context, out []interface{},
 	m map[interface{}]interface{}) error {
-	m[expression.ExprAggDone] = true
+	m[builtin.ExprAggDone] = true
 
 	var err error
 	// Eval aggregate field results done in ctx
@@ -234,7 +235,7 @@ func (r *GroupByDefaultPlan) evalEmptyTable(ctx context.Context) ([]interface{},
 
 	out := make([]interface{}, len(r.Fields))
 	// aggregate empty record set
-	m := map[interface{}]interface{}{expression.ExprEvalArgAggEmpty: true}
+	m := map[interface{}]interface{}{builtin.ExprEvalArgAggEmpty: true}
 
 	var err error
 	for i, fld := range r.Fields {

From 2c363b72b05ff9ff1faa22774def18a8b71693c7 Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 16:22:26 +0800
Subject: [PATCH 5/7] builtin: simplify file name

---
 expression/builtin/{builtin_control.go => control.go}           | 0
 expression/builtin/{builtin_control_test.go => control_test.go} | 0
 expression/builtin/{builtin_groupby.go => groupby.go}           | 0
 expression/builtin/{builtin_groupby_test.go => groupby_test.go} | 0
 expression/builtin/{builtin_info.go => info.go}                 | 0
 expression/builtin/{builtin_info_test.go => info_test.go}       | 0
 expression/builtin/{builtin_math.go => math.go}                 | 0
 expression/builtin/{builtin_math_test.go => math_test.go}       | 0
 expression/builtin/{builtin_string.go => string.go}             | 0
 expression/builtin/{builtin_string_test.go => string_test.go}   | 0
 expression/builtin/{builtin_time.go => time.go}                 | 0
 expression/builtin/{builtin_time_test.go => time_test.go}       | 0
 12 files changed, 0 insertions(+), 0 deletions(-)
 rename expression/builtin/{builtin_control.go => control.go} (100%)
 rename expression/builtin/{builtin_control_test.go => control_test.go} (100%)
 rename expression/builtin/{builtin_groupby.go => groupby.go} (100%)
 rename expression/builtin/{builtin_groupby_test.go => groupby_test.go} (100%)
 rename expression/builtin/{builtin_info.go => info.go} (100%)
 rename expression/builtin/{builtin_info_test.go => info_test.go} (100%)
 rename expression/builtin/{builtin_math.go => math.go} (100%)
 rename expression/builtin/{builtin_math_test.go => math_test.go} (100%)
 rename expression/builtin/{builtin_string.go => string.go} (100%)
 rename expression/builtin/{builtin_string_test.go => string_test.go} (100%)
 rename expression/builtin/{builtin_time.go => time.go} (100%)
 rename expression/builtin/{builtin_time_test.go => time_test.go} (100%)

diff --git a/expression/builtin/builtin_control.go b/expression/builtin/control.go
similarity index 100%
rename from expression/builtin/builtin_control.go
rename to expression/builtin/control.go
diff --git a/expression/builtin/builtin_control_test.go b/expression/builtin/control_test.go
similarity index 100%
rename from expression/builtin/builtin_control_test.go
rename to expression/builtin/control_test.go
diff --git a/expression/builtin/builtin_groupby.go b/expression/builtin/groupby.go
similarity index 100%
rename from expression/builtin/builtin_groupby.go
rename to expression/builtin/groupby.go
diff --git a/expression/builtin/builtin_groupby_test.go b/expression/builtin/groupby_test.go
similarity index 100%
rename from expression/builtin/builtin_groupby_test.go
rename to expression/builtin/groupby_test.go
diff --git a/expression/builtin/builtin_info.go b/expression/builtin/info.go
similarity index 100%
rename from expression/builtin/builtin_info.go
rename to expression/builtin/info.go
diff --git a/expression/builtin/builtin_info_test.go b/expression/builtin/info_test.go
similarity index 100%
rename from expression/builtin/builtin_info_test.go
rename to expression/builtin/info_test.go
diff --git a/expression/builtin/builtin_math.go b/expression/builtin/math.go
similarity index 100%
rename from expression/builtin/builtin_math.go
rename to expression/builtin/math.go
diff --git a/expression/builtin/builtin_math_test.go b/expression/builtin/math_test.go
similarity index 100%
rename from expression/builtin/builtin_math_test.go
rename to expression/builtin/math_test.go
diff --git a/expression/builtin/builtin_string.go b/expression/builtin/string.go
similarity index 100%
rename from expression/builtin/builtin_string.go
rename to expression/builtin/string.go
diff --git a/expression/builtin/builtin_string_test.go b/expression/builtin/string_test.go
similarity index 100%
rename from expression/builtin/builtin_string_test.go
rename to expression/builtin/string_test.go
diff --git a/expression/builtin/builtin_time.go b/expression/builtin/time.go
similarity index 100%
rename from expression/builtin/builtin_time.go
rename to expression/builtin/time.go
diff --git a/expression/builtin/builtin_time_test.go b/expression/builtin/time_test.go
similarity index 100%
rename from expression/builtin/builtin_time_test.go
rename to expression/builtin/time_test.go

From 85b436cad4254137d7d319122944d9961f58b5d4 Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 16:42:18 +0800
Subject: [PATCH 6/7] builtin: Address comment

---
 expression/builtin/groupby.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/expression/builtin/groupby.go b/expression/builtin/groupby.go
index 2cb6e0fc89024..69cd07253f186 100644
--- a/expression/builtin/groupby.go
+++ b/expression/builtin/groupby.go
@@ -28,7 +28,7 @@ import (
 	"github.com/pingcap/tidb/util/types"
 )
 
-// see https://dev.mysql.com/doc/refman/5.7/en/group-by-functions.html
+// See https://dev.mysql.com/doc/refman/5.7/en/group-by-functions.html
 
 // AggregateDistinct handles distinct data for aggregate function: count, sum, avg, and group_concat.
 type AggregateDistinct struct {
@@ -52,7 +52,7 @@ func CreateAggregateDistinct(f string, distinct bool) *AggregateDistinct {
 	return a
 }
 
-// check whether v is distinct or not, return true for distinct
+// Check whether v is distinct or not, return true for distinct
 func (a *AggregateDistinct) isDistinct(v ...interface{}) (bool, error) {
 	// no distinct flag
 	if a.Distinct == nil {

From 6d5b187db70eb337f14fe46804ff7b0ce97ab097 Mon Sep 17 00:00:00 2001
From: siddontang <siddontang@gmail.com>
Date: Wed, 23 Sep 2015 16:49:05 +0800
Subject: [PATCH 7/7] expression: cleanup creating distinct in call

---
 expression/call.go | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/expression/call.go b/expression/call.go
index 3c42080981b1a..b0016c32620f5 100644
--- a/expression/call.go
+++ b/expression/call.go
@@ -126,7 +126,10 @@ func (c *Call) Eval(ctx context.Context, args map[interface{}]interface{}) (v in
 		a[i] = v
 	}
 
-	c.createDistinct()
+	if c.d == nil {
+		// create a distinct if nil.
+		c.d = builtin.CreateAggregateDistinct(c.F, c.Distinct)
+	}
 
 	if args != nil {
 		args[builtin.ExprEvalFn] = c
@@ -141,14 +144,6 @@ func (c *Call) Accept(v Visitor) (Expression, error) {
 	return v.VisitCall(c)
 }
 
-func (c *Call) createDistinct() {
-	if c.d != nil {
-		return
-	}
-
-	c.d = builtin.CreateAggregateDistinct(c.F, c.Distinct)
-}
-
 func badNArgs(min int, s string, args []interface{}) error {
 	a := []string{}
 	for _, v := range args {