sql: add precision support for TIMESTAMP/TIMESTAMPTZ#42552
sql: add precision support for TIMESTAMP/TIMESTAMPTZ#42552otan wants to merge 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
rohany
left a comment
There was a problem hiding this comment.
A few comments, but I'd also like @solongordon to take a look at this PR just in case.
Reviewed 11 of 13 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan and @rohany)
pkg/sql/sem/tree/time.go, line 23 at r1 (raw file):
// duration to use to pass into time.Truncate to truncate to that duration. // Panics if the precision is not supported. func precisionToTruncDuration(precision int32) time.Duration {
i feel like this should be a method on the DTimeStamp and DTimeStampTZ so that its clear this is only meant to be used on timestamps. or maybe just change the name so its clear for use on the time precisions only.
pkg/sql/sem/tree/time.go, line 26 at r1 (raw file):
switch precision { case types.DefaultTimePrecision: return time.Microsecond
maybe make this a constant too, time.DefaultPrecision or something
pkg/sql/sem/tree/time.go, line 30 at r1 (raw file):
return time.Second case 1: return time.Millisecond * 100
tfw time.Decisecond is missing
pkg/sql/types/types.go, line 677 at r1 (raw file):
return Time } if precision != 6 {
whats the reason we aren't also adding precision to the time type?
pkg/sql/types/types.go, line 837 at r1 (raw file):
// PrecisionExpandDefault returns the Precision, but for default // values it will return the precision with default set. func (t *T) PrecisionExpandDefault() int32 {
it looks like this is unused
pkg/sql/types/types_test.go, line 227 at r1 (raw file):
// TIMESTAMP {MakeTimestamp(-1), Timestamp}, {MakeTimestamp(-1), MakeScalar(TimestampFamily, oid.T_timestamp, -1, 0, emptyLocale)},
is there some way you can explicitly test that the 19.2 disk table descriptor is read correctly?
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/time.go, line 23 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
i feel like this should be a method on the DTimeStamp and DTimeStampTZ so that its clear this is only meant to be used on timestamps. or maybe just change the name so its clear for use on the time precisions only.
I will change the naming.
pkg/sql/sem/tree/time.go, line 26 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
maybe make this a constant too, time.DefaultPrecision or something
Done.
pkg/sql/sem/tree/time.go, line 30 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
tfw time.Decisecond is missing
:P
pkg/sql/types/types.go, line 677 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
whats the reason we aren't also adding precision to the time type?
Needed more plumbing, and it's a separate issue. Was planning to do as a follow up.
pkg/sql/types/types.go, line 837 at r1 (raw file):
PrecisionExpandDefault
should be
pkg/sql/schemachange/alter_column_type.go
148: oldPrecision := oldType.PrecisionExpandDefault()
149: newPrecision := newType.PrecisionExpandDefault()
pkg/sql/types/types_test.go, line 227 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
is there some way you can explicitly test that the 19.2 disk table descriptor is read correctly?
I'm not sure, suggestions welcome.
Worst case, I can manually try upgrading... (if there's a wiki to do that lmk)
rohany
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/time.go, line 26 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
Done.
This doesn't look changed?
pkg/sql/types/types.go, line 837 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
PrecisionExpandDefaultshould be
pkg/sql/schemachange/alter_column_type.go
148: oldPrecision := oldType.PrecisionExpandDefault()
149: newPrecision := newType.PrecisionExpandDefault()
ah cmd f is lying to me
pkg/sql/types/types_test.go, line 227 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
I'm not sure, suggestions welcome.
Worst case, I can manually try upgrading... (if there's a wiki to do that lmk)
i guess what would convince me is explicitly make the type as it was before this commit (without the precision), and make sure that it reads the default precision.
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sem/tree/time.go, line 26 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
This doesn't look changed?
sigh, didn't press save.
pkg/sql/types/types.go, line 837 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
ah cmd f is lying to me
Done.
pkg/sql/types/types_test.go, line 227 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
i guess what would convince me is explicitly make the type as it was before this commit (without the precision), and make sure that it reads the default precision.
I can try the manual upgrade technique in lieu of making a test suite of upgrading versions of cockroach :)
rohany
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/types/types_test.go, line 227 at r1 (raw file):
Previously, otan (Oliver Tan) wrote…
I can try the manual upgrade technique in lieu of making a test suite of upgrading versions of cockroach :)
sorry i wasn't clear -- i meant in this test, make an instance of the timestamp struct as it was defined before this commit -- (without the precision)
otan
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/types/types_test.go, line 227 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
sorry i wasn't clear -- i meant in this test, make an instance of the timestamp struct as it was defined before this commit -- (without the precision)
Done.
Add precision support for TIMESTAMP/TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. When storing types, we use the value `0` to mean `DEFAULT` (which will be a precision of 6). A precision of `0` will be stored as `-1`. This allows backwards compatibility with older versions of TIMESTAMP/TIMESTAMPTZ, which had a precision of 0 when persisted. This detail is abstracted away in the Make* and Precision() functions. Release note (sql change): Introduces precision support for TIMESTAMP and TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. Previous versions of TIMESTAMP and TIMESTAMPTZ will default to 6 units of precision.
|
just realised this ain't backwards compatible, so going to close this off for review for a second. |
|
will note it in the PR message |
|
Oops, had another idea. Gimme a sec. |
41788: storage/rangefeed: push rangefeed.Filter into Raft processing goroutine r=danhhz a=nvanbenschoten This commit creates a `rangefeed.Filter` object, which informs the producer of logical operations of the information that a `rangefeed.Processor` is interested in, given its current set of registrations. It can be used to avoid performing extra work to provide the Processor with information which will be ignored. This is an important optimization for single-key or small key span rangefeeds, as it avoids some extraneous work for the key spans not being watched. For now, this extra work is just fetching the post-operation value for `MVCCWriteValueOp` and `MVCCCommitIntentOp` operations, but this can be extended in the future to include the pre-operation value as well. This will be important to avoid any perf regression when addressing #28666. 42580: sql: add precision support for TIMESTAMP/TIMESTAMPTZ (v2) r=otan a=otan Refs #32098 Alternative to #42552 ---- Add precision support for TIMESTAMP/TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. When storing this in the type proto, we introduce a `TimePrecisionIsSet` variable, which is used to differentiate 0 (unset) or 0 (explicitly set) for time related protos. This is abstracted away in the `Precision()` function which returns the correct precision (unset => 6) for time. This allows forwards and backwards compatibility. Note Time will come later, as it involves a bit more plumbing to the base library. Release note (sql change): Introduces precision support for TIMESTAMP and TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. Previous versions of TIMESTAMP and TIMESTAMPTZ will default to 6 units of precision. Note that if anyone downgrades whilst having precision set, they will have full precision (6) again, but if they re-upgrade they will find their precisions truncated again. Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Resolves #32098.
Add precision support for TIMESTAMP/TIMESTAMPTZ, supporting precisions
from 0 to 6 inclusive.
When storing types, we use the value
0to meanDEFAULT(which willbe a precision of 6). A precision of
0will be stored as-1. Thisallows backwards compatibility with older versions of
TIMESTAMP/TIMESTAMPTZ, which had a precision of 0 when persisted.
This detail is abstracted away in the Make* and Precision() functions.
Note this change is not backwards compatible for those who have set
TIMESTAMP(0)/TIMESTAMPTZ(0), as this is stored as-1. If someonedoes this in v20.1, it does not cover them well in case of a downgrade back
to v19.2.
Release note (sql change): Introduces precision support for TIMESTAMP
and TIMESTAMPTZ, supporting precisions from 0 to 6 inclusive. Previous
versions of TIMESTAMP and TIMESTAMPTZ will default to 6 units of
precision.