From 4589cb309e67ee73d4a7340bb4c698141a1e3bda Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Sat, 20 Jun 2015 13:10:28 +0000 Subject: [PATCH] Bug 56655: Fix Sumifs for cases where the criteria is in error. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1686610 13f79535-47bb-0310-9956-ffa450edef68 --- .../poi/ss/formula/functions/Countif.java | 11 +- .../poi/ss/formula/functions/Sumifs.java | 28 ++++- .../poi/xssf/usermodel/TestUnfixedBugs.java | 63 ---------- .../usermodel/TestXSSFFormulaEvaluation.java | 108 ++++++++++++++---- .../poi/ss/formula/functions/TestSumifs.java | 81 +++++++++++++ 5 files changed, 203 insertions(+), 88 deletions(-) diff --git a/src/java/org/apache/poi/ss/formula/functions/Countif.java b/src/java/org/apache/poi/ss/formula/functions/Countif.java index 6f27fdf48e..01f608a54b 100644 --- a/src/java/org/apache/poi/ss/formula/functions/Countif.java +++ b/src/java/org/apache/poi/ss/formula/functions/Countif.java @@ -30,7 +30,7 @@ import org.apache.poi.ss.formula.eval.RefEval; import org.apache.poi.ss.formula.eval.StringEval; import org.apache.poi.ss.formula.eval.ValueEval; import org.apache.poi.ss.formula.functions.CountUtils.I_MatchPredicate; -import org.apache.poi.ss.usermodel.ErrorConstants; +import org.apache.poi.ss.usermodel.FormulaError; /** * Implementation for the function COUNTIF @@ -254,6 +254,7 @@ public final class Countif extends Fixed2ArgFunction { // boolean values when the target(x) is a string return false; } + @SuppressWarnings("unused") StringEval se = (StringEval)x; Boolean val = parseBoolean(se.getStringValue()); if(val == null) { @@ -286,7 +287,7 @@ public final class Countif extends Fixed2ArgFunction { return evaluate(testValue - _value); } } - private static final class ErrorMatcher extends MatcherBase { + public static final class ErrorMatcher extends MatcherBase { private final int _value; @@ -296,7 +297,7 @@ public final class Countif extends Fixed2ArgFunction { } @Override protected String getValueText() { - return ErrorConstants.getText(_value); + return FormulaError.forInt(_value).getString(); } public boolean matches(ValueEval x) { @@ -306,6 +307,10 @@ public final class Countif extends Fixed2ArgFunction { } return false; } + + public int getValue() { + return _value; + } } public static final class StringMatcher extends MatcherBase { diff --git a/src/java/org/apache/poi/ss/formula/functions/Sumifs.java b/src/java/org/apache/poi/ss/formula/functions/Sumifs.java index 63d35439af..087b72bc12 100644 --- a/src/java/org/apache/poi/ss/formula/functions/Sumifs.java +++ b/src/java/org/apache/poi/ss/formula/functions/Sumifs.java @@ -20,8 +20,14 @@ package org.apache.poi.ss.formula.functions; import org.apache.poi.ss.formula.OperationEvaluationContext; -import org.apache.poi.ss.formula.eval.*; +import org.apache.poi.ss.formula.eval.AreaEval; +import org.apache.poi.ss.formula.eval.ErrorEval; +import org.apache.poi.ss.formula.eval.EvaluationException; +import org.apache.poi.ss.formula.eval.NumberEval; +import org.apache.poi.ss.formula.eval.RefEval; +import org.apache.poi.ss.formula.eval.ValueEval; import org.apache.poi.ss.formula.functions.CountUtils.I_MatchPredicate; +import org.apache.poi.ss.formula.functions.Countif.ErrorMatcher; /** * Implementation for the Excel function SUMIFS

@@ -60,10 +66,12 @@ public final class Sumifs implements FreeRefFunction { I_MatchPredicate[] mp = new I_MatchPredicate[ae.length]; for(int i = 1, k=0; i < args.length; i += 2, k++){ ae[k] = convertRangeArg(args[i]); + mp[k] = Countif.createCriteriaPredicate(args[i+1], ec.getRowIndex(), ec.getColumnIndex()); } validateCriteriaRanges(ae, sumRange); + validateCriteria(mp); double result = sumMatchingCells(ae, mp, sumRange); return new NumberEval(result); @@ -76,7 +84,7 @@ public final class Sumifs implements FreeRefFunction { * Verify that each criteriaRanges argument contains the same number of rows and columns * as the sumRange argument * - * @throws EvaluationException if + * @throws EvaluationException if the ranges do not match. */ private void validateCriteriaRanges(AreaEval[] criteriaRanges, AreaEval sumRange) throws EvaluationException { for(AreaEval r : criteriaRanges){ @@ -87,6 +95,22 @@ public final class Sumifs implements FreeRefFunction { } } + /** + * Verify that each criteria predicate is valid, i.e. not an error + * + * @throws EvaluationException if there are criteria which resulted in Errors. + */ + private void validateCriteria(I_MatchPredicate[] criteria) throws EvaluationException { + for(I_MatchPredicate predicate : criteria) { + + // check for errors in predicate and return immediately using this error code + if(predicate instanceof ErrorMatcher) { + throw new EvaluationException(ErrorEval.valueOf(((ErrorMatcher)predicate).getValue())); + } + } + } + + /** * * @param ranges criteria ranges, each range must be of the same dimensions as aeSum diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestUnfixedBugs.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestUnfixedBugs.java index b40c4ac1ea..83004cb3b8 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestUnfixedBugs.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestUnfixedBugs.java @@ -34,7 +34,6 @@ import org.apache.poi.ss.usermodel.Cell; import org.apache.poi.ss.usermodel.CellStyle; import org.apache.poi.ss.usermodel.DataFormatter; import org.apache.poi.ss.usermodel.DateUtil; -import org.apache.poi.ss.usermodel.FormulaError; import org.apache.poi.ss.usermodel.RichTextString; import org.apache.poi.ss.usermodel.Row; import org.apache.poi.ss.usermodel.Sheet; @@ -219,68 +218,6 @@ public final class TestUnfixedBugs extends TestCase { assertEquals(4, strBack.getIndexOfFormattingRun(2)); } - @Test - public void testBug56655() { - XSSFWorkbook wb = new XSSFWorkbook(); - Sheet sheet = wb.createSheet(); - - setCellFormula(sheet, 0, 0, "#VALUE!"); - setCellFormula(sheet, 0, 1, "SUMIFS(A:A,A:A,#VALUE!)"); - - XSSFFormulaEvaluator.evaluateAllFormulaCells(wb); - - assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0,0).getCachedFormulaResultType()); - assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0,0).getErrorCellValue()); - assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0,1).getCachedFormulaResultType()); - assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0,1).getErrorCellValue()); - } - - @Test - public void testBug56655a() { - XSSFWorkbook wb = new XSSFWorkbook(); - Sheet sheet = wb.createSheet(); - - setCellFormula(sheet, 0, 0, "B1*C1"); - sheet.getRow(0).createCell(1).setCellValue("A"); - setCellFormula(sheet, 1, 0, "B1*C1"); - sheet.getRow(1).createCell(1).setCellValue("A"); - setCellFormula(sheet, 0, 3, "SUMIFS(A:A,A:A,A2)"); - - XSSFFormulaEvaluator.evaluateAllFormulaCells(wb); - - assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0, 0).getCachedFormulaResultType()); - assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0, 0).getErrorCellValue()); - assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 1, 0).getCachedFormulaResultType()); - assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 1, 0).getErrorCellValue()); - assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0, 3).getCachedFormulaResultType()); - assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0, 3).getErrorCellValue()); - } - - /** - * @param row 0-based - * @param column 0-based - */ - private void setCellFormula(Sheet sheet, int row, int column, String formula) { - Row r = sheet.getRow(row); - if (r == null) { - r = sheet.createRow(row); - } - Cell cell = r.getCell(column); - if (cell == null) { - cell = r.createCell(column); - } - cell.setCellType(XSSFCell.CELL_TYPE_FORMULA); - cell.setCellFormula(formula); - } - - /** - * @param rowNo 0-based - * @param column 0-based - */ - private Cell getCell(Sheet sheet, int rowNo, int column) { - return sheet.getRow(rowNo).getCell(column); - } - @Test public void testBug55752() throws IOException { Workbook wb = new XSSFWorkbook(); 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 c2a7597d38..d56005a570 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFFormulaEvaluation.java @@ -25,6 +25,7 @@ import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.ss.usermodel.BaseTestFormulaEvaluator; import org.apache.poi.ss.usermodel.Cell; import org.apache.poi.ss.usermodel.CellValue; +import org.apache.poi.ss.usermodel.FormulaError; import org.apache.poi.ss.usermodel.FormulaEvaluator; import org.apache.poi.ss.usermodel.Row; import org.apache.poi.ss.usermodel.Sheet; @@ -186,28 +187,32 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator { // Link and re-try Workbook alt = new XSSFWorkbook(); - alt.createSheet().createRow(0).createCell(0).setCellValue("In another workbook"); - // TODO Implement the rest of this, see bug #57184 -/* - wb.linkExternalWorkbook("alt.xlsx", alt); - - cXSLX_nw_cell.setCellFormula("[alt.xlsx]Sheet1!$A$1"); - // Check it - TODO Is this correct? Or should it become [3]Sheet1!$A$1 ? - assertEquals("[alt.xlsx]Sheet1!$A$1", cXSLX_nw_cell.getCellFormula()); - - // Evaluate it, without a link to that workbook try { + alt.createSheet().createRow(0).createCell(0).setCellValue("In another workbook"); + // TODO Implement the rest of this, see bug #57184 +/* + wb.linkExternalWorkbook("alt.xlsx", alt); + + cXSLX_nw_cell.setCellFormula("[alt.xlsx]Sheet1!$A$1"); + // Check it - TODO Is this correct? Or should it become [3]Sheet1!$A$1 ? + assertEquals("[alt.xlsx]Sheet1!$A$1", cXSLX_nw_cell.getCellFormula()); + + // Evaluate it, without a link to that workbook + try { + evaluator.evaluate(cXSLX_nw_cell); + fail("No cached value and no link to workbook, shouldn't evaluate"); + } catch(Exception e) {} + + // Add a link, check it does + evaluators.put("alt.xlsx", alt.getCreationHelper().createFormulaEvaluator()); + evaluator.setupReferencedWorkbooks(evaluators); + evaluator.evaluate(cXSLX_nw_cell); - fail("No cached value and no link to workbook, shouldn't evaluate"); - } catch(Exception e) {} - - // Add a link, check it does - evaluators.put("alt.xlsx", alt.getCreationHelper().createFormulaEvaluator()); - evaluator.setupReferencedWorkbooks(evaluators); - - evaluator.evaluate(cXSLX_nw_cell); - assertEquals("In another workbook", cXSLX_nw_cell.getStringCellValue()); + assertEquals("In another workbook", cXSLX_nw_cell.getStringCellValue()); */ + } finally { + alt.close(); + } } /** @@ -519,7 +524,6 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator { } } - public void testBug55843f() throws IOException { XSSFWorkbook wb = new XSSFWorkbook(); try { @@ -539,4 +543,68 @@ public final class TestXSSFFormulaEvaluation extends BaseTestFormulaEvaluator { wb.close(); } } + + public void testBug56655() throws IOException { + Workbook wb = new XSSFWorkbook(); + Sheet sheet = wb.createSheet(); + + setCellFormula(sheet, 0, 0, "#VALUE!"); + setCellFormula(sheet, 0, 1, "SUMIFS(A:A,A:A,#VALUE!)"); + + wb.getCreationHelper().createFormulaEvaluator().evaluateAll(); + + assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0,0).getCachedFormulaResultType()); + assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0,0).getErrorCellValue()); + assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0,1).getCachedFormulaResultType()); + assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0,1).getErrorCellValue()); + + wb.close(); + } + + public void testBug56655a() throws IOException { + Workbook wb = new XSSFWorkbook(); + Sheet sheet = wb.createSheet(); + + setCellFormula(sheet, 0, 0, "B1*C1"); + sheet.getRow(0).createCell(1).setCellValue("A"); + setCellFormula(sheet, 1, 0, "B1*C1"); + sheet.getRow(1).createCell(1).setCellValue("A"); + setCellFormula(sheet, 0, 3, "SUMIFS(A:A,A:A,A2)"); + + wb.getCreationHelper().createFormulaEvaluator().evaluateAll(); + + assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0, 0).getCachedFormulaResultType()); + assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0, 0).getErrorCellValue()); + assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 1, 0).getCachedFormulaResultType()); + assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 1, 0).getErrorCellValue()); + assertEquals(XSSFCell.CELL_TYPE_ERROR, getCell(sheet, 0, 3).getCachedFormulaResultType()); + assertEquals(FormulaError.VALUE.getCode(), getCell(sheet, 0, 3).getErrorCellValue()); + + wb.close(); + } + + /** + * @param row 0-based + * @param column 0-based + */ + private void setCellFormula(Sheet sheet, int row, int column, String formula) { + Row r = sheet.getRow(row); + if (r == null) { + r = sheet.createRow(row); + } + Cell cell = r.getCell(column); + if (cell == null) { + cell = r.createCell(column); + } + cell.setCellType(XSSFCell.CELL_TYPE_FORMULA); + cell.setCellFormula(formula); + } + + /** + * @param rowNo 0-based + * @param column 0-based + */ + private Cell getCell(Sheet sheet, int rowNo, int column) { + return sheet.getRow(rowNo).getCell(column); + } } diff --git a/src/testcases/org/apache/poi/ss/formula/functions/TestSumifs.java b/src/testcases/org/apache/poi/ss/formula/functions/TestSumifs.java index 386b521f21..3f611cb502 100644 --- a/src/testcases/org/apache/poi/ss/formula/functions/TestSumifs.java +++ b/src/testcases/org/apache/poi/ss/formula/functions/TestSumifs.java @@ -21,6 +21,7 @@ package org.apache.poi.ss.formula.functions; import junit.framework.AssertionFailedError; import junit.framework.TestCase; + import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.hssf.usermodel.*; import org.apache.poi.ss.formula.OperationEvaluationContext; @@ -264,4 +265,84 @@ public final class TestSumifs extends TestCase { assertEquals(625000., ex5cell.getNumericCellValue()); } + + public void testBug56655() { + ValueEval[] a2a9 = new ValueEval[] { + new NumberEval(5), + new NumberEval(4), + new NumberEval(15), + new NumberEval(3), + new NumberEval(22), + new NumberEval(12), + new NumberEval(10), + new NumberEval(33) + }; + + ValueEval[] args = new ValueEval[]{ + EvalFactory.createAreaEval("A2:A9", a2a9), + ErrorEval.VALUE_INVALID, + new StringEval("A*"), + }; + + ValueEval result = invokeSumifs(args, EC); + assertTrue("Expect to have an error when an input is an invalid value, but had: " + result.getClass(), result instanceof ErrorEval); + + args = new ValueEval[]{ + EvalFactory.createAreaEval("A2:A9", a2a9), + EvalFactory.createAreaEval("A2:A9", a2a9), + ErrorEval.VALUE_INVALID, + }; + + result = invokeSumifs(args, EC); + assertTrue("Expect to have an error when an input is an invalid value, but had: " + result.getClass(), result instanceof ErrorEval); + } + + public void testBug56655b() { +/* + setCellFormula(sheet, 0, 0, "B1*C1"); + sheet.getRow(0).createCell(1).setCellValue("A"); + setCellFormula(sheet, 1, 0, "B1*C1"); + sheet.getRow(1).createCell(1).setCellValue("A"); + setCellFormula(sheet, 0, 3, "SUMIFS(A:A,A:A,A2)"); + */ + ValueEval[] a0a1 = new ValueEval[] { + NumberEval.ZERO, + NumberEval.ZERO + }; + + ValueEval[] args = new ValueEval[]{ + EvalFactory.createAreaEval("A0:A1", a0a1), + EvalFactory.createAreaEval("A0:A1", a0a1), + ErrorEval.VALUE_INVALID + }; + + ValueEval result = invokeSumifs(args, EC); + assertTrue("Expect to have an error when an input is an invalid value, but had: " + result.getClass(), result instanceof ErrorEval); + assertEquals(ErrorEval.VALUE_INVALID, result); + } + + + public void testBug56655c() { +/* + setCellFormula(sheet, 0, 0, "B1*C1"); + sheet.getRow(0).createCell(1).setCellValue("A"); + setCellFormula(sheet, 1, 0, "B1*C1"); + sheet.getRow(1).createCell(1).setCellValue("A"); + setCellFormula(sheet, 0, 3, "SUMIFS(A:A,A:A,A2)"); + */ + ValueEval[] a0a1 = new ValueEval[] { + NumberEval.ZERO, + NumberEval.ZERO + }; + + ValueEval[] args = new ValueEval[]{ + EvalFactory.createAreaEval("A0:A1", a0a1), + EvalFactory.createAreaEval("A0:A1", a0a1), + ErrorEval.NAME_INVALID + }; + + ValueEval result = invokeSumifs(args, EC); + assertTrue("Expect to have an error when an input is an invalid value, but had: " + result.getClass(), result instanceof ErrorEval); + assertEquals(ErrorEval.NAME_INVALID, result); + } } -- 2.39.5