add set time zone sometimezone as a exception while parsing keyword::set #727
Conversation
|
i'd like to make sure this implement make sense first, then i'll add some test cases later |
Pull Request Test Coverage Report for Build 3568533098
💛 - Coveralls |
src/parser.rs
Outdated
| } else if (self.consume_token(&Token::Eq) || self.parse_keyword(Keyword::TO)) | ||
| || (variable == ObjectName(vec!["TIMEZONE".into()])) | ||
| { | ||
| // when the object name is TIMEZONE, we support `SET TIMEZONE 'UTC'` without Eq sign or TO |
There was a problem hiding this comment.
SET TIMEZONE 'UTC' is an exceptional case in posetgresql
not sure whether it's a good idea to add it here
src/parser.rs
Outdated
| @@ -4553,7 +4553,11 @@ impl<'a> Parser<'a> { | |||
| charset_name, | |||
| collation_name, | |||
| }) | |||
| } else if self.consume_token(&Token::Eq) || self.parse_keyword(Keyword::TO) { | |||
| } else if self.consume_token(&Token::Eq) | |||
There was a problem hiding this comment.
Having tests would make it easier, but, tbh, I guess the SET TIME ZONE could be an individual command, no?
It is a singular command in many dialects. Having this manual conversion seems weird to me, considering that we can't have SET TIME ZONE = ... (I'm basing myself on PostgreSQLs Docs)
There was a problem hiding this comment.
@AugustoFKL @alamb @step-baby
thank you for the comments and feedbacks
here's the update:
- add a new statement
Statement::SetTimeZone SET <variable> [TO|=] <value>is parsed asStatement::SetVariableSET TIME ZONE [TO|=] <value>is parsed asStatement::SetVariableSET TIME ZONE <value>is parsed asStatement::SetTimeZoneSET <non-time-zone-variable> <value>doesn't work
| /// SET TIME ZONE <value> | ||
| /// | ||
| /// Note: this is a PostgreSQL-specific statements | ||
| /// SET TIME ZONE <value> is an alias for SET timezone TO <value> in PostgreSQL | ||
| SetTimeZone { local: bool, value: Expr }, |
There was a problem hiding this comment.
add a new Statement SetTimeZone to reflect the difference between SET TIME ZONE TO 'UTC' and SET TIME ZONE 'UTC'
tests/sqlparser_common.rs
Outdated
| #[test] | ||
| fn parse_set_time_zone_alias() { | ||
| println!("wew"); | ||
| match verified_stmt("SET TIME ZONE 'UTC'") { | ||
| Statement::SetTimeZone { local, value } => { | ||
| assert!(!local); | ||
| println!("wew"); | ||
| assert_eq!(value, Expr::Value(Value::SingleQuotedString("UTC".into()))); | ||
| } | ||
| _ => unreachable!(), | ||
| } | ||
| } |
There was a problem hiding this comment.
add a test case for set time zone 'utc'
| } else if variable.to_string().eq_ignore_ascii_case("TIMEZONE") { | ||
| // for some db (e.g. postgresql), SET TIME ZONE <value> is an alias for SET TIMEZONE [TO|=] <value> | ||
| match self.parse_expr() { | ||
| Ok(expr) => Ok(Statement::SetTimeZone { | ||
| local: modifier == Some(Keyword::LOCAL), | ||
| value: expr, | ||
| }), | ||
| _ => self.expected("timezone value", self.peek_token())?, | ||
| } |
There was a problem hiding this comment.
this captures the SET TIME ZONE <value> case
There was a problem hiding this comment.
Other than the println looks good to me -- thank you @waitingkuo
tests/sqlparser_common.rs
Outdated
| match verified_stmt("SET TIME ZONE 'UTC'") { | ||
| Statement::SetTimeZone { local, value } => { | ||
| assert!(!local); | ||
| println!("wew"); |
There was a problem hiding this comment.
is the println meant to be left in?
There was a problem hiding this comment.
updated, thank you @alamb
closes #303