planner: correlate subquery rule (#66206)#68752
Open
ti-chi-bot wants to merge 2 commits into
Open
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
13 tasks
Member
Author
|
@terry1purcell This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements alternative logical-plan rounds (including a correlate round), a CorrelateSolver rule to convert eligible semi-joins into correlated Apply plans, plan-cloning utilities, a BFS binding-plan generator, and comprehensive tests and testdata covering correlate and binding behaviors. ChangesCorrelate alternatives: non-correlated subquery optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (1)
pkg/bindinfo/binding_auto_test.go-341-345 (1)
341-345:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix unsafe
output[i].Plan[rowID]write in record mode.This branch can index out of range if it is taken (
len < rowIDcannot satisfy direct indexing atrowID). Use overwrite-else-append semantics instead.Proposed fix
testdata.OnRecord(func() { output[i].SQL = sql - if len(output[i].Plan) < rowID { - output[i].Plan[rowID] = plan - } else { + if rowID < len(output[i].Plan) { + output[i].Plan[rowID] = plan + } else { output[i].Plan = append(output[i].Plan, plan) } })🤖 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/bindinfo/binding_auto_test.go` around lines 341 - 345, The current branch in pkg/bindinfo/binding_auto_test.go writes to output[i].Plan[rowID] when len(output[i].Plan) < rowID which can index out of range; change the logic in that block (the code that assigns to output[i].Plan based on rowID and plan) to use overwrite-else-append semantics: if rowID is less than len(output[i].Plan) then overwrite output[i].Plan[rowID] with plan, otherwise append plan to output[i].Plan. Ensure you update the branch that currently checks the length and performs the unsafe write to use this corrected condition around output[i].Plan and rowID.
🤖 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/bindinfo/binding_plan_generation.go`:
- Around line 755-775: extractSelectTableNames currently walks the entire SELECT
AST (via selStmt.Accept(extractor)) and thus collects table names from nested
subqueries; change the tableNameExtractor so it only collects tables from the
current/top-level query block by not descending into nested SelectStmt nodes
(e.g. add a depth/flag on tableNameExtractor or make its Visit/Enter methods
return Skip for child traversal when encountering a *ast.SelectStmt that is not
the root), so extractSelectTableNames (and its call selStmt.Accept(extractor))
will only produce table names in scope for the outer SELECT and exclude subquery
tables from possibleLeading2.
- Around line 462-514: The sel.TableHints slice is not restored after every
explored state because the noDecorrelate hint (state.noDecorrelateQB) is
appended outside the rollback/defer that restores sel.TableHints, causing hint
leakage across BFS states; fix it in binding_plan_generation.go by moving the
originalHintsLen capture and the defer restoration (that slices sel.TableHints
back to originalHintsLen) to cover both the leading/index-hint block and the
noDecorrelate append, or alternatively move the noDecorrelate hint append inside
the existing defer scope so that any addition to sel.TableHints (including the
TableOptimizerHint created for state.noDecorrelateQB) is rolled back after
exploring the state; reference sel.TableHints, state.noDecorrelateQB,
state.leading2 and state.indexHints to locate the changes.
In `@pkg/planner/core/BUILD.bazel`:
- Around line 56-61: Remove the leftover merge conflict markers (<<<<<<<,
=======, >>>>>>>) in the BUILD.bazel srcs list and reconcile the entries so the
list contains the intended source files (include "rule_collect_plan_stats.go",
"rule_column_pruning.go" and "rule_correlate.go" as appropriate), ensuring the
srcs array is a valid, comma-separated string list with no conflict markers
remaining.
In `@pkg/planner/core/casetest/rule/BUILD.bazel`:
- Around line 8-13: The BUILD.bazel target contains unresolved Git merge
conflict markers (<<<<<<<, =======, >>>>>>>) around the test file list; remove
these markers and merge the two variants into a single, valid list (e.g.,
include "rule_cdc_join_reorder_test.go", "rule_common_handle_ordering_test.go",
"rule_correlate_test.go" along with the other files) so the target is a proper
Bazel srcs array and the rule tests (the files named in the conflict) can build.
In `@pkg/planner/core/casetest/rule/main_test.go`:
- Around line 32-49: Remove the leftover merge conflict markers and fix TestMain
to a single coherent block: delete the <<<<<<<, =======, and >>>>>>> markers and
keep the intended LoadTestSuiteData calls (use the variant with the boolean
flag, e.g. LoadTestSuiteData("testdata", "outer2inner", true) and the additional
suites like "outer_to_semi_join_suite", "correlate_suite",
"cdc_join_reorder_suite", "order_aware_join_reorder_suite"); ensure the TestMain
function and any suite helper loops use the correct LoadTestSuiteData signature
(the version accepting the bool) and remove duplicated or conflicting lines so
the file compiles.
In `@pkg/planner/core/casetest/rule/rule_correlate_test.go`:
- Around line 167-197: The test currently only yields one qualifying outer row,
making the Concurrency>1 check flaky; before running the explains, ensure
multiple outer rows qualify by inserting additional rows into t1 (e.g. via
tk.MustExec("insert into t1 ...") or similar) or by relaxing the query predicate
(modify sql to use a less selective condition such as b IN (...)); update the
setup immediately before the explain/analyze block (affecting the sql variable
and the analyzeRows/foundConcurrency check) so the Apply operator has multiple
outer rows and the EXPLAIN ANALYZE concurrency assertion becomes deterministic.
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 1058-1063: The code uses
vardef.TiDBOptEnableAlternativeLogicalPlans but the package isn’t imported; add
the import "github.com/pingcap/tidb/pkg/sessionctx/vardef" to the import block
in pkg/planner/core/expression_rewriter.go so references like
vardef.TiDBOptEnableAlternativeLogicalPlans (used alongside
b.ctx.GetSessionVars().RecordRelevantOptVar and EnableCorrelateSubquery) compile
successfully.
In `@pkg/planner/core/optimizer_test.go`:
- Around line 566-614: The test references wrong packages/types: replace
coretestsdk.MockContext() with the actual MockContext from the core package (use
core.MockContext()), import the joinversion package and use
joinversion.HashJoinVersionLegacy/HashJoinVersionOptimized where used, and use
the actual PhysicalHashJoin type as defined in
pkg/planner/core/physical_plans.go (replace physicalop.PhysicalHashJoin with the
PhysicalHashJoin type from the core/planner package or adjust the import alias
to point to that file) so imports and type references match the real symbols.
- Around line 563-630: Remove the leftover merge-conflict markers (<<<<<<< HEAD,
=======, >>>>>>>) in pkg/planner/core/optimizer_test.go and keep the incoming
test code block (the tests TestCanTiFlashUseHashJoinV2 and
TestOptRuleListFlagAlignment) as a single, contiguous section; ensure no
conflict markers remain so the file compiles and the functions
TestCanTiFlashUseHashJoinV2 and TestOptRuleListFlagAlignment are declared
normally.
In `@pkg/planner/core/optimizer.go`:
- Around line 101-107: Resolve the merge conflict in the optRuleList by removing
the conflict markers (<<<<<<<, =======, >>>>>>>) and ensure the final rule
entries include the intended rules in order: &rule.OuterJoinToSemiJoin{},
&CorrelateSolver{}, and &rule.ColumnPruner{} (remove the duplicated
&ColumnPruner{} entry if present), so optRuleList contains the correct rule
instances without inline conflict markers.
In `@pkg/planner/core/plan_clone_utils.go`:
- Around line 122-150: The compiler error arises because cloneDataSource and
rule_correlate.go reference DataSource.AllPossibleAccessPaths which doesn't
exist on release-8.5; to fix it, add a new field AllPossibleAccessPaths
[]*util.AccessPath to the DataSource type (logical_datasource.go) and ensure any
constructors/initializers set it (e.g., copy from PossibleAccessPaths where
appropriate), then keep the clone logic in cloneDataSource and any uses in
rule_correlate.go that call ap.Clone() working against that new field; ensure
the new field is included in any schema/clone operations and initialized to a
nil or empty slice to preserve backward compatibility.
- Around line 215-261: cloneTopN currently calls
clone.SetSchema(tn.Schema().Clone()) while cloneSort (and LogicalSort) rely on
schema inheritance from BaseLogicalPlan; remove the inconsistent schema override
in cloneTopN to match cloneSort: delete the clone.SetSchema(...) call in
cloneTopN (function cloneTopN and type LogicalTopN) so schema is inherited from
its child via BaseLogicalPlan, or alternatively, if LogicalTopN truly requires
an explicit schema, replace the removal with a short comment in cloneTopN
explaining why LogicalTopN deviates and keep the SetSchema; ensure the change is
limited to the cloneTopN function.
In `@pkg/planner/core/rule_correlate.go`:
- Around line 168-178: The call to PredicatePushDown in rule_correlate.go is
using the old signature and expecting an error; update the call on sel to match
the current signature PredicatePushDown(predicates []expression.Expression, opt
*optimizetrace.LogicalOptimizeOp) which returns only (pushedPredicates, plan).
Replace "_, innerPlan, err := sel.PredicatePushDown(nil)" with a two-value
assignment that supplies both arguments (e.g., nil and the appropriate optimize
op variable or nil for the second arg) and remove the subsequent "if err != nil"
branch and its error handling; ensure you use the returned plan value
(innerPlan) and handle the pushedPredicates value as needed by the surrounding
logic.
- Around line 110-126: Replace the incorrect references and wrong
PredicatePushDown call in rule_correlate.go: use logicalop.LeftOuterSemiJoin and
logicalop.AntiLeftOuterSemiJoin (the join-type constants live in
pkg/planner/core/operator/logicalop) and update imports accordingly; for the
selection pushdown use the correct LogicalSelection.PredicatePushDown signature
— pass the second opt argument (e.g., nil) and capture the two return values it
actually returns (predicates and innerPlan) instead of expecting three returns
and handling an err; adjust the call site (sel.PredicatePushDown) to accept two
results and remove the erroneous err handling.
In `@pkg/planner/core/rule/logical_rules.go`:
- Around line 42-46: Remove the leftover merge markers (<<<<<<<, =======,
>>>>>>>) from the optimizer flags const block and merge the two branches by
ensuring both FlagOuterJoinToSemiJoin and FlagCorrelate appear as consecutive
entries in the const declaration (preserving surrounding constants and
ordering), e.g., include FlagOuterJoinToSemiJoin followed by FlagCorrelate in
the list used by the const block that defines the optimizer flags so the file is
valid Go without conflict markers.
In `@pkg/planner/optimize.go`:
- Around line 472-532: The file contains unresolved Git conflict markers
(<<<<<<<, =======, >>>>>>>) around the new alternative-round helpers and the
alternativeRounds variable; remove the conflict markers and keep the intended
merged code: the functions shouldTryNonDecorrelationRound,
shouldTryOrderAwareReorderRound, shouldTryCorrelateRound, the alternativeRound
type, the savedEnableCorrelateSubquery var, and the alternativeRounds array
(including the "non-decorrelate", "order-aware-reorder", and "correlate" entries
with their adjustFlag/enabled/setup/cleanup closures). Ensure there is only one
definition of alternativeRounds and no leftover conflict markers so the file
compiles.
- Around line 592-656: Resolve the leftover git conflict markers in
pkg/planner/optimize.go by merging the two branches: remove the <<<<<<<,
=======, >>>>>>> markers and produce a single control flow that preserves both
behaviors — keep the existing cascades early-return (check
sessVars.GetEnableCascadesPlanner() and call
cascades.DefaultOptimizer.FindBestPlan(sctx, logic)) before running the
alternativeRounds logic, and retain the pre-computation of enabledRounds,
restoreLogicalPlanBuildCtx, the per-round closure that calls
buildAndOptimizeLogicalPlanRound, and the nonLogical handling and logging;
ensure core.RecheckCTE(logic) remains prior to these checks and that variables
returned (p, names, cost/0, err) follow the same semantics as originally
intended.
- Around line 502-528: The package-level savedEnableCorrelateSubquery causes
races across concurrent sessions; remove the global savedEnableCorrelateSubquery
and instead store the saved value per-round using a closure or a new field on
alternativeRound so setup/cleanup operate on session-local state. Concretely, in
the correlate alternativeRound literal (the element with name "correlate")
change its setup to capture a local saved := sv.EnableCorrelateSubquery (or
assign saved into a new alternativeRound.saved bool field) before mutating
sv.EnableCorrelateSubquery, and make cleanup restore that captured/field value;
update the alternativeRound type if needed so setup/cleanup signatures still
reference the same session variable type (variable.SessionVars) and no
package-level state remains.
In `@pkg/sessionctx/stmtctx/stmtctx.go`:
- Around line 449-465: The file contains unresolved Git conflict markers (e.g.,
"<<<<<<<", "=======", ">>>>>>>") around the stmtctx struct fields, which
prevents compilation; remove the conflict markers and reconcile the fields so
the struct contains the intended boolean flags
(AlternativeLogicalPlanDecorrelatedApply,
AlternativeLogicalPlanSameOrderIndexJoin,
AlternativeLogicalPlanOrderAwareJoinReorder,
AlternativeLogicalPlanPreferCorrelate) with proper comments and punctuation,
preserving the correct ordering and formatting of the declarations (locate the
block around the stmtctx struct where those symbol names appear).
- Around line 592-666: Remove the Git conflict markers and merge the duplicated
planner-state block so the file contains a single, consistent implementation of
SaveLogicalPlanBuildState, RestoreLogicalPlanBuildState and the various
Mark/Reset methods; then add a concrete definition for the referenced type
LogicalPlanBuildState (a struct) that includes fields referenced in
Restore/Save: warnings, extraWarnings, tables, tableStats, lockTableIDs,
tblInfo2UnionScan, useDynamicPruneMode, viewDepth, colRefFromUpdatePlan
(matching the Copy/CopyFrom methods used), and the plan-cache booleans/values
(planCacheUseCache, planCacheType, planCacheUnqualified, planCacheForce,
planCacheAlwaysWarn); place the type definition near other stmtctx types in the
same file and ensure imports/usages (slices.Clone, maps.Clone,
contextutil.NewRangeFallbackHandler, PlanCacheTracker methods) remain correct so
the package compiles.
---
Minor comments:
In `@pkg/bindinfo/binding_auto_test.go`:
- Around line 341-345: The current branch in pkg/bindinfo/binding_auto_test.go
writes to output[i].Plan[rowID] when len(output[i].Plan) < rowID which can index
out of range; change the logic in that block (the code that assigns to
output[i].Plan based on rowID and plan) to use overwrite-else-append semantics:
if rowID is less than len(output[i].Plan) then overwrite output[i].Plan[rowID]
with plan, otherwise append plan to output[i].Plan. Ensure you update the branch
that currently checks the length and performs the unsafe write to use this
corrected condition around output[i].Plan and rowID.
🪄 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: ce49602f-db52-4e22-b92a-6398d49933ef
📒 Files selected for processing (19)
pkg/bindinfo/binding_auto_test.gopkg/bindinfo/binding_plan_generation.gopkg/planner/core/BUILD.bazelpkg/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/expression_rewriter.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/optimizer.gopkg/planner/core/optimizer_test.gopkg/planner/core/plan_clone_utils.gopkg/planner/core/rule/logical_rules.gopkg/planner/core/rule_correlate.gopkg/planner/optimize.gopkg/sessionctx/stmtctx/stmtctx.gopkg/sessionctx/variable/session.go
Comment on lines
+462
to
+514
| if sel, isSel := stmt.(*ast.SelectStmt); isSel { | ||
| hasIndexHint := false | ||
| for _, indexHint := range state.indexHints { | ||
| if indexHint != nil { | ||
| hasIndexHint = true | ||
| break | ||
| } | ||
| } | ||
| if (state.leading2[0] != nil && state.leading2[1] != nil) || hasIndexHint { | ||
| originalHintsLen := len(sel.TableHints) | ||
| defer func() { | ||
| sel.TableHints = sel.TableHints[:originalHintsLen] | ||
| }() | ||
| if state.leading2[0] != nil && state.leading2[1] != nil { | ||
| leadingHint := &ast.TableOptimizerHint{ | ||
| HintName: ast.NewCIStr(hint.HintLeading), | ||
| Tables: []ast.HintTable{ | ||
| { | ||
| DBName: ast.NewCIStr(state.leading2[0].schema), | ||
| TableName: ast.NewCIStr(state.leading2[0].HintName()), | ||
| }, | ||
| { | ||
| DBName: ast.NewCIStr(state.leading2[1].schema), | ||
| TableName: ast.NewCIStr(state.leading2[1].HintName()), | ||
| }, | ||
| }, | ||
| } | ||
| sel.TableHints = append(sel.TableHints, leadingHint) | ||
| } | ||
| for _, indexHint := range state.indexHints { | ||
| if indexHint == nil { | ||
| continue | ||
| } | ||
| hintNode := &ast.TableOptimizerHint{ | ||
| HintName: ast.NewCIStr(hint.HintUseIndex), | ||
| Tables: []ast.HintTable{ | ||
| { | ||
| DBName: ast.NewCIStr(indexHint.table.schema), | ||
| TableName: ast.NewCIStr(indexHint.table.HintName()), | ||
| }, | ||
| }, | ||
| Indexes: []ast.CIStr{ast.NewCIStr(indexHint.index)}, | ||
| } | ||
| sel.TableHints = append(sel.TableHints, hintNode) | ||
| } | ||
| } | ||
| if state.noDecorrelateQB.L != "" { | ||
| noDecorrelateHint := &ast.TableOptimizerHint{ | ||
| HintName: ast.NewCIStr(hint.HintNoDecorrelate), | ||
| QBName: state.noDecorrelateQB, | ||
| } | ||
| sel.TableHints = append(sel.TableHints, noDecorrelateHint) | ||
| } |
There was a problem hiding this comment.
Restore sel.TableHints after every explored state.
noDecorrelateQB is appended outside the rollback block, so a state that only sets NO_DECORRELATE() leaves that hint on the shared stmt and contaminates later BFS states. That makes later candidates observe hints from previous states instead of their own state only.
Suggested fix
if sel, isSel := stmt.(*ast.SelectStmt); isSel {
+ originalHintsLen := len(sel.TableHints)
+ defer func() {
+ sel.TableHints = sel.TableHints[:originalHintsLen]
+ }()
+
hasIndexHint := false
for _, indexHint := range state.indexHints {
if indexHint != nil {
hasIndexHint = true
break
}
}
if (state.leading2[0] != nil && state.leading2[1] != nil) || hasIndexHint {
- originalHintsLen := len(sel.TableHints)
- defer func() {
- sel.TableHints = sel.TableHints[:originalHintsLen]
- }()
if state.leading2[0] != nil && state.leading2[1] != nil {
leadingHint := &ast.TableOptimizerHint{
HintName: ast.NewCIStr(hint.HintLeading),📝 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
| if sel, isSel := stmt.(*ast.SelectStmt); isSel { | |
| hasIndexHint := false | |
| for _, indexHint := range state.indexHints { | |
| if indexHint != nil { | |
| hasIndexHint = true | |
| break | |
| } | |
| } | |
| if (state.leading2[0] != nil && state.leading2[1] != nil) || hasIndexHint { | |
| originalHintsLen := len(sel.TableHints) | |
| defer func() { | |
| sel.TableHints = sel.TableHints[:originalHintsLen] | |
| }() | |
| if state.leading2[0] != nil && state.leading2[1] != nil { | |
| leadingHint := &ast.TableOptimizerHint{ | |
| HintName: ast.NewCIStr(hint.HintLeading), | |
| Tables: []ast.HintTable{ | |
| { | |
| DBName: ast.NewCIStr(state.leading2[0].schema), | |
| TableName: ast.NewCIStr(state.leading2[0].HintName()), | |
| }, | |
| { | |
| DBName: ast.NewCIStr(state.leading2[1].schema), | |
| TableName: ast.NewCIStr(state.leading2[1].HintName()), | |
| }, | |
| }, | |
| } | |
| sel.TableHints = append(sel.TableHints, leadingHint) | |
| } | |
| for _, indexHint := range state.indexHints { | |
| if indexHint == nil { | |
| continue | |
| } | |
| hintNode := &ast.TableOptimizerHint{ | |
| HintName: ast.NewCIStr(hint.HintUseIndex), | |
| Tables: []ast.HintTable{ | |
| { | |
| DBName: ast.NewCIStr(indexHint.table.schema), | |
| TableName: ast.NewCIStr(indexHint.table.HintName()), | |
| }, | |
| }, | |
| Indexes: []ast.CIStr{ast.NewCIStr(indexHint.index)}, | |
| } | |
| sel.TableHints = append(sel.TableHints, hintNode) | |
| } | |
| } | |
| if state.noDecorrelateQB.L != "" { | |
| noDecorrelateHint := &ast.TableOptimizerHint{ | |
| HintName: ast.NewCIStr(hint.HintNoDecorrelate), | |
| QBName: state.noDecorrelateQB, | |
| } | |
| sel.TableHints = append(sel.TableHints, noDecorrelateHint) | |
| } | |
| if sel, isSel := stmt.(*ast.SelectStmt); isSel { | |
| originalHintsLen := len(sel.TableHints) | |
| defer func() { | |
| sel.TableHints = sel.TableHints[:originalHintsLen] | |
| }() | |
| hasIndexHint := false | |
| for _, indexHint := range state.indexHints { | |
| if indexHint != nil { | |
| hasIndexHint = true | |
| break | |
| } | |
| } | |
| if (state.leading2[0] != nil && state.leading2[1] != nil) || hasIndexHint { | |
| if state.leading2[0] != nil && state.leading2[1] != nil { | |
| leadingHint := &ast.TableOptimizerHint{ | |
| HintName: ast.NewCIStr(hint.HintLeading), | |
| Tables: []ast.HintTable{ | |
| { | |
| DBName: ast.NewCIStr(state.leading2[0].schema), | |
| TableName: ast.NewCIStr(state.leading2[0].HintName()), | |
| }, | |
| { | |
| DBName: ast.NewCIStr(state.leading2[1].schema), | |
| TableName: ast.NewCIStr(state.leading2[1].HintName()), | |
| }, | |
| }, | |
| } | |
| sel.TableHints = append(sel.TableHints, leadingHint) | |
| } | |
| for _, indexHint := range state.indexHints { | |
| if indexHint == nil { | |
| continue | |
| } | |
| hintNode := &ast.TableOptimizerHint{ | |
| HintName: ast.NewCIStr(hint.HintUseIndex), | |
| Tables: []ast.HintTable{ | |
| { | |
| DBName: ast.NewCIStr(indexHint.table.schema), | |
| TableName: ast.NewCIStr(indexHint.table.HintName()), | |
| }, | |
| }, | |
| Indexes: []ast.CIStr{ast.NewCIStr(indexHint.index)}, | |
| } | |
| sel.TableHints = append(sel.TableHints, hintNode) | |
| } | |
| } | |
| if state.noDecorrelateQB.L != "" { | |
| noDecorrelateHint := &ast.TableOptimizerHint{ | |
| HintName: ast.NewCIStr(hint.HintNoDecorrelate), | |
| QBName: state.noDecorrelateQB, | |
| } | |
| sel.TableHints = append(sel.TableHints, noDecorrelateHint) | |
| } |
🤖 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/bindinfo/binding_plan_generation.go` around lines 462 - 514, The
sel.TableHints slice is not restored after every explored state because the
noDecorrelate hint (state.noDecorrelateQB) is appended outside the
rollback/defer that restores sel.TableHints, causing hint leakage across BFS
states; fix it in binding_plan_generation.go by moving the originalHintsLen
capture and the defer restoration (that slices sel.TableHints back to
originalHintsLen) to cover both the leading/index-hint block and the
noDecorrelate append, or alternatively move the noDecorrelate hint append inside
the existing defer scope so that any addition to sel.TableHints (including the
TableOptimizerHint created for state.noDecorrelateQB) is rolled back after
exploring the state; reference sel.TableHints, state.noDecorrelateQB,
state.leading2 and state.indexHints to locate the changes.
Comment on lines
+755
to
+775
| // extractSelectTableNames returns the table names in the SELECT statement. | ||
| func extractSelectTableNames(defaultSchema string, node ast.StmtNode) []*tableName { | ||
| selStmt, isSel := node.(*ast.SelectStmt) | ||
| if !isSel { | ||
| return nil // only support SELECT statement for now | ||
| } | ||
| extractor := &tableNameExtractor{ | ||
| defaultSchema: defaultSchema, | ||
| tableNames: make(map[string]*tableName), | ||
| } | ||
| selStmt.Accept(extractor) | ||
|
|
||
| names := make([]*tableName, 0, len(extractor.tableNames)) | ||
| for _, name := range extractor.tableNames { | ||
| names = append(names, name) | ||
| } | ||
| sort.Slice(names, func(i, j int) bool { | ||
| return names[i].String() < names[j].String() | ||
| }) | ||
| return names | ||
| } |
There was a problem hiding this comment.
Limit LEADING() candidates to the current query block.
extractSelectTableNames walks the full AST, so subquery tables are included in possibleLeading2. The generated LEADING() hint is then attached to the outer SELECT, where those inner tables are out of scope. For the subquery-heavy statements this generator targets, that burns BFS states on invalid/no-op hints and can crowd out useful candidates before hitting the 10k-state cap.
Suggested direction
-func extractSelectTableNames(defaultSchema string, node ast.StmtNode) []*tableName {
- selStmt, isSel := node.(*ast.SelectStmt)
- if !isSel {
- return nil // only support SELECT statement for now
- }
- extractor := &tableNameExtractor{
- defaultSchema: defaultSchema,
- tableNames: make(map[string]*tableName),
- }
- selStmt.Accept(extractor)
-
- names := make([]*tableName, 0, len(extractor.tableNames))
- for _, name := range extractor.tableNames {
- names = append(names, name)
- }
- sort.Slice(names, func(i, j int) bool {
- return names[i].String() < names[j].String()
- })
- return names
+func extractSelectTableNames(defaultSchema string, node ast.StmtNode) []*tableName {
+ selStmt, isSel := node.(*ast.SelectStmt)
+ if !isSel {
+ return nil
+ }
+ return extractSelectTableNamesWithAlias(defaultSchema, selStmt)
}🤖 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/bindinfo/binding_plan_generation.go` around lines 755 - 775,
extractSelectTableNames currently walks the entire SELECT AST (via
selStmt.Accept(extractor)) and thus collects table names from nested subqueries;
change the tableNameExtractor so it only collects tables from the
current/top-level query block by not descending into nested SelectStmt nodes
(e.g. add a depth/flag on tableNameExtractor or make its Visit/Enter methods
return Skip for child traversal when encountering a *ast.SelectStmt that is not
the root), so extractSelectTableNames (and its call selStmt.Accept(extractor))
will only produce table names in scope for the outer SELECT and exclude subquery
tables from possibleLeading2.
Comment on lines
+56
to
+61
| <<<<<<< HEAD | ||
| "rule_collect_plan_stats.go", | ||
| "rule_column_pruning.go", | ||
| ======= | ||
| "rule_correlate.go", | ||
| >>>>>>> 7357a2e2f90 (planner: correlate subquery rule (#66206)) |
There was a problem hiding this comment.
Resolve the leftover merge conflict in the srcs list.
The <<<<<<< / ======= / >>>>>>> markers are still present here, so this Bazel file will not parse and the cherry-pick cannot 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/BUILD.bazel` around lines 56 - 61, Remove the leftover merge
conflict markers (<<<<<<<, =======, >>>>>>>) in the BUILD.bazel srcs list and
reconcile the entries so the list contains the intended source files (include
"rule_collect_plan_stats.go", "rule_column_pruning.go" and "rule_correlate.go"
as appropriate), ensuring the srcs array is a valid, comma-separated string list
with no conflict markers remaining.
Comment on lines
+8
to
+13
| <<<<<<< HEAD | ||
| ======= | ||
| "rule_cdc_join_reorder_test.go", | ||
| "rule_common_handle_ordering_test.go", | ||
| "rule_correlate_test.go", | ||
| >>>>>>> 7357a2e2f90 (planner: correlate subquery rule (#66206)) |
There was a problem hiding this comment.
Resolve the merge conflict markers before merging.
This target still has unresolved <<<<<<< / ======= / >>>>>>> blocks, so Bazel cannot load it and the rule tests will not build.
Also applies to: 24-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/casetest/rule/BUILD.bazel` around lines 8 - 13, The
BUILD.bazel target contains unresolved Git merge conflict markers (<<<<<<<,
=======, >>>>>>>) around the test file list; remove these markers and merge the
two variants into a single, valid list (e.g., include
"rule_cdc_join_reorder_test.go", "rule_common_handle_ordering_test.go",
"rule_correlate_test.go" along with the other files) so the target is a proper
Bazel srcs array and the rule tests (the files named in the conflict) can build.
Comment on lines
+32
to
+49
| <<<<<<< HEAD | ||
| testDataMap.LoadTestSuiteData("testdata", "outer2inner") | ||
| testDataMap.LoadTestSuiteData("testdata", "derive_topn_from_window") | ||
| testDataMap.LoadTestSuiteData("testdata", "join_reorder_suite") | ||
| testDataMap.LoadTestSuiteData("testdata", "predicate_pushdown_suite") | ||
| testDataMap.LoadTestSuiteData("testdata", "predicate_simplification") | ||
| ======= | ||
| testDataMap.LoadTestSuiteData("testdata", "outer2inner", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "derive_topn_from_window", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "join_reorder_suite", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "predicate_pushdown_suite", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "predicate_simplification", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "outer_to_semi_join_suite", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "correlate_suite", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "cdc_join_reorder_suite", true) | ||
| testDataMap.LoadTestSuiteData("testdata", "order_aware_join_reorder_suite", true) | ||
|
|
||
| >>>>>>> 7357a2e2f90 (planner: correlate subquery rule (#66206)) |
There was a problem hiding this comment.
Resolve the merge conflict markers in TestMain and the suite helpers.
The <<<<<<< / ======= / >>>>>>> markers are still in this Go file, so the test package will not compile.
Also applies to: 87-105
🤖 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/main_test.go` around lines 32 - 49, Remove the
leftover merge conflict markers and fix TestMain to a single coherent block:
delete the <<<<<<<, =======, and >>>>>>> markers and keep the intended
LoadTestSuiteData calls (use the variant with the boolean flag, e.g.
LoadTestSuiteData("testdata", "outer2inner", true) and the additional suites
like "outer_to_semi_join_suite", "correlate_suite", "cdc_join_reorder_suite",
"order_aware_join_reorder_suite"); ensure the TestMain function and any suite
helper loops use the correct LoadTestSuiteData signature (the version accepting
the bool) and remove duplicated or conflicting lines so the file compiles.
ti-chi-bot
Bot
added
cherry-pick-approved
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@ti-chi-bot: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@ti-chi-bot: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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
2 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.
This is an automated cherry-pick of #66206
What problem does this PR solve?
Issue Number: close #66320
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests