Skip to content

Commit 90803e0

Browse files
committed
refactor: reuse parse_order_by_expr_inner and tighten exclude tests
Follow-up to the review feedback on the EXCLUDE constraint changes: - Replace the hand-rolled `{ expr [opclass] [ASC|DESC] [NULLS ...] }` lookahead inside `parse_exclusion_element` with a direct call to `parse_order_by_expr_inner(true)` so the `index_elem` grammar lives in a single place. `WITH FILL` is gated on a separate dialect capability, so EXCLUDE (PG-only) cannot accidentally consume it. - Add structural assertions to `parse_exclude_constraint_desc_nulls_first` to mirror the ascending-order test instead of relying on the round-trip alone. - Assert that `exclude` survives as a column name in MySQL/SQLite by checking the parsed AST rather than `is_ok()`. - Tighten `exclude_empty_element_list_errors` and strengthen the operator-class and function-expression tests with explicit `expr` assertions for completeness. - Document why `GenericDialect` is intentionally excluded from the rejection sweep (it opts into PG-style EXCLUDE).
1 parent 2d37749 commit 90803e0

2 files changed

Lines changed: 49 additions & 21 deletions

File tree

src/parser/mod.rs

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9973,18 +9973,16 @@ impl<'a> Parser<'a> {
99739973
}
99749974

99759975
fn parse_exclusion_element(&mut self) -> Result<ExclusionElement, ParserError> {
9976-
let expr = self.parse_expr()?;
9977-
9978-
// `index_elem` grammar: [ opclass ] [ ASC | DESC ] [ NULLS FIRST | LAST ]
9979-
let operator_class: Option<ObjectName> = if self
9980-
.peek_one_of_keywords(&[Keyword::ASC, Keyword::DESC, Keyword::NULLS, Keyword::WITH])
9981-
.is_some()
9982-
{
9983-
None
9984-
} else {
9985-
self.maybe_parse(|p| p.parse_object_name(false))?
9986-
};
9987-
let order = self.parse_order_by_options()?;
9976+
// `index_elem` grammar: { col | (expr) } [ opclass ] [ ASC | DESC ] [ NULLS FIRST | LAST ].
9977+
// Shared with `CREATE INDEX` columns.
9978+
let (
9979+
OrderByExpr {
9980+
expr,
9981+
options: order,
9982+
..
9983+
},
9984+
operator_class,
9985+
) = self.parse_order_by_expr_inner(true)?;
99889986

99899987
self.expect_keyword_is(Keyword::WITH)?;
99909988
let operator = self.parse_exclusion_operator()?;

tests/sqlparser_postgres.rs

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9376,6 +9376,7 @@ fn parse_exclude_constraint_operator_class() {
93769376
Statement::CreateTable(create_table) => match &create_table.constraints[0] {
93779377
TableConstraint::Exclusion(c) => {
93789378
assert_eq!(c.elements.len(), 1);
9379+
assert_eq!(c.elements[0].expr, Expr::Identifier(Ident::new("col")));
93799380
assert_eq!(
93809381
c.elements[0].operator_class,
93819382
Some(ObjectName::from(vec![Ident::new("text_pattern_ops")]))
@@ -9405,9 +9406,17 @@ fn parse_exclude_constraint_asc_nulls_last() {
94059406

94069407
#[test]
94079408
fn parse_exclude_constraint_desc_nulls_first() {
9408-
pg().verified_stmt(
9409-
"CREATE TABLE t (col INT, EXCLUDE USING btree (col DESC NULLS FIRST WITH =))",
9410-
);
9409+
let sql = "CREATE TABLE t (col INT, EXCLUDE USING btree (col DESC NULLS FIRST WITH =))";
9410+
match pg().verified_stmt(sql) {
9411+
Statement::CreateTable(create_table) => match &create_table.constraints[0] {
9412+
TableConstraint::Exclusion(c) => {
9413+
assert_eq!(c.elements[0].order.asc, Some(false));
9414+
assert_eq!(c.elements[0].order.nulls_first, Some(true));
9415+
}
9416+
other => panic!("Expected Exclusion, got {other:?}"),
9417+
},
9418+
_ => panic!("Expected CreateTable"),
9419+
}
94119420
}
94129421

94139422
#[test]
@@ -9418,11 +9427,20 @@ fn parse_exclude_constraint_function_expression() {
94189427
Statement::CreateTable(create_table) => match &create_table.constraints[0] {
94199428
TableConstraint::Exclusion(c) => {
94209429
assert_eq!(c.elements.len(), 1);
9421-
assert!(matches!(c.elements[0].expr, Expr::Nested(_)));
9430+
match &c.elements[0].expr {
9431+
Expr::Nested(inner) => match inner.as_ref() {
9432+
Expr::Function(func) => {
9433+
assert_eq!(func.name.to_string(), "lower");
9434+
}
9435+
other => panic!("Expected Function inside Nested, got {other:?}"),
9436+
},
9437+
other => panic!("Expected Nested expr, got {other:?}"),
9438+
}
94229439
assert_eq!(
94239440
c.elements[0].operator_class,
94249441
Some(ObjectName::from(vec![Ident::new("text_pattern_ops")]))
94259442
);
9443+
assert_eq!(c.elements[0].operator, "=");
94269444
}
94279445
other => panic!("Expected Exclusion, got {other:?}"),
94289446
},
@@ -9457,7 +9475,11 @@ fn exclude_missing_with_keyword_errors() {
94579475
#[test]
94589476
fn exclude_empty_element_list_errors() {
94599477
let sql = "CREATE TABLE t (CONSTRAINT c EXCLUDE USING gist ())";
9460-
assert!(pg().parse_sql_statements(sql).is_err());
9478+
let err = pg().parse_sql_statements(sql).unwrap_err();
9479+
assert!(
9480+
err.to_string().contains("Expected"),
9481+
"unexpected error: {err}"
9482+
);
94619483
}
94629484

94639485
#[test]
@@ -9472,6 +9494,8 @@ fn exclude_missing_operator_errors() {
94729494

94739495
#[test]
94749496
fn exclude_rejected_in_non_postgres_dialects() {
9497+
// `GenericDialect` is intentionally excluded — it opts in to the
9498+
// Postgres EXCLUDE syntax alongside `PostgreSqlDialect`.
94759499
let sql = "CREATE TABLE t (col INT, EXCLUDE USING gist (col WITH =))";
94769500
for dialect in
94779501
all_dialects_except(|d| d.is::<PostgreSqlDialect>() || d.is::<GenericDialect>()).dialects
@@ -9495,9 +9519,15 @@ fn exclude_as_column_name_parses_in_mysql_and_sqlite() {
94959519
] {
94969520
let type_name = format!("{dialect:?}");
94979521
let parser = TestedDialects::new(vec![dialect]);
9498-
assert!(
9499-
parser.parse_sql_statements(sql).is_ok(),
9500-
"dialect {type_name} failed to parse `exclude` as column name"
9501-
);
9522+
let stmts = parser
9523+
.parse_sql_statements(sql)
9524+
.unwrap_or_else(|e| panic!("{type_name} failed to parse {sql}: {e}"));
9525+
match &stmts[0] {
9526+
Statement::CreateTable(create_table) => {
9527+
assert_eq!(create_table.columns.len(), 1);
9528+
assert_eq!(create_table.columns[0].name.value, "exclude");
9529+
}
9530+
other => panic!("{type_name}: expected CreateTable, got {other:?}"),
9531+
}
95029532
}
95039533
}

0 commit comments

Comments
 (0)