DDL: Refine error message about "Invalid default value" #6333

spongedu · 2018-04-22T02:50:30Z

tidb> create table t(c1 decimal default 1.7976931348623157E308);
ERROR 1067 (42000): Invalid default value for 'c1'

sre-bot · 2018-04-22T02:50:34Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

shenli · 2018-04-22T15:39:47Z

/run-all-tests

alivxxx · 2018-04-23T03:06:12Z

ddl/ddl_api.go

@@ -471,7 +471,10 @@ func checkDefaultValue(ctx sessionctx.Context, c *table.Column, hasDefaultValue
 		if types.ErrTruncated.Equal(err) {
 			return types.ErrInvalidDefault.GenByArgs(c.Name)
 		}
-		return errors.Trace(err)
+		if err != nil {


Why not combine it with the upper if? Should all errors be considered as invalid default?

@lamxTyler It's a bit complicate here I found. Illegal default value doesn't necessary result in ErrInvalidDefault, actually. For example:

In MySQL:

mysql> create table t(a decimal(3) default 1.25567); Query OK, 0 rows affected, 1 warning (0.03 sec) mysql> show warnings; +-------+------+----------------------------------------+ | Level | Code | Message | +-------+------+----------------------------------------+ | Note | 1265 | Data truncated for column 'a' at row 1 | +-------+------+----------------------------------------+ 1 row in set (0.00 sec)

In TiDB:

tidb> create table t(a decimal(3) default 1.25567); ERROR 1067 (42000): Invalid default value for 'a'

But if default value check if set in ALTER TABLE situation(ddl/ddl_db_test.go: line 1139), The
ErrInvalidDefault error is raised.

another example:

Im MySQL:

mysql> create table t ( a date default '2012-01-01 01:01:01'); Query OK, 0 rows affected, 1 warning (0.02 sec) mysql> show warnings; +-------+------+---------------------------------------------------------------------+ | Level | Code | Message | +-------+------+---------------------------------------------------------------------+ | Note | 1292 | Incorrect date value: '2012-01-01 01:01:01' for column 'a' at row 1 | +-------+------+---------------------------------------------------------------------+ 1 row in set (0.00 sec)

In TiDB:

create table t ( a date default '2012-01-01 01:01:01'); Query OK, 0 rows affected (0.01 sec)

I think in case #6303 , we can fix the error message by throwing out ErrInvalidDefault in all means, and Fix the corner cases or situations in extra one or more prs ..

ciscoxll · 2018-04-24T09:51:21Z

/run-all-tests

alivxxx · 2018-04-25T02:42:55Z

ddl/ddl_api.go

-		if types.ErrTruncated.Equal(err) {
-			return types.ErrInvalidDefault.GenByArgs(c.Name)
+		if _, err := table.GetColDefaultValue(ctx, c.ToInfo()); err != nil {
+			return errors.Trace(types.ErrInvalidDefault.GenByArgs(c.Name))


Why trace the error? And please add a test for the fix.

shenli · 2018-04-26T13:48:27Z

/run-all-tests

zz-jason · 2018-04-28T01:07:34Z

ddl/integration_test.go

+	ctx   sessionctx.Context
+}
+
+func (s *testIntegrationSuite) cleanEnv(c *C) {


we can use TearDownTest to avoid calling cleanEnv in every tests.

I think we'd better clearEnv after every case. It's common that different cases use the same database name and table name. test.t for example. We'd better clean them as soon as possible.

TearDownTest will be called automatically after every test case, we can rename cleanEnv to TearDownTest to avoid manually calling this function.

zz-jason · 2018-04-28T01:07:43Z

ddl/integration_test.go

@@ -0,0 +1,88 @@
+// Copyright 2015 PingCAP, Inc.


s/2015/2018/

XuHuaiyu · 2018-04-28T03:21:42Z

ddl/integration_test.go

+
+import (
+	"fmt"
+	"github.com/juju/errors"


add a blank line between line 17 and line 18

@spongedu please address this comment.

@zz-jason ok

XuHuaiyu · 2018-04-28T03:47:47Z

ddl/ddl_api.go

@@ -467,11 +467,10 @@ func checkDefaultValue(ctx sessionctx.Context, c *table.Column, hasDefaultValue
 	}

 	if c.DefaultValue != nil {
-		_, err := table.GetColDefaultValue(ctx, c.ToInfo())
-		if types.ErrTruncated.Equal(err) {


We'd better not remove this check totally,
it may be safer to add another check for err here?

@XuHuaiyu as I commented above, it's a bit complicate here I found. Illegal default value doesn't necessary result in ErrInvalidDefault, actually. For example:

In MySQL:

mysql> create table t(a decimal(3) default 1.25567); Query OK, 0 rows affected, 1 warning (0.03 sec) mysql> show warnings; +-------+------+----------------------------------------+ | Level | Code | Message | +-------+------+----------------------------------------+ | Note | 1265 | Data truncated for column 'a' at row 1 | +-------+------+----------------------------------------+ 1 row in set (0.00 sec)

In TiDB:

tidb> create table t(a decimal(3) default 1.25567); ERROR 1067 (42000): Invalid default value for 'a'

But if default value check if set in ALTER TABLE situation(ddl/ddl_db_test.go: line 1139), The
ErrInvalidDefault error is raised.

another example:

Im MySQL:

mysql> create table t ( a date default '2012-01-01 01:01:01'); Query OK, 0 rows affected, 1 warning (0.02 sec) mysql> show warnings; +-------+------+---------------------------------------------------------------------+ | Level | Code | Message | +-------+------+---------------------------------------------------------------------+ | Note | 1292 | Incorrect date value: '2012-01-01 01:01:01' for column 'a' at row 1 | +-------+------+---------------------------------------------------------------------+ 1 row in set (0.00 sec)

In TiDB:

create table t ( a date default '2012-01-01 01:01:01'); Query OK, 0 rows affected (0.01 sec)

I think in case #6303 , we can fix the error message by throwing out ErrInvalidDefault in all means, ignore the error check, and Fix the corner cases or situations in extra one or more prs ..

alivxxx

LGTM

shenli · 2018-05-08T02:39:54Z

/run-all-tests

shenli · 2018-05-08T02:43:57Z

LGTM

shenli · 2018-05-08T02:44:05Z

@zimulala PTAL

zz-jason · 2018-05-09T07:20:24Z

@zimulala @coocood @winkyao @jackysp PTAL

coocood

LGTM

tiancaiamao · 2018-05-20T07:46:24Z

/run-all-tests

DDL: Refine errmsg for default value check when create table

f918013

spongedu force-pushed the fix_6303 branch from e243a22 to f918013 Compare April 22, 2018 03:00

Merge remote-tracking branch 'upstream/master' into fix_6303

8ef798a

shenli added contribution This PR is from a community contributor. DDL labels Apr 22, 2018

jackysp added the all-tests-passed label Apr 23, 2018

alivxxx reviewed Apr 23, 2018

View reviewed changes

zz-jason changed the title ~~DDL: Refine errmsg for default value check when create table~~ DDL: Refine error message about "Invalid default value" Apr 23, 2018

Code format refine

6896bd1

spongedu force-pushed the fix_6303 branch from 171d701 to 6896bd1 Compare April 23, 2018 13:16

spongedu force-pushed the fix_6303 branch from 6896bd1 to 8ef798a Compare April 24, 2018 16:42

spongedu added 2 commits April 25, 2018 01:12

Merge remote-tracking branch 'upstream/master' into fix_6303

d006d47

Merge branch 'fix_6303' of github.com:spongedu/tidb into fix_6303

ad72a76

alivxxx reviewed Apr 25, 2018

View reviewed changes

spongedu and others added 4 commits April 25, 2018 11:18

Merge remote-tracking branch 'upstream/master' into fix_6303

62ebafe

Merge branch 'master' into fix_6303

ffe44b1

1.add test case 2. remove Trace

c9f0fa4

Merge branch 'master' into fix_6303

2a0c1d4

zz-jason reviewed Apr 28, 2018

View reviewed changes

spongedu added 3 commits April 28, 2018 09:43

Merge remote-tracking branch 'upstream/master' into fix_6303

766c947

code refine

1ad5d86

code refine

49cd96f

XuHuaiyu reviewed Apr 28, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix_6303

8c6945b

alivxxx reviewed May 3, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix_6303

b1af9bf

zz-jason added the status/LGT1 Indicates that a PR has LGTM 1. label May 7, 2018

Merge branch 'master' into fix_6303

61eed9b

shenli added status/LGT2 Indicates that a PR has LGTM 2. require-LGT3 Indicates that the PR requires three LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 8, 2018

spongedu added 3 commits May 9, 2018 00:58

Merge remote-tracking branch 'upstream/master' into fix_6303

ad43730

Code format refine

cd8b849

Merge branch 'fix_6303' of github.com:spongedu/tidb into fix_6303

5329885

zimulala removed the require-LGT3 Indicates that the PR requires three LGTM. label May 9, 2018

Merge branch 'master' into fix_6303

fc387f5

coocood approved these changes May 18, 2018

View reviewed changes

Merge branch 'master' into fix_6303

560e04a

coocood added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels May 18, 2018

coocood and others added 2 commits May 18, 2018 21:17

Merge branch 'master' into fix_6303

fe80795

Merge branch 'master' into fix_6303

dab749b

tiancaiamao merged commit b4c0e6c into pingcap:master May 20, 2018

spongedu deleted the fix_6303 branch May 27, 2018 09:59

you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDL: Refine error message about "Invalid default value" #6333

DDL: Refine error message about "Invalid default value" #6333

spongedu commented Apr 22, 2018 •

edited

Loading

sre-bot commented Apr 22, 2018

shenli commented Apr 22, 2018

alivxxx Apr 23, 2018 •

edited

Loading

spongedu Apr 23, 2018

ciscoxll commented Apr 24, 2018

alivxxx Apr 25, 2018

shenli commented Apr 26, 2018

zz-jason Apr 28, 2018

spongedu Apr 28, 2018

zz-jason Apr 28, 2018

spongedu Apr 28, 2018

zz-jason Apr 28, 2018

spongedu Apr 28, 2018

XuHuaiyu Apr 28, 2018

zz-jason May 7, 2018

spongedu May 8, 2018

XuHuaiyu Apr 28, 2018

spongedu May 2, 2018

alivxxx left a comment

shenli commented May 8, 2018

shenli commented May 8, 2018

shenli commented May 8, 2018

zz-jason commented May 9, 2018

coocood left a comment

tiancaiamao commented May 20, 2018

DDL: Refine error message about "Invalid default value" #6333

DDL: Refine error message about "Invalid default value" #6333

Conversation

spongedu commented Apr 22, 2018 • edited Loading

sre-bot commented Apr 22, 2018

shenli commented Apr 22, 2018

alivxxx Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciscoxll commented Apr 24, 2018

Choose a reason for hiding this comment

shenli commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

shenli commented May 8, 2018

shenli commented May 8, 2018

shenli commented May 8, 2018

zz-jason commented May 9, 2018

coocood left a comment

Choose a reason for hiding this comment

tiancaiamao commented May 20, 2018

spongedu commented Apr 22, 2018 •

edited

Loading

alivxxx Apr 23, 2018 •

edited

Loading