sql: Added TIMETZ column type and datum

[windchan7]

This is an implementation of the PostgreSQL TIMETZ type, which represents time
of day, with time zone. We store this as a struct wrapper around an int64
(TimeOfDay) and a *time.Location. Just like TIMESTAMPTZ, TIMETZ does not store
any time zone information internally. The time zone is for user display only.
In addition, the CURRENT_TIME built in is added.

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.

Closes #20944.

Release note (SQL change): Added TIMETZ column type and datum.

[CLAassistant]

All committers have signed the CLA.

[cockroach-teamcity]

This change is 

[BramGruneir]

It might be nice to split this into two commits.

1. that adds timetz
2. that adds current_time.

---

Reviewed 12 of 48 files at r1.
Review status: 12 of 48 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

pkg/sql/logictest/testdata/logic_test/timetz, line 5 at r1 (raw file):

# Note that the odd '0000-01-01 hh:mi:ss +0000 +0000' result format is an
# artifact of how pq displays TIMETZs.

It would be nice if you used subtests when splitting the sections.

---

Comments from Reviewable

[BramGruneir]

It might be nice to split this into two commits.

1. that adds timetz
2. that adds current_time.

Overall the change looks good, but I'll leave most of the logic and test coverage reviewing to @dt.

---

Reviewed 12 of 48 files at r1.
Review status: 12 of 48 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[justinj]

Can you also extend the builtins.asJSON function? I get there's nothing to indicate to you that this was necessary, sorry about that!

[dt]

Great stuff here! Also, wow, there is a lot involved in adding a new type these days

I haven't quite done a line-by-line review yet, but from a quick look, there were two things I was wondering about: the encoding choice, and logic test coverage.

For encoding, looking at the sqlbase parts, it looks like we're encoding in kv as a time aka timestamp. I think we just need a varint, like DTime uses, for the timeofday. We don't actually store locations in times anyway, so the time encoding isn't storing that for you -- and it shouldn't anyway since the location is supposed to be the session location (this is confusing, I know). Basically, I think it should look exactly like DTime, except if you look at where timestamp and timestamptz differ, like casts, you'd do the similar thing where time and timetz differ (usually applying or removing the ctx.Location).

Second, there's chunk of the datetime logic test file dedicated to looking at exactly how timestamp and timestamptz behavior differs, and i think we want to do something similar here. Specifically, I think we want with to try different values on either side of midnight, combined with different session offsets, with both time and timetz side by side, to see what is different. We'll want to include casts both directions, and maybe from other types as well (e.g. int to time vs int to timetz, etc). Those test cases were created by trying to think of all the combinations and then running the corresponding queries in postgres to be sure we didn't get the expected behavior cases wrong (which is easy to do with how confusing and unintuitive it is). Unfortunately this is really tedious (speaking from experience) but it was the only way we finally hammered out the right (well, pg-compatible) semantics.

---

Reviewed 15 of 48 files at r1.
Review status: 26 of 48 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[justinj]

If you get a chance, you should rebase on master, I added a handful of tests in #24537 which will require modifications to this PR, I think.

[windchan7]

@dt I've changed encoding to int, and expanded the test coverage. All logic tests are in pkg/sql/logictest/testdata/logic_test/timetz.

[dt]

Nice! One more nit on encoding part.

---

Reviewed 1 of 48 files at r1.
Review status: 25 of 48 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/sqlbase/table.go, line 567 at r2 (raw file):

	case *tree.DTimeTZ:
		if dir == encoding.Ascending {
			return encoding.EncodeVarintAscending(b, int64(timeofday.FromTime(t.ToTime().UTC()))), nil

I'm not clear on why this is different from the DTime case above it? can it be? if not, can we comment why?

---

pkg/sql/sqlbase/table.go, line 656 at r2 (raw file):

		return encoding.EncodeIntValue(appendTo, uint32(colID), int64(*t)), nil
	case *tree.DTimeTZ:
		return encoding.EncodeIntValue(appendTo, uint32(colID), int64(timeofday.FromTime(t.ToTime().UTC()))), nil

ditto above.

---

Comments from Reviewable

[windchan7]

Review status: 25 of 48 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/sql/sqlbase/table.go, line 567 at r2 (raw file):

Previously, dt (David Taylor) wrote…

I'm not clear on why this is different from the DTime case above it? can it be? if not, can we comment why?

I don't think we can.

It's mainly because DTimeTZ is not just a wrapper around int64. It also holds a pointer to time zone. So we have to translate DTimeTZ to time.Time with time zone first. Then call UTC() because we internally do not store any time zone information. Then we need to translate it to timeofday to do the time mod appropriately.

I will add the comments shortly.

---

pkg/sql/sqlbase/table.go, line 656 at r2 (raw file):

Previously, dt (David Taylor) wrote…

ditto above.

Done.

---

Comments from Reviewable

[dt]

---

Reviewed 10 of 48 files at r1, 1 of 7 files at r2.
Review status: 36 of 48 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/sql/sqlbase/table.go, line 567 at r2 (raw file):

Previously, windchan7 (Victor Chen) wrote…

I don't think we can.

It's mainly because DTimeTZ is not just a wrapper around int64. It also holds a pointer to time zone. So we have to translate DTimeTZ to time.Time with time zone first. Then call UTC() because we internally do not store any time zone information. Then we need to translate it to timeofday to do the time mod appropriately.

I will add the comments shortly.

Huh, okay. So f it works as is -- i.e. all the test here match postgres behavior and pass -- then I'm fine with it. I'm just trying to figure out why it is different: In my head, since we don't actually store a zone in KV it seems like the in-memory representation could be just a timeofday as well, with the session offset logic being handled where we cast, print, or wire encode it.

again though, if the current approach works,

---

Comments from Reviewable

[windchan7]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)