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

[fix] makeDateTrunc calculating rolling_month wrong #52579

Merged
merged 2 commits into from May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 7 additions & 4 deletions internal/database/event_logs.go
Expand Up @@ -1760,16 +1760,19 @@ END

// makeDateTruncExpression returns an expression that converts the given
// SQL expression into the start of the containing date container specified
// by the unit parameter (e.g. day, week, month, or rolling month [prior 30 days]).
// by the unit parameter (e.g. day, week, month, or rolling month [prior 1 month]).
// Note: If unit is 'week', the function will truncate to the preceding Sunday.
// This is because some locales start the week on Sunday, unlike the Postgres default
// (and many parts of the world) which start the week on Monday.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and many parts of the world 😁😁😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked Cody to generate that, so shifting blame to it 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nono, liked that part, actually :)

func makeDateTruncExpression(unit, expr string) string {
if unit == "week" {
return fmt.Sprintf(`DATE_TRUNC('%s', TIMEZONE('UTC', %s) + '1 day'::interval) - '1 day'::interval`, unit, expr)
return fmt.Sprintf(`TIMEZONE('UTC', (DATE_TRUNC('week', TIMEZONE('UTC', %s) + '1 day'::interval) - '1 day'::interval))`, expr)
}
if unit == "rolling_month" {
return fmt.Sprintf(`DATE_TRUNC('day', TIMEZONE('UTC', %s)) - '30 day'::interval`, expr)
return fmt.Sprintf(`TIMEZONE('UTC', (DATE_TRUNC('day', TIMEZONE('UTC', %s)) - '1 month'::interval))`, expr)
}

return fmt.Sprintf(`DATE_TRUNC('%s', TIMEZONE('UTC', %s))`, unit, expr)
return fmt.Sprintf(`TIMEZONE('UTC', DATE_TRUNC('%s', TIMEZONE('UTC', %s)))`, unit, expr)
}

// RequestsByLanguage returns a map of language names to the number of requests of precise support for that language.
Expand Down
77 changes: 77 additions & 0 deletions internal/database/event_logs_test.go
Expand Up @@ -2027,3 +2027,80 @@ func TestEventLogs_AggregatedRepoMetadataStats(t *testing.T) {
})
}
}

func TestMakeDateTruncExpression(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love the tests!

if testing.Short() {
t.Skip("skipping long test")
}

logger := logtest.Scoped(t)
db := NewDB(logger, dbtest.NewDB(logger, t))
ctx := context.Background()

cases := []struct {
name string
unit string
expr string
expected string
}{
{
name: "truncates to beginning of day in UTC",
unit: "day",
expr: "'2023-02-14T20:53:24Z'",
expected: "2023-02-14T00:00:00Z",
},
{
name: "truncates to beginning of day in UTC, regardless of input timezone",
unit: "day",
expr: "'2023-02-14T20:53:24-09:00'",
expected: "2023-02-15T00:00:00Z",
},
{
name: "truncates to beginning of week in UTC, starting with Sunday",
unit: "week",
expr: "'2023-02-14T20:53:24Z'",
expected: "2023-02-12T00:00:00Z",
},
{
name: "truncates to beginning of month in UTC",
unit: "month",
expr: "'2023-02-14T20:53:24Z'",
expected: "2023-02-01T00:00:00Z",
},
{
name: "truncates to rolling month in UTC, if month has 30 days",
unit: "rolling_month",
expr: "'2023-04-20T20:53:24Z'",
expected: "2023-03-20T00:00:00Z",
},
{
name: "truncates to rolling month in UTC, even if March has 31 days",
unit: "rolling_month",
expr: "'2023-03-14T20:53:24Z'",
expected: "2023-02-14T00:00:00Z",
},
{
name: "truncates to rolling month in UTC, even if Feb only has 28 days",
unit: "rolling_month",
expr: "'2023-02-14T20:53:24Z'",
expected: "2023-01-14T00:00:00Z",
},
{
name: "truncates to rolling month in UTC, even for leap year February",
unit: "rolling_month",
expr: "'2024-02-29T20:53:24Z'",
expected: "2024-01-29T00:00:00Z",
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
format := fmt.Sprintf("SELECT %s AS date", makeDateTruncExpression(tc.unit, tc.expr))
q := sqlf.Sprintf(format)
date, _, err := basestore.ScanFirstTime(db.Handle().QueryContext(ctx, q.Query(sqlf.PostgresBindVar), q.Args()...))
require.NoError(t, err)

require.Equal(t, tc.expected, date.Format(time.RFC3339))
})
}
}