expression, plan: extract the same part from DNF's leaves. #5831

winoros · 2018-02-08T11:41:17Z

e.g.
select * from t1, t2 where (t1.a=t2.a and ...) or (t1.a=t2.a and ...) or (t1.a=t2.a and ...)
We should extract the t1.a=t2.a out of the DNF, so it can be used as the join's join key condition.
And this let tidb push more predicates across the join.

~~I'll add enough tests in next commit.~~
~~Unit test is not easy to see what predicates join holds, add explain tests in integration test is a better choice i think.~~
Unit test added.

PTAL @hanfei1991 @lamxTyler @zz-jason

zz-jason · 2018-02-08T11:45:58Z

expression/scalar_function.go

+	}
+	v := make([]types.Datum, 0, len(sf.GetArgs())+1)
+	var err error
+	sf.hashcode, err = codec.EncodeValue(sc, nil, types.NewStringDatum(sf.FuncName.L))


we'd better avoid to use Datum, it's not efficient.

Any good suggestion? This is just some code i recovered since it has been deleted once.
I came up with the idea that:

Constant: ConstantFlag+encoded value Column: ColumnFlag+encoded value ScalarFunction: SFFlag+encoded function name + 0(or other flag byte) + encoded arg_1 + 0 + ...

This way column and scalarFunction can call EncodeBytes([]byte) directly.

for ScalaFunction: "SFFlag+encoded function name + encoded arg0 + encoded arg1 ... " is distinguishable ?

you're right. I'll do this in later pr.

shenli · 2018-02-08T13:32:45Z

Good job!

hanfei1991 · 2018-02-09T02:18:00Z

expression/constant.go

+		terror.Log(errors.Trace(err))
+	}
+	c.hashcode, err = codec.EncodeValue(sc, nil, c.Value)
+	terror.Log(errors.Trace(err))


Should we check if the err is nil ?

alivxxx

I think that it is better to put the test code as close as possible to the code itself so that future modifications can find if there is something wrong ASAP. We can just write the unit test for extractFiltersFromDNFs.

alivxxx · 2018-02-09T03:19:59Z

plan/predicate_push_down.go

+		if cnt < len(dnfItems) {
+			delete(hashcode2Expr, hashcode)
+		}
+	}


If there is no such item, we can just finish here?

alivxxx · 2018-02-09T03:21:58Z

plan/predicate_push_down.go

+	hashcode2Expr := make(map[string]expression.Expression)
+	firstRun := true
+	for _, dnfItem := range dnfItems {
+		// We need this because there may be the case like `select * from t, t1 where (t.a=t1.a and t.a=t1.a) or (something).


Can we just make sure that no such thing will happen by eliminating them at an earlier stage?

Yes, what you say is a better practice.

winoros · 2018-02-09T05:44:03Z

/run-sqllogic-test

winoros · 2018-02-09T07:29:14Z

For TPCH-10G 19.sql execution time from N/A to 20s:

mysql> select
    sum(l_extendedprice* (1 - l_discount)) as revenue
from
    lineitem,
    part
where
    (
        p_partkey = l_partkey
        and p_brand = 'Brand#52'
        and p_container in ('SM CASE', 'SM BOX', 'SM PACK', 'SM PKG')
        and l_quantity >= 4 and l_quantity <= 4 + 10
        and p_size between 1 and 5
        and l_shipmode in ('AIR', 'AIR REG')
        and l_shipinstruct = 'DELIVER IN PERSON'
    )
    or
    (
        p_partkey = l_partkey
        and p_brand = 'Brand#11'
        and p_container in ('MED BAG', 'MED BOX', 'MED PKG', 'MED PACK')
        and l_quantity >= 18 and l_quantity <= 18 + 10
        and p_size between 1 and 10
        and l_shipmode in ('AIR', 'AIR REG')
        and l_shipinstruct = 'DELIVER IN PERSON'
    )
    or
    (
        p_partkey = l_partkey
        and p_brand = 'Brand#51'
        and p_container in ('LG CASE', 'LG BOX', 'LG PACK', 'LG PKG')
        and l_quantity >= 29 and l_quantity <= 29 + 10
        and p_size between 1 and 15
     ->     sum(l_extendedprice* (1 - l_discount)) as revenue
    -> from
    ->     lineitem,
    ->     part
    -> where
    ->     (
    ->         p_partkey = l_partkey
    ->         and p_brand = 'Brand#52'
    ->         and p_container in ('SM CASE', 'SM BOX', 'SM PACK', 'SM PKG')
    ->         and l_quantity >= 4 and l_quantity <= 4 + 10
    ->         and p_size between 1 and 5
    ->         and l_shipmode in ('AIR', 'AIR REG')
    ->         and l_shipinstruct = 'DELIVER IN PERSON'
    ->     )
    ->     or
    ->     (
    ->         p_partkey = l_partkey
    ->         and p_brand = 'Brand#11'
    ->         and p_container in ('MED BAG', 'MED BOX', 'MED PKG', 'MED PACK')
    ->         and l_quantity >= 18 and l_quantity <= 18 + 10
    ->         and p_size between 1 and 10
    ->         and l_shipmode in ('AIR', 'AIR REG')
    ->         and l_shipinstruct = 'DELIVER IN PERSON'
    ->     )
    ->     or
    ->     (
    ->         p_partkey = l_partkey
    ->         and p_brand = 'Brand#51'
    ->         and p_container in ('LG CASE', 'LG BOX', 'LG PACK', 'LG PKG')
    ->         and l_quantity >= 29 and l_quantity <= 29 + 10
    ->         and p_size between 1 and 15
    ->         and l_shipmode in ('AIR', 'AIR REG')
    ->         and l_shipinstruct = 'DELIVER IN PERSON'
    ->     );
+---------------+
| revenue       |
+---------------+
| 39074546.6022 |
+---------------+
1 row in set (20.04 sec)

hanfei1991 · 2018-02-12T04:41:50Z

expression/scalar_function.go

+	v := make([]types.Datum, 0, len(sf.GetArgs())+1)
+	var err error
+	sf.hashcode, err = codec.EncodeValue(sc, nil, types.NewStringDatum(sf.FuncName.L))
+	terror.Log(errors.Trace(err))


check if err is not nil ?

There's a check inside the terror.Log

hanfei1991 · 2018-02-12T04:41:59Z

expression/scalar_function.go

+		v = append(v, types.NewBytesDatum(arg.HashCode(sc)))
+	}
+	sf.hashcode = sf.hashcode[:0]
+	sf.hashcode, err = codec.EncodeValue(sc, sf.hashcode, v...)


hanfei1991 · 2018-02-12T05:05:02Z

expression/util.go

+		// We need this because there may be the case like `select * from t, t1 where (t.a=t1.a and t.a=t1.a) or (something).
+		// We should make sure that the two `t.a=t1.a` contributes only once.
+		innerMap := make(map[string]struct{})
+		if sf, ok := dnfItem.(*ScalarFunction); ok && sf.FuncName.L == ast.LogicAnd {


We don't need to check if it is a real cnf. It's all right to flatten conditions directly and loop it.

hanfei1991 · 2018-02-12T05:07:51Z

expression/util.go

+	}
+	newDNFItems := make([]Expression, 0, len(dnfItems))
+	for _, dnfItem := range dnfItems {
+		if sf, ok := dnfItem.(*ScalarFunction); ok && sf.FuncName.L == ast.LogicAnd {


hanfei1991 · 2018-02-12T05:08:52Z

expression/util.go

+			if len(newCNFItems) == 1 {
+				newDNFItems = append(newDNFItems, newCNFItems[0])
+			} else if len(newCNFItems) > 1 {
+				newDNFItems = append(newDNFItems, ComposeCNFCondition(ctx, newCNFItems...))


ComposeCNF can also handle single condition

hanfei1991 · 2018-02-12T05:10:19Z

expression/util.go

+	case 1:
+		return extractedExpr, newDNFItems[0]
+	default:
+		return extractedExpr, ComposeDNFCondition(ctx, newDNFItems...)


ComposeDNF also process the nil condition and single condition

hanfei1991 · 2018-02-12T05:10:50Z

expression/util.go

@@ -273,3 +274,104 @@ func Contains(exprs []Expression, e Expression) bool {
 	}
 	return false
 }
+
+// ExtractFiltersFromDNFs checks whether the cond is DNF and extract the same expression in its leaf items.


Explain the returning values.

hanfei1991 · 2018-02-12T07:30:25Z

expression/util.go

+	codeMap := make(map[string]int)
+	hashcode2Expr := make(map[string]Expression)
+	firstRun := true
+	for _, dnfItem := range dnfItems {


We use an index i and check if i == 0, so that we needn't firstRun.

hanfei1991 · 2018-02-12T07:31:33Z

expression/util.go

+	hashcode2Expr := make(map[string]Expression)
+	firstRun := true
+	for _, dnfItem := range dnfItems {
+		// We need this because there may be the case like `select * from t, t1 where (t.a=t1.a and t.a=t1.a) or (something).


move this comments to line 313

alivxxx · 2018-02-22T03:00:58Z

expression/integration_test.go

+		exprStr   string
+		result    string
+		remained  string
+		resultStr string


The above two fields are not used.

alivxxx · 2018-02-22T03:11:08Z

expression/integration_test.go

+		},
+		{
+			exprStr: "a = 1 or a = 1 or (a = 1 and b = 1)",
+			result:  "[eq(test.t.a, 1) eq(test.t.b, 1)]",


This is not correct? It should be a = 1, not a = 1 and b = 1.

No, the extracted part is a = 1, and the remained part is b = 1. So the result is a = 1 and b = 1

Consider the assignement that a = 1, b = 2, it is true for a = 1 or a = 1 or (a = 1 and b = 1), but false for a = 1 and b =1.

Now it's fixed.

alivxxx · 2018-02-22T03:12:58Z

expression/integration_test.go

+		},
+		{
+			exprStr: "(a = 1 and b = 1) or (a = 1 and b = 1 and c = 1) or (a = 1 and b = 1 and c > 2 and c < 3)",
+			result:  "[eq(test.t.a, 1) eq(test.t.b, 1) or(eq(test.t.c, 1), and(gt(test.t.c, 2), lt(test.t.c, 3)))]",


This is not correct too?

The extracted part is a = 1 and b = 1, the remained part is c = 1 or (c > 2 and c < 3).
So the result is a = 1 and b = 1 and (c = 1 or (c > 2 and c < 3)).

shenli · 2018-02-23T03:18:15Z

@winoros Please resolve the conflicts.

shenli · 2018-02-23T08:24:25Z

expression/expression.go

@@ -79,6 +80,9 @@ type Expression interface {

 	// ExplainInfo returns operator information to be explained.
 	ExplainInfo() string
+
+	// HashCode creates the hashcode for expression.


Please add more comments here. What is this function used for.

shenli · 2018-02-23T08:30:28Z

expression/util.go

+
+// extractFiltersFromDNF extracts the same condition that occurs in every DNF item and remove them from dnf leaves.
+func extractFiltersFromDNF(ctx context.Context, dnfFunc *ScalarFunction) ([]Expression, Expression) {
+	dnfItems := FlattenDNFConditions(dnfFunc)


If len(dnfItems) == 1, could we skip the following logic?

Or expression cannot have only one leaf.

shenli · 2018-02-23T08:31:35Z

/run-all-tests

iamxy · 2018-02-23T17:17:43Z

/run-all-tests

shenli · 2018-02-24T02:57:29Z

Please fix the CI.

alivxxx

LGTM

shenli · 2018-02-26T03:05:36Z

/run-all-tests

shenli · 2018-02-26T03:48:59Z

expression/util.go

@@ -273,3 +274,84 @@ func Contains(exprs []Expression, e Expression) bool {
 	}
 	return false
 }
+
+// ExtractFiltersFromDNFs checks whether the cond is DNF and extract the same expression in its leaf items and return them as a slice.


The comment is not correct. This function not only extract&return common conditions.

shenli · 2018-02-26T07:35:54Z

LGTM

winoros added 5 commits February 8, 2018 15:26

some pre-steps.

19a5974

extract the same filters from join's DNF conditions.

e953d1d

add buffer.

8cb119f

add more check.

b828648

undo some useless changes.

4745f0a

zz-jason reviewed Feb 8, 2018

View reviewed changes

hanfei1991 reviewed Feb 9, 2018

View reviewed changes

alivxxx reviewed Feb 9, 2018

View reviewed changes

address somment comments.

9993458

winoros added 2 commits February 9, 2018 13:55

move to expression package.

d9e0077

add test.

d338330

Merge branch 'master' into join-filters

f92c99c

winoros changed the title ~~expression, plan: extract the same filters from join's DNF conditions.~~ expression, plan: extract the same part from DNF's leaves. Feb 9, 2018

hanfei1991 reviewed Feb 12, 2018

View reviewed changes

winoros added 3 commits February 12, 2018 14:40

address comments.

1a2e692

map iterator is randomized, make the test more stable.

c1b0cdc

remove debug log.

b0ba886

hanfei1991 reviewed Feb 12, 2018

View reviewed changes

address comments.

bd2f3c7

alivxxx reviewed Feb 22, 2018

View reviewed changes

shenli reviewed Feb 23, 2018

View reviewed changes

winoros added 2 commits February 24, 2018 14:19

address comments.

1ee3abd

Merge branch 'master' into join-filters

569b34b

fix bug.

fd94051

alivxxx reviewed Feb 24, 2018

View reviewed changes

alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 24, 2018

Merge branch 'master' into join-filters

7028715

shenli reviewed Feb 26, 2018

View reviewed changes

shenli added the all-tests-passed label Feb 26, 2018

address comment.

7278d2c

Merge branch 'master' into join-filters

34e8eb6

shenli approved these changes Feb 26, 2018

View reviewed changes

winoros added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 26, 2018

winoros merged commit cd63332 into pingcap:master Feb 26, 2018

winoros deleted the join-filters branch February 26, 2018 08:03

expression, plan: extract the same part from DNF's leaves. #5831

expression, plan: extract the same part from DNF's leaves. #5831

Conversation

winoros commented Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

shenli commented Feb 8, 2018

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

winoros commented Feb 9, 2018

winoros commented Feb 9, 2018 • edited Loading

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

hanfei1991 Feb 12, 2018 • edited Loading

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

shenli commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Feb 23, 2018

iamxy commented Feb 23, 2018

shenli commented Feb 24, 2018

alivxxx left a comment

Choose a reason for hiding this comment

shenli commented Feb 26, 2018

Choose a reason for hiding this comment

shenli commented Feb 26, 2018

winoros commented Feb 8, 2018 •

edited

Loading

winoros Feb 9, 2018 •

edited

Loading

winoros Feb 9, 2018 •

edited

Loading

winoros commented Feb 9, 2018 •

edited

Loading

hanfei1991 Feb 12, 2018 •

edited

Loading