-
Notifications
You must be signed in to change notification settings - Fork 605
ANY
and ALL
contains their operators
#963
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…comparison operators which precede them a variation of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the contribution @SeanTroyUWO -- I apologize for the delay in reviewing.
This change also appears to be consistent with postgres any mysql (where the ANY/ALL is part of the comparison expression):
https://www.postgresql.org/docs/current/functions-comparisons.html
9.24.3. ANY/SOME (array)
expression operator ANY (array expression)
expression operator SOME (array expression)
https://dev.mysql.com/doc/refman/8.0/en/any-in-some-subqueries.html
The ANY keyword, which must follow a comparison operator, ...
Another alternative might be to add it directly to Expr::BinaryOp
but I think that would be far more disruptive to downstream users so this looks good to me
Great, I'll look forward to the next release! |
Next release is tracked by #955 ETA in about 2 weeks |
In sqlparser PR apache#963 a check was introduced which limits which operators can be used with `ANY` and `ALL` expressions. Postgres can parse more (possibly _all_ binary operators, investigation pending) in this location. Postgres only seems to care that the operator yields a boolean - which is a semantic error, not a syntax (parse) error. Example (semantic error, not a parse error): ``` select 123 % ANY(array[246]); ERROR: op ANY/ALL (array) requires operator to yield boolean LINE 1: select 123 % ANY(array[246]); ^ ``` The following code in `src/parser/mod.rs:2893-2908` is where the allowlist of operators is enforced: ```rust if !matches!( op, BinaryOperator::Gt | BinaryOperator::Lt | BinaryOperator::GtEq | BinaryOperator::LtEq | BinaryOperator::Eq | BinaryOperator::NotEq ) { return parser_err!( format!( "Expected one of [=, >, <, =>, =<, !=] as comparison operator, found: {op}" ), tok.span.start ); }; ```
Changes
Expr::AllOp
andExpr::AnyOp
to contain the operator which precedes them, rather than be contained by them. This fixes an issue I had as a polars user: pola-rs/polars#10081.The rational for this is that despite the order the syntax demands, the conditional operator modifies the behavior of the
ANY
command, and not vice versa. Similar toNOT
as withNOT IN
, the negation modifies the behavior of theIN
, and theIN
is the real operation. In both cases, its more convenient to parse the real operation than the modifier(s).I did not find any uses the the old
ANY
orALL
commands in any of DataFusion, LocustDB, Ballista, GlueSQL or Opteryx. This might still briefly break their builds however.If more unit tests are required, please let me know.
Fixes: #936