collation: cast charset according to the function's resulting charset #29029

xiongjiwei · 2021-10-21T13:28:21Z

What problem does this PR solve?

Issue Number: close #28356

some functions like concat, eq may have different charset among the args. we will infer the charset and collation according to the args, so, if the resulting charset is different from the arg's charset, we need to cast the arg's charset to the resulting charset. e.g.

select concat(a, 0x31) from t;

if a is gbk charset, we should convert 0x31 to gbk charset. If this convert is impossible, for example, 0x81 is not a valid gbk character, we will return an error.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2021-10-21T13:28:23Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

xhebox

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

xiongjiwei · 2021-10-27T02:56:40Z

/run-check_dev_2

xiongjiwei · 2021-10-27T07:11:38Z

expression/collation_test.go

@@ -654,23 +654,11 @@ func TestDeriveCollation(t *testing.T) {
 			false,
 			&ExprCollation{CoercibilitySysconst, UNICODE, charset.CharsetUTF8MB4, charset.CollationUTF8MB4},
 		},
-		{


We assume all the cast are implicit, keep the collation related fields to their original value, the test is meaningless

xiongjiwei · 2021-10-27T07:12:18Z

expression/distsql_builtin.go

@@ -1218,7 +1218,12 @@ func convertUint(val []byte) (*Constant, error) {
 func convertString(val []byte, tp *tipb.FieldType) (*Constant, error) {
 	var d types.Datum
 	d.SetBytesAsString(val, protoToCollation(tp.Collate), uint32(tp.Flen))
-	return &Constant{Value: d, RetType: types.NewFieldType(mysql.TypeVarString)}, nil
+	return &Constant{Value: d, RetType: &types.FieldType{


pb to string expression should use charset information in pb

xiongjiwei · 2021-10-27T07:13:38Z

expression/integration_test.go

@@ -1180,7 +1180,7 @@ func (s *testIntegrationSuite2) TestStringBuiltin(c *C) {

 	// for insert
 	result = tk.MustQuery(`select insert("中文", 1, 1, cast("aaa" as binary)), insert("ba", -1, 1, "aaa"), insert("ba", 1, 100, "aaa"), insert("ba", 100, 1, "aaa");`)
-	result.Check(testkit.Rows("aaa文 ba aaa ba"))
+	result.Check(testkit.Rows("aaa\xb8\xad文 ba aaa ba"))


it is compatible with mysql version before 8.0.24.

Why this change happened? Because of implicit cast?

no, before 8.0.24, MySQL uses 1st and 4th arguments to determine the resulting charset, after it, only uses 1st argument. in this case, the resulting charset will be binary for the former and utf8mb4 for the latter, and length of 1 for binary charset is a byte, utf8mb4 is a character.

ti-chi-bot · 2021-11-06T18:22:12Z

@xiongjiwei: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ti-chi-bot · 2021-11-08T02:57:28Z

@Defined2014: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

types/datum.go

tangenta · 2021-11-12T06:40:34Z

expression/collation.go

+			// if value is NULL or binary string, just skip it.
+			if isNull || types.IsBinaryStr(c.GetType()) {
+				continue
+			}


We can move types.IsBinaryStr(c.GetType()) to the beginning of this loop to avoid unnecessary EvalString.

@xiongjiwei Please address this comment.

tangenta · 2021-11-22T03:33:30Z

Close because there is another implementation: #29905

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2021

xiongjiwei mentioned this pull request Oct 21, 2021

expression: support GBK for built-in function concat() #29023

Closed

12 tasks

xiongjiwei force-pushed the binary-lit branch from c02af07 to 8e098c3 Compare October 25, 2021 03:33

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 25, 2021

xiongjiwei force-pushed the binary-lit branch 10 times, most recently from 2eaa454 to 37ebe20 Compare October 27, 2021 02:45

xiongjiwei force-pushed the binary-lit branch 2 times, most recently from 6a38dfe to b0d8916 Compare October 27, 2021 06:53

xiongjiwei commented Oct 27, 2021

View reviewed changes

xiongjiwei marked this pull request as ready for review October 27, 2021 07:13

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2021

xiongjiwei changed the title ~~collation: convert binary string~~ collation: cast charset according to the function's resulting charset Oct 27, 2021

xiongjiwei requested review from tangenta, xhebox and zimulala October 27, 2021 07:28

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 27, 2021

Defined2014 mentioned this pull request Oct 29, 2021

Report Illegal mix of collations error when character_set_server/database is gbk #29239

Closed

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021

xiongjiwei added 5 commits November 4, 2021 17:51

convert binary to non-binary

46269e0

add test

f10b6d6

add comments

9152dd6

change test to readable character

f3e9e04

resolve conflict

370da72

xiongjiwei force-pushed the binary-lit branch from 8cb0348 to 370da72 Compare November 4, 2021 10:01

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 4, 2021

xhebox approved these changes Nov 8, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 8, 2021

Defined2014 approved these changes Nov 8, 2021

View reviewed changes

tangenta reviewed Nov 12, 2021

View reviewed changes

types/datum.go Show resolved Hide resolved

tangenta reviewed Nov 12, 2021

View reviewed changes

AilinKid mentioned this pull request Nov 15, 2021

ERROR 1105 (HY000): runtime error: index out of range [10] with length 10 #29608

Closed

xiongjiwei closed this Nov 22, 2021

xiongjiwei deleted the binary-lit branch September 23, 2022 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

collation: cast charset according to the function's resulting charset #29029

collation: cast charset according to the function's resulting charset #29029

xiongjiwei commented Oct 21, 2021 •

edited

Loading

ti-chi-bot commented Oct 21, 2021 •

edited

Loading

xiongjiwei commented Oct 27, 2021

xiongjiwei Oct 27, 2021

xiongjiwei Oct 27, 2021

xiongjiwei Oct 27, 2021

Defined2014 Oct 28, 2021 •

edited

Loading

xiongjiwei Oct 29, 2021

ti-chi-bot commented Nov 6, 2021

ti-chi-bot commented Nov 8, 2021

tangenta Nov 12, 2021

zimulala Nov 17, 2021

tangenta commented Nov 22, 2021

collation: cast charset according to the function's resulting charset #29029

collation: cast charset according to the function's resulting charset #29029

Conversation

xiongjiwei commented Oct 21, 2021 • edited Loading

What problem does this PR solve?

Check List

Release note

ti-chi-bot commented Oct 21, 2021 • edited Loading

xiongjiwei commented Oct 27, 2021

xiongjiwei Oct 27, 2021

Choose a reason for hiding this comment

xiongjiwei Oct 27, 2021

Choose a reason for hiding this comment

xiongjiwei Oct 27, 2021

Choose a reason for hiding this comment

Defined2014 Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

xiongjiwei Oct 29, 2021

Choose a reason for hiding this comment

ti-chi-bot commented Nov 6, 2021

ti-chi-bot commented Nov 8, 2021

tangenta Nov 12, 2021

Choose a reason for hiding this comment

zimulala Nov 17, 2021

Choose a reason for hiding this comment

tangenta commented Nov 22, 2021

xiongjiwei commented Oct 21, 2021 •

edited

Loading

ti-chi-bot commented Oct 21, 2021 •

edited

Loading

Defined2014 Oct 28, 2021 •

edited

Loading