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.

Sign up for GitHub

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

Jump to bottom

executor: fix wrong behavior of set charset statement #16984

Merged
merged 7 commits into from
May 19, 2020
Merged

executor: fix wrong behavior of set charset statement #16984

merged 7 commits into from
May 19, 2020

Conversation

wjhuang2016
Copy link
Member

@wjhuang2016 wjhuang2016 commented May 6, 2020 •
edited
Loading

What problem does this PR solve?

Problem Summary:

Previously, we take set charset as set names, and set charsetCollation too. However, it's wrong, as it's stated in here

What is changed and how it works?

Distinguish set names and set charset statements and handle them differently.
Related Parser PR.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • UnitTest

Side effects

Release note

  • Fix wrong behavior of set charset statement

@wjhuang2016 wjhuang2016 requested a review from a team as a code owner May 6, 2020 08:53
@ghost ghost requested review from SunRunAway and removed request for a team May 6, 2020 08:53
@wjhuang2016 wjhuang2016 mentioned this pull request May 6, 2020
Merged
@github-actions github-actions bot added the sig/execution SIG execution label May 6, 2020
@tiancaiamao
Copy link
Contributor 

[2020-05-06T08:56:12.759Z] ----------------------------------------------------------------------
[2020-05-06T08:56:12.759Z] FAIL: set_test.go:421: testSuite5.TestSetCharset
[2020-05-06T08:56:12.759Z] 
[2020-05-06T08:56:12.759Z] set_test.go:500:
[2020-05-06T08:56:12.759Z]     check(
[2020-05-06T08:56:12.759Z]         "latin1",
[2020-05-06T08:56:12.759Z]         "latin1",
[2020-05-06T08:56:12.759Z]         "latin1",
[2020-05-06T08:56:12.759Z]         "utf8mb4",
[2020-05-06T08:56:12.759Z]         "utf8mb4",
[2020-05-06T08:56:12.759Z]         "utf8",
[2020-05-06T08:56:12.759Z]         "binary",
[2020-05-06T08:56:12.759Z]     )
[2020-05-06T08:56:12.759Z] set_test.go:440:
[2020-05-06T08:56:12.759Z]     c.Assert(sVar, Equals, args[i], Commentf("%d: %s", i, characterSetVariables[i]))
[2020-05-06T08:56:12.759Z] ... obtained string = "utf8mb4"
[2020-05-06T08:56:12.759Z] ... expected string = "latin1"
[2020-05-06T08:56:12.759Z] ... 1: character_set_connection

@wjhuang2016

@wjhuang2016
Copy link
Member Author 

[2020-05-06T08:56:12.759Z] ----------------------------------------------------------------------
[2020-05-06T08:56:12.759Z] FAIL: set_test.go:421: testSuite5.TestSetCharset
[2020-05-06T08:56:12.759Z] 
[2020-05-06T08:56:12.759Z] set_test.go:500:
[2020-05-06T08:56:12.759Z]     check(
[2020-05-06T08:56:12.759Z]         "latin1",
[2020-05-06T08:56:12.759Z]         "latin1",
[2020-05-06T08:56:12.759Z]         "latin1",
[2020-05-06T08:56:12.759Z]         "utf8mb4",
[2020-05-06T08:56:12.759Z]         "utf8mb4",
[2020-05-06T08:56:12.759Z]         "utf8",
[2020-05-06T08:56:12.759Z]         "binary",
[2020-05-06T08:56:12.759Z]     )
[2020-05-06T08:56:12.759Z] set_test.go:440:
[2020-05-06T08:56:12.759Z]     c.Assert(sVar, Equals, args[i], Commentf("%d: %s", i, characterSetVariables[i]))
[2020-05-06T08:56:12.759Z] ... obtained string = "utf8mb4"
[2020-05-06T08:56:12.759Z] ... expected string = "latin1"
[2020-05-06T08:56:12.759Z] ... 1: character_set_connection

@wjhuang2016

Fixed.

@codecov
Copy link

codecov bot commented May 15, 2020 •
edited
Loading

Codecov Report

Merging #16984 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #16984   +/-   ##
===========================================
  Coverage   80.0310%   80.0310%           
===========================================
  Files           517        517           
  Lines        140528     140528           
===========================================
  Hits         112466     112466           
  Misses        19122      19122           
  Partials       8940       8940

@wjhuang2016 wjhuang2016 requested review from tiancaiamao and bb7133 May 15, 2020 08:57
tangenta
tangenta reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

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

LGTM

@tangenta tangenta added the status/LGT1 Indicates that a PR has LGTM 1. label May 18, 2020
AilinKid
AilinKid reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

rest LGTM

executor/set.go Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
executor/set.go Show resolved Hide resolved
bb7133
bb7133 previously approved these changes May 18, 2020
View reviewed changes
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

AilinKid
AilinKid reviewed May 19, 2020
View reviewed changes
Copy link
Contributor

@AilinKid AilinKid 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 added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 19, 2020
AilinKid
AilinKid previously approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

resolve the conflicts before merge it

@wjhuang2016
Copy link
Member Author

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 19, 2020
@sre-bot
Copy link
Contributor

sre-bot commented May 19, 2020

/run-all-tests

@sre-bot sre-bot merged commit 838de2a into pingcap:master May 19, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request May 19, 2020
@wjhuang2016 @sre-bot
cherry pick pingcap#16984 to release-4.0
9947db8 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request May 19, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented May 19, 2020

cherry pick to release-4.0 in PR #17289

@wjhuang2016 wjhuang2016 mentioned this pull request Jul 8, 2020
Merged
bb7133 pushed a commit that referenced this pull request Jul 31, 2020
@sre-bot
executor: fix wrong behavior of set charset statement (#16984) (#17289)
7b0b0c0
@wjhuang2016 wjhuang2016 deleted the fix_charset branch November 17, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tangenta tangenta tangenta left review comments

@bb7133 bb7133 bb7133 approved these changes

@AilinKid AilinKid AilinKid left review comments

@SunRunAway SunRunAway Awaiting requested review from SunRunAway

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

Assignees
No one assigned
Labels
sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@wjhuang2016 @tiancaiamao @sre-bot @bb7133 @AilinKid @tangenta