LucaCappelletti94 commented on code in PR #2467:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2467#discussion_r3892507923
##########
tests/sqlparser_clickhouse.rs:
##########
@@ -233,6 +233,26 @@ 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:
These changes currently accept bare `ASSUME a > 0`, and
`AssumeConstraint::name` is `Option<Ident>`, but ClickHouse does not have that
form.
You may use these **RED** tests as a guide:
```suggestion
#[test]
fn parse_create_table_rejects_unnamed_assume_constraint() {
clickhouse()
.parse_sql_statements(
r#"CREATE TABLE "x" ("a" "int", ASSUME "a" > 0) ENGINE =
MergeTree"#,
)
.expect_err("ASSUME constraints require CONSTRAINT and a name");
}
#[test]
fn parse_alter_table_rejects_unnamed_assume_constraint() {
clickhouse()
.parse_sql_statements(r#"ALTER TABLE "x" ADD ASSUME "a" > 0"#)
.expect_err("ASSUME constraints require CONSTRAINT and a name");
}
```
##########
src/ast/table_constraints.rs:
##########
@@ -227,6 +244,38 @@ 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: Option<Ident>,
+ /// The boolean expression the ASSUME constraint enforces.
Review Comment:
ClickHouse's `ASSUME` is an optimizer assertion that is not enforced and may
produce incorrect results when false. Not a constraint, when false it may start
doing 2+2=3. It is just an optimization hint as far as I can recall, not an
enforced rule.
--
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]
