Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ddl: fix alter table charset bug that change blob column type to text #10477

Merged
merged 10 commits into from Jun 4, 2019

Address comment

  • Loading branch information...
crazycs520 committed May 15, 2019
commit d94379a851513559756959e9ee50bce06b00959c
@@ -22,6 +22,7 @@ import (
"github.com/pingcap/failpoint"
"github.com/pingcap/parser/charset"
"github.com/pingcap/parser/model"
field_types "github.com/pingcap/parser/types"
"github.com/pingcap/tidb/ddl/util"
"github.com/pingcap/tidb/infoschema"
"github.com/pingcap/tidb/kv"
@@ -735,10 +736,7 @@ func onModifyTableCharsetAndCollate(t *meta.Meta, job *model.Job) (ver int64, _
tblInfo.Collate = toCollate
// update column charset.
for _, col := range tblInfo.Columns {
if col.Charset == charset.CharsetBin {
continue
}
if typesNeedCharset(col.Tp) {
if field_types.HasCharset(&col.FieldType) {

This comment has been minimized.

Copy link
@bb7133

bb7133 May 15, 2019

Contributor

I guess typesNeedCharset can be replaced by HasCharset. Another code that calls this function is:

if typesNeedCharset(tp.Tp) {

Can you confirm it here?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 May 17, 2019

Author Contributor

No,

if typesNeedCharset(tp.Tp) {
here may be have not set the column Flag, use HasCharset here may be got some error.

This comment has been minimized.

Copy link
@winkyao

winkyao May 20, 2019

Member

so why you use HashCharset finally?

This comment has been minimized.

Copy link
@crazycs520

crazycs520 May 21, 2019

Author Contributor

HashCharset can be used here(ddl/table.go #739), but ddl/ddl_api.go #299 should use typesNeedCharset function.

This comment has been minimized.

Copy link
@zimulala

zimulala Jun 4, 2019

Member

Sorry, I still don't quite understand, why is it correct to change to HasCharset?

col.Charset = toCharset
col.Collate = toCollate
} else {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.