Skip to content

fix group by over empty result sets #1704

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 10, 2023
Merged

fix group by over empty result sets #1704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
39da0d8
fix and tests
Apr 6, 2023
a453344
refactor
Apr 7, 2023
7c4f5d5
adding more tests
Apr 7, 2023
9690b41
remove block
Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions enginetest/queries/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,72 @@ var QueryTests = []QueryTest{
{int64(3), "third row"},
},
},
{
Query: "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM emptytable;",
Expected: []sql.Row{
{0, nil, nil, 123, "abc", "abcdef"},
},
},
{
Query: "SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM mytable where false;",
Expected: []sql.Row{
{0, nil, nil, 123, "abc", "abcdef"},
},
},
{
Query: "SELECT pk, u, v FROM one_pk JOIN (SELECT count(*) AS u, 123 AS v FROM emptytable) uv WHERE pk = u;",
Expected: []sql.Row{
{0, 0, 123},
},
},
{
Query: "SELECT pk, u, v FROM one_pk JOIN (SELECT count(*) AS u, 123 AS v FROM mytable WHERE false) uv WHERE pk = u;",
Expected: []sql.Row{
{0, 0, 123},
},
},
{
Query: "SELECT pk FROM one_pk WHERE (pk, 123) IN (SELECT count(*) AS u, 123 AS v FROM emptytable);",
Expected: []sql.Row{
{0},
},
},
{
Query: "SELECT pk FROM one_pk WHERE (pk, 123) IN (SELECT count(*) AS u, 123 AS v FROM mytable WHERE false);",
Expected: []sql.Row{
{0},
},
},
{
Query: "SELECT pk FROM one_pk WHERE (pk, 123) NOT IN (SELECT count(*) AS u, 123 AS v FROM emptytable);",
Expected: []sql.Row{
{1},
{2},
{3},
},
},
{
Query: "SELECT pk FROM one_pk WHERE (pk, 123) NOT IN (SELECT count(*) AS u, 123 AS v FROM mytable WHERE false);",
Expected: []sql.Row{
{1},
{2},
{3},
},
},
{
Query: "SELECT i FROM mytable WHERE EXISTS (SELECT * FROM (SELECT count(*) as u, 123 as v FROM emptytable) uv);",
Expected: []sql.Row{
{1},
{2},
{3},
},
},
{
Query: "SELECT count(*), (SELECT i FROM mytable WHERE i = 1 group by i);",
Expected: []sql.Row{
{1, 1},
},
},
{
Query: "SELECT pk DIV 2, SUM(c3) FROM one_pk GROUP BY 1 ORDER BY 1",
Expected: []sql.Row{
Expand Down
171 changes: 171 additions & 0 deletions enginetest/queries/query_plans.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,28 @@ var PlanTests = []QueryPlanTest{
" └─ columns: [a]\n" +
"",
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add some tests 1) with this transform preformed inside subqueries, making sure they stay cached afterwards (select * from xy join (select count(*) as u, 1 as v from ...) uv on x = u, and 2)) this transform performed inside subquery expressions, with semi/anti joins still being generated afterwards ( select ... where x in/not in (select y from (select count(*) as u, 1 as v from ...) uv), select ... where x =/!= (...), select * from xy where exists (select y from (select count(*) as u, 1 as v from ...) uv) where x = y))

Copy link
Contributor

Choose a reason for hiding this comment

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

and idk if this is valid, but can the projection be a subquery expression? select count(*), (select * from uv where u = x) from xy group by x

Query: `SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM emptytable;`,
ExpectedPlan: "Project\n" +
" ├─ columns: [COUNT(1):0!null as count(*), emptytable.i:1!null, concat(i, i):2!null, 123 (tinyint), abc (longtext) as abc, concat(abc (longtext),def (longtext)) as concat('abc', 'def')]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint)), emptytable.i:0!null, concat(emptytable.i:0!null,emptytable.i:0!null) as concat(i, i)\n" +
" ├─ group: \n" +
" └─ Table\n" +
" ├─ name: emptytable\n" +
" └─ columns: [i]\n" +
"",
},
{
Query: `SELECT count(*), i, concat(i, i), 123, 'abc', concat('abc', 'def') FROM mytable where false;`,
ExpectedPlan: "Project\n" +
" ├─ columns: [COUNT(1):0!null as count(*), mytable.i:1!null, concat(i, i):2!null, 123 (tinyint), abc (longtext) as abc, concat(abc (longtext),def (longtext)) as concat('abc', 'def')]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint)), mytable.i:0!null, concat(mytable.i:0!null,mytable.i:0!null) as concat(i, i)\n" +
" ├─ group: \n" +
" └─ EmptyTable\n" +
"",
},
{
Query: `select count(*) cnt from ab where exists (select * from xy where x = a) group by a`,
ExpectedPlan: "Project\n" +
Expand All @@ -929,6 +951,155 @@ var PlanTests = []QueryPlanTest{
" └─ columns: [x y]\n" +
"",
},
{
Query: `SELECT pk, u, v FROM one_pk JOIN (SELECT count(*) AS u, 123 AS v FROM emptytable) uv WHERE pk = u;`,
ExpectedPlan: "Project\n" +
" ├─ columns: [one_pk.pk:2!null, uv.u:0!null, uv.v:1!null]\n" +
" └─ HashJoin\n" +
" ├─ Eq\n" +
" │ ├─ one_pk.pk:2!null\n" +
" │ └─ uv.u:0!null\n" +
" ├─ SubqueryAlias\n" +
" │ ├─ name: uv\n" +
" │ ├─ outerVisibility: false\n" +
" │ ├─ cacheable: true\n" +
" │ └─ Project\n" +
" │ ├─ columns: [COUNT(1):0!null as u, 123 (tinyint) as v]\n" +
" │ └─ GroupBy\n" +
" │ ├─ select: COUNT(1 (bigint))\n" +
" │ ├─ group: \n" +
" │ └─ Table\n" +
" │ ├─ name: emptytable\n" +
" │ └─ columns: []\n" +
" └─ HashLookup\n" +
" ├─ source: TUPLE(uv.u:0!null)\n" +
" ├─ target: TUPLE(one_pk.pk:0!null)\n" +
" └─ CachedResults\n" +
" └─ Table\n" +
" ├─ name: one_pk\n" +
" └─ columns: [pk]\n" +
"",
},
{
Query: `SELECT pk, u, v FROM one_pk JOIN (SELECT count(*) AS u, 123 AS v FROM mytable WHERE false) uv WHERE pk = u;`,
ExpectedPlan: "Project\n" +
" ├─ columns: [one_pk.pk:2!null, uv.u:0!null, uv.v:1!null]\n" +
" └─ HashJoin\n" +
" ├─ Eq\n" +
" │ ├─ one_pk.pk:2!null\n" +
" │ └─ uv.u:0!null\n" +
" ├─ SubqueryAlias\n" +
" │ ├─ name: uv\n" +
" │ ├─ outerVisibility: false\n" +
" │ ├─ cacheable: true\n" +
" │ └─ Project\n" +
" │ ├─ columns: [COUNT(1):0!null as u, 123 (tinyint) as v]\n" +
" │ └─ GroupBy\n" +
" │ ├─ select: COUNT(1 (bigint))\n" +
" │ ├─ group: \n" +
" │ └─ EmptyTable\n" +
" └─ HashLookup\n" +
" ├─ source: TUPLE(uv.u:0!null)\n" +
" ├─ target: TUPLE(one_pk.pk:0!null)\n" +
" └─ CachedResults\n" +
" └─ Table\n" +
" ├─ name: one_pk\n" +
" └─ columns: [pk]\n" +
"",
},
{
Query: `SELECT pk FROM one_pk WHERE (pk, 123) IN (SELECT count(*) AS u, 123 AS v FROM emptytable);`,
ExpectedPlan: "Project\n" +
" ├─ columns: [one_pk.pk:0!null]\n" +
" └─ SemiJoin\n" +
" ├─ Eq\n" +
" │ ├─ TUPLE(one_pk.pk:0!null, 123 (tinyint))\n" +
" │ └─ TUPLE(scalarSubq0.u:6!null, scalarSubq0.v:7!null)\n" +
" ├─ Table\n" +
" │ ├─ name: one_pk\n" +
" │ └─ columns: [pk c1 c2 c3 c4 c5]\n" +
" └─ SubqueryAlias\n" +
" ├─ name: scalarSubq0\n" +
" ├─ outerVisibility: false\n" +
" ├─ cacheable: true\n" +
" └─ Project\n" +
" ├─ columns: [COUNT(1):0!null as u, 123 (tinyint) as v]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint))\n" +
" ├─ group: \n" +
" └─ Table\n" +
" ├─ name: emptytable\n" +
" └─ columns: []\n" +
"",
},
{
Query: `SELECT pk FROM one_pk WHERE (pk, 123) IN (SELECT count(*) AS u, 123 AS v FROM mytable WHERE false);`,
ExpectedPlan: "Project\n" +
" ├─ columns: [one_pk.pk:0!null]\n" +
" └─ SemiJoin\n" +
" ├─ Eq\n" +
" │ ├─ TUPLE(one_pk.pk:0!null, 123 (tinyint))\n" +
" │ └─ TUPLE(scalarSubq0.u:6!null, scalarSubq0.v:7!null)\n" +
" ├─ Table\n" +
" │ ├─ name: one_pk\n" +
" │ └─ columns: [pk c1 c2 c3 c4 c5]\n" +
" └─ SubqueryAlias\n" +
" ├─ name: scalarSubq0\n" +
" ├─ outerVisibility: false\n" +
" ├─ cacheable: true\n" +
" └─ Project\n" +
" ├─ columns: [COUNT(1):0!null as u, 123 (tinyint) as v]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint))\n" +
" ├─ group: \n" +
" └─ EmptyTable\n" +
"",
},
{
Query: `SELECT i FROM mytable WHERE EXISTS (SELECT * FROM (SELECT count(*) as u, 123 as v FROM emptytable) uv);`,
ExpectedPlan: "Project\n" +
" ├─ columns: [mytable.i:0!null]\n" +
" └─ SemiJoin\n" +
" ├─ true (tinyint)\n" +
" ├─ Table\n" +
" │ ├─ name: mytable\n" +
" │ └─ columns: [i s]\n" +
" └─ Limit(1)\n" +
" └─ SubqueryAlias\n" +
" ├─ name: uv\n" +
" ├─ outerVisibility: true\n" +
" ├─ cacheable: true\n" +
" └─ Project\n" +
" ├─ columns: [COUNT(1):0!null as u, 123 (tinyint) as v]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint))\n" +
" ├─ group: \n" +
" └─ Table\n" +
" ├─ name: emptytable\n" +
" └─ columns: []\n" +
"",
},
{
Query: `SELECT count(*), (SELECT i FROM mytable WHERE i = 1 group by i);`,
ExpectedPlan: "Project\n" +
" ├─ columns: [COUNT(1):0!null as count(*), Subquery\n" +
" │ ├─ cacheable: true\n" +
" │ └─ GroupBy\n" +
" │ ├─ select: mytable.i:1!null\n" +
" │ ├─ group: mytable.i:1!null\n" +
" │ └─ IndexedTableAccess(mytable)\n" +
" │ ├─ index: [mytable.i]\n" +
" │ ├─ static: [{[1, 1]}]\n" +
" │ └─ columns: [i]\n" +
" │ as (SELECT i FROM mytable WHERE i = 1 group by i)]\n" +
" └─ GroupBy\n" +
" ├─ select: COUNT(1 (bigint))\n" +
" ├─ group: \n" +
" └─ Table\n" +
" ├─ name: \n" +
" └─ columns: []\n" +
"",
},
{
Query: `with cte(a,b) as (select * from ab) select * from xy where exists (select * from cte where a = x)`,
ExpectedPlan: "RightSemiLookupJoin\n" +
Expand Down
19 changes: 14 additions & 5 deletions sql/analyzer/aggregations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,20 @@ func replaceAggregatesWithGetFieldProjections(_ *sql.Context, scope *Scope, proj
}

if same {
newAggregates = append(newAggregates, e)
name, source := getNameAndSource(e)
newProjection[i] = expression.NewGetFieldWithTable(
scopeLen+len(newAggregates)-1, e.Type(), source, name, e.IsNullable(),
)
// add to plan.GroupBy.SelectedExprs iff expression has an expression.GetField
hasGetField := transform.InspectExpr(e, func(expr sql.Expression) bool {
_, ok := expr.(*expression.GetField)
return ok
})
if hasGetField {
newAggregates = append(newAggregates, e)
name, source := getNameAndSource(e)
newProjection[i] = expression.NewGetFieldWithTable(
scopeLen+len(newAggregates)-1, e.Type(), source, name, e.IsNullable(),
)
} else {
newProjection[i] = e
}
} else {
newProjection[i] = e
transform.InspectExpr(e, func(e sql.Expression) bool {
Expand Down
3 changes: 0 additions & 3 deletions sql/analyzer/resolve_columns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,9 +1021,6 @@ func pushdownGroupByAliases(ctx *sql.Context, a *Analyzer, n sql.Node, scope *Sc
if n.Resolved() || !ok || len(g.GroupByExprs) == 0 {
return n, transform.SameTree, nil
}
if !ok || len(g.GroupByExprs) == 0 {
return n, transform.SameTree, nil
}

// The reason we have two sets of columns, one for grouping and
// one for aggregate is because an alias can redefine a column name
Expand Down