Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: civil time package #19700

Open
jba opened this issue Mar 24, 2017 · 58 comments
Open

proposal: civil time package #19700

jba opened this issue Mar 24, 2017 · 58 comments
Labels
Projects
Milestone

Comments

@jba
Copy link
Contributor

@jba jba commented Mar 24, 2017

I propose a package with minimal implementations of the types Date, Time and DateTime, which represent times without a corresponding location.

A civil time or date does not represent a point or interval of time, but they are useful for representing events transpiring between humans. For example, your birthday begins at midnight on your birthdate regardless of where you are in the world. If you're turning 21, you can buy a drink in New York at midnight, but teleport instantaneously to San Francisco and you'll be denied, because it is 9 PM the day before.

In practice, the main motivation for these types is to represent values in database-like storage systems, like BigQuery and Spanner (and other, non-Google products).

The package currently exists at cloud.google.com/go/civil, and has been in use by the BigQuery and Spanner client libraries for a few months. For now, I'd like to move it to golang.org/x/time/civil. It is probably too esoteric to be worth including in the standard library, but if there were ever a "second-tier" set of packages that augmented the standard library, it could live there. (See #17244.)

A CL is in progress at https://go-review.googlesource.com/c/38571.

@rakyll rakyll added the Proposal label Mar 24, 2017
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 24, 2017

If you're turning 18, you can buy a drink in New York at midnight, but teleport instantaneously to San Francisco and you'll be denied, because it is 9 PM the day before.

And because the drinking age in California is 21. The ballot measure never happened.

@gopherbot gopherbot added this to the Proposal milestone Mar 24, 2017
@jba
Copy link
Contributor Author

@jba jba commented Mar 24, 2017

21 in NY too, it turns out. Edited comment.

@kevinburke
Copy link
Contributor

@kevinburke kevinburke commented Mar 26, 2017

fun story... the drinking age is 21 in every state because of a law that withheld federal highway money unless states mandated 21 as the drinking age. https://en.wikipedia.org/wiki/National_Minimum_Drinking_Age_Act

@rsc
Copy link
Contributor

@rsc rsc commented Mar 27, 2017

On hold for #17244, which in turn is essentially blocked on understanding the long-term plan for package management. Soon.

@FlorianUekermann
Copy link
Contributor

@FlorianUekermann FlorianUekermann commented Sep 21, 2017

I left a comment on the implementation in the CL. I'll mirror the parts that are not about implementation details here:

In my opinion time.Duration already implements a civil time (see #20757 for a list of issues that may be interesting).

Please clarify the difference between a civil date and just a normal date (dates don't have timezone issues to my knowledge, especially because they don't represent a moment in time).
If a civil date is just a date I don't think creating a civil package is appropriate. A simple date package or adding a Date type to the time package would be sufficient. The latter would also be useful for reusing unexported functionality from time.

There are multiple tiny "date" packages that look more or less the same (I implemented one here https://godoc.org/github.com/infobaleen/date, but there are others which are practically identical).

@jba
Copy link
Contributor Author

@jba jba commented Sep 25, 2017

time.Duration paired with a reference time could represent a civil DateTime (not a civil Time, which is an odd beast that is nevertheless present in many SQL versions). We'd still want another type for DateTime because of the reference time. Also, the range of time.Duration is about 290 years, not nearly long enough for the range of applications we'd like to support (Sumerian calendars to Star Trek chronologies).

Since we need a civil DateTime type, and a civil Time type to support SQL, it seems reasonable to put the Date type in the same package with them. Since the time package is big enough as it is, a small, separate package makes sense.

@FlorianUekermann
Copy link
Contributor

@FlorianUekermann FlorianUekermann commented Sep 30, 2017

time.Duration paired with a reference time could represent a civil DateTime. . We'd still want another type for DateTime because of the reference time.

type Civil struct{ Date time.Date; Time time.Duration }, assuming a time.Date type (#21365) is implemented. Basically just two int64s in a struct.

Also, the range of time.Duration is about 290 years, not nearly long enough for the range of applications we'd like to support (Sumerian calendars to Star Trek chronologies).

Maybe I misunderstood the goal, as I don't see the range problem you are mentioning. Are you considering other calendars where a day doesn't have 24*60*60*1e9 nanoseconds?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 30, 2017

Maybe I misunderstood the goal, as I don't see the range problem you are mentioning. Are you considering other calendars where a day doesn't have 246060*1e9 nanoseconds?

Other calendars are not a goal. The time package documents that:

The calendrical calculations always assume a Gregorian calendar, with no leap seconds.

@jba
Copy link
Contributor Author

@jba jba commented Oct 3, 2017

type Civil struct{ Date time.Date; Time time.Duration }, assuming a time.Date type (#21365) is implemented. Basically just two int64s in a struct.

Where does that type live? And what about the type that just represents a time of day (civil.TIme in my package)?

Maybe I misunderstood the goal, as I don't see the range problem you are mentioning. Are you considering other calendars where a day doesn't have 246060*1e9 nanoseconds?

I didn't understand your proposal. I thought you wanted to represent a DateTime as a duration from some single reference time, like Unix represents time as an offset from 1/1/1970.

@FlorianUekermann
Copy link
Contributor

@FlorianUekermann FlorianUekermann commented Oct 3, 2017

Where does that type live?

I don't have a strong opinion on that. I would suggest putting it in "exp" first and integrating it in "time" later

And what about the type that just represents a time of day (civil.TIme in my package)?

time.Duration is sufficient (look at it's metods). Maybe alias it.
If you need something to support SQL, I think that belongs in/under the sql package. The concept of civil Date and DateTime are useful in a much wider scope, that can be supported mote easily.

@lpar
Copy link

@lpar lpar commented Apr 13, 2018

I've also needed a pure Date for SQL and other things, but I implemented it by wrapping time.Time. Might that be a better implementation approach?

@Vitucho
Copy link

@Vitucho Vitucho commented May 21, 2018

i don't understand why is duplicate "time: add weekday helper functions #25469" (btw please see my last comment)... please don't take me wrong i only try to understand how do you think.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 26, 2019

Change https://golang.org/cl/38571 mentions this issue: civil: types for civil time

@jba
Copy link
Contributor Author

@jba jba commented Feb 28, 2019

If nothing is blocking this any longer, I'd like to proceed.

I appreciate the suggestions for alternative implementations (wrapping time.Time or using offsets from a base). They would support faster comparison and addition operations at the cost of slower construction time. I propose unexporting the civil.Date and civil.Time fields and adding getter methods, to allow for future change of implementation.

@sandipbgt

This comment was marked as off-topic.

@jba

This comment was marked as off-topic.

@fgblomqvist
Copy link

@fgblomqvist fgblomqvist commented Apr 10, 2019

What's the status of this? I'm a bit unfamiliar with how these CLs work, but it looks like it hasn't seen any activity for about a month. Is it just waiting for some final confirmation on its inclusion? Right now we're importing the google package, which is a huge import, just to get access to the civil one (can't get subdirectory importing to work with go modules).

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 10, 2019

The proposal of creating golang.org/x/time/civil is on hold until #17244 is decided. And #17244 is on hold until the modules work settles, which may happen in 1.13.

@lpar
Copy link

@lpar lpar commented Jul 22, 2019

I'd just like to put in a word for having both zoned and unzoned civil datetime types. For calendars, you want to be able to support implicit time zone, as well as explicit.

@jba
Copy link
Contributor Author

@jba jba commented Jul 22, 2019

Isn't a zoned civil datetime just a time.Time? As the first comment says, these types

represent times without a corresponding location.

@lpar
Copy link

@lpar lpar commented Jul 22, 2019

To clarify, I'm saying that a date/time library needs to support at least all of the following:

  • 15:04:05
  • 2006-01-02
  • 2006-01-02 15:04:05
  • 2006-01-02 15:04:05 -0500
@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 23, 2019

@rsc (or more generally the Proposal committee)

I would like to re-open this proposal for consideration in order to get closure on it.

Why Now?

The SQL Server Driver currently supports "https://godoc.org/cloud.google.com/go/civil" for date, time and date time types. However, "cloud.google.com/go" is a heavy module and I would prefer we do not require it, when I really just want the civil types from it. I have extracted it from that package as "github.com/golang-sql/civil" with a PR to switch to using it denisenkom/go-mssqldb#501 . However, if this proposal was accepted and https://golang.org/cl/38571 was accepted, I would want to use that.

Why use Civil types at all?

Business applications frequently use both Date, Timestamp and Timestampz (or equivalent) types. One type of bug I have encountered is where a DB Date type is used, but in certain instances time.Time will set the wrong date based on the time of day (due to timezone). This is fixable, but in newer code I've chosen to use civil.Date. This simplifies reading and understanding the code and prevents through fundamental design an entire class of bugs.

When working with Dates in a date based scheduler, converting to civil.Date simplified the code over time.Time. Equality and inequality became much simpler cognitively and slightly simpler in code.

When sending a parameter to a database instance, can be important to have proper types so they are represented correctly the the SQL:

  • "github.com/ziutek/mymysql/mysql".Date
  • "cloud.google.com/go/civil".(Date, Time, DateTime) for spanner
  • "github.com/kshvakov/clickhouse/lib/types".(Date, DateTime)
  • "github.com/mailru/go-clickhouse".Date

What do I want

Go1.13 is almost out. I would like resolution whether x/time/civil will be a package or not. If not, I will use "github.com/golang-sql/civil". If it may, then I will wait until it gets merged. Once this is sorted out, I will encurage other database drivers to support these civil types as well.


Date, DateTime, Time (of day), and Decimal types are the last types commonly supported by Databases. I'm addressing Decimal support with https://golang.org/issue/30870 .

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Aug 23, 2019

Seems like we still need to sort out #17244. But I'll take that one off hold.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 22, 2019

Removing the hold so we keep an eye on this discussion in proposal meetings.

@rsc rsc removed the Proposal-Hold label Nov 22, 2019
@adsouza
Copy link

@adsouza adsouza commented Nov 22, 2019

The use-cases for #2 are certainly far fewer than #1. One example I know of is modelling a standing time for a recurring event (e.g. meals), although even those would generally be tied to at least a specific day of the week.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 22, 2019

The Abseil project, from Google, provides civil dates and times for C++: https://abseil.io/blog/20181010-civil-time.

@dylan-bourque
Copy link

@dylan-bourque dylan-bourque commented Nov 22, 2019

I agree that #1 would be the most common by far. For myself, the case for #2 was definitely related to a scheduling/calendaring scenario where start and end times of the thing were the clock time and stored in MS SQL Server as TIME columns. I'm not an Oracle user, but PostgreSQL definitely also supports TIME columns.

And I think I agree that time.Time is both sufficient and correct for DATETIME and TIMESTAMP SQL values.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 23, 2019

how to represent a SQL DATETIME versus SQL TIMESTAMP

This is mostly an ecosystem thing. C#, Java, SQL Server, and MySQL all use DateTime. SQL Server also has DateTimeOffset (DateTime with timezone minutes offset).

Postgresql, Oracle, and the SQL Standard uses Timestamp and Timestamp with timezone (or timestampz).


Date is useful as you have mentioned. I use civil.Date in a scheduling package to prevent subtle errors.

DateTime (without zone) has fewer cases. The benefit are often slightly easier to reason about if zone (should) not vary in the logic, and easier to reason about when marshaling to datetime (timestamp without zone) types. In one application I maintain, I choose to represent many timestamps as DateTime (no timezone) and simply assume UTC. I believe Prometheus does a similar thing where all timestamps are stored with date time with assumed UTC in the storage. time.Time isn't bad when working with these times, but I've had bugs on a number of times where I would store or load the wrong time (offset through zone) when using time.Time with a datetime. So it isn't about capability, then it is for clarity and more obvious correctness.

I have not had the occasion to use Time (of day). I've typically heard uses for schedules where something happens at a time of day. I'll note that Time (of day) and duration and extremely similar; A Date + Duration = DateTime.

@lpar
Copy link

@lpar lpar commented Nov 23, 2019

Use cases for time only

Alarms. If I set my alarm for 8am, I want it to go off at 8am every day, in whatever time zone I happen to be in at the time.

Cron-like regular events. The before-business-day pull of data should happen at 8am every day, whether it's DST or not.

Use cases for date only

Calendar days: Christmas Day, Easter, Thanksgiving.

Use cases for date and time (but no time zone)

Start of an event occurring at a particular physical location. The time zone is implicit given the location. The event starts at 2019-11-23 13:00:00 in whatever the local time zone is of the location where it's happening, resolved on the day when it happens.

You might think you can get away with using a Go time.Time for this, but:

  1. Events change location. If an event is moved from Cleveland to Chicago for capacity reasons, it'll still start at 9am, but now in Chicago's time zone. Why encode the information redundantly in two places and have to keep them in sync?

  2. The time zone which will be in effect at a location may not be known far enough in advance to specify what it is now. See: Brexit, BST changeover dates.

  3. The time zone info in a time.Time is generally insufficient, you really need a full Olsen time zone.

Now, let me say something which I know is going to be welcomed like a vegan in a steakhouse: I think Go could benefit enormously from looking at what the Java community did with date and time APIs.

The initial Java 1.0 date and time handling was a thin layer over underlying POSIX epoch time, with a single Date type , just like Go. Issues like calendar handling came up pretty quickly, and a Calendar type was added in Java 1.1 to attempt to handle those requirements without having to do anything to Date. The result was a horrible mess.

Then, a community-driven effort produced a library called Joda Time, which rethought the whole problem from the ground up, providing time, date, time-date, and time-date-zone (and a few more). It was so clearly, obviously better that it became the de facto standard, so the JSR-310 project made a few minor changes and then enshrined it as the new Java date and time APIs, replacing all the old ones.

@neild
Copy link
Contributor

@neild neild commented Nov 24, 2019

I'll note that Time (of day) and duration and extremely similar; A Date + Duration = DateTime.

This doesn't really hold in the presence of daylight savings changes and leap seconds.

@dylan-bourque
Copy link

@dylan-bourque dylan-bourque commented Nov 24, 2019

@neild ignoring DST and leap seconds is precisely the use case for civil time

@neild
Copy link
Contributor

@neild neild commented Nov 24, 2019

@dylan-bourque Yes, which is why a duration is not a good representation of a civil time. 7200 seconds past midnight is not necessarily 0200.

@cespare
Copy link
Contributor

@cespare cespare commented Nov 25, 2019

@rsc speaking only of the code I've written and read:

  1. I've needed dates with no associated time/location several times.

    One type of code where this comes up has to do with an analytics system that collects daily data. At some level these days have locations, obviously (in order to decide to which day some event ought to be attributed) but higher up we refer to abstract dates without any time/location.

  2. I've never needed times without associated dates. They seem useful in some domains but not ones I've had occasion to explore.

  3. I've never used, nor felt a strong need for date+time without associated location. Possibly I would've used them if there were such a type in common use.

It seems to me that using time.Time to model (1) is a worse fit than using time.Time to model (3). If you model a date with a time.Time, you need to decide on conventions for two things (time + location). If you are using a time.Time to model a date+time without location you just need a convention for the location. That doesn't mean it's necessarily a good fit, as @neild and others have pointed out, but I do think that the case for a date type is the strongest one here.

@rsc rsc added this to Incoming in Proposals Nov 27, 2019
@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2019

As background, most languages developers are familiar with have two time types, one for location-independent time instants (put aside the obvious physics-based objections) and one for human-readable times in specific locations. This goes back at least as far as C, which has time_t and struct tm.

More recently, Google's C++ team has been using the terms "absolute time" and "civil time" to denote the two different kinds of types. So in this nomenclature time_t is absolute time while struct tm is civil time. See the https://github.com/google/cctz project for examples of this usage. I am not sure exactly where "civil time" as the canonical description of a struct tm-like time came from, whether we're seeing it from the Google C++ popularization or some earlier source. Doesn't really matter. I'll keep using those terms but I wanted to define them first.

This API split, where you have to explicitly convert an absolute time to a civil time, fragments the ecosystem by time representation. Some code takes one representation, other code takes another. When you decide you want to store a time, you need to make a decision yourself. And then you're either in the time\_t ecosystem or the struct tm ecosystem. Want to call code in the other ecosystem? Sorry, write the conversions. Want to change your mind? Sorry, update all your code. On top of that, the absolute time representation is nearly useless for printing. It's almost never what you as a human want to see. But it's all you can do with a time_t short of conversion.

The original Go time package followed the traditional C split and had all these drawbacks. In fact there were three time types, with functions to convert between them:

// Seconds since January 1, 1970 00:00:00 GMT
export func Seconds() (sec int64, err *os.Error)

// Nanoseconds since January 1, 1970 00:00:00 GMT
export func Nanoseconds() (nsec int64, err *os.Error)

export func SecondsToUTC(sec int64) *Time

export func SecondsToLocalTime(sec int64) *Time

export func UTC() (t *Time, err *os.Error)

export func LocalTime() (t *Time, err *os.Error)

// Compute number of seconds since January 1, 1970.
func (t *Time) Seconds() int64

This kind of fragmentation is particularly unfortunate when trying to establish agreed-upon conventions for interfaces. For example, which time type should os.FileInfo's ModTime method return? Making that decision would force every implementation into one fragment of the ecosystem or the other.

The major advance of the Go 1 time API was to unify the two types into the single type time.Time, removing those kinds of decisions and all the fragmentation and confusion they imply. Thanks to a careful, lightweight representation, the one time type works equally well for both time_t and struct tm uses, and we all benefit from the unified ecosystem.

The answer is that os.FileInfo's ModTime returns a time.Time, because that's the only possible answer.

All this is a bit of a long-winded way to say that my response to the summary “add civil time” is: we already have a type for civil time: time.Time. That is, to the extent that civil time means time instants interpreted in locations with time zones and a Gregorian calendar, time.Time does that for us already. We should not add something else called a “civil time.” Doing so would be very confusing and put us right back into the ecosystem fragmentation present in most other systems and in Go before Go 1.

That said, what I'm hearing in this issue is not so much “add civil time” as “add a type representing a day without a specific time” and maybe also “add a type representing a clock time without a specific day.” I realize that in some other languages these two things are called Date and Time, but those are already both exported names in package time, so they are off the table. My suggestion would be to use time.Day and time.Clock.

Specifically:

// A Day represents a single calendar day.
// Its range is January 1, 0001 to December 31, 9999.
type Day struct { ... unexported fields ... }

// DayOf returns the Day for the given year, month, and day-of-month.
//
// Example:
//
//	now := time.Now()
//	day := time.DayOf(now.Date())
func DayOf(year int, month Month, day int) Day

func (d Day) Year() int
func (d Day) Month() Month
func (d Day) MonthDay() int
func (d Day) YearDay() int
func (d Day) Weekday() Weekday
func (d Day) ISOWeek() (year, week int)

func (d Day) Add(years int, months int, days int) Day
func (d Day) Sub(e Day) (years int, months int, days int)
func (d Day) SubDays(e Day) (days int)

func (d Day) After(e Day) bool
func (d Day) Before(e Day) bool

func (d Day) Format(layout string) string
func (d Day) String() string

func (d Day) MarshalBinary() ([]byte, error)
func (d Day) MarshalText() ([]byte, error)
func (d *Day) UnmarshalBinary(data []byte) error
func (d *Day) UnmarshalText(data []byte) error

func ParseDay(layout, value string) (Day, error)

I am not thrilled with the name Day, but I have not come up with a better one.

And:

// A Clock represents a 24-hour clock time during an unspecified day.
// Its range is 00:00:00 (midnight) to 23:59:59.999999999
type Clock struct { ... unexported fields ... }

// ClockOf returns the Clock for the given hour, minute, second.
//
// Example:
//
//	now := time.Now()
//	clock := time.ClockOf(now.Clock())
func ClockOf(hour, min, sec int) 

// ClockOfNano returns the Clock for the given hour, minute, second, and nanoseconds.
func ClockOfNano(hour, min, sec, nano int)

func (c Clock) Hour() int
func (c Clock) Minute() int
func (c Clock) Second() int
func (c Clock) Nanosecond() int

func (c Clock) Add(hours, mins, secs, nanos int) Clock
func (c Clock) Sub(d Clock) (hours, mins, secs, nanos int)

func (c Clock) After(d Clock) bool
func (c Clock) Before(d Clock) bool

func (c Clock) Format(layout string) string
func (c Clock) String() string

func (c Clock) MarshalBinary() ([]byte, error)
func (c Clock) MarshalText() ([]byte, error)
func (c *Clock) UnmarshalBinary(data []byte) error
func (c *Clock) UnmarshalText(data []byte) error

func ParseClock(layout, value string) (Clock, error)

I am not 100% convinced Clock is necessary at all.

@dylan-bourque
Copy link

@dylan-bourque dylan-bourque commented Nov 27, 2019

@rsc The new types you are suggesting here accurately represent what I would want. In fact, they closely mirror the API and types I had begun porting from my own "old" C++ and C# versions.

One additional API I would probably suggest is:

// ToTime returns a Time for the given Day, Clock and Location
func ToTime(d Day, c Clock, l *Location) time.Time

For naming, it's unfortunate that the most obvious/intuitive names are already taken. I have called the Day type both Calendar and Date and have called Clock both Clock and TimeOfDay. Currently, I have each in its own sub-package so that the names are date.Value and timeofday.Value. That convention doesn't really fit within the standard library, but I figured I'd throw it out for comparison purposes.

While I think Clock may not be absolutely necessary, it is useful enough that I have implemented it pretty much in its entirety for myself already.

@lpar
Copy link

@lpar lpar commented Nov 27, 2019

I'd be happy with Day and Clock. I certainly don't see any advantage to splitting civil time APIs from absolute time APIs; that split was one of the worst things about Java's old API.

@jimmyfrasche
Copy link
Member

@jimmyfrasche jimmyfrasche commented Nov 27, 2019

👍 to Calendar It has a kind of symmetry with Clock.

Possibly terrible idea: could there be magic sentinel Locations like CivilDate, CivilTime, CivilDateTime that cause a regular Time to operate in the appropriate civil mode? The pro is that there are fewer types/less api but the con is that the extra types are still there effectively just not demarcated statically so code using it may need to handle "given civil expected absolute" and vice versa

@dylan-bourque
Copy link

@dylan-bourque dylan-bourque commented Nov 27, 2019

@jimmyfrasche I switched from Calendar to Date because of possible questions like "Which calendar, Julian or Gregorian?". 🤷‍♂

I think I would be against the magic sentinel Locations, though. The new APIs, as suggested, are small and direct enough to avoid confusion, imo, and I feel like introducing new, special locations would lead to confusion.

@neild
Copy link
Contributor

@neild neild commented Nov 28, 2019

That is, to the extent that civil time means time instants interpreted in locations with time zones and a Gregorian calendar, time.Time does that for us already.

I don't believe this is a correct description of a civil time as defined by ABSL. (https://abseil.io/docs/cpp/guides/time)

A civil time is not a time instant. A civil time is a tuple of (year, month, day, hour, minute, second). (Leave aside the question of fractional seconds for the moment.) A time.Time isn't a civil time precisely because a time.Time is a time instant.

@jba
Copy link
Contributor Author

@jba jba commented Dec 1, 2019

@rsc Your proposal looks on the right track to me, but I have two concerns.

First, I feel the time package is quite crowded already. The distinction between Ticker and Timer, the many functions on Time, the unusual formatting strings, and now monotonic clocks all contribute to a package that is hard to learn and hold in the mind. If you moved the two types you propose to a different package, you wouldn't make time more complicated, and you could choose better names. If you still don't like civil even after reading the abseil link that Damien provided above, then date.Date has a nice symmetry with time.Time, and maybe it's not too much of a stretch to have a date.TimeOfDay.

My second concern is that there still isn't a type for what SQL calls DATETIME or TIMESTAMP WITHOUT TIMEZONE. If database/sql used the two new types for SQL DATE and TIME, it would presumably define its own for DATETIME; but then you have the three civil types spread out over two packages.

I know I'm biased, but I still think that time/civil, with types Date, Time (or TimeOfDay) and DateTime, feels right.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2019

@jba, the problem with putting this all in another package is (1) for database/sql to use it, the package has to be in the standard library, and (2) a new package would either have to reproduce large chunks of time (the types time.Month & time.Weekday; code for Parse and Format, since we are not going to use a completely different formatting approach for dates vs times; and so on) or else it would have to require the user to import time to get at those, at which point it's not really a separate package.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2019

@neild, I see that time.Time is not exactly Abseil's civil time concept, and that's fine with me. The point was that we reduced the number of time representations intentionally in the design of package time, and it would be good to continue that decision.

I remain very skeptical about any widespread need for "times without locations". And on top of that it implies creating a second competing time representation and all the complexity and fragmentation that would accompany that. It seems to be all cost and no benefit.

In contrast, I can very easily see the need for a representation for a specific day, and it brings essentially no incidental complexity with it.

The arguments in favor of adding "time without location" seem to be "Google C++ has it" and "Java 8 has it". I don't find either of these compelling: the types here have to fit into the design of the time package overall, because they will live alongside it. Hence the long background section in my earlier response.

@jba and I spent a long time (probably too long) looking through old SQL specs trying to understand use cases and the like and came up empty. In practice my working hypothesis is that almost no one has the hypothetical problem that "without timezone" solves.

That's why I'm focusing on 'calendar day' and maybe 'clock time' and not 'time without location'.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 4, 2019

the problem with putting this all in another package is (1) for database/sql to use it, the package has to be in the standard library

I don't think that's true, at least since a number of releases ago: between the Scanner & driver.Valuer interfaces, types from other packages can go in & out of databases.

/cc @kardianos

@dylan-bourque
Copy link

@dylan-bourque dylan-bourque commented Dec 4, 2019

Not that my vote carries any real weight (since I'm really just a consumer of Go with some opinions) but I agree that "time without location" probably wouldn't be generally useful. I would even go so far as to argue that people using database columns to store timestamps without location/tz information are "doing it wrong", since those systems likely also have to make assumptions in code about what that missing time zone should be. I know we did at a prior job, which led us to convert every column to either TIME (a clock time with no date), DATE (a calendar date with no time or location), or DATETIMEOFFSET (a SQL Server specific type that represents a full date/time value, including the UTC offset for the source timezone).

Given those three storage representations, the two new proposed types + time.Time are sufficient to load/store those values in a Go program.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Dec 5, 2019

Individual drivers can support arbitrary types for parameters and scanning in.

DateTime would be useful to ensure a correct mapping between db types without tz and Go, but some adapter function or adapter type might also work for that purpose.

Civil date is most useful and would be welcome.

@kylelemons
Copy link
Contributor

@kylelemons kylelemons commented Dec 16, 2019

I have gone down the "convention" road for dates-without-times and times-without-dates in the past. They were all basically used for data analysis, though some of the quantities did come from databases or other structured formats.

The prime motivation is that humans use the clock on their wrist to think about time, and this typically doesn't consider time zone, date, daylight saving, or leap seconds.

One case boils down to number of events in what a local human would call their day or the time, irrespective of any real 24 hour period. For example, in a global dashboard of "cups of coffee dispensed per day" I want to care about what the date was for the person who dispensed the coffee. To keep this example going, I'd be interested in whether more cups of coffee are dispensed in the "morning" (before noon at the dispenser) or on "Monday" (local time). I should also be able to compare whether these trends carry in different geographies without having to add timezone offsets to normalize to some "common" location (hint: don't try this).

Another case where these distinctions have been really important is in computing SLA numbers. For example, the number of hours between submission of a code review and when the reviewer sent their first response. For this, we don't want to include non-business hours, weekends, and holidays.

Both of these analyses involve the following quantities:

  • When an event happened, i.e. the input. time.Time (date, time, timezone)
  • Aggregation time, i.e. "local time in the timezone", we'll call this civil.DateTime
  • Aggregation day-of-week (time.Weekday), date-without-time (civil.Date) and watch-time (civil.Time) separately for breakouts and heatmaps
  • When the workday begins and ends (civil.Time) to count events outside of "work hours"
  • What dates (civil.Date) are holidays (business logic applied e.g. by country, not by timezone)

It is certainly possible to do all of this with the time.Time package as it stands today by establishing conventions, and we have. I have a background in timekeeping, and I still get it wrong and miss things in code reviews. Proper types for the above would help considerably.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 4, 2020

@rsc I like your proposal for time.Day.

I feel like time.Clock would be useful, but the proposal presented isn't quite right I think. I would want a Clock to either: 1) be easy to convert to and from time.Duration, or 2) have some of these methods on time.Duration directly (Duration.Format, then retrieve days, hours, min, secs, nanos as integers rather then floats).

These types don't have to be in the standard library for drivers to use them.

While it isn't strictly necessary, I agree with @jba that having a DateTime (time.Time without zone) would be useful as it enhances code clarity; I've had bugs in Go today where the zone was incorrect going into or out of a database with a DateTime field that resulted in an incorrect value. The problem (as much as it is) is round tripping DateTime field values. If you put a DateTime into the database, you want to ensure that the same value is selected out. So if you put in a time.Time with a local timezone and just use the face (no zone) value, but then the sql scan returns a time.Time with a UTC timezone, it can get you in trouble. Should the scan as a localtime zone might be better, but the problem remains, just reversed UTC zone in, local out). This, can be fine, until you hit some other marshal function that has different assumptions (ie uses zone when you assume it should not).

If a next step were to be taken, would it to decide where such a package would reside (stdlib/time, golang.org/x/time/civil)?

@kardianos
Copy link
Contributor

@kardianos kardianos commented Feb 6, 2020

For reference, here is a comment where the timezone causes ambiguity:
denisenkom/go-mssqldb#396 (comment)
This is the SQL Server driver, looking for the best way to unmarshal datetime and datetime2 (both timestamp without zone types) into time.Time.

@yinzara
Copy link

@yinzara yinzara commented Apr 23, 2020

Any updates on this? :) Would love to see this added. See above mentioned issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.