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: add plan recreator statement #1278

Merged
merged 5 commits into from
Jul 28, 2021
Merged

parser: add plan recreator statement #1278

merged 5 commits into from
Jul 28, 2021

Conversation

rebelice
Copy link
Contributor

What problem does this PR solve?

This is a step in tidb/issues/26325

What is changed and how it works?

Add PLAN RECREATOR statement

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Side effects

  • Possible performance regression
  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

@rebelice rebelice mentioned this pull request Jul 19, 2021
Open
11 tasks
@rebelice rebelice requested review from kennytm and xhebox and removed request for kennytm July 19, 2021 03:26
xhebox
xhebox reviewed Jul 19, 2021
View reviewed changes
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM. BTW, the document is internal, and there is no corresponding RFC on github.

parser.y

$$ = x
}
| "PLAN" "RECREATOR" "DUMP" "EXPLAIN" "ANALYZE" "SLOW" "QUERY" WhereClauseOptional OrderByOptional SelectStmtLimitOpt
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think AnalyzeOptional will better? Not a necessary change request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you. I'll modify it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I added AnalyzeOptional, I found that this abstraction seemed not necessary but added more work. I currently prefer not to modify this.

@rebelice
Copy link
Contributor Author

Rest LGTM. BTW, the document is internal, and there is no corresponding RFC on github.

Here is a description of this feature in pingcap/tidb#26325. This is the new feature for Sprint 4, I'll add documents visible to everyone before this feature GA. Is this OK for you?

@xhebox
Copy link
Contributor

xhebox commented Jul 19, 2021

@rebelice Sure.

xhebox
xhebox reviewed Jul 21, 2021
View reviewed changes
ast/misc.go Outdated
@@ -343,6 +343,11 @@ func (n *PlanRecreatorStmt) Accept(v Visitor) (Node, bool) {
return n, false
}
n.Stmt = node.(StmtNode)
stmt, ok := n.Stmt.Accept(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Accept twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that. Fixed.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 21, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • tiancaiamao
  • xhebox

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.

@zhuo-zhi zhuo-zhi mentioned this pull request Jul 21, 2021
Open
12 tasks
@tiancaiamao tiancaiamao self-requested a review July 28, 2021 03:21
tiancaiamao
tiancaiamao reviewed Jul 28, 2021
View reviewed changes
parser.y Outdated
@@ -5582,6 +5586,7 @@ UnReservedKeyword:
| "DAY"
| "DEALLOCATE"
| "DO"
| "DUMP"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not in the UnReservedKeyword list:
https://dev.mysql.com/doc/refman/8.0/en/keywords.html

It should be NotKeyword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated all

tiancaiamao
tiancaiamao reviewed Jul 28, 2021
View reviewed changes
parser.y Outdated
@@ -5624,6 +5629,7 @@ UnReservedKeyword:
| "PROXY"
| "QUICK"
| "REBUILD"
| "RECREATOR"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

tiancaiamao
tiancaiamao reviewed Jul 28, 2021
View reviewed changes
parser.y Outdated
@@ -5719,6 +5725,7 @@ UnReservedKeyword:
| "PROFILES"
| "MICROSECOND"
| "MINUTE"
| "PLAN"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto ... plan is also not a keyword

@ti-chi-bot ti-chi-bot added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Jul 28, 2021
@tiancaiamao
Copy link
Collaborator

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 862cea8

@ti-chi-bot ti-chi-bot merged commit 75cff0c into pingcap:master Jul 28, 2021
@rebelice rebelice mentioned this pull request Aug 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiancaiamao tiancaiamao tiancaiamao approved these changes

@xhebox xhebox xhebox approved these changes

@kennytm kennytm Awaiting requested review from kennytm

Assignees
No one assigned
Labels
size/XXL status/can-merge status/LGT2 LGT2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rebelice @xhebox @ti-chi-bot @tiancaiamao