types: fix invalid time cast bug #4338

mccxj · 2017-08-27T13:25:00Z

fixed issue #4324.
act as invalid time when minute gt 60 or second gt 60.

zz-jason · 2017-08-27T13:42:09Z

util/types/time.go

+	// Invalid TIME values are converted to '00:00:00'
+	// See https://dev.mysql.com/doc/refman/5.7/en/time.html
+	if minute >= 60 || second >= 60 {
+		return ZeroDuration, errors.Trace(ErrInvalidTimeFormat)


just return ZeroDuration, ErrInvalidTimeFormat

what about hour >= 24?

@winkyao
See https://dev.mysql.com/doc/refman/5.7/en/time.html

MySQL retrieves and displays TIME values in 'HH:MM:SS' format (or 'HHH:MM:SS' format for large hours values). TIME values may range from '-838:59:59' to '838:59:59'. The hours part may be so large because the TIME type can be used not only to represent a time of day (which must be less than 24 hours), but also elapsed time or a time interval between two events (which may be much greater than 24 hours, or even negative).

zz-jason · 2017-08-27T13:48:12Z

expression/integration_test.go

+		c.Assert(errors.ErrorStack(err), Equals, "")
+		c.Assert(rs, NotNil)
+		_, err = tidb.GetRows(rs)
+		c.Assert(err.Error(), Equals, "invalid time format")


in mysql, the behavior of invalid time format is depended on sql mode, in strict sql mode:

MySQL > select cast('239010' as time); +------------------------+ | cast('239010' as time) | +------------------------+ | NULL | +------------------------+ 1 row in set, 1 warning (0.00 sec) MySQL > show warnings; +---------+------+------------------------------------------+ | Level | Code | Message | +---------+------+------------------------------------------+ | Warning | 1292 | Truncated incorrect time value: '239010' | +---------+------+------------------------------------------+ 1 row in set (0.00 sec)

and:

MySQL > create table t(a time); Query OK, 0 rows affected (0.03 sec) MySQL > insert into t select cast('239010' as time); ERROR 1292 (22007): Truncated incorrect time value: '239010'

Please use session.go:HandleTruncate to handle it.

ok. done. PTAL.

zz-jason · 2017-08-30T00:38:19Z

expression/integration_test.go

+	for _, invalidTime := range invalidTimes {
+		msg := fmt.Sprintf("Warning 1292 Truncated incorrect time value: '%s'", invalidTime)
+		result = tk.MustQuery(fmt.Sprintf("select cast('%s' as time);", invalidTime))
+		result.Check(testkit.Rows("00:00:00"))


in mysql:

MySQL > select cast('10009010' as time); +--------------------------+ | cast('10009010' as time) | +--------------------------+ | NULL | +--------------------------+ 1 row in set, 1 warning (0.00 sec) MySQL > set sql_mode=''; Query OK, 0 rows affected, 1 warning (0.01 sec) MySQL > select cast('10009010' as time); +--------------------------+ | cast('10009010' as time) | +--------------------------+ | NULL | +--------------------------+ 1 row in set, 1 warning (0.00 sec)

the test case is not compatible with mysql's behavior

@zz-jason PTAL.
changes:
1.when return value is ZeroDuration, then set isNull to true.
2.change the testcase.

zz-jason · 2017-08-30T00:40:52Z

expression/integration_test.go

+	for _, invalidTime := range invalidTimes {
+		msg := fmt.Sprintf("Warning 1292 Truncated incorrect time value: '%s'", invalidTime)
+		result = tk.MustQuery(fmt.Sprintf("select cast('%s' as time);", invalidTime))
+		result.Check(testkit.Rows("00:00:00"))


zz-jason · 2017-08-30T03:21:50Z

expression/builtin_cast.go

 	if isNull || err != nil {
 		return res, isNull, errors.Trace(err)
 	}
 	res, err = types.ParseDuration(strconv.FormatInt(val, 10), b.tp.Decimal)
+	if types.ErrTruncatedWrongVal.Equal(err) {
+		err = sc.HandleTruncate(err)
+		if res == types.ZeroDuration {


if err != nil { return res, true, errors.Trace(err) }

move if res == types.ZeroDuration { out of if types.ErrTruncatedWrongVal.Equal(err) { statement

if res is a zero duration, should we throw or handle a zero duration error ? see handle errors in strict sql mode #4373 for more detail

zero duration is valid value. so I need to check ErrTruncatedWrongVal err to distinguish invalid zero from valid zero duration.

mysql> select cast('000000' as time) ; +------------------------+ | cast('000000' as time) | +------------------------+ | 00:00:00 | +------------------------+ 1 row in set (0.00 sec)

expression.handleInvalidTimeError appended warning message, but was not compatible with mysql's behavior. ErrInvalidTimeFormat does not contains any params.
mysql:

mysql> select cast('123080' as time) ; +------------------------+ | cast('123080' as time) | +------------------------+ | NULL | +------------------------+ 1 row in set, 1 warning (0.00 sec) mysql> show warnings; +---------+------+------------------------------------------+ | Level | Code | Message | +---------+------+------------------------------------------+ | Warning | 1292 | Truncated incorrect time value: '123080' | +---------+------+------------------------------------------+ 1 row in set (0.00 sec)

@zz-jason

with ErrTruncatedWrongVal and zero duration, set isNull to true, append warning or not, then will show .

with ErrTruncatedWrongVal and not zero duration, the over range case, append warning or not.

zero duration without error, the normal case, just pass.

zz-jason · 2017-08-30T03:25:01Z

expression/builtin_cast.go

 	if isNull || err != nil {
 		return res, false, errors.Trace(err)
 	}
 	res, err = types.ParseDuration(string(val.ToString()), b.tp.Decimal)
+	if types.ErrTruncatedWrongVal.Equal(err) {
+		err = sc.HandleTruncate(err)
+		if res == types.ZeroDuration {


zz-jason · 2017-08-30T03:25:12Z

expression/builtin_cast.go

@@ -958,6 +972,9 @@ func (b *builtinCastStringAsDurationSig) evalDuration(row []types.Datum) (res ty
 	res, err = types.ParseDuration(val, b.tp.Decimal)
 	if types.ErrTruncatedWrongVal.Equal(err) {
 		err = sc.HandleTruncate(err)
+		if res == types.ZeroDuration {


I think when res == types.ZeroDuration, it is perhaps a zero duration, because the res is a value copy of types.ZeroDuration, types.ParseDuration return duration value, not duration pointer, so It can not distinguish it. Maybe it is better to add a boolean isNull return value to ParseDuration?

@winkyao
there are 3 cases:

with ErrTruncatedWrongVal and zero duration, set isNull to true, append warning or not, then will show .

with ErrTruncatedWrongVal and not zero duration, the over range case, append warning or not.

zero duration without error, the normal case, just pass.

so I think res == types.ZeroDuration condition inside err check statment will be ok.

we can trust the result of types.ParseDuration(val, b.tp.Decimal) only if err == nil, which means we should move res == types.ZeroDuration condition outside the err check statement, that is:

res, err = types.ParseDuration(val, b.tp.Decimal) if err != nil { if types.ErrTruncatedWrongVal.Equal(err) { ... } ... } else if res == types.ZeroDuration { ... }

zz-jason · 2017-09-01T06:31:51Z

@mccxj plz resolve conflicts.

XuHuaiyu · 2017-09-01T08:47:21Z

util/types/time.go

 	}

 	if err != nil {
 		return ZeroDuration, errors.Trace(err)
 	}
+	// Invalid TIME values are converted to '00:00:00'


add . at the end of this comment

XuHuaiyu · 2017-09-01T08:48:22Z

PTAL @winkyao

sre-bot · 2017-09-01T13:11:39Z

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.

iamxy · 2017-09-04T10:08:00Z

/ok-to-test

zz-jason · 2017-09-13T07:46:54Z

expression/builtin_cast.go

@@ -816,11 +816,18 @@ type builtinCastDecimalAsDurationSig struct {
 }

 func (b *builtinCastDecimalAsDurationSig) evalDuration(row []types.Datum) (res types.Duration, isNull bool, err error) {
-	val, isNull, err := b.args[0].EvalDecimal(row, b.getCtx().GetSessionVars().StmtCtx)
+	sc := b.getCtx().GetSessionVars().StmtCtx
+	val, isNull, err := b.args[0].EvalDecimal(row, sc)
 	if isNull || err != nil {
 		return res, false, errors.Trace(err)


should return res, true, errors.Trace(err) ?

zz-jason · 2017-09-13T07:55:45Z

expression/builtin_cast.go

@@ -958,6 +972,9 @@ func (b *builtinCastStringAsDurationSig) evalDuration(row []types.Datum) (res ty
 	res, err = types.ParseDuration(val, b.tp.Decimal)
 	if types.ErrTruncatedWrongVal.Equal(err) {
 		err = sc.HandleTruncate(err)
+		if res == types.ZeroDuration {


we can trust the result of types.ParseDuration(val, b.tp.Decimal) only if err == nil, which means we should move res == types.ZeroDuration condition outside the err check statement, that is:

res, err = types.ParseDuration(val, b.tp.Decimal) if err != nil { if types.ErrTruncatedWrongVal.Equal(err) { ... } ... } else if res == types.ZeroDuration { ... }

zz-jason · 2017-09-18T09:04:18Z

@mccxj any update ?

zz-jason · 2017-09-22T08:37:54Z

@mccxj ci failed.

mccxj · 2017-09-22T16:43:14Z

@zz-jason PTAL

zz-jason · 2017-09-28T05:26:27Z

expression/builtin_time.go

@@ -4687,7 +4687,7 @@ type builtinUTCTimeWithoutArgSig struct {
 // See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_utc-time
 func (b *builtinUTCTimeWithoutArgSig) evalDuration(row []types.Datum) (types.Duration, bool, error) {
 	// the types.ParseDuration here would never fail, so the err returned can be ignored.
-	v, _ := types.ParseDuration(time.Now().UTC().Format(types.TimeFormat), 0)
+	v, _, _ := types.ParseDuration(time.Now().UTC().Format(types.TimeFormat), 0)


why ignore isNull and err returned by types.ParseDuration ?

@zz-jason Input parameters can be predicted, so ignore is safe, just like the origin comments said.
But the modifications are minor, and I can modify it.

zz-jason · 2017-10-09T13:35:38Z

expression/builtin_cast.go

@@ -726,8 +726,7 @@ func (b *builtinCastRealAsDurationSig) evalDuration(row []types.Datum) (res type
 	if isNull || err != nil {
 		return res, isNull, errors.Trace(err)
 	}
-	res, err = types.ParseDuration(strconv.FormatFloat(val, 'f', -1, 64), b.tp.Decimal)
-	return res, false, errors.Trace(err)
+	return types.ParseDuration(strconv.FormatFloat(val, 'f', -1, 64), b.tp.Decimal)


res, err = types.ParseDuration(strconv.FormatFloat(val, 'f', -1, 64), b.tp.Decimal) return res, err != nil, errors.Trace(err)

I modified func ParseDuration to return isNull param, just return will be fine.

But we still should need to Trace the error when it is not nil.

zz-jason · 2017-10-09T13:36:44Z

expression/builtin_cast.go

+	if types.ErrTruncatedWrongVal.Equal(err) {
+		err = sc.HandleTruncate(err)
+	}
+	return


return res, isNull || err != nil, errors.Trace(err)

sc.HandleTruncate check is needed to deal with SQLMODE.

zz-jason · 2017-10-09T13:37:00Z

expression/builtin_cast.go

 	if types.ErrTruncatedWrongVal.Equal(err) {
 		err = sc.HandleTruncate(err)
 	}
-	return res, false, errors.Trace(err)
+	return


zz-jason · 2017-10-09T13:38:06Z

expression/builtin_cast.go

@@ -1287,7 +1290,7 @@ func (b *builtinCastJSONAsDurationSig) evalDuration(row []types.Datum) (res type
 	if err != nil {
 		return res, false, errors.Trace(err)
 	}
-	res, err = types.ParseDuration(s, b.tp.Decimal)
+	res, isNull, err = types.ParseDuration(s, b.tp.Decimal)
 	if types.ErrTruncatedWrongVal.Equal(err) {
 		err = sc.HandleTruncate(err)
 	}


return res, isNull || err != nil, errors.Trace(err)

zz-jason · 2017-10-09T13:39:17Z

expression/builtin_time.go

 	if types.ErrTruncatedWrongVal.Equal(err) {
 		err = sc.HandleTruncate(err)
 	}
-	return res, isNull, errors.Trace(err)
+	return


zz-jason · 2017-10-09T13:44:31Z

server/util_test.go

@@ -44,7 +44,7 @@ func (s *testUtilSuite) TestDumpBinaryTime(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(d, DeepEquals, []byte{4, 1, 0, 1, 1})

-	myDuration, err := types.ParseDuration("0000-00-00 00:00:00.0000000", 6)
+	myDuration, _, err := types.ParseDuration("0000-00-00 00:00:00.0000000", 6)


zz-jason · 2017-10-09T13:44:37Z

server/util_test.go

@@ -109,7 +109,7 @@ func (s *testUtilSuite) TestDumpTextValue(c *C) {
 	c.Assert(err, IsNil)
 	c.Assert(string(bs), Equals, "2017-01-06 00:00:00")

-	duration, err := types.ParseDuration("11:30:45", 0)
+	duration, _, err := types.ParseDuration("11:30:45", 0)


zz-jason · 2017-10-09T13:45:36Z

tablecodec/tablecodec_test.go

@@ -153,7 +153,7 @@ func (s *testTableCodecSuite) TestTimeCodec(c *C) {
 	ts, err := types.ParseTimestamp("2016-06-23 11:30:45")
 	c.Assert(err, IsNil)
 	row[2] = types.NewDatum(ts)
-	du, err := types.ParseDuration("12:59:59.999999", 6)
+	du, _, err := types.ParseDuration("12:59:59.999999", 6)


zz-jason · 2017-10-09T13:46:44Z

util/codec/codec_test.go

@@ -509,7 +509,7 @@ func parseTime(c *C, s string) types.Time {
 }

 func parseDuration(c *C, s string) types.Duration {
-	m, err := types.ParseDuration(s, types.DefaultFsp)
+	m, _, err := types.ParseDuration(s, types.DefaultFsp)


zz-jason · 2017-10-09T13:57:15Z

util/types/time.go

@@ -949,7 +949,7 @@ func (d Duration) MicroSecond() int {
 // ParseDuration parses the time form a formatted string with a fractional seconds part,
 // returns the duration type Time value.
 // See http://dev.mysql.com/doc/refman/5.7/en/fractional-seconds.html
-func ParseDuration(str string, fsp int) (Duration, error) {
+func ParseDuration(str string, fsp int) (Duration, bool, error) {


the second return value is true only when the returned error is ErrTruncatedWrongVal, can we keep the return values to (Duration, error), and check ErrTruncatedWrongVal outside ?

@zz-jason If I check errors inside func ParseDuration, then I needs StatementContext to deal with SQLMODE.
Any suggestions?

No, I mean maybe we don't need to check the error inside function ParseDuration(), you can just return the error and let the function caller to handle it(ignore or throw it).

tiancaiamao · 2017-10-16T05:41:31Z

Any update? @mccxj

zz-jason · 2017-10-26T10:53:53Z

Hi @mccxj, plz resolve conflicts and take a look at the comments.

…eds to be considered NULL

mccxj · 2017-11-15T06:22:26Z

@zz-jason I reverted to the original modification logic. PTAL.

tiancaiamao · 2017-11-20T07:13:30Z

/run-all-tests

tiancaiamao · 2017-11-20T07:51:57Z

LGTM

winoros · 2017-11-20T12:03:55Z

@XuHuaiyu PTAL

tiancaiamao · 2017-11-22T06:09:33Z

PTAL @XuHuaiyu @zz-jason

zz-jason

LGTM

winoros · 2017-11-22T07:01:35Z

/run-all-tests

zz-jason reviewed Aug 27, 2017

View reviewed changes

jackysp added the contribution This PR is from a community contributor. label Aug 28, 2017

zz-jason reviewed Aug 30, 2017

View reviewed changes

mccxj force-pushed the patch-issue-4324 branch from 2d8d100 to 66b72f3 Compare August 30, 2017 03:08

zz-jason reviewed Aug 30, 2017

View reviewed changes

mccxj force-pushed the patch-issue-4324 branch from 66b72f3 to e752569 Compare September 1, 2017 08:01

XuHuaiyu reviewed Sep 1, 2017

View reviewed changes

mccxj force-pushed the patch-issue-4324 branch from 7b81309 to f3ea756 Compare September 1, 2017 13:49

zz-jason reviewed Sep 13, 2017

View reviewed changes

mccxj force-pushed the patch-issue-4324 branch from 6cd162a to 4a8147c Compare September 14, 2017 13:05

mccxj force-pushed the patch-issue-4324 branch from 4a8147c to 30305d7 Compare September 20, 2017 07:35

mccxj force-pushed the patch-issue-4324 branch from 30305d7 to e7c124b Compare September 22, 2017 15:51

zz-jason reviewed Sep 28, 2017

View reviewed changes

mccxj force-pushed the patch-issue-4324 branch 2 times, most recently from 8bd8e51 to 819c89c Compare October 2, 2017 06:41

zz-jason reviewed Oct 9, 2017

View reviewed changes

zz-jason added this to the 1.1 milestone Oct 28, 2017

zimulala added the conflicting label Nov 8, 2017

mccxj added 2 commits November 15, 2017 12:45

add test case

e3cd797

revert to first commit, ZeroDuration of error ErrTruncatedWrongVal ne…

4960dbe

…eds to be considered NULL

mccxj force-pushed the patch-issue-4324 branch from b5940ce to 4960dbe Compare November 15, 2017 05:06

Merge branch 'master' into patch-issue-4324

12f732c

mccxj changed the title ~~types: fix invalid time cast bug. #4324~~ types: fix invalid time cast bug Nov 15, 2017

tiancaiamao added status/LGT1 Indicates that a PR has LGTM 1. and removed conflicting labels Nov 20, 2017

zz-jason approved these changes Nov 22, 2017

View reviewed changes

Merge branch 'master' into patch-issue-4324

5c1331e

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 22, 2017

winoros merged commit 8892bdf into pingcap:master Nov 22, 2017

mccxj deleted the patch-issue-4324 branch November 22, 2017 07:41

tiancaiamao mentioned this pull request Nov 23, 2017

types: fix parse duration when fsp round overflow 60 seconds #5199

Merged

types: fix invalid time cast bug #4338

types: fix invalid time cast bug #4338

Conversation

mccxj commented Aug 27, 2017

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

zz-jason Aug 30, 2017 • edited Loading

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

winkyao Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

mccxj Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Sep 1, 2017

sre-bot commented Sep 1, 2017

iamxy commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Sep 18, 2017

zz-jason commented Sep 22, 2017

mccxj commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccxj Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mccxj Oct 26, 2017 • edited Loading

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

tiancaiamao commented Oct 16, 2017

zz-jason commented Oct 26, 2017

mccxj commented Nov 15, 2017

tiancaiamao commented Nov 20, 2017

tiancaiamao commented Nov 20, 2017

winoros commented Nov 20, 2017

tiancaiamao commented Nov 22, 2017

zz-jason left a comment

Choose a reason for hiding this comment

winoros commented Nov 22, 2017

zz-jason Aug 30, 2017 •

edited

Loading

winkyao Sep 2, 2017 •

edited

Loading

mccxj Sep 6, 2017 •

edited

Loading

mccxj Oct 26, 2017 •

edited

Loading

mccxj Oct 26, 2017 •

edited

Loading