planner: lateral join support | tidb-test=pr/2664#65656
Open
terry1purcell wants to merge 1 commit intopingcap:masterfrom
Open
planner: lateral join support | tidb-test=pr/2664#65656terry1purcell wants to merge 1 commit intopingcap:masterfrom
terry1purcell wants to merge 1 commit intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR adds initial support for LATERAL joins in TiDB's query planner. LATERAL allows derived tables (subqueries) to reference columns from tables that appear to their left in the FROM clause, enabling correlated references in join contexts. The PR is marked as "WIP" (Work In Progress).
Changes:
- Added LATERAL keyword parsing support in the SQL parser
- Implemented LATERAL join planning using LogicalApply nodes
- Prevented join reordering for LATERAL joins to preserve execution dependencies
- Added error handling for invalid LATERAL join usage
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test_lateral_simple.go | Basic smoke test for LATERAL parsing and restoration |
| pkg/parser/parser.y | Added LATERAL keyword and grammar rule for LATERAL derived tables |
| pkg/parser/misc.go | Registered LATERAL token in keyword map |
| pkg/parser/ast/dml.go | Added Lateral boolean field to TableSource with Restore support |
| pkg/parser/mysql/errcode.go | Added ErrInvalidLateralJoin error code (3809) |
| pkg/errno/errcode.go | Added ErrInvalidLateralJoin constant |
| pkg/errno/errname.go | Added error message for invalid LATERAL usage |
| pkg/util/dbterror/plannererrors/planner_terror.go | Registered ErrInvalidLateralJoin in planner errors |
| pkg/planner/core/logical_plan_builder.go | Implemented buildLateralJoin using LogicalApply, disabled join reorder for LATERAL |
| pkg/planner/core/rule_join_reorder.go | Prevented reordering of LogicalApply nodes used by LATERAL |
| DEPS.bzl | Dependency updates (not reviewed in detail) |
| pkg/parser/parser.go | Generated parser code (not reviewed) |
Codecov Report❌ Patch coverage is Please upload reports for the commit 163fef2 to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## master #65656 +/- ##
================================================
+ Coverage 77.6816% 78.7131% +1.0315%
================================================
Files 2015 1955 -60
Lines 551729 537397 -14332
================================================
- Hits 428592 423002 -5590
+ Misses 121406 112944 -8462
+ Partials 1731 1451 -280
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Comment on lines
+7520
to
+7521
| // getResultCTESchemaWithRecur creates the result schema for a recursive CTE by using | ||
| // the WIDER type from both seed and recursive parts. This is similar to how regular |
There was a problem hiding this comment.
The comment refers to 'WIDER type' but should clarify that 'wider' means the type that can accommodate values from both schemas without data loss, as determined by unionJoinFieldType.
Comment on lines
+7538
to
+7539
| // Special case: if seed type is NULL, use recur type directly | ||
| // This handles cases like: WITH RECURSIVE cte AS (SELECT 1, NULL UNION ALL SELECT n+1, val FROM ...) |
There was a problem hiding this comment.
Corrected spelling of 'recur' to 'recursive' for clarity in the comment.
|
|
||
| // getResultCTESchemaWithRecur creates the result schema for a recursive CTE by using | ||
| // the wider/longer type from both seed and recursive parts. This is similar to how regular | ||
| // UNION handles type inference. For example, if seed has ” (varchar(0)) and recur |
There was a problem hiding this comment.
The comment contains a typo: 'if seed has " (varchar(0))' appears to have a misplaced quotation mark. It should likely be 'if seed has "" (varchar(0))' or 'if seed has '' (varchar(0))' to properly represent an empty string literal.
Suggested change
| // UNION handles type inference. For example, if seed has ” (varchar(0)) and recur | |
| // UNION handles type inference. For example, if seed has "" (varchar(0)) and recur |
Comment on lines
+465
to
+480
| if name.Hidden { | ||
| clonedNames[i] = name | ||
| continue | ||
| } | ||
| // Clone the field name and update table name | ||
| // For derived tables (subqueries), clear DBName to avoid confusion with actual tables | ||
| clonedNames[i] = &types.FieldName{ | ||
| DBName: ast.NewCIStr(""), // Clear DBName for derived table | ||
| OrigTblName: name.OrigTblName, | ||
| OrigColName: name.OrigColName, | ||
| TblName: x.AsName, | ||
| ColName: name.ColName, | ||
| NotExplicitUsable: name.NotExplicitUsable, | ||
| Redundant: name.Redundant, | ||
| Hidden: name.Hidden, | ||
| } |
There was a problem hiding this comment.
When cloning output names, hidden names are not being cloned (line 466 uses clonedNames[i] = name), which means the original FieldName struct reference is kept. This could potentially lead to unintended mutations if these hidden fields are modified elsewhere. For consistency and to prevent potential bugs, hidden fields should also be cloned.
Suggested change
| if name.Hidden { | |
| clonedNames[i] = name | |
| continue | |
| } | |
| // Clone the field name and update table name | |
| // For derived tables (subqueries), clear DBName to avoid confusion with actual tables | |
| clonedNames[i] = &types.FieldName{ | |
| DBName: ast.NewCIStr(""), // Clear DBName for derived table | |
| OrigTblName: name.OrigTblName, | |
| OrigColName: name.OrigColName, | |
| TblName: x.AsName, | |
| ColName: name.ColName, | |
| NotExplicitUsable: name.NotExplicitUsable, | |
| Redundant: name.Redundant, | |
| Hidden: name.Hidden, | |
| } | |
| // Clone the field name by value to avoid sharing the same struct instance. | |
| clonedName := *name | |
| if !clonedName.Hidden { | |
| // For derived tables (subqueries), clear DBName to avoid confusion with actual tables | |
| clonedName.DBName = ast.NewCIStr("") // Clear DBName for derived table | |
| clonedName.TblName = x.AsName | |
| } | |
| clonedNames[i] = &clonedName |
Contributor
Author
|
@pantheon-bot please take a review |
|
Hi @terry1purcell, I noticed you mentioned me, but I couldn't find your Pantheon account linked to your GitHub account. Please connect your GitHub account to Pantheon first, and then try again. Thank you! Learn more about Pantheon AI |
Contributor
|
@pantheon-bot please take a review, conduct a comprehensive bug hunt now |
🔍 Comprehensive Bug Hunt Results (Verified)Verification Status: ✅ All findings have been adversarially verified through rigorous testing. Critical Issues (P1) - 3 Issues1.
|
terry1purcell
changed the title
Contributor
Author
|
/retest-required |
0xPoe
reviewed
Feb 23, 2026
Member
There was a problem hiding this comment.
One unexpected case:
mysql> CREATE TABLE t (a INT);
Query OK, 0 rows affected (0.03 sec)
mysql> SELECT * FROM t AS t1 JOIN ( (SELECT t1.a) AS s JOIN LATERAL (SELECT 1) AS l ON true) ON true;
ERROR 1109 (42S02): Unknown table 't1' in field listThe above case has failed on MySQL 8.4.
But in TiDB:
// TestLateralJoinScopeIsolationForNonLateralDerivedTable verifies that adding a
// LATERAL sibling does not make non-LATERAL derived tables see outer columns.
func TestLateralJoinScopeIsolationForNonLateralDerivedTable(t *testing.T) {
s := coretestsdk.CreatePlannerSuiteElems()
defer s.Close()
ctx := context.Background()
sql := "SELECT * FROM t AS t1 JOIN ((SELECT t1.a) AS s JOIN LATERAL (SELECT 1) AS l ON true) ON true"
stmt, err := s.GetParser().ParseOneStmt(sql, "", "")
require.NoError(t, err)
nodeW := resolve.NewNodeW(stmt)
_, err = BuildLogicalPlanForTest(ctx, s.GetSCtx(), nodeW, s.GetIS())
require.Error(t, err, "non-LATERAL derived table must not capture outer columns")
require.Contains(t, err.Error(), "Unknown column 't1.a' in 'field list'")
}There is no error in this implementation.
I believe this because buildJoin sets isLateral if any LATERAL exists in the right subtree. It then pushes outerSchemas to build the entire right subtree. This allows non-LATERAL derived tables in that subtree to resolve outer references incorrectly.
Member
|
Another unexpected case: MySQL: mysql> CREATE TABLE t ( a INT, b INT);
mysql> INSERT INTO t (a, b) VALUES
-> (1, 10),
-> (1, 10),
-> (2, 20),
-> (2, 20),
-> (3, 30);
mysql> SELECT * FROM t AS t1 JOIN t AS t2 USING(a) JOIN t AS t3 ON t2.b=t3.b, LATERAL (SELECT COUNT(*) AS c FROM t AS t4 WHERE t4.a=t2.a) AS dt;
+------+------+------+------+------+---+
| a | b | b | a | b | c |
+------+------+------+------+------+---+
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 1 | 10 | 10 | 1 | 10 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 2 | 20 | 20 | 2 | 20 | 2 |
| 3 | 30 | 30 | 3 | 30 | 1 |
+------+------+------+------+------+---+On TiDB: TiDB root@127.0.0.1:test> CREATE TABLE t (
-> a INT,
-> b INT
-> );
Query OK, 0 rows affected
Time: 0.024s
TiDB root@127.0.0.1:test> INSERT INTO t (a, b) VALUES
-> (1, 10),
-> (1, 10),
-> (2, 20),
-> (2, 20),
-> (3, 30);
TiDB root@127.0.0.1:test> SELECT * FROM t AS t1 JOIN t AS t2 USING(a) JOIN t AS t3 ON t2.b=t3.b, LATERAL (SE
-> LECT COUNT(*) AS c FROM t AS t4 WHERE t4.a=t2.a) AS dt;
+---+----+----+---+----+---+
| a | b | b | a | b | c |
+---+----+----+---+----+---+
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 1 | 10 | 10 | 1 | 10 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 2 | 20 | 20 | 2 | 20 | 0 |
| 3 | 30 | 30 | 3 | 30 | 0 |
+---+----+----+---+----+---+This issue arises because DecorrelateSolver only switches to FullSchema when outerPlan is directly a LogicalJoin. For LATERAL over join trees wrapped by LogicalSelection (which is common with ON), correlation on merged USING columns can be overlooked. This oversight can result in len(apply.CorCols) being equal to 0, leading to an Apply to Join rewrite. |
Contributor
Author
|
@pantheon-bot please review |
|
✅ Review completed. Found 6 P2 issues requiring attention before merge. |
Contributor
Author
|
/retest-required |
1 similar comment
Contributor
Author
|
/retest-required |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| {"KILL", true, "reserved"}, | ||
| {"LAG", true, "reserved"}, | ||
| {"LAST_VALUE", true, "reserved"}, | ||
| {"LATERAL", true, "reserved"}, |
There was a problem hiding this comment.
P2: LATERAL is now a reserved keyword (upgrade compatibility risk)
LATERAL is marked as a reserved keyword. Existing tables, columns, or identifiers named lateral (unquoted) will fail to parse after upgrade.
Impact: Breaking change for schemas using lateral as an identifier.
Workaround: Use backticks (lateral) or rename affected identifiers. Note that qualified forms like t.lateral still parse due to lexer special-casing.
Recommendation: Document this breaking change in upgrade/release notes.
0xPoe
reviewed
Mar 1, 2026
Member
There was a problem hiding this comment.
Thanks for working on this. However, it seems that the Claude Code doesn't fully understand this feature. It missed many cases. I would suggest trying it with Codex 5.3/5.2 xhigh and asking it to ensure it replicates the MySQL behavior and cases.
Some additional mismatched behaviors with MySQL:
Case 1:
CREATE TABLE IF NOT EXISTS t (
id INT PRIMARY KEY AUTO_INCREMENT,
name VARCHAR(50),
value INT
);
INSERT INTO t (name, value) VALUES
('Alice', 100),
('Bob', 200),
('Charlie', 300);
SELECT * FROM t RIGHT JOIN LATERAL (SELECT 1 AS x) AS dt ON truemysql:
mysql> SELECT * FROM t RIGHT JOIN LATERAL (SELECT 1 AS x) AS dt ON true;
+------+---------+-------+---+
| id | name | value | x |
+------+---------+-------+---+
| 1 | Alice | 100 | 1 |
| 2 | Bob | 200 | 1 |
| 3 | Charlie | 300 | 1 |
+------+---------+-------+---+
3 rows in set (0.00 sec)TiDB:
TiDB root@127.0.0.1:test> SELECT * FROM t RIGHT JOIN LATERAL (SELECT 1 AS x) AS dt ON true;
(3809, 'Invalid use of LATERAL: LATERAL cannot be used with RIGHT JOIN')Case 2:
CREATE TABLE t (a INT, b INT);
INSERT INTO t VALUES (1, 10), (2, 20), (3, 30);Mysql:
mysql> SELECT * FROM (SELECT t1.a FROM t AS t1 JOIN t AS t2 USING(a)) AS j, LATERAL (SELECT j.a) AS dt
-> ;
+------+------+
| a | a |
+------+------+
| 1 | 1 |
| 2 | 2 |
| 3 | 3 |
+------+------+
3 rows in set (0.02 sec)TiDB:
TiDB root@127.0.0.1:test> SELECT * FROM (SELECT t1.a FROM t AS t1 JOIN t AS t2 USING(a)) AS j, LATERAL (SELE
-> CT j.a) AS dt
(1054, "Unknown column 'j.a' in 'field list'")Mysql:
mysql> SELECT * FROM (SELECT t1.a FROM t AS t1 JOIN t AS t2 USING(a)) AS j, LATERAL (SELECT t1.a) AS dt;
ERROR 1054 (42S22): Unknown column 't1.a' in 'field list'TiDB:
TiDB root@127.0.0.1:test> SELECT * FROM (SELECT t1.a FROM t AS t1 JOIN t AS t2 USING(a)) AS j, LATERAL (SELE
-> CT t1.a) AS dt;
+---+---+
| a | a |
+---+---+
| 1 | 1 |
| 2 | 2 |
| 3 | 3 |
+---+---+MySQL:
mysql> SELECT * FROM (SELECT t1.a FROM t AS t1 JOIN t AS t2 USING(a)) AS j, LATERAL (SELECT t2.a) AS dt;
ERROR 1054 (42S22): Unknown column 't2.a' in 'field list'TiDB:
TiDB root@127.0.0.1:test> SELECT * FROM (SELECT t1.a FROM t AS t1 JOIN t AS t2 USING(a)) AS j, LATERAL (SELE
-> CT t2.a) AS dt;
+---+--------+
| a | a |
+---+--------+
| 1 | <null> |
| 2 | <null> |
| 3 | <null> |
+---+--------+
📝 WalkthroughWalkthroughThis PR introduces SQL LATERAL join support to TiDB by extending the parser with LATERAL keyword recognition and SubSelect grammar, modifying the AST to track LATERAL flags and column names, adding planner logic to construct LogicalApply nodes for LATERAL joins with proper correlation handling, and implementing decorrelation and join-reordering constraints for LATERAL semantics. Comprehensive test coverage is provided across parser and planner layers. Changes
Sequence DiagramsequenceDiagram
participant User as SQL User
participant Parser as Parser
participant AST as AST Builder
participant PlanBuilder as Logical Plan Builder
participant Planner as Optimizer/Decorrelator
participant Executor as Executor
User->>Parser: LATERAL SubSelect Alias Syntax
Parser->>Parser: Recognize LATERAL Token
Parser->>AST: Create TableSource(Lateral=true, ColumnNames=[...])
AST-->>Parser: TableSource with LATERAL Flag
Parser-->>User: Parsed Query
User->>PlanBuilder: Build Logical Plan
PlanBuilder->>PlanBuilder: Detect LATERAL on TableSource
PlanBuilder->>PlanBuilder: Push Outer Schemas for LATERAL
PlanBuilder->>PlanBuilder: buildLateralJoin()
PlanBuilder->>Planner: Create LogicalApply(IsLateral=true, CorCols=[...])
Planner->>Planner: Extract Correlation via FullSchema
Planner->>Planner: Prevent Join Reordering (guard LATERAL ApplyNode)
Planner->>Planner: Decorrelation (preserving IsLateral)
Planner-->>PlanBuilder: Optimized LogicalApply Plan
PlanBuilder->>Executor: Execute Correlated Join
Executor->>Executor: For Each Left Row: Execute Right Subquery
Executor-->>User: Combined Results with LATERAL Correlation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/util/hack/map_abi_go126.go (1)
321-333:⚠️ Potential issue | 🟡 MinorAdd architecture constraint comment to the Go 1.26 MockSeedForTest helper.
The Go 1.26 variant truncates the
uint64 seedparameter touintptr(seed)when storing to the map's seed field. This mirrors the Go 1.25 implementation and carries the same architecture-dependent behavior: on 32-bit targets (386, arm), truncation will occur and the test assertions at line 161 ofmap_abi_test.go(seed == m.unwrap().seed) will fail if tested. Note that an identical concern already exists in thegroup()method in the same files, documented with a TODO comment about 32-bit truncation.Per coding guidelines, add a comment explaining this non-obvious architecture constraint and narrowing:
// MockSeedForTest sets the seed of the map internals. // Note: seed is narrowed to uintptr, which is architecture-dependent (32-bit on 386/arm, 64-bit on amd64/arm64). // The deterministic seed set here will only match on architectures where uint64 and uintptr have the same width. func (m *mapData) MockSeedForTest(seed uint64) (oriSeed uint64) { if m.Used != 0 { panic("MockSeedForTest can only be called on empty map") } oriSeed = uint64(m.seed) m.seed = uintptr(seed) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hack/map_abi_go126.go` around lines 321 - 333, Add a clarifying comment to the Go 1.26 MockSeedForTest helper explaining the architecture-dependent narrowing: update the comment above func (m *mapData) MockSeedForTest(seed uint64) to state that the seed is truncated to uintptr (so on 32-bit targets like 386/arm the high 32 bits are lost) and that deterministic equivalence with uint64 seeds only holds when uint64 and uintptr have the same width; keep the existing panic and conversion behavior (m.seed = uintptr(seed)) and add the note similarly to the existing TODO in group().pkg/util/hack/map_abi.go (1)
321-333:⚠️ Potential issue | 🟡 MinorAdd architecture constraint documentation or use architecture-safe seed representation.
The
MockSeedForTestAPI acceptsuint64but immediately casts touintptrat line 332 (m.seed = uintptr(seed)). On 32-bit architectures, this truncates the seed value, causing the test assertion at line 161 (require.True(t, seed == m.unwrap().seed)) to fail. Either:
- Add an explicit
//go:build amd64 || arm64 || ppc64 || ppc64le || s390xconstraint to the affected functions and document the architecture requirement in a comment, or- Change the API and internal field to use
uintptrconsistently instead of mixinguint64parameter withuintptrstorage.Per coding guidelines, non-obvious constraints like architecture dependencies must be explained in comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hack/map_abi.go` around lines 321 - 333, The MockSeedForTest API mixes uint64 input with uintptr storage causing truncation on 32-bit platforms; update the signatures of MemAwareMap.MockSeedForTest and swissMap.MockSeedForTest to accept uintptr (and adjust any callers/tests) so the parameter type matches the internal m.seed representation, or alternatively add an explicit build constraint (//go:build amd64 || arm64 || ppc64 || ppc64le || s390x) above these functions and document the architecture requirement in a comment referencing m.seed to avoid silent truncation; ensure tests/assertions compare the same uintptr-typed value after the change.pkg/planner/core/logical_plan_builder.go (1)
767-788:⚠️ Potential issue | 🟠 MajorPropagate
FullSchemathroughLogicalSelection-wrapped children.This merge path still only sees direct
*logicalop.LogicalJoin/*logicalop.LogicalApply. Once a child withFullSchemais wrapped by the inner-joinONselection, the next join falls back toSchema()/OutputNames()and drops the redundantUSING/NATURALcolumns again. A later LATERAL on top of a deeper join tree can still miss correlations and decorrelate incorrectly.💡 Suggested fix
- 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 767 - 788, The code only checks leftPlan/rightPlan for *logicalop.LogicalJoin or *logicalop.LogicalApply and falls back to Schema()/OutputNames(), which misses cases where a LogicalJoin/LogicalApply is wrapped by a *logicalop.LogicalSelection (the ON selection); update the branch that sets lFullSchema/lFullNames and rFullSchema/rFullNames to first detect if leftPlan or rightPlan is a *logicalop.LogicalSelection and, if so, inspect its child (selection.Child or equivalent) for *logicalop.LogicalJoin or *logicalop.LogicalApply and copy FullSchema/FullNames from that child; otherwise keep the existing fallback to Schema()/OutputNames() so FullSchema/FullNames are propagated through Selection wrappers.
♻️ Duplicate comments (3)
pkg/planner/core/expression_rewriter.go (1)
2656-2666:⚠️ Potential issue | 🟠 MajorThis only fixes the local lookup path;
LogicalApplyis still missing from the outer-schema path.
findFieldNameFromNaturalUsingJoin()now handles*logicalop.LogicalApply, butPlanBuilder.getExpressionRewriter()still snapshotser.schemaander.namesfromp.Schema()/p.OutputNames()unlesspis a*logicalop.LogicalJoin(Lines 248-254).expressionRewriter.buildSubquery()then pushes that truncated view intoouterSchemas/outerNames, so correlated subqueries can still missUSING/NATURALmerged columns when the outer plan is aLogicalApply. Please mirror theFullSchema/FullNameshandling inPlanBuilder.getExpressionRewriter()as well.Suggested follow-up
func (b *PlanBuilder) getExpressionRewriter(ctx context.Context, p base.LogicalPlan) (rewriter *expressionRewriter) { defer func() { if p != nil { - if join, ok := p.(*logicalop.LogicalJoin); ok && join.FullSchema != nil { - rewriter.schema = join.FullSchema - rewriter.names = join.FullNames - } else { - rewriter.schema = p.Schema() - rewriter.names = p.OutputNames() - } + switch x := p.(type) { + case *logicalop.LogicalJoin: + if x.FullSchema != nil { + rewriter.schema = x.FullSchema + rewriter.names = x.FullNames + return + } + case *logicalop.LogicalApply: + if x.FullSchema != nil { + rewriter.schema = x.FullSchema + rewriter.names = x.FullNames + return + } + } + rewriter.schema = p.Schema() + rewriter.names = p.OutputNames() } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/expression_rewriter.go` around lines 2656 - 2666, PlanBuilder.getExpressionRewriter() currently only uses p.FullSchema/FullNames for *logicalop.LogicalJoin, causing LogicalApply to lose USING/NATURAL merged columns for outer-schema snapshots; update getExpressionRewriter to mirror the LogicalJoin branch and, when p is a *logicalop.LogicalApply with non-nil FullSchema/FullNames, set er.schema and er.names from p.FullSchema/p.FullNames instead of p.Schema()/p.OutputNames(), so expressionRewriter.buildSubquery() will push the correct outerSchemas/outerNames for correlated subqueries.pkg/planner/core/logical_plan_builder.go (2)
3909-3933:⚠️ Potential issue | 🟠 MajorUse the wrapper-aware helper here too.
This still only inspects direct
Join/Applynodes. If an inner-joinONhas wrapped them inLogicalSelection, qualified wildcard expansion still misses the hiddenFullSchemaand can drop redundantUSING/NATURALcolumns.💡 Suggested fix
func (*PlanBuilder) unfoldWildStar(p base.LogicalPlan, selectFields []*ast.SelectField) (resultList []*ast.SelectField, err error) { - // 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) resultList = make([]*ast.SelectField, 0, max(2, len(selectFields)))🤖 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 3909 - 3933, unfoldWildStar currently only inspects p's direct type (LogicalJoin/LogicalApply) so it misses wrapped joins (e.g., a LogicalSelection over a LogicalJoin) and thus can drop USING/NATURAL columns; update unfoldWildStar to unwrap wrapper nodes before extracting FullSchema/FullNames by using the existing wrapper-aware helper (i.e., locate or add a helper that traverses wrappers like LogicalSelection to find the underlying LogicalJoin/LogicalApply) and use that helper to set fullSchema and fullNames instead of the direct type switch on p.
926-973:⚠️ Potential issue | 🟠 MajorDon't flatten metadata for the entire right subtree.
joinNode.Rightcan be a parenthesized join tree, not just a single derived table. BlankingDBNamefor every right-side output and assuming the subtree never carries its ownFullSchemadrops real-table metadata from cases like(LATERAL (...) AS dt JOIN t2 USING(a)), so schema-qualified references andt2.*/ redundantUSINGcolumns can still break.🤖 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 926 - 973, The code currently blanks DBName for every right-side output unconditionally, which strips real-table metadata when the right subtree is itself a join with a FullSchema; change the logic so DBName is cleared only when the immediate right side is a derived table without its own FullSchema. Use findJoinFullSchema(rightPlan) (similar to how lFullSchema is computed) to detect whether the right subtree has a FullSchema; if findJoinFullSchema(rightPlan) returns a non-nil fullSchema, copy rightPlan.OutputNames preserving DBName, otherwise clear DBName only for those outputs of the derived-table rightPlan before calling ap.SetOutputNames and when computing rFullSchema/rFullNames.
🧹 Nitpick comments (5)
br/pkg/stream/stream_mgr.go (2)
397-400: Consider adding a comment explaining why ParseUint errors are safe to ignore.The errors from
strconv.ParseUintare silently discarded. While the regex pre-validates that each group is exactly 16 hex digits (which always fits in uint64), discarding errors without explanation violates the guideline to avoid silently swallowing errors. A brief comment would clarify the invariant.📝 Suggested comment addition
+ // The regex guarantees exactly 16 hex digits per group, which always parses + // successfully into uint64. Errors are safely ignored. flushTs, _ := strconv.ParseUint(matches[1], 16, 64) minDefaultTs, _ := strconv.ParseUint(matches[2], 16, 64) minTs, _ := strconv.ParseUint(matches[3], 16, 64) maxTs, _ := strconv.ParseUint(matches[4], 16, 64)As per coding guidelines: "Keep error handling actionable and contextual; avoid silently swallowing errors in Go code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/stream/stream_mgr.go` around lines 397 - 400, Add a brief comment above the strconv.ParseUint calls (the lines assigning flushTs, minDefaultTs, minTs, maxTs) explaining that ParseUint errors are intentionally ignored because the regex that produced matches guarantees each capture is exactly 16 hex digits which always fits within a uint64, so parsing cannot fail; reference strconv.ParseUint and the matches[] groups in the comment to make the invariant explicit for future readers and satisfy the "don't silently swallow errors" guideline.
391-395: Defensive check is redundant but acceptable.After
metaPattern.MatchString(filename)returns true,FindStringSubmatchis guaranteed to return exactly 5 elements (full match + 4 groups). Thelen(matches) < 5check will never trigger. This is harmless defensive code, but consider removing it or adding a comment noting it's a safeguard against future regex changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/stream/stream_mgr.go` around lines 391 - 395, The len(matches) < 5 defensive check after calling metaPattern.MatchString(filename) is redundant because FindStringSubmatch will return exactly 5 elements when the regex matched; update the code around metaPattern.MatchString(filename) / FindStringSubmatch (variable matches) by removing the if len(matches) < 5 block and its log, or if you prefer to keep a safeguard, replace it with a short comment explaining this is purely defensive against future regex changes (e.g., "len check retained only as a safeguard if regex groups change") so reviewers understand the intent.br/pkg/stream/stream_misc_test.go (1)
148-155: Minor: test case overlaps with the one at lines 130-137.Both test cases verify the same code path (restoreTS < fallback minTs after fallback from
minDefaultTs > minTs), differing only in restoreTS value (8 vs 9). Consider consolidating or renaming to clarify the distinct boundary being tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/stream/stream_misc_test.go` around lines 148 - 155, The two table-driven tests using the case name "invalid: minDefaultTs > minTs, fallback, but restoreTS < fallback" are redundant because they exercise the same code path (restoreTS < fallback) with only a one-unit difference in restoreTS; update the test table to either remove one of the duplicate cases or rename and adjust one case to target a distinct boundary (e.g., explicitly test restoreTS == fallback and restoreTS < fallback by setting restoreTS to 10 and 9 respectively), modifying the entry in the test slice where the name and args (shiftStartTS/restoreTS) are defined so the table clearly covers both the strict-less and equal boundary scenarios for the logic handling minDefaultTs > minTs fallback.pkg/parser/ast/dml.go (1)
539-541: Tighten the MySQL version in this compatibility note.The PR target is MySQL 8.0.14+ behavior, so
MySQL 8.0+slightly overstates the compatibility window.8.0.14+(or dropping the version entirely) would be more precise.🤖 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 539 - 541, The compatibility note on the Lateral comment overstates the MySQL range; update the comment near the Lateral field in pkg/parser/ast/dml.go to reference MySQL 8.0.14+ (or remove the version entirely) instead of "MySQL 8.0+" so it accurately reflects the PR target behavior for the LATERAL derived table syntax used by the Lateral flag.pkg/planner/core/lateral_join_test.go (1)
58-62: Consider clarifyingexpectedErrorCodeusage.The
expectedErrorCode: 1value on line 61 appears arbitrary. The test checks forErrInvalidLateralJoin.Equal(err)which doesn't use this numeric code. Either use the actual MySQL error code (3809) for documentation purposes, or remove the field if unused.💡 Suggested clarification
{ name: "LATERAL with RIGHT JOIN and outer ref returns error", sql: "SELECT * FROM t RIGHT JOIN LATERAL (SELECT t.a) AS dt ON true", expectError: true, - expectedErrorCode: 1, + expectedErrorCode: 3809, // ErrInvalidLateralJoin MySQL error code },🤖 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 58 - 62, The test case in lateral_join_test.go sets expectedErrorCode: 1 but the test asserts ErrInvalidLateralJoin.Equal(err) (so the numeric field is misleading/unused); update the test case (the table entry with name "LATERAL with RIGHT JOIN and outer ref returns error") to either remove the expectedErrorCode field entirely if it isn't referenced elsewhere, or replace 1 with the MySQL/MariaDB error code 3809 to document the intended error; ensure this change is applied to the same test struct so other test logic that checks expectedErrorCode (if any) remains consistent with ErrInvalidLateralJoin.
🤖 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/parser.y`:
- Around line 10408-10414: The grammar currently accepts "LATERAL" SubSelect
TableAsNameOpt which still allows aliasless lateral derived tables and rejects
valid MySQL forms; change this by adding a dedicated production (e.g. a new
LateralTableSubquery or an alternative branch of TableFactor) that matches
"LATERAL" SubSelect ASopt Alias IdentListWithParenOpt where AS is optional but
Alias is required and IdentListWithParenOpt handles the optional column-list;
replace the use of TableAsNameOpt in the lateral branch with this new production
and reuse SubSelect and IdentListWithParenOpt to ensure forms like `LATERAL
(...) AS dt(c1, c2)` are accepted.
In `@pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json`:
- Around line 25-27: Restore explicit EXPLAIN format pinning to 'brief' for the
affected casetest inputs so output stays deterministic: update the EXPLAIN
statements (e.g., the entries containing "explain select /*+
READ_FROM_STORAGE(TIFLASH[s]) */ a from s ...", "explain select /*+
read_from_storage(tiflash[t3]) */ * from t3 ...", and other listed blocks at
36-39, 56, 105-117, 124-136, 142-162, 169-178) to include format='brief' (or
otherwise add a surrounding instruction that forces format='brief') so these
cases no longer depend on the default EXPLAIN output and avoid golden churn.
In `@pkg/planner/core/casetest/rule/testdata/outer2inner_xut.json`:
- Around line 629-644: The current change updated the golden for
TestOuter2InnerIssue55886 but did not add a reduced regression reproducing the
selection-wrapped LATERAL case that can drop CorCols and rewrite Apply->Join;
add a new test (in the same outer2inner_xut.json suite) that reproduces the
reported LATERAL + join/ON pattern: create a query using LATERAL (or correlated
subquery) where a LogicalSelection on t2 pushes Column#35/Column#36 into a
cop[tikv] Selection, and assert the plan still contains an Apply/Correlated
shape (not a converted Join) and the Selection on t2 (verify presence of
Selection and the correlated Column IDs), naming the test clearly (e.g.,
TestOuter2InnerIssue55886_LateralSelection) so CI will catch regressions
alongside the golden update.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 909-918: The LogicalApply path for LATERAL misses marking CTEs in
the right subtree as apply-scoped; after constructing ap (LogicalApply) and
before attaching rightPlan, call setIsInApplyForCTE(rightPlan, ap.Schema()) so
any CTEs inside the lateral subquery get the apply-scoped marker, then continue
with ap.SetChildren(leftPlan, rightPlan) and SetSchema; reference the
LogicalApply instance named ap, the rightPlan variable, and the helper
setIsInApplyForCTE to locate where to insert the call.
- Around line 973-979: ap.FullSchema keeps NOT NULL on the left side for
RightOuterJoin, causing inconsistency with ap.Schema(); update the
join-nullability handling so ap.FullSchema mirrors ap.Schema(): when joinType ==
base.RightOuterJoin call util.ResetNotNullFlag on ap.FullSchema for the
left-side range (start 0, length lFullSchema.Len()) to clear NOT NULL flags;
retain the existing LeftOuterJoin branch that clears the right side.
---
Outside diff comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 767-788: The code only checks leftPlan/rightPlan for
*logicalop.LogicalJoin or *logicalop.LogicalApply and falls back to
Schema()/OutputNames(), which misses cases where a LogicalJoin/LogicalApply is
wrapped by a *logicalop.LogicalSelection (the ON selection); update the branch
that sets lFullSchema/lFullNames and rFullSchema/rFullNames to first detect if
leftPlan or rightPlan is a *logicalop.LogicalSelection and, if so, inspect its
child (selection.Child or equivalent) for *logicalop.LogicalJoin or
*logicalop.LogicalApply and copy FullSchema/FullNames from that child; otherwise
keep the existing fallback to Schema()/OutputNames() so FullSchema/FullNames are
propagated through Selection wrappers.
In `@pkg/util/hack/map_abi_go126.go`:
- Around line 321-333: Add a clarifying comment to the Go 1.26 MockSeedForTest
helper explaining the architecture-dependent narrowing: update the comment above
func (m *mapData) MockSeedForTest(seed uint64) to state that the seed is
truncated to uintptr (so on 32-bit targets like 386/arm the high 32 bits are
lost) and that deterministic equivalence with uint64 seeds only holds when
uint64 and uintptr have the same width; keep the existing panic and conversion
behavior (m.seed = uintptr(seed)) and add the note similarly to the existing
TODO in group().
In `@pkg/util/hack/map_abi.go`:
- Around line 321-333: The MockSeedForTest API mixes uint64 input with uintptr
storage causing truncation on 32-bit platforms; update the signatures of
MemAwareMap.MockSeedForTest and swissMap.MockSeedForTest to accept uintptr (and
adjust any callers/tests) so the parameter type matches the internal m.seed
representation, or alternatively add an explicit build constraint (//go:build
amd64 || arm64 || ppc64 || ppc64le || s390x) above these functions and document
the architecture requirement in a comment referencing m.seed to avoid silent
truncation; ensure tests/assertions compare the same uintptr-typed value after
the change.
---
Duplicate comments:
In `@pkg/planner/core/expression_rewriter.go`:
- Around line 2656-2666: PlanBuilder.getExpressionRewriter() currently only uses
p.FullSchema/FullNames for *logicalop.LogicalJoin, causing LogicalApply to lose
USING/NATURAL merged columns for outer-schema snapshots; update
getExpressionRewriter to mirror the LogicalJoin branch and, when p is a
*logicalop.LogicalApply with non-nil FullSchema/FullNames, set er.schema and
er.names from p.FullSchema/p.FullNames instead of p.Schema()/p.OutputNames(), so
expressionRewriter.buildSubquery() will push the correct outerSchemas/outerNames
for correlated subqueries.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 3909-3933: unfoldWildStar currently only inspects p's direct type
(LogicalJoin/LogicalApply) so it misses wrapped joins (e.g., a LogicalSelection
over a LogicalJoin) and thus can drop USING/NATURAL columns; update
unfoldWildStar to unwrap wrapper nodes before extracting FullSchema/FullNames by
using the existing wrapper-aware helper (i.e., locate or add a helper that
traverses wrappers like LogicalSelection to find the underlying
LogicalJoin/LogicalApply) and use that helper to set fullSchema and fullNames
instead of the direct type switch on p.
- Around line 926-973: The code currently blanks DBName for every right-side
output unconditionally, which strips real-table metadata when the right subtree
is itself a join with a FullSchema; change the logic so DBName is cleared only
when the immediate right side is a derived table without its own FullSchema. Use
findJoinFullSchema(rightPlan) (similar to how lFullSchema is computed) to detect
whether the right subtree has a FullSchema; if findJoinFullSchema(rightPlan)
returns a non-nil fullSchema, copy rightPlan.OutputNames preserving DBName,
otherwise clear DBName only for those outputs of the derived-table rightPlan
before calling ap.SetOutputNames and when computing rFullSchema/rFullNames.
---
Nitpick comments:
In `@br/pkg/stream/stream_mgr.go`:
- Around line 397-400: Add a brief comment above the strconv.ParseUint calls
(the lines assigning flushTs, minDefaultTs, minTs, maxTs) explaining that
ParseUint errors are intentionally ignored because the regex that produced
matches guarantees each capture is exactly 16 hex digits which always fits
within a uint64, so parsing cannot fail; reference strconv.ParseUint and the
matches[] groups in the comment to make the invariant explicit for future
readers and satisfy the "don't silently swallow errors" guideline.
- Around line 391-395: The len(matches) < 5 defensive check after calling
metaPattern.MatchString(filename) is redundant because FindStringSubmatch will
return exactly 5 elements when the regex matched; update the code around
metaPattern.MatchString(filename) / FindStringSubmatch (variable matches) by
removing the if len(matches) < 5 block and its log, or if you prefer to keep a
safeguard, replace it with a short comment explaining this is purely defensive
against future regex changes (e.g., "len check retained only as a safeguard if
regex groups change") so reviewers understand the intent.
In `@br/pkg/stream/stream_misc_test.go`:
- Around line 148-155: The two table-driven tests using the case name "invalid:
minDefaultTs > minTs, fallback, but restoreTS < fallback" are redundant because
they exercise the same code path (restoreTS < fallback) with only a one-unit
difference in restoreTS; update the test table to either remove one of the
duplicate cases or rename and adjust one case to target a distinct boundary
(e.g., explicitly test restoreTS == fallback and restoreTS < fallback by setting
restoreTS to 10 and 9 respectively), modifying the entry in the test slice where
the name and args (shiftStartTS/restoreTS) are defined so the table clearly
covers both the strict-less and equal boundary scenarios for the logic handling
minDefaultTs > minTs fallback.
In `@pkg/parser/ast/dml.go`:
- Around line 539-541: The compatibility note on the Lateral comment overstates
the MySQL range; update the comment near the Lateral field in
pkg/parser/ast/dml.go to reference MySQL 8.0.14+ (or remove the version
entirely) instead of "MySQL 8.0+" so it accurately reflects the PR target
behavior for the LATERAL derived table syntax used by the Lateral flag.
In `@pkg/planner/core/lateral_join_test.go`:
- Around line 58-62: The test case in lateral_join_test.go sets
expectedErrorCode: 1 but the test asserts ErrInvalidLateralJoin.Equal(err) (so
the numeric field is misleading/unused); update the test case (the table entry
with name "LATERAL with RIGHT JOIN and outer ref returns error") to either
remove the expectedErrorCode field entirely if it isn't referenced elsewhere, or
replace 1 with the MySQL/MariaDB error code 3809 to document the intended error;
ensure this change is applied to the same test struct so other test logic that
checks expectedErrorCode (if any) remains consistent with ErrInvalidLateralJoin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 656befea-8cde-4d4b-8253-d0b64877b6fa
⛔ Files ignored due to path filters (1)
lightning/web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (55)
.github/copilot-instructions.mdbr/pkg/stream/BUILD.bazelbr/pkg/stream/backupmetas/BUILD.bazelbr/pkg/stream/backupmetas/parser.gobr/pkg/stream/stream_mgr.gobr/pkg/stream/stream_mgr_fuzz_test.gobr/pkg/stream/stream_misc_test.goerrors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/join/concurrent_map.gopkg/executor/join/concurrent_map_test.gopkg/executor/set.gopkg/executor/set_test.gopkg/infoschema/test/clustertablestest/cluster_tables_test.gopkg/parser/BUILD.bazelpkg/parser/ast/dml.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/lateral_test.gopkg/parser/misc.gopkg/parser/mysql/errcode.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/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/rule_decorrelate.gopkg/planner/core/rule_join_reorder.gopkg/store/mockstore/unistore/cophandler/BUILD.bazelpkg/store/mockstore/unistore/cophandler/cop_handler_test.gopkg/store/mockstore/unistore/cophandler/mpp_exec.gopkg/util/dbterror/plannererrors/planner_terror.gopkg/util/hack/map_abi.gopkg/util/hack/map_abi_go126.gopkg/util/hack/map_abi_test.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
💤 Files with no reviewable changes (7)
- pkg/executor/join/concurrent_map.go
- pkg/executor/set_test.go
- br/pkg/stream/BUILD.bazel
- br/pkg/stream/backupmetas/parser.go
- pkg/store/mockstore/unistore/cophandler/cop_handler_test.go
- br/pkg/stream/backupmetas/BUILD.bazel
- br/pkg/stream/stream_mgr_fuzz_test.go
pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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/rule_decorrelate.go (1)
414-414:⚠️ Potential issue | 🟡 MinorApply
outerSchemaconsistently to correlated column extraction in aggregation optimization paths.Lines 414 and 535 re-extract
apply.CorColsbut useapply.Children()[0].Schema()directly, missing the fix introduced at lines 241–244 that usesfindJoinFullSchema()to capture merged USING/NATURAL join columns. This inconsistency could cause incorrect zero-correlated-column detection and Apply→Join conversion when a LATERAL subquery with aggregation has a USING/NATURAL join (wrapped by Selection) as its outer plan.Proposed fix for consistency
- apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], apply.Children()[0].Schema()) + apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], outerSchema)Apply the same change at line 535:
- apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], apply.Children()[0].Schema()) + apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], outerSchema)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/rule_decorrelate.go` at line 414, The code re-extracts apply.CorCols using apply.Children()[0].Schema() directly (via coreusage.ExtractCorColumnsBySchema4LogicalPlan) but should use the merged outer schema produced by findJoinFullSchema() to account for USING/NATURAL joins; update the extraction sites that set apply.CorCols (the one currently calling coreusage.ExtractCorColumnsBySchema4LogicalPlan with apply.Children()[0].Schema()) to compute outerSchema := findJoinFullSchema(apply.Children()[0]) (or reuse the existing outerSchema variable if present) and pass that outerSchema to coreusage.ExtractCorColumnsBySchema4LogicalPlan so correlated-column detection is consistent for Apply→Join conversion.
♻️ Duplicate comments (2)
pkg/planner/core/logical_plan_builder.go (2)
916-918:⚠️ Potential issue | 🔴 CriticalMark LATERAL right-subtree CTEs as apply-scoped.
This
LogicalApplyconstruction path missessetIsInApplyForCTE(rightPlan, ap.Schema()). Correlated CTEs under LATERAL can reuse storage across outer-row iterations and return wrong results.Suggested fix
ap.SetChildren(leftPlan, rightPlan) ap.SetSchema(expression.MergeSchema(leftPlan.Schema(), rightPlan.Schema())) + setIsInApplyForCTE(rightPlan, ap.Schema())🤖 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 916 - 918, The LogicalApply construction is missing a call to mark CTEs in the right subtree as apply-scoped; after ap.SetChildren(leftPlan, rightPlan) and ap.SetSchema(expression.MergeSchema(leftPlan.Schema(), rightPlan.Schema())), call setIsInApplyForCTE(rightPlan, ap.Schema()) so correlated CTEs under LATERAL are flagged as apply-scoped and won’t erroneously reuse storage across outer-row iterations; locate this in the LogicalApply build path where ap, leftPlan, and rightPlan are assembled and add the setIsInApplyForCTE invocation using those identifiers.
973-979:⚠️ Potential issue | 🟠 MajorKeep
ap.FullSchemanullability consistent withap.Schema()for RIGHT JOIN LATERAL.
ap.Schema()clears left-side NOT NULL forRightOuterJoinat Line [922]-Line [924], butap.FullSchemaonly handlesLeftOuterJoinat Line [977]-Line [979]. This leaves inconsistent type nullability between the two schemas.Suggested fix
- // Clear NotNull flag for the inner side (right side) of FullSchema if it's a LEFT JOIN - // (consistent with buildJoin behavior at line 681-682) - if joinType == base.LeftOuterJoin { - util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()) - } + switch joinType { + case base.LeftOuterJoin: + util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()) + case base.RightOuterJoin: + util.ResetNotNullFlag(ap.FullSchema, 0, lFullSchema.Len()) + }🤖 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 973 - 979, ap.FullSchema's nullability isn’t updated for RightOuterJoin while ap.Schema() clears left-side NOT NULL for RightOuterJoin; add the same adjustment to ap.FullSchema by calling util.ResetNotNullFlag when joinType == base.RightOuterJoin to clear the left-side columns (start index 0, length lFullSchema.Len()), mirroring the existing LeftOuterJoin handling that clears the right-side with util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/planner/core/rule_decorrelate.go`:
- Line 414: The code re-extracts apply.CorCols using
apply.Children()[0].Schema() directly (via
coreusage.ExtractCorColumnsBySchema4LogicalPlan) but should use the merged outer
schema produced by findJoinFullSchema() to account for USING/NATURAL joins;
update the extraction sites that set apply.CorCols (the one currently calling
coreusage.ExtractCorColumnsBySchema4LogicalPlan with
apply.Children()[0].Schema()) to compute outerSchema :=
findJoinFullSchema(apply.Children()[0]) (or reuse the existing outerSchema
variable if present) and pass that outerSchema to
coreusage.ExtractCorColumnsBySchema4LogicalPlan so correlated-column detection
is consistent for Apply→Join conversion.
---
Duplicate comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 916-918: The LogicalApply construction is missing a call to mark
CTEs in the right subtree as apply-scoped; after ap.SetChildren(leftPlan,
rightPlan) and ap.SetSchema(expression.MergeSchema(leftPlan.Schema(),
rightPlan.Schema())), call setIsInApplyForCTE(rightPlan, ap.Schema()) so
correlated CTEs under LATERAL are flagged as apply-scoped and won’t erroneously
reuse storage across outer-row iterations; locate this in the LogicalApply build
path where ap, leftPlan, and rightPlan are assembled and add the
setIsInApplyForCTE invocation using those identifiers.
- Around line 973-979: ap.FullSchema's nullability isn’t updated for
RightOuterJoin while ap.Schema() clears left-side NOT NULL for RightOuterJoin;
add the same adjustment to ap.FullSchema by calling util.ResetNotNullFlag when
joinType == base.RightOuterJoin to clear the left-side columns (start index 0,
length lFullSchema.Len()), mirroring the existing LeftOuterJoin handling that
clears the right-side with util.ResetNotNullFlag(ap.FullSchema,
lFullSchema.Len(), ap.FullSchema.Len()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d600d72-856d-438e-b2be-c55c2f7c7d60
📒 Files selected for processing (9)
pkg/planner/core/BUILD.bazelpkg/planner/core/casetest/rule/testdata/outer2inner_out.jsonpkg/planner/core/casetest/rule/testdata/outer2inner_xut.jsonpkg/planner/core/expression_rewriter.gopkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/rule_decorrelate.gopkg/planner/core/rule_join_reorder.gotests/integrationtest/r/cte.result
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/planner/core/expression_rewriter.go
- pkg/planner/core/rule_join_reorder.go
- pkg/planner/core/BUILD.bazel
- tests/integrationtest/r/cte.result
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/store/mockstore/unistore/cophandler/mpp_exec.go (1)
951-962:⚠️ Potential issue | 🔴 CriticalCritical: Unconditional receive on
connectedChcan deadlock if tunnel connection is never established.The defer block unconditionally waits on
<-tunnel.connectedChfor every tunnel. According to the codebase:
connectedChis an unbuffered channel created during tunnel setup (mpp.go:424)- It is only closed in
getAndActiveTunnel(mpp.go:753) when the receiver successfully establishes the connection- If the receiver never calls
getAndActiveTunnelsuccessfully (due to network failure, peer crash, timeout, or task cancellation),connectedChremains open and unwrittenThis causes the sender's defer block to block indefinitely on line 954, leading to:
- Goroutine leak (sender goroutine never terminates)
- Resource leak (channels never closed, child executors never stopped)
- Potential cascade failures if other components wait on this sender
🐛 Suggested fix using select with timeout
func (e *exchSenderExec) next() (*chunk.Chunk, error) { defer func() { for _, tunnel := range e.tunnels { - <-tunnel.connectedCh + select { + case <-tunnel.connectedCh: + // Connection was established, proceed with cleanup + case <-time.After(30 * time.Second): + // Connection never established, log and proceed with cleanup anyway + logutil.BgLogger().Warn("tunnel connection not established within timeout", + zap.Int64("targetTask", tunnel.targetTask.TaskId)) + } close(tunnel.ErrCh) close(tunnel.DataCh) }Alternatively, if
mppCtxprovides a cancellation context, use that instead of a hardcoded timeout.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/store/mockstore/unistore/cophandler/mpp_exec.go` around lines 951 - 962, The defer in exchSenderExec.next currently does an unconditional receive on each tunnel.connectedCh which can deadlock if getAndActiveTunnel never closes connectedCh; change the cleanup to use a non-blocking select or a select that also listens on the executor/mpp context cancellation (or a timeout) so it won't block forever: in the deferred closure iterate tunnels and for each do select { case <-tunnel.connectedCh: /* proceed to close ErrCh/DataCh */ case <-mppCtx.Done(): /* proceed to close ErrCh/DataCh */ case <-time.After(timeout): /* fallback close */ } (or use ctx instead of time.After), then close ErrCh and DataCh and call e.stop() but handle its error without panic (return/log it or propagate), ensuring getAndActiveTunnel is still referenced as the creator of connectedCh for context.
♻️ Duplicate comments (2)
pkg/planner/core/logical_plan_builder.go (2)
909-918:⚠️ Potential issue | 🟠 MajorMark CTEs in the LATERAL right subtree as apply-scoped.
buildLateralJoincreatesLogicalApplybut skipssetIsInApplyForCTE(rightPlan, ap.Schema()). Correlated CTEs under the LATERAL side can be reused across outer-row iterations incorrectly.Suggested fix
ap.SetChildren(leftPlan, rightPlan) ap.SetSchema(expression.MergeSchema(leftPlan.Schema(), rightPlan.Schema())) + setIsInApplyForCTE(rightPlan, ap.Schema())🤖 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 909 - 918, The LATERAL LogicalApply built in buildLateralJoin is missing marking CTEs in the right subtree as apply-scoped; after creating ap := logicalop.LogicalApply{...}.Init(...) and before or after SetChildren, call setIsInApplyForCTE(rightPlan, ap.Schema()) so correlated CTEs under the LATERAL rightPlan are marked with the apply scope (use the existing setIsInApplyForCTE function and ap.Schema() to locate the schema).
973-979:⚠️ Potential issue | 🟠 MajorKeep
ap.FullSchemanullability consistent forRightOuterJoin.
ap.Schema()already clears left-sideNOT NULLfor right outer join (Line 922-924), butap.FullSchemaonly handles left outer join. This inconsistency can break consumers that resolve/types-check viaFullSchema.Suggested fix
- // Clear NotNull flag for the inner side (right side) of FullSchema if it's a LEFT JOIN - // (consistent with buildJoin behavior at line 681-682) - if joinType == base.LeftOuterJoin { - util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()) - } + switch joinType { + case base.LeftOuterJoin: + util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()) + case base.RightOuterJoin: + util.ResetNotNullFlag(ap.FullSchema, 0, lFullSchema.Len()) + }🤖 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 973 - 979, ap.FullSchema's nullability is not adjusted for RightOuterJoin, causing inconsistency with ap.Schema(); after merging schemas (ap.FullSchema = expression.MergeSchema(...)), add a branch for joinType == base.RightOuterJoin and call util.ResetNotNullFlag(ap.FullSchema, 0, lFullSchema.Len()) to clear NOT NULL on the left-side columns (mirroring the existing behavior in ap.Schema()); keep the existing LeftOuterJoin branch unchanged.
🧹 Nitpick comments (3)
tests/integrationtest/r/planner/cascades/integration.result (1)
999-1000: Consider resetting all modified session variables.The test modifies several session variables (
sql_mode,tidb_hashagg_partial_concurrency,tidb_hashagg_final_concurrency,tidb_executor_concurrency,tidb_hash_join_concurrency,tidb_distsql_scan_concurrency,tidb_opt_distinct_agg_push_down) but only resetstidb_opt_fix_controlandtidb_enable_cascades_plannerat the end.While integration tests typically run in isolated sessions, explicitly resetting all modified variables would improve test hygiene and prevent potential cross-test interference if the test framework behavior changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integrationtest/r/planner/cascades/integration.result` around lines 999 - 1000, The test modifies several session variables but only resets tidb_opt_fix_control and tidb_enable_cascades_planner; update the test teardown to explicitly reset all modified variables (sql_mode, tidb_hashagg_partial_concurrency, tidb_hashagg_final_concurrency, tidb_executor_concurrency, tidb_hash_join_concurrency, tidb_distsql_scan_concurrency, tidb_opt_distinct_agg_push_down as well as the already-present tidb_opt_fix_control and tidb_enable_cascades_planner) back to their defaults (e.g., using "SET SESSION <var> = DEFAULT" or "SET @@<var> = DEFAULT") so the session is fully restored after the test.tests/integrationtest/r/planner/core/lateral_join.result (1)
6-6:LIMIT 1withoutORDER BYmakes these assertions nondeterministic.These cases can return different rows depending on access path/runtime order, which risks flaky integration results. Add a stable
ORDER BYinside each lateral subquery beforeLIMIT 1.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, The lateral subquery uses "LIMIT 1" without a deterministic ordering, causing flaky results; update the lateral subquery "(select b from t2 where t2.a = t1.a limit 1) as lat" (and the other occurrences noted) to include a stable ORDER BY before LIMIT 1 (for example "ORDER BY <stable_column> ASC") so the returned row is deterministic across runs.br/pkg/stream/stream_mgr.go (1)
397-400: Silent error handling on hex parsing could mask data corruption.The
strconv.ParseUinterrors are discarded with_. If a filename matches the regex pattern but contains invalid hex values (e.g., due to file corruption or unexpected encoding), parsing will silently fail and default to0. This could cause:
flushTs = 0(harmless for current logic)minDefaultTs = 0triggers the fallback tominTsminTs = 0ormaxTs = 0causes incorrect filtering (files may be incorrectly included or excluded)Since the regex already validates hex characters
[0-9a-fA-F]{16}, parsing should not fail in practice. However, for defensive coding, consider logging a warning on parse failure or verifying the regex guarantees sufficient validation.🔧 Suggested defensive handling
- flushTs, _ := strconv.ParseUint(matches[1], 16, 64) - minDefaultTs, _ := strconv.ParseUint(matches[2], 16, 64) - minTs, _ := strconv.ParseUint(matches[3], 16, 64) - maxTs, _ := strconv.ParseUint(matches[4], 16, 64) + flushTs, err1 := strconv.ParseUint(matches[1], 16, 64) + minDefaultTs, err2 := strconv.ParseUint(matches[2], 16, 64) + minTs, err3 := strconv.ParseUint(matches[3], 16, 64) + maxTs, err4 := strconv.ParseUint(matches[4], 16, 64) + if err1 != nil || err2 != nil || err3 != nil || err4 != nil { + log.Warn("failed to parse hex values in meta file name", + zap.String("file", path), + zap.NamedError("flushTs", err1), + zap.NamedError("minDefaultTs", err2), + zap.NamedError("minTs", err3), + zap.NamedError("maxTs", err4), + ) + return path + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@br/pkg/stream/stream_mgr.go` around lines 397 - 400, The hex parsing using strconv.ParseUint for flushTs, minDefaultTs, minTs and maxTs currently discards errors; update the parsing to check each returned error and handle it defensively (e.g., log a warning including the filename or regex match and the parse error, and skip/ignore this file or return an error) instead of silently treating values as 0; locate the parsing calls that assign flushTs, minDefaultTs, minTs, maxTs and add error checks that either fall back safely or filter out the corrupted filename so downstream filtering logic cannot act on invalid zero timestamps.
🤖 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/errno/errcode.go`:
- Line 922: Remove the duplicate ErrInvalidLateralJoin constant from the legacy
parser error-code file and keep the single definition in the central errno list;
specifically, find the ErrInvalidLateralJoin symbol in the legacy parser/mysql
errcode file and delete its definition so only the ErrInvalidLateralJoin in the
pkg/errno centralized errcode remains. Ensure no other references or duplicate
constants are left in that legacy errcode file and run a quick build to verify
no missing symbols.
In `@pkg/parser/lateral_test.go`:
- Around line 155-171: Add an explicit negative assertion for cases where
tc.checkLateral is false: after extracting selectStmt and ensuring
selectStmt.From is not nil, call
findLateralTableSource(selectStmt.From.TableRefs) and assert it is nil (e.g.,
require.Nil) to catch accidental TableSource.Lateral=true regressions; keep the
existing positive checks inside the if tc.checkLateral block and add the
negative require.Nil check in the else branch referencing
findLateralTableSource, selectStmt.From.TableRefs and the TableSource.Lateral
behavior.
In `@pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.json`:
- Line 178: The MPP golden update did not include a targeted regression test for
the LATERAL decorrelation (selection-wrapped join tree / merged USING-column
correlation) that can cause Apply->Join to produce wrong results; add a focused
planner casetest that reproduces the reported SQL using a LATERAL query with the
problematic correlated USING column, place it in the LATERAL test suite
(pkg/planner testdata for LATERAL), include the exact SQL input that triggers
the failure and the expected planner output asserting that the Apply -> Join
rewrite is either not applied or produces the correct plan (verify Apply and
Join nodes in the logical/physical plan output), and update the corresponding
golden output so CI will fail if the unsafe Apply->Join rewrite regresses.
In `@tests/integrationtest/r/planner/core/lateral_join.result`:
- Around line 92-93: Add a regression test that reproduces the known
decorrelation bug by including a query using LATERAL with correlation threaded
through a join wrapped by a selection (e.g., a JOIN ... USING(...) or JOIN ...
ON ... around the correlated relation) so the Apply -> Join misrewrite would
produce zero rows; specifically add a variant of the existing lateral query
(select * from t1, t2, lateral (select c from t3 where t3.b = t2.b limit 1) as
lat where t1.a = t2.a order by t1.a, t2.b) but with the correlated subquery
placed behind a join that uses USING/ON to force the problematic path, and
assert the expected nonzero result counts (explicit expected rows) to ensure
DecorrelateSolver keeps the Apply semantics rather than turning it into a plain
Join.
---
Outside diff comments:
In `@pkg/store/mockstore/unistore/cophandler/mpp_exec.go`:
- Around line 951-962: The defer in exchSenderExec.next currently does an
unconditional receive on each tunnel.connectedCh which can deadlock if
getAndActiveTunnel never closes connectedCh; change the cleanup to use a
non-blocking select or a select that also listens on the executor/mpp context
cancellation (or a timeout) so it won't block forever: in the deferred closure
iterate tunnels and for each do select { case <-tunnel.connectedCh: /* proceed
to close ErrCh/DataCh */ case <-mppCtx.Done(): /* proceed to close ErrCh/DataCh
*/ case <-time.After(timeout): /* fallback close */ } (or use ctx instead of
time.After), then close ErrCh and DataCh and call e.stop() but handle its error
without panic (return/log it or propagate), ensuring getAndActiveTunnel is still
referenced as the creator of connectedCh for context.
---
Duplicate comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 909-918: The LATERAL LogicalApply built in buildLateralJoin is
missing marking CTEs in the right subtree as apply-scoped; after creating ap :=
logicalop.LogicalApply{...}.Init(...) and before or after SetChildren, call
setIsInApplyForCTE(rightPlan, ap.Schema()) so correlated CTEs under the LATERAL
rightPlan are marked with the apply scope (use the existing setIsInApplyForCTE
function and ap.Schema() to locate the schema).
- Around line 973-979: ap.FullSchema's nullability is not adjusted for
RightOuterJoin, causing inconsistency with ap.Schema(); after merging schemas
(ap.FullSchema = expression.MergeSchema(...)), add a branch for joinType ==
base.RightOuterJoin and call util.ResetNotNullFlag(ap.FullSchema, 0,
lFullSchema.Len()) to clear NOT NULL on the left-side columns (mirroring the
existing behavior in ap.Schema()); keep the existing LeftOuterJoin branch
unchanged.
---
Nitpick comments:
In `@br/pkg/stream/stream_mgr.go`:
- Around line 397-400: The hex parsing using strconv.ParseUint for flushTs,
minDefaultTs, minTs and maxTs currently discards errors; update the parsing to
check each returned error and handle it defensively (e.g., log a warning
including the filename or regex match and the parse error, and skip/ignore this
file or return an error) instead of silently treating values as 0; locate the
parsing calls that assign flushTs, minDefaultTs, minTs, maxTs and add error
checks that either fall back safely or filter out the corrupted filename so
downstream filtering logic cannot act on invalid zero timestamps.
In `@tests/integrationtest/r/planner/cascades/integration.result`:
- Around line 999-1000: The test modifies several session variables but only
resets tidb_opt_fix_control and tidb_enable_cascades_planner; update the test
teardown to explicitly reset all modified variables (sql_mode,
tidb_hashagg_partial_concurrency, tidb_hashagg_final_concurrency,
tidb_executor_concurrency, tidb_hash_join_concurrency,
tidb_distsql_scan_concurrency, tidb_opt_distinct_agg_push_down as well as the
already-present tidb_opt_fix_control and tidb_enable_cascades_planner) back to
their defaults (e.g., using "SET SESSION <var> = DEFAULT" or "SET @@<var> =
DEFAULT") so the session is fully restored after the test.
In `@tests/integrationtest/r/planner/core/lateral_join.result`:
- Line 6: The lateral subquery uses "LIMIT 1" without a deterministic ordering,
causing flaky results; update the lateral subquery "(select b from t2 where t2.a
= t1.a limit 1) as lat" (and the other occurrences noted) to include a stable
ORDER BY before LIMIT 1 (for example "ORDER BY <stable_column> ASC") so the
returned row is deterministic across runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1510698b-a86b-492b-b24c-6aa08bd2c9c1
⛔ Files ignored due to path filters (1)
lightning/web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.github/copilot-instructions.mdbr/pkg/stream/BUILD.bazelbr/pkg/stream/backupmetas/BUILD.bazelbr/pkg/stream/backupmetas/parser.gobr/pkg/stream/stream_mgr.gobr/pkg/stream/stream_mgr_fuzz_test.gobr/pkg/stream/stream_misc_test.goerrors.tomlpkg/errno/errcode.gopkg/errno/errname.gopkg/executor/join/concurrent_map.gopkg/executor/join/concurrent_map_test.gopkg/executor/set.gopkg/executor/set_test.gopkg/parser/BUILD.bazelpkg/parser/ast/dml.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/lateral_test.gopkg/parser/misc.gopkg/parser/mysql/errcode.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/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/rule_decorrelate.gopkg/planner/core/rule_join_reorder.gopkg/store/mockstore/unistore/cophandler/BUILD.bazelpkg/store/mockstore/unistore/cophandler/cop_handler_test.gopkg/store/mockstore/unistore/cophandler/mpp_exec.gopkg/util/dbterror/plannererrors/planner_terror.gopkg/util/hack/map_abi.gopkg/util/hack/map_abi_go126.gopkg/util/hack/map_abi_test.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
💤 Files with no reviewable changes (7)
- br/pkg/stream/stream_mgr_fuzz_test.go
- br/pkg/stream/backupmetas/BUILD.bazel
- br/pkg/stream/backupmetas/parser.go
- pkg/store/mockstore/unistore/cophandler/cop_handler_test.go
- br/pkg/stream/BUILD.bazel
- pkg/executor/set_test.go
- pkg/executor/join/concurrent_map.go
✅ Files skipped from review due to trivial changes (2)
- pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_xut.json
- pkg/planner/cascades/cascades_test.go
🚧 Files skipped from review as they are similar to previous changes (24)
- pkg/parser/BUILD.bazel
- tests/integrationtest/t/planner/core/lateral_join.test
- pkg/planner/core/planbuilder.go
- pkg/parser/keywords_test.go
- pkg/parser/mysql/errcode.go
- pkg/planner/core/casetest/rule/testdata/outer2inner_out.json
- pkg/store/mockstore/unistore/cophandler/BUILD.bazel
- tests/integrationtest/t/planner/cascades/integration.test
- pkg/planner/core/lateral_join_test.go
- pkg/executor/join/concurrent_map_test.go
- errors.toml
- pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json
- tests/integrationtest/r/expression/misc.result
- pkg/planner/core/expression_rewriter.go
- pkg/parser/ast/dml.go
- pkg/planner/core/rule_decorrelate.go
- pkg/executor/set.go
- tests/integrationtest/r/planner/core/integration.result
- pkg/planner/core/operator/logicalop/logical_join.go
- pkg/parser/misc.go
- pkg/parser/keywords.go
- tests/integrationtest/r/cte.result
- pkg/errno/errname.go
- .github/copilot-instructions.md
pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.json
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 6
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)
767-788:⚠️ Potential issue | 🔴 CriticalPreserve
FullSchemathrough wrapper nodes when merging join schemas.This branch only checks direct
*logicalop.LogicalJoin/*logicalop.LogicalApply. If a child is wrapped by*logicalop.LogicalSelection(common afterONon inner joins), redundant USING/NATURAL columns are dropped fromjoinPlan.FullSchema, which can later lose correlations and enable incorrect rewrites.💡 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 { + // Keep wrapper-awareness consistent with LATERAL correlation extraction. + 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() }Based on learnings: Correctness first. TiDB is a distributed SQL database; seemingly small changes can alter SQL semantics, consistency, or cluster behavior.
🤖 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 767 - 788, The current merge logic only inspects leftPlan/rightPlan for direct *logicalop.LogicalJoin or *logicalop.LogicalApply and falls back to Schema()/OutputNames(), which loses upstream FullSchema when those joins are wrapped (e.g., by *logicalop.LogicalSelection) and causes DROP of USING/NATURAL columns; update the logic to unwrap common wrapper nodes (at least *logicalop.LogicalSelection) to find an embedded *logicalop.LogicalJoin or *logicalop.LogicalApply and copy their FullSchema/FullNames into lFullSchema/lFullNames and rFullSchema/rFullNames when present, otherwise fall back to Schema()/OutputNames(); reference the symbols leftPlan/rightPlan, logicalop.LogicalJoin, logicalop.LogicalApply, logicalop.LogicalSelection, lFullSchema/lFullNames and rFullSchema/rFullNames when making the change.
♻️ Duplicate comments (4)
pkg/parser/lateral_test.go (1)
154-171:⚠️ Potential issue | 🟡 MinorAdd an explicit non-LATERAL AST assertion in the
tc.checkLateral == falsepath.When
tc.checkLateralis false, this block performs no AST-level guard, so accidentalTableSource.Lateral=trueregressions can slip through.Suggested assertion update
- // Verify AST flag: check if LATERAL table sources exist - if tc.checkLateral { - selectStmt, ok := stmt.(*ast.SelectStmt) - require.True(t, ok, "Statement should be SelectStmt") - require.NotNil(t, selectStmt.From, "FROM clause should not be nil") - - // Verify at least one LATERAL table source exists in the FROM clause - lateralTS := findLateralTableSource(selectStmt.From.TableRefs) - require.NotNil(t, lateralTS, "LATERAL TableSource not found in AST for: %s", tc.sql) - - // Verify column names if expected - if len(tc.columnNames) > 0 { - require.Len(t, lateralTS.ColumnNames, len(tc.columnNames), "column name count mismatch") - for i, expected := range tc.columnNames { - require.Equal(t, expected, lateralTS.ColumnNames[i].L, "column name mismatch at index %d", i) - } - } - } + selectStmt, ok := stmt.(*ast.SelectStmt) + require.True(t, ok, "Statement should be SelectStmt") + require.NotNil(t, selectStmt.From, "FROM clause should not be nil") + lateralTS := findLateralTableSource(selectStmt.From.TableRefs) + + if tc.checkLateral { + require.NotNil(t, lateralTS, "LATERAL TableSource not found in AST for: %s", tc.sql) + if len(tc.columnNames) > 0 { + require.Len(t, lateralTS.ColumnNames, len(tc.columnNames), "column name count mismatch") + for i, expected := range tc.columnNames { + require.Equal(t, expected, lateralTS.ColumnNames[i].L, "column name mismatch at index %d", i) + } + } + } else { + require.Nil(t, lateralTS, "Unexpected LATERAL TableSource in AST for: %s", tc.sql) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/lateral_test.go` around lines 154 - 171, The test is missing an explicit assertion for the tc.checkLateral == false case; update the test around the SelectStmt/From handling so that when tc.checkLateral is false you still parse the statement (use SelectStmt and selectStmt.From) and assert that findLateralTableSource(selectStmt.From.TableRefs) returns nil and that no TableSource has Lateral == true (check TableSource.Lateral across table refs) to prevent regressions where LATERAL is set unexpectedly.pkg/planner/core/logical_plan_builder.go (2)
916-918:⚠️ Potential issue | 🟠 MajorMark CTEs in the LATERAL right subtree as apply-scoped.
This manual
LogicalApplyconstruction still skipssetIsInApplyForCTE(rightPlan, ap.Schema()), so correlated CTEs under the lateral subtree may reuse storage across outer-row iterations.💡 Suggested fix
ap.SetChildren(leftPlan, rightPlan) ap.SetSchema(expression.MergeSchema(leftPlan.Schema(), rightPlan.Schema())) + setIsInApplyForCTE(rightPlan, ap.Schema())🤖 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 916 - 918, The LogicalApply construction currently sets children and schema via ap.SetChildren(leftPlan, rightPlan) and ap.SetSchema(...), but it omits marking correlated CTEs in the LATERAL right subtree as apply-scoped; call setIsInApplyForCTE(rightPlan, ap.Schema()) immediately after ap.SetChildren(leftPlan, rightPlan) (before or right after ap.SetSchema) to mark CTEs in the rightPlan as apply-scoped so they do not reuse storage across outer-row iterations; reference LogicalApply/ap, leftPlan, rightPlan, setIsInApplyForCTE, and ap.Schema() when making this change.
975-979:⚠️ Potential issue | 🟠 MajorKeep
ap.FullSchemanullability consistent forRIGHT JOIN LATERAL.
ap.Schema()clears NOT NULL on left-side columns forRightOuterJoin, butap.FullSchemaonly does this forLeftOuterJoin. That inconsistency can leak incorrect nullability to downstream resolution/type checks.💡 Suggested fix
- // Clear NotNull flag for the inner side (right side) of FullSchema if it's a LEFT JOIN - // (consistent with buildJoin behavior at line 681-682) - if joinType == base.LeftOuterJoin { - util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()) - } + switch joinType { + case base.LeftOuterJoin: + util.ResetNotNullFlag(ap.FullSchema, lFullSchema.Len(), ap.FullSchema.Len()) + case base.RightOuterJoin: + util.ResetNotNullFlag(ap.FullSchema, 0, lFullSchema.Len()) + }🤖 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 975 - 979, ap.FullSchema's nullability is only being adjusted for LeftOuterJoin but not for RightOuterJoin, causing inconsistency with ap.Schema() which already clears left-side NOT NULL for RightOuterJoin; update the join-handling code to also clear NOT NULL on the left-side range of ap.FullSchema when joinType == base.RightOuterJoin by calling util.ResetNotNullFlag with the left-side range (start 0, end lFullSchema.Len()) so ap.FullSchema nullability matches ap.Schema() and buildJoin behavior.tests/integrationtest/r/planner/core/integration.result (1)
2844-2844:⚠️ Potential issue | 🟡 MinorDon’t bake the truncated
OUTR.diagnostic into the golden.Line 2844 still expects
OUTR.as the offending column, so the test would bless a formatter regression instead of the real user-facing identifier/expression.As per coding guidelines: When recording outputs, verify changed result files before reporting completion.
🤖 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 golden contains a truncated diagnostic token "OUTR." which bakes a formatter regression into the expected output; open the failing golden result file and replace the truncated "OUTR." entry with the real user-facing identifier/expression from the actual DB error (or a stable placeholder like "<offending expression>") and then regenerate the golden by re-running the integration test to capture the true error text and verify the changed result before committing.
🧹 Nitpick comments (3)
tests/integrationtest/t/planner/cascades/integration.test (1)
279-280: Consider resetting all modified session variables at file end for test isolation.This file changes several session knobs earlier but only restores two at the end. Resetting the rest to
defaultwould reduce hidden coupling when adding future cases.Suggested cleanup
set session tidb_opt_fix_control = default; set @@tidb_enable_cascades_planner = default; +set @@session.tidb_executor_concurrency = default; +set @@session.tidb_hash_join_concurrency = default; +set @@session.tidb_distsql_scan_concurrency = default; +set @@session.tidb_hashagg_partial_concurrency = default; +set @@session.tidb_hashagg_final_concurrency = default; +set @@session.tidb_opt_distinct_agg_push_down = default; +set session sql_mode = default;🤖 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 279 - 280, The test currently only restores two session knobs (`set session tidb_opt_fix_control = default;` and `set @@tidb_enable_cascades_planner = default;`) but other session variables changed earlier in the file remain modified; update the test teardown at the file end to reset every session variable you modify earlier back to `default` (add corresponding `SET SESSION <var> = DEFAULT;` or `SET @@<var> = DEFAULT;` statements for each knob changed) so `tidb_opt_fix_control` and `@@tidb_enable_cascades_planner` and all other modified knobs are restored for test isolation.pkg/planner/core/rule_join_reorder.go (1)
37-45: Align the guard with the stated LATERAL-only intent.The comment describes a LATERAL-specific rule, but the check matches all
LogicalApply. Either gate onapply.IsLateralor update the comment to explicitly say all Apply nodes are excluded.♻️ Suggested alignment
- if _, isApply := p.(*logicalop.LogicalApply); isApply { + if apply, isApply := p.(*logicalop.LogicalApply); isApply && apply.IsLateral { return &joinGroupResult{ group: []base.LogicalPlan{p}, joinOrderHintInfo: nil, basicJoinGroupInfo: &basicJoinGroupInfo{}, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/rule_join_reorder.go` around lines 37 - 45, The guard currently excludes all LogicalApply nodes but the comment says only LATERAL joins should be excluded; change the condition to first type-assert p to *logicalop.LogicalApply (as done) and then check its IsLateral flag (e.g., if apply, isApply := p.(*logicalop.LogicalApply); isApply && apply.IsLateral { ... }) so only lateral Apply nodes return the joinGroupResult with group containing p and basicJoinGroupInfo; alternatively, if the intent was to exclude all Apply nodes, update the comment to reflect that.pkg/parser/lateral_test.go (1)
176-196:findLateralTableSourceonly returns the first match, limiting multi-LATERAL validation.For cases like
"Multiple LATERAL joins", the current helper proves existence of one LATERAL source, not that all expected LATERAL sources are present. Consider a count-based helper and per-case expected count assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/lateral_test.go` around lines 176 - 196, The helper findLateralTableSource only returns the first LATERAL TableSource which prevents verifying multiple LATERAL occurrences; replace or add a counting helper (e.g., countLateralTableSources) that recursively traverses ast.ResultSetNode (handling *ast.TableSource and *ast.Join as findLateralTableSource does) and returns the total number of nodes with Lateral==true, then update tests (the "Multiple LATERAL joins" case and any other relevant cases) to assert the returned count matches an expected per-case count instead of just checking existence via findLateralTableSource.
🤖 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/lateral_test.go`:
- Around line 30-123: Add a planner integration regression test (e.g.,
TestLateralApplyJoinDecorrelationRegression) that executes the problematic
join-tree+ON+LATERAL SQL shape (use a query that mirrors the bug: a base table
t1 LEFT JOIN LATERAL (subquery referencing t1) ... with GROUP BY/COUNT) and
assert MySQL-compatible non-zero counts for the grouped results; ensure the test
runs in the planner/integration test harness, uses existing helpers to prepare
test data, runs the SQL, and compares COUNT(*) results to expected non-zero
values so the test fails before the fix and passes after (refer to the LATERAL
occurrences in the current lateral_test cases and the Apply→Join rewrite path to
locate where to add the new test).
In `@pkg/planner/core/lateral_join_test.go`:
- Around line 202-205: The test's assertion is too weak: replace the generic
require.GreaterOrEqual(t, applyCount, 1) with a case-specific expectation that
verifies multiple Apply nodes; use the countLogicalApply(lp) result (applyCount)
and assert the exact or stronger lower bound for the "Multiple LATERAL joins
prevent reordering" case (e.g., require.GreaterOrEqual(t, applyCount, 2)) so the
test actually validates multiple LATERAL joins rather than just one; update the
assertion near where countLogicalApply and applyCount are used in the
lateral_join_test.go test case.
- Around line 35-41: The error-path tests in TestLateralJoinErrorPaths (and
similar test blocks using testCases with fields expectError and
expectedErrorCode) are under-asserting because expectedErrorCode is left unset
or used as a magic sentinel; update those test cases so every case that sets
expectError=true also sets an explicit expectedErrorCode (don't use 1 as a
sentinel) and change the assertion logic in the test runner to compare the
returned error's concrete error code/kind against expectedErrorCode (e.g.,
inspect the error via the planner error type or error.Code() in the test helper
used by TestLateralJoinErrorPaths and related tables) so the test fails if the
error kind is different. Ensure you update all referenced test tables/blocks
(testCases, TestLateralJoinErrorPaths, and other occurrences noted) to include
explicit expectedErrorCode values and a strict equality check rather than
relying on expectError alone.
In `@tests/integrationtest/r/planner/core/integration.result`:
- Around line 2347-2349: Add a regression reproducer that captures the LATERAL
wrong-result tied to decorrelation through a LogicalSelection-wrapped join tree:
create an integration test entry that reproduces the reported wrong result (the
case involving LATERAL, LogicalSelection and the ttest join pattern) and record
its expected output in the planner integration results so the CI will catch
regressions; place the new case alongside the existing ONLY_FULL_GROUP_BY
entries in the planner integration coverage output so it’s exercised by the
planner/integration tests.
In `@tests/integrationtest/r/planner/core/lateral_join.result`:
- Line 6: The lateral subquery "lateral (select b from t2 where t2.a = t1.a
limit 1) as lat" (and the similar occurrences flagged) relies on implicit row
order and must be made deterministic: add an explicit ORDER BY clause inside
each lateral or other subquery that uses LIMIT 1 (e.g., ORDER BY b ASC or
another stable key) immediately before LIMIT 1 so the returned row is
deterministic, mirroring the pattern used elsewhere in the file.
In `@tests/integrationtest/t/planner/cascades/integration.test`:
- Around line 225-239: TestApply lacks the specific LATERAL-over-join-tree
regression case that causes Apply→Join mis-rewrite when decorrelation clears
correlated columns; add the exact reproducer from 0xPoe’s report into TestApply
(the integration.test case) so it exercises a LATERAL (or correlated subquery)
over a join tree using USING plus ON/selection wrapper, include set session
tidb_enable_cascades_planner = 1, add both EXPLAIN FORMAT="plan_tree" and the
actual SELECT (result) assertions, and assert the EXPLAIN still contains an
Apply (or otherwise demonstrates that apply.CorCols was preserved and no rewrite
to Join occurred) to lock the regression down.
---
Outside diff comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 767-788: The current merge logic only inspects leftPlan/rightPlan
for direct *logicalop.LogicalJoin or *logicalop.LogicalApply and falls back to
Schema()/OutputNames(), which loses upstream FullSchema when those joins are
wrapped (e.g., by *logicalop.LogicalSelection) and causes DROP of USING/NATURAL
columns; update the logic to unwrap common wrapper nodes (at least
*logicalop.LogicalSelection) to find an embedded *logicalop.LogicalJoin or
*logicalop.LogicalApply and copy their FullSchema/FullNames into
lFullSchema/lFullNames and rFullSchema/rFullNames when present, otherwise fall
back to Schema()/OutputNames(); reference the symbols leftPlan/rightPlan,
logicalop.LogicalJoin, logicalop.LogicalApply, logicalop.LogicalSelection,
lFullSchema/lFullNames and rFullSchema/rFullNames when making the change.
---
Duplicate comments:
In `@pkg/parser/lateral_test.go`:
- Around line 154-171: The test is missing an explicit assertion for the
tc.checkLateral == false case; update the test around the SelectStmt/From
handling so that when tc.checkLateral is false you still parse the statement
(use SelectStmt and selectStmt.From) and assert that
findLateralTableSource(selectStmt.From.TableRefs) returns nil and that no
TableSource has Lateral == true (check TableSource.Lateral across table refs) to
prevent regressions where LATERAL is set unexpectedly.
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 916-918: The LogicalApply construction currently sets children and
schema via ap.SetChildren(leftPlan, rightPlan) and ap.SetSchema(...), but it
omits marking correlated CTEs in the LATERAL right subtree as apply-scoped; call
setIsInApplyForCTE(rightPlan, ap.Schema()) immediately after
ap.SetChildren(leftPlan, rightPlan) (before or right after ap.SetSchema) to mark
CTEs in the rightPlan as apply-scoped so they do not reuse storage across
outer-row iterations; reference LogicalApply/ap, leftPlan, rightPlan,
setIsInApplyForCTE, and ap.Schema() when making this change.
- Around line 975-979: ap.FullSchema's nullability is only being adjusted for
LeftOuterJoin but not for RightOuterJoin, causing inconsistency with ap.Schema()
which already clears left-side NOT NULL for RightOuterJoin; update the
join-handling code to also clear NOT NULL on the left-side range of
ap.FullSchema when joinType == base.RightOuterJoin by calling
util.ResetNotNullFlag with the left-side range (start 0, end lFullSchema.Len())
so ap.FullSchema nullability matches ap.Schema() and buildJoin behavior.
In `@tests/integrationtest/r/planner/core/integration.result`:
- Line 2844: The golden contains a truncated diagnostic token "OUTR." which
bakes a formatter regression into the expected output; open the failing golden
result file and replace the truncated "OUTR." entry with the real user-facing
identifier/expression from the actual DB error (or a stable placeholder like
"<offending expression>") and then regenerate the golden by re-running the
integration test to capture the true error text and verify the changed result
before committing.
---
Nitpick comments:
In `@pkg/parser/lateral_test.go`:
- Around line 176-196: The helper findLateralTableSource only returns the first
LATERAL TableSource which prevents verifying multiple LATERAL occurrences;
replace or add a counting helper (e.g., countLateralTableSources) that
recursively traverses ast.ResultSetNode (handling *ast.TableSource and *ast.Join
as findLateralTableSource does) and returns the total number of nodes with
Lateral==true, then update tests (the "Multiple LATERAL joins" case and any
other relevant cases) to assert the returned count matches an expected per-case
count instead of just checking existence via findLateralTableSource.
In `@pkg/planner/core/rule_join_reorder.go`:
- Around line 37-45: The guard currently excludes all LogicalApply nodes but the
comment says only LATERAL joins should be excluded; change the condition to
first type-assert p to *logicalop.LogicalApply (as done) and then check its
IsLateral flag (e.g., if apply, isApply := p.(*logicalop.LogicalApply); isApply
&& apply.IsLateral { ... }) so only lateral Apply nodes return the
joinGroupResult with group containing p and basicJoinGroupInfo; alternatively,
if the intent was to exclude all Apply nodes, update the comment to reflect
that.
In `@tests/integrationtest/t/planner/cascades/integration.test`:
- Around line 279-280: The test currently only restores two session knobs (`set
session tidb_opt_fix_control = default;` and `set @@tidb_enable_cascades_planner
= default;`) but other session variables changed earlier in the file remain
modified; update the test teardown at the file end to reset every session
variable you modify earlier back to `default` (add corresponding `SET SESSION
<var> = DEFAULT;` or `SET @@<var> = DEFAULT;` statements for each knob changed)
so `tidb_opt_fix_control` and `@@tidb_enable_cascades_planner` and all other
modified knobs are restored for test isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: de2c7a79-7d79-44e9-87d5-bf7dd325bd4f
📒 Files selected for processing (37)
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/mysql/errcode.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/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/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
🚧 Files skipped from review as they are similar to previous changes (15)
- pkg/planner/core/rule_decorrelate.go
- pkg/parser/keywords_test.go
- pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_out.json
- tests/integrationtest/r/cte.result
- errors.toml
- pkg/errno/errname.go
- pkg/planner/core/operator/logicalop/hash64_equals_generated.go
- pkg/planner/cascades/cascades_test.go
- pkg/parser/ast/dml.go
- pkg/planner/core/BUILD.bazel
- pkg/planner/core/casetest/rule/testdata/outer2inner_out.json
- pkg/parser/misc.go
- pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_xut.json
- pkg/planner/core/casetest/enforcempp/testdata/enforce_mpp_suite_in.json
- tests/integrationtest/r/expression/misc.result
Comment on lines
+30
to
+123
| testCases := []struct { | ||
| name string | ||
| sql string | ||
| expectError bool | ||
| checkLateral bool // whether to verify Lateral flag is set | ||
| columnNames []string | ||
| }{ | ||
| { | ||
| name: "LATERAL with comma syntax", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT t1.a) AS dt", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with LEFT JOIN", | ||
| sql: "SELECT * FROM t1 LEFT JOIN LATERAL (SELECT t1.b) AS dt ON true", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with CROSS JOIN", | ||
| sql: "SELECT * FROM t1 CROSS JOIN LATERAL (SELECT t1.c) AS dt", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with RIGHT JOIN", | ||
| sql: "SELECT * FROM t1 RIGHT JOIN LATERAL (SELECT t1.d) AS dt ON true", | ||
| expectError: false, // Parser allows it, planner will reject | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with INNER JOIN", | ||
| sql: "SELECT * FROM t1 JOIN LATERAL (SELECT t1.e) AS dt ON true", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with complex subquery", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT t1.a, COUNT(*) FROM t2 WHERE t2.x = t1.x GROUP BY t1.a) AS dt", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with nested subquery", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT * FROM (SELECT t1.a) AS inner_dt) AS dt", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "Multiple LATERAL joins", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT t1.a) AS dt1, LATERAL (SELECT t1.b) AS dt2", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "Non-LATERAL derived table", | ||
| sql: "SELECT * FROM t1, (SELECT a FROM t2) AS dt", | ||
| expectError: false, | ||
| // Lateral flag should be false for non-LATERAL | ||
| }, | ||
| { | ||
| name: "LATERAL with WHERE clause", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT * FROM t2 WHERE t2.x = t1.x) AS dt WHERE dt.y > 10", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| }, | ||
| { | ||
| name: "LATERAL with column list", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT t1.a, t1.b) AS dt(c1, c2)", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| columnNames: []string{"c1", "c2"}, | ||
| }, | ||
| { | ||
| name: "LATERAL with column list no AS", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT t1.a) dt(col1)", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| columnNames: []string{"col1"}, | ||
| }, | ||
| { | ||
| name: "LATERAL with column list and JOIN", | ||
| sql: "SELECT * FROM t1 LEFT JOIN LATERAL (SELECT t1.a, t1.b, t1.c) AS dt(x, y, z) ON true", | ||
| expectError: false, | ||
| checkLateral: true, | ||
| columnNames: []string{"x", "y", "z"}, | ||
| }, | ||
| { | ||
| name: "LATERAL without alias is rejected", | ||
| sql: "SELECT * FROM t1, LATERAL (SELECT t1.a)", | ||
| expectError: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Missing regression coverage for the reported LATERAL decorrelation correctness bug.
This suite validates parsing/AST/restore, but it does not cover the concrete planner/execution regression reported in this PR discussion (join tree + ON + LATERAL producing wrong zero counts). Please add a regression test in planner/integration tests using that SQL shape and assert MySQL-compatible non-zero counts so Apply→Join rewrite regressions are caught.
Based on learnings: "Applies to **/*.go : MUST add a regression test and verify it fails before fix and passes after fix for bug fixes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/parser/lateral_test.go` around lines 30 - 123, Add a planner integration
regression test (e.g., TestLateralApplyJoinDecorrelationRegression) that
executes the problematic join-tree+ON+LATERAL SQL shape (use a query that
mirrors the bug: a base table t1 LEFT JOIN LATERAL (subquery referencing t1) ...
with GROUP BY/COUNT) and assert MySQL-compatible non-zero counts for the grouped
results; ensure the test runs in the planner/integration test harness, uses
existing helpers to prepare test data, runs the SQL, and compares COUNT(*)
results to expected non-zero values so the test fails before the fix and passes
after (refer to the LATERAL occurrences in the current lateral_test cases and
the Apply→Join rewrite path to locate where to add the new test).
Comment on lines
+35
to
+41
| testCases := []struct { | ||
| name string | ||
| sql string | ||
| expectApply bool | ||
| expectError bool | ||
| expectedErrorCode int | ||
| }{ |
There was a problem hiding this comment.
Error-path tests are under-asserting the expected error type.
expectedErrorCode is used as a magic sentinel (e.g., 1) in one test and not set in TestLateralJoinErrorPaths, so wrong error kinds can still pass when expectError is true.
Suggested tightening
- testCases := []struct {
- name string
- sql string
- expectApply bool
- expectError bool
- expectedErrorCode int
- }{
+ testCases := []struct {
+ name string
+ sql string
+ expectApply bool
+ expectError bool
+ expectInvalidLateralErr bool
+ }{
{
name: "LATERAL with RIGHT JOIN and outer ref returns error",
sql: "SELECT * FROM t RIGHT JOIN LATERAL (SELECT t.a) AS dt ON true",
expectError: true,
- expectedErrorCode: 1,
+ expectInvalidLateralErr: true,
},
}
if tc.expectError {
require.Error(t, err, "Expected error for: %s", tc.sql)
- if tc.expectedErrorCode != 0 {
+ if tc.expectInvalidLateralErr {
require.True(t, plannererrors.ErrInvalidLateralJoin.Equal(err),
"Expected ErrInvalidLateralJoin error, got: %v", err)
}
return
}Also applies to: 58-62, 98-103, 327-332, 363-368
🤖 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 error-path
tests in TestLateralJoinErrorPaths (and similar test blocks using testCases with
fields expectError and expectedErrorCode) are under-asserting because
expectedErrorCode is left unset or used as a magic sentinel; update those test
cases so every case that sets expectError=true also sets an explicit
expectedErrorCode (don't use 1 as a sentinel) and change the assertion logic in
the test runner to compare the returned error's concrete error code/kind against
expectedErrorCode (e.g., inspect the error via the planner error type or
error.Code() in the test helper used by TestLateralJoinErrorPaths and related
tables) so the test fails if the error kind is different. Ensure you update all
referenced test tables/blocks (testCases, TestLateralJoinErrorPaths, and other
occurrences noted) to include explicit expectedErrorCode values and a strict
equality check rather than relying on expectError alone.
Comment on lines
+202
to
+205
| // Verify multiple Apply operators for multiple LATERAL joins | ||
| applyCount := countLogicalApply(lp) | ||
| require.GreaterOrEqual(t, applyCount, 1) | ||
| } |
There was a problem hiding this comment.
The “multiple LATERAL joins” assertion is too weak for its stated intent.
For the case named “Multiple LATERAL joins prevent reordering”, applyCount >= 1 does not verify multiple apply nodes. Please assert an expected count per case (e.g., >= 2 for that specific SQL).
🤖 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 202 - 205, The test's
assertion is too weak: replace the generic require.GreaterOrEqual(t, applyCount,
1) with a case-specific expectation that verifies multiple Apply nodes; use the
countLogicalApply(lp) result (applyCount) and assert the exact or stronger lower
bound for the "Multiple LATERAL joins prevent reordering" case (e.g.,
require.GreaterOrEqual(t, applyCount, 2)) so the test actually validates
multiple LATERAL joins rather than just one; update the assertion near where
countLogicalApply and applyCount are used in the lateral_join_test.go test case.
Comment on lines
+2347
to
+2349
| Error 1055 (42000): Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 't1.v2' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by | ||
| select v2, v1 from (select t1.v1, t2.v2 from ttest t1 join ttest t2) t3 join (select 1, 2) t2 group by v1; | ||
| Error 1055 (42000): Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'planner__core__integration.t3.v2' 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 't3.v2' 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.
Add the reported LATERAL wrong-result reproducer before merging.
Lines 2347-2349 refresh unrelated ONLY_FULL_GROUP_BY output, but the concrete LATERAL regression from the PR discussion is still missing from planner/integration coverage. That case already has a wrong-result report involving decorrelation through a LogicalSelection-wrapped join tree, so it should be captured here before this change ships.
Based on learnings: Correctness first. TiDB is a distributed SQL database; seemingly small changes can alter SQL semantics, consistency, or cluster behavior.
🤖 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` around lines 2347 -
2349, Add a regression reproducer that captures the LATERAL wrong-result tied to
decorrelation through a LogicalSelection-wrapped join tree: create an
integration test entry that reproduces the reported wrong result (the case
involving LATERAL, LogicalSelection and the ttest join pattern) and record its
expected output in the planner integration results so the CI will catch
regressions; place the new case alongside the existing ONLY_FULL_GROUP_BY
entries in the planner integration coverage output so it’s exercised by the
planner/integration tests.
Contributor
There was a problem hiding this comment.
Hmm, it constantly catches new issues.
There was a problem hiding this comment.
@fixdb, that's the nature of complex planner changes — each new capability (LATERAL joins here) tends to interact with existing machinery in subtle ways. The correctness regression flagged by 0xPoe (decorrelation through LogicalSelection-wrapped join trees) is a good example: it's a real edge case that's easy to miss without a dedicated test.
If it's helpful, adding a regression test for that specific pattern would also serve as living documentation of the known constraint so future contributors don't accidentally re-introduce it. 🐇
🧠 Learnings used
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: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.
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: joechenrh
Repo: pingcap/tidb PR: 66878
File: tests/realtikvtest/importintotest/import_into_test.go:253-267
Timestamp: 2026-03-11T06:26:14.065Z
Learning: In pingcap/tidb, PR `#66878` (cherry-pick of `#58401` to release-7.5) also bundles the follow-up nil-check fix from PR `#63146`. `TestOnUpdateColumn` in `tests/realtikvtest/importintotest/import_into_test.go` was cherry-picked from PR `#63146` and tests that `IMPORT INTO` on a table with an `ON UPDATE CURRENT_TIMESTAMP` column does NOT panic due to a nil `onDup` map in `resolveGeneratedColumns`. It is NOT intended to cover the transitive generated-column / index-inconsistency fix from `#58401/`#58400.
Learnt from: CR
Repo: pingcap/tidb PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-12T17:03:05.283Z
Learning: Correctness first. TiDB is a distributed SQL database; seemingly small changes can alter SQL semantics, consistency, or cluster behavior.
Learnt from: cryo-zd
Repo: pingcap/tidb PR: 65532
File: pkg/planner/core/preprocess.go:754-760
Timestamp: 2026-03-13T00:34:41.277Z
Learning: In TiDB's AST visitor pattern (pkg/planner/core/preprocess.go), when a child node's Accept returns (node, false), the parent node's Accept propagates false upward WITHOUT calling Leave on the parent. Because preprocessor.Leave returns `ok = (p.err == nil)`, any subtree error causes the entire walk to short-circuit before the enclosing SelectStmt's Leave is reached. Therefore, code inside `case *ast.SelectStmt:` in `Leave` (e.g., checkLockClauseTables) is only ever executed when all children completed successfully and p.err is still nil. A guard like `if p.err == nil { checkLockClauseTables(...) }` would be purely cosmetic/defensive, not fixing any reachable bug.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 64752
File: pkg/planner/core/planbuilder.go:3987-3991
Timestamp: 2026-03-12T02:22:08.966Z
Learning: In `pkg/planner/core/planbuilder.go`, `resolveGeneratedColumns` uses `onDups != nil` (not `len(onDups) > 0`) as the intentional guard to distinguish call sites: `nil` means IMPORT INTO / LOAD DATA (must not mutate the map, and ON UPDATE NOW columns should NOT be recorded); non-nil (even if empty) means the INSERT path, where ON UPDATE NOW columns should be added to `onDups` so that dependent generated columns can be included in `igc.OnDuplicates`. Suggesting `len(onDups) > 0` instead of `onDups != nil` is a false positive — the nil/non-nil contract was established by PR `#63146` to fix a panic when IMPORT INTO passed nil.
Learnt from: terry1purcell
Repo: pingcap/tidb PR: 66695
File: pkg/planner/cardinality/BUILD.bazel:62-66
Timestamp: 2026-03-13T01:48:14.212Z
Learning: In pingcap/tidb cherry-pick PRs, unresolved merge conflicts (e.g., conflict markers in BUILD.bazel or other files) are expected artifacts of the cherry-pick process and are already tracked by the ti-chi-bot hold mechanism. Do NOT flag merge conflicts as review issues — they are not a reason to block or comment on the code review.
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: joechenrh
Repo: pingcap/tidb PR: 64752
File: pkg/planner/core/planbuilder.go:3987-3991
Timestamp: 2026-03-12T02:22:08.966Z
Learning: In `pkg/planner/core/planbuilder.go` (pingcap/tidb), `resolveGeneratedColumns` uses `onDups != nil` (not `len(onDups) > 0`) as the intentional guard for all mutations to the `onDups` map. `nil` means the IMPORT INTO / LOAD DATA call path (these callers pass nil explicitly, and the map must not be mutated). Non-nil — even if the map is empty — means the INSERT path; in this case ON UPDATE NOW columns must be recorded in `onDups` so that dependent generated columns propagate to `igc.OnDuplicates` when a duplicate-key conflict arises. Suggesting `len(onDups) > 0` instead of `onDups != nil` is a false positive: the nil/non-nil contract was established by PR `#63146` ("fix missing nilness check for passed onDup map") as a follow-up to PR `#58401`.
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.
| 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.
Nondeterministic LIMIT 1 queries make this result set flaky.
These statements assert specific values while relying on implicit row order. Please add deterministic ORDER BY inside each lateral/non-lateral subquery before LIMIT 1 (same pattern already used at Line 11).
Deterministic query shape
-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, The
lateral subquery "lateral (select b from t2 where t2.a = t1.a limit 1) as lat"
(and the similar occurrences flagged) relies on implicit row order and must be
made deterministic: add an explicit ORDER BY clause inside each lateral or other
subquery that uses LIMIT 1 (e.g., ORDER BY b ASC or another stable key)
immediately before LIMIT 1 so the returned row is deterministic, mirroring the
pattern used elsewhere in the file.
Contributor
There was a problem hiding this comment.
Yep, we need an order by in the lateral subquery
There was a problem hiding this comment.
🧩 Analysis chain
Thanks for confirming, @fixdb! Let me inspect the relevant lines in both the test input and result files to prepare the fix.
🏁 Script executed:
#!/bin/bash
# Read the test input file
cat tests/integrationtest/t/planner/core/lateral_join.test
echo "=== RESULT FILE ==="
cat tests/integrationtest/r/planner/core/lateral_join.result
Length of output: 8011
@fixdb, agreed! Here's the full fix plan for both files. The output values stay the same since the test data already happens to satisfy the new ordering, so only the SQL statement text changes.
- Update 4
LIMIT 1queries intests/integrationtest/t/planner/core/lateral_join.testto addORDER BYbeforeLIMIT 1 - Mirror those same SQL text changes in
tests/integrationtest/r/planner/core/lateral_join.result - Output rows are unaffected (data is already in sorted order)
🧠 Learnings used
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: 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-12T15:52:55.260Z
Learning: Applies to pkg/ddl/** : For DDL schema changes, perform DDL-focused unit/integration tests and compatibility impact checks.
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: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.
Learnt from: joechenrh
Repo: pingcap/tidb PR: 66878
File: tests/realtikvtest/importintotest/import_into_test.go:253-267
Timestamp: 2026-03-11T06:26:14.065Z
Learning: In pingcap/tidb, PR `#66878` (cherry-pick of `#58401` to release-7.5) also bundles the follow-up nil-check fix from PR `#63146`. `TestOnUpdateColumn` in `tests/realtikvtest/importintotest/import_into_test.go` was cherry-picked from PR `#63146` and tests that `IMPORT INTO` on a table with an `ON UPDATE CURRENT_TIMESTAMP` column does NOT panic due to a nil `onDup` map in `resolveGeneratedColumns`. It is NOT intended to cover the transitive generated-column / index-inconsistency fix from `#58401/`#58400.
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.
Failed to handle agent chat message. Please try again.
Comment on lines
+225
to
+239
| # 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); | ||
|
|
There was a problem hiding this comment.
Add a targeted LATERAL regression case for the known Apply→Join mis-rewrite.
TestApply covers correlated subqueries, but it does not cover the reported correctness shape: LATERAL over a join tree (especially USING + ON/selection wrapper) where decorrelation can incorrectly make len(apply.CorCols)==0 and rewrite to Join. Please add the exact reproducer from 0xPoe’s report (with both EXPLAIN and result assertion) to lock this down.
Based on learnings: "Correctness first. TiDB is a distributed SQL database; seemingly small changes can alter SQL semantics, consistency, or cluster behavior."
🤖 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 225 -
239, TestApply lacks the specific LATERAL-over-join-tree regression case that
causes Apply→Join mis-rewrite when decorrelation clears correlated columns; add
the exact reproducer from 0xPoe’s report into TestApply (the integration.test
case) so it exercises a LATERAL (or correlated subquery) over a join tree using
USING plus ON/selection wrapper, include set session
tidb_enable_cascades_planner = 1, add both EXPLAIN FORMAT="plan_tree" and the
actual SELECT (result) assertions, and assert the EXPLAIN still contains an
Apply (or otherwise demonstrates that apply.CorCols was preserved and no rewrite
to Join occurred) to lock the regression down.
|
@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. |
|
@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. |
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
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?
Add LATERAL join support to MySQL compatible behavior. Include LATERAL + ORDER BY + LIMIT within a recursive query - where the ORDER BY + LIMIT is within a lateral join.
All identified review comments have been resolved.
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
Release Notes
New Features
Tests