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

4 files changed, 262 insertions, 162 deletions

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 <code>criteriaRanges</code> argument contains the same number of rows and columns
+     * including the <code>sumRange</code> 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 <code>criteria</code> 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 <code>ranges</code>
+     * @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
  * <p>
@@ -30,33 +25,20 @@ import org.apache.poi.ss.formula.eval.ValueEval;
  * </p>
  */
 
-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<p>
  *
@@ -48,127 +38,21 @@ import org.apache.poi.ss.formula.functions.Countif.ErrorMatcher;
  *    </ul>
  * </p>
  *
- * @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 <code>criteriaRanges</code> argument contains the same number of rows and columns
-     * as the <code>sumRange</code> 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 <code>criteria</code> 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 <code>aeSum</code>
-     * @param predicates  array of predicates, a predicate for each value in <code>ranges</code>
-     * @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/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java b/src/ooxml/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java
index 5dfc15ad67..3b3d71465e 100644
--- a/src/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java
+++ b/src/ooxml/testcases/org/apache/poi/ss/formula/functions/CountifsTests.java
@@ -18,6 +18,9 @@
 
 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;
@@ -25,13 +28,44 @@ 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;
 
-import junit.framework.TestCase;
-
-public class CountifsTests extends TestCase {
+/**
+ * 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() {
-        HSSFWorkbook workbook = new HSSFWorkbook();
         Sheet sheet = workbook.createSheet("test");
         Row row1 = sheet.createRow(0);
         Cell cellA1 = row1.createCell(0, CellType.FORMULA);
@@ -47,11 +81,14 @@ public class CountifsTests extends TestCase {
         cellA1.setCellFormula("COUNTIFS(B1:C1,1, D1:E1,2)");
         FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator();
         CellValue evaluate = evaluator.evaluate(cellA1);
-        assertEquals(1.0d, evaluate.getNumberValue());
+        assertEquals(1.0d, evaluate.getNumberValue(), 0.000000000000001);
     }
 
+    /**
+     * Test argument count check
+     */
+    @Test
     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);
@@ -68,4 +105,18 @@ public class CountifsTests extends TestCase {
         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);
+    }
 }