From 83543b8068b291122be6e4957d697035e05e4f01 Mon Sep 17 00:00:00 2001 From: FlorentP <35779988+frouioui@users.noreply.github.com> Date: Tue, 18 Oct 2022 08:55:50 +0200 Subject: [PATCH] Fix `HAVING` rewriting made in #11306 (#11515) * don't rewrite HAVING predicates that use table columns Signed-off-by: Andres Taylor * Revert the changes made in #11306 Signed-off-by: Florent Poinsard Signed-off-by: Andres Taylor * Fix early rewriter test Signed-off-by: Florent Poinsard Signed-off-by: Andres Taylor Signed-off-by: Florent Poinsard Co-authored-by: Andres Taylor --- go/vt/vtgate/planbuilder/rewrite.go | 49 +++++++----- .../planbuilder/testdata/filter_cases.json | 74 +++++++++++++++++++ go/vt/vtgate/semantics/early_rewriter.go | 14 +++- go/vt/vtgate/semantics/early_rewriter_test.go | 2 +- 4 files changed, 118 insertions(+), 21 deletions(-) diff --git a/go/vt/vtgate/planbuilder/rewrite.go b/go/vt/vtgate/planbuilder/rewrite.go index 284ffa9c079..4ff3edd9fd3 100644 --- a/go/vt/vtgate/planbuilder/rewrite.go +++ b/go/vt/vtgate/planbuilder/rewrite.go @@ -174,29 +174,40 @@ func rewriteHavingClause(node *sqlparser.Select) { exprs := sqlparser.SplitAndExpression(nil, node.Having.Expr) node.Having = nil for _, expr := range exprs { - var hasAggr bool - sqlparser.Rewrite(expr, func(cursor *sqlparser.Cursor) bool { - switch x := cursor.Node().(type) { - case *sqlparser.ColName: - if !x.Qualifier.IsEmpty() { - return false - } - originalExpr, isInMap := selectExprMap[x.Name.Lowered()] - if isInMap && sqlparser.ContainsAggregation(originalExpr) { - hasAggr = true - } - return false - default: - _, isAggregate := x.(sqlparser.AggrFunc) - hasAggr = hasAggr || isAggregate - } - return true - }, nil) - + hasAggr := sqlparser.ContainsAggregation(expr) + if !hasAggr { + sqlparser.Rewrite(expr, func(cursor *sqlparser.Cursor) bool { + visitColName(cursor.Node(), selectExprMap, func(original sqlparser.Expr) { + if sqlparser.ContainsAggregation(original) { + hasAggr = true + } + }) + return true + }, nil) + } if hasAggr { node.AddHaving(expr) } else { + sqlparser.Rewrite(expr, func(cursor *sqlparser.Cursor) bool { + visitColName(cursor.Node(), selectExprMap, func(original sqlparser.Expr) { + cursor.Replace(original) + }) + return true + }, nil) node.AddWhere(expr) } } } +func visitColName(cursor sqlparser.SQLNode, selectExprMap map[string]sqlparser.Expr, f func(original sqlparser.Expr)) { + switch x := cursor.(type) { + case *sqlparser.ColName: + if !x.Qualifier.IsEmpty() { + return + } + originalExpr, isInMap := selectExprMap[x.Name.Lowered()] + if isInMap { + f(originalExpr) + } + return + } +} diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.json b/go/vt/vtgate/planbuilder/testdata/filter_cases.json index 3f7d0ea26c1..c4d1c605aa5 100644 --- a/go/vt/vtgate/planbuilder/testdata/filter_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.json @@ -6421,5 +6421,79 @@ "user.user" ] } + }, + { + "comment": "HAVING predicates that use table columns are safe to rewrite if we can move them to the WHERE clause", + "query": "select user.col + 2 as a from user having a = 42", + "v3-plan": { + "QueryType": "SELECT", + "Original": "select user.col + 2 as a from user having a = 42", + "Instructions": { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.col + 2 as a from `user` where 1 != 1", + "Query": "select `user`.col + 2 as a from `user` having a = 42", + "Table": "`user`" + } + }, + "gen4-plan": { + "QueryType": "SELECT", + "Original": "select user.col + 2 as a from user having a = 42", + "Instructions": { + "OperatorType": "Route", + "Variant": "Scatter", + "Keyspace": { + "Name": "user", + "Sharded": true + }, + "FieldQuery": "select `user`.col + 2 as a from `user` where 1 != 1", + "Query": "select `user`.col + 2 as a from `user` where `user`.col + 2 = 42", + "Table": "`user`" + }, + "TablesUsed": [ + "user.user" + ] + } + }, + { + "comment": "HAVING predicates that use table columns should not get rewritten on unsharded keyspaces", + "query": "select col + 2 as a from unsharded having a = 42", + "v3-plan": { + "QueryType": "SELECT", + "Original": "select col + 2 as a from unsharded having a = 42", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select col + 2 as a from unsharded where 1 != 1", + "Query": "select col + 2 as a from unsharded having a = 42", + "Table": "unsharded" + } + }, + "gen4-plan": { + "QueryType": "SELECT", + "Original": "select col + 2 as a from unsharded having a = 42", + "Instructions": { + "OperatorType": "Route", + "Variant": "Unsharded", + "Keyspace": { + "Name": "main", + "Sharded": false + }, + "FieldQuery": "select col + 2 as a from unsharded where 1 != 1", + "Query": "select col + 2 as a from unsharded having a = 42", + "Table": "unsharded" + }, + "TablesUsed": [ + "main.unsharded" + ] + } } ] diff --git a/go/vt/vtgate/semantics/early_rewriter.go b/go/vt/vtgate/semantics/early_rewriter.go index d4ca8dc5816..85394a382d9 100644 --- a/go/vt/vtgate/semantics/early_rewriter.go +++ b/go/vt/vtgate/semantics/early_rewriter.go @@ -146,7 +146,19 @@ func rewriteHavingAndOrderBy(cursor *sqlparser.Cursor, node sqlparser.SQLNode) { continue } if ae.As.Equal(col.Name) { - inner.Replace(ae.Expr) + safeToRewrite := true + sqlparser.Rewrite(ae.Expr, func(cursor *sqlparser.Cursor) bool { + switch cursor.Node().(type) { + case *sqlparser.ColName: + safeToRewrite = false + case sqlparser.AggrFunc: + return false + } + return true + }, nil) + if safeToRewrite { + inner.Replace(ae.Expr) + } } } } diff --git a/go/vt/vtgate/semantics/early_rewriter_test.go b/go/vt/vtgate/semantics/early_rewriter_test.go index 79e64e1b293..cd0c91614da 100644 --- a/go/vt/vtgate/semantics/early_rewriter_test.go +++ b/go/vt/vtgate/semantics/early_rewriter_test.go @@ -157,7 +157,7 @@ func TestExpandStar(t *testing.T) { expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b", }, { sql: "select * from t1 join t5 using (b) having b = 12", - expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b having t1.b = 12", + expSQL: "select t1.b as b, t1.a as a, t1.c as c, t5.a as a from t1 join t5 where t1.b = t5.b having b = 12", }, { sql: "select 1 from t1 join t5 using (b) having b = 12", expSQL: "select 1 from t1 join t5 where t1.b = t5.b having t1.b = 12",