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

8247781: Day periods support #938

Closed
wants to merge 18 commits into from
Closed

8247781: Day periods support #938

wants to merge 18 commits into from

Conversation

@naotoj
Copy link
Member

@naotoj naotoj commented Oct 29, 2020

Hi,

Please review the changes for the subject issue. This is to enhance the java.time package to support day periods, such as "in the morning", defined in CLDR. It will add a new pattern character 'B' and its supporting builder method. The motivation and its spec are in this CSR:

https://bugs.openjdk.java.net/browse/JDK-8254629

Naoto


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) (1/9 running) (3/9 running) (8/9 running) (6/9 running)

Issue

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/938/head:pull/938
$ git checkout pull/938

@naotoj
Copy link
Member Author

@naotoj naotoj commented Oct 29, 2020

/csr

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 29, 2020

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the csr label Oct 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 29, 2020

@naotoj this pull request will not be integrated until the CSR request JDK-8254629 for issue JDK-8247781 has been approved.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 29, 2020

@naotoj The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@naotoj naotoj marked this pull request as ready for review Oct 29, 2020
@openjdk openjdk bot added the rfr label Oct 29, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 29, 2020

@openjdk openjdk bot removed the csr label Oct 30, 2020
src/java.base/share/classes/java/time/format/Parsed.java Outdated Show resolved Hide resolved
src/java.base/share/classes/java/time/format/Parsed.java Outdated Show resolved Hide resolved
if (resolverStyle != ResolverStyle.LENIENT) {
AMPM_OF_DAY.checkValidValue(ap);
}
updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint / 720);
Copy link
Contributor

@jodastephen jodastephen Oct 30, 2020

Choose a reason for hiding this comment

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

No need to put AMPM_OF_DAY back in here because you've already resolved it to HOUR_OF_DAY and MINUTE_OF_HOUR. There probably does need to be validation to check that the day period agrees with the AM/PM value.

Copy link
Contributor

@jodastephen jodastephen Nov 2, 2020

Choose a reason for hiding this comment

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

Line can still be removed AFAICT

Copy link
Member Author

@naotoj naotoj Nov 2, 2020

Choose a reason for hiding this comment

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

Fixed.

@naotoj
Copy link
Member Author

@naotoj naotoj commented Oct 30, 2020

/test

@openjdk
Copy link

@openjdk openjdk bot commented Oct 30, 2020

@naotoj you need to get approval to run the tests in tier1 for commits up until 6e1eade

@openjdk openjdk bot added the test-request label Oct 30, 2020
@magicus
Copy link
Member

@magicus magicus commented Nov 2, 2020

/label remove build

@openjdk openjdk bot removed the build label Nov 2, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 2, 2020

@magicus
The build label was successfully removed.

src/java.base/share/classes/java/time/format/Parsed.java Outdated Show resolved Hide resolved
if (resolverStyle != ResolverStyle.LENIENT) {
AMPM_OF_DAY.checkValidValue(ap);
}
updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint / 720);
Copy link
Contributor

@jodastephen jodastephen Nov 2, 2020

Choose a reason for hiding this comment

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

Line can still be removed AFAICT

{"K B", "11 in the morning", 11},
{"h B", "11 at night", 23},
{"h B", "3 at night", 3},
{"h B", "11 in the morning", 11},
Copy link
Contributor

@jodastephen jodastephen Nov 2, 2020

Choose a reason for hiding this comment

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

Need tests for "51 in the morning" (which should parse in LENIENT as "3 in the morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.

Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.

Copy link
Member Author

@naotoj naotoj Nov 2, 2020

Choose a reason for hiding this comment

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

Fixed.

updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint / 720);
}
}
}
Copy link
Contributor

@jodastephen jodastephen Nov 2, 2020

Choose a reason for hiding this comment

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

Pulling on this a little more.

As the PR stands, it seems that if the user passes in text with just a day-period of "AM" they get a LocalTime of 06:00 but if they pass in AMPM_OF_DAY of "AM" the get no LocalTime in the result. Is that right? If so, I don't think this is sustainable.

Thats why I think AMPM_OF_DAY will have to be resolved to a dayPeriod of "am" or "pm". If dayPeriod is more precise than AMPM_OF_DAY, then dayPeriod can silently take precedence

@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 3, 2020

/csr

@openjdk openjdk bot added the csr label Nov 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 3, 2020

@naotoj this pull request will not be integrated until the CSR request JDK-8254629 for issue JDK-8247781 has been approved.

@openjdk openjdk bot removed the csr label Nov 5, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 5, 2020

@naotoj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8247781: Day periods support

Reviewed-by: scolebourne, rriggs, joehw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 3 new commits pushed to the master branch:

  • 0357db3: 8256287: [windows] add loop fuse to map_or_reserve_memory_aligned
  • 6e35bcb: 8256205: Simplify compiler calling convention handling
  • 68fd71d: 8256414: add optimized build to submit workflow

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 5, 2020
@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 5, 2020

/reviewer credit jodastephen

@openjdk
Copy link

@openjdk openjdk bot commented Nov 5, 2020

@naotoj Could not parse jodastephen as a valid reviewer.
Syntax: /reviewer (credit|remove) [@user | openjdk-user]+. For example:

  • /reviewer credit @openjdk-bot
  • /reviewer credit duke
  • /reviewer credit @user1 @user2

@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 5, 2020

/reviewer credit @jodastephen

@openjdk
Copy link

@openjdk openjdk bot commented Nov 5, 2020

@naotoj Reviewer scolebourne has already made an authenticated review of this PR, and does not need to be credited manually.

updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint / 720);
}
}
}
Copy link
Contributor

@jodastephen jodastephen Nov 5, 2020

Choose a reason for hiding this comment

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

I find the whole "half way between the start and end" behaviour of day periods odd anyway, but if it is to be supported then it should be consistent as you've implemented.

Another option I should have thought of earlier would be to simply not support the "half way between the start and end" behaviour of LDML in either dayPeriod or AM/PM. But since LDML defines it, you've implemented it, and it isn't overly harmful I think its OK to leave it in.

Would it be possible for STRICT mode to not have the "half way between the start and end" behaviour?

{LENIENT, -1, -1},
{LENIENT, 2, 2},
{STRICT, 0, LocalTime.of(6, 0), 0},
{STRICT, 1, LocalTime.of(18, 0), 1},
Copy link
Contributor

@jodastephen jodastephen Nov 5, 2020

Choose a reason for hiding this comment

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

As mentioned in my other comment, this seems odd in STRICT mode.

Copy link
Member Author

@naotoj naotoj Nov 6, 2020

Choose a reason for hiding this comment

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

Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in SMART/LENIENT modes?

Copy link
Contributor

@jodastephen jodastephen Nov 6, 2020

Choose a reason for hiding this comment

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

No. I mean that when resolving AMPM/dayPeriod in strict mode, and there is no HOUR_OF_DAY or HOUR_OF_AMPM, then do not resolve using "half way between"(ie. fail). This will produce a result where LocalTime cannot be obtained.

var f = DateTimeFormatter.ofPattern("B").withResolverStyle(ResolverStyle.STRICT);
var t = LocalTime.from("at night", f);

would throw an exception in STRICT mode (whereas SMART or LENIENT would return a LocalTime). Same with pattern "a".

Copy link
Member Author

@naotoj naotoj Nov 6, 2020

Choose a reason for hiding this comment

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

Changed to throw an exception in STRICT mode.

public int compare(String str1, String str2) {
return str1.length() == str2.length() ? str1.compareTo(str2) : str1.length() - str2.length();
public boolean format(DateTimePrintContext context, StringBuilder buf) {
Long value = context.getValue(MINUTE_OF_DAY);
Copy link
Contributor

@jodastephen jodastephen Nov 5, 2020

Choose a reason for hiding this comment

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

This does not match the spec: " During formatting, the day period is obtained from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"

It is possible and legal to create a Temporal that returns HOUR_OF_DAY and MINUTE_OF_HOUR but not MINUTE_OF_DAY. As such, this method must be changed to follow the spec.


In addition, it is possible for HOUR_OF_DAY and MINUTE_OF_HOUR to be outside their normal bounds. The right behaviour would be to combine the two fields within this method, and then use mod to get the value into the range 0 to 1440 before calling dayPeriod.include. (While the fall back behaviour below does cover this, it would be better to do what I propose here.)

An example of this is a TransportTime class where the day runs from 03:00 to 27:00 each day (because trains run after midnight for no extra cost to the passenger, and it is more convenient for the operator to treat the date that way). A TransportTime of 26:30 should still resolve to "night1" rather than fall back to "am".

Copy link
Member Author

@naotoj naotoj Nov 6, 2020

Choose a reason for hiding this comment

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

Fixed.

@@ -1480,6 +1479,9 @@ public DateTimeFormatterBuilder appendLiteral(String literal) {
* and no {@code HOUR_OF_DAY} is resolved, the parsed day period takes precedence.
* If any conflict occurs in {@link ResolverStyle#LENIENT LENIENT} mode, no
* exception is thrown and the day period is ignored.
* <p>
* "midnight" type allows both "00:00" as the start-of-day and "24:00" as the
Copy link
Contributor

@RogerRiggs RogerRiggs Nov 12, 2020

Choose a reason for hiding this comment

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

Editorial: Add "The " at the beginning of the sentence.

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

assertEquals((long)dtf.parse("0 in the morning").getLong(ChronoField.HOUR_OF_DAY), 6L);
try {
dtf.parse("0 at night");
throw new RuntimeException("DateTimeParseException should be thrown");
Copy link
Contributor

@RogerRiggs RogerRiggs Nov 12, 2020

Choose a reason for hiding this comment

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

Testng has Assert.fail(message) for this case.

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

if (dayPeriod != null) {
long midpoint = dayPeriod.mid();
fieldValues.put(HOUR_OF_DAY, midpoint / 60);
fieldValues.put(MINUTE_OF_HOUR, midpoint % 60);
Copy link
Contributor

@jodastephen jodastephen Nov 12, 2020

Choose a reason for hiding this comment

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

Set dayPeriod = null here, as it has been successfully used.

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

if (resolverStyle != ResolverStyle.LENIENT) {
HOUR_OF_AMPM.checkValidValue(hoap);
}
if (dayPeriod.includes((hoap % 24 + 12) * 60)) {
Copy link
Contributor

@jodastephen jodastephen Nov 12, 2020

Choose a reason for hiding this comment

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

Pretty sure the 24 should be 12.

Also, it should use Math.floorMod() to handle HOUR_OF_AMPM = -1 (which is 11 o'clock).

Also, this doesn't take into account minutes. So if the day period rule is afternoon1 to 18:30 and evening1 after 18:30, and the input is HOUR_OF_AMPM = 6, MINUTE_OF_HOUR = 45, this will fail to resolve,

Something like this should work (no need for updateCheckDayPeriodConflict):

long hoap = fieldValues.remove(HOUR_OF_AMPM);
if (resolverStyle == ResolverStyle.LENIENT) {
  HOUR_OF_AMPM.checkValidValue(hoap);
}
Long mohObj = fieldValues.get(MINUTE_OF_HOUR);
long moh = mohObj != null ? Math.floorMod(mohObj, 60) : 0;
long excessHours = dayPeriod.includes(Math.floorMod(hoap, 12) + 12 * 60 + moh ? 12 : 0;
long hod = Math.addExact(hoap, excessHours);
updateCheckConflict(HOUR_OF_AMPM, HOUR_OF_DAY, hod);
dayPeriod = null;

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

@@ -506,9 +542,32 @@ private void resolveTimeLenient() {
fieldValues.put(NANO_OF_SECOND, cos * 1_000L);
}

// Set the hour-of-day, if not exist and not in STRICT, to the mid point of the day period or am/pm.
if (!fieldValues.containsKey(HOUR_OF_DAY) && resolverStyle != ResolverStyle.STRICT) {
Copy link
Contributor

@jodastephen jodastephen Nov 12, 2020

Choose a reason for hiding this comment

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

Since this logic replaces both HOUR_OF_DAY and MINUTE_OF_HOUR I think it should only be invoked if both do not exist. Beyond that, it doesn't really make sense to do this logic if second/sub-second are present. Thus, it needs to check all four fields and can then directly set the time.

if (!fieldValues.containsKey(HOUR_OF_DAY) && 
    !fieldValues.containsKey(MINUTE_OF_HOUR) && 
    !fieldValues.containsKey(SECOND_OF_MINUTE) && 
    !fieldValues.containsKey(NANO_OF_SECOND) && 
    resolverStyle != ResolverStyle.STRICT) {

  if (dayPeriod != null) {
    long midpoint = dayPeriod.mid();
    resolveTime(midpoint / 60, midpoint % 60, 0, 0);
    dayPeriod = null;
  } else if (fieldValues.containsKey(AMPM_OF_DAY)) {
    long ap = fieldValues.remove(AMPM_OF_DAY);
    if (resolverStyle == ResolverStyle.LENIENT) {
      resolveTime(Math.addExact(Math.multiplyExact(ap, 12), 6), 0, 0, 0);
    } else {  // SMART
      AMPM_OF_DAY.checkValidValue(ap);
      resolveTime(ap * 12 + 6, 0, 0, 0);
}

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

// merge hour/minute/second/nano leniently
Long hod = fieldValues.get(HOUR_OF_DAY);
if (hod != null) {
if (dayPeriod != null) {
// Check whether the hod is within the day period
updateCheckDayPeriodConflict(HOUR_OF_DAY, hod);
Copy link
Contributor

@jodastephen jodastephen Nov 12, 2020

Choose a reason for hiding this comment

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

With the other changes, this is the only use of this method and it can therefore be simplified (no need to check for conflicts, just whether it matches the day period).

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

{"h B", ResolverStyle.SMART, "51 at night", 3},
{"h B", ResolverStyle.SMART, "59 in the morning", 11},

{"H B", ResolverStyle.LENIENT, "47 at night", 23},
Copy link
Contributor

@jodastephen jodastephen Nov 12, 2020

Choose a reason for hiding this comment

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

This test should be split in two - smart (fails) and lenient (succeeds). The lenient tests should ensure that p.query(DateTimeFormatter.parsedExcessDays()) returns the expected number of excess days.

I'd also add a test for a negative value such as "-2 at night"

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

@jodastephen jodastephen left a comment

Approved with one overflow to fix.

The spec could do with some rewording too. It might be better to explicitly mention the "resolving phase" with the three parts:

The day period is combined with other fields to make a LocalTime in the resolving phase. If the HOUR_OF_AMPM field is present, it is combined with the day period to make HOUR_OF_DAY taking into account any MINUTE_OF_HOUR value. If HOUR_OF_DAY is present, it is validated against the day period taking into account any MINUTE_OF_HOUR value. If a day period is present without HOUR_OF_DAY, MINUTE_OF_HOUR, SECOND_OF_MINUTE and NANO_OF_SECOND then the midpoint of the day period is set as the time.

Note that the above is incomplete, and it doesn't describe STRICT/LENIENT, so the actual words will be more complex,

return false;
}
Long moh = context.getValue(MINUTE_OF_HOUR);
long value = (hod * 60 + (moh != null ? moh : 0)) % 1_440;
Copy link
Contributor

@jodastephen jodastephen Nov 12, 2020

Choose a reason for hiding this comment

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

long value = Math.floorMod(hod, 24) * 60 + (moh != null ? Math.floorMod(moh, 60) : 0);

and remove the next three lines

Copy link
Member Author

@naotoj naotoj Nov 12, 2020

Choose a reason for hiding this comment

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

Thanks, Stephen. I made the changes based on your comments.

@naotoj
Copy link
Member Author

@naotoj naotoj commented Nov 16, 2020

/integrate

@openjdk openjdk bot closed this Nov 16, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 16, 2020

@naotoj Since your change was applied there have been 3 commits pushed to the master branch:

  • 0357db3: 8256287: [windows] add loop fuse to map_or_reserve_memory_aligned
  • 6e35bcb: 8256205: Simplify compiler calling convention handling
  • 68fd71d: 8256414: add optimized build to submit workflow

Your commit was automatically rebased without conflicts.

Pushed as commit bf84dac.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk-notifier bot referenced this issue Nov 16, 2020
Reviewed-by: scolebourne, rriggs, joehw
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants