-
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, executor, plan: rewrite builtin function substring/substr. #3711
expression, executor, plan: rewrite builtin function substring/substr. #3711
Conversation
expression/builtin_string.go
Outdated
if mysql.HasBinaryFlag(argType.Flag) { | ||
types.SetBinChsClnFlag(bf.tp) | ||
} | ||
sig := &builtinSubstringSig{baseStringBuiltinFunc{bf}} | ||
return sig.setSelf(sig), errors.Trace(c.verifyArgs(args)) | ||
} | ||
|
||
type builtinSubstringSig struct { |
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.
I think It's better to define two signature: substr2Args/ substr3Args
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.
But is there any difference? If so, I need another shared eval function, and let the evalString function in substr2/substr3 call it?
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.
The difference is that we needn't check the length of args everytime we call eval
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.
Understood.
expression/builtin_string.go
Outdated
} | ||
// The forms without a len argument return a substring from string str starting at position pos. | ||
// The forms with a len argument return a substring len characters long from string str, starting at position pos. | ||
// The forms that use FROM are standard SQL syntax. It is also possible to use a negative value for pos. | ||
// In this case, the beginning of the substring is pos characters from the end of the string, rather than the beginning. | ||
// A negative value may be used for pos in any of the forms of this function. | ||
strLen := int64(len(str)) | ||
if pos < 0 { |
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.
@hanfei1991
But do I need to extract the following part into a shared method, or just write them separately in substr2/substr3?
If yes, the method may be like builtinSubstring(str string, pos, length int, hasLen bool)
.
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.
you can write them separately
cd4af0b
to
135f0bc
Compare
expression/builtin_string.go
Outdated
} | ||
} else { | ||
d.SetString(str[pos:]) | ||
if end := pos + length; end < pos { |
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 not clear. It is better to write like:
end := pos + length
if end < pos ...
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.
fixed
135f0bc
to
5656467
Compare
LGTM |
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
No description provided.