parser, ddl: support decoding binary literal in set/enum #35822

tangenta · 2022-06-29T10:50:50Z

What problem does this PR solve?

Issue Number: close #31338

Problem Summary:

This is because the binary literal like 0xD2BB in enum/set can only be decoded to utf8mb4. When the result is written back to the client, an error occurs because 0xD2BB cannot be encoded with gbk. Thus, the result is truncated.

What is changed and how it works?

After this PR, the enum/set values are saved by parser along with a flag, which is used to indicate that whether it is a binary literal:

// FieldType records field type information.
type FieldType struct {
        // ...
	// elems is the element list for enum and set type.
	elems            []string
	elemsIsBinaryLit []bool
}

In the stage of buildTableInfo, we decode column's FieldType.elems according to the column charset to UTF-8 values, and save it. Thus, the enum/set values are always valid UTF-8, nothing is truncated.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the incorrect result of `SHOW CREATE TABLE` when specifying binary literals as enum/set values.

ti-chi-bot · 2022-06-29T10:50:53Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Defined2014
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.

Defined2014

And seems some problems with ut

ddl/ddl_api.go

cmd/explaintest/r/new_character_set_invalid.result

cmd/explaintest/t/new_character_set_invalid.test

parser/types/field_type.go

xhebox · 2022-06-30T03:06:24Z

parser/parser.y

+		for i, e := range elems {
+			trimmed := strings.TrimRight(e.Value, " ")
+			tp.SetElemWithIsBinaryLit(i, trimmed, e.IsBinaryLiteral)
+			fieldLen += len(trimmed)


Why it is not len(tp.GetElem())?

They are the same string.

I knew. My point is that tp.GetElem() makes less assumption about what SetElemWithIsBinaryLit is doing. I won't insist on the change.

~~Actually the fieldLen should not be calculated here because it may change after decoding. Let's remove it.~~

The one who changes SetElemWithIsBinaryLit should check all the usages.

sre-bot · 2022-06-30T04:50:20Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/07cd79dc05f0757f41d58195588f5ef5c455fa7d

tangenta · 2022-06-30T05:39:46Z

/merge

ti-chi-bot · 2022-06-30T05:39:50Z

This pull request has been accepted and is ready to merge.

Commit hash: 934ea38

Defined2014 · 2022-06-30T06:06:07Z

@tangenta Please update mysql-test

xhebox · 2022-06-30T06:38:59Z

/hold

tangenta · 2022-06-30T07:22:27Z

/run-mysql-test tidb-test=pr/1856

tangenta · 2022-06-30T07:34:47Z

/unhold

sre-bot · 2022-06-30T08:15:49Z

TiDB MergeCI notify

🔴 Bad News! New failing [2] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🟥 failed 2, success 9, total 11	33 min	New failing
idc-jenkins-ci-tidb/common-test	🟥 failed 2, success 10, total 12	9 min 34 sec	New failing
idc-jenkins-ci/integration-cdc-test	🟢 all 35 tests passed	31 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	8 min 24 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 25 sec	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	6 min 6 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 26 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 17 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 17 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

Signed-off-by: you06 <you1474600@gmail.com> fix tests Signed-off-by: you06 <you1474600@gmail.com> fix test Signed-off-by: you06 <you1474600@gmail.com> update Signed-off-by: you06 <you1474600@gmail.com> update bazel Signed-off-by: you06 <you1474600@gmail.com> fix test Signed-off-by: you06 <you1474600@gmail.com> fix test Signed-off-by: you06 <you1474600@gmail.com> update Signed-off-by: you06 <you1474600@gmail.com> update client-go Signed-off-by: you06 <you1474600@gmail.com> update client-go Signed-off-by: you06 <you1474600@gmail.com> update grafana Signed-off-by: you06 <you1474600@gmail.com> update bazel Signed-off-by: you06 <you1474600@gmail.com> display by stack Signed-off-by: you06 <you1474600@gmail.com> set request source for ctx of Signed-off-by: you06 <you1474600@gmail.com> address comment Signed-off-by: you06 <you1474600@gmail.com> address comment Signed-off-by: you06 <you1474600@gmail.com> parser, ddl: support decoding binary literal in set/enum (pingcap#35822) close pingcap#31338 executor: fix a nil point when @@tidb_enable_collect_execution_info is off and cop cache is on (pingcap#35839) close pingcap#35829 ddl: support drop columns for multi-schema change (pingcap#35737) ref pingcap#14766 planner: don't push down predicates over table dual (pingcap#35669) close pingcap#35527 *: Remove some useless staleness code (pingcap#35849) close pingcap#35848 ddl: fix potential wrong column definition when setting default value (pingcap#35845) close pingcap#35846 *: add timeout for test cases (pingcap#35844) ddl: remove `onDropColumns` and `onAddColumns` (pingcap#35862) ref pingcap#14766 *: Fix Dockerfile (pingcap#35835) close pingcap#35828 server: refactor TestConnExecutionTimeout (pingcap#35865) close pingcap#35868 fix test Signed-off-by: you06 <you1474600@gmail.com> fix lint Signed-off-by: you06 <you1474600@gmail.com>

parser, ddl: support decoding binary literal in set/enum

4b7e893

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 29, 2022

test: specifying gbk_bin as table's collation

2ef864a

Defined2014 reviewed Jun 29, 2022

View reviewed changes

ddl/ddl_api.go Show resolved Hide resolved

fix integration tests

87fcbbf

Defined2014 reviewed Jun 29, 2022

View reviewed changes

cmd/explaintest/r/new_character_set_invalid.result Show resolved Hide resolved

cmd/explaintest/t/new_character_set_invalid.test Show resolved Hide resolved

xhebox reviewed Jun 30, 2022

View reviewed changes

tangenta added 2 commits June 30, 2022 11:21

explain_test: set sql mode to default before test

d47996a

update jsonFieldType

01ab3b8

Defined2014 approved these changes Jun 30, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 30, 2022

fix explain test

934ea38

xhebox approved these changes Jun 30, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 30, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 30, 2022

ti-chi-bot added 2 commits June 30, 2022 13:39

Merge branch 'master' into gbk-enum-literal

0d60868

Merge branch 'master' into gbk-enum-literal

4aeee46

Merge branch 'master' into gbk-enum-literal

07cd79d

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2022

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 30, 2022

ti-chi-bot merged commit c578d19 into pingcap:master Jun 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser, ddl: support decoding binary literal in set/enum #35822

parser, ddl: support decoding binary literal in set/enum #35822

tangenta commented Jun 29, 2022

ti-chi-bot commented Jun 29, 2022 •

edited

Loading

Defined2014 left a comment

xhebox Jun 30, 2022

tangenta Jun 30, 2022

xhebox Jun 30, 2022

tangenta Jun 30, 2022 •

edited

Loading

sre-bot commented Jun 30, 2022 •

edited

Loading

tangenta commented Jun 30, 2022

ti-chi-bot commented Jun 30, 2022

Defined2014 commented Jun 30, 2022

xhebox commented Jun 30, 2022

tangenta commented Jun 30, 2022

tangenta commented Jun 30, 2022

sre-bot commented Jun 30, 2022

parser, ddl: support decoding binary literal in set/enum #35822

parser, ddl: support decoding binary literal in set/enum #35822

Conversation

tangenta commented Jun 29, 2022

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jun 29, 2022 • edited Loading

Defined2014 left a comment

Choose a reason for hiding this comment

xhebox Jun 30, 2022

Choose a reason for hiding this comment

tangenta Jun 30, 2022

Choose a reason for hiding this comment

xhebox Jun 30, 2022

Choose a reason for hiding this comment

tangenta Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

sre-bot commented Jun 30, 2022 • edited Loading

tangenta commented Jun 30, 2022

ti-chi-bot commented Jun 30, 2022

Defined2014 commented Jun 30, 2022

xhebox commented Jun 30, 2022

tangenta commented Jun 30, 2022

tangenta commented Jun 30, 2022

sre-bot commented Jun 30, 2022

TiDB MergeCI notify

ti-chi-bot commented Jun 29, 2022 •

edited

Loading

tangenta Jun 30, 2022 •

edited

Loading

sre-bot commented Jun 30, 2022 •

edited

Loading