LucaCappelletti94 commented on code in PR #2617:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2617#discussion_r4257285477
##########
src/ast/spans.rs:
##########
Review Comment:
You should give `Setting` its own `Spanned` impl, as `Assignment` has, so
its span is defined once and not spelled out in every statement that carries
`SETTINGS`.
```suggestion
}
impl Spanned for Setting {
fn span(&self) -> Span {
let Setting { key, value } = self;
key.span.union(&value.span())
}
}
```
##########
tests/sqlparser_clickhouse.rs:
##########
@@ -2203,6 +2203,127 @@ fn global_in_bare_table_binds_before_equality() {
));
}
+#[test]
+fn parse_delete_on_cluster_and_settings() {
+ clickhouse_and_generic().verified_stmt("DELETE FROM db.hits ON CLUSTER c1
WHERE x = 1");
+ clickhouse_and_generic().verified_stmt("DELETE FROM hits ON CLUSTER
'{cluster}' WHERE x = 1");
+ clickhouse_and_generic()
+ .verified_stmt("DELETE FROM hits WHERE x = 1 SETTINGS
lightweight_deletes_sync = 0");
+
+ match clickhouse_and_generic()
+ .verified_stmt("DELETE FROM hits ON CLUSTER c1 WHERE x = 1 SETTINGS a
= 1, b = 'x'")
+ {
+ Statement::Delete(Delete {
+ on_cluster,
+ settings,
+ ..
+ }) => {
+ assert_eq!(on_cluster, Some(Ident::new("c1")));
+ assert_eq!(
+ settings,
+ Some(vec![
+ Setting {
+ key: Ident::new("a"),
+ value: Expr::value(number("1")),
+ },
+ Setting {
+ key: Ident::new("b"),
+ value:
Expr::value(Value::SingleQuotedString("x".into())),
+ },
+ ])
+ );
+ }
+ _ => unreachable!(),
+ }
+
+ for (sql, err) in [
+ (
+ "DELETE FROM hits SETTINGS a = 1 WHERE x = 1",
+ "Expected: end of statement, found: WHERE",
+ ),
+ (
+ "DELETE FROM hits WHERE x = 1 ON CLUSTER c1",
+ "Expected: end of statement, found: ON",
+ ),
+ (
+ "DELETE FROM hits ON CLUSTER 1 WHERE x = 1",
+ "Expected: identifier, found: 1",
+ ),
+ (
+ "DELETE FROM hits WHERE x = 1 SETTINGS a",
+ "Expected: =, found: EOF",
+ ),
+ ] {
+ assert_eq!(
+ clickhouse_and_generic()
+ .parse_sql_statements(sql)
+ .unwrap_err(),
+ ParserError(err.to_string())
+ );
+ }
+ assert!(all_dialects_where(|d| !d.supports_settings())
+ .parse_sql_statements("DELETE FROM hits WHERE x = 1 SETTINGS a = 1")
+ .is_err());
+}
Review Comment:
You should assert the expected error, as the cases above do. `is_err()` also
passes when the statement fails before `SETTINGS` is reached.
```suggestion
assert_eq!(
all_dialects_where(|d| !d.supports_settings())
.parse_sql_statements("DELETE FROM hits WHERE x = 1 SETTINGS a =
1")
.unwrap_err(),
ParserError("Expected: end of statement, found:
SETTINGS".to_string())
);
```
##########
tests/sqlparser_clickhouse.rs:
##########
@@ -2203,6 +2203,127 @@ fn global_in_bare_table_binds_before_equality() {
));
}
+#[test]
+fn parse_delete_on_cluster_and_settings() {
+ clickhouse_and_generic().verified_stmt("DELETE FROM db.hits ON CLUSTER c1
WHERE x = 1");
+ clickhouse_and_generic().verified_stmt("DELETE FROM hits ON CLUSTER
'{cluster}' WHERE x = 1");
+ clickhouse_and_generic()
+ .verified_stmt("DELETE FROM hits WHERE x = 1 SETTINGS
lightweight_deletes_sync = 0");
+
+ match clickhouse_and_generic()
+ .verified_stmt("DELETE FROM hits ON CLUSTER c1 WHERE x = 1 SETTINGS a
= 1, b = 'x'")
+ {
+ Statement::Delete(Delete {
+ on_cluster,
+ settings,
+ ..
+ }) => {
+ assert_eq!(on_cluster, Some(Ident::new("c1")));
+ assert_eq!(
+ settings,
+ Some(vec![
+ Setting {
+ key: Ident::new("a"),
+ value: Expr::value(number("1")),
+ },
+ Setting {
+ key: Ident::new("b"),
+ value:
Expr::value(Value::SingleQuotedString("x".into())),
+ },
+ ])
+ );
+ }
+ _ => unreachable!(),
+ }
+
+ for (sql, err) in [
+ (
+ "DELETE FROM hits SETTINGS a = 1 WHERE x = 1",
+ "Expected: end of statement, found: WHERE",
+ ),
+ (
+ "DELETE FROM hits WHERE x = 1 ON CLUSTER c1",
+ "Expected: end of statement, found: ON",
+ ),
+ (
+ "DELETE FROM hits ON CLUSTER 1 WHERE x = 1",
+ "Expected: identifier, found: 1",
+ ),
+ (
+ "DELETE FROM hits WHERE x = 1 SETTINGS a",
+ "Expected: =, found: EOF",
+ ),
+ ] {
+ assert_eq!(
+ clickhouse_and_generic()
+ .parse_sql_statements(sql)
+ .unwrap_err(),
+ ParserError(err.to_string())
+ );
+ }
+ assert!(all_dialects_where(|d| !d.supports_settings())
+ .parse_sql_statements("DELETE FROM hits WHERE x = 1 SETTINGS a = 1")
+ .is_err());
+}
+
+#[test]
+fn parse_update_on_cluster_and_settings() {
+ clickhouse_and_generic().verified_stmt("UPDATE db.hits ON CLUSTER c1 SET x
= 1 WHERE y = 2");
+ clickhouse_and_generic()
+ .verified_stmt("UPDATE hits ON CLUSTER '{cluster}' SET x = 1, z = 2
WHERE y = 2");
+ clickhouse_and_generic()
+ .verified_stmt("UPDATE hits SET x = 1 WHERE y = 2 SETTINGS
update_parallel_mode = 'auto'");
+
+ match clickhouse_and_generic()
+ .verified_stmt("UPDATE hits ON CLUSTER c1 SET x = 1 WHERE y = 2
SETTINGS a = 1")
+ {
+ Statement::Update(Update {
+ on_cluster,
+ settings,
+ ..
+ }) => {
+ assert_eq!(on_cluster, Some(Ident::new("c1")));
+ assert_eq!(
+ settings,
+ Some(vec![Setting {
+ key: Ident::new("a"),
+ value: Expr::value(number("1")),
+ }])
+ );
+ }
+ _ => unreachable!(),
+ }
+
+ for (sql, err) in [
+ (
+ "UPDATE hits SET x = 1 ON CLUSTER c1 WHERE y = 2",
+ "Expected: end of statement, found: ON",
+ ),
+ (
+ "UPDATE hits SET x = 1 SETTINGS a = 1 WHERE y = 2",
+ "Expected: end of statement, found: WHERE",
+ ),
+ (
+ "UPDATE hits ON CLUSTER 1 SET x = 1 WHERE y = 2",
+ "Expected: identifier, found: 1",
+ ),
+ (
+ "UPDATE hits SET x = 1 WHERE y = 2 SETTINGS a",
+ "Expected: =, found: EOF",
+ ),
+ ] {
+ assert_eq!(
+ clickhouse_and_generic()
+ .parse_sql_statements(sql)
+ .unwrap_err(),
+ ParserError(err.to_string())
+ );
+ }
+ assert!(all_dialects_where(|d| !d.supports_settings())
+ .parse_sql_statements("UPDATE hits SET x = 1 WHERE y = 2 SETTINGS a =
1")
+ .is_err());
Review Comment:
You should assert the expected error, as the cases above do. `is_err()` also
passes when the statement fails before `SETTINGS` is reached.
```suggestion
assert_eq!(
all_dialects_where(|d| !d.supports_settings())
.parse_sql_statements("UPDATE hits SET x = 1 WHERE y = 2
SETTINGS a = 1")
.unwrap_err(),
ParserError("Expected: end of statement, found:
SETTINGS".to_string())
);
```
--
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]
