planner: enable cd-c algorithm by default for join reorder | tidb-test=pr/2759#68880
planner: enable cd-c algorithm by default for join reorder | tidb-test=pr/2759#68880guo-shaoge wants to merge 33 commits into
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
…nto cp_pre_cdc_impl
…nto cp_pre_cdc_impl
Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
release-note-none
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
ti-chi-bot
Bot
added
size/XXL
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements a new join-order planner (ConflictDetector + DP/greedy algorithms), gates it behind a session var, adds benchmarks, registers CDC join-reorder casetests (inputs + expected outputs), updates test build rules, and regenerates many planner/executor expected fixtures. ChangesCDC Join Reordering Core Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant JoinReOrderSolver
participant joinorder.Optimize
participant ConflictDetector
Client->>JoinReOrderSolver: request Optimize(p)
JoinReOrderSolver->>joinorder.Optimize: delegate when TiDBOptEnableAdvancedJoinReorder=1
joinorder.Optimize->>ConflictDetector: Build(joinGroup)
joinorder.Optimize->>joinorder.Optimize: run DP or Greedy optimizer
joinorder.Optimize->>ConflictDetector: CheckConnection / MakeJoin during enumeration
joinorder.Optimize->>JoinReOrderSolver: return optimized plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/util/hint/hint.go (1)
955-983:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
LeadingListwhen the LEADING hint is invalidated.This block only resets
leadingJoinOrder, but the new optimizer path consumesPlanHints.LeadingListdirectly. With multipleLEADINGhints—orLEADINGplusSTRAIGHT_JOIN—the code still returns the first parsedLeadingList, so join reorder can keep applying a hint after warning that all LEADING hints are invalid.Suggested fix
if leadingHintCnt > 1 || (leadingHintCnt > 0 && straightJoinOrder) { // If there are more leading hints or the straight_join hint existes, all leading hints will be invalid. leadingJoinOrder = leadingJoinOrder[:0] + leadingList = nil if leadingHintCnt > 1 { warnHandler.SetHintWarning("We can only use one leading hint at most, when multiple leading hints are used, all leading hints will be invalid") } else if straightJoinOrder { warnHandler.SetHintWarning("We can only use the straight_join hint, when we use the leading hint and straight_join hint at the same time, all leading hints will be invalid") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/util/hint/hint.go` around lines 955 - 983, The LEADING invalidation branch currently clears leadingJoinOrder but leaves the parsed leadingList intact, so PlanHints.LeadingList is still returned; update the branch in the block that checks (leadingHintCnt > 1 || (leadingHintCnt > 0 && straightJoinOrder)) to also clear leadingList (e.g., set leadingList to nil or leadingList = leadingList[:0]) when you clear leadingJoinOrder, so PlanHints.LeadingList is empty when LEADING hints are invalidated; keep the existing warnHandler.SetHintWarning calls as-is.pkg/planner/core/casetest/rule/BUILD.bazel (1)
20-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing Bazel dep for
testfailpoint.
rule_cdc_join_reorder_test.goimportsgithub.com/pingcap/tidb/pkg/testkit/testfailpointon Line 22, butrule_testdoes not list//pkg/testkit/testfailpointindeps. Withrules_gostrict deps, this target will fail to compile once the new test file is built.Suggested fix
deps = [ "//pkg/config", "//pkg/domain", "//pkg/expression", "//pkg/expression/aggregation", "//pkg/expression/exprstatic", "//pkg/parser/ast", "//pkg/parser/mysql", "//pkg/planner/util/coreusage", "//pkg/testkit", "//pkg/testkit/testdata", + "//pkg/testkit/testfailpoint", "//pkg/testkit/testmain", "//pkg/testkit/testsetup", "//pkg/types", "//pkg/util/mock", "`@com_github_pingcap_failpoint//`:failpoint",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/casetest/rule/BUILD.bazel` around lines 20 - 38, The BUILD.bazel deps for the rule_test target are missing the testfailpoint package used by rule_cdc_join_reorder_test.go; update the deps array in pkg/planner/core/casetest/rule/BUILD.bazel to include the //pkg/testkit/testfailpoint bazel label so the import in rule_cdc_join_reorder_test.go resolves under rules_go strict deps.
🧹 Nitpick comments (1)
pkg/planner/core/joinorder/bitset_bench_test.go (1)
78-85: ⚡ Quick winBenchmark setup only measures the failing
TES ⊆ Spath.
buildFastEdges/buildBitEdgesalways putextraintotes, but the benchmark candidate setsis onlyleft ∪ right. That makestes.SubsetOf(s)/s.IsSuperSet(tes)false for every edge, so the first condition short-circuits and theIntersects/IntersectionCardinalitychecks from the realcheckInnerEdgeApplicablehot path never run. This will skew the comparison toward the failure case instead of the production path inpkg/planner/core/joinorder/conflict_detector.go.Possible adjustment
- s := e.left.Union(e.right) + s := e.left.Union(e.right).Union(e.tes) // or split pass/fail cases into separate sub-benchmarksAlso applies to: 103-110, 140-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/joinorder/bitset_bench_test.go` around lines 78 - 85, The benchmark currently always puts the `extra` node into `tes` (in `buildFastEdges`/`buildBitEdges`) while the candidate set `s` used in the benchmark is only `left ∪ right`, forcing `tes ⊆ s` to fail and only measuring the failing path; update the benchmark so `s` includes `extra` (or alternatively remove `extra` from how `tes` is constructed) so that `tes.SubsetOf(s)`/`s.IsSuperSet(tes)` can succeed and the hot path in `checkInnerEdgeApplicable` (the `Intersects`/`IntersectionCardinality` checks) is exercised. Use the unique symbols `tes`, `extra`, `left`, `right`, `buildFastEdges`/`buildBitEdges`, and `checkInnerEdgeApplicable` to locate and adjust the setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/parser/ast/misc.go`:
- Around line 3967-4005: RestoreWithQB currently forwards the query-block name
into nested LeadingList items, which moves the `@qb` inside tuples; change the
logic in LeadingList.RestoreWithQB so that if i==0 and currentQBName.L != "" and
not qbOnTable and the item is a *LeadingList, you emit the "@<qb>" before
restoring the nested group (use ctx.WriteKeyWord("@") and
ctx.WriteName(currentQBName.String()) plus a space), then call t.RestoreWithQB
with an empty model.CIStr (clearing the qb for the recursive call) so the marker
stays at the outer list; adjust currentQBName handling accordingly (do not pass
the qb into the recursive call and ensure currentQBName is cleared after
emitting).
In `@pkg/parser/hintparser_test.go`:
- Around line 369-474: The test cases in pkg/parser/hintparser_test.go use
ast.NewCIStr which doesn't exist; replace all uses of ast.NewCIStr with
model.NewCIStr in the table hint expectations (e.g. the LEADING test blocks that
construct ast.TableOptimizerHint, ast.LeadingList, and ast.HintTable) and update
imports to include the model package (removing or adjusting the ast import if it
becomes unused) so the file compiles.
In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_in.json`:
- Around line 5-245: The fixture contains JavaScript-style inline comments
(lines starting with "//", e.g. the group headings like "Group 1: Inner join
reorder (baseline)" and inline notes) which make the file invalid JSON; remove
all "// ..." comments throughout the file so only valid JSON remains (keep the
array of SQL string literals such as "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a
..." and the surrounding JSON punctuation), ensuring no stray comment characters
remain and the file parses as strict JSON.
In `@pkg/planner/core/joinorder/conflict_detector.go`:
- Around line 827-846: The appended post-join Selection built from
checkResult.appliedInnerEdges must have its NOT_NULL metadata realigned before
being attached to existingJoin; for each condition produced (from
expression.ScalarFuncs2Exprs(e.eqConds) and e.nonEQConds) call
alignNotNullWithSchema using the null-producing child's schema (the child schema
used when the outer join was reordered) so NOT_NULL flags reflect the current
child schemas, then Init the logicalop.LogicalSelection (as now) and
SetChildren(existingJoin) and return it.
In `@pkg/planner/core/joinorder/join_order.go`:
- Line 100: The code currently checks join.JoinType against
base.LeftOuterJoin/base.RightOuterJoin; update the condition to compare
join.JoinType to logicalop.LeftOuterJoin and logicalop.RightOuterJoin instead.
Specifically in the gate that reads
p.SCtx().GetSessionVars().EnableOuterJoinReorder && (join.JoinType == ...),
replace references to base.LeftOuterJoin and base.RightOuterJoin with
logicalop.LeftOuterJoin and logicalop.RightOuterJoin so the join-type
comparisons use the correct constants.
---
Outside diff comments:
In `@pkg/planner/core/casetest/rule/BUILD.bazel`:
- Around line 20-38: The BUILD.bazel deps for the rule_test target are missing
the testfailpoint package used by rule_cdc_join_reorder_test.go; update the deps
array in pkg/planner/core/casetest/rule/BUILD.bazel to include the
//pkg/testkit/testfailpoint bazel label so the import in
rule_cdc_join_reorder_test.go resolves under rules_go strict deps.
In `@pkg/util/hint/hint.go`:
- Around line 955-983: The LEADING invalidation branch currently clears
leadingJoinOrder but leaves the parsed leadingList intact, so
PlanHints.LeadingList is still returned; update the branch in the block that
checks (leadingHintCnt > 1 || (leadingHintCnt > 0 && straightJoinOrder)) to also
clear leadingList (e.g., set leadingList to nil or leadingList =
leadingList[:0]) when you clear leadingJoinOrder, so PlanHints.LeadingList is
empty when LEADING hints are invalidated; keep the existing
warnHandler.SetHintWarning calls as-is.
---
Nitpick comments:
In `@pkg/planner/core/joinorder/bitset_bench_test.go`:
- Around line 78-85: The benchmark currently always puts the `extra` node into
`tes` (in `buildFastEdges`/`buildBitEdges`) while the candidate set `s` used in
the benchmark is only `left ∪ right`, forcing `tes ⊆ s` to fail and only
measuring the failing path; update the benchmark so `s` includes `extra` (or
alternatively remove `extra` from how `tes` is constructed) so that
`tes.SubsetOf(s)`/`s.IsSuperSet(tes)` can succeed and the hot path in
`checkInnerEdgeApplicable` (the `Intersects`/`IntersectionCardinality` checks)
is exercised. Use the unique symbols `tes`, `extra`, `left`, `right`,
`buildFastEdges`/`buildBitEdges`, and `checkInnerEdgeApplicable` to locate and
adjust the setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5c32c72-29ea-456f-8f77-5335e2446935
📒 Files selected for processing (24)
pkg/expression/schema.gopkg/parser/ast/misc.gopkg/parser/ast/misc_test.gopkg/parser/hintparser.gopkg/parser/hintparser.ypkg/parser/hintparser_test.gopkg/parser/parser_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/main_test.gopkg/planner/core/casetest/rule/rule_cdc_join_reorder_test.gopkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_in.jsonpkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.jsonpkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.jsonpkg/planner/core/joinorder/BUILD.bazelpkg/planner/core/joinorder/bitset_bench_test.gopkg/planner/core/joinorder/conflict_detector.gopkg/planner/core/joinorder/join_order.gopkg/planner/core/joinorder/util.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/operator/logicalop/logical_projection.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/rule_join_reorder.gopkg/util/hint/hint.go
| func (lt *LeadingList) RestoreWithQB(ctx *format.RestoreCtx, qbName model.CIStr, needParen bool, isTop bool, qbOnTable bool) error { | ||
| if lt == nil || len(lt.Items) == 0 { | ||
| return nil | ||
| } | ||
| if needParen { | ||
| ctx.WritePlain("(") | ||
| } | ||
|
|
||
| currentQBName := qbName // hint level QBName | ||
|
|
||
| for i, item := range lt.Items { | ||
| if i > 0 { | ||
| ctx.WritePlain(", ") | ||
| } | ||
|
|
||
| switch t := item.(type) { | ||
| case *HintTable: | ||
| if i == 0 && currentQBName.L != "" && !qbOnTable { | ||
| ctx.WriteKeyWord("@") | ||
| ctx.WriteName(currentQBName.String()) | ||
| ctx.WritePlain(" ") | ||
| t.Restore(ctx) | ||
| currentQBName = model.CIStr{} | ||
| } else { | ||
| t.Restore(ctx) | ||
| } | ||
| case *LeadingList: | ||
| if err := t.RestoreWithQB(ctx, currentQBName, true, false, qbOnTable); err != nil { | ||
| return err | ||
| } | ||
| currentQBName = model.CIStr{} | ||
| default: | ||
| return fmt.Errorf("unexpected type in LeadingList: %T", t) | ||
| } | ||
| } | ||
| if needParen { | ||
| ctx.WritePlain(")") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Preserve hint-level QB placement when the first LEADING item is a nested group.
RestoreWithQB currently pushes @qb down to the first HintTable it sees. For LEADING(@sel1 (t1, t2), t3), that restores as LEADING((@sel1 t1, t2), t3), but pkg/parser/hintparser.y only parses the query-block marker before the whole leading list, not inside a nested tuple. That breaks restore/parse round-tripping for a valid hint.
Proposed fix
func (lt *LeadingList) RestoreWithQB(ctx *format.RestoreCtx, qbName model.CIStr, needParen bool, isTop bool, qbOnTable bool) error {
if lt == nil || len(lt.Items) == 0 {
return nil
}
if needParen {
ctx.WritePlain("(")
}
currentQBName := qbName // hint level QBName
for i, item := range lt.Items {
if i > 0 {
ctx.WritePlain(", ")
}
+ if i == 0 && currentQBName.L != "" && !qbOnTable {
+ ctx.WriteKeyWord("@")
+ ctx.WriteName(currentQBName.String())
+ ctx.WritePlain(" ")
+ currentQBName = model.CIStr{}
+ }
switch t := item.(type) {
case *HintTable:
- if i == 0 && currentQBName.L != "" && !qbOnTable {
- ctx.WriteKeyWord("@")
- ctx.WriteName(currentQBName.String())
- ctx.WritePlain(" ")
- t.Restore(ctx)
- currentQBName = model.CIStr{}
- } else {
- t.Restore(ctx)
- }
+ t.Restore(ctx)
case *LeadingList:
- if err := t.RestoreWithQB(ctx, currentQBName, true, false, qbOnTable); err != nil {
+ if err := t.RestoreWithQB(ctx, model.CIStr{}, true, false, qbOnTable); err != nil {
return err
}
- currentQBName = model.CIStr{}
default:
return fmt.Errorf("unexpected type in LeadingList: %T", t)
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (lt *LeadingList) RestoreWithQB(ctx *format.RestoreCtx, qbName model.CIStr, needParen bool, isTop bool, qbOnTable bool) error { | |
| if lt == nil || len(lt.Items) == 0 { | |
| return nil | |
| } | |
| if needParen { | |
| ctx.WritePlain("(") | |
| } | |
| currentQBName := qbName // hint level QBName | |
| for i, item := range lt.Items { | |
| if i > 0 { | |
| ctx.WritePlain(", ") | |
| } | |
| switch t := item.(type) { | |
| case *HintTable: | |
| if i == 0 && currentQBName.L != "" && !qbOnTable { | |
| ctx.WriteKeyWord("@") | |
| ctx.WriteName(currentQBName.String()) | |
| ctx.WritePlain(" ") | |
| t.Restore(ctx) | |
| currentQBName = model.CIStr{} | |
| } else { | |
| t.Restore(ctx) | |
| } | |
| case *LeadingList: | |
| if err := t.RestoreWithQB(ctx, currentQBName, true, false, qbOnTable); err != nil { | |
| return err | |
| } | |
| currentQBName = model.CIStr{} | |
| default: | |
| return fmt.Errorf("unexpected type in LeadingList: %T", t) | |
| } | |
| } | |
| if needParen { | |
| ctx.WritePlain(")") | |
| } | |
| return nil | |
| func (lt *LeadingList) RestoreWithQB(ctx *format.RestoreCtx, qbName model.CIStr, needParen bool, isTop bool, qbOnTable bool) error { | |
| if lt == nil || len(lt.Items) == 0 { | |
| return nil | |
| } | |
| if needParen { | |
| ctx.WritePlain("(") | |
| } | |
| currentQBName := qbName // hint level QBName | |
| for i, item := range lt.Items { | |
| if i > 0 { | |
| ctx.WritePlain(", ") | |
| } | |
| if i == 0 && currentQBName.L != "" && !qbOnTable { | |
| ctx.WriteKeyWord("@") | |
| ctx.WriteName(currentQBName.String()) | |
| ctx.WritePlain(" ") | |
| currentQBName = model.CIStr{} | |
| } | |
| switch t := item.(type) { | |
| case *HintTable: | |
| t.Restore(ctx) | |
| case *LeadingList: | |
| if err := t.RestoreWithQB(ctx, model.CIStr{}, true, false, qbOnTable); err != nil { | |
| return err | |
| } | |
| default: | |
| return fmt.Errorf("unexpected type in LeadingList: %T", t) | |
| } | |
| } | |
| if needParen { | |
| ctx.WritePlain(")") | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/parser/ast/misc.go` around lines 3967 - 4005, RestoreWithQB currently
forwards the query-block name into nested LeadingList items, which moves the `@qb`
inside tuples; change the logic in LeadingList.RestoreWithQB so that if i==0 and
currentQBName.L != "" and not qbOnTable and the item is a *LeadingList, you emit
the "@<qb>" before restoring the nested group (use ctx.WriteKeyWord("@") and
ctx.WriteName(currentQBName.String()) plus a space), then call t.RestoreWithQB
with an empty model.CIStr (clearing the qb for the recursive call) so the marker
stays at the outer list; adjust currentQBName handling accordingly (do not pass
the qb into the recursive call and ensure currentQBName is cleared after
emitting).
| // ============================================= | ||
| // Group 1: Inner join reorder (baseline) | ||
| // ============================================= | ||
|
|
||
| // 3-table inner join | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a ORDER BY t1.a", | ||
| // 4-table inner join | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a JOIN t4 ON t3.a = t4.a ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 2: LEFT JOIN + INNER JOIN | ||
| // ============================================= | ||
|
|
||
| // assoc(inner, left) = 1: (t1 INNER t2) LEFT t3 | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a ORDER BY t1.a, t2.a", | ||
| // assoc(left, inner) = 0: (t1 LEFT t2) INNER t3 on t1.a = t3.a | ||
| // A conflict rule should be added to prevent incorrect reorder. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a JOIN t3 ON t1.a = t3.a ORDER BY t1.a, t3.a", | ||
| // leftAsscom(left, inner) = 1: (t1 LEFT t2) INNER t3 on t2.a = t3.a | ||
| // leftAsscom allows reorder when the inner join condition references the right side of the left join. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a ORDER BY t1.a, t2.a, t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 3: RIGHT JOIN + INNER JOIN | ||
| // ============================================= | ||
|
|
||
| // assoc(right, inner) = 1: (t1 RIGHT t2) INNER t3 | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a ORDER BY t2.a, t3.a", | ||
| // assoc(inner, right) = 0: (t1 INNER t2) RIGHT t3 | ||
| // A conflict rule prevents incorrect reorder. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a RIGHT JOIN t3 ON t2.a = t3.a ORDER BY t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 4: Nested LEFT JOINs | ||
| // ============================================= | ||
|
|
||
| // assoc(left, left) = 1: (t1 LEFT t2) LEFT t3 on t2.a = t3.a | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a ORDER BY t1.a", | ||
| // leftAsscom(left, left) = 1: (t1 LEFT t2) LEFT t3 on t1.a = t3.a | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t1.a = t3.a ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 5: Nested RIGHT JOINs | ||
| // ============================================= | ||
|
|
||
| // assoc(right, right) = 1: (t1 RIGHT t2) RIGHT t3 on t2.a = t3.a | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a RIGHT JOIN t3 ON t2.a = t3.a ORDER BY t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 6: LEFT + RIGHT mixed | ||
| // ============================================= | ||
|
|
||
| // assoc(right, left) = 1: (t1 RIGHT t2) LEFT t3 on t2.a = t3.a | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a ORDER BY t2.a", | ||
| // assoc(left, right) = 0: (t1 LEFT t2) RIGHT t3 on t1.a = t3.a | ||
| // Conflict rule should prevent incorrect reorder. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a RIGHT JOIN t3 ON t1.a = t3.a ORDER BY t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 7: rightAsscom(right, left) = 1 | ||
| // ============================================= | ||
|
|
||
| // rightAsscom(right, left) = 1: t1 RIGHT (t2 LEFT t3) | ||
| // In SQL: t2 LEFT t3 is a subgroup; t1 RIGHT JOIN that. | ||
| // We write it as: t1 RIGHT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t1.a = t3.a | ||
| // Parse tree: (t1 RIGHT t2) LEFT t3 | ||
| // Use a different structure to test rightAsscom: | ||
| // t2 LEFT t3 ON t2.a = t3.a, then t1 RIGHT t2 ON t1.b = t2.b | ||
| // This is achieved via: t1 RIGHT JOIN (t2 LEFT JOIN t3 ON t2.a = t3.a) ON t1.a = t2.a | ||
| // But MySQL syntax doesn't allow this directly. Use: | ||
| "SELECT * FROM t2 LEFT JOIN t3 ON t2.a = t3.a RIGHT JOIN t1 ON t2.a = t1.a ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 8: Complex 4-table mixed outer joins | ||
| // ============================================= | ||
|
|
||
| // LEFT + INNER + LEFT: (t1 LEFT t2) INNER t3 LEFT t4 | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a JOIN t3 ON t1.a = t3.a LEFT JOIN t4 ON t3.a = t4.a ORDER BY t1.a", | ||
| // RIGHT + INNER + RIGHT: (t1 RIGHT t2) INNER t3 RIGHT t4 | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a RIGHT JOIN t4 ON t3.a = t4.a ORDER BY t4.a", | ||
| // INNER + LEFT + RIGHT: t1 INNER t2 LEFT t3 RIGHT t4 | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a RIGHT JOIN t4 ON t3.a = t4.a ORDER BY t4.a", | ||
|
|
||
| // ============================================= | ||
| // Group 9: All inner join with more complex conditions | ||
| // ============================================= | ||
|
|
||
| // Inner join with multiple conditions | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a AND t1.b < t2.b JOIN t3 ON t2.a = t3.a ORDER BY t1.a", | ||
| // Inner join chain with different join columns | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.b = t3.b ORDER BY t1.a, t2.a, t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 10: Join-condition orientation and filters | ||
| // ============================================= | ||
|
|
||
| // Reversed equality orientation to exercise eq condition alignment. | ||
| "SELECT * FROM t1 JOIN t2 ON t2.a = t1.a JOIN t3 ON t3.a = t2.a ORDER BY t1.a", | ||
| // Inner join with one-side filter in ON condition. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a AND t2.b > 150 JOIN t3 ON t2.a = t3.a ORDER BY t1.a, t2.a", | ||
| // Degenerate predicate (single-side reference) + normal edge. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.b > 15 JOIN t3 ON t2.a = t3.a WHERE t1.a = 3 ORDER BY t2.a, t3.a", | ||
| // Cross-style inner join with join predicate in WHERE. | ||
| "SELECT * FROM t1 JOIN t2 JOIN t3 ON t2.a = t3.a WHERE t1.a = t2.a ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 11: Reorder barriers and fallback paths | ||
| // ============================================= | ||
|
|
||
| // Null-safe equality should stop join-group expansion. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a <=> t2.a JOIN t3 ON t2.a = t3.a ORDER BY t1.a", | ||
| // STRAIGHT_JOIN should preserve explicit order. | ||
| "SELECT * FROM t1 STRAIGHT_JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a ORDER BY t1.a", | ||
| // Non-equi LEFT JOIN should not be reordered as non-inner edge. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a < t2.a JOIN t3 ON t1.a = t3.a ORDER BY t1.a, t2.a, t3.a", | ||
| // Non-equi RIGHT JOIN should not be reordered as non-inner edge. | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a < t2.a JOIN t3 ON t2.a = t3.a ORDER BY t2.a, t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 12: Parenthesized/derived join groups | ||
| // ============================================= | ||
|
|
||
| // Explicit parenthesized mixed joins. | ||
| "SELECT * FROM (t1 LEFT JOIN t2 ON t1.a = t2.a) JOIN (t3 JOIN t4 ON t3.a = t4.a) ON t1.a = t3.a ORDER BY t1.a, t3.a", | ||
| // Right join over an explicit sub-join. | ||
| "SELECT * FROM t1 RIGHT JOIN (t2 LEFT JOIN t3 ON t2.a = t3.a) ON t1.a = t2.a ORDER BY t2.a, t1.a", | ||
| // Derived-table leafs inside join reorder. | ||
| "SELECT * FROM (SELECT a, b FROM t1 WHERE a <= 3) x JOIN (SELECT a, b FROM t2 WHERE a <= 2) y ON x.a = y.a JOIN t3 ON y.a = t3.a ORDER BY x.a", | ||
| // Mixed outer chain with one-side outer condition. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a AND t2.b > 150 LEFT JOIN t3 ON t1.a = t3.a JOIN t4 ON t3.a = t4.a ORDER BY t1.a, t4.a", | ||
|
|
||
| // ============================================= | ||
| // Group 13: 5-table joins | ||
| // ============================================= | ||
|
|
||
| // 5-table inner join chain. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a JOIN t4 ON t3.a = t4.a JOIN t5 ON t4.a = t5.a ORDER BY t1.a", | ||
| // 5-table mixed: INNER + LEFT + INNER + RIGHT. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a JOIN t4 ON t1.a = t4.a RIGHT JOIN t5 ON t4.a = t5.a ORDER BY t5.a", | ||
| // 5-table all LEFT JOINs. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a LEFT JOIN t4 ON t3.a = t4.a LEFT JOIN t5 ON t4.a = t5.a ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 14: Deep outer join chains (4-table) | ||
| // ============================================= | ||
|
|
||
| // 4 consecutive LEFT JOINs: assoc(left,left) exercised at each level. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a LEFT JOIN t4 ON t3.a = t4.a ORDER BY t1.a", | ||
| // 4 consecutive RIGHT JOINs: assoc(right,right) exercised at each level. | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a RIGHT JOIN t3 ON t2.a = t3.a RIGHT JOIN t4 ON t3.a = t4.a ORDER BY t4.a", | ||
|
|
||
| // ============================================= | ||
| // Group 15: Alternating join type patterns | ||
| // ============================================= | ||
|
|
||
| // LEFT-RIGHT-LEFT alternating: conflict rules from mixed joins interact. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a RIGHT JOIN t3 ON t2.a = t3.a LEFT JOIN t4 ON t3.a = t4.a ORDER BY t3.a", | ||
| // RIGHT-LEFT-RIGHT alternating: another direction of mixed chain. | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a RIGHT JOIN t4 ON t3.a = t4.a ORDER BY t4.a", | ||
|
|
||
| // ============================================= | ||
| // Group 16: WHERE clause interaction with outer joins | ||
| // ============================================= | ||
|
|
||
| // WHERE filter on preserved side of 3-table LEFT JOIN chain. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a WHERE t1.b > 10 ORDER BY t1.a", | ||
| // WHERE IS NULL filter: preserves outer join semantics (anti-join pattern). | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t1.a = t3.a WHERE t2.a IS NULL ORDER BY t1.a", | ||
| // WHERE on null-extended side may convert LEFT to INNER before reorder. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a JOIN t3 ON t1.a = t3.a WHERE t2.b > 100 ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 17: Self-joins | ||
| // ============================================= | ||
|
|
||
| // Self inner join: same table joined with itself. | ||
| "SELECT * FROM t1 x JOIN t1 y ON x.a = y.b ORDER BY x.a, y.a", | ||
| // Self left join + another table: CD-C must track distinct vertex IDs. | ||
| "SELECT * FROM t1 x LEFT JOIN t1 y ON x.a = y.b JOIN t2 ON x.a = t2.a ORDER BY x.a, t2.a", | ||
|
|
||
| // ============================================= | ||
| // Group 18: Compound ON conditions for non-inner edges | ||
| // ============================================= | ||
|
|
||
| // LEFT JOIN with multiple ON conditions + INNER. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a AND t1.b < t2.b JOIN t3 ON t1.a = t3.a ORDER BY t1.a, t3.a", | ||
| // RIGHT JOIN with multiple ON conditions + INNER. | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a AND t1.b > t2.b JOIN t3 ON t2.a = t3.a ORDER BY t2.a, t3.a", | ||
|
|
||
| // ============================================= | ||
| // Group 19: leftAsscom/rightAsscom = 0 specific cases | ||
| // ============================================= | ||
|
|
||
| // leftAsscom(right, left) = 0: (t1 RIGHT t2) LEFT t3 ON t1.a = t3.a. | ||
| // The LEFT join references the inner side of the RIGHT join. | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t1.a = t3.a ORDER BY t2.a", | ||
| // leftAsscom(right, inner) = 0: (t1 RIGHT t2) INNER t3 ON t1.a = t3.a. | ||
| // The INNER join references the inner side of the RIGHT join. | ||
| "SELECT * FROM t1 RIGHT JOIN t2 ON t1.a = t2.a JOIN t3 ON t1.a = t3.a ORDER BY t2.a, t3.a", | ||
| // rightAsscom(left, inner) = 0: t1 LEFT (t2 INNER t3). | ||
| // Achieved via t2 JOIN t3 ON t2.a = t3.a LEFT JOIN t1 ON t1.a = t2.a, then reversed. | ||
| "SELECT * FROM t2 JOIN t3 ON t2.a = t3.a LEFT JOIN t1 ON t1.a = t3.a ORDER BY t2.a", | ||
|
|
||
| // ============================================= | ||
| // Group 20: Aggregation and LIMIT on top of join reorder | ||
| // ============================================= | ||
|
|
||
| // GROUP BY after 3-table mixed join. | ||
| "SELECT t1.a, COUNT(*) FROM t1 JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.a = t3.a GROUP BY t1.a ORDER BY t1.a", | ||
| // LIMIT after 3-table inner join. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a ORDER BY t1.a LIMIT 2", | ||
|
|
||
| // ============================================= | ||
| // Group 21: Semi/Anti join as join-group barriers | ||
| // ============================================= | ||
|
|
||
| // EXISTS subquery becomes semi join, forms a barrier for join group expansion. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a WHERE EXISTS (SELECT 1 FROM t3 WHERE t3.a = t1.a) ORDER BY t1.a", | ||
| // NOT EXISTS subquery becomes anti join. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a WHERE NOT EXISTS (SELECT 1 FROM t3 WHERE t3.a = t1.a) ORDER BY t1.a", | ||
| // IN subquery becomes semi join. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a WHERE t1.a IN (SELECT a FROM t3) ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 22: Join with OR and complex predicates | ||
| // ============================================= | ||
|
|
||
| // Inner join with OR in WHERE. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a JOIN t3 ON t2.a = t3.a WHERE t1.b > 10 OR t2.b > 150 ORDER BY t1.a", | ||
| // Left join with compound predicate in ON. | ||
| "SELECT * FROM t1 LEFT JOIN t2 ON t1.a = t2.a AND (t2.b > 100 OR t2.b < 50) JOIN t3 ON t1.a = t3.a ORDER BY t1.a", | ||
|
|
||
| // ============================================= | ||
| // Group 23: Cross join columns (join on column b) | ||
| // ============================================= | ||
|
|
||
| // Join on column b instead of a. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.b = t2.b LEFT JOIN t3 ON t2.b = t3.b ORDER BY t1.a", | ||
| // Mixed columns: a for one edge, b for another. | ||
| "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a LEFT JOIN t3 ON t2.b = t3.b ORDER BY t1.a" | ||
| ] |
There was a problem hiding this comment.
Remove the inline comments from this JSON fixture.
This file is committed as .json, but the // ... section comments make it invalid JSON. Static analysis is already failing to parse it, so the fixture is no longer consumable by standard JSON tooling or CI parsers.
🧰 Tools
🪛 Biome (2.4.16)
[error] 5-5: Expected an array, an object, or a literal but instead found '// ============================================='.
(parse)
[error] 10-10: End of file expected
(parse)
[error] 10-11: End of file expected
(parse)
[error] 12-12: End of file expected
(parse)
[error] 12-18: End of file expected
(parse)
[error] 19-19: End of file expected
(parse)
[error] 19-21: End of file expected
(parse)
[error] 22-22: End of file expected
(parse)
[error] 22-24: End of file expected
(parse)
[error] 25-25: End of file expected
(parse)
[error] 25-31: End of file expected
(parse)
[error] 32-32: End of file expected
(parse)
[error] 32-34: End of file expected
(parse)
[error] 35-35: End of file expected
(parse)
[error] 35-41: End of file expected
(parse)
[error] 42-42: End of file expected
(parse)
[error] 42-43: End of file expected
(parse)
[error] 44-44: End of file expected
(parse)
[error] 44-50: End of file expected
(parse)
[error] 51-51: End of file expected
(parse)
[error] 51-57: End of file expected
(parse)
[error] 58-58: End of file expected
(parse)
[error] 58-60: End of file expected
(parse)
[error] 61-61: End of file expected
(parse)
[error] 61-74: End of file expected
(parse)
[error] 75-75: End of file expected
(parse)
[error] 75-81: End of file expected
(parse)
[error] 82-82: End of file expected
(parse)
[error] 82-83: End of file expected
(parse)
[error] 84-84: End of file expected
(parse)
[error] 84-85: End of file expected
(parse)
[error] 86-86: End of file expected
(parse)
[error] 86-92: End of file expected
(parse)
[error] 93-93: End of file expected
(parse)
[error] 93-94: End of file expected
(parse)
[error] 95-95: End of file expected
(parse)
[error] 95-101: End of file expected
(parse)
[error] 102-102: End of file expected
(parse)
[error] 102-103: End of file expected
(parse)
[error] 104-104: End of file expected
(parse)
[error] 104-105: End of file expected
(parse)
[error] 106-106: End of file expected
(parse)
[error] 106-107: End of file expected
(parse)
[error] 108-108: End of file expected
(parse)
[error] 108-114: End of file expected
(parse)
[error] 115-115: End of file expected
(parse)
[error] 115-116: End of file expected
(parse)
[error] 117-117: End of file expected
(parse)
[error] 117-118: End of file expected
(parse)
[error] 119-119: End of file expected
(parse)
[error] 119-120: End of file expected
(parse)
[error] 121-121: End of file expected
(parse)
[error] 121-127: End of file expected
(parse)
[error] 128-128: End of file expected
(parse)
[error] 128-129: End of file expected
(parse)
[error] 130-130: End of file expected
(parse)
[error] 130-131: End of file expected
(parse)
[error] 132-132: End of file expected
(parse)
[error] 132-133: End of file expected
(parse)
[error] 134-134: End of file expected
(parse)
[error] 134-140: End of file expected
(parse)
[error] 141-141: End of file expected
(parse)
[error] 141-142: End of file expected
(parse)
[error] 143-143: End of file expected
(parse)
[error] 143-144: End of file expected
(parse)
[error] 145-145: End of file expected
(parse)
[error] 145-151: End of file expected
(parse)
[error] 152-152: End of file expected
(parse)
[error] 152-153: End of file expected
(parse)
[error] 154-154: End of file expected
(parse)
[error] 154-160: End of file expected
(parse)
[error] 161-161: End of file expected
(parse)
[error] 161-162: End of file expected
(parse)
[error] 163-163: End of file expected
(parse)
[error] 163-169: End of file expected
(parse)
[error] 170-170: End of file expected
(parse)
[error] 170-171: End of file expected
(parse)
[error] 172-172: End of file expected
(parse)
[error] 172-173: End of file expected
(parse)
[error] 174-174: End of file expected
(parse)
[error] 174-180: End of file expected
(parse)
[error] 181-181: End of file expected
(parse)
[error] 181-182: End of file expected
(parse)
[error] 183-183: End of file expected
(parse)
[error] 183-189: End of file expected
(parse)
[error] 190-190: End of file expected
(parse)
[error] 190-191: End of file expected
(parse)
[error] 192-192: End of file expected
(parse)
[error] 192-199: End of file expected
(parse)
[error] 200-200: End of file expected
(parse)
[error] 200-202: End of file expected
(parse)
[error] 203-203: End of file expected
(parse)
[error] 203-205: End of file expected
(parse)
[error] 206-206: End of file expected
(parse)
[error] 206-212: End of file expected
(parse)
[error] 213-213: End of file expected
(parse)
[error] 213-214: End of file expected
(parse)
[error] 215-215: End of file expected
(parse)
[error] 215-221: End of file expected
(parse)
[error] 222-222: End of file expected
(parse)
[error] 222-223: End of file expected
(parse)
[error] 224-224: End of file expected
(parse)
[error] 224-225: End of file expected
(parse)
[error] 226-226: End of file expected
(parse)
[error] 226-232: End of file expected
(parse)
[error] 233-233: End of file expected
(parse)
[error] 233-234: End of file expected
(parse)
[error] 235-235: End of file expected
(parse)
[error] 235-241: End of file expected
(parse)
[error] 242-242: End of file expected
(parse)
[error] 242-243: End of file expected
(parse)
[error] 244-244: End of file expected
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_in.json`
around lines 5 - 245, The fixture contains JavaScript-style inline comments
(lines starting with "//", e.g. the group headings like "Group 1: Inner join
reorder (baseline)" and inline notes) which make the file invalid JSON; remove
all "// ..." comments throughout the file so only valid JSON remains (keep the
array of SQL string literals such as "SELECT * FROM t1 JOIN t2 ON t1.a = t2.a
..." and the surrounding JSON punctuation), ensuring no stray comment characters
remain and the file parses as strict JSON.
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
guo-shaoge
force-pushed
the
cp_post_cdc_impl
branch
from
e85f7ce to
4c94c2c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/casetest/rule/rule_cdc_join_reorder_test.go`:
- Around line 118-123: The test preallocates output to len(input) in
LoadTestCasesByName but the loop in rule_cdc_join_reorder_test.go skips "set "
statements and still uses planCaseIdx, causing mismatched counts and trailing
empty records; fix by changing the preallocation logic to use the number of
non-"set " SQLs (or simply build output with append) so planCaseIdx increments
and output length stay in sync: update LoadTestCasesByName (and the loop that
uses planCaseIdx) to either compute validSQLCount (skip "set " entries) before
preallocating or remove fixed preallocation and append to output when processing
each non-"set " SQL, keeping references to planCaseIdx, input, output and the
for-loop that checks strings.HasPrefix(normalized, "set ") consistent.
- Around line 59-69: Phase-1 may inadvertently run with the advanced join
reorder enabled, so explicitly pin the legacy reorder before collecting the
baseline: before building expectedResults (i.e., prior to iterating over input
and calling tk.MustQuery), call the session setter to disable advanced reorder
(use tk.MustExec to set @@tidb_opt_enable_advanced_join_reorder = 0), then
proceed to compute expectedResults via tk.MustQuery/ConvertRowsToStrings; keep
the later tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 1")
unchanged for Phase-2 validation.
In `@pkg/planner/core/joinorder/join_order.go`:
- Around line 90-93: The selection look-through currently only blocks
mutable-effects expressions; update the guard in the LogicalSelection path
(where p is type-asserted to logicalop.LogicalSelection and sel.Conditions is
inspected) to also reject non-deterministic predicates by checking
expression.CheckNonDeterministic for any condition, alongside the existing
expression.IsMutableEffectsExpr check and session flag
TiDBOptJoinReorderThroughSel, before calling extractJoinGroup on
sel.Children()[0]; ensure both checks are applied (e.g., use slices.ContainsFunc
against sel.Conditions with both predicate checks) so non-deterministic
expressions like RAND() prevent join reorder through the selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 78ea7dda-f6c2-45bb-a8fc-8ee32c4f1b9e
📒 Files selected for processing (18)
pkg/planner/core/casetest/binaryplan/testdata/binary_plan_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.jsonpkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.jsonpkg/planner/core/casetest/hint/testdata/integration_suite_out.jsonpkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/rule_cdc_join_reorder_test.gopkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_in.jsonpkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.jsonpkg/planner/core/casetest/rule/testdata/join_reorder_suite_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/joinorder/BUILD.bazelpkg/planner/core/joinorder/conflict_detector.gopkg/planner/core/joinorder/join_order.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/rule_join_reorder.gopkg/session/bootstraptest/BUILD.bazelpkg/util/intset/fast_int_set.go
💤 Files with no reviewable changes (1)
- pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.json
✅ Files skipped from review due to trivial changes (4)
- pkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.json
- pkg/planner/core/casetest/hint/testdata/integration_suite_out.json
- pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.json
- pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_in.json
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/planner/core/joinorder/BUILD.bazel
- pkg/planner/core/operator/logicalop/logical_join.go
- pkg/planner/core/joinorder/conflict_detector.go
| // Phase 1: Collect expected results using the old join reorder algorithm | ||
| // (CD-C is NOT enabled yet). These serve as the ground-truth baseline. | ||
| expectedResults := make([][]string, len(input)) | ||
| for i, sql := range input { | ||
| expectedResults[i] = testdata.ConvertRowsToStrings(tk.MustQuery(sql).Rows()) | ||
| } | ||
|
|
||
| // Phase 2: Enable CD-C algorithm, then verify both the plan and the | ||
| // result correctness for every case. | ||
| tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 1") | ||
|
|
There was a problem hiding this comment.
Pin Phase-1 to legacy reorder explicitly.
Phase-1 is documented as the “old join reorder algorithm,” but this PR’s objective enables advanced reorder by default. Without explicitly setting @@tidb_opt_enable_advanced_join_reorder = 0, the baseline can accidentally use CD-C too, making the cross-check non-effective.
Suggested patch
// Phase 1: Collect expected results using the old join reorder algorithm
// (CD-C is NOT enabled yet). These serve as the ground-truth baseline.
+ tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 0")
expectedResults := make([][]string, len(input))
for i, sql := range input {
expectedResults[i] = testdata.ConvertRowsToStrings(tk.MustQuery(sql).Rows())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Phase 1: Collect expected results using the old join reorder algorithm | |
| // (CD-C is NOT enabled yet). These serve as the ground-truth baseline. | |
| expectedResults := make([][]string, len(input)) | |
| for i, sql := range input { | |
| expectedResults[i] = testdata.ConvertRowsToStrings(tk.MustQuery(sql).Rows()) | |
| } | |
| // Phase 2: Enable CD-C algorithm, then verify both the plan and the | |
| // result correctness for every case. | |
| tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 1") | |
| // Phase 1: Collect expected results using the old join reorder algorithm | |
| // (CD-C is NOT enabled yet). These serve as the ground-truth baseline. | |
| tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 0") | |
| expectedResults := make([][]string, len(input)) | |
| for i, sql := range input { | |
| expectedResults[i] = testdata.ConvertRowsToStrings(tk.MustQuery(sql).Rows()) | |
| } | |
| // Phase 2: Enable CD-C algorithm, then verify both the plan and the | |
| // result correctness for every case. | |
| tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 1") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/casetest/rule/rule_cdc_join_reorder_test.go` around lines 59
- 69, Phase-1 may inadvertently run with the advanced join reorder enabled, so
explicitly pin the legacy reorder before collecting the baseline: before
building expectedResults (i.e., prior to iterating over input and calling
tk.MustQuery), call the session setter to disable advanced reorder (use
tk.MustExec to set @@tidb_opt_enable_advanced_join_reorder = 0), then proceed to
compute expectedResults via tk.MustQuery/ConvertRowsToStrings; keep the later
tk.MustExec("set @@tidb_opt_enable_advanced_join_reorder = 1") unchanged for
Phase-2 validation.
| planCaseIdx := 0 | ||
| for _, sql := range input { | ||
| normalized := strings.ToLower(strings.TrimSpace(sql)) | ||
| if strings.HasPrefix(normalized, "set ") { | ||
| tk.MustExec(sql) | ||
| continue |
There was a problem hiding this comment.
Record-mode case counting can fail when set statements are present.
LoadTestCasesByName preallocates output to len(input) in record mode, but this loop skips set statements. That makes planCaseIdx smaller than len(output), causing a false failure and leaving trailing empty recorded cases.
Suggested patch
- require.Equalf(t, len(output), planCaseIdx,
- "unexpected output case count, output=%d, actual explain cases=%d", len(output), planCaseIdx)
+ if testdata.Record() {
+ output = output[:planCaseIdx]
+ } else {
+ require.Equalf(t, len(output), planCaseIdx,
+ "unexpected output case count, output=%d, actual explain cases=%d", len(output), planCaseIdx)
+ }Also applies to: 145-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/casetest/rule/rule_cdc_join_reorder_test.go` around lines
118 - 123, The test preallocates output to len(input) in LoadTestCasesByName but
the loop in rule_cdc_join_reorder_test.go skips "set " statements and still uses
planCaseIdx, causing mismatched counts and trailing empty records; fix by
changing the preallocation logic to use the number of non-"set " SQLs (or simply
build output with append) so planCaseIdx increments and output length stay in
sync: update LoadTestCasesByName (and the loop that uses planCaseIdx) to either
compute validSQLCount (skip "set " entries) before preallocating or remove fixed
preallocation and append to output when processing each non-"set " SQL, keeping
references to planCaseIdx, input, output and the for-loop that checks
strings.HasPrefix(normalized, "set ") consistent.
| if sel, isSel := p.(*logicalop.LogicalSelection); isSel { | ||
| if p.SCtx().GetSessionVars().TiDBOptJoinReorderThroughSel && | ||
| !slices.ContainsFunc(sel.Conditions, expression.IsMutableEffectsExpr) { | ||
| childGroup := extractJoinGroup(sel.Children()[0]) |
There was a problem hiding this comment.
Guard Selection look-through against non-deterministic predicates
Line 92 only checks mutable-effects expressions. This allows join reorder through non-deterministic predicates (e.g. RAND()), which can change evaluation behavior/results. Please align this with the legacy path by also rejecting expression.CheckNonDeterministic(expr).
Suggested fix
- if sel, isSel := p.(*logicalop.LogicalSelection); isSel {
- if p.SCtx().GetSessionVars().TiDBOptJoinReorderThroughSel &&
- !slices.ContainsFunc(sel.Conditions, expression.IsMutableEffectsExpr) {
+ if sel, isSel := p.(*logicalop.LogicalSelection); isSel {
+ if p.SCtx().GetSessionVars().TiDBOptJoinReorderThroughSel &&
+ !slices.ContainsFunc(sel.Conditions, func(expr expression.Expression) bool {
+ return expression.IsMutableEffectsExpr(expr) || expression.CheckNonDeterministic(expr)
+ }) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/joinorder/join_order.go` around lines 90 - 93, The selection
look-through currently only blocks mutable-effects expressions; update the guard
in the LogicalSelection path (where p is type-asserted to
logicalop.LogicalSelection and sel.Conditions is inspected) to also reject
non-deterministic predicates by checking expression.CheckNonDeterministic for
any condition, alongside the existing expression.IsMutableEffectsExpr check and
session flag TiDBOptJoinReorderThroughSel, before calling extractJoinGroup on
sel.Children()[0]; ensure both checks are applied (e.g., use slices.ContainsFunc
against sel.Conditions with both predicate checks) so non-deterministic
expressions like RAND() prevent join reorder through the selection.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68880 +/- ##
================================================
Coverage ? 55.1127%
================================================
Files ? 1830
Lines ? 660706
Branches ? 0
================================================
Hits ? 364133
Misses ? 269365
Partials ? 27208
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
guo-shaoge
changed the title
|
/retest |
|
/test mysql-test |
|
@guo-shaoge: The specified target(s) for Use 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 |
|
/retest |
1 participant
What problem does this PR solve?
Issue Number: close #63887
Problem Summary: manually cherry pick #66349
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests
Bug Fixes / Behavior