Add support for ClickHouse CSE. #2024
Conversation
Common scalar expressions: `WITH <expr> AS <ident>`.
Due to incompatible changes in `With`.
src/ast/query.rs
Outdated
| impl CteOrCse { | ||
| pub fn cte(&self) -> Option<&Cte> { | ||
| match self { | ||
| CteOrCse::Cte(cte) => Some(cte), | ||
| CteOrCse::Cse(_) => None, | ||
| } | ||
| } | ||
|
|
||
| pub fn cse(&self) -> Option<&Cse> { | ||
| match self { | ||
| CteOrCse::Cte(_) => None, | ||
| CteOrCse::Cse(cse) => Some(cse), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| impl CteOrCse { | |
| pub fn cte(&self) -> Option<&Cte> { | |
| match self { | |
| CteOrCse::Cte(cte) => Some(cte), | |
| CteOrCse::Cse(_) => None, | |
| } | |
| } | |
| pub fn cse(&self) -> Option<&Cse> { | |
| match self { | |
| CteOrCse::Cte(_) => None, | |
| CteOrCse::Cse(cse) => Some(cse), | |
| } | |
| } | |
| } |
I think we can skip this impl, in order to reduce the API surface of the library
There was a problem hiding this comment.
But then we'd need way more changes in the existing code that relies on https://docs.rs/sqlparser/latest/sqlparser/ast/struct.With.html#structfield.cte_tables being Cte. I am on the edge here.
There was a problem hiding this comment.
I think this is okay to skip, users can match directly on the enum when needed similar to other nodes in the AST
as dialect feature detection.
Otherwise, the compiler complains about lifetimes.
Only for ClickHouse for now.
Only for ClickHouse for now.
| fn supports_common_scalar_expressions(&self) -> bool { | ||
| false | ||
| } |
There was a problem hiding this comment.
Should we also add this to the generic dialect? I could not grasp the idea of what expressions should be supported in the generic dialect and what should be not.
For example, supports_select_expr_star isn't listed as supported. Neither is supports_select_exclude.
There was a problem hiding this comment.
In general we try to add support for new syntax to the generic dialect as long as the syntax doesn't conflict with existing syntax support on the dialect
There was a problem hiding this comment.
I think, CSE exist only in ClickHouse. Does it make sense to include this to the generic dialect? If yes, then:
as long as the syntax doesn't conflict with existing syntax support on the dialect
It's difficult for me to answer to this question - I don't know whether there can be a conflict. But since it's only a ClickHouse feature - I'd not include this to the generic dialect. But I won't oppose either.
There was a problem hiding this comment.
Ah yeah if there's a conflict that would show up in the tests, so that it should be a matter of flagging it to true in the dialect and then changing the tests to use all_dialects_where(|d| di.supports_common_scalar_expressions()) to test instead of clickhouse()
There was a problem hiding this comment.
@pravic just double checking the status of this PR, if you would have time to address this comment?
There was a problem hiding this comment.
@iffyio No, haven't touched this bit so far. I you can, I would appreciate changing this.
| // ClickHouse allows these, but not sqlparser | ||
| clickhouse() | ||
| .parse_sql_statements("WITH foo, bar SELECT 1") | ||
| .expect_err("Expected: AS, found: ,"); | ||
|
|
||
| clickhouse() | ||
| .parse_sql_statements("WITH foo(), bar SELECT 1") | ||
| .expect_err("Expected: identifier, found: )"); |
There was a problem hiding this comment.
Interesting: https://fiddle.clickhouse.com/1292850a-31e4-49c7-a309-79bc47a273a2
I am not sure whether it's a feature or a bug in the ClickHouse syntax parser. But since these examples don't make much sense, let's leave them as rejected.
https://clickhouse.com/docs/sql-reference/statements/select/with#common-scalar-expressions:
fixes #1514.