Add support for "week of month" in date parsing pattern #10511

NEUpanning · 2024-07-19T12:34:16Z

In date parsing pattern, "week of month" is used by java.text.SimpleDateFormat. So we need to support it for supporting SimpleDateFormat date parsing mode in DateTimeFormatter. It's presented by number and the range is [1,5].

relates issue:#6374

netlify · 2024-07-19T12:34:34Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`e6d51ae`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66a211db55a36f0008159315

NEUpanning · 2024-07-19T12:35:24Z

@PHILO-HE @mbasmanova Could you help to review this PR please? Thanks.

rui-mo

Would you add some unit tests? Thanks.

PHILO-HE · 2024-07-22T05:30:13Z

velox/functions/lib/DateTimeFormatterBuilder.cpp

@@ -56,6 +56,13 @@ DateTimeFormatterBuilder& DateTimeFormatterBuilder::appendWeekOfWeekYear(
  return *this;
 }

+DateTimeFormatterBuilder& DateTimeFormatterBuilder::appendWeekOfMonth(


Should this function be called in buildJodaDateTimeFormatter or buildMysqlDateTimeFormatter? Like https://github.com/facebookincubator/velox/blob/main/velox/functions/lib/DateTimeFormatter.cpp#L1463.

No. "Week Of Month" format is not supported in Joda and Mysql.

NEUpanning · 2024-07-22T06:35:25Z

@rui-mo Since "week of month" format is not supported in in Joda and Mysql date patterns, I can't add unit tests using MysqlDateTimeFormatter or JodaDateTimeFormatter. Could I add unit tests in the PR supporting SimpleDateFormat laterly? Or do you have any other suggestions?

mbasmanova

@NEUpanning As @rui-mo mentioned, it would be nice to add some tests. Some specific suggestions are below. Thanks.

mbasmanova · 2024-07-22T13:34:46Z

velox/type/TimestampConversion.h

@@ -107,6 +107,15 @@ Status daysSinceEpochFromWeekDate(
    int32_t dayOfWeek,
    int64_t& out);

+/// Computes the (signed) number of days since unix epoch (1970-01-01).
+/// Returns UserError status if the date is invalid.
+Status daysSinceEpochFromWeekOfMonthDate(


Perhaps, extract this function into a separate PR and add a unit test.

The DateTimeFormatter.parse function depends on this function. Do you mean to submit all changes to the DateTimeFormatter.parse function and this function as a separate PR?

I assume this comment suggest us opening another PR which contains the implementation and tests for daysSinceEpochFromWeekOfMonthDate function.

I opened a PR for this. #10604

mbasmanova · 2024-07-22T13:36:00Z

velox/type/TimestampConversion.cpp

+    int32_t weekOfMonth,
+    int32_t dayOfWeek,
+    int64_t& out) {
+  if (!isValidWeekOfMonthDate(year, month, weekOfMonth, dayOfWeek)) {


isValidWeekOfMonthDate doesn't perform full validation. For example, it allows weekOfMonth = 5 and dayOfWeek = 6, but these may not exist for all months. Where / how do we handle these cases of invalid inputs?

I have tested SimpleDateFormat and I see it can parse the "invalid" weekOfMonth and dayOfWeek etc. Perhaps we should remove the validation to align with SimpleDateFormat. Here is an example:

SimpleDateFormat sdf = new SimpleDateFormat("y W"); sdf.setTimeZone(TimeZone.getTimeZone("UTC")); Date date = sdf.parse("2024 1"); System.out.println(date); date = sdf.parse("2024 5"); System.out.println(date); // invalid week of month date = sdf.parse("2024 99"); System.out.println(date); -------------console output-------------- Sun Dec 31 00:00:00 CST 2023 Sun Jan 28 00:00:00 CST 2024 Sun Nov 16 00:00:00 CST 2025

NEUpanning · 2024-07-23T09:30:07Z

@mbasmanova @rui-mo I've added some unit tests in 7ec5bf4

rui-mo

Thanks.

rui-mo · 2024-07-25T03:16:14Z

velox/functions/lib/DateTimeFormatter.cpp

@@ -735,7 +740,6 @@ int32_t parseFromPattern(
      return -1;
    }
    cur += size;
-    date.weekDateFormat = true;


I wonder why we need this change, and could you clarify what is covered by weekDateFormat which seems to be a larger scope than weekOfMonthDateFormat?

weekDateFormat has different format with weekOfMonthDateFormat.weekDateFormat is composed of year, weekOfYear and dayOfWeek and weekOfMonthDateFormat is composed of year, month, weekOfMonth and dayOfWeek. dayOfWeek is used by both weekDateFormat and weekOfMonthDateFormat, so dayOfWeek can't indicate the format is weekDateFormat

rui-mo · 2024-07-25T03:17:07Z

velox/type/TimestampConversion.cpp

+    int32_t year,
+    int32_t month,
+    int32_t weekOfMonth,
+    int32_t dayOfWeek) {


Please add some unit tests for this function.

I've added tests in e6d51ae

rui-mo

@NEUpanning I notice some comments seems to be unresolved. Would you take a look first? Thanks.
#10511 (review) and #10511 (review)

NEUpanning · 2024-07-30T06:19:27Z

@rui-mo The two comments you mentioned, one said "added test" and the other said "thanks". I have already added the test. Are there any other comments I missed?

rui-mo · 2024-07-30T06:25:00Z

one said "added test" and the other said "thanks"

@NEUpanning Could you check the specific comments below them, e.g #10511 (comment)? If they have been addressed, it would be nice to reply at the comment section.

NEUpanning · 2024-07-30T06:51:35Z

@rui-mo Sorry. I haven't submitted comments in a while. I think you can see it now.

…ek-of-month date (#10604) Summary: This helper function is only used by Spark. To align with Spark's SimpleDateFormat behavior, this function offers two modes: lenient and non-lenient. For non-lenient mode, it returns an error status if the date is invalid. For lenient mode, it accepts a wider range of arguments. Part of #10511 Pull Request resolved: #10604 Reviewed By: xiaoxmeng Differential Revision: D62033315 Pulled By: bikramSingh91 fbshipit-source-id: f17e50c07272dc656038b221129aff6882bcf02c

NEUpanning · 2024-09-26T11:47:11Z

Opened a new PR #11103 for this feature.

support week of month

98d52c1

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 Jul 19, 2024

rui-mo reviewed Jul 22, 2024

View reviewed changes

fix format

a76c50b

PHILO-HE reviewed Jul 22, 2024

View reviewed changes

mbasmanova reviewed Jul 22, 2024

View reviewed changes

add unit tests

7ec5bf4

rui-mo reviewed Jul 25, 2024

View reviewed changes

add unit tests

e6d51ae

NEUpanning requested review from rui-mo and PHILO-HE July 29, 2024 06:42

rui-mo reviewed Jul 29, 2024

View reviewed changes

NEUpanning mentioned this pull request Aug 30, 2024

Add helper function to return the number of days since epoch for a week-of-month date #10604

Closed

ccat3z mentioned this pull request Sep 4, 2024

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

Open

17 tasks

NEUpanning closed this Sep 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for "week of month" in date parsing pattern #10511

Add support for "week of month" in date parsing pattern #10511

NEUpanning commented Jul 19, 2024

netlify bot commented Jul 19, 2024 •

edited

Loading

NEUpanning commented Jul 19, 2024

rui-mo left a comment

PHILO-HE Jul 22, 2024

NEUpanning Jul 22, 2024 •

edited

Loading

NEUpanning commented Jul 22, 2024

mbasmanova left a comment

mbasmanova Jul 22, 2024

NEUpanning Jul 23, 2024

rui-mo Jul 30, 2024

NEUpanning Jul 30, 2024

mbasmanova Jul 22, 2024

NEUpanning Jul 23, 2024

NEUpanning commented Jul 23, 2024

rui-mo left a comment

rui-mo Jul 25, 2024

NEUpanning Jul 25, 2024

rui-mo Jul 25, 2024

NEUpanning Jul 25, 2024

rui-mo left a comment

NEUpanning commented Jul 30, 2024

rui-mo commented Jul 30, 2024

NEUpanning commented Jul 30, 2024

NEUpanning commented Sep 26, 2024

Add support for "week of month" in date parsing pattern #10511

Add support for "week of month" in date parsing pattern #10511

Conversation

NEUpanning commented Jul 19, 2024

netlify bot commented Jul 19, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

NEUpanning commented Jul 19, 2024

rui-mo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NEUpanning Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

NEUpanning commented Jul 22, 2024

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NEUpanning commented Jul 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo left a comment

Choose a reason for hiding this comment

NEUpanning commented Jul 30, 2024

rui-mo commented Jul 30, 2024

NEUpanning commented Jul 30, 2024

NEUpanning commented Sep 26, 2024

netlify bot commented Jul 19, 2024 •

edited

Loading

NEUpanning Jul 22, 2024 •

edited

Loading