planner:dont push down right condition for anti semi join #12075

jingyugao · 2019-09-08T09:42:09Z

What problem does this PR solve?

TiDB will push down right condition for anti semi join.
But if we push down the right condition, we can't know why there is no matched rows, null or false.
Fix #12074.

What is changed and how it works?

For anti semi join don't push down the righ condition.

Check List

Tests

Unit test

Code changes

Simple change.

Side effects

After this pr,we can't push down the right condition for anti semi join.
So this might decrease the performance.

Related changes

Maybe not

Release note

Write release note for bug-fix or new feature.

codecov · 2019-09-08T09:46:34Z

Codecov Report

Merging #12075 into master will decrease coverage by 0.1426%.
The diff coverage is 79.3103%.

@@               Coverage Diff                @@
##             master     #12075        +/-   ##
================================================
- Coverage   80.0859%   79.9433%   -0.1426%     
================================================
  Files           473        473                
  Lines        116787     116186       -601     
================================================
- Hits          93530      92883       -647     
- Misses        15943      15978        +35     
- Partials       7314       7325        +11

francis0407 · 2019-09-09T01:40:55Z

planner/core/rule_predicate_push_down.go

@@ -173,6 +173,12 @@ func (p *LogicalJoin) PredicatePushDown(predicates []expression.Expression) (ret
 		p.OtherConditions = otherCond
 		leftCond = leftPushCond
 		rightCond = rightPushCond
+		if p.JoinType == AntiSemiJoin {


I think this might be too strict. Look at the explain-test(tpc-h q21)this PR has affected.

select s_name, count(*) as numwait from supplier, lineitem l1, orders, nation where s_suppkey = l1.l_suppkey and o_orderkey = l1.l_orderkey and o_orderstatus = 'F' and l1.l_receiptdate > l1.l_commitdate and exists ( select * from lineitem l2 where l2.l_orderkey = l1.l_orderkey and l2.l_suppkey <> l1.l_suppkey ) and not exists ( select * from lineitem l3 where l3.l_orderkey = l1.l_orderkey and l3.l_suppkey <> l1.l_suppkey and l3.l_receiptdate > l3.l_commitdate ) and s_nationkey = n_nationkey and n_name = 'EGYPT' group by s_name

After this change, the subquery condition l3.l_receiptdate > l3.l_commitdate cannot be pushed down. This can casue a performance regression.

Actually, although not exists and not in both use anti-semi-join, but they are not totally equal. I think we'd better fix this issue for not in, and not affect the behavior of not exists.

Here are some reference you can take a look:
This is how mysql deals with exists strategy.
https://dev.mysql.com/doc/refman/8.0/en/subquery-optimization-with-exists.html

And this is how SparkSQL uses a null aware anti join for not in(I prefer this method to rewrite the not in condition). https://databricks-prod-cloudfront.cloud.databricks.com/public/4027ec902e239c93eaaa8714f173bcfc/2728434780191932/1483312212640900/6987336228780374/latest.html

Here is how null aware anti join works (quoted from Apache Spark):

Expand the NOT IN expression with the NULL-aware semantic to its full form. That is from: (a1,a2,...) = (b1,b2,...) to (a1=b1 OR isnull(a1=b1)) AND (a2=b2 OR isnull(a2=b2)) AND ...

Thanks，maybe I need to rehink about it. Please remove the review request.This commit is too hasty.

You can check the InOperand field of RightConditions's arguments to decide if we should disable pushdown for it. InOperand is introduced to differentiate the AntiSemiJoin / LeftAntiSemiJoin / AntiLeftOuterSemiJoin generated from NOT IN and NOT EXISTS.

Signed-off-by: jingyugao <1121087373@qq.com>

jingyugao · 2019-09-13T16:15:38Z

/rebuild

Signed-off-by: jingyugao <1121087373@qq.com>

jingyugao · 2019-09-14T02:17:07Z

cmd/explaintest/r/explain_easy.result

-└─Projection_22	10000.00	root	cast(5_aux_0)
-  └─HashLeftJoin_21	10000.00	root	CARTESIAN left outer semi join, inner:TableReader_20
+└─Projection_21	10000.00	root	cast(5_aux_0)
+  └─HashLeftJoin_20	10000.00	root	CARTESIAN left outer semi join, inner:TableReader_19, other cond:eq(6, test.t2.c2)
    ├─IndexReader_17	10000.00	root	index:IndexScan_16


here the eq(6, test.t2.c2) is moved into other condition because it is from 6 in (subq)

jingyugao · 2019-09-14T02:19:00Z

cmd/explaintest/r/explain_easy.result

 node [style=filled, color=lightgrey]
 color=black
 label = "cop"
-"Selection_13" -> "TableScan_12"


Selection_13 is eq(1,t2.c2) which is from 1 in (subq)

jingyugao · 2019-09-14T02:19:28Z

cmd/explaintest/r/explain_easy_stats.result

@@ -123,12 +123,11 @@ set @@session.tidb_opt_insubq_to_join_and_agg=0;
 explain select 1 in (select c2 from t2) from t1;
 id	count	task	operator info
 Projection_6	1999.00	root	5_aux_0
-└─HashLeftJoin_7	1999.00	root	CARTESIAN left outer semi join, inner:TableReader_14
+└─HashLeftJoin_7	1999.00	root	CARTESIAN left outer semi join, inner:TableReader_13, other cond:eq(1, test.t2.c2)


eq(1, test.t2.c2) is from 1 in (subq)

jingyugao · 2019-09-14T02:20:17Z

cmd/explaintest/r/explain_easy_stats.result

 node [style=filled, color=lightgrey]
 color=black
 label = "cop"
-"Selection_13" -> "TableScan_12"


Selection_13 is from 1 in (subq)

jingyugao · 2019-09-14T02:22:10Z

executor/join_test.go

+	tk.MustQuery("select 1 in (select b from t2) from t1").Check(testkit.Rows("<nil>"))
+	tk.MustQuery("select 1 not in (select b from t2) from t1").Check(testkit.Rows("<nil>"))
+	// TODO: this query will cause an index out of range panic
+	// tk.MustQuery("select 1 not in (select null from t1) from t2").Check(testkit.Rows())


this is another bug when executing physical_plan,so I can't add this test case.

Signed-off-by: jingyugao <1121087373@qq.com>

eurekaka · 2019-11-15T11:48:34Z

planner/core/logical_plan_builder.go

@@ -299,7 +303,7 @@ func (p *LogicalJoin) extractOnCondition(conditions []expression.Expression, der
 					// false even if t.a is null or s.a is null. To make this join "empty aware",
 					// we should differentiate `t.a = s.a` from other column equal conditions, so
 					// we put it into OtherConditions instead of EqualConditions of join.
-					if binop.FuncName.L == ast.EQ && !arg0.InOperand && !arg1.InOperand {
+					if binop.FuncName.L == ast.EQ {


Please move the comments here to the front as well.

eurekaka · 2019-11-15T11:51:16Z

expression/expression.go

-	cols := make([]*Column, 0, 1)
-	cols = ExtractColumnsFromExpressions(cols, sf.GetArgs(), isColumnInOperand)
-	return len(cols) > 0
+	return exprsContainInOperand(sf.GetArgs())


This change is unnecessary now?

eurekaka · 2019-11-15T11:52:24Z

expression/util.go

@@ -114,6 +114,22 @@ func ExtractColumns(expr Expression) []*Column {
 	return extractColumns(result, expr, nil)
 }

+func exprsContainInOperand(exprs []Expression) bool {


winoros · 2019-11-15T12:06:11Z

Some comments not about the main changes.
Since the special condition cannot be pushed down to the join's children.
We can let the join have one more field like NullSensitiveConditions and put them into this field.
Then the InOperand field can be removed from expression.Column?
@eurekaka @lamxTyler @francis0407

expression/expression.go

fzhedu · 2019-11-15T13:32:18Z

planner/core/expression_rewriter.go

-			if ok && mysql.HasNotNullFlag(lCol.GetType().Flag) && mysql.HasNotNullFlag(rCol.GetType().Flag) {
+			// If both input columns of `!= all / = any` expression are not null, we can treat the expression
+			// as normal column equal condition.
+			if mysql.HasNotNullFlag(rCol.GetType().Flag) && expression.NotNull(larg) {


After considering scalar function, eq(Column#1, Column#4) is treated as euqal condition now.But how can it be converted to inner join?

I find that () is true is the only special case where the in subquery can be converted to exists subquery . In other most cases, the in subquery should return null, false or true in the projection clause.
So we may ignore the optimization for () is true here.

eurekaka · 2019-11-18T03:10:25Z

Some comments not about the main changes.
Since the special condition cannot be pushed down to the join's children.
We can let the join have one more field like NullSensitiveConditions and put them into this field.
Then the InOperand field can be removed from expression.Column?
@eurekaka @lamxTyler @francis0407

Yes, code would be cleaner then. We can do it after this PR get merged.

fzhedu · 2019-11-18T06:36:06Z

I find the converted conditions from in subquery in semi join are pushed down. I wonder whether this would induce errors? (eq(2, Column#4))

mysql> explain select * from s where (2 in (select a from t where (s.b=t.b) is null));
+------------------------+-------+-----------+--------------------------------------------------------------------------------------+
| id                     | count | task      | operator info                                                                        |
+------------------------+-------+-----------+--------------------------------------------------------------------------------------+
| HashLeftJoin_9         | 4.00  | root      | CARTESIAN semi join, inner:TableReader_14, other cond:isnull(eq(Column#2, Column#5)) |
| ├─TableReader_11       | 5.00  | root      | data:TableScan_10                                                                    |
| │ └─TableScan_10       | 5.00  | cop[tikv] | table:s, range:[-inf,+inf], keep order:false, stats:pseudo                           |
| └─TableReader_14       | 0.01  | root      | data:Selection_13                                                                    |
|   └─Selection_13       | 0.01  | cop[tikv] | eq(2, Column#4)                                                                      |
|     └─TableScan_12     | 5.00  | cop[tikv] | table:t, range:[-inf,+inf], keep order:false, stats:pseudo                           |
+------------------------+-------+-----------+--------------------------------------------------------------------------------------+
6 rows in set (0.00 sec)

fzhedu · 2019-11-18T06:45:46Z

Some comments not about the main changes.
Since the special condition cannot be pushed down to the join's children.
We can let the join have one more field like NullSensitiveConditions and put them into this field.
Then the InOperand field can be removed from expression.Column?
@eurekaka @lamxTyler @francis0407

Yes, code would be cleaner then. We can do it after this PR get merged.

I also got the same way to store the null-aware conditions, and they cannot be pushed down. Adding a nullAwareConditions can also solve the bugs solved by this PR. But some conditions in nullAwareConditions can also be pushed down, like the referred columns are not null. So it also needs to add the func NotNull(l Expression) bool {} functions in this PR for nullAwareConditions to push down the non-null conditions.

winoros · 2019-11-18T06:51:15Z

@fzhedu
When it's not null, inOperand is also false currently. This won't generate new cases which need to be special handled.

winoros · 2019-11-18T06:53:42Z

expression/expression.go

+	case *CorrelatedColumn:
+		return mysql.HasNotNullFlag(l.GetType().Flag)
+	case *ScalarFunction:
+		for _, arg := range l.GetArgs() {


Though the current implementation may satisfy the InSubquery's needs. But for the real NotNull property of scalar function, we need to take the IsNull sig in to special consideration at least.

yes, for example arg is true is not null when arg is null.

fzhedu · 2019-11-18T07:01:26Z

@fzhedu
When it's not null, inOperand is also false currently. This won't generate new cases which need to be special handled.

Yes. The way in this PR and adding a nullAwareConditions are two ways to solve the current bugs about null in subquery. Such two ways both needs to push down the non-null conditions converted from in subqueries. This is done by setting inOperand in this PR.
It is better to take the second way? @winoros

jingyugao · 2019-11-18T09:49:21Z

explain select * from s where (2 in (select a from t where (s.b=t.b) is null));

This pr seems not change this behavior. Execute plan is same between master and this pr.

fzhedu · 2019-11-26T08:24:56Z

I found the PR also has errors, please see #13743

sre-bot · 2020-01-04T13:38:37Z

@jingyugao, please update your pull request.

sre-bot · 2020-01-11T13:39:10Z

@jingyugao, please update your pull request.

sre-bot · 2020-01-27T13:39:21Z

@jingyugao PR closed due to no update for a long time. Feel free to reopen it anytime.

sre-bot added the contribution This PR is from a community contributor. label Sep 8, 2019

jingyugao force-pushed the issue#12074 branch from 90f89be to 9ac89fe Compare September 8, 2019 09:46

jingyugao force-pushed the issue#12074 branch from 9ac89fe to 6910ffc Compare September 8, 2019 09:48

francis0407 added sig/planner SIG: Planner type/bugfix This PR fixes a bug. labels Sep 8, 2019

francis0407 requested review from winoros, francis0407 and alivxxx September 8, 2019 12:41

francis0407 reviewed Sep 9, 2019

View reviewed changes

francis0407 removed request for winoros and alivxxx September 9, 2019 02:01

dont push down right condition for anti semi join

fb776a6

Signed-off-by: jingyugao <1121087373@qq.com>

jingyugao force-pushed the issue#12074 branch from 6910ffc to fb776a6 Compare September 13, 2019 15:46

jingyugao added 5 commits September 14, 2019 00:29

code format

807d96b

Signed-off-by: jingyugao <1121087373@qq.com>

code format

cdb1a81

Signed-off-by: jingyugao <1121087373@qq.com>

code format

7e25beb

Signed-off-by: jingyugao <1121087373@qq.com>

code format

4aaa082

Signed-off-by: jingyugao <1121087373@qq.com>

check fn is ast.EQ

becd318

jingyugao commented Sep 14, 2019

View reviewed changes

remove em

b7f4e5c

jingyugao force-pushed the issue#12074 branch from 513bfc1 to b7f4e5c Compare September 14, 2019 02:43

jingyugao added 2 commits September 14, 2019 11:54

add test cases

04f85a5

Signed-off-by: jingyugao <1121087373@qq.com>

code format

f8b7619

Signed-off-by: jingyugao <1121087373@qq.com>

jingyugao force-pushed the issue#12074 branch from d190c25 to f8b7619 Compare September 15, 2019 03:32

fix review comment

9dca971

eurekaka reviewed Nov 15, 2019

View reviewed changes

jingyugao added 2 commits November 15, 2019 20:12

tiny refactor

84cbce5

fix comment

f75d898

fzhedu reviewed Nov 15, 2019

View reviewed changes

expression/expression.go Show resolved Hide resolved

fzhedu reviewed Nov 15, 2019

View reviewed changes

jingyugao added 2 commits November 16, 2019 13:07

consider scalar function

82e2c4e

Merge branch 'master' into issue#12074

f7f3f51

winoros reviewed Nov 18, 2019

View reviewed changes

jingyugao added 2 commits November 22, 2019 10:57

merge master

994a73f

fix conflict

30387da

qw4990 removed their request for review November 26, 2019 07:39

fzhedu mentioned this pull request Nov 26, 2019

planner: don't push down conditions from in subquery with aggregators even though the columns in conditions is not null #13743

Closed

zz-jason added the status/ReqChange label Dec 28, 2019

XuHuaiyu removed their request for review January 2, 2020 03:13

sre-bot closed this Jan 27, 2020

eurekaka mentioned this pull request Aug 31, 2020

planner: don't push down null sensitive join conditions #19620

Merged

This was referenced Nov 12, 2020

planner: don't push down null sensitive join conditions (#19620) #21018

Closed

planner: don't push down null sensitive join conditions (#19620) #21019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner:dont push down right condition for anti semi join #12075

planner:dont push down right condition for anti semi join #12075

jingyugao commented Sep 8, 2019

codecov bot commented Sep 8, 2019 •

edited

Loading

francis0407 Sep 9, 2019

francis0407 Sep 9, 2019

jingyugao Sep 9, 2019

eurekaka Sep 10, 2019 •

edited

Loading

jingyugao commented Sep 13, 2019

jingyugao Sep 14, 2019

jingyugao Sep 14, 2019

jingyugao Sep 14, 2019

jingyugao Sep 14, 2019

jingyugao Sep 14, 2019

eurekaka Nov 15, 2019

jingyugao Nov 20, 2019

eurekaka Nov 15, 2019

eurekaka Nov 15, 2019

winoros commented Nov 15, 2019 •

edited

Loading

fzhedu Nov 15, 2019 •

edited

Loading

jingyugao Nov 16, 2019

fzhedu Nov 18, 2019

eurekaka commented Nov 18, 2019

fzhedu commented Nov 18, 2019 •

edited

Loading

fzhedu commented Nov 18, 2019 •

edited

Loading

winoros commented Nov 18, 2019

winoros Nov 18, 2019

jingyugao Nov 18, 2019

fzhedu commented Nov 18, 2019 •

edited

Loading

jingyugao commented Nov 18, 2019

fzhedu commented Nov 26, 2019

sre-bot commented Jan 4, 2020

sre-bot commented Jan 11, 2020

sre-bot commented Jan 27, 2020

planner:dont push down right condition for anti semi join #12075

planner:dont push down right condition for anti semi join #12075

Conversation

jingyugao commented Sep 8, 2019

What problem does this PR solve?

What is changed and how it works?

Check List

codecov bot commented Sep 8, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eurekaka Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

jingyugao commented Sep 13, 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

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

winoros commented Nov 15, 2019 • edited Loading

fzhedu Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eurekaka commented Nov 18, 2019

fzhedu commented Nov 18, 2019 • edited Loading

fzhedu commented Nov 18, 2019 • edited Loading

winoros commented Nov 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fzhedu commented Nov 18, 2019 • edited Loading

jingyugao commented Nov 18, 2019

fzhedu commented Nov 26, 2019

sre-bot commented Jan 4, 2020

sre-bot commented Jan 11, 2020

sre-bot commented Jan 27, 2020

codecov bot commented Sep 8, 2019 •

edited

Loading

eurekaka Sep 10, 2019 •

edited

Loading

winoros commented Nov 15, 2019 •

edited

Loading

fzhedu Nov 15, 2019 •

edited

Loading

fzhedu commented Nov 18, 2019 •

edited

Loading

fzhedu commented Nov 18, 2019 •

edited

Loading

fzhedu commented Nov 18, 2019 •

edited

Loading