From 32d0c7213e54f8d6fb68a37834a992bcf998afa6 Mon Sep 17 00:00:00 2001 From: Josh Micich Date: Thu, 23 Jul 2009 23:12:17 +0000 Subject: [PATCH] Improvements to formula evaluation treatment of -0.0. (Refinements to fix for bug 47198 git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@797258 13f79535-47bb-0310-9956-ffa450edef68 --- .../hssf/record/formula/eval/PercentEval.java | 9 +- .../formula/eval/RelationalOperationEval.java | 7 +- .../eval/TwoOperandNumericOperation.java | 8 +- .../record/formula/eval/UnaryMinusEval.java | 5 +- .../poi/ss/util/NumberToTextConverter.java | 92 +++++------ .../formula/eval/AllFormulaEvalTests.java | 5 +- .../record/formula/eval/TestEqualEval.java | 11 +- .../formula/eval/TestMinusZeroResult.java | 148 ++++++++++++++++++ 8 files changed, 226 insertions(+), 59 deletions(-) create mode 100644 src/testcases/org/apache/poi/hssf/record/formula/eval/TestMinusZeroResult.java diff --git a/src/java/org/apache/poi/hssf/record/formula/eval/PercentEval.java b/src/java/org/apache/poi/hssf/record/formula/eval/PercentEval.java index a7c0cbad4..145874e6e 100755 --- a/src/java/org/apache/poi/hssf/record/formula/eval/PercentEval.java +++ b/src/java/org/apache/poi/hssf/record/formula/eval/PercentEval.java @@ -34,14 +34,17 @@ public final class PercentEval implements OperationEval { if (args.length != 1) { return ErrorEval.VALUE_INVALID; } - double d0; + double d; try { ValueEval ve = OperandResolver.getSingleValue(args[0], srcRow, srcCol); - d0 = OperandResolver.coerceValueToDouble(ve); + d = OperandResolver.coerceValueToDouble(ve); } catch (EvaluationException e) { return e.getErrorEval(); } - return new NumberEval(d0 / 100); + if (d == 0.0) { // this '==' matches +0.0 and -0.0 + return NumberEval.ZERO; + } + return new NumberEval(d / 100); } public int getNumberOfOperands() { diff --git a/src/java/org/apache/poi/hssf/record/formula/eval/RelationalOperationEval.java b/src/java/org/apache/poi/hssf/record/formula/eval/RelationalOperationEval.java index d5eef3e26..c678507aa 100644 --- a/src/java/org/apache/poi/hssf/record/formula/eval/RelationalOperationEval.java +++ b/src/java/org/apache/poi/hssf/record/formula/eval/RelationalOperationEval.java @@ -19,7 +19,7 @@ package org.apache.poi.hssf.record.formula.eval; /** * Base class for all comparison operator evaluators - * + * * @author Amol S. Deshmukh < amolweb at ya hoo dot com > */ public abstract class RelationalOperationEval implements OperationEval { @@ -108,10 +108,7 @@ public abstract class RelationalOperationEval implements OperationEval { if (vb instanceof NumberEval) { NumberEval nA = (NumberEval) va; NumberEval nB = (NumberEval) vb; - if (nA.getNumberValue() == nB.getNumberValue()) { - // Excel considers -0.0 == 0.0 which is different to Double.compare() - return 0; - } + // Excel considers -0.0 < 0.0 which is the same as Double.compare() return Double.compare(nA.getNumberValue(), nB.getNumberValue()); } } diff --git a/src/java/org/apache/poi/hssf/record/formula/eval/TwoOperandNumericOperation.java b/src/java/org/apache/poi/hssf/record/formula/eval/TwoOperandNumericOperation.java index 5e0a91cc6..672c5a7d7 100644 --- a/src/java/org/apache/poi/hssf/record/formula/eval/TwoOperandNumericOperation.java +++ b/src/java/org/apache/poi/hssf/record/formula/eval/TwoOperandNumericOperation.java @@ -26,13 +26,19 @@ abstract class TwoOperandNumericOperation implements OperationEval { ValueEval ve = OperandResolver.getSingleValue(arg, srcCellRow, srcCellCol); return OperandResolver.coerceValueToDouble(ve); } - + public final Eval evaluate(Eval[] args, int srcCellRow, short srcCellCol) { double result; try { double d0 = singleOperandEvaluate(args[0], srcCellRow, srcCellCol); double d1 = singleOperandEvaluate(args[1], srcCellRow, srcCellCol); result = evaluate(d0, d1); + if (result == 0.0) { // this '==' matches +0.0 and -0.0 + // Excel converts -0.0 to +0.0 for '*', '/', '%', '+' and '^' + if (!(this instanceof SubtractEval)) { + return NumberEval.ZERO; + } + } if (Double.isNaN(result) || Double.isInfinite(result)) { return ErrorEval.NUM_ERROR; } diff --git a/src/java/org/apache/poi/hssf/record/formula/eval/UnaryMinusEval.java b/src/java/org/apache/poi/hssf/record/formula/eval/UnaryMinusEval.java index 1354a78a3..98f31c529 100644 --- a/src/java/org/apache/poi/hssf/record/formula/eval/UnaryMinusEval.java +++ b/src/java/org/apache/poi/hssf/record/formula/eval/UnaryMinusEval.java @@ -20,7 +20,7 @@ package org.apache.poi.hssf.record.formula.eval; /** * @author Amol S. Deshmukh < amolweb at ya hoo dot com > - * + * */ public final class UnaryMinusEval implements OperationEval { @@ -41,6 +41,9 @@ public final class UnaryMinusEval implements OperationEval { } catch (EvaluationException e) { return e.getErrorEval(); } + if (d == 0.0) { // this '==' matches +0.0 and -0.0 + return NumberEval.ZERO; + } return new NumberEval(-d); } diff --git a/src/java/org/apache/poi/ss/util/NumberToTextConverter.java b/src/java/org/apache/poi/ss/util/NumberToTextConverter.java index 1d7a901d2..efcb012fc 100644 --- a/src/java/org/apache/poi/ss/util/NumberToTextConverter.java +++ b/src/java/org/apache/poi/ss/util/NumberToTextConverter.java @@ -26,16 +26,16 @@ import java.math.BigInteger; * * IEEE 64-bit Double Rendering Comparison - * + * * * - * + * * * * @@ -96,8 +96,8 @@ import java.math.BigInteger; * * *
Raw bitsJavaExcel
0x0000000000000000L0.00
0x3FF0000000000000L1.01
0x3FF00068DB8BAC71L1.00011.0001
0x7FFFFFFFFFFFFFFFLNaN3.5953862697246E+308
0xFFF7FFFFFFFFFFFFLNaN2.6965397022935E+308
- * - * Note: + * + * Note: * Excel has inconsistent rules for the following numeric operations: * - * Excel's text to number conversion is not a true inverse of this operation. The + * Excel's text to number conversion is not a true inverse of this operation. The * allowable ranges are different. Some numbers that don't correctly convert to text actually * do get handled properly when used in arithmetic evaluations. - * + * * @author Josh Micich */ public final class NumberToTextConverter { @@ -119,49 +119,49 @@ public final class NumberToTextConverter { private static final int FRAC_BITS_WIDTH = EXPONENT_SHIFT; private static final int EXPONENT_BIAS = 1023; private static final long FRAC_ASSUMED_HIGH_BIT = ( 1L<value to the text representation that Excel would give if + * Converts the supplied value to the text representation that Excel would give if * the value were to appear in an unformatted cell, or as a literal number in a formula.
* Note - the results from this method differ slightly from those of Double.toString() * In some special cases Excel behaves quite differently. This function attempts to reproduce - * those results. + * those results. */ public static String toText(double value) { return rawDoubleBitsToText(Double.doubleToLongBits(value)); } /* package */ static String rawDoubleBitsToText(long pRawBits) { - + long rawBits = pRawBits; boolean isNegative = rawBits < 0; // sign bit is in the same place for long and double if (isNegative) { rawBits &= 0x7FFFFFFFFFFFFFFFL; } - + int biasedExponent = (int) ((rawBits & expMask) >> EXPONENT_SHIFT); if (biasedExponent == 0) { // value is 'denormalised' which means it is less than 2^-1022 // excel displays all these numbers as zero, even though calculations work OK - return "0"; + return isNegative ? "-0" : "0"; } - - int exponent = biasedExponent - EXPONENT_BIAS; - + + int exponent = biasedExponent - EXPONENT_BIAS; + long fracBits = FRAC_ASSUMED_HIGH_BIT | rawBits & FRAC_MASK; - - + + // Start by converting double value to BigDecimal BigDecimal bd; if (biasedExponent == 0x07FF) { @@ -175,26 +175,26 @@ public final class NumberToTextConverter { isNegative = false; // except that the sign bit is ignored } bd = convertToBigDecimal(exponent, fracBits); - + return formatBigInteger(isNegative, bd.unscaledValue(), bd.scale()); } private static BigDecimal convertToBigDecimal(int exponent, long fracBits) { byte[] joob = { - (byte) (fracBits >> 48), - (byte) (fracBits >> 40), - (byte) (fracBits >> 32), - (byte) (fracBits >> 24), - (byte) (fracBits >> 16), - (byte) (fracBits >> 8), - (byte) (fracBits >> 0), + (byte) (fracBits >> 48), + (byte) (fracBits >> 40), + (byte) (fracBits >> 32), + (byte) (fracBits >> 24), + (byte) (fracBits >> 16), + (byte) (fracBits >> 8), + (byte) (fracBits >> 0), }; - + BigInteger bigInt = new BigInteger(joob); int lastSigBitIndex = exponent-FRAC_BITS_WIDTH; if(lastSigBitIndex < 0) { BigInteger shifto = new BigInteger("1").shiftLeft(-lastSigBitIndex); - int scale = 1 -(int) (lastSigBitIndex/LOG2_10); + int scale = 1 -(int) (lastSigBitIndex/LOG2_10); BigDecimal bd1 = new BigDecimal(bigInt); BigDecimal bdShifto = new BigDecimal(shifto); return bd1.divide(bdShifto, scale, BigDecimal.ROUND_HALF_UP); @@ -208,10 +208,10 @@ public final class NumberToTextConverter { if (scale < 0) { throw new RuntimeException("negative scale"); } - + StringBuffer sb = new StringBuffer(unscaledValue.toString()); int numberOfLeadingZeros = -1; - + int unscaledLength = sb.length(); if (scale > 0 && scale >= unscaledLength) { // less than one @@ -226,7 +226,7 @@ public final class NumberToTextConverter { } return sb.toString(); } - + private static int getNumberOfSignificantFiguresDisplayed(int exponent) { int nLostDigits; // number of significand digits lost due big exponents if(exponent > 99) { @@ -241,19 +241,19 @@ public final class NumberToTextConverter { } return DEFAULT_COUNT_SIGNIFICANT_DIGITS - nLostDigits; } - + private static boolean needsScientificNotation(int nDigits) { return nDigits > MAX_TEXT_LEN; } private static void formatGreaterThanOne(StringBuffer sb, int nIntegerDigits) { - + int maxSigFigs = getNumberOfSignificantFiguresDisplayed(nIntegerDigits); int decimalPointIndex = nIntegerDigits; boolean roundCausedCarry = performRound(sb, 0, maxSigFigs); int endIx = Math.min(maxSigFigs, sb.length()-1); - + int nSigFigures; if(roundCausedCarry) { sb.insert(0, '1'); @@ -292,11 +292,11 @@ public final class NumberToTextConverter { if (pAbsExponent < 1) { throw new IllegalArgumentException("abs(exponent) must be positive"); } - + int numberOfLeadingZeros = pAbsExponent-1; - int absExponent = pAbsExponent; - int maxSigFigs = getNumberOfSignificantFiguresDisplayed(-absExponent); - + int absExponent = pAbsExponent; + int maxSigFigs = getNumberOfSignificantFiguresDisplayed(-absExponent); + boolean roundCausedCarry = performRound(sb, 0, maxSigFigs); int nRemainingSigFigs; if(roundCausedCarry) { @@ -309,9 +309,9 @@ public final class NumberToTextConverter { nRemainingSigFigs = countSignifantDigits(sb, 0 + maxSigFigs); sb.setLength(nRemainingSigFigs); } - + int normalLength = 2 + numberOfLeadingZeros + nRemainingSigFigs; // 2 == "0.".length() - + if (needsScientificNotation(normalLength)) { if (sb.length()>1) { sb.insert(1, '.'); @@ -319,7 +319,7 @@ public final class NumberToTextConverter { sb.append('E'); sb.append('-'); appendExp(sb, absExponent); - } else { + } else { sb.insert(0, "0."); for(int i=numberOfLeadingZeros; i>0; i--) { sb.insert(2, '0'); @@ -345,7 +345,7 @@ public final class NumberToTextConverter { return; } sb.append(val); - + } @@ -391,12 +391,12 @@ public final class NumberToTextConverter { while(sb.charAt(changeDigitIx) == '9') { sb.setCharAt(changeDigitIx, '0'); changeDigitIx--; - // All nines, rounded up. Notify caller + // All nines, rounded up. Notify caller if(changeDigitIx < 0) { return true; } } - // no more '9's to round up. + // no more '9's to round up. // Last digit to be changed is still inside sb char prevDigit = sb.charAt(changeDigitIx); sb.setCharAt(changeDigitIx, (char) (prevDigit + 1)); diff --git a/src/testcases/org/apache/poi/hssf/record/formula/eval/AllFormulaEvalTests.java b/src/testcases/org/apache/poi/hssf/record/formula/eval/AllFormulaEvalTests.java index a26539cdd..325e2e919 100755 --- a/src/testcases/org/apache/poi/hssf/record/formula/eval/AllFormulaEvalTests.java +++ b/src/testcases/org/apache/poi/hssf/record/formula/eval/AllFormulaEvalTests.java @@ -22,11 +22,11 @@ import junit.framework.TestSuite; /** * Collects all tests the package org.apache.poi.hssf.record.formula.eval. - * + * * @author Josh Micich */ public class AllFormulaEvalTests { - + public static Test suite() { TestSuite result = new TestSuite(AllFormulaEvalTests.class.getName()); result.addTestSuite(TestAreaEval.class); @@ -36,6 +36,7 @@ public class AllFormulaEvalTests { result.addTestSuite(TestExternalFunction.class); result.addTestSuite(TestFormulaBugs.class); result.addTestSuite(TestFormulasFromSpreadsheet.class); + result.addTestSuite(TestMinusZeroResult.class); result.addTestSuite(TestMissingArgEval.class); result.addTestSuite(TestPercentEval.class); result.addTestSuite(TestRangeEval.class); diff --git a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestEqualEval.java b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestEqualEval.java index 3053660b7..19d1fa1fb 100644 --- a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestEqualEval.java +++ b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestEqualEval.java @@ -93,12 +93,21 @@ public final class TestEqualEval extends TestCase { } /** - * Excel considers -0.0 to be equal to 0.0 + * Bug 47198 involved a formula "-A1=0" where cell A1 was 0.0. + * Excel evaluates "-A1=0" to TRUE, not because it thinks -0.0==0.0 + * but because "-A1" evaluated to +0.0 + *

+ * Note - the original diagnosis of bug 47198 was that + * "Excel considers -0.0 to be equal to 0.0" which is NQR + * See {@link TestMinusZeroResult} for more specific tests regarding -0.0. */ public void testZeroEquality_bug47198() { NumberEval zero = new NumberEval(0.0); NumberEval mZero = (NumberEval) UnaryMinusEval.instance.evaluate(new Eval[] { zero, }, 0, (short) 0); + if (Double.doubleToLongBits(mZero.getNumberValue()) == 0x8000000000000000L) { + throw new AssertionFailedError("Identified bug 47198: unary minus should convert -0.0 to 0.0"); + } Eval[] args = { zero, mZero, }; BoolEval result = (BoolEval) EqualEval.instance.evaluate(args, 0, (short) 0); if (!result.getBooleanValue()) { diff --git a/src/testcases/org/apache/poi/hssf/record/formula/eval/TestMinusZeroResult.java b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestMinusZeroResult.java new file mode 100644 index 000000000..b68b1f74d --- /dev/null +++ b/src/testcases/org/apache/poi/hssf/record/formula/eval/TestMinusZeroResult.java @@ -0,0 +1,148 @@ +/* ==================================================================== + 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.hssf.record.formula.eval; + +import junit.framework.ComparisonFailure; +import junit.framework.TestCase; + +import org.apache.poi.util.HexDump; + +/** + * IEEE 754 defines a quantity '-0.0' which is distinct from '0.0'. + * Negative zero is not easy to observe in Excel, since it is usually converted to 0.0. + * (Note - the results of XLL add-in functions don't seem to be converted, so they are one + * reliable avenue to observe Excel's treatment of '-0.0' as an operand.) + *

+ * POI attempts to emulate Excel faithfully, so this class tests + * two aspects of '-0.0' in formula evaluation: + *

    + *
  1. For most operation results '-0.0' is converted to '0.0'.
    2. + *
  2. Comparison operators have slightly different rules regarding '-0.0'.
    3. + *
+ * @author Josh Micich + */ +public final class TestMinusZeroResult extends TestCase { + private static final double MINUS_ZERO = -0.0; + + + public void testSimpleOperators() { + + // unary plus is a no-op + checkEval(MINUS_ZERO, UnaryPlusEval.instance, MINUS_ZERO); + + // most simple operators convert -0.0 to +0.0 + checkEval(0.0, UnaryMinusEval.instance, 0.0); + checkEval(0.0, PercentEval.instance, MINUS_ZERO); + checkEval(0.0, MultiplyEval.instance, MINUS_ZERO, 1.0); + checkEval(0.0, DivideEval.instance, MINUS_ZERO, 1.0); + checkEval(0.0, PowerEval.instance, MINUS_ZERO, 1.0); + + // but SubtractEval does not convert -0.0, so '-' and '+' work like java + checkEval(MINUS_ZERO, SubtractEval.instance, MINUS_ZERO, 0.0); // this is the main point of bug 47198 + checkEval(0.0, AddEval.instance, MINUS_ZERO, 0.0); + } + + /** + * These results are hard to see in Excel (since -0.0 is usually converted to +0.0 before it + * gets to the comparison operator) + */ + public void testComparisonOperators() { + checkEval(false, EqualEval.instance, 0.0, MINUS_ZERO); + checkEval(true, GreaterThanEval.instance, 0.0, MINUS_ZERO); + checkEval(true, LessThanEval.instance, MINUS_ZERO, 0.0); + } + + public void testTextRendering() { + confirmTextRendering("-0", MINUS_ZERO); + // sub-normal negative numbers also display as '-0' + confirmTextRendering("-0", Double.longBitsToDouble(0x8000100020003000L)); + } + + /** + * Uses {@link ConcatEval} to force number-to-text conversion + */ + private static void confirmTextRendering(String expRendering, double d) { + Eval[] args = { StringEval.EMPTY_INSTANCE, new NumberEval(d), }; + StringEval se = (StringEval) ConcatEval.instance.evaluate(args, -1, (short)-1); + String result = se.getStringValue(); + assertEquals(expRendering, result); + } + + private static void checkEval(double expectedResult, OperationEval instance, double... dArgs) { + NumberEval result = (NumberEval) evaluate(instance, dArgs); + assertDouble(expectedResult, result.getNumberValue()); + } + private static void checkEval(boolean expectedResult, OperationEval instance, double... dArgs) { + BoolEval result = (BoolEval) evaluate(instance, dArgs); + assertEquals(expectedResult, result.getBooleanValue()); + } + private static Eval evaluate(OperationEval instance, double... dArgs) { + Eval[] evalArgs; + evalArgs = new Eval[dArgs.length]; + for (int i = 0; i < evalArgs.length; i++) { + evalArgs[i] = new NumberEval(dArgs[i]); + } + Eval r = instance.evaluate(evalArgs, -1, (short)-1); + return r; + } + + /** + * Not really a POI test - just shows similar behaviour of '-0.0' in Java. + */ + public void testJava() { + + assertEquals(0x8000000000000000L, Double.doubleToLongBits(MINUS_ZERO)); + + // The simple operators consider all zeros to be the same + assertTrue(MINUS_ZERO == MINUS_ZERO); + assertTrue(MINUS_ZERO == +0.0); + assertFalse(MINUS_ZERO < +0.0); + + // Double.compare() considers them different + assertTrue(Double.compare(MINUS_ZERO, +0.0) < 0); + + // multiplying zero by any negative quantity yields minus zero + assertDouble(MINUS_ZERO, 0.0*-1); + assertDouble(MINUS_ZERO, 0.0*-1e300); + assertDouble(MINUS_ZERO, 0.0*-1e-300); + + // minus zero can be produced as a result of underflow + assertDouble(MINUS_ZERO, -1e-300 / 1e100); + + // multiplying or dividing minus zero by a positive quantity yields minus zero + assertDouble(MINUS_ZERO, MINUS_ZERO * 1.0); + assertDouble(MINUS_ZERO, MINUS_ZERO / 1.0); + + // subtracting positive zero gives minus zero + assertDouble(MINUS_ZERO, MINUS_ZERO - 0.0); + // BUT adding positive zero gives positive zero + assertDouble(0.0, MINUS_ZERO + 0.0); // <<---- + } + + /** + * Just so there is no ambiguity. The two double values have to be exactly equal + */ + private static void assertDouble(double a, double b) { + long bitsA = Double.doubleToLongBits(a); + long bitsB = Double.doubleToLongBits(b); + if (bitsA != bitsB) { + throw new ComparisonFailure("value different to expected", + new String(HexDump.longToHex(bitsA)), new String(HexDump.longToHex(bitsB))); + } + } +}