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: add error code for package types #13300

Merged
merged 11 commits into from
Nov 26, 2019
Merged

types: add error code for package types #13300

merged 11 commits into from
Nov 26, 2019

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Nov 8, 2019

Signed-off-by: Shuaipeng Yu jackysp@gmail.com

What problem does this PR solve?

Add error code for package types.

What is changed and how it works?

Check List

Tests

  • Unit test

Code changes

  • Has exported variable/fields change

Side effects

  • Breaking backward compatibility

@jackysp jackysp added type/enhancement The issue or PR belongs to an enhancement. status/WIP labels Nov 8, 2019
@jackysp jackysp force-pushed the types_errors branch 2 times, most recently from 7892680 to 3785a62 Compare November 12, 2019 03:31
@codecov
Copy link

codecov bot commented Nov 12, 2019 •
edited
Loading

Codecov Report

Merging #13300 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13300   +/-   ##
===========================================
  Coverage   80.3224%   80.3224%           
===========================================
  Files           474        474           
  Lines        117677     117677           
===========================================
  Hits          94521      94521           
  Misses        15743      15743           
  Partials       7413       7413

@jackysp jackysp marked this pull request as ready for review November 12, 2019 04:43
@jackysp jackysp requested a review from a team as a code owner November 12, 2019 04:43
@ghost ghost requested review from qw4990 and SunRunAway and removed request for a team November 12, 2019 04:43
@jackysp jackysp requested review from bb7133, cfzjywxk and lysu November 12, 2019 04:43
@cfzjywxk
Copy link
Contributor

LGTM, the tidb-test suite may fail as the error codes changed and it's not included in CI

@jackysp
Copy link
Member Author

jackysp commented Nov 12, 2019

/run-all-tests

@jackysp
types: add error code for package types
813f7b7 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Nov 12, 2019

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

@jackysp
Copy link
Member Author

jackysp commented Nov 12, 2019

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

2 similar comments
@jackysp
Copy link
Member Author

jackysp commented Nov 12, 2019

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

@jackysp
Copy link
Member Author

jackysp commented Nov 12, 2019

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

@jackysp
Copy link
Member Author

jackysp commented Nov 12, 2019

Thank you for the reminder, @cfzjywxk PTAL.

@SunRunAway SunRunAway removed their request for review November 15, 2019 09:37
@jackysp
Copy link
Member Author

jackysp commented Nov 18, 2019

PTAL @lysu

@jackysp
Copy link
Member Author

jackysp commented Nov 18, 2019

PTAL @bb7133

lysu
lysu reviewed Nov 18, 2019
View reviewed changes
executor/insert_common.go
colTp byte
colName string
)
if col != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

if col == nil does following code will generate error message use unspecified type and '' column name?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes...

@jackysp
Copy link
Member Author

jackysp commented Nov 21, 2019

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

@jackysp
Copy link
Member Author

jackysp commented Nov 21, 2019

PTAL @zimulala

@jackysp
Copy link
Member Author

jackysp commented Nov 22, 2019

PTAL @zimulala @lysu

2 similar comments
@jackysp
Copy link
Member Author

jackysp commented Nov 25, 2019

PTAL @zimulala @lysu

@jackysp
Copy link
Member Author

jackysp commented Nov 26, 2019

PTAL @zimulala @lysu

@lysu
Copy link
Contributor

lysu commented Nov 26, 2019

LGTM

@qw4990 qw4990 removed their request for review November 26, 2019 07:39
zimulala
zimulala reviewed Nov 26, 2019
View reviewed changes
executor/show_test.go
@@ -105,9 +105,9 @@ func (s *testSuite5) TestShowWarnings(c *C) {
tk.MustExec("set @@sql_mode=''")
tk.MustExec("insert show_warnings values ('a')")
c.Assert(tk.Se.GetSessionVars().StmtCtx.WarningCount(), Equals, uint16(1))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1265|Data Truncated"))
tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Truncated incorrect FLOAT value: 'a'"))
Copy link
Contributor

Choose a reason for hiding this comment

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

In MySQL, I test this case, but it result is as follows:

mysql> create table if not exists show_warnings (a int);
Query OK, 0 rows affected (0.06 sec)

mysql> set @@sql_mode='';
Query OK, 0 rows affected (0.01 sec)

mysql> insert show_warnings values ('a');
Query OK, 1 row affected, 1 warning (0.01 sec)

mysql> show warnings;
+---------+------+------------------------------------------------------+
| Level   | Code | Message                                              |
+---------+------+------------------------------------------------------+
| Warning | 1366 | Incorrect integer value: 'a' for column 'a' at row 1 |
+---------+------+------------------------------------------------------+
1 row in set (0.00 sec)

Do we need to update this error code?

Copy link
Member Author

@jackysp jackysp Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It is difficult for us to be completely consistent with MySQL in terms of error codes and error messages. What I want to do is to keep us self-consistent, the unique error code + reasonable error message. I think it can help users search the MySQL manual to find the problem if we use the code which MySQL also has (maybe not the same in the same scenes). If want to be completely consistent, I don't feel I can do it. :)

zimulala
zimulala previously approved these changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/can-merge Indicates a PR has been approved by a committer. label Nov 26, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

@jackysp merge failed.

@jackysp
fix compile
d3e0375 
Signed-off-by: Shuaipeng Yu <jackysp@gmail.com>
@jackysp
Copy link
Member Author

jackysp commented Nov 26, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

@jackysp merge failed.

@jackysp
Copy link
Member Author

jackysp commented Nov 26, 2019

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

Your auto merge job has been accepted, waiting for 13745, 13527

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Nov 26, 2019

@jackysp merge failed.

@jackysp jackysp merged commit a1dc047 into pingcap:master Nov 26, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
@jackysp @XiaTianliang
types: add error code for package types (pingcap#13300)
fffb21c
@jackysp jackysp deleted the types_errors branch February 27, 2020 13:19
@jackysp jackysp mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lysu lysu lysu approved these changes

@zimulala zimulala zimulala left review comments

@bb7133 bb7133 Awaiting requested review from bb7133

@cfzjywxk cfzjywxk Awaiting requested review from cfzjywxk

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jackysp @cfzjywxk @lysu @sre-bot @zimulala