Merge pull request #3134 from dolthub/angela/conversions

Properly convert enum/set columns to string columns with alter table

Author: angelamayxie

enginetest/queries/script_queries.go

@@ -9224,7 +9224,6 @@ where
 				},
 			},
 			{
-				Skip:  true,
 				Query: "select e from t where e like 'a%' order by e;",
 				Expected: []sql.Row{
 					{"abc"},
@@ -9389,6 +9388,39 @@ where
 			},
 		},
 	},
+	{
+		Name:    "Convert enum columns to string columns with alter table",
+		Dialect: "mysql",
+		SetUpScript: []string{
+			"create table t(pk int primary key, c0 enum('a', 'b', 'c'));",
+			"insert into t values(0, 'a'), (1, 'b'), (2, 'c');",
+			"alter table t modify column c0 varchar(100);",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query:    "select * from t",
+				Expected: []sql.Row{{0, "a"}, {1, "b"}, {2, "c"}},
+			},
+		},
+	},
+	{
+		// https://github.com/dolthub/dolt/issues/9613
+		Skip:    true,
+		Name:    "Convert enum columns to string columns when copying table",
+		Dialect: "mysql",
+		SetUpScript: []string{
+			"create table t(pk int primary key, c0 enum('a', 'b', 'c'));",
+			"insert into t values(0, 'a'), (1, 'b'), (2, 'c');",
+			"create table tt(pk int primary key, c0 varchar(10))",
+			"insert into tt select * from t",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query:    "select * from tt",
+				Expected: []sql.Row{{0, "a"}, {1, "b"}, {2, "c"}},
+			},
+		},
+	},
 	{
 		Name:    "enums with foreign keys",
 		Dialect: "mysql",
@@ -10001,6 +10033,39 @@ where
 			},
 		},
 	},
+	{
+		Name:    "Convert set columns to string columns with alter table",
+		Dialect: "mysql",
+		SetUpScript: []string{
+			"create table t(pk int primary key, c0 set('abc', 'def','ghi'))",
+			"insert into t values(0, 'abc,def'), (1, 'def'), (2, 'ghi');",
+			"alter table t modify column c0 varchar(100);",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query:    "select * from t",
+				Expected: []sql.Row{{0, "abc,def"}, {1, "def"}, {2, "ghi"}},
+			},
+		},
+	},
+	{
+		// https://github.com/dolthub/dolt/issues/9613
+		Skip:    true,
+		Name:    "Convert set columns to string columns when copying table",
+		Dialect: "mysql",
+		SetUpScript: []string{
+			"create table t(pk int primary key, c0 set('abc', 'def','ghi'))",
+			"insert into t values(0, 'abc,def'), (1, 'def'), (2, 'ghi');",
+			"create table tt(pk int primary key, c0 varchar(10))",
+			"insert into tt select * from t",
+		},
+		Assertions: []ScriptTestAssertion{
+			{
+				Query:    "select * from tt",
+				Expected: []sql.Row{{0, "abc,def"}, {1, "def"}, {2, "ghi"}},
+			},
+		},
+	},
 	{
 		Name:    "set with duplicates",
 		Dialect: "mysql",

memory/table.go

@@ -1436,7 +1436,8 @@ func (t *Table) ModifyColumn(ctx *sql.Context, columnName string, column *sql.Co
 			var oldRowWithoutVal sql.Row
 			oldRowWithoutVal = append(oldRowWithoutVal, row[:oldIdx]...)
 			oldRowWithoutVal = append(oldRowWithoutVal, row[oldIdx+1:]...)
-			newVal, inRange, err := column.Type.Convert(ctx, row[oldIdx])
+			oldType := data.schema.Schema[oldIdx].Type
+			newVal, inRange, err := types.TypeAwareConversion(ctx, row[oldIdx], oldType, column.Type)
 			if err != nil {
 				if sql.ErrNotMatchingSRID.Is(err) {
 					err = sql.ErrNotMatchingSRIDWithColName.New(columnName, err)

sql/expression/case.go

@@ -136,7 +136,7 @@ func (c *Case) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
 			}
 			// When unable to convert to the type of the case, return the original value
 			// A common error here is "Out of bounds value for decimal type"
-			if ret, err := types.TypeAwareConversion(ctx, bval, b.Value.Type(), t); err == nil {
+			if ret, inRange, err := types.TypeAwareConversion(ctx, bval, b.Value.Type(), t); inRange && err == nil {
 				return ret, nil
 			}
 			return bval, nil
@@ -150,7 +150,7 @@ func (c *Case) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) {
 		}
 		// When unable to convert to the type of the case, return the original value
 		// A common error here is "Out of bounds value for decimal type"
-		if ret, err := types.TypeAwareConversion(ctx, val, c.Else.Type(), t); err == nil {
+		if ret, inRange, err := types.TypeAwareConversion(ctx, val, c.Else.Type(), t); inRange && err == nil {
 			return ret, nil
 		}
 		return val, nil

sql/expression/comparison.go

@@ -188,7 +188,7 @@ func (c *comparison) castLeftAndRight(ctx *sql.Context, left, right interface{})
 			if r, inRange, err := leftType.Convert(ctx, right); inRange && err == nil {
 				return left, r, leftType, nil
 			} else {
-				l, err := types.TypeAwareConversion(ctx, left, leftType, rightType)
+				l, _, err := types.TypeAwareConversion(ctx, left, leftType, rightType)
 				if err != nil {
 					return nil, nil, nil, err
 				}
@@ -200,7 +200,7 @@ func (c *comparison) castLeftAndRight(ctx *sql.Context, left, right interface{})
 			if l, inRange, err := rightType.Convert(ctx, left); inRange && err == nil {
 				return l, right, rightType, nil
 			} else {
-				r, err := types.TypeAwareConversion(ctx, right, rightType, leftType)
+				r, _, err := types.TypeAwareConversion(ctx, right, rightType, leftType)
 				if err != nil {
 					return nil, nil, nil, err
 				}

sql/expression/convert.go

@@ -291,10 +291,7 @@ func convertValue(ctx *sql.Context, val interface{}, castTo string, originType s
 	}
 	switch strings.ToLower(castTo) {
 	case ConvertToBinary:
-		if types.IsSet(originType) || types.IsEnum(originType) {
-			val, _ = types.TypeAwareConversion(ctx, val, originType, types.LongText)
-		}
-		b, _, err := types.LongBlob.Convert(ctx, val)
+		b, _, err := types.TypeAwareConversion(ctx, val, originType, types.LongBlob)
 		if err != nil {
 			return nil, nil
 		}
@@ -312,8 +309,7 @@ func convertValue(ctx *sql.Context, val interface{}, castTo string, originType s
 		}
 		return truncateConvertedValue(b, typeLength)
 	case ConvertToChar, ConvertToNChar:
-		val, _ = types.TypeAwareConversion(ctx, val, originType, types.LongText)
-		s, _, err := types.LongText.Convert(ctx, val)
+		s, _, err := types.TypeAwareConversion(ctx, val, originType, types.LongText)
 		if err != nil {
 			return nil, nil
 		}

sql/rowexec/ddl_iters.go

@@ -570,7 +570,7 @@ func (i *modifyColumnIter) rewriteTable(ctx *sql.Context, rwt sql.RewritableTabl
 			}
 		}
 
-		newRow, err := projectRowWithTypes(ctx, newSch, projections, r)
+		newRow, err := projectRowWithTypes(ctx, targetSchema, newSch, projections, r)
 		if err != nil {
 			_ = inserter.DiscardChanges(ctx, err)
 			_ = inserter.Close(ctx)
@@ -905,21 +905,21 @@ func (i *loggingKeyValueIter) Close(ctx *sql.Context) error {
 
 // projectRowWithTypes projects the row given with the projections given and additionally converts them to the
 // corresponding types found in the schema given, using the standard type conversion logic.
-func projectRowWithTypes(ctx *sql.Context, sch sql.Schema, projections []sql.Expression, r sql.Row) (sql.Row, error) {
+func projectRowWithTypes(ctx *sql.Context, oldSchema, newSchema sql.Schema, projections []sql.Expression, r sql.Row) (sql.Row, error) {
 	newRow, err := ProjectRow(ctx, projections, r)
 	if err != nil {
 		return nil, err
 	}
 
 	for i := range newRow {
-		converted, inRange, err := sch[i].Type.Convert(ctx, newRow[i])
+		converted, inRange, err := types.TypeAwareConversion(ctx, newRow[i], oldSchema[i].Type, newSchema[i].Type)
 		if err != nil {
 			if sql.ErrNotMatchingSRID.Is(err) {
-				err = sql.ErrNotMatchingSRIDWithColName.New(sch[i].Name, err)
+				err = sql.ErrNotMatchingSRIDWithColName.New(newSchema[i].Name, err)
 			}
 			return nil, err
 		} else if !inRange {
-			return nil, sql.ErrValueOutOfRange.New(newRow[i], sch[i].Type)
+			return nil, sql.ErrValueOutOfRange.New(newRow[i], newSchema[i].Type)
 		}
 		newRow[i] = converted
 	}

sql/types/conversion.go

@@ -738,19 +738,16 @@ func GeneralizeTypes(a, b sql.Type) sql.Type {
 // TypeAwareConversion converts a value to a specified type, with awareness of the value's original type. This is
 // necessary because some types, such as EnumType and SetType, are stored as ints and require information from the
 // original type to properly convert to strings.
-func TypeAwareConversion(ctx *sql.Context, val interface{}, originalType sql.Type, convertedType sql.Type) (interface{}, error) {
+func TypeAwareConversion(ctx *sql.Context, val interface{}, originalType sql.Type, convertedType sql.Type) (interface{}, sql.ConvertInRange, error) {
 	if val == nil {
-		return nil, nil
+		return nil, sql.InRange, nil
 	}
-	var converted interface{}
 	var err error
-	if IsTextOnly(convertedType) {
-		converted, _, err = ConvertToCollatedString(ctx, val, originalType)
-	} else {
-		converted, _, err = convertedType.Convert(ctx, val)
-	}
-	if err != nil {
-		return nil, err
+	if (IsEnum(originalType) || IsSet(originalType)) && IsText(convertedType) {
+		val, _, err = ConvertToCollatedString(ctx, val, originalType)
+		if err != nil {
+			return nil, sql.OutOfRange, err
+		}
 	}
-	return converted, nil
+	return convertedType.Convert(ctx, val)
 }