bugfix: wrong field type returned for SUM (#15192)

Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Vicent Marti <[email protected]>
Co-authored-by: Vicent Marti <[email protected]>

go/test/endtoend/utils/mysql.go

@@ -27,15 +27,14 @@ import (
 
 	"github.com/stretchr/testify/assert"
 
+	"vitess.io/vitess/go/mysql"
 	"vitess.io/vitess/go/mysql/collations"
-
 	"vitess.io/vitess/go/sqltypes"
-	"vitess.io/vitess/go/vt/dbconfigs"
-	"vitess.io/vitess/go/vt/sqlparser"
-
-	"vitess.io/vitess/go/mysql"
 	"vitess.io/vitess/go/test/endtoend/cluster"
+	"vitess.io/vitess/go/vt/dbconfigs"
 	"vitess.io/vitess/go/vt/mysqlctl"
+	querypb "vitess.io/vitess/go/vt/proto/query"
+	"vitess.io/vitess/go/vt/sqlparser"
 )
 
 const mysqlShutdownTimeout = 1 * time.Minute
@@ -160,7 +159,9 @@ func prepareMySQLWithSchema(params mysql.ConnParams, sql string) error {
 	return nil
 }
 
-func compareVitessAndMySQLResults(t *testing.T, query string, vtConn *mysql.Conn, vtQr, mysqlQr *sqltypes.Result, compareColumns bool) error {
+func compareVitessAndMySQLResults(t *testing.T, query string, vtConn *mysql.Conn, vtQr, mysqlQr *sqltypes.Result, compareColumnNames bool) error {
+	t.Helper()
+
 	if vtQr == nil && mysqlQr == nil {
 		return nil
 	}
@@ -173,28 +174,29 @@ func compareVitessAndMySQLResults(t *testing.T, query string, vtConn *mysql.Conn
 		return errors.New("MySQL result is 'nil' while Vitess' is not.\n")
 	}
 
-	var errStr string
-	if compareColumns {
-		vtColCount := len(vtQr.Fields)
-		myColCount := len(mysqlQr.Fields)
-		if vtColCount > 0 && myColCount > 0 {
-			if vtColCount != myColCount {
-				t.Errorf("column count does not match: %d vs %d", vtColCount, myColCount)
-				errStr += fmt.Sprintf("column count does not match: %d vs %d\n", vtColCount, myColCount)
-			}
-
-			var vtCols []string
-			var myCols []string
-			for i, vtField := range vtQr.Fields {
-				vtCols = append(vtCols, vtField.Name)
-				myCols = append(myCols, mysqlQr.Fields[i].Name)
-			}
-			if !assert.Equal(t, myCols, vtCols, "column names do not match - the expected values are what mysql produced") {
-				errStr += "column names do not match - the expected values are what mysql produced\n"
-				errStr += fmt.Sprintf("Not equal: \nexpected: %v\nactual: %v\n", myCols, vtCols)
-			}
+	vtColCount := len(vtQr.Fields)
+	myColCount := len(mysqlQr.Fields)
+
+	if vtColCount != myColCount {
+		t.Errorf("column count does not match: %d vs %d", vtColCount, myColCount)
+	}
+
+	if vtColCount > 0 {
+		var vtCols []string
+		var myCols []string
+		for i, vtField := range vtQr.Fields {
+			myField := mysqlQr.Fields[i]
+			checkFields(t, myField.Name, vtField, myField)
+
+			vtCols = append(vtCols, vtField.Name)
+			myCols = append(myCols, myField.Name)
+		}
+
+		if compareColumnNames && !assert.Equal(t, myCols, vtCols, "column names do not match - the expected values are what mysql produced") {
+			t.Errorf("column names do not match - the expected values are what mysql produced\nNot equal: \nexpected: %v\nactual: %v\n", myCols, vtCols)
 		}
 	}
+
 	stmt, err := sqlparser.NewTestParser().Parse(query)
 	if err != nil {
 		t.Error(err)
@@ -209,7 +211,7 @@ func compareVitessAndMySQLResults(t *testing.T, query string, vtConn *mysql.Conn
 		return nil
 	}
 
-	errStr += "Query (" + query + ") results mismatched.\nVitess Results:\n"
+	errStr := "Query (" + query + ") results mismatched.\nVitess Results:\n"
 	for _, row := range vtQr.Rows {
 		errStr += fmt.Sprintf("%s\n", row)
 	}
@@ -229,6 +231,20 @@ func compareVitessAndMySQLResults(t *testing.T, query string, vtConn *mysql.Conn
 	return errors.New(errStr)
 }
 
+func checkFields(t *testing.T, columnName string, vtField, myField *querypb.Field) {
+	t.Helper()
+	if vtField.Type != myField.Type {
+		t.Errorf("for column %s field types do not match\nNot equal: \nMySQL: %v\nVitess: %v\n", columnName, myField.Type.String(), vtField.Type.String())
+	}
+
+	// starting in Vitess 20, decimal types are properly sized in their field information
+	if BinaryIsAtLeastAtVersion(20, "vtgate") && vtField.Type == sqltypes.Decimal {
+		if vtField.Decimals != myField.Decimals {
+			t.Errorf("for column %s field decimals count do not match\nNot equal: \nMySQL: %v\nVitess: %v\n", columnName, myField.Decimals, vtField.Decimals)
+		}
+	}
+}
+
 func compareVitessAndMySQLErrors(t *testing.T, vtErr, mysqlErr error) {
 	if vtErr != nil && mysqlErr != nil || vtErr == nil && mysqlErr == nil {
 		return

go/vt/vtgate/engine/aggregations.go

@@ -91,9 +91,9 @@ func (ap *AggregateParams) String() string {
 
 func (ap *AggregateParams) typ(inputType querypb.Type) querypb.Type {
 	if ap.OrigOpcode != AggregateUnassigned {
-		return ap.OrigOpcode.Type(inputType)
+		return ap.OrigOpcode.SQLType(inputType)
 	}
-	return ap.Opcode.Type(inputType)
+	return ap.Opcode.SQLType(inputType)
 }
 
 type aggregator interface {

go/vt/vtgate/engine/opcode/constants.go

@@ -19,8 +19,10 @@ package opcode
 import (
 	"fmt"
 
+	"vitess.io/vitess/go/mysql/collations"
 	"vitess.io/vitess/go/sqltypes"
 	querypb "vitess.io/vitess/go/vt/proto/query"
+	"vitess.io/vitess/go/vt/vtgate/evalengine"
 )
 
 // PulloutOpcode is a number representing the opcode
@@ -138,7 +140,7 @@ func (code AggregateOpcode) MarshalJSON() ([]byte, error) {
 }
 
 // Type returns the opcode return sql type, and a bool telling is we are sure about this type or not
-func (code AggregateOpcode) Type(typ querypb.Type) querypb.Type {
+func (code AggregateOpcode) SQLType(typ querypb.Type) querypb.Type {
 	switch code {
 	case AggregateUnassigned:
 		return sqltypes.Null
@@ -169,6 +171,28 @@ func (code AggregateOpcode) Type(typ querypb.Type) querypb.Type {
 	}
 }
 
+func (code AggregateOpcode) Nullable() bool {
+	switch code {
+	case AggregateCount, AggregateCountStar:
+		return false
+	default:
+		return true
+	}
+}
+
+func (code AggregateOpcode) ResolveType(t evalengine.Type, env *collations.Environment) evalengine.Type {
+	sqltype := code.SQLType(t.Type())
+	collation := collations.CollationForType(sqltype, env.DefaultConnectionCharset())
+	nullable := code.Nullable()
+	size := t.Size()
+
+	scale := t.Scale()
+	if code == AggregateAvg {
+		scale += 4
+	}
+	return evalengine.NewTypeEx(sqltype, collation, nullable, size, scale)
+}
+
 func (code AggregateOpcode) NeedsComparableValues() bool {
 	switch code {
 	case AggregateCountDistinct, AggregateSumDistinct, AggregateMin, AggregateMax:

go/vt/vtgate/engine/opcode/constants_test.go

@@ -30,7 +30,7 @@ import (
 func TestCheckAllAggrOpCodes(t *testing.T) {
 	// This test is just checking that we never reach the panic when using Type() on valid opcodes
 	for i := AggregateOpcode(0); i < _NumOfOpCodes; i++ {
-		i.Type(sqltypes.Null)
+		i.SQLType(sqltypes.Null)
 	}
 }
 
@@ -56,7 +56,7 @@ func TestType(t *testing.T) {
 
 	for _, tc := range tt {
 		t.Run(tc.opcode.String()+"_"+tc.typ.String(), func(t *testing.T) {
-			out := tc.opcode.Type(tc.typ)
+			out := tc.opcode.SQLType(tc.typ)
 			assert.Equal(t, tc.out, out)
 		})
 	}
@@ -70,7 +70,7 @@ func TestType_Panic(t *testing.T) {
 			assert.Contains(t, errMsg, "ERROR", "Expected panic message containing 'ERROR'")
 		}
 	}()
-	AggregateOpcode(999).Type(sqltypes.VarChar)
+	AggregateOpcode(999).SQLType(sqltypes.VarChar)
 }
 
 func TestNeedsListArg(t *testing.T) {

go/vt/vtgate/engine/projection.go

@@ -158,10 +158,12 @@ func (p *Projection) evalFields(env *evalengine.ExpressionEnv, infields []*query
 			fl |= uint32(querypb.MySqlFlag_NOT_NULL_FLAG)
 		}
 		fields = append(fields, &querypb.Field{
-			Name:    col,
-			Type:    typ.Type(),
-			Charset: uint32(typ.Collation()),
-			Flags:   fl,
+			Name:         col,
+			Type:         typ.Type(),
+			Charset:      uint32(typ.Collation()),
+			ColumnLength: uint32(typ.Size()),
+			Decimals:     uint32(typ.Scale()),
+			Flags:        fl,
 		})
 	}
 	return fields, nil