planner: handle the selection between the join group#64535
planner: handle the selection between the join group#64535ti-chi-bot[bot] merged 4 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
release-note-none
|
Hi @Reminiscent. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@winoros PTAL |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #64535 +/- ##
================================================
- Coverage 77.7973% 77.6002% -0.1972%
================================================
Files 1993 1916 -77
Lines 544116 531805 -12311
================================================
- Hits 423308 412682 -10626
+ Misses 119149 119117 -32
+ Partials 1659 6 -1653
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
bot
added
size/XXL
| group []base.LogicalPlan | ||
| joinOrderHintInfo []*h.PlanHints | ||
| eqEdges []*expression.ScalarFunction | ||
| otherConds []expression.Expression |
There was a problem hiding this comment.
It should be here. There's no need to create new slice.
ti-chi-bot
bot
added
size/L
|
/retest |
|
@Reminiscent: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Reminiscent
force-pushed
the
handle-selection-in-join-order
branch
from
32a4b05 to
ca633a9
Compare
| // to otherConds and continue extracting the join group from the child. | ||
| if selection, isSelection := p.(*logicalop.LogicalSelection); isSelection && p.SCtx().GetSessionVars().TiDBOptJoinReorderSel { | ||
| child := selection.Children()[0] | ||
| if _, isChildJoin := child.(*logicalop.LogicalJoin); isChildJoin { |
There was a problem hiding this comment.
We should also handle the volatile function here. Not sure it has been handled somewhere else.
if slices.ContainsFunc(selection.Conditions, expression.IsMutableEffectsExpr) {
return &joinGroupResult{
group: []base.LogicalPlan{p},
joinOrderHintInfo: joinOrderHintInfo,
basicJoinGroupInfo: &basicJoinGroupInfo{},
}
}
fixdb
left a comment
fixdb
left a comment
There was a problem hiding this comment.
Can we also test cases where the SELECTION has correlated subuqery?
|
@fixdb updated. PTAL |
|
Did you push your latest commit? |
Reminiscent
force-pushed
the
handle-selection-in-join-order
branch
from
ca633a9 to
72740c9
Compare
ti-chi-bot
bot
added
size/XXL
Sorry. Rebased and pushed. PTAL @fixdb |
|
|
||
| // Check if the current plan is a Selection. If its child is a join, add the selection conditions | ||
| // to otherConds and continue extracting the join group from the child. | ||
| if selection, isSelection := p.(*logicalop.LogicalSelection); isSelection && p.SCtx().GetSessionVars().TiDBOptJoinReorderSel && allowReorderThroughSelection(selection.Conditions) { |
There was a problem hiding this comment.
Why not !slices.ContainsFunc(selection.Conditions, expression.IsMutableEffectsExpr) but define a allowReorderThroughSelection?
There was a problem hiding this comment.
Originally, I was thinking of making it a function so that it would be easier to add more conditions in the future (if needed) and keep it more flexible. However, for the current situation, you’re right that the approach you suggested is simpler and clearer. I’ve already updated it accordingly. PTAL @fixdb
|
/retest |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
Reminiscent
force-pushed
the
handle-selection-in-join-order
branch
from
584c14f to
851b039
Compare
|
@yudongusa PTAL |
|
/assign @yudongusa |
|
@Reminiscent: GitHub didn't allow me to assign the following users: yudongusa. Note that only pingcap members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/retest |
|
@Reminiscent: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
yudongusa
left a comment
yudongusa
left a comment
There was a problem hiding this comment.
Please open a document PR on sysvar
|
|
||
| // Check if the current plan is a Selection. If its child is a join, add the selection conditions | ||
| // to otherConds and continue extracting the join group from the child. | ||
| // Join reorder may distribute/push down conditions during constructing the new join tree. |
There was a problem hiding this comment.
CODEX has tried his best to come up with false case, but failed. We may some confidence though.
• 我尝试构造 “master 正确 / PR 开启 tidb_opt_join_reorder_sel=1 错误” 的用例，但目前没复现出结果差异。下面是我实际试过的两个候选 case，SET_VAR=0/1 输出一致： Case A（Selection 依赖 outer join 两侧） select /*+ SET_VAR(tidb_opt_join_reorder_sel=0) */ x.id, x.a, x.v from ( select t1.id, t1.a, t2.v from t1 left join t2 on t1.id=t2.id where t1.a + ifnull(t2.v,0) > 0 ) x join t3 on x.id = t3.id order by x.id; select /*+ SET_VAR(tidb_opt_join_reorder_sel=1) */ x.id, x.a, x.v from ( select t1.id, t1.a, t2.v from t1 left join t2 on t1.id=t2.id where t1.a + ifnull(t2.v,0) > 0 ) x join t3 on x.id = t3.id order by x.id; 输出一致： 1 1 0 2 -5 10 3 1 NULL Case B（非空拒绝性不强的条件） select /*+ SET_VAR(tidb_opt_join_reorder_sel=0) */ x.id, x.a, x.v from ( select t1.id, t1.a, t2.v from t1 left join t2 on t1.id=t2.id where t2.v is null or t1.a > 0 ) x join t3 on x.id = t3.id order by x.id; select /*+ SET_VAR(tidb_opt_join_reorder_sel=1) */ x.id, x.a, x.v from ( select t1.id, t1.a, t2.v from t1 left join t2 on t1.id=t2.id where t2.v is null or t1.a > 0 ) x join t3 on x.id = t3.id order by x.id; 输出一致： 1 1 0 3 1 NULL
@AilinKid please use English in the comments.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, fixdb, yudongusa The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
get, updated |
|
/cherrypick release-8.5 |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
@Reminiscent: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
…#64535) (pingcap#65742)" This reverts commit c46c8a3.
6 participants
What problem does this PR solve?
Issue Number: close #59972
Problem Summary:
Handle the selection between the join group. Let more join group to participate the join order phase.
What changed and how does it work?
The
selectionplan node prevent the functionextractJoinGroupto get more join groups.So when we detect the
selectionnode in theextractJoinGroup, we'll store them.And we'll add them in the correct position(as low as possible) when we rebuild the join node.
Add the variables to keep the default behavior.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.