-
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 12 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 |
---|---|---|
|
@@ -86,19 +86,36 @@ func foldConstant(expr Expression) (Expression, bool) { | |
} | ||
|
||
args := x.GetArgs() | ||
canFold := true | ||
sc := x.GetCtx().GetSessionVars().StmtCtx | ||
nonConstArgIdx := make([]int, 0, len(args)) | ||
hasNullArg := false | ||
isDeferredConst := false | ||
for i := 0; i < len(args); i++ { | ||
foldedArg, isDeferred := foldConstant(args[i]) | ||
x.GetArgs()[i] = foldedArg | ||
_, conOK := foldedArg.(*Constant) | ||
if !conOK { | ||
canFold = false | ||
con, conOK := foldedArg.(*Constant) | ||
if conOK { | ||
if con.Value.IsNull() { | ||
hasNullArg = true | ||
} | ||
} else { | ||
nonConstArgIdx = append(nonConstArgIdx, i) | ||
} | ||
isDeferredConst = isDeferredConst || isDeferred | ||
} | ||
if !canFold { | ||
return expr, isDeferredConst | ||
if len(nonConstArgIdx) > 0 { | ||
if !hasNullArg || !sc.InNullRejectCheck { | ||
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. Do we need to check whether it's in null-reject? 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. Yes we need, for cases like |
||
return expr, isDeferredConst | ||
} | ||
dummyScalarFunc, _ := x.Clone().(*ScalarFunction) | ||
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 use 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. Updated |
||
for _, idx := range nonConstArgIdx { | ||
dummyScalarFunc.GetArgs()[idx] = One | ||
} | ||
value, err := dummyScalarFunc.Eval(chunk.Row{}) | ||
if err != nil || !value.IsNull() { | ||
return expr, isDeferredConst | ||
} | ||
return &Constant{Value: value, RetType: x.RetType}, false | ||
} | ||
value, err := x.Eval(chunk.Row{}) | ||
if err != nil { | ||
|
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,58 @@ 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", | ||
}, | ||
} | ||
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 |
---|---|---|
|
@@ -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.
how about:
Maybe this is more clearer.
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.
thanks, patch updated