Skip to content

allow renaming views with RENAME TABLE statement #1712

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
Merged
merged 5 commits into from
Apr 14, 2023
Merged

allow renaming views with RENAME TABLE statement #1712

merged 5 commits into from
Apr 14, 2023

Conversation

jennifersp
Copy link
Contributor

@jennifersp jennifersp commented Apr 14, 2023 •
edited
Loading

  • Added renaming of views with RENAME TABLE ... TO ... statement
  • Added ViewDatabase implementation for PrivilegedDatabase

TODO: ALTER TABLE ... RENAME ... should fail for renaming of views. Currently, vitess parses both the statements into the same node, which makes GMS parser not be able to detect the difference.
Should return error: ERROR 1347 (HY000): 'mydb.myview' is not BASE TABLE

@jennifersp jennifersp marked this pull request as ready for review April 14, 2023 18:30
@jennifersp jennifersp requested a review from fulghum April 14, 2023 18:38
fulghum
fulghum reviewed Apr 14, 2023
View reviewed changes
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Code looks correct, just a few minor comments that I think will make it easier to read and maintain.

@jennifersp jennifersp requested a review from fulghum April 14, 2023 21:05
fulghum
fulghum approved these changes Apr 14, 2023
View reviewed changes
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looks good! 🚢

@jennifersp jennifersp merged commit dac7262 into main Apr 14, 2023
@jennifersp jennifersp deleted the jennifer/view-rename branch April 14, 2023 21:22
fulghum added a commit that referenced this pull request Apr 17, 2023
@fulghum
Revert "allow renaming views with RENAME TABLE statement (#1712)"
84cbf30 
This reverts commit dac7262.
@fulghum fulghum mentioned this pull request Apr 17, 2023
Merged
@fulghum
Copy link
Contributor

fulghum commented Apr 17, 2023

The change to PrivilegedDatabase is breaking some behavior in the Dolt cluster DB integration tests (dolthub/dolt#5751), so I'm going to temporarily revert this change so we can get other changes flowing through. Then we can follow up and figure out why the cluster tests are breaking from this. Might be worth separating the ViewDatabase / PrivilegedDatabase change from the bug fix here.

fulghum added a commit that referenced this pull request Apr 17, 2023
@fulghum
Merge pull request #1716 from dolthub/fulghum/revert-dac7262c9b7904b9…
5f335ee 
…31f9792ac8ce3eb52f6f9ccf

Revert "allow renaming views with `RENAME TABLE` statement (#1712)"
@jennifersp jennifersp mentioned this pull request May 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fulghum fulghum fulghum approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jennifersp @fulghum