Add support for PostgreSQL's ORDER BY ... USING <operator> clause#2246
Add support for PostgreSQL's ORDER BY ... USING <operator> clause#2246LucaCappelletti94 wants to merge 3 commits intoapache:mainfrom
Conversation
src/parser/mod.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn is_valid_order_by_using_operator_symbol(symbol: &str) -> bool { |
There was a problem hiding this comment.
Removed, parse_order_by_using_operator now uses dialect.is_custom_operator_part(...) directly. No separate symbol-check helper anymore. I wasn't aware there was a dialect helper method.
src/parser/mod.rs
Outdated
|
|
||
| let options = self.parse_order_by_options()?; | ||
| let using_operator = if !with_operator_class | ||
| && dialect_of!(self is PostgreSqlDialect) |
There was a problem hiding this comment.
if we need to dialect guard this, I think we can introduce a dialect method instead of the macro
There was a problem hiding this comment.
Added supports_order_by_using_operator() on Dialect (default false), enabled in PostgreSqlDialect, and parser now checks that method instead of dialect_of!. I double checked that no other dialect supports it at this time.
src/parser/mod.rs
Outdated
| Token::Word(w) | ||
| if w.quote_style.is_none() && terminal_keywords.contains(&w.keyword) => | ||
| { | ||
| break; |
There was a problem hiding this comment.
are these diffs required for this PR?
There was a problem hiding this comment.
No, it was nightly clippy fixes. Removed from git history.
tests/sqlparser_postgres.rs
Outdated
| ); | ||
|
|
||
| // `USING` in ORDER BY is PostgreSQL-specific and should not parse in GenericDialect. | ||
| let generic = TestedDialects::new(vec![Box::new(GenericDialect {})]); |
There was a problem hiding this comment.
note, per the dialect comment, we'd want to avoid hardcoding dialects in tests
There was a problem hiding this comment.
Replaced the GenericDialect negative case with a local wrapper dialect that behaves like Postgres but returns false for supports_order_by_using_operator().
3b2da72 to
7c2e3b6
Compare
|
I’ve applied the requested changes and force-pushed the branch, so to remove irrelevant clippy changes from the commits.
|
| let using_operator = if !with_operator_class | ||
| && self.dialect.supports_order_by_using_operator() | ||
| && self.parse_keyword(Keyword::USING) | ||
| { | ||
| Some(self.parse_order_by_using_operator()?) | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let options = if using_operator.is_some() { | ||
| if self | ||
| .peek_one_of_keywords(&[Keyword::ASC, Keyword::DESC]) | ||
| .is_some() | ||
| { | ||
| return parser_err!( | ||
| "ASC/DESC cannot be used together with USING in ORDER BY".to_string(), | ||
| self.peek_token_ref().span.start | ||
| ); | ||
| } | ||
| OrderByOptions { | ||
| asc: None, | ||
| nulls_first: self.parse_order_by_nulls_first_last(), | ||
| } |
There was a problem hiding this comment.
would it be possible to use something like this representation instead?
pub enum OrderByExpr {
Asc,
Desc,
Using(...),
UsingOperator(...),
}
pub struct OrderByOptions {
pub expr: Option<OrderByExpr>,
pub nulls_first: Option<bool>,
}| #[derive(Debug)] | ||
| struct OrderByUsingDisabledDialect; | ||
|
|
||
| impl Dialect for OrderByUsingDisabledDialect { | ||
| fn is_identifier_start(&self, ch: char) -> bool { | ||
| PostgreSqlDialect {}.is_identifier_start(ch) | ||
| } | ||
|
|
||
| fn is_identifier_part(&self, ch: char) -> bool { | ||
| PostgreSqlDialect {}.is_identifier_part(ch) | ||
| } | ||
|
|
||
| fn supports_order_by_using_operator(&self) -> bool { | ||
| false | ||
| } | ||
| } | ||
|
|
||
| let without_order_by_using = TestedDialects::new(vec![Box::new(OrderByUsingDisabledDialect)]); | ||
| assert!(without_order_by_using | ||
| .parse_sql_statements("SELECT a FROM t ORDER BY a USING <;") |
There was a problem hiding this comment.
I think we can have the tests in common.rs instead to cover dialects via the all_dialects_where(|d| d.support*) pattern. Since the impl is guarded by the dialect method vs explicit to postgres
| pub struct OrderByExpr { | ||
| /// The expression to order by. | ||
| pub expr: Expr, | ||
| /// Optional PostgreSQL `USING <operator>` clause. |
There was a problem hiding this comment.
can we include reference doc to maybe this?
https://www.postgresql.org/docs/current/sql-select.html
so folks know where to find this feature easily
This PR adds parser and AST support for PostgreSQL
USINGoperators inORDER BYexpressions, including:ORDER BYORDER BY(e.g. insideaggfns(...)/string_agg(...)style clauses)Problem
sqlparser-rspreviously rejected valid PostgreSQL statements containing:ORDER BY <expr> USING <operator>with errors like:
Expected: ), found: usingThis blocked parsing of several PostgreSQL regression-style aggregate statements.
Root Cause
parse_order_by_expr_inneronly supported:ASC | DESCNULLS FIRST | NULLS LASTand had no branch to parse
USING <operator>.Dialect Scope Decision
ORDER BY ... USING <operator>is intentionally enabled for PostgreSQL only.Rationale:
ORDER BY expression [ USING operator ] ...inSELECT.ORDER BYwith expression +ASC|DESC(+ null ordering), but notUSING <operator>.References:
Complete Examples
PostgreSQL: accepted
PostgreSQL: rejected (invalid syntax)