-
Notifications
You must be signed in to change notification settings - Fork 13
Backport 19003 #204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport 19003 #204
Conversation
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
This fixes a bug where copying from an expression that had no type information would cause the target of the copy to have invalid information (as we'd be writing the null value which would prevent proper recalculation). Also added a bunch of unit test cases to cover `CopyExprInfo`. Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR backports a fix from upstream PR vitessio#19003 that addresses issue vitessio#18987, which relates to proper handling of semantic information when copying expressions in the Vitess query planner, particularly affecting derived tables and subqueries.
- Fixed
CopySemanticInfoto validate map keys before copying and to differentiate behavior based on whether the target is a column name - Improved correlated subquery detection to only check for dependencies on outer tables
- Added comprehensive test coverage for semantic information copying behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| go/vt/vtgate/semantics/semantic_table.go | Enhanced CopySemanticInfo method to add validation checks and conditional copying logic based on target expression type |
| go/vt/vtgate/semantics/semantic_table_test.go | Added two comprehensive test functions to verify correct behavior when copying semantic information to both ColName and non-ColName expressions |
| go/vt/vtgate/planbuilder/operators/subquery_builder.go | Fixed correlated subquery detection logic to properly filter dependencies by outerID; added unused createSubqueryFromPath function |
| go/vt/vtgate/planbuilder/testdata/select_cases.json | Updated test expectations with new window function test cases and minor changes to existing test output |
Comments suppressed due to low confidence (1)
go/vt/vtgate/planbuilder/operators/subquery_builder.go:231
- The function
createSubqueryFromPathappears to be a duplicate ofcreateSubquerywith nearly identical implementation. The only difference is howoriginalSqis obtained (line 206 usessqlparser.GetNodeFromPath(original, path)vs line 166 usescloneASTAndSemState(ctx, subq)). However, this function doesn't appear to be called anywhere in the codebase, suggesting it may be dead code that was accidentally included in this backport.
func createSubqueryFromPath(
ctx *plancontext.PlanningContext,
original sqlparser.Expr,
subq *sqlparser.Subquery,
path sqlparser.ASTPath,
outerID semantics.TableSet,
parent sqlparser.Expr,
argName string,
filterType opcode.PulloutOpcode,
isArg bool,
) *SubQuery {
topLevel := ctx.SemTable.EqualsExpr(original, parent)
original = cloneASTAndSemState(ctx, original)
originalSq := sqlparser.GetNodeFromPath(original, path).(*sqlparser.Subquery)
subqID := findTablesContained(ctx, originalSq.Select)
totalID := subqID.Merge(outerID)
sqc := &SubQueryBuilder{totalID: totalID, subqID: subqID, outerID: outerID}
predicates, joinCols := sqc.inspectStatement(ctx, subq.Select)
subqDependencies := ctx.SemTable.RecursiveDeps(subq)
correlated := subqDependencies.KeepOnly(outerID).NotEmpty()
opInner := translateQueryToOp(ctx, subq.Select)
opInner = sqc.getRootOperator(opInner, nil)
return &SubQuery{
FilterType: filterType,
Subquery: opInner,
Predicates: predicates,
Original: original,
ArgName: argName,
originalSubquery: originalSq,
IsArgument: isArg,
TopLevel: topLevel,
JoinColumns: joinCols,
correlated: correlated,
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
This PR backports fix for vitessio#18987
upstream PR vitessio#19003
Description
Related Issue(s)
Checklist
Deployment Notes