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

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() +
+				"]";
 	}
 }

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);
 	}
 }

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);
+    }
 }

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));
+    }
 }