Skip to content

Commit

Permalink
WeekFunction calculates long years incorrectly (#10599)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #10599

The WeekFunction treats leap years that end on Saturday, and any year that ends on Friday or
Thursday as a year with 53 weeks.

However, according to Wikipedia https://en.wikipedia.org/wiki/ISO_week_date#Weeks_per_year years
with 53 weeks are leap years that end on Friday and any year that ends on Thursday.

I updated the logic to reflect this.

I added tests to cover cases where we end up in Week 0, which is the only case where this logic is
used, and verified the results match Presto Java.

Reviewed By: amitkdutta

Differential Revision: D60427017

fbshipit-source-id: 22781fdfe04c31969dd1d30bc4d618ce7cb0ad03
  • Loading branch information
Kevin Wilfong authored and facebook-github-bot committed Jul 30, 2024
1 parent e47f2d4 commit f8f538e
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
6 changes: 4 additions & 2 deletions velox/functions/prestosql/DateTimeFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,11 @@ struct WeekFunction : public InitSessionTimezone<T>,
auto firstMondayOfYear =
1 + (mondayOfWeek + kDaysInWeek - 1) % kDaysInWeek;

// A long year is any year ending on Thursday and any leap year ending on
// Friday.
if ((util::isLeapYear(time.tm_year + 1900 - 1) &&
firstMondayOfYear == 2) ||
firstMondayOfYear == 3 || firstMondayOfYear == 4) {
firstMondayOfYear == 3) ||
firstMondayOfYear == 4) {
week = 53;
} else {
week = 52;
Expand Down
16 changes: 16 additions & 0 deletions velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,22 @@ TEST_F(DateTimeFunctionsTest, weekDate) {
EXPECT_EQ(1, weekDate("1970-01-01"));
EXPECT_EQ(1, weekDate("0001-01-01"));
EXPECT_EQ(52, weekDate("9999-12-31"));

// Test various cases where the last week of the previous year extends into
// the next year.

// Leap year that ends on Thursday.
EXPECT_EQ(53, weekDate("2021-01-01"));
// Leap year that ends on Friday.
EXPECT_EQ(53, weekDate("2005-01-01"));
// Leap year that ends on Saturday.
EXPECT_EQ(52, weekDate("2017-01-01"));
// Common year that ends on Thursday.
EXPECT_EQ(53, weekDate("2016-01-01"));
// Common year that ends on Friday.
EXPECT_EQ(52, weekDate("2022-01-01"));
// Common year that ends on Saturday.
EXPECT_EQ(52, weekDate("2023-01-01"));
}

TEST_F(DateTimeFunctionsTest, week) {
Expand Down

0 comments on commit f8f538e

Please sign in to comment.