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

evalEngine: Implement string INSERT #15201

Merged
merged 8 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions go/vt/vtgate/evalengine/cached_size.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions go/vt/vtgate/evalengine/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,15 @@ func (c *compiler) compileNullCheck3(arg1, arg2, arg3 ctype) *jump {
return nil
}

func (c *compiler) compileNullCheck4(arg1, arg2, arg3, arg4 ctype) *jump {
if arg1.nullable() || arg2.nullable() || arg3.nullable() || arg4.nullable() {
j := c.asm.jumpFrom()
c.asm.NullCheck4(j)
return j
}
return nil
}

func (c *compiler) compileNullCheckArg(ct ctype, offset int) *jump {
if ct.nullable() {
j := c.asm.jumpFrom()
Expand Down
33 changes: 33 additions & 0 deletions go/vt/vtgate/evalengine/compiler_asm.go
Original file line number Diff line number Diff line change
Expand Up @@ -2345,6 +2345,28 @@
}, "FN BIT_LENGTH VARCHAR(SP-1)")
}

func (asm *assembler) Fn_INSERT(col collations.TypedCollation) {
asm.adjustStack(-3)

asm.emit(func(env *ExpressionEnv) int {
str := env.vm.stack[env.vm.sp-4].(*evalBytes)
pos := env.vm.stack[env.vm.sp-3].(*evalInt64).i
l := env.vm.stack[env.vm.sp-2].(*evalInt64).i
newstr := env.vm.stack[env.vm.sp-1].(*evalBytes)

res := insert(str, newstr, int(pos), int(l))
if !validMaxLength(int64(len(res)), 1) {
env.vm.stack[env.vm.sp-4] = nil
env.vm.sp -= 3
return 1

Check warning on line 2361 in go/vt/vtgate/evalengine/compiler_asm.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/compiler_asm.go#L2359-L2361

Added lines #L2359 - L2361 were not covered by tests
}

env.vm.stack[env.vm.sp-4] = env.vm.arena.newEvalText(res, col)
env.vm.sp -= 3
return 1
}, "FN INSERT VARCHAR(SP-4) INT64(SP-3) INT64(SP-2) VARCHAR(SP-1)")
}

func (asm *assembler) Fn_LUCASE(upcase bool) {
if upcase {
asm.emit(func(env *ExpressionEnv) int {
Expand Down Expand Up @@ -3147,6 +3169,17 @@
}, "NULLCHECK SP-1, SP-2, SP-3")
}

func (asm *assembler) NullCheck4(j *jump) {
asm.emit(func(env *ExpressionEnv) int {
if env.vm.stack[env.vm.sp-4] == nil || env.vm.stack[env.vm.sp-3] == nil || env.vm.stack[env.vm.sp-2] == nil || env.vm.stack[env.vm.sp-1] == nil {
env.vm.stack[env.vm.sp-4] = nil
env.vm.sp -= 3
return j.offset()
}
return 1

Check warning on line 3179 in go/vt/vtgate/evalengine/compiler_asm.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/compiler_asm.go#L3179

Added line #L3179 was not covered by tests
}, "NULLCHECK SP-1, SP-2, SP-3, SP-4")
}

func (asm *assembler) NullCheckArg(j *jump, offset int) {
asm.emit(func(env *ExpressionEnv) int {
if env.vm.stack[env.vm.sp-1] == nil {
Expand Down
123 changes: 123 additions & 0 deletions go/vt/vtgate/evalengine/fn_string.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import (
"bytes"
"math"

"vitess.io/vitess/go/mysql/collations"
"vitess.io/vitess/go/mysql/collations/charset"
Expand All @@ -29,6 +30,11 @@
)

type (
builtinInsert struct {
CallExpr
collate collations.ID
}

builtinChangeCase struct {
CallExpr
upcase bool
Expand Down Expand Up @@ -106,6 +112,7 @@
}
)

var _ IR = (*builtinInsert)(nil)
var _ IR = (*builtinChangeCase)(nil)
var _ IR = (*builtinCharLength)(nil)
var _ IR = (*builtinLength)(nil)
Expand All @@ -120,6 +127,122 @@
var _ IR = (*builtinPad)(nil)
var _ IR = (*builtinTrim)(nil)

func insert(str, newstr *evalBytes, pos, l int) []byte {
pos--

cs := colldata.Lookup(str.col.Collation).Charset()
strLen := charset.Length(cs, str.bytes)

if pos < 0 || strLen <= pos {
return str.bytes
}
if l < 0 {
l = strLen
}

front := charset.Slice(cs, str.bytes, 0, pos)
var back []byte
if pos <= math.MaxInt-l && pos+l < strLen {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pos <= math.MaxInt-l can be used here to check overflow, as both pos and l are positive (checks added above).

back = charset.Slice(cs, str.bytes, pos+l, strLen)
}

res := make([]byte, len(front)+len(newstr.bytes)+len(back))

copy(res[:len(front)], front)
copy(res[len(front):], newstr.bytes)
copy(res[len(front)+len(newstr.bytes):], back)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vmg Do you think it's worth extending charset.Slice to allow passing in an existing output []byte or is that a bit premature just for this function?

Copy link
Collaborator

@vmg vmg Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make for a more awkward API, don't you think? Right now Slice doesn't allocate (it returns a sub-slice of the input), so if we add an out parameter, we'd have to make it always allocate or have two code paths in the function for when out is nil.

Furthermore, if the Slice API had an out parameter, it would allocate more than once when used for INSERT, since we wouldn't know the required capacity of the out buffer, so it'd have to grow it at least twice.

With the current non-allocating Slice, we just call the function twice and then we get to compose the final result outside of the function with a single allocation, since we know the final size. I quite dig the version of the code as it is written.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, good point. It's already non-allocating so it doesn't really help in any way.


return res
}

func (call *builtinInsert) eval(env *ExpressionEnv) (eval, error) {
args, err := call.args(env)
if err != nil {
return nil, err

Check warning on line 161 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L161

Added line #L161 was not covered by tests
}
if args[0] == nil || args[1] == nil || args[2] == nil || args[3] == nil {
return nil, nil
}
beingnoble03 marked this conversation as resolved.
Show resolved Hide resolved

str, ok := args[0].(*evalBytes)
if !ok {
str, err = evalToVarchar(args[0], call.collate, true)
if err != nil {
return nil, err

Check warning on line 171 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L171

Added line #L171 was not covered by tests
}
}

pos := evalToInt64(args[1]).i
l := evalToInt64(args[2]).i

newstr, err := evalToVarchar(args[3], str.col.Collation, true)
if err != nil {
return nil, err
}

res := insert(str, newstr, int(pos), int(l))
if !validMaxLength(int64(len(res)), 1) {
return nil, nil

Check warning on line 185 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L185

Added line #L185 was not covered by tests
}
return newEvalText(res, str.col), nil
}

func (call *builtinInsert) compile(c *compiler) (ctype, error) {
str, err := call.Arguments[0].compile(c)
if err != nil {
return ctype{}, err

Check warning on line 193 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L193

Added line #L193 was not covered by tests
}

pos, err := call.Arguments[1].compile(c)
if err != nil {
return ctype{}, err

Check warning on line 198 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L198

Added line #L198 was not covered by tests
}

l, err := call.Arguments[2].compile(c)
if err != nil {
return ctype{}, err

Check warning on line 203 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L203

Added line #L203 was not covered by tests
}

newstr, err := call.Arguments[3].compile(c)
if err != nil {
return ctype{}, err

Check warning on line 208 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L208

Added line #L208 was not covered by tests
}

skip := c.compileNullCheck4(str, pos, l, newstr)

_ = c.compileToInt64(pos, 3)
_ = c.compileToInt64(l, 2)

if err != nil {
return ctype{}, nil

Check warning on line 217 in go/vt/vtgate/evalengine/fn_string.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/fn_string.go#L217

Added line #L217 was not covered by tests
}

col := str.Col

switch {
case str.isTextual():
default:
c.asm.Convert_xce(4, sqltypes.VarChar, c.collation)
col = typedCoercionCollation(sqltypes.VarChar, c.collation)
}

switch {
case newstr.isTextual():
fromCharset := colldata.Lookup(newstr.Col.Collation).Charset()
toCharset := colldata.Lookup(col.Collation).Charset()
if fromCharset != toCharset && !toCharset.IsSuperset(fromCharset) {
c.asm.Convert_xce(1, sqltypes.VarChar, col.Collation)
}
default:
c.asm.Convert_xce(1, sqltypes.VarChar, col.Collation)
}

c.asm.Fn_INSERT(col)
c.asm.jumpDestination(skip)

return ctype{Type: sqltypes.VarChar, Col: col, Flag: flagNullable}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there cases where this function can return NULL if all inputs are not NULL?

Copy link
Member Author

@beingnoble03 beingnoble03 Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if the resultant string size is greater than max_allowed_packet, I think I forgot to add the validation for that. Pushed now. 😅

}

func (call *builtinChangeCase) eval(env *ExpressionEnv) (eval, error) {
arg, err := call.arg1(env)
if err != nil {
Expand Down
23 changes: 23 additions & 0 deletions go/vt/vtgate/evalengine/testcases/cases.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ var Cases = []TestCase{
{Run: TupleComparisons},
{Run: Comparisons},
{Run: InStatement},
{Run: FnInsert},
{Run: FnLower},
{Run: FnUpper},
{Run: FnCharLength},
Expand Down Expand Up @@ -1314,6 +1315,28 @@ var JSONExtract_Schema = []*querypb.Field{
},
}

func FnInsert(yield Query) {
for _, s := range insertStrings {
for _, ns := range insertStrings {
for _, l := range inputBitwise {
for _, p := range inputBitwise {
yield(fmt.Sprintf("INSERT(%s, %s, %s, %s)", s, p, l, ns), nil)
}
}
}
}

mysqlDocSamples := []string{
"INSERT('Quadratic', 3, 4, 'What')",
"INSERT('Quadratic', -1, 4, 'What')",
"INSERT('Quadratic', 3, 100, 'What')",
}

for _, q := range mysqlDocSamples {
yield(q, nil)
}
}

func FnLower(yield Query) {
for _, str := range inputStrings {
yield(fmt.Sprintf("LOWER(%s)", str), nil)
Expand Down
34 changes: 34 additions & 0 deletions go/vt/vtgate/evalengine/testcases/inputs.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,40 @@ var inputStrings = []string{
// "_ucs2 'AabcÅå'",
}

var insertStrings = []string{
"NULL",
"\"\"",
"\"a\"",
"\"abc\"",
"1",
"-1",
"0123",
"0xAACC",
"3.1415926",
// MySQL has broken behavior for these inputs,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can reference mysql/mysql-server#517 here so we know what this is about in the future too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

// "\"Å å\"",
// "\"中文测试\"",
// "\"日本語テスト\"",
// "\"한국어 시험\"",
// "\"😊😂🤢\"",
// "_utf8mb4 'abcABCÅå'",
"DATE '2022-10-11'",
"TIME '11:02:23'",
"'123'",
"9223372036854775807",
"-9223372036854775808",
"999999999999999999999999",
"-999999999999999999999999",
"_binary 'Müller' ",
"_latin1 0xFF",
// TODO: support other multibyte encodings
// "_dec8 'ÒòÅå'",
// "_utf8mb3 'abcABCÅå'",
// "_utf16 'AabcÅå'",
// "_utf32 'AabcÅå'",
// "_ucs2 'AabcÅå'",
}

var inputConversionTypes = []string{
"BINARY", "BINARY(1)", "BINARY(0)", "BINARY(16)", "BINARY(-1)",
"CHAR", "CHAR(1)", "CHAR(0)", "CHAR(16)", "CHAR(-1)",
Expand Down
29 changes: 29 additions & 0 deletions go/vt/vtgate/evalengine/translate_builtin.go
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,35 @@
return &builtinRegexpReplace{
CallExpr: CallExpr{Arguments: args, Method: "REGEXP_REPLACE"},
}, nil

case *sqlparser.InsertExpr:
str, err := ast.translateExpr(call.Str)
if err != nil {
return nil, err

Check warning on line 991 in go/vt/vtgate/evalengine/translate_builtin.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/translate_builtin.go#L991

Added line #L991 was not covered by tests
}

pos, err := ast.translateExpr(call.Pos)
if err != nil {
return nil, err

Check warning on line 996 in go/vt/vtgate/evalengine/translate_builtin.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/translate_builtin.go#L996

Added line #L996 was not covered by tests
}

len, err := ast.translateExpr(call.Len)
if err != nil {
return nil, err

Check warning on line 1001 in go/vt/vtgate/evalengine/translate_builtin.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/translate_builtin.go#L1001

Added line #L1001 was not covered by tests
}

newstr, err := ast.translateExpr(call.NewStr)
if err != nil {
return nil, err

Check warning on line 1006 in go/vt/vtgate/evalengine/translate_builtin.go

View check run for this annotation

Codecov / codecov/patch

go/vt/vtgate/evalengine/translate_builtin.go#L1006

Added line #L1006 was not covered by tests
}

args := []IR{str, pos, len, newstr}

var cexpr = CallExpr{Arguments: args, Method: "INSERT"}
return &builtinInsert{
CallExpr: cexpr,
collate: ast.cfg.Collation,
}, nil
default:
return nil, translateExprNotSupported(call)
}
Expand Down
18 changes: 9 additions & 9 deletions go/vt/vtgate/planbuilder/testdata/select_cases.json
Original file line number Diff line number Diff line change
Expand Up @@ -3048,15 +3048,15 @@
"QueryType": "SELECT",
"Original": "select insert('Quadratic', 3, 4, 'What')",
"Instructions": {
"OperatorType": "Route",
"Variant": "Reference",
"Keyspace": {
"Name": "main",
"Sharded": false
},
"FieldQuery": "select insert('Quadratic', 3, 4, 'What') from dual where 1 != 1",
"Query": "select insert('Quadratic', 3, 4, 'What') from dual",
"Table": "dual"
"OperatorType": "Projection",
"Expressions": [
"'QuWhattic' as insert('Quadratic', 3, 4, 'What')"
],
"Inputs": [
{
"OperatorType": "SingleRow"
}
]
},
"TablesUsed": [
"main.dual"
Expand Down
Loading