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 last_day_of_month for timestamp with time zone #2852

Closed
wants to merge 1 commit into from
Closed

Fix last_day_of_month for timestamp with time zone #2852

wants to merge 1 commit into from

Conversation

oneonestar
Copy link
Member

Fix #2851

@cla-bot cla-bot bot added the cla-signed label Feb 17, 2020
millis = unpackChronology(timestampWithTimeZone).monthOfYear().roundCeiling(millis + 1);
ISOChronology isoChronology = unpackChronology(timestampWithTimeZone);
millis = isoChronology.monthOfYear().roundCeiling(millis + 1);
// millis is currently midnight in timezone of the original value
Copy link
Member

Choose a reason for hiding this comment

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

This comment makes me think about the case when there is no midnight and the date/time is near the end of the month.

For example, America/Bahia_Banderas and the local date/time 1970-01-01 00:00:00 ±1ms.

Can you add this to your tests? (along with other zones and date/times you have)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added tests for timezone America/Bahia_Banderas and 2019-01-01 00:00:00.000 ±1ms.
Not sure how to test for day without midnight. Could you provide a concrete example for this?
1970-01-01 00:00:00 is epoch and difficult to test.

@oneonestar
Copy link
Member Author

The error seems unrelated. How can I trigger a re-test?

@dain dain requested a review from findepi February 20, 2020 02:06
@dain
Copy link
Member

dain commented Feb 20, 2020

@oneonestar there is a button on the actions page to rerun the jobs, but I'm not sure if you can see it (github permission are weird). You can always re-push to trigger a rebuild. I triggered a rebuild.

Copy link
Member

@dain dain left a comment

Choose a reason for hiding this comment

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

Looks good to me. @findepi did you have any further comments?

@dain
Copy link
Member

dain commented Feb 20, 2020

@oneonestar Github actions keep failing during the git check out. I'm not sure if there is something wrong with actions right now, but can you try re pushing your code?

@oneonestar
Copy link
Member Author

Still there are some errors that looks like unrelated.

@dain
Copy link
Member

dain commented Feb 20, 2020

I tried to rerun those tests, and they fail during check out again. I can't see the original failures anymore. Can you re push? I'll just manually verify next time.

millis = isoChronology.monthOfYear().roundCeiling(millis + 1);
// millis is currently midnight in timezone of the original value
// convert to UTC
millis = millis + isoChronology.getZone().getOffset(millis) - MILLISECONDS_IN_DAY;
Copy link
Member

Choose a reason for hiding this comment

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

convertUTCToLocal

@findepi
Copy link
Member

findepi commented Feb 21, 2020

Merged as 13e53d7, thanks!

(i slightly changed comments)

@findepi findepi closed this Feb 21, 2020
@findepi findepi added this to the 331 milestone Feb 21, 2020
@findepi findepi mentioned this pull request Feb 21, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Incorrect results for last_day_of_month function
3 participants