Skip to content
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

plan: fix predicate push down for UnionScan #7695

Merged
merged 4 commits into from
Sep 17, 2018

Conversation

coocood
Copy link
Member

@coocood coocood commented Sep 14, 2018

What problem does this PR solve?

mysql> create table ta (a varchar(20));
Query OK, 0 rows affected (0.08 sec)

mysql> insert ta values ('1'), ('2');
Query OK, 2 rows affected (0.11 sec)

mysql> create table tb (a int);
Query OK, 0 rows affected (0.09 sec)

mysql> begin;
Query OK, 0 rows affected (0.00 sec)

mysql> insert tb values (0);
Query OK, 1 row affected (0.00 sec)

mysql> select * from ta where a = 1;
2 rows in set (0.00 sec)

Expected result is

1

Got

1
2

What is changed and how it works?

The predicate push down for UnionScan plan returns retained predicates instead of nil.

If we return nil predicates, the parent Selection will be removed, but conditions in UnionScan is only used to evaluate dirty table added rows, the conditions that cannot be pushed down to coprocessor will be lost.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

@coocood coocood added the type/bugfix This PR fixes a bug. label Sep 14, 2018
@coocood
Copy link
Member Author

coocood commented Sep 14, 2018

@jackysp @zz-jason @shenli PTAL

@coocood
Copy link
Member Author

coocood commented Sep 14, 2018

/run-all-tests

@zz-jason
Copy link
Member

@winoros @eurekaka @XuHuaiyu PTAL

@zz-jason zz-jason added the sig/planner SIG: Planner label Sep 14, 2018
@@ -78,3 +78,16 @@ func (s *testSuite) TestDirtyTransaction(c *C) {
tk.MustQuery("select * from t use index(idx) where c > 1 and d = 4").Check(testkit.Rows("1 2 3 4"))
tk.MustExec("commit")
}

func (s *testSuite) TestUnionScanWithCastCondition(c *C) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to add another explain test.

@winoros
Copy link
Member

winoros commented Sep 14, 2018

Eval it in getSnapshotRow?

@coocood
Copy link
Member Author

coocood commented Sep 14, 2018

@winoros
I think do filter in Selection is better.

@coocood
Copy link
Member Author

coocood commented Sep 14, 2018

@zz-jason PTAL

id count task operator info
Projection_5 8000.00 root test.ta.a
└─Selection_6 8000.00 root eq(cast(test.ta.a), 1)
└─UnionScan_7 10000.00 root eq(cast(test.ta.a), 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not Eval it in getSnapshotRow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's out of the scope of UnionScan's responsibility

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then why not removing all the filters inside the UnionScan?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be refactored later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filter a = 1, which is Selection_6, should be added as a parent of TableReader_9.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better, we can do it in the next PR.

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT

@shenli
Copy link
Member

shenli commented Sep 17, 2018

/run-all-tests

@coocood coocood merged commit 9f25032 into pingcap:master Sep 17, 2018
@coocood coocood deleted the fix-union-scan branch September 17, 2018 06:45
@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed labels Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/planner SIG: Planner status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants