plan, executor: no longer treat dml as logical/physical plan. #5230
Conversation
|
Why should we do this? |
|
@hanfei1991 |
|
@shenli make code cleaner, better and stronger. |
|
@coocood I don't understand what you mean. |
|
/run-all-tests |
executor/builder.go
Outdated
| tblID2table[id], _ = b.is.TableByID(id) | ||
| } | ||
| return &DeleteExec{ | ||
| baseExecutor: newBaseExecutor(nil, b.ctx), | ||
| SelectExec: b.build(v.Children()[0]), | ||
| SelectExec: b.build(v.SelectPlan), |
executor/builder.go
Outdated
| tblID2table[id], _ = b.is.TableByID(id) | ||
| } | ||
| return &UpdateExec{ | ||
| baseExecutor: newBaseExecutor(nil, b.ctx), | ||
| SelectExec: b.build(v.Children()[0]), | ||
| SelectExec: b.build(v.SelectPlan), |
There was a problem hiding this comment.
after building, we should check b.err:
selectExec := b.build(v.SelectPlan)
if b.err != nil {
b.err = errors.Trace(b.err)
return nil
}There was a problem hiding this comment.
if b.err is not nil, it's ok to return any executor.
There was a problem hiding this comment.
It's better to discover error as soon as possible so that we can avoid useless executor building.
|
/run-common-test tidb-test=pr/410 |
|
/run-common-test tidb-test=pr/410 |
|
@hanfei1991 Please fix the conflicts. |
| updt.SetSchema(p.Schema()) | ||
| updt.SelectPlan, b.err = doOptimize(b.optFlag, p, b.ctx) |
There was a problem hiding this comment.
If b.err is not nil, we can also return. It's no need to check this.
| @@ -52,6 +52,9 @@ type Plan interface { | |||
| // findColumn finds the column in basePlan's schema. | |||
| // If the column is not in the schema, returns error. | |||
| findColumn(*ast.ColumnName) (*expression.Column, int, error) | |||
|
|
|||
| // ResolveIndices resolves the indices for columns. After doing this, the columns can evaluate the rows by their indices. | |||
| ResolveIndices() | |||
There was a problem hiding this comment.
Insert/Delete/Update also needs resolveIndices
| for _, cols := range p.schema.TblID2Handle { | ||
| for _, col := range cols { | ||
| col.ResolveIndices(p.schema) | ||
| if p.schema != nil { |
There was a problem hiding this comment.
comment for when this check will be false.
There was a problem hiding this comment.
I also don't know when it would fail. It's strange and I will refactor the schema setting in the future.
| if len(v.Children()) > 0 { | ||
| ivs.SelectExec = b.build(v.Children()[0]) | ||
| ivs.SelectExec = b.build(v.SelectPlan) | ||
| if b.err != nil { |
| tblID2table[id], _ = b.is.TableByID(id) | ||
| } | ||
| selExec := b.build(v.SelectPlan) | ||
| if b.err != nil { |
|
@XuHuaiyu PTAL |
| @@ -612,19 +612,19 @@ func (s *testPlanSuite) TestPlanBuilder(c *C) { | |||
| }, | |||
| { | |||
| sql: "update t set t.a = t.a * 1.5 where t.a >= 1000 order by t.a desc limit 10", | |||
| plan: "DataScan(t)->Sel([ge(test.t.a, 1000)])->Sort->Limit->*plan.Update", | |||
| plan: "TableReader(Table(t)->Limit)->Limit->Update", | |||
There was a problem hiding this comment.
Why DataScan is replaced with TableReader?
There was a problem hiding this comment.
We build update's SelectPlan at one time.
|
Should you consider the |
|
@shenli replace is a kind of insert |
|
/run-all-tests |
|
@hanfei1991 CI fail. |
|
/run-common-test tidb-test=pr/414 |
|
/run-common-tests tidb-test=pr/414 |
|
/run-common-test tidb-test=pr/414 |
No description provided.