address comment

pingcap · Apr 24, 2023 · cbd7c65 · cbd7c65
1 parent f16b7b9
commit cbd7c65
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 10 deletions.
diff --git a/ddl/column_change_test.go b/ddl/column_change_test.go
@@ -484,5 +484,5 @@ func TestIssue38988And24321(t *testing.T) {
 	// For issue https://github.com/pingcap/tidb/issues/24321
 	// Note, the result is not the same with MySQL, since the limitation of the current modify column implementation.
 	tk.MustExec("create table t2(id int, a int, b int generated always as (abs(a)) virtual);")
-	tk.MustGetErrCode("alter table t2 modify column a bigint;", errno.ErrDependentByGeneratedColumn)
+	tk.MustGetErrCode("alter table t2 modify column a bigint;", errno.ErrUnsupportedOnGeneratedColumn)
 }
diff --git a/ddl/column_modify_test.go b/ddl/column_modify_test.go
@@ -582,13 +582,15 @@ func TestGeneratedColumnDDL(t *testing.T) {
 	result = tk.MustQuery(`DESC test_gv_ddl`)
 	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b int(11) YES  <nil> VIRTUAL GENERATED`, `c int(11) YES  <nil> STORED GENERATED`))
 
-	tk.MustExec(`alter table test_gv_ddl change column b b bigint as (a+100) virtual`)
-	result = tk.MustQuery(`DESC test_gv_ddl`)
-	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b bigint(20) YES  <nil> VIRTUAL GENERATED`, `c int(11) YES  <nil> STORED GENERATED`))
+	// According to https://github.com/pingcap/tidb/issues/24321, this test case is not supported.
+	// Although in MySQL this is a legal one.
+	// tk.MustExec(`alter table test_gv_ddl change column b b bigint as (a+100) virtual`)
+	// result = tk.MustQuery(`DESC test_gv_ddl`)
+	// result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b bigint(20) YES  <nil> VIRTUAL GENERATED`, `c int(11) YES  <nil> STORED GENERATED`))
 
 	tk.MustExec(`alter table test_gv_ddl change column c cnew bigint`)
 	result = tk.MustQuery(`DESC test_gv_ddl`)
-	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b bigint(20) YES  <nil> VIRTUAL GENERATED`, `cnew bigint(20) YES  <nil> `))
+	result.Check(testkit.Rows(`a int(11) YES  <nil> `, `b int(11) YES  <nil> VIRTUAL GENERATED`, `cnew bigint(20) YES  <nil> `))
 
 	// Test generated column `\\`.
 	tk.MustExec("drop table if exists t")

diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go
@@ -4947,17 +4947,20 @@ func GetModifiableColumnJob(
 	if newColName.L == model.ExtraHandleName.L {
 		return nil, dbterror.ErrWrongColumnName.GenWithStackByArgs(newColName.L)
 	}
+	errG := checkModifyColumnWithGeneratedColumnsConstraint(t.Cols(), originalColName)
+
 	// If we want to rename the column name, we need to check whether it already exists.
 	if newColName.L != originalColName.L {
 		c := table.FindCol(t.Cols(), newColName.L)
 		if c != nil {
 			return nil, infoschema.ErrColumnExists.GenWithStackByArgs(newColName)
 		}
-	}
-	// And also check the generated columns dependency.
-	err = checkModifyColumnWithGeneratedColumnsConstraint(t.Cols(), originalColName)
-	if err != nil {
-		return nil, errors.Trace(err)
+
+		// And also check the generated columns dependency, if some generated columns
+		// depend on this column, we can't rename the column name.
+		if errG != nil {
+			return nil, errors.Trace(errG)
+		}
 	}
 
 	// Constraints in the new column means adding new constraints. Errors should thrown,
@@ -5167,6 +5170,11 @@ func GetModifiableColumnJob(
 	if err = checkModifyGeneratedColumn(sctx, schema.Name, t, col, newCol, specNewColumn, spec.Position); err != nil {
 		return nil, errors.Trace(err)
 	}
+	if errG != nil {
+		// According to issue https://github.com/pingcap/tidb/issues/24321,
+		// changing the type of a column involving generating a column is prohibited.
+		return nil, dbterror.ErrUnsupportedOnGeneratedColumn.GenWithStackByArgs(errG.Error())
+	}
 
 	if t.Meta().TTLInfo != nil {
 		// the column referenced by TTL should be a time type