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

types: fix string to integer cast #11295

Merged
merged 16 commits into from
Jul 26, 2019
Merged

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented Jul 17, 2019

What problem does this PR solve?

part of #11223 fix issue #11179

What is changed and how it works?

add a ConvertStrToIntStrict to control the way we convert string to int.

  • if ConvertStrToIntStrict is false, the cast way is getValidFloatPrefix, then floatStrToIntStr
  • otherwise, we cast string to integer prefix in a strict way, only extract 0-9, (+ or - in first bit).

only set ConvertStrToIntStrict to true in select/explain context, which is compatible with MySQL

following is the cast behavior in insert of MySQL 5.7

mysql> select version();
+------------+
| version()  |
+------------+
| 5.7.24-log |
+------------+
1 row in set (0.01 sec)

mysql> use test;
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Database changed
mysql> drop table if exists t;                                                                                                                                                    Query OK, 0 rows affected (0.04 sec)

mysql> create table t (id int);
Query OK, 0 rows affected (0.06 sec)

mysql> insert into t values ('1e2'), ('10e-1'), ('0.123e3');
Query OK, 3 rows affected (0.03 sec)
Records: 3  Duplicates: 0  Warnings: 0

mysql> select * from t;
+------+
| id   |
+------+
|  100 |
|    1 |
|  123 |
+------+
3 rows in set (0.00 sec)

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch

types/convert_test.go Outdated Show resolved Hide resolved
@amyangfei amyangfei changed the title types: fix string to integer convert types: fix string to integer cast Jul 18, 2019
@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

Merging #11295 into master will decrease coverage by 0.47%.
The diff coverage is 93.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11295      +/-   ##
==========================================
- Coverage   81.84%   81.36%   -0.48%     
==========================================
  Files         424      424              
  Lines       93108    90833    -2275     
==========================================
- Hits        76202    73906    -2296     
- Misses      11602    11612      +10     
- Partials     5304     5315      +11
Impacted Files Coverage Δ
sessionctx/stmtctx/stmtctx.go 99.19% <ø> (-0.27%) ⬇️
planner/core/point_get_plan.go 85.24% <0%> (-7.64%) ⬇️
executor/executor.go 78.75% <100%> (-0.27%) ⬇️
types/convert.go 93.03% <100%> (-1.52%) ⬇️
types/datum.go 83.87% <100%> (ø) ⬆️
store/tikv/gcworker/gc_worker.go 61.33% <0%> (-6.66%) ⬇️
store/tikv/snapshot.go 76.66% <0%> (-5.65%) ⬇️
store/tikv/lock_resolver.go 43.75% <0%> (-5.15%) ⬇️
store/tikv/rawkv.go 66.83% <0%> (-4.19%) ⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd419c8...b58c2ea. Read the comment docs.

@amyangfei
Copy link
Contributor Author

/run-all-tests tidb-test=pr/863

@qw4990 qw4990 self-requested a review July 22, 2019 05:25
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

@amyangfei amyangfei added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 25, 2019
@amyangfei
Copy link
Contributor Author

PTAL @zz-jason

@@ -219,7 +219,10 @@ func tryPointGetPlan(ctx sessionctx.Context, selStmt *ast.SelectStmt) *PointGetP
p.IsTableDual = true
return p
}
return nil
// some scenarios cast to int with error, but we may use this value in point get
if !terror.ErrorEqual(types.ErrTruncatedWrongVal, err) {
Copy link
Member

Choose a reason for hiding this comment

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

should we return a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

warning has been added in truncated error handler

types/convert.go Outdated Show resolved Hide resolved
@@ -625,7 +649,7 @@ func getValidFloatPrefix(sc *stmtctx.StatementContext, s string) (valid string,
valid = "0"
}
if validLen == 0 || validLen != len(s) {
Copy link
Member

Choose a reason for hiding this comment

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

if len(str) == 0, validLen can still be zero, but it's not truncated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked some behavior in MySQL, and get the following result

mysql> show create table t;                                                                                                                                                                                                                   +-------+--------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                             |
+-------+--------------------------------------------------------------------------------------------------------------------------+
| t     | CREATE TABLE `t` (
  `id` int(11) DEFAULT NULL,
  `val` varchar(255) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=latin1 |
+-------+--------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> insert into t values ('', 'a');
ERROR 1366 (HY000): Incorrect integer value: '' for column 'id' at row 1
mysql> insert into t values ('0', 'a'), ('1', 'a');
Query OK, 2 rows affected (0.01 sec)
Records: 2  Duplicates: 0  Warnings: 0

mysql> select * from t where id = '';
+------+------+
| id   | val  |
+------+------+
|    0 | a    |
+------+------+
1 row in set (0.00 sec)

mysql> update t set val = 'b' where id = '';
Query OK, 1 row affected (0.00 sec)
Rows matched: 1  Changed: 1  Warnings: 0

mysql> select * from t;
+------+------+
| id   | val  |
+------+------+
|    0 | b    |
|    1 | a    |
+------+------+
2 rows in set (0.00 sec)

mysql> select * from t where id = '1.0';
+------+------+
| id   | val  |
+------+------+
|    1 | a    |
+------+------+
1 row in set (0.00 sec)

mysql> update t set val = 'c' where id = '1.0';
Query OK, 1 row affected (0.01 sec)
Rows matched: 1  Changed: 1  Warnings: 0

Copy link
Contributor Author

@amyangfei amyangfei Jul 26, 2019

Choose a reason for hiding this comment

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

so in select and update context, '' will be cast to 0 without no warnings. Added these two conditions in the first logic of getValidFloatPrefix

@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei
Copy link
Contributor Author

/run-all-tests tidb-test=pr/863

@amyangfei
Copy link
Contributor Author

/run-all-tests tidb-test=pr/863

@amyangfei
Copy link
Contributor Author

/run-unit-tests

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason merged commit 1e1cc1f into pingcap:master Jul 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 26, 2019

cherry pick to release-2.1 failed

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Jul 26, 2019

cherry pick to release-3.0 failed

@XuHuaiyu
Copy link
Contributor

@amyangfei Please cherry-pick this to release-2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants