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

expression: implement the Coercibility() function #14739

Merged
merged 11 commits into from
Feb 13, 2020

Conversation

qw4990
Copy link
Contributor

@qw4990 qw4990 commented Feb 12, 2020

What problem does this PR solve?

Solve this issue: #14687.
Implement the Coercibility() function for TiDB.

What is changed and how it works?

  1. Introduce a new interface CollationExpr and implement it for all our Expressions
  2. Add deriveCoercibilityForXXX functions according to this doc.
  3. Add some tests.

Check List

Tests

  • Unit test

@qw4990 qw4990 requested a review from a team as a code owner February 12, 2020 04:02
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team February 12, 2020 04:02
@qw4990 qw4990 requested review from winoros, wjhuang2016, bb7133 and a team and removed request for SunRunAway and XuHuaiyu February 12, 2020 04:02
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team February 12, 2020 04:02
if _, ok := sysConstFuncs[sf.FuncName.L]; ok {
return CoercibilitySysconst
}
if !types.IsString(sf.RetType.Tp) {
Copy link
Member

Choose a reason for hiding this comment

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

What if Bytes, the collation is binary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mysql> create table tt (b binary(10), vb varbinary(10));
mysql> insert into tt values (null, null);
mysql> select coercibility(b), coercibility(vb) from tt;
+-----------------+------------------+
| coercibility(b) | coercibility(vb) |
+-----------------+------------------+
|               2 |                2 |
+-----------------+------------------+

After testing, it seems ok to use this function since the Tp of Bytes(binary) in TiDB is 254(TypeString) now.

@bb7133 bb7133 added this to the v4.0.0-beta.1 milestone Feb 12, 2020
expression/collation.go Outdated Show resolved Hide resolved
expression/collation.go Outdated Show resolved Hide resolved
expression/collation.go Outdated Show resolved Hide resolved
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 13, 2020

/rebuild

Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 13, 2020
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990 qw4990 added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 13, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 13, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 13, 2020

@qw4990 merge failed.

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 13, 2020

/run-integration-common-test

1 similar comment
@qw4990
Copy link
Contributor Author

qw4990 commented Feb 13, 2020

/run-integration-common-test

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 13, 2020

/run-all-tests

@qw4990
Copy link
Contributor Author

qw4990 commented Feb 13, 2020

/run-all-tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/charset component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants