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

Support client specified collation #4409

Merged
merged 6 commits into from Sep 5, 2017

Conversation

Projects
None yet
5 participants
@mxlxm
Contributor

mxlxm commented Sep 4, 2017

  • since client connection charset parameter would issue additional queries "SET NAMES xxx", use collation is better. see:https://github.com/go-sql-driver/mysql#charset
  • accept cli default-character-set arg value as session charset.

mxlxm added some commits Sep 1, 2017

support client collation
- since charset would issue additional queries "SET NAMES xxx", use
collation is better. see:https://github.com/go-sql-driver/mysql#charset
- use cli default-character-set arg value as session charset.
@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 4, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

sre-bot commented Sep 4, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

// charset, utf-8 default
data = append(data, uint8(mysql.DefaultCollationID))
// charset
if cc.collation == 0 {

This comment has been minimized.

@coocood

coocood Sep 4, 2017

Member

The cc.collation is initialized to mysql.DefaultCollationID in newConn already.
So this branch never run.

@coocood

coocood Sep 4, 2017

Member

The cc.collation is initialized to mysql.DefaultCollationID in newConn already.
So this branch never run.

This comment has been minimized.

@mxlxm

mxlxm Sep 4, 2017

Contributor

yep, but TestInitialHandshake in conn_test.go initialized a clientConn without collation field, and then called cc.writeInitialHandshake(), without this branch the case would fail.

@mxlxm

mxlxm Sep 4, 2017

Contributor

yep, but TestInitialHandshake in conn_test.go initialized a clientConn without collation field, and then called cc.writeInitialHandshake(), without this branch the case would fail.

Show outdated Hide outdated session.go
Show outdated Hide outdated server/conn_test.go

@coocood coocood added the contribution label Sep 4, 2017

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 4, 2017

Member

/ok-to-test

Member

iamxy commented Sep 4, 2017

/ok-to-test

@mxlxm

This comment has been minimized.

Show comment
Hide comment
@mxlxm

mxlxm Sep 4, 2017

Contributor

@coocood PTAL

Contributor

mxlxm commented Sep 4, 2017

@coocood PTAL

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 5, 2017

Member

LGTM

Member

coocood commented Sep 5, 2017

LGTM

@coocood coocood added the status/LGT1 label Sep 5, 2017

@zimulala

This comment has been minimized.

Show comment
Hide comment
@zimulala

zimulala Sep 5, 2017

Member

LGTM

Member

zimulala commented Sep 5, 2017

LGTM

@zimulala zimulala added status/LGT2 and removed status/LGT1 labels Sep 5, 2017

@coocood coocood merged commit 778b221 into pingcap:master Sep 5, 2017

4 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@mxlxm mxlxm deleted the mxlxm:support-client-collation branch Sep 5, 2017

mccxj added a commit to mccxj/tidb that referenced this pull request Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment