mvzink commented on code in PR #1757:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1757#discussion_r1986222154
##########
src/ast/mod.rs:
##########
@@ -2947,6 +2947,17 @@ pub enum Statement {
variables: OneOrManyWithParens<ObjectName>,
value: Vec<Expr>,
},
+
+ /// ```sql
+ /// SET <variable> = expression [, <variable> = expression]*;
+ /// ```
+ ///
+ /// Note: this is a MySQL-specific statement.
+ /// Refer to [`Dialect.supports_comma_separated_set_assignments`]
+ SetVariables {
+ variables: Vec<ObjectName>,
+ values: Vec<Expr>,
+ },
Review Comment:
It seems @MohamedAbdeen21 was right that this aspect should be a different
PR to have a more comprehensive discussion. I suggested this new variant in
[another
comment](https://github.com/apache/datafusion-sqlparser-rs/pull/1757#discussion_r1984144488)
because I think that this is a different enough syntactic construct from the
other two existing styles (single-variable assignment a la Postgres and
parenthesized multiple-variable assignment a la Snowflake) that adding another
flag to differentiate them would be unclear and cumbersome. Already, the use of
`OneOrManyWithParens` to differentiate the Postgres and Snowflake styles is, in
my opinion, inferior to a higher level enum variant differeniating the two
styles. Furthermore, especially in the case of the new MySQL-style
multiple-assignments-of-single-variables, I don't think that there enough in
common or a way in which dialects would need elements from both, that warrants
having them in the same variant.
However, I think my opinion would be clearer with two changes. First, when I
initially suggested adding a new variant, I was imagining something more
similar to `Case` with its `Vec<CaseWhen>`: i.e. having a
`Vec<VariableAssignment>` or `Vec<(ObjectName, Expr)>` instead of two vecs
which you later have to `zip` together to do anything with (i.e. in the
`Display` implementation). This loses the structure that was parsed and only
makes sense for the Snowflake style. Again, having that syle also be used for
the Postgres single-assignment style is cumbersome and unclear, and this is
similar. I actually didn't notice this difference from my original suggestion
when re-reviewing this PR or I would have raised it.
The second change that might make my thinking clearer is the other one I
suggested in that comment, which is to unify some or all `SET` statements in
another struct/enum. I originally suggested `Statement::Set(SetStatement)` to
include `SetNames` etc., but I actually think
`Statement::SetVariable(SetVariable)` only covering these 3 cases would be
better. Then any common handling between different types of variable
assignments could still be handled centrally, but the 3 distinct styles could
be represented more concretely, similar to this:
```rust
enum SetVariable {
/// SQL Standard-style
/// SET a = 1;
SingleAssignment {
local: bool,
hivevar: bool,
variable: ObjectName,
value: Expr,
}
/// Snowflake-style
/// SET (a, b) = (1, 2);
ParenthesizedAssignments {
variables: Vec<ObjectName>,
values: Vec<Expr>,
}
/// MySQL-style
/// SET a = 1, b = 2;
MultipleAssignments {
assignments: Vec<(ObjectName, Expr)>
}
}
```
Shoe-horning all 3 of these into one struct/variant via a combination of
`OneOrManyWithParens` and a new boolean flag differentiating Snowflake-style
from MySQL-style that only applies when `variables` matches
`OneOrManyWithParens::Many(_))` seems really annoying without, as far as I can
tell, buying us anything.
If y'all think it would be clear enough to do it that way, I won't stand in
the way, but I really think some kind of new variant/structure (if not
precisely this one) would be a huge improvement.
--
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]
