planner: cherry-pick multi alternative framework and fts like rewrite back to fts branch | tidb-test=42c2474b6e2e1ef3430d07a1743ca7e17fd97acc tiflash=feature-fts tikv=feature-fts#68383
Merged
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Hi @AilinKid. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
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:
📝 WalkthroughWalkthroughAdds correlate-solver and multi-round optimize flow, fixes CommonHandle ORDER BY and correlated-range access, refactors per-build QB hint state and builder plumbing, implements ordered ParallelApply with reorder/backpressure, adds StatementContext/PlanCache snapshot/restore, and updates tests/BUILD/golden files. ChangesPlanner + Hint-state + Executor
✨ Finishing Touches🧪 Generate unit tests (beta)
|
ti-chi-bot
Bot
added
size/XXL
AilinKid
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go (1)
132-135: ⚡ Quick winStrengthen negative-plan assertions to avoid false positives.
These checks currently allow a plan with neither
keep order:truenorTopN/Sort, which weakens regression detection for ORDER BY correctness. For negative cases, assertTopN/Sortexplicitly.Proposed tightening of assertions
- hasKeepOrderTrue := explainHas(rows, "keep order:true") - hasSortOp := explainHas(rows, "TopN") || explainHas(rows, "Sort") - require.True(t, !hasKeepOrderTrue || hasSortOp, - "case 4: unique index should not satisfy ORDER BY PK without sort") + hasSortOp := explainHas(rows, "TopN") || explainHas(rows, "Sort") + require.True(t, hasSortOp, + "case 4: expected TopN/Sort for ORDER BY PK on unique secondary index") @@ - hasKeepOrderTrue = explainHas(rows, "keep order:true") hasSortOp = explainHas(rows, "TopN") || explainHas(rows, "Sort") - require.True(t, !hasKeepOrderTrue || hasSortOp, - "case 6: mixed ASC/DESC should not be satisfied by keep order alone") + require.True(t, hasSortOp, + "case 6: expected TopN/Sort for mixed ASC/DESC ordering") @@ - hasKeepOrderTrue = explainHas(rows, "keep order:true") hasSortOp = explainHas(rows, "TopN") || explainHas(rows, "Sort") - // The prefixed PK column cannot satisfy ORDER BY on the full column, - // so either keep order should be false or a TopN/Sort must be present. - require.True(t, !hasKeepOrderTrue || hasSortOp, - "case 7: prefixed clustered PK should not satisfy ORDER BY without sort") + require.True(t, hasSortOp, + "case 7: expected TopN/Sort for prefixed clustered PK ORDER BY full columns")Also applies to: 164-167, 190-195
🤖 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_common_handle_ordering_test.go` around lines 132 - 135, The current assertion (!hasKeepOrderTrue || hasSortOp) allows plans that have neither keep order:true nor TopN/Sort; for negative ORDER BY tests ensure a sort operator is present by replacing the condition with an explicit assertion that hasSortOp is true (or equivalently assert not (hasKeepOrderTrue && !hasSortOp)). Update the checks using the local variables hasKeepOrderTrue and hasSortOp in the failing cases (the block containing the require.True call and the analogous blocks at the other locations) to assert hasSortOp explicitly and keep the existing failure message.
🤖 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.
Nitpick comments:
In `@pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go`:
- Around line 132-135: The current assertion (!hasKeepOrderTrue || hasSortOp)
allows plans that have neither keep order:true nor TopN/Sort; for negative ORDER
BY tests ensure a sort operator is present by replacing the condition with an
explicit assertion that hasSortOp is true (or equivalently assert not
(hasKeepOrderTrue && !hasSortOp)). Update the checks using the local variables
hasKeepOrderTrue and hasSortOp in the failing cases (the block containing the
require.True call and the analogous blocks at the other locations) to assert
hasSortOp explicitly and keep the existing failure message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d0fd0175-8f88-43b1-90af-9708956564e4
📒 Files selected for processing (7)
pkg/dxf/importinto/BUILD.bazelpkg/dxf/importinto/conflictedkv/BUILD.bazelpkg/executor/importer/BUILD.bazelpkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/rule_common_handle_ordering_test.gopkg/planner/core/find_best_task.gopkg/tici/BUILD.bazel
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/planbuilder.go`:
- Around line 476-479: The PlanBuilder leaves stale hint state when Init is
called with a nil processor; update the Init logic around b.hintProcessor so
that when processor is nil you explicitly clear b.hintState (set to nil) as well
as assigning b.hintProcessor = nil, ensuring GetHintState() cannot return the
previous build's state; locate the block that currently does "b.hintProcessor =
processor" and the conditional that only sets b.hintState on non-nil processors
and add the explicit clear for the nil case.
In `@pkg/util/hint/hint_query_block.go`:
- Around line 222-237: The function QBHintHandler.HandleUnusedViewHints
currently resets the incoming warn slice (warn = warn[:0]) which discards any
previously accumulated warnings; remove that reset so you append
unused-view-hint messages onto the existing warn slice instead. Keep the nil
guard for state, retain the loop over p.ViewQBNameToTable and the check against
state.ViewQBNameUsed, and only append when p.warnHandler != nil; ensure the
function returns the original warn slice (possibly nil) augmented with new
messages rather than a freshly cleared slice.
🪄 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: 055786ff-cd68-4c2f-acd1-656a92e0c1ac
📒 Files selected for processing (8)
pkg/planner/core/logical_plan_builder.gopkg/planner/core/planbuilder.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/BUILD.bazelpkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.gopkg/util/context/plancache.gopkg/util/hint/hint_query_block.go
Comment on lines
476
to
+479
| b.hintProcessor = processor | ||
| if processor != nil { | ||
| b.hintState = processor.NewBuildState() | ||
| } |
There was a problem hiding this comment.
Clear stale hintState when processor is nil.
Line 477 only initializes hintState for non-nil processors. If a PlanBuilder instance is reused, a later Init(..., nil) keeps the previous build's QB hint state alive and reachable via GetHintState().
Suggested fix
b.ctx = sctx
b.is = is
b.hintProcessor = processor
+ b.hintState = nil
if processor != nil {
b.hintState = processor.NewBuildState()
}📝 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.
Suggested change
| b.hintProcessor = processor | |
| if processor != nil { | |
| b.hintState = processor.NewBuildState() | |
| } | |
| b.hintProcessor = processor | |
| b.hintState = nil | |
| if processor != nil { | |
| b.hintState = processor.NewBuildState() | |
| } |
🤖 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/planbuilder.go` around lines 476 - 479, The PlanBuilder
leaves stale hint state when Init is called with a nil processor; update the
Init logic around b.hintProcessor so that when processor is nil you explicitly
clear b.hintState (set to nil) as well as assigning b.hintProcessor = nil,
ensuring GetHintState() cannot return the previous build's state; locate the
block that currently does "b.hintProcessor = processor" and the conditional that
only sets b.hintState on non-nil processors and add the explicit clear for the
nil case.
winoros
added
the
tide/merge-method-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/planner/core/casetest/parallelapply/parallel_apply_test.go (1)
73-98: 💤 Low valueFragile
inBuildSidetracking by substring scan.The toggling of
inBuildSidepurely on the presence of"Build"/"Probe"substrings can be misled by nested operators inside the outer subtree (e.g., anIndexHashJoinwith its own(Probe)line will flipinBuildSideback tofalsewhile you're still inside the Apply's outer/Build branch). For the simple Apply shapes used in these tests it currently works, but the assertion is order- and topology-sensitive.Consider matching on the explicit
Applyrow prefix and the immediate child operator label, or using indentation depth from the tree prefix, so the keep-order check is anchored to the Apply's outer child rather than to any line containing"Build".🤖 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/parallelapply/parallel_apply_test.go` around lines 73 - 98, The current checkHasApply callback is fragile because it toggles inBuildSide based on any "Build"/"Probe" substring; update checkHasApply to anchor the keep-order check to the Apply operator itself (function: checkHasApply) by locating the line that contains "Apply", recording that line's tree-prefix/indentation (or its index in rows), then scanning only the Apply's outer/Build child subtree (either the immediate child line(s) following the Apply line or lines with greater indent than the Apply line until indent returns to the same level) to search for "keep order:true"; replace the boolean inBuildSide toggle logic and use the Apply-line based index/indentation to set the scope for setting foundKeepOrder so nested operators elsewhere don't flip the state.pkg/executor/parallel_apply.go (1)
385-433: ⚖️ Poor tradeoffPer-outer-row fresh chunk allocation differs from unordered mode.
processOneOuterRowallocates a new chunk viaexec.NewFirstChunk(e)for every outer row (and again whenever the chunk fills inside the inner-match loop), and those chunks are not returned tofreeChkChafter the reorder worker copies their rows intooutputChk. In contrast, the unorderedinnerWorkerreuses chunks fromfreeChkCh, so ordered mode has measurably higher allocation/GC pressure under high-volume workloads.This is a deliberate tradeoff (it decouples inner-worker chunk lifetime from the reorder buffer's pending map), and
outerPaceChbounds how many of these chunks are alive at once, so it's correct. Worth considering a small pool (or letting the reorder worker recycle theorderedResult.chksback to inner workers) if profiling later shows GC churn here.🤖 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/executor/parallel_apply.go` around lines 385 - 433, processOneOuterRow currently creates new chunks via exec.NewFirstChunk(e) per outer row which increases allocations; change it to reuse chunks from the existing pool used by unordered innerWorker (freeChkCh) or accept a chunk allocator argument so it pulls from freeChkCh instead of NewFirstChunk, and ensure chunks produced (orderedResult.chks) are returned to freeChkCh by the reorder worker after copying into outputChk; update processOneOuterRow, any call sites that allocate chunks, and the reorder worker logic so the lifetime matches unordered mode and avoids leaking chunks.pkg/executor/parallel_apply_test.go (1)
893-921: 💤 Low valueTime-based assertion may flake under heavy CI load.
The 2-second cap on
elapsedis generous relative to the 200ms kill / 500ms-per-row sleep, so in normal conditions it's fine — but under-race, ASan, or contended CI runners, scheduler hiccups can push process latency well past 2s while still being "promptly interrupted" in the correctness sense. If this test starts to flake, the more robust signal here is thaterrmatchesErrQueryInterrupted(the kill error) rather than any other error type, plus elapsed being meaningfully less than the un-killed runtime (5 × 500ms / 3 ≈ ~1s, padded), rather than a fixed wall-clock bound.Consider asserting the error identity in addition to (or instead of) the wall-clock bound:
🛡️ Suggested hardening
start := time.Now() err := tk.QueryToErr(sql) elapsed := time.Since(start) require.Error(t, err, "query should be interrupted by kill signal") + require.True(t, strings.Contains(err.Error(), "Query execution was interrupted"), + "expected interrupted-by-kill error, got: %v", err) - // The query should be interrupted well before all 5 rows × 500ms - // sleep would complete (~2.5s). Allow generous headroom but verify - // it didn't run to completion. - require.Less(t, elapsed, 2*time.Second, - "kill signal should abort execution promptly, but took %v", elapsed) + // Sanity check that the kill aborted early rather than running through all rows. + require.Less(t, elapsed, 3*time.Second, + "kill signal should abort execution promptly, but took %v", elapsed) wg.Wait()🤖 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/executor/parallel_apply_test.go` around lines 893 - 921, Assert the observed error is the kill error and relax the brittle fixed 2s wall-clock check: after calling tk.QueryToErr(sql) add an assertion that err is ErrQueryInterrupted (use require.ErrorIs/require.Equal with ErrQueryInterrupted) to ensure the kill was the cause, and replace the hard require.Less(elapsed, 2*time.Second) check with a comparison against the un-killed runtime (e.g., compute unKilledRuntime as 5 * 500ms) and assert elapsed is meaningfully less than that (for example elapsed < unKilledRuntime * 3/4) so the test verifies prompt interruption without depending on a narrow absolute bound; reference tk.QueryToErr, err, elapsed, and ErrQueryInterrupted when making the changes.
🤖 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.
Nitpick comments:
In `@pkg/executor/parallel_apply_test.go`:
- Around line 893-921: Assert the observed error is the kill error and relax the
brittle fixed 2s wall-clock check: after calling tk.QueryToErr(sql) add an
assertion that err is ErrQueryInterrupted (use require.ErrorIs/require.Equal
with ErrQueryInterrupted) to ensure the kill was the cause, and replace the hard
require.Less(elapsed, 2*time.Second) check with a comparison against the
un-killed runtime (e.g., compute unKilledRuntime as 5 * 500ms) and assert
elapsed is meaningfully less than that (for example elapsed < unKilledRuntime *
3/4) so the test verifies prompt interruption without depending on a narrow
absolute bound; reference tk.QueryToErr, err, elapsed, and ErrQueryInterrupted
when making the changes.
In `@pkg/executor/parallel_apply.go`:
- Around line 385-433: processOneOuterRow currently creates new chunks via
exec.NewFirstChunk(e) per outer row which increases allocations; change it to
reuse chunks from the existing pool used by unordered innerWorker (freeChkCh) or
accept a chunk allocator argument so it pulls from freeChkCh instead of
NewFirstChunk, and ensure chunks produced (orderedResult.chks) are returned to
freeChkCh by the reorder worker after copying into outputChk; update
processOneOuterRow, any call sites that allocate chunks, and the reorder worker
logic so the lifetime matches unordered mode and avoids leaking chunks.
In `@pkg/planner/core/casetest/parallelapply/parallel_apply_test.go`:
- Around line 73-98: The current checkHasApply callback is fragile because it
toggles inBuildSide based on any "Build"/"Probe" substring; update checkHasApply
to anchor the keep-order check to the Apply operator itself (function:
checkHasApply) by locating the line that contains "Apply", recording that line's
tree-prefix/indentation (or its index in rows), then scanning only the Apply's
outer/Build child subtree (either the immediate child line(s) following the
Apply line or lines with greater indent than the Apply line until indent returns
to the same level) to search for "keep order:true"; replace the boolean
inBuildSide toggle logic and use the Apply-line based index/indentation to set
the scope for setting foundKeepOrder so nested operators elsewhere don't flip
the state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 87f09d6b-26f9-4df5-b2f5-5c2337df1a83
📒 Files selected for processing (14)
pkg/executor/builder.gopkg/executor/parallel_apply.gopkg/executor/parallel_apply_test.gopkg/executor/partition_table_test.gopkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_out.jsonpkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_xut.jsonpkg/planner/core/casetest/parallelapply/BUILD.bazelpkg/planner/core/casetest/parallelapply/parallel_apply_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/integration_test.gopkg/planner/core/operator/physicalop/physical_apply.gopkg/planner/core/optimizer.gopkg/planner/util/path.gotests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.result
✅ Files skipped from review due to trivial changes (3)
- pkg/planner/core/casetest/parallelapply/BUILD.bazel
- pkg/executor/builder.go
- pkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_out.json
winoros
approved these changes
May 14, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/fts #68383 +/- ##
====================================================
- Coverage 76.8610% 45.2589% -31.6021%
====================================================
Files 1960 1805 -155
Lines 555677 491883 -63794
====================================================
- Hits 427099 222621 -204478
- Misses 127116 248240 +121124
- Partials 1462 21022 +19560
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AilinKid
changed the title
Contributor
Author
|
/retest-required |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
Author
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json (1)
701-723:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve unresolved git conflict markers in testdata JSON.
Line 701 through Line 723 still contain
<<<<<<<,=======, and>>>>>>>, which makes this file invalid JSON and will break plan test parsing/loading.🧹 Minimal fix shape
-<<<<<<< HEAD -...first variant... -======= -...second variant... ->>>>>>> 102f7af5ce (planner: Adjust cost for semijoin and apply (`#66786`)) +...keep exactly one resolved `Result` variant and remove all conflict markers...🤖 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/tpch/testdata/tpch_suite_xut.json` around lines 701 - 723, The file contains unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>) around the plan nodes (look for symbols like Projection_26, TopN_30, HashAgg_37, IndexHashJoin_47) which makes the JSON invalid; remove the conflict markers and reconcile the two variants by keeping the intended plan text (pick the correct block or merge differing fields), ensuring the resulting content is valid JSON (no leftover markers, matching quotes/brackets, and proper commas) so plan test parsing/loading succeeds.
🤖 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/tpch/testdata/tpch_suite_out.json`:
- Around line 711-720: The expected plan for TestQ21 contains a duplicated root
subtree starting at the second occurrence of Projection_26 -> TopN_30 ->
HashAgg_37 (a bad snapshot merge); remove the duplicated root chain so only a
single coherent root subtree remains (keep the correct Projection_26 -> TopN_30
-> HashAgg_37 -> IndexHashJoin_47 ... chain) — easiest fix is to regenerate the
TestQ21 entry in tpch_suite_out.json from the current planner output or manually
delete the redundant Projection_26..HashJoin_190 block so the JSON contains only
one root subtree for TestQ21.
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 2848-2865: The code currently only computes outerExpectedCnt via
calcOuterExpectedCnt when prop.IsSortItemEmpty() is false, leaving unordered
Apply plans with math.MaxFloat64 and preventing ExpectedCnt scaling; always call
calcOuterExpectedCnt(la.SCtx(), prop, stats0.RowCount, la.StatsInfo().RowCount)
to compute outerExpectedCnt (remove the !prop.IsSortItemEmpty() gate) and pass
that value into the property.PhysicalProperty for the outer side so unordered
Apply also benefits from scaled ExpectedCnt.
---
Outside diff comments:
In `@pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json`:
- Around line 701-723: The file contains unresolved Git conflict markers
(<<<<<<<, =======, >>>>>>>) around the plan nodes (look for symbols like
Projection_26, TopN_30, HashAgg_37, IndexHashJoin_47) which makes the JSON
invalid; remove the conflict markers and reconcile the two variants by keeping
the intended plan text (pick the correct block or merge differing fields),
ensuring the resulting content is valid JSON (no leftover markers, matching
quotes/brackets, and proper commas) so plan test parsing/loading succeeds.
🪄 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: 455dea4e-9b21-4c82-9d27-b7283724e8e6
📒 Files selected for processing (45)
pkg/bindinfo/binding_auto_test.gopkg/bindinfo/binding_plan_generation.gopkg/planner/BUILD.bazelpkg/planner/cardinality/BUILD.bazelpkg/planner/cardinality/selectivity_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/casetest/cascades/testdata/cascades_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.jsonpkg/planner/core/casetest/correlated/BUILD.bazelpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.jsonpkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/main_test.gopkg/planner/core/casetest/rule/rule_correlate_test.gopkg/planner/core/casetest/rule/testdata/correlate_suite_in.jsonpkg/planner/core/casetest/rule/testdata/correlate_suite_out.jsonpkg/planner/core/casetest/rule/testdata/correlate_suite_xut.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_xut.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.jsonpkg/planner/core/exhaust_physical_plans.gopkg/planner/core/expression_rewriter.gopkg/planner/core/operator/logicalop/logical_apply.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/operator/physicalop/physical_index_join.gopkg/planner/core/optimizer.gopkg/planner/core/optimizer_test.gopkg/planner/core/plan_clone_utils.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/rule/logical_rules.gopkg/planner/core/rule_correlate.gopkg/planner/core/rule_decorrelate.gopkg/planner/core/task.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/BUILD.bazelpkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/varsutil_test.gotests/integrationtest/r/planner/core/grouped_ranges_order_by.resulttests/integrationtest/r/topn_push_down.result
✅ Files skipped from review due to trivial changes (5)
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json
- pkg/sessionctx/variable/setvar_affect.go
- pkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.json
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
- pkg/planner/core/casetest/rule/testdata/correlate_suite_out.json
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/sessionctx/stmtctx/BUILD.bazel
- pkg/sessionctx/stmtctx/stmtctx.go
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json (1)
701-723:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winResolve the leftover merge conflict markers.
<<<<<<<,=======, and>>>>>>>are committed into this JSON fixture, so the file no longer parses and the TPCH casetest data cannot be loaded. Keep only the intendedTestQ21expected-plan block before merging.🤖 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/tpch/testdata/tpch_suite_xut.json` around lines 701 - 723, The file contains unresolved git conflict markers (<<<<<<<, =======, >>>>>>>) inside the TestQ21 expected-plan JSON block; remove the conflict markers and the duplicated alternate block so only the intended TestQ21 expected-plan remains (the canonical block that begins with Projection_26 / TopN_30 / HashAgg_37 etc.), ensuring the JSON stays valid (no leftover markers or duplicated entries) and the plan for TestQ21 is a single coherent object.
🤖 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/executor/parallel_apply_test.go`:
- Around line 992-995: The test's detection of serial vs parallel Apply is
brittle because it relies on exact casing of strings in execInfo; update the
check around execInfo (the logic that increments serialApplyCount and
parallelApplyCount) to perform case-insensitive matching and to distinguish
"off" explicitly from a numeric concurrency value (e.g., lower-case execInfo and
test for the token "off" vs a digits pattern or presence of any digit), so that
"OFF", "Off", or formatted variants are handled deterministically without
changing other test expectations.
In `@pkg/planner/core/casetest/rule/rule_common_handle_ordering_test.go`:
- Around line 130-136: The test's negative assertion is too permissive: replace
the combined check using hasKeepOrderTrue and hasSortOp with explicit assertions
so a plan lacking both markers fails; specifically, in the block using
explainHas(rows, "keep order:true") and explainHas(rows, "TopN") ||
explainHas(rows, "Sort"), change require.True(t, !hasKeepOrderTrue || hasSortOp,
...) to two deterministic assertions such as require.False(t, hasKeepOrderTrue)
and require.True(t, hasSortOp) (or the inverse where appropriate) so the test
insists there is no keep-order and that a Sort/TopN exists; apply the same
explicit assertion changes to the other occurrences around the variables
hasKeepOrderTrue/hasSortOp noted in the comment (the blocks at the other
mentioned locations).
In `@pkg/planner/core/casetest/rule/rule_correlate_test.go`:
- Around line 132-135: The helper explainContains currently uses an unchecked
type assertion row[0].(string) which can panic if the EXPLAIN row shape changes;
update explainContains to first check len(row) > 0, then use the safe comma-ok
type assertion (s, ok := row[0].(string)) and handle non-string or empty rows by
continuing or returning false rather than panicking, and mirror the same guarded
extraction pattern for the similar helper(s) around lines 142-146 to keep tests
deterministic and avoid crashes.
In `@pkg/sessionctx/stmtctx/stmtctx.go`:
- Around line 72-90: The LogicalPlanBuildState currently omits the
alternative-plan flags, causing signal leakage between candidate builds; update
the LogicalPlanBuildState struct to include fields for
AlternativeLogicalPlanDecorrelatedApply,
AlternativeLogicalPlanSameOrderIndexJoin, and
AlternativeLogicalPlanPreferCorrelate (or ensure RestoreLogicalPlanBuildState
clears those globals), and then modify
SaveLogicalPlanBuildState/RestoreLogicalPlanBuildState to save and restore (or
reset) those flags alongside warnings/tables/planCache* so the snapshot/restore
contract is complete for functions using LogicalPlanBuildState.
---
Outside diff comments:
In `@pkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.json`:
- Around line 701-723: The file contains unresolved git conflict markers
(<<<<<<<, =======, >>>>>>>) inside the TestQ21 expected-plan JSON block; remove
the conflict markers and the duplicated alternate block so only the intended
TestQ21 expected-plan remains (the canonical block that begins with
Projection_26 / TopN_30 / HashAgg_37 etc.), ensuring the JSON stays valid (no
leftover markers or duplicated entries) and the plan for TestQ21 is a single
coherent object.
🪄 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: da8904e1-b83b-4691-811b-68643af7cc33
📒 Files selected for processing (68)
pkg/bindinfo/binding_auto_test.gopkg/bindinfo/binding_plan_generation.gopkg/dxf/importinto/BUILD.bazelpkg/dxf/importinto/conflictedkv/BUILD.bazelpkg/executor/builder.gopkg/executor/importer/BUILD.bazelpkg/executor/parallel_apply.gopkg/executor/parallel_apply_test.gopkg/executor/partition_table_test.gopkg/planner/BUILD.bazelpkg/planner/cardinality/BUILD.bazelpkg/planner/cardinality/selectivity_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/casetest/cascades/testdata/cascades_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.jsonpkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.jsonpkg/planner/core/casetest/correlated/BUILD.bazelpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.jsonpkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.jsonpkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_out.jsonpkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_xut.jsonpkg/planner/core/casetest/parallelapply/BUILD.bazelpkg/planner/core/casetest/parallelapply/parallel_apply_test.gopkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/main_test.gopkg/planner/core/casetest/rule/rule_common_handle_ordering_test.gopkg/planner/core/casetest/rule/rule_correlate_test.gopkg/planner/core/casetest/rule/testdata/correlate_suite_in.jsonpkg/planner/core/casetest/rule/testdata/correlate_suite_out.jsonpkg/planner/core/casetest/rule/testdata/correlate_suite_xut.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_xut.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_out.jsonpkg/planner/core/casetest/tpch/testdata/tpch_suite_xut.jsonpkg/planner/core/exhaust_physical_plans.gopkg/planner/core/expression_rewriter.gopkg/planner/core/find_best_task.gopkg/planner/core/integration_test.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_apply.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/operator/physicalop/physical_apply.gopkg/planner/core/operator/physicalop/physical_index_join.gopkg/planner/core/optimizer.gopkg/planner/core/optimizer_test.gopkg/planner/core/plan_clone_utils.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/planbuilder.gopkg/planner/core/rule/logical_rules.gopkg/planner/core/rule_correlate.gopkg/planner/core/rule_decorrelate.gopkg/planner/core/task.gopkg/planner/optimize.gopkg/planner/util/path.gopkg/sessionctx/stmtctx/BUILD.bazelpkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/stmtctx/stmtctx_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/varsutil_test.gopkg/tici/BUILD.bazelpkg/util/context/plancache.gopkg/util/hint/hint_query_block.gotests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.resulttests/integrationtest/r/planner/core/grouped_ranges_order_by.resulttests/integrationtest/r/topn_push_down.result
✅ Files skipped from review due to trivial changes (16)
- pkg/planner/core/casetest/correlated/BUILD.bazel
- pkg/tici/BUILD.bazel
- pkg/planner/core/casetest/rule/testdata/outer2inner_xut.json
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_xut.json
- pkg/sessionctx/variable/varsutil_test.go
- pkg/planner/core/casetest/cbotest/testdata/analyze_suite_out.json
- tests/integrationtest/r/topn_push_down.result
- pkg/planner/core/casetest/rule/testdata/outer2inner_out.json
- pkg/executor/importer/BUILD.bazel
- pkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_out.json
- pkg/planner/BUILD.bazel
- tests/integrationtest/r/planner/core/grouped_ranges_order_by.result
- pkg/planner/core/rule/logical_rules.go
- pkg/planner/core/casetest/indexmerge/testdata/index_merge_suite_xut.json
- pkg/planner/core/casetest/rule/testdata/correlate_suite_out.json
- tests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.result
🚧 Files skipped from review as they are similar to previous changes (42)
- pkg/planner/core/BUILD.bazel
- pkg/executor/builder.go
- pkg/planner/core/casetest/rule/BUILD.bazel
- pkg/sessionctx/vardef/tidb_vars.go
- pkg/planner/core/casetest/parallelapply/BUILD.bazel
- pkg/planner/core/operator/physicalop/physical_apply.go
- pkg/planner/core/task.go
- pkg/dxf/importinto/BUILD.bazel
- pkg/bindinfo/binding_plan_generation.go
- pkg/dxf/importinto/conflictedkv/BUILD.bazel
- pkg/planner/core/casetest/cbotest/testdata/analyze_suite_xut.json
- pkg/sessionctx/stmtctx/BUILD.bazel
- pkg/planner/core/casetest/cascades/testdata/cascades_suite_out.json
- pkg/sessionctx/variable/setvar_affect.go
- pkg/planner/core/casetest/correlated/testdata/correlated_subquery_suite_out.json
- pkg/planner/core/rule_decorrelate.go
- pkg/planner/core/casetest/rule/testdata/correlate_suite_xut.json
- pkg/util/context/plancache.go
- pkg/sessionctx/variable/session.go
- pkg/planner/core/operator/logicalop/logical_apply.go
- pkg/planner/core/optimizer_test.go
- pkg/planner/core/operator/physicalop/physical_index_join.go
- pkg/planner/core/operator/logicalop/logical_join.go
- pkg/planner/core/plan_cost_ver2.go
- pkg/planner/core/casetest/parallelapply/parallel_apply_test.go
- pkg/planner/core/casetest/rule/main_test.go
- pkg/bindinfo/binding_auto_test.go
- pkg/executor/partition_table_test.go
- pkg/planner/core/planbuilder.go
- pkg/planner/cardinality/selectivity_test.go
- pkg/planner/util/path.go
- pkg/planner/core/optimizer.go
- pkg/sessionctx/stmtctx/stmtctx_test.go
- pkg/planner/core/logical_plan_builder.go
- pkg/planner/core/casetest/rule/testdata/correlate_suite_in.json
- pkg/planner/core/integration_test.go
- pkg/planner/core/plan_clone_utils.go
- pkg/executor/parallel_apply.go
- pkg/planner/core/exhaust_physical_plans.go
- pkg/planner/optimize.go
- pkg/util/hint/hint_query_block.go
- pkg/planner/core/expression_rewriter.go
Comment on lines
+992
to
+995
| if strings.Contains(execInfo, "concurrency:OFF") { | ||
| serialApplyCount++ | ||
| } else if strings.Contains(execInfo, "Concurrency:") { | ||
| parallelApplyCount++ |
There was a problem hiding this comment.
Make serial/parallel Apply detection in EXPLAIN ANALYZE format-robust.
This branch is fragile because it depends on exact casing/text (concurrency:OFF vs Concurrency:). A small formatting change can misclassify Apply type and make the test fail for non-functional reasons. Prefer parsing execInfo with a case-insensitive check (or a stricter token parser) for OFF vs numeric concurrency.
As per coding guidelines: “Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.”
🤖 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/executor/parallel_apply_test.go` around lines 992 - 995, The test's
detection of serial vs parallel Apply is brittle because it relies on exact
casing of strings in execInfo; update the check around execInfo (the logic that
increments serialApplyCount and parallelApplyCount) to perform case-insensitive
matching and to distinguish "off" explicitly from a numeric concurrency value
(e.g., lower-case execInfo and test for the token "off" vs a digits pattern or
presence of any digit), so that "OFF", "Off", or formatted variants are handled
deterministically without changing other test expectations.
Comment on lines
+130
to
+136
| // For a unique index with a range scan, the optimizer should not | ||
| // be able to satisfy ORDER BY PK via keep order on the index. | ||
| hasKeepOrderTrue := explainHas(rows, "keep order:true") | ||
| hasSortOp := explainHas(rows, "TopN") || explainHas(rows, "Sort") | ||
| require.True(t, !hasKeepOrderTrue || hasSortOp, | ||
| "case 4: unique index should not satisfy ORDER BY PK without sort") | ||
|
|
There was a problem hiding this comment.
Negative plan assertions are too permissive and can hide ORDER BY regressions.
The check !hasKeepOrderTrue || hasSortOp passes when both are false, which means a plan with neither keep order:true nor TopN/Sort still passes. For these negative cases, assert the expected failure mode explicitly (e.g., require.False(hasKeepOrderTrue) and require.True(hasSortOp)), so the test cannot silently accept an invalid plan shape.
As per coding guidelines: “Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.”
Also applies to: 164-167, 190-195
🤖 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_common_handle_ordering_test.go` around
lines 130 - 136, The test's negative assertion is too permissive: replace the
combined check using hasKeepOrderTrue and hasSortOp with explicit assertions so
a plan lacking both markers fails; specifically, in the block using
explainHas(rows, "keep order:true") and explainHas(rows, "TopN") ||
explainHas(rows, "Sort"), change require.True(t, !hasKeepOrderTrue || hasSortOp,
...) to two deterministic assertions such as require.False(t, hasKeepOrderTrue)
and require.True(t, hasSortOp) (or the inverse where appropriate) so the test
insists there is no keep-order and that a Sort/TopN exists; apply the same
explicit assertion changes to the other occurrences around the variables
hasKeepOrderTrue/hasSortOp noted in the comment (the blocks at the other
mentioned locations).
Comment on lines
+132
to
+135
| func explainContains(rows [][]any, substr string) bool { | ||
| for _, row := range rows { | ||
| if strings.Contains(row[0].(string), substr) { | ||
| return true |
There was a problem hiding this comment.
Avoid panic-prone type assertions in EXPLAIN helpers.
row[0].(string) can panic if EXPLAIN output shape changes. Use guarded extraction (len(row) > 0, type check) and fail assertions explicitly instead of crashing the test helper.
As per coding guidelines: “Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.”
Also applies to: 142-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_correlate_test.go` around lines 132 -
135, The helper explainContains currently uses an unchecked type assertion
row[0].(string) which can panic if the EXPLAIN row shape changes; update
explainContains to first check len(row) > 0, then use the safe comma-ok type
assertion (s, ok := row[0].(string)) and handle non-string or empty rows by
continuing or returning false rather than panicking, and mirror the same guarded
extraction pattern for the similar helper(s) around lines 142-146 to keep tests
deterministic and avoid crashes.
Comment on lines
+72
to
+90
| // LogicalPlanBuildState stores the statement-scoped planner state that is mutated while | ||
| // building a logical plan from AST. | ||
| type LogicalPlanBuildState struct { | ||
| warnings []SQLWarn | ||
| extraWarnings []SQLWarn | ||
| tables []TableEntry | ||
| tableStats map[int64]any | ||
| lockTableIDs map[int64]struct{} | ||
| tblInfo2UnionScan map[*model.TableInfo]bool | ||
| useDynamicPruneMode bool | ||
| viewDepth int32 | ||
| colRefFromUpdatePlan intset.FastIntSet | ||
| // plan cache related stuff | ||
| planCacheUseCache bool | ||
| planCacheType contextutil.PlanCacheType | ||
| planCacheUnqualified string | ||
| planCacheForce bool | ||
| planCacheAlwaysWarn bool | ||
| } |
There was a problem hiding this comment.
Snapshot the alternative-plan flags with the rest of the build state.
LogicalPlanBuildState is meant to carry round-local planner state across candidate builds, but it omits AlternativeLogicalPlanDecorrelatedApply, AlternativeLogicalPlanSameOrderIndexJoin, and AlternativeLogicalPlanPreferCorrelate. A discarded round can therefore leak those signals into the next candidate unless every caller remembers to reset them separately, which makes the save/restore contract brittle. Please either include them in the snapshot or clear them in RestoreLogicalPlanBuildState().
💡 Suggested fix
type LogicalPlanBuildState struct {
warnings []SQLWarn
extraWarnings []SQLWarn
tables []TableEntry
tableStats map[int64]any
lockTableIDs map[int64]struct{}
tblInfo2UnionScan map[*model.TableInfo]bool
useDynamicPruneMode bool
viewDepth int32
colRefFromUpdatePlan intset.FastIntSet
+ alternativeLogicalPlanDecorrelatedApply bool
+ alternativeLogicalPlanSameOrderIndexJoin bool
+ alternativeLogicalPlanPreferCorrelate bool
// plan cache related stuff
planCacheUseCache bool
planCacheType contextutil.PlanCacheType
planCacheUnqualified string
@@
return LogicalPlanBuildState{
warnings: slices.Clone(sc.GetWarnings()),
extraWarnings: slices.Clone(sc.GetExtraWarnings()),
tables: slices.Clone(sc.Tables),
tableStats: maps.Clone(sc.TableStats),
lockTableIDs: maps.Clone(sc.LockTableIDs),
tblInfo2UnionScan: maps.Clone(sc.TblInfo2UnionScan),
useDynamicPruneMode: sc.UseDynamicPruneMode,
viewDepth: sc.ViewDepth,
colRefFromUpdatePlan: sc.ColRefFromUpdatePlan.Copy(),
+ alternativeLogicalPlanDecorrelatedApply: sc.AlternativeLogicalPlanDecorrelatedApply,
+ alternativeLogicalPlanSameOrderIndexJoin: sc.AlternativeLogicalPlanSameOrderIndexJoin,
+ alternativeLogicalPlanPreferCorrelate: sc.AlternativeLogicalPlanPreferCorrelate,
planCacheUseCache: planCacheUseCache,
planCacheType: planCacheType,
planCacheUnqualified: planCacheUnqualified,
@@
sc.UseDynamicPruneMode = state.useDynamicPruneMode
sc.ViewDepth = state.viewDepth
sc.ColRefFromUpdatePlan.CopyFrom(state.colRefFromUpdatePlan)
+ sc.AlternativeLogicalPlanDecorrelatedApply = state.alternativeLogicalPlanDecorrelatedApply
+ sc.AlternativeLogicalPlanSameOrderIndexJoin = state.alternativeLogicalPlanSameOrderIndexJoin
+ sc.AlternativeLogicalPlanPreferCorrelate = state.alternativeLogicalPlanPreferCorrelate
sc.PlanCacheTracker.Restore(state.planCacheUseCache, state.planCacheType, state.planCacheUnqualified, state.planCacheForce, state.planCacheAlwaysWarn)
sc.RangeFallbackHandler = contextutil.NewRangeFallbackHandler(&sc.PlanCacheTracker, sc)
}Also applies to: 475-484, 618-651
🤖 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/sessionctx/stmtctx/stmtctx.go` around lines 72 - 90, The
LogicalPlanBuildState currently omits the alternative-plan flags, causing signal
leakage between candidate builds; update the LogicalPlanBuildState struct to
include fields for AlternativeLogicalPlanDecorrelatedApply,
AlternativeLogicalPlanSameOrderIndexJoin, and
AlternativeLogicalPlanPreferCorrelate (or ensure RestoreLogicalPlanBuildState
clears those globals), and then modify
SaveLogicalPlanBuildState/RestoreLogicalPlanBuildState to save and restore (or
reset) those flags alongside warnings/tables/planCache* so the snapshot/restore
contract is complete for functions using LogicalPlanBuildState.
Contributor
Author
|
/retest-required |
|
@AilinKid: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
AilinKid
changed the title
Contributor
Author
|
/ok-to-test |
Contributor
|
/retest |
Signed-off-by: AilinKid <314806019@qq.com>
terry1purcell
changed the title
AilinKid
removed
the
tide/merge-method-rebase
AilinKid
changed the title
qw4990
changed the title
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
qw4990
approved these changes
May 16, 2026
D3Hunter
approved these changes
May 16, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: D3Hunter, qw4990, terry1purcell, winoros 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 |
winoros
added
the
tide/merge-method-merge
Contributor
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #66644
Problem Summary:
Removed prior tidb-test tag: 42c2474b6e2e1ef3430d07a1743ca7e17fd97acc tiflash=feature-fts tikv=feature-fts
planner: build multi alternative logical plan from shared AST by AilinKid · Pull Request #66677 · pi
pkg/planner: build multiple logical plans from shared AST in optimize by AilinKid · Pull Request #66
pkg/planner, pkg/sessionctx: support non-decorrelate alternative
planner: correlate subquery rule by terry1purcell · Pull Request #66206 · pingcap/tidb
pkg/planner: add order-aware logical join reorder rule by AilinKid · Pull Request #67305 · pingcap/t
planner: rewrite FTS predicates to LIKE for evaluation of non-TiCI query plan by terry1purcell · Pul
main cdc join reorder checker: planner: fix join reorder correctness with conflict detection algorithm by guo-shaoge · Pull Request
case fix for cdc join reorder: planner: enable cd-c algorithm by default for join reorder | tidb-test=pr/2676 by guo-shaoge · Pull
Selection case for cdc join reorder: planner: support Selection operator in JoinGroup for the new join reorder by guo-shaoge · Pull Reque
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
Bug Fixes
New Features
Refactor
Tests