-
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
Conversation
only do null fold in null reject check
plan cache test should pass now, reopen it
/run-all-tests |
80a48f9
to
c9b6239
Compare
/run-all-tests |
f6008c6
to
0304e81
Compare
expression/constant_fold.go
Outdated
if !hasNullArg || !sc.InNullRejectCheck { | ||
return expr, isDeferredConst | ||
} | ||
dummyScalarFunc, _ := x.Clone().(*ScalarFunction) |
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.
Can we use NewFunction
instead?
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.
Updated
expression/constant_fold.go
Outdated
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we need, for cases like select null and a < 1 from t;
, if we fold null constant for the projection part, the result would be null
for all rows, but actually for t.a >= 1
, the result should be 0
fold null if result is null or false
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.
lgtm
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
If we make it a parameter of foldConstant
, we have to modify signature of FoldConstant
, NewFunction
, NewFunctionInternal
, EvaluateExprWithNull
, this would lead to a lot of unnecessary code changes
if argIsConst[i] { | ||
constArgs[i] = arg | ||
} else { | ||
constArgs[i] = One |
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
and is 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.
@@ -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 comment
The 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 comment
The 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.
/run-all-tests |
expression/constant_fold.go
Outdated
if !conOK { | ||
canFold = false | ||
con, conOK := foldedArg.(*Constant) | ||
if conOK { |
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:
argIsConst[i] = conOk
allConstArg = allConstArg && conOk
hasNullArg = hasNullArg || (conOk && con.Value.IsNull())
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
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 comment
The reason will be displayed to describe this comment to others. Learn more.
should it be err == nil && isTrue != 0
?
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.
isTrue != 0
means the where condition is true if the inner table columns are null, so it cannot be rejected?
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.
LGTM
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.
LGTM
What problem does this PR solve?
Fix #7679
Currently,
foldConstant
only handles expressions likeconstant op constant
, for expressions likeconstant op column
, it does nothing. Actually, ifconstant
inconstant op column
isnull
, for a lot of operators, this expression can be folded intonull
. This PR works on this enhancement.What is changed and how it works?
Substitute
expression
inconstant op expression
to constantOne
if theconstant
isnull
, and evaluate the new expression. If the result isnull
, we can fold this expression, otherwise, keep the original expression.Check List
Tests
Side effects