executor: utilities for disk-based hash join #12116

SunRunAway · 2019-09-10T06:00:04Z

What problem does this PR solve?

part of #11607
The utilities for disk-based hash join

What is changed and how it works?

goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/util/chunk
BenchmarkListInDiskAdd-12       	  300000	      4681 ns/op	     360 B/op	       4 allocs/op
BenchmarkListInDiskGetRow-12    	  500000	      2878 ns/op	     994 B/op	      21 allocs/op
BenchmarkListMemoryUsage-12     	2000000000	         0.37 ns/op	       0 B/op	       0 allocs/op
BenchmarkListAdd-12             	20000000	       104 ns/op	      41 B/op	       0 allocs/op
BenchmarkListGetRow-12          	500000000	         4.13 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/pingcap/tidb/util/chunk	31.892s

Check List

Tests

Unit test

Code changes

Has exported function/method change

Side effects

None

Related changes

None

Release note

None

util/chunk/disk.go

XuHuaiyu

.

XuHuaiyu · 2019-09-10T08:05:37Z

util/chunk/disk.go

+	var n int64
+	numRows := chk.NumRows()
+	chk.offsetsOfRows = make([]int64, 0, numRows)
+	var format *diskFormatRow


s/ format/ rowInDiskFormat ?

util/chunk/disk.go

XuHuaiyu · 2019-09-10T08:18:45Z

util/chunk/disk.go

+}
+
+// toRow deserializes diskFormatRow to Row.
+func (format *diskFormatRow) toRow(fields []*types.FieldType) Row {


I'm not sure that will MutRow help here?

I've rewritten a zero-copy implementation, while feels a little hack.

XuHuaiyu · 2019-09-10T08:30:12Z

util/chunk/disk.go

+func convertFromRow(row Row, reuse *diskFormatRow) (format *diskFormatRow) {
+	numCols := row.Chunk().NumCols()
+	if reuse != nil {
+		format = reuse


s/ format/ rowInDiskFormat ?

I think it's better to keep the short-lived value short.

XuHuaiyu · 2019-09-10T08:30:38Z

util/chunk/disk.go

+	bufReader.Reset(r)
+	defer bufReaderPool.Put(bufReader)
+
+	format := rowInDisk{numCol: len(l.fieldTypes)}


s/ format/ rowInDiskFormat ?

zz-jason · 2019-09-10T06:49:22Z

util/chunk/disk.go

+	New: func() interface{} { return bufio.NewReaderSize(nil, readBufSize) },
+}
+
+var tmpDir = path.Join(os.TempDir(), "tidb-server-hashJoin")


It's better to include pid as part of the file name. Consider the situation that one machine have multiply tidb-server processes.

I've included os.Args[0] as part of the file name so that we can clean the uncleared temp file during the last run.

codecov · 2019-09-11T03:42:30Z

Codecov Report

Merging #12116 into master will decrease coverage by 0.303%.
The diff coverage is 84.6666%.

@@               Coverage Diff                @@
##             master     #12116        +/-   ##
================================================
- Coverage   81.6069%   81.3039%   -0.3031%     
================================================
  Files           452        453         +1     
  Lines         98064      96940      -1124     
================================================
- Hits          80027      78816      -1211     
- Misses        12393      12459        +66     
- Partials       5644       5665        +21

SunRunAway · 2019-09-11T04:41:10Z

/run-all-tests

SunRunAway · 2019-09-11T04:55:30Z

/run-all-tests

SunRunAway · 2019-09-11T05:39:45Z

/run-all-tests

zyxbest · 2019-09-11T06:08:34Z

/build

SunRunAway · 2019-09-11T06:51:09Z

/run-unit-test

util/memory/tracker.go

qw4990

LGTM

zz-jason · 2019-09-11T08:35:21Z

util/chunk/disk_test.go

+
+	chks := make([]*Chunk, 0, numChk)
+	for chkIdx := 0; chkIdx < numChk; chkIdx++ {
+		chk := NewChunkWithCapacity(fields, 2)


Suggested change

chk := NewChunkWithCapacity(fields, 2)

chk := NewChunkWithCapacity(fields, numRow)

zz-jason · 2019-09-11T08:35:43Z

util/chunk/disk_test.go

+	"github.com/pingcap/tidb/types/json"
+)
+
+func initChunks(numChk, numRow int) ([]*Chunk, []*types.FieldType, error) {


no need to return an error?

zz-jason · 2019-09-11T08:36:45Z

util/chunk/disk_test.go

+	defer func() {
+		err := l.Close()
+		c.Check(err, check.IsNil)
+		c.Check(l.disk, check.Not(check.IsNil))


Suggested change

c.Check(l.disk, check.Not(check.IsNil))

c.Check(l.disk, check.NotNil)

zz-jason · 2019-09-11T08:47:50Z

util/chunk/disk.go

+	n, err := chk2.WriteTo(l.bufWriter)
+	l.offWrite += n
+	if err != nil {
+		return


It's better to directly use the return err form to improve code readability.

zz-jason · 2019-09-11T08:57:57Z

util/chunk/disk.go

+	// sizesOfColumns stores the size of each column in a row.
+	// -1 means the value of this column is null.
+	sizesOfColumns []int64 // -1 means null
+	cells          [][]byte


Suggested change

cells [][]byte

cells [][]byte // raw data is shallow copied to a cell

zz-jason

LGTM

sre-bot · 2019-09-11T09:31:14Z

/run-all-tests

This was referenced Sep 10, 2019

executor: implement disk-based hash join #12067

Merged

Consider using a disk-based hash table for hash join avoiding OOM #11607

Open

SunRunAway requested review from zz-jason, qw4990 and XuHuaiyu September 10, 2019 06:23

SunRunAway added the sig/execution SIG execution label Sep 10, 2019

qw4990 reviewed Sep 10, 2019

View reviewed changes

util/chunk/disk.go Outdated Show resolved Hide resolved

qw4990 reviewed Sep 10, 2019

View reviewed changes

util/chunk/disk.go Show resolved Hide resolved

qw4990 reviewed Sep 10, 2019

View reviewed changes

util/chunk/disk.go Outdated Show resolved Hide resolved

XuHuaiyu reviewed Sep 10, 2019

View reviewed changes

zz-jason reviewed Sep 10, 2019

View reviewed changes

SunRunAway added the status/all tests passed label Sep 11, 2019

SunRunAway force-pushed the disk-join-issue11607-disk branch from 71da850 to 4b30699 Compare September 11, 2019 04:50

SunRunAway requested review from zz-jason, qw4990 and XuHuaiyu September 11, 2019 04:50

SunRunAway added the status/PTAL label Sep 11, 2019

SunRunAway added status/all tests passed and removed status/all tests passed labels Sep 11, 2019

qw4990 reviewed Sep 11, 2019

View reviewed changes

util/memory/tracker.go Show resolved Hide resolved

qw4990 reviewed Sep 11, 2019

View reviewed changes

zz-jason reviewed Sep 11, 2019

View reviewed changes

SunRunAway added 2 commits September 11, 2019 17:28

executor: utilities for disk-based hash join

ce67343

format

6de4583

SunRunAway added 5 commits September 11, 2019 17:28

address comments

dcb84c9

address comments

d329a79

address comments

95865a6

more benchmark

7d42f05

address comments

9373cdd

SunRunAway force-pushed the disk-join-issue11607-disk branch from 22b83f1 to 9373cdd Compare September 11, 2019 09:28

zz-jason approved these changes Sep 11, 2019

View reviewed changes

zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/PTAL labels Sep 11, 2019

sre-bot merged commit c1e6b8d into pingcap:master Sep 11, 2019

SunRunAway deleted the disk-join-issue11607-disk branch September 11, 2019 09:56

XuHuaiyu mentioned this pull request Jan 8, 2020

use external storage to reduce the memory space usage of some executors #14392

Open

SunRunAway mentioned this pull request Feb 7, 2020

Implement grace hash join when build input is bigger than the available memory #14675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: utilities for disk-based hash join #12116

executor: utilities for disk-based hash join #12116

SunRunAway commented Sep 10, 2019 •

edited

Loading

XuHuaiyu left a comment

XuHuaiyu Sep 10, 2019

XuHuaiyu Sep 10, 2019

SunRunAway Sep 11, 2019 •

edited

Loading

XuHuaiyu Sep 10, 2019

SunRunAway Sep 11, 2019

XuHuaiyu Sep 10, 2019

zz-jason Sep 10, 2019

SunRunAway Sep 11, 2019

codecov bot commented Sep 11, 2019 •

edited

Loading

SunRunAway commented Sep 11, 2019

SunRunAway commented Sep 11, 2019

SunRunAway commented Sep 11, 2019

zyxbest commented Sep 11, 2019

SunRunAway commented Sep 11, 2019

qw4990 left a comment

zz-jason Sep 11, 2019

zz-jason Sep 11, 2019

zz-jason Sep 11, 2019

zz-jason Sep 11, 2019

zz-jason Sep 11, 2019

zz-jason left a comment

sre-bot commented Sep 11, 2019

	chk := NewChunkWithCapacity(fields, 2)
	chk := NewChunkWithCapacity(fields, numRow)

	c.Check(l.disk, check.Not(check.IsNil))
	c.Check(l.disk, check.NotNil)

	cells [][]byte
	cells [][]byte // raw data is shallow copied to a cell

executor: utilities for disk-based hash join #12116

executor: utilities for disk-based hash join #12116

Conversation

SunRunAway commented Sep 10, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

XuHuaiyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunRunAway Sep 11, 2019 • edited Loading

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

codecov bot commented Sep 11, 2019 • edited Loading

Codecov Report

SunRunAway commented Sep 11, 2019

SunRunAway commented Sep 11, 2019

SunRunAway commented Sep 11, 2019

zyxbest commented Sep 11, 2019

SunRunAway commented Sep 11, 2019

qw4990 left a 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

sre-bot commented Sep 11, 2019

SunRunAway commented Sep 10, 2019 •

edited

Loading

SunRunAway Sep 11, 2019 •

edited

Loading

codecov bot commented Sep 11, 2019 •

edited

Loading