From: Greg Woolsey Date: Tue, 14 Feb 2017 22:05:49 +0000 (+0000) Subject: Bug #56822 fix COUNTIFS() X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=7dfa32c28bea0ff9f5244957727164e181b11a28;p=poi.git Bug #56822 fix COUNTIFS() includes unit test from the issue. Verified unit test results in Excel vs. incorrect previous POI results. Test passes new code, as do existing tests. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1783037 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/src/java/org/apache/poi/ss/formula/functions/Baseifs.java b/src/java/org/apache/poi/ss/formula/functions/Baseifs.java new file mode 100644 index 0000000000..2fcc3c668b --- /dev/null +++ b/src/java/org/apache/poi/ss/formula/functions/Baseifs.java @@ -0,0 +1,183 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ==================================================================== + */ + +package org.apache.poi.ss.formula.functions; + +import org.apache.poi.ss.formula.OperationEvaluationContext; +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; + +/** + * Base class for SUMIFS() and COUNTIFS() functions, as they share much of the same logic, + * the difference being the source of the totals. + */ +/*package*/ abstract class Baseifs implements FreeRefFunction { + + /** + * Implementations must be stateless. + * @return true if there should be a range argument before the criteria pairs + */ + protected abstract boolean hasInitialRange(); + + public ValueEval evaluate(ValueEval[] args, OperationEvaluationContext ec) { + final boolean hasInitialRange = hasInitialRange(); + final int firstCriteria = hasInitialRange ? 1 : 0; + + if( args.length < (2+firstCriteria) || args.length % 2 != firstCriteria ) { + return ErrorEval.VALUE_INVALID; + } + + try { + AreaEval sumRange = null; + if (hasInitialRange) { + sumRange = convertRangeArg(args[0]); + } + + // collect pairs of ranges and criteria + AreaEval[] ae = new AreaEval[(args.length - firstCriteria)/2]; + I_MatchPredicate[] mp = new I_MatchPredicate[ae.length]; + for(int i = firstCriteria, 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(sumRange, ae); + validateCriteria(mp); + + double result = aggregateMatchingCells(sumRange, ae, mp); + return new NumberEval(result); + } catch (EvaluationException e) { + return e.getErrorEval(); + } + } + + /** + * Verify that each criteriaRanges argument contains the same number of rows and columns + * including the sumRange argument if present + * @param sumRange if used, it must match the shape of the criteriaRanges + * @param criteriaRanges to check + * @throws EvaluationException if the ranges do not match. + */ + private static void validateCriteriaRanges(AreaEval sumRange, AreaEval[] criteriaRanges) throws EvaluationException { + int h = criteriaRanges[0].getHeight(); + int w = criteriaRanges[0].getWidth(); + + if (sumRange != null + && (sumRange.getHeight() != h + || sumRange.getWidth() != w) ) { + throw EvaluationException.invalidValue(); + } + + for(AreaEval r : criteriaRanges){ + if(r.getHeight() != h || + r.getWidth() != w ) { + throw EvaluationException.invalidValue(); + } + } + } + + /** + * Verify that each criteria predicate is valid, i.e. not an error + * @param criteria to check + * + * @throws EvaluationException if there are criteria which resulted in Errors. + */ + private static 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 sumRange the range to sum, if used (uses 1 for each match if not present) + * @param ranges criteria ranges + * @param predicates array of predicates, a predicate for each value in ranges + * @return the computed value + */ + private static double aggregateMatchingCells(AreaEval sumRange, AreaEval[] ranges, I_MatchPredicate[] predicates) { + int height = ranges[0].getHeight(); + int width = ranges[0].getWidth(); + + double result = 0.0; + for (int r = 0; r < height; r++) { + for (int c = 0; c < width; c++) { + + boolean matches = true; + for(int i = 0; i < ranges.length; i++){ + AreaEval aeRange = ranges[i]; + I_MatchPredicate mp = predicates[i]; + + if (!mp.matches(aeRange.getRelativeValue(r, c))) { + matches = false; + break; + } + + } + + if(matches) { // sum only if all of the corresponding criteria specified are true for that cell. + result += accumulate(sumRange, r, c); + } + } + } + return result; + } + + /** + * For counts, this would return 1, for sums it returns a cell value or zero. + * This is only called after all the criteria are confirmed true for the coordinates. + * @param sumRange if used + * @param relRowIndex + * @param relColIndex + * @return the aggregate input value corresponding to the given range coordinates + */ + private static double accumulate(AreaEval sumRange, int relRowIndex, int relColIndex) { + if (sumRange == null) return 1.0; // count + + ValueEval addend = sumRange.getRelativeValue(relRowIndex, relColIndex); + if (addend instanceof NumberEval) { + return ((NumberEval)addend).getNumberValue(); + } + // everything else (including string and boolean values) counts as zero + return 0.0; + + } + + protected static AreaEval convertRangeArg(ValueEval eval) throws EvaluationException { + if (eval instanceof AreaEval) { + return (AreaEval) eval; + } + if (eval instanceof RefEval) { + return ((RefEval)eval).offset(0, 0, 0, 0); + } + throw new EvaluationException(ErrorEval.VALUE_INVALID); + } + +} diff --git a/src/java/org/apache/poi/ss/formula/functions/Countifs.java b/src/java/org/apache/poi/ss/formula/functions/Countifs.java index 15e6a1521e..0ac50635aa 100644 --- a/src/java/org/apache/poi/ss/formula/functions/Countifs.java +++ b/src/java/org/apache/poi/ss/formula/functions/Countifs.java @@ -18,11 +18,6 @@ package org.apache.poi.ss.formula.functions; -import org.apache.poi.ss.formula.OperationEvaluationContext; -import org.apache.poi.ss.formula.eval.ErrorEval; -import org.apache.poi.ss.formula.eval.NumberEval; -import org.apache.poi.ss.formula.eval.ValueEval; - /** * Implementation for the function COUNTIFS *

@@ -30,33 +25,20 @@ import org.apache.poi.ss.formula.eval.ValueEval; *

*/ -public class Countifs implements FreeRefFunction { +public class Countifs extends Baseifs { + /** + * Singleton + */ public static final FreeRefFunction instance = new Countifs(); - public ValueEval evaluate(ValueEval[] args, OperationEvaluationContext ec) { - // https://support.office.com/en-us/article/COUNTIFS-function-dda3dc6e-f74e-4aee-88bc-aa8c2a866842?ui=en-US&rs=en-US&ad=US - // COUNTIFS(criteria_range1, criteria1, [criteria_range2, criteria2]...) - // need at least 2 arguments and need to have an even number of arguments (criteria_range1, criteria1 plus x*(criteria_range, criteria)) - if (args.length < 2 || args.length % 2 != 0) { - return ErrorEval.VALUE_INVALID; - } - - Double result = null; - // for each (criteria_range, criteria) pair - for (int i = 0; i < args.length; i += 2) { - ValueEval firstArg = args[i]; - ValueEval secondArg = args[i + 1]; - NumberEval evaluate = (NumberEval) new Countif().evaluate( - new ValueEval[] {firstArg, secondArg}, - ec.getRowIndex(), - ec.getColumnIndex()); - if (result == null) { - result = evaluate.getNumberValue(); - } else if (evaluate.getNumberValue() < result) { - result = evaluate.getNumberValue(); - } - } - return new NumberEval(result == null ? 0 : result); + /** + * https://support.office.com/en-us/article/COUNTIFS-function-dda3dc6e-f74e-4aee-88bc-aa8c2a866842?ui=en-US&rs=en-US&ad=US + * COUNTIFS(criteria_range1, criteria1, [criteria_range2, criteria2]...) + * need at least 2 arguments and need to have an even number of arguments (criteria_range1, criteria1 plus x*(criteria_range, criteria)) + * @see org.apache.poi.ss.formula.functions.Baseifs#hasInitialRange() + */ + protected boolean hasInitialRange() { + return false; } } 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 81d0003fc5..edc138a437 100644 --- a/src/java/org/apache/poi/ss/formula/functions/Sumifs.java +++ b/src/java/org/apache/poi/ss/formula/functions/Sumifs.java @@ -19,16 +19,6 @@ package org.apache.poi.ss.formula.functions; -import org.apache.poi.ss.formula.OperationEvaluationContext; -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

* @@ -48,127 +38,21 @@ import org.apache.poi.ss.formula.functions.Countif.ErrorMatcher; * *

* - * @author Yegor Kozlov */ -public final class Sumifs implements FreeRefFunction { - public static final FreeRefFunction instance = new Sumifs(); - - public ValueEval evaluate(ValueEval[] args, OperationEvaluationContext ec) { - // https://support.office.com/en-us/article/SUMIFS-function-c9e748f5-7ea7-455d-9406-611cebce642b - // COUNTIFS(sum_range, criteria_range1, criteria1, [criteria_range2, criteria2], ... - // need at least 3 arguments and need to have an odd number of arguments (sum-range plus x*(criteria_range, criteria)) - if(args.length < 3 || args.length % 2 == 0) { - return ErrorEval.VALUE_INVALID; - } - - try { - AreaEval sumRange = convertRangeArg(args[0]); - - // collect pairs of ranges and criteria - AreaEval[] ae = new AreaEval[(args.length - 1)/2]; - 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); - } catch (EvaluationException e) { - return e.getErrorEval(); - } - } - +public final class Sumifs extends Baseifs { /** - * Verify that each criteriaRanges argument contains the same number of rows and columns - * as the sumRange argument - * - * @throws EvaluationException if the ranges do not match. + * Singleton */ - private void validateCriteriaRanges(AreaEval[] criteriaRanges, AreaEval sumRange) throws EvaluationException { - for(AreaEval r : criteriaRanges){ - if(r.getHeight() != sumRange.getHeight() || - r.getWidth() != sumRange.getWidth() ) { - throw EvaluationException.invalidValue(); - } - } - } - - /** - * 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())); - } - } - } - + public static final FreeRefFunction instance = new Sumifs(); /** - * - * @param ranges criteria ranges, each range must be of the same dimensions as aeSum - * @param predicates array of predicates, a predicate for each value in ranges - * @param aeSum the range to sum - * - * @return the computed value + * https://support.office.com/en-us/article/SUMIFS-function-c9e748f5-7ea7-455d-9406-611cebce642b + * COUNTIFS(sum_range, criteria_range1, criteria1, [criteria_range2, criteria2], ... + * need at least 3 arguments and need to have an odd number of arguments (sum-range plus x*(criteria_range, criteria)) + * @see org.apache.poi.ss.formula.functions.Baseifs#hasInitialRange() */ - private static double sumMatchingCells(AreaEval[] ranges, I_MatchPredicate[] predicates, AreaEval aeSum) { - int height = aeSum.getHeight(); - int width = aeSum.getWidth(); - - double result = 0.0; - for (int r = 0; r < height; r++) { - for (int c = 0; c < width; c++) { - - boolean matches = true; - for(int i = 0; i < ranges.length; i++){ - AreaEval aeRange = ranges[i]; - I_MatchPredicate mp = predicates[i]; - - if (!mp.matches(aeRange.getRelativeValue(r, c))) { - matches = false; - break; - } - - } - - if(matches) { // sum only if all of the corresponding criteria specified are true for that cell. - result += accumulate(aeSum, r, c); - } - } - } - return result; + @Override + protected boolean hasInitialRange() { + return true; } - - private static double accumulate(AreaEval aeSum, int relRowIndex, - int relColIndex) { - - ValueEval addend = aeSum.getRelativeValue(relRowIndex, relColIndex); - if (addend instanceof NumberEval) { - return ((NumberEval)addend).getNumberValue(); - } - // everything else (including string and boolean values) counts as zero - return 0.0; - } - - private static AreaEval convertRangeArg(ValueEval eval) throws EvaluationException { - if (eval instanceof AreaEval) { - return (AreaEval) eval; - } - if (eval instanceof RefEval) { - return ((RefEval)eval).offset(0, 0, 0, 0); - } - throw new EvaluationException(ErrorEval.VALUE_INVALID); - } - } diff --git a/src/ooxml/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java b/src/ooxml/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java new file mode 100644 index 0000000000..3b3d71465e --- /dev/null +++ b/src/ooxml/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java @@ -0,0 +1,122 @@ +/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + + +package org.apache.poi.ss.formula.functions; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + +import org.apache.poi.hssf.usermodel.HSSFWorkbook; +import org.apache.poi.ss.usermodel.Cell; +import org.apache.poi.ss.usermodel.CellType; +import org.apache.poi.ss.usermodel.CellValue; +import org.apache.poi.ss.usermodel.FormulaEvaluator; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.Workbook; +import org.apache.poi.ss.util.SheetUtil; +import org.apache.poi.util.IOUtils; +import org.apache.poi.xssf.XSSFTestDataSamples; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** + * Test the COUNTIFS() function + */ +public class CountifsTests { + + private Workbook workbook; + + /** + * initialize a workbook + */ + @Before + public void before() { + // not sure why we allow this, COUNTIFS() is only available + // in OOXML, it was introduced with Office 2007 + workbook = new HSSFWorkbook(); + } + + /** + * Close the workbook if needed + */ + @After + public void after() { + IOUtils.closeQuietly(workbook); + } + + /** + * Basic call + */ + @Test + public void testCallFunction() { + Sheet sheet = workbook.createSheet("test"); + Row row1 = sheet.createRow(0); + Cell cellA1 = row1.createCell(0, CellType.FORMULA); + Cell cellB1 = row1.createCell(1, CellType.NUMERIC); + Cell cellC1 = row1.createCell(2, CellType.NUMERIC); + Cell cellD1 = row1.createCell(3, CellType.NUMERIC); + Cell cellE1 = row1.createCell(4, CellType.NUMERIC); + cellB1.setCellValue(1); + cellC1.setCellValue(1); + cellD1.setCellValue(2); + cellE1.setCellValue(4); + + cellA1.setCellFormula("COUNTIFS(B1:C1,1, D1:E1,2)"); + FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator(); + CellValue evaluate = evaluator.evaluate(cellA1); + assertEquals(1.0d, evaluate.getNumberValue(), 0.000000000000001); + } + + /** + * Test argument count check + */ + @Test + public void testCallFunction_invalidArgs() { + Sheet sheet = workbook.createSheet("test"); + Row row1 = sheet.createRow(0); + Cell cellA1 = row1.createCell(0, CellType.FORMULA); + cellA1.setCellFormula("COUNTIFS()"); + FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator(); + CellValue evaluate = evaluator.evaluate(cellA1); + assertEquals(15, evaluate.getErrorValue()); + cellA1.setCellFormula("COUNTIFS(A1:C1)"); + evaluator = workbook.getCreationHelper().createFormulaEvaluator(); + evaluate = evaluator.evaluate(cellA1); + assertEquals(15, evaluate.getErrorValue()); + cellA1.setCellFormula("COUNTIFS(A1:C1,2,2)"); + evaluator = workbook.getCreationHelper().createFormulaEvaluator(); + evaluate = evaluator.evaluate(cellA1); + assertEquals(15, evaluate.getErrorValue()); + } + + /** + * the bug returned the wrong count, this verifies the fix + * @throws Exception if the file can't be read + */ + @Test + public void testBug56822() throws Exception { + workbook = XSSFTestDataSamples.openSampleWorkbook("56822-Countifs.xlsx"); + FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator(); + Cell cell = SheetUtil.getCell(workbook.getSheetAt(0), 0, 3); + assertNotNull("Test workbook missing cell D1", cell); + CellValue evaluate = evaluator.evaluate(cell); + assertEquals(2.0d, evaluate.getNumberValue(), 0.00000000000001); + } +} diff --git a/src/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java b/src/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java deleted file mode 100644 index 5dfc15ad67..0000000000 --- a/src/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java +++ /dev/null @@ -1,71 +0,0 @@ -/* ==================================================================== - Licensed to the Apache Software Foundation (ASF) under one or more - contributor license agreements. See the NOTICE file distributed with - this work for additional information regarding copyright ownership. - The ASF licenses this file to You under the Apache License, Version 2.0 - (the "License"); you may not use this file except in compliance with - the License. You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -==================================================================== */ - - -package org.apache.poi.ss.formula.functions; - -import org.apache.poi.hssf.usermodel.HSSFWorkbook; -import org.apache.poi.ss.usermodel.Cell; -import org.apache.poi.ss.usermodel.CellType; -import org.apache.poi.ss.usermodel.CellValue; -import org.apache.poi.ss.usermodel.FormulaEvaluator; -import org.apache.poi.ss.usermodel.Row; -import org.apache.poi.ss.usermodel.Sheet; - -import junit.framework.TestCase; - -public class CountifsTests extends TestCase { - - public void testCallFunction() { - HSSFWorkbook workbook = new HSSFWorkbook(); - Sheet sheet = workbook.createSheet("test"); - Row row1 = sheet.createRow(0); - Cell cellA1 = row1.createCell(0, CellType.FORMULA); - Cell cellB1 = row1.createCell(1, CellType.NUMERIC); - Cell cellC1 = row1.createCell(2, CellType.NUMERIC); - Cell cellD1 = row1.createCell(3, CellType.NUMERIC); - Cell cellE1 = row1.createCell(4, CellType.NUMERIC); - cellB1.setCellValue(1); - cellC1.setCellValue(1); - cellD1.setCellValue(2); - cellE1.setCellValue(4); - - cellA1.setCellFormula("COUNTIFS(B1:C1,1, D1:E1,2)"); - FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator(); - CellValue evaluate = evaluator.evaluate(cellA1); - assertEquals(1.0d, evaluate.getNumberValue()); - } - - public void testCallFunction_invalidArgs() { - HSSFWorkbook workbook = new HSSFWorkbook(); - Sheet sheet = workbook.createSheet("test"); - Row row1 = sheet.createRow(0); - Cell cellA1 = row1.createCell(0, CellType.FORMULA); - cellA1.setCellFormula("COUNTIFS()"); - FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator(); - CellValue evaluate = evaluator.evaluate(cellA1); - assertEquals(15, evaluate.getErrorValue()); - cellA1.setCellFormula("COUNTIFS(A1:C1)"); - evaluator = workbook.getCreationHelper().createFormulaEvaluator(); - evaluate = evaluator.evaluate(cellA1); - assertEquals(15, evaluate.getErrorValue()); - cellA1.setCellFormula("COUNTIFS(A1:C1,2,2)"); - evaluator = workbook.getCreationHelper().createFormulaEvaluator(); - evaluate = evaluator.evaluate(cellA1); - assertEquals(15, evaluate.getErrorValue()); - } -} diff --git a/test-data/spreadsheet/56822-Countifs.xlsx b/test-data/spreadsheet/56822-Countifs.xlsx new file mode 100644 index 0000000000..8444d198c0 Binary files /dev/null and b/test-data/spreadsheet/56822-Countifs.xlsx differ