evalEngine: Implement string `INSERT` #15201

beingnoble03 · 2024-02-10T16:21:12Z

Description

This PR adds implementation of INSERT func in evalengine.

Related Issue(s)

Fixes part of #9647

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Noble Mittal <[email protected]>

vitess-bot · 2024-02-10T16:21:14Z

go/vt/vtgate/evalengine/fn_string.go

dbussink · 2024-02-10T19:00:47Z

go/vt/vtgate/evalengine/testcases/cases.go

+		"5",
+		"0",
+		"2",
+	}


I think this list should also include a bunch of non ideal cases. Like NULL (see the earlier comment), decimals, floats etc. I think for example inputBitwise is a good existing list with mostly numerical values.

The way to think about this is "how would I break this" by throwing explicitly bad / unexpected data at the function to see how it behaves then to make sure it's consistent with MySQL (and doesn't panic or error unexpectedly etc).

thanks, I forgot to add overflow checks.

go/vt/vtgate/evalengine/fn_string.go

dbussink · 2024-02-10T19:02:55Z

go/vt/vtgate/evalengine/fn_string.go

+	switch {
+	case str.isTextual():
+	default:
+		c.asm.Convert_xce(4, str.Type, call.collate)


Should this convert to the collation given by call.collate or by the other argument? Or should we merge collations with the appropriate logic? See also #15195 for how collations are handled there.

I don't think we can merge the collations here, as MySQL returns the collations of the str, not newstr if it is textual.
This can be observed by running these 2 queries: SELECT COLLATION(INSERT(_latin1 0xFF, 1, 3, "asd")); and SELECT COLLATION(INSERT("asd", 2, 3, _latin1 0xFF));.

But, I think there was a mistake here, as it should be c.asm.Convert_xce(4, sqltypes.VarChar, c.collation)

We have handled a similar case in LPAD/RPAD.

In the pad functions, we convert to the collation of str, and looking at how this works, we should do the same here?

So should it be c.asm.Convert_xce(4, sqltypes.VarChar, col.collation) instead?

Ah wait, that's already the case, but that's missing checks to not emit that conversion if it's not needed.

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 · 2024-02-11T10:53:25Z

go/vt/vtgate/evalengine/fn_string.go

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


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

dbussink · 2024-02-11T13:27:43Z

go/vt/vtgate/evalengine/fn_string.go

+		col = typedCoercionCollation(sqltypes.VarChar, c.collation)
+	}
+
+	c.asm.Convert_xce(1, sqltypes.VarChar, col.Collation)


I think this conversion should be conditional? This is only needed if it's not already a string?

dbussink · 2024-02-11T13:29:43Z

go/vt/vtgate/evalengine/fn_string.go

+	c.asm.Fn_INSERT(col)
+	c.asm.jumpDestination(skip)
+
+	return ctype{Type: sqltypes.VarChar, Col: col, Flag: flagNullable}, nil


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

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

Signed-off-by: Noble Mittal <[email protected]>

dbussink · 2024-02-11T15:46:59Z

@beingnoble03 So there's a failure in the planbuilder tests, but the new output is correct since now the insert function is supported in the evalengine and it doesn't have to send the query down to a tablet. You can update the expectations (it should tell you how if you run those tests).

Otherwise I can also push up that change if you want.

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 · 2024-02-11T17:08:33Z

@dbussink done.

codecov · 2024-02-11T17:25:34Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (ea62cee) 67.28% compared to head (04490f4) 67.33%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
go/vt/vtgate/evalengine/fn_string.go	88.88%	8 Missing ⚠️
go/vt/vtgate/evalengine/compiler_asm.go	81.81%	4 Missing ⚠️
go/vt/vtgate/evalengine/translate_builtin.go	78.94%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15201      +/-   ##
==========================================
+ Coverage   67.28%   67.33%   +0.05%     
==========================================
  Files        1560     1560              
  Lines      192264   192395     +131     
==========================================
+ Hits       129366   129556     +190     
+ Misses      62898    62839      -59

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbussink · 2024-02-11T18:43:38Z

go/vt/vtgate/evalengine/testcases/inputs.go

+	"0123",
+	"0xAACC",
+	"3.1415926",
+	// MySQL has broken behavior for these inputs,


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

Signed-off-by: Noble Mittal <[email protected]>

dbussink · 2024-02-12T09:49:44Z

go/vt/vtgate/evalengine/fn_string.go

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


@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?

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.

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

vmg

Lookin' great as usual!

vmg · 2024-02-12T11:21:13Z

go/vt/vtgate/evalengine/fn_string.go

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


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.

beingnoble03 added 4 commits February 8, 2024 23:14

evalEngine: Initial implementation of INSERT

f6d2e9d

Signed-off-by: Noble Mittal <[email protected]>

Add collation support and fix insert logic

fb1cae3

Signed-off-by: Noble Mittal <[email protected]>

Merge branch 'main' into eval-insert

39a6729

Signed-off-by: Noble Mittal <[email protected]>

Remove unnecessary comments

372a9b4

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 requested review from harshit-gangal, systay, frouioui and GuptaManan100 as code owners February 10, 2024 16:21

github-actions bot added this to the v20.0.0 milestone Feb 10, 2024

dbussink reviewed Feb 10, 2024

View reviewed changes

Add better testcases for INSERT and handle overflow in INSERT

ebc5eb4

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 commented Feb 11, 2024

View reviewed changes

dbussink reviewed Feb 11, 2024

View reviewed changes

Add max_allowed_packet validation

0a54117

Signed-off-by: Noble Mittal <[email protected]>

Update insert tests in select_cases.json

bde399e

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 requested a review from arthurschreiber as a code owner February 11, 2024 17:07

dbussink added the Component: Evalengine changes to the evaluation engine label Feb 11, 2024

dbussink reviewed Feb 11, 2024

View reviewed changes

dbussink added Type: Enhancement Logical improvement (somewhere between a bug and feature) and removed Type: Enhancement Logical improvement (somewhere between a bug and feature) labels Feb 11, 2024

Add reference to the mysql PR

04490f4

Signed-off-by: Noble Mittal <[email protected]>

dbussink approved these changes Feb 12, 2024

View reviewed changes

vmg approved these changes Feb 12, 2024

View reviewed changes

vmg merged commit 8586b6d into vitessio:main Feb 12, 2024
102 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evalEngine: Implement string `INSERT` #15201

evalEngine: Implement string `INSERT` #15201

beingnoble03 commented Feb 10, 2024 •

edited

Loading

vitess-bot bot commented Feb 10, 2024

dbussink Feb 10, 2024

dbussink Feb 10, 2024

beingnoble03 Feb 11, 2024

dbussink Feb 10, 2024

beingnoble03 Feb 11, 2024

dbussink Feb 11, 2024

dbussink Feb 11, 2024

beingnoble03 Feb 11, 2024

dbussink Feb 11, 2024

beingnoble03 Feb 11, 2024

dbussink Feb 11, 2024

beingnoble03 Feb 11, 2024 •

edited

Loading

dbussink commented Feb 11, 2024

beingnoble03 commented Feb 11, 2024

codecov bot commented Feb 11, 2024 •

edited

Loading

dbussink Feb 11, 2024

beingnoble03 Feb 11, 2024

dbussink Feb 12, 2024

vmg Feb 12, 2024 •

edited

Loading

dbussink Feb 12, 2024

vmg left a comment

vmg Feb 12, 2024 •

edited

Loading

evalEngine: Implement string INSERT #15201

evalEngine: Implement string INSERT #15201

Conversation

beingnoble03 commented Feb 10, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Feb 10, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beingnoble03 Feb 11, 2024 • edited Loading

Choose a reason for hiding this comment

dbussink commented Feb 11, 2024

beingnoble03 commented Feb 11, 2024

codecov bot commented Feb 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmg Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmg left a comment

Choose a reason for hiding this comment

vmg Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

evalEngine: Implement string `INSERT` #15201

evalEngine: Implement string `INSERT` #15201

beingnoble03 commented Feb 10, 2024 •

edited

Loading

beingnoble03 Feb 11, 2024 •

edited

Loading

codecov bot commented Feb 11, 2024 •

edited

Loading

vmg Feb 12, 2024 •

edited

Loading

vmg Feb 12, 2024 •

edited

Loading