*: add builtin function FLOOR #2484

zhexuany · 2017-01-16T21:40:27Z

floor function is implemented in this PR
test case is also added in this PR.
passed all test cases.
A function wanted by Support more math functions #2420.

CLAassistant · 2017-01-16T21:40:28Z

All committers have signed the CLA.

ngaut · 2017-01-17T02:04:54Z

Good job.

shenli · 2017-01-17T02:06:42Z

parser/parser.y

@@ -237,6 +237,7 @@ import (
 	events		"EVENTS"
 	fieldKwd	"FIELD_KWD"
 	findInSet	"FIND_IN_SET"
+  floor   "FLOOR"


Use tab to make the code align.

shenli · 2017-01-17T02:07:15Z

parser/parser.y

@@ -2684,6 +2685,10 @@ FunctionCallNonKeyword:
 	{
 		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr($1), Args: []ast.ExprNode{$3.(ast.ExprNode)}}
 	}
+| "FLOOR" '(' Expression ')'


Use tab to make the code align.

shenli · 2017-01-17T02:07:44Z

plan/typeinferer.go

@@ -274,7 +274,7 @@ func (v *typeInferrer) handleFuncCallExpr(x *ast.FuncCallExpr) {
 		}
 	case "interval":
 		tp = types.NewFieldType(mysql.TypeLonglong)
-	case "ceil", "ceiling":
+	case "ceil", "ceiling", "floor":


Add a test case in plan/typeinferer_test.go

I tried. But I could not find ceil in this test file. This is kinda confused me. If I add test for floor, then so does ceil and ceiling.

DONE. Let me know, if you want me to add test case for ceil and ceiling.

Nice! But you could do this in another PR.

XuHuaiyu · 2017-01-17T02:25:53Z

expression/builtin_math.go

+		return args[0], nil
+	}
+
+	f, err := args[0].ToFloat64(ctx.GetSessionVars().StmtCtx)


Please check the result of floor("abc") in MySQL and your implementation in TiDB. It seems ToFloat64 will cause an error.

hanfei1991 · 2017-01-17T06:42:00Z

expression/builtin.go

@@ -147,6 +147,7 @@ var Funcs = map[string]Func{
 	ast.Abs:     {builtinAbs, 1, 1},
 	ast.Ceil:    {builtinCeil, 1, 1},
 	ast.Ceiling: {builtinCeil, 1, 1},
+	ast.Floor:   {builtinFloor, 1, 1},


also add this for this line: https://github.com/pingcap/tidb/pull/2484/files#diff-cdbd511e3e5f2bbe0a3c5c173d3938c2L298

hanfei1991 · 2017-01-17T06:45:40Z

expression/builtin_math.go

+		return args[0], nil
+	}
+	if args[0].Kind() == types.KindString {
+		args[0].SetFloat64(0)


why set args[0] to 0, instead of d ?

Ah, This is outdated commit. Already changed in lasted commit.

zhexuany · 2017-01-17T08:04:54Z

@XuHuaiyu Passed all test cases. Ready to merge into mater. Already rebased and applied all my commit on top of lasted master branch.

zhexuany · 2017-01-17T10:37:46Z

@shenli PTAL, is this ok?

shenli · 2017-01-17T18:37:17Z

expression/builtin_math.go

+		sc.IgnoreTruncate = false
+		return d, errors.Trace(err)
+	}
+	sc.IgnoreTruncate = false


Why change this? Maybe the original value of sc.IgnoreTruncate is true.

FLOOR(-1.b2) = -1 This will cause ErrTruncated error. Hence, sc.IgnoreTruncate has false value. I need to set sc.IgnoreTruncate to true in order to avoid ErrTruncated error which is came from getValidFloatPrefix(sc, str) in https://github.com/pingcap/tidb/blob/master/util/types/convert.go#L242

Maybe the original IgnoreTruncate value is true. So you must record an old IgnoreTruncate value

shenli · 2017-01-17T18:38:38Z

plan/typeinferer.go

@@ -274,7 +274,7 @@ func (v *typeInferrer) handleFuncCallExpr(x *ast.FuncCallExpr) {
 		}
 	case "interval":
 		tp = types.NewFieldType(mysql.TypeLonglong)
-	case "ceil", "ceiling":
+	case "ceil", "ceiling", "floor":


Nice! But you could do this in another PR.

zhexuany · 2017-01-18T04:11:35Z

@XuHuaiyu PTAL, is this ok to you?

XuHuaiyu

Rest LGTM

XuHuaiyu · 2017-01-18T04:04:09Z

expression/builtin_math.go

@@ -33,6 +33,7 @@ import (
 var (
 	_ functionClass = &absFunctionClass{}
 	_ functionClass = &ceilFunctionClass{}
+	_ functionClass = &floorFunctionClass{}


Please add definitions of floorFunctionClass and builtinFloorSig at the beginning of this file.

DONE @XuHuaiyu

ngaut · 2017-01-19T07:30:52Z

PTAL

hanfei1991 · 2017-01-19T07:34:12Z

expression/builtin_math.go

@@ -49,6 +50,7 @@ var (
 var (
 	_ builtinFunc = &builtinAbsSig{}
 	_ builtinFunc = &builtinCeilSig{}
+	_ builtinFunc = &builtinFloorSig{}


Also add for floorFunctionClass and add it into map funcs

hanfei1991 · 2017-01-19T08:28:03Z

expression/builtin.go

@@ -309,6 +310,7 @@ var funcs = map[string]functionClass{
 	ast.Abs:     &absFunctionClass{baseFunctionClass{ast.Abs, 1, 1}},
 	ast.Ceil:    &ceilFunctionClass{baseFunctionClass{ast.Ceil, 1, 1}},
 	ast.Ceiling: &ceilFunctionClass{baseFunctionClass{ast.Ceiling, 1, 1}},
+	ast.Floor:   &ceilFunctionClass{baseFunctionClass{ast.Floor, 1, 1}},


Why use ceilFunctionClass?

My bad. I did it too fast.

hanfei1991 · 2017-01-19T08:28:20Z

expression/builtin_math_test.go

+	Dtbl := tblToDtbl(tbl)
+
+	for _, t := range Dtbl {
+		v, err := builtinFloor(t["Arg"], s.ctx)


Test it by getting function class from funcs.

hanfei1991 · 2017-01-19T09:00:00Z

@zhexuany fix CI

zhexuany · 2017-01-19T09:32:54Z

@hanfei1991 fixed.

hanfei1991 · 2017-01-19T09:38:38Z

expression/builtin_math_test.go

+		{-1.23, -2, IsNil},
+		{"1.23", 1, IsNil},
+		{"-1.23", -2, IsNil},
+		// {"-1.b23", -1, NotNil},


Why comment these?

zhexuany · 2017-01-19T13:21:00Z

@hanfei1991 passed the CI and rewrite the test cases. Shall be all good now.

*: address the test failure *: fixed indentation issue in parser.y *: added more test cases *: fixed truncated data error in floor *: fixed test case in plan related with function floor *: added builtin func *: adding floor into funs map *: changing test case style *: fixing CI

zhexuany · 2017-01-21T14:33:33Z

@hanfei1991 @XuHuaiyu @shenli PTAL

shenli · 2017-01-21T14:34:32Z

LGTM
@hanfei1991 @XuHuaiyu PTAL

hanfei1991 · 2017-01-21T15:06:17Z

expression/builtin_math.go

+		return d, errors.Trace(err)
+	}
+
+	sc.IgnoreTruncate = false


The original IgnoreTruncate may be true, so you need to store the old value as a local var.

hanfei1991

LGTM

zhexuany force-pushed the builtin-floor branch from 502a9b4 to fd0dea4 Compare January 16, 2017 22:13

shenli reviewed Jan 17, 2017

View reviewed changes

XuHuaiyu reviewed Jan 17, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch 4 times, most recently from a878a6d to 12f34ef Compare January 17, 2017 06:37

hanfei1991 reviewed Jan 17, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch 5 times, most recently from 71267ae to c7142d4 Compare January 17, 2017 10:32

zhexuany force-pushed the builtin-floor branch 2 times, most recently from 5daa544 to c7142d4 Compare January 17, 2017 11:30

shenli reviewed Jan 17, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch from c7142d4 to ee68dba Compare January 17, 2017 21:26

zhexuany force-pushed the builtin-floor branch from ee68dba to f1aa04c Compare January 18, 2017 04:13

XuHuaiyu reviewed Jan 18, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch 2 times, most recently from 825505f to b3490f3 Compare January 18, 2017 09:48

hanfei1991 reviewed Jan 19, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch from 8d72fea to c0c89a3 Compare January 19, 2017 08:22

hanfei1991 reviewed Jan 19, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch from c0c89a3 to 5732755 Compare January 19, 2017 08:33

zhexuany force-pushed the builtin-floor branch 2 times, most recently from 0c0b642 to 647d8fd Compare January 19, 2017 09:28

hanfei1991 reviewed Jan 19, 2017

View reviewed changes

zhexuany force-pushed the builtin-floor branch 2 times, most recently from 5492695 to 56e3f85 Compare January 19, 2017 13:17

zhexuany force-pushed the builtin-floor branch from 56e3f85 to 052dffb Compare January 21, 2017 14:32

hanfei1991 reviewed Jan 21, 2017

View reviewed changes

*: imporve handling IgnoreTrucate

cbbff92

hanfei1991 approved these changes Jan 22, 2017

View reviewed changes

Merge branch 'master' into builtin-floor

e971818

hanfei1991 merged commit 383f203 into pingcap:master Jan 22, 2017

zhexuany deleted the builtin-floor branch January 22, 2017 06:22

*: add builtin function FLOOR #2484

*: add builtin function FLOOR #2484

Conversation

zhexuany commented Jan 16, 2017 • edited

CLAassistant commented Jan 16, 2017 • edited

ngaut commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany commented Jan 17, 2017 • edited

zhexuany commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhexuany commented Jan 18, 2017

XuHuaiyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngaut commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 commented Jan 19, 2017

zhexuany commented Jan 19, 2017

Choose a reason for hiding this comment

zhexuany commented Jan 19, 2017

zhexuany commented Jan 21, 2017

shenli commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 left a comment

Choose a reason for hiding this comment

zhexuany commented Jan 16, 2017 •

edited

CLAassistant commented Jan 16, 2017 •

edited

zhexuany commented Jan 17, 2017 •

edited