Repository navigation
planner: Add optimizer support to LATERAL join - #67095
Closed
terry1purcell wants to merge 9 commits into
Closed
terry1purcell wants to merge 9 commits into
terry1purcell wants to merge 9 commits into
Conversation
Add parser support for the LATERAL keyword in derived tables, following MySQL 8.0 syntax. This is parser-only; planner support follows in a separate PR. Grammar: LATERAL SubSelect TableAsName IdentListWithParenOpt - Alias is required (TableAsName, not TableAsNameOpt) - AS keyword is optional - Optional column list: LATERAL (...) AS dt(c1, c2) AST changes: - TableSource.Lateral bool flag - TableSource.ColumnNames []CIStr for column aliases - Restore() emits LATERAL keyword and column list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lands The parser now accepts the LATERAL keyword (from the previous commit), but the planner does not yet support it. Add a preprocessor check that returns a clear "not supported yet" error for any TableSource with Lateral=true, so users get an actionable message instead of a confusing plan-build failure. This guard will be removed when planner support for LATERAL is added. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a guard in TableSource.Restore() that returns a descriptive error if ColumnNames is non-empty but AsName is empty, instead of silently dropping the column list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify that LATERAL derived tables are rejected during preprocessing with ErrNotSupportedYet, covering both comma syntax and LEFT JOIN forms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Check Lateral flag and ColumnNames on both the original and re-parsed (round-tripped) AST so that regressions in Restore() column-list emission are caught. Also adds negative assertion for non-LATERAL cases on the round-tripped statement. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…yntax Implement planner support for LATERAL derived tables with INNER JOIN semantics (explicit JOIN and comma syntax). LATERAL makes left-side columns available to the right-side subquery via LogicalApply. Supported forms: - FROM t1, LATERAL (SELECT ...) AS dt - FROM t1 JOIN LATERAL (SELECT ...) AS dt ON <cond> - FROM t1 CROSS JOIN LATERAL (SELECT ...) AS dt Not yet supported (returns ErrNotSupportedYet): - LEFT/RIGHT JOIN LATERAL Key changes: - buildLateralJoin() in logical_plan_builder.go builds LogicalApply - IsLateral flag on LogicalApply/LogicalJoin prevents unsafe rewrites - IsLateral guard in pruneRedundantApply (rule_decorrelate.go) - LATERAL-aware join reorder prevention (rule_join_reorder.go) - setIsInApplyForCTE for CTE-in-LATERAL correctness - Decorrelation enabled for safe equi-join patterns Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ti-chi-bot
Bot
added
the
release-note-none
|
Review Complete Findings: 2 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XXL
Contributor
Author
|
/ok-to-test |
|
[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 |
|
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. |
📝 WalkthroughWalkthroughAdds SQL LATERAL support: parser recognizes LATERAL and records column aliases; planner treats LATERAL derived tables as LogicalApply (IsLateral), preserves outer-scope correlation, prevents reorder/prune, and introduces ErrInvalidLateralJoin. Extensive unit and integration tests added/updated. Changes
Sequence DiagramsequenceDiagram
participant Parser as Parser
participant PlanBuilder as Plan Builder
participant Apply as LogicalApply
participant Decorrelate as DecorrelateSolver
participant Reorder as JoinReorder
participant Optimizer as Optimizer
Parser->>Parser: tokenize/parse SQL, recognize `LATERAL`
Parser->>PlanBuilder: produce TableSource(Lateral=true, ColumnNames?)
PlanBuilder->>PlanBuilder: detect LATERAL on join right side
PlanBuilder->>PlanBuilder: findJoinFullSchema() to capture outer scope
PlanBuilder->>Apply: buildLateralJoin() -> create LogicalApply (IsLateral=true, CorCols)
Apply->>Decorrelate: DecorrelateSolver.optimize()
Decorrelate->>Decorrelate: see IsLateral -> skip pruneRedundantApply
Apply->>Reorder: extractJoinGroup()
Reorder->>Reorder: detect LogicalApply -> early return (no reorder)
Optimizer->>Optimizer: continue optimization with preserved Apply
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.3)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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/planner/core/rule_decorrelate.go (1)
164-205:⚠️ Potential issue | 🟠 MajorNested LATERAL Apply can still be pruned incorrectly.
Line 167 blocks only the current Apply. The loop (Lines 191-205) still removes downstream
LogicalApplynodes unconditionally; if anynextApply.IsLateralis true, pruning can change multiplicity and query results.💡 Proposed guard in the nested-apply traversal
for { child := finalResult.Children()[0] nextApply, ok := child.(*logicalop.LogicalApply) if !ok { if len(groupByColumn) == 0 { return child, true } for col := range groupByColumn { if apply.Schema().Contains(col) && !child.Schema().Contains(col) { return nil, false } } return child, true // Return the child of the last LogicalApply } + if nextApply.IsLateral { + return nil, false + } finalResult = nextApply }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/rule_decorrelate.go` around lines 164 - 205, The loop that walks through nested LogicalApply nodes (finalResult/nextApply) must stop and avoid pruning if any downstream apply has IsLateral==true; currently only the top-level apply.IsLateral is checked. Modify the traversal in the for loop that inspects child := finalResult.Children()[0] / nextApply := child.(*logicalop.LogicalApply) to check nextApply.IsLateral and if true return nil, false (or otherwise abort pruning) before assigning finalResult = nextApply; this ensures any nested lateral apply prevents elimination and preserves result multiplicity (also keep existing groupByColumn and schema containment checks intact).pkg/planner/core/operator/logicalop/logical_apply.go (1)
146-174:⚠️ Potential issue | 🟠 MajorCarry right-side NDVs into lateral Apply stats.
IsLateralupdatesRowCount, but the NDV block below still treats every right-side output column asleftProfile.RowCount. For multi-row lateral joins that collapses right-column distinctness and can badly distort follow-up estimates. WhenIsLateralis true, seed right-side NDVs fromrightProfile.ColNDVscapped byrowCount, similar toLogicalJoin.DeriveStats.Suggested fix
la.SetStats(&property.StatsInfo{ RowCount: rowCount, ColNDVs: make(map[int64]float64, selfSchema.Len()), }) // TODO: investigate why this cannot be replaced with maps.Copy() for id, c := range leftProfile.ColNDVs { la.StatsInfo().ColNDVs[id] = c } if la.JoinType == base.LeftOuterSemiJoin || la.JoinType == base.AntiLeftOuterSemiJoin { la.StatsInfo().ColNDVs[selfSchema.Columns[selfSchema.Len()-1].UniqueID] = 2.0 + } else if la.IsLateral { + for id, c := range rightProfile.ColNDVs { + la.StatsInfo().ColNDVs[id] = min(c, rowCount) + } } else { for i := childSchema[0].Len(); i < selfSchema.Len(); i++ { la.StatsInfo().ColNDVs[selfSchema.Columns[i].UniqueID] = leftProfile.RowCount } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_apply.go` around lines 146 - 174, The NDV population currently always assigns right-side output columns NDV = leftProfile.RowCount; instead, when la.IsLateral is true incorporate rightProfile.ColNDVs for right output columns (seed each right column NDV = min(rightProfile.ColNDVs[colID], rowCount)) and fall back to leftProfile.RowCount only when a right NDV is missing; update the block that fills la.StatsInfo().ColNDVs (after la.SetStats) to copy leftProfile.ColNDVs as before and then for i from childSchema[0].Len() to selfSchema.Len() use rightProfile.ColNDVs capped by rowCount (mirroring LogicalJoin.DeriveStats behavior) and ensure the LeftOuterSemi/Anti case still sets the trailing column to 2.0.
🧹 Nitpick comments (5)
pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json (1)
25-25: Fix typo in inline SQL comment (huristic→heuristic).Non-functional, but correcting this helps readability and grep-ability across test artifacts.
✏️ Suggested tweak
- "explain format='brief' select /*+ READ_FROM_STORAGE(TIFLASH[s]) */ a from s where a = 10 and b is null; -- index path huristic rule will prune tiflash path", + "explain format='brief' select /*+ READ_FROM_STORAGE(TIFLASH[s]) */ a from s where a = 10 and b is null; -- index path heuristic rule will prune tiflash path",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json` at line 25, Update the inline SQL comment in the test string that currently reads "index path huristic rule will prune tiflash path" to correct the typo to "index path heuristic rule will prune tiflash path" in the SQL literal ("explain format='brief' select /*+ READ_FROM_STORAGE(TIFLASH[s]) */ a from s where a = 10 and b is null; -- index path huristic rule will prune tiflash path") so the comment is spelled correctly for readability and grep-ability.tests/integrationtest/t/planner/cascades/integration.test (1)
199-239: Consider adding explicit cascadesLATERALSQL cases in this suite.This file validates Join/Apply broadly, but it does not directly assert cascades behavior for
LATERALsyntax. Adding one positive case (INNER/CROSS/,) and one unsupported-case assertion (LEFT/RIGHT JOIN LATERAL) here would improve regression coverage for the planner path targeted by this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/planner/cascades/integration.test` around lines 199 - 239, Add explicit LATERAL cases to the existing cascades planner tests: under the TestApply block add a positive inner/cross LATERAL case (e.g. "select t1.a, x.b from t1 cross join lateral (select b from t2 where t2.a = t1.a) as x;" with both explain format="plan_tree" and the select to assert plan + result), and add unsupported-case assertions for LEFT/RIGHT JOIN LATERAL (e.g. "select ... from t1 left join lateral (...)" and right join lateral) that check the planner rejects or produces the expected unsupported-plan/error; place these near the other apply/join examples so they exercise the same session settings (tidb_enable_cascades_planner) and use the same test tables t1/t2 to match existing patterns.tests/integrationtest/t/planner/core/lateral_join.test (1)
127-133: Stabilize EXPLAIN assertions at Line 127 and Line 133.These two EXPLAIN checks should use
--replace_columnlike earlier cases to avoid brittle plan-id/estRows diffs.Suggested patch
## EXPLAIN: LATERAL + USING + ON must show Apply, not plain Join +--replace_column 1 <id> 2 <estRows> 3 <task> 4 <access_object> 5 <operator_info> explain format='brief' select * from t1 join t2 using(a) join t3 on t2.b=t3.b, lateral (select count(*) as c from t4 where t4.a=t2.a) as dt; @@ ## EXPLAIN: LATERAL + USING + ON with aggregate that blocks decorrelation +--replace_column 1 <id> 2 <estRows> 3 <task> 4 <access_object> 5 <operator_info> explain format='brief' select * from t1 join t2 using(a) join t3 on t2.b=t3.b, lateral (select sum(t4.c) as s from t4 where t4.a=t2.a) as dt;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/t/planner/core/lateral_join.test` around lines 127 - 133, Update the two EXPLAIN assertions that are currently brittle by adding the same --replace_column handling used in earlier tests: modify the EXPLAIN for "select * from t1 join t2 using(a) join t3 on t2.b=t3.b, lateral (select count(*) as c from t4 where t4.a=t2.a) as dt" (Line 127) and the EXPLAIN for "select * from t1 join t2 using(a) join t3 on t2.b=t3.b, lateral (select sum(t4.c) as s from t4 where t4.a=t2.a) as dt" (Line 133) to invoke --replace_column for plan-id/estRows columns so the assertions ignore transient plan identifiers; follow the same --replace_column syntax and column names used in the nearby tests to ensure consistency.pkg/planner/core/lateral_join_test.go (2)
35-41: Dead code:expectedErrorCodefield is never used.The
expectedErrorCodefield is defined but never set to a non-zero value in any test case. The conditional check on lines 94-97 will never execute. Either populate this field with actual error codes (e.g.,3809forErrInvalidLateralJoin) or remove it to keep tests minimal.🔧 Option 1: Remove unused field and dead code
testCases := []struct { name string sql string expectApply bool expectError bool - expectedErrorCode int }{And remove lines 94-97:
if tc.expectError { require.Error(t, err, "Expected error for: %s", tc.sql) - if tc.expectedErrorCode != 0 { - require.True(t, plannererrors.ErrInvalidLateralJoin.Equal(err), - "Expected ErrInvalidLateralJoin error, got: %v", err) - } + require.True(t, plannererrors.ErrInvalidLateralJoin.Equal(err), + "Expected ErrInvalidLateralJoin error, got: %v", err) return }🔧 Option 2: Use the field by setting error codes
{ name: "LATERAL with LEFT JOIN not yet supported", sql: "SELECT * FROM t LEFT JOIN LATERAL (SELECT t.b) AS dt ON true", expectError: true, + expectedErrorCode: 3809, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/lateral_join_test.go` around lines 35 - 41, The tests define an unused struct field expectedErrorCode in the testCases slice and the follow-up conditional that checks it (referencing expectedErrorCode) is dead; either remove the expectedErrorCode field from the testCases struct and delete the conditional that inspects it, or set expectedErrorCode values in each test case (e.g., 3809 for ErrInvalidLateralJoin) and update the test assertions to compare against that field; locate the testCases declaration and the conditional that reads expectedErrorCode in lateral_join_test.go and apply one of these two changes to eliminate the dead code.
118-156: Test assertions don't match the function name's promise.
TestLateralJoinOptimizationstates it "tests decorrelation and optimization behavior" but only verifies that plans build successfully and have a non-nil schema. Consider either:
- Adding assertions that verify actual optimization outcomes (e.g., checking if Apply nodes were preserved/converted), or
- Renaming to
TestLateralJoinOptimizationBuildsto reflect what it actually tests.Note:
TestLateralJoinDecorrelateWithUSINGAndON(line 536) does proper decorrelation verification and could serve as a model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/lateral_join_test.go` around lines 118 - 156, The test TestLateralJoinOptimization currently only asserts that a plan builds and has a non-nil schema; either rename the test to TestLateralJoinOptimizationBuilds to reflect that minimal check, or enhance it to assert actual decorrelation/optimization outcomes by inspecting the logical plan returned from BuildLogicalPlanForTest: for each case inspect the plan tree to confirm whether Apply nodes (e.g., LogicalApply/Apply) remain or were converted to Join nodes (e.g., LogicalJoin/Join) as expected (mirror the approach used in TestLateralJoinDecorrelateWithUSINGAndON), failing the test when the node types do not match the expected optimization result.
🤖 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/parser/ast/dml.go`:
- Around line 553-563: The validation currently only forbids LATERAL and column
lists for *TableName but still allows them on other non-derived sources like
*Join; update the guard in the validation block in the TableSource logic (check
TableSource.Source) so that LATERAL and ColumnNames are permitted only when the
Source is a derived table type: *SelectStmt or *SetOprStmt, and reject them for
any other concrete source (including *Join and *TableName); use the existing
fields n.Lateral, n.ColumnNames and n.AsName.String() to emit the same error
messages when the Source is not one of the allowed derived types.
In `@pkg/planner/core/casetest/rule/testdata/outer2inner_out.json`:
- Around line 675-686: The golden plan's Projection node is missing test.t1.c1
(it lists only test.t1.a1, test.t1.b1, test.t2.a2, Column#9), so either the
planner output was truncated or the test fixture schema baked into
outer2inner_out.json is wrong; update the Projection entry in
outer2inner_out.json (the JSON under "Plan" for the SQL containing "lateral ...
count(*) as cnt") to include test.t1.c1 alongside a1 and b1 (and verify Column#9
remains the aggregate count), or if the planner should emit that column ensure
the test harness captures the full projection from the Projection node rather
than dropping columns produced by the outer TableFullScan/TableReader for
test.t1.
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 2733-2743: The resolver only remaps redundant USING/NATURAL
columns for LogicalJoin, causing qualified names over LATERAL LogicalApply to
bind to names not present in Apply.Schema() and break later index resolution;
update resolveRedundantColumnFromNaturalUsingJoinPlan to also handle
*logicalop.LogicalApply (or detect LogicalJoin embedding) by checking
x.FullSchema / x.FullNames like the new case at the top of
expression_rewriter.go and remapping the redundant column into Apply.Schema()
(or return the corresponding Column/Name pair) so that resolve-indices sees the
correct Apply.Schema() entries; reference
resolveRedundantColumnFromNaturalUsingJoinPlan, logicalop.LogicalApply,
LogicalJoin, FullSchema, FullNames, and Apply.Schema() when making the change.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 706-715: The current code sets b.buildingLateralSubquery for the
entire right branch while building a join (flipping it before processing
joinNode.Right), which allows non-LATERAL siblings to bypass recursive-CTE ORDER
BY/LIMIT checks; instead, only set b.buildingLateralSubquery when the
TableSource node is actually a LATERAL (check ast.TableSource{Lateral:true} on
the specific node) and restore it immediately after processing that TableSource.
Locate the block that saves and defers restoring b.buildingLateralSubquery (the
saveBuildingLateral variable and defer that pops outerSchemas/outerNames and
resets flags) and change the scope so the flag is set and reset around building
the specific ast.TableSource LATERAL node (not the whole joinNode.Right),
keeping the other cleanup (outerSchemas/outerNames/lateralOuterCount) intact in
the existing defer.
- Around line 950-959: The right child's FullSchema is being dropped by
unconditionally using rightPlan.Schema()/OutputNames(); instead call
findJoinFullSchema on rightPlan (just like done for leftPlan) and if it returns
non-nil set rFullSchema and rFullNames from that result, otherwise fall back to
rightPlan.Schema() and rightPlan.OutputNames(); reference the existing
findJoinFullSchema function and the rFullSchema/rFullNames variables (and
consider LogicalJoin/LogicalApply as possible rightPlan types that carry
FullSchema).
In `@tests/integrationtest/r/planner/cascades/integration.result`:
- Around line 891-899: The snapshot is asserting volatile columns from
information_schema.processlist (the SELECT that lists USER, DB, COMMAND, TIME,
STATE, INFO, `DIGEST`) which causes flakiness; change the test to only assert
stable columns (e.g., USER, DB, COMMAND, STATE) or normalize/ignore TIME, INFO
and `DIGEST` in the comparison (for example, remove them from the SELECT used
for the snapshot or mask them before asserting) so the MemTableScan/select
expectation no longer depends on timing-sensitive values.
In `@tests/integrationtest/r/planner/core/integration.result`:
- Line 2844: The expected error text currently shows a truncated column
reference 'OUTR.'; update the test fixture to use the full identifier (e.g.,
replace 'OUTR.' with 'OUTR.col2' in the expected output) and/or fix the
error-message serialization routine (e.g., the function that formats SQL errors
or expression references such as serializeExpression/formatSqlError) so it
preserves the column name after the table alias instead of dropping it; ensure
the produced and expected messages match exactly with the full reference
'OUTR.col2'.
In `@tests/integrationtest/r/planner/core/lateral_join.result`:
- Line 6: Stabilize the nondeterministic lateral subqueries by adding an
explicit ORDER BY before LIMIT 1 so the chosen row is deterministic; update the
lateral clauses like "lateral (select b from t2 where t2.a = t1.a limit 1) as
lat" to include a deterministic sort (for example "order by b" or the table's
primary key) in each occurrence (the queries containing the lateral subquery at
the instances shown).
In `@tests/integrationtest/t/planner/cascades/integration.test`:
- Around line 1-282: The result file
tests/integrationtest/r/planner/cascades/integration.result is empty and must be
populated with the expected outputs for the 14 tests defined in
tests/integrationtest/t/planner/cascades/integration.test (e.g.,
TestSimpleProjDual, TestPKIsHandleRangeScan, TestIndexScan, TestBasicShow,
TestSort, TestAggregation, TestPushdownDistinctEnable,
TestPushdownDistinctDisable, TestSimplePlans, TestJoin, TestApply,
TestMemTableScan, TestTopN, TestCascadePlannerHashedPartTable); run the
integration test harness (or the equivalent test runner used by the repo)
against that .test file with the correct environment flags
(tidb_enable_cascades_planner etc.), capture the explain/select outputs, replace
the empty integration.result with the regenerated outputs, and commit the
updated tests/integrationtest/r/planner/cascades/integration.result so it
contains the expected results for each named test case.
- Around line 164-175: TestPushdownDistinctDisable contains nondeterministic
assertions (the query with "a, count(distinct a)" and the several "GROUP BY c"
selects) so mirror the normalization directives used in
TestPushdownDistinctEnable: add the same output-normalization/comments (the
stable result ordering/normalization directives) immediately before the affected
statements in TestPushdownDistinctDisable so the plan/result comparisons are
deterministic for the "select /*+ ... */ a, count(distinct a) ..." case and for
all "group by c" queries; ensure you apply the directives to the explain/select
pairs that include "count(distinct ...)" and "group by c" so they follow the
same normalization rules as TestPushdownDistinctEnable.
---
Outside diff comments:
In `@pkg/planner/core/operator/logicalop/logical_apply.go`:
- Around line 146-174: The NDV population currently always assigns right-side
output columns NDV = leftProfile.RowCount; instead, when la.IsLateral is true
incorporate rightProfile.ColNDVs for right output columns (seed each right
column NDV = min(rightProfile.ColNDVs[colID], rowCount)) and fall back to
leftProfile.RowCount only when a right NDV is missing; update the block that
fills la.StatsInfo().ColNDVs (after la.SetStats) to copy leftProfile.ColNDVs as
before and then for i from childSchema[0].Len() to selfSchema.Len() use
rightProfile.ColNDVs capped by rowCount (mirroring LogicalJoin.DeriveStats
behavior) and ensure the LeftOuterSemi/Anti case still sets the trailing column
to 2.0.
In `@pkg/planner/core/rule_decorrelate.go`:
- Around line 164-205: The loop that walks through nested LogicalApply nodes
(finalResult/nextApply) must stop and avoid pruning if any downstream apply has
IsLateral==true; currently only the top-level apply.IsLateral is checked. Modify
the traversal in the for loop that inspects child := finalResult.Children()[0] /
nextApply := child.(*logicalop.LogicalApply) to check nextApply.IsLateral and if
true return nil, false (or otherwise abort pruning) before assigning finalResult
= nextApply; this ensures any nested lateral apply prevents elimination and
preserves result multiplicity (also keep existing groupByColumn and schema
containment checks intact).
---
Nitpick comments:
In `@pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json`:
- Line 25: Update the inline SQL comment in the test string that currently reads
"index path huristic rule will prune tiflash path" to correct the typo to "index
path heuristic rule will prune tiflash path" in the SQL literal ("explain
format='brief' select /*+ READ_FROM_STORAGE(TIFLASH[s]) */ a from s where a =
10 and b is null; -- index path huristic rule will prune tiflash path") so the
comment is spelled correctly for readability and grep-ability.
In `@pkg/planner/core/lateral_join_test.go`:
- Around line 35-41: The tests define an unused struct field expectedErrorCode
in the testCases slice and the follow-up conditional that checks it (referencing
expectedErrorCode) is dead; either remove the expectedErrorCode field from the
testCases struct and delete the conditional that inspects it, or set
expectedErrorCode values in each test case (e.g., 3809 for
ErrInvalidLateralJoin) and update the test assertions to compare against that
field; locate the testCases declaration and the conditional that reads
expectedErrorCode in lateral_join_test.go and apply one of these two changes to
eliminate the dead code.
- Around line 118-156: The test TestLateralJoinOptimization currently only
asserts that a plan builds and has a non-nil schema; either rename the test to
TestLateralJoinOptimizationBuilds to reflect that minimal check, or enhance it
to assert actual decorrelation/optimization outcomes by inspecting the logical
plan returned from BuildLogicalPlanForTest: for each case inspect the plan tree
to confirm whether Apply nodes (e.g., LogicalApply/Apply) remain or were
converted to Join nodes (e.g., LogicalJoin/Join) as expected (mirror the
approach used in TestLateralJoinDecorrelateWithUSINGAndON), failing the test
when the node types do not match the expected optimization result.
In `@tests/integrationtest/t/planner/cascades/integration.test`:
- Around line 199-239: Add explicit LATERAL cases to the existing cascades
planner tests: under the TestApply block add a positive inner/cross LATERAL case
(e.g. "select t1.a, x.b from t1 cross join lateral (select b from t2 where t2.a
= t1.a) as x;" with both explain format="plan_tree" and the select to assert
plan + result), and add unsupported-case assertions for LEFT/RIGHT JOIN LATERAL
(e.g. "select ... from t1 left join lateral (...)" and right join lateral) that
check the planner rejects or produces the expected unsupported-plan/error; place
these near the other apply/join examples so they exercise the same session
settings (tidb_enable_cascades_planner) and use the same test tables t1/t2 to
match existing patterns.
In `@tests/integrationtest/t/planner/core/lateral_join.test`:
- Around line 127-133: Update the two EXPLAIN assertions that are currently
brittle by adding the same --replace_column handling used in earlier tests:
modify the EXPLAIN for "select * from t1 join t2 using(a) join t3 on t2.b=t3.b,
lateral (select count(*) as c from t4 where t4.a=t2.a) as dt" (Line 127) and the
EXPLAIN for "select * from t1 join t2 using(a) join t3 on t2.b=t3.b, lateral
(select sum(t4.c) as s from t4 where t4.a=t2.a) as dt" (Line 133) to invoke
--replace_column for plan-id/estRows columns so the assertions ignore transient
plan identifiers; follow the same --replace_column syntax and column names used
in the nearby tests to ensure consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3fff7cea-ceda-4444-965d-0659405aed62
📒 Files selected for processing (40)
errors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/parser/BUILD.bazelpkg/parser/ast/dml.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/lateral_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/planner/cascades/cascades_test.gopkg/planner/core/BUILD.bazelpkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.jsonpkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.jsonpkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_xut.jsonpkg/planner/core/casetest/rule/BUILD.bazelpkg/planner/core/casetest/rule/rule_outer2inner_test.gopkg/planner/core/casetest/rule/testdata/outer2inner_in.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_xut.jsonpkg/planner/core/expression_rewriter.gopkg/planner/core/lateral_join_test.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/hash64_equals_generated.gopkg/planner/core/operator/logicalop/logical_apply.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/planbuilder.gopkg/planner/core/preprocess_test.gopkg/planner/core/rule_decorrelate.gopkg/planner/core/rule_join_reorder.gopkg/util/dbterror/plannererrors/planner_terror.gotests/integrationtest/r/cte.resulttests/integrationtest/r/executor/aggregate.resulttests/integrationtest/r/expression/misc.resulttests/integrationtest/r/planner/cascades/integration.resulttests/integrationtest/r/planner/core/integration.resulttests/integrationtest/r/planner/core/lateral_join.resulttests/integrationtest/t/planner/cascades/integration.testtests/integrationtest/t/planner/core/lateral_join.test
Comment on lines
+675
to
+686
| "SQL": "select * from t1, lateral (select t2.a2, count(*) as cnt from t2 where t2.b2 = t1.a1 group by t2.a2) as dt where dt.cnt > 0 -- LATERAL with aggregate: Apply must not be converted to Join", | ||
| "Plan": [ | ||
| "Projection 64000.00 root test.t1.a1, test.t1.b1, test.t2.a2, Column#9", | ||
| "└─Apply 64000.00 root CARTESIAN inner join", | ||
| " ├─TableReader(Build) 10000.00 root data:TableFullScan", | ||
| " │ └─TableFullScan 10000.00 cop[tikv] table:t1 keep order:false, stats:pseudo", | ||
| " └─Selection(Probe) 64000.00 root gt(Column#9, 0)", | ||
| " └─HashAgg 80000.00 root group by:test.t2.a2, funcs:count(1)->Column#9, funcs:firstrow(test.t2.a2)->test.t2.a2", | ||
| " └─TableReader 100000.00 root data:Selection", | ||
| " └─Selection 100000.00 cop[tikv] eq(test.t2.b2, test.t1.a1)", | ||
| " └─TableFullScan 100000000.00 cop[tikv] table:t2 keep order:false, stats:pseudo" | ||
| ] |
There was a problem hiding this comment.
SELECT * is losing test.t1.c1 in this golden plan.
Line 677 projects only test.t1.a1, test.t1.b1, test.t2.a2, Column#9, but the rest of this suite shows test.t1 has a1, b1, c1. If this is the real plan, the LATERAL aggregate path is dropping part of the outer row; if it is not, this fixture is baking in the wrong schema.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/casetest/rule/testdata/outer2inner_out.json` around lines
675 - 686, The golden plan's Projection node is missing test.t1.c1 (it lists
only test.t1.a1, test.t1.b1, test.t2.a2, Column#9), so either the planner output
was truncated or the test fixture schema baked into outer2inner_out.json is
wrong; update the Projection entry in outer2inner_out.json (the JSON under
"Plan" for the SQL containing "lateral ... count(*) as cnt") to include
test.t1.c1 alongside a1 and b1 (and verify Column#9 remains the aggregate
count), or if the planner should emit that column ensure the test harness
captures the full projection from the Projection node rather than dropping
columns produced by the outer TableFullScan/TableReader for test.t1.
Contributor
Author
There was a problem hiding this comment.
Claude code's response:
The golden output is correct. The table is create table t1(a1 int, b1 int) — only two columns. The Projection test.t1.a1, test.t1.b1, test.t2.a2, Column#9 correctly lists all columns from the SELECT *:
- t1.a1, t1.b1 — both columns of t1
- t2.a2 — from the LATERAL subquery's select t2.a2, count(*) as cnt
- Column#9 — the count(*) aggregate
There is no c1 column in the schema, so nothing is missing. This appears to be a false positive from the reviewer — they may have been looking at a different test's table definition (e.g., TestOuter2Inner uses t1(a1 int, b1 int, c1 int) with 3 columns, but TestOuter2InnerLateralSelection creates its own t1(a1 int, b1 int) with only 2).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test file for TestOuter2InnerLateralSelection to check the table schema
rg -n "TestOuter2InnerLateralSelection" --type=go -A 50 | head -80
Length of output: 5493
@terry1purcell, you're absolutely right — my original comment was a false positive. The script confirms that TestOuter2InnerLateralSelection defines t1(a1 int, b1 int) (Line 186), a two-column table with no c1. The Projection test.t1.a1, test.t1.b1, test.t2.a2, Column#9 correctly captures all columns from SELECT *:
t1.a1,t1.b1— both columns oft1t2.a2— from the LATERAL subquery'sselect t2.a2, count(*)Column#9— thecount(*)aggregate
I was incorrectly cross-referencing the schema from TestOuter2Inner (which uses t1(a1, b1, c1)). The golden plan fixture is correct as-is. Apologies for the noise!
🧠 Learnings used
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T14:43:04.903Z
Learning: Applies to pkg/planner/** : For planner rule or logical/physical plan changes, perform targeted planner unit tests and update rule testdata when needed.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-17T04:39:16.731Z
Learning: Follow `.agents/skills/tidb-test-guidelines` for placement, naming, `shard_count` guidance, planner-specific casetest rules, and related testdata conventions
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T12:48:57.065Z
Learning: Applies to pkg/executor/** : For SQL behavior changes in executor, perform targeted unit test plus relevant integration test.
Learnt from: guo-shaoge
Repo: pingcap/tidb PR: 66349
File: tests/integrationtest/r/tpch.result:447-447
Timestamp: 2026-03-11T03:48:51.420Z
Learning: In `pkg/planner/core/joinorder/` (TiDB join reorder), cartesian edges should only be considered in two specific scenarios:
1. After the entire join reorder is complete and there are still disjoint join groups that need to be connected.
2. When a `leading` hint explicitly requires a cartesian join.
PR `#66213` attempted to fix case 2 but accidentally enabled case 1 as well. The correct logic ensures cartesian edges are gated to only these two situations.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:1906-1910
Timestamp: 2026-03-12T08:03:56.097Z
Learning: In pingcap/tidb (PR `#65532`), `buildSelect` in `pkg/planner/core/logical_plan_builder.go` derives the privilege-check DB for `FOR UPDATE OF` lock targets using `getLowerDB(tName.Schema, ...)`. For alias or unqualified lock targets whose `TableName.Schema` is empty after preprocessing, this falls back to `currentDB` and authorizes against the wrong database. The correct fix is to use `tNameW.DBInfo.Name.L` (with a nil guard) directly, since `tNameW` is already fully resolved by `checkLockClauseTables` in preprocess.go. This is consistent with how privilege checks for INSERT/DELETE use `tnW.DBInfo.Name.L` elsewhere in the codebase.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:1887-1898
Timestamp: 2026-03-12T08:41:44.509Z
Learning: In pkg/planner/core/preprocess.go (pingcap/tidb PR `#65532`), the `checkLockClauseTables` function intentionally accepts `FOR UPDATE OF <base_table>` even when the FROM item was introduced with an alias (`matched.alias.L != "" && !matchedByAlias`). Rather than returning an error (strict MySQL behavior), TiDB emits a warning to guide users toward the alias form. This backward-compatibility fallback is a deliberate acceptance criterion for the change — do not flag it as a MySQL-incompatibility bug in future reviews.
Comment on lines
+950
to
+959
| if fullSchema, fullNames := findJoinFullSchema(leftPlan); fullSchema != nil { | ||
| lFullSchema = fullSchema | ||
| lFullNames = fullNames | ||
| } else { | ||
| lFullSchema = leftPlan.Schema() | ||
| lFullNames = leftPlan.OutputNames() | ||
| } | ||
| // For LATERAL, right side is always a derived table (never a LogicalJoin with FullSchema) | ||
| rFullSchema = rightPlan.Schema() | ||
| rFullNames = rightPlan.OutputNames() |
There was a problem hiding this comment.
Preserve the right child's FullSchema here as well.
rightPlan is not always a plain derived table at this point; parenthesized right operands can already build into a LogicalJoin / LogicalApply with FullSchema from an inner USING / NATURAL join. Falling back to rightPlan.Schema() / OutputNames() drops those redundant columns, so upper qualified references and wildcards resolve against the wrong shape.
♻️ Suggested fix
- // For LATERAL, right side is always a derived table (never a LogicalJoin with FullSchema)
- rFullSchema = rightPlan.Schema()
- rFullNames = rightPlan.OutputNames()
+ if fullSchema, fullNames := findJoinFullSchema(rightPlan); fullSchema != nil {
+ rFullSchema = fullSchema
+ rFullNames = fullNames
+ } else {
+ rFullSchema = rightPlan.Schema()
+ rFullNames = rightPlan.OutputNames()
+ }🤖 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 950 - 959, The right
child's FullSchema is being dropped by unconditionally using
rightPlan.Schema()/OutputNames(); instead call findJoinFullSchema on rightPlan
(just like done for leftPlan) and if it returns non-nil set rFullSchema and
rFullNames from that result, otherwise fall back to rightPlan.Schema() and
rightPlan.OutputNames(); reference the existing findJoinFullSchema function and
the rFullSchema/rFullNames variables (and consider LogicalJoin/LogicalApply as
possible rightPlan types that carry FullSchema).
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin AUTO_INCREMENT=2000001; | ||
| explain format='plan_tree' SELECT OUTR . col2 AS X FROM (SELECT INNR . col1 as col1, SUM( INNR . col2 ) as col2 FROM (SELECT INNR . `col_int_not_null` + 1 as col1, INNR . `pk` as col2 FROM BB AS INNR) AS INNR GROUP BY col1) AS OUTR2 INNER JOIN (SELECT INNR . col1 as col1, MAX( INNR . col2 ) as col2 FROM (SELECT INNR . `col_int_not_null` + 1 as col1, INNR . `pk` as col2 FROM BB AS INNR) AS INNR GROUP BY col1) AS OUTR ON OUTR2.col1 = OUTR.col1 GROUP BY OUTR . col1, OUTR2 . col1 HAVING X <> 'b'; | ||
| Error 1055 (42000): Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column '.OUTR.' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by | ||
| Error 1055 (42000): Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'OUTR.' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by |
There was a problem hiding this comment.
Error text appears to drop the referenced column name.
At Line 2844, the expected message uses 'OUTR.', which loses the actual column identifier and makes the diagnostic less actionable. Please keep the full reference (e.g., OUTR.col2) and align the output with that.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/r/planner/core/integration.result` at line 2844, The
expected error text currently shows a truncated column reference 'OUTR.'; update
the test fixture to use the full identifier (e.g., replace 'OUTR.' with
'OUTR.col2' in the expected output) and/or fix the error-message serialization
routine (e.g., the function that formats SQL errors or expression references
such as serializeExpression/formatSqlError) so it preserves the column name
after the table alias instead of dropping it; ensure the produced and expected
messages match exactly with the full reference 'OUTR.col2'.
| create table t2 (a int, b varchar(10)); | ||
| insert into t1 values (1), (2), (3); | ||
| insert into t2 values (1, 'a'), (1, 'b'), (2, 'c'), (3, 'd'), (3, 'e'); | ||
| select * from t1, lateral (select b from t2 where t2.a = t1.a limit 1) as lat order by t1.a; |
There was a problem hiding this comment.
Stabilize LIMIT 1 lateral subqueries to avoid flaky test outputs.
Line 6, Line 26, and Line 114 use LIMIT 1 without ORDER BY, so chosen rows are not deterministic.
Suggested deterministic update
-select * from t1, lateral (select b from t2 where t2.a = t1.a limit 1) as lat order by t1.a;
+select * from t1, lateral (select b from t2 where t2.a = t1.a order by b limit 1) as lat order by t1.a;
-select * from t1 left join lateral (select b from t2 where t2.a = t1.a limit 1) as lat on true order by t1.a;
+select * from t1 left join lateral (select b from t2 where t2.a = t1.a order by b limit 1) as lat on true order by t1.a;
-select * from t1, lateral (select b from t2 limit 1) as lat order by t1.a;
+select * from t1, lateral (select b from t2 order by b limit 1) as lat order by t1.a;Also applies to: 26-26, 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/r/planner/core/lateral_join.result` at line 6,
Stabilize the nondeterministic lateral subqueries by adding an explicit ORDER BY
before LIMIT 1 so the chosen row is deterministic; update the lateral clauses
like "lateral (select b from t2 where t2.a = t1.a limit 1) as lat" to include a
deterministic sort (for example "order by b" or the table's primary key) in each
occurrence (the queries containing the lateral subquery at the instances shown).
Comment on lines
+1
to
+282
| # TestSimpleProjDual | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select 1; | ||
| select 1; | ||
|
|
||
| # TestPKIsHandleRangeScan | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int); | ||
| insert into t values(1,2),(3,4),(5,6); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select b from t where a > 1; | ||
| select b from t where a > 1; | ||
| explain format="plan_tree" select b from t where a > 1 and a < 3; | ||
| select b from t where a > 1 and a < 3; | ||
| explain format="plan_tree" select b from t where a > 1 and b < 6; | ||
| select b from t where a > 1 and b < 6; | ||
| explain format="plan_tree" select a from t where a * 3 + 1 > 9 and a < 5; | ||
| select a from t where a * 3 + 1 > 9 and a < 5; | ||
| explain format="plan_tree" select a from t group by a having sum(b) > 4; | ||
| select a from t group by a having sum(b) > 4; | ||
|
|
||
| # TestIndexScan | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int, c int, d int, index idx_b(b), index idx_c_b(c, b)); | ||
| insert into t values(1,2,3,100),(4,5,6,200),(7,8,9,300); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select b from t; | ||
| select b from t; | ||
| explain format="plan_tree" select a from t order by b; | ||
| select a from t order by b; | ||
| explain format="plan_tree" select c from t; | ||
| select c from t; | ||
| explain format="plan_tree" select a from t order by c; | ||
| select a from t order by c; | ||
| explain format="plan_tree" select a, b from t where b > 5 order by b; | ||
| select a, b from t where b > 5 order by b; | ||
| explain format="plan_tree" select a, b, c from t where c = 3 and b > 1 order by b; | ||
| select a, b, c from t where c = 3 and b > 1 order by b; | ||
| explain format="plan_tree" select a, b from t where c > 1 and b > 1 order by c; | ||
| select a, b from t where c > 1 and b > 1 order by c; | ||
|
|
||
| # TestBasicShow | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| desc t; | ||
|
|
||
| # TestSort | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int); | ||
| insert into t values (1, 11), (4, 44), (2, 22), (3, 33); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select a from t order by a; | ||
| select a from t order by a; | ||
| explain format="plan_tree" select b from t order by b; | ||
| select b from t order by b; | ||
| explain format="plan_tree" select b from t order by a+b; | ||
| select b from t order by a+b; | ||
| explain format="plan_tree" select b from t order by b, a+b, a; | ||
| select b from t order by b, a+b, a; | ||
|
|
||
| # TestAggregation | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int); | ||
| insert into t values (1, 11), (4, 44), (2, 22), (3, 33); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| set session tidb_executor_concurrency = 4; | ||
| set @@session.tidb_hash_join_concurrency = 5; | ||
| set @@session.tidb_distsql_scan_concurrency = 15; | ||
| explain format="plan_tree" select sum(a) from t; | ||
| select sum(a) from t; | ||
| explain format="plan_tree" select max(a), min(b) from t; | ||
| select max(a), min(b) from t; | ||
| explain format="plan_tree" select b, avg(a) from t group by b order by b; | ||
| select b, avg(a) from t group by b order by b; | ||
| explain format="plan_tree" select b, sum(a) from t group by b order by b; | ||
| select b, sum(a) from t group by b order by b; | ||
| explain format="plan_tree" select b, avg(a) from t group by b having sum(a) > 1 order by b; | ||
| select b, avg(a) from t group by b having sum(a) > 1 order by b; | ||
| explain format="plan_tree" select max(a+b) from t; | ||
| select max(a+b) from t; | ||
| explain format="plan_tree" select sum(a) from t group by a, a+b order by a; | ||
| select sum(a) from t group by a, a+b order by a; | ||
| explain format="plan_tree" select b, sum(a) from t group by b having b > 1 order by b; | ||
| select b, sum(a) from t group by b having b > 1 order by b; | ||
| explain format="plan_tree" select c, sum(a) from (select a+b as c, a from t) t1 group by c having c > 1 order by c; | ||
| select c, sum(a) from (select a+b as c, a from t) t1 group by c having c > 1 order by c; | ||
| explain format="plan_tree" select max(a.a) from t a left join t b on a.a = b.a; | ||
| select max(a.a) from t a left join t b on a.a = b.a; | ||
| explain format="plan_tree" select avg(a.b) from t a left join t b on a.a = b.a; | ||
| select avg(a.b) from t a left join t b on a.a = b.a; | ||
| explain format="plan_tree" select t1.a, max(t1.b) from t as t1 left join (select * from t) as t2 on t1.a = t2.a and t1.b = 3 group by t1.a order by a; | ||
| select t1.a, max(t1.b) from t as t1 left join (select * from t) as t2 on t1.a = t2.a and t1.b = 3 group by t1.a order by a; | ||
| explain format="plan_tree" select t1.a, sum(distinct t1.b) from t as t1 left join (select * from t) as t2 on t1.b = t2.b group by t1.a order by a; | ||
| select t1.a, sum(distinct t1.b) from t as t1 left join (select * from t) as t2 on t1.b = t2.b group by t1.a order by a; | ||
| explain format="plan_tree" select t2.a, max(t2.b) from t as t1 right join (select * from t) as t2 on t1.a = t2.a group by t2.a order by a; | ||
| select t2.a, max(t2.b) from t as t1 right join (select * from t) as t2 on t1.a = t2.a group by t2.a order by a; | ||
| explain format="plan_tree" select t3.a, max(t3.b) from (select t1.a, t1.b from t as t1 left join t as t2 on t1.b = t2.b) t3 group by t3.a order by a; | ||
| select t3.a, max(t3.b) from (select t1.a, t1.b from t as t1 left join t as t2 on t1.b = t2.b) t3 group by t3.a order by a; | ||
| explain format="plan_tree" select max(a) from t; | ||
| select max(a) from t; | ||
| explain format="plan_tree" select sum(case when a > 0 and a <= 1000 then b else 0 end) from t; | ||
| select sum(case when a > 0 and a <= 1000 then b else 0 end) from t; | ||
| explain format="plan_tree" select sum(case when a > 0 then (case when a <= 1000 then b end) else 0 end) from t; | ||
| select sum(case when a > 0 then (case when a <= 1000 then b end) else 0 end) from t; | ||
| explain format="plan_tree" select sum(case when a <= 0 or a > 1000 then 0.0 else b end) from t; | ||
| select sum(case when a <= 0 or a > 1000 then 0.0 else b end) from t; | ||
| explain format="plan_tree" select count(case when a > 0 and a <= 1000 then b end) from t; | ||
| select count(case when a > 0 and a <= 1000 then b end) from t; | ||
| explain format="plan_tree" select count(case when a <= 0 or a > 1000 then null else b end) from t; | ||
| select count(case when a <= 0 or a > 1000 then null else b end) from t; | ||
| explain format="plan_tree" select count(distinct case when a > 0 and a <= 1000 then b end) from t; | ||
| select count(distinct case when a > 0 and a <= 1000 then b end) from t; | ||
| explain format="plan_tree" select approx_count_distinct(case when a > 0 and a <= 1000 then b end) from t; | ||
| select approx_count_distinct(case when a > 0 and a <= 1000 then b end) from t; | ||
| explain format="plan_tree" select count(b), sum(b), avg(b), b, max(b), min(b), bit_and(b), bit_or(b), bit_xor(b) from t group by a having sum(b) >= 0 and count(b) >= 0 order by b; | ||
| select count(b), sum(b), avg(b), b, max(b), min(b), bit_and(b), bit_or(b), bit_xor(b) from t group by a having sum(b) >= 0 and count(b) >= 0 order by b; | ||
| explain format="plan_tree" select group_concat(a, b), min(b), avg(a / b), a from t group by (a+b), a order by a; | ||
| select group_concat(a, b), min(b), avg(a / b), a from t group by (a+b), a order by a; | ||
| explain format="plan_tree" select approx_percentile(a, 50) from t order by b; | ||
| select approx_percentile(a, 50) from t order by b; | ||
|
|
||
| # TestPushdownDistinctEnable | ||
| drop table if exists t; | ||
| create table t(a int, b int, c int, index(c)); | ||
| insert into t values (1, 1, 1), (1, 1, 3), (1, 2, 3), (2, 1, 3), (1, 2, NULL); | ||
| set session sql_mode=''; | ||
| set session tidb_hashagg_partial_concurrency=1; | ||
| set session tidb_hashagg_final_concurrency=1; | ||
| set session tidb_enable_cascades_planner = 1; | ||
| set @@session.tidb_opt_distinct_agg_push_down = 1; | ||
| explain format="plan_tree" select /*+ HASH_AGG() */ avg(distinct a) from t; | ||
| select /*+ HASH_AGG() */ avg(distinct a) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG() */ a, count(distinct a) from t; | ||
| --replace_column 1 1 | ||
| select /*+ HASH_AGG() */ a, count(distinct a) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c; | ||
| --sorted_result | ||
| select /*+ HASH_AGG() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c; | ||
| explain format="plan_tree" select /*+ STREAM_AGG() */ count(distinct c) from t group by c; | ||
| --sorted_result | ||
| select /*+ STREAM_AGG() */ count(distinct c) from t group by c; | ||
| explain format="plan_tree" select /*+ STREAM_AGG() */ count(distinct c) from t; | ||
| select /*+ STREAM_AGG() */ count(distinct c) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG() */ count(distinct c) from t; | ||
| select /*+ HASH_AGG() */ count(distinct c) from t; | ||
| explain format="plan_tree" select count(distinct c) from t group by c; | ||
| --sorted_result | ||
| select count(distinct c) from t group by c; | ||
| explain format="plan_tree" select count(distinct c) from t; | ||
| select count(distinct c) from t; | ||
|
|
||
| # TestPushdownDistinctDisable | ||
| drop table if exists t; | ||
| create table t(a int, b int, c int, index(c)); | ||
| insert into t values (1, 1, 1), (1, 1, 3), (1, 2, 3), (2, 1, 3), (1, 2, NULL); | ||
| set session sql_mode=''; | ||
| set session tidb_hashagg_partial_concurrency=1; | ||
| set session tidb_hashagg_final_concurrency=1; | ||
| set session tidb_enable_cascades_planner = 1; | ||
| set @@session.tidb_opt_distinct_agg_push_down = 0; | ||
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ avg(distinct a) from t; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ avg(distinct a) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ a, count(distinct a) from t; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ a, count(distinct a) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c; | ||
| explain format="plan_tree" select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t group by c; | ||
| select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t group by c; | ||
| explain format="plan_tree" select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| explain format="plan_tree" select /*+ AGG_TO_COP() */ count(distinct c) from t group by c; | ||
| select /*+ AGG_TO_COP() */ count(distinct c) from t group by c; | ||
|
|
||
| # TestSimplePlans | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int); | ||
| insert into t values (1, 11), (4, 44), (2, 22), (3, 33); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select a from t limit 2; | ||
| select a from t limit 2; | ||
| explain format="plan_tree" select a from t limit 1 offset 2; | ||
| select a from t limit 1 offset 2; | ||
| explain format="plan_tree" select b from t order by b limit 3; | ||
| select b from t order by b limit 3; | ||
| explain format="plan_tree" select a from t order by a limit 1 offset 2; | ||
| select a from t order by a limit 1 offset 2; | ||
| explain format="plan_tree" select * from ((select a as aa from t t1) union all (select b as aa from t t2)) as t3 order by aa; | ||
| select * from ((select a as aa from t t1) union all (select b as aa from t t2)) as t3 order by aa; | ||
| explain format="plan_tree" select a, b, lag(a,1) over (order by b) from t order by b; | ||
| select a, b, lag(a,1) over (order by b) from t order by b; | ||
| explain format="plan_tree" select * from (select a+1 as c, a+b as d from t) as t1 order by c+d limit 10; | ||
| select * from (select a+1 as c, a+b as d from t) as t1 order by c+d limit 10; | ||
| explain format="plan_tree" select t1.a, t1.b from t as t1 left join t as t2 on t1.a = t2.a and t1.b = 3 order by a; | ||
| select t1.a, t1.b from t as t1 left join t as t2 on t1.a = t2.a and t1.b = 3 order by a; | ||
|
|
||
| # TestJoin | ||
| set @@session.tidb_executor_concurrency = 4; | ||
| set @@session.tidb_hash_join_concurrency = 5; | ||
| set @@session.tidb_distsql_scan_concurrency = 15; | ||
| drop table if exists t1; | ||
| drop table if exists t2; | ||
| create table t1(a int primary key, b int); | ||
| create table t2(a int primary key, b int); | ||
| insert into t1 values (1, 11), (4, 44), (2, 22), (3, 33); | ||
| insert into t2 values (1, 111), (2, 222), (3, 333), (5, 555); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select t1.a, t1.b from t1, t2 where t1.a = t2.a and t1.a > 2; | ||
| select t1.a, t1.b from t1, t2 where t1.a = t2.a and t1.a > 2; | ||
| explain format="plan_tree" select t1.a, t1.b from t1, t2 where t1.a > t2.a and t2.b > 200; | ||
| select t1.a, t1.b from t1, t2 where t1.a > t2.a and t2.b > 200; | ||
| explain format="plan_tree" select t1.a, t1.b from t1 left join t2 on t1.a = t2.a where t1.a > 2 and t2.b > 200; | ||
| select t1.a, t1.b from t1 left join t2 on t1.a = t2.a where t1.a > 2 and t2.b > 200; | ||
| explain format="plan_tree" select t2.a, t2.b from t1 right join t2 on t1.a = t2.a where t1.a > 2 and t2.b > 200; | ||
| select t2.a, t2.b from t1 right join t2 on t1.a = t2.a where t1.a > 2 and t2.b > 200; | ||
| explain format="plan_tree" select t1.a, t1.b from t1, t2 where t1.a = t2.a order by t1.a; | ||
| select t1.a, t1.b from t1, t2 where t1.a = t2.a order by t1.a; | ||
| explain format="plan_tree" select * from t1 join t2 on t1.a = t2.a; | ||
| select * from t1 join t2 on t1.a = t2.a; | ||
| explain format="plan_tree" select * from t1 join t2 on t1.a = t2.a and t1.a > 2; | ||
| select * from t1 join t2 on t1.a = t2.a and t1.a > 2; | ||
|
|
||
| # TestApply | ||
| drop table if exists t1, t2; | ||
| create table t1(a int primary key, b int); | ||
| create table t2(a int primary key, b int); | ||
| insert into t1 values (1, 11), (4, 44), (2, 22), (3, 33); | ||
| insert into t2 values (1, 11), (2, 22), (3, 33); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select a = (select a from t2 where t1.b = t2.b order by a limit 1) from t1; | ||
| select a = (select a from t2 where t1.b = t2.b order by a limit 1) from t1; | ||
| explain format="plan_tree" select sum(a), (select t1.a from t1 where t1.a = t2.a limit 1), (select t1.b from t1 where t1.b = t2.b limit 1) from t2; | ||
| select sum(a), (select t1.a from t1 where t1.a = t2.a limit 1), (select t1.b from t1 where t1.b = t2.b limit 1) from t2; | ||
| explain format="plan_tree" select /*+ set_var(tidb_hash_join_version=legacy) */ a from t1 where exists(select 1 from t2 where t1.a = t2.a); | ||
| explain format="plan_tree" select /*+ set_var(tidb_hash_join_version=optimized) */ a from t1 where exists(select 1 from t2 where t1.a = t2.a); | ||
| select a from t1 where exists(select 1 from t2 where t1.a = t2.a); | ||
|
|
||
| # TestMemTableScan | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select USER, DB, COMMAND, TIME, STATE, INFO, `DIGEST` from information_schema.processlist where DB = "planner__cascades__integration"; | ||
| select USER, DB, COMMAND, TIME, STATE, INFO, `DIGEST` from information_schema.processlist where DB = "planner__cascades__integration"; | ||
|
|
||
| # TestTopN | ||
| drop table if exists t; | ||
| create table t(a int primary key, b int); | ||
| insert into t values (1, 11), (4, 44), (2, 22), (3, 33); | ||
| set session tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select a from (select a from t where b > 2 order by a limit 3 offset 1) as t1 order by a limit 2 offset 1; | ||
| select a from (select a from t where b > 2 order by a limit 3 offset 1) as t1 order by a limit 2 offset 1; | ||
| explain format="plan_tree" select * from (select * from t order by a limit 3) as t1 order by a limit 5; | ||
| select * from (select * from t order by a limit 3) as t1 order by a limit 5; | ||
| explain format="plan_tree" select b from (select b from t order by b limit 10 offset 10) as t1 order by b limit 10 offset 5; | ||
| select b from (select b from t order by b limit 10 offset 10) as t1 order by b limit 10 offset 5; | ||
| explain format="plan_tree" select b from (select b from t order by b limit 10 offset 2) as t1 order by b limit 3 offset 5; | ||
| select b from (select b from t order by b limit 10 offset 2) as t1 order by b limit 3 offset 5; | ||
| explain format="plan_tree" select a from (select a from t order by a limit 3 offset 5) as t1 order by a limit 3 offset 5; | ||
| select a from (select a from t order by a limit 3 offset 5) as t1 order by a limit 3 offset 5; | ||
| explain format="plan_tree" select a from (select a from t where b > 2 order by a, b limit 3 offset 1) as t1 order by a limit 2 offset 1; | ||
| select a from (select a from t where b > 2 order by a, b limit 3 offset 1) as t1 order by a limit 2 offset 1; | ||
| explain format="plan_tree" select * from (select * from t order by a limit 3) as t1 order by a, b limit 5; | ||
| select * from (select * from t order by a limit 3) as t1 order by a, b limit 5; | ||
|
|
||
|
|
||
| # TestCascadePlannerHashedPartTable | ||
| SET SESSION tidb_opt_fix_control = '44262:ON'; | ||
| drop table if exists pt1; | ||
| create table pt1(a bigint, b bigint) partition by hash(a) partitions 4; | ||
| insert into pt1 values(1,10); | ||
| insert into pt1 values(2,20); | ||
| insert into pt1 values(3,30); | ||
| insert into pt1 values(4,40); | ||
| insert into pt1 values(5,50); | ||
| set @@tidb_enable_cascades_planner = 1; | ||
| explain format="plan_tree" select * from pt1 order by a; | ||
| select * from pt1 order by a; | ||
|
|
||
| set session tidb_opt_fix_control = default; | ||
| set @@tidb_enable_cascades_planner = default; | ||
|
|
||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TEST_FILE="tests/integrationtest/t/planner/cascades/integration.test"
RESULT_FILE="tests/integrationtest/r/planner/cascades/integration.result"
echo "Checking result file presence..."
fd -p "$(basename "$RESULT_FILE")" tests/integrationtest/r/planner/cascades
python - <<'PY'
import re
from pathlib import Path
test_file = Path("tests/integrationtest/t/planner/cascades/integration.test")
result_file = Path("tests/integrationtest/r/planner/cascades/integration.result")
if not result_file.exists():
print(f"ERROR: missing {result_file}")
raise SystemExit(1)
def case_headers(path: Path):
out = []
for line in path.read_text().splitlines():
m = re.match(r"^#\s*(.+)$", line)
if m:
out.append(m.group(1).strip())
return out
tc = case_headers(test_file)
rc = case_headers(result_file)
print(f"test headers: {len(tc)}")
print(f"result headers: {len(rc)}")
missing = [c for c in tc if c not in rc]
extra = [c for c in rc if c not in tc]
print("missing_in_result:", missing[:20])
print("extra_in_result:", extra[:20])
if missing:
raise SystemExit(2)
PYRepository: pingcap/tidb
Length of output: 513
Regenerate the .result file with all test case results.
The .result file at tests/integrationtest/r/planner/cascades/integration.result exists but contains no test case results. All 14 test cases in the corresponding .test file (TestSimpleProjDual, TestPKIsHandleRangeScan, TestIndexScan, TestBasicShow, TestSort, TestAggregation, TestPushdownDistinctEnable, TestPushdownDistinctDisable, TestSimplePlans, TestJoin, TestApply, TestMemTableScan, TestTopN, TestCascadePlannerHashedPartTable) must be regenerated and checked in with their expected output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/t/planner/cascades/integration.test` around lines 1 -
282, The result file tests/integrationtest/r/planner/cascades/integration.result
is empty and must be populated with the expected outputs for the 14 tests
defined in tests/integrationtest/t/planner/cascades/integration.test (e.g.,
TestSimpleProjDual, TestPKIsHandleRangeScan, TestIndexScan, TestBasicShow,
TestSort, TestAggregation, TestPushdownDistinctEnable,
TestPushdownDistinctDisable, TestSimplePlans, TestJoin, TestApply,
TestMemTableScan, TestTopN, TestCascadePlannerHashedPartTable); run the
integration test harness (or the equivalent test runner used by the repo)
against that .test file with the correct environment flags
(tidb_enable_cascades_planner etc.), capture the explain/select outputs, replace
the empty integration.result with the regenerated outputs, and commit the
updated tests/integrationtest/r/planner/cascades/integration.result so it
contains the expected results for each named test case.
Comment on lines
+164
to
+175
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ a, count(distinct a) from t; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ a, count(distinct a) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c; | ||
| explain format="plan_tree" select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t group by c; | ||
| select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t group by c; | ||
| explain format="plan_tree" select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| select /*+ HASH_AGG(), AGG_TO_COP() */ count(distinct c) from t; | ||
| explain format="plan_tree" select /*+ AGG_TO_COP() */ count(distinct c) from t group by c; | ||
| select /*+ AGG_TO_COP() */ count(distinct c) from t group by c; |
There was a problem hiding this comment.
Stabilize nondeterministic assertions in TestPushdownDistinctDisable.
select ... a, count(distinct a) on Line [165] is nondeterministic with sql_mode='', and the GROUP BY c result sets on Lines [167], [169], and [175] can reorder. Please mirror the normalization directives already used in TestPushdownDistinctEnable.
Proposed patch
explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ a, count(distinct a) from t;
+--replace_column 1 1
select /*+ HASH_AGG(), AGG_TO_COP() */ a, count(distinct a) from t;
explain format="plan_tree" select /*+ HASH_AGG(), AGG_TO_COP() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c;
+--sorted_result
select /*+ HASH_AGG(), AGG_TO_COP() */ avg(b), c, avg(b), count(distinct A, B), count(distinct A), count(distinct c), sum(b) from t group by c;
explain format="plan_tree" select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t group by c;
+--sorted_result
select /*+ STREAM_AGG(), AGG_TO_COP() */ count(distinct c) from t group by c;
@@
explain format="plan_tree" select /*+ AGG_TO_COP() */ count(distinct c) from t group by c;
+--sorted_result
select /*+ AGG_TO_COP() */ count(distinct c) from t group by c;Based on learnings: Follow .agents/skills/tidb-test-guidelines for planner-specific casetest rules and related testdata conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integrationtest/t/planner/cascades/integration.test` around lines 164 -
175, TestPushdownDistinctDisable contains nondeterministic assertions (the query
with "a, count(distinct a)" and the several "GROUP BY c" selects) so mirror the
normalization directives used in TestPushdownDistinctEnable: add the same
output-normalization/comments (the stable result ordering/normalization
directives) immediately before the affected statements in
TestPushdownDistinctDisable so the plan/result comparisons are deterministic for
the "select /*+ ... */ a, count(distinct a) ..." case and for all "group by c"
queries; ensure you apply the directives to the explain/select pairs that
include "count(distinct ...)" and "group by c" so they follow the same
normalization rules as TestPushdownDistinctEnable.
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| } | ||
| if x.AsName.L != "" { | ||
| name.TblName = x.AsName | ||
| // Clone output names before modifying to avoid mutating shared structs |
There was a problem hiding this comment.
[P2] LATERAL derived-table column alias list parsed but not applied
Why: MySQL compatibility issue - queries using LATERAL ... AS dt(c1, c2) syntax will fail name resolution because the planner never applies the column alias list to the derived table's output fields.
Evidence: The parser sets ts.ColumnNames = $4.([]ast.CIStr) at pkg/parser/parser.y:10408, but the planner at pkg/planner/core/logical_plan_builder.go:500 only rewrites the table alias (TblName) and keeps ColName: name.ColName unchanged, never consulting x.ColumnNames. This means dt.c1 cannot resolve and the column list can't fix duplicate output-name errors.
Impact: Affects LATERAL derived tables using the optional column-list syntax. Workaround: use SELECT ... AS c1, ... AS c2 inside the subquery.
| lFullSchema = leftPlan.Schema() | ||
| lFullNames = leftPlan.OutputNames() | ||
| } | ||
| // For LATERAL, right side is always a derived table (never a LogicalJoin with FullSchema) |
There was a problem hiding this comment.
[P1] buildLateralJoin drops right-side FullSchema/FullNames
Why: Can break redundant-column name resolution (USING/NATURAL joins) when a LATERAL inside a parenthesized right subtree forces an outer Apply. This causes "Unknown column" errors for otherwise-valid SQL.
Evidence: At pkg/planner/core/logical_plan_builder.go:957, the code hardcodes rFullSchema = rightPlan.Schema(); rFullNames = rightPlan.OutputNames() under the assumption that "the right side is always a derived table". However, when the right subtree is a join tree containing a LATERAL node, the right plan can be a LogicalJoin/LogicalApply that already has FullSchema/FullNames with USING/NATURAL redundant columns. Dropping them breaks name resolution.
Minimal reproducer: SELECT t3.a FROM t AS t1 JOIN (t AS t2 JOIN t AS t3 USING(a) JOIN LATERAL (SELECT 1) AS dt ON true) ON true → yields [planner:1054]Unknown column 't3.a' in 'field list' because the redundant t3.a only exists in the right subtree FullSchema, which the outer Apply drops.
Note: By contrast, non-LATERAL joins correctly propagate FullSchema/FullNames at pkg/planner/core/logical_plan_builder.go:779.
… sources The previous guard only checked *TableName but still allowed LATERAL and ColumnNames on other non-derived sources like *Join. Switch to an allowlist: only permit these fields when Source is *SelectStmt or *SetOprStmt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/parser/ast/dml.go (1)
555-559: Avoid duplicated derived-source checks inRestore.
needParenrepeats the same source-type test already used forisDerived. ReuseisDerivedto keep one source of truth.♻️ Suggested cleanup
- isDerived := false - switch n.Source.(type) { - case *SelectStmt, *SetOprStmt: - isDerived = true - } + isDerived := false + switch n.Source.(type) { + case *SelectStmt, *SetOprStmt: + isDerived = true + } @@ - needParen := false - switch n.Source.(type) { - case *SelectStmt, *SetOprStmt: - needParen = true - } + needParen := isDerivedAs per coding guidelines "Code SHOULD remain maintainable for future readers with basic TiDB familiarity; follow existing package-local conventions first and keep style consistent with nearby files."
Also applies to: 570-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/ast/dml.go` around lines 555 - 559, The Restore method currently computes isDerived via a type switch on n.Source and then recomputes the same test in the needParen logic; update Restore to compute isDerived once (using the existing type switch for *SelectStmt and *SetOprStmt) and then have needParen reference that isDerived variable instead of redoing the type assertion, and remove the duplicate source-type checks (including the secondary block around the code at the other occurrence between the regions corresponding to lines ~570-574) so there is a single source-of-truth for derived-source detection.
🤖 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/parser/ast/dml.go`:
- Around line 555-559: The Restore method currently computes isDerived via a
type switch on n.Source and then recomputes the same test in the needParen
logic; update Restore to compute isDerived once (using the existing type switch
for *SelectStmt and *SetOprStmt) and then have needParen reference that
isDerived variable instead of redoing the type assertion, and remove the
duplicate source-type checks (including the secondary block around the code at
the other occurrence between the regions corresponding to lines ~570-574) so
there is a single source-of-truth for derived-source detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0c4c0141-f729-44e1-9912-e94d0b2b7d64
📒 Files selected for processing (1)
pkg/parser/ast/dml.go
…gJoinPlan LogicalApply embeds LogicalJoin and has the same FullSchema/FullNames fields for USING/NATURAL column remapping. Without this case, qualified column names over a LATERAL Apply would bind to names not present in Apply.Schema(), breaking later index resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ableSource Move the buildingLateralSubquery flag from buildJoin (where it covers the entire right branch including non-LATERAL siblings) into buildResultSetNode's TableSource handler (where x.Lateral is checked). This prevents non-LATERAL derived tables from bypassing recursive-CTE ORDER BY/LIMIT checks when they happen to be siblings of a LATERAL table in the same join tree. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@terry1purcell: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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/planner/core/logical_plan_builder.go (1)
4064-4089:⚠️ Potential issue | 🟠 MajorUnwrap wrapper plans before expanding qualified
*.Top-level inner joins with
ONare usuallyLogicalSelectionover the real join/apply. This direct type switch misses the underlyingFullSchema, sotbl.*on top of aUSING/NATURALsubtree still drops the redundant table-qualified columns.♻️ Suggested fix
- // Extract FullSchema/FullNames from LogicalJoin or LogicalApply (which embeds LogicalJoin). - var fullSchema *expression.Schema - var fullNames types.NameSlice - switch x := p.(type) { - case *logicalop.LogicalJoin: - fullSchema, fullNames = x.FullSchema, x.FullNames - case *logicalop.LogicalApply: - fullSchema, fullNames = x.FullSchema, x.FullNames - } + fullSchema, fullNames := findJoinFullSchema(p)🤖 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 4064 - 4089, In unfoldWildStar, the type switch on p only recognizes *logicalop.LogicalJoin and *logicalop.LogicalApply and therefore misses wrapped joins (e.g., a *logicalop.LogicalSelection that wraps the real join), causing qualified `tbl.*` to lose FullSchema/FullNames; fix by unwrapping simple unary wrapper plans before the switch (e.g., while p is a *logicalop.LogicalSelection or other single-child wrapper, set p = p.Children()[0] or use the plan's embedded child accessor) so that the subsequent switch in unfoldWildStar sees the underlying *logicalop.LogicalJoin or *logicalop.LogicalApply and can obtain FullSchema and FullNames correctly.
♻️ Duplicate comments (2)
pkg/planner/core/logical_plan_builder.go (2)
918-963:⚠️ Potential issue | 🟠 MajorDon't treat the whole right subtree as a plain derived table.
For parenthesized operands that contain a
LATERALsource,rightPlancan already be aLogicalJoin/LogicalApplywithFullSchemaand base-tableOutputNames. UsingrightPlan.Schema()/OutputNames()here drops redundantUSING/NATURALcolumns again, and the unconditional DB-name clearing above also de-qualifies those base-table outputs.🤖 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 918 - 963, The code incorrectly assumes the entire right subtree is a derived table: when building outputNames and setting rFullSchema/rFullNames, detect whether rightPlan already has a join-style FullSchema/FullNames (use the same helper as left, e.g. call findJoinFullSchema(rightPlan)) and, if present, use that FullSchema/FullNames and preserve the existing right output FieldName.DBName/qualification instead of clearing DBName; only clear DBName and treat outputs as derived-table names when no FullSchema/FullNames are found for rightPlan. Ensure ap.SetOutputNames uses the chosen name slice consistently and that rFullSchema/rFullNames are set from the findJoinFullSchema result when non-nil.
505-550:⚠️ Potential issue | 🟠 MajorApply the derived-table column alias list here.
This branch only rewrites
TblName, so... AS dt(c1, c2)still exposes the innerColNames.dt.c1then cannot resolve, and the duplicate-name check below runs against the wrong names.🤖 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 505 - 550, When cloning output names for an aliased table (the block using x.AsName, isTableName, p.OutputNames(), and p.SetOutputNames), also apply any derived-table column alias list so the FieldName.ColName reflects the alias list instead of the inner names; detect the alias list on the table alias node (e.g. the derived-table column alias container on x/alias node) and for each i set clonedNames[i].ColName to the corresponding alias when present (preserving Hidden, DBName handling, and other flags), then call p.SetOutputNames(clonedNames) so the duplicate-name check and resolution use the alias names.
🤖 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 771-791: The current logic only preserves FullSchema/FullNames
when leftPlan/rightPlan are directly LogicalJoin/LogicalApply, dropping them
when wrapped (e.g., by LogicalSelection). Update the checks in the join-building
block to also detect a LogicalSelection wrapper: if leftPlan (or rightPlan) is
*logicalop.LogicalSelection and its child is a *logicalop.LogicalJoin or
*logicalop.LogicalApply with non-nil FullSchema, set lFullSchema/lFullNames (or
rFullSchema/rFullNames) from the child.FullSchema/child.FullNames instead of
falling back to Schema()/OutputNames(); otherwise keep the existing fallback.
Ensure you reference leftPlan/rightPlan, logicalop.LogicalSelection,
logicalop.LogicalJoin, logicalop.LogicalApply, FullSchema and FullNames in the
updated checks.
---
Outside diff comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 4064-4089: In unfoldWildStar, the type switch on p only recognizes
*logicalop.LogicalJoin and *logicalop.LogicalApply and therefore misses wrapped
joins (e.g., a *logicalop.LogicalSelection that wraps the real join), causing
qualified `tbl.*` to lose FullSchema/FullNames; fix by unwrapping simple unary
wrapper plans before the switch (e.g., while p is a *logicalop.LogicalSelection
or other single-child wrapper, set p = p.Children()[0] or use the plan's
embedded child accessor) so that the subsequent switch in unfoldWildStar sees
the underlying *logicalop.LogicalJoin or *logicalop.LogicalApply and can obtain
FullSchema and FullNames correctly.
---
Duplicate comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 918-963: The code incorrectly assumes the entire right subtree is
a derived table: when building outputNames and setting rFullSchema/rFullNames,
detect whether rightPlan already has a join-style FullSchema/FullNames (use the
same helper as left, e.g. call findJoinFullSchema(rightPlan)) and, if present,
use that FullSchema/FullNames and preserve the existing right output
FieldName.DBName/qualification instead of clearing DBName; only clear DBName and
treat outputs as derived-table names when no FullSchema/FullNames are found for
rightPlan. Ensure ap.SetOutputNames uses the chosen name slice consistently and
that rFullSchema/rFullNames are set from the findJoinFullSchema result when
non-nil.
- Around line 505-550: When cloning output names for an aliased table (the block
using x.AsName, isTableName, p.OutputNames(), and p.SetOutputNames), also apply
any derived-table column alias list so the FieldName.ColName reflects the alias
list instead of the inner names; detect the alias list on the table alias node
(e.g. the derived-table column alias container on x/alias node) and for each i
set clonedNames[i].ColName to the corresponding alias when present (preserving
Hidden, DBName handling, and other flags), then call
p.SetOutputNames(clonedNames) so the duplicate-name check and resolution use the
alias names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99c72a7c-48db-4a86-b180-04025d949b0f
📒 Files selected for processing (2)
pkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.go
Comment on lines
+771
to
791
| // Check for LogicalJoin or LogicalApply (which embeds LogicalJoin) | ||
| // LogicalApply is used for LATERAL joins and correlated subqueries | ||
| if left, ok := leftPlan.(*logicalop.LogicalJoin); ok && left.FullSchema != nil { | ||
| lFullSchema = left.FullSchema | ||
| lFullNames = left.FullNames | ||
| } else if left, ok := leftPlan.(*logicalop.LogicalApply); ok && left.FullSchema != nil { | ||
| lFullSchema = left.FullSchema | ||
| lFullNames = left.FullNames | ||
| } else { | ||
| lFullSchema = leftPlan.Schema() | ||
| lFullNames = leftPlan.OutputNames() | ||
| } | ||
| if right, ok := rightPlan.(*logicalop.LogicalJoin); ok && right.FullSchema != nil { | ||
| rFullSchema = right.FullSchema | ||
| rFullNames = right.FullNames | ||
| } else if right, ok := rightPlan.(*logicalop.LogicalApply); ok && right.FullSchema != nil { | ||
| rFullSchema = right.FullSchema | ||
| rFullNames = right.FullNames | ||
| } else { | ||
| rFullSchema = rightPlan.Schema() | ||
| rFullNames = rightPlan.OutputNames() |
There was a problem hiding this comment.
Preserve wrapped child FullSchema here too.
A child can already be a LogicalSelection over a LogicalJoin/LogicalApply that carries FullSchema from an inner USING/NATURAL join. Falling back to Schema()/OutputNames() drops those redundant columns again, so parent joins lose the shape later name-resolution code expects.
♻️ Suggested fix
- // Check for LogicalJoin or LogicalApply (which embeds LogicalJoin)
- // LogicalApply is used for LATERAL joins and correlated subqueries
- if left, ok := leftPlan.(*logicalop.LogicalJoin); ok && left.FullSchema != nil {
- lFullSchema = left.FullSchema
- lFullNames = left.FullNames
- } else if left, ok := leftPlan.(*logicalop.LogicalApply); ok && left.FullSchema != nil {
- lFullSchema = left.FullSchema
- lFullNames = left.FullNames
- } else {
+ if fullSchema, fullNames := findJoinFullSchema(leftPlan); fullSchema != nil {
+ lFullSchema = fullSchema
+ lFullNames = fullNames
+ } else {
lFullSchema = leftPlan.Schema()
lFullNames = leftPlan.OutputNames()
}
- if right, ok := rightPlan.(*logicalop.LogicalJoin); ok && right.FullSchema != nil {
- rFullSchema = right.FullSchema
- rFullNames = right.FullNames
- } else if right, ok := rightPlan.(*logicalop.LogicalApply); ok && right.FullSchema != nil {
- rFullSchema = right.FullSchema
- rFullNames = right.FullNames
- } else {
+ if fullSchema, fullNames := findJoinFullSchema(rightPlan); fullSchema != nil {
+ rFullSchema = fullSchema
+ rFullNames = fullNames
+ } else {
rFullSchema = rightPlan.Schema()
rFullNames = rightPlan.OutputNames()
}🤖 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 771 - 791, The current
logic only preserves FullSchema/FullNames when leftPlan/rightPlan are directly
LogicalJoin/LogicalApply, dropping them when wrapped (e.g., by
LogicalSelection). Update the checks in the join-building block to also detect a
LogicalSelection wrapper: if leftPlan (or rightPlan) is
*logicalop.LogicalSelection and its child is a *logicalop.LogicalJoin or
*logicalop.LogicalApply with non-nil FullSchema, set lFullSchema/lFullNames (or
rFullSchema/rFullNames) from the child.FullSchema/child.FullNames instead of
falling back to Schema()/OutputNames(); otherwise keep the existing fallback.
Ensure you reference leftPlan/rightPlan, logicalop.LogicalSelection,
logicalop.LogicalJoin, logicalop.LogicalApply, FullSchema and FullNames in the
updated checks.
|
@terry1purcell: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Contributor
Author
|
Will rebase and create new PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #40328
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
Error Handling
Tests