executor, planner: eliminate projection after aggregation pruning #7909

XuHuaiyu · 2018-10-16T03:05:41Z

What problem does this PR solve?

Eliminate redundant projection after aggregation pruning.

What is changed and how it works?

e.g.

create table t(a int primary key, b varchar(10));
insert into t value(1,11), (2, null);
select a, min(b), max(b) from t group by a;

Before this commit, the above query will panic.
Rationale:
explain result:

+-----------------------+----------+------+------------------------------------------------------------+
| id                    | count    | task | operator info                                              |
+-----------------------+----------+------+------------------------------------------------------------+
| Projection_5          | 10000.00 | root | test.t.a, 2_col_0, 2_col_1                                 |
| └─Projection_6        | 10000.00 | root | test.t.b, test.t.b, test.t.a                               |
|   └─TableReader_8     | 10000.00 | root | data:TableScan_7                                           |
|     └─TableScan_7     | 10000.00 | cop  | table:t, range:[-inf,+inf], keep order:false, stats:pseudo |
+-----------------------+----------+------+------------------------------------------------------------+

After aggregation pruning, all the expressions in Projection_6 are type Column.
And Projection5.2_col_0.Index is 0, Projection_5.2_col_1.Index is 1.

As the image in #7841 shows,
when evaluating 2_col_0, an empty chunk.column will be swapped to the first test.t.b of Prj_6, and the second test.t.b points to it.

When evaluating 2_col_1, the same empty chunk.column will be swapped to 2_col_1, which will raise panic later on.

Note:
I call projectionEliminator.optimize here because that I cannot find a proper way to reset the Index info of the Schema in Prj_5.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change

Side effects

N/A

Related changes

XuHuaiyu · 2018-10-16T03:06:01Z

PTAL @zz-jason @eurekaka @lamxTyler

zz-jason

LGTM

zz-jason · 2018-10-16T04:33:07Z

/run-all-tests

alivxxx · 2018-10-16T05:31:49Z

planner/core/rule_aggregation_elimination.go

 		newChildren = append(newChildren, newChild)
 	}
 	p.SetChildren(newChildren...)
+	if needProjEliminate {
+		return (&projectionEliminater{}).optimize(p)


In https://github.com/pingcap/tidb/pull/7531/files#diff-6a7fc6244769b48bc271de3d766c529eR37, we introduce the flagEliminateProjection2, could this be done by setting that flag? Or we may eliminate the projection multiply times.

XuHuaiyu · 2018-10-16T07:05:29Z

Refer https://github.com/pingcap/tidb/pull/7531/files#r224341848,
I changed the implementation to solve the problem.
PTAL @zz-jason @lamxTyler

XuHuaiyu · 2018-10-16T07:05:37Z

/run-all-tests

alivxxx

LGTM

zz-jason

LGTM

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

XuHuaiyu added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Oct 16, 2018

zz-jason reviewed Oct 16, 2018

View reviewed changes

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 16, 2018

alivxxx reviewed Oct 16, 2018

View reviewed changes

*: move projEliminate behind aggEliminate

b45dc36

XuHuaiyu force-pushed the agg_prune branch from 1ee27fb to b45dc36 Compare October 16, 2018 07:04

alivxxx reviewed Oct 16, 2018

View reviewed changes

alivxxx added the status/all tests passed label Oct 16, 2018

jackysp approved these changes Oct 16, 2018

View reviewed changes

Merge branch 'master' into agg_prune

4da2219

zz-jason approved these changes Oct 16, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 16, 2018

alivxxx and others added 3 commits October 16, 2018 16:44

Merge branch 'master' into agg_prune

b808474

Merge branch 'master' into agg_prune

ae1c383

Merge branch 'master' into agg_prune

7560697

eurekaka merged commit c520132 into pingcap:master Oct 16, 2018

XuHuaiyu mentioned this pull request Oct 16, 2018

plan: convert in subquery to agg and inner join #7531

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

executor, planner: eliminate projection after aggregation pruning #7909

executor, planner: eliminate projection after aggregation pruning #7909

XuHuaiyu commented Oct 16, 2018

XuHuaiyu commented Oct 16, 2018

zz-jason left a comment

zz-jason commented Oct 16, 2018

alivxxx Oct 16, 2018

XuHuaiyu commented Oct 16, 2018

XuHuaiyu commented Oct 16, 2018

alivxxx left a comment

zz-jason left a comment

executor, planner: eliminate projection after aggregation pruning #7909

executor, planner: eliminate projection after aggregation pruning #7909

Conversation

XuHuaiyu commented Oct 16, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

XuHuaiyu commented Oct 16, 2018

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Oct 16, 2018

alivxxx Oct 16, 2018

Choose a reason for hiding this comment

XuHuaiyu commented Oct 16, 2018

XuHuaiyu commented Oct 16, 2018

alivxxx left a comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment