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

planner: support subquery in SHOW statement #10942

Merged
merged 13 commits into from
Jul 11, 2019
Merged

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Jun 25, 2019

What problem does this PR solve?

Before this PR:

mysql> create table tbl(a int, b int);
Query OK, 0 rows affected (0.03 sec)

mysql> show columns from tbl;
+-------+---------+------+------+---------+-------+
| Field | Type    | Null | Key  | Default | Extra |
+-------+---------+------+------+---------+-------+
| a     | int(11) | YES  |      | NULL    |       |
| b     | int(11) | YES  |      | NULL    |       |
+-------+---------+------+------+---------+-------+
2 rows in set (0.01 sec)

mysql> show columns from tbl where field in (select 'a');
ERROR 2013 (HY000): Lost connection to MySQL server during query

What is changed and how it works?

The panic is caused by the fact that we didn't consider the possible subquery in the filter of show statement, if that happens, the rewritten expression would be nil, and hence panic would be raised in ResolveIndices.

This PR solves this panic by:

  • change Show from Plan implementation to PhysicalPlan implementation;
  • treat Show as a kind of data source, and build physical operators such as aggregation / join on top of it;
  • add an additional projection on top of the built physical plan, to keep the output fields consistent with normal show statement;

After this PR:

mysql> show columns from t where field in (select 'b');
+-------+---------+------+------+---------+-------+
| Field | Type    | Null | Key  | Default | Extra |
+-------+---------+------+------+---------+-------+
| b     | int(11) | YES  |      | NULL    |       |
+-------+---------+------+------+---------+-------+

Check List

Tests

  • Unit test

Code changes

N/A

Side effects

N/A

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@eurekaka eurekaka added type/enhancement The issue or PR belongs to an enhancement. type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Jun 25, 2019
@codecov
Copy link

codecov bot commented Jun 25, 2019

Codecov Report

Merging #10942 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #10942   +/-   ##
===========================================
  Coverage   81.6603%   81.6603%           
===========================================
  Files           423        423           
  Lines         91152      91152           
===========================================
  Hits          74435      74435           
  Misses        11486      11486           
  Partials       5231       5231

@eurekaka eurekaka marked this pull request as ready for review June 25, 2019 14:05
@eurekaka
Copy link
Contributor Author

/run-all-tests

@eurekaka
Copy link
Contributor Author

/run-unit-test

@eurekaka eurekaka requested a review from alivxxx July 2, 2019 10:07
@eurekaka
Copy link
Contributor Author

eurekaka commented Jul 3, 2019

/run-all-tests

@eurekaka
Copy link
Contributor Author

eurekaka commented Jul 3, 2019

/run-unit-test

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@eurekaka eurekaka requested a review from zz-jason July 8, 2019 06:52
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@eurekaka eurekaka requested a review from zz-jason July 10, 2019 10:10
@eurekaka eurekaka added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 10, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit 5c436bb into pingcap:master Jul 11, 2019
@eurekaka eurekaka deleted the panic branch July 25, 2019 08:16
eurekaka added a commit to eurekaka/tidb that referenced this pull request Jul 26, 2019
eurekaka added a commit to eurekaka/tidb that referenced this pull request Jul 26, 2019
Conflicts:
	planner/core/common_plans.go
	planner/core/find_best_task.go
	planner/core/initialize.go
	planner/core/planbuilder.go
	planner/core/resolve_indices.go
sre-bot pushed a commit that referenced this pull request Jul 26, 2019
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. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants