plan: get correct columns #5818
Conversation
model/model.go
Outdated
| @@ -114,6 +114,9 @@ type TableInfo struct { | |||
| // Now it only uses for compatibility with the old version that already uses this field. | |||
| OldSchemaID int64 `json:"old_schema_id,omitempty"` | |||
|
|
|||
| publicColumns []*ColumnInfo | |||
| writableColumns []*ColumnInfo | |||
model/model.go
Outdated
| @@ -172,6 +175,45 @@ func (t *TableInfo) GetPkColInfo() *ColumnInfo { | |||
| return nil | |||
| } | |||
|
|
|||
| // Cols returns the columns of the table in public state. | |||
| func (t *TableInfo) Cols() []*ColumnInfo { | |||
| if len(t.publicColumns) > 0 { | |||
There was a problem hiding this comment.
Will this field be modified by multiple goroutines?
ddl/ddl_db_change_test.go
Outdated
| func (s *testStateChangeSuite) TestUpdateOrDelete(c *C) { | ||
| sqls := make([]string, 2) | ||
| sqls[0] = "delete from t where c2 = 'a'" | ||
| sqls[1] = "update t set c2 = 'c2_update' where c2 = 'a'" |
There was a problem hiding this comment.
Yes.
The delete SQL can't use index in a statement directly, so these two statements use drop state table to have the index scan.
There was a problem hiding this comment.
We can add index hint to update statement.
"update t use index(c2) set c2 = 'c2_update' where c2 = 'a'"
There was a problem hiding this comment.
So how to deal with delete statement?
|
/run-all-tests tidb-test=release-1.0 tidb-private-test=release-1.0 tikv=release-1.0 |
| _, err = se.Execute("use test_db_state") | ||
| c.Assert(err, IsNil) | ||
| callback.OnJobUpdatedExported = func(job *model.Job) { | ||
| if job.SchemaState == prevState || checkErr != nil || times >= 3 { |
There was a problem hiding this comment.
It is used for excluding the effects of other DDL statements.
Make the testing stable.
|
The original problem is the mismatch of the column position in slice and the column offset. A cleaner way to fix this issue is to change |
|
If we append the new column to the end in non-public state, we don't need this PR #3754 |
plan/plan_to_pb.go
Outdated
| @@ -89,14 +89,18 @@ func (p *PhysicalTableScan) ToPB(ctx context.Context) (*tipb.Executor, error) { | |||
| // ToPB implements PhysicalPlan ToPB interface. | |||
| func (p *PhysicalIndexScan) ToPB(ctx context.Context) (*tipb.Executor, error) { | |||
| columns := make([]*model.ColumnInfo, 0, p.schema.Len()) | |||
| tableColumns := p.Table.Cols() | |||
| if ctx.GetSessionVars().StmtCtx.InUpdateStmt { | |||
There was a problem hiding this comment.
We don't need to check it and get writable columns because all the columns in an index must be public.
There was a problem hiding this comment.
Yes, now the column in an index must be public.
| @@ -48,6 +48,7 @@ func (s *testStateChangeSuite) SetUpSuite(c *C) { | |||
| s.store, err = tikv.NewMockTikvStore() | |||
| c.Assert(err, IsNil) | |||
| tidb.SetSchemaLease(s.lease) | |||
There was a problem hiding this comment.
never mind, I read it wrong.
|
@coocood |
|
LGTM |
|
@shenli PTAL |
| @@ -172,6 +172,22 @@ func (t *TableInfo) GetPkColInfo() *ColumnInfo { | |||
| return nil | |||
| } | |||
|
|
|||
| // Cols returns the columns of the table in public state. | |||
| func (t *TableInfo) Cols() []*ColumnInfo { | |||
There was a problem hiding this comment.
The implementation is not efficient. Could you do some pre-calculation?
|
LGTM |
Use the correct columns in PhysicalIndexScan's ToPB function.
I need use the same columns in function
buildDataSourceandToPB.