planner: pre-refactor for join reorder conflict detection algorithm#68870
Merged
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
📝 WalkthroughWalkthroughThis PR extracts join-order hint handling logic into a new ChangesJoin-order utility extraction and planner integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68870 +/- ##
================================================
Coverage ? 55.0895%
================================================
Files ? 1826
Lines ? 657991
Branches ? 0
================================================
Hits ? 362484
Misses ? 268482
Partials ? 27025
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
…_impl Signed-off-by: guo-shaoge <shaoge1994@163.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/rule_join_reorder.go`:
- Around line 449-474: Remove the dead duplicate function
checkAndGenerateLeadingHint from this file: the project already uses
joinorder.CheckAndGenerateLeadingHint(joinOrderHintInfo) (see usage at the other
call site), so delete the local checkAndGenerateLeadingHint definition to avoid
duplicate/unreferenced code; ensure no other local references to
checkAndGenerateLeadingHint remain and run tests/compile to confirm nothing else
relied on this duplicate.
🪄 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: 7a9ba0bb-3c35-4ac1-913e-5b9d4467fa69
📒 Files selected for processing (4)
pkg/parser/hintparser.ypkg/parser/parser_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/rule_join_reorder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/planner/core/BUILD.bazel
- pkg/parser/parser_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/rule_join_reorder.go`:
- Around line 449-474: Remove the dead duplicate function
checkAndGenerateLeadingHint from this file: the project already uses
joinorder.CheckAndGenerateLeadingHint(joinOrderHintInfo) (see usage at the other
call site), so delete the local checkAndGenerateLeadingHint definition to avoid
duplicate/unreferenced code; ensure no other local references to
checkAndGenerateLeadingHint remain and run tests/compile to confirm nothing else
relied on this duplicate.
🪄 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: 7a9ba0bb-3c35-4ac1-913e-5b9d4467fa69
📒 Files selected for processing (4)
pkg/parser/hintparser.ypkg/parser/parser_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/rule_join_reorder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/planner/core/BUILD.bazel
- pkg/parser/parser_test.go
🛑 Comments failed to post (1)
pkg/planner/core/rule_join_reorder.go (1)
449-474:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDead code: local
checkAndGenerateLeadingHintduplicatesjoinorder.CheckAndGenerateLeadingHintand is never called.Line 342 uses
joinorder.CheckAndGenerateLeadingHint(joinOrderHintInfo), while this local function has an identical implementation and signature but is never invoked.🧹 Remove dead code
-// checkAndGenerateLeadingHint used to check and generate the valid leading hint. -// We are allowed to use at most one leading hint in a join group. When more than one, -// all leading hints in the current join group will be invalid. -// For example: select /*+ leading(t3) */ * from (select /*+ leading(t1) */ t2.b from t1 join t2 on t1.a=t2.a) t4 join t3 on t4.b=t3.b -// The Join Group {t1, t2, t3} contains two leading hints includes leading(t3) and leading(t1). -// Although they are in different query blocks, they are conflicting. -// In addition, the table alias 't4' cannot be recognized because of the join group. -func checkAndGenerateLeadingHint(hintInfo []*h.PlanHints) (*h.PlanHints, bool) { - leadingHintNum := len(hintInfo) - var leadingHintInfo *h.PlanHints - hasDiffLeadingHint := false - if leadingHintNum > 0 { - leadingHintInfo = hintInfo[0] - // One join group has one leading hint at most. Check whether there are different join order hints. - for i := 1; i < leadingHintNum; i++ { - if hintInfo[i] != hintInfo[i-1] { - hasDiffLeadingHint = true - break - } - } - if hasDiffLeadingHint { - leadingHintInfo = nil - } - } - return leadingHintInfo, hasDiffLeadingHint -}📝 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.🤖 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/rule_join_reorder.go` around lines 449 - 474, Remove the dead duplicate function checkAndGenerateLeadingHint from this file: the project already uses joinorder.CheckAndGenerateLeadingHint(joinOrderHintInfo) (see usage at the other call site), so delete the local checkAndGenerateLeadingHint definition to avoid duplicate/unreferenced code; ensure no other local references to checkAndGenerateLeadingHint remain and run tests/compile to confirm nothing else relied on this duplicate.
…_impl Signed-off-by: guo-shaoge <shaoge1994@163.com>
ti-chi-bot
Bot
added
size/XL
Collaborator
Author
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AilinKid, windtalker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
ti-chi-bot
Bot
added
cherry-pick-approved
This was referenced Jun 5, 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
3 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
there is no logic change in this PR, only new uilt functions are added, which will be used in the next PR: #68878
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
LEADINGhints, enabling more efficient query plan generation.