Repository navigation
planner: index correlated range predicates | tidb-test=pr/2696 - #66719
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
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:
📝 WalkthroughWalkthroughDetects correlated-column range predicates (LT/GT/LE/GE) and promotes them to index access conditions; adds a test verifying this behavior and updates planner/executor test expectations and plan outputs to reflect IndexRangeScan usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 |
|
Hi @terry1purcell. 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. |
![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
🤖 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/integration_test.go`:
- Around line 1583-1667: The test TestCorColRangePredicateAccess claims to cover
LT/GT/LE/GE but omits GE; add a GE case mirroring the LT/GT/LE blocks: run an
"explain format='brief' select ... where exists (select /*+ NO_DECORRELATE() */
1 from t1 t1b where t1b.b = t1a.b and t1b.a >= t1a.a)" into the existing rows
variable, scan rows like the other cases for info := fmt.Sprintf("%v", row[4])
and assert strings.Contains(info, "decided by") && strings.Contains(info,
"ge(test.t1.a, test.t1.a)") with require.True and caller message, and add a
small correctness query (similar to the LE/GT correctness checks) that selects
the matching rows and calls .Check(...) to validate results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5473b2ca-0e0b-485d-a1ef-a58c592cfdd7
📒 Files selected for processing (2)
pkg/planner/core/integration_test.gopkg/planner/util/path.go
terry1purcell
changed the title
terry1purcell
changed the title
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| foundRangeAccess := false | ||
| for _, row := range rows { | ||
| info := fmt.Sprintf("%v", row[4]) | ||
| if strings.Contains(info, "decided by") && strings.Contains(info, "lt(test.t1.a, test.t1.a)") { |
There was a problem hiding this comment.
[P2] Test asserts raw EXPLAIN string fragments — brittle CI
Why: strings.Contains matching against human-readable EXPLAIN FORMAT='brief' output means any unrelated formatting change (operator rename, column-qualification change) can silently break this test despite correct plan semantics.
Evidence: pkg/planner/core/integration_test.go:1607 — strings.Contains(info, "lt(test.t1.a, test.t1.a)"). The same fragile pattern also appears at lines 1637 (gt(...)) and 1659 (le(...)). Consider instead asserting on a structured plan representation or a purpose-built test helper that is decoupled from EXPLAIN's exact text format.
There was a problem hiding this comment.
This is not a critical concern. Many tests use this type of string comparison to search for text within the plan.
There was a problem hiding this comment.
Fair point — strings.Contains on EXPLAIN FORMAT='brief' output is a well-established convention throughout TiDB's integration tests, so this is consistent with the project's existing style. The P2 flag was a low-priority heads-up rather than a blocker. No action needed here.
ℹ️ Learn more details on Pantheon AI.
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/executor/partition_table_test.go (1)
1570-1581:⚠️ Potential issue | 🟠 MajorIndexLookUp plan assertions still validate
tinnerinstead of partition tables.Line 1570 and Line 1613
EXPLAINqueries are still againsttinner, so the updated expectations on Line 1578 and Line 1621 don’t actually verify the hash/range partition IndexLookUp path described by those test sections.🔧 Proposed test correction
- tk.MustQuery(`explain format='brief' select * from touter where touter.a > (select sum(tinner.b) from tinner use index(a) where tinner.a>touter.b)`).Check(testkit.Rows( + tk.MustQuery(`explain format='brief' select * from touter where touter.a > (select sum(thash.b) from thash use index(a) where thash.a>touter.b)`).Check(testkit.Rows( ... - ` ├─Selection(Build) 80000000.00 cop[tikv] gt(test_parallel_apply.tinner.a, test_parallel_apply.touter.b)`, - ` │ └─IndexRangeScan 100000000.00 cop[tikv] table:tinner, index:a(a) range: decided by [gt(test_parallel_apply.tinner.a, test_parallel_apply.touter.b)], keep order:false, stats:pseudo`, - ` └─HashAgg(Probe) 10000.00 cop[tikv] funcs:sum(test_parallel_apply.tinner.b)->Column#11`, - ` └─TableRowIDScan 80000000.00 cop[tikv] table:tinner keep order:false, stats:pseudo`)) + ` ├─Selection(Build) 80000000.00 cop[tikv] gt(test_parallel_apply.thash.a, test_parallel_apply.touter.b)`, + ` │ └─IndexRangeScan 100000000.00 cop[tikv] table:thash, index:a(a) range: decided by [gt(test_parallel_apply.thash.a, test_parallel_apply.touter.b)], keep order:false, stats:pseudo`, + ` └─HashAgg(Probe) 10000.00 cop[tikv] funcs:sum(test_parallel_apply.thash.b)->Column#11`, + ` └─TableRowIDScan 80000000.00 cop[tikv] table:thash keep order:false, stats:pseudo`)) - tk.MustQuery(`explain format='brief' select * from touter where touter.a > (select sum(tinner.b) from tinner use index(a) where tinner.a>touter.b)`).Check(testkit.Rows( + tk.MustQuery(`explain format='brief' select * from touter where touter.a > (select sum(trange.b) from trange use index(a) where trange.a>touter.b)`).Check(testkit.Rows( ... - ` ├─Selection(Build) 80000000.00 cop[tikv] gt(test_parallel_apply.tinner.a, test_parallel_apply.touter.b)`, - ` │ └─IndexRangeScan 100000000.00 cop[tikv] table:tinner, index:a(a) range: decided by [gt(test_parallel_apply.tinner.a, test_parallel_apply.touter.b)], keep order:false, stats:pseudo`, - ` └─HashAgg(Probe) 10000.00 cop[tikv] funcs:sum(test_parallel_apply.tinner.b)->Column#11`, - ` └─TableRowIDScan 80000000.00 cop[tikv] table:tinner keep order:false, stats:pseudo`)) + ` ├─Selection(Build) 80000000.00 cop[tikv] gt(test_parallel_apply.trange.a, test_parallel_apply.touter.b)`, + ` │ └─IndexRangeScan 100000000.00 cop[tikv] table:trange, index:a(a) range: decided by [gt(test_parallel_apply.trange.a, test_parallel_apply.touter.b)], keep order:false, stats:pseudo`, + ` └─HashAgg(Probe) 10000.00 cop[tikv] funcs:sum(test_parallel_apply.trange.b)->Column#11`, + ` └─TableRowIDScan 80000000.00 cop[tikv] table:trange keep order:false, stats:pseudo`))As per coding guidelines
**/*_test.go: MUST add a regression test and verify it fails before fix and passes after fix for bug fixes.Also applies to: 1613-1624
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/partition_table_test.go` around lines 1570 - 1581, The EXPLAIN assertions still reference the non-partition table `tinner`; update the two EXPLAIN queries and their expected output to target the partitioned tables used in the surrounding tests (e.g., `thash` for the hash-partition test and `trange` for the range-partition test) so the IndexLookUp/IndexRangeScan lines assert against `table:thash`/`table:trange` and `test_parallel_apply.thash.*`/`test_parallel_apply.trange.*` columns instead of `tinner`; modify the expected strings (the lines containing "table:tinner" and related selection/index text) to match the partition table names and ensure the inner child comment about IndexLookUp still aligns with the `Apply` child structure (methods: the EXPLAIN queries and their Check expectations around IndexLookUp/IndexRangeScan/Selection/HashAgg).
🤖 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/planner/core/casetest/rule/rule_join_reorder.result`:
- Line 4625: Add a regression test to rule_join_reorder.test that covers a NULL
outer correlated value (e.g., create rows where t1.a IS NULL and run the
correlated range query such as "select min(t1.a) from t2 where t2.a > t1.a") so
the planner sees col > NULL and should not treat it as a usable range predicate;
update the test input in rule_join_reorder.test to include the NULL-case and
then regenerate the snapshot file
tests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.result so
the expected plan no longer shows the correlated predicate being used as an
index range.
---
Outside diff comments:
In `@pkg/executor/partition_table_test.go`:
- Around line 1570-1581: The EXPLAIN assertions still reference the
non-partition table `tinner`; update the two EXPLAIN queries and their expected
output to target the partitioned tables used in the surrounding tests (e.g.,
`thash` for the hash-partition test and `trange` for the range-partition test)
so the IndexLookUp/IndexRangeScan lines assert against
`table:thash`/`table:trange` and
`test_parallel_apply.thash.*`/`test_parallel_apply.trange.*` columns instead of
`tinner`; modify the expected strings (the lines containing "table:tinner" and
related selection/index text) to match the partition table names and ensure the
inner child comment about IndexLookUp still aligns with the `Apply` child
structure (methods: the EXPLAIN queries and their Check expectations around
IndexLookUp/IndexRangeScan/Selection/HashAgg).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 28639803-65b2-4d35-87d6-2a1826e476b5
📒 Files selected for processing (2)
pkg/executor/partition_table_test.gotests/integrationtest/r/planner/core/casetest/rule/rule_join_reorder.result
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66719 +/- ##
================================================
+ Coverage 77.6930% 77.8330% +0.1400%
================================================
Files 2008 1929 -79
Lines 549479 540774 -8705
================================================
- Hits 426907 420901 -6006
+ Misses 120907 119858 -1049
+ Partials 1665 15 -1650
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/integration_test.go (1)
1631-1701: Consider one swapped-operand case to cover both correlated-range directions.Current predicates are all
inner_col OP outer_col. Adding oneouter_col OP inner_colcase would protect the alternate operand-order branch in correlated range detection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/integration_test.go` around lines 1631 - 1701, Add a complementary test that uses the swapped operand order (outer_col OP inner_col) to exercise the alternate correlated-range detection branch: add an explain+correctness pair like the existing GT/LE/GE blocks but with the predicate reversed (e.g., "select * from t1 t1a where exists (select 1 from t1 t1b where t1a.a < t1b.a and t1b.b = t1a.b)" with NO_DECORRELATE hint), assert that explain output contains the expected "decided by" string for the corresponding operator (e.g., "lt(test.t1.a, test.t1.a)") using the same foundRangeAccess/rows checks, and add a matching tk.MustQuery correctness check (ordered results) similar to the other blocks so the swapped-operand case is covered.
🤖 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/integration_test.go`:
- Around line 1631-1701: Add a complementary test that uses the swapped operand
order (outer_col OP inner_col) to exercise the alternate correlated-range
detection branch: add an explain+correctness pair like the existing GT/LE/GE
blocks but with the predicate reversed (e.g., "select * from t1 t1a where exists
(select 1 from t1 t1b where t1a.a < t1b.a and t1b.b = t1a.b)" with
NO_DECORRELATE hint), assert that explain output contains the expected "decided
by" string for the corresponding operator (e.g., "lt(test.t1.a, test.t1.a)")
using the same foundRangeAccess/rows checks, and add a matching tk.MustQuery
correctness check (ordered results) similar to the other blocks so the
swapped-operand case is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69c7a565-cb29-41a0-a133-3eabe3266a66
📒 Files selected for processing (1)
pkg/planner/core/integration_test.go
|
/retest-required |
|
@terry1purcell: PRs from untrusted users cannot be marked as trusted with 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. |
| ` └─IndexLookUp 10000.00 root `, // IndexLookUp is a inner child of Apply | ||
| ` ├─Selection(Build) 80000000.00 cop[tikv] gt(test_parallel_apply.tinner.a, test_parallel_apply.touter.b)`, | ||
| ` │ └─IndexFullScan 100000000.00 cop[tikv] table:tinner, index:a(a) keep order:false, stats:pseudo`, | ||
| ` │ └─IndexRangeScan 100000000.00 cop[tikv] table:tinner, index:a(a) range: decided by [gt(test_parallel_apply.tinner.a, test_parallel_apply.touter.b)], keep order:false, stats:pseudo`, |
There was a problem hiding this comment.
These are good changes, IndexRangeScan should be better than IndexFullScan.
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fixdb, qw4990 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 |
1 similar comment
|
/retest |
4 participants
What problem does this PR solve?
Issue Number: close #66720
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
Improvements
Tests