expression: avoid padding 0 when implicitly cast to binary

[Willendless]

What problem does this PR solve?

Issue Number: close #34823

Problem Summary:

For between and expression, implicitly casting string charset to binary string will pad 0 to the origin string, which causes different string comparison results from mysql. We can instead use mysql.TypeVarStringto avoid this problem. In order to match the behavior of mysql, we can instead avoid doing implicit casting here. See String::needs_conversion() in mysql-server for details.

Modification rationale:

For one thing, we still need to build cast function to retain binary collation information till execution phase. For another, we can not keep using fixed length representation otherwise 0 padding will affect later string comparison result. Therefore, we need to use VarString type of cast here.

drop table if exists t1;drop table if exists t2;
create table t1(a char(20));
create table t2(b binary(20), c binary(20));
insert into t1 value('-1');
insert into t2 value(0x2D31, 0x67);
insert into t2 value(0x2D31, 0x73);
select * from t1, t2 where t1.a between t2.b and t2.c;

// previously

mysql> select * from t1, t2 where t1.a between t2.b and t2.c;
+------+--------------------------------------------+--------------------------------------------+
| a    | b                                          | c                                          |
+------+--------------------------------------------+--------------------------------------------+
| -1   | 0x2D31000000000000000000000000000000000000 | 0x7300000000000000000000000000000000000000 |
| -1   | 0x2D31000000000000000000000000000000000000 | 0x6700000000000000000000000000000000000000 |
+------+--------------------------------------------+--------------------------------------------+
2 rows in set (0.06 sec)

// now

mysql> select * from t1, t2 where t1.a between t2.b and t2.c;
Empty set (0.01 sec)

What is changed and how it works?

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

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

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

None

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• wjhuang2016
• zanmato1984

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.

[sre-bot]

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/08278cd94532850feb188317977d90f7a19f84ee

[zanmato1984]

LGTM

[wjhuang2016]

/run-mysql-test

[xiongjiwei]

we can not just return if the target charset is binary.

because we had support charset gbk, if charset of t1.a is gbk it will have a to_binary function, after we return directly, it will be missed I think.

mysql> desc select * from t1, t2 where t1.a between t2.b and t2.c;
+-----------------------------+---------+-----------+---------------+-----------------------------------------------------------------------------------------------------------------------------------------------+
| id                          | estRows | task      | access object | operator info                                                                                                                                 |
+-----------------------------+---------+-----------+---------------+-----------------------------------------------------------------------------------------------------------------------------------------------+
| HashJoin_8                  | 4.00    | root      |               | CARTESIAN inner join, other cond:ge(cast(to_binary(test.t1.a), binary(20)), test.t2.b), le(cast(to_binary(test.t1.a), binary(20)), test.t2.c) |
| ├─TableReader_13(Build)     | 2.00    | root      |               | data:TableFullScan_12                                                                                                                         |
| │ └─TableFullScan_12        | 2.00    | cop[tikv] | table:t2      | keep order:false, stats:pseudo                                                                                                                |
| └─TableReader_11(Probe)     | 2.00    | root      |               | data:TableFullScan_10                                                                                                                         |
|   └─TableFullScan_10        | 2.00    | cop[tikv] | table:t1      | keep order:false, stats:pseudo                                                                                                                |
+-----------------------------+---------+-----------+---------------+-----------------------------------------------------------------------------------------------------------------------------------------------+

see why we have to_binary #29736

[wjhuang2016]

create table t4(b binary(20), c char(20)) collate utf8mb4_general_ci;
create table t3(a char(20) ) collate utf8mb4_general_ci;
insert into t3 values ("a");
insert into t4 values (0x0, "A");
select * from t3, t4 where t3.a between t4.b and t4.c;

The result is wrong because t3.a doesn't convert to binary collation.

[Willendless]

For gbk, tidb will build a to_binary is because gbk doesn't belong to legacy charset and when argument is legacy charset (for example the default charset utf8mb4), there is no need to build to_binary function according to the code HandleBinaryLiteral.

My change actually will still add the to_binary function when the code build function for le/ge.

mysql> drop table t1;
Query OK, 0 rows affected (0.01 sec)

mysql> create table t1(a char(20)) default charset gbk;
Query OK, 0 rows affected (0.00 sec)

mysql> desc select * from t1, t2 where t1.a between t2.b and t2.c;
+-------------------------------+----------+-----------+---------------+-----------------------------------------------------------------------------------------------------------+
| id                            | estRows  | task      | access object | operator info                                                                                             |
+-------------------------------+----------+-----------+---------------+-----------------------------------------------------------------------------------------------------------+
| Projection_9                  | 20000.00 | root      |               | test.t1.a, test.t2.b, test.t2.c                                                                           |
| └─HashJoin_11                 | 20000.00 | root      |               | CARTESIAN inner join, other cond:ge(to_binary(test.t1.a), test.t2.b), le(to_binary(test.t1.a), test.t2.c) |
|   ├─TableReader_13(Build)     | 2.00     | root      |               | data:TableFullScan_12                                                                                     |
|   │ └─TableFullScan_12        | 2.00     | cop[tikv] | table:t2      | keep order:false, stats:pseudo                                                                            |
|   └─TableReader_15(Probe)     | 10000.00 | root      |               | data:TableFullScan_14                                                                                     |
|     └─TableFullScan_14        | 10000.00 | cop[tikv] | table:t1      | keep order:false, stats:pseudo                                                                            |
+-------------------------------+----------+-----------+---------------+-----------------------------------------------------------------------------------------------------------+
6 rows in set (0.01 sec)

[Willendless]

As for

create table t4(b binary(20), c char(20)) collate utf8mb4_general_ci;
create table t3(a char(20) ) collate utf8mb4_general_ci;
insert into t3 values ("a");
insert into t4 values (0x0, "A");
select * from t3, t4 where t3.a between t4.b and t4.c;

It is the expected behavior that t3.a doesn't convert to binary collation since its charset (utf8mb4) is legacy...

[wjhuang2016]

As for

create table t4(b binary(20), c char(20)) collate utf8mb4_general_ci;
create table t3(a char(20) ) collate utf8mb4_general_ci;
insert into t3 values ("a");
insert into t4 values (0x0, "A");
select * from t3, t4 where t3.a between t4.b and t4.c;

It is the expected behavior that t3.a doesn't convert to binary collation since its charset (utf8mb4) is legacy...

The point is that we need to make t3.a <= t4.c use the binary collation.

[Willendless]

Not sure the behavior of to_binary. If it will pad 0 for other charsets then forcing t3.a <= t4.c to use the binary collation might still cause mismatch between tidb and mysql. And mysql actually doesn't do conversion when target charset is binary.

Issue #34823 shows that the return result of mysql is not the same as using cast(test.t1.a, binary(20)) since it will pad 0 at the end of a and affect the comparison result.

create table t4(b binary(20), c char(20)) collate utf8mb4_general_ci;
create table t3(a char(20) ) collate utf8mb4_general_ci;
insert into t3 values ("a");
insert into t4 values (0x0, "A");
select * from t3, t4 where t3.a between t4.b and t4.c;

This one is indeed a problem..

There are only following cases:

a	b	c	result
binary	any	any	cast in BuildCastCollationFunction is not necessary
not binary	binary	not binary	should use binary collation but should not padding 0
not binary	not binary	binary	same as above
not binary	binary	binary	not necessary
not binary	not binary	not binary	not involved in this pr

[Willendless]

PTAL @xiongjiwei @wjhuang2016

[zanmato1984]

/run-mysql-test

[zanmato1984]

/run-mysql-test

[zanmato1984]

/run-mysql-test

[zanmato1984]

/run-mysql-test

[zanmato1984]

/run-mysql-test

[zanmato1984]

/run-mysql-test

[zanmato1984]

/run-mysql-test

[ti-srebot]

cherry pick to release-5.4 in PR #35557

[ti-srebot]

cherry pick to release-6.0 in PR #35558

[ti-srebot]

cherry pick to release-6.1 in PR #35559

[sre-bot]

TiDB MergeCI notify

🔴 Bad News! [2] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 1, success 10, total 11	23 min	Existing failure
idc-jenkins-ci-tidb/common-test	🔴 failed 2, success 10, total 12	13 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 34 tests passed	24 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	7 min 15 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	6 min 24 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	5 min 54 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	5 min 24 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 14 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 3 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed