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
Grammar: adds support for CREATE/ALTER/DROP DATABASE for Postgres dialect #2081
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2081 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 148 148
Lines 10575 10590 +15
=========================================
+ Hits 10575 10590 +15
Continue to review full report at Codecov.
|
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.
@derickl a couple things for CREATE/DROP whilst I'm still reviewing ALTER 👍
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.
A couple more suggestions for ALTER DATABASE.
It's looking great so far! 🙌
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
Based on this #2081 (comment) I've grep'd through the source code and haven't come across this usage pattern What would be the difference between When does one choose |
Thank you for the review @jpy-git! |
Co-authored-by: Joseph Young <80432516+jpy-git@users.noreply.github.com>
A good question and one I’ve wondered in the past! As far as I can tell there is no real difference and it comes down to personal preference. In theory using Personally I have a slight preference for
But honestly it doesn’t make a big difference. Maybe we should standardise on a preferred style but when I looked before they were both used quite a bit and I’ll bet it’s got worse since then. |
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.
Looks good, couple final things to add then we should be good to get this going 👍
Ref("DatabaseReferenceSegment"), | ||
OneOf( | ||
Sequence( | ||
Sequence("WITH", optional=True), |
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.
Sequence("WITH", optional=True), | |
Ref.keyword("WITH", optional=True), |
To be consistent with CREATE/DROP
@derickl good spot on the Basically Ref.keyword is the syntax for explicitly specifying to search for the string in the keywords. You can actually think of any of the keywords as being wrapped by Ref.keyword parse_grammar = Sequence(
Ref.keyword("CREATE"),
Ref.keyword("DATABASE"),
... but for shorthand you can exclude However, in the specific example of the WITH in this PR we need a way to tell the parser that it is optional. That's why we pass the keyword via the more explicit I agree that Sequence could work but think Ref.keyword is more accurate to the logic we're trying to express (the maintainers can have a discussion around consistency outside of this PR). |
315edc4
to
a128385
Compare
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.
Awesome work @derickl and really robust test cases, LGTM 🚀 🌟
Thanks for the great review @jpy-git |
Thank you for sharing your thoughts here @tunetheweb. I hope we converge to some consistency down the road. |
Or maybe not if @jpy-git and I can't even agree! 🤣 |
Brief summary of the change made
Adds full support for [CREATE | ALTER | DROP] DATABASE statements.
Also fixes #2017
Are there any other side effects of this change that we should be aware of?
None
Pull Request checklist
Please confirm you have completed any of the necessary steps below.
Included test cases to demonstrate any code changes, which may be one or more of the following:
.yml
rule test cases intest/fixtures/rules/std_rule_cases
..sql
/.yml
parser test cases intest/fixtures/dialects
(note YML files can be auto generated withpython test/generate_parse_fixture_yml.py
or by runningtox
locally).test/fixtures/linter/autofix
.Added appropriate documentation for the change.
Created GitHub issues for any relevant followup/future enhancements if appropriate.