-
Notifications
You must be signed in to change notification settings - Fork 489
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: implement Restore for PrivElem, GrantStmt and RevokeStmt #198
Conversation
ast/misc.go
Outdated
@@ -93,6 +93,18 @@ type AuthOption struct { | |||
// TODO: support auth_plugin | |||
} | |||
|
|||
// Restore implements Node interface. | |||
func (n *AuthOption) Restore(ctx *RestoreCtx) error { |
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.
Restore "IDENTIFIED WITH" will get "IDENTIFIED BY" ?
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.
And there is no test to cover AutoOption
?
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.
-
According to
parser.y
, it looks like syntaxIDENTIFIED WITH string
has no effect:
Lines 7302 to 7319 in 63e2db7
| "IDENTIFIED" "BY" AuthString { $$ = &ast.AuthOption { AuthString: $3.(string), ByAuthString: true, } } | "IDENTIFIED" "WITH" StringName { $$ = nil } | "IDENTIFIED" "WITH" StringName "BY" AuthString { $$ = &ast.AuthOption { AuthString: $5.(string), ByAuthString: true, } } -
To test
Restore
ofAuthOption
(andUserSpec
,UserIdentity
), and to follow the testing routine of other types, it should be promoted as aNode
and addAccept
for it. Is that suitable?
LGTM |
PTAL @leoppro |
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.
Rest LGTM
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.
LGTM
2935269
to
97f22fc
Compare
case ObjectTypeNone: | ||
// do nothing | ||
case ObjectTypeTable: | ||
ctx.WriteKeyWord("TABLE ") |
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.
ctx.WriteKeyWord("TABLE ") | |
ctx.WriteKeyWord("TABLE") |
This also means line 1389 and 1447 would need to look like this 🤷♂️:
if n.ObjectType != ObjectTypeNone {
if err := n.ObjectType.Restore(ctx); err != nil {
return errors.Annotate(err, "An error occurred while restore RevokeStmt.ObjectType")
}
ctx.WritePlain(" ")
}
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.
Yes, this is better. Should I create a new pull request to fix it? Sorry for the trouble.
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.
@exialin Yes please. Thanks.
What problem does this PR solve?
Implement
Restore
forPrivElem
,GrantStmt
andRevokeStmt
.Waiting for some helper
Restore
s implemented in #197 .Issue: pingcap/tidb#8532
Check List
Tests