Support WEEK_YEAR for date time formatter #10930

ccat3z · 2024-09-05T09:29:18Z

Support WEEK_YEAR for date time formatter. getWeekYear() is cloned from jdk 8 supporting both ISO8601 and Java SimpleDateFormat standards.

Test case is generated by the scala script below.

def gen_week_years_more() = {
  import java.util.Calendar
  import java.util.GregorianCalendar

  for {
    y <- (2017 to 2022);
    (m, d) <- Seq(
      (Calendar.JANUARY, 1),
      (Calendar.JANUARY, 2),
      (Calendar.JANUARY, 3),
      (Calendar.JANUARY, 4),
      (Calendar.JANUARY, 5),
      (Calendar.JANUARY, 6),
      (Calendar.JANUARY, 7),
      (Calendar.DECEMBER, 25),
      (Calendar.DECEMBER, 26),
      (Calendar.DECEMBER, 27),
      (Calendar.DECEMBER, 28),
      (Calendar.DECEMBER, 29),
      (Calendar.DECEMBER, 30),
      (Calendar.DECEMBER, 31),
    );
    (fd, md) <- Seq(
      (Calendar.SUNDAY, 1), // SimpleDateFormat
      (Calendar.MONDAY, 4)  // ISO
    )
  } yield {
    val cal = Calendar.getInstance();
    cal.setFirstDayOfWeek(fd)
    cal.setMinimalDaysInFirstWeek(md)

    cal.set(Calendar.YEAR, y)
    cal.set(Calendar.MONTH, m)
    cal.set(Calendar.DAY_OF_MONTH, d)
    cal.getTime

    val wy = cal.getWeekYear
    val woy = cal.get(Calendar.WEEK_OF_YEAR)

    f"  std::make_tuple(${y}%4d, ${m+1}%02d, $d%02d, ${fd-1}, ${md}, ${wy}%4d)," ++
    f" // ${wy}W${woy}"
  }
}

Presto test case is generated by the following query on presto 0.289:

with
  dates as (select
    date_parse(cast(year as varchar) || '-' || cast(month as varchar) || '-' || cast(day as varchar), '%Y-%m-%d') as d
  from
    (select * from unnest(sequence(2017, 2022)) as years(year)),
    (select * from (values
      (1, 1),
      (1, 2),
      (1, 3),
      (1, 4),
      (1, 5),
      (1, 6),
      (1, 7),
      (12, 25),
      (12, 26),
      (12, 27),
      (12, 28),
      (12, 29),
      (12, 30),
      (12, 31)
    ) as monthdays(month, day))
  )
select date_format(d, '%Y-%m-%d'), date_format(d, '%x') from dates
where day(d) in (1, 31) or year(d) != year_of_week(d);

SparkSQL test case is generated by the following query on spark 3.5.2:

SET spark.sql.legacy.timeParserPolicy=LEGACY;
with
  dates as (select
    cast(year as string) || '-' || cast(month as string) || '-' || cast(day as string) as d
  from
    (select * from (values
      (2017),
      (2018),
      (2019),
      (2020),
      (2021),
      (2022)
    ) as years(year)),
    (select * from (values
      (1, 1),
      (1, 2),
      (1, 3),
      (1, 4),
      (1, 5),
      (1, 6),
      (1, 7),
      (12, 25),
      (12, 26),
      (12, 27),
      (12, 28),
      (12, 29),
      (12, 30),
      (12, 31)
    ) as monthdays(month, day))
  )
select date_format(d, 'yyyy-MM-dd') as date, date_format(d, 'Y') as weekyear from dates
where day(d) in (1, 31) or date_format(d, 'yyyy') != date_format(d, 'YYYY')
order by d;

netlify · 2024-09-05T09:29:43Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`f31a643`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/672c8b69ba65ef0008ef6781

ccat3z · 2024-09-10T04:10:13Z

@rui-mo

rui-mo

Thanks!

velox/functions/lib/TimeUtils.h

rui-mo · 2024-09-11T06:44:26Z

velox/functions/lib/DateTimeFormatter.cpp

+              static_cast<int>(calDate.year()),
+              static_cast<uint32_t>(calDate.month()),
+              static_cast<uint32_t>(calDate.day()),
+              2, // (ISO 8601) Monday = 2


Wondering how we plan to support the legacy Spark behavior where this value should be 1.

Maybe it can be obtained from java.util.Calendar in Java and then passed in through QueryConf.

ccat3z · 2024-09-13T02:20:37Z

@rui-mo I have updated the code. Could you please review it?

rui-mo

Thanks.

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

velox/functions/lib/TimeUtils.h

rui-mo · 2024-09-13T08:11:26Z

velox/functions/lib/TimeUtils.h

+/// (1582-10-15) would yield mismatched results.
+///
+/// The algorithm refers to the weekyear algorithm in jdk:
+/// https://github.com/openjdk/jdk8/blob/6a383433a9f4661a96a90b2a4c7b5b9a85720031/jdk/src/share/classes/java/util/GregorianCalendar.java#L2077


This link points to an implementation in JDK 8. Is there any difference in the implementations among JDK versions?

There's no difference. I will updated the URL to the most recent openjdk. https://github.com/openjdk/jdk/blob/d9c67443f7d7f03efb2837b63ee2acc6113f737f/src/java.base/share/classes/java/util/GregorianCalendar.java#L2058

velox/functions/lib/TimeUtils.h

ccat3z · 2024-09-24T06:26:57Z

@rui-mo I have updated the code. Could you please review it?

Presto test case is generated by the following query on presto 0.289:

with
  dates as (select
    date_parse(cast(year as varchar) || '-' || cast(month as varchar) || '-' || cast(day as varchar), '%Y-%m-%d') as d
  from
    (select * from unnest(sequence(2017, 2022)) as years(year)),
    (select * from (values
      (1, 1),
      (1, 2),
      (1, 3),
      (1, 4),
      (1, 5),
      (1, 6),
      (1, 7),
      (12, 25),
      (12, 26),
      (12, 27),
      (12, 28),
      (12, 29),
      (12, 30),
      (12, 31)
    ) as monthdays(month, day))
  )
select date_format(d, '%Y-%m-%d'), date_format(d, '%x') from dates
where day(d) in (1, 31) or year(d) != year_of_week(d);

NEUpanning · 2024-09-25T03:01:41Z

@ccat3z After simple date time formatter was introduced in #10966 , could you help to add support of WEEK_YEAR which specifier is 'W' in simple date time formatter ? Thanks.

velox/functions/lib/TimeUtils.h

ccat3z · 2024-09-25T09:49:32Z

@ccat3z After simple date time formatter was introduced in #10966 , could you help to add support of WEEK_YEAR which specifier is 'W' in simple date time formatter ? Thanks.

Sure. I will support it in this pr.

rui-mo

Thanks.

velox/functions/lib/TimeUtils.h

velox/functions/prestosql/tests/DateTimeFunctionsTest.cpp

velox/functions/lib/tests/TimeUtilsTest.cpp

assignUser · 2024-10-15T13:55:30Z

velox/functions/lib/TimeUtils.h

+/// `minimalDaysInFirstWeek` is 1.
+///
+/// The algorithm refers to the getWeekYear algorithm in openjdk:
+/// https://github.com/openjdk/jdk/blob/d9c67443f7d7f03efb2837b63ee2acc6113f737f/src/java.base/share/classes/java/util/GregorianCalendar.java#L2058


⚠️ The upstream code is licensed under GPL 2.0 and while APIs and algorithms don't fall under copyright, afaik the implementation does. So this should probably not be merged without checking with some form of legal professional. @pedroerp @mbasmanova

@ccat3z @rui-mo is this different from the vendored date library we have at:

https://github.com/facebookincubator/velox/blob/main/velox/external/date/iso_week.h

we use it's iso_week functionality in a few places already. If it's the same logic, it would be nice to consolidate the usage.

iso_week only support ISO 8601 standard week year (iso_week.h#L1519). The purpose of this implementation is to support non-ISO standard week year, which is required by Spark.

@assignUser @ccat3z @rui-mo I asked for guidance from license experts on this. Give me a day or two until they get back to me.

@pedroerp Any updates?

ccat3z · 2024-10-16T06:12:15Z

@rui-mo I have updated the support for simple date time formatter. Could you please review it?
cc @NEUpanning

rui-mo

Thanks for iterating. Just some nits.

velox/core/QueryConfig.h

velox/functions/sparksql/DateTimeFunctions.h

pedroerp · 2024-10-23T01:39:01Z

velox/functions/lib/DateTimeFormatter.h

 private:
  std::unique_ptr<char[]> literalBuf_;
  size_t bufSize_;
  std::vector<DateTimeToken> tokens_;
  DateTimeFormatterType type_;
+
+  /// The first day-of-week varies by culture.
+  /// firstDayOfWeek is is a 1-based weekday number starting with Sunday. It


nit: "is is" repeated

ccat3z · 2024-11-05T06:33:50Z

@rui-mo I have updated the code. Could you please review it again?

rui-mo

Thanks! Looks good to me overall % we might need community's feedback on #10930 (comment).

velox/core/QueryConfig.h

velox/functions/lib/TimeUtils.h

velox/functions/lib/tests/DateTimeFormatterTest.cpp

velox/functions/lib/tests/TimeUtilsTest.cpp

velox/functions/sparksql/DateTimeFunctions.h

velox/functions/sparksql/tests/DateTimeFunctionsTest.cpp

velox/functions/lib/tests/DateTimeFormatterTest.cpp

velox/functions/sparksql/DateTimeFunctions.h

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 5, 2024

ccat3z mentioned this pull request Sep 6, 2024

[VL] Result mismatch in date format week year apache/incubator-gluten#7069

Open

ccat3z force-pushed the weekyear-fb branch from 4b65e61 to 2b632f1 Compare September 10, 2024 02:52

ccat3z force-pushed the weekyear-fb branch from 2b632f1 to 83581b3 Compare September 10, 2024 06:04

rui-mo reviewed Sep 11, 2024

View reviewed changes

ccat3z force-pushed the weekyear-fb branch from 2589ebd to 6f830bc Compare September 12, 2024 08:06

rui-mo reviewed Sep 13, 2024

View reviewed changes

ccat3z force-pushed the weekyear-fb branch 2 times, most recently from 032c621 to 31bb656 Compare September 24, 2024 04:31

rui-mo reviewed Sep 25, 2024

View reviewed changes

velox/functions/lib/TimeUtils.h Show resolved Hide resolved

velox/functions/lib/TimeUtils.h Outdated Show resolved Hide resolved

velox/functions/lib/TimeUtils.h Outdated Show resolved Hide resolved

ccat3z force-pushed the weekyear-fb branch from ee010e0 to 0f7c4f8 Compare September 25, 2024 10:01

rui-mo reviewed Sep 27, 2024

View reviewed changes

NEUpanning mentioned this pull request Oct 8, 2024

Velox doesn't support legacy date format behavior in Spark SQL #10354

Open

17 tasks

ccat3z force-pushed the weekyear-fb branch from 0f7c4f8 to d8ba5bb Compare October 15, 2024 08:09

ccat3z requested review from assignUser and majetideepak as code owners October 15, 2024 08:09

ccat3z force-pushed the weekyear-fb branch from 9bcd62a to a682a3b Compare October 15, 2024 08:20

assignUser requested changes Oct 15, 2024

View reviewed changes

ccat3z force-pushed the weekyear-fb branch 2 times, most recently from 5d7e994 to 174b4bc Compare October 16, 2024 04:07

rui-mo reviewed Oct 18, 2024

View reviewed changes

pedroerp reviewed Oct 23, 2024

View reviewed changes

ccat3z force-pushed the weekyear-fb branch from a1e70b0 to d764b04 Compare November 5, 2024 04:18

rui-mo reviewed Nov 6, 2024

View reviewed changes

Support week year

dedbdaf

ccat3z force-pushed the weekyear-fb branch from d764b04 to 5e52aff Compare November 7, 2024 03:29

update

80b62d4

ccat3z force-pushed the weekyear-fb branch from 5e52aff to 80b62d4 Compare November 7, 2024 03:43

rui-mo reviewed Nov 7, 2024

View reviewed changes

velox/functions/lib/tests/DateTimeFormatterTest.cpp Outdated Show resolved Hide resolved

velox/functions/sparksql/DateTimeFunctions.h Outdated Show resolved Hide resolved

update

f31a643

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support WEEK_YEAR for date time formatter #10930

Support WEEK_YEAR for date time formatter #10930

ccat3z commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

ccat3z commented Sep 10, 2024

rui-mo left a comment

rui-mo Sep 11, 2024

ccat3z Sep 12, 2024

ccat3z commented Sep 13, 2024

rui-mo left a comment

rui-mo Sep 13, 2024

ccat3z Sep 19, 2024

ccat3z commented Sep 24, 2024 •

edited

Loading

NEUpanning commented Sep 25, 2024

ccat3z commented Sep 25, 2024

rui-mo left a comment

assignUser Oct 15, 2024

pedroerp Oct 19, 2024

ccat3z Oct 21, 2024 •

edited

Loading

pedroerp Nov 6, 2024

ccat3z Nov 25, 2024

ccat3z commented Oct 16, 2024 •

edited

Loading

rui-mo left a comment

pedroerp Oct 23, 2024

ccat3z commented Nov 5, 2024

rui-mo left a comment

Support WEEK_YEAR for date time formatter #10930

Are you sure you want to change the base?

Support WEEK_YEAR for date time formatter #10930

Conversation

ccat3z commented Sep 5, 2024 • edited Loading

netlify bot commented Sep 5, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

ccat3z commented Sep 10, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccat3z commented Sep 13, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccat3z commented Sep 24, 2024 • edited Loading

NEUpanning commented Sep 25, 2024

ccat3z commented Sep 25, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccat3z Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccat3z commented Oct 16, 2024 • edited Loading

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccat3z commented Nov 5, 2024

rui-mo left a comment

Choose a reason for hiding this comment

ccat3z commented Sep 5, 2024 •

edited

Loading

netlify bot commented Sep 5, 2024 •

edited

Loading

ccat3z commented Sep 24, 2024 •

edited

Loading

ccat3z Oct 21, 2024 •

edited

Loading

ccat3z commented Oct 16, 2024 •

edited

Loading