Skip to content

planner: refine PhysicalProjection.ResolveIndices during neighbouring Proj#8073

Merged
zz-jason merged 9 commits intopingcap:masterfrom
XuHuaiyu:union_proj
Nov 2, 2018
Merged

planner: refine PhysicalProjection.ResolveIndices during neighbouring Proj#8073
zz-jason merged 9 commits intopingcap:masterfrom
XuHuaiyu:union_proj

Conversation

@XuHuaiyu
Copy link
Contributor

@XuHuaiyu XuHuaiyu commented Oct 26, 2018

Description and test to be added.

What problem does this PR solve?

create table t1(a int, b int);
insert into t1 values(1,1),(2,2);

create table t2(a int, b int);
insert into t2 values(1,1),(2,2);

select sum(c) from (select t1.a as a, t1.a as c, length(t1.b) from t1  union select a, b, b from t2) t;

Before this PR, this query will cause a panic.

What is changed and how it works?

explain info:

+--------------------------------+----------+------+-------------------------------------------------------------+
| id                             | count    | task | operator info                                               |
+--------------------------------+----------+------+-------------------------------------------------------------+
| StreamAgg_16                   | 1.00     | root | funcs:sum(t.c)                                              |
| └─HashAgg_30                   | 16000.00 | root | group by:length(t1.b), t1.a, t1.c, funcs:firstrow(t1.c)     |
|   └─Union_20                   | 20000.00 | root |                                                             |
|     ├─Projection_21            | 10000.00 | root | test.t1.a, test.t1.c, cast(length(t1.b))                    |
|     │ └─Projection_22          | 10000.00 | root | test.t1.a, test.t1.a, length(cast(test.t1.b))               |
|     │   └─TableReader_24       | 10000.00 | root | data:TableScan_23                                           |
|     │     └─TableScan_23       | 10000.00 | cop  | table:t1, range:[-inf,+inf], keep order:false, stats:pseudo |
|     └─Projection_25            | 10000.00 | root | test.t2.a, test.t2.b, cast(test.t2.b)                       |
|       └─TableReader_27         | 10000.00 | root | data:TableScan_26                                           |
|         └─TableScan_26         | 10000.00 | cop  | table:t2, range:[-inf,+inf], keep order:false, stats:pseudo |
+--------------------------------+----------+------+-------------------------------------------------------------+

After buildProjection4Union, Projection_21:test.t1.a.Index and Projection_21:test.t1.c.Index will be 0 and 1 individually.
While both the two Projection_22.test.t1.a.Index are 0.
Thus during execution, when Projection_21:test.t1.c calling chunk.SwapColumn, it will get an empty chunk.column, which would cause the panic.

Check List

Tests

  • Integration test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the release branch

release-2.1 release-2.0

@XuHuaiyu XuHuaiyu added type/bug The issue is confirmed as a bug. sig/planner SIG: Planner labels Oct 26, 2018
@XuHuaiyu
Copy link
Contributor Author

PTAL @zz-jason

@shenli
Copy link
Member

shenli commented Oct 26, 2018

@XuHuaiyu Please fill the description and add a unit test.

@zimulala zimulala added type/bugfix This PR fixes a bug. and removed type/bug The issue is confirmed as a bug. labels Oct 29, 2018
@XuHuaiyu XuHuaiyu force-pushed the union_proj branch 2 times, most recently from 00ac877 to 2c9aa76 Compare October 29, 2018 07:11
@XuHuaiyu
Copy link
Contributor Author

PTAL @winoros @zz-jason

@XuHuaiyu
Copy link
Contributor Author

/run-all-tests

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Nov 1, 2018

PTAL @winoros @zz-jason

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Nov 1, 2018

/run-all-tests

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

@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Nov 1, 2018

To keep the unique of UniqueID, I revert the code, and add some comments for helping understand the logic easily.
@zz-jason @winoros

@XuHuaiyu XuHuaiyu changed the title planner: refine buildProjection4Union planner: refine PhysicalProjection.ResolveIndices during neighbouring Proj Nov 2, 2018
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

@winoros winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 2, 2018
@XuHuaiyu
Copy link
Contributor Author

XuHuaiyu commented Nov 2, 2018

/run-all-tests

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 added status/LGT2 Indicates that a PR has LGTM 2. status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 2, 2018
@zz-jason zz-jason merged commit 8ed4330 into pingcap:master Nov 2, 2018
@zz-jason
Copy link
Member

zz-jason commented Nov 2, 2018

@XuHuaiyu Please cherrypick this bugfix to the release-2.0 and release-2.1 branch.

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