-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696
expression, plan: aggressive constant fold for null parameter expression to simplify outer join #7696
Changes from 23 commits
4653f81
b23e33d
26cc553
6260ca0
ce4033e
615d985
6effd4f
fbef1a3
0304e81
21f75ca
be77917
33190d3
b6825da
2feffbe
9a781b5
fb05e5a
210f8d5
14826cd
84b357c
33c620a
a6302bd
d4a3200
c900e9c
8bbcdfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -286,9 +286,8 @@ func mockContext() sessionctx.Context { | |
func (s *testPlanSuite) TestPredicatePushDown(c *C) { | ||
defer testleak.AfterTest(c)() | ||
tests := []struct { | ||
sql string | ||
first string | ||
best string | ||
sql string | ||
best string | ||
}{ | ||
{ | ||
sql: "select count(*) from t a, t b where a.a = b.a", | ||
|
@@ -548,8 +547,8 @@ func (s *testPlanSuite) TestOuterWherePredicatePushDown(c *C) { | |
}{ | ||
// issue #7628, left join with where condition | ||
{ | ||
sql: "select * from t as t1 left join t as t2 on t1.b = t2.b where (t1.a=1 and t2.a=1) or (t1.a=2 and t2.a=2)", | ||
sel: "[or(and(eq(t1.a, 1), eq(t2.a, 1)), and(eq(t1.a, 2), eq(t2.a, 2)))]", | ||
sql: "select * from t as t1 left join t as t2 on t1.b = t2.b where (t1.a=1 and t2.a is null) or (t1.a=2 and t2.a=2)", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why change this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of this test case is to verify predicate pushdown over outer join, after applying this PR, the outer join would be converted to inner join now, so I changed the where condition to keep the outer join. |
||
sel: "[or(and(eq(t1.a, 1), isnull(t2.a)), and(eq(t1.a, 2), eq(t2.a, 2)))]", | ||
left: "[or(eq(t1.a, 1), eq(t1.a, 2))]", | ||
right: "[]", | ||
}, | ||
|
@@ -560,8 +559,8 @@ func (s *testPlanSuite) TestOuterWherePredicatePushDown(c *C) { | |
right: "[]", | ||
}, | ||
{ | ||
sql: "select * from t as t1 left join t as t2 on t1.b = t2.b where (t1.c=1 and ((t1.a=3 and t2.a=3) or (t1.a=4 and t2.a=4))) or (t1.a=2 and t2.a=2)", | ||
sel: "[or(and(eq(t1.c, 1), or(and(eq(t1.a, 3), eq(t2.a, 3)), and(eq(t1.a, 4), eq(t2.a, 4)))), and(eq(t1.a, 2), eq(t2.a, 2)))]", | ||
sql: "select * from t as t1 left join t as t2 on t1.b = t2.b where (t1.c=1 and ((t1.a=3 and t2.a=3) or (t1.a=4 and t2.a=4))) or (t1.a=2 and t2.a is null)", | ||
sel: "[or(and(eq(t1.c, 1), or(and(eq(t1.a, 3), eq(t2.a, 3)), and(eq(t1.a, 4), eq(t2.a, 4)))), and(eq(t1.a, 2), isnull(t2.a)))]", | ||
left: "[or(and(eq(t1.c, 1), or(eq(t1.a, 3), eq(t1.a, 4))), eq(t1.a, 2))]", | ||
right: "[]", | ||
}, | ||
|
@@ -593,6 +592,63 @@ func (s *testPlanSuite) TestOuterWherePredicatePushDown(c *C) { | |
} | ||
} | ||
|
||
func (s *testPlanSuite) TestSimplifyOuterJoin(c *C) { | ||
defer testleak.AfterTest(c)() | ||
tests := []struct { | ||
sql string | ||
best string | ||
joinType string | ||
}{ | ||
{ | ||
sql: "select * from t t1 left join t t2 on t1.b = t2.b where t1.c > 1 or t2.c > 1;", | ||
best: "Join{DataScan(t1)->DataScan(t2)}(t1.b,t2.b)->Sel([or(gt(t1.c, 1), gt(t2.c, 1))])->Projection", | ||
joinType: "left outer join", | ||
}, | ||
{ | ||
sql: "select * from t t1 left join t t2 on t1.b = t2.b where t1.c > 1 and t2.c > 1;", | ||
best: "Join{DataScan(t1)->DataScan(t2)}(t1.b,t2.b)->Projection", | ||
joinType: "inner join", | ||
}, | ||
{ | ||
sql: "select * from t t1 left join t t2 on t1.b = t2.b where not (t1.c > 1 or t2.c > 1);", | ||
best: "Join{DataScan(t1)->DataScan(t2)}(t1.b,t2.b)->Projection", | ||
joinType: "inner join", | ||
}, | ||
{ | ||
sql: "select * from t t1 left join t t2 on t1.b = t2.b where not (t1.c > 1 and t2.c > 1);", | ||
best: "Join{DataScan(t1)->DataScan(t2)}(t1.b,t2.b)->Sel([not(and(le(t1.c, 1), le(t2.c, 1)))])->Projection", | ||
joinType: "left outer join", | ||
}, | ||
{ | ||
sql: "select * from t t1 left join t t2 on t1.b > 1 where t1.c = t2.c;", | ||
best: "Join{DataScan(t1)->DataScan(t2)}(t1.c,t2.c)->Projection", | ||
joinType: "inner join", | ||
}, | ||
{ | ||
sql: "select * from t t1 left join t t2 on true where t1.b <=> t2.b;", | ||
best: "Join{DataScan(t1)->DataScan(t2)}->Sel([nulleq(t1.b, t2.b)])->Projection", | ||
joinType: "left outer join", | ||
}, | ||
} | ||
for _, ca := range tests { | ||
comment := Commentf("for %s", ca.sql) | ||
stmt, err := s.ParseOneStmt(ca.sql, "", "") | ||
c.Assert(err, IsNil, comment) | ||
p, err := BuildLogicalPlan(s.ctx, stmt, s.is) | ||
c.Assert(err, IsNil, comment) | ||
p, err = logicalOptimize(flagPredicatePushDown|flagPrunColumns, p.(LogicalPlan)) | ||
c.Assert(err, IsNil, comment) | ||
c.Assert(ToString(p), Equals, ca.best, comment) | ||
join, ok := p.(LogicalPlan).Children()[0].(*LogicalJoin) | ||
if !ok { | ||
join, ok = p.(LogicalPlan).Children()[0].Children()[0].(*LogicalJoin) | ||
c.Assert(ok, IsTrue, comment) | ||
} | ||
joinType := fmt.Sprintf("%s", join.JoinType.String()) | ||
c.Assert(joinType, Equals, ca.joinType, comment) | ||
} | ||
} | ||
|
||
func newPartitionInfoSchema(definitions []model.PartitionDefinition) infoschema.InfoSchema { | ||
tableInfo := *MockTable() | ||
cols := make([]*model.ColumnInfo, 0, len(tableInfo.Columns)) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -260,15 +260,18 @@ func simplifyOuterJoin(p *LogicalJoin, predicates []expression.Expression) { | |
// If it is a conjunction containing a null-rejected condition as a conjunct. | ||
// If it is a disjunction of null-rejected conditions. | ||
func isNullRejected(ctx sessionctx.Context, schema *expression.Schema, expr expression.Expression) bool { | ||
expr = expression.PushDownNot(nil, expr, false) | ||
sc := ctx.GetSessionVars().StmtCtx | ||
sc.InNullRejectCheck = true | ||
result := expression.EvaluateExprWithNull(ctx, schema, expr) | ||
sc.InNullRejectCheck = false | ||
x, ok := result.(*expression.Constant) | ||
if !ok { | ||
return false | ||
} | ||
sc := ctx.GetSessionVars().StmtCtx | ||
if x.Value.IsNull() { | ||
return true | ||
} else if isTrue, err := x.Value.ToBool(sc); err != nil || isTrue == 0 { | ||
} else if isTrue, err := x.Value.ToBool(sc); err == nil && isTrue == 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should it be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
return true | ||
} | ||
return false | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,7 @@ type StatementContext struct { | |
UseCache bool | ||
PadCharToFullLength bool | ||
BatchCheck bool | ||
InNullRejectCheck bool | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we make this an input parameter for foldConstant? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make it a parameter of |
||
|
||
// mu struct holds variables that change during execution. | ||
mu struct { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be incorrect if the ScalarFunction is NullEQ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is null
andis not null
should have only one parameter, so they would not fall into code path of this patch IMHO.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mentioned here is the operator
<=>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you are right, will fix this, thanks.