executor: add batch copy to inner join, left and right outer join. #7493

crazycs520 · 2018-08-27T00:20:17Z

What problem does this PR solve?

Use vectorize filter and batch copy.

here is benchmark of batch copy VS copy one by one:

ᐅ go test -bench="BenchmarkChunk.*Row"  -run=xx -benchtime="3s" -count=3
goos: linux
goarch: amd64
pkg: github.com/pingcap/tidb/util/chunk
BenchmarkCopySelectedJoinRows     100000             24923 ns/op               0 B/op          0 allocs/op
BenchmarkCopySelectedJoinRows     100000             24639 ns/op               0 B/op          0 allocs/op
BenchmarkAppendSelectedRow         50000             57097 ns/op               0 B/op          0 allocs/op
BenchmarkAppendSelectedRow         50000             57146 ns/op               0 B/op          0 allocs/op

#1
sql1: select count(*) from tid1 inner join tid2 where tid1.id!=tid2.id;
sql2: select count(*) from tid1 inner join tid2 where tid1.id>tid2.id;

create table t1 (id int);
create table t2 (id int);
# t1 data as below:
+-------+
| id    |
+-------+
| 0     |
| 1     |
| 2     |
| ...   |
| ...   |
| ...   |
| 10000 |
+-------+

# t2 data as below:
+-------+
| id    |
+-------+
| 10000 |
| 10001 |
| 10002 |
| ...   |
| ...   |
| ...   |
| 20000 |
+-------+

#2
sql3: select count(*) from t1,t2 where t1.id != t2.id and t1.name != t2.name and t1.addr != t2.addr and t1.course != t2.course;
sql4: select count(*) from t1,t2 where t1.id != t2.id and t1.name != t2.name and t1.addr != t2.addr and t1.course != t2.course and t1.addr > t2.course;

create table t1 (id int, name varchar(30),addr varchar(30), course varchar(30))
create table t2 (id int, name varchar(30),addr varchar(30), course varchar(30))

# both t1, t2 data as below:
+------+----------------+-------------------+------------------+
| id   | name           | addr              | course           |
+------+----------------+-------------------+------------------+
| 0    | name_abcd_0    | address_abcd_0    | course_abcd_0    |
| 1    | name_abcd_1    | address_abcd_1    | course_abcd_1    |
| 2    | name_abcd_2    | address_abcd_2    | course_abcd_2    |
| 3    | name_abcd_3    | address_abcd_3    | course_abcd_3    |
...
10000 rows

//on my Mac, only TiDB , chunk_size=1024

	Sql1	Sql2	Sql3	Sql4
master	5.4	3.6	13.6	11.2
master + batch copy	4.5	3.6	12.1	11
Shadow	6	3.2	14.1	11
Shadow + batch	4.6	3.7	12.1	11

//on server, TiDB + PD + TiKV , chunk_size=1024

	Sql1	Sql2	Sql3	Sql4
master	7.5	6.1	18.5	15.6
master + batch copy	6	4.7	15.6	14
Shadow	11	5.8	22.7	11.5
Shadow + batch	6.2	6.78	14	16

crazycs520 · 2018-08-27T00:31:49Z

/run-all-tests

crazycs520 · 2018-08-28T10:22:13Z

/run-all-tests

crazycs520 · 2018-08-28T14:23:33Z

/run-common-test

…tch-copy

crazycs520 · 2018-08-29T01:16:32Z

/run-common-test

crazycs520 · 2018-08-29T04:49:05Z

/run-all-tests

crazycs520 · 2018-08-29T05:57:54Z

@zz-jason @XuHuaiyu PTAL

…tch-copy

zz-jason · 2018-09-03T05:45:22Z

executor/joiner.go

@@ -158,19 +158,12 @@ func (j *baseJoiner) makeShallowJoinRow(isRightJoin bool, inner, outer chunk.Row
 	j.shallowRow.ShallowCopyPartialRow(inner.Len(), outer)
 }

-func (j *baseJoiner) filter(input, output *chunk.Chunk) (matched bool, err error) {
+func (j *baseJoiner) filter(input, output *chunk.Chunk) (err error) {


Can we make j.selected as a return value for this function? The function caller can directly use the returned []bool, no need to dig into this function to know that the result is stored in j.selected.

zz-jason · 2018-09-03T05:52:16Z

util/chunk/chunk.go

@@ -232,6 +232,77 @@ func (c *Chunk) AppendPartialRow(colIdx int, row Row) {
 	}
 }

+// BatchCopyJoinRowToChunk uses for join to batch copy inner rows and outer row to chunk.
+func (c *Chunk) BatchCopyJoinRowToChunk(isRight bool, chkForJoin *Chunk, outer Row, selected []bool) bool {


Can this function be simplified to

func CopySelectedRows(src *Chunk, selected []bool, dst *Chunk)

Maybe we cann't ...
There is a special optimizer for copy outer row.

No, chkForJoin shares the same schema with c

zz-jason · 2018-09-03T09:30:43Z

util/chunk/chunk_util.go

+}
+
+// appendInnerRows appends different inner rows to the chunk.
+func appendInnerRows(innerColOffset, outerColOffset int, src *Chunk, selected []bool, dst *Chunk) int {


how about:

s/appendInnerRows/copySelectedInnerRows/ s/columns/srcCols/ s/rowCol/srcCol/ s/chkCol/dstCol/

zz-jason · 2018-09-03T09:32:29Z

util/chunk/chunk_util.go

+		return false
+	}
+
+	selectedRowNum := appendInnerRows(innerColOffset, outerColOffset, src, selected, dst)


how about s/selectedRowNum/numSelected/?

zz-jason · 2018-09-03T09:36:36Z

util/chunk/chunk_util.go

+			chkCol.appendNullBitmap(!rowCol.isNull(i))
+			chkCol.length++
+
+			if rowCol.isFixed() {


It's better to move this branch predicate outside the for loop.

zz-jason · 2018-09-03T11:44:07Z

util/chunk/chunk_util.go

+			start, end := srcCol.offsets[row.idx], srcCol.offsets[row.idx+numRows]
+			dstCol.data = append(dstCol.data, srcCol.data[start:end]...)
+			offsets := dstCol.offsets
+			l := srcCol.offsets[row.idx+1] - srcCol.offsets[row.idx]


how about `s/l/elemLen/?

zz-jason · 2018-09-03T11:50:27Z

util/chunk/chunk_util.go

+	return dst.columns[innerColOffset].length - oldLen
+}
+
+// copyOuterRows appends same outer row to the chunk with `numRows` times.


how about:

// copyOuterRows copies the continuous 'numRows' outer rows in the source Chunk // to the destination Chunk.

zz-jason · 2018-09-03T11:52:41Z

util/chunk/chunk_util.go

+	return numSelected > 0
+}
+
+// copySelectedInnerRows appends different inner rows to the chunk.


how about:

// copySelectedInnerRows copies the selected inner rows from the source Chunk // to the destination Chunk.

zz-jason · 2018-09-03T11:57:18Z

util/chunk/chunk_util.go

+
+package chunk
+
+// CopySelectedJoinRows uses for join to batch copy inner rows and outer row to chunk.


how about:

// CopySelectedJoinRows copies the selected joined rows from the source Chunk // to the destination Chunk. // // NOTE: All the outer rows in the source Chunk should be the same.

As the file and function name already have the join key word, I think it's not necessary reiterate that this function is only used for the join operator.

zz-jason · 2018-09-03T12:00:25Z

util/chunk/chunk_util_test.go

+	return srcChk, dstChk, selected
+}
+
+func TestBatchCopyJoinRowToChunk(t *testing.T) {


s/TestBatchCopyJoinRowToChunk/TestCopySelectedJoinRows/

zz-jason · 2018-09-03T12:00:47Z

util/chunk/chunk_util_test.go

+	}
+}
+
+func BenchmarkChunkBatchCopyJoinRow(b *testing.B) {


the benchmark name needs to be updated.

zz-jason · 2018-09-03T12:03:10Z

util/chunk/chunk_util_test.go

+	}
+}
+
+func BenchmarkChunkAppendRow(b *testing.B) {


how about BenchmarkAppendSelectedRow?

zz-jason · 2018-09-03T12:06:00Z

util/chunk/column.go

@@ -85,6 +85,34 @@ func (c *column) appendNullBitmap(on bool) {
 	}
 }

+func (c *column) appendMultiSameNullBitmap(on bool, num int) {
+	l := ((c.length + num - 1) >> 3) - len(c.nullBitmap)


it should be:

l := ((c.length + num + 7) >> 3) - len(c.nullBitmap)

how about:

s/l/numNewBytes/

zz-jason · 2018-09-03T12:14:15Z

util/chunk/column.go

+func (c *column) appendMultiSameNullBitmap(on bool, num int) {
+	l := ((c.length + num - 1) >> 3) - len(c.nullBitmap)
+	for i := 0; i <= l; i++ {
+		c.nullBitmap = append(c.nullBitmap, 0)


Maybe it's much clear and easier to understand if we change the copy strategy to:

set all the higher x bits of c.nullBitmap[len(c.nullBitmap)-1] to 0 or 1 according to the value of on.

memset the new bytes to 0xFF or 0x00 according to the value of on.

zz-jason · 2018-09-03T12:20:35Z

util/chunk/column.go

+			}
+		}
+	} else {
+		c.nullCount += num


we also need to set all the existing bits to zero even if on is set to false, because this Chunk maybe is truncated, and the null bitmap is not reset in that scenario.

zz-jason · 2018-09-04T03:31:09Z

@XuHuaiyu PTAL

XuHuaiyu · 2018-09-04T06:40:20Z

executor/joiner.go

-		}
-		matched = true
-		output.AppendRow(input.GetRow(i))
+	// batch copy selected row to output chunk


Batch copies selected rows to output chunk.

Add a . at the end of this comment.

XuHuaiyu · 2018-09-04T07:02:10Z

util/chunk/chunk_util.go

+
+// copySelectedInnerRows copies the selected inner rows from the source Chunk
+// to the destination Chunk.
+func copySelectedInnerRows(innerColOffset, outerColOffset int, src *Chunk, selected []bool, dst *Chunk) int {


add a comment for the return value.

XuHuaiyu · 2018-09-04T07:18:50Z

util/chunk/chunk_util.go

+			start, end := srcCol.offsets[row.idx], srcCol.offsets[row.idx+numRows]
+			dstCol.data = append(dstCol.data, srcCol.data[start:end]...)
+			offsets := dstCol.offsets
+			elemLen := srcCol.offsets[row.idx+1] - srcCol.offsets[row.idx]


Will it be possible that row.Idx+1 out of range?

In the normal case, it will not out of range.

Will it be possible that src.NumRows equals 1, if so, it seems this will be out of range? @crazycs520

if src.NumRows equals 1, the lengh of offsets should be 2.

XuHuaiyu · 2018-09-04T07:22:06Z

util/chunk/column.go

@@ -85,6 +85,29 @@ func (c *column) appendNullBitmap(on bool) {
 	}
 }

+func (c *column) appendMultiSameNullBitmap(on bool, num int) {


s/ on/ notNull ?

I see appendNullBitmap also use on, so, both use on or notNull ?

I prefer to both use on.

XuHuaiyu · 2018-09-04T07:23:30Z

util/chunk/column.go

@@ -85,6 +85,29 @@ func (c *column) appendNullBitmap(on bool) {
 	}
 }

+func (c *column) appendMultiSameNullBitmap(on bool, num int) {


add a comment for this function and its parameters.

XuHuaiyu · 2018-09-04T07:46:33Z

util/chunk/column.go

+	for i := 0; i < numNewBytes; i++ {
+		c.nullBitmap = append(c.nullBitmap, b)
+	}
+	if on {


if !on{ c.nulCount += num return } xxxx

This can help avoiding the else.

crazycs520 · 2018-09-04T08:43:49Z

/run-all-tests

…tch-copy

XuHuaiyu · 2018-09-04T08:47:58Z

util/chunk/column.go

 		pos := uint(c.length) & 7
 		c.nullBitmap[idx] |= byte(1 << pos)
 	} else {
 		c.nullCount++
 	}
 }

-func (c *column) appendMultiSameNullBitmap(on bool, num int) {
+// appendMultiSameNullBitmap appends multiple same bit value to `nullBitMap`.
+// notNull mean not not null.


s/ mean/ means
remove redundant not

XuHuaiyu · 2018-09-04T08:49:53Z

util/chunk/column.go

-func (c *column) appendMultiSameNullBitmap(on bool, num int) {
+// appendMultiSameNullBitmap appends multiple same bit value to `nullBitMap`.
+// notNull mean not not null.
+// num mean appends `num` bit value to `nullBitMap`.


num means the number of bits that should be appended.

XuHuaiyu · 2018-09-04T08:57:07Z

util/chunk/column.go

 	}
+	// 1. Set all the higher 8-'numOldBits' bits in the last old byte to 1.


Set all the remained bits in the last slot of old c.numBitMap to 1.

Set all the redundant bits in the last slot of new c.numBitMap to 0.

wait, I ask @CaitinChen

@CaitinChen

@crazycs520 I prefer "the remaining bits"

zz-jason

LGTM

crazycs520 · 2018-09-04T12:10:22Z

/run-all-tests

crazycs520 · 2018-09-04T12:12:03Z

@XuHuaiyu PTAL

…tch-copy

…into only-batch-copy

XuHuaiyu

LGTM

batch copy init

0cc4ab9

crazycs520 mentioned this pull request Aug 27, 2018

executor/join : use shallow copy for join. #7433

Merged

crazycs520 added sig/execution SIG execution type/enhancement The issue or PR belongs to an enhancement. labels Aug 27, 2018

crazycs520 added 3 commits August 28, 2018 12:57

add comment

2592034

add batch copy test and benchmark

265dd58

refine code

39cde9c

crazycs520 force-pushed the only-batch-copy branch from 6c30d15 to 39cde9c Compare August 28, 2018 06:28

fix bugs

7b19c1c

Merge branch 'master' of https://github.com/pingcap/tidb into only-ba…

e312fa4

…tch-copy

crazycs520 added 2 commits August 29, 2018 16:32

Merge branch 'master' of https://github.com/pingcap/tidb into only-ba…

4022038

…tch-copy

Merge branch 'master' of https://github.com/pingcap/tidb into only-ba…

9b6ce0f

…tch-copy

zz-jason reviewed Sep 3, 2018

View reviewed changes

refactor code and comment

315c1fa

crazycs520 force-pushed the only-batch-copy branch 3 times, most recently from 029afe3 to 0cfa549 Compare September 3, 2018 09:22

refine code

37902a2

crazycs520 force-pushed the only-batch-copy branch from 0cfa549 to 37902a2 Compare September 3, 2018 09:30

zz-jason reviewed Sep 3, 2018

View reviewed changes

address comment

c4556b4

crazycs520 force-pushed the only-batch-copy branch from 569b86d to c4556b4 Compare September 3, 2018 10:57

zz-jason reviewed Sep 3, 2018

View reviewed changes

refactor code and comment

cefae06

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 4, 2018

zz-jason assigned XuHuaiyu Sep 4, 2018

zz-jason added the status/all tests passed label Sep 4, 2018

zz-jason added this to the 2.1 milestone Sep 4, 2018

XuHuaiyu reviewed Sep 4, 2018

View reviewed changes

address comment

d3f8e85

crazycs520 force-pushed the only-batch-copy branch 2 times, most recently from eea2182 to 301a0db Compare September 4, 2018 08:43

crazycs520 force-pushed the only-batch-copy branch from 301a0db to c5aab73 Compare September 4, 2018 08:46

Merge branch 'master' of https://github.com/pingcap/tidb into only-ba…

026be7f

…tch-copy

crazycs520 force-pushed the only-batch-copy branch from c5aab73 to 026be7f Compare September 4, 2018 08:48

XuHuaiyu reviewed Sep 4, 2018

View reviewed changes

crazycs520 added 2 commits September 4, 2018 17:01

address comment

50a698d

address comment

28018f9

zz-jason reviewed Sep 4, 2018

View reviewed changes

zz-jason and others added 5 commits September 4, 2018 20:45

Merge branch 'master' into only-batch-copy

2d04676

Merge branch 'master' into only-batch-copy

e47fb7c

refine comments

3208738

Merge branch 'master' of https://github.com/pingcap/tidb into only-ba…

cd6eda1

…tch-copy

Merge branch 'only-batch-copy' of https://github.com/crazycs520/tidb …

cedcd0e

…into only-batch-copy

XuHuaiyu approved these changes Sep 5, 2018

View reviewed changes

crazycs520 merged commit 92e6a5a into pingcap:master Sep 5, 2018

zz-jason mentioned this pull request Sep 6, 2018

use a more cache-friendly way to make join rows #7420

Closed


		package chunk

		// CopySelectedJoinRows uses for join to batch copy inner rows and outer row to chunk.

		}
		// 1. Set all the higher 8-'numOldBits' bits in the last old byte to 1.

executor: add batch copy to inner join, left and right outer join. #7493

executor: add batch copy to inner join, left and right outer join. #7493

Conversation

crazycs520 commented Aug 27, 2018 • edited Loading

What problem does this PR solve?

crazycs520 commented Aug 27, 2018

crazycs520 commented Aug 28, 2018

crazycs520 commented Aug 28, 2018

crazycs520 commented Aug 29, 2018

crazycs520 commented Aug 29, 2018

crazycs520 commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason Sep 3, 2018 • edited Loading

Choose a reason for hiding this comment

zz-jason commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crazycs520 commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

crazycs520 commented Sep 4, 2018

crazycs520 commented Sep 4, 2018

XuHuaiyu left a comment

Choose a reason for hiding this comment

crazycs520 commented Aug 27, 2018 •

edited

Loading

zz-jason Sep 3, 2018 •

edited

Loading