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

support gbk charset #808

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

support gbk charset #808

wants to merge 10 commits into from

Conversation

spongedu
Copy link
Contributor

@spongedu spongedu commented Apr 13, 2020

  1. Support charset gbk in parser;
  2. fix description for charsetlatin and charsetbin1 and charsetbin

What problem does this PR solve?

We're using parser to develop some our systems and found that charset gbk is not supported yet in parser, which makes some SQL fail to work.
As a MySQL compatible parser, I think we should support gbk as MySQL's parser do, and leave the charset-compatible issues to TiDB layer.

What is changed and how it works?

Add gbk into supported charsets, and add related tests

Check List

Tests

  • Unit test
  • Manual test

@spongedu spongedu requested a review from a team April 13, 2020 03:27
@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #808 into master will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #808      +/-   ##
==========================================
- Coverage   78.35%   78.27%   -0.09%     
==========================================
  Files          40       40              
  Lines       14773    14707      -66     
==========================================
- Hits        11576    11512      -64     
- Misses       2509     2515       +6     
+ Partials      688      680       -8     

Copy link
Contributor

@kennytm kennytm 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.

Why do we support GBK rather than its superset GB18030?

charset/charset.go Outdated Show resolved Hide resolved
@spongedu
Copy link
Contributor Author

spongedu commented Apr 13, 2020

Rest LGTM.

Why do we support GBK rather than its superset GB18030?

@kennytm Because we just met gbk in our cases. maybe gb18030 should be supported as well

@kennytm kennytm added the status/LGT1 LGT1 label Apr 13, 2020
@tangenta tangenta changed the title 1. Support charset gbk in parser; 2. fix description for charsetlatin and charsetbin support gbk charset Apr 14, 2020
@tiancaiamao
Copy link
Collaborator

PTAL @bb7133 @wjhuang2016

charset/charset.go Outdated Show resolved Hide resolved
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

GetSupportedCharsets need to change too.

@spongedu
Copy link
Contributor Author

spongedu commented Jun 4, 2020

@wjhuang2016 PTAL :)

@ti-chi-bot
Copy link
Member

@spongedu: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

Successfully merging this pull request may close these issues.

7 participants