From c3d24e5bb5bbf62c29f0ab4eef3a8d2eb22ce749 Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Fri, 22 Aug 2025 16:16:55 -0700 Subject: [PATCH] WIP Some Additional Support for Intersection and Union Add some additional support for Intersection and Union in the Calculation Engine. This allows me to reinstate 2 tests which were formerly skipped, without breaking any existing tests. There are almost certainly edge cases which I haven't thought of yet; I will leave this PR as a draft for several weeks before moving it formward. This change gives some opportunities for users to go wrong. If you place the following formula in a cell: ``` =B1:B8 A7:D7 ``` Excel will treat the space as an intersection operator, and will return the value in B7, which is where the ranges intersect. PhpSpreadsheet will do so as well. But, if you use the following formula: ``` =B1:B8,A7:D7 ``` Excel will return `#VALUE!`. This seems like something they forgot to take care of when adding dynamic arrays. The comma should be interpreted as a union operator, and PhpSpreadsheet will now return the union of the ranges. It seems very difficult for PhpSpreadsheet to return an error when the formula seems easily evaluated. Furthermore, if you use the following formula: ``` =SUM(B1:B8,A7:D7) ``` Excel evaluates it as you would expect, summing the union of the ranges. So does PhpSpreadsheet. I guess there's no rule requiring Excel to be consistent, but ... Another way that users might go wrong is by actually entering the union or intersection symbols in a formula rather than comma or space. Excel will not allow this; PhpSpreadsheet needs to change comma or space to the appropriate symbol in order for the rest of this PR to work properly. By the time the parser gets to it, it can't tell whether the symbol was part of the cell's original formula or if PhpSpreadsheet substituted it, so it just has to permit it. I do not expect many users to fall afoul of this problem, at least not more than once. Just by way of explanation, the reason why PhpSpreadsheet has to make the substitution is because intersection has a higher priority than union, just as multiplication has a higher priority than addition. So, if you don't change the symbols beforehand, you may wind up figuring out that you need to perform an intersection too late - a lower-priority union may have already taken place. --- .../Calculation/Calculation.php | 30 ++++++++++++- src/PhpSpreadsheet/Cell/Cell.php | 31 +++++++++++++ .../Calculation/Engine/RangeTest.php | 45 ++++++++++++++----- .../Worksheet/Table/Issue3659Test.php | 13 +++--- 4 files changed, 100 insertions(+), 19 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index c57724926..1f6a84108 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -1095,6 +1095,10 @@ class Calculation extends CalculationLocale $this->branchPruner->initialiseForLoop(); $opCharacter = $formula[$index]; // Get the first character of the value at the current index position + if ($opCharacter === "\xe2") { // intersection or union + $opCharacter .= $formula[++$index]; + $opCharacter .= $formula[++$index]; + } // Check for two-character operators (e.g. >=, <=, <>) if ((isset(self::COMPARISON_OPERATORS[$opCharacter])) && (strlen($formula) > $index) && isset($formula[$index + 1], self::COMPARISON_OPERATORS[$formula[$index + 1]])) { @@ -1115,7 +1119,7 @@ class Calculation extends CalculationLocale ++$index; } elseif ($opCharacter === '+' && !$expectingOperator) { // Positive (unary plus rather than binary operator plus) can be discarded? ++$index; // Drop the redundant plus symbol - } elseif ((($opCharacter === '~') || ($opCharacter === '∩') || ($opCharacter === '∪')) && (!$isOperandOrFunction)) { + } elseif ((($opCharacter === '~') /*|| ($opCharacter === '∩') || ($opCharacter === '∪')*/) && (!$isOperandOrFunction)) { // We have to explicitly deny a tilde, union or intersect because they are legal return $this->raiseFormulaError("Formula Error: Illegal character '~'"); // on the stack but not in the input expression } elseif ((isset(self::CALCULATION_OPERATORS[$opCharacter]) || $isOperandOrFunction) && $expectingOperator) { // Are we putting an operator on the stack? @@ -1232,7 +1236,13 @@ class Calculation extends CalculationLocale // because at least the braces are paired up (at this stage in the formula) // MS Excel allows this if the content is cell references; but doesn't allow actual values, // but at this point, we can't differentiate (so allow both) - return $this->raiseFormulaError('Formula Error: Unexpected ,'); + //return $this->raiseFormulaError('Formula Error: Unexpected ,'); + $stack->push('Binary Operator', '∪'); + + ++$index; + $expectingOperator = false; + + continue; } /** @var array $d */ @@ -1927,6 +1937,14 @@ class Calculation extends CalculationLocale $stack->push('Value', $cellIntersect, $cellRef); } + break; + case '∪': // union + /** @var mixed[][] $operand1 */ + /** @var mixed[][] $operand2 */ + $cellUnion = array_merge($operand1, $operand2); + $this->debugLog->writeDebugLog('Evaluation Result is %s', $this->showTypeDetails($cellUnion)); + $stack->push('Value', $cellUnion, 'A1'); + break; } } elseif (($token === '~') || ($token === '%')) { @@ -2792,6 +2810,14 @@ class Calculation extends CalculationLocale $definedNameValue = $namedRange->getValue(); $definedNameType = $namedRange->isFormula() ? 'Formula' : 'Range'; + if ($definedNameType === 'Range') { + if (preg_match('/^(.*!)?(.*)$/', $definedNameValue, $matches) === 1) { + $matches2 = trim($matches[2]); + $matches2 = preg_replace('/ +/', ' ∩ ', $matches2) ?? $matches2; + $matches2 = preg_replace('/,/', ' ∪ ', $matches2) ?? $matches2; + $definedNameValue = $matches[1] . $matches2; + } + } $definedNameWorksheet = $namedRange->getWorksheet(); if ($definedNameValue[0] !== '=') { diff --git a/src/PhpSpreadsheet/Cell/Cell.php b/src/PhpSpreadsheet/Cell/Cell.php index 8cf71f9d9..54bc3ac4e 100644 --- a/src/PhpSpreadsheet/Cell/Cell.php +++ b/src/PhpSpreadsheet/Cell/Cell.php @@ -453,6 +453,7 @@ class Cell implements Stringable } $newColumn = $this->getColumn(); if (is_array($result)) { + $result = self::convertSpecialArray($result); $this->formulaAttributes['t'] = 'array'; $this->formulaAttributes['ref'] = $maxCoordinate = $coordinate; $newRow = $row = $this->getRow(); @@ -582,6 +583,36 @@ class Cell implements Stringable return $this->convertDateTimeInt($this->value); } + /** + * Convert array like the following (preserve values, lose indexes): + * [ + * rowNumber1 => [colLetter1 => value, colLetter2 => value ...], + * rowNumber2 => [colLetter1 => value, colLetter2 => value ...], + * ... + * ]. + * + * @param mixed[] $array + * + * @return mixed[] + */ + private static function convertSpecialArray(array $array): array + { + $newArray = []; + foreach ($array as $rowIndex => $row) { + if (!is_int($rowIndex) || $rowIndex <= 0 || !is_array($row)) { + return $array; + } + $keys = array_keys($row); + $key0 = $keys[0] ?? ''; + if (!is_string($key0)) { + return $array; + } + $newArray[] = array_values($row); + } + + return $newArray; + } + /** * Set old calculated value (cached). * diff --git a/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php b/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php index cd161da95..0b2d36a98 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Engine/RangeTest.php @@ -13,8 +13,6 @@ use PHPUnit\Framework\TestCase; class RangeTest extends TestCase { - private string $incompleteMessage = 'Must be revisited'; - private ?Spreadsheet $spreadSheet = null; protected function getSpreadsheet(): Spreadsheet @@ -162,9 +160,6 @@ class RangeTest extends TestCase #[DataProvider('providerCompositeNamedRangeEvaluation')] public function testCompositeNamedRangeEvaluation(string $composite, int $expectedSum, int $expectedCount): void { - if ($this->incompleteMessage !== '') { - self::markTestIncomplete($this->incompleteMessage); - } $this->spreadSheet = $this->getSpreadsheet(); $workSheet = $this->spreadSheet->getActiveSheet(); @@ -182,17 +177,47 @@ class RangeTest extends TestCase public static function providerCompositeNamedRangeEvaluation(): array { return [ - // Calculation engine doesn't yet handle union ranges with overlap 'Union with overlap' => [ - 'A1:C1,A3:C3,B1:C3', - 63, + '$A$1:$C$1,$A$3:$C$3,$B$1:$C$3', + 99, 12, ], 'Union and Intersection' => [ - 'A1:C1,A3:C3 B1:C3', - 23, + '$A$1:$C$1,$A$3:$C$3 $B$1:$C$3', + 35, 5, ], ]; } + + public function testIntersectCellFormula(): void + { + $this->spreadSheet = $this->getSpreadsheet(); + + $sheet = $this->spreadSheet->getActiveSheet(); + $array = [ + [null, 'Planets', 'Lives', 'Babies'], + ['Batman', 5, 10, 4], + ['Superman', 4, 56, 34], + ['Spiderman', 23, 45, 67], + ['Hulk', 12, 34, 58], + ['Steve', 10, 34, 78], + ]; + $sheet->fromArray($array, null, 'A3', true); + $this->spreadSheet->addNamedRange(new NamedRange('Hulk', $sheet, '$B$7:$D$7')); + $this->spreadSheet->addNamedRange(new NamedRange('Planets', $sheet, '$B$4:$B$8')); + $this->spreadSheet->addNamedRange(new NamedRange('Intersect', $sheet, '$A$6:$D$6 $C$4:$C$8')); + $this->spreadSheet->addNamedRange(new NamedRange('SupHulk', $sheet, '$B$5:$D$5,$B$7:$D$7')); + + $sheet->setCellValue('F1', '=Intersect'); + $sheet->setCellValue('F2', '=SUM(SupHulk)'); + $sheet->setCellValue('F3', '=Planets Hulk'); + $sheet->setCellValue('F4', '=B4:D4 B4:C5'); + + $this->spreadSheet->returnArrayAsArray(); + self::assertSame(45, $sheet->getCell('F1')->getCalculatedValue()); + self::assertSame(198, $sheet->getCell('F2')->getCalculatedValue()); + self::assertSame(12, $sheet->getCell('F3')->getCalculatedValue()); + self::assertSame([[5, 10]], $sheet->getCell('F4')->getCalculatedValue()); + } } diff --git a/tests/PhpSpreadsheetTests/Worksheet/Table/Issue3659Test.php b/tests/PhpSpreadsheetTests/Worksheet/Table/Issue3659Test.php index 515db70d0..5df4613b6 100644 --- a/tests/PhpSpreadsheetTests/Worksheet/Table/Issue3659Test.php +++ b/tests/PhpSpreadsheetTests/Worksheet/Table/Issue3659Test.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace PhpOffice\PhpSpreadsheetTests\Worksheet\Table; -use PhpOffice\PhpSpreadsheet\Calculation\Calculation; use PhpOffice\PhpSpreadsheet\Worksheet\Table; class Issue3659Test extends SetupTeardown @@ -49,7 +48,7 @@ class Issue3659Test extends SetupTeardown public function testTableAsArray(): void { $spreadsheet = $this->getSpreadsheet(); - Calculation::getInstance($spreadsheet)->setInstanceArrayReturnType(Calculation::RETURN_ARRAY_AS_ARRAY); + $spreadsheet->returnArrayAsArray(); $sheet = $this->getSheet(); $sheet->setTitle('Feuil1'); $tableSheet = $spreadsheet->createSheet(); @@ -76,13 +75,13 @@ class Issue3659Test extends SetupTeardown $sheet->getCell('F9')->setValue('=Tableau1'); $sheet->getCell('J9')->setValue('=CONCAT(Tableau1)'); $sheet->getCell('J11')->setValue('=SUM(Tableau1[])'); - $expectedResult = [2 => ['B' => 10], ['B' => 2], ['B' => 3], ['B' => 4]]; + $expectedResult = [[10], [2], [3], [4]]; self::assertSame($expectedResult, $sheet->getCell('F1')->getCalculatedValue()); $expectedResult = [ - 2 => ['B' => 10, 'C' => 20, 'D' => null], - ['B' => 2, 'C' => null, 'D' => null], - ['B' => 3, 'C' => null, 'D' => null], - ['B' => 4, 'C' => null, 'D' => null], + [10, 20, null], + [2, null, null], + [3, null, null], + [4, null, null], ]; self::assertSame($expectedResult, $sheet->getCell('H1')->getCalculatedValue()); self::assertSame($expectedResult, $sheet->getCell('F9')->getCalculatedValue());