Bugzilla 48195 - short-circuit evaluation of IF() and CHOOSE()

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

2 files changed, 70 insertions, 7 deletions

diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestFormulaEvaluatorBugs.java b/src/testcases/org/apache/poi/hssf/usermodel/TestFormulaEvaluatorBugs.java
index d94cfd9a96..939634b629 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestFormulaEvaluatorBugs.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestFormulaEvaluatorBugs.java
@@ -31,6 +31,7 @@ import org.apache.poi.hssf.record.aggregates.FormulaRecordAggregate;
 import org.apache.poi.hssf.record.formula.AreaPtg;
 import org.apache.poi.hssf.record.formula.FuncVarPtg;
 import org.apache.poi.hssf.record.formula.Ptg;
+import org.apache.poi.hssf.record.formula.eval.ErrorEval;
 import org.apache.poi.hssf.record.formula.eval.ValueEval;
 import org.apache.poi.ss.formula.EvaluationCell;
 import org.apache.poi.ss.formula.EvaluationListener;
@@ -319,6 +320,10 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 	 * The HSSFFormula evaluator performance benefits greatly from caching of intermediate cell values
 	 */
 	public void testSlowEvaluate45376() {
+		/*
+		 * Note - to observe behaviour without caching, disable the call to
+		 * updateValue() from FormulaCellCacheEntry.updateFormulaResult().
+		 */
 
 		// Firstly set up a sequence of formula cells where each depends on the  previous multiple
 		// times.  Without caching, each subsequent cell take about 4 times longer to evaluate.
@@ -330,30 +335,39 @@ public final class TestFormulaEvaluatorBugs extends TestCase {
 			char prevCol = (char) ('A' + i-1);
 			String prevCell = prevCol + "1";
 			// this formula is inspired by the offending formula of the attachment for bug 45376
+			// IF(DATE(YEAR(A1),MONTH(A1)+1,1)<=$D$3,DATE(YEAR(A1),MONTH(A1)+1,1),NA()) etc
 			String formula = "IF(DATE(YEAR(" + prevCell + "),MONTH(" + prevCell + ")+1,1)<=$D$3," +
 					"DATE(YEAR(" + prevCell + "),MONTH(" + prevCell + ")+1,1),NA())";
 			cell.setCellFormula(formula);
-
 		}
 		Calendar cal = new GregorianCalendar(2000, 0, 1, 0, 0, 0);
 		row.createCell(0).setCellValue(cal);
 
-		// Choose cell A9, so that the failing test case doesn't take too long to execute.
+		// Choose cell A9 instead of A10, so that the failing test case doesn't take too long to execute.
 		HSSFCell cell = row.getCell(8);
 		EvalListener evalListener = new EvalListener();
 		WorkbookEvaluator evaluator = WorkbookEvaluatorTestHelper.createEvaluator(wb, evalListener);
-		evaluator.evaluate(HSSFEvaluationTestHelper.wrapCell(cell));
+		ValueEval ve = evaluator.evaluate(HSSFEvaluationTestHelper.wrapCell(cell));
 		int evalCount = evalListener.getCountCacheMisses();
 		if (evalCount > 10) {
 			// Without caching, evaluating cell 'A9' takes 21845 evaluations which consumes
 			// much time (~3 sec on Core 2 Duo 2.2GHz)
+			// short-circuit-if optimisation cuts this down to 255 evaluations which is still too high
 			System.err.println("Cell A9 took " + evalCount + " intermediate evaluations");
 			throw new AssertionFailedError("Identifed bug 45376 - Formula evaluator should cache values");
 		}
-		// With caching, the evaluationCount is 8 which is a big improvement
-		// Note - these expected values may change if the WorkbookEvaluator is
-		// ever optimised to short circuit 'if' functions.
+		// With caching, the evaluationCount is 8 which is exactly the
+		// number of formula cells that needed to be evaluated.
 		assertEquals(8, evalCount);
-		assertEquals(24, evalListener.getCountCacheHits());
+
+		// The cache hits would be 24 if fully evaluating all arguments of the
+		// "IF()" functions (Each of the 8 formulas has 4 refs to formula cells
+		// which result in 1 cache miss and 3 cache hits). However with the
+		// short-circuit-if optimisation, 2 of the cell refs get skipped
+		// reducing this metric 8.
+		assertEquals(8, evalListener.getCountCacheHits());
+
+		// confirm the evaluation result too
+		assertEquals(ErrorEval.NA, ve);
 	}
 }
diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFFormulaEvaluator.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFFormulaEvaluator.java
index 082f07a84e..02e383f33a 100644
--- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFFormulaEvaluator.java
+++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFFormulaEvaluator.java
@@ -22,6 +22,13 @@ import junit.framework.TestCase;
 
 import org.apache.poi.hssf.HSSFTestDataSamples;
 import org.apache.poi.hssf.record.NameRecord;
+import org.apache.poi.hssf.record.formula.Ptg;
+import org.apache.poi.hssf.record.formula.eval.NumberEval;
+import org.apache.poi.hssf.record.formula.eval.ValueEval;
+import org.apache.poi.ss.formula.EvaluationCell;
+import org.apache.poi.ss.formula.EvaluationListener;
+import org.apache.poi.ss.formula.WorkbookEvaluator;
+import org.apache.poi.ss.formula.WorkbookEvaluatorTestHelper;
 import org.apache.poi.ss.usermodel.Cell;
 import org.apache.poi.ss.usermodel.CellValue;
 /**
@@ -167,4 +174,46 @@ public final class TestHSSFFormulaEvaluator extends TestCase {
 		assertEquals(Cell.CELL_TYPE_NUMERIC, value.getCellType());
 		assertEquals(5.33, value.getNumberValue(), 0.0);
 	}
+	private static final class EvalCountListener extends EvaluationListener {
+		private int _evalCount;
+		public EvalCountListener() {
+			_evalCount = 0;
+		}
+		public void onStartEvaluate(EvaluationCell cell, ICacheEntry entry, Ptg[] ptgs) {
+			_evalCount++;
+		}
+		public int getEvalCount() {
+			return _evalCount;
+		}
+	}
+
+	/**
+	 * The HSSFFormula evaluator performance benefits greatly from caching of intermediate cell values
+	 */
+	public void testShortCircuitIfEvaluation() {
+
+		// Set up a simple IF() formula that has measurable evaluation cost for its operands.
+		HSSFWorkbook wb = new HSSFWorkbook();
+		HSSFSheet sheet = wb.createSheet("Sheet1");
+		HSSFRow row = sheet.createRow(0);
+		HSSFCell cellA1 = row.createCell(0);
+		cellA1.setCellFormula("if(B1,C1,D1+E1+F1)");
+		// populate cells B1..F1 with simple formulas instead of plain values so we can use
+		// EvaluationListener to check which parts of the first formula get evaluated
+		for (int i=1; i<6; i++) {
+			// formulas are just literal constants "1".."5"
+			row.createCell(i).setCellFormula(String.valueOf(i));
+		}
+
+		EvalCountListener evalListener = new EvalCountListener();
+		WorkbookEvaluator evaluator = WorkbookEvaluatorTestHelper.createEvaluator(wb, evalListener);
+		ValueEval ve = evaluator.evaluate(HSSFEvaluationTestHelper.wrapCell(cellA1));
+		int evalCount = evalListener.getEvalCount();
+		if (evalCount == 6) {
+			// Without short-circuit-if evaluation, evaluating cell 'A1' takes 3 extra evaluations (for D1,E1,F1)
+			throw new AssertionFailedError("Identifed bug 48195 - Formula evaluator should short-circuit IF() calculations.");
+		}
+		assertEquals(3, evalCount);
+		assertEquals(2.0, ((NumberEval)ve).getNumberValue(), 0D);
+	}
 }