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: refactor parser first step #4545

Merged
merged 4 commits into from Sep 18, 2017

Conversation

Projects
None yet
6 participants
@coocood
Member

coocood commented Sep 17, 2017

This is the first step to refactor parser. it solve several problems.

  1. function name can be general identifier, we don't need to define a keyword for a builtin function.

  2. write test to make sure keyword is consistent with lexer token. this fixes #4538.

  3. Removed ReservedKeyword, solve the identifier in scanner.

  4. Simplified precedence definition.

parser: refactor parser first step
This is the first step to refactor parser. it solve several problems.

1. function name can be general identifier, we don't need to define a keyword for a builtin function.

2. write test to make sure keyword is consistent with lever token. this fixes #4538.

3. Removed ReservedKeyword, solve the identifier in scanner.

4. Simplified precedence definition.
@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood
Member

coocood commented Sep 17, 2017

@ngaut

This comment has been minimized.

Show comment
Hide comment
@ngaut

ngaut Sep 17, 2017

Member

Well done.

Member

ngaut commented Sep 17, 2017

Well done.

@shenli

This comment has been minimized.

Show comment
Hide comment
@shenli

shenli Sep 17, 2017

Member

Is there an issue or trello card to trace the whole refactor work?

Member

shenli commented Sep 17, 2017

Is there an issue or trello card to trace the whole refactor work?

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 17, 2017

Member

@shenli
Here is the issue, I should mention it in PR description
#4525

Member

coocood commented Sep 17, 2017

@shenli
Here is the issue, I should mention it in PR description
#4525

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 18, 2017

Contributor

This PR changes a lot of code, but I think it's ok as long as the tests works.
Well done! @coocood

Contributor

tiancaiamao commented Sep 18, 2017

This PR changes a lot of code, but I think it's ok as long as the tests works.
Well done! @coocood

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 18, 2017

Member

@tiancaiamao
There are about 200 lines are caused by go fmt, only about 500 added lines need to review.

Member

coocood commented Sep 18, 2017

@tiancaiamao
There are about 200 lines are caused by go fmt, only about 500 added lines need to review.

@tiancaiamao

This comment has been minimized.

Show comment
Hide comment
@tiancaiamao

tiancaiamao Sep 18, 2017

Contributor

LGTM

Contributor

tiancaiamao commented Sep 18, 2017

LGTM

@coocood coocood added the status/LGT1 label Sep 18, 2017

@winkyao

LGTM, well done

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 18, 2017

Member

/run-all-test

Member

coocood commented Sep 18, 2017

/run-all-test

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 18, 2017

Member

/run-integration-common-test

Member

coocood commented Sep 18, 2017

/run-integration-common-test

@coocood coocood added status/LGT2 and removed status/LGT1 labels Sep 18, 2017

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 18, 2017

Member

/run-all-test

Member

coocood commented Sep 18, 2017

/run-all-test

@coocood coocood merged commit dd06221 into master Sep 18, 2017

9 of 10 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@coocood coocood deleted the coocood/parser-function branch Sep 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment