planner: preserve index join hint warning after advanced join reorder#67838
planner: preserve index join hint warning after advanced join reorder#67838hawkingrei wants to merge 5 commits into
Conversation
ti-chi-bot
Bot
added
release-note-none
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
📝 WalkthroughWalkthroughRemaps join-method and vertex hints after join-group optimization by rebinding hint entries keyed by pre-optimization plan IDs to optimized plan IDs when eligible; adds detection for which join-method hints require rebinding; and strengthens a test to assert inapplicable Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.11.4)Command failed 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: 1
🧹 Nitpick comments (3)
pkg/planner/core/physical_plan_test.go (1)
361-361: Redundant session variable set.
@@session.tidb_opt_advanced_join_hint=0is already set at line 306 and is not mutated between there and here, so thissetis a no-op. Minor nit; feel free to drop it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/physical_plan_test.go` at line 361, Remove the redundant session variable set by deleting the duplicate tk.MustExec("set @@session.tidb_opt_advanced_join_hint=0") call in the test; the variable is already set earlier and not changed, so simply remove this no-op invocation (search for the exact tk.MustExec call in the physical plan test to locate and drop it).pkg/planner/core/joinorder/join_order.go (1)
325-338: Duplicate ofrule_join_reorder.go::rebindJoinMethodHints.Same signature (modulo the named type alias), same body. Consider exporting this helper from the
joinorderpackage and havingrule_join_reorder.gocall into it, to keep the two sites in sync. See the paired comment onrule_join_reorder.gofor a consolidation sketch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/joinorder/join_order.go` around lines 325 - 338, The function rebindVertexHints duplicates logic from rebindJoinMethodHints; extract the common logic into a single exported helper (e.g., RebindJoinMethodHints) in the joinorder package and have both rebindVertexHints and rule_join_reorder.go call that helper; update references to use the new exported function name and remove the duplicate body from rebindVertexHints (or replace it with a thin wrapper) so both sites share the same implementation and stay in sync.pkg/planner/core/rule_join_reorder.go (1)
418-431: Consider consolidating withjoinorder.rebindVertexHints.
rebindJoinMethodHintshere andrebindVertexHintsinpkg/planner/core/joinorder/join_order.gooperate on the exact same type (map[int]*joinorder.JoinMethodHint) with identical semantics. You could expose a single helper from thejoinorderpackage and reuse it in both sites to avoid drift between the two implementations.♻️ Suggested consolidation sketch
// In pkg/planner/core/joinorder/join_order.go (rename/export): // RebindVertexHints remaps hint keys from pre-optimization plan IDs to the // corresponding optimized plan IDs using vertexMap. func RebindVertexHints(vertexHints map[int]*JoinMethodHint, vertexMap map[int]base.LogicalPlan) map[int]*JoinMethodHint { ... }Then in
rule_join_reorder.go, replace the local helper withjoinorder.RebindVertexHints(result.joinMethodHintInfo, optimizedVertexMap).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/rule_join_reorder.go` around lines 418 - 431, The two functions rebindJoinMethodHints (in rule_join_reorder.go) and rebindVertexHints (in pkg/planner/core/joinorder/join_order.go) duplicate logic for remapping map[int]*joinorder.JoinMethodHint keys using an optimizedVertexMap; remove the local rebindJoinMethodHints and instead export or add a single helper in the joinorder package (e.g., RebindVertexHints(vertexHints map[int]*JoinMethodHint, vertexMap map[int]base.LogicalPlan) map[int]*JoinMethodHint) and call joinorder.RebindVertexHints(result.joinMethodHintInfo, optimizedVertexMap) from rule_join_reorder.go so both call sites share the same implementation and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/physical_plan_test.go`:
- Around line 325-367: The new test block asserting Warning 1815 is only valid
for the legacy planner and will fail under the cascades planner; modify the test
inside TestJoinHintCompatibilityWithVariable to early-skip when cascades is
enabled by checking the cascades variable (cascades == "on") and calling t.Skip
with a short message (e.g., "cascades planner does not check join hint
applicability") before executing the sql, expectedWarn and session toggles so
the warning assertions only run for the legacy planner.
---
Nitpick comments:
In `@pkg/planner/core/joinorder/join_order.go`:
- Around line 325-338: The function rebindVertexHints duplicates logic from
rebindJoinMethodHints; extract the common logic into a single exported helper
(e.g., RebindJoinMethodHints) in the joinorder package and have both
rebindVertexHints and rule_join_reorder.go call that helper; update references
to use the new exported function name and remove the duplicate body from
rebindVertexHints (or replace it with a thin wrapper) so both sites share the
same implementation and stay in sync.
In `@pkg/planner/core/physical_plan_test.go`:
- Line 361: Remove the redundant session variable set by deleting the duplicate
tk.MustExec("set @@session.tidb_opt_advanced_join_hint=0") call in the test; the
variable is already set earlier and not changed, so simply remove this no-op
invocation (search for the exact tk.MustExec call in the physical plan test to
locate and drop it).
In `@pkg/planner/core/rule_join_reorder.go`:
- Around line 418-431: The two functions rebindJoinMethodHints (in
rule_join_reorder.go) and rebindVertexHints (in
pkg/planner/core/joinorder/join_order.go) duplicate logic for remapping
map[int]*joinorder.JoinMethodHint keys using an optimizedVertexMap; remove the
local rebindJoinMethodHints and instead export or add a single helper in the
joinorder package (e.g., RebindVertexHints(vertexHints map[int]*JoinMethodHint,
vertexMap map[int]base.LogicalPlan) map[int]*JoinMethodHint) and call
joinorder.RebindVertexHints(result.joinMethodHintInfo, optimizedVertexMap) from
rule_join_reorder.go so both call sites share the same implementation and avoid
drift.
🪄 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: 75650891-1879-4378-8215-2dbc677eb0ae
📒 Files selected for processing (3)
pkg/planner/core/joinorder/join_order.gopkg/planner/core/physical_plan_test.gopkg/planner/core/rule_join_reorder.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/joinorder/join_order.go (1)
319-332: Minor: potential key collision between rebound and fallback entries.
rebindVertexHintswritesrebuilt[optimizedVertex.ID()]for entries found invertexMapand falls back torebuilt[oldID]otherwise. In practice every hint key should be present invertexMap(hints are keyed by the child IDs that become vertexes inextractJoinGroupat lines 183/190, andvertexMapis populated for every vertex at lines 271–278), so the fallback branch is effectively dead code today.If the invariant ever weakens, though, a fallback
oldIDcould shadow another entry's reboundoptimizedVertex.ID()(or vice versa) depending on iteration order, silently dropping a hint. Consider either:
- Asserting the invariant with
intest.Assert(ok, ...)/ a warning log so a future regression is visible, or- Detecting collisions before overwriting.
Not blocking for this PR — the current call site guarantees the mapping is total.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/joinorder/join_order.go` around lines 319 - 332, The function rebindVertexHints can silently drop hints when an oldID fallback collides with a rebound optimizedVertex.ID(); add an explicit invariant check so regressions are visible: inside loop over vertexHints in rebindVertexHints, assert that optimizedVertex exists (using intest.Assert or your project's assertion utility) instead of silently using the fallback branch, or if you prefer non-fatal handling, detect when rebuilt[optimizedVertex.ID()] or rebuilt[oldID] would overwrite an existing entry and emit a warning/log including oldID and optimizedVertex.ID() before choosing which value to keep; reference rebindVertexHints, vertexHints, vertexMap, optimizedVertex.ID() and oldID when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/joinorder/join_order.go`:
- Around line 319-332: The function rebindVertexHints can silently drop hints
when an oldID fallback collides with a rebound optimizedVertex.ID(); add an
explicit invariant check so regressions are visible: inside loop over
vertexHints in rebindVertexHints, assert that optimizedVertex exists (using
intest.Assert or your project's assertion utility) instead of silently using the
fallback branch, or if you prefer non-fatal handling, detect when
rebuilt[optimizedVertex.ID()] or rebuilt[oldID] would overwrite an existing
entry and emit a warning/log including oldID and optimizedVertex.ID() before
choosing which value to keep; reference rebindVertexHints, vertexHints,
vertexMap, optimizedVertex.ID() and oldID when implementing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ded127f0-011e-4255-9d8b-3c71c96ac66a
📒 Files selected for processing (2)
pkg/planner/core/joinorder/join_order.gopkg/planner/core/rule_join_reorder.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/rule_join_reorder.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67838 +/- ##
================================================
- Coverage 77.5894% 76.9830% -0.6064%
================================================
Files 1982 1966 -16
Lines 548964 550269 +1305
================================================
- Hits 425938 423614 -2324
- Misses 122221 126635 +4414
+ Partials 805 20 -785
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/joinorder/util.go (1)
37-41: Optional: hoist the mask to a package-level const and use!= 0.The
indexJoinMaskis a pure compile-time constant combination ofhint.Prefer*flags. Defining it once at package scope makes the intent explicit (and documents which hint families are considered "index-join family") and avoids recomputing it on each call. Using!= 0is also the more idiomatic bit-test in Go than> 0.♻️ Proposed refactor
+// indexJoinHintMask covers the hint.Prefer* flags for the index-join family +// (INLJ / INLHJ / INLMJ and their NO_* counterparts). Hints carrying any of +// these bits are the ones that must follow a vertex across recursive subtree +// optimization, so their inapplicable-warning path is preserved. +const indexJoinHintMask = hint.PreferINLJ | hint.PreferINLHJ | hint.PreferINLMJ | + hint.PreferNoIndexJoin | hint.PreferNoIndexHashJoin | hint.PreferNoIndexMergeJoin + // ShouldRebindJoinMethodHint reports whether a join method hint should follow // the optimized vertex ID after recursive subtree optimization. func ShouldRebindJoinMethodHint(preferJoinMethod uint) bool { - indexJoinMask := hint.PreferINLJ | hint.PreferINLHJ | hint.PreferINLMJ | - hint.PreferNoIndexJoin | hint.PreferNoIndexHashJoin | hint.PreferNoIndexMergeJoin - return preferJoinMethod&indexJoinMask > 0 + return preferJoinMethod&indexJoinHintMask != 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/joinorder/util.go` around lines 37 - 41, Hoist the local mask in ShouldRebindJoinMethodHint into a package-level constant (e.g., indexJoinMask) that combines hint.PreferINLJ, hint.PreferINLHJ, hint.PreferINLMJ, hint.PreferNoIndexJoin, hint.PreferNoIndexHashJoin and hint.PreferNoIndexMergeJoin so the set is defined once and documented, then update ShouldRebindJoinMethodHint to test the bits with `preferJoinMethod & indexJoinMask != 0` instead of `> 0`; keep the constant name `indexJoinMask` and the function name ShouldRebindJoinMethodHint to make the change easy to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/planner/core/joinorder/util.go`:
- Around line 37-41: Hoist the local mask in ShouldRebindJoinMethodHint into a
package-level constant (e.g., indexJoinMask) that combines hint.PreferINLJ,
hint.PreferINLHJ, hint.PreferINLMJ, hint.PreferNoIndexJoin,
hint.PreferNoIndexHashJoin and hint.PreferNoIndexMergeJoin so the set is defined
once and documented, then update ShouldRebindJoinMethodHint to test the bits
with `preferJoinMethod & indexJoinMask != 0` instead of `> 0`; keep the constant
name `indexJoinMask` and the function name ShouldRebindJoinMethodHint to make
the change easy to locate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 70a84138-a2da-4f06-a46f-1926ccc3f3cc
📒 Files selected for processing (3)
pkg/planner/core/joinorder/join_order.gopkg/planner/core/joinorder/util.gopkg/planner/core/rule_join_reorder.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/joinorder/join_order.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/planner/core/rule_join_reorder.go
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/planner/core/joinorder/util.go (1)
35-64: Consider documenting why the rebind mask is index-join-only, and the fallback intent afterintest.Assert.Two small maintainability notes on the new rebind helpers:
indexJoinHintMaskdeliberately excludesPreferHashJoin/PreferMergeJoin/side-specific hash-join build/probe hints that can also land inhints(seerule_join_reorder.go:122-132). If this narrowing is intentional (scope of the current fix, or those cases are handled elsewhere), please add a one-line comment above the mask explaining the rationale — otherwise a future reader extending join-method hints will likely wire a new constant intoPreferJoinTypewithout knowing it also needs to appear here.ShouldRebindJoinMethodHintreads like a general predicate but actually encodes "is an index-join-family hint". A name likeisIndexJoinHint(keeping the exportedRebindJoinMethodHintsas the public entry point) would better match what it does; alternatively, keep the name and widen its scope. As-is, the name-vs-behavior gap is a minor readability hazard.- On the
intest.Assert(ok, ...)/if ok { … continue }/ fall-through torebuilt[oldID] = hintInfopattern: in release builds the assert is a no-op and the hint silently keeps the stale key, which means a missing vertex invertexMapwill quietly produce a dropped hint (and, for this PR's motivating case, a missing inapplicable-hint warning). Since the invariant is supposedly guaranteed by the caller, either dropping the fallback (keep only the assert) or logging once would make the failure mode less silent.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants … and SHOULD NOT restate what the code already makes clear."
🧹 Suggested comment additions
-const indexJoinHintMask = hint.PreferINLJ | hint.PreferINLHJ | hint.PreferINLMJ | - hint.PreferNoIndexJoin | hint.PreferNoIndexHashJoin | hint.PreferNoIndexMergeJoin +// indexJoinHintMask covers the join-method hints whose binding to a specific +// child vertex must follow the optimized vertex ID after recursive subtree +// optimization (side-specific INL_JOIN / INL_HASH_JOIN / INL_MERGE_JOIN and +// their NO_* counterparts). Other join-method hints are not rebound here +// because <explain reason>. +const indexJoinHintMask = hint.PreferINLJ | hint.PreferINLHJ | hint.PreferINLMJ | + hint.PreferNoIndexJoin | hint.PreferNoIndexHashJoin | hint.PreferNoIndexMergeJoin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/joinorder/util.go` around lines 35 - 64, Add a one-line comment above indexJoinHintMask explaining it intentionally covers only index-join-related hint flags (e.g., PreferINLJ/INLHJ/INLMJ and their "no index" variants) because hash/merge/side-specific hints are handled elsewhere; either rename ShouldRebindJoinMethodHint to isIndexJoinHint (or change its implementation) so its name matches its narrow intent, and in RebindJoinMethodHints replace the silent fall-through after intest.Assert(ok, ...) with an explicit handling strategy (remove the fallback so the assert is the only behavior, or emit a one-time log/error when ok is false) to avoid quietly keeping stale keys when vertexMap lacks oldID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/rule_join_reorder.go`:
- Around line 315-326: The rebinding here uses optimizedVertexMap and
joinorder.RebindJoinMethodHints(result.joinMethodHintInfo, optimizedVertexMap)
to remap child IDs, but indexJoinHintMask currently documents/handles only
index-join and anti-index-join hints and deliberately excludes side-specific
hash-join hints (PreferHJBuild/PreferHJProbe) and merge-join side hints, which
causes those hints to be silently dropped when curJoinGroup entries are replaced
by optimizeRecursive; add a clear comment on the indexJoinHintMask definition
explaining it covers only index/anti-index join hints and explicitly call out
that side-specific hash-join and merge-join hints are not handled and must be
addressed in a follow-up change to avoid silent loss of those hints during
rebinding (reference indexJoinHintMask, optimizedVertexMap, curJoinGroup,
result.joinMethodHintInfo, joinorder.RebindJoinMethodHints).
---
Nitpick comments:
In `@pkg/planner/core/joinorder/util.go`:
- Around line 35-64: Add a one-line comment above indexJoinHintMask explaining
it intentionally covers only index-join-related hint flags (e.g.,
PreferINLJ/INLHJ/INLMJ and their "no index" variants) because
hash/merge/side-specific hints are handled elsewhere; either rename
ShouldRebindJoinMethodHint to isIndexJoinHint (or change its implementation) so
its name matches its narrow intent, and in RebindJoinMethodHints replace the
silent fall-through after intest.Assert(ok, ...) with an explicit handling
strategy (remove the fallback so the assert is the only behavior, or emit a
one-time log/error when ok is false) to avoid quietly keeping stale keys when
vertexMap lacks oldID.
🪄 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: c02d2b3a-7fd5-4f76-92eb-5620193eff41
📒 Files selected for processing (4)
pkg/planner/core/joinorder/join_order.gopkg/planner/core/joinorder/util.gopkg/planner/core/physical_plan_test.gopkg/planner/core/rule_join_reorder.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/planner/core/physical_plan_test.go
- pkg/planner/core/joinorder/join_order.go
|
/retest |
|
@pantheon-bot review |
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
1 participant
What problem does this PR solve?
Issue Number: close #65838
Problem Summary:
When
tidb_opt_advanced_join_hint=ON, a side-specificINL_JOIN(t1)hint on a join with semijoin-derived children could silently lose its inapplicable warning even though the final plan still did not honor the original hinted join edge.What changed and how does it work?
INL_JOIN(t1)warning is preserved under bothtidb_opt_advanced_join_hint=0and=1.The root cause was that advanced join reorder stored hint metadata by the original child vertex IDs, but recursive optimization could rebuild those child vertices before join reconstruction. After that vertex-ID drift, the reconstructed join could lose the original hint metadata and therefore skip the warning path.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests