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

types: check KindNull / KindMinNotNull / KindMaxValue when comparing datums #19259

Merged
merged 5 commits into from
Aug 24, 2020
Merged

types: check KindNull / KindMinNotNull / KindMaxValue when comparing datums #19259

merged 5 commits into from
Aug 24, 2020

Conversation

eurekaka
Copy link
Contributor

@eurekaka eurekaka commented Aug 18, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #19258

Problem Summary:

Different plans for select / update query.

What is changed and how it works?

What's Changed:

In compareBinaryLiterals, ToInt raises an error since its length is 16, which is larger than 8. In select query, TruncateAsWarning is set true, so the error is ignored and warnings are reported. In update query, TruncateAsWarning is false, so deriveStatsByFilter would use default selectionFactor, which is 0.8, as selectivity. Hence the path.CountAfterAccess is different for select / update query.

How it Works:
Check KindNull / KindMaxValue when comparing datums before calling ToInt, so there is no truncate error.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Integration test

Side effects

N/A

Release note

  • Fix the different plans between select and update query.

@eurekaka eurekaka added type/bugfix This PR fixes a bug. sig/planner SIG: Planner labels Aug 18, 2020
@eurekaka eurekaka requested a review from a team as a code owner August 18, 2020 08:36
@eurekaka eurekaka requested review from XuHuaiyu and removed request for a team August 18, 2020 08:36
@eurekaka eurekaka removed the request for review from XuHuaiyu August 18, 2020 08:37
@eurekaka
Copy link
Contributor Author

/run-all-tests

@eurekaka
Copy link
Contributor Author

/run-unit-test

@coocood
Copy link
Member

coocood commented Aug 19, 2020

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 19, 2020
jackysp
jackysp approved these changes Aug 24, 2020
View reviewed changes
Copy link
Member

@jackysp jackysp 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 removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 24, 2020
@jackysp
Copy link
Member

jackysp commented Aug 24, 2020

/merge

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

Your auto merge job has been accepted, waiting for:

  • 19308

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit bb675c0 into pingcap:master Aug 24, 2020
@ti-srebot ti-srebot mentioned this pull request Aug 24, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-2.1 in PR #19400

@ti-srebot ti-srebot mentioned this pull request Aug 24, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #19401

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 24, 2020
@eurekaka @ti-srebot
cherry pick pingcap#19259 to release-3.1
8e10890 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 24, 2020
Closed
@ti-srebot
Copy link
Contributor

cherry pick to release-3.1 in PR #19402

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Aug 24, 2020
@eurekaka @ti-srebot
cherry pick pingcap#19259 to release-4.0
631970f 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Aug 24, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19403

@eurekaka eurekaka deleted the update_index_path branch August 25, 2020 03:14
eurekaka added a commit to ti-srebot/tidb that referenced this pull request Aug 25, 2020
@eurekaka
cherry pick pingcap#19259 to release-2.1
24cdb45 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
eurekaka added a commit to ti-srebot/tidb that referenced this pull request Aug 25, 2020
@eurekaka
cherry pick pingcap#19259 to release-3.0
f783866 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
eurekaka added a commit that referenced this pull request Aug 25, 2020
@ti-srebot @eurekaka
cherry pick #19259 to release-4.0 (#19403)
3f0740e 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: Kenan Yao <cauchy1992@gmail.com>
ti-srebot added a commit that referenced this pull request Aug 27, 2020
@ti-srebot
types: check KindNull / KindMinNotNull / KindMaxValue when comparing …
0aae420 
…datums (#19259) (#19400)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
ti-srebot added a commit that referenced this pull request Aug 27, 2020
@ti-srebot
types: check KindNull / KindMinNotNull / KindMaxValue when comparing …
ba5ee00 
…datums (#19259) (#19401)

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@jackysp jackysp jackysp approved these changes

@winoros winoros Awaiting requested review from winoros

@qw4990 qw4990 Awaiting requested review from qw4990

@coocood coocood Awaiting requested review from coocood

Assignees
No one assigned
Labels
component/expression 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

different plans for select / update
4 participants
@eurekaka @coocood @jackysp @ti-srebot