sql: add precision support for TIMESTAMP/TIMESTAMPTZ (v2)#42580
sql: add precision support for TIMESTAMP/TIMESTAMPTZ (v2)#42580craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
7b15353 to
58237f4
Compare
acba26b to
9834dca
Compare
solongordon
left a comment
There was a problem hiding this comment.
A few nits but generally looks great!
Reviewed 19 of 19 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @otan and @rohany)
pkg/sql/sem/tree/datum.go, line 2118 at r1 (raw file):
} // Round returns a new DTimestampTZ to the specified precision.
Nit: DTimestamp
pkg/sql/sem/tree/time_test.go, line 21 at r1 (raw file):
) func TestPrecisionToTruncDuration(t *testing.T) {
Nit: misnamed?
pkg/sql/types/types.go, line 701 at r1 (raw file):
// the given number of fractional second digits. // // If precision is -1, it will return the default precision.
I may be missing something but what is the purpose of accepting -1? Seems like it is only passed by tests, which could just use Timestamp instead.
pkg/sql/types/types.go, line 720 at r1 (raw file):
// most the given number of fractional second digits. // // If precision is -1, it will return the default precision.
Same question about accepting -1.
pkg/sql/types/types.proto, line 408 at r1 (raw file):
// * If Precision is 0, it will default to 6 if TimePrecisionIsSet is false // (for compatibility reasons). // * Otherwise, the precision is 6.
Did you mean 0?
|
One more thought: when we discovered that the previous implementation of this was not backward compatible, @andy-kimball highlighted that we have |
otan
left a comment
There was a problem hiding this comment.
Ah, that would've been useful as well, but I think with the way this is done we don't have to bother with doing that.
Reviewable status:
pkg/sql/types/types.go, line 701 at r1 (raw file):
Previously, solongordon (Solon) wrote…
I may be missing something but what is the purpose of accepting -1? Seems like it is only passed by tests, which could just use
Timestampinstead.
Yeah I think that's cleaner. changing!
pkg/sql/types/types.go, line 720 at r1 (raw file):
Previously, solongordon (Solon) wrote…
Same question about accepting -1.
Done.
pkg/sql/types/types.proto, line 408 at r1 (raw file):
Previously, solongordon (Solon) wrote…
Did you mean 0?
Yes. Fixed!
9834dca to
8d8e303
Compare
solongordon
left a comment
There was a problem hiding this comment.
Nit: Your commit message still references the -1 business.
Reviewed 2 of 6 files at r2.
Reviewable status:
pkg/sql/types/types.go, line 711 at r2 (raw file):
// most the given number of fractional second digits. // // To use the default, use the `Timestamp` variable.
Nit: TimestampTZ
solongordon
left a comment
There was a problem hiding this comment.
And I believe the intention is to backport this fix to the release-19.2 branch. (See https://github.com/benesch/backport to make this process easy.)
Reviewable status:
8d8e303 to
1b5668d
Compare
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.
1b5668d to
a8994c9
Compare
otan
left a comment
There was a problem hiding this comment.
Are we sure about backporting this? Seems like a strictly new feature instead of a bug fix?
Reviewable status:
|
we can discuss the backporting afterwards - thanks for the review! bors r+ |
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>
Build succeeded |
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
TimePrecisionIsSetvariable, 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.