Add support for ORDER BY x USING <op> (PostgreSQL) #6
Conversation
coderdan
force-pushed
the
add-orderby-using-support
branch
from
0b7830d
to
d8c459c
Compare
| pub asc: Option<bool>, | ||
| /// Optional `NULLS FIRST` or `NULLS LAST` | ||
| pub nulls_first: Option<bool>, | ||
| /// Optional `USING` operator for ordering (PostgreSQL only) | ||
| pub using: Option<BinaryOperator>, |
There was a problem hiding this comment.
This might be OK in our fork, but I'd probably put more thought into this before raising upstream since we'd have to make a breaking change to support ops other than binary ops.
There was a problem hiding this comment.
Sorry, I don't understand what you mean.
There was a problem hiding this comment.
If the AST is part of the API, changing Option<BinaryOperator> to something else will be a breaking change.
| @@ -6013,6 +6013,22 @@ impl<'a> Parser<'a> { | |||
| Ok(expr) | |||
| } | |||
|
|
|||
| pub fn parse_operator(&mut self) -> Option<BinaryOperator> { | |||
There was a problem hiding this comment.
Since this is for ORDER BY ... USING ... maybe make the name more specific? Something like parse_using_operator?
Also feels like a smell thatUSING needs its own operator parsing function, though.
There was a problem hiding this comment.
I'd have to dig more into how PG handles this itself vs what sqlparser does now for operators to have more concrete suggestions.
There was a problem hiding this comment.
No this function is intended to be a replacement for operator parsing which is a dogs breakfast and buried in the expression parsing stuff. Adding yet another way to parse operators is a bad idea IMHO. Calling this function parse_using_operator sets the expectation for future developers that this is just for that purpose and exacerbates the problem.
There was a problem hiding this comment.
I'm taking the approach of trying to leave the code in a better place than I found it, rather than just throw stuff on the pile.
What's the argument for parsing using operators on their own? Any binary operator is supported AFAICT.
There was a problem hiding this comment.
What's the argument for parsing using operators on their own? Any binary operator is supported AFAICT.
Ah, that's not what I was suggesting. I was confused that this is only used for ORDER BY ... USING .... I don't think that this leaves the code in a better place to be honest because it adds a special case to be aware of that's partially implemented.
Might be fine just to unblock us in our fork so that things parse. Probably worth properly designing before raising upstream.
Adds support for the
USINGkeyword when usingORDER BYin PostgreSQL queries. Only handles a handful of binary operators at the moment but I realised that Postgres operators can be any string of up to 63 valid characters! SqlParser doesn't support that yet, either.