executor: refactor `joinResultGenerator` to handle the unmatched outer records #7288

zz-jason · 2018-08-06T12:44:04Z

What have you changed? (mandatory)

refactor the joinResultGenerator:

add a function onMissMatch to handle the unmatched outer records
refactor the emit method to tryToMatch, which returns a boolean value to indicate whether the outer record can be joined with any inner record.

This PR is a bugfix, we should not handle the unmatched outer records in the old emit function: this function only joins a batch of inner rows, not all of them. We only know that the outer records can not be joined with any inner record on a serials of emit function calls, and then decide whether the outer record is unmatched.

After this PR, a typical function call to the recordJoiner is:

hasMatch := false
for innerIter.Current() != innerIter.End() {
    matched, err := g.tryToMatch(outer, innerIter, chk)
    // handle err
    hasMatch = hasMatch || matched
}
if !hasMatch {
    g.onMissMatch(outer)
}

What is the type of the changes? (mandatory)

Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

unit test
explain test

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No

Does this PR affect tidb-ansible update? (mandatory)

No

Does this PR need to be added to the release notes? (mandatory)

release note:
Fix a bug that the `Outer Join` may output the same unmatched outer records multiply times under certain special conditions.

Add a few positive/negative examples (optional)

An negative case, which is already added in the test:

Before this PR:

drop table if exists t;
create table t(a bigint, b bigint, unique key idx1(a, b));
insert into t values(1, 1), (1, 2), (1, 3), (1, 4), (1, 5), (1, 6);

set @@tidb_max_chunk_size = 2; -- this is important, which expose the bug in a small data set
select * from t t1 left join t t2 on t1.a = t2.a and t1.b = t2.b + 4;

and the wrong result is:

TiDB(localhost:4000) > select * from t t1 left join t t2 on t1.a = t2.a and t1.b = t2.b + 4;
+------+------+------+------+
| a    | b    | a    | b    |
+------+------+------+------+
|    1 |    1 | NULL | NULL |
|    1 |    1 | NULL | NULL |
|    1 |    1 | NULL | NULL |
|    1 |    1 | NULL | NULL |
|    1 |    2 | NULL | NULL |
|    1 |    2 | NULL | NULL |
|    1 |    2 | NULL | NULL |
|    1 |    2 | NULL | NULL |
|    1 |    3 | NULL | NULL |
|    1 |    3 | NULL | NULL |
|    1 |    3 | NULL | NULL |
|    1 |    3 | NULL | NULL |
|    1 |    4 | NULL | NULL |
|    1 |    4 | NULL | NULL |
|    1 |    4 | NULL | NULL |
|    1 |    4 | NULL | NULL |
|    1 |    5 |    1 |    1 |
|    1 |    5 | NULL | NULL |
|    1 |    5 | NULL | NULL |
|    1 |    5 | NULL | NULL |
|    1 |    6 |    1 |    2 |
|    1 |    6 | NULL | NULL |
|    1 |    6 | NULL | NULL |
|    1 |    6 | NULL | NULL |
+------+------+------+------+
24 rows in set (0.00 sec)

This PR fixes the above issue:

TiDB(localhost:4000) > select * from t t1 left join t t2 on t1.a = t2.a and t1.b = t2.b + 4;
+------+------+------+------+
| a    | b    | a    | b    |
+------+------+------+------+
|    1 |    1 | NULL | NULL |
|    1 |    2 | NULL | NULL |
|    1 |    3 | NULL | NULL |
|    1 |    4 | NULL | NULL |
|    1 |    5 |    1 |    1 |
|    1 |    6 |    1 |    2 |
+------+------+------+------+
6 rows in set (0.00 sec)

…-join-generator

zz-jason · 2018-08-06T12:44:32Z

/run-all-tests

winoros · 2018-08-06T14:11:38Z

executor/join_result_generators.go

+//     }
+//
+// NOTE: This interface is **not** thread-safe.
+type recordJoiner interface {


It should be joinRecorder?

record means data record, it should be recordJoiner or tupleJoiner.

record have both verb and noun, and not used very often in executor package. What about rowJoiner.

The struct name can be discussed later, we can firstly focus on the code logic.

Can we keep the original name joinResultGenerator and do the name refactor later?
Because there are too many places to change.

zz-jason · 2018-08-07T02:13:02Z

/run-unit-test

coocood · 2018-08-07T04:54:55Z

executor/index_lookup_join.go

@@ -60,7 +60,7 @@ type IndexLookUpJoin struct {
 	joinResult *chunk.Chunk
 	innerIter  chunk.Iterator

-	resultGenerator joinResultGenerator
+	resultGenerator recordJoiner


The field name needs to be updated accordingly?

This reverts commit a2dda9e.

This reverts commit a2db5b8.

zz-jason · 2018-08-07T08:59:31Z

@coocood @lysu @winoros PTAL

CaitinChen · 2018-08-08T05:23:12Z

executor/join_result_generators.go

@@ -32,21 +32,51 @@ var (
 	_ joinResultGenerator = &innerJoinResultGenerator{}
 )

-// joinResultGenerator is used to generate join results according the join type, see every implementor for detailed information.
+// joinResultGenerator is used to generate join results according the join type.


joinResultGenerator is used to generate join results according to the join type.

CaitinChen · 2018-08-08T05:35:15Z

executor/join_result_generators.go

+	// onMissMatch operates on the unmatched outer row according to the join
+	// type. An outer row can be considered miss matched if:
+	//   1. it can not pass the filter on the outer table side.
+	//   2. there does not exist an inner row with the same join key.


Maybe you can use "there is no inner row with the same join key." Just for your reference.

CaitinChen · 2018-08-08T05:37:31Z

executor/join_result_generators.go

+	// unmatched outer rows according to the current join type:
+	//   1. 'SemiJoin': ignores the unmatched outer row.
+	//   2. 'AntiSemiJoin': appends the unmatched outer row to the result buffer.
+	//   3. 'LeftOuterSemiJoin': concates the unmatched outer row with 0 and


concates -> concats

CaitinChen · 2018-08-08T05:38:04Z

executor/join_result_generators.go

+	//   1. 'SemiJoin': ignores the unmatched outer row.
+	//   2. 'AntiSemiJoin': appends the unmatched outer row to the result buffer.
+	//   3. 'LeftOuterSemiJoin': concates the unmatched outer row with 0 and
+	//      appended it to the result buffer.


appended -> appends

CaitinChen · 2018-08-08T05:38:29Z

executor/join_result_generators.go

+	//   2. 'AntiSemiJoin': appends the unmatched outer row to the result buffer.
+	//   3. 'LeftOuterSemiJoin': concates the unmatched outer row with 0 and
+	//      appended it to the result buffer.
+	//   4. 'AntiLeftOuterSemiJoin': concates the unmatched outer row with 0 and


concates -> concats

CaitinChen · 2018-08-08T05:40:04Z

executor/join_result_generators.go

+	//      appended it to the result buffer.
+	//   4. 'AntiLeftOuterSemiJoin': concates the unmatched outer row with 0 and
+	//      appended to the result buffer.
+	//   5. 'LeftOuterJoin': concates the unmatched outer row with a row of NULLs


concates -> concats

CaitinChen · 2018-08-08T05:40:33Z

executor/join_result_generators.go

+	//   4. 'AntiLeftOuterSemiJoin': concates the unmatched outer row with 0 and
+	//      appended to the result buffer.
+	//   5. 'LeftOuterJoin': concates the unmatched outer row with a row of NULLs
+	//      and appended to the result buffer.


appended -> appends it

CaitinChen · 2018-08-08T05:40:48Z

executor/join_result_generators.go

+	//      appended to the result buffer.
+	//   5. 'LeftOuterJoin': concates the unmatched outer row with a row of NULLs
+	//      and appended to the result buffer.
+	//   6. 'RightOuterJoin': concates the unmatched outer row with a row of NULLs


concates -> concats

CaitinChen · 2018-08-08T05:41:02Z

executor/join_result_generators.go

+	//   5. 'LeftOuterJoin': concates the unmatched outer row with a row of NULLs
+	//      and appended to the result buffer.
+	//   6. 'RightOuterJoin': concates the unmatched outer row with a row of NULLs
+	//      and appended to the result buffer.


appended -> appends it

CaitinChen · 2018-08-08T05:43:23Z

executor/join_result_generators.go

+	// tryToMatch tries to join an outer row with a batch of inner rows. When
+	// 'inners.Len != 0' but all the joined rows are filtered, the outer row is
+	// considered unmatched. Otherwise, the outer row is matched and some joined
+	// rows is appended to `chk`. The size of `chk` is limited to MaxChunkSize.


rows are appended to chk. The size of chk is limited to MaxChunkSize.

zz-jason · 2018-08-08T05:54:05Z

@CaitinChen Done, PTAL again, thanks!

CaitinChen · 2018-08-08T05:55:40Z

executor/join_result_generators.go

@@ -60,21 +60,21 @@ type joinResultGenerator interface {
 	// onMissMatch operates on the unmatched outer row according to the join
 	// type. An outer row can be considered miss matched if:
 	//   1. it can not pass the filter on the outer table side.
-	//   2. there does not exist an inner row with the same join key.
+	//   2. there is no inner row with the same join key..


Please delete the extra period at the end of this sentence.

coocood · 2018-08-08T05:56:14Z

executor/join_result_generators.go

+		if !(err == nil && matched) {
+			return
+		}
+		// here we handle the matched outer.
 		chk.AppendPartialRow(0, outer)


I think do the main work in defer function is too tricky.
How about just

defer func() { if matched { innters.ReachEnd() } }

And AppendPartialRow before return true, nil.

Actually, innters.ReachEnd() should be called together when we have a matched inner row and do the join work. So I chose the defer style to reduce the code duplication.

I think we should not use defer to reducing code duplication.
It splits the execution flow, makes the code harder to read.

We can extract functions to reducing code duplication.

coocood · 2018-08-08T06:00:14Z

executor/join_result_generators.go

 		chk.AppendPartialRow(0, outer)
 		chk.AppendInt64(outer.Len(), 1)
-		return nil
+		inners.ReachEnd()


coocood · 2018-08-08T06:00:56Z

executor/join_result_generators.go

 		chk.AppendPartialRow(0, outer)
 		chk.AppendInt64(outer.Len(), 0)
-		return nil
+		inners.ReachEnd()


zz-jason · 2018-08-08T06:37:11Z

@coocood @CaitinChen @winoros PTAL

CaitinChen

LGTM

zz-jason · 2018-08-08T06:46:23Z

/run-all-tests

coocood · 2018-08-08T06:48:03Z

LGTM

winoros

lgtm

…r records (pingcap#7288)

…r records (#7288) (#7315)

zz-jason added 4 commits August 6, 2018 19:08

introduce onMissMatch

0207e91

rename some variables

6b74d45

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

0ee08ad

…-join-generator

add test

59a2dea

zz-jason added type/bugfix This PR fixes a bug. sig/execution SIG execution labels Aug 6, 2018

Merge branch 'master' into dev/fix-join-generator

1aa436b

zz-jason changed the title ~~Dev/fix join generator~~ executor: refactor joinResultGenerator to handle the unmatched outer records Aug 6, 2018

winoros reviewed Aug 6, 2018

View reviewed changes

zz-jason mentioned this pull request Aug 7, 2018

executor: refactor joinResultGenerator to handle the unmatched outer records (#7288) #7290

Closed

zz-jason added the status/all tests passed label Aug 7, 2018

coocood reviewed Aug 7, 2018

View reviewed changes

zz-jason added 5 commits August 7, 2018 16:53

rename bac

a2dda9e

Revert "rename bac"

6358166

This reverts commit a2dda9e.

rename back, again

a2db5b8

Revert "rename back, again"

6232125

This reverts commit a2db5b8.

rename back, again...

ead095d

Merge branch 'master' into dev/fix-join-generator

b14f81a

CaitinChen reviewed Aug 8, 2018

View reviewed changes

zz-jason added 2 commits August 8, 2018 13:53

fix comment

491d5cc

Merge branch 'master' into dev/fix-join-generator

50234ad

CaitinChen reviewed Aug 8, 2018

View reviewed changes

coocood reviewed Aug 8, 2018

View reviewed changes

address comment

1a338d6

CaitinChen reviewed Aug 8, 2018

View reviewed changes

Merge branch 'master' into dev/fix-join-generator

948dd3f

winoros approved these changes Aug 8, 2018

View reviewed changes

coocood merged commit b39b5f5 into pingcap:master Aug 8, 2018

zz-jason added a commit to zz-jason/tidb that referenced this pull request Aug 8, 2018

executor: refactor joinResultGenerator to handle the unmatched oute…

fd8e276

…r records (pingcap#7288)

zz-jason mentioned this pull request Aug 8, 2018

executor: refactor joinResultGenerator to handle the unmatched outer records (#7288) #7315

Merged

zz-jason deleted the dev/fix-join-generator branch August 8, 2018 08:17

zz-jason added a commit that referenced this pull request Aug 8, 2018

executor: refactor joinResultGenerator to handle the unmatched oute…

343ed54

…r records (#7288) (#7315)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor: refactor `joinResultGenerator` to handle the unmatched outer records #7288

executor: refactor `joinResultGenerator` to handle the unmatched outer records #7288

zz-jason commented Aug 6, 2018 •

edited

Loading

zz-jason commented Aug 6, 2018

winoros Aug 6, 2018

zz-jason Aug 7, 2018

winoros Aug 7, 2018

zz-jason Aug 7, 2018

coocood Aug 7, 2018

zz-jason commented Aug 7, 2018

coocood Aug 7, 2018

zz-jason commented Aug 7, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

CaitinChen Aug 8, 2018

zz-jason commented Aug 8, 2018

CaitinChen Aug 8, 2018

coocood Aug 8, 2018

zz-jason Aug 8, 2018

coocood Aug 8, 2018

coocood Aug 8, 2018

coocood Aug 8, 2018

coocood Aug 8, 2018

zz-jason commented Aug 8, 2018

CaitinChen left a comment

zz-jason commented Aug 8, 2018

coocood commented Aug 8, 2018

winoros left a comment

executor: refactor joinResultGenerator to handle the unmatched outer records #7288

executor: refactor joinResultGenerator to handle the unmatched outer records #7288

Conversation

zz-jason commented Aug 6, 2018 • edited Loading

What have you changed? (mandatory)

What is the type of the changes? (mandatory)

How has this PR been tested? (mandatory)

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Does this PR affect tidb-ansible update? (mandatory)

Does this PR need to be added to the release notes? (mandatory)

Add a few positive/negative examples (optional)

zz-jason commented Aug 6, 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

zz-jason commented Aug 7, 2018

Choose a reason for hiding this comment

zz-jason commented Aug 7, 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

zz-jason commented Aug 8, 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 commented Aug 8, 2018

CaitinChen left a comment

Choose a reason for hiding this comment

zz-jason commented Aug 8, 2018

coocood commented Aug 8, 2018

winoros left a comment

Choose a reason for hiding this comment

executor: refactor `joinResultGenerator` to handle the unmatched outer records #7288

executor: refactor `joinResultGenerator` to handle the unmatched outer records #7288

zz-jason commented Aug 6, 2018 •

edited

Loading