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
support CREATE/DROP STAGE for Snowflake #833
support CREATE/DROP STAGE for Snowflake #833
Conversation
Pull Request Test Coverage Report for Build 4466156302
💛 - Coveralls |
dcf9fd1
to
e1c420a
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.
Looks good to me -- thank you @pawel-big-lebowski
Any concerns @ankrgyl ?
@pawel-big-lebowski I think if you merge up from main the CI will pass (it was fixed in another PR) |
@@ -1524,6 +1525,20 @@ pub enum Statement { | |||
/// Optional parameters. | |||
params: CreateFunctionBody, | |||
}, | |||
/// ```sql | |||
/// CREATE STAGE | |||
/// ``` |
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.
could you add a link to the docs? https://docs.snowflake.com/en/sql-reference/sql/create-stage
src/dialect/snowflake.rs
Outdated
parser.expect_token(&Token::Eq)?; | ||
url = Some(match parser.next_token().token { | ||
Token::SingleQuotedString(word) => Ok(word), | ||
_ => parser.expected("an URL statement", parser.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.
can you say "a URL"?
src/dialect/snowflake.rs
Outdated
}) | ||
} | ||
|
||
/// Parses options provided within parenthesis like: |
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.
should be parentheses (not paranthesis) in the commend and function name below
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.
sure, will change it.
}; | ||
assert_eq!(snowflake().verified_stmt(sql).to_string(), sql); | ||
|
||
let extended_sql = concat!( |
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.
can you add at least one test case that roundtrips CREATE [OR REPLACE] [TEMPORARY] STAGE
(via one_statement_parses_to
)?
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.
I am testing roundtrip with:
assert_eq!(snowflake().verified_stmt(sql).to_string(), sql);
In case it's not sufficient, please provide more details (or example) to help me understand what did you mean. one_statement_parses_to
helps to verify if similar SQLs are semantically the same and I couldn't see how to fit this here.
left a few nit comments! |
Signed-off-by: Pawel Leszczynski <leszczynski.pawel@gmail.com>
e1c420a
to
fe6cac7
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.
Looks good to me -- what do you think @ankrgyl ?
Support snowflake
CREATE STAGE
syntax from here.PR supports whole syntax from Snowflake docs except for two things:
[ [ WITH ] TAG ( <tag_name> = '<tag_value>' [ , <tag_name> = '<tag_value>' , ... ] )
,NULL_IF = ( '<string>' [ , '<string>' ... ] )
.which can be added later on if necessary.
I would like later on to add support for other Snowflake loading/unloading syntaxes like
COPY INTO
.I've created a separate file
src/ast/helpers/stmt_data_loading.rs
to contain newly created structs. These are required as syntax of data loading in Snowflake is pretty complex and I did not want to put them into existingmod.rs
which is already quite massive.