From e818ca560f4562951cd4343413281be1914c9790 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9s=20Taylor?= <andres@planetscale.com>
Date: Thu, 30 Nov 2023 13:13:30 +0100
Subject: [PATCH] refactor: minor cleanups in planner code (#14642)

---
 go/vt/vtgate/planbuilder/operators/aggregator.go |  2 +-
 go/vt/vtgate/planbuilder/operators/phases.go     | 15 ++++++++++-----
 go/vt/vtgate/planbuilder/simplifier_test.go      | 11 +++--------
 go/vt/vtgate/simplifier/expression_simplifier.go |  5 +++--
 go/vt/vtgate/simplifier/simplifier.go            |  2 +-
 go/vt/vtgate/simplifier/simplifier_test.go       |  8 +++-----
 6 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/go/vt/vtgate/planbuilder/operators/aggregator.go b/go/vt/vtgate/planbuilder/operators/aggregator.go
index 269c4616274..e1848752e75 100644
--- a/go/vt/vtgate/planbuilder/operators/aggregator.go
+++ b/go/vt/vtgate/planbuilder/operators/aggregator.go
@@ -201,7 +201,7 @@ func (a *Aggregator) findColInternal(ctx *plancontext.PlanningContext, ae *sqlpa
 	}
 
 	if addToGroupBy {
-		panic(vterrors.VT13001("did not expect to add group by here"))
+		panic(vterrors.VT13001(fmt.Sprintf("did not expect to add group by here: %s", sqlparser.String(expr))))
 	}
 
 	return -1
diff --git a/go/vt/vtgate/planbuilder/operators/phases.go b/go/vt/vtgate/planbuilder/operators/phases.go
index f180cfc2ce0..557124e9320 100644
--- a/go/vt/vtgate/planbuilder/operators/phases.go
+++ b/go/vt/vtgate/planbuilder/operators/phases.go
@@ -101,13 +101,18 @@ type phaser struct {
 }
 
 func (p *phaser) next(ctx *plancontext.PlanningContext) Phase {
-	for phas := p.current; phas < DONE; phas++ {
-		if phas.shouldRun(ctx.SemTable.QuerySignature) {
-			p.current = p.current + 1
-			return phas
+	for {
+		curr := p.current
+		if curr == DONE {
+			return DONE
+		}
+
+		p.current++
+
+		if curr.shouldRun(ctx.SemTable.QuerySignature) {
+			return curr
 		}
 	}
-	return DONE
 }
 
 func removePerformanceDistinctAboveRoute(_ *plancontext.PlanningContext, op ops.Operator) (ops.Operator, error) {
diff --git a/go/vt/vtgate/planbuilder/simplifier_test.go b/go/vt/vtgate/planbuilder/simplifier_test.go
index 4c83af77933..56d310d2949 100644
--- a/go/vt/vtgate/planbuilder/simplifier_test.go
+++ b/go/vt/vtgate/planbuilder/simplifier_test.go
@@ -21,19 +21,14 @@ import (
 	"fmt"
 	"testing"
 
-	"vitess.io/vitess/go/test/vschemawrapper"
-
-	"vitess.io/vitess/go/vt/vterrors"
-
 	"github.com/stretchr/testify/assert"
-
-	"vitess.io/vitess/go/vt/vtgate/simplifier"
-
 	"github.com/stretchr/testify/require"
 
+	"vitess.io/vitess/go/test/vschemawrapper"
 	"vitess.io/vitess/go/vt/log"
-
 	"vitess.io/vitess/go/vt/sqlparser"
+	"vitess.io/vitess/go/vt/vterrors"
+	"vitess.io/vitess/go/vt/vtgate/simplifier"
 )
 
 // TestSimplifyBuggyQuery should be used to whenever we get a planner bug reported
diff --git a/go/vt/vtgate/simplifier/expression_simplifier.go b/go/vt/vtgate/simplifier/expression_simplifier.go
index 4537a137e76..5582ac3993d 100644
--- a/go/vt/vtgate/simplifier/expression_simplifier.go
+++ b/go/vt/vtgate/simplifier/expression_simplifier.go
@@ -21,7 +21,6 @@ import (
 	"strconv"
 
 	"vitess.io/vitess/go/vt/log"
-
 	"vitess.io/vitess/go/vt/sqlparser"
 )
 
@@ -44,10 +43,10 @@ func SimplifyExpr(in sqlparser.Expr, test CheckF) sqlparser.Expr {
 			cursor.Replace(expr)
 
 			valid := test(smallestKnown[0])
-			log.Errorf("test: %t: simplified %s to %s, full expr: %s", valid, sqlparser.String(node), sqlparser.String(expr), sqlparser.String(smallestKnown))
 			if valid {
 				break // we will still continue trying to simplify other expressions at this level
 			} else {
+				log.Errorf("failed attempt: tried changing {%s} to {%s} in {%s}", sqlparser.String(node), sqlparser.String(expr), sqlparser.String(in))
 				// undo the change
 				cursor.Replace(node)
 			}
@@ -105,6 +104,8 @@ func (s *shrinker) fillQueue() bool {
 		s.queue = append(s.queue, e.Left, e.Right)
 	case *sqlparser.BinaryExpr:
 		s.queue = append(s.queue, e.Left, e.Right)
+	case *sqlparser.BetweenExpr:
+		s.queue = append(s.queue, e.Left, e.From, e.To)
 	case *sqlparser.Literal:
 		switch e.Type {
 		case sqlparser.StrVal:
diff --git a/go/vt/vtgate/simplifier/simplifier.go b/go/vt/vtgate/simplifier/simplifier.go
index 0e19935caba..522da172557 100644
--- a/go/vt/vtgate/simplifier/simplifier.go
+++ b/go/vt/vtgate/simplifier/simplifier.go
@@ -201,7 +201,7 @@ func tryRemoveTable(tables []semantics.TableInfo, in sqlparser.SelectStatement,
 		simplified := removeTable(clone, searchedTS, currentDB, si)
 		name, _ := tbl.Name()
 		if simplified && test(clone) {
-			log.Errorf("removed table %s: %s -> %s", sqlparser.String(name), sqlparser.String(in), sqlparser.String(clone))
+			log.Errorf("removed table `%s`: \n%s\n%s", sqlparser.String(name), sqlparser.String(in), sqlparser.String(clone))
 			return clone
 		}
 	}
diff --git a/go/vt/vtgate/simplifier/simplifier_test.go b/go/vt/vtgate/simplifier/simplifier_test.go
index c9edbbab8d8..e2270a551b5 100644
--- a/go/vt/vtgate/simplifier/simplifier_test.go
+++ b/go/vt/vtgate/simplifier/simplifier_test.go
@@ -20,14 +20,12 @@ import (
 	"fmt"
 	"testing"
 
-	"vitess.io/vitess/go/vt/log"
-
-	"vitess.io/vitess/go/mysql/collations"
-	"vitess.io/vitess/go/vt/vtgate/evalengine"
-
 	"github.com/stretchr/testify/require"
 
+	"vitess.io/vitess/go/mysql/collations"
+	"vitess.io/vitess/go/vt/log"
 	"vitess.io/vitess/go/vt/sqlparser"
+	"vitess.io/vitess/go/vt/vtgate/evalengine"
 )
 
 func TestFindAllExpressions(t *testing.T) {