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.

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

ast: add set default role #266

Merged
merged 2 commits into from
Apr 9, 2019
Merged

ast: add set default role #266

merged 2 commits into from
Apr 9, 2019

Conversation

imtbkcat
Copy link

What problem does this PR solve?

Support SET DEFAULT ROLE to set default active roles for user.

What is changed and how it works?

Add SetDefaultRoleStmt for ast which contain information for executor to do next step.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Copy link
Contributor

@kennytm kennytm 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.

ast/misc.go Outdated
case SetRoleAll:
ctx.WriteKeyWord(" ALL")
default:
ctx.WriteKeyWord("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ctx.WriteKeyWord("")

No need to write nothing 🙂

Copy link
Author

Choose a reason for hiding this comment

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

@kennytm fixed.

@jackysp
Copy link
Member

jackysp commented Apr 4, 2019

Please address the comment, @imtbkcat .

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

kennytm
kennytm previously approved these changes Apr 4, 2019
@imtbkcat
Copy link
Author

imtbkcat commented Apr 8, 2019

@kennytm this pr need an approve 😊

kennytm
kennytm previously approved these changes Apr 8, 2019
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm
Copy link
Contributor

kennytm commented Apr 8, 2019

Please resolve the merge conflict 🙂

@imtbkcat
Copy link
Author

imtbkcat commented Apr 9, 2019

@kennytm conflict has been reslove, but approve was dismissed. Please approve again and merge this pr.

@jackysp jackysp merged commit a0b3014 into pingcap:master Apr 9, 2019
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
lyonzhi pushed a commit to lyonzhi/parser that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants