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.

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

Address issue #36118, allow "24:00" to be parsed. #37898

Merged
merged 1 commit into from May 30, 2019

Conversation

Projects
None yet
3 participants
@rohany
Copy link
Contributor

commented May 28, 2019

Addressed #36118.

Achieved by special casing when the input string represents the time 24:00, and adjusting some maximum values for timeofday objects.

@rohany rohany requested a review from solongordon May 28, 2019

@rohany rohany requested a review from cockroachdb/sql-rest-prs as a code owner May 28, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 28, 2019

This change is Reviewable

@solongordon
Copy link
Contributor

left a comment

Nice job getting this up so quickly!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)


pkg/sql/sem/tree/datum.go, line 1823 at r1 (raw file):

	// special case on 24:00 and 24:00:00 as the parser
	// does not handle these correctly.
	if s == "24:00" || s == "24:00:00" {

We're getting into edge cases here but what about "24:00:00.000"? Might need a regular expression to catch all the cases unfortunately.


pkg/sql/sem/tree/datum.go, line 1824 at r1 (raw file):

	// does not handle these correctly.
	if s == "24:00" || s == "24:00:00" {
		return MakeDTime(timeofday.New(24, 0, 0, 0)), nil

timeofday.Max should work here


pkg/util/timeofday/time_of_day.go, line 72 at r1 (raw file):

	// 24:00 is a valid time, so differentiate it from 00:00
	if i == microsecondsPerDay {
		return TimeOfDay(i)

Nit: Could just return Max here.


pkg/util/timeofday/time_of_day.go, line 119 at r1 (raw file):

// Hour returns the hour specified by t, in the range [0, 24].
func (t TimeOfDay) Hour() int {
	if int64(t) == microsecondsPerDay {

Alternatively: if t == Max


pkg/util/timeofday/time_of_day.go, line 120 at r1 (raw file):

func (t TimeOfDay) Hour() int {
	if int64(t) == microsecondsPerDay {
		return int(int64(t) / microsecondsPerHour)

How about just return 24? No need to do the math. (Or maybe return hoursPerDay to avoid the magic number.)

@solongordon
Copy link
Contributor

left a comment

Great, getting close! I added a few more thoughts. Also, would you please change the commit and PR messages to match our standard? The subject should indicate the top level package affected, like sql: allow 24:00 to be parsed. The body should be wrapped at 72 characters. And rather than referencing the issue number in the subject, you can put Fixes #37898 as a separate line at the end.

More details (maybe too many) here: https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany)


pkg/sql/logictest/testdata/logic_test/time, line 32 at r4 (raw file):


statement error could not parse
SELECT ('24:00:01'::TIME);

Nit: Unnecessary parens


pkg/sql/logictest/testdata/logic_test/time, line 35 at r4 (raw file):


statement error could not parse
SELECT ('24:00:00.001'::TIME);

Nit: Unnecessary parens


pkg/sql/sem/tree/datum.go, line 1823 at r1 (raw file):

Previously, solongordon (Solon) wrote…

We're getting into edge cases here but what about "24:00:00.000"? Might need a regular expression to catch all the cases unfortunately.

Thanks for the regex. Couple more requests here:

  • I think we should support any number of trailing zeroes, not just up to 6.
  • Should probably add a ^ anchor at the beginning. (Seems like 124:00 would get parsed to 24:00 as it stands.)
  • For efficiency we should probably compile the regex once up front as we discussed.
  • Could you add a few tests in datum_test.go to make sure we're parsing the various forms correctly?

pkg/util/timeofday/time_of_day.go, line 56 at r4 (raw file):

	micros := time.Duration(micro) * time.Microsecond
	sum := int64((hours + minutes + seconds + micros) / time.Microsecond)
	return FromInt(sum)

Nit: Looks like this changed for no reason.

@solongordon
Copy link
Contributor

left a comment

:lgtm_strong: One nit about the tests which you can fix if you want.

By the way, it's helpful if when you address review comments you reply to the comments with "Done." Otherwise the reviewer doesn't get notified that you fixed them.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany)


pkg/sql/sem/tree/datum_test.go, line 115 at r6 (raw file):

			`'00:00:00'`, `'23:59:59.999999'`},
		{`'24:00:00.000000':::time`, `'23:59:59.999999'`, `'00:00:00.000001'`,
			`'00:00:00'`, `'23:59:59.999999'`},

Nit: I think it would make more sense to put these tests in TestParseDTime rather than TestDatumOrdering. Or maybe keep the first one here to verify the ordering and put the rest in the parse test.

@solongordon
Copy link
Contributor

left a comment

Oh, also I would suggest squashing your six commits into one before merging.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany)

sql: Allow 24:00 and variants to be parsed
Fixes: #36118

Release note (sql change): Add support for the time value 24:00 and variants.

@rohany rohany force-pushed the rohany:time_parsing_fix branch from b158b9b to 096c8e0 May 30, 2019

@rohany

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 30, 2019

Merge #37898
37898: Address issue #36118, allow "24:00" to be parsed. r=rohany a=rohany

Addressed #36118.

Achieved by special casing when the input string represents the time 24:00, and adjusting some maximum values for timeofday objects.

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig

This comment has been minimized.

Copy link

commented May 30, 2019

Build succeeded

@craig craig bot merged commit 096c8e0 into cockroachdb:master May 30, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.