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.

Sign up for GitHub

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

Jump to bottom

Add support for "week of month" in SimpleDateTimeFormatter #11103

Closed
wants to merge 4 commits into from
Closed

Add support for "week of month" in SimpleDateTimeFormatter #11103

wants to merge 4 commits into from

Conversation

NEUpanning
Copy link
Contributor

@NEUpanning NEUpanning commented Sep 26, 2024 •
edited by rui-mo
Loading

java.text.SimpleDateFormat supports using 'week of month' to parse/format
date. The specifier of 'week of month' is 'W'. Now DateTimeFormatter supports 3
group of fields specifying the day within the year. They are following
combinations:

year + week + dayOfWeek
year + dayOfYear
year + month + day

This PR introduces a new combination that is
year + month + weekOfMonth + dayOfWeek and adds support for "week of month"
in SimpleDateTimeFormatter.

Relates issue : #10354

@facebook-github-bot 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 26, 2024
@netlify Netlify
Copy link

netlify bot commented Sep 26, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9b14344
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67073c2e7cbac70008c17029

@NEUpanning NEUpanning marked this pull request as draft September 26, 2024 11:30
@NEUpanning NEUpanning force-pushed the support_week_of_month_newest branch from 4a95917 to a3205d0 Compare September 26, 2024 11:43
@NEUpanning NEUpanning marked this pull request as ready for review September 26, 2024 11:44
@NEUpanning
Copy link
Contributor Author

@rui-mo Could you help to review this PR please? Thanks.

This was referenced Sep 26, 2024
Closed
Open
rui-mo
rui-mo reviewed Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to me overall.

velox/functions/lib/DateTimeFormatter.cpp Outdated Show resolved Hide resolved
velox/functions/lib/DateTimeFormatter.cpp Outdated Show resolved Hide resolved
velox/functions/lib/DateTimeFormatter.cpp Outdated Show resolved Hide resolved
@NEUpanning
Copy link
Contributor Author

@rui-mo I've updated code based on the feedback. Could you review it again? Thanks.

@NEUpanning NEUpanning requested a review from rui-mo October 11, 2024 08:09
@NEUpanning
Copy link
Contributor Author

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

@rui-mo
Copy link
Collaborator

rui-mo commented Oct 12, 2024

cc: @pedroerp

@NEUpanning
Copy link
Contributor Author

@mbasmanova @pedroerp Just a gentle ping.

pedroerp
pedroerp reviewed Oct 24, 2024
View reviewed changes
velox/functions/lib/DateTimeFormatter.cpp
bool centuryFormat = false;

bool isYearOfEra = false; // Year of era cannot be zero or negative.
bool hasYear = false; // Whether year was explicitly specified.
bool hasDayOfWeek = false; // Whether dayOfWeek was explicitly specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR, but maybe we should use std::optional to prevent these flags controlling what has been explicitly set and what not.

Copy link
Contributor Author

@NEUpanning NEUpanning Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your suggestion! Using std::optional would be a more elegant solution to avoid using boolean flags to control whether a value has been explicitly set. I'd like to work on it. Should I open an issue for this first?

@facebook-github-bot
Copy link
Contributor

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pedroerp
Copy link
Contributor

@NEUpanning sorry I was on PTO last few weeks.

@rui-mo assuming this one is also ready to merge?

@rui-mo
Copy link
Collaborator

rui-mo commented Oct 25, 2024

@pedroerp Yes, it looks good AFAICS.

@facebook-github-bot
Copy link
Contributor

@pedroerp merged this pull request in a1d923e.

@Yohahaha Yohahaha mentioned this pull request Oct 29, 2024
Closed
facebook-github-bot pushed a commit that referenced this pull request Nov 5, 2024
@Yohahaha @facebook-github-bot
Fix GCC build error with std::to_char<double> (#11369)
f9b24d5 
Summary:
#11103 breaks GCC build
```
In instantiation of function template specialization 'facebook::velox::functions::(anonymous namespace)::padContent<double>' call to 'to_chars' is ambiguous
```

To minimize code changes, this PR add unsigned cast for ceil to fix it.

Pull Request resolved: #11369

Reviewed By: Yuhta

Differential Revision: D65420211

Pulled By: mbasmanova

fbshipit-source-id: 18aba3a62012a86d1bc3e7fec57dda54c992050e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroerp pedroerp pedroerp left review comments

@rui-mo rui-mo rui-mo approved these changes

@Yuhta Yuhta Yuhta approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@NEUpanning @rui-mo @facebook-github-bot @pedroerp @Yuhta