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

[release-14.0] Do not multiply AggregateRandom in JOINs #11671

Merged
merged 4 commits into from
Nov 9, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions doc/releasenotes/14_0_0_release_notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
## Known Issues

- [VTOrc doesn't discover the tablets](https://github.com/vitessio/vitess/issues/10650) of a keyspace if the durability policy doesn't exist in the topo server when it comes up. This can be resolved by restarting VTOrc.
- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

Expand Down
1 change: 1 addition & 0 deletions doc/releasenotes/14_0_0_summary.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
## Known Issues

- [VTOrc doesn't discover the tablets](https://github.com/vitessio/vitess/issues/10650) of a keyspace if the durability policy doesn't exist in the topo server when it comes up. This can be resolved by restarting VTOrc.
- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

Expand Down
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_1_release_notes.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Release of Vitess v14.0.1
## Known Issues

- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

### Upgrade to `go1.18.4`
Expand Down
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_1_summary.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## Known Issues

- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

### Upgrade to `go1.18.4`
Expand Down
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_2_release_notes.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Release of Vitess v14.0.2
## Known Issues

- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

### Upgrade to `go1.18.5`
Expand Down
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_2_summary.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## Known Issues

- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

### Upgrade to `go1.18.5`
Expand Down
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_3_release_notes.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Release of Vitess v14.0.3
## Known Issues

- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

### Fix VTOrc Discovery
Expand Down
4 changes: 4 additions & 0 deletions doc/releasenotes/14_0_3_summary.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## Known Issues

- [Corrupted results for non-full-group-by queries with JOINs](https://github.com/vitessio/vitess/issues/11625). This can be resolved by using full-group-by queries.

## Major Changes

### Fix VTOrc Discovery
Expand Down
6 changes: 6 additions & 0 deletions doc/releasenotes/14_0_4_summary.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Major Changes

### Corrupted results for non-full-group-by queries with JOINs

An issue in versions `<= v14.0.3` and `<= v15.0.0` that generated corrupted results for non-full-group-by queries with a JOIN
is now fixed. The full issue can be found [here](https://github.com/vitessio/vitess/issues/11625), and its fix [here](https://github.com/vitessio/vitess/pull/11633).
10 changes: 10 additions & 0 deletions go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,3 +368,13 @@ func TestEmptyTableAggr(t *testing.T) {
}

}

func TestAggregateRandom(t *testing.T) {
mcmp, closer := start(t)
defer closer()

mcmp.Exec("insert into t1(t1_id, name, value, shardKey) values (1, 'name 1', 'value 1', 1), (2, 'name 2', 'value 2', 2)")
mcmp.Exec("insert into t2(id, shardKey) values (1, 10), (2, 20)")

mcmp.AssertMatches("SELECT /*vt+ PLANNER=gen4 */ t1.shardKey, t1.name, count(t2.id) FROM t1 JOIN t2 ON t1.value != t2.shardKey GROUP BY t1.t1_id", `[[INT64(1) VARCHAR("name 1") INT64(2)] [INT64(2) VARCHAR("name 2") INT64(2)]]`)
}
8 changes: 6 additions & 2 deletions go/vt/vtgate/planbuilder/aggregation_pushing.go
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,10 @@ func isMinOrMax(in engine.AggregateOpcode) bool {
}
}

func isRandom(in engine.AggregateOpcode) bool {
return in == engine.AggregateRandom
}

func splitAggregationsToLeftAndRight(
ctx *plancontext.PlanningContext,
aggregations []abstract.Aggr,
Expand All @@ -445,8 +449,8 @@ func splitAggregationsToLeftAndRight(
} else {
deps := ctx.SemTable.RecursiveDeps(aggr.Original.Expr)
var other *abstract.Aggr
// if we are sending down min/max, we don't have to multiply the results with anything
if !isMinOrMax(aggr.OpCode) {
// if we are sending down min/max/random, we don't have to multiply the results with anything
if !isMinOrMax(aggr.OpCode) && !isRandom(aggr.OpCode) {
other = countStarAggr()
}
switch {
Expand Down
93 changes: 85 additions & 8 deletions go/vt/vtgate/planbuilder/testdata/aggr_cases.json
Original file line number Diff line number Diff line change
Expand Up @@ -4719,15 +4719,15 @@
{
"OperatorType": "Projection",
"Expressions": [
"[COLUMN 2] * [COLUMN 3] as col",
"[COLUMN 2] as col",
"[COLUMN 1]",
"[COLUMN 0] as id"
],
"Inputs": [
{
"OperatorType": "Join",
"Variant": "Join",
"JoinColumnIndexes": "L:1,L:2,L:0,R:1",
"JoinColumnIndexes": "L:1,L:2,L:0",
"JoinVars": {
"user_col": 0
},
Expand All @@ -4752,8 +4752,8 @@
"Name": "user",
"Sharded": true
},
"FieldQuery": "select 1, count(*) from user_extra where 1 != 1 group by 1",
"Query": "select 1, count(*) from user_extra where user_extra.col = :user_col group by 1",
"FieldQuery": "select 1 from user_extra where 1 != 1 group by 1",
"Query": "select 1 from user_extra where user_extra.col = :user_col group by 1",
"Table": "user_extra"
}
]
Expand Down Expand Up @@ -4858,15 +4858,15 @@
{
"OperatorType": "Projection",
"Expressions": [
"[COLUMN 2] * [COLUMN 3] as id",
"[COLUMN 2] as id",
"[COLUMN 1]",
"[COLUMN 0] as id"
],
"Inputs": [
{
"OperatorType": "Join",
"Variant": "Join",
"JoinColumnIndexes": "L:0,L:1,L:0,R:1",
"JoinColumnIndexes": "L:0,L:1,L:0",
"TableName": "`user`_user_extra",
"Inputs": [
{
Expand All @@ -4888,8 +4888,8 @@
"Name": "user",
"Sharded": true
},
"FieldQuery": "select 1, count(*) from user_extra where 1 != 1 group by 1",
"Query": "select 1, count(*) from user_extra group by 1",
"FieldQuery": "select 1 from user_extra where 1 != 1 group by 1",
"Query": "select 1 from user_extra group by 1",
"Table": "user_extra"
}
]
Expand Down Expand Up @@ -5144,5 +5144,82 @@
]
}
}
},
{
"comment": "AggregateRandom in non full group by query",
"query": "select u.id, u.name, count(m.predef1) from user.user as u join user.user_extra as m on u.id = m.order group by u.id",
"v3-plan": "unsupported: cross-shard query with aggregates",
"gen4-plan": {
"QueryType": "SELECT",
"Original": "select u.id, u.name, count(m.predef1) from user.user as u join user.user_extra as m on u.id = m.order group by u.id",
"Instructions": {
"OperatorType": "Aggregate",
"Variant": "Ordered",
"Aggregates": "random(1) AS name, sum_count(2) AS count(m.predef1)",
"GroupBy": "(0|3)",
"ResultColumns": 3,
"Inputs": [
{
"OperatorType": "Projection",
"Expressions": [
"[COLUMN 0] as id",
"[COLUMN 2] as name",
"[COLUMN 3] * [COLUMN 4] as count(m.predef1)",
"[COLUMN 1]"
],
"Inputs": [
{
"OperatorType": "Sort",
"Variant": "Memory",
"OrderBy": "(0|1) ASC",
"Inputs": [
{
"OperatorType": "Join",
"Variant": "Join",
"JoinColumnIndexes": "R:2,R:3,R:0,L:1,R:1",
"JoinVars": {
"m_order": 0
},
"TableName": "user_extra_`user`",
"Inputs": [
{
"OperatorType": "Route",
"Variant": "Scatter",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select m.`order`, count(m.predef1), weight_string(m.`order`) from user_extra as m where 1 != 1 group by m.`order`, weight_string(m.`order`)",
"Query": "select m.`order`, count(m.predef1), weight_string(m.`order`) from user_extra as m group by m.`order`, weight_string(m.`order`)",
"Table": "user_extra"
},
{
"OperatorType": "Route",
"Variant": "EqualUnique",
"Keyspace": {
"Name": "user",
"Sharded": true
},
"FieldQuery": "select u.`name`, count(*), u.id, weight_string(u.id) from `user` as u where 1 != 1 group by u.id, weight_string(u.id)",
"Query": "select u.`name`, count(*), u.id, weight_string(u.id) from `user` as u where u.id = :m_order group by u.id, weight_string(u.id)",
"Table": "`user`",
"Values": [
":m_order"
],
"Vindex": "user_index"
}
]
}
]
}
]
}
]
},
"TablesUsed": [
"user.user",
"user.user_extra"
]
}
}
]