Implement SnowFlake ALTER SESSION #1712
Conversation
There was a problem hiding this comment.
Thanks @osipovartem -- I started the CI checks on this PR (looks like there are some failures)
@yoavcloud since you are more familiar with Snowflake syntax, would you be able to review this PR as well?
src/ast/mod.rs
Outdated
| /// true is to set for the session parameters, false is to unset | ||
| set: bool, | ||
| /// The session parameters to set or unset | ||
| session_params: DataLoadingOptions, |
There was a problem hiding this comment.
DataLoading options come from here
There was a problem hiding this comment.
DataLoadingOptions is a very useful struct IMO, I would rename it to KeyValueOptions and promote it to its own helper file.
tests/sqlparser_snowflake.rs
Outdated
|
|
||
| #[test] | ||
| fn test_alter_session() { | ||
| snowflake().verified_stmt("ALTER SESSION SET"); |
There was a problem hiding this comment.
This is not a valid Snowflake statement, Snowflake doesn't allow an empty list of params.
tests/sqlparser_snowflake.rs
Outdated
| #[test] | ||
| fn test_alter_session() { | ||
| snowflake().verified_stmt("ALTER SESSION SET"); | ||
| snowflake().verified_stmt("ALTER SESSION UNSET"); |
There was a problem hiding this comment.
This is not a valid Snowflake statement, Snowflake doesn't allow an empty list of params.
tests/sqlparser_snowflake.rs
Outdated
| snowflake().verified_stmt("ALTER SESSION SET AUTOCOMMIT=TRUE"); | ||
| snowflake().verified_stmt("ALTER SESSION SET AUTOCOMMIT=FALSE QUERY_TAG='tag'"); | ||
| snowflake().verified_stmt("ALTER SESSION UNSET AUTOCOMMIT"); | ||
| snowflake().verified_stmt("ALTER SESSION UNSET AUTOCOMMIT QUERY_TAG"); |
There was a problem hiding this comment.
Snowflake requires a comma for unsetting multiple params
src/dialect/snowflake.rs
Outdated
| @@ -120,6 +120,12 @@ impl Dialect for SnowflakeDialect { | |||
| } | |||
|
|
|||
| fn parse_statement(&self, parser: &mut Parser) -> Option<Result<Statement, ParserError>> { | |||
| if parser.parse_keywords(&[Keyword::ALTER, Keyword::SESSION]) { | |||
| // ALTER SESSION | |||
| let set = parser.parse_keyword(Keyword::SET) | !parser.parse_keyword(Keyword::UNSET); | |||
There was a problem hiding this comment.
This would accept any statements that are not accepted by Snowflake such as ALTER SESSION XYZ. Suggest to rewrite as:
let set = match parser.parse_one_of_keywords(&[Keyword::SET, Keyword::UNSET]) {
Some(Keyword::SET) => true,
Some(Keyword::UNSET) => false,
_ => return Some(parser.expected("SET or UNSET", parser.peek_token()))
};
src/dialect/snowflake.rs
Outdated
| }, | ||
| } | ||
| } | ||
| Ok(options) |
There was a problem hiding this comment.
Perhaps add a check here for empty options and raise error
Sounds good -- @osipovartem -- what do you think? |
|
@osipovartem could you take a look into the CI failure? |
|
Marking as draft as we sort out the CI failure |
Closes #1710
https://docs.snowflake.com/en/sql-reference/sql/alter-session