clear warnings better and separate warning count from actual warnings (#2696)

Author: jycor

engine.go

@@ -253,6 +253,23 @@ func (e *Engine) Query(ctx *sql.Context, query string) (sql.Schema, sql.RowIter,
 	return e.QueryWithBindings(ctx, query, nil, nil, nil)
 }
 
+func clearWarnings(ctx *sql.Context, node sql.Node) {
+	if ctx == nil || ctx.Session == nil {
+		return
+	}
+
+	switch n := node.(type) {
+	case *plan.Offset, *plan.Limit:
+		// `show warning limit x offset y` is valid, so we need to recurse
+		clearWarnings(ctx, n.Children()[0])
+	case plan.ShowWarnings:
+		// ShowWarnings should not clear the warnings, but should still reset the warning count.
+		ctx.ClearWarningCount()
+	default:
+		ctx.ClearWarnings()
+	}
+}
+
 func bindingsToExprs(bindings map[string]*querypb.BindVariable) (map[string]sql.Expression, error) {
 	res := make(map[string]sql.Expression, len(bindings))
 	for k, v := range bindings {
@@ -387,10 +404,18 @@ func (e *Engine) QueryWithBindings(ctx *sql.Context, query string, parsed sqlpar
 		return nil, nil, nil, err
 	}
 
+	// planbuilding can produce warnings, so we need to preserve them
+	numPrevWarnings := len(ctx.Session.Warnings())
 	bound, qFlags, err := e.bindQuery(ctx, query, parsed, bindings, binder, qFlags)
 	if err != nil {
 		return nil, nil, nil, err
 	}
+	newWarnings := ctx.Session.Warnings()[numPrevWarnings:]
+	clearWarnings(ctx, bound)
+	// restore new warnings (backwards because they are in reverse order)
+	for i := len(newWarnings) - 1; i >= 0; i-- {
+		ctx.Session.Warn(newWarnings[i])
+	}
 
 	analyzed, err := e.analyzeNode(ctx, query, bound, qFlags)
 	if err != nil {

enginetest/queries/script_queries.go

@@ -8809,21 +8809,17 @@ var CreateDatabaseScripts = []ScriptTest{
 				Expected: []sql.Row{{types.NewOkResult(1)}},
 			},
 			{
-				SkipResultCheckOnServerEngine: true, // tracking issue here, https://github.com/dolthub/dolt/issues/6921. Also for when run with prepares, the warning is added twice
-				Query:                         "SHOW WARNINGS /* 1 */",
-				Expected:                      []sql.Row{{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"}},
+				Query:    "SHOW WARNINGS /* 1 */",
+				Expected: []sql.Row{{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"}},
 			},
 			{
 				Query:    "CREATE DATABASE newtest1db DEFAULT COLLATE binary ENCRYPTION='Y'",
 				Expected: []sql.Row{{types.NewOkResult(1)}},
 			},
 			{
-				SkipResultCheckOnServerEngine: true, // tracking issue here, https://github.com/dolthub/dolt/issues/6921.
-				// TODO: There should only be one warning (the warnings are not clearing for create database query) AND 'PREPARE' statements should not create warning from its query
 				Query: "SHOW WARNINGS /* 2 */",
 				Expected: []sql.Row{
 					{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"},
-					{"Warning", 1235, "Setting CHARACTER SET, COLLATION and ENCRYPTION are not supported yet"},
 				},
 			},
 			{
@@ -8939,10 +8935,8 @@ var DropDatabaseScripts = []ScriptTest{
 				Expected: []sql.Row{{types.OkResult{RowsAffected: 1}}},
 			},
 			{
-				// TODO: there should not be warning
-				//  https://github.com/dolthub/dolt/issues/6921
 				Query:    "SHOW WARNINGS",
-				Expected: []sql.Row{{"Note", 1008, "Can't drop database mydb; database doesn't exist "}},
+				Expected: []sql.Row{},
 			},
 			{
 				Query:    "SELECT DATABASE()",

sql/analyzer/analyzer.go

@@ -406,8 +406,7 @@ func NewProcRuleSelector(sel RuleSelector) RuleSelector {
 			// once after default rules should only be run once
 			AutocommitId,
 			TrackProcessId,
-			parallelizeId,
-			clearWarningsId:
+			parallelizeId:
 			return false
 		}
 		return sel(id)

sql/analyzer/rule_ids.go

@@ -90,5 +90,4 @@ const (
 	AutocommitId                     // addAutocommit
 	TrackProcessId                   // trackProcess
 	parallelizeId                    // parallelize
-	clearWarningsId                  // clearWarnings
 )

sql/analyzer/ruleid_string.go

@@ -29,7 +29,6 @@ func init() {
 		{AutocommitId, addAutocommit},
 		{TrackProcessId, trackProcess},
 		{parallelizeId, parallelize},
-		{clearWarningsId, clearWarnings},
 	}
 }
 

sql/analyzer/rules.go

@@ -41,7 +41,7 @@ type BaseSession struct {
 	viewReg          *ViewRegistry
 	warnings         []*Warning
 	warningLock      bool
-	warncnt          uint16
+	warningCount     uint16 // num of warnings from recent query; does not always equal to len(warnings)
 	locks            map[string]bool
 	queriedDb        string
 	lastQueryInfo    map[string]*atomic.Value
@@ -331,6 +331,7 @@ func (s *BaseSession) SetConnectionId(id uint32) {
 // Warn stores the warning in the session.
 func (s *BaseSession) Warn(warn *Warning) {
 	s.warnings = append(s.warnings, warn)
+	s.warningCount = uint16(len(s.warnings))
 }
 
 // Warnings returns a copy of session warnings (from the most recent - the last one)
@@ -354,25 +355,25 @@ func (s *BaseSession) UnlockWarnings() {
 	s.warningLock = false
 }
 
+// ClearWarningCount cleans up session warnings
+func (s *BaseSession) ClearWarningCount() {
+	s.warningCount = 0
+}
+
 // ClearWarnings cleans up session warnings
 func (s *BaseSession) ClearWarnings() {
 	if s.warningLock {
 		return
 	}
-	cnt := uint16(len(s.warnings))
-	if s.warncnt != cnt {
-		s.warncnt = cnt
-		return
-	}
 	if s.warnings != nil {
 		s.warnings = s.warnings[:0]
 	}
-	s.warncnt = 0
+	s.ClearWarningCount()
 }
 
 // WarningCount returns a number of session warnings
 func (s *BaseSession) WarningCount() uint16 {
-	return uint16(len(s.warnings))
+	return s.warningCount
 }
 
 // AddLock adds a lock to the set of locks owned by this user which will need to be released if this session terminates

sql/analyzer/warnings.go

@@ -104,6 +104,8 @@ type Session interface {
 	Warn(warn *Warning)
 	// Warnings returns a copy of session warnings (from the most recent).
 	Warnings() []*Warning
+	// ClearWarningCount clears the warning count without clearing the actual warnings.
+	ClearWarningCount()
 	// ClearWarnings cleans up session warnings.
 	ClearWarnings()
 	// WarningCount returns a number of session warnings