-
Notifications
You must be signed in to change notification settings - Fork 490
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
restore: support restoring SQL wrapping with TiDB special comments #1287
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
what is the reason removing (i mean i don't really like to keep |
FeatureIDForceAutoInc: {}, | ||
} | ||
|
||
func CanParseFeature(fs ...string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code is more general, the parser is not specific to TiDB and you can register the feature to control it's behavior ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, since you are the original author of this code, if you thinks it's OK, I have no problem with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the parser is not specific to TiDB
But the keywords(AUTO_RANDOM
, CLUSTERED_INDEX
...) are TiDB-specific. It is reasonable to attach these keywords to fixed feature IDs.
you can register the feature to control it's behavior
The register mechanism is not that convenient. If the user registers a new feature ID(/*T![new_feature_id] */
), he must also modify the code in the parser to add a new keyword to wrap. If the user doesn't need the new keyword, he should use /*T! xxx */
instead.
This comment has been minimized.
This comment has been minimized.
/merge |
@tangenta: Your PR was out of date, I have automatically updated it for you. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
/merge cancel |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: d0b457f
|
What problem does this PR solve?
TiDB Binlog and Lightning use regex to substitute the TiDB-specific keyword with special comments:
The regex is fragile because it is easy to get things wrong when the table names or column names contain these keywords, for example:
is incorrectly rewritten to
Comparing with pattern matching, using
ast.Restore
to wrap special comments is not prone to error.What is changed and how it works?
tidb/features.go
as the single source of truth.RestoreTiDBSpecialComment
, support wrapping with TiDB special comment.SpecialCommentsController
.Check List
Tests
Code changes
Side effects
NA
Related changes