executor: preserve ParentIdx for correlated index_lookup_pushdown under Apply#67548
Conversation
…x_lookup_pushdown
ti-chi-bot
Bot
added
the
release-note
|
@lcwangchao 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
the
size/L
📝 WalkthroughWalkthroughThis pull request preserves un‑natural order parent/child index mapping for correlated index‑lookup pushdown: the planner's Changes
Sequence Diagram(s)sequenceDiagram
participant Planner
participant IndexLookupExecutor
participant Builder
participant TiKV
Planner->>IndexLookupExecutor: create with IndexPlansUnNatureOrders
IndexLookupExecutor->>IndexLookupExecutor: open() checks corColInIdxSide
alt corColInIdxSide == true
IndexLookupExecutor->>Builder: ConstructListBasedDistExecForUnNatureOrderPlans(idxPlans, unNatureOrders)
Builder->>Builder: build list executors
Builder->>Builder: set ParentIdx on executors per unNatureOrders
Builder-->>IndexLookupExecutor: return executors with ParentIdx
else
IndexLookupExecutor->>Builder: ConstructListBasedDistExec(idxPlans)
Builder-->>IndexLookupExecutor: return executors
end
IndexLookupExecutor->>TiKV: send DAG with ParentIdx relationships
TiKV-->>IndexLookupExecutor: execute and return rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
|
@pantheon-ai review this PR |
|
⏳ @lcwangchao I've received your follow-up and will continue on this pull request. I'll update this comment when I have something to share. ℹ️ Learn more details on Pantheon AI. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/executor/internal/builder/builder_utils.go (1)
56-59: GuardunNatureOrdersindices before assigningParentIdx.This loop can panic if a malformed mapping slips in (
executors[i]out of range). Since the function already returnserror, validate indices and fail with context.Proposed defensive patch
import ( + "github.com/pingcap/errors" "github.com/pingcap/tidb/pkg/distsql" "github.com/pingcap/tidb/pkg/kv" plannercore "github.com/pingcap/tidb/pkg/planner/core/base" @@ for i, j := range unNatureOrders { + if i < 0 || i >= len(executors) || j < 0 || j >= len(executors) { + return nil, errors.Errorf("invalid unNatureOrders mapping child=%d parent=%d, executors=%d", i, j, len(executors)) + } parentIdx := uint32(j) executors[i].ParentIdx = &parentIdx }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/internal/builder/builder_utils.go` around lines 56 - 59, The loop assigning ParentIdx from unNatureOrders can panic if indices are out of range; update the loop that iterates over unNatureOrders to validate bounds before assigning: for each pair (i, j) ensure i < len(executors) and 0 <= j < len(executors) (or whatever target slice length is appropriate), and if a check fails return a descriptive error (include i and j) instead of panicking; keep using parentIdx as uint32 and only set executors[i].ParentIdx = &parentIdx after the validations succeed.
🤖 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/executor/builder.go`:
- Line 4568: Add a unit test that exercises IndexLookUpExecutor.open() when
indexLookUpPushDown && corColInIdxSide to validate the rebuild path preserves
ParentIdx mappings after idxPlanUnNatureOrders propagation; locate the
IndexLookUpExecutor type and its open() method and construct a minimal executor
stack (mock/real IndexReader/IndexLookup/IndexPlan variants) where
idxPlanUnNatureOrders differs, call open(), and assert rebuilt index-side
executors' ParentIdx fields equal the originals (and that behavior tied to
idxPlanUnNatureOrders is preserved). Ensure the test lives under pkg/executor,
uses existing test helpers to build indexLookUpPushDown scenarios, and focuses
only on the rebuild/ParentIdx assertions (no integration dependencies).
In `@tests/integrationtest/t/executor/index_lookup_pushdown.test`:
- Around line 204-225: Add a focused unit test in the executor package that
specifically exercises the ParentIdx restoration path used by list-based
distributed execution rebuilds: create a small synthetic plan/operator that
simulates an Apply with a correlated subquery and the index_lookup_pushdown
hint, then force the "unnatural" parent mapping scenario and assert that
RestoreParentIdx (or the method in pkg/executor responsible for restoring parent
index mappings) correctly re-maps parent indices after rebuilding the exec list;
reference the executor code paths that handle ParentIdx, RestoreParentIdx (or
equivalent functions/classes in pkg/executor), and the list-based dist exec
rebuild logic so the new UT isolates and validates the mapping restoration
behavior independent of the larger integration test.
---
Nitpick comments:
In `@pkg/executor/internal/builder/builder_utils.go`:
- Around line 56-59: The loop assigning ParentIdx from unNatureOrders can panic
if indices are out of range; update the loop that iterates over unNatureOrders
to validate bounds before assigning: for each pair (i, j) ensure i <
len(executors) and 0 <= j < len(executors) (or whatever target slice length is
appropriate), and if a check fails return a descriptive error (include i and j)
instead of panicking; keep using parentIdx as uint32 and only set
executors[i].ParentIdx = &parentIdx after the validations succeed.
🪄 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: dda119ec-7aa3-411a-ac99-d60c32315eec
📒 Files selected for processing (7)
pkg/executor/builder.gopkg/executor/distsql.gopkg/executor/internal/builder/builder_utils.gopkg/store/mockstore/unistore/cophandler/cop_handler.gotests/integrationtest/r/executor/index_lookup_pushdown.resulttests/integrationtest/t/executor/index_lookup_pushdown.testtests/realtikvtest/pushdowntest/index_lookup_pushdown_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67548 +/- ##
================================================
+ Coverage 77.7526% 78.7419% +0.9892%
================================================
Files 1959 1973 +14
Lines 543378 553466 +10088
================================================
+ Hits 422491 435810 +13319
+ Misses 120046 117216 -2830
+ Partials 841 440 -401
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrationtest/r/executor/index_lookup_pushdown.result`:
- Around line 568-606: Add a focused executor unit test that exercises the
ParentIdx restoration code path used when rebuilding IndexLookUp executors
(specifically target IndexLookUpExecutor.open and the list-based distributed
execution construction / ParentIdx restore logic), rather than only relying on
the integration golden test; create a test under pkg/executor that constructs an
IndexLookUpExecutor in the same way the dist exec rebuild path does (simulate
list-based child construction or call the helper used for distributed rebuild),
invoke open() to trigger ParentIdx restoration, and assert that restored
ParentIdx/state is correct (e.g., index ranges, pushed-down selection, and
aggregate behavior) to prevent regressions during refactors.
🪄 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: 920cf8c0-d926-4362-9251-03df3d604bd1
📒 Files selected for processing (3)
tests/integrationtest/r/executor/index_lookup_pushdown.resulttests/integrationtest/t/executor/index_lookup_pushdown.testtests/realtikvtest/pushdowntest/index_lookup_pushdown_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/realtikvtest/pushdowntest/index_lookup_pushdown_test.go
- tests/integrationtest/t/executor/index_lookup_pushdown.test
|
/retest |
|
/test unit-test |
|
@lcwangchao: The specified target(s) for Use DetailsIn response to this:
Instructions 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. |
|
/retest |
1 similar comment
|
/retest |
|
/test unit-test |
|
@lcwangchao: The specified target(s) for Use DetailsIn response to this:
Instructions 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
approved
needs-1-more-lgtm
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, winoros 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
lcwangchao
added
the
needs-cherry-pick-release-8.5
|
In response to a cherrypick label: new pull request created to branch |
5 participants
What problem does this PR solve?
Issue Number: close #67546
Problem Summary:
Correlated subqueries under
Applycan fail when the inner side usesINDEX_LOOKUP_PUSHDOWN. In that path,IndexLookUpExecutor.open()rebuilds the index-side list DAG whencorColInIdxSideis true, but it previously rebuilt the executor list without restoring the non-natural parent relationship needed byLocalIndexLookUp. This can surface asUnexpected non-first executor TypeTableScanfrom TiKV batch executor.What changed and how does it work?
This PR preserves the un-natural parent mapping when rebuilding the index-side pushed-down DAG for correlated
IndexLookUpExecutors.IndexPlansUnNatureOrdersinIndexLookUpExecutorParentIdxIndexLookUpExecutor.open()whenindexLookUpPushDown && corColInIdxSideINDEX_LOOKUP_PUSHDOWNunderApplyEXPLAIN FORMAT='plan_tree'and deterministic orderingCheck List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests
Chores