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: time: add binning #54392

Closed
udf2457 opened this issue Aug 11, 2022 · 8 comments
Closed

proposal: time: add binning #54392

udf2457 opened this issue Aug 11, 2022 · 8 comments
Labels
Milestone

Comments

@udf2457
Copy link

udf2457 commented Aug 11, 2022

At present the nearest thing to this proposal is func (t Time) Round(d Duration) Time.

The problem with that function is that it is essentially a simple mathematical round. There is no option to take an origin timestamp into account.

What I am suggesting is something closer to, for example, the PostgreSQL date_bin function. I have copy/pasted the examples from the PG website to give you a feel:

SELECT date_bin('15 minutes', TIMESTAMP '2020-02-11 15:44:17', TIMESTAMP '2001-01-01');
Result: 2020-02-11 15:30:00

SELECT date_bin('15 minutes', TIMESTAMP '2020-02-11 15:44:17', TIMESTAMP '2001-01-01 00:02:30');
Result: 2020-02-11 15:32:30

The present Go behaviour is closer to the first example (i.e. 15:44:17 gets rounded to the nearest 15 minute, i.e. 15:30:00).

However Go has no stdlib functionality to achieve what is shown in the second example (i.e. 15:44:17 gets rounded to 15:32:30 because the base "origin" is defined as 00:02:30 and therefore the 15 minute bins are calculated on the basis of 00:02:30 instead of 00:00:00).

@gopherbot gopherbot added this to the Proposal milestone Aug 11, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: Time: Add binning proposal: time: add binning Aug 11, 2022
@danp
Copy link
Contributor

danp commented Aug 20, 2022

Maybe something like this? https://go.dev/play/p/umMkwm4nBiE

If you're able to link up any Go code in the wild (eg from the Go source or other open source projects) that are doing this kind of calculation currently, or any packages out there that implement it, that would probably help further guide discussion and possible implementation.

@udf2457
Copy link
Author

udf2457 commented Aug 21, 2022

Thanks @danp at a glance that looks along the right lines.

Unfortunately for all their boasts about being postgres compatible, Cockroach have not implemented date_bin() otherwise that would have been the obvious place to check !

I'm a bit busy with $work at the moment, but I'll aim to find some time to both revisit your example and also see if I can find any open source stuff lurking around.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2023

The proposal is

// Bin rounds the time down to the nearest multiple of d offset from the start time.
func (t Time) Bin(d Duration, start time.Time) time.Time

This is a reasonably simple function. The question is how often it comes up.

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

Adding to minutes. The question remains how commonly needed this is.

@rsc
Copy link
Contributor

rsc commented Apr 12, 2023

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Apr 19, 2023

As noted earlier, this function is pretty short and easy to write outside the standard library:

func Bin(t time.Time, d time.Duration, origin time.Time) time.Time {
	r := t.Sub(origin) % d
	if r < 0 {
		r += d
	}
	return t.Add(-r)
}

This one does have the downside of not working if t.Sub(origin) overflows or underflows, which happens if the origin time is more than 584 years away from the current time. For any actual use of Bin that's not going to happen, so this code is fine in a third-party library. (If it was in package time we'd have to deal with that case.)

I've asked twice for how often this comes up with little response, so it sounds like maybe we should leave this for implementation outside the standard library.

@rsc
Copy link
Contributor

rsc commented May 3, 2023

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 10, 2023

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Declined
Development

No branches or pull requests

4 participants