Add TIME column type and datum.

[solongordon]

This is an implementation of the PostgreSQL TIME type, which represents
time of day (no date). We store this as an int64 representing
microseconds since midnight UTC.

Note that this commit does not attempt to support any new time formats
beyond what we already support in the time portion of TIMESTAMP. This
means that many formats which Postgres accepts (04:05, 040506,
04:05 PM, allballs) are not yet handled.

We also do not support TIME WITH TIME ZONE, which the Postgres docs
describe as having "questionable usefulness."

Fixes #16490.

[CLAassistant]

All committers have signed the CLA.

[cockroach-teamcity]

This change is 

[knz]

Nice work! thanks for the hard work and sorry for the last-minute code shuffle.

I'd like to look at this too, Justin please let me know when you're seeing the dust settle. I don't want to disturb the conversation until then.

[solongordon]

Thanks @knz! Would be happy for your feedback.

---

Review status: 0 of 39 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[justinj]

Super solid first PR! Mostly just nit comments. If you add Fixes #<github issue number> to your commit message the relevant issue will get closed upon merge.

Just looking at this list:

• I notice that PG has current_time, but it evaluates to a TIME WITH TIME ZONE so I guess it's not applicable to us?
• It also has localtime which we probably want.
• Do we (and should we) support OVERLAPS with TIME arguments?
• Rebecca added DATE_TRUNC recently, not sure if there's anything that needs to be done to support TIME in it? The PG docs say that TIME arguments are automatically converted to timestamps.

Would you also mind adding a short jdbc test? I think it's a good idea to test stuff against jdbc specifically since it tends to do a lot more funky stuff with its pg_catalog introspection than other drivers.

---

Reviewed 39 of 39 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

pkg/sql/logictest/testdata/logic_test/time, line 187 at r1 (raw file):

  (ARRAY[]),
  (ARRAY['00:00:00']),
  (ARRAY['00:00:00', '12:00:00.000001'])

We also support constructing arrays in the form '{00:00:00}'::TIME[], could you add a test for that as well?

---

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

0000-01-01 12:00:00 +0000 UTC

# Casting

Is it also worth testing other types -> TIME? Say, '2017-11-08 23:20:52.712757+00:00':::TIMESTAMP::TIME?

---

pkg/sql/logictest/testdata/logic_test/time, line 213 at r2 (raw file):

43200

query error extract\(\): unsupported timespan: day

We don't do it everywhere but here as well I think it's a good idea to annotate these expects with the expected pgcode: query error pgerror <code> ...

---

pkg/sql/parser/builtins.go, line 1382 at r1 (raw file):

			fn: func(ctx *EvalContext, args Datums) (Datum, error) {
				fromTime := args[1].(*DTime)
				timeSpan := strings.ToLower(string(MustBeDString(args[0])))

Hm, I hadn't noticed we do this elsewhere, but in the worst case this could result in a bunch of extra allocations (although I think strings.ToLower doesn't reallocate if the input string is already lowercase). A fix for that is definitely out of scope for this PR, but since extract has its wonky syntax I wonder if we could just lowercase these strings ahead of time in the parser 🤔 (as far as I can tell we don't do that already). That would at least solve the case where they're known at parse-time, which I expect would be the common case.

---

pkg/sql/parser/expr.go, line 1117 at r1 (raw file):

	bytesCastTypes     = []types.T{types.Null, types.String, types.FamCollatedString, types.Bytes, types.UUID}
	dateCastTypes      = []types.T{types.Null, types.String, types.FamCollatedString, types.Date, types.Timestamp, types.TimestampTZ, types.Int}
	timeCastTypes      = []types.T{types.Null, types.String, types.FamCollatedString, types.Time}

Do we have any tests for casting from a collated string? I don't expect that it would have any issues, but it would be good to test that case.

---

pkg/sql/parser/parse_test.go, line 506 at r2 (raw file):

		{`SELECT DECIMAL 'foo'`},
		{`SELECT DATE 'foo'`},
		{`SELECT TIME 'foo'`},

Do we test that this syntax has the expected semantics anywhere?

---

pkg/sql/pgwire/types.go, line 619 at r1 (raw file):

			d, err := parser.ParseDTime(string(b))
			if err != nil {
				return nil, errors.Errorf("could not parse string %q as time", b)

We don't do it everywhere currently, but new errors should have a pg error code (pgerror.NewErrorf(...)). You can check what code Postgres sends by first running \set VERBOSITY verbose in psql, and then looking in pgwire/pgerror/codes.go to find the variable for it.

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r1 (raw file):

[

Personally I think the value-add of these binary tests is somewhat dubious, lol, but I guess it's good that we have the explicit binary format tested somewhere.

---

pkg/sql/sqlbase/table.go, line 1829 at r1 (raw file):

		return encoding.Time, nil
	case types.Time:
		return encoding.Int, nil

How come this uses encoding.Int and the other time types use encoding.Time?

---

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

// TimeOfDay represents a time of day (no date), stored as microseconds since
// midnight UTC.

Since we only have TIME WITHOUT TIME ZONE and not TIME WITH TIME ZONE is it meaningful that this is since midnight UTC (...or is it meaningful because of that? I can never keep those straight)?

---

pkg/util/timeofday/time_of_day.go, line 35 at r2 (raw file):

	Max = TimeOfDay(microsecondsPerDay - 1)

	format             = "15:04:05.999999"

this paradigm is still so weird to me

---

pkg/util/timeofday/time_of_day.go, line 37 at r2 (raw file):

	format             = "15:04:05.999999"
	microsecondsPerDay = int64(24 * time.Hour / time.Microsecond)
	nanosPerMicro      = 1000

Doesn't matter much since this is such a universal constant, but could even write this as time.Nanosecond / time.Microsecond I think?

---

pkg/util/timeofday/time_of_day.go, line 41 at r2 (raw file):

// NewTimeOfDay creates a TimeOfDay representing the specified time.
func NewTimeOfDay(hour int, min int, sec int, micro int) TimeOfDay {

nit: I don't care either way, but if you prefer something slightly shorter you can write this hour, min, sec, micro int

Should we consider asserting (probably returning an error) that the values all fall within the expected ranges? I'm not sure if that's desirable or not, this function might be more convenient if it doesn't do that.

---

pkg/util/timeofday/time_of_day_test.go, line 36 at r1 (raw file):

		fromTime, err := time.Parse(time.RFC3339Nano, td.s)
		if err != nil {
			panic(err)

t.Fatal(err) is pretty normal here. I might be wrong on this but I thiiink if you panic you get a less readable stacktrace than if you t.Fatal.

---

pkg/util/timeofday/time_of_day_test.go, line 56 at r1 (raw file):

		{Min, -1, Max},
	}
	for _, td := range testData {

It can be nice in cases like this to use a subtest:

t.Run("<test name>", func(t *testing.T) {
  ...
})

and then the failure is already annotated for you with the case that failed (you can also run t.Parallel() within there if the tests are slow, which can be nice, but I don't think matters here at all).

---

Comments from Reviewable

[nvanbenschoten]

Congrats on the first PR!

---

Reviewed 39 of 39 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

pkg/sql/parser/datum_test.go, line 22 at r2 (raw file):

	"time"

	"github.com/cockroachdb/cockroach/pkg/util/timeofday"

Move that down.

---

pkg/sql/parser/eval.go, line 2558 at r2 (raw file):

		case *DDate:
			res = NewDInt(DInt(int64(*v)))
		case *DTime:

For the cast from TIME to FLOAT and DECIMAL, I get the error cannot cast type time without time zone to real in Postgres. It also doesn't look like these are reflected in floatCastTypes or decimalCastTypes, so I don't think they can actually be used right now.

---

pkg/sql/parser/eval.go, line 2762 at r2 (raw file):

		case *DCollatedString:
			return ParseDTime(d.Contents)
		case *DInt:

When I try this in Postgres I get cannot cast type integer to time without time zone. Is there a reason we can avoid this error?

---

pkg/sql/parser/eval.go, line 2764 at r2 (raw file):

		case *DInt:
			return MakeDTime(timeofday.TimeOfDay(int64(*d))), nil
		case *DTime:

No cast from TIMESTAMP? I would imagine that would be one of the most useful and it looks like it's supported in Postgres.

---

pkg/sql/pgwire/binary_test.go, line 235 at r2 (raw file):

}

func TestBinaryUuid(t *testing.T) {

While you're here, do you mind moving these two tests above TestRandomBinaryDecimal?

---

pkg/sql/pgwire/types.go, line 423 at r2 (raw file):

// formatTime formats t into a format lib/pq understands, appending to the
// provided tmp buffer and reallocating if needed.

Add to this that " The function will then return the resulting buffer." like we have below.

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Personally I think the value-add of these binary tests is somewhat dubious, lol, but I guess it's good that we have the explicit binary format tested somewhere.

I disagree @justinj, they've caught a number of issues in the past for data types with more complicated binary representations like DECIMAL, DATE, and TIMEZONE. As long as these data files are being populated with binary output straight from Postgres, the tests are a nice safety net to have.

---

pkg/util/timeofday/time_of_day.go, line 37 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Doesn't matter much since this is such a universal constant, but could even write this as time.Nanosecond / time.Microsecond I think?

Or even better, time.Microsecond.Nanoseconds(). We might run into some const issues doing that though, so I'd go with Justin's suggestion.

---

pkg/util/timeofday/time_of_day.go, line 41 at r2 (raw file):

// NewTimeOfDay creates a TimeOfDay representing the specified time.
func NewTimeOfDay(hour int, min int, sec int, micro int) TimeOfDay {

nit, just call this New so that it doesn't stutter with the package name.

---

pkg/util/timeofday/time_of_day.go, line 58 at r2 (raw file):

}

// ToTime converts a TimeOfDay to a time.Time, using the Unix epoch as the date.

Should this be a method on TimeOfDay? What do you think?

---

pkg/util/timeofday/time_of_day.go, line 69 at r2 (raw file):

// Add adds a Duration to a TimeOfDay, wrapping into the next day if necessary.
func Add(t TimeOfDay, d duration.Duration) TimeOfDay {

Same here, I think these would be more idiomatic as methods. That's what time.Time does.

---

pkg/util/timeofday/time_of_day_test.go, line 36 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

t.Fatal(err) is pretty normal here. I might be wrong on this but I thiiink if you panic you get a less readable stacktrace than if you t.Fatal.

Yeah, t.Fatal is what we want. A panic will also stop other tests from running.

---

Comments from Reviewable

[lgo]

Awesome first PR :)! Made a few remarks.

---

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

pkg/sql/parser/eval.go, line 464 at r2 (raw file):

		},
		BinOp{
			LeftType:   types.Time,

There's another binary operator involving Time. date + time -> timestamp (from https://www.postgresql.org/docs/9.1/static/functions-datetime.html). Just want to make sure this is on your radar.

---

pkg/sql/parser/eval.go, line 2597 at r2 (raw file):

			return NewDFloat(DFloat(float64(*v))), nil
		case *DTime:
			return NewDFloat(DFloat(float64(*v))), nil

As a heads up, it seems postgres doesn't allow casting to float unless the time has a timezone. I'm not sure if this will change our decision on supporting time-of-day to float casting.

Furthermore, from observation, it seems like it's just outright broken for both cases, hah.

postgres=# select TIME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT';
  timezone
-------------
 20:38:40-05
(1 row)

postgres=# select (TIME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT')::FLOAT;
ERROR:  cannot cast type time with time zone to double precision
LINE 1: ...ME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT')::FLOAT;
                                                               ^

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r2 (raw file):

[

We use pkg/cmd/generate-binary/main.go to generate the testdata for us. You pop in the test cases and make sure postgres is running locally. Then the command will test the inputs against postgres to get the binary formats and make the test file, so you will want to do that here :).

---

Comments from Reviewable

[lgo]

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

pkg/sql/parser/eval.go, line 2597 at r2 (raw file):

Previously, lego (Joey Pereira) wrote…

As a heads up, it seems postgres doesn't allow casting to float unless the time has a timezone. I'm not sure if this will change our decision on supporting time-of-day to float casting.

Furthermore, from observation, it seems like it's just outright broken for both cases, hah.

postgres=# select TIME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT';
  timezone
-------------
 20:38:40-05
(1 row)

postgres=# select (TIME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT')::FLOAT;
ERROR:  cannot cast type time with time zone to double precision
LINE 1: ...ME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT')::FLOAT;
                                                               ^

Oof. Looks like you just can't cast time to float in postgres at all. The part of the error time with time zone just refers to the name of the time and not the fact that a time without a timezone works.

So you'll want to remove these casts. This helps hide the internal bits about what the actual int timeofday actually stores behind the scenes, so it makes sense why not to have these.

(and probably the same for decimal as nathan said)

---

Comments from Reviewable

[solongordon]

• Yeah, current_time doesn't seem to make sense without time zone.
• We don't support localtimestamp so it seemed consistent not to support localtime.
• We don't seem to support OVERLAPS for dates and timestamps so I ignored this.
• Good catch, I missed that DATE_TRUNC accepts times. Done.
• JDBC test added.

---

Review status: 26 of 44 files reviewed at latest revision, 27 unresolved discussions.

---

pkg/sql/logictest/testdata/logic_test/time, line 187 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

We also support constructing arrays in the form '{00:00:00}'::TIME[], could you add a test for that as well?

Done.

---

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

Previously, justinj (Justin Jaffray) wrote…

Is it also worth testing other types -> TIME? Say, '2017-11-08 23:20:52.712757+00:00':::TIMESTAMP::TIME?

Shoot, for some reason I thought Postgres didn't allow casting timestamp to time so didn't support it. But I just double-checked and it does. I'll add this.

Added the following casts:

• TIMESTAMP → TIME
• TIMESTAMPTZ → TIME
• INTERVAL → TIME
• TIME → INTERVAL

---

pkg/sql/logictest/testdata/logic_test/time, line 213 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

We don't do it everywhere but here as well I think it's a good idea to annotate these expects with the expected pgcode: query error pgerror <code> ...

Good to know. Done.

---

pkg/sql/parser/builtins.go, line 1382 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Hm, I hadn't noticed we do this elsewhere, but in the worst case this could result in a bunch of extra allocations (although I think strings.ToLower doesn't reallocate if the input string is already lowercase). A fix for that is definitely out of scope for this PR, but since extract has its wonky syntax I wonder if we could just lowercase these strings ahead of time in the parser 🤔 (as far as I can tell we don't do that already). That would at least solve the case where they're known at parse-time, which I expect would be the common case.

Should we make a separate issue to track this? One extra string allocation for a short string doesn't strike me as terribly wasteful, but not sure how sensitive we are to this stuff.

---

pkg/sql/parser/datum_test.go, line 22 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Move that down.

Done.

---

pkg/sql/parser/eval.go, line 464 at r2 (raw file):

Previously, lego (Joey Pereira) wrote…

There's another binary operator involving Time. date + time -> timestamp (from https://www.postgresql.org/docs/9.1/static/functions-datetime.html). Just want to make sure this is on your radar.

Missed that one! Thanks. I added date + time, time + date, and date - time.

---

pkg/sql/parser/eval.go, line 2558 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

For the cast from TIME to FLOAT and DECIMAL, I get the error cannot cast type time without time zone to real in Postgres. It also doesn't look like these are reflected in floatCastTypes or decimalCastTypes, so I don't think they can actually be used right now.

Yeah, meant to remove this. Thanks.

---

pkg/sql/parser/eval.go, line 2597 at r2 (raw file):

Previously, lego (Joey Pereira) wrote…

As a heads up, it seems postgres doesn't allow casting to float unless the time has a timezone. I'm not sure if this will change our decision on supporting time-of-day to float casting.

Furthermore, from observation, it seems like it's just outright broken for both cases, hah.

postgres=# select TIME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT';
  timezone
-------------
 20:38:40-05
(1 row)

postgres=# select (TIME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT')::FLOAT;
ERROR:  cannot cast type time with time zone to double precision
LINE 1: ...ME WITH TIME ZONE '20:38:40' AT TIME ZONE 'EST5EDT')::FLOAT;
                                                               ^

Yeah, meant to remove this. Thanks.

---

pkg/sql/parser/eval.go, line 2762 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

When I try this in Postgres I get cannot cast type integer to time without time zone. Is there a reason we can avoid this error?

Meant to remove this. For some reason we do allow this cast for timestamps and dates, which Postgres also does not.

---

pkg/sql/parser/eval.go, line 2764 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

No cast from TIMESTAMP? I would imagine that would be one of the most useful and it looks like it's supported in Postgres.

Yup, missed this. Done.

---

pkg/sql/parser/expr.go, line 1117 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Do we have any tests for casting from a collated string? I don't expect that it would have any issues, but it would be good to test that case.

Nope. Added a logic test.

---

pkg/sql/parser/parse_test.go, line 506 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Do we test that this syntax has the expected semantics anywhere?

Don't think so. Added a logic test.

---

pkg/sql/pgwire/binary_test.go, line 235 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

While you're here, do you mind moving these two tests above TestRandomBinaryDecimal?

Done.

---

pkg/sql/pgwire/types.go, line 619 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

We don't do it everywhere currently, but new errors should have a pg error code (pgerror.NewErrorf(...)). You can check what code Postgres sends by first running \set VERBOSITY verbose in psql, and then looking in pgwire/pgerror/codes.go to find the variable for it.

Ah ok. Is that also the case for protocol-level errors like these? Not sure the best way to force this in psql. Also it feels a little weird to fix this for just this error since there's a slew of other could not parse errors in this file. Should I go ahead and fix them all?

---

pkg/sql/pgwire/types.go, line 423 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Add to this that " The function will then return the resulting buffer." like we have below.

Done.

---

pkg/sql/sqlbase/table.go, line 1829 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

How come this uses encoding.Int and the other time types use encoding.Time?

We discussed in person and the Date one is wrong. It should be encoding.Int as well. Opened #19942 to track.

---

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

Previously, justinj (Justin Jaffray) wrote…

Since we only have TIME WITHOUT TIME ZONE and not TIME WITH TIME ZONE is it meaningful that this is since midnight UTC (...or is it meaningful because of that? I can never keep those straight)?

Oh, good point. Yeah now that you mention it, it's irrespective of time zone. I'll fix my comments.

---

pkg/util/timeofday/time_of_day.go, line 37 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Or even better, time.Microsecond.Nanoseconds(). We might run into some const issues doing that though, so I'd go with Justin's suggestion.

Done, though this meant casting to int64 in a few places since it's no longer an untyped int. Worth it?

---

pkg/util/timeofday/time_of_day.go, line 41 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: I don't care either way, but if you prefer something slightly shorter you can write this hour, min, sec, micro int

Should we consider asserting (probably returning an error) that the values all fall within the expected ranges? I'm not sure if that's desirable or not, this function might be more convenient if it doesn't do that.

Yeah, given that it works fine if you exceed the ranges I'm tempted to keep it as-is.

---

pkg/util/timeofday/time_of_day.go, line 41 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit, just call this New so that it doesn't stutter with the package name.

Done.

---

pkg/util/timeofday/time_of_day.go, line 58 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this be a method on TimeOfDay? What do you think?

Yeah, I wondered about this. Happy to go with methods. Done.

---

pkg/util/timeofday/time_of_day.go, line 69 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Same here, I think these would be more idiomatic as methods. That's what time.Time does.

Done.

---

pkg/util/timeofday/time_of_day_test.go, line 36 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

t.Fatal(err) is pretty normal here. I might be wrong on this but I thiiink if you panic you get a less readable stacktrace than if you t.Fatal.

Done.

---

pkg/util/timeofday/time_of_day_test.go, line 56 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

It can be nice in cases like this to use a subtest:

t.Run("<test name>", func(t *testing.T) {
  ...
})

and then the failure is already annotated for you with the case that failed (you can also run t.Parallel() within there if the tests are slow, which can be nice, but I don't think matters here at all).

Ah cool, I changed it up. Let me know if it's what you had in mind.

---

Comments from Reviewable

[solongordon]

Review status: 26 of 44 files reviewed at latest revision, 20 unresolved discussions, some commit checks pending.

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r2 (raw file):

Previously, lego (Joey Pereira) wrote…

We use pkg/cmd/generate-binary/main.go to generate the testdata for us. You pop in the test cases and make sure postgres is running locally. Then the command will test the inputs against postgres to get the binary formats and make the test file, so you will want to do that here :).

I think I tried running that and got a cryptic error, so I ended up exporting data from Postgres in binary format via COPY TO and using that as my expected values. I should go back and try out the tool again though.

---

Comments from Reviewable

[justinj]

Ok! Can you open an issue to improve our coverage of OVERLAPS?

This modulo some small comments! If anyone else has any comments feel free to bring them up cc @knz @jordanlewis

---

Reviewed 18 of 18 files at r3.
Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks pending.

---

pkg/sql/logictest/testdata/logic_test/time, line 213 at r2 (raw file):

Previously, solongordon wrote…

Good to know. Done.

Sorry about leading you astray on the directive name!

---

pkg/sql/parser/builtins.go, line 1382 at r1 (raw file):

Previously, solongordon wrote…

Should we make a separate issue to track this? One extra string allocation for a short string doesn't strike me as terribly wasteful, but not sure how sensitive we are to this stuff.

Yeah I think it's probably not too big of a deal. If you have a large result set that you're calling this function on it could result in doing the allocation unnecessarily for every row. I'll leave it up to @knz to share his thoughts on if doing that preprocessing is worthwhile.

---

pkg/sql/parser/builtins.go, line 1440 at r3 (raw file):

				if err != nil {
					return nil, err
				} else {

nit: it's idiomatic to just omit the else and deindent the other block.

---

pkg/sql/parser/builtins.go, line 1460 at r3 (raw file):

				if err != nil {
					return nil, err
				} else {

ditto

---

pkg/sql/parser/builtins.go, line 1479 at r3 (raw file):

				if err != nil {
					return nil, err
				} else {

ditto

---

pkg/sql/parser/builtins.go, line 1497 at r3 (raw file):

				if err != nil {
					return nil, err
				} else {

ditto

---

pkg/sql/pgwire/types.go, line 619 at r1 (raw file):

Previously, solongordon wrote…

Ah ok. Is that also the case for protocol-level errors like these? Not sure the best way to force this in psql. Also it feels a little weird to fix this for just this error since there's a slew of other could not parse errors in this file. Should I go ahead and fix them all?

Ah, I see, yeah, sort of a pain to generate this particular case in psql... I guess just leave it for now until we do the eventual purging from pgwire (parser already lints these away).

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I disagree @justinj, they've caught a number of issues in the past for data types with more complicated binary representations like DECIMAL, DATE, and TIMEZONE. As long as these data files are being populated with binary output straight from Postgres, the tests are a nice safety net to have.

I guess I don't see the value these provide over one or more driver tests, which are more legible and test the same thing? But I also didn't realize they were auto-generated, that seems useful then!

---

pkg/util/timeofday/time_of_day.go, line 37 at r2 (raw file):

Previously, solongordon wrote…

Done, though this meant casting to int64 in a few places since it's no longer an untyped int. Worth it?

Hm, I have no strong opinion! Up to you.

---

pkg/util/timeofday/time_of_day_test.go, line 56 at r1 (raw file):

Previously, solongordon wrote…

Ah cool, I changed it up. Let me know if it's what you had in mind.

Yup looks good to me!

---

Comments from Reviewable

[solongordon]

Review status: 43 of 44 files reviewed at latest revision, 22 unresolved discussions.

---

pkg/sql/parser/builtins.go, line 1440 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: it's idiomatic to just omit the else and deindent the other block.

Done.

---

pkg/sql/parser/builtins.go, line 1460 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

ditto

Done.

---

pkg/sql/parser/builtins.go, line 1479 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

ditto

Done.

---

pkg/sql/parser/builtins.go, line 1497 at r3 (raw file):

Previously, justinj (Justin Jaffray) wrote…

ditto

Done.

---

Comments from Reviewable

[lgo]

Review status: 43 of 44 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r2 (raw file):

Previously, solongordon wrote…

I think I tried running that and got a cryptic error, so I ended up exporting data from Postgres in binary format via COPY TO and using that as my expected values. I should go back and try out the tool again though.

Yea it's no longer working and there haven't been any changes on our end. cc @mjibson
(not a big deal, but weird that all of the sudden it stopped)

panic: EOF: ""

goroutine 1 [running]:
main.makeEncodingFunc.func1(0x1130b59, 0xe, 0x112f7a3, 0x7, 0x0, 0x0, 0x0, 0x0, 0x0)
	/Users/joey/cockroach/pkg/cmd/generate-binary/main.go:249 +0xd2b
main.main()
	/Users/joey/cockroach/pkg/cmd/generate-binary/main.go:65 +0x292
exit status 2

---

Comments from Reviewable

[solongordon]

Review status: 41 of 45 files reviewed at latest revision, 21 unresolved discussions, some commit checks pending.

---

pkg/sql/pgwire/testdata/time_test.json, line 1 at r2 (raw file):

Previously, lego (Joey Pereira) wrote…

Yea it's no longer working and there haven't been any changes on our end. cc @mjibson
(not a big deal, but weird that all of the sudden it stopped)

panic: EOF: ""

goroutine 1 [running]:
main.makeEncodingFunc.func1(0x1130b59, 0xe, 0x112f7a3, 0x7, 0x0, 0x0, 0x0, 0x0, 0x0)
	/Users/joey/cockroach/pkg/cmd/generate-binary/main.go:249 +0xd2b
main.main()
	/Users/joey/cockroach/pkg/cmd/generate-binary/main.go:65 +0x292
exit status 2

I won't try and get it working as part of this PR but at @LEGO's suggestion I added my test cases to the script.

---

Comments from Reviewable

[knz]

Reviewed 22 of 39 files at r1, 2 of 4 files at r2, 17 of 18 files at r3, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

pkg/sql/copy_in_test.go, line 171 at r4 (raw file):

	types := []sqlbase.ColumnType_SemanticType{
		sqlbase.ColumnType_INT,
		sqlbase.ColumnType_INTERVAL,

If INT and INTERVAL were indeed missing, I'd argue this should be populated in a separate commit.

---

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

SELECT '2017-01-01':::DATE - '12:00:00':::TIME
----
2016-12-31 12:00:00 +0000 +0000

I'm missing :: conversion tests between:

TIME and INT (both ways)
TIME and STRING (both ways)

---

pkg/sql/parser/builtins.go, line 1382 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Yeah I think it's probably not too big of a deal. If you have a large result set that you're calling this function on it could result in doing the allocation unnecessarily for every row. I'll leave it up to @knz to share his thoughts on if doing that preprocessing is worthwhile.

So the problem is that next to the SQL shorthand syntax EXTRACT(SECONDS FROM ...), where do lowercase things upfront, we also support the function syntax extract("seconds", ...) where the input string may not be normalized. So this here is necessary. Although arguably, we should implement this differently to avoid the normalization in the common case. Can you file an issue and cc me on it?

---

pkg/sql/parser/datum.go, line 1444 at r4 (raw file):

		return 1
	}
	var v DTime

simpler:

v, ok := other.(*DTime)
if !ok {
  panic(...)
}

---

pkg/sql/parser/eval.go, line 2762 at r2 (raw file):

Previously, solongordon wrote…

Meant to remove this. For some reason we do allow this cast for timestamps and dates, which Postgres also does not.

Yes we found it rather useful.

---

pkg/util/timeofday/time_of_day.go, line 35 at r2 (raw file):

Previously, justinj (Justin Jaffray) wrote…

this paradigm is still so weird to me

(it's garbage. garbage, I tell you!)

---

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

func (t TimeOfDay) String() string {
	return t.ToTime().Format(format)

That's also an expensive roundtrip via the time package.
This is probably measurably faster:

return fmt.Sprintf("%02d:%02d:%02d.%06d",
t/microsPerHour, (t/microsPerMinute)%24, (t/microsPerSecond)%60, t%1000000)

---

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

// FromTime constructs a TimeOfDay from a time.Time, ignoring the date portion.
func FromTime(t time.Time) TimeOfDay {
	return New(t.Hour(), t.Minute(), t.Second(), t.Nanosecond()/nanosPerMicro)

Calling these 4 methods on time.Time does the work of time.abs() 4 times, and that's rather expensive (check it out!)
This is probably measurably faster:

return (t.Unix()%secondsPerDay)*1000000 + t.Nanosecond()/nanosPerMicro

---

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

// Add adds a Duration to a TimeOfDay, wrapping into the next day if necessary.
func (t TimeOfDay) Add(d duration.Duration) TimeOfDay {
	return FromTime(duration.Add(t.ToTime(), d))

I do not like this Add method. If my reading of the code in Go's time is right, it is subject to leap day / leap second adjustments.
Even if that's not the case, the arithmetic around Duration's day/months fields is ... unwarranted here. Not to speak of the overhead of that roundtrip via time.

Do you really need adding a duration containing days/months?
If so, please split this method in two -- one called AddTimeDifference and one called AddArbitraryDuration. Document the latter to be expensive.

The following code is suitable for AddTimeDifference, or Add if you estimate that Add never needs to process durations with a non-zero day/month field:

return d.Nanos/nanosPerMicro + t

---

Comments from Reviewable

[knz]

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

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

Previously, knz (kena) wrote…

I do not like this Add method. If my reading of the code in Go's time is right, it is subject to leap day / leap second adjustments.
Even if that's not the case, the arithmetic around Duration's day/months fields is ... unwarranted here. Not to speak of the overhead of that roundtrip via time.

Do you really need adding a duration containing days/months?
If so, please split this method in two -- one called AddTimeDifference and one called AddArbitraryDuration. Document the latter to be expensive.

The following code is suitable for AddTimeDifference, or Add if you estimate that Add never needs to process durations with a non-zero day/month field:

return d.Nanos/nanosPerMicro + t

Please disregard my previous comment. I was not fully awake yet.

Of course the day and month components of a Duration never impact a time computation. So this method can always be simplified to this:

func (t TimeOfDay) Add(d duration.Duration) TimeOfDay {
 return d.Nanos/nanosPerMicro + t
}

---

Comments from Reviewable

[solongordon]

Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

pkg/sql/copy_in_test.go, line 171 at r4 (raw file):

Previously, knz (kena) wrote…

If INT and INTERVAL were indeed missing, I'd argue this should be populated in a separate commit.

They actually weren't missing, just handled separately from this types array, which made it confusing when I needed to index into the array. I did a minor refactor to make things clearer. Let me know if that makes sense.

---

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

Previously, knz (kena) wrote…

I'm missing :: conversion tests between:

TIME and INT (both ways)
TIME and STRING (both ways)

Will do, though INT is currently not supported. By the way, is SELECT '12:00:00'::TIME sufficient to test STRING to TIME conversion, or do I need to be more explicit?

---

pkg/sql/parser/builtins.go, line 1382 at r1 (raw file):

Previously, knz (kena) wrote…

So the problem is that next to the SQL shorthand syntax EXTRACT(SECONDS FROM ...), where do lowercase things upfront, we also support the function syntax extract("seconds", ...) where the input string may not be normalized. So this here is necessary. Although arguably, we should implement this differently to avoid the normalization in the common case. Can you file an issue and cc me on it?

Opened #19965.

---

pkg/sql/parser/eval.go, line 2762 at r2 (raw file):

Previously, knz (kena) wrote…

Yes we found it rather useful.

Would you suggest doing the same for TIME?

---

Comments from Reviewable

[solongordon]

Review status: 36 of 44 files reviewed at latest revision, 27 unresolved discussions.

---

pkg/sql/parser/datum.go, line 1444 at r4 (raw file):

Previously, knz (kena) wrote…

simpler:

v, ok := other.(*DTime)
if !ok {
  panic(...)
}

Done.

---

pkg/sql/parser/eval.go, line 2762 at r2 (raw file):

Previously, solongordon wrote…

Would you suggest doing the same for TIME?

Followed up with @knz "in person". We'll hold off on adding these casts for TIME until the need arises.

---

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

Previously, knz (kena) wrote…

That's also an expensive roundtrip via the time package.
This is probably measurably faster:

return fmt.Sprintf("%02d:%02d:%02d.%06d",
t/microsPerHour, (t/microsPerMinute)%24, (t/microsPerSecond)%60, t%1000000)

Done. I ended up making Hour, Minute, etc. functions and reusing those in extract and date_trunc to avoid more time roundtrips.

---

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

Previously, knz (kena) wrote…

Calling these 4 methods on time.Time does the work of time.abs() 4 times, and that's rather expensive (check it out!)
This is probably measurably faster:

return (t.Unix()%secondsPerDay)*1000000 + t.Nanosecond()/nanosPerMicro

Done.

---

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

Previously, knz (kena) wrote…

Please disregard my previous comment. I was not fully awake yet.

Of course the day and month components of a Duration never impact a time computation. So this method can always be simplified to this:

func (t TimeOfDay) Add(d duration.Duration) TimeOfDay {
 return d.Nanos/nanosPerMicro + t
}

Good point! I had to add a bit of modular arithmetic but otherwise works great.

---

Comments from Reviewable

[knz]

Reviewed 48 of 50 files at r5.
Review status: 46 of 48 files reviewed at latest revision, 26 unresolved discussions, all commit checks successful.

---

pkg/sql/copy_in_test.go, line 171 at r4 (raw file):

Previously, solongordon wrote…

They actually weren't missing, just handled separately from this types array, which made it confusing when I needed to index into the array. I did a minor refactor to make things clearer. Let me know if that makes sense.

Yes it makes sense!

---

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

Previously, solongordon wrote…

Will do, though INT is currently not supported. By the way, is SELECT '12:00:00'::TIME sufficient to test STRING to TIME conversion, or do I need to be more explicit?

Ok given we're postponing conversions <-> INT.

---

pkg/util/timeofday/time_of_day.go, line 49 at r5 (raw file):

// normalize adjusts the underlying int64 value to fall between Min and Max.
// This is essentially just modding by microsecondsPerDay, with some extra
// finagling to avoid negative values.

Can you provide a few example input values with expected output value (or better: write a test).
Be sure to include values close to the minimum and maximum admissible values for the input parameter.

---

pkg/util/timeofday/time_of_day.go, line 74 at r5 (raw file):

// FromTime constructs a TimeOfDay from a time.Time, ignoring the date portion.
func FromTime(t time.Time) TimeOfDay {
	_, offset := t.Zone()

Add a comment to explain why this is necessary (and what fails otherwise).

---

pkg/util/timeofday/time_of_day.go, line 87 at r5 (raw file):

// Random generates a random TimeOfDay.
func Random(rng *rand.Rand) TimeOfDay {
	out := TimeOfDay(rng.Int63n(microsecondsPerDay))

return TimeOfDay ...

---

Comments from Reviewable

[knz]

Reviewed 2 of 50 files at r5.
Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[solongordon]

Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

pkg/util/timeofday/time_of_day.go, line 49 at r5 (raw file):

Previously, knz (kena) wrote…

Can you provide a few example input values with expected output value (or better: write a test).
Be sure to include values close to the minimum and maximum admissible values for the input parameter.

Simplified this a bit and made it more clear.

---

pkg/util/timeofday/time_of_day.go, line 74 at r5 (raw file):

Previously, knz (kena) wrote…

Add a comment to explain why this is necessary (and what fails otherwise).

Done.

---

pkg/util/timeofday/time_of_day.go, line 87 at r5 (raw file):

Previously, knz (kena) wrote…

return TimeOfDay ...

Done.

---

Comments from Reviewable