executor: support chunk for Sort #5221
Conversation
executor/sort.go
Outdated
| fields := e.schema.GetTypes() | ||
| var totalCount int | ||
| for { | ||
| chk := chunk.NewChunk(fields) |
There was a problem hiding this comment.
chk := e.children[0].newChunk()There was a problem hiding this comment.
As there are many chunks to create, reuse one fields can save memory allocation.
util/chunk/compare.go
Outdated
| ) | ||
|
|
||
| // CompareFunc is a function to compare the two values in Row, the two columns must have the same type. | ||
| type CompareFunc func(a Row, aCol int, b Row, bCol int) int |
There was a problem hiding this comment.
- interface will be more efficient than function pointer
- s/a/lhs/, s/b/rhs/
There was a problem hiding this comment.
I just did a micro benchmark, the result shows that function pointer is faster.
There was a problem hiding this comment.
The preview micro benchmark didn't define the function as a type, I did it again with function type and interface is slightly faster
| @@ -679,6 +679,7 @@ func (b *executorBuilder) buildSort(v *plan.Sort) Executor { | |||
| ByItems: v.ByItems, | |||
| schema: v.Schema(), | |||
| } | |||
| sortExec.supportChk = true | |||
There was a problem hiding this comment.
while building child:
childExec := b.build(v.Children()[0])
if b.err != nil {
b.err = errors.Trace(b.err)
return nil
}
executor/sort.go
Outdated
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| sort.Slice(e.rowPointers, e.newLess) |
There was a problem hiding this comment.
golang/go#22504
Seems sometimes sort.Slice is slower than sort.Sort, can we do some experiment on sort.Sort vs. sort.Slice ?
There was a problem hiding this comment.
I did a micro benchmark with the rowPointer type slice, and the result show sort.Slice is faster.
|
@zz-jason |
| lBit := types.BinaryLiteral(l.GetBytes(lCol)) | ||
| rBit := types.BinaryLiteral(r.GetBytes(rCol)) | ||
| lUint, err := lBit.ToInt() | ||
| terror.Log(err) |
There was a problem hiding this comment.
We log the error to pass errcheck, we never get this error or there is data corruption.
There was a problem hiding this comment.
Just for my personal culture, why simply not replace this:
lUint, err := lBit.ToInt()
terror.Log(err)
by this:
lUint, _ := lBit.ToInt()
Thank's in advance 😉
executor/sort.go
Outdated
| totalCount int | ||
| } | ||
|
|
||
| type rowPointer struct { |
executor/sort.go
Outdated
|
|
||
| // keyColumns is an optimization that when all by items are column, we don't need to evaluate them to keyChunks. | ||
| // just use the row chunks instead. | ||
| keyColumns []int |
There was a problem hiding this comment.
Add comment for the following attributes.
| for i, colIdx := range e.keyColumns { | ||
| cmpFunc := e.keyCmpFuncs[i] | ||
| cmp := cmpFunc(rowI, colIdx, rowJ, colIdx) | ||
| if e.ByItems[i].Desc { |
There was a problem hiding this comment.
How about passing bool desc as one of the parameter of the comparers ?
There was a problem hiding this comment.
Sometimes we only need to compare equal, Desc is not needed.
|
|
||
| // rowPointer stores the address of a row in rowChunks by its chunk index and row index. | ||
| type rowPointer struct { | ||
| chkIdx uint32 |
There was a problem hiding this comment.
How about using int, thus we can avoid many manual type conversions ?
There was a problem hiding this comment.
The rowPointers slice need to allocate large contiguous memory, we should make the size as small as possible.
|
LGTM |
|
/run-all-tests |
|
/run-unit-test |
No description provided.