Bug 56822

Summary: Countifs function implements wrong logic
Product: POI Reporter: kirill.frolov77 <kirill.frolov77>
Component: XSSFAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: critical    
Priority: P1    
Version: 3.15-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: All   
Attachments: Countifs test file (same as unit test)

Description kirill.frolov77@gmail.com 2014-08-06 18:32:41 UTC
Hello!

Unfortunately has to rewrite Countifs function from scratch due to utterly incorrect algorithm logic in existing implementation.
In contrast, sumifs function looks good.
So I adapted sumifs implementation to provide correct countifs implementation.

The source code goes below.

----------------

/* ====================================================================
   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;

/**
 * Implementation for the function COUNTIFS
 * <p>
 * Syntax: COUNTIFS(criteria_range1, criteria1, [criteria_range2, criteria2])
 * </p>
 */

public class Countifs implements FreeRefFunction {
	public static final FreeRefFunction instance = new Countifs();

	@Override
	public ValueEval evaluate(ValueEval[] args, OperationEvaluationContext ec) {
		if (args.length == 0 || args.length % 2 != 0) {
			return ErrorEval.VALUE_INVALID;
		}
		try {
			// collect pairs of ranges and criteria
			int len = args.length / 2;
			AreaEval[] ae = new AreaEval[len];
			I_MatchPredicate[] mp = new I_MatchPredicate[len];
			for (int i = 0; i < len; i++) {
				ae[i] = convertRangeArg(args[2 * i]);
				mp[i] = Countif.createCriteriaPredicate(args[2 * i + 1],
						ec.getRowIndex(), ec.getColumnIndex());
			}

			validateCriteriaRanges(ae);

			int result = countMatchingCells(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 as the <code>sumRange</code> argument
	 *
	 * @throws EvaluationException
	 *             if
	 */
	private static void validateCriteriaRanges(AreaEval[] criteriaRanges) throws EvaluationException {
		for (AreaEval r : criteriaRanges) {
			if (r.getHeight() != criteriaRanges[0].getHeight()
					|| r.getWidth() != criteriaRanges[0].getWidth()) {
				throw EvaluationException.invalidValue();
			}
		}
	}

	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);
	}

	/**
	 *
	 * @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
	 */
	private static int countMatchingCells(AreaEval[] ranges,
			I_MatchPredicate[] predicates) {
		int height = ranges[0].getHeight();
		int width = ranges[0].getWidth();

		int result = 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];

					ValueEval relativeValue = aeRange.getRelativeValue(r, c);
					if (!mp.matches(relativeValue)) {
						matches = false;
						break;
					}

				}

				if (matches) { // count only if all of the corresponding
								// criteria specified are true for that cell.
					result += 1;
				}
			}
		}
		return result;
	}
}
Comment 1 Nick Burch 2014-08-07 07:33:38 UTC
Given how much of the logic you've had to copy, is it possible for you to either re-write it so that one class does both functions (eg similar to AggregateFunction), or so they have a common parent class with the core bits in?

Also, if you could write a small unit test that shows the incorrect logic, that'd be a big help too!
Comment 2 Sebastiaan Blommers 2016-11-17 19:36:10 UTC
This test fails formula produces 3 instead of 2

	@Test
	public void test2() {
	        HSSFWorkbook workbook = new HSSFWorkbook();
	        Sheet sheet = workbook.createSheet("test");
	        Row row3 = sheet.createRow(2);
	        Row row4 = sheet.createRow(3);
	        Row row5 = sheet.createRow(4);
	        Row row6 = sheet.createRow(5);
	        Row row7 = sheet.createRow(6);
	        Row row8 = sheet.createRow(7);
	        
	        Cell cellA3 = row3.createCell(0, CellType.STRING);
	        Cell cellA4 = row4.createCell(0, CellType.STRING);
	        Cell cellA5 = row5.createCell(0, CellType.STRING);
	        Cell cellA6 = row6.createCell(0, CellType.STRING);
	        Cell cellA7 = row7.createCell(0, CellType.STRING);
	        Cell cellA8 = row8.createCell(0, CellType.STRING);

	        Cell cellB3 = row3.createCell(1, CellType.NUMERIC);
	        Cell cellB4 = row4.createCell(1, CellType.NUMERIC);
	        Cell cellB5 = row5.createCell(1, CellType.NUMERIC);
	        Cell cellB6 = row6.createCell(1, CellType.NUMERIC);
	        Cell cellB7 = row7.createCell(1, CellType.NUMERIC);
	        Cell cellB8 = row8.createCell(1, CellType.NUMERIC);
	        
	        cellA3.setCellValue("a");
	        cellA4.setCellValue("a");
	        cellA5.setCellValue("a");
	        cellA6.setCellValue("b");
	        cellA7.setCellValue("b");
	        cellA8.setCellValue("b");
	        
	        cellB3.setCellValue(1);
	        cellB4.setCellValue(1);
	        cellB5.setCellValue(2);
	        cellB6.setCellValue(2);
	        cellB7.setCellValue(1);
	        cellB8.setCellValue(2);

	        Cell cellD3 = row3.createCell(2, CellType.FORMULA);
	        cellD3.setCellFormula("COUNTIFS(A3:A8,\"a\",B3:B8,1)");
	        FormulaEvaluator evaluator = workbook.getCreationHelper().createFormulaEvaluator();
	        CellValue evaluate = evaluator.evaluate(cellD3);
	        Assert.assertEquals(2.0d, evaluate.getNumberValue());
	}

Above code of countifs fixes at least this unit test
Comment 3 Sebastiaan Blommers 2016-11-17 19:37:03 UTC
Created attachment 34458 [details]
Countifs test file (same as unit test)
Comment 4 Greg Woolsey 2017-02-14 18:38:39 UTC
I have a user that needs this, so I'll work on it.  The code here shows what needs to happen, but it should be abstracted as Nick says in his comment, and reused between COUNTIFS() and SUMIFS().

With the unit test, which I've verified, it shouldn't be hard.
Comment 5 Greg Woolsey 2017-02-14 22:07:12 UTC
Fixed with commit r1783037.  Created a base class for COUNTIFS() and SUMIFS() to reuse the common code (almost everything).
Comment 6 Sebastiaan Blommers 2017-03-08 08:46:26 UTC
(In reply to Greg Woolsey from comment #5)
> Fixed with commit r1783037.  Created a base class for COUNTIFS() and
> SUMIFS() to reuse the common code (almost everything).

I am very very happy with this commit :-)