ICU-23104 Fix the handling of part-per-1e9

See #3663

Author: younies

icu4c/source/i18n/measunit_extra.cpp

@@ -1418,21 +1418,21 @@ MeasureUnit MeasureUnit::withPrefix(UMeasurePrefix prefix,
 }
 
 uint64_t MeasureUnit::getConstantDenominator(UErrorCode &status) const {
-    auto complexity = this->getComplexity(status);
+    // TODO(ICU-23219)
+    auto measureUnitImpl = MeasureUnitImpl::forMeasureUnitMaybeCopy(*this, status);
     if (U_FAILURE(status)) {
         return 0;
     }
 
+    auto complexity = measureUnitImpl.complexity;
+
     if (complexity != UMEASURE_UNIT_SINGLE && complexity != UMEASURE_UNIT_COMPOUND) {
         status = U_ILLEGAL_ARGUMENT_ERROR;
         return 0;
     }
 
-    if (this->fImpl == nullptr) {
-        return 0;
-    }
 
-    return this->fImpl->constantDenominator;
+    return measureUnitImpl.constantDenominator;
 }
 
 MeasureUnit MeasureUnit::withConstantDenominator(uint64_t denominator, UErrorCode &status) const {
@@ -1501,8 +1501,8 @@ MeasureUnit MeasureUnit::product(const MeasureUnit& other, UErrorCode& status) c
         impl.appendSingleUnit(*otherImpl.singleUnits[i], status);
     }
 
-    uint64_t currentConstatDenominator = this->getConstantDenominator(status);
-    uint64_t otherConstantDenominator = other.getConstantDenominator(status);
+    uint64_t currentConstatDenominator = impl.constantDenominator;
+    uint64_t otherConstantDenominator = otherImpl.constantDenominator;
 
     // TODO: we can also multiply the constant denominators instead of returning an error.
     if (currentConstatDenominator != 0 && otherConstantDenominator != 0) {

icu4c/source/test/intltest/numbertest_skeletons.cpp

@@ -7,6 +7,7 @@
 
 #include "unicode/dcfmtsym.h"
 #include "unicode/ustring.h"
+#include "unicode/unistr.h"
 
 #include "cstr.h"
 #include "numbertest.h"
@@ -467,28 +468,35 @@ void NumberSkeletonTest::perUnitToSkeleton() {
         {u"area", u"acre"},
         {u"concentr", u"percent"},
         {u"concentr", u"permille"},
-        {u"concentr", u"part-per-1e6"},
         {u"concentr", u"permyriad"},
         {u"digital", u"bit"},
         {u"length", u"yard"},
+        {u"concentr", u"part-per-1e9"},
     };
 
-    for (const auto& cas1 : cases) {
-        for (const auto& cas2 : cases) {
+    for (const auto& numeratorCase : cases) {
+        for (const auto& denominatorCase : cases) {
             UnicodeString skeleton(u"measure-unit/");
-            skeleton += cas1.type;
+            skeleton += numeratorCase.type;
             skeleton += u"-";
-            skeleton += cas1.subtype;
+            skeleton += numeratorCase.subtype;
             skeleton += u" ";
             skeleton += u"per-measure-unit/";
-            skeleton += cas2.type;
+            skeleton += denominatorCase.type;
             skeleton += u"-";
-            skeleton += cas2.subtype;
+            skeleton += denominatorCase.subtype;
+
+            // Convert UTF-16 string to UTF-8 for forIdentifier
+            UnicodeString temp(denominatorCase.subtype);
+            CStr utf8String(temp);
+            MeasureUnit denominatorUnit = MeasureUnit::forIdentifier(utf8String(), status);
+            
+            // If the units has a constant denominator, we skip the test because we could not have a per unit with a constant denominator.
+            if (denominatorUnit.getConstantDenominator(status) != 0) {
+                continue;
+            }
 
             status.setScope(skeleton);
-            if (u_strcmp(cas1.subtype, u"part-per-1e6")==0 || u_strcmp(cas2.subtype, u"part-per-1e6")==0) {
-                logKnownIssue("ICU-23104", "Strange handling of part-per-1e9 & volumes in skeletons");
-            } else if (cas1.type != cas2.type && cas1.subtype != cas2.subtype) {
                 UnicodeString toSkeleton = NumberFormatter::forSkeleton(
                     skeleton, status).toSkeleton(status);
                 if (status.errIfFailureAndReset()) {
@@ -498,19 +506,18 @@ void NumberSkeletonTest::perUnitToSkeleton() {
                 UnicodeString msg;
                 msg.append(toSkeleton)
                     .append(" should contain '")
-                    .append(UnicodeString(cas1.subtype))
+                    .append(UnicodeString(numeratorCase.subtype))
                     .append("' when constructed from ")
                     .append(skeleton);
-                assertTrue(msg, toSkeleton.indexOf(cas1.subtype) >= 0);
+                assertTrue(msg, toSkeleton.indexOf(numeratorCase.subtype) >= 0);
 
                 msg.remove();
                 msg.append(toSkeleton)
                     .append(" should contain '")
-                    .append(UnicodeString(cas2.subtype))
+                    .append(UnicodeString(denominatorCase.subtype))
                     .append("' when constructed from ")
                     .append(skeleton);
-                assertTrue(msg, toSkeleton.indexOf(cas2.subtype) >= 0);
-            }
+                assertTrue(msg, toSkeleton.indexOf(denominatorCase.subtype) >= 0);
         }
     }
 }

icu4j/main/core/src/main/java/com/ibm/icu/util/MeasureUnit.java

@@ -602,16 +602,17 @@ public MeasureUnit withConstantDenominator(long denominator) {
      * @draft ICU 77
      */
     public long getConstantDenominator() {
-        if (this.getComplexity() != Complexity.COMPOUND && this.getComplexity() != Complexity.SINGLE) {
+        // TODO(ICU-23219)
+        MeasureUnitImpl measureUnitImpl = getCopyOfMeasureUnitImpl();
+
+        if (measureUnitImpl.getComplexity() != Complexity.COMPOUND
+                && measureUnitImpl.getComplexity() != Complexity.SINGLE) {
             throw new UnsupportedOperationException(
                     "Constant denominator is only supported for COMPOUND & SINGLE units");
         }
 
-        if (this.measureUnitImpl == null) {
-            return 0;
-        }
 
-        return this.measureUnitImpl.getConstantDenominator();
+        return measureUnitImpl.getConstantDenominator();
     }
 
     /**
@@ -716,8 +717,8 @@ public MeasureUnit product(MeasureUnit other) {
             implCopy.appendSingleUnit(singleUnit);
         }
 
-        long thisConstantDenominator = this.getConstantDenominator();
-        long otherConstantDenominator = other.getConstantDenominator();
+        long thisConstantDenominator = implCopy.getConstantDenominator();
+        long otherConstantDenominator = otherImplRef.getConstantDenominator();
 
         // TODO: we can also multiply the constant denominators instead of throwing an
         // exception.

icu4j/main/core/src/test/java/com/ibm/icu/dev/test/number/NumberSkeletonTest.java

@@ -11,6 +11,7 @@
 import com.ibm.icu.number.LocalizedNumberFormatter;
 import com.ibm.icu.number.NumberFormatter;
 import com.ibm.icu.number.SkeletonSyntaxException;
+import com.ibm.icu.util.MeasureUnit;
 import com.ibm.icu.util.ULocale;
 
 /**
@@ -428,35 +429,52 @@ public void perUnitInArabic() {
 
     @Test
     public void perUnitToSkeleton() {
-        String[][] cases = {
-            {"area", "acre"},
-            {"concentr", "percent"},
-            {"concentr", "permille"},
-            {"concentr", "part-per-1e9"},
-            {"concentr", "permyriad"},
-            {"digital", "bit"},
-            {"length", "yard"},
+        class TestCase {
+            String type;
+            String subType;
+
+            TestCase(String type, String sub_type) {
+                this.type = type;
+                this.subType = sub_type;
+            }
+        }
+
+        TestCase[] cases = new TestCase[] {
+                new TestCase("area", "acre"),
+                new TestCase("concentr", "percent"),
+                new TestCase("concentr", "permille"),
+                new TestCase("concentr", "permyriad"),
+                new TestCase("digital", "bit"),
+                new TestCase("length", "yard"),
+                new TestCase("concentr", "part-per-1e9"),
         };
 
-        for (String[] cas1 : cases) {
-            for (String[] cas2 : cases) {
-                String skeleton = "measure-unit/" + cas1[0] + "-" + cas1[1] + " per-measure-unit/" +
-                                  cas2[0] + "-" + cas2[1];
-
-                if (cas1[1].equals("part-per-1e9") || cas2[1].equals("part-per-1e9")) {
-                    logKnownIssue("ICU-23104", "Strange handling of part-per-1e9 in skeletons");
-                } else if (cas1[0] != cas2[0] && cas1[1] != cas2[1]) {
-                    String toSkeleton = NumberFormatter.forSkeleton(skeleton).toSkeleton();
-
-                    // Ensure both subtype are in the toSkeleton.
-                    String msg;
-                    msg = toSkeleton + " should contain '" + cas1[1] + "' when constructed from " +
-                          skeleton;
-                    assertTrue(msg, toSkeleton.indexOf(cas1[1]) >= 0);
-                    msg = toSkeleton + " should contain '" + cas2[1] + "' when constructed from " +
-                          skeleton;
-                    assertTrue(msg, toSkeleton.indexOf(cas2[1]) >= 0);
+        for (TestCase numeratorCase : cases) {
+            for (TestCase denominatorCase : cases) {
+                MeasureUnit denominatorUnit = MeasureUnit.forIdentifier(denominatorCase.subType);
+                
+                // If the units has a constant denominator, we skip the test because we could
+                // not have a per unit with a constant denominator.
+                if (denominatorUnit.getConstantDenominator() != 0) {
+                    continue;
                 }
+
+                String skeleton = "measure-unit/" + numeratorCase.type + "-" + numeratorCase.subType
+                        + " per-measure-unit/" +
+                        denominatorCase.type + "-" + denominatorCase.subType;
+                String toSkeleton = NumberFormatter.forSkeleton(skeleton).toSkeleton();
+
+
+                // Ensure both subtype (Units Cldr IDs) are in the toSkeleton.
+                String skeletonMsgNumString = toSkeleton + " should contain '" + numeratorCase.subType
+                        + "' when constructed from " +
+                        skeleton;
+                assertTrue(skeletonMsgNumString, toSkeleton.indexOf(numeratorCase.subType) >= 0);
+
+                String skeletonMsgDenString = toSkeleton + " should contain '" + denominatorCase.subType
+                        + "' when constructed from " +
+                        skeleton;
+                assertTrue(skeletonMsgDenString, toSkeleton.indexOf(denominatorCase.subType) >= 0);
             }
         }
     }