Repository navigation
planner: fix deferred window subquery rewrite - #67203
hawkingrei wants to merge 13 commits into
Conversation
ti-chi-bot
Bot
added
release-note
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughIntroduces a dedicated subquery plan builder that extracts subquery expressions, clones and builds their inner plans to discover correlated outer columns and inject auxiliary select fields; centralizes subquery enter/restore state handling; updates tests and expectations for window+subquery scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Planner
participant PlanBuilder
participant Parser
participant CorrelationDetector
Client->>Planner: Request plan for query with subquery-containing exprs
Planner->>PlanBuilder: appendAuxiliaryFieldsForSubqueries(exprs, selectFields)
PlanBuilder->>PlanBuilder: subqueryExprExtractor.collect(exprs)
alt recognized subquery expr
PlanBuilder->>Parser: restore & parse cloned inner ResultSetNode
Parser-->>PlanBuilder: inner AST (ResultSetNode)
PlanBuilder->>PlanBuilder: enterSubqueryBuild(snapshot outer scope)
PlanBuilder->>PlanBuilder: buildResultSetNode(innerQuery)
PlanBuilder-->>PlanBuilder: inner logical plan
PlanBuilder->>CorrelationDetector: ExtractCorrelatedCols4LogicalPlan(innerPlan)
CorrelationDetector-->>PlanBuilder: correlated outer column uniqueIDs
PlanBuilder->>PlanBuilder: map uniqueIDs -> ast.ColumnName, append Auxiliary select fields
PlanBuilder->>PlanBuilder: restore outer scope
else fallback
PlanBuilder->>PlanBuilder: b.rewrite(expr, true) to produce subquery plan
end
PlanBuilder-->>Planner: augmented selectFields / plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/planner/core/logical_plan_builder.go (1)
2644-2674: Consider sharing this subquery-state push/pop withexpressionRewriter.buildSubquery().This is now a second copy of the same
outerSchemas/outerNames/outerBlockExpand/subQueryCtx/subQueryHintFlags/windowSpecschoreography frompkg/planner/core/expression_rewriter.go. Any future PlanBuilder state added to one path can easily be missed in the other. A small shared helper would make this logic much easier to audit.As per coding guidelines, "Follow existing package-local conventions first and keep style consistent with nearby files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plan_builder.go` around lines 2644 - 2674, Extract the duplicated subquery-state push/pop logic into a shared PlanBuilder helper (e.g., PlanBuilder.pushSubqueryState or a small helper that returns a restore function) and replace the copy in both logical_plan_builder.go and expression_rewriter.go to call it; the helper should accept the subquery context and the plan node (so it can detect LogicalJoin and pick FullSchema/FullNames), perform the outerSchemas/outerNames cloning, append outerBlockExpand/currentBlockExpand, save and set subQueryCtx, subQueryHintFlags and windowSpecs, and return a closure or token that restores those fields (popping outerSchemas/outerNames/outerBlockExpand, restoring currentBlockExpand, windowSpecs, subQueryCtx and subQueryHintFlags) to be used in a defer in both buildResultSetNode and expressionRewriter.buildSubquery.pkg/planner/core/issuetest/planner_issue_test.go (1)
322-323: Consider adding.Check()to verify EXPLAIN output.The EXPLAIN query runs without verifying the plan structure. While this is sufficient to confirm no ERROR 3593 is thrown (the main fix), other EXPLAIN tests in this file validate expected plan nodes. Adding a basic Check() would make the test more robust against future regressions in plan generation.
💡 Optional: Add basic plan verification
- tk.MustQuery("explain format='brief' select /* issue:67152 explain */ last_value(a) over (order by a) in (select b from t) from t") + tk.MustQuery("explain format='brief' select /* issue:67152 explain */ last_value(a) over (order by a) in (select b from t) from t"). + MultiCheckContain([]string{"Window", "HashJoin"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/issuetest/planner_issue_test.go` around lines 322 - 323, Replace the bare tk.MustQuery("explain format='brief' select /* issue:67152 explain */ last_value(a) over (order by a) in (select b from t) from t") with a query assertion that calls .Check(...) on the returned result to verify the EXPLAIN output contains the expected plan node(s); specifically, assert that the brief EXPLAIN for the query containing last_value(a) over (order by a) includes a "Window" (or "last_value") node string so the test validates the plan structure as well as absence of ERROR 3593.
🤖 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/logical_plan_builder.go`:
- Around line 2626-2642: The subquery extractor currently only builds the RHS
subquery plan for CompareSubqueryExpr and PatternInExpr and then short-circuits,
which misses scalar subqueries nested on the non-subquery (left) side; update
the logic in subqueryExprExtractor so that when handling ast.CompareSubqueryExpr
and ast.PatternInExpr (the branches that set subq and sCtx to
handlingCompareSubquery/handlingInSubquery) you also recurse into or scan the
non-subquery operand to collect any nested subquery nodes (or call the existing
rewrite(ctx, expr, ...) path for that operand) before returning, ensuring outer
correlations (e.g., t.k) are discovered and included in the subquery plan
construction.
---
Nitpick comments:
In `@pkg/planner/core/issuetest/planner_issue_test.go`:
- Around line 322-323: Replace the bare tk.MustQuery("explain format='brief'
select /* issue:67152 explain */ last_value(a) over (order by a) in (select b
from t) from t") with a query assertion that calls .Check(...) on the returned
result to verify the EXPLAIN output contains the expected plan node(s);
specifically, assert that the brief EXPLAIN for the query containing
last_value(a) over (order by a) includes a "Window" (or "last_value") node
string so the test validates the plan structure as well as absence of ERROR
3593.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 2644-2674: Extract the duplicated subquery-state push/pop logic
into a shared PlanBuilder helper (e.g., PlanBuilder.pushSubqueryState or a small
helper that returns a restore function) and replace the copy in both
logical_plan_builder.go and expression_rewriter.go to call it; the helper should
accept the subquery context and the plan node (so it can detect LogicalJoin and
pick FullSchema/FullNames), perform the outerSchemas/outerNames cloning, append
outerBlockExpand/currentBlockExpand, save and set subQueryCtx, subQueryHintFlags
and windowSpecs, and return a closure or token that restores those fields
(popping outerSchemas/outerNames/outerBlockExpand, restoring currentBlockExpand,
windowSpecs, subQueryCtx and subQueryHintFlags) to be used in a defer in both
buildResultSetNode and expressionRewriter.buildSubquery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15d252f7-dc7a-464f-ad4e-dd2829095a1e
📒 Files selected for processing (2)
pkg/planner/core/issuetest/planner_issue_test.gopkg/planner/core/logical_plan_builder.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67203 +/- ##
================================================
- Coverage 77.8088% 77.5128% -0.2961%
================================================
Files 2023 1943 -80
Lines 556523 543665 -12858
================================================
- Hits 433024 421410 -11614
- Misses 121753 122252 +499
+ Partials 1746 3 -1743
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 `@pkg/planner/core/logical_plan_builder.go`:
- Around line 2617-2684: The temporary subquery build in
buildSubqueryPlanForAuxiliaryFields runs buildResultSetNode but only restores a
subset of PlanBuilder state; snapshot and restore all mutable planner fields
touched by the probe (at minimum b.correlatedAggMapper, b.curClause,
b.skipAggMap-related state, and any other maps/lists that buildResultSetNode may
mutate such as handleHelper state) or perform the probe on an isolated
PlanBuilder clone (create a shallow copy or new PlanBuilder and run
buildResultSetNode there) so the outer builder state is unchanged; update the
defer block to restore the saved fields (or discard the cloned builder) and add
a regression unit test that exercises a correlated aggregate + inner HAVING
through this codepath to ensure no leakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cbb52e03-0420-4946-9227-10f865976124
📒 Files selected for processing (4)
pkg/planner/core/casetest/windows/testdata/window_push_down_suite_out.jsonpkg/planner/core/casetest/windows/testdata/window_push_down_suite_xut.jsonpkg/planner/core/casetest/windows/window_with_exist_subquery_test.gopkg/planner/core/logical_plan_builder.go
|
/retest |
|
/retest |
|
⏳ @hawkingrei 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. |
|
@pantheon-bot review |
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
|
/retest |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/subquery_plan_builder.go (1)
159-175: Consider adding a brief comment explaining the cloning approach.The serialize-reparse pattern is correct for this use case (where the cloned AST is only used for correlated column discovery and doesn't need the resolve context). A short comment would help future readers understand why a deep-clone utility wasn't used instead.
📝 Suggested documentation
+// cloneResultSetNodeForAuxiliaryFields creates a clone of the result set node by +// serializing it to SQL and reparsing. This avoids the complexity of deep-cloning +// AST nodes while providing a fresh copy that can be built without affecting the +// original. The cloned AST lacks resolution context, which is acceptable since it's +// only used for correlated outer-column discovery. func cloneResultSetNodeForAuxiliaryFields(ctx base.PlanContext, node ast.ResultSetNode) (ast.ResultSetNode, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/subquery_plan_builder.go` around lines 159 - 175, Add a short explanatory comment above cloneResultSetNodeForAuxiliaryFields describing the chosen "serialize then reparse" cloning approach: note that the function serializes the node with format.NewRestoreCtx and reparses it with parser.New().ParseOneStmt because we only need a syntactic copy for correlated-column discovery (no name-resolution or resolve context required), so a full deep-clone utility or resolver-aware copy isn’t necessary; mention that this approach intentionally drops resolver context and is acceptable for auxiliary subqueries to avoid surprising future readers.
🤖 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/subquery_plan_builder.go`:
- Around line 159-175: Add a short explanatory comment above
cloneResultSetNodeForAuxiliaryFields describing the chosen "serialize then
reparse" cloning approach: note that the function serializes the node with
format.NewRestoreCtx and reparses it with parser.New().ParseOneStmt because we
only need a syntactic copy for correlated-column discovery (no name-resolution
or resolve context required), so a full deep-clone utility or resolver-aware
copy isn’t necessary; mention that this approach intentionally drops resolver
context and is acceptable for auxiliary subqueries to avoid surprising future
readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2ceefa5-8923-4455-8765-389b0aec176c
📒 Files selected for processing (5)
pkg/planner/core/BUILD.bazelpkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/planbuilder_test.gopkg/planner/core/subquery_plan_builder.go
💤 Files with no reviewable changes (1)
- pkg/planner/core/logical_plan_builder.go
✅ Files skipped from review due to trivial changes (1)
- pkg/planner/core/BUILD.bazel
|
@pantheon-bot review |
|
Review completed with mixed results due to infrastructure constraints: Execution Summary:
Prior Review Finding (P2): The Location: Recommendation: ℹ️ Learn more details on Pantheon AI. |
|
/retest |
|
@pantheon-bot review |
|
The master agent encountered an error while processing. Please try again later. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/L
ti-chi-bot
Bot
added
size/XL
|
/retest |
|
@pantheon-bot review |
|
The master agent encountered an error while processing. Please try again later. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
size/L
|
[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 |
|
/retest |
|
@pantheon-bot review |
|
The master agent encountered an error while processing. Please try again later. ℹ️ Learn more details on Pantheon AI. |
|
@pantheon-bot review |
|
The master agent encountered an error while processing. Please try again later. ℹ️ Learn more details on Pantheon AI. |
|
@pantheon-bot review |
|
The master agent encountered an error while processing. Please try again later. ℹ️ Learn more details on Pantheon AI. |
|
@pantheon-bot review |
|
Review failed due to infrastructure/execution failure after retries. Please re-trigger review. ℹ️ Learn more details on Pantheon AI. |
|
@pantheon-bot review |
|
⏳ @hawkingrei 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. |
|
@pantheon-bot review |
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
| rewriter := &expressionRewriter{ | ||
| schema: outerSchema, | ||
| names: outerNames, | ||
| } |
There was a problem hiding this comment.
We'd better use the getExpressionRewriter to reuse the pool.
| var sb strings.Builder | ||
| restoreCtx := format.NewRestoreCtx(format.DefaultRestoreFlags, &sb) | ||
| if err := node.Restore(restoreCtx); err != nil { | ||
| return nil, err | ||
| } | ||
| charset, collation := ctx.GetSessionVars().GetCharsetInfo() | ||
| p := parserutil.GetParser() | ||
| defer parserutil.DestroyParser(p) | ||
| stmt, err := p.ParseOneStmt(sb.String(), charset, collation) |
There was a problem hiding this comment.
Why do we need to restore it first, then parse it again?
This seems buggy and costly.
This branch has not been deployed
3 participants
What problem does this PR solve?
Issue Number: close #67152
Problem Summary:
LAST_VALUE(...) OVER (...) IN (SELECT ...)is legal in the select list, but TiDB reportsplanner:3593during planning.The same window expression works with a constant
IN (...)list, so the failure comes from the subquery rewrite path instead of window-function semantics.What changed and how does it work?
resolveWindowFunction()needs to discover correlated outer columns for subqueries inside deferred window expressions.Before this change,
appendAuxiliaryFieldsForSubqueries()rewrote the wholeIN (subquery)/ compare-subquery wrapper during that discovery step.That early rewrite visited the outer window expression before window outputs were materialized, so the legal window function fell into
ErrWindowInvalidWindowFuncUse.This PR changes that discovery path to build only the inner subquery plan with the corresponding subquery context.
That keeps correlated-column discovery intact while avoiding premature rewrites of the outer deferred window expression.
The regression test covers:
LAST_VALUE(...) OVER (...) IN (SELECT ...)EXPLAIN FORMAT='brief'for the same query= ANY (SELECT ...)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
Bug Fixes
Tests