*: support system variable tidb_index_lookup_pushdown_policy and hi…
#20
Conversation
…nt `NO_INDEX_LOOKUP_PUSHDOWN` (pingcap#64932) (pingcap#65035) ref pingcap#62575
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR introduces the NO_INDEX_LOOKUP_PUSHDOWN optimizer hint and refactors index lookup push-down mechanism from a boolean flag to a typed enum system. A new session variable controls push-down policy (hint-only, force, affinity-force), enabling flexible control over when index lookups are pushed down. Changes
Sequence DiagramsequenceDiagram
participant Parser as Hint Parser
participant PlanHints as Plan Hints<br/>(ParsePlanHints)
participant SessionVar as Session Variable<br/>(tidb_index_lookup_pushdown_policy)
participant PlanBuilder as Plan Builder<br/>(checkIndexLookUpPushDown)
participant AccessPath as Access Path<br/>(IndexLookUpPushDownBy)
participant PlannedTask as Planned Task<br/>(PhysicalIndexLookUpReader)
Note over Parser,PlannedTask: Index Lookup Push-Down Decision Flow
Parser->>PlanHints: Parse NO_INDEX_LOOKUP_PUSHDOWN hint
PlanHints->>PlanHints: Populate NoIndexLookUpPushDown field
alt NO_INDEX_LOOKUP_PUSHDOWN Present
PlanBuilder->>PlanBuilder: forceNoIndexLookUpPushDown = true
PlanBuilder->>AccessPath: Set IndexLookUpPushDownBy = None<br/>(skip push-down)
else No Hint Restriction
PlanBuilder->>SessionVar: Read policy value
SessionVar-->>PlanBuilder: Policy (hint-only/force/affinity-force)
alt Policy = hint-only
PlanBuilder->>PlanHints: Check INDEX_LOOKUP_PUSHDOWN
alt INDEX_LOOKUP_PUSHDOWN Present
PlanBuilder->>AccessPath: Set IndexLookUpPushDownBy = ByHint
else No Hint
PlanBuilder->>AccessPath: Set IndexLookUpPushDownBy = None
end
else Policy = force
PlanBuilder->>AccessPath: Set IndexLookUpPushDownBy = BySysVar
else Policy = affinity-force
PlanBuilder->>AccessPath: Require affinity info
PlanBuilder->>AccessPath: Set IndexLookUpPushDownBy = BySysVar
end
end
AccessPath->>PlannedTask: Pass push-down type to Init()
PlannedTask->>PlannedTask: tryPushDownLookUp(ctx, pushDownType)
rect rgb(200, 220, 255)
Note over PlannedTask: Early Return if<br/>pushDownType = None
end
alt pushDownType = ByHint & keepOrder = true
PlannedTask->>PlannedTask: Emit warning: keep-order<br/>not supported with hint
end
PlannedTask-->>PlanBuilder: Finalized plan with<br/>push-down decision
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @kennedy8312, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility and control over the index lookup pushdown optimization within the query optimizer. By introducing a new system variable and a complementary SQL hint, users can now fine-tune the application of this optimization to better suit their specific workload requirements and performance goals. The underlying implementation has also been modernized to clearly distinguish the source of the pushdown decision. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the tidb_index_lookup_pushdown_policy system variable and the NO_INDEX_LOOKUP_PUSHDOWN hint, providing more granular control over the index lookup pushdown optimization. The implementation involves refactoring the boolean flag IsIndexLookUpPushDown to a new enum-like type IndexLookUpPushDownByType to represent the source of the pushdown decision (hint, system variable, or none). This change is propagated through various parts of the planner and execution logic. The PR also adds new test cases to cover the new functionality. Overall, the changes are well-structured and enhance the flexibility of the optimizer. I have one suggestion to improve a warning message for better user experience.
| ctx.GetSessionVars().StmtCtx.SetHintWarning( | ||
| "hint INDEX_LOOKUP_PUSHDOWN cannot be inapplicable, NO_INDEX_LOOKUP_PUSHDOWN is specified", | ||
| ) |
There was a problem hiding this comment.
The warning message "hint INDEX_LOOKUP_PUSHDOWN cannot be inapplicable, NO_INDEX_LOOKUP_PUSHDOWN is specified" is confusing due to the double negative "cannot be inapplicable". A clearer message would be better for user experience.
| ctx.GetSessionVars().StmtCtx.SetHintWarning( | |
| "hint INDEX_LOOKUP_PUSHDOWN cannot be inapplicable, NO_INDEX_LOOKUP_PUSHDOWN is specified", | |
| ) | |
| ctx.GetSessionVars().StmtCtx.SetHintWarning( | |
| "hint INDEX_LOOKUP_PUSHDOWN is inapplicable because NO_INDEX_LOOKUP_PUSHDOWN is also specified", | |
| ) |
![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
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/planbuilder_test.go (1)
1070-1170: Use enum-based Init correctly, but fix variable captured in helper checkYou’re correctly passing
util.IndexLookUpPushDownByHintintoPhysicalIndexLookUpReader.InitsoTestIndexLookUpReaderTryLookUpPushDownexercises the hint-driven path for pushdown.However, in the second
checkclosure you call:checkPushDownIndexLookUpReaderCommon(reader)instead of using the local
ryou just type-asserted fromp. This means the common assertions run against the originalreader, not the cloned / cached readers being tested, so part of the test isn’t actually validating those instances.Consider switching to
r:Proposed fix
- check = func(p base.Plan) { - r, ok := p.(*PhysicalIndexLookUpReader) - require.True(t, ok) - checkPushDownIndexLookUpReaderCommon(reader) + check = func(p base.Plan) { + r, ok := p.(*PhysicalIndexLookUpReader) + require.True(t, ok) + checkPushDownIndexLookUpReaderCommon(r) require.Equal(t, map[int]int{ }, r.IndexPlansUnNatureOrders) // ...
♻️ Duplicate comments (1)
pkg/planner/core/planbuilder.go (1)
1401-1412: Clarify the conflicting-hints warning messageThe conflict handling between
INDEX_LOOKUP_PUSHDOWNandNO_INDEX_LOOKUP_PUSHDOWNis correct, but the warning string
hint INDEX_LOOKUP_PUSHDOWN cannot be inapplicable, NO_INDEX_LOOKUP_PUSHDOWN is specifiedreads awkwardly due to the double negative “cannot be inapplicable”.
A clearer wording would be:
hint INDEX_LOOKUP_PUSHDOWN is inapplicable because NO_INDEX_LOOKUP_PUSHDOWN is also specifiedFor example:
- ctx.GetSessionVars().StmtCtx.SetHintWarning( - "hint INDEX_LOOKUP_PUSHDOWN cannot be inapplicable, NO_INDEX_LOOKUP_PUSHDOWN is specified", - ) + ctx.GetSessionVars().StmtCtx.SetHintWarning( + "hint INDEX_LOOKUP_PUSHDOWN is inapplicable because NO_INDEX_LOOKUP_PUSHDOWN is also specified", + )This keeps the same semantics while being easier for users to understand.
🧹 Nitpick comments (6)
pkg/sessionctx/variable/tidb_vars.go (1)
1254-1255: Newtidb_index_lookup_pushdown_policysysvar name and default are coherentDefining
TiDBIndexLookUpPushDownPolicy = "tidb_index_lookup_pushdown_policy"plusDefTiDBIndexLookUpPushDownPolicy = IndexLookUpPushDownPolicyHintOnlywires the new system variable cleanly into the existing naming/defaults structure and keeps current behavior as “hint-only” by default. Just ensure the correspondingSysVarentry’sPossibleValuesalign with the string constants invariable.goso validation remains centralized.Also applies to: 1632-1633
tests/integrationtest/t/executor/index_lookup_pushdown_partition.test (1)
14-25: Consider restoringtidb_index_lookup_pushdown_policyto default at test end.The test modifies
tidb_index_lookup_pushdown_policythrough multiple values (force, affinity-force, hint-only) but only sets it to 'hint-only' at line 23 and doesn't restore it to its default value. While line 69 restorestidb_scatter_region, missing a similar restore for the policy variable could affect subsequent tests if run in the same session.🔎 Suggested addition at end of test file
# restore tidb_scatter_region set @@tidb_scatter_region=default; +set @@tidb_index_lookup_pushdown_policy=default;pkg/planner/util/path.go (1)
108-109: Consider adding aString()method for debugging.The new
IndexLookUpPushDownByTypetype is used in plan construction and could benefit from aString()method for logging, debugging, or explain output. This is optional but would improve observability.🔎 Optional enhancement
// String returns the string representation of IndexLookUpPushDownByType. func (t IndexLookUpPushDownByType) String() string { switch t { case IndexLookUpPushDownNone: return "none" case IndexLookUpPushDownByHint: return "hint" case IndexLookUpPushDownBySysVar: return "sysvar" default: return "unknown" } }Also applies to: 144-144
pkg/util/hint/hint.go (1)
98-99: New NO_INDEX_LOOKUP_PUSHDOWN hint plumbing is coherentThe added
HintNoIndexLookUpPushDownconstant,PlanHints.NoIndexLookUpPushDownfield, theIfNoIndexLookUpPushDownhelper, and the constructor wiring all line up and are consistent with how other table-level hints are represented and queried viaMatchTableName. One minor follow-up you might consider (not blocking):
- If you want unmatched
NO_INDEX_LOOKUP_PUSHDOWNhints to surface warnings like other hints, you could extendCollectUnmatchedHintWarningsto look atPlanHints.NoIndexLookUpPushDownusing the existingExtractUnmatchedTables/collectUnmatchedJoinHintWarninghelpers.Also applies to: 533-552, 700-703, 949-971
pkg/planner/core/planbuilder.go (2)
1158-1188: Suppressible warnings incheckIndexLookUpPushDownSupportedlook correctThe refactor to gate warnings behind
suppressWarningcleanly separates hint-driven warnings from sysvar-driven auto-pushdown (which should stay silent). Callers are usingtrueonly for the policy-based path, which matches the intended behavior that only explicit hints emit user-facing warnings.If you want to make this easier to maintain, consider adding a short comment on the function explaining when
suppressWarningshould betrue(i.e., for internal auto decisions, not for user hints).
1190-1203:checkAutoForceIndexLookUpPushDownpolicy handling matches the new sysvar semanticsThe policy switch correctly encodes:
force→ always try auto pushdown (subject to support checks).affinity-force→ only auto push down whentblInfo.Affinity != nil.- default /
hint-only→ no auto pushdown.Delegating to
checkIndexLookUpPushDownSupported(..., true)avoids duplicate support logic and prevents spurious warnings from the sysvar path.Optionally, you might consider emitting a (low-level) diagnostic when
affinity-forceis set but a table has no affinity, to aid debugging misconfiguration, but that’s not strictly necessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
pkg/parser/ast/misc.gopkg/parser/hintparser.gopkg/parser/hintparser.ypkg/parser/misc.gopkg/planner/core/find_best_task.gopkg/planner/core/initialize.gopkg/planner/core/physical_plans.gopkg/planner/core/planbuilder.gopkg/planner/core/planbuilder_test.gopkg/planner/core/task.gopkg/planner/core/task_base.gopkg/planner/util/path.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.gopkg/sessionctx/variable/variable.gopkg/util/hint/hint.gotests/integrationtest/r/executor/index_lookup_pushdown.resulttests/integrationtest/r/executor/index_lookup_pushdown_partition.resulttests/integrationtest/t/executor/index_lookup_pushdown.testtests/integrationtest/t/executor/index_lookup_pushdown_partition.test
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/planner/core/planbuilder_test.go (1)
pkg/planner/util/path.go (2)
IndexLookUpPushDownNone(35-35)IndexLookUpPushDownByHint(37-37)
pkg/util/hint/hint.go (1)
pkg/parser/model/model.go (1)
NewCIStr(266-270)
pkg/sessionctx/variable/session.go (1)
pkg/sessionctx/variable/tidb_vars.go (1)
DefTiDBIndexLookUpPushDownPolicy(1632-1632)
pkg/sessionctx/variable/sysvar.go (3)
pkg/sessionctx/variable/variable.go (6)
ScopeGlobal(40-40)ScopeSession(42-42)TypeEnum(53-53)IndexLookUpPushDownPolicyHintOnly(100-100)IndexLookUpPushDownPolicyAffinityForce(102-102)IndexLookUpPushDownPolicyForce(104-104)pkg/metrics/metrics.go (2)
ScopeGlobal(58-58)ScopeSession(59-59)pkg/sessionctx/variable/tidb_vars.go (2)
TiDBIndexLookUpPushDownPolicy(1255-1255)DefTiDBIndexLookUpPushDownPolicy(1632-1632)
pkg/planner/core/planbuilder.go (5)
pkg/planner/core/base/plan_base.go (1)
PlanContext(35-35)pkg/meta/model/table.go (1)
TableInfo(95-205)pkg/sessionctx/variable/variable.go (2)
IndexLookUpPushDownPolicyForce(104-104)IndexLookUpPushDownPolicyAffinityForce(102-102)pkg/util/hint/hint.go (1)
HintedTable(559-565)pkg/planner/core/memtable_infoschema_extractor.go (1)
DBName(65-65)
pkg/planner/core/physical_plans.go (2)
pkg/planner/core/base/plan_base.go (1)
PlanContext(35-35)pkg/planner/util/path.go (3)
IndexLookUpPushDownByType(31-31)IndexLookUpPushDownNone(35-35)IndexLookUpPushDownByHint(37-37)
pkg/planner/core/task_base.go (1)
pkg/planner/util/path.go (1)
IndexLookUpPushDownByType(31-31)
🔇 Additional comments (20)
pkg/parser/ast/misc.go (1)
3942-3942: LGTM! Properly integrated into index hint restoration logic.The addition of "no_index_lookup_pushdown" correctly follows the same pattern as "index_lookup_pushdown" and other index-level hints, ensuring proper SQL text restoration.
pkg/parser/misc.go (1)
1074-1074: LGTM! Token mapping is consistent.The hint token mapping correctly follows the established pattern, with naming consistent with the positive hint variant "INDEX_LOOKUP_PUSHDOWN".
pkg/parser/hintparser.y (3)
117-117: LGTM! Token properly declared.The hint token declaration follows the established pattern and is consistent with the positive hint "INDEX_LOOKUP_PUSHDOWN" on line 116.
672-672: LGTM! Correctly categorized as index-level hint.The hint is properly added to
SupportedIndexLevelOptimizerHintName, making it available as an index-level optimizer hint alongside related hints like "INDEX_LOOKUP_PUSHDOWN".
776-776: Integration confirmed: NO_INDEX_LOOKUP_PUSHDOWN hint is properly wired through the planner.The hint is fully integrated with the planning layer:
- Defined in
pkg/util/hint/hint.gowith helper functionIfNoIndexLookUpPushDown- Consumed in
pkg/planner/core/planbuilder.gowhere it controls theforceNoIndexLookUpPushDownflag (lines 1242-1403)- System variable
TiDBIndexLookUpPushDownPolicyexists inpkg/sessionctx/variable/tidb_vars.go- Properly prevents index lookup push-down and prevents conflicting hints when specified
pkg/sessionctx/variable/session.go (2)
1723-1725: Session-level state for index lookup pushdown policy looks consistentThe new
IndexLookUpPushDownPolicyfield and comment are consistent with nearby optimizer/session switches and with the new system variable name. Exposing it atSessionVarslevel makes sense for planner access and per-session overrides. No structural issues here.
2261-2262: Default initialization matches defined policy constantInitializing
IndexLookUpPushDownPolicywithDefTiDBIndexLookUpPushDownPolicyaligns with the sysvar default (currentlyIndexLookUpPushDownPolicyHintOnlyintidb_vars.go), so new sessions start in the intended “hint-only” mode without relying on zero values. This wiring looks correct.pkg/planner/core/task.go (1)
520-531: Pass typed pushdown source intoPhysicalIndexLookUpReader.InitSwitching the Init argument from the old boolean to
t.indexLookUpPushDownBy(the typedIndexLookUpPushDownByType) matches the updated API and keeps the reader aware of whether pushdown is driven by hint/sysvar/none. As long asCopTask.indexLookUpPushDownByis populated in path construction (which this PR does elsewhere), this change is correct and improves clarity.pkg/planner/core/find_best_task.go (1)
2197-2255: Correctly propagating index lookup pushdown mode intoCopTaskWiring
cop.indexLookUpPushDownBy = candidate.path.IndexLookUpPushDownByin the double-read branch keeps the runtime task state consistent with the access path’s chosen pushdown mode and preserves the previous default (IndexLookUpPushDownNoneas zero value). No issues from this change alone.pkg/planner/core/planbuilder_test.go (1)
323-333: Init call updated to enum-based pushdown mode (clone test)Using
util.IndexLookUpPushDownNoneinindexLookup.Init(ctx, 0, util.IndexLookUpPushDownNone)matches the new Init signature and preserves the prior “no pushdown” behavior under clone tests. Looks consistent with the new enum design.pkg/sessionctx/variable/variable.go (1)
99-104: String constants for index lookup pushdown policy look consistentThe new
IndexLookUpPushDownPolicy*string constants are clear, documented, and suitable for use asPossibleValuesfor the corresponding sysvar. They also give you a single source of truth for default and validation logic.pkg/planner/core/physical_plans.go (1)
633-669: LGTM - Well-structured push-down type handling.The refactored
tryPushDownLookUpmethod properly handles the three push-down scenarios:
- Early return for
IndexLookUpPushDownNone- Warning emission only when the user explicitly requested push-down via hint (
IndexLookUpPushDownByHint) but keep-order is required- Silent fallback for system variable-driven push-down when keep-order prevents optimization
The conditional warning logic at lines 646-649 is appropriate - users should be informed when their explicit hint cannot be applied.
pkg/planner/core/task_base.go (1)
227-229: LGTM - Clean field type refactoring.The change from
indexLookUpPushDown booltoindexLookUpPushDownBy util.IndexLookUpPushDownByTypeis well-structured. The new field name clearly indicates both the purpose (push-down) and the source information (By type).tests/integrationtest/t/executor/index_lookup_pushdown.test (1)
49-76: LGTM - Comprehensive test coverage for new hint and policy.The test cases cover important scenarios:
- Priority verification:
NO_INDEX_LOOKUP_PUSHDOWNoverridesINDEX_LOOKUP_PUSHDOWN(line 50)- Syntax validation:
NO_INDEX_LOOKUP_PUSHDOWNdoesn't accept index name (line 53)- Policy values: force, affinity-force, hint-only (lines 56-73)
- Table affinity interaction (lines 65-68)
- Proper cleanup by restoring default (lines 74-75)
Good coverage for ensuring the feature works correctly across different configurations.
pkg/planner/core/initialize.go (1)
254-263: LGTM - Init signature updated correctly.The updated
Initmethod signature properly accepts the newIndexLookUpPushDownByTypeparameter and passes it totryPushDownLookUp. This ensures the push-down decision is made with full knowledge of its origin (none, hint, or system variable).pkg/planner/util/path.go (1)
30-40: LGTM - Well-defined enum type for push-down source tracking.The new
IndexLookUpPushDownByTypeenum cleanly categorizes the push-down source:
IndexLookUpPushDownNone(default/zero value): No push-downIndexLookUpPushDownByHint: User explicitly requested via hintIndexLookUpPushDownBySysVar: System variable triggered push-downUsing
iotaensures consistent ordering and the documentation for each constant is clear.pkg/sessionctx/variable/sysvar.go (1)
3554-3569: Sysvartidb_index_lookup_pushdown_policywiring looks consistentEnum setup, default value, hint-updatability flag, and
SetSessionassignment toSessionVars.IndexLookUpPushDownPolicyalign with surrounding sysvar patterns and the new tests; no issues spotted here.tests/integrationtest/r/executor/index_lookup_pushdown.result (1)
140-231: Result expectations for new policy + NO_INDEX_LOOKUP_PUSHDOWN behavior look consistentThe added blocks exercise conflicts between
INDEX_LOOKUP_PUSHDOWNandNO_INDEX_LOOKUP_PUSHDOWN, invalid argument forms, all enum policy values (force / affinity-force / hint-only / default), andSET_VARusage. The plans and warning texts align with the new hint handling and sysvar semantics in the code.pkg/planner/core/planbuilder.go (1)
1242-1253: NO_INDEX_LOOKUP_PUSHDOWN correctly overrides both sysvar and positive hintsThe
forceNoIndexLookUpPushDownflag andIfNoIndexLookUpPushDowncheck ensure:
- When NO_INDEX_LOOKUP_PUSHDOWN matches the current table,
forceNoIndexLookUpPushDownis true.- In the access-path loop, auto-pushdown from
tidb_index_lookup_pushdown_policyis skipped whenforceNoIndexLookUpPushDownis true.- Later, when processing
INDEX_LOOKUP_PUSHDOWNhints, the conflict branch seesforceNoIndexLookUpPushDownand prevents settingIndexLookUpPushDownBy.This yields the intended precedence: explicit NO hint > sysvar policy and positive hints, without altering unrelated tables.
Also applies to: 1287-1290
tests/integrationtest/r/executor/index_lookup_pushdown_partition.result (1)
33-66: Tests accurately exercise new pushdown policy and affinity behaviorThe added expectations around
@@tidb_index_lookup_pushdown_policy,forcevsaffinity-force, andalter table tp1 affinity = 'partition'line up with the planner changes:
- No
LocalIndexLookUpunderhint-only.- Auto pushdown under
force.- Auto pushdown under
affinity-forceonly after affinity is set.- The global-index-on-partition warning for
index_lookup_pushdown(tp1, c)matches the updated support check.This is good coverage of the new sysvar and hint semantics.
| sortMergeTables, inljTables, inlhjTables, inlmjTables, hashJoinTables, bcTables []HintedTable | ||
| noIndexJoinTables, noIndexHashJoinTables, noIndexMergeJoinTables []HintedTable | ||
| noHashJoinTables, noMergeJoinTables []HintedTable | ||
| noHashJoinTables, noMergeJoinTables, noIndexLookUpPushDownTables []HintedTable | ||
| shuffleJoinTables []HintedTable |
There was a problem hiding this comment.
Harden NO_INDEX_LOOKUP_PUSHDOWN parsing against empty table lists and reuse shared helper
case HintNoIndexLookUpPushDown currently indexes hint.Tables[0] directly. If a malformed hint ever produces an empty hint.Tables slice (e.g. NO_INDEX_LOOKUP_PUSHDOWN()), this will panic. Other table-scoped hints either:
- are pre-guarded in the “requires table names” switch, or
- go through
tableNames2HintTableInfo, which safely handleslen(hint.Tables) == 0.
You can avoid the potential panic and also get QB/partition handling for free by delegating to tableNames2HintTableInfo instead of manually constructing HintedTable:
Proposed fix for NO_INDEX_LOOKUP_PUSHDOWN handling
@@ func ParsePlanHints(...
- noHashJoinTables, noMergeJoinTables, noIndexLookUpPushDownTables []HintedTable
+ noHashJoinTables, noMergeJoinTables, noIndexLookUpPushDownTables []HintedTable
@@
- case HintAggToCop:
+ case HintAggToCop:
preferAggToCop = true
- case HintNoIndexLookUpPushDown:
- if len(hint.Indexes) > 0 {
- warnHandler.SetHintWarning(
- "hint NO_INDEX_LOOKUP_PUSH_DOWN is inapplicable, only table name without indexes is supported",
- )
- continue
- }
- dbName := hint.Tables[0].DBName
- if dbName.L == "" {
- dbName = pmodel.NewCIStr(currentDB)
- }
- noIndexLookUpPushDownTables = append(noIndexLookUpPushDownTables, HintedTable{
- DBName: dbName,
- TblName: hint.Tables[0].TableName,
- })
+ case HintNoIndexLookUpPushDown:
+ if len(hint.Indexes) > 0 {
+ warnHandler.SetHintWarning(
+ "hint NO_INDEX_LOOKUP_PUSH_DOWN is inapplicable, only table name without indexes is supported",
+ )
+ continue
+ }
+ noIndexLookUpPushDownTables = append(
+ noIndexLookUpPushDownTables,
+ tableNames2HintTableInfo(currentDB, hint.HintName.L, hint.Tables, hintProcessor, currentLevel, warnHandler)...,
+ )Also applies to: 835-849
🤖 Prompt for AI Agents
In pkg/util/hint/hint.go around lines 760 to 763, the NO_INDEX_LOOKUP_PUSHDOWN
case currently indexes hint.Tables[0] directly which will panic if hint.Tables
is empty; instead call the existing tableNames2HintTableInfo helper (which
safely handles len(hint.Tables)==0 and preserves QB/partition info) to produce
the []HintedTable and append that to noIndexLookUpPushDownTables; ensure you
remove the manual construction using hint.Tables[0] and reuse the same helper
pattern used by other table-scoped hints so malformed hints (e.g.
NO_INDEX_LOOKUP_PUSHDOWN()) do not cause a panic.
…nt
NO_INDEX_LOOKUP_PUSHDOWN(pingcap#64932) (pingcap#65035)ref pingcap#62575
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
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
NO_INDEX_LOOKUP_PUSHDOWNhint to disable index lookup push-down for specified tables.tidb_index_lookup_pushdown_policysystem variable with three modes (hint-only, affinity-force, force) for controlling default push-down behavior.Tests
✏️ Tip: You can customize this high-level summary in your review settings.