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: add builtin function WEIGHT_STRING() #14792

Merged
merged 11 commits into from
Feb 27, 2020

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Feb 14, 2020

What problem does this PR solve?

This PR tries to close #3580

Related parser PR: pingcap/parser#743

What is changed and how it works?

Add builtin-function WEIGHT_STRING(). MySQL manual described most of its behavior except:

  • If the input str is of numeric types, WEIGHT_STRING() in MySQL returns NULL, so does this implementation.
  • If the input str is of other types rather than string or numeric, the result of WEIGHT_STRING() in MySQL is not cleared. I simply evaluated the input as string.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

  • NA

@bb7133 bb7133 added the type/enhancement The issue or PR belongs to an enhancement. label Feb 14, 2020
@bb7133 bb7133 added this to the v4.0.0-beta.1 milestone Feb 14, 2020
@bb7133 bb7133 requested a review from a team as a code owner February 14, 2020 10:36
@ghost ghost requested review from SunRunAway and XuHuaiyu and removed request for a team February 14, 2020 10:36
@bb7133
Copy link
Member Author

bb7133 commented Feb 14, 2020

PTAL @wjhuang2016 @qw4990 @kennytm

}

// TODO: refactor padding codes after padding is supported by collators.
switch b.padding {
Copy link
Member

Choose a reason for hiding this comment

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

Please update it since the padding PR is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@bb7133 bb7133 force-pushed the bb7133/add_weight_string branch from 760b9b5 to 455e361 Compare February 17, 2020 04:12
@bb7133 bb7133 force-pushed the bb7133/add_weight_string branch from 455e361 to c65845b Compare February 17, 2020 15:09
@qw4990 qw4990 self-requested a review February 18, 2020 10:47
@bb7133 bb7133 force-pushed the bb7133/add_weight_string branch 4 times, most recently from 01768e5 to c96e628 Compare February 19, 2020 01:44
@bb7133
Copy link
Member Author

bb7133 commented Feb 19, 2020

PTAL @qw4990 @kennytm @wjhuang2016

expression/builtin_string.go Outdated Show resolved Hide resolved
c.Assert(test.expect, IsNil)
continue
}
res, err := result.ToString()
Copy link
Member

Choose a reason for hiding this comment

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

ToString may be wrong if the collation is general_ci

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine, the string in Go does not have to be a valid utf8 sequence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the tests for utf8mb4_general_ci.

@bb7133 bb7133 force-pushed the bb7133/add_weight_string branch 3 times, most recently from 3070c68 to 93eb676 Compare February 22, 2020 02:57
@bb7133
Copy link
Member Author

bb7133 commented Feb 22, 2020

Addressed, PTAL @wjhuang2016 @qw4990 @kennytm

@bb7133 bb7133 requested a review from kennytm February 24, 2020 02:50
@Deardrops Deardrops self-requested a review February 24, 2020 02:50
expression/builtin_string.go Outdated Show resolved Hide resolved
Comment on lines 2320 to 2344
{nil, "NONE", 0, nil},
{7, "NONE", 0, nil},
{"a", "NONE", 0, "a"},
{"a ", "NONE", 0, "a "},
{"中", "NONE", 0, "中"},
{"中 ", "NONE", 0, "中 "},
{nil, "CHAR", 5, nil},
{7, "CHAR", 5, nil},
{"a", "CHAR", 5, "a "},
{"a ", "CHAR", 5, "a "},
{"中", "CHAR", 5, "中 "},
{"中 ", "CHAR", 5, "中 "},
{nil, "BINARY", 5, nil},
{7, "BINARY", 2, "7\x00"},
{"a", "BINARY", 1, "a"},
{"ab", "BINARY", 1, "a"},
{"a", "BINARY", 5, "a\x00\x00\x00\x00"},
{"a ", "BINARY", 5, "a \x00\x00\x00"},
{"中", "BINARY", 1, "\xe4"},
{"中", "BINARY", 2, "\xe4\xb8"},
{"中", "BINARY", 3, "中"},
{"中", "BINARY", 5, "中\x00\x00"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Test the result when expr is a datetime or some types else.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's kind of complicated to add them here, I've added the integration test and put those cases there, PTAL.

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.

Please add some integration tests.

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

Copy link
Contributor

@djshow832 djshow832 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 26, 2020
@kennytm kennytm added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 26, 2020
@Deardrops Deardrops removed their request for review February 26, 2020 06:17
AilinKid
AilinKid previously approved these changes Feb 26, 2020
Copy link
Contributor

@AilinKid AilinKid 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
Copy link
Member Author

bb7133 commented Feb 27, 2020

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 27, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Feb 27, 2020

@bb7133 merge failed.

@bb7133
Copy link
Member Author

bb7133 commented Feb 27, 2020

/run-all-tests

@wjhuang2016 wjhuang2016 merged commit 568cc22 into pingcap:master Feb 27, 2020
@bb7133 bb7133 deleted the bb7133/add_weight_string branch December 29, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtin function WEIGHT_STRING() not support
7 participants