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.

Sign up for GitHub

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

Jump to bottom

expression: fix wrong result when select with collation #18665

Merged
merged 8 commits into from
Jul 23, 2020
Merged

expression: fix wrong result when select with collation #18665

merged 8 commits into from
Jul 23, 2020

Conversation

xiongjiwei
Copy link
Contributor

@xiongjiwei xiongjiwei commented Jul 17, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #18662

What changed

add newBaseBuiltinFuncWithFieldType, this function create a new basebuiltinFunc without check.

How it works

remove unnecessary mix collation check, use collation decode from pb directly, do not re-calculate it. pb does not contains coercibility, re-calculate collation will get wrong result.

Tests

  • Unit test
  • Integration test

Release note

  • fix incorrect collator when getSignatureByPB and remove unnecessary recover

@ti-srebot ti-srebot added the contribution This PR is from a community contributor. label Jul 17, 2020
@xiongjiwei xiongjiwei marked this pull request as ready for review July 17, 2020 06:51
@xiongjiwei xiongjiwei requested a review from a team as a code owner July 17, 2020 06:51
@xiongjiwei xiongjiwei requested review from qw4990 and removed request for a team July 17, 2020 06:51
@codecov
Copy link

codecov bot commented Jul 17, 2020 •
edited
Loading

Codecov Report

Merging #18665 into master will increase coverage by 0.0780%.
The diff coverage is 75.0000%.

@@               Coverage Diff                @@
##             master     #18665        +/-   ##
================================================
+ Coverage   79.4037%   79.4818%   +0.0780%     
================================================
  Files           542        540         -2     
  Lines        147080     145622      -1458     
================================================
- Hits         116787     115743      -1044     
+ Misses        20943      20550       -393     
+ Partials       9350       9329        -21

@xiongjiwei xiongjiwei changed the title expression: fix select using collation get wrong result expression: fix wrong result when select with collation Jul 19, 2020
bb7133
bb7133 reviewed Jul 20, 2020
View reviewed changes
expression/distsql_builtin.go Outdated Show resolved Hide resolved
expression/distsql_builtin.go Show resolved Hide resolved
wjhuang2016
wjhuang2016 reviewed Jul 22, 2020
View reviewed changes
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

@ti-srebot
Copy link
Contributor

@wjhuang2016,Thanks for your review. However, LGTM is restricted to Reviewers or higher roles.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@xiongjiwei
Copy link
Contributor Author

/rebuild

qw4990
qw4990 reviewed Jul 22, 2020
View reviewed changes
expression/builtin.go Outdated Show resolved Hide resolved
qw4990
qw4990 reviewed Jul 22, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

Almost LGTM, please address the comment @xiongjiwei

@qw4990
Copy link
Contributor

qw4990 commented Jul 22, 2020

Should we pick this PR to v4.0? @wjhuang2016

@wjhuang2016
Copy link
Member

Should we pick this PR to v4.0? @wjhuang2016

Yeah

qw4990
qw4990 approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 22, 2020
@qw4990 qw4990 added the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2020
@ti-srebot
Copy link
Contributor

@qw4990 Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

@qw4990
Copy link
Contributor

qw4990 commented Jul 22, 2020

/merge

@ti-srebot
Copy link
Contributor

@qw4990 Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

qw4990
qw4990 approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@qw4990 qw4990 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
Copy link
Contributor

qw4990 commented Jul 22, 2020

/merge

@ti-srebot
Copy link
Contributor

@qw4990 Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

@qw4990 qw4990 added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 22, 2020
@qw4990
Copy link
Contributor

qw4990 commented Jul 22, 2020

/merge

@ti-srebot
Copy link
Contributor

@qw4990 Oops! This PR requires at least 2 LGTMs to merge. The current number of LGTM is 1.

@XuHuaiyu
Copy link
Contributor

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 23, 2020
@qw4990
Copy link
Contributor

qw4990 commented Jul 23, 2020

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 18721

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@xiongjiwei merge failed.

@xiongjiwei
Copy link
Contributor Author

/run-all-tests

1 similar comment
@qw4990
Copy link
Contributor

qw4990 commented Jul 23, 2020

/run-all-tests

@qw4990 qw4990 merged commit d1b728b into pingcap:master Jul 23, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 23, 2020
@xiongjiwei @ti-srebot
cherry pick pingcap#18665 to release-4.0
2a6ae38 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jul 23, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18735

@xiongjiwei xiongjiwei deleted the bug branch July 23, 2020 05:10
bb7133 pushed a commit that referenced this pull request Jul 27, 2020
@ti-srebot
expression: fix wrong result when select with collation (#18665) (#18735
75171f0 
)
@wjhuang2016 wjhuang2016 added the sig/sql-infra SIG: SQL Infra label Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bb7133 bb7133 bb7133 left review comments

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@qw4990 qw4990 qw4990 approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
component/expression contribution This PR is from a community contributor. sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

select using collation get wrong result
6 participants
@xiongjiwei @ti-srebot @qw4990 @wjhuang2016 @XuHuaiyu @bb7133