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 return err when decimal from string value #22407

Merged
merged 16 commits into from
Nov 22, 2021

Conversation

erwadba
Copy link
Contributor

@erwadba erwadba commented Jan 15, 2021

What problem does this PR solve?

Issue Number: close #22394

Problem Summary:

test> drop table t1;
Query OK, 0 rows affected (0.27 sec)
test> create table t1(id decimal(10));
Query OK, 0 rows affected (0.12 sec)
test> insert into t1 values('1dsf');    
Query OK, 1 row affected (0.03 sec)   <<<<<<<<<<< should report error there
test> 

What is changed and how it works?

What's Changed:
When string value to decimal not meet word e/E. Report ErrTruncated.
How it Works:

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
mysql> create table td(id decimal(10,6));
Query OK, 0 rows affected (0.04 sec)
mysql> insert into td values('1sdf');
ERROR 1366 (HY000): Incorrect decimal value: '1sdf' for column 'id' at row 1
mysql>

Side effects

  • N/A

Release note

  • No release note

@erwadba erwadba requested a review from a team as a code owner January 15, 2021 06:35
@erwadba erwadba requested review from XuHuaiyu and removed request for a team January 15, 2021 06:35
@erwadba
Copy link
Contributor Author

erwadba commented Jan 15, 2021

@wshwsh12 PTAL

@erwadba
Copy link
Contributor Author

erwadba commented Jan 15, 2021

@XuHuaiyu PTAL

@github-actions github-actions bot added the sig/sql-infra SIG: SQL Infra label Jan 15, 2021
@ichn-hu ichn-hu mentioned this pull request Jan 16, 2021
@erwadba erwadba changed the title execute: fix return err when decimal from string value types: fix return err when decimal from string value Jan 17, 2021
@erwadba
Copy link
Contributor Author

erwadba commented Jan 21, 2021

@ti-srebot /run-all-tests

@XuHuaiyu XuHuaiyu requested review from AilinKid and removed request for XuHuaiyu February 19, 2021 03:24
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed sig/infra labels Feb 22, 2021
@xiongjiwei
Copy link
Contributor

hi, could you please add the Manual test into your PR?

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 1, 2021
@github-actions github-actions bot added the sig/execution SIG execution label Apr 1, 2021
@erwadba
Copy link
Contributor Author

erwadba commented Apr 1, 2021

@xiongjiwei PTAL
Maybe this pr #22507 already fix this bug. I test in the master branch. It will return error ERROR 8029 (HY000): Bad Number. The test like this below

In TIDB master branch
mysql> create table td(id decimal(10));
Query OK, 0 rows affected (0.04 sec)
mysql> insert into td values('1dsf');
ERROR 8029 (HY000): Bad Number
mysql>

But the pr #22507 didn't consider the situation like below. I test in the master branch.

In TIDB master branch
mysql>  insert into td values('1.1.1.1.1');          <<<< there need report error
Query OK, 1 row affected, 1 warning (0.00 sec)
mysql> show warnings;
+---------+------+------------------------------------------+
| Level   | Code | Message                                  |
+---------+------+------------------------------------------+
| Warning | 1292 | Truncated incorrect DECIMAL value: '1.1' |
+---------+------+------------------------------------------+
1 row in set (0.00 sec)
mysql>

In MySQL8
mysql> insert into td values('1.1.1.1.1');
ERROR 1366 (HY000): Incorrect decimal value: '1.1.1.1.1' for column 'id' at row 1
mysql>

@erwadba
Copy link
Contributor Author

erwadba commented Apr 1, 2021

@AilinKid PTAL

@erwadba
Copy link
Contributor Author

erwadba commented Apr 1, 2021

@lysu PTAL

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021
tk.MustExec("use test")
tk.MustExec("drop table if exists t")
tk.MustExec("create table t (id decimal(10))")
tk.MustGetErrCode("insert into t values('1sdf')", errno.ErrTruncatedWrongValueForField)
Copy link
Contributor

@AilinKid AilinKid May 27, 2021

Choose a reason for hiding this comment

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

we can add more tests here to simplify the coverage.

eg: for the line you remove in L424, the '12e-3', '12A' should be added. and seems we need a warning for the latter.

Copy link
Contributor Author

@erwadba erwadba Jun 1, 2021

Choose a reason for hiding this comment

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

Done. PTAL~ Thx.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2021
@tisonkun
Copy link
Contributor

tisonkun commented Oct 5, 2021

/cc @AilinKid @xiongjiwei

Conflict resolved, please take a look >_<

@Defined2014
Copy link
Contributor

/cc @xiongjiwei @xhebox @wjhuang2016
PTAL

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 22, 2021
@xiongjiwei
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 457b905

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 22, 2021
@wjhuang2016
Copy link
Member

/run-check_dev_2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeNewDecimal handle wrong about string value.
8 participants