Skip to content

Commit

Permalink
planner, executor: eliminate extra columns introduced by OrderBy upon…
Browse files Browse the repository at this point in the history
… Union (#8290)
  • Loading branch information
AndrewDi committed Nov 14, 2018
1 parent 162fa84 commit 72ed3e6
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 2 deletions.
9 changes: 9 additions & 0 deletions executor/executor_test.go
Expand Up @@ -1053,6 +1053,15 @@ func (s *testSuite) TestUnion(c *C) {
tk.MustExec("insert into t1 value(1,2),(1,1),(2,2),(2,2),(3,2),(3,2)")
tk.MustExec("set @@tidb_max_chunk_size=2;")
tk.MustQuery("select count(*) from (select a as c, a as d from t1 union all select a, b from t1) t;").Check(testkit.Rows("12"))

// #issue 8189 and #issue 8199
tk.MustExec("drop table if exists t1")
tk.MustExec("drop table if exists t2")
tk.MustExec("CREATE TABLE t1 (a int not null, b char (10) not null)")
tk.MustExec("insert into t1 values(1,'a'),(2,'b'),(3,'c'),(3,'c')")
tk.MustExec("CREATE TABLE t2 (a int not null, b char (10) not null)")
tk.MustExec("insert into t2 values(1,'a'),(2,'b'),(3,'c'),(3,'c')")
tk.MustQuery("select a from t1 union select a from t1 order by (select a+1);").Check(testkit.Rows("1", "2", "3"))
}

func (s *testSuite) TestNeighbouringProj(c *C) {
Expand Down
20 changes: 18 additions & 2 deletions plan/logical_plan_builder.go
Expand Up @@ -713,6 +713,8 @@ func (b *planBuilder) buildUnion(union *ast.UnionStmt) LogicalPlan {
unionPlan = unionAllPlan
}

oldLen := unionPlan.Schema().Len()

if union.OrderBy != nil {
unionPlan = b.buildSort(unionPlan, union.OrderBy.Items, nil)
if b.err != nil {
Expand All @@ -727,6 +729,20 @@ func (b *planBuilder) buildUnion(union *ast.UnionStmt) LogicalPlan {
return nil
}
}

// Fix issue #8189 (https://github.com/pingcap/tidb/issues/8189).
// If there are extra expressions generated from `ORDER BY` clause, generate a `Projection` to remove them.
if oldLen != unionPlan.Schema().Len() {
proj := LogicalProjection{Exprs: expression.Column2Exprs(unionPlan.Schema().Columns[:oldLen])}.init(b.ctx)
proj.SetChildren(unionPlan)
schema := expression.NewSchema(unionPlan.Schema().Clone().Columns[:oldLen]...)
for _, col := range schema.Columns {
col.FromID = proj.ID()
}
proj.SetSchema(schema)
return proj
}

return unionPlan
}

Expand Down Expand Up @@ -862,7 +878,7 @@ func (b *planBuilder) buildLimit(src LogicalPlan, limit *ast.Limit) LogicalPlan
return li
}

// colMatch(a,b) means that if a match b, e.g. t.a can match test.t.a but test.t.a can't match t.a.
// colMatch means that if a match b, e.g. t.a can match test.t.a but test.t.a can't match t.a.
// Because column a want column from database test exactly.
func colMatch(a *ast.ColumnName, b *ast.ColumnName) bool {
if a.Schema.L == "" || a.Schema.L == b.Schema.L {
Expand Down Expand Up @@ -912,7 +928,7 @@ func resolveFromSelectFields(v *ast.ColumnNameExpr, fields []*ast.SelectField, i
return
}

// AggregateFuncExtractor visits Expr tree.
// havingAndOrderbyExprResolver visits Expr tree.
// It converts ColunmNameExpr to AggregateFuncExpr and collects AggregateFuncExpr.
type havingAndOrderbyExprResolver struct {
inAggFunc bool
Expand Down
10 changes: 10 additions & 0 deletions plan/logical_plan_test.go
Expand Up @@ -1459,6 +1459,16 @@ func (s *testPlanSuite) TestUnion(c *C) {
best: "",
err: true,
},
{
sql: "select * from (select 1 as a union select 1 union all select 2) t order by a",
best: "UnionAll{UnionAll{Dual->Projection->Projection->Dual->Projection->Projection}->Aggr(firstrow(a))->Projection->Dual->Projection->Projection}->Projection->Sort",
err: false,
},
{
sql: "select * from (select 1 as a union select 1 union all select 2) t order by (select a)",
best: "Apply{UnionAll{UnionAll{Dual->Projection->Projection->Dual->Projection->Projection}->Aggr(firstrow(a))->Projection->Dual->Projection->Projection}->Dual->Projection->MaxOneRow}->Sort->Projection",
err: false,
},
}
for i, tt := range tests {
comment := Commentf("case:%v sql:%s", i, tt.sql)
Expand Down

0 comments on commit 72ed3e6

Please sign in to comment.