planner: lateral join quality updates (#67482)#68889
Open
ti-chi-bot wants to merge 1 commit into
Open
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
13 tasks
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
Member
Author
|
@terry1purcell This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
ti-chi-bot
Bot
added
size/XL
|
[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 |
📝 WalkthroughWalkthroughThis PR adds LATERAL join construction infrastructure and improves correlated column extraction in the TiDB planner. It introduces a new ChangesLATERAL Join Planning and Correlation Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/planner/core/logical_plan_builder.go`:
- Around line 469-503: Remove the git conflict markers (<<<<<<<, =======,
>>>>>>>) and keep the intended updated logic that clones and rewrites output
field names when x.AsName.L != "" instead of the old loop; specifically, retain
the clonedNames creation and per-name cloning that sets DBName based on
isTableName and assigns TblName = x.AsName (and the comment about clearing
DBName for derived tables), then replace usages of p.OutputNames() with
clonedNames and ensure plannerSelectBlockAsName logic that sets leading() still
runs; apply the same removal of conflict markers and retention of the new
cloning logic in the other affected regions (around the other ranges mentioned)
so the file parses and the LATERAL/alias changes are fully applied.
- Around line 626-687: buildJoin currently ignores LATERAL: it always builds the
right side as a normal result set and never pushes the left schema, so
containsLateralTableSource/isImmediateLateralTableSource and buildLateralJoin
remain unused. Update buildJoin to (1) detect whether the right ResultSetNode
contains any nested LATERAL (use containsLateralTableSource) and whether the
immediate right is a LATERAL TableSource (use isImmediateLateralTableSource);
(2) if containsLateralTableSource returns true, push the left/outer schema into
the binder/environment before constructing the rightPlan so right-side LATERAL
references can bind; and (3) after building rightPlan, if
isImmediateLateralTableSource or the rightPlan has correlations, route to
buildLateralJoin to produce a LogicalApply instead of a regular join. Ensure
parenthesized single-table joins (Join{Left:..., Right:nil}) are handled by the
same unwrapping logic already in the helpers so nested forms and set-ops are
covered.
- Around line 474-503: The code constructs clonedNames to avoid mutating shared
types.FieldName instances but never applies them to the plan; call
p.SetOutputNames(clonedNames) after the clonedNames slice is populated (in the
same block that builds clonedNames, e.g. right after the loop that creates
clonedNames) so the alias/TblName changes become visible to the rest of the
builder; ensure you reference the existing p.OutputNames() usage and replace the
plan's output names via p.SetOutputNames(clonedNames) so downstream logic
(including plannerSelectBlockAsName and later passes) sees the updated names.
In `@pkg/planner/core/rule_decorrelate.go`:
- Around line 142-157: This hunk contains leftover git conflict markers and must
be cleaned up: remove the conflict markers and keep the updated branch logic
that defines outerSchema and uses it when extracting correlated columns;
specifically, in the function handling apply (references: apply, outerPlan,
apply.IsLateral, findJoinFullSchema), compute outerSchema as shown (default to
outerPlan.Schema(), override with fullSchema when IsLateral and
findJoinFullSchema returns non-nil) and call
coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema); ensure
all later references to outerSchema (lines that depend on outerSchema) compile
after the change.
- Around line 145-156: The later recomputation of apply.CorCols in the
aggregation branch currently uses apply.Children()[0].Schema(), which loses
correlated columns for lateral applies over USING/NATURAL joins; update those
recomputation sites to propagate and use the previously-determined outerSchema
(the one computed via findJoinFullSchema when apply.IsLateral) instead of
calling .Children()[0].Schema(), ensuring apply.CorCols =
coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema) is used
wherever CorCols is recomputed in rule_decorrelate.go (e.g., the aggregation
branch and the other overwrite site) so hidden correlated columns are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3df316f3-1039-4f7e-94b9-3ed29ed8eb02
📒 Files selected for processing (3)
pkg/planner/core/logical_plan_builder.gopkg/planner/core/operator/logicalop/logical_cte.gopkg/planner/core/rule_decorrelate.go
Comment on lines
+469
to
+503
| <<<<<<< HEAD | ||
| for _, name := range p.OutputNames() { | ||
| if name.Hidden { | ||
| continue | ||
| ======= | ||
| if x.AsName.L != "" { | ||
| // Clone output names before modifying to avoid mutating shared structs. | ||
| // This is critical for CTEs whose output names are shared across multiple | ||
| // references — in-place mutation would corrupt other consumers. | ||
| clonedNames := make([]*types.FieldName, len(p.OutputNames())) | ||
| for i, name := range p.OutputNames() { | ||
| if name.Hidden { | ||
| clonedNames[i] = name | ||
| continue | ||
| } | ||
| // Clone the field name and update table name. | ||
| // For derived tables, clear DBName so that error messages (e.g. only_full_group_by) | ||
| // show "alias.col" not "db.alias.col". The current-database qualifier needed for | ||
| // hint generation (leading()) is set separately on plannerSelectBlockAsName below. | ||
| // For base-table aliases (isTableName), inherit DBName for DEFAULT() resolution. | ||
| dbName := ast.NewCIStr("") | ||
| if isTableName { | ||
| dbName = name.DBName | ||
| } | ||
| clonedNames[i] = &types.FieldName{ | ||
| DBName: dbName, | ||
| OrigTblName: name.OrigTblName, | ||
| OrigColName: name.OrigColName, | ||
| TblName: x.AsName, | ||
| ColName: name.ColName, | ||
| NotExplicitUsable: name.NotExplicitUsable, | ||
| Redundant: name.Redundant, | ||
| Hidden: name.Hidden, | ||
| } | ||
| >>>>>>> 72f9da0b023 (planner: lateral join quality updates (#67482)) |
There was a problem hiding this comment.
Resolve the cherry-pick conflict markers before merging.
<<<<<<<, =======, and >>>>>>> are still in the file, so this revision will not parse or compile. They also leave the aliasing and LATERAL changes only half-applied.
Also applies to: 591-689, 838-983
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/logical_plan_builder.go` around lines 469 - 503, Remove the
git conflict markers (<<<<<<<, =======, >>>>>>>) and keep the intended updated
logic that clones and rewrites output field names when x.AsName.L != "" instead
of the old loop; specifically, retain the clonedNames creation and per-name
cloning that sets DBName based on isTableName and assigns TblName = x.AsName
(and the comment about clearing DBName for derived tables), then replace usages
of p.OutputNames() with clonedNames and ensure plannerSelectBlockAsName logic
that sets leading() still runs; apply the same removal of conflict markers and
retention of the new cloning logic in the other affected regions (around the
other ranges mentioned) so the file parses and the LATERAL/alias changes are
fully applied.
Comment on lines
+474
to
+503
| if x.AsName.L != "" { | ||
| // Clone output names before modifying to avoid mutating shared structs. | ||
| // This is critical for CTEs whose output names are shared across multiple | ||
| // references — in-place mutation would corrupt other consumers. | ||
| clonedNames := make([]*types.FieldName, len(p.OutputNames())) | ||
| for i, name := range p.OutputNames() { | ||
| if name.Hidden { | ||
| clonedNames[i] = name | ||
| continue | ||
| } | ||
| // Clone the field name and update table name. | ||
| // For derived tables, clear DBName so that error messages (e.g. only_full_group_by) | ||
| // show "alias.col" not "db.alias.col". The current-database qualifier needed for | ||
| // hint generation (leading()) is set separately on plannerSelectBlockAsName below. | ||
| // For base-table aliases (isTableName), inherit DBName for DEFAULT() resolution. | ||
| dbName := ast.NewCIStr("") | ||
| if isTableName { | ||
| dbName = name.DBName | ||
| } | ||
| clonedNames[i] = &types.FieldName{ | ||
| DBName: dbName, | ||
| OrigTblName: name.OrigTblName, | ||
| OrigColName: name.OrigColName, | ||
| TblName: x.AsName, | ||
| ColName: name.ColName, | ||
| NotExplicitUsable: name.NotExplicitUsable, | ||
| Redundant: name.Redundant, | ||
| Hidden: name.Hidden, | ||
| } | ||
| >>>>>>> 72f9da0b023 (planner: lateral join quality updates (#67482)) |
There was a problem hiding this comment.
Publish the cloned output names back to the plan.
clonedNames is built here but never passed to p.SetOutputNames(...). That stops the shared-FieldName mutation, but it also means the alias never becomes visible to the rest of the builder.
Suggested fix
if x.AsName.L != "" {
// Clone output names before modifying to avoid mutating shared structs.
// This is critical for CTEs whose output names are shared across multiple
// references — in-place mutation would corrupt other consumers.
clonedNames := make([]*types.FieldName, len(p.OutputNames()))
for i, name := range p.OutputNames() {
if name.Hidden {
clonedNames[i] = name
continue
}
// Clone the field name and update table name.
// For derived tables, clear DBName so that error messages (e.g. only_full_group_by)
// show "alias.col" not "db.alias.col". The current-database qualifier needed for
// hint generation (leading()) is set separately on plannerSelectBlockAsName below.
// For base-table aliases (isTableName), inherit DBName for DEFAULT() resolution.
dbName := ast.NewCIStr("")
if isTableName {
dbName = name.DBName
}
clonedNames[i] = &types.FieldName{
DBName: dbName,
OrigTblName: name.OrigTblName,
OrigColName: name.OrigColName,
TblName: x.AsName,
ColName: name.ColName,
NotExplicitUsable: name.NotExplicitUsable,
Redundant: name.Redundant,
Hidden: name.Hidden,
}
}
+ p.SetOutputNames(clonedNames)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if x.AsName.L != "" { | |
| // Clone output names before modifying to avoid mutating shared structs. | |
| // This is critical for CTEs whose output names are shared across multiple | |
| // references — in-place mutation would corrupt other consumers. | |
| clonedNames := make([]*types.FieldName, len(p.OutputNames())) | |
| for i, name := range p.OutputNames() { | |
| if name.Hidden { | |
| clonedNames[i] = name | |
| continue | |
| } | |
| // Clone the field name and update table name. | |
| // For derived tables, clear DBName so that error messages (e.g. only_full_group_by) | |
| // show "alias.col" not "db.alias.col". The current-database qualifier needed for | |
| // hint generation (leading()) is set separately on plannerSelectBlockAsName below. | |
| // For base-table aliases (isTableName), inherit DBName for DEFAULT() resolution. | |
| dbName := ast.NewCIStr("") | |
| if isTableName { | |
| dbName = name.DBName | |
| } | |
| clonedNames[i] = &types.FieldName{ | |
| DBName: dbName, | |
| OrigTblName: name.OrigTblName, | |
| OrigColName: name.OrigColName, | |
| TblName: x.AsName, | |
| ColName: name.ColName, | |
| NotExplicitUsable: name.NotExplicitUsable, | |
| Redundant: name.Redundant, | |
| Hidden: name.Hidden, | |
| } | |
| >>>>>>> 72f9da0b023 (planner: lateral join quality updates (#67482)) | |
| if x.AsName.L != "" { | |
| // Clone output names before modifying to avoid mutating shared structs. | |
| // This is critical for CTEs whose output names are shared across multiple | |
| // references — in-place mutation would corrupt other consumers. | |
| clonedNames := make([]*types.FieldName, len(p.OutputNames())) | |
| for i, name := range p.OutputNames() { | |
| if name.Hidden { | |
| clonedNames[i] = name | |
| continue | |
| } | |
| // Clone the field name and update table name. | |
| // For derived tables, clear DBName so that error messages (e.g. only_full_group_by) | |
| // show "alias.col" not "db.alias.col". The current-database qualifier needed for | |
| // hint generation (leading()) is set separately on plannerSelectBlockAsName below. | |
| // For base-table aliases (isTableName), inherit DBName for DEFAULT() resolution. | |
| dbName := ast.NewCIStr("") | |
| if isTableName { | |
| dbName = name.DBName | |
| } | |
| clonedNames[i] = &types.FieldName{ | |
| DBName: dbName, | |
| OrigTblName: name.OrigTblName, | |
| OrigColName: name.OrigColName, | |
| TblName: x.AsName, | |
| ColName: name.ColName, | |
| NotExplicitUsable: name.NotExplicitUsable, | |
| Redundant: name.Redundant, | |
| Hidden: name.Hidden, | |
| } | |
| } | |
| p.SetOutputNames(clonedNames) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/logical_plan_builder.go` around lines 474 - 503, The code
constructs clonedNames to avoid mutating shared types.FieldName instances but
never applies them to the plan; call p.SetOutputNames(clonedNames) after the
clonedNames slice is populated (in the same block that builds clonedNames, e.g.
right after the loop that creates clonedNames) so the alias/TblName changes
become visible to the rest of the builder; ensure you reference the existing
p.OutputNames() usage and replace the plan's output names via
p.SetOutputNames(clonedNames) so downstream logic (including
plannerSelectBlockAsName and later passes) sees the updated names.
Comment on lines
+626
to
+687
| // containsLateralTableSource checks if a ResultSetNode contains a LATERAL table source | ||
| // anywhere in its subtree. Used only to decide whether to push outerSchemas before | ||
| // building the right side, so nested LATERAL sources can resolve outer columns. | ||
| func containsLateralTableSource(node ast.ResultSetNode) bool { | ||
| switch n := node.(type) { | ||
| case *ast.TableSource: | ||
| if n.Lateral { | ||
| return true | ||
| } | ||
| // Descend into the inner source (derived table / set-op) so nested | ||
| // LATERAL inside a subquery or set-op used as a table source is detected. | ||
| return containsLateralTableSource(n.Source) | ||
| case *ast.Join: | ||
| // For parenthesized single table refs, the parser creates Join{Left: TableSource, Right: nil} | ||
| if n.Right == nil { | ||
| return containsLateralTableSource(n.Left) | ||
| } | ||
| // Check both sides for nested LATERAL | ||
| return containsLateralTableSource(n.Left) || containsLateralTableSource(n.Right) | ||
| case *ast.SelectStmt: | ||
| // Descend into the FROM clause of a derived subquery. | ||
| if n.From != nil { | ||
| return containsLateralTableSource(n.From.TableRefs) | ||
| } | ||
| return false | ||
| case *ast.SetOprStmt: | ||
| // Check each operand in the UNION/INTERSECT/EXCEPT list. | ||
| if n.SelectList != nil { | ||
| for _, sel := range n.SelectList.Selects { | ||
| if rs, ok := sel.(ast.ResultSetNode); ok && containsLateralTableSource(rs) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // isImmediateLateralTableSource checks whether the top-level ResultSetNode is itself a | ||
| // LATERAL TableSource, without recursing into both sides of a multi-table nested join. | ||
| // A parenthesized single-table form (Join{Left: source, Right: nil}) is transparent and | ||
| // is unwrapped, but a multi-table join on the right side is not itself a LATERAL source. | ||
| // This is used after rightPlan is built to decide whether to produce a LogicalApply: | ||
| // only the immediate right operand being LATERAL (or actual correlations in rightPlan) | ||
| // should trigger that, not a LATERAL nested deeper in the right subtree. | ||
| func isImmediateLateralTableSource(node ast.ResultSetNode) bool { | ||
| switch n := node.(type) { | ||
| case *ast.TableSource: | ||
| return n.Lateral | ||
| case *ast.Join: | ||
| // Parenthesized single-table ref: parser creates Join{Left: source, Right: nil} | ||
| if n.Right == nil { | ||
| return isImmediateLateralTableSource(n.Left) | ||
| } | ||
| // A multi-table join is not itself a single LATERAL source | ||
| return false | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Wire the new LATERAL path into buildJoin.
These helpers and buildLateralJoin are dead code right now. buildJoin still builds the right side as an ordinary result set and never pushes the left schema before doing so, so right-side LATERAL references still cannot bind to left-side columns and the new LogicalApply path never runs.
Expected integration points
leftPlan, err := b.buildResultSetNode(ctx, joinNode.Left, false)
if err != nil {
return nil, err
}
+ if containsLateralTableSource(joinNode.Right) {
+ outerSchema := leftPlan.Schema()
+ outerNames := leftPlan.OutputNames()
+ if fullSchema, fullNames := findJoinFullSchema(leftPlan); fullSchema != nil {
+ outerSchema, outerNames = fullSchema, fullNames
+ }
+ b.outerSchemas = append(b.outerSchemas, outerSchema)
+ b.outerNames = append(b.outerNames, outerNames)
+ defer func() {
+ b.outerSchemas = b.outerSchemas[:len(b.outerSchemas)-1]
+ b.outerNames = b.outerNames[:len(b.outerNames)-1]
+ }()
+ }
+
rightPlan, err := b.buildResultSetNode(ctx, joinNode.Right, false)
if err != nil {
return nil, err
}
+
+ if isImmediateLateralTableSource(joinNode.Right) {
+ return b.buildLateralJoin(ctx, leftPlan, rightPlan, joinNode)
+ }Also applies to: 840-980
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/logical_plan_builder.go` around lines 626 - 687, buildJoin
currently ignores LATERAL: it always builds the right side as a normal result
set and never pushes the left schema, so
containsLateralTableSource/isImmediateLateralTableSource and buildLateralJoin
remain unused. Update buildJoin to (1) detect whether the right ResultSetNode
contains any nested LATERAL (use containsLateralTableSource) and whether the
immediate right is a LATERAL TableSource (use isImmediateLateralTableSource);
(2) if containsLateralTableSource returns true, push the left/outer schema into
the binder/environment before constructing the rightPlan so right-side LATERAL
references can bind; and (3) after building rightPlan, if
isImmediateLateralTableSource or the rightPlan has correlations, route to
buildLateralJoin to produce a LogicalApply instead of a regular join. Ensure
parenthesized single-table joins (Join{Left:..., Right:nil}) are handled by the
same unwrapping logic already in the helpers so nested forms and set-ops are
covered.
Comment on lines
+142
to
+157
| <<<<<<< HEAD | ||
| apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], apply.Children()[0].Schema()) | ||
| ======= | ||
| // Use FullSchema when outer plan is a USING/NATURAL join, so we capture | ||
| // correlated columns that reference the redundant (merged) join columns. | ||
| // Walk through wrapper operators (e.g., LogicalSelection from ON clauses) | ||
| // to find the underlying LogicalJoin, matching the schema used for name | ||
| // resolution in LATERAL subqueries (see logical_plan_builder.go buildJoin). | ||
| outerSchema := outerPlan.Schema() | ||
| if apply.IsLateral { | ||
| if fullSchema, _ := findJoinFullSchema(outerPlan); fullSchema != nil { | ||
| outerSchema = fullSchema | ||
| } | ||
| } | ||
| apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema) | ||
| >>>>>>> 72f9da0b023 (planner: lateral join quality updates (#67482)) |
There was a problem hiding this comment.
Resolve the cherry-pick conflict in this hunk before merge.
This file still contains conflict markers, so it will not compile. Also, the HEAD side does not define outerSchema, while Lines 172, 212, and 214 now depend on it, so keeping that side would still leave this function broken.
Suggested resolution
-<<<<<<< HEAD
- apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], apply.Children()[0].Schema())
-=======
// Use FullSchema when outer plan is a USING/NATURAL join, so we capture
// correlated columns that reference the redundant (merged) join columns.
// Walk through wrapper operators (e.g., LogicalSelection from ON clauses)
// to find the underlying LogicalJoin, matching the schema used for name
// resolution in LATERAL subqueries (see logical_plan_builder.go buildJoin).
outerSchema := outerPlan.Schema()
if apply.IsLateral {
if fullSchema, _ := findJoinFullSchema(outerPlan); fullSchema != nil {
outerSchema = fullSchema
}
}
apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema)
->>>>>>> 72f9da0b023 (planner: lateral join quality updates (`#67482`))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <<<<<<< HEAD | |
| apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], apply.Children()[0].Schema()) | |
| ======= | |
| // Use FullSchema when outer plan is a USING/NATURAL join, so we capture | |
| // correlated columns that reference the redundant (merged) join columns. | |
| // Walk through wrapper operators (e.g., LogicalSelection from ON clauses) | |
| // to find the underlying LogicalJoin, matching the schema used for name | |
| // resolution in LATERAL subqueries (see logical_plan_builder.go buildJoin). | |
| outerSchema := outerPlan.Schema() | |
| if apply.IsLateral { | |
| if fullSchema, _ := findJoinFullSchema(outerPlan); fullSchema != nil { | |
| outerSchema = fullSchema | |
| } | |
| } | |
| apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema) | |
| >>>>>>> 72f9da0b023 (planner: lateral join quality updates (#67482)) | |
| // Use FullSchema when outer plan is a USING/NATURAL join, so we capture | |
| // correlated columns that reference the redundant (merged) join columns. | |
| // Walk through wrapper operators (e.g., LogicalSelection from ON clauses) | |
| // to find the underlying LogicalJoin, matching the schema used for name | |
| // resolution in LATERAL subqueries (see logical_plan_builder.go buildJoin). | |
| outerSchema := outerPlan.Schema() | |
| if apply.IsLateral { | |
| if fullSchema, _ := findJoinFullSchema(outerPlan); fullSchema != nil { | |
| outerSchema = fullSchema | |
| } | |
| } | |
| apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/rule_decorrelate.go` around lines 142 - 157, This hunk
contains leftover git conflict markers and must be cleaned up: remove the
conflict markers and keep the updated branch logic that defines outerSchema and
uses it when extracting correlated columns; specifically, in the function
handling apply (references: apply, outerPlan, apply.IsLateral,
findJoinFullSchema), compute outerSchema as shown (default to
outerPlan.Schema(), override with fullSchema when IsLateral and
findJoinFullSchema returns non-nil) and call
coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema); ensure
all later references to outerSchema (lines that depend on outerSchema) compile
after the change.
Comment on lines
+145
to
+156
| // Use FullSchema when outer plan is a USING/NATURAL join, so we capture | ||
| // correlated columns that reference the redundant (merged) join columns. | ||
| // Walk through wrapper operators (e.g., LogicalSelection from ON clauses) | ||
| // to find the underlying LogicalJoin, matching the schema used for name | ||
| // resolution in LATERAL subqueries (see logical_plan_builder.go buildJoin). | ||
| outerSchema := outerPlan.Schema() | ||
| if apply.IsLateral { | ||
| if fullSchema, _ := findJoinFullSchema(outerPlan); fullSchema != nil { | ||
| outerSchema = fullSchema | ||
| } | ||
| } | ||
| apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan, outerSchema) |
There was a problem hiding this comment.
Propagate outerSchema to the later CorCols recomputation path as well.
This change fixes the initial apply.CorCols extraction for lateral applies, but the aggregation branch still overwrites it with apply.Children()[0].Schema() at Lines 326 and 379. For a lateral apply over a USING/NATURAL join, that can drop correlated columns hidden from the output schema and make the len(apply.CorCols) == 0 check wrongly fire in that branch.
Suggested follow-up
outerSchema := outerPlan.Schema()
if apply.IsLateral {
if fullSchema, _ := findJoinFullSchema(outerPlan); fullSchema != nil {
outerSchema = fullSchema
}
}
@@
- apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], apply.Children()[0].Schema())
+ apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(apply.Children()[1], outerSchema)
@@
- 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/planner/core/rule_decorrelate.go` around lines 145 - 156, The later
recomputation of apply.CorCols in the aggregation branch currently uses
apply.Children()[0].Schema(), which loses correlated columns for lateral applies
over USING/NATURAL joins; update those recomputation sites to propagate and use
the previously-determined outerSchema (the one computed via findJoinFullSchema
when apply.IsLateral) instead of calling .Children()[0].Schema(), ensuring
apply.CorCols = coreusage.ExtractCorColumnsBySchema4LogicalPlan(innerPlan,
outerSchema) is used wherever CorCols is recomputed in rule_decorrelate.go
(e.g., the aggregation branch and the other overwrite site) so hidden correlated
columns are preserved.
|
@ti-chi-bot: 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. |
|
@ti-chi-bot: 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
2 participants
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.
This is an automated cherry-pick of #67482
What problem does this PR solve?
Issue Number: ref #40328
Problem Summary:
What changed and how does it work?
During a cherry pick of lateral join to a prior branch - new AI reviews found additional issues with the original implementation. These are addressed in this PR.
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
Bug Fixes
Improvements