planner: enable cd-c algorithm by default for join reorder | tidb-test=pr/2676 (#66349)

[ti-chi-bot]

This is an automated cherry-pick of #66349

What problem does this PR solve?

Issue Number: close #63887

Problem Summary:

1. enable new join reorder impl by default
2. clean all the cases

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

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

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

None

Summary by CodeRabbit

• New Features

  • Added advanced join order optimization with enhanced conflict detection for complex queries, particularly improving outer join reordering and Cartesian join handling.

• Tests

  • Expanded test coverage for join optimization scenarios, including outer joins, semi joins, and multi-table reordering strategies.

[ti-chi-bot]

This cherry pick PR is for a release branch and has not yet been approved by triage owners.
Adding the do-not-merge/cherry-pick-not-approved label.

To merge this cherry pick:

1. It must be LGTMed and approved by the reviewers firstly.
2. For pull requests to TiDB-x branches, it must have no failed tests.
3. AFTER it has lgtm and approved labels, please wait for the cherry-pick merging approval from triage owners.

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.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign winoros for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS
• pkg/planner/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

@guo-shaoge This PR has conflicts, I have hold it.
Please resolve them or ask others to resolve them, then comment /unhold to remove the hold label.

[ti-chi-bot]

@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide.

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 ti-community-infra/tichi repository.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR implements a new join order optimizer based on the CD-C (Conflict Detector with Conflict Rules) algorithm for handling outer, semi, and anti joins. It adds a new joinorder package with conflict detection logic, integrates it into the planner's join reorder rule, and updates numerous test expectations to reflect changes in query plan generation under the new optimization strategy.

Changes

Cohort / File(s)	Summary
Join Order Optimizer Implementation pkg/planner/core/joinorder/BUILD.bazel, pkg/planner/core/joinorder/conflict_detector.go, pkg/planner/core/joinorder/join_order.go	New package implementing CD-C-based join reordering with conflict detection, edge conflict rules, cartesian-join handling, and greedy/bushy join tree assembly. Includes exported ConflictDetector type and Optimize function for plan optimization.
Join Reorder Rule Integration pkg/planner/core/rule_join_reorder.go	Updated join reorder entrypoint to conditionally route to new joinorder.Optimize() when TiDBOptEnableAdvancedJoinReorder is enabled; exports JoinReOrderSolver.Optimize() method and removes previous tracing logic.
Binary Plan Test Fixtures pkg/planner/core/casetest/binaryplan/testdata/binary_plan_suite_out.json, ...binary_plan_suite_xut.json	Updated and added binary plan expected outputs for explain analyze with new operator numbering and node IDs; includes merge conflict markers in one file.
CBO Test Fixtures pkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.json, ...analyze_suite_xut.json	Updated inequality ordering in join range expressions and added comprehensive CBO test suite with multiple test groups covering join planning, estimation, and optimization scenarios.
Join Reorder Test Fixtures pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.json, ...cdc_join_reorder_suite_xut.json, ...join_reorder_suite_out.json	Added and updated join reorder test suites for CDC-aware reordering and standard reordering, including multi-table join scenarios; includes unresolved merge markers.
Outer-to-Semi/Inner Join Test Fixtures pkg/planner/core/casetest/rule/testdata/outer2inner_out.json, ...outer2inner_xut.json, ...outer_to_semi_join_suite_out.json, ...outer_to_semi_join_suite_xut.json	Added new test suites for outer-to-inner and outer-to-semi join transformations with various predicate patterns and null-handling semantics.
JSON Plan Test Fixtures pkg/planner/core/casetest/testdata/json_plan_suite_out.json, ...json_plan_suite_xut.json, pkg/planner/core/casetest/hint/testdata/integration_suite_out.json, pkg/planner/core/casetest/mpp/testdata/integration_suite_out.json	Updated JSON plan outputs with new operator metadata, column ID projections, and execution info; includes merge conflict markers; removed expected warnings in one case.
TPC-DS Test Fixtures pkg/planner/core/casetest/tpcds/testdata/tpcds_suite_out.json, ...tpcds_suite_xut.json	Added new TPC-DS Q64 test suite fixtures with expected physical plan and result specifications.
Integration Test Results tests/integrationtest/r/planner/core/casetest/index/index.result, tests/integrationtest/r/planner/core/issuetest/planner_issue.result, tests/integrationtest/r/planner/core/join_reorder2.result, tests/integrationtest/r/planner/core/plan_cache.result, tests/integrationtest/r/planner/core/rule_join_reorder.result, tests/integrationtest/r/select.result, tests/integrationtest/r/tpch.result	Updated expected EXPLAIN outputs across multiple integration tests reflecting new plan shapes, join strategies, and operator numbering under the new join reorder optimization; several files contain unresolved merge conflict markers.

Sequence Diagram(s)

sequenceDiagram
    actor Planner
    participant JoinReorderRule as Join Reorder Rule
    participant JoinOrder as joinorder.Optimize()
    participant ConflictDetector as Conflict Detector
    participant JoinGroup as Join Group
    participant EdgeValidator as Edge Validator
    participant TreeBuilder as Tree Builder

    Planner->>JoinReorderRule: Optimize(plan)
    alt Advanced Reorder Enabled
        JoinReorderRule->>JoinOrder: Optimize(plan)
        JoinOrder->>ConflictDetector: Build(joinGroup)
        ConflictDetector->>JoinGroup: Extract vertexes, hints, edges
        ConflictDetector->>EdgeValidator: Compute conflict rules for edges
        JoinOrder->>TreeBuilder: optimizeForJoinGroup()
        TreeBuilder->>TreeBuilder: Greedy join enumeration
        TreeBuilder->>EdgeValidator: CheckConnection(node1, node2)
        EdgeValidator-->>TreeBuilder: Valid connection + rules satisfied?
        TreeBuilder->>ConflictDetector: MakeJoin(checkResult)
        ConflictDetector-->>TreeBuilder: New reordered join node
        TreeBuilder->>TreeBuilder: Build remaining bushy tree
        TreeBuilder-->>JoinOrder: Optimized join tree
        JoinOrder-->>JoinReorderRule: Reordered plan
    else Standard Reorder
        JoinReorderRule->>JoinReorderRule: optimizeRecursive(plan)
    end
    JoinReorderRule-->>Planner: Optimized plan

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

• planner: enable cd-c algorithm by default for join reorder | tidb-test=pr/2676 #66349: Enables the CD-C join reorder algorithm implementation that this PR builds upon, including foundational conflict detection and join graph construction logic.
• planner/core: disable new join reorder when DP is enabled #67079: Modifies the join-reorder entrypoint gating in rule_join_reorder.go to coordinate with DP threshold settings when enabling the new advanced reorder path.
• planner: align core casetests with plan_tree #67113: Updates planner core test fixtures by normalizing EXPLAIN output formats across many of the same testdata files modified here.

Suggested labels

cherry-pick-approved

Suggested reviewers

• guo-shaoge
• qw4990
• winoros
• AilinKid

Poem

🐰 A conflict detector hops through joins with care,
Reordering tables while preserving what's fair,
Semi and outer joins now find their place,
As vertices dance in the optimizer's space,
Each edge checked thrice before the bind—
Join order perfected, no bugs left behind! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 19.23% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The description is incomplete. It is missing detailed information about what changed and how it works. While it mentions the issue number and intent to enable the new join reorder implementation, it lacks technical details and explanation.	Add more detailed explanation of the changes made, implementation approach, and how the cd-c algorithm fix addresses the incorrect join reorder issue described in #63887.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: enabling the cd-c algorithm by default for join reorder, which is the primary purpose of this PR.
Linked Issues check	✅ Passed	The PR changes address the requirements of linked issue #63887 by implementing and enabling the cd-c algorithm for join reorder, which fixes the incorrect outer-join reordering behavior that was causing incorrect query results.
Out of Scope Changes check	✅ Passed	The PR contains only test data updates for the join reorder implementation and does not introduce unrelated changes outside the scope of enabling the cd-c algorithm for join reorder.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.3)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (3)

pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.json (1)

1208-1340: Add a leading() case that explicitly requires a cartesian edge.

TestJoinReorderPushSelection exercises leading(...), but none of these cases pins the special exception where a hint is allowed to force a cartesian join. That edge regressed before, so I’d add one positive hinted-cartesian case and one comparable non-hinted case to keep the gating behavior locked down.

Based on learnings: "In pkg/planner/core/joinorder/ (TiDB join reorder), cartesian edges should only be considered in two specific scenarios: 1. After the entire join reorder is complete and there are still disjoint join groups that need to be connected. 2. When a leading hint explicitly requires a cartesian join."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.json`
around lines 1208 - 1340, Add two cases to the TestJoinReorderPushSelection
suite: (1) a SQL with a leading(...) hint that forces a cartesian edge (so the
expected Plan shows a cartesian join/connect between disjoint join groups) and
(2) a comparable SQL without the leading hint that must NOT produce that
cartesian edge (expected Plan should show normal reorder without cartesian).
Locate the JSON array under "Cases" for TestJoinReorderPushSelection and add the
two entries with unique SQL strings and matching "Plan" arrays; ensure the
hinted case references the same table groups as the non-hinted case and that the
expected plan text explicitly indicates the cartesian/connect behavior so the
test locks the special-case allowing cartesian only when leading(...) forces it.

pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json (1)

3-5: Add a regression for cartesian-edge gating.

This suite still misses the case that previously regressed: disconnected join subgraphs with no leading(...) should only get a cartesian join in the final stitch, while a leading(...) case may require one earlier. Please lock that down here.

Based on learnings: planner rule changes should add targeted planner tests, and in pkg/planner/core/joinorder/ cartesian edges should only be considered after the entire join reorder is complete with disjoint groups, or when a leading hint explicitly requires a cartesian join.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json`
around lines 3 - 5, Add a targeted regression case to the TestCDCJoinReorder
suite that covers disconnected join subgraphs: create a test input where two
disjoint join groups are present with no leading(...) hints and assert that a
cartesian edge is only introduced in the final stitch (not during intermediate
reorder steps); also add a companion case where a leading(...) hint forces an
earlier cartesian join so the planner emits the cartesian edge earlier. Ensure
the new cases exercise the planner join reorder logic (joinorder/cartesian-edge
handling) and assert the presence/absence and position of cartesian joins
accordingly.

pkg/planner/core/casetest/testdata/json_plan_suite_xut.json (1)

101-162: Add one outer/semi/anti-join tidb_json case here.

The new explain analyze format = tidb_json coverage only exercises a plain inner join. Since this PR turns cd-c on by default for outer/semi/anti reorder, one golden from that family would make this suite hit the changed planner path instead of only a stable inner-join plan.

Based on learnings: For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.

Also applies to: 272-333

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/testdata/json_plan_suite_xut.json` around lines 101
- 162, Add a new test case object alongside the existing one that uses "SQL":
"explain analyze format = tidb_json ..." with an outer/semi/anti join (e.g., a
LEFT JOIN like "select * from t1 left join t2 on t1.id = t2.id" or a SEMI/ANTI
variant) so the suite exercises the CD-C outer/semi/anti reorder path; place it
as another top-level entry with its own "JSONPlan" stub/expected plan (mirroring
structure of the existing "JSONPlan" key) and ensure a corresponding expected
operator id (e.g., an outer-join operator id or semi/anti join id) is present so
the planner unit test will hit the changed planner path—repeat the same addition
in the other suite block noted by the reviewer.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/planner/core/casetest/testdata/json_plan_suite_xut.json`:
- Around line 1-4: The new json_plan_suite_xut.json fixture is not picked up
because LoadTestSuiteData in pkg/testkit/testdata/testdata.go only looks for
*_in.json/*_out.json pairs; either rename json_plan_suite_xut.json to follow the
existing *_in.json/*_out.json convention so it will be discovered, or update
LoadTestSuiteData (and any helper functions/matchers it uses) to also recognize
the _xut.json suffix by adding that pattern to the file discovery logic and
ensure it is parsed/paired the same way as the existing in/out files.

In `@pkg/planner/core/joinorder/join_order.go`:
- Around line 454-473: The greedy pass currently conflates allowing non-equi
joins and permitting cartesian stitching via allowNoEQ; change the API and call
sites so greedyConnectJoinNodes only controls non-equi edge usage (rename/keep
allowNonEq), and add a separate boolean or mode (e.g., allowCartesian) that is
NOT set in the normal greedy passes. Remove any calls inside
greedyConnectJoinNodes that invoke TryCreateCartesianCheckResult when
allowCartesian is false. Ensure cartesian creation is only invoked from
buildJoinByHint (when a leading hint explicitly requests it) and from the final
fallback (makeBushyTree) after the whole reorder if
detector.HasRemainingEdges(usedEdges) still indicates disconnected groups; keep
the cartesianFactor logic and clamping only in those cartesian-specific paths
and do not enable cartesian stitching in the intermediate retry where you only
want to enable non-equi joins (the second greedyConnectJoinNodes call should
pass allowCartesian=false but allowNonEq=true).

In `@tests/integrationtest/r/planner/core/casetest/index/index.result`:
- Around line 1-823: Missing pairing for the integration test result:
tests/integrationtest/r/planner/core/casetest/index/index.result has no
corresponding test input file; either add
tests/integrationtest/t/planner/core/casetest/index/index.test containing the
test SQLs that produced this result or confirm that the result should be merged
into an existing file such as
tests/integrationtest/t/planner/core/casetest/integration.test, then regenerate
the .result by running the framework's recording command so the result and test
file are in sync.

In `@tests/integrationtest/r/planner/core/issuetest/planner_issue.result`:
- Around line 668-679: The test's non-deterministic LIMIT usage (the SQL "select
* from t t1 left join t t2 on t1.a=t2.a limit 1, 1" and its corresponding
"explain format='plan_tree' ... limit 1, 1") must include an explicit ORDER BY
to guarantee a stable returned row; update both the query and the EXPLAIN query
to add a deterministic ordering key (for example "ORDER BY t1.a" or a stable
primary key/row id) so the expected row assertion is deterministic and matches
the recorded plan output.

---

Nitpick comments:
In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.json`:
- Around line 1208-1340: Add two cases to the TestJoinReorderPushSelection
suite: (1) a SQL with a leading(...) hint that forces a cartesian edge (so the
expected Plan shows a cartesian join/connect between disjoint join groups) and
(2) a comparable SQL without the leading hint that must NOT produce that
cartesian edge (expected Plan should show normal reorder without cartesian).
Locate the JSON array under "Cases" for TestJoinReorderPushSelection and add the
two entries with unique SQL strings and matching "Plan" arrays; ensure the
hinted case references the same table groups as the non-hinted case and that the
expected plan text explicitly indicates the cartesian/connect behavior so the
test locks the special-case allowing cartesian only when leading(...) forces it.

In `@pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json`:
- Around line 3-5: Add a targeted regression case to the TestCDCJoinReorder
suite that covers disconnected join subgraphs: create a test input where two
disjoint join groups are present with no leading(...) hints and assert that a
cartesian edge is only introduced in the final stitch (not during intermediate
reorder steps); also add a companion case where a leading(...) hint forces an
earlier cartesian join so the planner emits the cartesian edge earlier. Ensure
the new cases exercise the planner join reorder logic (joinorder/cartesian-edge
handling) and assert the presence/absence and position of cartesian joins
accordingly.

In `@pkg/planner/core/casetest/testdata/json_plan_suite_xut.json`:
- Around line 101-162: Add a new test case object alongside the existing one
that uses "SQL": "explain analyze format = tidb_json ..." with an
outer/semi/anti join (e.g., a LEFT JOIN like "select * from t1 left join t2 on
t1.id = t2.id" or a SEMI/ANTI variant) so the suite exercises the CD-C
outer/semi/anti reorder path; place it as another top-level entry with its own
"JSONPlan" stub/expected plan (mirroring structure of the existing "JSONPlan"
key) and ensure a corresponding expected operator id (e.g., an outer-join
operator id or semi/anti join id) is present so the planner unit test will hit
the changed planner path—repeat the same addition in the other suite block noted
by the reviewer.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 915dd777-f94c-43ec-8892-9dc6ff0427fa

📥 Commits

Reviewing files that changed from the base of the PR and between edcf353 and 3d6e2eb.

📒 Files selected for processing (34)

• pkg/planner/core/casetest/binaryplan/testdata/binary_plan_suite_out.json
• pkg/planner/core/casetest/binaryplan/testdata/binary_plan_suite_xut.json
• pkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.json
• pkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.json
• pkg/planner/core/casetest/hint/testdata/integration_suite_out.json
• pkg/planner/core/casetest/hint/testdata/integration_suite_xut.json
• pkg/planner/core/casetest/mpp/testdata/integration_suite_out.json
• pkg/planner/core/casetest/mpp/testdata/integration_suite_xut.json
• pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_out.json
• pkg/planner/core/casetest/rule/testdata/cdc_join_reorder_suite_xut.json
• pkg/planner/core/casetest/rule/testdata/join_reorder_suite_out.json
• pkg/planner/core/casetest/rule/testdata/join_reorder_suite_xut.json
• pkg/planner/core/casetest/rule/testdata/outer2inner_out.json
• pkg/planner/core/casetest/rule/testdata/outer2inner_xut.json
• pkg/planner/core/casetest/rule/testdata/outer_to_semi_join_suite_out.json
• pkg/planner/core/casetest/rule/testdata/outer_to_semi_join_suite_xut.json
• pkg/planner/core/casetest/testdata/json_plan_suite_out.json
• pkg/planner/core/casetest/testdata/json_plan_suite_xut.json
• pkg/planner/core/casetest/tpcds/testdata/tpcds_suite_out.json
• pkg/planner/core/casetest/tpcds/testdata/tpcds_suite_xut.json
• pkg/planner/core/casetest/tpch/testdata/tpch_suite_out.json
• pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json
• pkg/planner/core/joinorder/BUILD.bazel
• pkg/planner/core/joinorder/conflict_detector.go
• pkg/planner/core/joinorder/join_order.go
• pkg/planner/core/rule_join_reorder.go
• tests/integrationtest/r/planner/core/casetest/index/index.result
• tests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.result
• tests/integrationtest/r/planner/core/issuetest/planner_issue.result
• tests/integrationtest/r/planner/core/join_reorder2.result
• tests/integrationtest/r/planner/core/plan_cache.result
• tests/integrationtest/r/planner/core/rule_join_reorder.result
• tests/integrationtest/r/select.result
• tests/integrationtest/r/tpch.result

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "New xut fixtures under pkg/planner:"
fd '_xut\.json$' pkg/planner

echo
echo "Direct references to the new suffix or exact filenames:"
rg -n --glob '*.go' 'json_plan_suite_xut|binary_plan_suite_xut|_xut\.json|xut'

echo
echo "Planner casetest fixture-loading code paths:"
rg -n --glob '*.go' 'testdata|_out\.json|Load.*json|Read.*json' pkg/planner

Repository: pingcap/tidb

Length of output: 877

---

🏁 Script executed:

# Find the test runner/loader code in casetest packages
find pkg/planner/core/casetest -name "*test.go" -type f | head -5

# Look for the main test file that loads these fixtures
find pkg/planner/core/casetest -name "casetest_test.go" -o -name "*_test.go" | grep -E "(json_plan|casetest)" | head -10

Repository: pingcap/tidb

Length of output: 809

---

🏁 Script executed:

# Search for file discovery patterns and glob expressions in casetest
rg -n -A5 -B5 'json_plan_suite|testdata' pkg/planner/core/casetest --glob '*.go' | head -100

Repository: pingcap/tidb

Length of output: 9057

---

🏁 Script executed:

# Check the json plan test file specifically
find pkg/planner/core/casetest -name "*json*" -type f | grep -E '\.go$'

Repository: pingcap/tidb

Length of output: 38

---

🏁 Script executed:

# Look for any references to file patterns like *_out.json or fixture loading
rg -n '\*_out\.json|\*\.json|filepath.Glob|ioutil.ReadDir' pkg/planner/core/casetest --glob '*.go' -A2

Repository: pingcap/tidb

Length of output: 38

---

🏁 Script executed:

# Find the json plan test file and its suite definition
find pkg/planner/core/casetest -name "*json_plan*" -type f

Repository: pingcap/tidb

Length of output: 234

---

🏁 Script executed:

# Search for GetJSONPlanSuiteData or similar suite getters
rg -n "GetJSONPlanSuiteData|JSONPlanSuite|json_plan" pkg/planner/core/casetest --glob '*.go'

Repository: pingcap/tidb

Length of output: 414

---

🏁 Script executed:

# Look for suite_test.go files that typically define loaders
find pkg/planner/core/casetest -name "suite_test.go" -o -name "*suite*.go" | grep -v '.json'

Repository: pingcap/tidb

Length of output: 38

---

🏁 Script executed:

# Examine main_test.go to see how LoadTestSuiteData works and understand the loader pattern
cat -n pkg/planner/core/casetest/main_test.go | head -100

Repository: pingcap/tidb

Length of output: 2772

---

🏁 Script executed:

# Check plan_test.go to see how it uses the suite data
sed -n '250,270p' pkg/planner/core/casetest/plan_test.go

Repository: pingcap/tidb

Length of output: 853

---

🏁 Script executed:

# Search for LoadTestSuiteData implementation
rg -n "LoadTestSuiteData" --glob '*.go' -A10 | head -80

Repository: pingcap/tidb

Length of output: 6247

---

🏁 Script executed:

# Look at the loadTestSuiteData implementation
sed -n '100,250p' pkg/testkit/testdata/testdata.go

Repository: pingcap/tidb

Length of output: 4558

---

🏁 Script executed:

# Also search for how the suite data discovery works - looking for file pattern matching
rg -n "loadTestSuiteData|_in\.json|_out\.json|_xut\.json" pkg/testkit/testdata --glob '*.go' -B2 -A5 | head -100

Repository: pingcap/tidb

Length of output: 2391

---

The _xut.json files are not loaded by the test framework and will not execute.

The testdata loader in pkg/testkit/testdata/testdata.go (lines 59–70) only discovers *_in.json and *_out.json file pairs via LoadTestSuiteData(dir, suiteName). It does not load _xut.json files. The new json_plan_suite_xut.json fixture exists on disk but is orphaned and will never run. Either rename the files to match the expected _in.json/_out.json naming convention or extend the loader to support the _xut.json suffix.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/casetest/testdata/json_plan_suite_xut.json` around lines 1 -
4, The new json_plan_suite_xut.json fixture is not picked up because
LoadTestSuiteData in pkg/testkit/testdata/testdata.go only looks for
*_in.json/*_out.json pairs; either rename json_plan_suite_xut.json to follow the
existing *_in.json/*_out.json convention so it will be discovered, or update
LoadTestSuiteData (and any helper functions/matchers it uses) to also recognize
the _xut.json suffix by adding that pattern to the file discovery logic and
ensure it is parsed/paired the same way as the existing in/out files.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Keep cartesian joins out of the greedy passes.

allowNoEQ is doing double duty here: once it is true, greedyConnectJoinNodes() can both accept non-equi edges and call TryCreateCartesianCheckResult() on disconnected subgraphs. That lets the main pass stitch disjoint groups together early, and the retry path does the same even when CartesianJoinOrderThreshold <= 0. Please split those controls and reserve cartesian creation for buildJoinByHint() and the final makeBushyTree() fallback.

Based on learnings: in pkg/planner/core/joinorder/, cartesian edges should only be considered in two specific scenarios — after the entire join reorder is complete and there are still disjoint join groups that need to be connected, or when a leading hint explicitly requires a cartesian join.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/planner/core/joinorder/join_order.go` around lines 454 - 473, The greedy
pass currently conflates allowing non-equi joins and permitting cartesian
stitching via allowNoEQ; change the API and call sites so greedyConnectJoinNodes
only controls non-equi edge usage (rename/keep allowNonEq), and add a separate
boolean or mode (e.g., allowCartesian) that is NOT set in the normal greedy
passes. Remove any calls inside greedyConnectJoinNodes that invoke
TryCreateCartesianCheckResult when allowCartesian is false. Ensure cartesian
creation is only invoked from buildJoinByHint (when a leading hint explicitly
requests it) and from the final fallback (makeBushyTree) after the whole reorder
if detector.HasRemainingEdges(usedEdges) still indicates disconnected groups;
keep the cartesianFactor logic and clamping only in those cartesian-specific
paths and do not enable cartesian stitching in the intermediate retry where you
only want to enable non-equi joins (the second greedyConnectJoinNodes call
should pass allowCartesian=false but allowNonEq=true).

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the corresponding .t test file exists
fd -t f 'index.test' tests/integrationtest/t/planner/core/casetest/index/

# Check if there's documentation about how to record integration tests
if [ -f "docs/agents/testing-flow.md" ]; then
  echo "=== Recording instructions from testing-flow.md ==="
  rg -A 10 -i "integration test" docs/agents/testing-flow.md || echo "No integration test section found"
fi

# Verify the result file line count matches expected structure
echo "=== Result file statistics ==="
wc -l tests/integrationtest/r/planner/core/casetest/index/index.result

Repository: pingcap/tidb

Length of output: 312

---

🏁 Script executed:

# Find the actual structure of integrationtest directories
fd -t d 'integrationtest' . | head -20

# Look for the index test file with broader search
find . -name '*index*' -path '*/integrationtest/*' -type f 2>/dev/null | grep -E '\.(t|result)$' | head -20

# Check for the docs directory and testing guidance
ls -la docs/ 2>/dev/null | head -20

# Look for any testing documentation
find . -name '*testing*' -o -name '*TESTING*' 2>/dev/null | grep -v '.git' | head -20

Repository: pingcap/tidb

Length of output: 1280

---

🏁 Script executed:

# Find .t test files (test input files)
find tests/integrationtest/t -name '*.t' -o -name '*.test' 2>/dev/null | head -20

# Look for agents documentation referenced in learnings
find . -path '*/.agents/*' -o -path '*/agents/*' 2>/dev/null | grep -i test | head -20

# Search for the specific testing documentation mentioned in learnings
find . -name 'testing-flow.md' -o -name '*testing*flow*' 2>/dev/null | head -10

# Check if there's a .agents/skills directory
ls -la .agents/skills/ 2>/dev/null || echo "No .agents/skills found"

# List top-level test files in integrationtest
ls -la tests/integrationtest/t/ 2>/dev/null | head -30

Repository: pingcap/tidb

Length of output: 3075

---

🏁 Script executed:

# Search for index.test file in planner/core/casetest directories
find tests/integrationtest/t -path '*planner*' -name '*.test' 2>/dev/null | head -20

# List contents of planner directory if it exists
ls -la tests/integrationtest/t/planner/ 2>/dev/null || echo "No planner directory"

# Search for any documentation about integration tests
rg -l "integration.*test" docs/ 2>/dev/null | head -10

# Look for README or instructions in integrationtest directory
ls -la tests/integrationtest/ 2>/dev/null

# Check if there's any CI/workflow documentation that mentions recording
find . -name '*.md' -path '*docs*' 2>/dev/null | xargs grep -l "integration.*test" 2>/dev/null | head -10

Repository: pingcap/tidb

Length of output: 3210

---

🏁 Script executed:

# Check the casetest directory structure more carefully
ls -la tests/integrationtest/t/planner/core/casetest/ 2>/dev/null

# Look for any integration test that might contain index tests
find tests/integrationtest/t -name '*index*' 2>/dev/null

# Check if the result file directory has a corresponding test file
ls -la tests/integrationtest/r/planner/core/casetest/ 2>/dev/null

# Look at README for integration test instructions
cat tests/integrationtest/README.md 2>/dev/null

# Check the run-tests.sh script to understand test execution
head -100 tests/integrationtest/run-tests.sh

Repository: pingcap/tidb

Length of output: 7387

---

🏁 Script executed:

# Check what's in the result index directory
ls -la tests/integrationtest/r/planner/core/casetest/index/ 2>/dev/null

# Search for any test file containing index-related tests in casetest
find tests/integrationtest/t/planner/core/casetest -name '*.test' -exec grep -l "index\|INDEX" {} \; 2>/dev/null | head -10

# Check if index test file exists elsewhere
find tests/integrationtest -name 'index.test' 2>/dev/null

# Look at the integration.test file to see if it contains index tests
head -50 tests/integrationtest/t/planner/core/casetest/integration.test

# Check if there's a planner/core/index subdirectory
find tests/integrationtest/t/planner/core -type d -name 'index*' 2>/dev/null

Repository: pingcap/tidb

Length of output: 3361

---

🏁 Script executed:

# Verify: the result file exists but check if there's a corresponding .test file
echo "=== Checking for index.test file ==="
[ -f tests/integrationtest/t/planner/core/casetest/index/index.test ] && echo "FOUND: index.test" || echo "NOT FOUND: index.test"

# List all files in both directories
echo ""
echo "=== Files in t/planner/core/casetest/index/ ==="
ls -la tests/integrationtest/t/planner/core/casetest/index/ 2>/dev/null || echo "Directory does not exist"

echo ""
echo "=== Files in r/planner/core/casetest/index/ ==="
ls -la tests/integrationtest/r/planner/core/casetest/index/ 2>/dev/null

# Check git status to see if this is a new file
echo ""
echo "=== Git status of the result file ==="
cd tests/integrationtest && git status r/planner/core/casetest/index/index.result 2>/dev/null || echo "Not in git or no git info"

Repository: pingcap/tidb

Length of output: 519

---

🏁 Script executed:

# Double-check: search for any .test file that might generate this index.result
rg -l "planner.*index" tests/integrationtest/t --type test | head -20

# Check what the README says about the structure
echo "=== Test file discovery ==="
find tests/integrationtest/t -name '*.test' | wc -l
echo "Test files found. Now checking result files:"
find tests/integrationtest/r -name '*.result' | wc -l
echo "Result files found."

# Verify if there's any pairing of test and result files with different paths
echo ""
echo "=== Checking result files without test counterparts ==="
for result in $(find tests/integrationtest/r -name '*.result' -type f); do
  testfile="${result/\/r\//\/t\/}"
  testfile="${testfile/\.result/.test}"
  if [ ! -f "$testfile" ]; then
    echo "ORPHAN RESULT: $result (no test file at $testfile)"
  fi
done | head -20

Repository: pingcap/tidb

Length of output: 1978

---

🏁 Script executed:

# Verify the structure - check how other result files map to test files
echo "=== Sample of properly paired test/result files ==="
ls tests/integrationtest/t/explain*.test | head -5 | while read f; do
  result="${f/\/t\//\/r\/}"
  result="${result/\.test/.result}"
  echo "Test: $f"
  echo "Result: $result"
  [ -f "$result" ] && echo "✓ Result exists" || echo "✗ Result missing"
  echo ""
done

# Specifically check if index.result should be paired with a test file
echo ""
echo "=== Files in integration test directories ==="
echo "Test count in planner/core/casetest:"
find tests/integrationtest/t/planner/core/casetest -name '*.test' | wc -l
echo ""
echo "Result count in planner/core/casetest:"
find tests/integrationtest/r/planner/core/casetest -name '*.result' | wc -l

# Get file size to understand scale
echo ""
echo "=== File size comparison ==="
echo "index.result size:"
wc -c tests/integrationtest/r/planner/core/casetest/index/index.result

# Check integration.test which seems to be the main test file
echo ""
echo "=== Checking integration.test ==="
wc -l tests/integrationtest/t/planner/core/casetest/integration.test
echo ""
echo "First lines of integration.test:"
head -5 tests/integrationtest/t/planner/core/casetest/integration.test

Repository: pingcap/tidb

Length of output: 1240

---

🏁 Script executed:

# Critical: check if there's a test file that should be generating this result
echo "=== Looking for test files that might generate index.result ==="
rg -l "index" tests/integrationtest/t/planner/core/casetest/*.test 2>/dev/null

# Check the integration.test to see if it contains index-related tests
echo ""
echo "=== Searching for index-related tests in integration.test ==="
rg -c "index|INDEX" tests/integrationtest/t/planner/core/casetest/integration.test

# Check the result files to see which test file they correspond to
echo ""
echo "=== List of result files in casetest ==="
ls -1 tests/integrationtest/r/planner/core/casetest/*.result 2>/dev/null

# Check size of integration.result
echo ""
echo "=== Comparing sizes ==="
echo "integration.result:"
wc -c tests/integrationtest/r/planner/core/casetest/integration.result
echo ""
echo "index.result:"
wc -c tests/integrationtest/r/planner/core/casetest/index/index.result

Repository: pingcap/tidb

Length of output: 919

---

🏁 Script executed:

# Critical verification: check if there's any indication of where this should come from
echo "=== Checking git log for this file ==="
cd tests/integrationtest && git log --oneline -20 -- r/planner/core/casetest/index/index.result 2>/dev/null || echo "No recent changes"

# Check for any test setup or configuration that might generate this
echo ""
echo "=== Searching for 'index' test references in the codebase ==="
rg "index\.result|index\.test" tests/integrationtest/ --type-list 2>/dev/null | head -20

# Look at the first few lines of the orphaned result to understand its source
echo ""
echo "=== First 30 lines of index.result ==="
head -30 tests/integrationtest/r/planner/core/casetest/index/index.result

Repository: pingcap/tidb

Length of output: 390

---

🏁 Script executed:

# Trace back: check the git commit that added this file
echo "=== Git commit details ==="
cd tests/integrationtest && git show --stat 3d6e2ebc 2>/dev/null | head -50

# Look for PR reference
echo ""
echo "=== Commit message ==="
cd tests/integrationtest && git log -1 --format="%B" 3d6e2ebc 2>/dev/null

Repository: pingcap/tidb

Length of output: 3043

---

🏁 Script executed:

# Go back to repo root and check the file directly
echo "=== Current directory ==="
pwd

echo ""
echo "=== Checking if index.result exists ==="
ls -la tests/integrationtest/r/planner/core/casetest/index/index.result 2>/dev/null && echo "FILE EXISTS" || echo "FILE NOT FOUND"

# Try to read the file from repo root
echo ""
echo "=== First 30 lines of index.result ==="
head -30 tests/integrationtest/r/planner/core/casetest/index/index.result 2>/dev/null || echo "Cannot read file"

# Check the git log from repo root
echo ""
echo "=== Git log for index.result ==="
git log --oneline -5 -- tests/integrationtest/r/planner/core/casetest/index/index.result 2>/dev/null

# Check if this is part of the PR being reviewed
echo ""
echo "=== Current HEAD ==="
git log --oneline -1

Repository: pingcap/tidb

Length of output: 3391

---

🏁 Script executed:

# Final verification: check if there's supposed to be a test file
echo "=== Checking PR `#66349` reference ==="
git log --all --grep="66349" --oneline | head -5

echo ""
echo "=== Verify file pairing requirement ==="
# The README says test files in t/ and result files in r/ should be paired
# Check the integration.test size and structure
echo "Integration.test lines:"
wc -l tests/integrationtest/t/planner/core/casetest/integration.test
echo ""
echo "Integration.result lines:"
wc -l tests/integrationtest/r/planner/core/casetest/integration.result

echo ""
echo "=== Checking if index.result content is in integration.test ==="
# Search if the content of index.result is actually in integration.test
rg "drop table if exists t1, t2;" tests/integrationtest/t/planner/core/casetest/integration.test

echo ""
echo "=== Alternative: check if this should be in a different test file ==="
rg "TIDB_INLJ\|IndexMerge" tests/integrationtest/t/planner/core/casetest/*.test | cut -d: -f1 | sort -u

Repository: pingcap/tidb

Length of output: 615

---

🏁 Script executed:

# Final check: verify the actual issue
echo "=== Summary of findings ==="
echo ""
echo "1. Result file exists:"
[ -f tests/integrationtest/r/planner/core/casetest/index/index.result ] && echo "   ✓ tests/integrationtest/r/planner/core/casetest/index/index.result (74352 bytes)"

echo ""
echo "2. Test file exists:"
[ -f tests/integrationtest/t/planner/core/casetest/index/index.test ] && echo "   ✓ tests/integrationtest/t/planner/core/casetest/index/index.test" || echo "   ✗ tests/integrationtest/t/planner/core/casetest/index/index.test NOT FOUND"

echo ""
echo "3. Test/result pairing:"
echo "   Expected structure: t/<name>.test → r/<name>.result"
echo "   Actual for index: No test file found"

echo ""
echo "4. File origin (from git):"
git show 3d6e2ebc:tests/integrationtest/r/planner/core/casetest/index/index.result 2>/dev/null | wc -l
echo "   Lines in commit 3d6e2ebc: $(git show 3d6e2ebc:tests/integrationtest/r/planner/core/casetest/index/index.result 2>/dev/null | wc -l)"

echo ""
echo "5. PR context:"
git log --all --oneline | grep -i "66349" | head -1

Repository: pingcap/tidb

Length of output: 544

---

Missing test file for integration test result.

The result file tests/integrationtest/r/planner/core/casetest/index/index.result (823 lines) has no corresponding test file at tests/integrationtest/t/planner/core/casetest/index/index.test. Integration test result files must be paired with test files to be regenerated via the recording command per the framework's expectations. Add the missing test file or verify this result should be incorporated into an existing test file like tests/integrationtest/t/planner/core/casetest/integration.test.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integrationtest/r/planner/core/casetest/index/index.result` around
lines 1 - 823, Missing pairing for the integration test result:
tests/integrationtest/r/planner/core/casetest/index/index.result has no
corresponding test input file; either add
tests/integrationtest/t/planner/core/casetest/index/index.test containing the
test SQLs that produced this result or confirm that the result should be merged
into an existing file such as
tests/integrationtest/t/planner/core/casetest/integration.test, then regenerate
the .result by running the framework's recording command so the result and test
file are in sync.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Make this LIMIT assertion deterministic.

LIMIT 1, 1 without ORDER BY does not guarantee which row is returned, and the adjacent plan_tree already shows keep order:false. As written, this result can flap when planner/executor behavior changes. Please add an explicit ordering key before recording the expected row.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/integrationtest/r/planner/core/issuetest/planner_issue.result` around
lines 668 - 679, The test's non-deterministic LIMIT usage (the SQL "select *
from t t1 left join t t2 on t1.a=t2.a limit 1, 1" and its corresponding "explain
format='plan_tree' ... limit 1, 1") must include an explicit ORDER BY to
guarantee a stable returned row; update both the query and the EXPLAIN query to
add a deterministic ordering key (for example "ORDER BY t1.a" or a stable
primary key/row id) so the expected row assertion is deterministic and matches
the recorded plan output.

[ti-chi-bot]

@ti-chi-bot: 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
idc-jenkins-ci-tidb/build	3d6e2eb	link	true	/test build
idc-jenkins-ci-tidb/check_dev	3d6e2eb	link	true	/test check-dev
idc-jenkins-ci-tidb/check_dev_2	3d6e2eb	link	true	/test check-dev2
idc-jenkins-ci-tidb/unit-test	3d6e2eb	link	true	/test unit-test
idc-jenkins-ci-tidb/mysql-test	3d6e2eb	link	true	/test mysql-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.