Skip to content
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

Merged
merged 10 commits into from
Dec 29, 2018
11 changes: 11 additions & 0 deletions planner/core/logical_plan_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -2381,6 +2381,8 @@ func (b *PlanBuilder) buildDelete(delete *ast.DeleteStmt) (Plan, error) {
if err != nil {
return nil, errors.Trace(err)
}
oldSchema := p.Schema()
oldLen := oldSchema.Len()

if sel.Where != nil {
p, err = b.buildSelection(p, sel.Where, nil)
Expand All @@ -2403,6 +2405,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 !delete.IsMultiTable && oldLen != p.Schema().Len() {
proj := LogicalProjection{Exprs: expression.Column2Exprs(p.Schema().Columns[:oldLen])}.Init(b.ctx)
Copy link
Contributor

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 🤣

Copy link
Member

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.

Copy link
Contributor Author

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 B
but for
delete from A where A join B ..., the projection is missing.

Copy link
Member

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?

proj.SetChildren(p)
proj.SetSchema(oldSchema.Clone())
p = proj
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add projection for Update as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, update have the same problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simply add a projection but the CI failed.
I'll fix delete first, and leave update to somebody else.

var tables []*ast.TableName
if delete.Tables != nil {
tables = delete.Tables.Tables
Expand Down
8 changes: 8 additions & 0 deletions planner/core/logical_plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,14 @@ func (s *testPlanSuite) TestPlanBuilder(c *C) {
sql: "select * from t t1 natural join t t2",
plan: "Join{DataScan(t1)->DataScan(t2)}->Projection",
},
{
sql: "delete from t where a in (select b from t where c = 666) or b in (select a from t where c = 42)",
// Note the Projection before Delete: the final schema should be the schema of
// table t rather than Join.
// If this schema is not set correctly, table.RemoveRecord would fail when adding
// binlog columns, because the schema and data are not consistent.
plan: "LeftHashJoin{LeftHashJoin{TableReader(Table(t))->IndexLookUp(Index(t.c_d_e)[[666,666]], Table(t))}(test.t.a,test.t.b)->IndexReader(Index(t.c_d_e)[[42,42]])}(test.t.b,test.t.a)->Sel([or(6_aux_0, 10_aux_0)])->Projection->Delete",
},
}
for _, ca := range tests {
comment := Commentf("for %s", ca.sql)
Expand Down
16 changes: 15 additions & 1 deletion sessionctx/binloginfo/binloginfo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,8 @@ func (s *testBinlogSuite) TearDownSuite(c *C) {
s.ddl.Stop()
s.serv.Stop()
os.Remove(s.unixFile)
s.store.Close()
s.domain.Close()
s.store.Close()
}

func (s *testBinlogSuite) TestBinlog(c *C) {
Expand Down Expand Up @@ -419,3 +419,17 @@ func (s *testBinlogSuite) TestPartitionedTable(c *C) {
c.Assert(tids[i], Equals, tids[0])
}
}

func (s *testBinlogSuite) TestDeleteSchema(c *C) {
tk := testkit.NewTestKit(c, s.store)
tk.MustExec("use test")
tk.MustExec("CREATE TABLE `b1` (`id` int(11) NOT NULL AUTO_INCREMENT, `job_id` varchar(50) NOT NULL, `split_job_id` varchar(30) DEFAULT NULL, PRIMARY KEY (`id`), KEY `b1` (`job_id`)) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;")
tk.MustExec("CREATE TABLE `b2` (`id` int(11) NOT NULL AUTO_INCREMENT, `job_id` varchar(50) NOT NULL, `batch_class` varchar(20) DEFAULT NULL, PRIMARY KEY (`id`), UNIQUE KEY `bu` (`job_id`)) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4")
tk.MustExec("insert into b2 (job_id, batch_class) values (2, 'TEST');")
tk.MustExec("insert into b1 (job_id) values (2);")

// This test cover a bug that the final schema and the binlog row inconsistent.
// The final schema of this SQL should be the schema of table b1, rather than the schema of join result.
tk.MustExec("delete from b1 where job_id in (select job_id from b2 where batch_class = 'TEST') or split_job_id in (select job_id from b2 where batch_class = 'TEST');")
lysu marked this conversation as resolved.
Show resolved Hide resolved
tk.MustExec("delete b1 from b2 right join b1 on b1.job_id = b2.job_id and batch_class = 'TEST';")
}