feat(frontend): support InSubquery. #2187
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2187 +/- ##
==========================================
+ Coverage 71.02% 71.05% +0.02%
==========================================
Files 657 657
Lines 83592 83633 +41
==========================================
+ Hits 59373 59423 +50
+ Misses 24219 24210 -9
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| - sql: | | ||
| create table t1(x int, y int); | ||
| create table t2(x int, y int); | ||
| select x from t1 where y in (select y from t2 where t1.x = t2.x); |
There was a problem hiding this comment.
Can you add more test cases? e.g.
not in- with other conditions in the subquery
- with more than 1 correlated variable in the subquery
- with non-equi correlated condition in the subquery
- ...
There was a problem hiding this comment.
I have added tests for not in in subquery_expr.yaml, because we cann't unnest LogicalAntiApply now. I will add other types of tests.
There was a problem hiding this comment.
It's okay. You may add a test expecting error output if that's not supported yet.
There was a problem hiding this comment.
I just tried it and it will panic, so it doesn't display any error. I just asked @skyzh about this, and he said that planner test can't handle panic.
There was a problem hiding this comment.
create_apply_or_join may not be enough. This issue is mostly specific to ExprImpl::Subquery, ExprImpl::CorrelatedInputRef, and LogicalApply: they are supposed to be present in the expr tree or plan tree in early phases, and completely replaced later. Another way to report errors is to add a dedicate phase than scans the whole plan for any undesired residuals - if we do not want to make existing to_prost / to_batch / to_stream to return Result.
There was a problem hiding this comment.
Another way to report errors is to add a dedicate phase than scans the whole plan for any undesired residuals
Do you mean do this check right after unnesting? If so, then we should add optimizer_error.
There was a problem hiding this comment.
I meant impl PlanVisitor just to check the existence of one of the three, and call it in PlanRoot::gen_batch_query_plan / PlanRoot::gen_stream_plan immediately after gen_optimized_logical_plan.
There was a problem hiding this comment.
In conclusion, we will add optimizer error or something else in future prs to report error instead of panicing.
There was a problem hiding this comment.
impl PlanVisitor just to check the existence of one of the three
+1
likg227
force-pushed
the
lkg/in_subquery
branch
from
cca0cbe
to
ac4546f
Compare
NOT EXISTS will be parsed to NOT + EXISTS. They are not an entity. |
likg227
force-pushed
the
lkg/in_subquery
branch
from
cc9817e
to
d84566e
Compare
What's changed and what's your intention?
PLEASE DO NOT LEAVE THIS EMPTY !!!
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Summarize your change (mandatory)
In this pr, I supported (NOT) InSubquery for WHERE clause and add
onforLogicalApply.I bind InSubquery as normal InList and plan it as Left-Semi-Apply using equal condition as its predicate.
Describe any limitations of the current code (optional)
Please note that InSubquery that goes into others cannot be handled by substitute_subqueries yet.
Checklist
Refer to a related PR or issue link (optional)
close #2027