planner: fix LEADING hint generation for derived query blocks - #68561
planner: fix LEADING hint generation for derived query blocks#68561lichunzhu wants to merge 18 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
@lichunzhu I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
contribution
|
Hi @lichunzhu. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
|
|
Hi @lichunzhu. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
|
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:
📝 WalkthroughWalkthroughAdds staged alias-recovery and refined LEADING hint generation/restoration so EXPLAIN FORMAT='hint' and binding-from-history export stable, replayable leading(...) hints for queries with derived tables and CTEs; includes unit and cluster tests exercising generation, replay, and binding behavior. ChangesJoin leading hint recovery and export
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
lichunzhu
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: 1
🧹 Nitpick comments (1)
pkg/planner/core/explain_format_hint_roundtrip_test.go (1)
25-54: ⚡ Quick winConsolidate this derived-table roundtrip case with the existing planner tests.
This file duplicates the same schema/query setup and leading-hint expectation that the PR also adds in
pkg/planner/core/plan_test.go, which makes future hint-format updates easy to fix in one place and miss in the other.As per coding guidelines, "Prefer extending existing test suites and fixtures over creating new scaffolding".
🤖 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/explain_format_hint_roundtrip_test.go` around lines 25 - 54, This duplicates the derived-table hint roundtrip test: remove the duplicate constant explainFormatHintDerivedTableSQL, the helper prepareExplainFormatHintDerivedTableAliasTestKit, and the test TestExplainFormatHintRecoverableForDerivedTableAlias from this file and instead add the same case into the existing planner test suite (e.g., extend the test in plan_test.go) so it reuses the shared testkit/fixture; make the new case use the existing test helper/fixture in plan_test.go and assert the same expectations (contains leading(...) and no warnings) to avoid duplicated scaffolding.
🤖 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/hint_utils.go`:
- Around line 401-404: When qbOffset < 0 you must not carry the unresolved join
table handle into later LEADING generation; update the branch that currently
does qbOffsets = append(qbOffsets, -1) and hintTbls = append(hintTbls, ht) so
that hintTbls receives a nil entry instead of ht (i.e., append(nil)) so
unresolved join nodes are represented as nil and will be rejected by the LEADING
path; adjust any nearby logic that builds qbOffsets/hintTbls to keep the same
indexing semantics for -1 + nil pairs (look for qbOffset, ht, qbOffsets,
hintTbls and the LEADING generation code that checks for nil).
---
Nitpick comments:
In `@pkg/planner/core/explain_format_hint_roundtrip_test.go`:
- Around line 25-54: This duplicates the derived-table hint roundtrip test:
remove the duplicate constant explainFormatHintDerivedTableSQL, the helper
prepareExplainFormatHintDerivedTableAliasTestKit, and the test
TestExplainFormatHintRecoverableForDerivedTableAlias from this file and instead
add the same case into the existing planner test suite (e.g., extend the test in
plan_test.go) so it reuses the shared testkit/fixture; make the new case use the
existing test helper/fixture in plan_test.go and assert the same expectations
(contains leading(...) and no warnings) to avoid duplicated scaffolding.
🪄 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: abf7eac8-ddd5-4a0a-9cd2-5b48c1545263
📒 Files selected for processing (6)
pkg/infoschema/test/clustertablestest/cluster_tables_test.gopkg/parser/ast/misc.gopkg/planner/core/explain_format_hint_roundtrip_test.gopkg/planner/core/hint_utils.gopkg/planner/core/joinorder/util.gopkg/planner/core/plan_test.go
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
Codecov Report❌ Patch coverage is Please upload reports for the commit 81446a2 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #68561 +/- ##
================================================
- Coverage 76.3321% 73.4379% -2.8942%
================================================
Files 2041 2058 +17
Lines 558864 607736 +48872
================================================
+ Hits 426593 446309 +19716
- Misses 131371 158833 +27462
- Partials 900 2594 +1694
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
There was a problem hiding this comment.
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/joinorder/util.go (1)
524-532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same
CurrentDBfallback here.
extractDerivedTableAliasFromDescendants()now normalizes empty derived-table DB names toCurrentDB, but this path still matches against the raw empty DB name. A schema-qualified replayed/exportedLEADINGhint can still miss the derived alias here.Proposed fix
- derivedDBName := queryBlockNames[blockOffset].DBName.L + derivedDBName := queryBlockNames[blockOffset].DBName.L + if derivedDBName == "" { + derivedDBName = p.SCtx().GetSessionVars().CurrentDB + } // Check if this alias appears in the LEADING hint return containsTableInLeadingList(leadingHint.LeadingList, derivedDBName, derivedTableAlias)🤖 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 524 - 532, The code retrieves derivedDBName from queryBlockNames[blockOffset].DBName.L and may be empty even though extractDerivedTableAliasFromDescendants() normalizes empty DB names to CurrentDB; update the logic in extractDerivedTableAliasFromDescendants() call site (around derivedTableAlias/derivedDBName usage) to fallback to CurrentDB when derivedDBName == "" before calling containsTableInLeadingList so schema-qualified LEADING hints match exported/replayed aliases; reference derivedTableAlias, derivedDBName, queryBlockNames, blockOffset, containsTableInLeadingList and the CurrentDB fallback.
🤖 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.
Outside diff comments:
In `@pkg/planner/core/joinorder/util.go`:
- Around line 524-532: The code retrieves derivedDBName from
queryBlockNames[blockOffset].DBName.L and may be empty even though
extractDerivedTableAliasFromDescendants() normalizes empty DB names to
CurrentDB; update the logic in extractDerivedTableAliasFromDescendants() call
site (around derivedTableAlias/derivedDBName usage) to fallback to CurrentDB
when derivedDBName == "" before calling containsTableInLeadingList so
schema-qualified LEADING hints match exported/replayed aliases; reference
derivedTableAlias, derivedDBName, queryBlockNames, blockOffset,
containsTableInLeadingList and the CurrentDB fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1d34ca81-0ac6-4c8a-a8c7-a3df181df41b
📒 Files selected for processing (2)
pkg/planner/core/hint_utils.gopkg/planner/core/joinorder/util.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/planner/core/explain_format_hint_roundtrip_test.go (1)
77-82: ⚡ Quick winAdd replay assertion for the mixed-query-block case.
Line 80 currently verifies generation only. Add one replay pass to confirm the exported hint is parseable/stable through
EXPLAIN FORMAT='hint'roundtrip.Proposed test extension
func TestExplainFormatHintGeneratesMixedQueryBlockLeading(t *testing.T) { tk := prepareExplainFormatHintMixedLeadingTestKit(t) hints := tk.MustQuery("explain format='hint' " + explainFormatHintMixedQueryBlockLeadingSQL).Rows()[0][0] require.Contains(t, hints, "leading(`test`.`t2`, `test`.`t1`, `test`.`t3`@`sel_2`)") + + replayed := tk.MustQuery(fmt.Sprintf("explain format='hint' select /*+ %s */ * from t1 join t2 on t1.a = t2.a where t2.b in (select a from t3 where t3.b = 1) and t1.c = 1", hints)).Rows()[0][0] + require.Contains(t, replayed, "leading(`test`.`t2`, `test`.`t1`, `test`.`t3`@`sel_2`)") }🤖 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/explain_format_hint_roundtrip_test.go` around lines 77 - 82, The test TestExplainFormatHintGeneratesMixedQueryBlockLeading only checks hint generation; add a replay/assertion that the exported hint round-trips through EXPLAIN FORMAT='hint' and is parseable/stable. After capturing hints via hints := tk.MustQuery("explain format='hint' "+explainFormatHintMixedQueryBlockLeadingSQL).Rows()[0][0], call tk.MustQuery("explain format='hint' "+hints) and assert the result still contains the expected leading hint (e.g. require.Contains(..., "leading(`test`.`t2`, `test`.`t1`, `test`.`t3`@`sel_2`)")) so the exported hint can be parsed back; use the same test helpers (tk.MustQuery, require.Contains) and the existing explainFormatHintMixedQueryBlockLeadingSQL and hints variables to locate where to add this replay check.
🤖 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/hint_utils.go`:
- Around line 750-755: The fallback that restores the original mixed group is
too broad; narrow it to only the known-safe 3-item case by changing the
condition that checks trimmed and hintTbls. Replace the current condition "if
len(trimmed) <= 2 && len(hintTbls) > 2 { return hintTbls }" so it only returns
the original when the original group had exactly three items (e.g., "if
len(trimmed) <= 2 && len(hintTbls) == 3 { return hintTbls }"), ensuring larger
mixed groups like "2 outer + 2 inner" are not preserved.
---
Nitpick comments:
In `@pkg/planner/core/explain_format_hint_roundtrip_test.go`:
- Around line 77-82: The test
TestExplainFormatHintGeneratesMixedQueryBlockLeading only checks hint
generation; add a replay/assertion that the exported hint round-trips through
EXPLAIN FORMAT='hint' and is parseable/stable. After capturing hints via hints
:= tk.MustQuery("explain format='hint'
"+explainFormatHintMixedQueryBlockLeadingSQL).Rows()[0][0], call
tk.MustQuery("explain format='hint' "+hints) and assert the result still
contains the expected leading hint (e.g. require.Contains(...,
"leading(`test`.`t2`, `test`.`t1`, `test`.`t3`@`sel_2`)")) so the exported hint
can be parsed back; use the same test helpers (tk.MustQuery, require.Contains)
and the existing explainFormatHintMixedQueryBlockLeadingSQL and hints variables
to locate where to add this replay check.
🪄 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: 021acc57-19b2-4d8a-8027-e9b96a018c6d
📒 Files selected for processing (2)
pkg/planner/core/explain_format_hint_roundtrip_test.gopkg/planner/core/hint_utils.go
|
/retest |
ti-chi-bot
Bot
added
size/XXL
|
/retest |
ti-chi-bot
Bot
added
size/XL
ti-chi-bot
Bot
added
size/XL
ti-chi-bot
Bot
added
size/XXL
|
@lichunzhu: 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. |
|
[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 |
|
/test check-dev2 |
lichunzhu
changed the title
|
@lichunzhu: 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. |
3 participants
What problem does this PR solve?
Issue Number: close #68564
Problem Summary:
EXPLAIN FORMAT='hint'may miss or generate invalidLEADINGhints when a join group contains derived-table aliases or spans multiple query blocks. This makes exported hints incomplete and can breakcreate ... binding from historyreplay for these queries.What changed and how does it work?
LEADINGgeneration and matching; fail closed when visibility cannot be proven.LeadingListinHintDataso restored query-block placement remains stable.This PR guarantees round-trip generation and application for derived-table owner/visible aliases. It does not claim complete reproduction of every physical join order: physical join-group extraction still stops at join-group-preserving
Selection/Projectionwrappers, and traversal through those wrappers is deferred to #70351. Complete CTE alias support is tracked by #68977. Reconciling an original statementLEADINGhint with a plan-derivedLEADINGhint is tracked separately by #70238.Check List
Tests
check a complicate join case using a local tidb cluster. After this MR:
explain format='hint'can export correct leading hint.create binding from historycan make sure the physical plan is affected by given leading hint.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.