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

util: fix wrong range when building from string column is true. #16135

Merged
merged 9 commits into from
Apr 17, 2020

Conversation

wshwsh12
Copy link
Contributor

@wshwsh12 wshwsh12 commented Apr 7, 2020

What problem does this PR solve?

Issue Number: close #15990

Problem Summary: Using a column both in a string comparison and as a boolean yields an incorrect result.

What is changed and how it works?

What's Changed: Build a range from string column wrongly. So I change the endpoint from int 0 to string ''

How it Works:

Related changes

  • Need to cherry-pick to the release branch 2.1,3.0,3.1,4.0

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note

Fix the wrong behavior when string column in where stmt to check True/False.

@wshwsh12 wshwsh12 requested review from a team as code owners April 7, 2020 16:48
@ghost ghost requested review from SunRunAway and francis0407 and removed request for a team April 7, 2020 16:48
@wshwsh12 wshwsh12 requested review from winoros and a team April 7, 2020 16:48
@ghost ghost removed request for a team April 7, 2020 16:48
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

Will the column with other type cause this error too?
Such as decimal and float?

@wshwsh12
Copy link
Contributor Author

wshwsh12 commented Apr 15, 2020

Will the column with other type cause this error too?
Such as decimal and float?

I have a try. Decimal and float will get a right range, beacuse decimal and float are comparable to Int.
Other typies, time will be always true if it's vaild, so it also doesn't cause this error.
I will have a try with Json Type, but I can create another pr to fix the bug if Json type has something wrong.

@wshwsh12 wshwsh12 requested a review from winoros April 15, 2020 09:38
@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #16135 into master will increase coverage by 0.0455%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #16135        +/-   ##
================================================
+ Coverage   80.4113%   80.4568%   +0.0455%     
================================================
  Files           506        506                
  Lines        136625     136805       +180     
================================================
+ Hits         109862     110069       +207     
+ Misses        18192      18178        -14     
+ Partials       8571       8558        -13     

Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

ok i got it. lgtm

@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

@wshwsh12 merge failed.

@zz-jason zz-jason added the status/LGT2 Indicates that a PR has LGTM 2. label Apr 17, 2020
@zz-jason zz-jason merged commit 2b03cf2 into pingcap:master Apr 17, 2020
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 17, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

cherry pick to release-2.1 in PR #16552

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 17, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

cherry pick to release-3.0 in PR #16553

sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 17, 2020
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

cherry pick to release-3.1 in PR #16554

@sre-bot
Copy link
Contributor

sre-bot commented Apr 17, 2020

cherry pick to release-4.0 in PR #16555

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression component/util sig/planner SIG: Planner 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
Development

Successfully merging this pull request may close these issues.

Using a column both in a string comparison and as a boolean yields an incorrect result
5 participants