expression: support disable expression pushdown based on store type #16389

windtalker · 2020-04-15T05:13:34Z

What problem does this PR solve?

Issue Number: close #16353, close #16504

Problem Summary:
TiDB support expr pushdown blacklist, if the expr is added to mysql.expr_pushdown_blacklist, then it will not be pushed to cop layer, however, this blacklist does not take store type into acount, so it is impossible to disable an expr pushdown to a specified store(like tikv or tiflash)

What is changed and how it works?

What's Changed:
Add two columns store_type and reason to mysql.expr_pushdown_blacklist
How it Works:
store_type in mysql.expr_pushdown_blacklist indicates which store should the expr pushdown be disabled

Check List

Tests

Integration test
Manual test (add detailed scripts or steps below)

Release note

…expr_blacklist

codecov · 2020-04-15T05:35:44Z

Codecov Report

Merging #16389 into master will decrease coverage by 0.2605%.
The diff coverage is 88.3116%.

@@               Coverage Diff                @@
##             master     #16389        +/-   ##
================================================
- Coverage   80.6470%   80.3864%   -0.2606%     
================================================
  Files           506        506                
  Lines        137643     136666       -977     
================================================
- Hits         111005     109861      -1144     
- Misses        18096      18228       +132     
- Partials       8542       8577        +35

lzmhhh123 · 2020-04-15T07:47:18Z

executor/reload_expr_pushdown_blacklist.go

+		}
+		storeTypes := strings.Split(storeTypeString, ",")
+		for _, typeString := range storeTypes {
+			if typeString == "tidb" {


We can use the kv.TiDB.Name() instead of "tidb".

lzmhhh123 · 2020-04-15T07:56:06Z

session/bootstrap.go

+
+func upgradeToVer42(s Session) {
+	doReentrantDDL(s, "ALTER TABLE mysql.expr_pushdown_blacklist ADD COLUMN `store_type` char(100) NOT NULL DEFAULT 'tikv,tiflash,tidb'", infoschema.ErrColumnExists)
+	doReentrantDDL(s, "ALTER TABLE mysql.expr_pushdown_blacklist ADD COLUMN `reason` varchar(200)", infoschema.ErrColumnExists)


Do we really need this column?

I prefer to keep this column, since we will add some default rows in expr_pushdown_blacklist, this column can make it self-explained, otherwise, the users may keep asking why those exprs are disabled by default.

I prefer to keep this column, since we will add some default rows in expr_pushdown_blacklist, this column can make it self-explained, otherwise, the users may keep asking why those exprs are disabled by default.

Yeah, this explanation is also OK for me.

…/tidb into refine_expr_blacklist

…expr_blacklist

lzmhhh123 · 2020-04-15T09:07:14Z

expression/integration_test.go

+		"Selection_5 2133.33 root  lt(test.t.b, 1994-01-01)",
+		"└─Selection_9 2666.67 root  gt(cast(test.t.a), 10.10)",
+		"  └─TableReader_8 2666.67 root  data:Selection_7",
+		"    └─Selection_7 2666.67 cop[tiflash]  eq(date_format(test.t.b, \"%m\"), \"11\"), gt(test.t.b, 1988-01-01)",
+		"      └─TableFullScan_6 10000.00 cop[tiflash] table:t keep order:false, stats:pseudo"))


We'd better not to compare the explain result directly. That is not friendly enough for other developers. You can get the testkit.rows and compare the expression info in the second-to-last row.

lzmhhh123

LGTM for other changes.

…expr_blacklist

wshwsh12

LGTM

lzmhhh123

LGTM

windtalker · 2020-04-16T11:16:24Z

/run-integration-copr-test

zz-jason · 2020-04-16T14:15:04Z

planner/core/testdata/integration_suite_out.json

+          "HashJoin_7 9990.00 root  inner join, equal:[eq(test.t.a, test.t.a)]",
+          "├─Selection_15(Build) 7992.00 root  from_unixtime(cast(test.t.b))",
+          "│ └─TableReader_14 7992.00 root  data:Selection_13",
+          "│   └─Selection_13 7992.00 cop[tikv]  not(isnull(test.t.a))",


why the estimated output row count is changed from 9990.00 to 7992.00?

I think this is because from_unixtime is now pushed down by predicate pushdown rule, so when estimate output rowcount, the cop task thought it has two filter conditions(is not null and from_unixtime)

This reverts commit 1128fce.

…expr_blacklist

windtalker · 2020-04-17T05:11:34Z

/run-integration-copr-test

windtalker · 2020-04-17T05:17:02Z

/run-all-tests

lzmhhh123

LGTM

lzmhhh123 · 2020-04-17T06:33:25Z

/merge

sre-bot · 2020-04-17T06:33:45Z

/run-all-tests

sre-bot · 2020-04-17T06:36:46Z

@windtalker merge failed.

Signed-off-by: sre-bot <[email protected]>

sre-bot · 2020-04-17T06:54:29Z

cherry pick to release-3.1 in PR #16513

…cap#16389)

sre-bot · 2020-04-17T06:55:37Z

cherry pick to release-4.0 in PR #16514

This reverts commit 20f30db.

…16389) (#16513)

windtalker added 3 commits April 15, 2020 11:59

support disable expr pushdown based on store type

3b1cb7d

Merge branch 'master' of https://github.com/pingcap/tidb into refine_…

5765393

…expr_blacklist

refine

fccbd19

windtalker requested a review from a team as a code owner April 15, 2020 05:13

ghost requested review from wshwsh12 and removed request for a team April 15, 2020 05:13

github-actions bot added sig/execution SIG execution component/expression labels Apr 15, 2020

windtalker added 2 commits April 15, 2020 13:27

fix ci failure

38f2e96

Merge branch 'master' of https://github.com/pingcap/tidb into refine_…

a98056d

…expr_blacklist

windtalker requested a review from a team as a code owner April 15, 2020 05:28

ghost requested review from francis0407 and removed request for a team April 15, 2020 05:28

windtalker added 3 commits April 15, 2020 13:45

Merge branch 'master' into refine_expr_blacklist

a4bc1b3

Merge branch 'master' into refine_expr_blacklist

0c26f8d

Merge branch 'master' into refine_expr_blacklist

8ada0f1

lzmhhh123 reviewed Apr 15, 2020

View reviewed changes

windtalker added 3 commits April 15, 2020 16:24

address comments

15dc34a

Merge branch 'refine_expr_blacklist' of https://github.com/windtalker…

61319a1

…/tidb into refine_expr_blacklist

Merge branch 'master' of https://github.com/pingcap/tidb into refine_…

40f5eb5

…expr_blacklist

lzmhhh123 reviewed Apr 15, 2020

View reviewed changes

lzmhhh123 added the type/enhancement The issue or PR belongs to an enhancement. label Apr 15, 2020

windtalker added 2 commits April 15, 2020 18:22

address comments

293f755

Merge branch 'master' of https://github.com/pingcap/tidb into refine_…

3d80a5e

…expr_blacklist

wshwsh12 reviewed Apr 15, 2020

View reviewed changes

wshwsh12 added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 15, 2020

Merge branch 'master' into refine_expr_blacklist

bc10e01

lzmhhh123 previously approved these changes Apr 16, 2020

View reviewed changes

windtalker dismissed lzmhhh123’s stale review via 1128fce April 16, 2020 11:11

Merge branch 'master' into refine_expr_blacklist

9e6faf2

zz-jason reviewed Apr 16, 2020

View reviewed changes

windtalker added 3 commits April 17, 2020 13:01

Revert "fix ci error"

6d9d201

This reverts commit 1128fce.

Merge branch 'master' of https://github.com/pingcap/tidb into refine_…

c8eae0b

…expr_blacklist

fix ci and add tests

a12c30c

windtalker added 2 commits April 17, 2020 13:40

refine tests

7bb7774

remove useless code

edd2488

lzmhhh123 approved these changes Apr 17, 2020

View reviewed changes

lzmhhh123 merged commit b8494e7 into pingcap:master Apr 17, 2020

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 17, 2020

cherry pick pingcap#16389 to release-3.1

20f30db

Signed-off-by: sre-bot <[email protected]>

sre-bot mentioned this pull request Apr 17, 2020

expression: support disable expression pushdown based on store type (#16389) #16513

Merged

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 17, 2020

expression: support disable expression pushdown based on store… (ping…

716f8ed

…cap#16389)

sre-bot mentioned this pull request Apr 17, 2020

expression: support disable expression pushdown based on store type (#16389) #16514

Merged

windtalker added a commit to sre-bot/tidb that referenced this pull request Apr 17, 2020

Revert "cherry pick pingcap#16389 to release-3.1"

53a7293

This reverts commit 20f30db.

windtalker mentioned this pull request Apr 20, 2020

reference: update doc about expr_pushdown_blacklist pingcap/docs-cn#2817

Merged

5 tasks

sre-bot mentioned this pull request Apr 22, 2020

reference: update doc about expr_pushdown_blacklist (#2817) pingcap/docs-cn#2860

Merged

5 tasks

sre-bot added a commit that referenced this pull request Apr 25, 2020

expression: support disable expression pushdown based on store type (#…

f60b2a8

…16389) (#16513)

windtalker mentioned this pull request May 8, 2020

executor: fix wrong plan type for dataReaderBuilder error #17028

Merged

This was referenced May 8, 2020

executor: fix wrong plan type for dataReaderBuilder error (#17028) #17034

Merged

executor: fix wrong plan type for dataReaderBuilder... (#17028) #17036

Merged

windtalker deleted the refine_expr_blacklist branch December 28, 2020 04:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: support disable expression pushdown based on store type #16389

expression: support disable expression pushdown based on store type #16389

windtalker commented Apr 15, 2020 •

edited by SunRunAway

Loading

codecov bot commented Apr 15, 2020 •

edited

Loading

lzmhhh123 Apr 15, 2020

lzmhhh123 Apr 15, 2020

windtalker Apr 15, 2020

lzmhhh123 Apr 15, 2020

lzmhhh123 Apr 15, 2020

lzmhhh123 left a comment

wshwsh12 left a comment

lzmhhh123 left a comment

windtalker commented Apr 16, 2020

zz-jason Apr 16, 2020

windtalker Apr 16, 2020

windtalker commented Apr 17, 2020

windtalker commented Apr 17, 2020

lzmhhh123 left a comment

lzmhhh123 commented Apr 17, 2020

sre-bot commented Apr 17, 2020

sre-bot commented Apr 17, 2020

sre-bot commented Apr 17, 2020

sre-bot commented Apr 17, 2020

expression: support disable expression pushdown based on store type #16389

expression: support disable expression pushdown based on store type #16389

Conversation

windtalker commented Apr 15, 2020 • edited by SunRunAway Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

codecov bot commented Apr 15, 2020 • edited Loading

Codecov Report

lzmhhh123 Apr 15, 2020

Choose a reason for hiding this comment

lzmhhh123 Apr 15, 2020

Choose a reason for hiding this comment

windtalker Apr 15, 2020

Choose a reason for hiding this comment

lzmhhh123 Apr 15, 2020

Choose a reason for hiding this comment

lzmhhh123 Apr 15, 2020

Choose a reason for hiding this comment

lzmhhh123 left a comment

Choose a reason for hiding this comment

wshwsh12 left a comment

Choose a reason for hiding this comment

lzmhhh123 left a comment

Choose a reason for hiding this comment

windtalker commented Apr 16, 2020

zz-jason Apr 16, 2020

Choose a reason for hiding this comment

windtalker Apr 16, 2020

Choose a reason for hiding this comment

windtalker commented Apr 17, 2020

windtalker commented Apr 17, 2020

lzmhhh123 left a comment

Choose a reason for hiding this comment

lzmhhh123 commented Apr 17, 2020

sre-bot commented Apr 17, 2020

sre-bot commented Apr 17, 2020

sre-bot commented Apr 17, 2020

sre-bot commented Apr 17, 2020

windtalker commented Apr 15, 2020 •

edited by SunRunAway

Loading

codecov bot commented Apr 15, 2020 •

edited

Loading