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

lexer: support more syntaxes regarding 'SET Syntax' #7020

Merged
merged 7 commits into from Jul 10, 2018

Conversation

@qazbnm456
Copy link
Contributor

commented Jul 9, 2018

What have you changed? (mandatory)

I've changed lines of the code of lexer.go in order to support more syntaxes regarding 'SET Syntax', which also fixes #6990.

What are the type of the changes (mandatory)?

Improvement.

How has this PR been tested (mandatory)?

Manual test.

Does this PR affect documentation (docs/docs-cn) update? (optional)

No.

Refer to a related PR or issue link (optional)

#6990

@sre-bot

This comment has been minimized.

Copy link

commented Jul 9, 2018

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.

@jackysp jackysp added the contribution label Jul 9, 2018
@qazbnm456 qazbnm456 force-pushed the qazbnm456:master branch from 92f1250 to c1f179f Jul 9, 2018
@shenli

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

@qazbnm456 Thanks!

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

/ok-to-test

1 similar comment
@shenli

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

/ok-to-test

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

/run-all-tests

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

Is there a MySQL document link about it? @qazbnm456

@qazbnm456

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2018

@shenli

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

/run-all-tests

Copy link
Member

left a comment

@qazbnm456 thank you for your contribution! Please add some test cases in

func (s *testParserSuite) TestDBAStmt(c *C) {

@qazbnm456 qazbnm456 force-pushed the qazbnm456:master branch from c55d3a5 to bfafd16 Jul 10, 2018
@qazbnm456

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@JackDrogon Added! Thanks.

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

Are they all equal?
@
@''
@""

@``

How about this:

set @ = 3;
select @'';

@qazbnm456

@qazbnm456

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@tiancaiamao

Yes, they're the same in MySQL. Example: http://sqlfiddle.com/#!9/9eecb/46076.
After applying this patch to the master branch of TiDB, it works the same:
image

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

I mean, could this PR handle the case, in TiDB

@qazbnm456

This comment has been minimized.

Copy link
Contributor Author

commented Jul 10, 2018

@tiancaiamao I just updated my comment with a picture showing that the case would be handled correctly in TiDB.

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2018

LGTM

Copy link
Member

left a comment

LGTM

@jackysp

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

/run-all-tests

@jackysp jackysp added status/LGT2 and removed status/LGT1 labels Jul 10, 2018
@jackysp jackysp merged commit fd37061 into pingcap:master Jul 10, 2018
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
zhexuany added a commit to zhexuany/tidb that referenced this pull request Jul 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.