Repository: vitessio/vitess
Pull request #11671 (2022-11-08 21:06:09)

[release-14.0] Do not multiply `AggregateRandom` in JOINs
## Description

This is a backport of #11633.


vitess-bot[bot]: ### Review Checklist
            
Hello reviewers! :wave: Please follow this checklist when reviewing this Pull Request.

#### General
- [ ] Ensure that the Pull Request has a descriptive title.
- [ ] If this is a change that users need to know about, please apply the `release notes (needs details)` label so that merging is blocked unless the summary release notes document is included.

#### If a new flag is being introduced:
- [ ] Is it really necessary to add this flag?
- [ ] Flag names should be clear and intuitive (as far as possible)
- [ ] Help text should be descriptive.
- [ ] Flag names should use dashes (`-`) as word separators rather than underscores (`_`).

#### If a workflow is added or modified:
- [ ] Each item in `Jobs` should be named in order to mark it as `required`.
- [ ] If the workflow should be required, the maintainer team should be notified.

#### Bug fixes
- [ ] There should be at least one unit or end-to-end test.
- [ ] The Pull Request description should include a link to an issue that describes the bug.

#### Non-trivial changes
- [ ] There should be some code comments as to why things are implemented the way they are.

#### New/Existing features
- [ ] Should be documented, either by modifying the existing documentation or creating new documentation.
- [ ] New features should have a link to a feature request issue or an RFC that documents the use cases, corner cases and test cases.

#### Backward compatibility            
- [ ] Protobuf changes should be wire-compatible.
- [ ] Changes to `_vt` tables and RPCs need to be backward compatible.
- [ ] `vtctl` command output order should be stable and `awk`-able.
- [ ] RPC changes should be compatible with vitess-operator
- [ ] If a flag is removed, then it should also be removed from VTop, if used there.


Patch:
diff --git a/doc/releasenotes/14_0_0_release_notes.md b/doc/releasenotes/14_0_0_release_notes.md
index 2228991c67..a59e59d96f 100644
--- a/doc/releasenotes/14_0_0_release_notes.md
+++ b/doc/releasenotes/14_0_0_release_notes.md
@@ -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
 
diff --git a/doc/releasenotes/14_0_0_summary.md b/doc/releasenotes/14_0_0_summary.md
index 2b4a2a7fce..a98887505b 100644
--- a/doc/releasenotes/14_0_0_summary.md
+++ b/doc/releasenotes/14_0_0_summary.md
@@ -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
 
diff --git a/doc/releasenotes/14_0_1_release_notes.md b/doc/releasenotes/14_0_1_release_notes.md
index 293bffea8f..928bea5a43 100644
--- a/doc/releasenotes/14_0_1_release_notes.md
+++ b/doc/releasenotes/14_0_1_release_notes.md
@@ -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`
diff --git a/doc/releasenotes/14_0_1_summary.md b/doc/releasenotes/14_0_1_summary.md
index 1f2f985bab..5a176b6af1 100644
--- a/doc/releasenotes/14_0_1_summary.md
+++ b/doc/releasenotes/14_0_1_summary.md
@@ -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`
diff --git a/doc/releasenotes/14_0_2_release_notes.md b/doc/releasenotes/14_0_2_release_notes.md
index 61e526304a..d2a1ae1c93 100644
--- a/doc/releasenotes/14_0_2_release_notes.md
+++ b/doc/releasenotes/14_0_2_release_notes.md
@@ -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`
diff --git a/doc/releasenotes/14_0_2_summary.md b/doc/releasenotes/14_0_2_summary.md
index c399e313d7..a71683f89a 100644
--- a/doc/releasenotes/14_0_2_summary.md
+++ b/doc/releasenotes/14_0_2_summary.md
@@ -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`
diff --git a/doc/releasenotes/14_0_3_release_notes.md b/doc/releasenotes/14_0_3_release_notes.md
index d9cd4ac7a6..721c47640e 100644
--- a/doc/releasenotes/14_0_3_release_notes.md
+++ b/doc/releasenotes/14_0_3_release_notes.md
@@ -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
diff --git a/doc/releasenotes/14_0_3_summary.md b/doc/releasenotes/14_0_3_summary.md
index 9d9364f67b..121d9054a1 100644
--- a/doc/releasenotes/14_0_3_summary.md
+++ b/doc/releasenotes/14_0_3_summary.md
@@ -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
diff --git a/doc/releasenotes/14_0_4_summary.md b/doc/releasenotes/14_0_4_summary.md
new file mode 100644
index 0000000000..972a00c8bf
--- /dev/null
+++ b/doc/releasenotes/14_0_4_summary.md
@@ -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).
\ No newline at end of file
diff --git a/go/vt/vtgate/planbuilder/aggregation_pushing.go b/go/vt/vtgate/planbuilder/aggregation_pushing.go
index 88bcfbb309..7dd363b1ae 100644
--- a/go/vt/vtgate/planbuilder/aggregation_pushing.go
+++ b/go/vt/vtgate/planbuilder/aggregation_pushing.go
@@ -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,
@@ -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 {


Test patch:
diff --git a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go
index dd237cf0c8..8467cb30ab 100644
--- a/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go
+++ b/go/test/endtoend/vtgate/queries/aggregation/aggregation_test.go
@@ -368,3 +368,15 @@ 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)")
+
+	utils.Exec(t, mcmp.VtConn, "set sql_mode=''")
+	utils.Exec(t, mcmp.MySQLConn, "set sql_mode=''")
+	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)]]`)
+}
diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json
index d21e585919..2c5a3eb2d6 100644
--- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json
@@ -4719,7 +4719,7 @@
           {
             "OperatorType": "Projection",
             "Expressions": [
-              "[COLUMN 2] * [COLUMN 3] as col",
+              "[COLUMN 2] as col",
               "[COLUMN 1]",
               "[COLUMN 0] as id"
             ],
@@ -4727,7 +4727,7 @@
               {
                 "OperatorType": "Join",
                 "Variant": "Join",
-                "JoinColumnIndexes": "L:1,L:2,L:0,R:1",
+                "JoinColumnIndexes": "L:1,L:2,L:0",
                 "JoinVars": {
                   "user_col": 0
                 },
@@ -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"
                   }
                 ]
@@ -4858,7 +4858,7 @@
           {
             "OperatorType": "Projection",
             "Expressions": [
-              "[COLUMN 2] * [COLUMN 3] as id",
+              "[COLUMN 2] as id",
               "[COLUMN 1]",
               "[COLUMN 0] as id"
             ],
@@ -4866,7 +4866,7 @@
               {
                 "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": [
                   {
@@ -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"
                   }
                 ]
@@ -5144,5 +5144,78 @@
         ]
       }
     }
+  },
+  {
+    "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"
+                      }
+                    ]
+                  }
+                ]
+              }
+            ]
+          }
+        ]
+      }
+    }
   }
 ]