support function rpad #2270
support function rpad #2270
Conversation
|
|
|
@silentred Thanks for your PR! Please sign the CLA. |
|
@silentred Please add typeinfer info in typeinferer.go. |
|
weird, I have signed the CLA. Should I close this one and redo a PR? |
|
no, wait for CI pass, and we'll review it. |
| {"hi", 5, "?", "hi???"}, | ||
| {"hi", 1, "?", "h"}, | ||
| {"hi", -1, "?", ""}, | ||
| {"hi", 1, "", ""}, |
There was a problem hiding this comment.
Have you run this test case in MySQL?
The result of rpad("hi", 1, "") is h.
|
@silentred |
| if len(str) >= l { | ||
| d.SetString(str[:l]) | ||
| } else { | ||
| dest := str strings.Repeat(padStr, l-len(str)) |
There was a problem hiding this comment.
I think it's not strings.Repeat(padStr, l-len(str)) , padStr may not just one char, the total length after Rpad is l:
mysql> SELECT RPAD('h',5,'ab');
------------------
| RPAD('h',5,'ab') |
------------------
| habab |
------------------
1 row in set (0.00 sec)
| {"hi", 5, "?", "hi???"}, | ||
| {"hi", 1, "?", "h"}, | ||
| {"hi", -1, "?", ""}, | ||
| {"hi", 1, "", ""}, |
There was a problem hiding this comment.
please add case for padStr not one char
|
@shenli @tiancaiamao @coocood Thanks for the advises. I will fix these. |
| {"hi", 1, "", "h"}, | ||
| {"hi", 5, "", ""}, | ||
| {"hi", 5, "ab", "hiaba"}, | ||
| {"hi", 6, "ab", "hiabab"}, |
There was a problem hiding this comment.
The test can't distinguish null and empty string.
rpad("hi", 0, "?") is empty string, not NULL.
| if err != nil { | ||
| return d, errors.Trace(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
IMHO, the following logic too much complex than it should be, and the loop is hard to read.
Here is a simpler example:
func test(str string, length int, pad string) {
for len(str) < length {
str = str pad
}
return str[:length]
}
just check pad != "" before this logic.
There was a problem hiding this comment.
Thanks, it is simpler indeed. I worried that str = str pad would allocate memory every time, so I chose pre-allocated []byte.
| } | ||
|
|
||
| tailLen := l - len(str) | ||
| padStrBytes := []byte(padStr) |
There was a problem hiding this comment.
How about
repeatCount := tailLen/len(padStr) 1
dest := str strings.Repeat(padStr, repeatCount)
dest = dest[:l]
There was a problem hiding this comment.
Looks better. I will change.
|
@silentred |
|
LGTM |
1 similar comment
|
LGTM |
|
@silentred |
Tried to support the function rpad. It goes alright with
make gotestandmake server. The rpad query works with my build on mac.