some feedback

Author: James Cor

sql/base_session.go

@@ -254,20 +254,24 @@ func (s *BaseSession) IncrementStatusVariable(ctx *Context, statVarName string,
 	return
 }
 
+// NewStoredProcParam creates a new Stored Procedure Parameter in the Session
 func (s *BaseSession) NewStoredProcParam(name string, param *StoredProcParam) {
 	if _, ok := s.storedProcParams[name]; ok {
 		return
 	}
 	s.storedProcParams[name] = param
 }
 
+// GetStoredProcParam retrieves the named stored procedure parameter, from the Session, returning nil if not found.
 func (s *BaseSession) GetStoredProcParam(name string) *StoredProcParam {
 	if param, ok := s.storedProcParams[name]; ok {
 		return param
 	}
 	return nil
 }
 
+// SetStoredProcParam sets the named Stored Procedure Parameter from the Session to val and marks it as HasSet.
+// If the Parameter has not been initialized, this will throw an error.
 func (s *BaseSession) SetStoredProcParam(name string, val any) error {
 	param := s.GetStoredProcParam(name)
 	if param == nil {

sql/core.go

@@ -880,13 +880,17 @@ func IncrementStatusVariable(ctx *Context, name string, val int) {
 	ctx.Session.IncrementStatusVariable(ctx, name, val)
 }
 
+// StoredProcParam is a Parameter for a Stored Procedure.
+// Stored Procedures Parameters can be referenced from within other Stored Procedures, so we need to store them
+// somewhere that is accessible between interpreter calls to the engine.
 type StoredProcParam struct {
 	Type       Type
 	Value      any
 	HasBeenSet bool
 	Reference  *StoredProcParam
 }
 
+// SetValue saves val to the StoredProcParam, and set HasBeenSet to true.
 func (s *StoredProcParam) SetValue(val any) {
 	s.Value = val
 	s.HasBeenSet = true

sql/plan/call.go

@@ -23,8 +23,6 @@ import (
 	"github.com/dolthub/go-mysql-server/sql/types"
 )
 
-// TODO: we need different types of calls: one for external procedures one for stored procedures
-
 type Call struct {
 	db        sql.Database
 	Name      string
@@ -39,7 +37,7 @@ type Call struct {
 	Runner sql.StatementRunner
 	Ops    []procedures.InterpreterOperation
 
-	// TODO: sure whatever
+	// retain the result schema
 	resSch sql.Schema
 }
 

sql/plan/procedure.go

@@ -138,6 +138,7 @@ func (p *Procedure) Resolved() bool {
 	return true
 }
 
+// IsReadOnly implements the sql.Node interface.
 func (p *Procedure) IsReadOnly() bool {
 	if p.ExternalProc != nil {
 		return p.ExternalProc.IsReadOnly()

sql/procedures/interpreter_logic.go

@@ -40,6 +40,8 @@ type InterpreterNode interface {
 	SetSchema(sch sql.Schema)
 }
 
+// replaceVariablesInExpr will search for every ast.Node and handle each one on a case by case basis.
+// If a new ast.Node is added to the vitess parser we may need to add a case for it here.
 func replaceVariablesInExpr(ctx *sql.Context, stack *InterpreterStack, expr ast.SQLNode, asOf *ast.AsOf) (ast.SQLNode, error) {
 	switch e := expr.(type) {
 	case *ast.ColName:
@@ -286,6 +288,9 @@ func query(ctx *sql.Context, runner sql.StatementRunner, stmt ast.Statement) (sq
 			if rErr == io.EOF {
 				break
 			}
+			if cErr := rowIter.Close(ctx); cErr != nil {
+				return nil, nil, cErr
+			}
 			return nil, nil, rErr
 		}
 		rows = append(rows, row)
@@ -402,6 +407,9 @@ func execOp(ctx *sql.Context, runner sql.StatementRunner, stack *InterpreterStac
 			return 0, nil, nil, nil, err
 		}
 		if _, err = rowIter.Next(ctx); err != io.EOF {
+			if rErr := rowIter.Close(ctx); rErr != nil {
+				return 0, nil, nil, nil, rErr
+			}
 			return 0, nil, nil, nil, err
 		}
 		if err = rowIter.Close(ctx); err != nil {
@@ -507,7 +515,7 @@ func execOp(ctx *sql.Context, runner sql.StatementRunner, stack *InterpreterStac
 				return 0, nil, nil, nil, fmt.Errorf("warnings not yet implemented")
 			}
 		} else {
-			cond := stack.GetCondition(strings.ToLower(signalStmt.ConditionName))
+			cond := stack.GetCondition(signalStmt.ConditionName)
 			if cond == nil {
 				return 0, nil, nil, nil, sql.ErrDeclareConditionNotFound.New(signalStmt.ConditionName)
 			}
@@ -587,7 +595,7 @@ func execOp(ctx *sql.Context, runner sql.StatementRunner, stack *InterpreterStac
 
 	case OpCode_Open:
 		openCur := operation.PrimaryData.(*ast.OpenCursor)
-		cursor := stack.GetCursor(strings.ToLower(openCur.Name))
+		cursor := stack.GetCursor(openCur.Name)
 		if cursor == nil {
 			return 0, nil, nil, nil, sql.ErrCursorNotFound.New(openCur.Name)
 		}
@@ -607,7 +615,7 @@ func execOp(ctx *sql.Context, runner sql.StatementRunner, stack *InterpreterStac
 
 	case OpCode_Fetch:
 		fetchCur := operation.PrimaryData.(*ast.FetchCursor)
-		cursor := stack.GetCursor(strings.ToLower(fetchCur.Name))
+		cursor := stack.GetCursor(fetchCur.Name)
 		if cursor == nil {
 			return 0, nil, nil, nil, sql.ErrCursorNotFound.New(fetchCur.Name)
 		}
@@ -641,7 +649,7 @@ func execOp(ctx *sql.Context, runner sql.StatementRunner, stack *InterpreterStac
 
 	case OpCode_Close:
 		closeCur := operation.PrimaryData.(*ast.CloseCursor)
-		cursor := stack.GetCursor(strings.ToLower(closeCur.Name))
+		cursor := stack.GetCursor(closeCur.Name)
 		if cursor == nil {
 			return 0, nil, nil, nil, sql.ErrCursorNotFound.New(closeCur.Name)
 		}
@@ -680,7 +688,7 @@ func execOp(ctx *sql.Context, runner sql.StatementRunner, stack *InterpreterStac
 			return 0, nil, nil, nil, err
 		}
 
-		err = stack.SetVariable(strings.ToLower(operation.Target), row[0])
+		err = stack.SetVariable(operation.Target, row[0])
 		if err != nil {
 			err = ctx.Session.SetStoredProcParam(operation.Target, row[0])
 			if err != nil {

sql/procedures/interpreter_stack.go

@@ -17,6 +17,7 @@ package procedures
 import (
 	"fmt"
 	"strconv"
+	"strings"
 
 	"github.com/dolthub/go-mysql-server/sql"
 	"github.com/dolthub/go-mysql-server/sql/types"
@@ -205,6 +206,7 @@ func (is *InterpreterStack) NewVariableAlias(alias string, variable *Interpreter
 // GetVariable traverses the stack (starting from the top) to find a variable with a matching name. Returns nil if no
 // variable was found.
 func (is *InterpreterStack) GetVariable(name string) *InterpreterVariable {
+	name = strings.ToLower(name)
 	for i := 0; i < is.stack.Len(); i++ {
 		if iv, ok := is.stack.PeekDepth(i).variables[name]; ok {
 			return iv
@@ -248,6 +250,7 @@ func (is *InterpreterStack) NewCondition(name string, sqlState string, mysqlErrC
 // GetCondition traverses the stack (starting from the top) to find a condition with a matching name. Returns nil if no
 // variable was found.
 func (is *InterpreterStack) GetCondition(name string) *InterpreterCondition {
+	name = strings.ToLower(name)
 	for i := 0; i < is.stack.Len(); i++ {
 		if ic, ok := is.stack.PeekDepth(i).conditions[name]; ok {
 			return ic
@@ -266,6 +269,7 @@ func (is *InterpreterStack) NewCursor(name string, selStmt ast.SelectStatement)
 // GetCursor traverses the stack (starting from the top) to find a condition with a matching name. Returns nil if no
 // variable was found.
 func (is *InterpreterStack) GetCursor(name string) *InterpreterCursor {
+	name = strings.ToLower(name)
 	for i := 0; i < is.stack.Len(); i++ {
 		if ic, ok := is.stack.PeekDepth(i).cursors[name]; ok {
 			return ic

sql/rowexec/proc.go

@@ -262,11 +262,13 @@ func (b *BaseBuilder) buildCall(ctx *sql.Context, n *plan.Call, row sql.Row) (sq
 
 	// We might close transactions in the procedure, so we need to start a new one if we're not in one already
 	if sess, ok := ctx.Session.(sql.TransactionSession); ok {
-		tx, tErr := sess.StartTransaction(ctx, sql.ReadWrite)
-		if tErr != nil {
-			return nil, tErr
+		if tx := ctx.GetTransaction(); tx == nil {
+			tx, err = sess.StartTransaction(ctx, sql.ReadWrite)
+			if err != nil {
+				return nil, err
+			}
+			ctx.SetTransaction(tx)
 		}
-		ctx.SetTransaction(tx)
 	}
 
 	return &callIter{