diff --git a/src/documentation/content/xdocs/changes.xml b/src/documentation/content/xdocs/changes.xml index 0dc4e7708..3528da02e 100644 --- a/src/documentation/content/xdocs/changes.xml +++ b/src/documentation/content/xdocs/changes.xml @@ -37,7 +37,7 @@ - YK: remove me. required to keep Forrest DTD compiler quiet + 46053 - fixed evaluation cache dependency analysis when changing blank cells 45866 - allowed for change of unicode compression across Continue records diff --git a/src/documentation/content/xdocs/status.xml b/src/documentation/content/xdocs/status.xml index a7624486d..09b2af426 100644 --- a/src/documentation/content/xdocs/status.xml +++ b/src/documentation/content/xdocs/status.xml @@ -34,7 +34,7 @@ - YK: remove me. required to keep Forrest DTD compiler quiet + 46053 - fixed evaluation cache dependency analysis when changing blank cells 45866 - allowed for change of unicode compression across Continue records diff --git a/src/java/org/apache/poi/ss/formula/EvaluationCache.java b/src/java/org/apache/poi/ss/formula/EvaluationCache.java index 81745825c..1681cc7eb 100644 --- a/src/java/org/apache/poi/ss/formula/EvaluationCache.java +++ b/src/java/org/apache/poi/ss/formula/EvaluationCache.java @@ -52,16 +52,21 @@ final class EvaluationCache { public void notifyUpdateCell(int bookIndex, int sheetIndex, EvaluationCell cell) { FormulaCellCacheEntry fcce = _formulaCellCache.get(cell); - + Loc loc = new Loc(bookIndex, sheetIndex, cell.getRowIndex(), cell.getColumnIndex()); PlainValueCellCacheEntry pcce = _plainCellCache.get(loc); if (cell.getCellType() == HSSFCell.CELL_TYPE_FORMULA) { if (fcce == null) { - if (pcce == null) { - updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, cell.getRowIndex(), cell.getColumnIndex()); - } fcce = new FormulaCellCacheEntry(); + if (pcce == null) { + if (_evaluationListener != null) { + _evaluationListener.onChangeFromBlankValue(sheetIndex, cell.getRowIndex(), + cell.getColumnIndex(), cell, fcce); + } + updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, cell.getRowIndex(), + cell.getColumnIndex()); + } _formulaCellCache.put(cell, fcce); } else { fcce.recurseClearCachedFormulaResults(_evaluationListener); @@ -77,18 +82,28 @@ final class EvaluationCache { } else { ValueEval value = WorkbookEvaluator.getValueFromNonFormulaCell(cell); if (pcce == null) { - if (fcce == null) { - updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, cell.getRowIndex(), cell.getColumnIndex()); - } - pcce = new PlainValueCellCacheEntry(value); - _plainCellCache.put(loc, pcce); - if (_evaluationListener != null) { - _evaluationListener.onReadPlainValue(sheetIndex, cell.getRowIndex(), cell.getColumnIndex(), pcce); + if (value != BlankEval.INSTANCE) { + // only cache non-blank values in the plain cell cache + // (dependencies on blank cells are managed by + // FormulaCellCacheEntry._usedBlankCellGroup) + pcce = new PlainValueCellCacheEntry(value); + if (fcce == null) { + if (_evaluationListener != null) { + _evaluationListener.onChangeFromBlankValue(sheetIndex, cell + .getRowIndex(), cell.getColumnIndex(), cell, pcce); + } + updateAnyBlankReferencingFormulas(bookIndex, sheetIndex, + cell.getRowIndex(), cell.getColumnIndex()); + } + _plainCellCache.put(loc, pcce); } } else { - if(pcce.updateValue(value)) { + if (pcce.updateValue(value)) { pcce.recurseClearCachedFormulaResults(_evaluationListener); } + if (value == BlankEval.INSTANCE) { + _plainCellCache.remove(loc); + } } if (fcce == null) { // was plain cell before - no change of type diff --git a/src/java/org/apache/poi/ss/formula/IEvaluationListener.java b/src/java/org/apache/poi/ss/formula/IEvaluationListener.java index caf254edf..ed3fe442b 100644 --- a/src/java/org/apache/poi/ss/formula/IEvaluationListener.java +++ b/src/java/org/apache/poi/ss/formula/IEvaluationListener.java @@ -19,7 +19,6 @@ package org.apache.poi.ss.formula; import org.apache.poi.hssf.record.formula.Ptg; import org.apache.poi.hssf.record.formula.eval.ValueEval; -import org.apache.poi.hssf.usermodel.HSSFCell; /** * Tests can implement this class to track the internal working of the {@link WorkbookEvaluator}.
@@ -51,4 +50,6 @@ interface IEvaluationListener { */ void sortDependentCachedValues(ICacheEntry[] formulaCells); void onClearDependentCachedValue(ICacheEntry formulaCell, int depth); + void onChangeFromBlankValue(int sheetIndex, int rowIndex, int columnIndex, + EvaluationCell cell, ICacheEntry entry); } diff --git a/src/testcases/org/apache/poi/ss/formula/EvaluationListener.java b/src/testcases/org/apache/poi/ss/formula/EvaluationListener.java index 46bb06172..1158c73b2 100644 --- a/src/testcases/org/apache/poi/ss/formula/EvaluationListener.java +++ b/src/testcases/org/apache/poi/ss/formula/EvaluationListener.java @@ -19,7 +19,6 @@ package org.apache.poi.ss.formula; import org.apache.poi.hssf.record.formula.Ptg; import org.apache.poi.hssf.record.formula.eval.ValueEval; -import org.apache.poi.hssf.usermodel.HSSFCell; /** * Tests should extend this class if they need to track the internal working of the {@link WorkbookEvaluator}.
@@ -47,6 +46,10 @@ public abstract class EvaluationListener implements IEvaluationListener { public void onClearCachedValue(ICacheEntry entry) { // do nothing } + public void onChangeFromBlankValue(int sheetIndex, int rowIndex, int columnIndex, + EvaluationCell cell, ICacheEntry entry) { + // do nothing + } public void sortDependentCachedValues(ICacheEntry[] entries) { // do nothing } diff --git a/src/testcases/org/apache/poi/ss/formula/TestEvaluationCache.java b/src/testcases/org/apache/poi/ss/formula/TestEvaluationCache.java index 9c95508ad..59cdb458f 100644 --- a/src/testcases/org/apache/poi/ss/formula/TestEvaluationCache.java +++ b/src/testcases/org/apache/poi/ss/formula/TestEvaluationCache.java @@ -38,9 +38,11 @@ import org.apache.poi.hssf.record.formula.eval.StringEval; import org.apache.poi.hssf.record.formula.eval.ValueEval; import org.apache.poi.hssf.usermodel.HSSFCell; import org.apache.poi.hssf.usermodel.HSSFEvaluationTestHelper; +import org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator; import org.apache.poi.hssf.usermodel.HSSFRow; import org.apache.poi.hssf.usermodel.HSSFSheet; import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.hssf.usermodel.HSSFFormulaEvaluator.CellValue; import org.apache.poi.hssf.util.CellReference; import org.apache.poi.ss.formula.PlainCellCache.Loc; @@ -134,7 +136,19 @@ public class TestEvaluationCache extends TestCase { } public void onClearDependentCachedValue(ICacheEntry entry, int depth) { EvaluationCell cell = (EvaluationCell) _formulaCellsByCacheEntry.get(entry); - log("clear" + depth, cell.getRowIndex(), cell.getColumnIndex(), entry.getValue()); + log("clear" + depth, cell.getRowIndex(), cell.getColumnIndex(), entry.getValue()); + } + + public void onChangeFromBlankValue(int sheetIndex, int rowIndex, int columnIndex, + EvaluationCell cell, ICacheEntry entry) { + log("changeFromBlank", rowIndex, columnIndex, entry.getValue()); + if (entry.getValue() == null) { // hack to tell the difference between formula and plain value + // perhaps the API could be improved: onChangeFromBlankToValue, onChangeFromBlankToFormula + _formulaCellsByCacheEntry.put(entry, cell); + } else { + Loc loc = new Loc(0, sheetIndex, rowIndex, columnIndex); + _plainCellLocsByCacheEntry.put(entry, loc); + } } private void log(String tag, int rowIndex, int columnIndex, Object value) { StringBuffer sb = new StringBuffer(64); @@ -213,6 +227,11 @@ public class TestEvaluationCache extends TestCase { cell.setCellValue(value); _evaluator.notifyUpdateCell(wrapCell(cell)); } + public void clearCell(String cellRefText) { + HSSFCell cell = getOrCreateCell(cellRefText); + cell.setCellType(HSSFCell.CELL_TYPE_BLANK); + _evaluator.notifyUpdateCell(wrapCell(cell)); + } public void setCellFormula(String cellRefText, String formulaText) { HSSFCell cell = getOrCreateCell(cellRefText); @@ -555,6 +574,75 @@ public class TestEvaluationCache extends TestCase { }); } + /** + * Make sure that when blank cells are changed to value/formula cells, any dependent formulas + * have their cached results cleared. + */ + public void testBlankCellChangedToValueCell_bug46053() { + HSSFWorkbook wb = new HSSFWorkbook(); + HSSFSheet sheet = wb.createSheet("Sheet1"); + HSSFRow row = sheet.createRow(0); + HSSFCell cellA1 = row.createCell(0); + HSSFCell cellB1 = row.createCell(1); + HSSFFormulaEvaluator fe = new HSSFFormulaEvaluator(wb); + + cellA1.setCellFormula("B1+2.2"); + cellB1.setCellValue(1.5); + + fe.notifyUpdateCell(cellA1); + fe.notifyUpdateCell(cellB1); + + CellValue cv; + cv = fe.evaluate(cellA1); + assertEquals(3.7, cv.getNumberValue(), 0.0); + + cellB1.setCellType(HSSFCell.CELL_TYPE_BLANK); + fe.notifyUpdateCell(cellB1); + cv = fe.evaluate(cellA1); // B1 was used to evaluate A1 + assertEquals(2.2, cv.getNumberValue(), 0.0); + + cellB1.setCellValue(0.4); // changing B1, so A1 cached result should be cleared + fe.notifyUpdateCell(cellB1); + cv = fe.evaluate(cellA1); + if (cv.getNumberValue() == 2.2) { + // looks like left-over cached result from before change to B1 + throw new AssertionFailedError("Identified bug 46053"); + } + assertEquals(2.6, cv.getNumberValue(), 0.0); + } + + /** + * same use-case as the test for bug 46053, but checking trace values too + */ + public void testBlankCellChangedToValueCell() { + + MySheet ms = new MySheet(); + + ms.setCellFormula("A1", "B1+2.2"); + ms.setCellValue("B1", 1.5); + ms.clearAllCachedResultValues(); + ms.clearCell("B1"); + ms.getAndClearLog(); + + confirmEvaluate(ms, "A1", 2.2); + confirmLog(ms, new String[] { + "start A1 B1+2.2", + "end A1 2.2", + }); + ms.setCellValue("B1", 0.4); + confirmLog(ms, new String[] { + "changeFromBlank B1 0.4", + "clear A1", + }); + + confirmEvaluate(ms, "A1", 2.6); + confirmLog(ms, new String[] { + "start A1 B1+2.2", + "hit B1 0.4", + "end A1 2.6", + }); + } + private static void confirmEvaluate(MySheet ms, String cellRefText, double expectedValue) { ValueEval v = ms.evaluateCell(cellRefText); assertEquals(NumberEval.class, v.getClass());