iffyio commented on code in PR #1476:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1476#discussion_r1805072782


##########
tests/sqlparser_common.rs:
##########
@@ -11432,3 +11432,24 @@ fn test_any_some_all_comparison() {
     verified_stmt("SELECT c1 FROM tbl WHERE c1 <> SOME(SELECT c2 FROM tbl)");
     verified_stmt("SELECT 1 = ANY(WITH x AS (SELECT 1) SELECT * FROM x)");
 }
+
+#[test]
+fn test_parse_alter_table_update() {
+    let dialects = all_dialects_where(|d| d.supports_alter_table_update());

Review 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](https://github.com/apache/datafusion-sqlparser-rs/blob/17080f1994689009a319cbb8b08314e7c70c228b/tests/sqlparser_common.rs#L11340-L11358)
   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



##########
src/dialect/clickhouse.rs:
##########
@@ -31,6 +31,10 @@ impl Dialect for ClickHouseDialect {
         self.is_identifier_start(ch) || ch.is_ascii_digit()
     }
 
+    fn supports_alter_table_update(&self) -> bool {

Review Comment:
   Can we include the link to the clickhouse docs here as well?



##########
src/parser/mod.rs:
##########
@@ -7156,6 +7156,33 @@ impl<'a> Parser<'a> {
                 partition,
                 with_name,
             }
+        } else if self.dialect.supports_alter_table_update() && 
self.parse_keyword(Keyword::UPDATE)
+        {
+            let mut assignments = vec![];
+            loop {
+                let target = self.parse_assignment_target()?;
+                self.expect_token(&Token::Eq)?;
+                let value = 
self.parse_subexpr(self.dialect.prec_value(Precedence::Between))?;

Review 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`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to