planner: correlate subquery rule#66206
Conversation
|
General (non-inline) review: systemic optimizer issues surfaced by the new correlate rule This PR adds a new logical rule P1: Predicate push down cannot reliably participate in follow-up or interaction optimizations
Recommendation:
P2:
Recommendation:
|
|
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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66206 +/- ##
================================================
+ Coverage 77.7850% 78.1728% +0.3877%
================================================
Files 1983 1976 -7
Lines 549122 550035 +913
================================================
+ Hits 427135 429978 +2843
+ Misses 121067 119627 -1440
+ Partials 920 430 -490
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
Adds an (optional) logical-optimizer rule to “re-correlate” certain semi-joins back into correlated Apply, gated by a new session/global system variable. This aims to enable correlated index lookups when they’re expected to outperform a hash semi-join.
Changes:
- Introduce
CorrelateSolverlogical rule and wire it into the logical optimizer behind a newFlagCorrelate. - Add sysvar + session var plumbing for
tidb_opt_enable_correlate_subquery. - Add casetests for the rule and a unit test to ensure
optRuleListandFlag*alignment.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sessionctx/variable/sysvar.go | Registers tidb_opt_enable_correlate_subquery sysvar and applies it to SessionVars. |
| pkg/sessionctx/variable/session.go | Adds EnableCorrelateSubquery to SessionVars and initializes it. |
| pkg/sessionctx/vardef/tidb_vars.go | Defines sysvar name constant and default value. |
| pkg/planner/core/rule_correlate.go | Implements the new semi-join → correlated Apply transformation rule. |
| pkg/planner/core/rule/logical_rules.go | Adds FlagCorrelate to the logical rule flag bitset. |
| pkg/planner/core/optimizer.go | Appends CorrelateSolver to optRuleList and conditionally enables FlagCorrelate; adjusts FlagPruneColumnsAgain computation. |
| pkg/planner/core/optimizer_test.go | Adds a test to detect misalignment between optRuleList and Flag* constants. |
| pkg/planner/core/casetest/rule/testdata/correlate_suite_in.json | Input SQLs for correlate rule casetests. |
| pkg/planner/core/casetest/rule/testdata/correlate_suite_out.json | Expected plans/results (non-xunit) for casetests. |
| pkg/planner/core/casetest/rule/testdata/correlate_suite_xut.json | Expected plans/results (xunit) for casetests. |
| pkg/planner/core/casetest/rule/rule_correlate_test.go | Adds correlate rule casetest runner and enables the sysvar during tests. |
| pkg/planner/core/casetest/rule/main_test.go | Loads the new correlate test suite data. |
| pkg/planner/core/casetest/rule/BUILD.bazel | Includes the new test file and updates shard count. |
| pkg/planner/core/BUILD.bazel | Adds rule_correlate.go to the core library build. |
|
@pantheon-bot please review |
|
Re-review note: this PR has 3 new commits since my last pass. I rechecked the correlate-subquery rewrite path (gated by Severity note: all findings are P2 mainly because the feature is default OFF. For the executor-side memory risk, the statement memory tracker (default 1GB, action CANCEL) provides a safety net, but the amplification can still be severe for sessions that enable the flag. P2-1: OOM/DoS risk - Apply materializes full inner result per outer rowWhat happens Evidence
Why it matters Recommendations
P2-2: Unconditional rewrite without cost or access-path guardWhat happens Evidence
Why it matters Recommendations
P2-3: Join hint / preference metadata dropped during transformWhat happens Evidence
Why it matters Recommendations
P2-4: Missing relevant optimizer variable trackingWhat happens Evidence
Why it matters Recommendations
If helpful, I can sketch a minimal fix set: (a) propagate hint fields in the rewrite, (b) add |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qw4990, yudongusa 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 |
|
/retest-required |
4 similar comments
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
|
/retest-required |
1 similar comment
|
/retest-required |
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #66320
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
New Features
Tests