executor: support Chunk in LimitExec #5200
Conversation
|
/run-all-tests |
util/chunk/chunk.go
Outdated
| if src.isFixed() { | ||
| elemLen := len(src.elemBuf) | ||
| if len(dst.data) < elemLen*(end-begin) { | ||
| dst.data = make([]byte, elemLen*(end-begin), elemLen*(end-begin)) |
There was a problem hiding this comment.
If we make data here, we won't be able to reuse the memory.
util/chunk/chunk.go
Outdated
| if len(dst.nullBitmap) < numBytesInBitmap { | ||
| dst.nullBitmap = make([]byte, numBytesInBitmap) | ||
| } | ||
| for i := begin; i < end; i++ { |
There was a problem hiding this comment.
We should be able to append the bitmap bytes directly, in the case of not aligned, we can shift the bitmap.
util/chunk/chunk.go
Outdated
| col.data = col.data[:(col.length-tailRows)*elemLen] | ||
| } else if col.isVarlen() { | ||
| col.data = col.data[:col.offsets[col.length-tailRows+1]] | ||
| col.offsets = col.offsets[:col.length-tailRows] |
There was a problem hiding this comment.
The length of offsets is larger than col.length by 1.
|
@zz-jason |
util/chunk/chunk_test.go
Outdated
| src.AppendNull(1) | ||
| src.AppendNull(2) | ||
|
|
||
| for i := 0; i < 7; i++ { |
There was a problem hiding this comment.
Why not i < 8 to save the above 6 lines?
executor/executor.go
Outdated
| if e.cursor > e.end { | ||
| batchSize -= e.cursor - e.end | ||
| } | ||
| fmt.Printf("matched, batchSize: %v\n", batchSize) |
executor/executor.go
Outdated
| if batchSize == 0 { | ||
| return nil | ||
| } | ||
| e.cursor += batchSize |
There was a problem hiding this comment.
I think to update the cursor after we append the data is easier to understand.
Then the cursor will never be greater than e.end.
There was a problem hiding this comment.
It's not that hard to understand and if we update cursor after appending data the code will be more or less ugly.
There was a problem hiding this comment.
I think there is a way to make the code easier to understand.
executor/executor.go
Outdated
| return errors.Trace(err) | ||
| } | ||
| e.cursor = 0 | ||
| e.meetFirstBatch = false |
There was a problem hiding this comment.
If the offset is 0, we can set meetFirstBatch to true
util/chunk/chunk.go
Outdated
| } | ||
|
|
||
| // Truncate truncates "tailRows" rows from the tail.. | ||
| func (c *Chunk) Truncate(tailRows int) { |
There was a problem hiding this comment.
I think truncate to length is more commonly used.
executor/executor.go
Outdated
| if batchSize == 0 { | ||
| return nil | ||
| } | ||
| if e.cursor > e.end { |
There was a problem hiding this comment.
Should update cursor before this check.
Seems not covered by tests.
|
@zz-jason |
|
@coocood we can add a session variable to control max chunk capacity, before testing, we can set a small chunk size, like 2. |
|
LGTM |
executor/executor.go
Outdated
| end uint64 | ||
| cursor uint64 | ||
|
|
||
| meetFirstBatch bool |
| @@ -146,6 +149,51 @@ func (c *Chunk) AppendRow(colIdx int, row Row) { | |||
| } | |||
| } | |||
|
|
|||
| // Append appends rows in [begin, end) in another Chunk to a Chunk. | |||
| func (c *Chunk) Append(other *Chunk, begin, end int) { | |||
There was a problem hiding this comment.
should we check the validation of begin and end first.
There was a problem hiding this comment.
no need to, all Chunk's API should not do the validation work.
| } | ||
|
|
||
| // TruncateTo truncates rows from tail to head in a Chunk to "numRows" rows. | ||
| func (c *Chunk) TruncateTo(numRows int) { |
There was a problem hiding this comment.
check validation of numRows?
|
/run-all-tests tidb-test=pr/411 |
|
@XuHuaiyu PTAL |
|
LGTM |
zz-jason
added
status/LGT2
No description provided.