planner: pre-refactor for join reorder conflict detection algorithm#68870
Open
guo-shaoge wants to merge 6 commits into
Open
planner: pre-refactor for join reorder conflict detection algorithm#68870guo-shaoge wants to merge 6 commits into
guo-shaoge wants to merge 6 commits into
Conversation
Signed-off-by: guo-shaoge <shaoge1994@163.com>
Signed-off-by: guo-shaoge <shaoge1994@163.com>
…nto cp_pre_cdc_impl
…nto cp_pre_cdc_impl
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/cherry-pick-not-approved
release-note-none
|
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. |
ti-chi-bot
Bot
added
size/XXL
📝 WalkthroughWalkthroughThis PR introduces structured AST support for nested ChangesLEADING Hint Parsing and Propagation
Join-order Utility Library and Rule Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
guo-shaoge
changed the title
![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/util/hint/hint.go (1)
955-983:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
LeadingListwhen LEADING is invalidated.This branch invalidates conflicting
LEADINGhints by emptyingLeadingJoinOrder, but it still returns the parsedLeadingList. The new join-order helpers readPlanHints.LeadingListdirectly, soLEADING + STRAIGHT_JOINor multipleLEADINGhints can still influence join-order selection after being marked invalid here.Suggested fix
if leadingHintCnt > 1 || (leadingHintCnt > 0 && straightJoinOrder) { // If there are more leading hints or the straight_join hint existes, all leading hints will be invalid. leadingJoinOrder = leadingJoinOrder[:0] + leadingList = nil if leadingHintCnt > 1 { warnHandler.SetHintWarning("We can only use one leading hint at most, when multiple leading hints are used, all leading hints will be invalid") } else if straightJoinOrder { warnHandler.SetHintWarning("We can only use the straight_join hint, when we use the leading hint and straight_join hint at the same time, all leading hints will be invalid") } }🤖 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/util/hint/hint.go` around lines 955 - 983, The code invalidates LEADING by clearing leadingJoinOrder but still returns LeadingList, so PlanHints.LeadingList remains populated and can affect join-order; update the branch that checks leadingHintCnt and straightJoinOrder (the block manipulating leadingJoinOrder and warnHandler.SetHintWarning) to also clear leadingList (or set PlanHints.LeadingList to empty) when LEADING is deemed invalidated so the returned PlanHints has LeadingList cleared alongside LeadingJoinOrder and the same warning behavior is preserved.
🧹 Nitpick comments (1)
pkg/expression/schema.go (1)
86-100: ⚡ Quick winClarify that
Equalonly compares ordered columns.The implementation is narrower than the name/comment suggest: it ignores
KeysandUniqueKeys. That is fine for the new join-reorder caller, but the exported contract should say so explicitly or use a more specific name to avoid future misuse.As per coding guidelines, "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement".
🤖 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/expression/schema.go` around lines 86 - 100, The Schema.Equal method currently only compares ordered Columns and ignores Keys and UniqueKeys, which is narrower than its name suggests; update the exported doc comment for Schema.Equal to explicitly state it compares only the ordered Columns (and not Keys or UniqueKeys) or rename the method to a more specific name (e.g., EqualColumnsOrdered) to avoid misuse; locate the method by the symbol Schema.Equal and modify its comment to describe the exact contract (ordered column-wise equality) and, if renaming, update all call sites to use the new name.
🤖 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/parser/ast/misc.go`:
- Around line 3967-4005: The bug is that when the first LeadingList item is
itself a nested LeadingList, RestoreWithQB pushes the hint-level QB into the
child causing LEADING((`@qb` a, b), c) instead of LEADING(`@qb` (a, b), c); modify
LeadingList.RestoreWithQB so that in the case "case *LeadingList" if i == 0 and
currentQBName.L != "" and !qbOnTable you emit the "@<qb>"
(ctx.WriteKeyWord("@"); ctx.WriteName(currentQBName.String()); ctx.WritePlain("
")) before restoring the child and then call t.RestoreWithQB with an empty
model.CIStr for qbName (so the child does not receive the QB), afterwards clear
currentQBName; otherwise keep the existing behavior of passing currentQBName
into the child.
In `@pkg/parser/hintparser_test.go`:
- Around line 369-474: The LEADING hint test cases use ast.NewCIStr but NewCIStr
is defined in the model package; update the LEADING test block to replace all
ast.NewCIStr(...) occurrences with model.NewCIStr(...). Specifically modify the
HintName fields and all HintTable.TableName constructors in the
TableOptimizerHint / LeadingList test cases so they use model.NewCIStr, leaving
the surrounding structures (TableOptimizerHint, LeadingList, HintTable,
HintName, Tables) unchanged.
In `@pkg/planner/core/joinorder/util.go`:
- Around line 221-225: The fallback path in util.go currently computes dbMatch
as "astTbl.DBName.L == '' || astTbl.DBName.L == blockName.DBName.L", which drops
the "*" DB wildcard semantics; update the dbMatch logic used in the blockOffset
> 1 fallback so it treats "*" as a wildcard (e.g., accept when astTbl.DBName.L
== "*" or blockName.DBName.L == "*") in addition to the empty or exact-match
cases, keeping tableMatch unchanged; modify the dbMatch check near variables
blockOffset, queryBlockNames, blockName, astTbl, dbMatch so LEADING(*.alias)
still matches derived-table aliases in this fallback path.
---
Outside diff comments:
In `@pkg/util/hint/hint.go`:
- Around line 955-983: The code invalidates LEADING by clearing leadingJoinOrder
but still returns LeadingList, so PlanHints.LeadingList remains populated and
can affect join-order; update the branch that checks leadingHintCnt and
straightJoinOrder (the block manipulating leadingJoinOrder and
warnHandler.SetHintWarning) to also clear leadingList (or set
PlanHints.LeadingList to empty) when LEADING is deemed invalidated so the
returned PlanHints has LeadingList cleared alongside LeadingJoinOrder and the
same warning behavior is preserved.
---
Nitpick comments:
In `@pkg/expression/schema.go`:
- Around line 86-100: The Schema.Equal method currently only compares ordered
Columns and ignores Keys and UniqueKeys, which is narrower than its name
suggests; update the exported doc comment for Schema.Equal to explicitly state
it compares only the ordered Columns (and not Keys or UniqueKeys) or rename the
method to a more specific name (e.g., EqualColumnsOrdered) to avoid misuse;
locate the method by the symbol Schema.Equal and modify its comment to describe
the exact contract (ordered column-wise equality) and, if renaming, update all
call sites to use the new name.
🪄 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: 362404df-e24a-4cdb-ab10-6a67cd93d0ba
📒 Files selected for processing (14)
pkg/expression/schema.gopkg/parser/ast/misc.gopkg/parser/ast/misc_test.gopkg/parser/hintparser.gopkg/parser/hintparser.ypkg/parser/hintparser_test.gopkg/parser/parser_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/joinorder/BUILD.bazelpkg/planner/core/joinorder/util.gopkg/planner/core/operator/logicalop/logical_projection.gopkg/planner/core/plan_cost_ver2.gopkg/planner/core/rule_join_reorder.gopkg/util/hint/hint.go
Comment on lines
+3967
to
+4005
| func (lt *LeadingList) RestoreWithQB(ctx *format.RestoreCtx, qbName model.CIStr, needParen bool, isTop bool, qbOnTable bool) error { | ||
| if lt == nil || len(lt.Items) == 0 { | ||
| return nil | ||
| } | ||
| if needParen { | ||
| ctx.WritePlain("(") | ||
| } | ||
|
|
||
| currentQBName := qbName // hint level QBName | ||
|
|
||
| for i, item := range lt.Items { | ||
| if i > 0 { | ||
| ctx.WritePlain(", ") | ||
| } | ||
|
|
||
| switch t := item.(type) { | ||
| case *HintTable: | ||
| if i == 0 && currentQBName.L != "" && !qbOnTable { | ||
| ctx.WriteKeyWord("@") | ||
| ctx.WriteName(currentQBName.String()) | ||
| ctx.WritePlain(" ") | ||
| t.Restore(ctx) | ||
| currentQBName = model.CIStr{} | ||
| } else { | ||
| t.Restore(ctx) | ||
| } | ||
| case *LeadingList: | ||
| if err := t.RestoreWithQB(ctx, currentQBName, true, false, qbOnTable); err != nil { | ||
| return err | ||
| } | ||
| currentQBName = model.CIStr{} | ||
| default: | ||
| return fmt.Errorf("unexpected type in LeadingList: %T", t) | ||
| } | ||
| } | ||
| if needParen { | ||
| ctx.WritePlain(")") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Preserve the hint-level QB before a parenthesized first LEADING group.
When the first LEADING element is nested, Line 3994 pushes QBName into the child list, so Restore emits LEADING((@QB a, b), c) instead of LEADING(@QB (a, b), c). That breaks restore/parse round-tripping for valid hints with a hint-level query block.
Suggested fix
case *LeadingList:
- if err := t.RestoreWithQB(ctx, currentQBName, true, false, qbOnTable); err != nil {
+ childQBName := currentQBName
+ if i == 0 && childQBName.L != "" && !qbOnTable {
+ ctx.WriteKeyWord("@")
+ ctx.WriteName(childQBName.String())
+ ctx.WritePlain(" ")
+ childQBName = model.CIStr{}
+ }
+ if err := t.RestoreWithQB(ctx, childQBName, true, false, qbOnTable); err != nil {
return err
}
currentQBName = model.CIStr{}🤖 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/parser/ast/misc.go` around lines 3967 - 4005, The bug is that when the
first LeadingList item is itself a nested LeadingList, RestoreWithQB pushes the
hint-level QB into the child causing LEADING((`@qb` a, b), c) instead of
LEADING(`@qb` (a, b), c); modify LeadingList.RestoreWithQB so that in the case
"case *LeadingList" if i == 0 and currentQBName.L != "" and !qbOnTable you emit
the "@<qb>" (ctx.WriteKeyWord("@"); ctx.WriteName(currentQBName.String());
ctx.WritePlain(" ")) before restoring the child and then call t.RestoreWithQB
with an empty model.CIStr for qbName (so the child does not receive the QB),
afterwards clear currentQBName; otherwise keep the existing behavior of passing
currentQBName into the child.
Comment on lines
+369
to
+474
| { | ||
| input: "LEADING(a,(b,(c,d)))", | ||
| output: []*ast.TableOptimizerHint{ | ||
| { | ||
| HintName: ast.NewCIStr("LEADING"), | ||
| HintData: &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("a")}, | ||
| &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("b")}, | ||
| &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("c")}, | ||
| &ast.HintTable{TableName: ast.NewCIStr("d")}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Tables: []ast.HintTable{ | ||
| {TableName: ast.NewCIStr("a")}, | ||
| {TableName: ast.NewCIStr("b")}, | ||
| {TableName: ast.NewCIStr("c")}, | ||
| {TableName: ast.NewCIStr("d")}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| input: "LEADING(a,b,c)", | ||
| output: []*ast.TableOptimizerHint{ | ||
| { | ||
| HintName: ast.NewCIStr("LEADING"), | ||
| HintData: &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("a")}, | ||
| &ast.HintTable{TableName: ast.NewCIStr("b")}, | ||
| &ast.HintTable{TableName: ast.NewCIStr("c")}, | ||
| }, | ||
| }, | ||
| Tables: []ast.HintTable{ | ||
| {TableName: ast.NewCIStr("a")}, | ||
| {TableName: ast.NewCIStr("b")}, | ||
| {TableName: ast.NewCIStr("c")}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| input: "LEADING((a,b),(c,d))", | ||
| output: []*ast.TableOptimizerHint{ | ||
| { | ||
| HintName: ast.NewCIStr("LEADING"), | ||
| HintData: &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("a")}, | ||
| &ast.HintTable{TableName: ast.NewCIStr("b")}, | ||
| }, | ||
| }, | ||
| &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("c")}, | ||
| &ast.HintTable{TableName: ast.NewCIStr("d")}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| Tables: []ast.HintTable{ | ||
| {TableName: ast.NewCIStr("a")}, | ||
| {TableName: ast.NewCIStr("b")}, | ||
| {TableName: ast.NewCIStr("c")}, | ||
| {TableName: ast.NewCIStr("d")}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| input: "LEADING(x,(y,z),w)", | ||
| output: []*ast.TableOptimizerHint{ | ||
| { | ||
| HintName: ast.NewCIStr("LEADING"), | ||
| HintData: &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("x")}, | ||
| &ast.LeadingList{ | ||
| Items: []interface{}{ | ||
| &ast.HintTable{TableName: ast.NewCIStr("y")}, | ||
| &ast.HintTable{TableName: ast.NewCIStr("z")}, | ||
| }, | ||
| }, | ||
| &ast.HintTable{TableName: ast.NewCIStr("w")}, | ||
| }, | ||
| }, | ||
| Tables: []ast.HintTable{ | ||
| {TableName: ast.NewCIStr("x")}, | ||
| {TableName: ast.NewCIStr("y")}, | ||
| {TableName: ast.NewCIStr("z")}, | ||
| {TableName: ast.NewCIStr("w")}, | ||
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking whether pkg/parser/ast defines or re-exports NewCIStr..."
rg -n '(^func NewCIStr\b|^var NewCIStr\b|type CIStr\b)' pkg/parser/ast pkg/parser/model
echo
echo "Showing NewCIStr call sites in the updated test..."
rg -n 'ast\.NewCIStr|model\.NewCIStr' pkg/parser/hintparser_test.goRepository: pingcap/tidb
Length of output: 6357
Fix build break in LEADING hint assertions: use model.NewCIStr (not ast.NewCIStr)
pkg/parser/hintparser_test.go’s LEADING cases construct HintName: ast.NewCIStr(...) and ast.HintTable{TableName: ast.NewCIStr(...)}, but NewCIStr is defined in pkg/parser/model (not pkg/parser/ast). Replace all ast.NewCIStr(...) with model.NewCIStr(...) in this block.
Suggested fix
- HintName: ast.NewCIStr("LEADING"),
+ HintName: model.NewCIStr("LEADING"),
...
- &ast.HintTable{TableName: ast.NewCIStr("a")},
+ &ast.HintTable{TableName: model.NewCIStr("a")},📝 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
| { | |
| input: "LEADING(a,(b,(c,d)))", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: ast.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("a")}, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("b")}, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("c")}, | |
| &ast.HintTable{TableName: ast.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: ast.NewCIStr("a")}, | |
| {TableName: ast.NewCIStr("b")}, | |
| {TableName: ast.NewCIStr("c")}, | |
| {TableName: ast.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING(a,b,c)", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: ast.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("a")}, | |
| &ast.HintTable{TableName: ast.NewCIStr("b")}, | |
| &ast.HintTable{TableName: ast.NewCIStr("c")}, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: ast.NewCIStr("a")}, | |
| {TableName: ast.NewCIStr("b")}, | |
| {TableName: ast.NewCIStr("c")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING((a,b),(c,d))", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: ast.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("a")}, | |
| &ast.HintTable{TableName: ast.NewCIStr("b")}, | |
| }, | |
| }, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("c")}, | |
| &ast.HintTable{TableName: ast.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: ast.NewCIStr("a")}, | |
| {TableName: ast.NewCIStr("b")}, | |
| {TableName: ast.NewCIStr("c")}, | |
| {TableName: ast.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING(x,(y,z),w)", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: ast.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("x")}, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: ast.NewCIStr("y")}, | |
| &ast.HintTable{TableName: ast.NewCIStr("z")}, | |
| }, | |
| }, | |
| &ast.HintTable{TableName: ast.NewCIStr("w")}, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: ast.NewCIStr("x")}, | |
| {TableName: ast.NewCIStr("y")}, | |
| {TableName: ast.NewCIStr("z")}, | |
| {TableName: ast.NewCIStr("w")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING(a,(b,(c,d)))", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: model.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("a")}, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("b")}, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("c")}, | |
| &ast.HintTable{TableName: model.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: model.NewCIStr("a")}, | |
| {TableName: model.NewCIStr("b")}, | |
| {TableName: model.NewCIStr("c")}, | |
| {TableName: model.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING(a,b,c)", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: model.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("a")}, | |
| &ast.HintTable{TableName: model.NewCIStr("b")}, | |
| &ast.HintTable{TableName: model.NewCIStr("c")}, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: model.NewCIStr("a")}, | |
| {TableName: model.NewCIStr("b")}, | |
| {TableName: model.NewCIStr("c")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING((a,b),(c,d))", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: model.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("a")}, | |
| &ast.HintTable{TableName: model.NewCIStr("b")}, | |
| }, | |
| }, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("c")}, | |
| &ast.HintTable{TableName: model.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: model.NewCIStr("a")}, | |
| {TableName: model.NewCIStr("b")}, | |
| {TableName: model.NewCIStr("c")}, | |
| {TableName: model.NewCIStr("d")}, | |
| }, | |
| }, | |
| }, | |
| }, | |
| { | |
| input: "LEADING(x,(y,z),w)", | |
| output: []*ast.TableOptimizerHint{ | |
| { | |
| HintName: model.NewCIStr("LEADING"), | |
| HintData: &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("x")}, | |
| &ast.LeadingList{ | |
| Items: []interface{}{ | |
| &ast.HintTable{TableName: model.NewCIStr("y")}, | |
| &ast.HintTable{TableName: model.NewCIStr("z")}, | |
| }, | |
| }, | |
| &ast.HintTable{TableName: model.NewCIStr("w")}, | |
| }, | |
| }, | |
| Tables: []ast.HintTable{ | |
| {TableName: model.NewCIStr("x")}, | |
| {TableName: model.NewCIStr("y")}, | |
| {TableName: model.NewCIStr("z")}, | |
| {TableName: model.NewCIStr("w")}, | |
| }, | |
| }, | |
| }, | |
| }, |
🤖 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/parser/hintparser_test.go` around lines 369 - 474, The LEADING hint test
cases use ast.NewCIStr but NewCIStr is defined in the model package; update the
LEADING test block to replace all ast.NewCIStr(...) occurrences with
model.NewCIStr(...). Specifically modify the HintName fields and all
HintTable.TableName constructors in the TableOptimizerHint / LeadingList test
cases so they use model.NewCIStr, leaving the surrounding structures
(TableOptimizerHint, LeadingList, HintTable, HintName, Tables) unchanged.
Comment on lines
+221
to
+225
| if blockOffset > 1 && blockOffset < len(queryBlockNames) { | ||
| blockName := queryBlockNames[blockOffset] | ||
| dbMatch := astTbl.DBName.L == "" || astTbl.DBName.L == blockName.DBName.L | ||
| tableMatch := astTbl.TableName.L == blockName.TableName.L | ||
| if dbMatch && tableMatch { |
There was a problem hiding this comment.
Preserve * DB wildcard in the subquery-alias fallback.
The fallback path drops the wildcard DB semantics that direct matching already supports. LEADING(*.alias) can match a derived-table alias in step 1, but the same binding stops matching in step 2 because dbMatch only accepts empty or exact DB names here.
Suggested fix
- dbMatch := astTbl.DBName.L == "" || astTbl.DBName.L == blockName.DBName.L
+ dbMatch := astTbl.DBName.L == "" || astTbl.DBName.L == blockName.DBName.L || astTbl.DBName.L == "*"🤖 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/joinorder/util.go` around lines 221 - 225, The fallback path
in util.go currently computes dbMatch as "astTbl.DBName.L == '' ||
astTbl.DBName.L == blockName.DBName.L", which drops the "*" DB wildcard
semantics; update the dbMatch logic used in the blockOffset > 1 fallback so it
treats "*" as a wildcard (e.g., accept when astTbl.DBName.L == "*" or
blockName.DBName.L == "*") in addition to the empty or exact-match cases,
keeping tableMatch unchanged; modify the dbMatch check near variables
blockOffset, queryBlockNames, blockName, astTbl, dbMatch so LEADING(*.alias)
still matches derived-table aliases in this fallback path.
|
@guo-shaoge: 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68870 +/- ##
================================================
Coverage ? 42.2244%
================================================
Files ? 1535
Lines ? 428602
Branches ? 0
================================================
Hits ? 180975
Misses ? 231545
Partials ? 16082
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
AilinKid
approved these changes
Jun 2, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: AilinKid 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 |
[LGTM Timeline notifier]Timeline:
|
|
@guo-shaoge: 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.
What problem does this PR solve?
Issue Number: close #66088
Problem Summary: manually cherry pick #66087
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
Release Notes
New Features
Improvements