feat: add ANY/ALL/SOME(array) operator to support get_indexes in sqlalchemy
#6717
Conversation
HuaHuaY
requested review from
xiangjinwu,
BugenZhao,
BowenXiao1999 and
st1page
Codecov Report
@@ Coverage Diff @@
## main #6717 +/- ##
==========================================
- Coverage 73.02% 72.96% -0.06%
==========================================
Files 1063 1064 +1
Lines 169728 169983 +255
==========================================
+ Hits 123937 124028 +91
- Misses 45791 45955 +164
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // TODO: support `all/any/some(subquery)`. | ||
| if let Expr::Subquery(_) = &right { | ||
| parser_err!("ANY/SOME/ALL(Subquery) is not implemented")?; | ||
| } |
There was a problem hiding this comment.
Actually this only covers part of the cases:
select 1 < ALL(select 1); -- parse_expr error above
select 1 < ALL((select 1)); -- this check
select 1 < ALL(((select 1))); -- allowed by PG but rejected for mismatching type by us
select 1 < ALL(((select array[1]))); -- disallowed by PG but evaluated as scalar subquery by us
A full fix may take a bigger effort. Just pointing it out and lets focus on non-subquery first.
There was a problem hiding this comment.
I will create a new issue to implement all/any/some(subquery) and avoid inconsistent behavior.
src/expr/src/expr/expr_some_all.rs
Outdated
| self.compare_left_right( | ||
| left.map(|s| s.into_scalar_impl()), | ||
| right.map(|s| s.into_scalar_impl()), | ||
| )? |
There was a problem hiding this comment.
In general, we should avoid relying on repeated eval_row calls inside eval. Especially when we are already considering deprecate it. cc @TennyZhuang
Ideally, SomeAllExpression (or ScalarArrayOpExpr in PostgreSQL) takes 3 generic parameter <L, R, F> similar to BinaryExpression<L, R, O, F>, where output array type is omitted because it is always bool. That is, we store the inner operator in the form of a rust function (func: F) rather than a BoxedExpression. But this seems requires a refactor of our way of building expressions, similar to the frontend binder refactor of introducing resolve_binary_operator - we just want the match arms without building the whole expr.
Alternatively, we can still embed BoxExpression but avoid eval_row. Borrowing from the idea of short-circuit evaluation (#6208), we can first eval over a chunk of 2 columns consisting (lhs, rhs[0]), mask out with visibility for rows already determined (ALL-false/SOME-true/array-len-reached), then continue with another chunk of (lhs, rhs[1])...
There was a problem hiding this comment.
is this what we will change in this PR?
There was a problem hiding this comment.
Yes. But I'm doing other work now, so this modification will be delayed for a while.
| &bound_right, | ||
| )?); | ||
|
|
||
| FunctionCall::new_binary_op_func(func_types, vec![bound_left, bound_right]) |
There was a problem hiding this comment.
This refactor and introduction of dedicated resolve_binary_operator/new_binary_op_func/infer_some_all seems have some space of improvement. But compared to the backend eval_row issue, let's focus on that first and then unblock future development of sqlalchemy.
Talking about the better solution, note that:
Concatnever returns a boolean and can be excluded / treated specially.PGRegexMatch/PGRegexNotMatchactually needs to have their dedicated ExprType rather than currentRegexpMatch+IsNull. fix(expr): regex operator~/!~should return null when either input is null #5136- After the fix above,
resolve_binary_operatorno longer needs to returnVec<ExprType>for arbitrary levels of nesting. It only needs(ExprType, bool)for one level and an optional negation. - PostgreSQL also forbids
x SIMILAR TO ALL(y)simply becauseSIMILAR TOis not a native operator but rewritten into multiple exprs.
The fact of operator being a single ExprType may help simplify these parts.
There was a problem hiding this comment.
I will create a new issue to track this.
There was a problem hiding this comment.
LGTM for the query part. I'm not so sure about the impl in binder and expression so pls follow @xiangjinwu
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 2607 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 1248 | 1 | 1358 | 0 |
Click to see the invalid file list
- src/expr/src/expr/expr_some_all.rs
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
As the title.
These operators are used in the right side of binary operators which return boolean value. They require that there are some expressions in the parentheses, which return arrays.
SOMEis a synonym forANY.In PostgreSQL, there are some other operators named
ANY/ALL/SOME(subquery). They are not supported in this PR.Checklist
./risedev check(or alias,./risedev c)Documentation
Types of user-facing changes
Release note
support
ANY/ALL/SOME(array)operator.PostgreSQL's doc:
ANY/SOME (array): https://www.postgresql.org/docs/current/functions-comparisons.html#id-1.5.8.30.16
ALL (array): https://www.postgresql.org/docs/current/functions-comparisons.html#id-1.5.8.30.17
Refer to a related PR or issue link (optional)
related #6636