iffyio commented on code in PR #2024:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2024#discussion_r2336066226
##########
src/ast/query.rs:
##########
@@ -641,6 +641,56 @@ impl fmt::Display for CteAsMaterialized {
}
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum CteOrCse {
+ Cte(Cte),
+ Cse(Cse),
+}
+
+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),
+ }
+ }
+}
Review Comment:
```suggestion
```
I think we can skip this impl, in order to reduce the API surface of the
library
##########
src/ast/query.rs:
##########
@@ -641,6 +641,56 @@ impl fmt::Display for CteAsMaterialized {
}
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum CteOrCse {
+ Cte(Cte),
+ Cse(Cse),
+}
+
+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),
+ }
+ }
+}
+
+impl fmt::Display for CteOrCse {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ match self {
+ CteOrCse::Cte(cte) => cte.fmt(f),
+ CteOrCse::Cse(cse) => cse.fmt(f),
+ }
+ }
+}
+
+/// A single CSE (used after `WITH`): `<expr> AS <ident>`.
Review Comment:
```suggestion
/// A common scalar expression.
/// Example:
/// ```sql
/// <expr> AS <ident>
/// ```
```
Can we add a link to the clickhouse docs here as well? in case the reader
would like further context from here.
##########
src/parser/mod.rs:
##########
@@ -12260,6 +12260,27 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a CTE or CSE.
+ pub fn parse_cte_or_cse(&mut self) -> Result<CteOrCse, ParserError> {
+ Ok(if dialect_of!(self is ClickHouseDialect) {
+ if let Some(cse) = self.maybe_parse(Parser::parse_cse)? {
+ CteOrCse::Cse(cse)
+ } else {
+ CteOrCse::Cte(self.parse_cte()?)
+ }
+ } else {
+ CteOrCse::Cte(self.parse_cte()?)
+ })
+ }
+
+ /// Parse a CSE (`<expr> AS <ident>`).
+ pub fn parse_cse(&mut self) -> Result<Cse, ParserError> {
+ let expr = self.parse_expr()?;
+ let _after_as = self.parse_keyword(Keyword::AS);
+ let ident = self.parse_identifier()?;
Review Comment:
Yeah I think we'd want to use expect (we can also add a test case for this
e.g. `WITH foo bar SELECT 1`)
##########
src/parser/mod.rs:
##########
@@ -12260,6 +12260,27 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a CTE or CSE.
+ pub fn parse_cte_or_cse(&mut self) -> Result<CteOrCse, ParserError> {
Review Comment:
```suggestion
/// Parse an expression in a `WITH` clause.
fn parse_with_expression(&mut self) -> Result<WithExpression,
ParserError> {
```
##########
src/parser/mod.rs:
##########
@@ -12260,6 +12260,27 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a CTE or CSE.
+ pub fn parse_cte_or_cse(&mut self) -> Result<CteOrCse, ParserError> {
+ Ok(if dialect_of!(self is ClickHouseDialect) {
Review Comment:
```suggestion
Ok(if self.dialect.supports_common_scalar_expressions()) {
```
Can we use a dialect method instead? The `dialect_of` macro we try to avoid
for new code.
##########
tests/sqlparser_clickhouse.rs:
##########
@@ -1729,6 +1729,24 @@ fn test_parse_not_null_in_column_options() {
);
}
+#[test]
+fn parse_cses() {
+ clickhouse().verified_stmt("WITH x AS (SELECT 1) UPDATE t SET bar =
(SELECT * FROM x)");
+
+ let with = concat!(
+ "WITH",
+ " toIntervalSecond(300) AS bucket_size,",
Review Comment:
Can we include a CTE somewhere in this list to demonstrate the parser's
behavior when CTE and CSE are mixed in the same WITH clause?
##########
src/ast/query.rs:
##########
@@ -641,6 +641,56 @@ impl fmt::Display for CteAsMaterialized {
}
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum CteOrCse {
+ Cte(Cte),
+ Cse(Cse),
+}
Review Comment:
```suggestion
pub enum WithExpression {
Cte(Cte),
Cse(Cse),
}
```
##########
src/parser/mod.rs:
##########
@@ -12260,6 +12260,27 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a CTE or CSE.
+ pub fn parse_cte_or_cse(&mut self) -> Result<CteOrCse, ParserError> {
+ Ok(if dialect_of!(self is ClickHouseDialect) {
+ if let Some(cse) = self.maybe_parse(Parser::parse_cse)? {
+ CteOrCse::Cse(cse)
+ } else {
+ CteOrCse::Cte(self.parse_cte()?)
+ }
+ } else {
+ CteOrCse::Cte(self.parse_cte()?)
+ })
+ }
+
+ /// Parse a CSE (`<expr> AS <ident>`).
Review Comment:
```suggestion
/// Parse a [CSE].
```
Since we already mention the syntax in the struct description, to avoid them
going out of sync in the future.
##########
src/parser/mod.rs:
##########
@@ -12260,6 +12260,27 @@ impl<'a> Parser<'a> {
})
}
+ /// Parse a CTE or CSE.
+ pub fn parse_cte_or_cse(&mut self) -> Result<CteOrCse, ParserError> {
+ Ok(if dialect_of!(self is ClickHouseDialect) {
+ if let Some(cse) = self.maybe_parse(Parser::parse_cse)? {
+ CteOrCse::Cse(cse)
+ } else {
+ CteOrCse::Cte(self.parse_cte()?)
+ }
+ } else {
+ CteOrCse::Cte(self.parse_cte()?)
+ })
+ }
+
+ /// Parse a CSE (`<expr> AS <ident>`).
+ pub fn parse_cse(&mut self) -> Result<Cse, ParserError> {
Review Comment:
```suggestion
fn parse_cse(&mut self) -> Result<Cse, ParserError> {
```
--
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]
