Add Date, Timestamp and Interval types. #2314
Conversation
|
This change is a first step towards introducing Date, Timestamp and Interval types. It still requires values to be cast when specifying them in sql to convert them to the correct type. |
vivekmenezes
force-pushed
the
vivek/time
branch
from
2ca8ee2
to
a77e8be
Compare
|
Hmmm, I can't use time.Time to represent a Date. I'm already hitting truncation related inaccuracies. I'll change the representation. |
|
|
||
| func (d DDate) String() string { | ||
| year, month, day := d.Date() | ||
| return fmt.Sprintf("%4d-%02d-%02d", year, month, day) |
There was a problem hiding this comment.
This is simply and obvious, though I'm wondering if you should use Time.Format() instead.
|
Generally looks good. I'll need to give this another pass later as it is quite a substantial addition. Pretty nice how the date/timestamp/interval stuff maps to As a follow-up, it would be good to add support for various time related functions such as |
| } | ||
| t, err = time.Parse(timestampWithNamedZoneFormat, str) | ||
| if err == nil { | ||
| // This is more involved. See https://github.com/golang/go/issues/12388 |
There was a problem hiding this comment.
Seems like the bug you're pointing to only affects the Go playground. Am I missing something?
There was a problem hiding this comment.
A couple of issues here. A shortform like PST can be interpreted in two ways PST US and PST phillipines
See:
http://www.timeanddate.com/time/zones/
Secondly GO doesn't reliably parse and interpret these without passing in a location. There is no way to load the entire database and parse. Also it's not clear whether we want to do that to support this feature.
I think we want to have the user SET TIME ZONE and use that that time zone to time.ParseInLocation()
There was a problem hiding this comment.
That issue doesn't seem to provide useful context here. Can you update this comment?
There was a problem hiding this comment.
Updated the comment and added a TODO.
| @@ -441,15 +589,27 @@ var binOps = map[binArgs]func(Datum, Datum) (Datum, error){ | |||
|
|
|||
| // TODO(pmattis): Overflow/underflow checks? | |||
|
|
|||
| // TODO(pmattis): Should we allow the implicit conversion from int to float | |||
There was a problem hiding this comment.
the issue is closed
There was a problem hiding this comment.
this TODO is clearly not resolved
There was a problem hiding this comment.
Perhap it needs to be reworded. The implicit casts were removed in
Did you have anything specific in mind?
There was a problem hiding this comment.
Ah, this comment is ambiguous. I was thinking it was referring to the (int, int) case which still casts to floats. I guess this is OK to remove.
|
I'm not sure about the way we're handling time zones here. MySQL's timestamps do not contain any time zone information; postgres has a separate type for "timestamp with time zone", but they recommend not using it (a UTC offset isn't enough; real time-zone aware applications need to know the time zone name, not just a single offset). I think we should probably store timestamps as UTC with no offset information; supporting time zones properly will require a lot more thought. |
| // IsMin implements the Datum interface. | ||
| func (d DTimestamp) IsMin() bool { | ||
| // Subtracting 1 underflows to a larger value. | ||
| return d.Time.Before(d.Time.Add(-1)) |
vivekmenezes
force-pushed
the
vivek/time
branch
from
69bdf7f
to
c62a8d0
Compare
|
This turned out to be a non-trivial rebase, so I went ahead and did it. It's on my fork here: https://github.com/tamird/cockroach/tree/vivek/time. To get it: |
vivekmenezes
force-pushed
the
vivek/time
branch
from
c62a8d0
to
8c235c4
Compare
| {`1::decimal`, `invalid cast: int -> DECIMAL`}, | ||
| {`1::date`, `invalid cast: int -> DATE`}, | ||
| {`1::time`, `invalid cast: int -> TIME`}, | ||
| {`1::timestamp`, `invalid cast: int -> TIMESTAMP`}, |
There was a problem hiding this comment.
I've taken these tests out because they are implemented elsewhere
This does add in the three types but the usage of these types is rather restrictive (you need to explictly cast a string to these types). Making these types more user friendly will be implemented as a followup.
vivekmenezes
force-pushed
the
vivek/time
branch
from
cd31e0f
to
230d105
Compare
|
LGTM |
Add Date, Timestamp and Interval types.
No description provided.