planner: fix join reorder correctness with conflict detection algorithm | tidb-test=pr/2676 #65705

guo-shaoge · 2026-01-21T13:41:52Z

What problem does this PR solve?

Issue Number: close #63887

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

planner: fix join reorder correctness with conflict detection algorithm

Signed-off-by: guo-shaoge <[email protected]>

Copilot

Pull request overview

This is a work-in-progress pull request that introduces a new join reorder implementation to address correctness problems with outer join reordering. The PR adds a new joinorder package with conflict detection capabilities based on join reorder theory, while maintaining the existing join reorder logic as a fallback.

Changes:

Adds new join order optimizer with conflict detector for handling outer joins
Introduces conditional logic to enable the new implementation via session variable
Adds schema equality checking method for validation after reorder

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 24 comments.

Show a summary per file

File	Description
pkg/planner/core/rule_join_reorder.go	Adds conditional branching to use new join order implementation when EnableOuterJoinReorder is enabled
pkg/planner/core/joinorder/join_order.go	Implements new join order optimization with greedy algorithm, leading hint support, and join group extraction
pkg/planner/core/joinorder/conflict_detector.go	Implements conflict detection logic for join reordering with associativity and commutativity rules
pkg/planner/core/base/plan_base.go	Adds documentation comment noting that JoinType enum order must remain unchanged for conflict detector
pkg/expression/util.go	Adds TODO comment about refining the FilterOutInPlace function
pkg/expression/schema.go	Adds Equal method to Schema for comparing schema equality after join reorder

pkg/planner/core/joinorder/join_order.go

pkg/planner/core/joinorder/conflict_detector.go

Copilot · 2026-01-21T13:55:54Z

pkg/planner/core/joinorder/join_order.go

+				resNodes = append(resNodes, cartesianNodes[i])
+				break
+			}
+			newJoin, err := newCartesianJoin(ctx, cartesianNodes[i].p, cartesianNodes[i+1].p)


The function newCartesianJoin accepts a joinType parameter but is called without specifying the join type. The call should be newCartesianJoin(ctx, base.InnerJoin, left, right) to specify an inner join for Cartesian products.

Suggested change

newJoin, err := newCartesianJoin(ctx, cartesianNodes[i].p, cartesianNodes[i+1].p)

newJoin, err := newCartesianJoin(ctx, base.InnerJoin, cartesianNodes[i].p, cartesianNodes[i+1].p)

pkg/planner/core/rule_join_reorder.go

Copilot · 2026-01-21T13:55:55Z

pkg/planner/core/joinorder/join_order.go

+		}
+	}
+	// gjt todo set children?
+	if p, err = j.optimizeForJoinGroup(p.SCtx(), joinGroup); err != nil {


The variable j is used but not defined. The function optimizeRecursive calls j.optimizeForJoinGroup but j is not a receiver and is not declared anywhere in scope.

pkg/planner/core/joinorder/conflict_detector.go

Copilot · 2026-01-21T13:55:59Z

pkg/planner/core/joinorder/conflict_detector.go

+			selection.Conditions = append(selection.Conditions, e.eqConds...)
+			selection.Conditions = append(selection.Conditions, e.nonEQConds...)


The function attempts to convert expressions from []*expression.ScalarFunction to []expression.Expression by appending them directly. While ScalarFunction implements Expression, the type conversion in line 380 may cause issues. More critically, lines 380-381 should use type conversion or a helper function to properly handle the conversion.

Suggested change

selection.Conditions = append(selection.Conditions, e.eqConds...)

selection.Conditions = append(selection.Conditions, e.nonEQConds...)

selection.Conditions = append(selection.Conditions, expression.ScalarFuncs2Exprs(e.eqConds)...)

selection.Conditions = append(selection.Conditions, expression.ScalarFuncs2Exprs(e.nonEQConds)...)

pkg/planner/core/joinorder/conflict_detector.go

Copilot · 2026-01-21T13:55:59Z

pkg/planner/core/joinorder/conflict_detector.go

+		vertexMap[v.ID()] = &Node{
+			bitSet: newBitSet(int64(i)),
+			p:      v,
+		}


The cumCost field is not initialized when creating Nodes for vertexes. This field is used in the greedy join order algorithm to sort and compare nodes (line 276 in join_order.go), but leaf nodes will have cumCost of 0. The cumCost should be initialized based on the vertex plan's statistics or row count.

Suggested change

vertexMap[v.ID()] = &Node{

bitSet: newBitSet(int64(i)),

p: v,

}

node := &Node{

bitSet: newBitSet(int64(i)),

p: v,

}

if stats := v.StatsInfo(); stats != nil {

node.cumCost = stats.RowCount

}

vertexMap[v.ID()] = node

pkg/planner/core/joinorder/conflict_detector.go

winoros · 2026-01-22T01:42:07Z

pkg/planner/core/joinorder/conflict_detector.go

+	// It could be otherCond, leftCond or rightCond.
+	nonEQConds expression.CNFExprs
+
+	tes   BitSet


There's an imported bitset

winoros · 2026-01-22T01:44:22Z

pkg/planner/core/joinorder/conflict_detector.go

+// 0: rule doesn't apply
+// 1: rule applies
+// 2: rule applies when null-rejective holds
+type ruleTableEntry int


Why it's not bool

winoros · 2026-01-22T01:49:34Z

pkg/planner/core/joinorder/conflict_detector.go

+	return rule
+}
+
+func (d *ConflictDetector) calcTES(conds []base.Expression) BitSet {


add comments

winoros · 2026-01-22T01:52:01Z

pkg/planner/core/joinorder/conflict_detector.go

+		// Append selections to existing join
+		selection := logicalop.LogicalSelection{
+			Conditions: []expression.Expression{}, // gjt todo reserve space
+		}


When will we enter this? Why is it not in makeNonInnerJoin?

Signed-off-by: guo-shaoge <[email protected]>

hawkingrei · 2026-02-03T07:12:55Z

/retest

Signed-off-by: guo-shaoge <[email protected]>

hawkingrei · 2026-02-03T08:15:12Z

/retest

Signed-off-by: guo-shaoge <[email protected]>

hawkingrei · 2026-02-03T08:40:16Z

/retest

Signed-off-by: guo-shaoge <[email protected]>

tiprow · 2026-02-03T11:41:42Z

@guo-shaoge: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`b92650e`	link	true	`/test fast_test_tiprow`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

ti-chi-bot · 2026-02-03T12:52:56Z

@guo-shaoge: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-integration-ddl-test	`874eedc`	link	true	`/test pull-integration-ddl-test`
pull-unit-test-next-gen	`b92650e`	link	true	`/test pull-unit-test-next-gen`
idc-jenkins-ci-tidb/unit-test	`b92650e`	link	true	`/test unit-test`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

terry1purcell · 2026-02-03T13:46:32Z

@pantheon-bot please review

pantheon-ai · 2026-02-03T13:46:49Z

P2: Hard limit of 64 join vertices causes planning failure
- When a join group has >64 leaf vertices, the conflict detector returns an error that propagates to query planning, causing the query to fail instead of gracefully falling back.
- Evidence: pkg/planner/core/joinorder/conflict_detector.go:108 - if len(group.vertexes) > 64 { return nil, errors.Errorf("too many vertexes...") }
- Impact: Queries with very wide joins (>64 tables in one group) will fail with an error on the default path.
P2: DP join reorder support removed, threshold ignored
- The new join reorder implementation hardcodes greedy mode, making tidb_opt_join_reorder_threshold ineffective.
- Evidence: pkg/planner/core/joinorder/join_order.go:261 - useGreedy := true with DP path as panic("not implement yet")
- Impact: Configuration/feature parity gap for non-default deployments using threshold tuning.
P2: Session variable tidb_opt_join_reorder_through_sel silently ignored
- When outer join reorder is enabled (default), the code bypasses legacy join group extraction that honors this session variable.
- Evidence: pkg/planner/core/rule_join_reorder.go:248 - short-circuits to joinorder.Optimize which doesn't check the variable
- Impact: The session variable becomes a no-op under default configuration.
P2: alignEQConds can create invalid join keys
- The function has logic that may build equality functions with nil arguments and doesn't propagate injected projections back to join children.
- Evidence: pkg/planner/core/joinorder/conflict_detector.go:506 - declares var lCol, rCol expression.Expression but may not assign both before using
- Impact: Latent bug, likely unreachable with current planner invariants but represents faulty porting from legacy code.

winoros · 2026-02-03T13:41:58Z

pkg/planner/core/base/plan_base.go

 // JoinType contains CrossJoin, InnerJoin, LeftOuterJoin, RightOuterJoin, SemiJoin, AntiJoin.
 type JoinType int

+// NOTE: keep the order and value unchanged, because they are used in conflict_detector.go!!!


Add an intest.Assert instead.

winoros · 2026-02-03T13:43:08Z

pkg/planner/core/joinorder/conflict_detector.go

+	// It could be otherCond, leftCond or rightCond.
+	nonEQConds expression.CNFExprs
+
+	tes   intset.FastIntSet


use the imported bitset instead.

FastIntSet is used for sparse case.

go.mod: github.com/bits-and-blooms/bitset v1.14.3

winoros · 2026-02-03T13:45:03Z

pkg/planner/core/joinorder/join_order.go

+	return nodes, nil
+}
+
+// TODO add example


remember to modify this comment

winoros · 2026-02-03T13:46:30Z

pkg/executor/test/indexmergereadtest/index_merge_reader_test.go

 			where t1.c1 < 10 or t1.c2 < 10 for update`).Check(testkit.Rows(
 		"Projection 16635.64 root  test.t1.c1",
 		"└─SelectLock 16635.64 root  for update 0",
-		"  └─Projection 16635.64 root  test.t1.c1, test.t1._tidb_rowid, test.t1._tidb_tid, test.t2._tidb_rowid",


Why did this test change? Is this expected?

winoros · 2026-02-03T13:47:06Z

tests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.result

@@ -1956,7 +1950,7 @@ HashJoin	root		CARTESIAN inner join
        └─Selection	cop[tikv]		not(isnull(planner__core__casetest__rule__rule_join_reorder.t8.a))
          └─TableFullScan	cop[tikv]	table:t8	keep order:false, stats:pseudo
 Level	Code	Message
-Warning	1815	We can only use one leading hint at most, when multiple leading hints are used, all leading hints will be invalid


Is this change expected?

guo-shaoge added 5 commits January 5, 2026 14:39

tmp save

3b85d7c

Signed-off-by: guo-shaoge <[email protected]>

fix

bae4ef7

Signed-off-by: guo-shaoge <[email protected]>

fix

eb11e42

Signed-off-by: guo-shaoge <[email protected]>

tmp save(design done)

44a245f

Signed-off-by: guo-shaoge <[email protected]>

impl

57fa419

Signed-off-by: guo-shaoge <[email protected]>

Copilot AI review requested due to automatic review settings January 21, 2026 13:41

Copilot started reviewing on behalf of guo-shaoge January 21, 2026 13:42 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

winoros reviewed Jan 22, 2026

View reviewed changes

guo-shaoge added 9 commits January 23, 2026 11:00

fix

7e5c761

Signed-off-by: guo-shaoge <[email protected]>

refine

92269a6

Signed-off-by: guo-shaoge <[email protected]>

check usedEdges

3eb3a30

Signed-off-by: guo-shaoge <[email protected]>

check all edges used before finish

bef8026

Signed-off-by: guo-shaoge <[email protected]>

fix

e1e1352

Signed-off-by: guo-shaoge <[email protected]>

refine

61fc874

Signed-off-by: guo-shaoge <[email protected]>

refine replace children

af52953

Signed-off-by: guo-shaoge <[email protected]>

refine check all used edges

1d89f56

Signed-off-by: guo-shaoge <[email protected]>

refine

874eedc

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge changed the title ~~[WIP] join reorder correctness problem~~ [WIP] planner: join reorder correctness problem Jan 25, 2026

ti-chi-bot bot removed the do-not-merge/invalid-title label Jan 25, 2026

guo-shaoge added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2026

update

4bdcd27

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge added 2 commits February 3, 2026 14:44

bazel

3efff7b

Signed-off-by: guo-shaoge <[email protected]>

remove dup code injectExpr

2b50212

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge added 3 commits February 3, 2026 15:32

remove dup code(SetNewJoinMethodHint)

6dac2f6

Signed-off-by: guo-shaoge <[email protected]>

remove some todos

8d0f3f2

Signed-off-by: guo-shaoge <[email protected]>

refine

dcb1d6b

Signed-off-by: guo-shaoge <[email protected]>

comment

529a65d

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge force-pushed the cdc_impl branch from 0f5d8b0 to 529a65d Compare February 3, 2026 08:38

refine

284adfd

Signed-off-by: guo-shaoge <[email protected]>

guo-shaoge changed the title ~~[WIP] planner: fix join reorder correctness with CD-C conflict detection | tidb-test=pr/2676~~ planner: fix join reorder correctness with CD-C conflict detection | tidb-test=pr/2676 Feb 3, 2026

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2026

guo-shaoge changed the title ~~planner: fix join reorder correctness with CD-C conflict detection | tidb-test=pr/2676~~ planner: fix join reorder correctness with conflict detection algorithm | tidb-test=pr/2676 Feb 3, 2026

guo-shaoge added 5 commits February 3, 2026 19:15

remove dup code(hint related)

dd76b69

Signed-off-by: guo-shaoge <[email protected]>

Merge branch 'cdc_impl' of github.com:guo-shaoge/tidb into cdc_impl_bkp

a899f59

refine

f1d0bff

Signed-off-by: guo-shaoge <[email protected]>

milestone-1(ok but still duplicated hint func)

86e74fd

Signed-off-by: guo-shaoge <[email protected]>

Merge branch 'cdc_impl_bkp' of github.com:guo-shaoge/tidb into cdc_impl

b92650e

guo-shaoge requested a review from winoros February 3, 2026 13:39

winoros reviewed Feb 3, 2026

View reviewed changes

	newJoin, err := newCartesianJoin(ctx, cartesianNodes[i].p, cartesianNodes[i+1].p)
	newJoin, err := newCartesianJoin(ctx, base.InnerJoin, cartesianNodes[i].p, cartesianNodes[i+1].p)

		selection.Conditions = append(selection.Conditions, e.eqConds...)
		selection.Conditions = append(selection.Conditions, e.nonEQConds...)

-		vertexMap[v.ID()] = &Node{
-			bitSet: newBitSet(int64(i)),
-			p:      v,
-		}
+		node := &Node{
+			bitSet: newBitSet(int64(i)),
+			p:      v,
+		}
+		if stats := v.StatsInfo(); stats != nil {
+			node.cumCost = stats.RowCount
+		}
+		vertexMap[v.ID()] = node

planner: fix join reorder correctness with conflict detection algorithm | tidb-test=pr/2676 #65705

Are you sure you want to change the base?

planner: fix join reorder correctness with conflict detection algorithm | tidb-test=pr/2676 #65705

Conversation

guo-shaoge commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hawkingrei commented Feb 3, 2026

Uh oh!

hawkingrei commented Feb 3, 2026

Uh oh!

hawkingrei commented Feb 3, 2026

Uh oh!

tiprow bot commented Feb 3, 2026

Uh oh!

ti-chi-bot bot commented Feb 3, 2026

Uh oh!

terry1purcell commented Feb 3, 2026

Uh oh!

pantheon-ai bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

guo-shaoge commented Jan 21, 2026 •

edited

Loading

pantheon-ai bot commented Feb 3, 2026 •

edited

Loading