LucaCappelletti94 commented on code in PR #2467:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2467#discussion_r4091735528
##########
src/parser/mod.rs:
##########
@@ -10180,9 +10180,16 @@ impl<'a> Parser<'a> {
))
}
Token::Word(w) if w.keyword == Keyword::CHECK => {
- self.expect_token(&Token::LParen)?;
+ let has_paren = if
self.dialect.supports_unparenthesized_check_constraint() {
Review Comment:
The unparenthesized `CHECK` path has the same defect. `CONSTRAINT c CHECK (a
+ 1) > 0` fails here but ClickHouse accepts it. Add an appropriate test for it,
and correct this parser.
##########
src/parser/mod.rs:
##########
@@ -10203,6 +10210,32 @@ impl<'a> Parser<'a> {
.into(),
))
}
+ Token::Word(w)
+ if w.keyword == Keyword::ASSUME &&
self.dialect.supports_assume_constraint() =>
+ {
+ let Some(identifier) = name else {
+ return self.expected(
+ "CONSTRAINT <name> before ASSUME",
+ TokenWithSpan {
+ token: Token::make_keyword("ASSUME"),
+ span: next_token.span,
+ },
+ );
+ };
+ let has_paren = self.consume_token(&Token::LParen);
+ let expr = Box::new(self.parse_expr()?);
+ if has_paren {
+ self.expect_token(&Token::RParen)?;
+ }
Review Comment:
You should parse the whole expression and strip one `Nested` layer. A
leading `(` is consumed as the wrapper today, so `CONSTRAINT c ASSUME (a + 1) >
0` and `ASSUME (a) IN (1, 2)` fail with `Expected: ',' or ')'`. ClickHouse
accepts both.
```suggestion
let expr = match self.parse_expr()? {
Expr::Nested(expr) => expr,
expr => Box::new(expr),
};
```
##########
tests/sqlparser_clickhouse.rs:
##########
@@ -233,6 +233,49 @@ fn parse_create_table() {
);
}
+#[test]
+fn parse_table_constraints() {
+ // The parentheses around the expression are optional to parse, but the
+ // constraint always displays with parentheses.
+ clickhouse().one_statement_parses_to(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK "a" > 0) ENGINE =
MergeTree"#,
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK ("a" > 0)) ENGINE
= MergeTree"#,
+ );
+ clickhouse().verified_stmt(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK ("a" > 0)) ENGINE
= MergeTree"#,
+ );
+ clickhouse().one_statement_parses_to(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME "a" > 0) ENGINE
= MergeTree"#,
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME ("a" > 0))
ENGINE = MergeTree"#,
+ );
+ clickhouse().verified_stmt(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME ("a" > 0))
ENGINE = MergeTree"#,
+ );
Review Comment:
You should cover expressions that start with `(`. Both assertions fail on
the current head.
```suggestion
clickhouse().verified_stmt(
r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME ("a" > 0))
ENGINE = MergeTree"#,
);
clickhouse().verified_stmt(
r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK (("a" + 1) >
0)) ENGINE = MergeTree"#,
);
clickhouse().one_statement_parses_to(
r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME ("a" + 1) > 0)
ENGINE = MergeTree"#,
r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME (("a" + 1) >
0)) ENGINE = MergeTree"#,
);
```
##########
tests/sqlparser_clickhouse.rs:
##########
@@ -233,6 +233,49 @@ fn parse_create_table() {
);
}
+#[test]
+fn parse_table_constraints() {
+ // The parentheses around the expression are optional to parse, but the
+ // constraint always displays with parentheses.
+ clickhouse().one_statement_parses_to(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK "a" > 0) ENGINE =
MergeTree"#,
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK ("a" > 0)) ENGINE
= MergeTree"#,
+ );
+ clickhouse().verified_stmt(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" CHECK ("a" > 0)) ENGINE
= MergeTree"#,
+ );
+ clickhouse().one_statement_parses_to(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME "a" > 0) ENGINE
= MergeTree"#,
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME ("a" > 0))
ENGINE = MergeTree"#,
+ );
+ clickhouse().verified_stmt(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT "y" ASSUME ("a" > 0))
ENGINE = MergeTree"#,
+ );
+}
+
+#[test]
+fn parse_create_table_rejects_unnamed_assume_constraint() {
+ clickhouse()
+ .parse_sql_statements(
+ r#"CREATE TABLE "x" ("a" "int", "y" ASSUME "a" > 0) ENGINE =
MergeTree"#,
+ )
+ .expect_err("ASSUME constraints require CONSTRAINT");
+ clickhouse()
+ .parse_sql_statements(
+ r#"CREATE TABLE "x" ("a" "int", CONSTRAINT ASSUME "a" > 0) ENGINE
= MergeTree"#,
+ )
+ .expect_err("ASSUME constraints require name");
+ clickhouse()
+ .parse_sql_statements(r#"CREATE TABLE "x" ("a" "int", ASSUME "a" > 0)
ENGINE = MergeTree"#)
+ .expect_err("ASSUME constraints require CONSTRAINT and a name");
+}
Review Comment:
This test fails on the current head and passes on `main`.
```suggestion
}
#[test]
fn parse_create_table_column_named_assume() {
clickhouse().verified_stmt("CREATE TABLE x (assume Int32) ENGINE =
MergeTree");
}
```
##########
src/ast/table_constraints.rs:
##########
@@ -227,6 +245,36 @@ impl crate::ast::Spanned for CheckConstraint {
}
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+/// An `ASSUME` constraint (`CONSTRAINT <name> ASSUME <expr>`).
+pub struct AssumeConstraint {
+ /// Optional constraint name.
Review Comment:
The name is mandatory, so this doc comment is wrong.
```suggestion
/// Constraint name.
```
##########
src/ast/table_constraints.rs:
##########
@@ -227,6 +245,36 @@ impl crate::ast::Spanned for CheckConstraint {
}
}
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+/// An `ASSUME` constraint (`CONSTRAINT <name> ASSUME <expr>`).
+pub struct AssumeConstraint {
+ /// Optional constraint name.
+ pub name: Ident,
+ /// The boolean expression the ASSUME constraint claims is true.
+ pub expr: Box<Expr>,
+}
+
+impl fmt::Display for AssumeConstraint {
+ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+ use crate::ast::ddl::display_constraint_name;
+ write!(
+ f,
+ "{}ASSUME ({})",
+ display_constraint_name(&Some(self.name.clone())),
+ self.expr
+ )?;
+ Ok(())
+ }
+}
Review Comment:
You could write the name directly.
`display_constraint_name(&Some(self.name.clone()))` allocates a clone on every
render.
```suggestion
impl fmt::Display for AssumeConstraint {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
write!(f, "CONSTRAINT {} ASSUME ({})", self.name, self.expr)
}
}
```
##########
src/parser/mod.rs:
##########
@@ -10203,6 +10210,32 @@ impl<'a> Parser<'a> {
.into(),
))
}
+ Token::Word(w)
+ if w.keyword == Keyword::ASSUME &&
self.dialect.supports_assume_constraint() =>
+ {
+ let Some(identifier) = name else {
+ return self.expected(
+ "CONSTRAINT <name> before ASSUME",
+ TokenWithSpan {
+ token: Token::make_keyword("ASSUME"),
+ span: next_token.span,
+ },
+ );
+ };
Review Comment:
You should fall back to column parsing here instead of erroring. `ASSUME` is
not reserved in ClickHouse, and this arm now rejects `CREATE TABLE x (assume
Int32) ENGINE = MergeTree`, which parses on `main` and in ClickHouse 26.9. The
unnamed-`ASSUME` tests still fail as they should.
```suggestion
let Some(identifier) = name else {
self.prev_token();
return Ok(None);
};
```
--
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]
