Bug 51498 - Countif.evaluate(int cmpResult) wrong return value for GE
Summary: Countif.evaluate(int cmpResult) wrong return value for GE
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: POI Overall (show other bugs)
Version: 3.8-dev
Hardware: PC All
: P2 trivial (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 19:04 UTC by captainwucher
Modified: 2012-03-14 14:28 UTC (History)
1 user (show)



Attachments
JUnit test case (1.12 KB, text/plain)
2012-02-08 15:16 UTC, Maciej
Details
Test workbook for JUnit test case (17.00 KB, application/vnd.ms-excel)
2012-02-08 15:17 UTC, Maciej
Details
Correct test workbook for JUnit test cases (13.50 KB, application/vnd.ms-excel)
2012-02-08 17:06 UTC, Maciej
Details
proposed patch (2.11 KB, patch)
2012-02-09 18:59 UTC, Yegor Kozlov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description captainwucher 2011-07-11 19:04:42 UTC
In org.apache.poi.ss.formula.functions.Countif, method 
public boolean evaluate(int cmpResult) (LINE 127)

The retunvalue for GE is wrong, it is the same like LE.

switch (_code) {			
  ...
  case LE: return cmpResult <= 0;
  ...
  case GE: return cmpResult <= 0; (LINE 136)
}

wrong:  case GE: return cmpResult <= 0
right:  case GE: return cmpResult >= 0
Comment 1 Nick Burch 2011-07-12 11:17:23 UTC
Could you suggest a couple of simple formulas we could use for a unit test for this? Ideally say 4 formulas, some of which pass currently and at least one that fails until the patch is applied
Comment 2 Maciej 2012-02-08 15:16:27 UTC
Created attachment 28288 [details]
JUnit test case
Comment 3 Maciej 2012-02-08 15:17:31 UTC
Created attachment 28289 [details]
Test workbook for JUnit test case
Comment 4 Maciej 2012-02-08 15:22:18 UTC
Nick,
just stacked up on same bug.

Please find attached requested test case and test work book.
It reveals another bug with "<>" NE operator - Excel counts blank values in range as not equal to any value, so formula "<>5" applied to values ["", "", "5"] should return 2, not 0 as current implementation does.

Regards,
Maciej.
Comment 5 Maciej 2012-02-08 15:23:21 UTC
Nick,
just stacked up on same bug. It is reported to be solved with https://issues.apache.org/bugzilla/show_bug.cgi?id=51809

Please find attached requested test case and test work book.
It reveals another bug with "<>" NE operator - Excel counts blank values in range as not equal to any value, so formula "<>5" applied to values ["", "", "5"] should return 2, not 0 as current implementation does.

Regards,
Maciej.
Comment 6 Nick Burch 2012-02-08 15:47:55 UTC
Are you sure that's the correct workbook? Only it doesn't seem to have any countif formulas in it...
Comment 7 Maciej 2012-02-08 17:06:41 UTC
Created attachment 28290 [details]
Correct test workbook for JUnit test cases

I put wrong file by mistake. Thanks for checking this.
Comment 8 Nick Burch 2012-02-08 17:21:59 UTC
OpenOffice seems to be coming up with different answers to excel on countif for empty cells - it agrees with POI on the lower value (eg 29 rather than 34 for D4 in your example spreadsheet)
Comment 9 Nick Burch 2012-02-08 17:23:37 UTC
Unit test added in r1241993, though skipping the one row that Excel, POI and OpenOffice disagree on. Need to figure out exactly why and what the correct answer is, then fix/report bugs as appropriate
Comment 10 Yegor Kozlov 2012-02-09 18:59:31 UTC
Created attachment 28294 [details]
proposed patch

I fugured out how to fix it. The condition "<>5" instantiates Countif.NumberMatcher and it didn't handle blank values. See the proposed patch. 

My fix works only for conditions with numbers. Maciej, would you like to check how Countif works for booleans and strings: the conditions "<>TRUE" and "<>'ABC'" should agree with Excel too.
  
Yegor
Comment 11 Yegor Kozlov 2012-02-11 13:23:44 UTC
Should be fixed in r1243054

The fix applies to number, boolean and string predicates - they all had troubles with evaluating blank cells: "<>5", "<>"ABC"", "<>FALSE" returned wrong result if the range contained blank cells. 

Another problem cropped up with evaluating string predicates: it appears that string criteria in COUNTIF are case insensitive; for example, the string "apples" and the string "APPLES" will match the same cells. This is also true for pattern matching: "ABC*" and "abc*" match the same cells. Before r1243054 POI did everything in the case-sensitive mode which didn't agree with Excel. 

Yegor