Repository: vitessio/vitess
Pull request #12825 (2023-04-04 10:10:33)

[release-14.0] planbuilder bugfix - do not push aggregations into derived tables
## Description

This is a backport of #12810.



Patch:
diff --git a/go/vt/vtgate/planbuilder/abstract/derived.go b/go/vt/vtgate/planbuilder/abstract/derived.go
index 13a7d8a62d..cf8ca75e02 100644
--- a/go/vt/vtgate/planbuilder/abstract/derived.go
+++ b/go/vt/vtgate/planbuilder/abstract/derived.go
@@ -17,7 +17,10 @@ limitations under the License.
 package abstract
 
 import (
+	"io"
+
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
+
 	"vitess.io/vitess/go/vt/sqlparser"
 	"vitess.io/vitess/go/vt/vterrors"
 	"vitess.io/vitess/go/vt/vtgate/semantics"
@@ -54,11 +57,31 @@ func (d *Derived) PushPredicate(expr sqlparser.Expr, semTable *semantics.SemTabl
 	if err != nil {
 		return nil, err
 	}
+	if !canBePushedDownIntoDerived(newExpr) {
+		// if we have an aggregation, we don't want to push it inside
+		return &Filter{Source: d, Predicates: []sqlparser.Expr{expr}}, nil
+	}
 	newSrc, err := d.Inner.PushPredicate(newExpr, semTable)
 	d.Inner = newSrc
 	return d, err
 }
 
+func canBePushedDownIntoDerived(expr sqlparser.Expr) (canBePushed bool) {
+	canBePushed = true
+	_ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) {
+		switch n := node.(type) {
+		case *sqlparser.FuncExpr:
+			if n.Name.EqualString("min") || n.Name.EqualString("max") {
+				break
+			}
+			canBePushed = false
+			return false, io.EOF
+		}
+		return true, nil
+	}, expr)
+	return
+}
+
 // UnsolvedPredicates implements the Operator interface
 func (d *Derived) UnsolvedPredicates(semTable *semantics.SemTable) []sqlparser.Expr {
 	return d.Inner.UnsolvedPredicates(semTable)


Test patch:
diff --git a/go/vt/vtgate/planbuilder/abstract/operator_test.go b/go/vt/vtgate/planbuilder/abstract/operator_test.go
index f518f3ab55..e1197e8d65 100644
--- a/go/vt/vtgate/planbuilder/abstract/operator_test.go
+++ b/go/vt/vtgate/planbuilder/abstract/operator_test.go
@@ -98,10 +98,9 @@ func TestOperator(t *testing.T) {
 			require.NoError(t, err)
 			tree, err := sqlparser.Parse(tc.query)
 			require.NoError(t, err)
-			stmt := tree.(sqlparser.Statement)
-			semTable, err := semantics.Analyze(stmt, "", si)
+			semTable, err := semantics.Analyze(tree, "", si)
 			require.NoError(t, err)
-			optree, err := CreateLogicalOperatorFromAST(stmt, semTable)
+			optree, err := CreateLogicalOperatorFromAST(tree, semTable)
 			require.NoError(t, err)
 			output := testString(optree)
 			if tc.expected != output {
diff --git a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json
index 6016622d2e..0235bef0e7 100644
--- a/go/vt/vtgate/planbuilder/testdata/aggr_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/aggr_cases.json
@@ -5340,9 +5340,46 @@
           "Sharded": true
         },
         "FieldQuery": "select t1.portalId, t1.flowId from (select portalId, flowId, count(*) as count from user_extra where 1 != 1 group by user_id, flowId) as t1 where 1 != 1",
-        "Query": "select t1.portalId, t1.flowId from (select portalId, flowId, count(*) as count from user_extra where localDate > :v1 group by user_id, flowId having count(*) >= :v2 order by null) as t1",
+        "Query": "select t1.portalId, t1.flowId from (select portalId, flowId, count(*) as count from user_extra where localDate > :v1 group by user_id, flowId order by null) as t1 where count >= :v2",
         "Table": "user_extra"
       }
     }
+  },
+  {
+    "comment": "aggregation, where and derived tables - we can push extremums",
+    "query": "SELECT foo FROM (SELECT foo, max(baz) as bazo FROM (SELECT foo, baz FROM user) f GROUP BY foo) tt WHERE bazo BETWEEN 100 AND 200",
+    "v3-plan": "unsupported: filtering on results of cross-shard subquery",
+    "gen4-plan": {
+      "QueryType": "SELECT",
+      "Original": "SELECT foo FROM (SELECT foo, max(baz) as bazo FROM (SELECT foo, baz FROM user) f GROUP BY foo) tt WHERE bazo BETWEEN 100 AND 200",
+      "Instructions": {
+        "OperatorType": "SimpleProjection",
+        "Columns": [
+          0
+        ],
+        "Inputs": [
+          {
+            "OperatorType": "Aggregate",
+            "Variant": "Ordered",
+            "Aggregates": "max(1) AS bazo",
+            "GroupBy": "(0|2)",
+            "Inputs": [
+              {
+                "OperatorType": "Route",
+                "Variant": "Scatter",
+                "Keyspace": {
+                  "Name": "user",
+                  "Sharded": true
+                },
+                "FieldQuery": "select foo, max(baz) as bazo, weight_string(foo) from (select foo, baz from `user` where 1 != 1) as f where 1 != 1 group by foo, weight_string(foo)",
+                "OrderBy": "(0|2) ASC",
+                "Query": "select foo, max(baz) as bazo, weight_string(foo) from (select foo, baz from `user` having max(baz) between 100 and 200) as f group by foo, weight_string(foo) order by foo asc",
+                "Table": "`user`"
+              }
+            ]
+          }
+        ]
+      }
+    }
   }
 ]
diff --git a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json
index 39e43ff854..909b4a7d92 100644
--- a/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json
+++ b/go/vt/vtgate/planbuilder/testdata/unsupported_cases.json
@@ -471,5 +471,11 @@
     "comment": "mix lock with other expr",
     "query": "select get_lock('xyz', 10), 1 from dual",
     "plan": "unsupported: lock function and other expression in same select query"
+  },
+  {
+    "comment": "aggregation, where and derived tables - we can't push aggregations that might need a second layer of aggregation",
+    "query": "SELECT foo FROM (SELECT foo, count(baz) as bazo FROM (SELECT foo, baz FROM user) f GROUP BY foo) tt WHERE bazo BETWEEN 100 AND 200",
+    "v3-plan": "unsupported: filtering on results of cross-shard subquery",
+    "gen4-plan": "expr cannot be translated, not supported: bazo between 100 and 200"
   }
 ]