sql: add types, datum and encoding for TimeTZ #42481

otan · 2019-11-14T15:23:41Z

This PR adds the types to the type system to support TimeTZ, adding the
tree.Datum class to ensure it can be parsed and then also adds the
encoding for TimeTZ to be put in the database. This is the minimum
amount of work needed before any set of tests can pass - trying to keep
the PRs small but in the end did not succeed.

Changes per package (and recommended review order) are as follows:

sql/types: added the new TimeTZ type and type family.

pkg/util/timetz: introduce new TimeTZ type

Added a custom wrapper around timeofday and offset seconds, which form
follows postgres' implementation of TimeTzADT. Note that OffsetSecs
being negative / not what you expect is fully handled by all utility
functions from this interface.

pkg/sql/sem/tree: add Datum for DTimeTZ

A wrapper around TimeTZ for the tree expressions.
Also removed some magic constants I found whilst form following other
things.

pkg/util/timeutil/pgdate: remove usage of testutil.IsError

This causes a circular dependency on the new timetz type, and this was a
very straightforward way of going around it.

pkg/util/encoding: added encoding for DTimeTZ
Form following postgres, we store the timeofday and offset into the
database as 12 bytes - an 8 and 4 byte varint. To have sort order right,
and to form follow the btree encoding for TimeTZ, we use UTC time when
storing it for (En|De)codeTimeTZ[As|Des]cending.

pkg/sql/pgwire: added pgwire for DTimeTZ

pkg/sql/sqlbase: added necessary changes to encode TimeTZ data

ccl/changefeedccl: added encoding for ccl
Note we must use the string type here as we cannot encode two ints
nicely with avro, nor is one int enough to not lose data.

sql/parser: added parsing support for TimeTZ
I wanted to omit this in this PR but some of the automated tests
iterating over every time needs this. Note there are more bits to
implement which I will leave for a later PR.

pkg/cli: add ability to dump TimeTZ

sql/logictest/testdata/logic_test: added basic tests for TimeTZ
More to come, but here are some now since the parser is all hooked up.

As this is not the fully featured commit yet, I am omitting the release
note until the test suite is more fleshed out.

Release note: none

cockroach-teamcity · 2019-11-14T15:23:51Z

This change is

knz · 2019-11-14T16:26:55Z

I haven't reviewed yet (and I'll let Solon do the first pass anyway) but I'd like to commend you on the elaborate commit message.

Also you probably want to refer to #26097 in the PR description.

solongordon

Looks great in general! I left some mostly minor thoughts.

Reviewed 51 of 51 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

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

# structure underneath should still be correct.
query B
SELECT '24:00:00-1459' > '23:59:59-1459';

Looks like you're just comparing strings here, not TIMETZs. Better to do SELECT '24:00:00-1459':::TIMETZ > '23:59:59-1459':::TIMETZ or SELECT b > '23:59:59-1459' FROM timetz_test WHERE c = 4.

Also note that if it makes things less confusing, you can cast to string when you select from timetz_test in the queries below.

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

var (
	dMaxTimeTZOffsetSecs = int32((14*time.Hour + 59*time.Minute) / time.Second)

This value is mysterious to me. Add a comment?

Also, just from fooling around in a Postgres shell it seems like maybe 15:59 is the max it accepts, not 14:59?

Also it doesn't look like you currently prevent storing a larger offset than this. Though maybe you're planning to handle that in follow-up parsing work?

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

			desc:     "same DTimeTZ are equal",
			left:     NewDTimeTZFromOffset(timeofday.New(22, 0, 0, 0), sydneyTimeZone),
			right:    NewDTimeTZFromOffset(timeofday.New(22, 0, 0, 0), sydneyTimeZone),

🇦🇺

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

	// For example:
	//
	//   HH:MM:SS.sssss+-ZZ:ZZ

🔎 Is there an s missing?

pkg/util/encoding/encoding.go, line 955 at r1 (raw file):

func EncodeTimeTZAscending(b []byte, t timetz.TimeTZ) []byte {
	// Do not use TimeOfDay's add function, as it loses 24:00:00 encoding.
	return encodeTimeTZ(b, int64(t.TimeOfDay)+int64(t.OffsetSecs)*1000000, t.OffsetSecs)

Nit: Would be nice to make 1000000 a named constant.

pkg/util/encoding/encoding_test.go, line 1288 at r1 (raw file):

	return timetz.MakeTimeTZ(
		timeofday.FromInt(rd.Int63n(int64(timeofday.Max))),
		rd.Int31n(28*60*60)-14*60*60,

Minor: Seems like this doesn't quite cover the max and min values.

pkg/util/timetz/timetz.go, line 27 at r1 (raw file):

var (
	// TimeTZMaxTimeRegex is a compiled regex for parsing the 24:00 timetz value
	TimeTZMaxTimeRegex = regexp.MustCompile(`^24:00((:00)|(:00.0+))?([+-](([\d]+)|(\d{1,2}(:\d{1,2}){0,2})))?$`)

I'm probably missing something but seems like this regex could just be ^24 without any loss of functionality.

pkg/util/timetz/timetz.go, line 116 at r1 (raw file):

	}
	// time.Format does not work for negative zone offsets
	// which are in seconds less than 60.

wat

otan

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

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