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.

Sign up for GitHub

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

Jump to bottom

parser: fix compiling, add unit tests #28700

Merged
merged 5 commits into from
Oct 11, 2021
Merged

parser: fix compiling, add unit tests #28700

merged 5 commits into from
Oct 11, 2021

Conversation

xhebox
Copy link
Contributor

@xhebox xhebox commented Oct 11, 2021 •
edited
Loading

Signed-off-by: xhe [email protected]

What problem does this PR solve?

Issue Number: close #28699

Problem Summary: As title, after this PR, parser migration is completed.

Depends on pingcap/enterprise-plugin#50.

What is changed and how it works?

What's Changed:

  1. import path is changed.
  2. LDFLAGS = -X "github.com/pingcap/tidb/parser/mysql.TiDBReleaseVersion=$(shell git describe --tags --dirty --always)" to correct version.
  3. replace github.com/pingcap/tidb/parser => ./parser in three cmd/*test and the enterprise-plugin. It is needed to pass CI. Once the PR is merged, they can be removed/reverted.

Check List

Tests

  • Unit test
  • Integration test

Release note 

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 11, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mjonss
  • tangenta

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 11, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

  • *: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

@xhebox xhebox requested a review from a team as a code owner October 11, 2021 03:51
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 11, 2021
@sre-bot
Copy link
Contributor

sre-bot commented Oct 11, 2021

Please follow PR Title Format:

  • pkg [, pkg2, pkg3]: what is changed

Or if the count of mainly changed packages are more than 3, use

  • *: what is changed

After you have format title, you can leave a comment /run-check_title to recheck it

@xhebox xhebox changed the title [parser] fix compiling, add unit tests parser: fix compiling, add unit tests Oct 11, 2021
tangenta
tangenta reviewed Oct 11, 2021
View reviewed changes
executor/grant.go Outdated Show resolved Hide resolved
tangenta
tangenta reviewed Oct 11, 2021
View reviewed changes
expression/builtin.go Outdated Show resolved Hide resolved
tangenta
tangenta reviewed Oct 11, 2021
View reviewed changes
go.mod
@@ -97,4 97,4 @@ require (
// we need keep the replacement until go.etcd.io supports the higher version of grpc.
replace google.golang.org/grpc => google.golang.org/grpc v1.29.1

replace github.com/pingcap/parser => ./parser
replace github.com/pingcap/tidb/parser => ./parser
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this line?

Copy link
Contributor Author

@xhebox xhebox Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

No. It is intended. It is added such that changes in ./parser will take effect in this PR immediately. Otherwise CI/go will pull code from the latest commit, i.e. master, which does not contain your new changes. Then you need two PRs, 😕 , again.

The one in three tests and enterprise-plugin can be removed. Most of the time, they does not rely parser directly.

parser is still a separate module. So you will still need to first upload changes/push to github, to let other modules(tidb/plugin/tests) to import new changes. But this replace directive will help you out of doing so for tidb. @tangenta

Note that this directive does not make sense for other modules, e.g. three tests and enterprise-plugin.

tangenta
tangenta reviewed Oct 11, 2021
View reviewed changes
parser/ast/dml_test.go Outdated Show resolved Hide resolved
tangenta
tangenta reviewed Oct 11, 2021
View reviewed changes
parser/model/model.go Outdated Show resolved Hide resolved
tangenta
tangenta reviewed Oct 11, 2021
View reviewed changes
parser/parser_test.go Outdated Show resolved Hide resolved
parser/parser_test.go Outdated Show resolved Hide resolved
parser/parser_test.go Outdated Show resolved Hide resolved
parser/parser_test.go Outdated Show resolved Hide resolved
parser/yy_parser.go Outdated Show resolved Hide resolved
@tangenta
Copy link
Contributor

Rest LGTM

@xhebox
Copy link
Contributor Author

xhebox commented Oct 11, 2021

/run-build plugin=pr/50

1 similar comment
@xhebox
Copy link
Contributor Author

xhebox commented Oct 11, 2021

/run-build plugin=pr/50

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 11, 2021
@tangenta
Copy link
Contributor

We had better merge this ASAP. Otherwise, the conflicting files occur endlessly.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2021
mjonss
mjonss approved these changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

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

LGTM with some minor suggestion for make dev

Makefile Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 11, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2021
@xhebox
Copy link
Contributor Author

xhebox commented Oct 11, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 75f65ea

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Oct 11, 2021
@ti-chi-bot ti-chi-bot merged commit 94e30df into pingcap:master Oct 11, 2021
@xhebox xhebox deleted the parser_1 branch January 11, 2022 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tangenta tangenta tangenta approved these changes

@mjonss mjonss mjonss approved these changes

@bb7133 bb7133 bb7133 approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

parser migration: add unit tests
6 participants
@xhebox @ti-chi-bot @sre-bot @tangenta @bb7133 @mjonss