Bug 59736: Incorrect evaluation of SUBTOTAL with composite interval

git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1754674 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/src/java/org/apache/poi/ss/formula/LazyRefEval.java b/src/java/org/apache/poi/ss/formula/LazyRefEval.java
index e0b5e19f93d8d97a8b05c0036e0197367ff222a3..c71b0e97792a4ad6a644c3d692da79d1e07852fa 100644 (file)
--- a/src/java/org/apache/poi/ss/formula/LazyRefEval.java
+++ b/src/java/org/apache/poi/ss/formula/LazyRefEval.java
@@ -27,7 +27,7 @@ import org.apache.poi.ss.util.CellReference;
 /**
  * Provides Lazy Evaluation to a 3D Reference
  */
-final class LazyRefEval extends RefEvalBase {
+public final class LazyRefEval extends RefEvalBase {
        private final SheetRangeEvaluator _evaluator;
 
        public LazyRefEval(int rowIndex, int columnIndex, SheetRangeEvaluator sre) {
@@ -47,14 +47,17 @@ final class LazyRefEval extends RefEvalBase {
                return new LazyAreaEval(area, _evaluator);
        }
 
+       public boolean isSubTotal() {
+               SheetRefEvaluator sheetEvaluator = _evaluator.getSheetEvaluator(getFirstSheetIndex());
+               return sheetEvaluator.isSubTotal(getRow(), getColumn());
+       }
+
        public String toString() {
                CellReference cr = new CellReference(getRow(), getColumn());
-               StringBuffer sb = new StringBuffer();
-               sb.append(getClass().getName()).append("[");
-               sb.append(_evaluator.getSheetNameRange());
-               sb.append('!');
-               sb.append(cr.formatAsString());
-               sb.append("]");
-               return sb.toString();
+               return getClass().getName() + "[" +
+                               _evaluator.getSheetNameRange() +
+                               '!' +
+                               cr.formatAsString() +
+                               "]";
        }
 }

diff --git a/src/java/org/apache/poi/ss/formula/functions/Subtotal.java b/src/java/org/apache/poi/ss/formula/functions/Subtotal.java
index fca55c502e8cd4fb173e5958ea3a27229fa591d6..f3b24cd1c42095d95cde15d13743107a4d43f8a5 100644 (file)
--- a/src/java/org/apache/poi/ss/formula/functions/Subtotal.java
+++ b/src/java/org/apache/poi/ss/formula/functions/Subtotal.java
@@ -19,6 +19,7 @@ package org.apache.poi.ss.formula.functions;
 
 import static org.apache.poi.ss.formula.functions.AggregateFunction.subtotalInstance;
 
+import org.apache.poi.ss.formula.LazyRefEval;
 import org.apache.poi.ss.formula.eval.ErrorEval;
 import org.apache.poi.ss.formula.eval.EvaluationException;
 import org.apache.poi.ss.formula.eval.NotImplementedException;
@@ -26,6 +27,11 @@ import org.apache.poi.ss.formula.eval.NotImplementedFunctionException;
 import org.apache.poi.ss.formula.eval.OperandResolver;
 import org.apache.poi.ss.formula.eval.ValueEval;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Iterator;
+import java.util.List;
+
 /**
  * Implementation for the Excel function SUBTOTAL<p>
  *
@@ -61,7 +67,6 @@ import org.apache.poi.ss.formula.eval.ValueEval;
 public class Subtotal implements Function {
 
        private static Function findFunction(int functionCode) throws EvaluationException {
-               Function func;
         switch (functionCode) {
                        case 1: return subtotalInstance(AggregateFunction.AVERAGE);
                        case 2: return Count.subtotalInstance();
@@ -87,7 +92,7 @@ public class Subtotal implements Function {
                        return ErrorEval.VALUE_INVALID;
                }
 
-               Function innerFunc;
+               final Function innerFunc;
                try {
                        ValueEval ve = OperandResolver.getSingleValue(args[0], srcRowIndex, srcColumnIndex);
                        int functionCode = OperandResolver.coerceValueToInt(ve);
@@ -96,9 +101,24 @@ public class Subtotal implements Function {
                        return e.getErrorEval();
                }
 
-               ValueEval[] innerArgs = new ValueEval[nInnerArgs];
-               System.arraycopy(args, 1, innerArgs, 0, nInnerArgs);
+               // ignore the first arg, this is the function-type, we check for the length above
+               final List<ValueEval> list = new ArrayList<ValueEval>(Arrays.asList(args).subList(1, args.length));
+
+               Iterator<ValueEval> it = list.iterator();
+
+               // See https://support.office.com/en-us/article/SUBTOTAL-function-7b027003-f060-4ade-9040-e478765b9939
+               // "If there are other subtotals within ref1, ref2,... (or nested subtotals), these nested subtotals are ignored to avoid double counting."
+               // For array references it is handled in other evaluation steps, but we need to handle this here for references to subtotal-functions
+               while(it.hasNext()) {
+                       ValueEval eval = it.next();
+                       if(eval instanceof LazyRefEval) {
+                               LazyRefEval lazyRefEval = (LazyRefEval) eval;
+                               if(lazyRefEval.isSubTotal()) {
+                                       it.remove();
+                               }
+                       }
+               }
 
-               return innerFunc.evaluate(innerArgs, srcRowIndex, srcColumnIndex);
+               return innerFunc.evaluate(list.toArray(new ValueEval[list.size()]), srcRowIndex, srcColumnIndex);
        }
 }

diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java
index 566944d18db4583179247638b1d152ec0d60b39e..9885fd8863387141e478e563db7f1544a4d725a2 100644 (file)
--- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java
+++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java
@@ -154,7 +154,9 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator {
             evaluator.evaluate(cXSL_cell);
             fail("Without a fix for #56752, shouldn't be able to evaluate a " +
                  "reference to a non-provided linked workbook");
-        } catch(Exception e) {}
+        } catch(Exception e) {
+            // expected here
+        }
         
         // Setup the environment
         Map<String,FormulaEvaluator> evaluators = new HashMap<String, FormulaEvaluator>();
@@ -196,7 +198,9 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator {
         try {
             cXSLX_nw_cell.setCellFormula("[alt.xlsx]Sheet1!$A$1");
             fail("New workbook not linked, shouldn't be able to add");
-        } catch (Exception e) {}
+        } catch (Exception e) {
+            // expected here
+        }
         
         // Link and re-try
         Workbook alt = new XSSFWorkbook();
@@ -651,4 +655,20 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator {
     private Cell getCell(Sheet sheet, int rowNo, int column) {
         return sheet.getRow(rowNo).getCell(column);
     }
+
+    @Test
+    public void test59736() {
+        Workbook wb = XSSFTestDataSamples.openSampleWorkbook("59736.xlsx");
+        FormulaEvaluator evaluator = wb.getCreationHelper().createFormulaEvaluator();
+        Cell cell = wb.getSheetAt(0).getRow(0).getCell(0);
+        assertEquals(1, cell.getNumericCellValue(), 0.001);
+
+        cell = wb.getSheetAt(0).getRow(1).getCell(0);
+        CellValue value = evaluator.evaluate(cell);
+        assertEquals(1, value.getNumberValue(), 0.001);
+
+        cell = wb.getSheetAt(0).getRow(2).getCell(0);
+        value = evaluator.evaluate(cell);
+        assertEquals(1, value.getNumberValue(), 0.001);
+    }
 }

diff --git a/src/testcases/org/apache/poi/ss/formula/functions/TestSubtotal.java b/src/testcases/org/apache/poi/ss/formula/functions/TestSubtotal.java
index 7b63eaf799b38e984adc598f74d7a6f5ed8d274f..f2750b591c66e987930243d716ad77092e2e4ada 100644 (file)
--- a/src/testcases/org/apache/poi/ss/formula/functions/TestSubtotal.java
+++ b/src/testcases/org/apache/poi/ss/formula/functions/TestSubtotal.java
@@ -20,9 +20,8 @@ package org.apache.poi.ss.formula.functions;
 import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.hssf.usermodel.HSSFSheet;
 import org.apache.poi.hssf.usermodel.HSSFWorkbook;
-import org.apache.poi.ss.formula.eval.AreaEval;
-import org.apache.poi.ss.formula.eval.NumberEval;
-import org.apache.poi.ss.formula.eval.ValueEval;
+import org.apache.poi.ss.formula.FormulaParseException;
+import org.apache.poi.ss.formula.eval.*;
 
 import junit.framework.TestCase;
 import org.apache.poi.ss.usermodel.*;
@@ -75,7 +74,6 @@ public final class TestSubtotal extends TestCase {
        }
 
      public void testAvg(){
-
         Workbook wb = new HSSFWorkbook();
 
         FormulaEvaluator fe = wb.getCreationHelper().createFormulaEvaluator();
@@ -95,16 +93,18 @@ public final class TestSubtotal extends TestCase {
         a6.setCellFormula("SUBTOTAL(1,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(1,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(1,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(2.0, a3.getNumericCellValue());
         assertEquals(8.0, a6.getNumericCellValue());
         assertEquals(3.0, a7.getNumericCellValue());
+        assertEquals(3.0, a8.getNumericCellValue());
     }
 
     public void testSum(){
-
         Workbook wb = new HSSFWorkbook();
 
         FormulaEvaluator fe = wb.getCreationHelper().createFormulaEvaluator();
@@ -124,12 +124,15 @@ public final class TestSubtotal extends TestCase {
         a6.setCellFormula("SUBTOTAL(9,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(9,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(9,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(4.0, a3.getNumericCellValue());
         assertEquals(26.0, a6.getNumericCellValue());
         assertEquals(12.0, a7.getNumericCellValue());
+        assertEquals(12.0, a8.getNumericCellValue());
     }
 
     public void testCount(){
@@ -147,18 +150,21 @@ public final class TestSubtotal extends TestCase {
         a3.setCellFormula("SUBTOTAL(2,B2:B3)");
         Cell a4 = sh.createRow(4).createCell(1);
         a4.setCellValue("POI");                  // A4 is string and not counted
-        Cell a5 = sh.createRow(5).createCell(1); // A5 is blank and not counted
+        /*Cell a5 =*/ sh.createRow(5).createCell(1); // A5 is blank and not counted
 
         Cell a6 = sh.createRow(6).createCell(1);
         a6.setCellFormula("SUBTOTAL(2,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(2,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(2,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(2.0, a3.getNumericCellValue());
         assertEquals(6.0, a6.getNumericCellValue());
         assertEquals(2.0, a7.getNumericCellValue());
+        assertEquals(2.0, a8.getNumericCellValue());
     }
 
     public void testCounta(){
@@ -176,18 +182,21 @@ public final class TestSubtotal extends TestCase {
         a3.setCellFormula("SUBTOTAL(3,B2:B3)");
         Cell a4 = sh.createRow(4).createCell(1);
         a4.setCellValue("POI");                  // A4 is string and not counted
-        Cell a5 = sh.createRow(5).createCell(1); // A5 is blank and not counted
+        /*Cell a5 =*/ sh.createRow(5).createCell(1); // A5 is blank and not counted
 
         Cell a6 = sh.createRow(6).createCell(1);
         a6.setCellFormula("SUBTOTAL(3,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(3,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(3,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(2.0, a3.getNumericCellValue());
         assertEquals(8.0, a6.getNumericCellValue());
         assertEquals(3.0, a7.getNumericCellValue());
+        assertEquals(3.0, a8.getNumericCellValue());
     }
 
     public void testMax(){
@@ -211,12 +220,15 @@ public final class TestSubtotal extends TestCase {
         a6.setCellFormula("SUBTOTAL(4,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(4,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(4,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(3.0, a3.getNumericCellValue());
         assertEquals(16.0, a6.getNumericCellValue());
         assertEquals(7.0, a7.getNumericCellValue());
+        assertEquals(7.0, a8.getNumericCellValue());
     }
 
     public void testMin(){
@@ -240,12 +252,15 @@ public final class TestSubtotal extends TestCase {
         a6.setCellFormula("SUBTOTAL(5,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(5,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(5,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(1.0, a3.getNumericCellValue());
         assertEquals(4.0, a6.getNumericCellValue());
         assertEquals(1.0, a7.getNumericCellValue());
+        assertEquals(1.0, a8.getNumericCellValue());
     }
 
     public void testStdev(){
@@ -269,12 +284,15 @@ public final class TestSubtotal extends TestCase {
         a6.setCellFormula("SUBTOTAL(7,B2:B6)*2 + 2");
         Cell a7 = sh.createRow(7).createCell(1);
         a7.setCellFormula("SUBTOTAL(7,B2:B7)");
+        Cell a8 = sh.createRow(8).createCell(1);
+        a8.setCellFormula("SUBTOTAL(7,B2,B3,B4,B5,B6,B7,B8)");
 
         fe.evaluateAll();
 
         assertEquals(1.41421, a3.getNumericCellValue(), 0.0001);
         assertEquals(7.65685, a6.getNumericCellValue(), 0.0001);
         assertEquals(2.82842, a7.getNumericCellValue(), 0.0001);
+        assertEquals(2.82842, a8.getNumericCellValue(), 0.0001);
     }
 
     public void test50209(){
@@ -328,4 +346,69 @@ public final class TestSubtotal extends TestCase {
         confirmExpectedResult(evaluator, "SUBTOTAL(COUNT;B2:B8,C2:C8)", cellC2, 3.0);
         confirmExpectedResult(evaluator, "SUBTOTAL(COUNTA;B2:B8,C2:C8)", cellC3, 5.0);
     }
+
+    public void testUnimplemented(){
+        Workbook wb = new HSSFWorkbook();
+
+        FormulaEvaluator fe = wb.getCreationHelper().createFormulaEvaluator();
+
+        Sheet sh = wb.createSheet();
+        Cell a3 = sh.createRow(3).createCell(1);
+        a3.setCellFormula("SUBTOTAL(8,B2:B3)");
+
+        try {
+            fe.evaluateAll();
+            fail("Should catch an NotImplementedFunctionException here, adjust these tests if it was actually implemented");
+        } catch (NotImplementedException e) {
+            // expected here
+        }
+
+        a3.setCellFormula("SUBTOTAL(10,B2:B3)");
+
+        try {
+            fe.evaluateAll();
+            fail("Should catch an NotImplementedFunctionException here, adjust these tests if it was actually implemented");
+        } catch (NotImplementedException e) {
+            // expected here
+        }
+
+        a3.setCellFormula("SUBTOTAL(11,B2:B3)");
+
+        try {
+            fe.evaluateAll();
+            fail("Should catch an NotImplementedFunctionException here, adjust these tests if it was actually implemented");
+        } catch (NotImplementedException e) {
+            // expected here
+        }
+
+        a3.setCellFormula("SUBTOTAL(107,B2:B3)");
+
+        try {
+            fe.evaluateAll();
+            fail("Should catch an NotImplementedFunctionException here, adjust these tests if it was actually implemented");
+        } catch (NotImplementedException e) {
+            // expected here
+        }
+
+        a3.setCellFormula("SUBTOTAL(0,B2:B3)");
+        fe.evaluateAll();
+        assertEquals(FormulaError.VALUE.getCode(), a3.getErrorCellValue());
+
+        try {
+            a3.setCellFormula("SUBTOTAL(9)");
+            fail("Should catch an exception here");
+        } catch (FormulaParseException e) {
+            // expected here
+        }
+
+        try {
+            a3.setCellFormula("SUBTOTAL()");
+            fail("Should catch an exception here");
+        } catch (FormulaParseException e) {
+            // expected here
+        }
+
+        Subtotal subtotal = new Subtotal();
+        assertEquals(ErrorEval.VALUE_INVALID, subtotal.evaluate(new ValueEval[] {}, 0, 0));
+    }
 }

diff --git a/test-data/spreadsheet/59736.xlsx b/test-data/spreadsheet/59736.xlsx
new file mode 100644 (file)
index 0000000..1f29d54
Binary files /dev/null and b/test-data/spreadsheet/59736.xlsx differ