parser: support sql_mode 'IGNORE SPACE' #5106

spongedu · 2017-11-14T16:35:31Z

support sql_mode 'IGNORE SPACE' for TiDB, and refine function name resolution rule according to MySQL's behavior. See https://dev.mysql.com/doc/refman/5.7/en/function-resolution.html for details.

sre-bot · 2017-11-14T16:35:34Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

XuHuaiyu · 2017-11-15T02:34:20Z

/ok-to-test

shenli · 2017-11-16T02:14:15Z

@spongedu Thanks!
Please resolve the conflicts.

shenli · 2017-11-16T02:16:16Z

mysql/const.go

@@ -452,6 +452,16 @@ func (m SQLMode) HasRealAsFloatMode() bool {
 	return m&ModeRealAsFloat == ModeRealAsFloat
 }

+// HasNoBackslashEscapesMode detects if 'NO_BACKSLASH_ESCAPES' mode is set in SQLMode


This function is not related with IGNORE_SPACE.

@shenli It seems to be taken into this pr in a merge. I'll delete this.

shenli · 2017-11-16T02:21:14Z

parser/parser_test.go

@@ -828,7 +828,7 @@ func (s *testParserSuite) TestBuiltin(c *C) {

 		// for date, day, weekday
 		{"SELECT CURRENT_DATE, CURRENT_DATE(), CURDATE()", true},
-		{"SELECT CURRENT_DATE, CURRENT_DATE(), CURDATE(1)", true},
+		{"SELECT CURRENT_DATE, CURRENT_DATE(), CURDATE(1)", false},


Why this is false case?

@shenli it seems that MySQL doesn't support CURDATE with one parameter. see: https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_curdate

shenli · 2017-11-16T02:21:33Z

/run-all-tests

zz-jason · 2017-11-17T08:35:35Z

@spongedu plz resolve conflicts.

Conflicts: mysql/const.go mysql/const_test.go

zz-jason · 2017-11-20T07:11:37Z

parser/parser.y

@@ -2989,10 +3019,18 @@ FunctionCallKeyword:
 	{
 		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr($1), Args: $3.([]ast.ExprNode)}
 	}
+|   builtinUser '('	ExpressionListOpt ')'


keep the code indentation

@zz-jason ok

tiancaiamao · 2017-11-20T07:38:10Z

parser/lexer.go

@@ -435,7 +470,16 @@ func scanIdentifier(s *Scanner) (int, Pos, string) {
 	pos := s.r.pos()
 	s.r.inc()
 	s.r.incAsLongAs(isIdentChar)
-	return identifier, pos, s.r.data(&pos)
+	lit := s.r.data(&pos)


It's quite performance sensitive here.
strings.ToLower(lit) would cause allocation, and map access is not that fast.
I think you can switch the order, check IgnoreSpaceMode first, try the best not to hurt the performance in most cases.

@tiancaiamao ok, I'll take a look. thx :)

@tiancaiamao I've refine this place. PTAL

Conflicts: mysql/const_test.go

zz-jason · 2017-11-22T03:09:16Z

parser/lexer.go

@@ -25,6 +25,41 @@ import (

 var _ = yyLexer(&Scanner{})

+var ignoreSpaceFuncMap = map[string]int{


How do we obtain tis map ?

@zz-jason I've rewrite this part. PTAL

…RE_SPACE' mode 2. Move check for 'IGNORE_SPACE' to 'isTokenIdentifier' 3. add more tests

tiancaiamao · 2017-11-27T02:39:34Z

/run-all-tests

tiancaiamao · 2017-11-27T03:19:53Z

LGTM

misoperation

tiancaiamao · 2017-11-27T03:26:24Z

PTAL @zz-jason @shenli @XuHuaiyu

zz-jason · 2017-11-27T03:37:55Z

parser/parser.y

 |	FunctionNameOptionalBraces OptionalBraces
 	{
 		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr($1)}
 	}
+|   builtinCurDate '(' ')'


keep code indentation

zz-jason · 2017-11-27T03:38:01Z

parser/parser.y

+	{
+		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr($1), Args: $3.([]ast.ExprNode)}
+	}
+| builtinSysDate '(' FuncDatetimePrecListOpt ')'


zz-jason · 2017-11-27T08:06:12Z

/run-all-tests

zimulala

LGTM

spongedu · 2017-12-06T17:19:50Z

@zimulala Fixed the failed cases. PTAL, Thanks :)

spongedu added 2 commits November 15, 2017 00:29

parser: support sql_mode 'IGNORE_SPACE'

8a4bbe8

Merge remote-tracking branch 'upstream/master' into mode_ignore_space

694b8c0

spongedu added 2 commits November 15, 2017 09:10

Fix regression

8439169

revert pingcap#5073 related codes according to pingcap#5104

2730b3b

shenli reviewed Nov 16, 2017

View reviewed changes

zz-jason added this to the 1.1 milestone Nov 16, 2017

zz-jason added conflicting contribution This PR is from a community contributor. labels Nov 16, 2017

Merge remote-tracking branch 'upstream/master' into mode_ignore_space

a04a3b2

Conflicts: mysql/const.go mysql/const_test.go

spongedu force-pushed the mode_ignore_space branch from df777db to a04a3b2 Compare November 18, 2017 02:16

zz-jason reviewed Nov 20, 2017

View reviewed changes

Code format refine

6276a97

tiancaiamao reviewed Nov 20, 2017

View reviewed changes

coocood removed the conflicting label Nov 21, 2017

Merge remote-tracking branch 'upstream/master' into mode_ignore_space

a941c25

Conflicts: mysql/const_test.go

zz-jason reviewed Nov 22, 2017

View reviewed changes

spongedu added 2 commits November 23, 2017 23:25

Merge branch 'master' into mode_ignore_space

fdb6110

1, Bug Fix: 'bit_xxx' and 'now' are not correctly recognized in 'IGNO…

e61e76a

…RE_SPACE' mode 2. Move check for 'IGNORE_SPACE' to 'isTokenIdentifier' 3. add more tests

tiancaiamao previously approved these changes Nov 27, 2017

View reviewed changes

zz-jason reviewed Nov 27, 2017

View reviewed changes

Merge branch 'master' into mode_ignore_space

2d53514

Format refine

8380384

spongedu force-pushed the mode_ignore_space branch from b60485f to 8380384 Compare November 28, 2017 07:26

Merge branch 'master' into mode_ignore_space

6ef56f7

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 30, 2017

Merge branch 'master' into mode_ignore_space

dd69562

zimulala previously approved these changes Dec 6, 2017

View reviewed changes

Merge branch 'master' into mode_ignore_space

763145c

zimulala added all-tests-passed status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 6, 2017

zimulala and others added 4 commits December 6, 2017 19:25

Merge branch 'master' into mode_ignore_space

8846cc7

Merge branch 'master' into mode_ignore_space

87b62fd

Merge remote-tracking branch 'upstream/master' into mode_ignore_space

5b75b17

Fix case fail

76b4f26

spongedu dismissed zimulala’s stale review via 76b4f26 December 6, 2017 17:04

coocood approved these changes Dec 7, 2017

View reviewed changes

coocood merged commit d742d92 into pingcap:master Dec 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser: support sql_mode 'IGNORE SPACE' #5106

parser: support sql_mode 'IGNORE SPACE' #5106

spongedu commented Nov 14, 2017

sre-bot commented Nov 14, 2017

XuHuaiyu commented Nov 15, 2017

shenli commented Nov 16, 2017

shenli Nov 16, 2017

spongedu Nov 16, 2017

shenli Nov 16, 2017

spongedu Nov 16, 2017

shenli commented Nov 16, 2017

zz-jason commented Nov 17, 2017

zz-jason Nov 20, 2017

spongedu Nov 20, 2017

tiancaiamao Nov 20, 2017

spongedu Nov 20, 2017

spongedu Nov 24, 2017

zz-jason Nov 22, 2017

spongedu Nov 25, 2017

tiancaiamao commented Nov 27, 2017

tiancaiamao commented Nov 27, 2017

tiancaiamao commented Nov 27, 2017

zz-jason Nov 27, 2017

spongedu Nov 27, 2017

zz-jason Nov 27, 2017

spongedu Nov 27, 2017

zz-jason commented Nov 27, 2017

zimulala left a comment

spongedu commented Dec 6, 2017

		@@ -25,6 +25,41 @@ import (

		var _ = yyLexer(&Scanner{})

		var ignoreSpaceFuncMap = map[string]int{

parser: support sql_mode 'IGNORE SPACE' #5106

parser: support sql_mode 'IGNORE SPACE' #5106

Conversation

spongedu commented Nov 14, 2017

sre-bot commented Nov 14, 2017

XuHuaiyu commented Nov 15, 2017

shenli commented Nov 16, 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

shenli commented Nov 16, 2017

zz-jason commented Nov 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

tiancaiamao commented Nov 27, 2017

tiancaiamao commented Nov 27, 2017

tiancaiamao commented Nov 27, 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

zz-jason commented Nov 27, 2017

zimulala left a comment

Choose a reason for hiding this comment

spongedu commented Dec 6, 2017