-
Notifications
You must be signed in to change notification settings - Fork 628
Snowflake create database #1939
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
Snowflake create database #1939
Conversation
@iffyio please take a look |
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 @osipovartem! Left a minor comment otherwise the changes look good to me
src/parser/mod.rs
Outdated
/// Parse a boolean string | ||
pub fn parse_boolean_string(&mut self) -> Result<bool, ParserError> { | ||
match self.parse_one_of_keywords(&[Keyword::TRUE, Keyword::FALSE]) { | ||
Some(Keyword::TRUE) => Ok(true), | ||
Some(Keyword::FALSE) => Ok(false), | ||
_ => self.expected("TRUE or FALSE", self.peek_token()), | ||
} | ||
} |
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.
oh this looks like its only used once, can we inline it instead?
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.
This will be used in the next PR
related to #1982
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.
Ah I see, but I think we can inline it in this PR and if it ends up being used in the next PR we can break it out into a function. Also heads up for the function we would want to make it either private or pub(crate)
visibility wise instead of pub
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.
Replaced duplicated code by parse_boolean_string call
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.
Please review again
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! Thanks @osipovartem!
cc @alamb
https://docs.snowflake.com/en/sql-reference/sql/create-database
Added support for
Closes #1938