Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parser: parse error compatible #4238

Merged
merged 13 commits into from
Oct 10, 2017
Merged

Conversation

mxlxm
Copy link
Contributor

@mxlxm mxlxm commented Aug 18, 2017

  1. add ParseErrorWith function to return mysql compatible parse error.
  2. error on unclose comment, fix Some incorrect SQLs in prepare doesn't return the parse error #3738.

@hanfei1991 hanfei1991 added the contribution This PR is from a community contributor. label Aug 18, 2017
mysql/const.go Outdated
@@ -62,6 +62,9 @@ const (
MaxKeyParts int = 16
)

// ErrTextLength error text length limit.
const ErrTextLength = 80
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it is 80 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested with mysql:
sql prepare p from "delete from t where a=1 /*' ccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc";
and got response like:
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '/*' cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc' at line 1
only keep nearest 80 bytes.

@@ -1551,6 +1551,8 @@ func (s *testParserSuite) TestComment(c *C) {
{"START TRANSACTION /*!40108 WITH CONSISTENT SNAPSHOT */", true},
// for comment in query
{"/*comment*/ /*comment*/ select c /* this is a comment */ from t;", true},
// for unclosed comment
{"delete from t where a = 7 or 1=1/*' and b = 'p'", false},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add more tests for error messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

@@ -90,7 +90,7 @@ func (s *Scanner) Errors() []error {

// reset resets the sql string to be scanned.
func (s *Scanner) reset(sql string) {
s.r = reader{s: sql}
s.r = reader{s: sql, p: Pos{Line: 1}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sql line counter should always start with 1.

@zz-jason
Copy link
Member

@shenli @jackysp PTAL

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sre-bot
Copy link
Contributor

sre-bot commented Sep 4, 2017

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.

@iamxy
Copy link
Member

iamxy commented Sep 4, 2017

/ok-to-test

@jackysp
Copy link
Member

jackysp commented Sep 4, 2017

@zz-jason PTAL

@shenli
Copy link
Member

shenli commented Sep 27, 2017

/run-all-tests

@zz-jason
Copy link
Member

zz-jason commented Oct 9, 2017

/run-all-test

@zz-jason
Copy link
Member

/ok-to-test

@zz-jason
Copy link
Member

/run-all-test

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Oct 10, 2017
@zz-jason zz-jason merged commit a7aaa64 into pingcap:master Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some incorrect SQLs in prepare doesn't return the parse error
7 participants