-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat: allow extensions to implement CREATE/DROP DATABASE #6115
Conversation
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.
Thanks for the changes! One more thing that I did not realize before - the code for DROP DATABASE
seems to work with the existing DETACH
code. These are two distinct concepts that should be kept separate in my opinion. Perhaps we can move the DETACH
code to a separate operator instead?
The PR also seems to be missing code that extends the grammar with support for DROP DATABASE
. Should that not be added as well?
auto result = make_unique<DetachStatement>(); | ||
auto info = make_unique<DetachInfo>(); | ||
info->name = stmt->db_name; | ||
info->if_exists = stmt->missing_ok; |
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.
Planning to change missing_ok
=> if_exists
@@ -100,6 +100,8 @@ string LogicalOperatorToString(LogicalOperatorType type) { | |||
return "CREATE_SCHEMA"; | |||
case LogicalOperatorType::LOGICAL_ATTACH: | |||
return "ATTACH"; | |||
case LogicalOperatorType::LOGICAL_DETACH: | |||
return "ATTACH"; |
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.
Is this supposed to return "DETACH"?
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.
Good catch yes indeed.
Thanks! |
Supersedes PR #6075