You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@poi.apache.org by fa...@apache.org on 2026/10/26 14:25:11 UTC
(poi) branch trunk updated: Fraction formatting: fix SimpleFraction overflow and cap denominator … (#1473)
This is an automated email from the ASF dual-hosted git repository.
pjfanning pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/poi.git
The following commit(s) were added to refs/heads/trunk by this push:
new 68d4db94e0 Fraction formatting: fix SimpleFraction overflow and cap denominator … (#1473)
68d4db94e0 is described below
commit 68d4db94e081df3d8f5f3159a7e07386c67b8613
Author: Tim Allison <ta...@apache.org>
AuthorDate: Mon Oct 26 10:25:04 2026 -0400
Fraction formatting: fix SimpleFraction overflow and cap denominator … (#1473)
* Fraction formatting: fix SimpleFraction overflow and cap denominator digits
* clean up
* fixes/updates based on PJ's feedback
* drop to debug
* Probe fraction-with-text formats with a fixed value, so the cached format does not depend on cell order
---
.../apache/poi/ss/format/CellNumberFormatter.java | 8 +-
.../org/apache/poi/ss/format/SimpleFraction.java | 8 ++
.../org/apache/poi/ss/usermodel/DataFormatter.java | 57 ++++++++-----
.../org/apache/poi/ss/format/TestCellFormat.java | 40 ++++++++++
.../apache/poi/ss/format/TestSimpleFraction.java | 93 ++++++++++++++++++++++
.../apache/poi/ss/usermodel/TestDataFormatter.java | 26 ++++++
6 files changed, 212 insertions(+), 20 deletions(-)
diff --git a/poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java b/poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java
index b4559f72ca..00a190d743 100644
--- a/poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java
+++ b/poi/src/main/java/org/apache/poi/ss/format/CellNumberFormatter.java
@@ -62,6 +62,8 @@ public class CellNumberFormatter extends CellFormatter {
private final List<Special> denominatorSpecials = new ArrayList<>();
private final List<Special> exponentSpecials = new ArrayList<>();
private final List<Special> exponentDigitSpecials = new ArrayList<>();
+ /** 10^9 - 1 is the largest all-nines denominator that fits in an int */
+ private static final int MAX_DENOMINATOR_DIGITS = 9;
private final int maxDenominator;
/** the denominator given as a number, e.g. 4 for "# ?/4", or 0 */
private final int fixedDenominator;
@@ -335,7 +337,7 @@ public class CellNumberFormatter extends CellFormatter {
}
private static int maxValue(List<Special> s) {
- return Math.toIntExact(Math.round(Math.pow(10, s.size()) - 1));
+ return Math.toIntExact(Math.round(Math.pow(10, Math.min(s.size(), MAX_DENOMINATOR_DIGITS)) - 1));
}
private List<Special> specialsFor(int pos, int takeFirst) {
@@ -661,6 +663,10 @@ public class CellNumberFormatter extends CellFormatter {
@SuppressWarnings("unchecked")
private void writeFraction(double value, StringBuffer result,
double fractional, StringBuffer output, Set<CellNumberStringMod> mods) {
+ // the numerator is an int, so an improper fraction of a huge value cannot be shown
+ if (Math.abs(fractional) * Math.max(fixedDenominator, 1) >= Integer.MAX_VALUE) {
+ throw new IllegalArgumentException("Value is too large to format as a fraction: " + fractional);
+ }
// Work out the fraction first, as it can round to 0, or up to 1
int n;
diff --git a/poi/src/main/java/org/apache/poi/ss/format/SimpleFraction.java b/poi/src/main/java/org/apache/poi/ss/format/SimpleFraction.java
index 5c821cda8b..31b37a45dd 100644
--- a/poi/src/main/java/org/apache/poi/ss/format/SimpleFraction.java
+++ b/poi/src/main/java/org/apache/poi/ss/format/SimpleFraction.java
@@ -102,6 +102,10 @@ public class SimpleFraction {
do {
++n;
double r1 = 1.0 / (r0 - a0);
+ if (r1 > overflow) {
+ // a zero or tiny remainder: the next convergent would overflow, and p1/q1 is already the closest
+ return new SimpleFraction(Math.toIntExact(p1), Math.toIntExact(q1));
+ }
long a1 = (long)Math.floor(r1);
p2 = (a1 * p1) + p0;
q2 = (a1 * q1) + q0;
@@ -111,6 +115,10 @@ public class SimpleFraction {
return new SimpleFraction(Math.toIntExact(p1), Math.toIntExact(q1));
}
+ if (epsilon == 0.0f && maxDenominator > 0 && Math.abs(p2) > overflow) {
+ // an improper fraction's numerator can overflow before the denominator reaches its maximum
+ return new SimpleFraction(Math.toIntExact(p1), Math.toIntExact(q1));
+ }
if ((p2 > overflow) || (q2 > overflow)) {
throw new IllegalStateException("Overflow trying to convert "+value+" to fraction ("+p2+"/"+q2+")");
}
diff --git a/poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java b/poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java
index 6f9370c455..09678a5fa1 100644
--- a/poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java
+++ b/poi/src/main/java/org/apache/poi/ss/usermodel/DataFormatter.java
@@ -553,28 +553,19 @@ public class DataFormatter {
// shows 3.75 as "15 =/=4": let CellFormat handle any fraction with text or spacing in it
boolean fractionWithText = formatStr.indexOf('/') >= 0 && hasTextInFraction(formatStr);
if (plainFraction || fractionWithText) {
+ Format fractionFormat = createFractionFormat(formatStr);
if (fractionWithText) {
try {
- return new CellFormatWrapper(CellFormat.getInstance(locale, formatStr));
+ CellFormat cellFormat = CellFormat.getInstance(locale, formatStr);
+ // probe with a fixed value, not cellValue, so the cached format does not depend on which
+ // value comes first; a format that only fails for some values falls back per value
+ cellFormat.apply(1.25);
+ return new CellFormatWrapper(cellFormat, fractionFormat);
} catch (Exception e) {
LOG.atWarn().withThrowable(e).log("Formatting failed for format {}, falling back", formatStr);
}
}
- String[] chunks = formatStr.split(";");
- for (String chunk1 : chunks) {
- String chunk = chunk1.replace("?", "#");
- Matcher matcher = fractionStripper.matcher(chunk);
- chunk = matcher.replaceAll(" ");
- chunk = multipleSpaces.matcher(chunk).replaceAll(" ");
- Matcher fractionMatcher = fractionPattern.matcher(chunk);
- //take the first match
- if (fractionMatcher.find()) {
- String wholePart = (fractionMatcher.group(1) == null) ? "" : defaultFractionWholePartFormat;
- return new FractionFormat(wholePart, fractionMatcher.group(3));
- }
- }
-
- return new FractionFormat(defaultFractionWholePartFormat, defaultFractionFractionPartFormat);
+ return fractionFormat;
}
// DecimalFormat cannot show literal text between digit placeholders, e.g. the
@@ -588,7 +579,7 @@ public class DataFormatter {
CellFormat cellFormat = CellFormat.getInstance(locale, formatStr.replace("'%'", "\\%"));
// a format CellFormat cannot make sense of, e.g. "#''0#0'1#10L16EE", fails here, not when formatting
cellFormat.apply(cellValue);
- return new CellFormatWrapper(cellFormat);
+ return new CellFormatWrapper(cellFormat, null);
} catch (Exception e) {
LOG.atWarn().withThrowable(e).log("Formatting failed for format {}, falling back", formatStr);
}
@@ -1023,6 +1014,24 @@ public class DataFormatter {
return sb.toString();
}
+ private Format createFractionFormat(String formatStr) {
+ String[] chunks = formatStr.split(";");
+ for (String chunk1 : chunks) {
+ String chunk = chunk1.replace("?", "#");
+ Matcher matcher = fractionStripper.matcher(chunk);
+ chunk = matcher.replaceAll(" ");
+ chunk = multipleSpaces.matcher(chunk).replaceAll(" ");
+ Matcher fractionMatcher = fractionPattern.matcher(chunk);
+ //take the first match
+ if (fractionMatcher.find()) {
+ String wholePart = (fractionMatcher.group(1) == null) ? "" : defaultFractionWholePartFormat;
+ return new FractionFormat(wholePart, fractionMatcher.group(3));
+ }
+ }
+
+ return new FractionFormat(defaultFractionWholePartFormat, defaultFractionFractionPartFormat);
+ }
+
/**
* Whether any section of the number format has quoted or escaped text with a digit
* placeholder both before and after it, e.g. {@code 0\.0} or {@code 0"-"00}, or
@@ -2252,12 +2261,22 @@ public class DataFormatter {
@SuppressWarnings("serial")
private final class CellFormatWrapper extends Format {
private final CellFormat cellFormat;
- private CellFormatWrapper(CellFormat cellFormat) {
+ /** used for a value the CellFormat fails on; null for General */
+ private final Format fallback;
+ private CellFormatWrapper(CellFormat cellFormat, Format fallback) {
this.cellFormat = cellFormat;
+ this.fallback = fallback;
}
@Override
public StringBuffer format(Object obj, StringBuffer toAppendTo, FieldPosition pos) {
- String text = cellFormat.apply(obj).text;
+ String text;
+ try {
+ text = cellFormat.apply(obj).text;
+ } catch (RuntimeException e) {
+ // the format was checked when it was created, but this value fails
+ LOG.atDebug().withThrowable(e).log("Formatting failed for value {}, falling back", obj);
+ return (fallback != null ? fallback : generalNumberFormat).format(obj, toAppendTo, pos);
+ }
return toAppendTo.append(emulateCSV ? text : text.trim());
}
@Override
diff --git a/poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java b/poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java
index 4ecb1ef419..554de36601 100644
--- a/poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java
+++ b/poi/src/test/java/org/apache/poi/ss/format/TestCellFormat.java
@@ -930,6 +930,46 @@ class TestCellFormat {
}
}
+ @Test
+ void testFractionDenominatorDigitsCapped() {
+ // more than 9 denominator digits overflowed an int and the format threw when applied;
+ // Excel gives the same fraction however many digits there are, but pads to every '?'
+ for (int digits : new int[]{9, 10, 20}) {
+ String q = "?".repeat(digits);
+ String pad = " ".repeat(digits - 1);
+ assertEquals("3 " + pad + "1/4" + pad, CellFormat.getInstance("# " + q + "/" + q).apply(3.25).text);
+ }
+
+ // a value with a long denominator gets the same fraction past 9 digits (POI's differs from Excel's for PI)
+ String nine = CellFormat.getInstance("# ?????????/?????????").apply(Math.PI).text.trim();
+ for (int digits : new int[]{10, 20}) {
+ String q = "?".repeat(digits);
+ String text = CellFormat.getInstance("# " + q + "/" + q).apply(Math.PI).text;
+ assertEquals(nine.replaceAll(" +", " "), text.trim().replaceAll(" +", " "));
+ }
+ }
+
+ @Test
+ void testImproperFractionTooLarge() {
+ // the numerator is an int, so this cannot be shown as 6000000001/2
+ assertThrows(IllegalArgumentException.class, () -> CellFormat.getInstance("?/?").apply(3e9 + 0.5));
+ assertThrows(IllegalArgumentException.class, () -> CellFormat.getInstance("?/16").apply(2.5e8 + 0.5));
+ assertEquals("3000000000 1/2", CellFormat.getInstance("# ?/?").apply(3e9 + 0.5).text);
+ }
+
+ @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());
+ }
+
+ @Test
+ void testFractionWithZeroRemainder() {
+ // the continued fraction for this value has a zero remainder, which used to overflow
+ assertEquals(" 1/324", CellFormat.getInstance("# ???/???").apply(0.00308641975308642).text);
+ assertEquals("0 ", CellFormat.getInstance("# ?/?").apply(0.00308641975308642).text);
+ }
+
@Test
void testAccountingFormats() {
char pound = '\u00A3';
diff --git a/poi/src/test/java/org/apache/poi/ss/format/TestSimpleFraction.java b/poi/src/test/java/org/apache/poi/ss/format/TestSimpleFraction.java
new file mode 100644
index 0000000000..a5d359b7f1
--- /dev/null
+++ b/poi/src/test/java/org/apache/poi/ss/format/TestSimpleFraction.java
@@ -0,0 +1,93 @@
+/* ====================================================================
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements. See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+==================================================================== */
+
+package org.apache.poi.ss.format;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.util.Random;
+
+import org.junit.jupiter.api.Test;
+
+class TestSimpleFraction {
+
+ private static void assertFraction(int numerator, int denominator, SimpleFraction actual) {
+ assertEquals(numerator + "/" + denominator, actual.getNumerator() + "/" + actual.getDenominator());
+ }
+
+ /**
+ * 1/0.00308641975308642 is exactly 324.0 as a double, so the remainder after the first step is zero
+ * and the next partial quotient was Long.MAX_VALUE, which overflowed the denominator.
+ */
+ @Test
+ void testZeroRemainder() {
+ assertFraction(1, 324, SimpleFraction.buildFractionMaxDenominator(0.00308641975308642, 1000));
+ assertFraction(0, 1, SimpleFraction.buildFractionMaxDenominator(0.00308641975308642, 100));
+ }
+
+ @Test
+ void testReciprocals() {
+ for (int k = 2; k < 10_000; k++) {
+ double value = 1.0 / k;
+ assertFraction(1, k, SimpleFraction.buildFractionMaxDenominator(value, 10_000));
+ }
+ }
+
+ @Test
+ void testTinyRemainder() {
+ // a partial quotient too large for the limit leaves the previous convergent
+ assertFraction(1, 3, SimpleFraction.buildFractionMaxDenominator(1.0 / 3 + 1e-15, 1000));
+ assertFraction(7, 2, SimpleFraction.buildFractionMaxDenominator(3.5 - 1e-14, 100));
+ }
+
+ @Test
+ void testNumeratorOverflow() {
+ // the numerator passes Integer.MAX_VALUE first, leaving the previous convergent
+ assertFraction(202510, 81, SimpleFraction.buildFractionMaxDenominator(2500.123456789, 1_000_000_000));
+ }
+
+ @Test
+ void testMaxDenominator() {
+ assertFraction(3, 5, SimpleFraction.buildFractionMaxDenominator(0.6, 10));
+ assertFraction(22, 7, SimpleFraction.buildFractionMaxDenominator(Math.PI, 10));
+ assertFraction(355, 113, SimpleFraction.buildFractionMaxDenominator(Math.PI, 1000));
+ }
+
+ /**
+ * One ulp away from p/q, 1/(remainder) is sometimes exactly an integer, so the next remainder is zero.
+ * About 0.12% of these draws overflowed before, so 200,000 draws all but always hit one.
+ */
+ @Test
+ void testOneUlpFromRationals() {
+ long seed = System.nanoTime();
+ Random random = new Random(seed);
+ for (int i = 0; i < 200_000; i++) {
+ int q = 2 + random.nextInt(998);
+ int p = 1 + random.nextInt(q - 1);
+ int g = gcd(p, q);
+ double exact = (double) p / q;
+ double value = random.nextBoolean() ? Math.nextUp(exact) : Math.nextDown(exact);
+ SimpleFraction f = SimpleFraction.buildFractionMaxDenominator(value, 1000);
+ assertEquals(p / g + "/" + q / g, f.getNumerator() + "/" + f.getDenominator(),
+ "value " + value + ", seed " + seed);
+ }
+ }
+
+ private static int gcd(int a, int b) {
+ return b == 0 ? a : gcd(b, a % b);
+ }
+}
diff --git a/poi/src/test/java/org/apache/poi/ss/usermodel/TestDataFormatter.java b/poi/src/test/java/org/apache/poi/ss/usermodel/TestDataFormatter.java
index 6de9ac274e..d8cac2692e 100644
--- a/poi/src/test/java/org/apache/poi/ss/usermodel/TestDataFormatter.java
+++ b/poi/src/test/java/org/apache/poi/ss/usermodel/TestDataFormatter.java
@@ -464,6 +464,32 @@ class TestDataFormatter {
}
}
+ @Test
+ void testFractionWithTextAndLongDenominator() {
+ DataFormatter dfUS = new DataFormatter(Locale.US);
+
+ // text in a fraction goes to CellFormat, where more than 9 denominator digits overflowed and the format threw
+ assertEquals("3: 1/4", dfUS.formatRawCellContents(3.25, -1, "#\\:??????????/??????????"));
+ }
+
+ @Test
+ void testFractionWithTextFallsBackForValueTooLarge() {
+ DataFormatter dfUS = new DataFormatter(Locale.US);
+
+ // CellFormat cannot show an improper fraction this large, so FractionFormat does, without the text
+ assertEquals("13=/=4", dfUS.formatRawCellContents(3.25, -1, "??\\=/=??"));
+ assertEquals("6000000001/2", dfUS.formatRawCellContents(3e9 + 0.5, -1, "??\\=/=??"));
+ }
+
+ @Test
+ void testFractionWithTextFirstValueTooLarge() {
+ DataFormatter dfUS = new DataFormatter(Locale.US);
+
+ // a first value too large to show must not decide the cached format for later values
+ assertEquals("6000000001/2", dfUS.formatRawCellContents(3e9 + 0.5, -1, "??\\=/=??"));
+ assertEquals("13=/=4", dfUS.formatRawCellContents(3.25, -1, "??\\=/=??"));
+ }
+
/**
* Test that _x (blank with the space taken by "x")
* and *x (fill to the column width with "x"s) are
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@poi.apache.org
For additional commands, e-mail: commits-help@poi.apache.org