Skip to content

Commit

Permalink
Fix for Issue 2158 (AverageIf Calculation Problem) (#2160)
Browse files Browse the repository at this point in the history
* Improve Identification of Samples in Coverage Report

The Phpunit coverage report currently contains bullet items like `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "49"`. This extremely simple change takes advantage of Phpunit's ability to accept an array with keys which are either strings or integers, by using the sample filenames as the array keys rather than sequential but otherwise meaningless integers (e.g. `49` in the earlier cited item). The bullet item will now read `PhpOffice\PhpSpreadsheetTests\Helper\SampleTest\testSample with data set "Basic/38_Clone_worksheet.php"`.

* Fix for Issue 2158 (AverageIf Calculation Problem)

Issue #2158 reports an error calculating AverageIf because a function returns null rather than a string. There turn out to be several components to this problem:
- The nominal fix to the problem is to add some null-to-nullstring coercion in DatabaseAbstract.
- This fixes the error, but does not necessarily lead to the correct result because buildQuery treats values of null and null-string identically, whereas Excel does not. So change that to treat null-string as any other string.
- But that doesn't lead to the correct result either. That's because Functions/ifCondition recognizes a null string, but then continues to (over-)process it until it returns the wrong result. Fix this problem in conjunction with the other two, and we finally get the correct result.

A new unit test is added for AVERAGEIF, and new test cases are added for SUMIF. In each case, there are complementary tests for conditions of null and null-string, and the results agree with Excel. There may or may not be value in adding new tests to other functions, and I will be glad to do so for any functions which you care to identify, but no existing tests broke as a result of these changes.
  • Loading branch information
oleibman committed Jun 15, 2021
1 parent 4f06d84 commit 1e74282
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 6 deletions.
6 changes: 3 additions & 3 deletions src/PhpSpreadsheet/Calculation/Database/DatabaseAbstract.php
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ private static function buildQuery(array $criteriaNames, array $criteria): strin
foreach ($criteria as $key => $criterion) {
foreach ($criterion as $field => $value) {
$criterionName = $criteriaNames[$field];
if ($value !== null && $value !== '') {
if ($value !== null) {
$condition = self::buildCondition($value, $criterionName);
$baseQuery[$key][] = $condition;
}
Expand All @@ -104,12 +104,12 @@ private static function buildQuery(array $criteriaNames, array $criteria): strin

$rowQuery = array_map(
function ($rowValue) {
return (count($rowValue) > 1) ? 'AND(' . implode(',', $rowValue) . ')' : $rowValue[0];
return (count($rowValue) > 1) ? 'AND(' . implode(',', $rowValue) . ')' : ($rowValue[0] ?? '');
},
$baseQuery
);

return (count($rowQuery) > 1) ? 'OR(' . implode(',', $rowQuery) . ')' : $rowQuery[0];
return (count($rowQuery) > 1) ? 'OR(' . implode(',', $rowQuery) . ')' : ($rowQuery[0] ?? '');
}

private static function buildCondition($criterion, string $criterionName): string
Expand Down
2 changes: 1 addition & 1 deletion src/PhpSpreadsheet/Calculation/Functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ public static function ifCondition($condition)
$condition = self::flattenSingleValue($condition);

if ($condition === '') {
$condition = '=""';
return '=""';
}
if (!is_string($condition) || !in_array($condition[0], ['>', '<', '='])) {
$condition = self::operandSpecialHandling($condition);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ public function testSUMIF2($expectedResult, array $array1, $condition, ?array $a
$sheet->fromArray($array1, null, 'A1', true);
$maxARow = count($array1);
$firstArg = "A1:A$maxARow";
//$secondArg = is_string($condition) ? "\"$condition\"" : $condition;
$sheet->getCell('B1')->setValue($condition);
$this->setCell('B1', $condition);
$secondArg = 'B1';
if (empty($array2)) {
$sheet->getCell('D1')->setValue("=SUMIF($firstArg, $secondArg)");
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\Statistical;

use PhpOffice\PhpSpreadsheet\Spreadsheet;
use PHPUnit\Framework\TestCase;

class AverageIf2Test extends TestCase
{
public function testAVERAGEIF(): void
{
$table = [
['n', 1],
['', 2],
['y', 3],
['', 4],
['n', 5],
['n', 6],
['n', 7],
['', 8],
[null, 9],
];

$spreadsheet = new Spreadsheet();
$sheet = $spreadsheet->getActiveSheet();
$sheet->fromArray($table, null, 'A1', true);
$sheet->getCell('C1')->setValue('=AVERAGEIF(A:A,"",B:B)');
$sheet->getCell('C2')->setValue('=AVERAGEIF(A:A,,B:B)');
$sheet->getCell('C3')->setValue('=AVERAGEIF(A:A,"n",B:B)');
$sheet->getCell('C4')->setValue('=AVERAGEIF(A:A,"y",B:B)');
$sheet->getCell('C5')->setValue('=AVERAGEIF(A:A,"x",B:B)');

self::assertEqualsWithDelta(5.75, $sheet->getCell('C1')->getCalculatedValue(), 1E-12);
self::assertEquals('#DIV/0!', $sheet->getCell('C2')->getCalculatedValue());
self::assertEqualsWithDelta(4.75, $sheet->getCell('C3')->getCalculatedValue(), 1E-12);
self::assertEqualsWithDelta(3, $sheet->getCell('C4')->getCalculatedValue(), 1E-12);
self::assertEquals('#DIV/0!', $sheet->getCell('C5')->getCalculatedValue());
$spreadsheet->disconnectWorksheets();
}
}
18 changes: 18 additions & 0 deletions tests/data/Calculation/MathTrig/SUMIF.php
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,22 @@
'North ?',
[[36693], [22100], ['=3/0'], [34440], [29889], [50090], [32080], [45500]],
],
[
23,
[['n'], [''], ['y'], [''], ['n'], ['n'], ['n'], [''], []],
'',
[[1], [2], [3], [4], [5], [6], [7], [8], [9]],
],
[
0,
[['n'], [''], ['y'], [''], ['n'], ['n'], ['n'], [''], []],
null,
[[1], [2], [3], [4], [5], [6], [7], [8], [9]],
],
[
19,
[['n'], [''], ['y'], [''], ['n'], ['n'], ['n'], [''], []],
'n',
[[1], [2], [3], [4], [5], [6], [7], [8], [9]],
],
];

0 comments on commit 1e74282

Please sign in to comment.