expression: propagate more filters in PropagateConstant #7276

bb7133 · 2018-08-04T05:21:38Z

What have you changed? (mandatory)

This PR try to propagate more filters/constraints in PropagateConstant to archive the optimization mentioned in #7098. As long as an expression is deterministic, it can be propagated through the equality condition.

For example(the case in #7098), before this PR:

TiDB(localhost:4000) > desc select * from t1 join t2 on t1.a=t2.a and t1.a in (12, 13) and t2.a in (14, 15);
+-------------------------+----------+------+--------------------------------------------------------------------+
| id                      | count    | task | operator info                                                      |
+-------------------------+----------+------+--------------------------------------------------------------------+
| HashLeftJoin_6          | 25.00    | root | inner join, inner:TableReader_13, equal:[eq(test.t1.a, test.t2.a)] |
| ├─TableReader_10        | 20.00    | root | data:Selection_9                                                   |
| │ └─Selection_9         | 20.00    | cop  | in(test.t1.a, 12, 13)                                              |
| │   └─TableScan_8       | 10000.00 | cop  | table:t1, range:[-inf,+inf], keep order:false, stats:pseudo        |
| └─TableReader_13        | 20.00    | root | data:Selection_12                                                  |
|   └─Selection_12        | 20.00    | cop  | in(test.t2.a, 14, 15)                                              |
|     └─TableScan_11      | 10000.00 | cop  | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo        |
+-------------------------+----------+------+--------------------------------------------------------------------+
7 rows in set (0.00 sec)

After this PR, we have:

TiDB(localhost:4000) > desc select * from t1 join t2 on t1.a=t2.a and t1.a in (12, 13) and t2.a in (14, 15);
+-------------------------+----------+------+--------------------------------------------------------------------+
| id                      | count    | task | operator info                                                      |
+-------------------------+----------+------+--------------------------------------------------------------------+
| HashLeftJoin_6          | 0.00     | root | inner join, inner:TableReader_13, equal:[eq(test.t1.a, test.t2.a)] |
| ├─TableReader_10        | 0.00     | root | data:Selection_9                                                   |
| │ └─Selection_9         | 0.00     | cop  | in(test.t1.a, 12, 13), in(test.t1.a, 14, 15)                       |
| │   └─TableScan_8       | 10000.00 | cop  | table:t1, range:[-inf,+inf], keep order:false, stats:pseudo        |
| └─TableReader_13        | 0.00     | root | data:Selection_12                                                  |
|   └─Selection_12        | 0.00     | cop  | in(test.t2.a, 14, 15), in(test.t2.a, 12, 13)                       |
|     └─TableScan_11      | 10000.00 | cop  | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo        |
+-------------------------+----------+------+--------------------------------------------------------------------+
7 rows in set (0.00 sec)

However, propagation should be avoid for conditions with non-deterministic expression like rand(). A nonDeterministicFuncNameMap is added to make sure those expressions are excluded.

What is the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Some unit test cases are added

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)

No

Refer to a related PR or issue link (optional)

#7098

Add a few positive/negative examples (optional)

Since we try to do propagations as much as possible, some propagated filters may be un-necessary, or at least can be folded/combined, for example:

mysql> desc select * from t t1, t t2 where t1.a = t2.a and t1.a < t2.a + 1;
+--------------------------+----------+------+----------------------------------------------------------------------------------------------+
| id                       | count    | task | operator info                                                                                |
+--------------------------+----------+------+----------------------------------------------------------------------------------------------+
| HashLeftJoin_8           | 10000.00 | root | inner join, inner:TableReader_15, equal:[eq(t1.a, t2.a)], other cond:lt(t1.a, plus(t2.a, 1)) |
| ├─TableReader_12         | 8000.00  | root | data:Selection_11                                                                            |
| │ └─Selection_11         | 8000.00  | cop  | lt(t1.a, plus(t1.a, 1))                                                                      |
| │   └─TableScan_10       | 10000.00 | cop  | table:t1, range:[-inf,+inf], keep order:false, stats:pseudo                                  |
| └─TableReader_15         | 8000.00  | root | data:Selection_14                                                                            |
|   └─Selection_14         | 8000.00  | cop  | lt(t2.a, plus(t2.a, 1))                                                                      |
|     └─TableScan_13       | 10000.00 | cop  | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo                                  |
+--------------------------+----------+------+----------------------------------------------------------------------------------------------+
7 rows in set (0.00 sec)

zz-jason · 2018-08-04T06:11:03Z

/run-all-tests

zz-jason · 2018-08-04T08:17:39Z

expression/constant_propagation.go

-	ast.GE: true,
-	ast.NE: true,
+// nonDeterministicFuncNameMap stores all the non-deterministic operators that cannot be propagated.
+var nonDeterministicFuncNameMap = map[string]bool{


Maybe we can use unFoldableFunctions in "expression/function_traits.go" to replace this map?

zz-jason · 2018-08-04T08:21:44Z

expression/constant_propagation.go

+
+	condsLen := len(s.conditions)
+	for j, colj := range s.columns {
+		for k, colk := range s.columns {


how about:

for j, colj := range s.columns { for k := j+1; k < len(s.columns); k++ { ... replaced, _, newExpr := s.tryToReplaceCond(colj, colk, cond) replaced, _, newExpr := s.tryToReplaceCond(colk, colj, cond)

Good idea! thank you!

zz-jason · 2018-08-04T08:22:27Z

expression/constant_propagation.go

+func (s *propagateConstantSolver) tryToReplaceCond(src *Column, tgt *Column, cond Expression) (bool, bool, Expression) {
+	replaced := false
+	var r *ScalarFunction
+	if funct, ok := cond.(*ScalarFunction); ok {


s/funct/sf/?

shenli · 2018-08-05T07:22:24Z

@bb7133 Thanks! Please fix the CI.

winoros · 2018-08-06T07:53:55Z

/run-unit-test

winoros · 2018-08-07T05:39:47Z

/run-unit-test

winoros · 2018-08-07T05:57:33Z

expression/constant_propagation.go

+		if _, ok := unFoldableFunctions[sf.FuncName.L]; ok {
+			return false, true, cond
+		}
+		if _, isEq := eqFuncNameMap[sf.FuncName.L]; isEq {


just sf.FuncName.L == ast.EQ is okay. And add comment why we skip it.

Should we still keep eqFuncNameMap? @winoros

@bb7133 Seems that it can be removed. validPropagateCond now is only used to check eq cond. You can modify this method and remove the map.

Thanks! @winoros

winoros · 2018-08-07T06:01:39Z

expression/constant_propagation.go

+				if r == nil {
+					r = sf.Clone().(*ScalarFunction)
+				}
+				r.GetArgs()[idx] = tgt


You'd better call NewFunction. Don't set scalar function's arg.

I see, it's better to treat scalar function immutable

winoros · 2018-08-07T06:01:46Z

expression/constant_propagation.go

+					if r == nil {
+						r = sf.Clone().(*ScalarFunction)
+					}
+					r.GetArgs()[idx] = subExpr


winoros · 2018-08-10T11:45:26Z

@bb7133
Please fix the test

FAIL: constant_test.go:56: testExpressionSuite.TestConstantPropagation

constant_test.go:158:
    c.Assert(strings.Join(result, ", "), Equals, tt.result, Commentf("different for expr %s", tt.conditions))
... obtained string = "eq(test.t.0, test.t.1), le(mul(test.t.0, test.t.0), 50), le(mul(test.t.1, test.t.0), 50)"
... expected string = "eq(test.t.0, test.t.1), le(mul(test.t.0, test.t.0), 50), le(mul(test.t.1, test.t.1), 50)"
... different for expr [eq(test.t.0, test.t.1) le(mul(test.t.0, test.t.0), 50)]

You can run go test locally to test it.

bb7133 · 2018-08-11T01:29:39Z

Fixed, PTAL @winoros, thanks!

zz-jason · 2018-08-11T01:44:38Z

/run-all-tests

zz-jason · 2018-08-11T01:47:34Z

LGTM

XuHuaiyu · 2018-08-13T07:05:02Z

expression/constant_propagation.go

 	if eq, ok := cond.(*ScalarFunction); ok {
-		if _, ok := funNameMap[eq.FuncName.L]; !ok {
+		if eq.FuncName.L != ast.EQ {


update the comment of this function since we do not consider non-equal ops in this function.

s/ validPropagateCond/ validEqualCond?

XuHuaiyu · 2018-08-13T07:07:55Z

expression/constant_propagation.go

+//  bool: if 'cond' contains non-deterministic expression
+//  Expression: the replaced expression, or original 'cond' if the replacement is not happened
+func (s *propagateConstantSolver) tryToReplaceCond(src *Column, tgt *Column, cond Expression) (bool, bool, Expression) {
+	if sf, ok := cond.(*ScalarFunction); ok {


sf, ok := cond.(*ScalarFunction); !ok {
return false, false, cond
}
......

XuHuaiyu · 2018-08-13T07:09:41Z

expression/constant_propagation.go

+		if _, ok := unFoldableFunctions[sf.FuncName.L]; ok {
+			return false, true, cond
+		}
+		// Equality is handled in propagateEQ already


propagateEQ only handles equal functions like column = constant but not handle the case like
a = b and b = funcname(), should we consider it here?

Or may be we do not need propagateEQ now?

There are many optimizations we can consider in constant propagation and now I think it may be a good idea to have a doc so we can:

Make further discussions and details.

Make a list of subtasks.

I will try to give this initial doc soon. Your comment is appreciated, thanks

I've updated the code to support b = func() case, thanks

XuHuaiyu · 2018-08-13T07:25:11Z

expression/constant_propagation.go

+				continue
+			}
+			colk := s.columns[k]
+			for i := 0; i < condsLen; i++ {


s/ condsLen/ len(s.conditions)
and remove line 107
since we only use it here.

I think it's better to keep condsLen because we're adding elements to conditions while iterating it. Iteration on new elements are not needed.

We need to comment for the behavior of the 3-layer loop.

addressed, thanks

XuHuaiyu · 2018-08-13T07:32:00Z

expression/constant_propagation.go

+// tryToReplaceCond aims to replace all occurances of column 'src' and try to replace it with 'tgt' in 'cond'
+// It returns
+//  bool: if a replacement happened
+//  bool: if 'cond' contains non-deterministic expression


It seems that we can remove this return value?

This boolean value is used to avoid some bad case like: a = b and cast(b, int) < rand(), for which we cannot propagate a cast(a, int) < rand().

There may be more sophisticated solution, do you have any advise? thanks!

ok, never mind

XuHuaiyu · 2018-08-13T07:37:10Z

expression/constant_propagation.go

@@ -158,6 +146,51 @@ func (s *propagateConstantSolver) validPropagateCond(cond Expression, funNameMap
 	return nil, nil
 }

+// tryToReplaceCond aims to replace all occurances of column 'src' and try to replace it with 'tgt' in 'cond'


We need a more detailed comment here,
an example may help.

Yeah,I will try to state more details

winoros · 2018-08-13T12:14:49Z

/unit-test

zz-jason · 2018-08-20T03:42:00Z

@bb7133 any update?

bb7133 · 2018-08-29T06:02:30Z

code updated, sorry it costed a long time @zz-jason

…onstant

1. add propagation for EQ condition like 'a eq func' 2. refined some method/variable names 3. added more comments 4. added 1 more unit test Signed-off-by: bb7133 <bb7133@gmail.com>

shenli · 2018-08-30T05:42:45Z

/run-all-tests

zz-jason · 2018-08-30T06:47:19Z

expression/constant_propagation.go

+// e.g. For expression a = b and b = c and c = d and c < 1 , we can get extra a < 1 and b < 1 and d < 1.
+// However, for a = b and a < rand(), we cannot propagate a < rand() to b < rand() because rand() is non-deterministic
+//
+// This propagation may bring redundancies that we need to resolve later, for example:


In the present planner, we can do this by adding a post-process after the logical optimization, before deriving the statistics and build index/table ranges.

In the new cascades planner, we can do this by adding a rule which only works on a Filter operator and remove the duplicated filters.

@bb7133 What's your opinion?

Current ranger can spot these redundant filters and merge them?

@eurekaka maybe we can do the enhancement in ranger?

Seems like ranger would only simplify and merge these filters when they are considered to be index filters, if there is no index and no int primary key, these filters would be in PhysicalSelection as what they are originally; maybe we should extract the range simplification logic out of ranger, and this should be like the 'post-process' you mentioned above.

@zz-jason Sounds interesting! And looks @tiancaiamao is trying to solve (at least part of) the redundant filters in his PR, which we can expect.

zz-jason · 2018-08-30T06:47:52Z

expression/constant_propagation.go

+// for a = b and a < 3 and b < 4, we get new a < 4 and b < 3 but should expect a < 3 and b < 3
+// for a = b and a in (3) and b in (4), we get b in (3) and a in (4) but should expect 'false'
+//
+// TODO: remove redundancies later


It's better to add a github issue for this TODO item.

zz-jason · 2018-08-30T06:50:26Z

@winoros @XuHuaiyu PTAL

tiancaiamao · 2018-08-30T06:53:58Z

FYI: I'm rewriting a much more aggressive constant propagate... @bb7133 @zz-jason
Hope I could to pull the request today.

zz-jason · 2018-08-30T06:57:05Z

@tiancaiamao What's the idea of the "aggressive constant propagate"?

tiancaiamao · 2018-08-30T07:04:45Z

Current constant propagate doesn't infer much on InEQ condition:

"a > 3" && "a > 5" => "a > 5"
"a > 5" && "a < 3" => false

And it doesn't handle function:

to_days(t) > date '2018-03-05' && to_days(t) < '2018-03-01' => false

I'm writing an "aggressive" one to solve this issue #7516

lgtm

XuHuaiyu

LGTM

eurekaka · 2018-08-30T12:50:07Z

LGTM

zz-jason · 2018-08-30T14:00:15Z

/run-all-tests

bb7133 · 2018-08-30T15:35:10Z

Well, @tiancaiamao your idea is mentioned in my TODO list in the comment, it would be great if you can solve it. BTW, I wrote a doc about my opinions on related optimizations: https://docs.google.com/document/d/1G3wVBaiza9GI5q9nwHLCB2I7r1HUbg6RpTNFpQWTFhQ/edit?usp=sharing

zz-jason self-assigned this Aug 4, 2018

zz-jason added contribution This PR is from a community contributor. sig/planner SIG: Planner type/enhancement The issue or PR belongs to an enhancement. labels Aug 4, 2018

zz-jason reviewed Aug 4, 2018

View reviewed changes

winoros changed the title ~~expression/constant_propagation: propagate more filters in PropagateC…~~ expression: propagate more filters in PropagateConstant Aug 7, 2018

winoros reviewed Aug 7, 2018

View reviewed changes

winoros previously requested changes Aug 7, 2018

View reviewed changes

bb7133 force-pushed the bb7133/propagation branch from 115b0b0 to e5a08a7 Compare August 10, 2018 08:57

bb7133 force-pushed the bb7133/propagation branch from e5a08a7 to 49a99ea Compare August 10, 2018 16:20

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 11, 2018

XuHuaiyu reviewed Aug 13, 2018

View reviewed changes

bb7133 force-pushed the bb7133/propagation branch from 5dcb6eb to d7b470a Compare August 29, 2018 06:03

bb7133 added 5 commits August 29, 2018 23:01

expression/constant_propagation: propagate more filters in PropagateC…

a739864

…onstant

Updates based on CR

4d7d0dd

Updates based on CR #2

d599bb1

Bug fix

4f153e4

some updates on XuHuaiyu's comments:

fd15734

1. add propagation for EQ condition like 'a eq func' 2. refined some method/variable names 3. added more comments 4. added 1 more unit test Signed-off-by: bb7133 <bb7133@gmail.com>

bb7133 force-pushed the bb7133/propagation branch from d7b470a to fd15734 Compare August 29, 2018 15:07

Merge branch 'master' into bb7133/propagation

4d56250

shenli assigned XuHuaiyu Aug 30, 2018

Merge branch 'master' into bb7133/propagation

64174cc

zz-jason reviewed Aug 30, 2018

View reviewed changes

XuHuaiyu approved these changes Aug 30, 2018

View reviewed changes

winoros mentioned this pull request Aug 30, 2018

plan: change the logic of converting to index join #7553

Merged

Merge branch 'master' into bb7133/propagation

c7579ec

eurekaka added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 30, 2018

Merge branch 'master' into bb7133/propagation

7cfb801

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

zz-jason mentioned this pull request Aug 30, 2018

TiDB logical optimization: extend outer join elimination #7559

Closed

zz-jason merged commit 7f37bad into pingcap:master Aug 30, 2018

bb7133 deleted the bb7133/propagation branch September 8, 2018 09:26

eurekaka mentioned this pull request Sep 27, 2018

plan: propagate constant over outer join #7794

Merged

expression: propagate more filters in PropagateConstant #7276

expression: propagate more filters in PropagateConstant #7276

Conversation

bb7133 commented Aug 4, 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)

Refer to a related PR or issue link (optional)

Add a few positive/negative examples (optional)

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

shenli commented Aug 5, 2018

winoros commented Aug 6, 2018

winoros 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

bb7133 Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros commented Aug 10, 2018

bb7133 commented Aug 11, 2018

zz-jason commented Aug 11, 2018

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

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 Aug 13, 2018

zz-jason commented Aug 20, 2018

bb7133 commented Aug 29, 2018

shenli commented Aug 30, 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 30, 2018

tiancaiamao commented Aug 30, 2018

zz-jason commented Aug 30, 2018

tiancaiamao commented Aug 30, 2018

XuHuaiyu left a comment

Choose a reason for hiding this comment

eurekaka commented Aug 30, 2018

zz-jason commented Aug 30, 2018

bb7133 commented Aug 30, 2018

bb7133 commented Aug 4, 2018 •

edited

Loading

bb7133 Aug 7, 2018 •

edited

Loading