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

planner: fix should not use point get plan (#21124) #21244

Merged
merged 4 commits into from
Nov 25, 2020

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #21124 to release-4.0


What problem does this PR solve?

Issue Number: close #21123 , close #21204

Problem Summary:

In MySQL string convert to numbers it will use number prefix to convert to number, for example:

'asdf' => 0
'123asdf' => 123

So if we create a table like create table t (id varchar(10) primary key), then insert ('asdf'), ('1asdf') into this table. When we execute a query like:

select * from t where id = 0;

the plan should be convert column id into int and compare with number 0, so we should get 'asdf' as the result. But in TiDB it will convert number 0 to string '0' and use point get plan to fetch id = '0' rows, so we will get an empty set result.

What is changed and how it works?

What's Changed:

Add type check for TryFastPlan, if column type is string like and in or = value is number just return empty plan.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • planner: fix wrong point get plan generation in fast plan code path

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@blacktear23 you're already a collaborator in bot's repo.

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 ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 24, 2020
Copy link
Contributor

@lzmhhh123 lzmhhh123 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 24, 2020
@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor Author

Sorry @lzmhhh123, this branch cannot be merged without an approval of release maintainers

@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 25, 2020
@SunRunAway
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 25, 2020
@ti-srebot
Copy link
Contributor Author

Your auto merge job has been accepted, waiting for:

  • 20889
  • 21273
  • 20998

@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot ti-srebot merged commit beb0862 into pingcap:release-4.0 Nov 25, 2020
@SunRunAway SunRunAway deleted the release-4.0-8808a65d49a3 branch November 25, 2020 10:02
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/LGT3 The PR has already had 3 LGTM. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants