-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
planner/core,binloginfo: add a projection to fix 'delete from join' schema #8805
Changes from 4 commits
288794f
b61e780
a5fd91d
e08a3ff
242e0f5
3a569d6
ac8e2d8
a630b4c
302c15b
216a43b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2328,6 +2328,7 @@ func (b *PlanBuilder) buildDelete(delete *ast.DeleteStmt) (Plan, error) { | |
if err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
oldSchema := p.Schema() | ||
|
||
if sel.Where != nil { | ||
p, err = b.buildSelection(p, sel.Where, nil) | ||
|
@@ -2350,6 +2351,15 @@ func (b *PlanBuilder) buildDelete(delete *ast.DeleteStmt) (Plan, error) { | |
} | ||
} | ||
|
||
// Add a projection for the following case, otherwise the final schema will be the schema of the join. | ||
// delete from t where a in (select ...) or b in (select ...) | ||
if oldLen := oldSchema.Len(); oldLen != p.Schema().Len() { | ||
proj := LogicalProjection{Exprs: expression.Column2Exprs(p.Schema().Columns[:oldLen])}.Init(b.ctx) | ||
proj.SetChildren(p) | ||
proj.SetSchema(oldSchema.Clone()) | ||
p = proj | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add projection for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, update have the same problem There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I simply add a projection but the CI failed. |
||
var tables []*ast.TableName | ||
if delete.Tables != nil { | ||
tables = delete.Tables.Tables | ||
|
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.
it seems to delete target table isn't always the first table, MySQL support delete multiple tables syntax, although I never use that https://dev.mysql.com/doc/refman/5.7/en/delete.html 🤣
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.
Yes, we should consider the case of multi-table deletion.
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.
Delete from A where ... // delete single table
Delete A B ... from ... // delete multiple table
Delete from A B using ... // delete multiple table
Multiple tables case is more complex. This PR will only handle the single table cases.
Let's have a look what's the bug actually:
select from A where A join B ...
there is a projection on A join Bbut for
delete from A where A join B ...
, the projection is missing.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.
What output should it be if it's a
multiple tables deleting
?