Combine the two rules: DecorrelateWhereExists and DecorrelateWhereIn #6271
Conversation
There was a problem hiding this comment.
Thank you @mingmwang -- this is looking awesome. I reviewed the plan changes carefully and spot checked the code.
Everything I saw looks great, but I think it would help if someone who was more familiar with this code (like @ygf11 or @jackwener , perhaps) could review it more carefully.
If they don't have time, I will try and find time to fully review this PR later this week
| @@ -5,13 +5,14 @@ | |||
| | | Projection: orders.o_orderpriority, COUNT(UInt8(1)) AS order_count | | |||
There was a problem hiding this comment.
I reviewed these plans and they see to be all the same, structurally (there are some aliases that are different)
|
@jackwener |
|
I am going to review this PR carefully in the next two days, Thanks @mingmwang |
| @@ -54,7 +59,7 @@ impl DecorrelateWhereIn { | |||
| predicate: &Expr, | |||
| config: &dyn OptimizerConfig, | |||
| ) -> Result<(Vec<SubqueryInfo>, Vec<Expr>)> { | |||
| let filters = split_conjunction(predicate); // TODO: disjunctions | |||
| let filters = split_conjunction(predicate); // TODO: add ExistenceJoin to support disjunctions | |||
There was a problem hiding this comment.
This PR already support disjunction of scalar/in subquery?
There was a problem hiding this comment.
No, it doesn't. To support disjunction of predicate subqueries, need to add a specific join type ExistenceJoin
There was a problem hiding this comment.
Ok, BTW, there is a issue to track it MarkJoin #5368
|
Can we merge this PR, so that I can continue working on the other sub query related improvements/enhancements. |
I review this PR an there is nothing wrong with this PR I think we can push this PR to continue to do more work |
|
THanks @jackwener and @mingmwang |
|
I believe this caused a minor diff on main causing Ci to fail: #6353 |
Which issue does this PR close?
Closes #6086
Rationale for this change
Combine the two rules into one unify rule
DecorrelatePredicateSubqueryWhat changes are included in this PR?
Are these changes tested?
Yes, covered by existing UTs.
Are there any user-facing changes?