Repository: vitessio/vitess
Pull request #9701 (2022-02-16 07:06:25)

Fixed missing order/group by, limit, having in derived
## Description

This pull request fixes an issue encountered when we plan a query that can be merged in a single route and contains a derived table with one of the following: having, group by, order by, limit.

The planner was not adding those clauses to the merged query leading to invalid results being returned.

A query like this:
```sql
select 1 
from (
  select col, count(*) as a 
  from main.unsharded
  group by col
  having a > 0 
  limit 12 offset 0) as f
left join unsharded as u on f.col = u.id
```

Would produce a plan that contains a query like this:
```sql
select 1
from (
  select col, count(*) as a
  from unsharded) as f
left join unsharded as u on f.col = u.id
```

## Checklist
- [x] Should this PR be backported?
- [x] Tests were added or are not required
- [x] Documentation was added or is not required


Patch:
diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go
index 4385e8701a..0584e21afd 100644
--- a/go/vt/sqlparser/ast_funcs.go
+++ b/go/vt/sqlparser/ast_funcs.go
@@ -1621,7 +1621,12 @@ func defaultRequiresParens(ct *ColumnType) bool {
 
 // RemoveKeyspaceFromColName removes the Qualifier.Qualifier on all ColNames in the expression tree
 func RemoveKeyspaceFromColName(expr Expr) Expr {
-	Rewrite(expr, nil, func(cursor *Cursor) bool {
+	return RemoveKeyspace(expr).(Expr) // This hard cast is safe because we do not change the type the input
+}
+
+// RemoveKeyspace removes the Qualifier.Qualifier on all ColNames in the AST
+func RemoveKeyspace(in SQLNode) SQLNode {
+	return Rewrite(in, nil, func(cursor *Cursor) bool {
 		switch col := cursor.Node().(type) {
 		case *ColName:
 			if !col.Qualifier.Qualifier.IsEmpty() {
@@ -1629,7 +1634,5 @@ func RemoveKeyspaceFromColName(expr Expr) Expr {
 			}
 		}
 		return true
-	}) // This hard cast is safe because we do not change the type the input
-
-	return expr
+	})
 }
diff --git a/go/vt/vtgate/planbuilder/operator_to_query.go b/go/vt/vtgate/planbuilder/operator_to_query.go
index 0f519f2ef6..ed6935731b 100644
--- a/go/vt/vtgate/planbuilder/operator_to_query.go
+++ b/go/vt/vtgate/planbuilder/operator_to_query.go
@@ -77,7 +77,12 @@ func buildQuery(op abstract.PhysicalOperator, qb *queryBuilder) {
 		buildQuery(op.Source, qb)
 		sel := qb.sel.(*sqlparser.Select) // we can only handle SELECT in derived tables at the moment
 		qb.sel = nil
-		sel.SelectExprs = sqlparser.GetFirstSelect(op.Query).SelectExprs
+		opQuery := sqlparser.RemoveKeyspace(op.Query).(*sqlparser.Select)
+		sel.Limit = opQuery.Limit
+		sel.OrderBy = opQuery.OrderBy
+		sel.GroupBy = opQuery.GroupBy
+		sel.Having = opQuery.Having
+		sel.SelectExprs = opQuery.SelectExprs
 		qb.addTableExpr(op.Alias, op.Alias, op.TableID(), &sqlparser.DerivedTable{
 			Select: sel,
 		}, nil)


Test patch:
diff --git a/go/vt/vtgate/planbuilder/testdata/select_cases.txt b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
index 7ee7d2fe0b..9a07140afe 100644
--- a/go/vt/vtgate/planbuilder/testdata/select_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/select_cases.txt
@@ -2902,3 +2902,41 @@ Gen4 plan same as above
     ]
   }
 }
+
+# mergeable derived table with order by and limit
+"select 1 from (select col from main.unsharded order by main.unsharded.col1 desc limit 12 offset 0) as f left join unsharded as u on f.col = u.id"
+{
+  "QueryType": "SELECT",
+  "Original": "select 1 from (select col from main.unsharded order by main.unsharded.col1 desc limit 12 offset 0) as f left join unsharded as u on f.col = u.id",
+  "Instructions": {
+    "OperatorType": "Route",
+    "Variant": "Unsharded",
+    "Keyspace": {
+      "Name": "main",
+      "Sharded": false
+    },
+    "FieldQuery": "select 1 from (select col from unsharded where 1 != 1) as f left join unsharded as u on f.col = u.id where 1 != 1",
+    "Query": "select 1 from (select col from unsharded order by unsharded.col1 desc limit 0, 12) as f left join unsharded as u on f.col = u.id",
+    "Table": "unsharded"
+  }
+}
+Gen4 plan same as above
+
+# mergeable derived table with group by and limit
+"select 1 from (select col, count(*) as a from main.unsharded group by col having a > 0 limit 12 offset 0) as f left join unsharded as u on f.col = u.id"
+{
+  "QueryType": "SELECT",
+  "Original": "select 1 from (select col, count(*) as a from main.unsharded group by col having a \u003e 0 limit 12 offset 0) as f left join unsharded as u on f.col = u.id",
+  "Instructions": {
+    "OperatorType": "Route",
+    "Variant": "Unsharded",
+    "Keyspace": {
+      "Name": "main",
+      "Sharded": false
+    },
+    "FieldQuery": "select 1 from (select col, count(*) as a from unsharded where 1 != 1 group by col) as f left join unsharded as u on f.col = u.id where 1 != 1",
+    "Query": "select 1 from (select col, count(*) as a from unsharded group by col having a \u003e 0 limit 0, 12) as f left join unsharded as u on f.col = u.id",
+    "Table": "unsharded"
+  }
+}
+Gen4 plan same as above
diff --git a/go/vt/vtgate/planbuilder/testdata/tpcc_cases.txt b/go/vt/vtgate/planbuilder/testdata/tpcc_cases.txt
index 63ecdc0a9e..3c55ecacea 100644
--- a/go/vt/vtgate/planbuilder/testdata/tpcc_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/tpcc_cases.txt
@@ -921,8 +921,8 @@ Gen4 plan same as above
       "Name": "main",
       "Sharded": true
     },
-    "FieldQuery": "select o.o_id, o.o_d_id from orders1 as o, (select o_c_id, o_w_id, o_d_id, count(distinct o_w_id), o_id from orders1 where 1 != 1) as t where 1 != 1",
-    "Query": "select o.o_id, o.o_d_id from orders1 as o, (select o_c_id, o_w_id, o_d_id, count(distinct o_w_id), o_id from orders1 where o_w_id = 1 and o_id \u003e 2100 and o_id \u003c 11153) as t where t.o_w_id = o.o_w_id and t.o_d_id = o.o_d_id and t.o_c_id = o.o_c_id limit 1",
+    "FieldQuery": "select o.o_id, o.o_d_id from orders1 as o, (select o_c_id, o_w_id, o_d_id, count(distinct o_w_id), o_id from orders1 where 1 != 1 group by o_c_id, o_d_id, o_w_id) as t where 1 != 1",
+    "Query": "select o.o_id, o.o_d_id from orders1 as o, (select o_c_id, o_w_id, o_d_id, count(distinct o_w_id), o_id from orders1 where o_w_id = 1 and o_id \u003e 2100 and o_id \u003c 11153 group by o_c_id, o_d_id, o_w_id having count(distinct o_id) \u003e 1 limit 1) as t where t.o_w_id = o.o_w_id and t.o_d_id = o.o_d_id and t.o_c_id = o.o_c_id limit 1",
     "Table": "orders1",
     "Values": [
       "INT64(1)"