executor: implement the execution part of the outer hash join #12882

fzhedu · 2019-10-22T12:20:32Z

What problem does this PR solve?

implement the execution part of the outer hash join. Related to #6868

What is changed and how it works?

update the hash join for outer join, after reverse the inner and the outer for all outer joins when building hash joins if the outer size is smaller.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Increased code complexity

Related changes

Need to cherry-pick to the release branch

executor/benchmark_test.go

executor/hash_table.go

executor/join.go

executor/builder.go

executor/join.go

util/bitmap/concurrent.go

XuHuaiyu · 2019-10-29T07:02:49Z

Please address the comments

executor/join.go

util/bitmap/concurrent.go

util/bitmap/concurrent_test.go

executor/hash_table.go

XuHuaiyu · 2019-10-31T02:38:56Z

executor/builder.go

+	}
+	// reverse the inner and the outer
+	if e.outerHashJoin {
+		v.InnerChildIdx = 1 - v.InnerChildIdx


Actually, we'd better move these lines to the plan building phase?

May be not.
The plan building phase just make choose whether to adopt the outer hash join, and the executing phase reverses the inner and the outer internally. This way does not need to change a lot of code in the plan build phase.

Execute phase better not change the physical plan.

Other members may not think so according to previous daily meeting when discussing the solutions.
On the other hand, the current way is implemented and tested. If taking to way to change physical plans at builing plan phase, it needs to rewrite a lot of code.

Why does it need to rewrite a lot of code?
Only the *LogicalJoin.getHashJoin and the PhysicalHashJoin.GetCost function of HashJoin may be affected?

Besides, buildHashJoin here and explain()
In other words, it needs to rewrite the PR #12883 , as well as the function here.

executor/builder.go

executor/join.go

util/bitmap/concurrent.go

XuHuaiyu · 2019-10-31T04:36:02Z

Please merge the master and resolve the conflicts.

codecov · 2019-10-31T07:51:53Z

Codecov Report

Merging #12882 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12882   +/-   ##
===========================================
  Coverage   80.5351%   80.5351%           
===========================================
  Files           471        471           
  Lines        114437     114437           
===========================================
  Hits          92162      92162           
  Misses        15237      15237           
  Partials       7038       7038

executor/builder.go

executor/hash_table.go

util/bitmap/concurrent.go

executor/join.go

XuHuaiyu · 2019-11-04T03:49:19Z

PTAL @SunRunAway @qw4990

XuHuaiyu · 2019-11-11T02:23:56Z

util/bitmap/concurrent_test.go

+	wg.Wait()
+	// If clearCounter is too big, it means setter concurrency of this test is not enough.
+	c.Assert(clearCounter < loopCount, Equals, true)
+	c.Assert(setterCounter, Equals, clearCounter+1)


Will this always be true?

If some errors in bm.Set() at Line:76 occur, these checks may be false.

XuHuaiyu · 2019-11-11T03:48:07Z

executor/join.go

+	} else { // Sequentially handling unmatched rows from the hash table that is from disk
+		numChks := e.rowContainer.recordsInDisk.NumChunks()
+		for i := 0; i < numChks; i++ {
+			numOfRows := e.rowContainer.recordsInDisk.NumRowsOfChunk(i)


It a little ugly here...
I think we can do the following things in an individual PR:

rename chunk.List to chunk.ListInMemory

define an interface

type List interface{ Add(chk *Chunk) NumChunks() GetRow(chunk.RowPtr) }

remove the useless duplicate code for this function

@fzhedu @SunRunAway

This change effects too much code, so regect this suggestion.

XuHuaiyu

LGTM

sre-bot · 2019-11-11T10:33:44Z

Your auto merge job has been accepted, waiting for 13230, 13235, 13231

sre-bot · 2019-11-11T10:55:01Z

/run-all-tests

sre-bot · 2019-11-11T10:57:06Z

@fzhedu merge failed.

sykp241095 · 2019-11-11T11:04:48Z

/rebuild

…shJoin-execution

SunRunAway · 2019-11-12T09:43:19Z

/merge

sre-bot · 2019-11-12T09:45:31Z

/run-all-tests

…p#12882)

implement the execution of the outer hash join

c62e84b

fzhedu added the sig/execution SIG execution label Oct 22, 2019

fzhedu requested review from SunRunAway and XuHuaiyu and removed request for SunRunAway October 22, 2019 12:20

fzhedu changed the title ~~implement the execution part of the outer hash join~~ executor: implement the execution part of the outer hash join Oct 22, 2019

fzhedu commented Oct 22, 2019

View reviewed changes

executor/benchmark_test.go Outdated Show resolved Hide resolved

zz-jason mentioned this pull request Oct 22, 2019

use the smaller table to build hash table for hash join #6868

Closed

2 tasks

XuHuaiyu reviewed Oct 28, 2019

View reviewed changes

fzhedu requested review from XuHuaiyu and qw4990 October 29, 2019 06:43

XuHuaiyu removed their request for review October 29, 2019 07:02

fzhedu requested a review from XuHuaiyu October 29, 2019 13:47

change code according to reviews

0083940

XuHuaiyu reviewed Oct 31, 2019

View reviewed changes

reformat code according to reviews

322f6b4

XuHuaiyu reviewed Oct 31, 2019

View reviewed changes

util/bitmap/concurrent.go Outdated Show resolved Hide resolved

util/bitmap/concurrent.go Outdated Show resolved Hide resolved

util/bitmap/concurrent.go Show resolved Hide resolved

util/bitmap/concurrent.go Show resolved Hide resolved

util/bitmap/concurrent.go Outdated Show resolved Hide resolved

fzhedu mentioned this pull request Oct 31, 2019

planner: consider build hashtable using the outer side for outer join during plan building #12883

Merged

fzhedu added 2 commits October 31, 2019 14:14

update code according to reviews

725ae32

merge with upstream/master

16849b4

fzhedu requested a review from XuHuaiyu October 31, 2019 08:15

XuHuaiyu reviewed Nov 4, 2019

View reviewed changes

executor/builder.go Outdated Show resolved Hide resolved

executor/hash_table.go Outdated Show resolved Hide resolved

util/bitmap/concurrent.go Show resolved Hide resolved

executor/join.go Outdated Show resolved Hide resolved

executor/join.go Outdated Show resolved Hide resolved

update code according to reviews

1bda77d

fzhedu requested a review from XuHuaiyu November 4, 2019 07:02

rename probeSideFilter

87acbcd

XuHuaiyu reviewed Nov 11, 2019

View reviewed changes

fzhedu requested a review from XuHuaiyu November 11, 2019 03:33

XuHuaiyu reviewed Nov 11, 2019

View reviewed changes

fzhedu mentioned this pull request Nov 11, 2019

executor, util: rename original List to ListInMemory and add a new interface List #13353

Merged

upsate comments

53b3a60

fzhedu force-pushed the OuterHashJoin-execution branch from f03af87 to 53b3a60 Compare November 11, 2019 09:55

merge with upstream/master

0a809ab

XuHuaiyu previously approved these changes Nov 11, 2019

View reviewed changes

XuHuaiyu 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/LGT1 Indicates that a PR has LGTM 1. labels Nov 11, 2019

Merge branch 'master' of https://github.com/pingcap/tidb into OuterHa…

70ca7fe

…shJoin-execution

qw4990 dismissed XuHuaiyu’s stale review via 70ca7fe November 11, 2019 11:15

fzhedu dismissed XuHuaiyu’s stale review via 70ca7fe November 11, 2019 11:15

fzhedu force-pushed the OuterHashJoin-execution branch from 1b1af78 to 70ca7fe Compare November 11, 2019 11:15

recovery a function after merged with master

38263f6

fzhedu requested a review from SunRunAway November 11, 2019 11:36

SunRunAway approved these changes Nov 11, 2019

View reviewed changes

qw4990 removed their request for review November 12, 2019 08:04

Merge branch 'master' into OuterHashJoin-execution

b74d60c

SunRunAway added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 12, 2019

SunRunAway added this to the v4.0.0 milestone Nov 12, 2019

sre-bot merged commit b84c5a7 into pingcap:master Nov 12, 2019

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019

executor: implement the execution part of the outer hash join (pingca…

491f146

…p#12882)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: implement the execution part of the outer hash join #12882

executor: implement the execution part of the outer hash join #12882

fzhedu commented Oct 22, 2019 •

edited

Loading

XuHuaiyu commented Oct 29, 2019

XuHuaiyu Oct 31, 2019

fzhedu Oct 31, 2019

XuHuaiyu Oct 31, 2019

fzhedu Oct 31, 2019

XuHuaiyu Nov 1, 2019

fzhedu Nov 1, 2019 •

edited

Loading

XuHuaiyu commented Oct 31, 2019

codecov bot commented Oct 31, 2019 •

edited

Loading

XuHuaiyu commented Nov 4, 2019

XuHuaiyu Nov 11, 2019

fzhedu Nov 11, 2019

XuHuaiyu Nov 11, 2019

fzhedu Nov 11, 2019

XuHuaiyu left a comment

sre-bot commented Nov 11, 2019

sre-bot commented Nov 11, 2019

sre-bot commented Nov 11, 2019

sykp241095 commented Nov 11, 2019

SunRunAway commented Nov 12, 2019

sre-bot commented Nov 12, 2019

executor: implement the execution part of the outer hash join #12882

executor: implement the execution part of the outer hash join #12882

Conversation

fzhedu commented Oct 22, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

XuHuaiyu commented Oct 29, 2019

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

fzhedu Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

XuHuaiyu commented Oct 31, 2019

codecov bot commented Oct 31, 2019 • edited Loading

Codecov Report

XuHuaiyu commented Nov 4, 2019

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

XuHuaiyu left a comment

Choose a reason for hiding this comment

sre-bot commented Nov 11, 2019

sre-bot commented Nov 11, 2019

sre-bot commented Nov 11, 2019

sykp241095 commented Nov 11, 2019

SunRunAway commented Nov 12, 2019

sre-bot commented Nov 12, 2019

fzhedu commented Oct 22, 2019 •

edited

Loading

fzhedu Nov 1, 2019 •

edited

Loading

codecov bot commented Oct 31, 2019 •

edited

Loading