diff --git a/docs/RFCS/20191014_halloween.md b/docs/RFCS/20191014_halloween.md new file mode 100644 index 000000000000..760eee8d63e7 --- /dev/null +++ b/docs/RFCS/20191014_halloween.md @@ -0,0 +1,252 @@ +- Feature Name: SQL step-wise execution +- Status: in-progress +- Start Date: 2019-10-14 +- Authors: andrei knz nathan +- RFC PR: [#42864](https://github.com/cockroachdb/cockroach/pull/42864) +- Cockroach Issue: + [#28842](https://github.com/cockroachdb/cockroach/issues/28842) + [#33473](https://github.com/cockroachdb/cockroach/issues/33473) + [#33475](https://github.com/cockroachdb/cockroach/issues/33475) + +# Summary + +This RFC proposes to introduce *sequence points* around and inside the +execution of SQL statements, so that all KV reads performed after a +sequence point observe the data at the time the sequence point was +established. + +This change intends to solve issue +[#28842](https://github.com/cockroachdb/cockroach/issues/28842) which +is a serious—and classical—semantic error that can cause business +problems in customer applications, see +https://en.wikipedia.org/wiki/Halloween_Problem for details. + +It's also an area where CockroachDB diverges from PostgreSQL, so +fixing this would provide a modicum of additional compatibility. + +The proposed solution capitalizes on the *KV sequence numbers* +previously introduced at the KV and storage levels for an unrelated +reason (txn performance optimizations) and can be summarized as +"annotate every read request with the current read seqnum" on the +common path, with the new `Step()` API to copy the current write +seqnum as new current read seqnum. + +# Motivation + +See above. + +# Guide-level explanation + +After this change, the "read" portion of SQL mutations operate using a +snapshot of the database as per the moment the statment started. In +particular it cannot see its own writes. + +This guarantees e.g. that a statement like `INSERT INTO t SELECT * +FROM t` always terminates, or that `UPDATE t SET x = x+2` never +operates on the same row two times. + +# Reference-level explanation + +The following API is added to `TxnCoordSender` and `*client.Txn`: + +```go + // Step creates a sequencing point in the current transaction. A + // sequencing point establishes a snapshot baseline for subsequent + // read operations: until the next sequencing point, read operations + // observe the data at the time the snapshot was established and + // ignore writes performed since. + // + // Before the first step is taken, the transaction operates as if + // there was a step after every write: each read to a key is able to + // see the latest write before it. This makes the step behavior + // opt-in and backward-compatible with existing code which does not + // need it. + // The method is idempotent. + Step() error + + // DisableStepping disables the sequencing point behavior and + // ensures that every read can read the latest write. The + // effect remains disabled until the next call to Step(). + // The method is idempotent. + DisableStepping() error +``` + +- `Step()` is called every time a SQL execution step is reached: + - before the execution of each regular statement; + - at the end of the mutations, before and in-between the FK and cascading action phases, if any. +- the implementation of `Step()` forwards the call to the RootTxn's + TxnCoordSender `Step()` method (new), which in turn saves the current + write seqnum as reference seqnum for future reads. +- `DisableStepping()` is merely a convenience, but is provided + for use with the execution part of all the DDL statements. + These currently contain many internal steps where each + internal step observes the results of steps prior. Without + `DisableStepping()`, we would need to add calls to `Step()` + throughout, which would be error-prone. + +## Detailed design + +The design is prototyped here: + +https://github.com/cockroachdb/cockroach/pull/42854 + +https://github.com/cockroachdb/cockroach/pull/42862 + +Additional topics of interest: + +- [Uniqueness violations](#Uniqueness-violations) +- [ON CONFLICT processing](#ON-CONFLICT-processing) +- [FK existence checks under a single mutation](#FK-existence-checks-under-a-single-mutation) +- [FK cascading actions under a single mutation](#FK-cascading-actions-under-a-single-mutation) +- [Multiple mutations with CTEs](#Multiple-mutations-with-CTEs) +- [Schema changes](#Schema-changes) + +### Uniqueness violations + +There are really two cases: + +- we insert/modify a single row, and doing so creating a duplicate of + a row that was modified in a previous statement (or sequencing + step). This case is simple and transparently handled by "read at + seqnum of previous step". + +- we insert/modify the same row two times inside the same mutation + statement, or two rows such that they are duplicate according to + some unique index. + Here the problem is seemingly that the 2nd row update will not + see the first. + +However, when looking more closely there is no new problem here. + +All writes to a unique index go through a KV `CPut` on the uniqueness key. +By ensuring that `CPuts` read their _condition_ at the current write +seqnum, we can always pick up the latest write and detect duplicates. + +(CPut will still skip over ignored / rolled back seqnums like other KV +ops. It's only the target read seqnum that's ratcheted up to the +present for CPut, in contrast to other mvcc ops that will be blocked by +the configured target read seqnum.) + +This opens a question of whether we need a variant of CPut which does +not do this. TBD. (Initial analysis says no.) + +### ON CONFLICT processing + +Question arises of what to do when the result of ON CONFLICT +processing changes a row in a read-modify-write fashion. For example: + +```sql +INSERT INTO t(x) VALUES (1), (1) ON CONFLICT(x) DO UPDATE SET x = t.x + excluded.x +-- ^^^^^^^^ notice the dup row +``` + +Here conceptually the INSERT suggests that the 2nd ON CONFLICT resolution +will observe the row as it was left over by the 1st. This would not work +with "read at seqnum of previous statement". + +The answer here is from a previous discussion around mutations that +observed the following: + +- postgres does not support updating the same row two times in an ON + CONFLICT clause. + +- it is desirable to batch / pre-compute the ON CONFLICT result values + concurrently with the mutation for performance, and we've already + established back in 2018 that the lack of support for updating the + same row twice in pg makes this optimization possible. + +- the implementation was updated when bringing this logic under the CBO + +From here, it follows that we don't care about "read at seqnum" +inconsistencies as the current CBO logic already assumes that it's +fine to miss earlier conflict resolutions. + +### FK existence checks under a single mutation + +FK existence checks must observe the data values post-mutation. For +this we need to introduce a sequence point (call to `Step()`) between +the end of the "run" phase (where results were produced for the +client) and the FK existence checks. + +This way the reads for FK existence checks can see all the writes by +the mutation. + +Note that today FK existence checks are "interleaved" with mutations +on the common path, which is a useful optimization but incorrect in +some cases. This will need to be adjusted. See the following issue for details: +https://github.com/cockroachdb/cockroach/issues/33475 + +In 19.2/20.1 there is a new notion of "post-queries" which the CBO is +increasingly using to perform FK checks and cascading actions. These +benefit simply by adding a sequence point before the execution of each +post-query. + +### FK cascading actions under a single mutation + +Postgres uses post-statement triggers to process FK cascading actions +and existence checks. Cascading actions that result in mutations to +other tables themselves append more triggers to run. + +Each subsequent step in this cascade of effects is able to read its +own writes (for futher FK checks). + +We emulate this in CockroachDB by introducing a step boundary between +iterations of the cascading algorithm. + +### Multiple mutations with CTEs + +It's possible for a single statement to define multiple mutations for example: + +```sql +WITH + a AS (INSERT ... RETURNING ...), + b AS (INSERT ... RETURNING ...) + SELECT ... +``` + +PostgreSQL does not guarantee that the effect of one mutation is +visible to another, or even to the later read-only parts of the +statement. In fact it requires that all mutations operate +on the same data at the beginning of the statement: + +More specifically: https://www.postgresql.org/docs/12/queries-with.html + +> The sub-statements in WITH are executed concurrently with each other +> and with the main query. Therefore, when using data-modifying +> statements in WITH, the order in which the specified updates +> actually happen is unpredictable. **All the statements are executed +> with the same snapshot (see Chapter 13), so they cannot “see” one +> another's effects on the target tables.** This alleviates the effects +> of the unpredictability of the actual order of row updates, and +> means that RETURNING data is the only way to communicate changes +> between different WITH sub-statements and the main query. + +So with the logic proposed so far, all the mutations inside the same +statement execute from the same read seqnum. + +If there is FK work to be done, the first sequencing step necessary +for FK checks (to advance the read seqnum) will only occur after all +mutations have completed. + +(The observations from [Uniqueness violations](#Uniqueness-violations) above apply here as well.) + +### Schema changes + +Schema changers that operate synchronously operate "under the sequence +point" and need no further adjustment. + +Schema changers that operate asynchronously already operate under +independent `*client.Txn` instances and are thus unaffected. + + +## Drawbacks + +None known. + +## Rationale and Alternatives + +No alternative was evaluated. + +## Unresolved questions + +None known. diff --git a/docs/generated/sql/bnf/stmt_block.bnf b/docs/generated/sql/bnf/stmt_block.bnf index 3ef2369ed6b5..a717ba5f5466 100644 --- a/docs/generated/sql/bnf/stmt_block.bnf +++ b/docs/generated/sql/bnf/stmt_block.bnf @@ -2091,7 +2091,10 @@ character_without_length ::= const_datetime ::= 'DATE' + | 'TIME' opt_timezone + | 'TIME' '(' iconst32 ')' opt_timezone | 'TIMETZ' + | 'TIMETZ' '(' iconst32 ')' | 'TIMESTAMP' opt_timezone | 'TIMESTAMP' '(' iconst32 ')' opt_timezone | 'TIMESTAMPTZ' diff --git a/pkg/ccl/changefeedccl/avro.go b/pkg/ccl/changefeedccl/avro.go index a5bcdf18904f..4a87e7cbb4bf 100644 --- a/pkg/ccl/changefeedccl/avro.go +++ b/pkg/ccl/changefeedccl/avro.go @@ -230,7 +230,7 @@ func columnDescToAvroSchema(colDesc *sqlbase.ColumnDescriptor) (*avroSchemaField return d.(*tree.DTimeTZ).TimeTZ.String(), nil } schema.decodeFn = func(x interface{}) (tree.Datum, error) { - return tree.ParseDTimeTZ(nil, x.(string)) + return tree.ParseDTimeTZ(nil, x.(string), time.Microsecond) } case types.TimestampFamily: avroType = avroLogicalType{ diff --git a/pkg/sql/logictest/testdata/logic_test/time b/pkg/sql/logictest/testdata/logic_test/time index 87b0fe6c4960..6209582fadc8 100644 --- a/pkg/sql/logictest/testdata/logic_test/time +++ b/pkg/sql/logictest/testdata/logic_test/time @@ -343,3 +343,66 @@ SELECT extract('day' from time '12:00:00') query error pgcode 22023 extract\(\): unsupported timespan: day SELECT extract('DAY' from time '12:00:00') + +subtest precision_tests + +query error precision 7 out of range +select '1:00:00.001':::TIME(7) + +statement ok +CREATE TABLE time_precision_test ( + id integer PRIMARY KEY, + t TIME(5) +) + +statement ok +INSERT INTO time_precision_test VALUES + (1,'12:00:00.123456+03:00'), + (2,'12:00:00.12345+03:00'), + (3,'12:00:00.1234+03:00'), + (4,'12:00:00.123+03:00'), + (5,'12:00:00.12+03:00'), + (6,'12:00:00.1+03:00'), + (7,'12:00:00+03:00') + +query IT +SELECT * FROM time_precision_test ORDER BY id ASC +---- +1 0000-01-01 12:00:00.12346 +0000 UTC +2 0000-01-01 12:00:00.12345 +0000 UTC +3 0000-01-01 12:00:00.1234 +0000 UTC +4 0000-01-01 12:00:00.123 +0000 UTC +5 0000-01-01 12:00:00.12 +0000 UTC +6 0000-01-01 12:00:00.1 +0000 UTC +7 0000-01-01 12:00:00 +0000 UTC + +query TT +select column_name, data_type FROM [SHOW COLUMNS FROM time_precision_test] ORDER BY column_name +---- +id INT8 +t TIME(5) + +statement ok +ALTER TABLE time_precision_test ALTER COLUMN t TYPE time(6) + +statement ok +INSERT INTO time_precision_test VALUES + (100,'12:00:00.123456+03:00') + +query IT +SELECT * FROM time_precision_test ORDER BY id ASC +---- +1 0000-01-01 12:00:00.12346 +0000 UTC +2 0000-01-01 12:00:00.12345 +0000 UTC +3 0000-01-01 12:00:00.1234 +0000 UTC +4 0000-01-01 12:00:00.123 +0000 UTC +5 0000-01-01 12:00:00.12 +0000 UTC +6 0000-01-01 12:00:00.1 +0000 UTC +7 0000-01-01 12:00:00 +0000 UTC +100 0000-01-01 12:00:00.123456 +0000 UTC + +query TT +select column_name, data_type FROM [SHOW COLUMNS FROM time_precision_test] ORDER BY column_name +---- +id INT8 +t TIME(6) diff --git a/pkg/sql/logictest/testdata/logic_test/timetz b/pkg/sql/logictest/testdata/logic_test/timetz index b3b67ccf87c0..745d872e8679 100644 --- a/pkg/sql/logictest/testdata/logic_test/timetz +++ b/pkg/sql/logictest/testdata/logic_test/timetz @@ -90,3 +90,66 @@ SELECT '2001-01-01 11:00+04:00'::timestamptz::timetz statement ok SET TIME ZONE UTC + +subtest precision_tests + +query error precision 7 out of range +select '1:00:00.001':::TIMETZ(7) + +statement ok +CREATE TABLE timetz_precision_test ( + id integer PRIMARY KEY, + t TIMETZ(5) +) + +statement ok +INSERT INTO timetz_precision_test VALUES + (1,'12:00:00.123456+03:00'), + (2,'12:00:00.12345+03:00'), + (3,'12:00:00.1234+03:00'), + (4,'12:00:00.123+03:00'), + (5,'12:00:00.12+03:00'), + (6,'12:00:00.1+03:00'), + (7,'12:00:00+03:00') + +query IT +SELECT * FROM timetz_precision_test ORDER BY id ASC +---- +1 0000-01-01 12:00:00.12346 +0300 +0300 +2 0000-01-01 12:00:00.12345 +0300 +0300 +3 0000-01-01 12:00:00.1234 +0300 +0300 +4 0000-01-01 12:00:00.123 +0300 +0300 +5 0000-01-01 12:00:00.12 +0300 +0300 +6 0000-01-01 12:00:00.1 +0300 +0300 +7 0000-01-01 12:00:00 +0300 +0300 + +query TT +select column_name, data_type FROM [SHOW COLUMNS FROM timetz_precision_test] ORDER BY column_name +---- +id INT8 +t TIMETZ(5) + +statement ok +ALTER TABLE timetz_precision_test ALTER COLUMN t TYPE timetz(6) + +statement ok +INSERT INTO timetz_precision_test VALUES + (100,'12:00:00.123456+03:00') + +query IT +SELECT * FROM timetz_precision_test ORDER BY id ASC +---- +1 0000-01-01 12:00:00.12346 +0300 +0300 +2 0000-01-01 12:00:00.12345 +0300 +0300 +3 0000-01-01 12:00:00.1234 +0300 +0300 +4 0000-01-01 12:00:00.123 +0300 +0300 +5 0000-01-01 12:00:00.12 +0300 +0300 +6 0000-01-01 12:00:00.1 +0300 +0300 +7 0000-01-01 12:00:00 +0300 +0300 +100 0000-01-01 12:00:00.123456 +0300 +0300 + +query TT +select column_name, data_type FROM [SHOW COLUMNS FROM timetz_precision_test] ORDER BY column_name +---- +id INT8 +t TIMETZ(6) diff --git a/pkg/sql/parser/parse_test.go b/pkg/sql/parser/parse_test.go index 1bcd0bb93d29..b7b3413ac4ec 100644 --- a/pkg/sql/parser/parse_test.go +++ b/pkg/sql/parser/parse_test.go @@ -112,6 +112,8 @@ func TestParse(t *testing.T) { {`CREATE TABLE a (b SERIAL8)`}, {`CREATE TABLE a (b TIME)`}, {`CREATE TABLE a (b TIMETZ)`}, + {`CREATE TABLE a (b TIME(3))`}, + {`CREATE TABLE a (b TIMETZ(3))`}, {`CREATE TABLE a (b UUID)`}, {`CREATE TABLE a (b INET)`}, {`CREATE TABLE a (b "char")`}, @@ -1469,11 +1471,20 @@ func TestParse2(t *testing.T) { {`SELECT CAST(1 AS "_int8")`, `SELECT CAST(1 AS INT8[])`}, {`SELECT SERIAL8 'foo', 'foo'::SERIAL8`, `SELECT INT8 'foo', 'foo'::INT8`}, + {`SELECT 'a'::TIMESTAMP(3)`, `SELECT 'a'::TIMESTAMP(3)`}, {`SELECT 'a'::TIMESTAMP(3) WITHOUT TIME ZONE`, `SELECT 'a'::TIMESTAMP(3)`}, + {`SELECT 'a'::TIMESTAMPTZ(3)`, `SELECT 'a'::TIMESTAMPTZ(3)`}, {`SELECT 'a'::TIMESTAMP(3) WITH TIME ZONE`, `SELECT 'a'::TIMESTAMPTZ(3)`}, {`SELECT TIMESTAMP(3) 'a'`, `SELECT TIMESTAMP(3) 'a'`}, {`SELECT TIMESTAMPTZ(3) 'a'`, `SELECT TIMESTAMPTZ(3) 'a'`}, + {`SELECT 'a'::TIME(3)`, `SELECT 'a'::TIME(3)`}, + {`SELECT 'a'::TIME(3) WITHOUT TIME ZONE`, `SELECT 'a'::TIME(3)`}, + {`SELECT 'a'::TIMETZ(3)`, `SELECT 'a'::TIMETZ(3)`}, + {`SELECT 'a'::TIME(3) WITH TIME ZONE`, `SELECT 'a'::TIMETZ(3)`}, + {`SELECT TIME(3) 'a'`, `SELECT TIME(3) 'a'`}, + {`SELECT TIMETZ(3) 'a'`, `SELECT TIMETZ(3) 'a'`}, + {`SELECT 'a' FROM t@{FORCE_INDEX=bar}`, `SELECT 'a' FROM t@bar`}, {`SELECT 'a' FROM t@{ASC,FORCE_INDEX=idx}`, `SELECT 'a' FROM t@{FORCE_INDEX=idx,ASC}`}, @@ -3127,13 +3138,6 @@ func TestUnimplementedSyntax(t *testing.T) { {`SELECT 'a'::INTERVAL SECOND(123)`, 32564, `interval second`}, {`SELECT INTERVAL(3) 'a'`, 32564, ``}, - {`SELECT 'a'::TIME(123)`, 32565, ``}, - {`SELECT 'a'::TIME(123) WITHOUT TIME ZONE`, 32565, ``}, - {`SELECT 'a'::TIMETZ(123)`, 26097, `type with precision`}, - {`SELECT 'a'::TIME(123) WITH TIME ZONE`, 32565, ``}, - {`SELECT TIME(3) 'a'`, 32565, ``}, - {`SELECT TIMETZ(3) 'a'`, 26097, `type with precision`}, - {`SELECT a(b) 'c'`, 0, `a(...) SCONST`}, {`SELECT (a,b) OVERLAPS (c,d)`, 0, `overlaps`}, {`SELECT UNIQUE (SELECT b)`, 0, `UNIQUE predicate`}, diff --git a/pkg/sql/parser/sql.y b/pkg/sql/parser/sql.y index e4e4769f094f..abd0b13b8985 100644 --- a/pkg/sql/parser/sql.y +++ b/pkg/sql/parser/sql.y @@ -7322,19 +7322,35 @@ const_datetime: } | TIME opt_timezone { - if $2.bool() { return unimplementedWithIssueDetail(sqllex, 26097, "type") } - $$.val = types.Time + if $2.bool() { + $$.val = types.TimeTZ + } else { + $$.val = types.Time + } } | TIME '(' iconst32 ')' opt_timezone { prec := $3.int32() - if prec != 6 { - return unimplementedWithIssue(sqllex, 32565) + if prec < 0 || prec > 6 { + sqllex.Error(fmt.Sprintf("precision %d out of range", prec)) + return 1 + } + if $5.bool() { + $$.val = types.MakeTimeTZ(prec) + } else { + $$.val = types.MakeTime(prec) } - $$.val = types.MakeTime(prec) } | TIMETZ { $$.val = types.TimeTZ } -| TIMETZ '(' ICONST ')' { return unimplementedWithIssueDetail(sqllex, 26097, "type with precision") } +| TIMETZ '(' iconst32 ')' + { + prec := $3.int32() + if prec < 0 || prec > 6 { + sqllex.Error(fmt.Sprintf("precision %d out of range", prec)) + return 1 + } + $$.val = types.MakeTimeTZ(prec) + } | TIMESTAMP opt_timezone { if $2.bool() { diff --git a/pkg/sql/pgwire/pgwirebase/encoding.go b/pkg/sql/pgwire/pgwirebase/encoding.go index ac2ceb0a5df2..9c8f58b71f43 100644 --- a/pkg/sql/pgwire/pgwirebase/encoding.go +++ b/pkg/sql/pgwire/pgwirebase/encoding.go @@ -281,13 +281,13 @@ func DecodeOidDatum( } return d, nil case oid.T_time: - d, err := tree.ParseDTime(nil, string(b)) + d, err := tree.ParseDTime(nil, string(b), time.Microsecond) if err != nil { return nil, pgerror.Newf(pgcode.Syntax, "could not parse string %q as time", b) } return d, nil case oid.T_timetz: - d, err := tree.ParseDTimeTZ(ctx, string(b)) + d, err := tree.ParseDTimeTZ(ctx, string(b), time.Microsecond) if err != nil { return nil, pgerror.Newf(pgcode.Syntax, "could not parse string %q as timetz", b) } diff --git a/pkg/sql/schemachange/alter_column_type.go b/pkg/sql/schemachange/alter_column_type.go index 0f9bb9a35b06..f2140365952a 100644 --- a/pkg/sql/schemachange/alter_column_type.go +++ b/pkg/sql/schemachange/alter_column_type.go @@ -120,6 +120,12 @@ var classifiers = map[types.Family]map[types.Family]classifier{ types.TimestampFamily: classifierPrecision, types.TimestampTZFamily: classifierPrecision, }, + types.TimeFamily: { + types.TimeFamily: classifierPrecision, + }, + types.TimeTZFamily: { + types.TimeTZFamily: classifierPrecision, + }, } // classifierHardestOf creates a composite classifier that returns the diff --git a/pkg/sql/schemachange/alter_column_type_test.go b/pkg/sql/schemachange/alter_column_type_test.go index 1e7a1ca204ee..6bfd7423b578 100644 --- a/pkg/sql/schemachange/alter_column_type_test.go +++ b/pkg/sql/schemachange/alter_column_type_test.go @@ -97,6 +97,15 @@ func TestColumnConversions(t *testing.T) { "STRING(5)": { "BYTES": ColumnConversionTrivial, }, + + "TIME": { + "TIME": ColumnConversionTrivial, + "TIME(5)": ColumnConversionValidate, + }, + "TIMETZ": { + "TIMETZ": ColumnConversionTrivial, + "TIMETZ(5)": ColumnConversionValidate, + }, "TIMESTAMP": { "TIMESTAMPTZ": ColumnConversionTrivial, "TIMESTAMP": ColumnConversionTrivial, @@ -224,9 +233,11 @@ func TestColumnConversions(t *testing.T) { case types.TimeFamily, types.TimestampFamily, - types.TimestampTZFamily: + types.TimestampTZFamily, + types.TimeTZFamily: const timeOnly = "15:04:05" + const timeOnlyWithZone = "15:04:05 -0700" const noZone = "2006-01-02 15:04:05" const withZone = "2006-01-02 15:04:05 -0700" @@ -238,6 +249,8 @@ func TestColumnConversions(t *testing.T) { fromFmt = noZone case types.TimestampTZFamily: fromFmt = withZone + case types.TimeTZFamily: + fromFmt = timeOnlyWithZone } // Always use a non-UTC zone for this test @@ -255,7 +268,8 @@ func TestColumnConversions(t *testing.T) { case types.TimeFamily, types.TimestampFamily, - types.TimestampTZFamily: + types.TimestampTZFamily, + types.TimeTZFamily: // We're going to re-parse the text as though we're in UTC // so that we can drop the TZ info. if parsed, err := time.ParseInLocation(fromFmt, now, time.UTC); err == nil { diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index cdd1e01f8167..1aaec7a898ee 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/pkg/sql/sem/builtins/builtins_test.go @@ -348,7 +348,7 @@ func TestExtractStringFromTimeTZ(t *testing.T) { for _, tc := range testCases { t.Run(fmt.Sprintf("%s_%s", tc.timeSpan, tc.timeTZString), func(t *testing.T) { - timeTZ, err := tree.ParseDTimeTZ(nil, tc.timeTZString) + timeTZ, err := tree.ParseDTimeTZ(nil, tc.timeTZString, time.Microsecond) assert.NoError(t, err) datum, err := extractStringFromTimeTZ(timeTZ, tc.timeSpan) diff --git a/pkg/sql/sem/tree/constant_test.go b/pkg/sql/sem/tree/constant_test.go index 64a6c90d6de1..45c17c2a1f3e 100644 --- a/pkg/sql/sem/tree/constant_test.go +++ b/pkg/sql/sem/tree/constant_test.go @@ -205,7 +205,14 @@ func mustParseDDate(t *testing.T, s string) tree.Datum { return d } func mustParseDTime(t *testing.T, s string) tree.Datum { - d, err := tree.ParseDTime(nil, s) + d, err := tree.ParseDTime(nil, s, time.Microsecond) + if err != nil { + t.Fatal(err) + } + return d +} +func mustParseDTimeTZ(t *testing.T, s string) tree.Datum { + d, err := tree.ParseDTimeTZ(nil, s, time.Microsecond) if err != nil { t.Fatal(err) } @@ -270,6 +277,7 @@ var parseFuncs = map[*types.T]func(*testing.T, string) tree.Datum{ types.Bool: mustParseDBool, types.Date: mustParseDDate, types.Time: mustParseDTime, + types.TimeTZ: mustParseDTimeTZ, types.Timestamp: mustParseDTimestamp, types.TimestampTZ: mustParseDTimestampTZ, types.Interval: mustParseDInterval, diff --git a/pkg/sql/sem/tree/datum.go b/pkg/sql/sem/tree/datum.go index ba9c8a2ce052..829b065ec658 100644 --- a/pkg/sql/sem/tree/datum.go +++ b/pkg/sql/sem/tree/datum.go @@ -1862,7 +1862,7 @@ func MakeDTime(t timeofday.TimeOfDay) *DTime { // ParseDTime parses and returns the *DTime Datum value represented by the // provided string, or an error if parsing is unsuccessful. -func ParseDTime(ctx ParseTimeContext, s string) (*DTime, error) { +func ParseDTime(ctx ParseTimeContext, s string, precision time.Duration) (*DTime, error) { now := relativeParseTime(ctx) // special case on 24:00 and 24:00:00 as the parser @@ -1876,7 +1876,7 @@ func ParseDTime(ctx ParseTimeContext, s string) (*DTime, error) { // Build our own error message to avoid exposing the dummy date. return nil, makeParseError(s, types.Time, nil) } - return MakeDTime(timeofday.FromTime(t)), nil + return MakeDTime(timeofday.FromTime(t).Round(precision)), nil } // ResolvedType implements the TypedExpr interface. @@ -1899,6 +1899,11 @@ func (d *DTime) Prev(_ *EvalContext) (Datum, bool) { return &prev, true } +// Round returns a new DTime to the specified precision. +func (d *DTime) Round(precision time.Duration) *DTime { + return MakeDTime(timeofday.TimeOfDay(*d).Round(precision)) +} + // Next implements the Datum interface. func (d *DTime) Next(_ *EvalContext) (Datum, bool) { next := *d + 1 @@ -1981,9 +1986,9 @@ func NewDTimeTZFromLocation(t timeofday.TimeOfDay, loc *time.Location) *DTimeTZ // ParseDTimeTZ parses and returns the *DTime Datum value represented by the // provided string, or an error if parsing is unsuccessful. -func ParseDTimeTZ(ctx ParseTimeContext, s string) (*DTimeTZ, error) { +func ParseDTimeTZ(ctx ParseTimeContext, s string, precision time.Duration) (*DTimeTZ, error) { now := relativeParseTime(ctx) - d, err := timetz.ParseTimeTZ(now, s) + d, err := timetz.ParseTimeTZ(now, s, precision) if err != nil { return nil, err } @@ -2035,6 +2040,11 @@ func (d *DTimeTZ) Max(_ *EvalContext) (Datum, bool) { return dMaxTimeTZ, true } +// Round returns a new DTimeTZ to the specified precision. +func (d *DTimeTZ) Round(precision time.Duration) *DTimeTZ { + return NewDTimeTZ(d.TimeTZ.Round(precision)) +} + // Min implements the Datum interface. func (d *DTimeTZ) Min(_ *EvalContext) (Datum, bool) { return dMinTimeTZ, true diff --git a/pkg/sql/sem/tree/datum_test.go b/pkg/sql/sem/tree/datum_test.go index ebe0b8d7e9bd..bc6a65ae30bc 100644 --- a/pkg/sql/sem/tree/datum_test.go +++ b/pkg/sql/sem/tree/datum_test.go @@ -484,19 +484,21 @@ func TestParseDTime(t *testing.T) { // Since ParseDTime mostly delegates parsing logic to ParseDTimestamp, we only test a subset of // the timestamp test cases. testData := []struct { - str string - expected timeofday.TimeOfDay + str string + precision time.Duration + expected timeofday.TimeOfDay }{ - {"04:05:06", timeofday.New(4, 5, 6, 0)}, - {"04:05:06.000001", timeofday.New(4, 5, 6, 1)}, - {"04:05:06-07", timeofday.New(4, 5, 6, 0)}, - {"4:5:6", timeofday.New(4, 5, 6, 0)}, - {"24:00:00", timeofday.Time2400}, - {"24:00:00.000", timeofday.Time2400}, - {"24:00:00.000000", timeofday.Time2400}, + {"04:05:06", time.Microsecond, timeofday.New(4, 5, 6, 0)}, + {"04:05:06.000001", time.Microsecond, timeofday.New(4, 5, 6, 1)}, + {"04:05:06.000001", time.Second, timeofday.New(4, 5, 6, 0)}, + {"04:05:06-07", time.Microsecond, timeofday.New(4, 5, 6, 0)}, + {"4:5:6", time.Microsecond, timeofday.New(4, 5, 6, 0)}, + {"24:00:00", time.Microsecond, timeofday.Time2400}, + {"24:00:00.000", time.Microsecond, timeofday.Time2400}, + {"24:00:00.000000", time.Microsecond, timeofday.Time2400}, } for _, td := range testData { - actual, err := tree.ParseDTime(nil, td.str) + actual, err := tree.ParseDTime(nil, td.str, td.precision) if err != nil { t.Errorf("unexpected error while parsing TIME %s: %s", td.str, err) continue @@ -515,7 +517,7 @@ func TestParseDTimeError(t *testing.T) { "01", } for _, s := range testData { - actual, _ := tree.ParseDTime(nil, s) + actual, _ := tree.ParseDTime(nil, s, time.Microsecond) if actual != nil { t.Errorf("TIME %s: got %s, expected error", s, actual) } @@ -592,15 +594,15 @@ func TestMakeDJSON(t *testing.T) { func TestDTimeTZ(t *testing.T) { defer leaktest.AfterTest(t)() - maxTime, err := tree.ParseDTimeTZ(nil, "24:00:00-1559") + maxTime, err := tree.ParseDTimeTZ(nil, "24:00:00-1559", time.Microsecond) require.NoError(t, err) - minTime, err := tree.ParseDTimeTZ(nil, "00:00:00+1559") + minTime, err := tree.ParseDTimeTZ(nil, "00:00:00+1559", time.Microsecond) require.NoError(t, err) // These are all the same UTC time equivalents. - utcTime, err := tree.ParseDTimeTZ(nil, "11:14:15+0") + utcTime, err := tree.ParseDTimeTZ(nil, "11:14:15+0", time.Microsecond) require.NoError(t, err) - sydneyTime, err := tree.ParseDTimeTZ(nil, "21:14:15+10") + sydneyTime, err := tree.ParseDTimeTZ(nil, "21:14:15+10", time.Microsecond) require.NoError(t, err) // No daylight savings in Hawaii! diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index bf8632e9a070..cbdb62b9028d 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -3438,36 +3438,38 @@ func PerformCast(ctx *EvalContext, d Datum, t *types.T) (Datum, error) { } case types.TimeFamily: + roundTo := TimeFamilyPrecisionToRoundDuration(t.Precision()) switch d := d.(type) { case *DString: - return ParseDTime(ctx, string(*d)) + return ParseDTime(ctx, string(*d), roundTo) case *DCollatedString: - return ParseDTime(ctx, d.Contents) + return ParseDTime(ctx, d.Contents, roundTo) case *DTime: - return d, nil + return d.Round(roundTo), nil case *DTimeTZ: - return MakeDTime(d.TimeOfDay), nil + return MakeDTime(d.TimeOfDay.Round(roundTo)), nil case *DTimestamp: - return MakeDTime(timeofday.FromTime(d.Time)), nil + return MakeDTime(timeofday.FromTime(d.Time).Round(roundTo)), nil case *DTimestampTZ: // Strip time zone. Times don't carry their location. - return MakeDTime(timeofday.FromTime(d.stripTimeZone(ctx).Time)), nil + return MakeDTime(timeofday.FromTime(d.stripTimeZone(ctx).Time).Round(roundTo)), nil case *DInterval: - return MakeDTime(timeofday.Min.Add(d.Duration)), nil + return MakeDTime(timeofday.Min.Add(d.Duration).Round(roundTo)), nil } case types.TimeTZFamily: + roundTo := TimeFamilyPrecisionToRoundDuration(t.Precision()) switch d := d.(type) { case *DString: - return ParseDTimeTZ(ctx, string(*d)) + return ParseDTimeTZ(ctx, string(*d), roundTo) case *DCollatedString: - return ParseDTimeTZ(ctx, d.Contents) + return ParseDTimeTZ(ctx, d.Contents, roundTo) case *DTime: - return NewDTimeTZFromLocation(timeofday.TimeOfDay(*d), ctx.GetLocation()), nil + return NewDTimeTZFromLocation(timeofday.TimeOfDay(*d).Round(roundTo), ctx.GetLocation()), nil case *DTimeTZ: - return d, nil + return d.Round(roundTo), nil case *DTimestampTZ: - return NewDTimeTZFromTime(d.Time), nil + return NewDTimeTZFromTime(d.Time.Round(roundTo)), nil } case types.TimestampFamily: diff --git a/pkg/sql/sem/tree/parse_string.go b/pkg/sql/sem/tree/parse_string.go index 06b1978efe75..71ec8f9bd530 100644 --- a/pkg/sql/sem/tree/parse_string.go +++ b/pkg/sql/sem/tree/parse_string.go @@ -79,9 +79,9 @@ func parseStringAs(t *types.T, s string, ctx ParseTimeContext) (Datum, error) { case types.StringFamily: return NewDString(s), nil case types.TimeFamily: - return ParseDTime(ctx, s) + return ParseDTime(ctx, s, TimeFamilyPrecisionToRoundDuration(t.Precision())) case types.TimeTZFamily: - return ParseDTimeTZ(ctx, s) + return ParseDTimeTZ(ctx, s, TimeFamilyPrecisionToRoundDuration(t.Precision())) case types.TimestampFamily: return ParseDTimestamp(ctx, s, TimeFamilyPrecisionToRoundDuration(t.Precision())) case types.TimestampTZFamily: diff --git a/pkg/sql/sem/tree/parse_string_test.go b/pkg/sql/sem/tree/parse_string_test.go index 1dbb7778b700..2a6de7856ea2 100644 --- a/pkg/sql/sem/tree/parse_string_test.go +++ b/pkg/sql/sem/tree/parse_string_test.go @@ -100,17 +100,6 @@ func TestParseDatumStringAs(t *testing.T) { "abc\x00", "ab⚣ cd", }, - types.Time: { - "01:02:03", - "02:03:04.123456", - }, - types.TimeTZ: { - "01:02:03+00:00:00", - "01:02:03+11:00:00", - "01:02:03+11:00:00", - "01:02:03-11:00:00", - "02:03:04.123456+11:00:00", - }, types.Timestamp: { "2001-01-01 01:02:03+00:00", "2001-01-01 02:03:04.123456+00:00", @@ -143,6 +132,40 @@ func TestParseDatumStringAs(t *testing.T) { "2001-01-01 01:02:03+00:00", "2001-01-01 02:03:04.123456+00:00", }, + types.Time: { + "01:02:03", + "02:03:04.123456", + }, + types.MakeTime(0): { + "01:02:03", + "02:03:04", + }, + types.MakeTime(3): { + "01:02:03", + "02:03:04.123", + }, + types.MakeTime(6): { + "01:02:03", + "02:03:04.123456", + }, + types.TimeTZ: { + "01:02:03+00:00:00", + "01:02:03+11:00:00", + "01:02:03+11:00:00", + "01:02:03-11:00:00", + "02:03:04.123456+11:00:00", + }, + types.MakeTimeTZ(0): { + "01:02:03+03:30:00", + }, + types.MakeTimeTZ(3): { + "01:02:03+03:30:00", + "02:03:04.123+03:30:00", + }, + types.MakeTimeTZ(6): { + "01:02:03+03:30:00", + "02:03:04.123456+03:30:00", + }, types.Uuid: { uuid.MakeV4().String(), }, diff --git a/pkg/sql/types/types.go b/pkg/sql/types/types.go index a77721c6fc66..10acaf7d9fed 100644 --- a/pkg/sql/types/types.go +++ b/pkg/sql/types/types.go @@ -246,7 +246,12 @@ var ( // HH:MM:SS.ssssss // Time = &T{InternalType: InternalType{ - Family: TimeFamily, Oid: oid.T_time, Locale: &emptyLocale}} + Family: TimeFamily, + Precision: 0, + TimePrecisionIsSet: false, + Oid: oid.T_time, + Locale: &emptyLocale, + }} // TimeTZ is the type specifying hour, minute, second and timezone with // no date component. By default, it has microsecond precision. @@ -254,7 +259,12 @@ var ( // // HH:MM:SS.ssssss+-ZZ:ZZ TimeTZ = &T{InternalType: InternalType{ - Family: TimeTZFamily, Oid: oid.T_timetz, Locale: &emptyLocale}} + Family: TimeTZFamily, + Precision: 0, + TimePrecisionIsSet: false, + Oid: oid.T_timetz, + Locale: &emptyLocale, + }} // Timestamp is the type of a value specifying year, month, day, hour, minute, // and second, but with no associated timezone. By default, it has microsecond @@ -489,12 +499,12 @@ func MakeScalar(family Family, o oid.Oid, precision, width int32, locale string) timePrecisionIsSet := false switch family { - case TimestampFamily, TimestampTZFamily: + case TimestampFamily, TimestampTZFamily, TimeFamily, TimeTZFamily: if precision < 0 || precision > 6 { panic(errors.AssertionFailedf("precision must be between 0 and 6 inclusive")) } timePrecisionIsSet = true - case DecimalFamily, TimeFamily, TimeTZFamily: + case DecimalFamily: if precision < 0 { panic(errors.AssertionFailedf("negative precision is not allowed")) } @@ -663,38 +673,36 @@ func MakeDecimal(precision, scale int32) *T { // MakeTime constructs a new instance of a TIME type (oid = T_time) that has at // most the given number of fractional second digits. +// +// To use the default precision, use the `Time` variable. func MakeTime(precision int32) *T { - if precision == 0 { - return Time - } - if precision != 6 { - panic(errors.AssertionFailedf("precision %d is not currently supported", precision)) - } return &T{InternalType: InternalType{ - Family: TimeFamily, - Oid: oid.T_time, - Precision: precision, - Locale: &emptyLocale, + Family: TimeFamily, + Oid: oid.T_time, + Precision: precision, + TimePrecisionIsSet: true, + Locale: &emptyLocale, }} } // MakeTimeTZ constructs a new instance of a TIME type (oid = T_timetz) that has at // most the given number of fractional second digits. +// +// To use the default precision, use the `TimeTZ` variable. func MakeTimeTZ(precision int32) *T { - if precision == 0 { - return TimeTZ - } - if precision != 6 { - panic(errors.AssertionFailedf("precision %d is not currently supported", precision)) - } return &T{InternalType: InternalType{ - Family: TimeTZFamily, Oid: oid.T_timetz, Precision: precision, Locale: &emptyLocale}} + Family: TimeTZFamily, + Oid: oid.T_timetz, + Precision: precision, + TimePrecisionIsSet: true, + Locale: &emptyLocale, + }} } // MakeTimestamp constructs a new instance of a TIMESTAMP type that has at most // the given number of fractional second digits. // -// To use the default, use the `Timestamp` variable. +// To use the default precision, use the `Timestamp` variable. func MakeTimestamp(precision int32) *T { return &T{InternalType: InternalType{ Family: TimestampFamily, @@ -708,7 +716,7 @@ func MakeTimestamp(precision int32) *T { // MakeTimestampTZ constructs a new instance of a TIMESTAMPTZ type that has at // most the given number of fractional second digits. // -// To use the default, use the `TimestampTZ` variable. +// To use the default precision, use the `TimestampTZ` variable. func MakeTimestampTZ(precision int32) *T { return &T{InternalType: InternalType{ Family: TimestampTZFamily, @@ -824,6 +832,7 @@ func (t *T) Width() int32 { // // DECIMAL : max # digits (must be >= Width/Scale) // TIME : max # fractional second digits +// TIMETZ : max # fractional second digits // TIMESTAMP : max # fractional second digits // TIMESTAMPTZ: max # fractional second digits // @@ -833,7 +842,7 @@ func (t *T) Width() int32 { // Precision is always 0 for other types. func (t *T) Precision() int32 { switch t.InternalType.Family { - case TimestampFamily, TimestampTZFamily: + case TimestampFamily, TimestampTZFamily, TimeFamily, TimeTZFamily: if t.InternalType.Precision == 0 && !t.InternalType.TimePrecisionIsSet { return defaultTimePrecision } @@ -1136,12 +1145,12 @@ func (t *T) SQLStandardNameWithTypmod(haveTypmod bool, typmod int) string { return buf.String() case TimeFamily: - if !haveTypmod || typmod <= 0 { + if !haveTypmod || typmod < 0 { return "time without time zone" } return fmt.Sprintf("time(%d) without time zone", typmod) case TimeTZFamily: - if !haveTypmod || typmod <= 0 { + if !haveTypmod || typmod < 0 { return "time with time zone" } return fmt.Sprintf("time(%d) with time zone", typmod) @@ -1228,11 +1237,7 @@ func (t *T) SQLString() string { case JsonFamily: // Only binary JSON is currently supported. return "JSONB" - case TimeFamily, TimeTZFamily: - if t.Precision() > 0 { - return fmt.Sprintf("%s(%d)", strings.ToUpper(t.Name()), t.Precision()) - } - case TimestampFamily, TimestampTZFamily: + case TimestampFamily, TimestampTZFamily, TimeFamily, TimeTZFamily: if t.InternalType.Precision > 0 || t.InternalType.TimePrecisionIsSet { return fmt.Sprintf("%s(%d)", strings.ToUpper(t.Name()), t.Precision()) } @@ -1738,7 +1743,7 @@ func (t *T) String() string { buf.WriteByte('}') } return buf.String() - case TimestampFamily, TimestampTZFamily: + case TimestampFamily, TimestampTZFamily, TimeFamily, TimeTZFamily: if t.InternalType.Precision > 0 || t.InternalType.TimePrecisionIsSet { return fmt.Sprintf("%s(%d)", t.Name(), t.Precision()) } diff --git a/pkg/sql/types/types_test.go b/pkg/sql/types/types_test.go index 335c8e20961b..8a7e27af4247 100644 --- a/pkg/sql/types/types_test.go +++ b/pkg/sql/types/types_test.go @@ -216,22 +216,56 @@ func TestTypes(t *testing.T) { {Name, MakeScalar(StringFamily, oid.T_name, 0, 0, emptyLocale)}, // TIME - {MakeTime(0), Time}, + {Time, &T{InternalType: InternalType{ + Family: TimeFamily, + Oid: oid.T_time, + Locale: &emptyLocale, + // Precision and PrecisionIsSet is not set. + }}}, + {MakeTime(0), MakeScalar(TimeFamily, oid.T_time, 0, 0, emptyLocale)}, {MakeTime(0), &T{InternalType: InternalType{ - Family: TimeFamily, Oid: oid.T_time, Locale: &emptyLocale}}}, + Family: TimeFamily, + Precision: 0, + TimePrecisionIsSet: true, + Oid: oid.T_time, + Locale: &emptyLocale, + }}}, + {MakeTime(3), &T{InternalType: InternalType{ + Family: TimeFamily, Oid: oid.T_time, Precision: 3, TimePrecisionIsSet: true, Locale: &emptyLocale}}}, + {MakeTime(3), MakeScalar(TimeFamily, oid.T_time, 3, 0, emptyLocale)}, {MakeTime(6), &T{InternalType: InternalType{ - Family: TimeFamily, Oid: oid.T_time, Precision: 6, Locale: &emptyLocale}}}, + Family: TimeFamily, Oid: oid.T_time, Precision: 6, TimePrecisionIsSet: true, Locale: &emptyLocale}}}, {MakeTime(6), MakeScalar(TimeFamily, oid.T_time, 6, 0, emptyLocale)}, // TIMETZ - {MakeTimeTZ(0), TimeTZ}, + {TimeTZ, &T{InternalType: InternalType{ + Family: TimeTZFamily, + Oid: oid.T_timetz, + Locale: &emptyLocale, + // Precision and PrecisionIsSet is not set. + }}}, + {MakeTimeTZ(0), MakeScalar(TimeTZFamily, oid.T_timetz, 0, 0, emptyLocale)}, {MakeTimeTZ(0), &T{InternalType: InternalType{ - Family: TimeTZFamily, Oid: oid.T_timetz, Locale: &emptyLocale}}}, + Family: TimeTZFamily, + Precision: 0, + TimePrecisionIsSet: true, + Oid: oid.T_timetz, + Locale: &emptyLocale, + }}}, + {MakeTimeTZ(3), &T{InternalType: InternalType{ + Family: TimeTZFamily, Oid: oid.T_timetz, Precision: 3, TimePrecisionIsSet: true, Locale: &emptyLocale}}}, + {MakeTimeTZ(3), MakeScalar(TimeTZFamily, oid.T_timetz, 3, 0, emptyLocale)}, {MakeTimeTZ(6), &T{InternalType: InternalType{ - Family: TimeTZFamily, Oid: oid.T_timetz, Precision: 6, Locale: &emptyLocale}}}, + Family: TimeTZFamily, Oid: oid.T_timetz, Precision: 6, TimePrecisionIsSet: true, Locale: &emptyLocale}}}, {MakeTimeTZ(6), MakeScalar(TimeTZFamily, oid.T_timetz, 6, 0, emptyLocale)}, // TIMESTAMP + {Timestamp, &T{InternalType: InternalType{ + Family: TimestampFamily, + Oid: oid.T_timestamp, + Locale: &emptyLocale, + // Precision and PrecisionIsSet is not set. + }}}, {MakeTimestamp(0), MakeScalar(TimestampFamily, oid.T_timestamp, 0, 0, emptyLocale)}, {MakeTimestamp(0), &T{InternalType: InternalType{ Family: TimestampFamily, @@ -248,6 +282,12 @@ func TestTypes(t *testing.T) { {MakeTimestamp(6), MakeScalar(TimestampFamily, oid.T_timestamp, 6, 0, emptyLocale)}, // TIMESTAMPTZ + {TimestampTZ, &T{InternalType: InternalType{ + Family: TimestampTZFamily, + Oid: oid.T_timestamptz, + Locale: &emptyLocale, + // Precision and PrecisionIsSet is not set. + }}}, {MakeTimestampTZ(0), MakeScalar(TimestampTZFamily, oid.T_timestamptz, 0, 0, emptyLocale)}, {MakeTimestampTZ(0), &T{InternalType: InternalType{ Family: TimestampTZFamily, diff --git a/pkg/util/encoding/encoding_test.go b/pkg/util/encoding/encoding_test.go index 404ce039e71a..35250c3964cd 100644 --- a/pkg/util/encoding/encoding_test.go +++ b/pkg/util/encoding/encoding_test.go @@ -1114,7 +1114,7 @@ func TestEncodeDecodeTimeTZ(t *testing.T) { t.Run(fmt.Sprintf("dir:%d", dir), func(t *testing.T) { for i := range testCases { t.Run(fmt.Sprintf("tc:%d", i), func(t *testing.T) { - current, err := timetz.ParseTimeTZ(timeutil.Now(), testCases[i]) + current, err := timetz.ParseTimeTZ(timeutil.Now(), testCases[i], time.Microsecond) assert.NoError(t, err) var b []byte diff --git a/pkg/util/timeofday/time_of_day.go b/pkg/util/timeofday/time_of_day.go index d8222fa0f997..c4cf4620dcf1 100644 --- a/pkg/util/timeofday/time_of_day.go +++ b/pkg/util/timeofday/time_of_day.go @@ -97,6 +97,20 @@ func Random(rng *rand.Rand) TimeOfDay { return TimeOfDay(rng.Int63n(microsecondsPerDay)) } +// Round takes a TimeOfDay, and rounds it to the given precision. +func (t TimeOfDay) Round(precision time.Duration) TimeOfDay { + if t == Time2400 { + return t + } + ret := t.ToTime().Round(precision) + // Rounding Max should give Time2400, not 00:00. + // To catch this, see if we are comparing against the same day. + if ret.Day() != t.ToTime().Day() { + return Time2400 + } + return FromTime(ret) +} + // Add adds a Duration to a TimeOfDay, wrapping into the next day if necessary. func (t TimeOfDay) Add(d duration.Duration) TimeOfDay { return FromInt(int64(t) + d.Nanos()/nanosPerMicro) diff --git a/pkg/util/timeofday/time_of_day_test.go b/pkg/util/timeofday/time_of_day_test.go index 065a92c0ee11..49d5b993645a 100644 --- a/pkg/util/timeofday/time_of_day_test.go +++ b/pkg/util/timeofday/time_of_day_test.go @@ -66,6 +66,28 @@ func TestFromAndToTime(t *testing.T) { } } +func TestRound(t *testing.T) { + testData := []struct { + t TimeOfDay + round time.Duration + exp TimeOfDay + }{ + {New(12, 0, 0, 1000), time.Second, New(12, 0, 0, 0)}, + {New(12, 0, 0, 1000), time.Millisecond, New(12, 0, 0, 1000)}, + {Max, time.Second, Time2400}, + {Time2400, time.Second, Time2400}, + {Min, time.Second, Min}, + } + for _, td := range testData { + t.Run(fmt.Sprintf("%s,%s", td.t, td.round), func(t *testing.T) { + actual := td.t.Round(td.round) + if actual != td.exp { + t.Errorf("expected %s, got %s", td.exp, actual) + } + }) + } +} + func TestAdd(t *testing.T) { testData := []struct { t TimeOfDay diff --git a/pkg/util/timetz/timetz.go b/pkg/util/timetz/timetz.go index ef3075134a0e..f7559f01b03f 100644 --- a/pkg/util/timetz/timetz.go +++ b/pkg/util/timetz/timetz.go @@ -75,7 +75,7 @@ func Now() TimeTZ { // ParseTimeTZ parses and returns the TimeTZ represented by the // provided string, or an error if parsing is unsuccessful. -func ParseTimeTZ(now time.Time, s string) (TimeTZ, error) { +func ParseTimeTZ(now time.Time, s string, precision time.Duration) (TimeTZ, error) { // Special case as we have to use `ParseTimestamp` to get the date. // We cannot use `ParseTime` as it does not have timezone awareness. if s == "" { @@ -95,7 +95,7 @@ func ParseTimeTZ(now time.Time, s string) (TimeTZ, error) { s, ) } - retTime := timeofday.FromTime(t) + retTime := timeofday.FromTime(t.Round(precision)) // Special case on 24:00 and 24:00:00 as the parser // does not handle these correctly. if timeTZMaxTimeRegex.MatchString(s) { @@ -143,6 +143,11 @@ func (t *TimeTZ) ToTime() time.Time { return t.TimeOfDay.ToTime().Add(time.Duration(t.OffsetSecs) * time.Second).In(loc) } +// Round rounds a DTimeTZ to the given duration. +func (t *TimeTZ) Round(precision time.Duration) TimeTZ { + return MakeTimeTZ(t.TimeOfDay.Round(precision), t.OffsetSecs) +} + // Before returns whether the current is before the other TimeTZ. func (t *TimeTZ) Before(other TimeTZ) bool { return t.ToTime().Before(other.ToTime()) || (t.ToTime().Equal(other.ToTime()) && t.OffsetSecs < other.OffsetSecs) diff --git a/pkg/util/timetz/timetz_test.go b/pkg/util/timetz/timetz_test.go index d6cfd472c867..4b8b1c213df2 100644 --- a/pkg/util/timetz/timetz_test.go +++ b/pkg/util/timetz/timetz_test.go @@ -30,10 +30,10 @@ func TestParseTimeTZToStringRoundTrip(t *testing.T) { } for _, tc := range testCases { t.Run(tc, func(t *testing.T) { - exampleTime, err := ParseTimeTZ(timeutil.Now(), tc) + exampleTime, err := ParseTimeTZ(timeutil.Now(), tc, time.Microsecond) assert.NoError(t, err) - exampleTimeFromString, err := ParseTimeTZ(timeutil.Now(), exampleTime.String()) + exampleTimeFromString, err := ParseTimeTZ(timeutil.Now(), exampleTime.String(), time.Microsecond) assert.NoError(t, err) assert.True(t, exampleTime.Equal(exampleTimeFromString)) @@ -63,15 +63,18 @@ func TestTimeTZString(t *testing.T) { } func TestTimeTZ(t *testing.T) { - maxTime, err := ParseTimeTZ(timeutil.Now(), "24:00:00-1559") + maxTime, err := ParseTimeTZ(timeutil.Now(), "24:00:00-1559", time.Microsecond) require.NoError(t, err) - minTime, err := ParseTimeTZ(timeutil.Now(), "00:00:00+1559") + minTime, err := ParseTimeTZ(timeutil.Now(), "00:00:00+1559", time.Microsecond) require.NoError(t, err) // These are all the same UTC time equivalents. - utcTime, err := ParseTimeTZ(timeutil.Now(), "11:14:15+0") + utcTime, err := ParseTimeTZ(timeutil.Now(), "11:14:15+0", time.Microsecond) require.NoError(t, err) - sydneyTime, err := ParseTimeTZ(timeutil.Now(), "21:14:15+10") + sydneyTime, err := ParseTimeTZ(timeutil.Now(), "21:14:15+10", time.Microsecond) + require.NoError(t, err) + + sydneyTimeWithMillisecond, err := ParseTimeTZ(timeutil.Now(), "21:14:15.001+10", time.Microsecond) require.NoError(t, err) // No daylight savings in Hawaii! @@ -82,53 +85,68 @@ func TestTimeTZ(t *testing.T) { weirdTimeZone := MakeTimeTZ(timeofday.New(10, 0, 0, 0), -((5 * 60 * 60) + 30*60 + 15)) testCases := []struct { - t TimeTZ - toTime time.Time - largerThan []TimeTZ - smallerThan []TimeTZ - equalTo []TimeTZ + t TimeTZ + toTime time.Time + largerThan []TimeTZ + smallerThan []TimeTZ + equalTo []TimeTZ + roundedToSecond TimeTZ }{ { - t: weirdTimeZone, - toTime: time.Date(1970, 1, 1, 10, 0, 0, 0, timeutil.FixedOffsetTimeZoneToLocation((5*60*60)+(30*60)+15, "TimeTZ")), - largerThan: []TimeTZ{minTime}, - smallerThan: []TimeTZ{maxTime}, - equalTo: []TimeTZ{weirdTimeZone}, + t: weirdTimeZone, + toTime: time.Date(1970, 1, 1, 10, 0, 0, 0, timeutil.FixedOffsetTimeZoneToLocation((5*60*60)+(30*60)+15, "TimeTZ")), + largerThan: []TimeTZ{minTime}, + smallerThan: []TimeTZ{maxTime}, + equalTo: []TimeTZ{weirdTimeZone}, + roundedToSecond: weirdTimeZone, + }, + { + t: utcTime, + toTime: time.Date(1970, 1, 1, 11, 14, 15, 0, timeutil.FixedOffsetTimeZoneToLocation(0, "TimeTZ")), + largerThan: []TimeTZ{minTime, sydneyTime}, + smallerThan: []TimeTZ{maxTime, hawaiiTime}, + equalTo: []TimeTZ{utcTime}, + roundedToSecond: utcTime, }, { - t: utcTime, - toTime: time.Date(1970, 1, 1, 11, 14, 15, 0, timeutil.FixedOffsetTimeZoneToLocation(0, "TimeTZ")), - largerThan: []TimeTZ{minTime, sydneyTime}, - smallerThan: []TimeTZ{maxTime, hawaiiTime}, - equalTo: []TimeTZ{utcTime}, + t: sydneyTime, + toTime: time.Date(1970, 1, 1, 21, 14, 15, 0, timeutil.FixedOffsetTimeZoneToLocation(10*60*60, "TimeTZ")), + largerThan: []TimeTZ{minTime}, + smallerThan: []TimeTZ{maxTime, utcTime, hawaiiTime}, + equalTo: []TimeTZ{sydneyTime}, + roundedToSecond: sydneyTime, }, { - t: sydneyTime, - toTime: time.Date(1970, 1, 1, 21, 14, 15, 0, timeutil.FixedOffsetTimeZoneToLocation(10*60*60, "TimeTZ")), - largerThan: []TimeTZ{minTime}, - smallerThan: []TimeTZ{maxTime, utcTime, hawaiiTime}, - equalTo: []TimeTZ{sydneyTime}, + t: sydneyTimeWithMillisecond, + toTime: time.Date(1970, 1, 1, 21, 14, 15, 1000000, timeutil.FixedOffsetTimeZoneToLocation(10*60*60, "TimeTZ")), + largerThan: []TimeTZ{minTime, utcTime, hawaiiTime, sydneyTime}, + smallerThan: []TimeTZ{maxTime}, + equalTo: []TimeTZ{sydneyTimeWithMillisecond}, + roundedToSecond: sydneyTime, }, { - t: hawaiiTime, - toTime: time.Date(1970, 1, 1, 1, 14, 15, 0, timeutil.FixedOffsetTimeZoneToLocation(-10*60*60, "TimeTZ")), - largerThan: []TimeTZ{minTime, utcTime, sydneyTime}, - smallerThan: []TimeTZ{maxTime}, - equalTo: []TimeTZ{hawaiiTime}, + t: hawaiiTime, + toTime: time.Date(1970, 1, 1, 1, 14, 15, 0, timeutil.FixedOffsetTimeZoneToLocation(-10*60*60, "TimeTZ")), + largerThan: []TimeTZ{minTime, utcTime, sydneyTime}, + smallerThan: []TimeTZ{maxTime}, + equalTo: []TimeTZ{hawaiiTime}, + roundedToSecond: hawaiiTime, }, { - t: minTime, - toTime: time.Date(1970, 1, 1, 0, 0, 0, 0, timeutil.FixedOffsetTimeZoneToLocation(15*60*60+59*60, "TimeTZ")), - largerThan: []TimeTZ{}, - smallerThan: []TimeTZ{maxTime, utcTime, sydneyTime, hawaiiTime}, - equalTo: []TimeTZ{minTime}, + t: minTime, + toTime: time.Date(1970, 1, 1, 0, 0, 0, 0, timeutil.FixedOffsetTimeZoneToLocation(15*60*60+59*60, "TimeTZ")), + largerThan: []TimeTZ{}, + smallerThan: []TimeTZ{maxTime, utcTime, sydneyTime, hawaiiTime}, + equalTo: []TimeTZ{minTime}, + roundedToSecond: minTime, }, { - t: maxTime, - toTime: time.Date(1970, 1, 2, 0, 0, 0, 0, timeutil.FixedOffsetTimeZoneToLocation(-(15*60*60+59*60), "TimeTZ")), - largerThan: []TimeTZ{minTime, utcTime, sydneyTime, hawaiiTime}, - smallerThan: []TimeTZ{}, - equalTo: []TimeTZ{maxTime}, + t: maxTime, + toTime: time.Date(1970, 1, 2, 0, 0, 0, 0, timeutil.FixedOffsetTimeZoneToLocation(-(15*60*60+59*60), "TimeTZ")), + largerThan: []TimeTZ{minTime, utcTime, sydneyTime, hawaiiTime}, + smallerThan: []TimeTZ{}, + equalTo: []TimeTZ{maxTime}, + roundedToSecond: maxTime, }, } for i, tc := range testCases { @@ -146,33 +164,41 @@ func TestTimeTZ(t *testing.T) { for _, equalTo := range tc.equalTo { assert.True(t, tc.t.Equal(equalTo), "%s = %s", tc.t.String(), equalTo) } + + assert.Equal(t, tc.roundedToSecond, tc.t.Round(time.Second)) }) } } func TestParseTimeTZ(t *testing.T) { testCases := []struct { - str string + str string + precision time.Duration + expected TimeTZ expectedError bool }{ - {str: "01:02:03", expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 0)}, - {str: "01:02:03.000123", expected: MakeTimeTZ(timeofday.New(1, 2, 3, 123), 0)}, - {str: "4:5:6", expected: MakeTimeTZ(timeofday.New(4, 5, 6, 0), 0)}, - {str: "24:00", expected: MakeTimeTZ(timeofday.Time2400, 0)}, - {str: "24:00:00", expected: MakeTimeTZ(timeofday.Time2400, 0)}, - {str: "24:00:00.000", expected: MakeTimeTZ(timeofday.Time2400, 0)}, - {str: "24:00:00.000000", expected: MakeTimeTZ(timeofday.Time2400, 0)}, - {str: "01:02:03+13", expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), -13*60*60)}, - {str: "01:02:03-13", expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 13*60*60)}, - {str: "01:02:03+7", expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), -7*60*60)}, - {str: "01:02:03-0730", expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 7*60*60+30*60)}, - {str: "24:00+3", expected: MakeTimeTZ(timeofday.Time2400, -3*60*60)}, - {str: "24:00:00+4", expected: MakeTimeTZ(timeofday.Time2400, -4*60*60)}, - {str: "24:00:00.000-5", expected: MakeTimeTZ(timeofday.Time2400, 5*60*60)}, - {str: "24:00:00.000000+6", expected: MakeTimeTZ(timeofday.Time2400, -6*60*60)}, - {str: "00:00-1559", expected: MakeTimeTZ(timeofday.New(0, 0, 0, 0), MaxTimeTZOffsetSecs)}, - {str: "00:00+1559", expected: MakeTimeTZ(timeofday.New(0, 0, 0, 0), MinTimeTZOffsetSecs)}, + {str: "01:02:03", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 0)}, + {str: "01:02:03.000123", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 123), 0)}, + {str: "4:5:6", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(4, 5, 6, 0), 0)}, + {str: "24:00", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, 0)}, + {str: "24:00:00", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, 0)}, + {str: "24:00:00.000", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, 0)}, + {str: "24:00:00.000000", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, 0)}, + {str: "01:02:03+13", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), -13*60*60)}, + {str: "01:02:03-13", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 13*60*60)}, + {str: "01:02:03+7", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), -7*60*60)}, + {str: "01:02:03-0730", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 7*60*60+30*60)}, + {str: "24:00+3", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, -3*60*60)}, + {str: "24:00:00+4", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, -4*60*60)}, + {str: "24:00:00.000-5", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, 5*60*60)}, + {str: "24:00:00.000000+6", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.Time2400, -6*60*60)}, + {str: "00:00-1559", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(0, 0, 0, 0), MaxTimeTZOffsetSecs)}, + {str: "00:00+1559", precision: time.Microsecond, expected: MakeTimeTZ(timeofday.New(0, 0, 0, 0), MinTimeTZOffsetSecs)}, + + {str: "01:02:03.000123", precision: time.Millisecond, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 0), 0)}, + {str: "01:02:03.000123", precision: time.Millisecond / 10, expected: MakeTimeTZ(timeofday.New(1, 2, 3, 100), 0)}, + {str: "01:02:03.500123", precision: time.Second, expected: MakeTimeTZ(timeofday.New(1, 2, 4, 0), 0)}, {str: "", expectedError: true}, {str: "foo", expectedError: true}, @@ -183,7 +209,7 @@ func TestParseTimeTZ(t *testing.T) { } for i, tc := range testCases { t.Run(fmt.Sprintf("#%d: %s", i, tc.str), func(t *testing.T) { - actual, err := ParseTimeTZ(timeutil.Now(), tc.str) + actual, err := ParseTimeTZ(timeutil.Now(), tc.str, tc.precision) if tc.expectedError { assert.Error(t, err) } else {