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

*: fix the problem that PointGet returns wrong results in the case of overflow #14776

Merged
merged 13 commits into from
Apr 23, 2020

Conversation

zimulala
Copy link
Contributor

What problem does this PR solve?

tidb> create table t6 (a bigint);
Query OK, 0 rows affected (0.00 sec)
tidb> insert into t6 values(9223372036854775807);
Query OK, 1 row affected (0.00 sec)
tidb> alter table t6 add unique key idx(a);
Query OK, 0 rows affected (0.04 sec)

Before this PR:

tidb> select * from t6 where a = 9223372036854775808;
+---------------------+
| a                   |
+---------------------+
| 9223372036854775807 |
+---------------------+
1 row in set, 1 warning (0.01 sec)

After this PR:

tidb>  select * from t6 where a = 9223372036854775808;
Empty set (0.00 sec)

What is changed and how it works?

Do ConvertTo to the point-get value. Handling the overflow and truncated errors.

Check List

Tests

  • Unit test

Related changes

  • Need to cherry-pick to the release branch

Release note

  • Write release note for bug-fix or new feature.

@zimulala zimulala requested a review from a team as a code owner February 13, 2020 07:25
@ghost ghost requested review from alivxxx and lzmhhh123 and removed request for a team February 13, 2020 07:25
@zimulala zimulala added sig/execution SIG execution type/bugfix This PR fixes a bug. labels Feb 13, 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.

Please resolve conflicts and fix ci.

@zimulala zimulala requested a review from a team as a code owner March 25, 2020 02:59
@ghost ghost requested review from qw4990 and wshwsh12 and removed request for a team March 25, 2020 02:59
@zimulala
Copy link
Contributor Author

PTAL @lzmhhh123

@zimulala zimulala removed the request for review from alivxxx April 14, 2020 04:09
@zimulala
Copy link
Contributor Author

PTAL @lzmhhh123 @XuHuaiyu

@codecov
Copy link

codecov bot commented Apr 14, 2020

Codecov Report

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

@@               Coverage Diff                @@
##             master     #14776        +/-   ##
================================================
+ Coverage   80.3457%   80.4066%   +0.0609%     
================================================
  Files           506        506                
  Lines        136968     137164       +196     
================================================
+ Hits         110048     110289       +241     
+ Misses        18304      18274        -30     
+ Partials       8616       8601        -15     

@@ -652,6 +653,9 @@ func tryPointGetPlan(ctx sessionctx.Context, selStmt *ast.SelectStmt) *PointGetP
return nil
}
pi := tbl.GetPartitionInfo()
if pi != nil && pi.Type != model.PartitionTypeHash {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment why partitions rather than HashPartition are not supported.

PS: It is actually not related to this PR, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's the original code, I only move it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, we only support for HashPartition.

@zimulala
Copy link
Contributor Author

PTAL @crazycs520 @lzmhhh123 @XuHuaiyu

}
col := model.FindColumnInfo(tbl.Cols(), colName.Name.Name.L)
if col == nil || // Handling the case when the column is _tidb_rowid.
(col.Tp == mysql.TypeString && col.Collate == charset.CollationBin) { // This type we needn't to pad `\0` in here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any test case for that? I don't know this is used for what.

Copy link
Contributor Author

@zimulala zimulala Apr 20, 2020

Choose a reason for hiding this comment

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

Yes. These tests already exist. The test case for the table of create table t(a binary(2) primary key, b binary(2)); table, like select * from t where a = "a ";
@crazycs520

@zimulala
Copy link
Contributor Author

@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

@zimulala merge failed.

@zimulala
Copy link
Contributor Author

/run-common-test
/run-sqllogic-test-1

@wshwsh12 wshwsh12 removed request for qw4990 and wshwsh12 April 23, 2020 06:07
@zimulala
Copy link
Contributor Author

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

Your auto merge job has been accepted, waiting for:

  • 16550

@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

/run-all-tests

@zimulala zimulala merged commit e521ea9 into pingcap:master Apr 23, 2020
@zimulala zimulala deleted the overflows branch April 23, 2020 07:18
sre-bot pushed a commit to sre-bot/tidb that referenced this pull request Apr 23, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

cherry pick to release-3.0 in PR #16753

@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

cherry pick to release-3.1 in PR #16754

@sre-bot
Copy link
Contributor

sre-bot commented Apr 23, 2020

cherry pick to release-4.0 in PR #16755

@wjhuang2016
Copy link
Member

mysql> select * from t6 where a != 9223372036854775808;
ERROR 1690 (22003): constant 9223372036854775808 overflows bigint

PTAL @zimulala

@zimulala
Copy link
Contributor Author

@wjhuang2016
This PR fixed the problem that PointGet returns wrong results in the case of overflow. The specific scene of the repair is stated in the description. The scene you mentioned has not been changed before or after this PR. You can create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution 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.

6 participants