util/types: re-implement check time related #2233

tiancaiamao · 2016-12-12T12:58:42Z

provide the check method for Time, and functions
checkTimestampType
checkDatetimeType
checkDateType

FromDate doesn't check time now, newTime is removed.

In one word, rewrite the check time related.

"121231113045.9999999" should parse to "2012-12-31 11:30:46.000000" but we get "2012-12-31 11:30:45.100000"

use Format to implement String(), so 00 month and day can be display

Conflicts: util/types/fsp.go util/types/time.go util/types/time_test.go

Conflicts: util/types/format_test.go util/types/fsp.go util/types/mytime.go util/types/time.go util/types/time_test.go

shenli · 2016-12-12T15:38:32Z

util/types/mytime.go

 	tm := gotime.Date(t.Year(), gotime.Month(t.Month()), t.Day(), t.Hour(), t.Minute(), t.Second(), t.Microsecond()*1000, gotime.Local)
-	return tm, errors.Trace(err)
+	year, month, day := tm.Date()


Please add comment for the following logic.

zimulala · 2016-12-13T02:28:22Z

util/types/time_test.go

@@ -406,7 +406,7 @@ func (s *testTimeSuite) TestParseTimeFromNum(c *C) {
 		ExpectDateValue      string
 	}{
 		{20101010111111, false, "2010-10-10 11:11:11", false, "2010-10-10 11:11:11", false, "2010-10-10"},
-		{2010101011111, true, zeroDatetimeStr, true, zeroDatetimeStr, true, zeroDateStr},
+		{2010101011111, false, "0201-01-01 01:11:11", true, zeroDatetimeStr, false, "0201-01-01"},


Why use 0201-01-01 01:11:11 instead of zeroDatetimeStr ?

I check MySQL's behavior, its result is "0201-01-01 01:11:11"...
There is a document say datetime range is '1000-01-01 00:00:00' to '9999-12-31 23:59:59', but it seems the implement doesn't obey it.

zimulala · 2016-12-13T02:42:41Z

util/types/mytime.go

+		microsec != t.Microsecond() {
+		err = ErrInvalidTimeFormat
+	}
+	return tm, err


Add err trace.

hanfei1991 · 2016-12-13T03:30:40Z

util/types/time.go

@@ -1047,7 +1041,11 @@ func ParseTime(str string, tp byte, fsp int) (Time, error) {
 		return Time{Time: ZeroTime, Type: tp}, errors.Trace(err)
 	}

-	return t.Convert(tp)
+	t.Type = tp
+	if err = t.check(); err != nil {


Why not use t.Convert?

Convert is now just a wrap around t.check() ....
set t.Type, check it's valid, that's all what Convert does.

Convert's semantic is not best fit for here.
Convert means I already have something, wants convert it to another,
but here I just want to new something, and check it's valid.

@hanfei1991

convert = new + check
here we already new, just need check

hanfei1991 · 2016-12-13T03:30:56Z

util/types/time.go

 	t.Fsp = fsp
-	return t.Convert(tp)
+	if err := t.check(); err != nil {


hanfei1991 · 2016-12-13T03:32:29Z

util/types/time.go

@@ -274,34 +263,38 @@ func (t Time) ConvertToDuration() (Duration, error) {
 // Compare returns an integer comparing the time instant t to o.
 // If t is after o, return 1, equal o, return 0, before o, return -1.
 func (t Time) Compare(o Time) int {
+	return compareTime(t.Time, o.Time)


Why change this?

Somewhere else need to compare TimeInternal, so I extract a compareTime function rather than
make Compare a method of Time.

hanfei1991 · 2016-12-13T03:36:07Z

util/types/time.go

-	if t.IsZero() {
-		return true
+func checkDateRange(t TimeInternal) error {
+	// if compareTime(t, FromDate(1000, 1, 1, 0, 0, 0, 0)) < 0 {


remove useless comments

coocood · 2016-12-13T08:32:17Z

util/types/time.go

 	}
+	if compareTime(t, FromDate(9999, 12, 31, 23, 59, 59, 999999)) > 0 {


Can this be extracted to a variable?

zimulala · 2016-12-13T03:37:23Z

util/types/mytime.go

+	hour, minute, second := tm.Clock()
+	microsec := tm.Nanosecond() / 1000
+	var err error
+	// This function will check the result, and return error if it's not the same with the origin input.


s/error/an error

zimulala · 2016-12-13T09:07:04Z

util/types/time.go

+	if second < 0 || second >= 60 {
+		return ErrInvalidTimeFormat
+	}
+	if hour == 0 && minute == 0 && second == 0 {


Remove this judgment.

hanfei1991 · 2016-12-13T08:45:05Z

util/types/mytime.go

@@ -85,8 +85,19 @@ func (t mysqlTime) ISOWeek() (int, int) {
 }

 func (t mysqlTime) GoTime() (gotime.Time, error) {
-	err := checkTime(int(t.year), int(t.month), int(t.day), int(t.hour), int(t.minute), int(t.second), int(t.microsecond))
+	// gotime.Time can't represent month 0 or day 0, date contains 0 would be converted to a nearest date,
+	// for example, 2006-12-00 00:00:00 would become 2015-11-30 23:59:59.


hanfei1991 · 2016-12-13T08:47:37Z

util/types/mytime.go

+	if year != t.Year() || int(month) != t.Month() || day != t.Day() ||
+		hour != t.Hour() || minute != t.Minute() || second != t.Second() ||
+		microsec != t.Microsecond() {
+		err = ErrInvalidTimeFormat


return tm, err directly ? and we needn't init var at line 94

zimulala · 2016-12-13T09:57:43Z

Rest LGTM

hanfei1991 · 2016-12-13T10:01:39Z

util/types/time.go

-		return true
+func checkDateType(t TimeInternal) error {
+	year, month, day := t.Year(), t.Month(), t.Day()
+	if year == 0 && month == 0 && day == 0 {


why not move this check into "checkDateRange"

they are different: checkDateRange check date in range 1000-01-01 to 9999-12-31
while here check whether the date is ZeroDate.

hanfei1991 · 2016-12-13T10:06:16Z

util/types/time.go

+}
+
+func checkTimestampType(t TimeInternal) error {
+	if t.Year() == 0 && t.Month() == 0 && t.Day() == 0 &&


Can we add a IsZero interface?

do you mean add IsZero to TimeInternal interface?

okay, I'll extract an IsZero function, but rather keep the TimeInternal a general interface.

hanfei1991 · 2016-12-13T11:11:35Z

LGTM

zimulala · 2016-12-14T02:02:39Z

LGTM

tiancaiamao added 12 commits December 6, 2016 18:28

*: types.TimeInternal GoTime method would return error

47ebaab

recover checkTime

9c03502

address comment

d1e394c

util/time: ToPackedUint would return error for invalid timestamp

3c103db

util/types: fix bug parseFrac overflow ignored during time Parse

6c964c9

"121231113045.9999999" should parse to "2012-12-31 11:30:46.000000" but we get "2012-12-31 11:30:45.100000"

fix and add more test

0fa8e1a

make golint happy

06331ee

util/types: provide time Format method

3031d2f

use Format to implement String(), so 00 month and day can be display

Merge branch 'master' into time

b9b54c9

Merge branch 'tiancaiamao/overflow' into time

57d3cbf

Conflicts: util/types/fsp.go util/types/time.go util/types/time_test.go

refact check method for Time

6f79941

Merge branch 'master' into time

0dfb6a1

Conflicts: util/types/format_test.go util/types/fsp.go util/types/mytime.go util/types/time.go util/types/time_test.go

shenli reviewed Dec 12, 2016

View reviewed changes

zimulala reviewed Dec 13, 2016

View reviewed changes

util/types/mytime.go

microsec != t.Microsecond() {

err = ErrInvalidTimeFormat

}

return tm, err

Copy link

Contributor

zimulala Dec 13, 2016

Choose a reason for hiding this comment

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

Add err trace.

address comment

a23fb69

hanfei1991 reviewed Dec 13, 2016

View reviewed changes

util/types/time.go

t.Fsp = fsp

return t.Convert(tp)

if err := t.check(); err != nil {

Copy link

Member

hanfei1991 Dec 13, 2016

Choose a reason for hiding this comment

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

ditto

hanfei1991 reviewed Dec 13, 2016

View reviewed changes

address comment

09b521f

coocood reviewed Dec 13, 2016

View reviewed changes

util/types/time.go

}

if compareTime(t, FromDate(9999, 12, 31, 23, 59, 59, 999999)) > 0 {

Copy link

Member

coocood Dec 13, 2016

Choose a reason for hiding this comment

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

Can this be extracted to a variable?

address comment

b002458

zimulala reviewed Dec 13, 2016

View reviewed changes

address comment

52e67b5

hanfei1991 reviewed Dec 13, 2016

View reviewed changes

remove unused checkTimeType

c36a514

address comment

31dce0b

hanfei1991 reviewed Dec 13, 2016

View reviewed changes

address comment

f0b208f

Merge branch 'master' into tiancaiamao/check-time

530195b

tiancaiamao merged commit 4741960 into master Dec 14, 2016

tiancaiamao deleted the tiancaiamao/check-time branch December 14, 2016 02:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/types: re-implement check time related #2233

util/types: re-implement check time related #2233

tiancaiamao commented Dec 12, 2016

shenli Dec 12, 2016

zimulala Dec 13, 2016

tiancaiamao Dec 13, 2016

zimulala Dec 13, 2016

hanfei1991 Dec 13, 2016

tiancaiamao Dec 13, 2016

tiancaiamao Dec 13, 2016

tiancaiamao Dec 13, 2016

hanfei1991 Dec 13, 2016

hanfei1991 Dec 13, 2016

tiancaiamao Dec 13, 2016

hanfei1991 Dec 13, 2016

coocood Dec 13, 2016

zimulala Dec 13, 2016

zimulala Dec 13, 2016

hanfei1991 Dec 13, 2016

hanfei1991 Dec 13, 2016

zimulala commented Dec 13, 2016

hanfei1991 Dec 13, 2016

tiancaiamao Dec 13, 2016

hanfei1991 Dec 13, 2016

tiancaiamao Dec 13, 2016

hanfei1991 Dec 13, 2016

tiancaiamao Dec 13, 2016

hanfei1991 commented Dec 13, 2016

zimulala commented Dec 14, 2016

		}
		if compareTime(t, FromDate(9999, 12, 31, 23, 59, 59, 999999)) > 0 {

util/types: re-implement check time related #2233

util/types: re-implement check time related #2233

Conversation

tiancaiamao commented Dec 12, 2016

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

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

zimulala commented Dec 13, 2016

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

hanfei1991 commented Dec 13, 2016

zimulala commented Dec 14, 2016