-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: rewrite builtin function: INSERT #4414
Conversation
expression/builtin_string.go
Outdated
bf.tp.Flen = mysql.MaxBlobWidth | ||
SetBinFlagOrBinStr(args[0].GetType(), bf.tp) | ||
SetBinFlagOrBinStr(args[3].GetType(), bf.tp) | ||
var sig builtinFunc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move the definition of sig and err to line 2619.
expression/builtin_string.go
Outdated
if err != nil { | ||
return d, errors.Trace(err) | ||
strLength := int64(len(str)) | ||
if pos < 1 || pos > strLength { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check can be moved to line 2654?
expression/builtin_string.go
Outdated
runes := []rune(str) | ||
runeLength := int64(len(runes)) | ||
|
||
if pos < 1 || pos > runeLength { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
sig := &builtinInsertFuncSig{newBaseBuiltinFunc(args, ctx)} | ||
bf := newBaseBuiltinFuncWithTp(args, ctx, tpString, tpString, tpInt, tpInt, tpString) | ||
bf.tp.Flen = mysql.MaxBlobWidth | ||
SetBinFlagOrBinStr(args[0].GetType(), bf.tp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bug fix: for INSERT(str,pos,len,newstr)
, if str
is a binary string, we should treat it like a byte array, otherwise we should treat it like a code point array. see builtinInsertBinarySig
and builtinInsertSig
for more detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use args[0].GetType() after newBaseBuiltinFunc,
args[0] has already been wrapped with cast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when arg[0]
is a binary string, it will not be wrapped with a cast.
btw, I think wrapped cast functions should guarantee the correct type inference, and every function should do its type inference based on its children, no matter what expression the children is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@jackysp @breeswish PTAL |
if length > runeLength-pos+1 || length < 0 { | ||
return string(runes[0:pos-1]) + newstr, false, nil | ||
} | ||
return string(runes[0:pos-1]) + newstr + string(runes[pos+length-1:]), false, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use fmt.Sprintf
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package main
import "fmt"
import "time"
func main() {
str := ""
str1 := "aaaa"
str2 := "bbbb"
str3 := "cccc"
start := time.Now()
for i, round := 0, 1000000; i < round; i++ {
str = str1 + str2 + str3
}
fmt.Printf("%vns\n", time.Now().Sub(start).Nanoseconds())
fmt.Printf("%s\n", str)
start = time.Now()
for i, round := 0, 1000000; i < round; i++ {
str = fmt.Sprintf("%s%s%s", str1, str2, str3)
}
fmt.Printf("%vns\n", time.Now().Sub(start).Nanoseconds())
fmt.Printf("%s\n", str)
}
$go build a.go
$./a
56394494ns
aaaabbbbcccc
279845006ns
aaaabbbbcccc
It seems fmt.Sprintf()
has less efficiency.
…n/expression/string/insert
@jackysp PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
to #4080