Repository navigation
planner, sessionctx: enable non-eval scalar subquery explain by default - #68566
hawkingrei wants to merge 6 commits into
Conversation
ti-chi-bot
Bot
added
release-note
|
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:
📝 WalkthroughWalkthroughFlip EXPLAIN-evaluated-subquery default to show non-eval scalar subqueries, add a centralized EXPLAIN gating helper in the rewriter, snapshot/restore the EXPLAIN flag in QueryPlanCost, and update unit/integration tests and golden outputs accordingly. ChangesScalar Subquery Explanation Default
Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionVars
participant ExpressionRewriter
participant Planner
participant IndexAdvisor
Client->>SessionVars: SET/GET @@tidb_opt_enable_non_eval_scalar_subquery
Client->>Planner: EXPLAIN format='plan_tree' query(with scalar subquery)
Planner->>ExpressionRewriter: shouldExplainNonEvaledScalarSubQuery(...)
ExpressionRewriter-->>Planner: register non-eval ScalarSubQuery (RetType=mysql.TypeTiny)
Planner->>IndexAdvisor: QueryPlanCost (InExplainStmt=true, ExplainNonEvaledSubQuery=false)
IndexAdvisor-->>Planner: cost snapshot/restore
Planner-->>Client: plan rows (contains ScalarSubQuery / outputs)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/scalarsubquery/cases_test.go (1)
49-61: ⚡ Quick winConsider adding a negative test case for completeness.
The test validates that with the new default (variable = 1), scalar subqueries are not evaluated during EXPLAIN. For comprehensive regression coverage, consider adding a test case that explicitly sets
@@tidb_opt_enable_non_eval_scalar_subquery=0and verifies that:
PreprocessSubQueriesis > 0 (subquery was evaluated)- The plan output does NOT contain "ScalarSubQuery" or "sleep(0.3)" (subquery was replaced with its result)
This would ensure both behaviors are tested and prevent future regressions if the variable semantics change.
📋 Optional: Add negative test case
require.True(t, hasScalarSubQuery, "plan should keep the scalar subquery visible: %v", planRows) require.True(t, hasSleepFunc, "plan should keep sleep(0.3) visible: %v", planRows) + + // Verify the old behavior when variable is explicitly disabled + testKit.MustExec("set @@tidb_opt_enable_non_eval_scalar_subquery=0") + testKit.Session().GetSessionVars().RewritePhaseInfo.Reset() + rows = testKit.MustQuery("explain format = 'plan_tree' select * from t1 where a = (select sleep(0.3))").Rows() + require.Greater(t, testKit.Session().GetSessionVars().RewritePhaseInfo.PreprocessSubQueries, 0, "with variable disabled, scalar subquery should be evaluated")🤖 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/casetest/scalarsubquery/cases_test.go` around lines 49 - 61, Add a negative test: set @@tidb_opt_enable_non_eval_scalar_subquery=0 via testKit.MustExec, reset session RewritePhaseInfo (testKit.Session().GetSessionVars().RewritePhaseInfo.Reset()), run explain format='plan_tree' on the same query used here (select * from t1 where a = (select sleep(0.3))), then assert RewritePhaseInfo.PreprocessSubQueries is > 0 and verify the planRows (from testdata.ConvertRowsToStrings(rows)) do NOT contain "ScalarSubQuery" nor "sleep(0.3)"; use the same boolean checks (hasScalarSubQuery, hasSleepFunc) but expect false and include the planRows in failure messages for debugging.
🤖 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.
Nitpick comments:
In `@pkg/planner/core/casetest/scalarsubquery/cases_test.go`:
- Around line 49-61: Add a negative test: set
@@tidb_opt_enable_non_eval_scalar_subquery=0 via testKit.MustExec, reset session
RewritePhaseInfo (testKit.Session().GetSessionVars().RewritePhaseInfo.Reset()),
run explain format='plan_tree' on the same query used here (select * from t1
where a = (select sleep(0.3))), then assert
RewritePhaseInfo.PreprocessSubQueries is > 0 and verify the planRows (from
testdata.ConvertRowsToStrings(rows)) do NOT contain "ScalarSubQuery" nor
"sleep(0.3)"; use the same boolean checks (hasScalarSubQuery, hasSleepFunc) but
expect false and include the planRows in failure messages for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8859146e-ec97-4d1b-a299-1fe885023a0b
📒 Files selected for processing (2)
pkg/planner/core/casetest/scalarsubquery/cases_test.gopkg/sessionctx/vardef/tidb_vars.go
ti-chi-bot
Bot
added
size/M
|
[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 |
ti-chi-bot
Bot
added
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68566 +/- ##
================================================
- Coverage 76.3753% 75.4307% -0.9447%
================================================
Files 2039 2021 -18
Lines 562887 566065 +3178
================================================
- Hits 429907 426987 -2920
- Misses 132062 139075 +7013
+ Partials 918 3 -915
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/casetest/scalarsubquery/cases_test.go (1)
89-91: ⚡ Quick winTighten the “outer TableDual” assertion to avoid false positives.
The current check passes if any row contains
TableDual; that can match an inner node and miss a root-shape regression. Assert on the root/first explain row (or exact expected id position) instead.🤖 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/casetest/scalarsubquery/cases_test.go` around lines 89 - 91, The test currently looks for "TableDual" anywhere in the explain output via planContains(rows, ...), which can match inner nodes; instead assert the root/first explain row contains "TableDual". Change the assertion to inspect only the first explain row (use rows[0] or call planContains on a single-row slice) and require that that first row includes "TableDual" (replace require.True on hasOuterTableDual with an assertion that rows[0] contains "TableDual"); keep references to rows, planContains, hasOuterTableDual and planRows to locate and update the 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.
Nitpick comments:
In `@pkg/planner/core/casetest/scalarsubquery/cases_test.go`:
- Around line 89-91: The test currently looks for "TableDual" anywhere in the
explain output via planContains(rows, ...), which can match inner nodes; instead
assert the root/first explain row contains "TableDual". Change the assertion to
inspect only the first explain row (use rows[0] or call planContains on a
single-row slice) and require that that first row includes "TableDual" (replace
require.True on hasOuterTableDual with an assertion that rows[0] contains
"TableDual"); keep references to rows, planContains, hasOuterTableDual and
planRows to locate and update the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bd73d049-84ab-4462-9096-342b4ec329a3
📒 Files selected for processing (2)
pkg/planner/core/casetest/scalarsubquery/cases_test.gopkg/planner/core/expression_rewriter.go
|
/test mysql-test |
|
@hawkingrei: 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. |
|
/test check-dev2 |
|
@hawkingrei: 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. |
This branch has not been deployed
1 participant
What problem does this PR solve?
Issue Number: close #66679
Problem Summary:
EXPLAINcan currently evaluate scalar subqueries before producing the plan unlesstidb_opt_enable_non_eval_scalar_subqueryis enabled explicitly. That can makeEXPLAINslow for heavy scalar subqueries, while the generated plan hides the subquery work that caused the delay.What changed and how does it work?
This PR enables
tidb_opt_enable_non_eval_scalar_subqueryby default.With the default enabled, plain
EXPLAINkeeps scalar subqueries visible in the displayed plan instead of pre-evaluating them during preprocessing. The regression test also checks thatEXPLAIN ... (select sleep(0.3))does not increaseRewritePhaseInfo.PreprocessSubQueries, while bothScalarSubQueryandsleep(0.3)remain visible in the plan.Check List
Tests
Commands:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Behavior
Tests