refactor: minor cleanups in planner code (#14642)

vitessio · Nov 30, 2023 · e818ca5 · e818ca5
1 parent 3176543
commit e818ca5
Show file tree

Hide file tree

Showing 6 changed files with 21 additions and 22 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {