-
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
executor: DeleteExec suport NextChunk #5368
Conversation
executor/write.go
Outdated
// NextChunk implements the Executor NextChunk interface. | ||
func (e *DeleteExec) NextChunk(goCtx goctx.Context, chk *chunk.Chunk) error { | ||
chk.Reset() | ||
_, err := e.exec(goCtx) |
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.
Should avoid calling SelectExec.Next
when NextChunk
is called.
executor/builder.go
Outdated
@@ -863,13 +863,15 @@ func (b *executorBuilder) buildDelete(v *plan.Delete) Executor { | |||
b.err = errors.Trace(b.err) | |||
return nil | |||
} | |||
return &DeleteExec{ | |||
deleteExec := &DeleteExec{ | |||
baseExecutor: newBaseExecutor(nil, b.ctx), |
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.
make selExec
a child.
if e.finished { | ||
return nil | ||
} | ||
defer func() { |
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.
e.finished = true
is ok
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.
e.finished must set after the exec finished, so use defer is more reasonable.
executor/write.go
Outdated
break | ||
} | ||
|
||
for rowIdx := 0; rowIdx < chk.NumRows(); rowIdx++ { |
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.
for chunkRow := chk.Begin(); chunkRow != chk.End(); chunkRow = chunkRow.Next() {
}
executor/write.go
Outdated
batchDelete := e.ctx.GetSessionVars().BatchDelete && !e.ctx.GetSessionVars().InTxn() | ||
fields := e.children[0].Schema().GetTypes() | ||
for { | ||
chk := chunk.NewChunk(fields) |
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.
chk := e.children[0].newChunk()
executor/write.go
Outdated
tblRowMap := make(tableRowMapType) | ||
fields := e.children[0].Schema().GetTypes() | ||
for { | ||
chk := chunk.NewChunk(fields) |
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.
ditto
executor/write.go
Outdated
break | ||
} | ||
|
||
for rowIdx := 0; rowIdx < chk.NumRows(); rowIdx++ { |
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.
ditto
@@ -901,13 +901,15 @@ func (b *executorBuilder) buildDelete(v *plan.Delete) Executor { | |||
b.err = errors.Trace(b.err) | |||
return nil | |||
} | |||
return &DeleteExec{ | |||
baseExecutor: newBaseExecutor(nil, b.ctx), | |||
deleteExec := &DeleteExec{ |
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.
can we un-export SelectExec
, Tables
and IsMultiTable
?
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.
we should modify it in another pr.
break | ||
} | ||
|
||
for joinedChunkRow := chk.Begin(); joinedChunkRow != chk.End(); joinedChunkRow = joinedChunkRow.Next() { |
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 does joinedChunkRow
mean ?
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 multi tables actually delete the data from joined tables.
executor/write.go
Outdated
type tblColPosInfo struct { | ||
tblID int64 | ||
colBeginIndex int | ||
colEndIndex int | ||
handleIndex int | ||
} | ||
|
||
// tableRowMapType is a map for unique (Table, Row) pair. | ||
type tableRowMapType map[int64]map[int64][]types.Datum |
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.
add a comment to explain what the key(int64
) and value(map[int64][]types.Datum
) represent for.
executor/write.go
Outdated
}() | ||
|
||
if e.IsMultiTable { | ||
return e.deleteMultiTablesByChunk(goCtx) |
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.
errors.Trace(e.deleteMultiTablesByChunk(goCtx))
executor/write.go
Outdated
if e.IsMultiTable { | ||
return e.deleteMultiTablesByChunk(goCtx) | ||
} | ||
return e.deleteSingleTableByChunk(goCtx) |
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.
ditto
…to support_deleteexec_chunk
…to support_deleteexec_chunk
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.
LGTM
@coocood PTAL |
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.
LGTM
/run-all-tests |
/run-common-test |
to #5261