builtin: add format built-in function #2883
Conversation
expression/builtin_string.go
Outdated
| return d, errFunctionNotExists.GenByArgs("format") | ||
| args, err := b.evalArgs(row) | ||
| if err != nil { | ||
| return types.Datum{}, errors.Trace(err) |
expression/builtin_string.go
Outdated
| var arg2 string | ||
|
|
||
| // TODO: Neet to be moved away. | ||
| supportedFormat := map[string]formatFunc{ |
There was a problem hiding this comment.
You can define some consts in mysql/const.go.
expression/builtin_string.go
Outdated
| } | ||
|
|
||
| d.SetString(supportedFormat[arg2](number, arg1)) | ||
| return d, errors.Trace(err) |
expression/builtin_string.go
Outdated
| } | ||
|
|
||
| // TODO: Move those functions away | ||
| func _formatENUS(number float64, precision int64) string { |
There was a problem hiding this comment.
The function name is not in golang style. Rename it to formatENUS.
expression/builtin_string.go
Outdated
| } | ||
|
|
||
| func _formatZHCN(number float64, precision int64) string { | ||
| // TODO |
expression/builtin_string.go
Outdated
| // TODO: Neet to be moved away. | ||
| supportedFormat := map[string]formatFunc{ | ||
| "en-US": _formatENUS, | ||
| "zh-CN": _formatZHCN, |
There was a problem hiding this comment.
_ is seldom used in golang for naming
expression/builtin_string.go
Outdated
| if !exist { | ||
| arg2 = "en-US" | ||
| } | ||
| number, err := arg0.ToFloat64() |
There was a problem hiding this comment.
Will this operation loss precision? SELECT FORMAT(12332.1234567890123456789012345678901, 22);
PTAL @coocood
expression/builtin_string.go
Outdated
| number = 0 - number | ||
| } | ||
| comma := []byte{','} | ||
| parts := strings.Split(strconv.FormatFloat(number, 'f', -1, 64), ".") |
There was a problem hiding this comment.
check len(parts) == 2 ?
if you strconv.format(1), maybe no decimal part there, I guess...
There was a problem hiding this comment.
ok, I see line 1652...
expression/builtin_string_test.go
Outdated
| {-12332.123456, 4, "-12,332.1234"}, | ||
| {-12332.123456, 0, "-12,332"}, | ||
| {-12332.123456, -4, "-12,332"}, | ||
| {"-12332.123456", "4", "-12,332.1234"}, |
There was a problem hiding this comment.
add case that decimal part is very long
add case no decimal part
There was a problem hiding this comment.
and test the invalid input such as NULL or string and other type, check mysql behavior to confirm
|
@tiancaiamao @shenli @coocood please take a look again. |
expression/builtin_string_test.go
Outdated
| {-12332.123456, 0, "-12,332"}, | ||
| {-12332.123456, -4, "-12,332"}, | ||
| {"-12332.123456", "4", "-12,332.1234"}, | ||
| {"12332.1234567890123456789012345678901", 22, "12,332.1234567890123456789012"}, |
There was a problem hiding this comment.
Hi, @ariesdevil
I mean you should also handle this case in your branch:
mysql> select format("abcd", 32);
ERROR 2013 (HY000): Lost connection to MySQL server during query
|
@tiancaiamao Add more tests and please take a look again. |
expression/builtin_string_test.go
Outdated
| {[]interface{}{"Quadratic", 3, -1, "What"}, "QuWhat"}, | ||
| {[]interface{}{"Quadratic", 3, 1, "What"}, "QuWhatdratic"}, | ||
|
|
||
| {[]interface{}{"我叫小雨呀", 3, 2, "王雨叶"}, "我叫王雨叶呀"}, |
There was a problem hiding this comment.
Who is "王雨叶" ? Foreigners will feel 干(yi)的(lian)漂(meng)亮(bi) when they see this.
TiDB is a global project, it should not use those meaningless string.
There was a problem hiding this comment.
@tiancaiamao It's not my code. That's TestInsert, not TestFormat.
There was a problem hiding this comment.
@tiancaiamao The Insert function was implemented by @hiwjd , PR: #2863
No description provided.