planner: build multi alternative logical plan from shared AST (#66677)#68859
planner: build multi alternative logical plan from shared AST (#66677)#68859ti-chi-bot wants to merge 1 commit into
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@AilinKid 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. |
|
[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 |
📝 WalkthroughWalkthroughThis PR refactors QB hint handling to be per-build state rather than globally mutable handler fields. It introduces ChangesPer-Build Hint State Infrastructure and Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/logical_plan_builder.go (1)
5082-5111:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix unused locals in view-hint filtering (won’t compile)
pkg/planner/core/logical_plan_builder.go:5082-5111:currentQbNameMap4ViewandcurrentQbHints4Vieware only declared/assigned, but the final wiring setshintProcessor.ViewQBNameToTable = qbNameMap4ViewandhintProcessor.ViewQBNameToHints = viewHints, leaving the new locals unused → Go will fail with “declared and not used”.Suggested safe fix: remove the dead locals
- currentQbNameMap4View := make(map[string][]ast.HintTable) - currentQbHints4View := make(map[string][]*ast.TableOptimizerHint) currentQbHints := make(map[int][]*ast.TableOptimizerHint) currentQbNameMap := make(map[string]int) @@ - currentQbNameMap4View[qbName] = viewQbNameHint - currentQbHints4View[qbName] = viewHints[qbName] }🤖 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/logical_plan_builder.go` around lines 5082 - 5111, The temporary maps currentQbNameMap4View and currentQbHints4View are declared and populated but never used, causing a compile error; remove both declarations and any assignments to currentQbNameMap4View/currentQbHints4View inside the loop and simply continue using qbNameMap4View and viewHints (which are later assigned to hintProcessor.ViewQBNameToTable and hintProcessor.ViewQBNameToHints); ensure only currentQbHints and currentQbNameMap (and deletions of qbNameMap4View/viewHints) remain as needed.
🤖 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/sessionctx/stmtctx/BUILD.bazel`:
- Around line 48-52: Remove the leftover merge conflict markers in BUILD.bazel
around the shard_count attribute (delete the <<<<<<<, =======, and >>>>>>>
lines) and leave a single shard_count = <correct_value> entry (e.g., shard_count
= 14 or shard_count = 17 as intended) so the Bazel rule parses cleanly; ensure
only one shard_count line remains in the rule.
In `@pkg/sessionctx/stmtctx/stmtctx.go`:
- Around line 68-93: There is an unresolved git conflict block left in
stmtctx.go; remove the conflict markers (<<<<<<<, =======, >>>>>>>) and
reconcile the two definitions so the file parses: either restore the jsonSQLWarn
type if needed or keep LogicalPlanBuildState (or combine them appropriately),
ensuring only valid Go types remain and references to jsonSQLWarn and
LogicalPlanBuildState are correctly defined and compiled; update imports if
required and run go build to verify the file parses.
- Around line 607-609: The code references sc.ViewDepth but StatementContext
lacks a ViewDepth field; add a ViewDepth field to the StatementContext struct
(matching the original type used elsewhere in the project), and update any
constructors/copy/snapshot/restore logic so ViewDepth is properly preserved and
copied (ensure sc.ViewDepth is set/returned where StatementContext is
initialized and that the snapshot/Restore/Copy methods include ViewDepth
alongside useDynamicPruneMode, viewDepth, and colRefFromUpdatePlan). Use the
existing symbols StatementContext, sc.ViewDepth, Copy(), snapshot/restore (or
the struct's constructor functions) to locate where to add and wire this field.
---
Outside diff comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 5082-5111: The temporary maps currentQbNameMap4View and
currentQbHints4View are declared and populated but never used, causing a compile
error; remove both declarations and any assignments to
currentQbNameMap4View/currentQbHints4View inside the loop and simply continue
using qbNameMap4View and viewHints (which are later assigned to
hintProcessor.ViewQBNameToTable and hintProcessor.ViewQBNameToHints); ensure
only currentQbHints and currentQbNameMap (and deletions of
qbNameMap4View/viewHints) remain as needed.
🪄 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: d4653a90-f1e4-428b-8276-6e34087b6fed
📒 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
| <<<<<<< HEAD | ||
| shard_count = 14, | ||
| ======= | ||
| shard_count = 17, | ||
| >>>>>>> d59e531fe61 (planner: build multi alternative logical plan from shared AST (#66677)) |
There was a problem hiding this comment.
Remove the leftover conflict markers from the Bazel rule.
Line 48 still contains merge-conflict markers, so Bazel cannot parse this file. Keep only the intended shard_count entry.
🤖 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/BUILD.bazel` around lines 48 - 52, Remove the leftover
merge conflict markers in BUILD.bazel around the shard_count attribute (delete
the <<<<<<<, =======, and >>>>>>> lines) and leave a single shard_count =
<correct_value> entry (e.g., shard_count = 14 or shard_count = 17 as intended)
so the Bazel rule parses cleanly; ensure only one shard_count line remains in
the rule.
| <<<<<<< HEAD | ||
| type jsonSQLWarn struct { | ||
| Level string `json:"level"` | ||
| SQLErr *terror.Error `json:"err,omitempty"` | ||
| Msg string `json:"msg,omitempty"` | ||
| ======= | ||
| // 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 | ||
| >>>>>>> d59e531fe61 (planner: build multi alternative logical plan from shared AST (#66677)) | ||
| } |
There was a problem hiding this comment.
Resolve the cherry-pick conflict before merge.
Line 68 still starts a <<<<<<< / ======= / >>>>>>> block, so this file does not parse. It also leaves jsonSQLWarn and LogicalPlanBuildState in an unresolved state.
🤖 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 68 - 93, There is an
unresolved git conflict block left in stmtctx.go; remove the conflict markers
(<<<<<<<, =======, >>>>>>>) and reconcile the two definitions so the file
parses: either restore the jsonSQLWarn type if needed or keep
LogicalPlanBuildState (or combine them appropriately), ensuring only valid Go
types remain and references to jsonSQLWarn and LogicalPlanBuildState are
correctly defined and compiled; update imports if required and run go build to
verify the file parses.
| useDynamicPruneMode: sc.UseDynamicPruneMode, | ||
| viewDepth: sc.ViewDepth, | ||
| colRefFromUpdatePlan: sc.ColRefFromUpdatePlan.Copy(), |
There was a problem hiding this comment.
ViewDepth is snapshotted/restored but not declared on StatementContext.
Lines 608 and 628 reference sc.ViewDepth, but there is no ViewDepth field on StatementContext anywhere in this file. This cherry-pick will still fail to compile after the conflict markers are fixed unless that field addition is brought over too.
Also applies to: 627-629
🤖 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 607 - 609, The code
references sc.ViewDepth but StatementContext lacks a ViewDepth field; add a
ViewDepth field to the StatementContext struct (matching the original type used
elsewhere in the project), and update any constructors/copy/snapshot/restore
logic so ViewDepth is properly preserved and copied (ensure sc.ViewDepth is
set/returned where StatementContext is initialized and that the
snapshot/Restore/Copy methods include ViewDepth alongside useDynamicPruneMode,
viewDepth, and colRefFromUpdatePlan). Use the existing symbols StatementContext,
sc.ViewDepth, Copy(), snapshot/restore (or the struct's constructor functions)
to locate where to add and wire this field.
|
done in #68646 |
2 participants
This is an automated cherry-pick of #66677
What problem does this PR solve?
Issue Number: ref #66676
Problem Summary:
This work is split into two steps for shared-AST multi logical plan build support.
This PR is Step 1 only: isolate planner build state so repeated builds from the same AST can be added safely in the follow-up step.
What changed and how does it work?
Main work in this PR (Step 1):
QBHintHandlerinto:StatementContextlogical-plan-build snapshot/restore support for mutable planning fields used by planner build/optimize paths (including warnings, plan-cache tracker state, andTableStats).Not included in this PR:
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