fmguerreiro commented on code in PR #2307:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/2307#discussion_r3223389383
##########
src/parser/mod.rs:
##########
@@ -9926,6 +9952,71 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_exclusion_element(&mut self) -> Result<ExclusionElement,
ParserError> {
Review Comment:
renamed to `parse_exclude_constraint_element` in 60b5fd2.
##########
src/parser/mod.rs:
##########
@@ -9926,6 +9952,71 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_exclusion_element(&mut self) -> Result<ExclusionElement,
ParserError> {
+ // `index_elem` grammar: { col | (expr) } [ opclass ] [ ASC | DESC ] [
NULLS FIRST | LAST ].
+ // Shared with `CREATE INDEX` columns.
Review Comment:
removed in 4e5e49d.
##########
src/parser/mod.rs:
##########
@@ -9926,6 +9952,71 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_exclusion_element(&mut self) -> Result<ExclusionElement,
ParserError> {
+ // `index_elem` grammar: { col | (expr) } [ opclass ] [ ASC | DESC ] [
NULLS FIRST | LAST ].
+ // Shared with `CREATE INDEX` columns.
+ let (
+ OrderByExpr {
+ expr,
+ options: order,
+ ..
+ },
+ operator_class,
Review Comment:
Refactored in 60b5fd2: the destructuring is gone.
`parse_exclude_constraint_element` now calls `parse_create_index_expr()` which
returns an `IndexColumn` directly, so the field is stored as-is rather than
being unpacked from `OrderByExpr`.
##########
src/parser/mod.rs:
##########
@@ -9926,6 +9952,71 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_exclusion_element(&mut self) -> Result<ExclusionElement,
ParserError> {
+ // `index_elem` grammar: { col | (expr) } [ opclass ] [ ASC | DESC ] [
NULLS FIRST | LAST ].
+ // Shared with `CREATE INDEX` columns.
+ let (
+ OrderByExpr {
+ expr,
+ options: order,
+ ..
+ },
+ operator_class,
+ ) = self.parse_order_by_expr_inner(true)?;
+
+ self.expect_keyword_is(Keyword::WITH)?;
+ let operator = self.parse_exclusion_operator()?;
+
+ Ok(ExclusionElement {
+ expr,
+ operator_class,
+ order,
+ operator,
+ })
+ }
+
+ /// Parse the operator that follows `WITH` in an `EXCLUDE` element.
+ ///
+ /// Accepts either a single operator token (e.g. `=`, `&&`, `<->`) or the
+ /// Postgres `OPERATOR(schema.op)` form for schema-qualified operators.
Review Comment:
No longer applicable: the destructuring pattern was replaced entirely in
60b5fd2. The function now calls `parse_create_index_expr()` and stores the
returned `IndexColumn` directly.
##########
src/parser/mod.rs:
##########
@@ -9926,6 +9952,71 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_exclusion_element(&mut self) -> Result<ExclusionElement,
ParserError> {
+ // `index_elem` grammar: { col | (expr) } [ opclass ] [ ASC | DESC ] [
NULLS FIRST | LAST ].
+ // Shared with `CREATE INDEX` columns.
+ let (
+ OrderByExpr {
+ expr,
+ options: order,
+ ..
+ },
+ operator_class,
+ ) = self.parse_order_by_expr_inner(true)?;
+
+ self.expect_keyword_is(Keyword::WITH)?;
+ let operator = self.parse_exclusion_operator()?;
+
+ Ok(ExclusionElement {
+ expr,
+ operator_class,
+ order,
+ operator,
+ })
+ }
+
+ /// Parse the operator that follows `WITH` in an `EXCLUDE` element.
+ ///
+ /// Accepts either a single operator token (e.g. `=`, `&&`, `<->`) or the
+ /// Postgres `OPERATOR(schema.op)` form for schema-qualified operators.
+ fn parse_exclusion_operator(&mut self) -> Result<ExclusionOperator,
ParserError> {
Review Comment:
renamed to `parse_exclude_constraint_operator` in 60b5fd2.
##########
src/parser/mod.rs:
##########
@@ -9926,6 +9952,71 @@ impl<'a> Parser<'a> {
}
}
+ fn parse_exclusion_element(&mut self) -> Result<ExclusionElement,
ParserError> {
+ // `index_elem` grammar: { col | (expr) } [ opclass ] [ ASC | DESC ] [
NULLS FIRST | LAST ].
+ // Shared with `CREATE INDEX` columns.
+ let (
+ OrderByExpr {
+ expr,
+ options: order,
+ ..
+ },
+ operator_class,
+ ) = self.parse_order_by_expr_inner(true)?;
+
+ self.expect_keyword_is(Keyword::WITH)?;
+ let operator = self.parse_exclusion_operator()?;
+
+ Ok(ExclusionElement {
+ expr,
+ operator_class,
+ order,
+ operator,
+ })
+ }
+
+ /// Parse the operator that follows `WITH` in an `EXCLUDE` element.
+ ///
+ /// Accepts either a single operator token (e.g. `=`, `&&`, `<->`) or the
+ /// Postgres `OPERATOR(schema.op)` form for schema-qualified operators.
+ fn parse_exclusion_operator(&mut self) -> Result<ExclusionOperator,
ParserError> {
+ if self.parse_keyword(Keyword::OPERATOR) {
+ return Ok(ExclusionOperator::PgCustom(
+ self.parse_pg_operator_ident_parts()?,
+ ));
+ }
+
+ let operator_token = self.next_token();
+ match &operator_token.token {
+ Token::EOF | Token::RParen | Token::Comma | Token::SemiColon => {
Review Comment:
`RParen` is in the rejection list to catch the missing-operator case, not
because it is an optional valid operator. After `WITH`, the parser consumes one
token as the operator. If that token is `)`, `,`, `;`, or `EOF`, the user wrote
something like `col WITH)` or `col WITH,` — a structural delimiter where an
operator should appear. Treating those tokens as operator strings would produce
a silently wrong AST, so the function errors immediately with "expected
exclusion operator". The `)` that closes the element list will be consumed by
the caller (`parse_exclude_constraint`) via `expect_token(&Token::RParen)`
after `parse_comma_separated` returns.
##########
tests/sqlparser_postgres.rs:
##########
@@ -9193,3 +9243,310 @@ fn parse_lock_table() {
}
}
}
+
+#[test]
+fn parse_exclude_constraint_with_where() {
+ let sql = "CREATE TABLE t (col INT, EXCLUDE USING gist (col WITH =) WHERE
(col > 0))";
+ match pg().verified_stmt(sql) {
+ Statement::CreateTable(create_table) => {
+ assert_eq!(1, create_table.constraints.len());
+ match &create_table.constraints[0] {
+ TableConstraint::Exclusion(c) => {
+ assert!(c.where_clause.is_some());
+ match c.where_clause.as_ref().unwrap().as_ref() {
+ Expr::BinaryOp { left, op, right } => {
+ assert_eq!(**left,
Expr::Identifier(Ident::new("col")));
+ assert_eq!(*op, BinaryOperator::Gt);
+ assert_eq!(**right,
Expr::Value(number("0").with_empty_span()));
+ }
+ other => panic!("Expected BinaryOp, got {other:?}"),
+ }
+ }
+ other => panic!("Expected Exclusion, got {other:?}"),
+ }
+ }
+ _ => panic!("Expected CreateTable"),
+ }
+}
+
+#[test]
+fn parse_exclude_constraint_with_include() {
+ let sql = "CREATE TABLE t (col INT, EXCLUDE USING gist (col WITH =)
INCLUDE (col))";
+ match pg().verified_stmt(sql) {
+ Statement::CreateTable(create_table) => {
+ assert_eq!(1, create_table.constraints.len());
+ match &create_table.constraints[0] {
+ TableConstraint::Exclusion(c) => {
+ assert_eq!(c.elements.len(), 1);
+ assert_eq!(c.include, vec![Ident::new("col")]);
+ assert!(c.where_clause.is_none());
+ assert!(c.characteristics.is_none());
+ }
+ other => panic!("Expected Exclusion, got {other:?}"),
+ }
+ }
+ _ => panic!("Expected CreateTable"),
+ }
+}
+
+#[test]
+fn parse_exclude_constraint_no_using() {
+ let sql = "CREATE TABLE t (col INT, EXCLUDE (col WITH =))";
+ match pg().verified_stmt(sql) {
+ Statement::CreateTable(create_table) => {
+ assert_eq!(1, create_table.constraints.len());
+ match &create_table.constraints[0] {
+ TableConstraint::Exclusion(c) => {
+ assert!(c.index_method.is_none());
+ }
+ other => panic!("Expected Exclusion, got {other:?}"),
+ }
+ }
+ _ => panic!("Expected CreateTable"),
+ }
+}
+
+#[test]
+fn parse_exclude_constraint_deferrable() {
+ let sql =
+ "CREATE TABLE t (col INT, EXCLUDE USING gist (col WITH =) DEFERRABLE
INITIALLY DEFERRED)";
+ match pg().verified_stmt(sql) {
+ Statement::CreateTable(create_table) => {
+ assert_eq!(1, create_table.constraints.len());
+ match &create_table.constraints[0] {
+ TableConstraint::Exclusion(c) => {
+ let characteristics = c.characteristics.as_ref().unwrap();
+ assert_eq!(characteristics.deferrable, Some(true));
+ assert_eq!(characteristics.initially,
Some(DeferrableInitial::Deferred));
+ }
+ other => panic!("Expected Exclusion, got {other:?}"),
+ }
+ }
+ _ => panic!("Expected CreateTable"),
+ }
+}
+
+#[test]
+fn parse_exclude_constraint_in_alter_table() {
+ let sql = "ALTER TABLE t ADD CONSTRAINT no_overlap EXCLUDE USING gist
(room WITH =)";
+ match pg().verified_stmt(sql) {
+ Statement::AlterTable(alter_table) => match &alter_table.operations[0]
{
+ AlterTableOperation::AddConstraint {
+ constraint: TableConstraint::Exclusion(c),
+ ..
+ } => {
+ assert_eq!(c.name, Some(Ident::new("no_overlap")));
+ assert_eq!(c.elements[0].operator.to_string(), "=");
+ }
+ other => panic!("Expected AddConstraint(Exclusion), got
{other:?}"),
+ },
+ _ => panic!("Expected AlterTable"),
+ }
+}
+
+#[test]
+fn roundtrip_exclude_constraint() {
+ let sql = "CREATE TABLE t (CONSTRAINT no_overlap EXCLUDE USING gist (room
WITH =, during WITH &&) INCLUDE (id) WHERE (active = true))";
Review Comment:
Done in 60b5fd2: all tests were merged into a single
`parse_exclude_constraint` function in `tests/sqlparser_common.rs`. The one
AST-asserting case is kept; everything else is covered by `verified_stmt`
round-trips.
--
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]
