pjfanning commented on code in PR #1492:
URL: https://github.com/apache/poi/pull/1492#discussion_r4381059174
##########
poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java:
##########
@@ -967,8 +969,15 @@ void testFractionWholeNumberAboveLongMax() {
@Test
void testImproperFractionNumeratorOverflow() {
- // the numerator overflows an int long before the denominator reaches
9 digits
- assertEquals("202510/81",
CellFormat.getInstance("?????????/?????????").apply(2500.123456789).text.trim());
+ // the numerator overflows an int long before the denominator reaches
9 digits, which used to
+ // lose precision: 202510/81 here, and 2100000000/1 for 2.1e9 +
0.123456789
+ CellFormat cf = CellFormat.getInstance("?????????/?????????");
+ assertEquals("1044102120552/417620225",
cf.apply(2500.123456789).text.trim());
+ for (double value : new double[]{300000.7071067811, 2.1e9 +
0.123456789}) {
+ String[] parts = cf.apply(value).text.trim().split("/");
+ double shown = new BigDecimal(parts[0]).divide(new
BigDecimal(parts[1]), MathContext.DECIMAL64).doubleValue();
+ assertEquals(value, shown, Math.ulp(value), "value " + value);
Review Comment:
The round-trip check doesn't test what Excel shows. It would be better to
pin the exact strings Excel's `=TEXT(...)` gives for these two values. If the
round-trip stays, note that dividing with `DECIMAL64` and then calling
`doubleValue()` rounds twice, so a 1-ulp tolerance could fail near the
boundary; `DECIMAL128` would be safer.
##########
poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java:
##########
@@ -662,10 +662,11 @@ private void writeScientific(double value, StringBuffer
output, Set<CellNumberSt
@SuppressWarnings("unchecked")
private void writeFraction(double value, StringBuffer result,
double fractional, StringBuffer output, Set<CellNumberStringMod>
mods) {
- // the fraction is worked out with ints, so for an improper fraction
of a large value it is worked out
- // for the part after the whole number, and the whole number is added
to the numerator when it is written
+ // the fraction is worked out with ints, so for an improper fraction
it is worked out for the part after
+ // the whole number, and the whole number is added to the numerator
when it is written; the closest
+ // fraction to v - floor(v) with a given maximum denominator is the
closest to v, less floor(v)
BigInteger wholeNumber = null;
- if (improperFraction && fractional * Math.max(fixedDenominator, 1) >=
Integer.MAX_VALUE) {
+ if (improperFraction) {
Review Comment:
Excel doesn't agree with the unconditional split.
`=TEXT(2500.123456789,"?????????/?????????")` in Excel gives `202510/81`, the
same as before this PR. Excel stops well before the denominator limit (for pi
with 9 `?` it gives `244252/1725033`), so the old threshold happened to match
Excel here. Could the split stay narrower, e.g. only where the old path lost
the fraction entirely (the `2100000000/1` case)?
Also `NaN`: before, `NaN * d >= Integer.MAX_VALUE` was false, so `NaN`
skipped this block. Now `new BigDecimal(Math.floor(NaN))` throws
`NumberFormatException`, and that happens outside the try/catch further down. A
`Double.isFinite(fractional)` guard would avoid it.
##########
poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java:
##########
@@ -967,8 +969,15 @@ void testFractionWholeNumberAboveLongMax() {
@Test
void testImproperFractionNumeratorOverflow() {
- // the numerator overflows an int long before the denominator reaches
9 digits
- assertEquals("202510/81",
CellFormat.getInstance("?????????/?????????").apply(2500.123456789).text.trim());
+ // the numerator overflows an int long before the denominator reaches
9 digits, which used to
+ // lose precision: 202510/81 here, and 2100000000/1 for 2.1e9 +
0.123456789
+ CellFormat cf = CellFormat.getInstance("?????????/?????????");
+ assertEquals("1044102120552/417620225",
cf.apply(2500.123456789).text.trim());
Review Comment:
Excel shows `202510/81` for this value (checked with
`=TEXT(2500.123456789,"?????????/?????????")`), so the original assertion was
the Excel-compatible one and should stay.
##########
poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java:
##########
@@ -674,8 +675,7 @@ private void writeFraction(double value, StringBuffer
result,
// Work out the fraction first, as it can round to 0, or up to 1
int n;
int d;
- // the "fractional % 1" captures integer values in improper fractions
- if (fractional == 0 || (improperFraction && fractional % 1 == 0)) {
+ if (fractional == 0) {
// 0 as a fraction is reported by excel as 0/1
n = Math.toIntExact(Math.round(fractional));
Review Comment:
With the `fractional % 1 == 0` case gone, `fractional` is always 0 here, so
this is always `n = 0`, and the `n *= d` below has nothing to multiply.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
