ClickHouse: support alter table update ...
#1476
Conversation
src/parser/mod.rs
Outdated
| let target = self.parse_assignment_target()?; | ||
| self.expect_token(&Token::Eq)?; | ||
| let value = self.parse_subexpr(self.dialect.prec_value(Precedence::Between))?; |
There was a problem hiding this comment.
Oh was it something precluding us from calling self.parse_assignment() to do this bit?
Also can we use self.parse_comma_separated() in place of the loop?
There was a problem hiding this comment.
Yep, self.parse_assignment() fails to parse SQLs like ... UPDATE c = 1 IN PARTITION ... due to it's not look ahead enough before parsing expressions. It's the same reason that I can't use parse_comma_separated as well.
Also, I have to mention that there may still have bugs when parsing something like ... UPDATE c = 1 in [1,2,3] IN PARTITION ..., which's valid in clickhouse. The good news is it throws errors instead of paring them into wrong ASTs.
|
|
||
| #[test] | ||
| fn test_parse_alter_table_update() { | ||
| let dialects = all_dialects_where(|d| d.supports_alter_table_update()); |
There was a problem hiding this comment.
Since the change introduces a new node in the AST, one style of test I think could be useful is to assert the returned AST that it matches what we expect. i.e a test in this style for alter_policy
We can take one scenario like ALTER TABLE t UPDATE col1 = 1, col2 = 2 IN PARTITION abc WHERE col3 = 1 and verify that the assignments, partition and selection are where we expect them to be in the node
| @@ -1081,7 +1081,7 @@ impl<'a> Parser<'a> { | |||
| self.parse_bigquery_struct_literal() | |||
| } | |||
| Keyword::PRIOR if matches!(self.state, ParserState::ConnectBy) => { | |||
| let expr = self.parse_subexpr(self.dialect.prec_value(Precedence::PlusMinus))?; | |||
| let expr = self.parse_subexpr(self.dialect.prec_value(Precedence::Pipe))?; | |||
There was a problem hiding this comment.
Oh was there a reason for this change?
| @@ -7156,6 +7156,35 @@ impl<'a> Parser<'a> { | |||
| partition, | |||
| with_name, | |||
| } | |||
| } else if self.dialect.supports_alter_table_update() && self.parse_keyword(Keyword::UPDATE) | |||
| { | |||
| let mut assignments = vec![]; | |||
There was a problem hiding this comment.
One minor request, could we move this new block to its own function to keep the branch of the current function smaller/more-focused?
| loop { | ||
| let target = self.parse_assignment_target()?; | ||
| self.expect_token(&Token::Eq)?; | ||
| // NOTE: Maybe it's better to save the index before parse_subexpr to do a real look |
There was a problem hiding this comment.
Ah so regarding the earlier comment, I think what we could do would be to make self.parse_assignment reusable:
pub fn parse_assignment(&mut self) -> Result<Assignment, ParserError> {
self.parse_assignment_with_prec(self.dialect.prec_unknown())
}
fn parse_assignment_with_precedence(self, precedence: u8) -> Result<Assignment, ParserError> {
let target = self.parse_assignment_target()?;
self.expect_token(&Token::Eq)?;
let value = self.parse_subexpr(precedence)?;
Ok(Assignment { target, value })
}That would make it possible to use parse_comma_separated as well, since the issue seems to be with the IN PARTITION precedence when parsing the assigned expr
|
Setting this to draft since its not waiting for review in the meantime, please feel free to undraft when ready! |
It's to support
ALTER TABLE [<database>.]<table> UPDATE <column> = <expression> WHERE <filter_expr>.