ICU-21947 Replace FixedDecimal with DecimalQuantity in PluralRule sample parsing

[echeran]

Checklist

• Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-21947
• Required: The PR title must be prefixed with a JIRA Issue number.
• Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
• Required: Each commit message must be prefixed with a JIRA Issue number.
• Issue accepted (done by Technical Committee after discussion)
• Tests included, if applicable
• API docs and/or User Guide docs changed or added, if applicable

[macchiati]

Did you ensure that CLDR can build with these changes? (You can't delete FixedDecimal, etc. unless that is true.)

[echeran]

Yes, I haven't yet gotten to the CLDR part, but you're right, this shouldn't be merged without testing it together with a PR on the CLDR side.

FYI, FixedDecimal isn't being removed, this PR is only attempting to remove its parse function. The current implementation for FixedDecimal parsing is flawed for compact notation numbers -- the resulting FixedDecimals sometimes give incorrect values for certain plural operands (but somehow, so far, it happens in a way that hasn't burned us....yet). So I thought it made sense to remove parsing for FixedDecimals so that we don't reach for it again, now that we have the proper corresponding replacement DecimalQuantity via #2012 with reliable parse and format functions.

Another FYI, the file that this PR is attempting to remove is PluralSamples.java. I thought to do so because: 1) it is not being referenced anywhere, and isn't running a test, 2) it still uses FixedDecimal (as stated above, we should stop using), 3) relevant functionality seems duplicated in PluralRules.java, 4) it's marked as @deprecated with class-level Java doc saying "Refactor samples as first step to moving into CLDR". I think that is reasonable, but let me know if I'm missing something regarding PluralSamples.java.

[sffc]

This is too disruptive of a change to land right now, and C++ is not done. I suggest holding this until at least after RC.

[echeran]

Let me know what you think of these changes to the ICU side of work based on the review feedback so far.

FYI, in parallel, I'm working on the CLDR side of work, over in PR #1800. I've managed to reverse-engineer the instructions for how to do this type of ICU-CLDR tandem development, I've gotten the tests to compile, and I've gotten test failures down to 6 cases across 2 tests.

[sffc]

done with PluralRules.java

[macchiati]

A good test would be that each of the samples in the plural and ordinal rules in CLDR round-trip; you get exactly the same results from ICU code. This doesn't fully test what CLDR does, because it also *selects* the samples using code that depends on FixedDecimal — but it goes a long way, and lets you iterate in the ICU environment in case there are problems.

…

On Fri, Mar 18, 2022 at 8:15 AM Shane F. Carr ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In icu4j/main/classes/core/src/com/ibm/icu/text/PluralRules.java <#2007 (comment)>: > @@ -1225,10 +1225,12 @@ static DecimalQuantitySamples parse(String source) { } private static void checkDecimal(SampleType sampleType2, DecimalQuantity sample) { - if ((sampleType2 == SampleType.INTEGER && sample.getPluralOperand(Operand.v) != 0) || - (sampleType2 == SampleType.DECIMAL && sample.getPluralOperand(Operand.v) == 0 - && sample.getPluralOperand(Operand.e) == 0 - && sample.getPluralOperand(Operand.n) != 0)) { // is this double == 0 comparison fine? I think it is, just want to make sure. + // TODO: Remove the need for the fallback check for exponent notation integers classified Nit: TODO(CLDR-15452) — Reply to this email directly, view it on GitHub <#2007 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMFETGUDRJAPLKHBILTVASMY5ANCNFSM5PLBLYBQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[echeran]

A good test would be that each of the samples in the plural and ordinal rules in CLDR round-trip; you get exactly the same results from ICU code. This doesn't fully test what CLDR does, because it also selects the samples using code that depends on FixedDecimal ...

@macchiati I believe that we already have tests in ICU that do what you were describing as occurring in CLDR. I just now added source comments on relevant tests to clarify what each test is actually doing because it is not always obvious. So for the round trip that goes plural rule -> keyword(s) -> sample(s) -> select -> keyword (round trip on the plural rule keyword), the relevant test is PluralRulesTest.TestDecimalQuantitySamples. This is performed over all locales available via API. If the round trip you were thinking of was the narrower concern of sample string -> parse -> format -> sample string, we did that in the previous PR #2012 .

And the CLDR tests are still passing when I run the corresponding CLDR changes unicode-org/cldr#1800 against this PR, via the instructions in the CLDR PR. So long as there are tests in CLDR that cover any extra functionality we are concerned about, then we have checked all the impacts that we know how to check.

[sffc]

Nit: do a global replacement for e with c in samplesString and then perform the equality check as usual

[echeran]

Done.

[sffc]

Looks like I approved these changes 6 months ago, and the changes since then look reasonable, so I will approve again :)

[echeran]

@macchiati You still have the "changes requested" bit set that blocks merging. How does this PR look, and how does the above discussion sound?

[macchiati]

It looks good, but I'm concerned about the staging. We use FixedDecimal heavily in CLDR. If that disappears, we would not be able to update to the latest version of ICU until CLDR is fixed to work around it. We're heads-to-the ground trying to get the freeze done by the 17th.

Would it be feasible to leave FixedDecimal around so that we have the chance to get rid of it when the timing is better (and having both of them work at the same time, to ease the migration)?

[jira-pull-request-webhook]

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

[markusicu]

rslgtm