sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug #6919

mail2fish · 2018-06-27T06:10:55Z

What have you changed? (mandatory)

fix #6835

What are the type of the changes (mandatory)?

bug fixed

How has this PR been tested (mandatory)?

make dev

Does this PR affect documentation (docs/docs-cn) update? (optional)

Refer to a related PR or issue link (optional)

#6835

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot · 2018-06-27T06:10:59Z

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.

lysu · 2018-06-27T06:29:27Z

@mail2fish Thanks for your PR!

lysu · 2018-06-27T06:29:40Z

/run-all-tests

zz-jason · 2018-06-27T07:57:39Z

Review status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @mail2fish)

types/time.go, line 2321 at r1 (raw file):

}

var digitRegex = regexp.MustCompile("^[0-9]+")

It'e better to put these two regex patterns together to a code block and make some comment for each one.

types/time.go, line 2348 at r1 (raw file):

	if length > 2 {
		result = result[2:]

Since result is not used in the following execution, we can remove this line.

Comments from Reviewable

zhexuany · 2018-06-27T08:14:16Z

types/time.go

+	result := digitRegex.FindString(input) // 0..31
+	length := len(result)
+
+	if length > 2 {


is it better to add some comments here why do you only take the suffix after index 2?

zz-jason · 2018-06-27T09:03:34Z

Review status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @mail2fish and @zhexuany)

types/time.go, line 2357 at r2 (raw file):

	v, ok := parseDigits(input, length)

	if !ok || v >= 32 {

I think we should keep the origin v > 31, this check form is consistent with the validation check of month, which is v > 12 at line 2458

types/time.go, line 2447 at r2 (raw file):

func monthNumeric(t *MysqlTime, input string, ctx map[string]int) (string, bool) {

can we remove this empty line?

Comments from Reviewable

shenli · 2018-06-27T10:35:53Z

types/time.go

-	"%Y": yearNumericFourDigits,      // Year, numeric, four digits
+	"%b": abbreviatedMonth,      // Abbreviated month name (Jan..Dec)
+	"%c": monthNumeric,          // Month, numeric (0..12)
+	"%d": dayOfMonthNumeric,     // Day of the month, numeric (00..31)


Please update the comment.

Which the comment be needed to update?

update "(00..31)" to "(0..31)"

shenli · 2018-06-27T10:36:00Z

types/time.go

+	"%k": hour24Numeric,         // Hour (0..23)
+	"%l": hour12Numeric,         // Hour (1..12)
+	"%M": fullNameMonth,         // Month name (January..December)
+	"%m": monthNumeric,          // Month, numeric (00..12)


Please update the comment.

zz-jason · 2018-06-27T11:34:32Z

types/time.go

-	v, rem := parseTwoNumeric(input)
-	if len(rem) == len(input) || v > 12 {
-		return rem, false
+


please remove this empty line.

mail2fish · 2018-06-27T11:49:48Z

/run-all-tests

mail2fish · 2018-06-28T01:11:56Z

/run-all-tests

zz-jason · 2018-06-28T01:41:49Z

@mail2fish Please remove the empty line between the function declaration and the first code of that function, that empty line is meaningless and redundant.

shenli · 2018-06-28T02:33:13Z

types/time.go

+	"%k": hour24Numeric,         // Hour (0..23)
+	"%l": hour12Numeric,         // Hour (1..12)
+	"%M": fullNameMonth,         // Month name (January..December)
+	"%m": monthNumeric,          // Month, numeric (00..12)


Please update the comment.

shenli · 2018-06-28T02:35:59Z

types/time.go

-	return input[2:], true
-}
+// digitRegex: it was used to scan a variable-length monthly day or month in the string. Ex:  "01" or "1" or "30"
+var digitRegex = regexp.MustCompile("^[0-9]+")


^[0-9]{1,2} is suitable?

Yes, It's better

shenli · 2018-06-28T02:37:06Z

types/time.go

+		length = 2
+	}
+
+	v, ok := parseDigits(input, length)


For "20181120" the v would be 20?

shenli · 2018-06-28T02:37:58Z

types/time.go

 		return input, false
 	}
-	t.month = uint8(v)
-	return remain, true
+	t.day = uint8(v)


t.month = uint8(v) is a bug?

Yes, I think so.

mail2fish · 2018-06-28T04:11:02Z

/run-all-tests

zhexuany · 2018-06-28T04:53:14Z

types/time.go

+	result := digitRegex.FindString(input) // 1..12
+	length := len(result)
+
+	// Some time the input is a consecutive digital string. Ex: 20181120


You want to say e.g.?

Just forgot to remove those lines

shenli · 2018-06-28T06:31:06Z

/run-all-tests

shenli · 2018-06-28T08:08:03Z

LGTM

winoros

rest LGTM

winoros · 2018-06-28T10:11:33Z

types/time.go

-	"%T": time24Hour,                 // Time, 24-hour (hh:mm:ss)
-	"%Y": yearNumericFourDigits,      // Year, numeric, four digits
+	"%b": abbreviatedMonth,      // Abbreviated month name (Jan..Dec)
+	"%c": monthNumeric,          // Month, numeric (0..12)


These 0 should be 00?

from the https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_date-format

%c Month, numeric (0..12)

lysu added the status/all tests passed label Jun 27, 2018

mail2fish force-pushed the str_to_date branch from fb576c4 to cf37e04 Compare June 27, 2018 07:16

zz-jason added contribution This PR is from a community contributor. component/expression type/bugfix This PR fixes a bug. labels Jun 27, 2018

zhexuany reviewed Jun 27, 2018

View reviewed changes

mail2fish force-pushed the str_to_date branch from 33537b9 to 62573d9 Compare June 27, 2018 09:20

shenli reviewed Jun 27, 2018

View reviewed changes

zz-jason reviewed Jun 27, 2018

View reviewed changes

mail2fish added 6 commits June 28, 2018 09:09

sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug

60bb7c1

fix str_to_date() bug

b3a7213

fix str_to_date() bug

32688ef

fix str_to_date() bug

5e7066a

fix str_to_date() bug

33baf06

sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug

702ffac

mail2fish force-pushed the str_to_date branch from 0347cda to 702ffac Compare June 28, 2018 01:10

shenli reviewed Jun 28, 2018

View reviewed changes

optimize str_to_date

8b0850a

mail2fish force-pushed the str_to_date branch from a9db4bd to 8b0850a Compare June 28, 2018 04:09

zhexuany reviewed Jun 28, 2018

View reviewed changes

optimize str_to_date

0bd3bec

Merge branch 'master' into str_to_date

eab51e1

shenli added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 28, 2018

winoros reviewed Jun 28, 2018

View reviewed changes

winoros approved these changes Jun 28, 2018

View reviewed changes

Merge branch 'master' into str_to_date

c63fa87

winoros added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 28, 2018

zz-jason merged commit 39d00d3 into pingcap:master Jun 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug #6919

sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug #6919

mail2fish commented Jun 27, 2018 •

edited by zz-jason

Loading

sre-bot commented Jun 27, 2018

lysu commented Jun 27, 2018

lysu commented Jun 27, 2018

zz-jason commented Jun 27, 2018

zhexuany Jun 27, 2018

zz-jason commented Jun 27, 2018

shenli Jun 27, 2018

mail2fish Jun 27, 2018

zz-jason Jun 27, 2018

shenli Jun 27, 2018

shenli Jun 28, 2018

zz-jason Jun 27, 2018

mail2fish commented Jun 27, 2018

mail2fish commented Jun 28, 2018

zz-jason commented Jun 28, 2018

shenli Jun 28, 2018

shenli Jun 28, 2018

mail2fish Jun 28, 2018

shenli Jun 28, 2018

shenli Jun 28, 2018

mail2fish Jun 28, 2018

mail2fish commented Jun 28, 2018

zhexuany Jun 28, 2018

mail2fish Jun 28, 2018

shenli commented Jun 28, 2018

shenli commented Jun 28, 2018

winoros left a comment

winoros Jun 28, 2018

mail2fish Jun 28, 2018

sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug #6919

sqltostr: fix str_to_date('2018-6-14', '%Y-%m-%d') bug #6919

Conversation

mail2fish commented Jun 27, 2018 • edited by zz-jason Loading

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

Does this PR affect documentation (docs/docs-cn) update? (optional)

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

sre-bot commented Jun 27, 2018

lysu commented Jun 27, 2018

lysu commented Jun 27, 2018

zz-jason commented Jun 27, 2018

Choose a reason for hiding this comment

zz-jason commented Jun 27, 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

mail2fish commented Jun 27, 2018

mail2fish commented Jun 28, 2018

zz-jason commented Jun 28, 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

mail2fish commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shenli commented Jun 28, 2018

shenli commented Jun 28, 2018

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mail2fish commented Jun 27, 2018 •

edited by zz-jason

Loading