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

expression/expressions: replace iconv-go with text/transform #28

Closed
wants to merge 1 commit into from
Closed

Conversation

unknwon
Copy link
Contributor

@unknwon unknwon commented Sep 6, 2015

Remove CGO requirement for compiling iconv-go currently is using.

Replace it with native implementation of Go.

make test has passed, but test cases don't look very detailed on expression, so not sure if everything will go well.

Remove CGO requirement for compiling iconv-go currently is using.

Replace it with native implementation of Go.

Signed-off-by: Unknwon <u@gogs.io>
@unknwon
Copy link
Contributor Author

unknwon commented Sep 6, 2015

Thank god, passed this time.

@xiang90
Copy link

xiang90 commented Sep 6, 2015

LGTM. But I cannot press the merge button. :(

@unknwon
Copy link
Contributor Author

unknwon commented Sep 6, 2015

Godep is killing GitHub diff. So, you can't even find where did I changed.

@xiang90
Copy link

xiang90 commented Sep 6, 2015

@unknwon I usually keep them into separate commits:

  1. change
  2. godep

@unknwon
Copy link
Contributor Author

unknwon commented Sep 6, 2015

godep save doesn't work with tidb out of box, so I changed everything manually 😂 really don't want to go through hell again.

@unknwon
Copy link
Contributor Author

unknwon commented Sep 6, 2015

OK... there is the diff:

diff --git a/expression/expressions/convert.go b/expression/expressions/convert.go
index b636cb5..8fc7918 100644
--- a/expression/expressions/convert.go
+++ b/expression/expressions/convert.go
@@ -17,11 +17,12 @@ import (
        "fmt"
        "strings"

-       iconv "github.com/djimenez/iconv-go"
        "github.com/juju/errors"
        "github.com/ngaut/log"
        "github.com/pingcap/tidb/context"
        "github.com/pingcap/tidb/expression"
+       "golang.org/x/net/html/charset"
+       "golang.org/x/text/transform"
 )

 // FunctionConvert provides a way to convert data between different character sets.
@@ -74,7 +75,13 @@ func (f *FunctionConvert) Eval(ctx context.Context, args map[interface{}]interfa
        } else if strings.ToLower(f.Charset) == "utf8mb4" {
                return value, nil
        }
-       target, err := iconv.ConvertString(str, "utf-8", f.Charset)
+
+       encoding, _ := charset.Lookup(f.Charset)
+       if encoding == nil {
+               return nil, fmt.Errorf("unknow char decoder %s", f.Charset)
+       }
+
+       target, _, err := transform.String(encoding.NewDecoder(), str)
        if err != nil {
                log.Errorf("Convert %s to %s with error: %v", str, f.Charset, err)
                return nil, errors.Trace(err)

@ngaut
Copy link
Member

ngaut commented Sep 7, 2015

Thanks @unknwon

And @xiang90 is right, keep them into separate commits:

  1. change
  2. godep

The correct step is:
cd tidb
godep restore
godep save ./...

Could you fix it ? @unknwon Thanks.

@coocood
Copy link
Member

coocood commented Sep 7, 2015

@unknwon this PR introduced too much dependencies, can you remove the depedency of "golang.org/x/net/html/charset", all we need is a map from charset name to encoding.
How about copy the code in https://github.com/golang/net/blob/master/html/charset/table.go

@unknwon unknwon closed this Sep 7, 2015
@unknwon
Copy link
Contributor Author

unknwon commented Sep 7, 2015

#42

@sre-bot sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019
SunRunAway pushed a commit to SunRunAway/tidb that referenced this pull request Oct 15, 2020
YuJuncen pushed a commit to YuJuncen/tidb that referenced this pull request Apr 23, 2021
YuJuncen pushed a commit to YuJuncen/tidb that referenced this pull request Apr 23, 2021
* add version subcommand

Signed-off-by: Neil Shen <overvenus@gmail.com>

* restore: fix typo

Signed-off-by: Neil Shen <overvenus@gmail.com>

* Init cmd on subcommands

Signed-off-by: Neil Shen <overvenus@gmail.com>

* tests: clean up tikv-importer

Signed-off-by: Neil Shen <overvenus@gmail.com>
xhebox pushed a commit to xhebox/tidb that referenced this pull request Sep 28, 2021
xhebox pushed a commit to xhebox/tidb that referenced this pull request Oct 8, 2021
ti-chi-bot pushed a commit that referenced this pull request Oct 9, 2021
okJiang pushed a commit to okJiang/tidb that referenced this pull request Oct 19, 2021
Connor1996 pushed a commit to Connor1996/tidb that referenced this pull request Dec 14, 2022
* set resource group ID for read request

* replace group id with name
ywqzzy pushed a commit to ywqzzy/tidb that referenced this pull request Jul 26, 2023
guoshouyan pushed a commit to guoshouyan/tidb that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants