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 a bug in column charset and collate when create table and modify column (#11300) #11424

Closed
wants to merge 2 commits into from

Conversation

AilinKid
Copy link
Contributor

What problem does this PR solve?

cherry-pick to release 3.0 #11300

Check List

Tests

  • Unit test
  • Integration test

Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AilinKid AilinKid changed the title DDL: fix a bug in column charset and collate when create table and modify column DDL: fix a bug in column charset and collate when create table and modify column#11300 Jul 27, 2019
@AilinKid AilinKid changed the title DDL: fix a bug in column charset and collate when create table and modify column#11300 DDL: fix a bug in column charset and collate when create table and modify column(#11300) Jul 27, 2019
bb7133
bb7133 previously approved these changes Jul 27, 2019
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bb7133 bb7133 added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed component/DDL-need-LGT3 labels Jul 27, 2019
@bb7133 bb7133 changed the title DDL: fix a bug in column charset and collate when create table and modify column(#11300) DDL: fix a bug in column charset and collate when create table and modify column (#11300) Jul 27, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 27, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jul 27, 2019

@AilinKid merge failed.

@AilinKid
Copy link
Contributor Author

/run-all-tests

}
if len(tp.Charset) == 0 {
tp.Charset = derivedCollation.CharsetName
} else if tp.Charset != derivedCollation.CharsetName {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it dead code in this else if clause? The condition len(tp.Charset) in line 319 is always to be true, because line 301 checks the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it dead code in this else if clause? The condition len(tp.Charset) in line 319 is always to be true, because line 301 checks the same thing.

The condition len(tp.Charset) in line 319 not always to be true. For the first time, it is. After we assigned charset to it, it should be judged in following loop.

op := def.Options[i]
if op.Tp == ast.ColumnOptionCollate {
specifiedCollates = append(specifiedCollates, op.StrValue)
def.Options = append(def.Options[:i], def.Options[i+1:]...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if we don't delete the ColumnOptionCollate option in ColumnDef?

Copy link
Contributor Author

@AilinKid AilinKid Jul 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if we don't delete the ColumnOptionCollate option in ColumnDef?

Here if we don't remove it in Option, it is ok for the subsequent logic, not a big deal.
But for ast.Restore func, it will cause duplicate collate in restored SQL string.

@AilinKid
Copy link
Contributor Author

/run-all-tests

@AilinKid
Copy link
Contributor Author

/run-all-tests

@AilinKid
Copy link
Contributor Author

/rebuild

@AilinKid AilinKid closed this Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants