From 5e0919226af8fa463bb9defb4fbd9a4f8b56d2f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Tue, 27 Nov 2018 16:55:06 +0100 Subject: [PATCH 1/8] Introduce elements to identify ifs and enable better branch resolution (pruning). We tag parsed tokens to associate a branch identifier to them --- .../Calculation/Calculation.php | 206 ++++++++++++++++-- .../Calculation/Token/Stack.php | 56 ++++- src/PhpSpreadsheet/Cell/Cell.php | 1 + 3 files changed, 232 insertions(+), 31 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 4f9ef63988..c785abf529 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -66,6 +66,13 @@ class Calculation */ private $calculationCacheEnabled = true; + /** + * Used to generate unique store keys + * + * @var int + */ + private $storeKeyCounter = 0; + /** * List of operators that can be used within formulae * The true/false value indicates whether it is a binary operator or a unary operator. @@ -2710,6 +2717,7 @@ public function calculate(Cell $pCell = null) */ public function calculateCellValue(Cell $pCell = null, $resetLog = true) { + var_dump('calculateCellValue start: Working on ' . $pCell->getCoordinate()); if ($pCell === null) { return null; } @@ -2736,6 +2744,7 @@ public function calculateCellValue(Cell $pCell = null, $resetLog = true) $cellAddress = array_pop($this->cellStack); $this->spreadsheet->getSheetByName($cellAddress['sheet'])->getCell($cellAddress['cell']); } catch (\Exception $e) { + throw $e; // @todo HCK remove $cellAddress = array_pop($this->cellStack); $this->spreadsheet->getSheetByName($cellAddress['sheet'])->getCell($cellAddress['cell']); @@ -2774,6 +2783,8 @@ public function calculateCellValue(Cell $pCell = null, $resetLog = true) return Functions::NAN(); } + var_dump('calculateCellValue end: Resolving ' . $pCell->getCoordinate() . ' at ' . $result); + return $result; } @@ -3308,9 +3319,37 @@ private function _parseFormula($formula, Cell $pCell = null) // - is a negation or + is a positive operator rather than an operation $expectingOperand = false; // We use this test in syntax-checking the expression to determine whether an operand // should be null in a function call + + // HCK: support for IF branch pruning + // currently pending storeKey (last item of the storeKeysStack + $pendingStoreKey = null; + // stores a list of storeKeys (string[]) + $pendingStoreKeysStack = []; + $expectingConditionMap = []; // ['storeKey' => true, ...] + $expectingThenMap = []; // ['storeKey' => true, ...] + $expectingElseMap = []; // ['storeKey' => true, ...] + $parenthesisDepthMap = []; // ['storeKey' => 4, ...] + + // The guts of the lexical parser // Loop through the formula extracting each operator and operand in turn while (true) { + // HCK: we adapt the output item to the context (it will + // be used to limit its computation) + $currentCondition = null; + $currentOnlyIf = null; + $currentOnlyIfNot = null; + if ($expectingConditionMap[$pendingStoreKey] ?? false) { + $currentCondition = $pendingStoreKey; + } + if ($expectingThenMap[$pendingStoreKey] ?? false) { + $currentOnlyIf = $pendingStoreKey; + } + if ($expectingElseMap[$pendingStoreKey] ?? false) { + $currentOnlyIfNot = $pendingStoreKey; + } + $pendingStoreKey = end($pendingStoreKeysStack); + $opCharacter = $formula[$index]; // Get the first character of the value at the current index position if ((isset(self::$comparisonOperators[$opCharacter])) && (strlen($formula) > $index) && (isset(self::$comparisonOperators[$formula[$index + 1]]))) { $opCharacter .= $formula[++$index]; @@ -3320,10 +3359,13 @@ private function _parseFormula($formula, Cell $pCell = null) $isOperandOrFunction = preg_match($regexpMatchString, substr($formula, $index), $match); if ($opCharacter == '-' && !$expectingOperator) { // Is it a negation instead of a minus? - $stack->push('Unary Operator', '~'); // Put a negation on the stack + // Put a negation on the stack + // HCK we inject context information + $stack->push('Unary Operator', '~', null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); ++$index; // and drop the negation symbol } elseif ($opCharacter == '%' && $expectingOperator) { - $stack->push('Unary Operator', '%'); // Put a percentage on the stack + // Put a percentage on the stack + $stack->push('Unary Operator', '%', null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); ++$index; } elseif ($opCharacter == '+' && !$expectingOperator) { // Positive (unary plus rather than binary operator plus) can be discarded? ++$index; // Drop the redundant plus symbol @@ -3336,7 +3378,9 @@ private function _parseFormula($formula, Cell $pCell = null) @(self::$operatorAssociativity[$opCharacter] ? self::$operatorPrecedence[$opCharacter] < self::$operatorPrecedence[$o2['value']] : self::$operatorPrecedence[$opCharacter] <= self::$operatorPrecedence[$o2['value']])) { $output[] = $stack->pop(); // Swap operands and higher precedence operators from the stack to the output } - $stack->push('Binary Operator', $opCharacter); // Finally put our current operator onto the stack + // Finally put our current operator onto the stack + // HCK we inject context information + $stack->push('Binary Operator', $opCharacter, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); ++$index; $expectingOperator = false; } elseif ($opCharacter == ')' && $expectingOperator) { // Are we expecting to close a parenthesis? @@ -3349,6 +3393,23 @@ private function _parseFormula($formula, Cell $pCell = null) } $d = $stack->last(2); if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $d['value'], $matches)) { // Did this parenthesis just close a function? + + // HCK + if (isset($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == 0) { + // we are closing an IF( + if ($d['value'] != 'IF(') { + return $this->raiseFormulaError('Parser bug we should be in an "IF("'); + } + if ($expectingConditionMap[$pendingStoreKey]) { + return $this->raiseFormulaError('We should not be expecting a condition'); + } + $expectingThenMap[$pendingStoreKey] = false; + $expectingElseMap[$pendingStoreKey] = false; + $parenthesisDepthMap[$pendingStoreKey] -= 1; + array_pop($pendingStoreKeysStack); + unset($pendingStoreKey); + } + $functionName = $matches[1]; // Get the function name $d = $stack->pop(); $argumentCount = $d['value']; // See how many arguments there were (argument count is the next value stored on the stack) @@ -3406,9 +3467,31 @@ private function _parseFormula($formula, Cell $pCell = null) if ($argumentCountError) { return $this->raiseFormulaError("Formula Error: Wrong number of arguments for $functionName() function: $argumentCount given, " . $expectedArgumentCountString . ' expected'); } + } else { + // HCK we decrease the depth whether is it a function call or a + // parenthesis + if (isset($pendingStoreKey)) { + $parenthesisDepthMap[$pendingStoreKey] -= 1; + } } ++$index; } elseif ($opCharacter == ',') { // Is this the separator for function arguments? + + if ( + isset($pendingStoreKey) && + $parenthesisDepthMap[$pendingStoreKey] == 0 + ) { + // HCK we must step to the IF next step + if ($expectingConditionMap[$pendingStoreKey]) { + $expectingConditionMap[$pendingStoreKey] = false; + $expectingThenMap[$pendingStoreKey] = true; + } elseif ($expectingThenMap[$pendingStoreKey]) { + $expectingThenMap[$pendingStoreKey] = false; + $expectingElseMap[$pendingStoreKey] = true; + } elseif ($expectingElseMap[$pendingStoreKey]) { + return $this->raiseFormulaError('Reaching fourth argument of an IF'); + } + } while (($o2 = $stack->pop()) && $o2['value'] != '(') { // Pop off the stack back to the last ( if ($o2 === null) { return $this->raiseFormulaError('Formula Error: Unexpected ,'); @@ -3432,6 +3515,9 @@ private function _parseFormula($formula, Cell $pCell = null) $expectingOperand = true; ++$index; } elseif ($opCharacter == '(' && !$expectingOperator) { + if (isset($pendingStoreKey)) { // HCK we go deeper + $parenthesisDepthMap[$pendingStoreKey] += 1; + } $stack->push('Brace', '('); ++$index; } elseif ($isOperandOrFunction && !$expectingOperator) { // do we now have a function/variable/number? @@ -3440,16 +3526,33 @@ private function _parseFormula($formula, Cell $pCell = null) $val = $match[1]; $length = strlen($val); + if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $val, $matches)) { $val = preg_replace('/\s/u', '', $val); if (isset(self::$phpSpreadsheetFunctions[strtoupper($matches[1])]) || isset(self::$controlFunctions[strtoupper($matches[1])])) { // it's a function - $stack->push('Function', strtoupper($val)); + $valToUpper = strtoupper($val); + // HCK: here $matches[1] will contain values like IF, and $val IF( + $injectStoreKey = null; + if ($valToUpper == 'IF(') { // we handle a new if + $pendingStoreKey = $this->getUnusedStoreKey(); + $pendingStoreKeysStack[] = $pendingStoreKey; + $expectingConditionMap[$pendingStoreKey] = true; + $parenthesisDepthMap[$pendingStoreKey] = 0; + } else { // this is not a if but we good deeper + $parenthesisDepthMap[$pendingStoreKey] += 1; + } + + $stack->push('Function', $valToUpper); + // tests if the function is closed right after opening $ax = preg_match('/^\s*(\s*\))/ui', substr($formula, $index + $length), $amatch); if ($ax) { - $stack->push('Operand Count for Function ' . strtoupper($val) . ')', 0); + $stack->push('Operand Count for Function ' . $valToUpper . ')', 0); $expectingOperator = true; + + // HCK (this is a closed function we don't go deeper) + $parenthesisDepthMap[$pendingStoreKey] -= 1; } else { - $stack->push('Operand Count for Function ' . strtoupper($val) . ')', 1); + $stack->push('Operand Count for Function ' . $valToUpper . ')', 1); $expectingOperator = false; } $stack->push('Brace', '('); @@ -3477,7 +3580,12 @@ private function _parseFormula($formula, Cell $pCell = null) } } - $output[] = ['type' => 'Cell Reference', 'value' => $val, 'reference' => $val]; + // HCK patching on context + $outputItem = $stack->getStackItem('Cell Reference', $val, + $val, $currentCondition, $currentOnlyIf, + $currentOnlyIfNot); + + $output[] = $outputItem; } else { // it's a variable, constant, string, number or boolean // If the last entry on the stack was a : operator, then we may have a row or column range reference $testPrevOp = $stack->last(1); @@ -3524,7 +3632,7 @@ private function _parseFormula($formula, Cell $pCell = null) } elseif (($localeConstant = array_search(trim(strtoupper($val)), self::$localeBoolean)) !== false) { $val = self::$excelConstants[$localeConstant]; } - $details = ['type' => 'Value', 'value' => $val, 'reference' => null]; + $details = $stack->getStackItem('Value', $val, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); if ($localeConstant) { $details['localeValue'] = $localeConstant; } @@ -3580,6 +3688,7 @@ private function _parseFormula($formula, Cell $pCell = null) } } + while (($op = $stack->pop()) !== null) { // pop everything off the stack and push onto output if ((is_array($op) && $op['value'] == '(') || ($op === '(')) { return $this->raiseFormulaError("Formula Error: Expecting ')'"); // if there are any opening braces on the stack, then braces were unbalanced @@ -3617,6 +3726,9 @@ private static function dataTestReference(&$operandData) */ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) { + + var_dump('begin process stack for ' . $cellID); + if ($tokens == false) { return false; } @@ -3627,9 +3739,37 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $pCellParent = ($pCell !== null) ? $pCell->getParent() : null; $stack = new Stack(); + //var_dump($tokens);die; + // Loop through each token in turn foreach ($tokens as $tokenData) { + $token = $tokenData['value']; + // HCK skip useless keys + $storeKey = $tokenData['storeKey'] ?? null; + + if (isset($tokenData['onlyIf'])) { + $onlyIfStoreKey = $tokenData['onlyIf']; + $this->getValueFromCache($onlyIfStoreKey, $storeValue); + if (isset($storeValue) && $storeValue) { + // ... now ... how do we bypass this ? :/ + // continue; + } + } + + if (isset($tokenData['onlyIfNot'])) { + $onlyIfNotStoreKey = $tokenData['onlyIfNot']; + $this->getValueFromCache($onlyIfNotStoreKey, $storeValue); + if (isset($storeValue) && !$storeValue) { + // ... now ... how do we bypass this ? :/ + // continue; + } + } + + var_dump(''); + var_dump('processing ', $token); + var_dump(''); + // if the token is a binary operator, pop the top two values off the stack, do the operation, and push the result back on the stack if (isset(self::$binaryOperators[$token])) { // We must have two operands, error if we don't @@ -3659,8 +3799,8 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) case '<=': // Less than or Equal to case '=': // Equality case '<>': // Inequality - $this->executeBinaryComparisonOperation($cellID, $operand1, $operand2, $token, $stack); - + $result = $this->executeBinaryComparisonOperation($cellID, $operand1, $operand2, $token, $stack); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; // Binary Operators case ':': // Range @@ -3715,24 +3855,24 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) break; case '+': // Addition - $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'plusEquals', $stack); - + $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'plusEquals', $stack); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; case '-': // Subtraction - $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'minusEquals', $stack); - + $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'minusEquals', $stack); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; case '*': // Multiplication - $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayTimesEquals', $stack); - + $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayTimesEquals', $stack); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; case '/': // Division - $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayRightDivide', $stack); - + $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayRightDivide', $stack); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; case '^': // Exponential - $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'power', $stack); - + $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'power', $stack); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; case '&': // Concatenation // If either of the operands is a matrix, we need to treat them both as matrices @@ -3764,6 +3904,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $this->debugLog->writeDebugLog('Evaluation Result is ', $this->showTypeDetails($result)); $stack->push('Value', $result); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } break; case '|': // Intersect $rowIntersect = array_intersect_key($operand1, $operand2); @@ -3808,6 +3949,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } $this->debugLog->writeDebugLog('Evaluation Result is ', $this->showTypeDetails($result)); $stack->push('Value', $result); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } } else { $this->executeNumericBinaryOperation($multiplier, $arg, '*', 'arrayTimesEquals', $stack); } @@ -3881,6 +4023,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } } $stack->push('Value', $cellValue, $cellRef); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $cellValue); } // if the token is a function, pop arguments off the stack, hand them to the function, and push the result back on } elseif (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $token, $matches)) { @@ -3904,6 +4047,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $args = $argArrayVals = []; for ($i = 0; $i < $argCount; ++$i) { $arg = $stack->pop(); + var_dump($arg); $a = $argCount - $i - 1; if (($passByReference) && (isset(self::$phpSpreadsheetFunctions[$functionName]['passByReference'][$a])) && @@ -3926,6 +4070,8 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } } } + + var_dump($functionName, 'done if'); // Reverse the order of the arguments krsort($args); @@ -3956,15 +4102,18 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $this->debugLog->writeDebugLog('Evaluation Result for ', self::localeFunc($functionName), '() function call is ', $this->showTypeDetails($result)); } $stack->push('Value', self::wrapResult($result)); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } } } else { // if the token is a number, boolean, string or an Excel error, push it onto the stack if (isset(self::$excelConstants[strtoupper($token)])) { $excelConstant = strtoupper($token); $stack->push('Constant Value', self::$excelConstants[$excelConstant]); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, self::$excelConstants[$excelConstant]); } $this->debugLog->writeDebugLog('Evaluating Constant ', $excelConstant, ' as ', $this->showTypeDetails(self::$excelConstants[$excelConstant])); } elseif ((is_numeric($token)) || ($token === null) || (is_bool($token)) || ($token == '') || ($token[0] == '"') || ($token[0] == '#')) { $stack->push('Value', $token); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $token); } // if the token is a named range, push the named range name onto the stack } elseif (preg_match('/^' . self::CALCULATION_REGEXP_NAMEDRANGE . '$/i', $token, $matches)) { $namedRange = $matches[6]; @@ -3974,6 +4123,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $pCell->attach($pCellParent); $this->debugLog->writeDebugLog('Evaluation Result for named range ', $namedRange, ' is ', $this->showTypeDetails($cellValue)); $stack->push('Named Range', $cellValue, $namedRange); + if (isset($storeKey)) { $this->saveValueToCache($storeKey, $cellValue); } } else { return $this->raiseFormulaError("undefined variable '$token'"); } @@ -3986,6 +4136,8 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $output = $stack->pop(); $output = $output['value']; + var_dump('case N end process stack for ' . $cellID); + return $output; } @@ -4035,7 +4187,7 @@ private function validateBinaryOperand(&$operand, &$stack) * @param Stack $stack * @param bool $recursingArrays * - * @return bool + * @return mixed */ private function executeBinaryComparisonOperation($cellID, $operand1, $operand2, $operation, Stack &$stack, $recursingArrays = false) { @@ -4072,7 +4224,7 @@ private function executeBinaryComparisonOperation($cellID, $operand1, $operand2, // And push the result onto the stack $stack->push('Array', $result); - return true; + return $result; } // Simple validate the two operands if they are string values @@ -4188,7 +4340,7 @@ private function strcmpLowercaseFirst($str1, $str2) * @param string $matrixFunction * @param mixed $stack * - * @return bool + * @return mixed|bool */ private function executeNumericBinaryOperation($operand1, $operand2, $operation, $matrixFunction, &$stack) { @@ -4266,7 +4418,7 @@ private function executeNumericBinaryOperation($operand1, $operand2, $operation, // And push the result onto the stack $stack->push('Value', $result); - return true; + return $result; } // trigger an error, but nicely, if need be @@ -4470,4 +4622,10 @@ private function addCellReference(array $args, $passCellReference, $functionCall return $args; } + + private function getUnusedStoreKey() { + $storeKeyValue = 'storeKey-' . $this->storeKeyCounter; + $this->storeKeyCounter++; + return $storeKeyValue; + } } diff --git a/src/PhpSpreadsheet/Calculation/Token/Stack.php b/src/PhpSpreadsheet/Calculation/Token/Stack.php index b8dccf95c2..74d5293e9b 100644 --- a/src/PhpSpreadsheet/Calculation/Token/Stack.php +++ b/src/PhpSpreadsheet/Calculation/Token/Stack.php @@ -36,14 +36,27 @@ public function count() * @param mixed $type * @param mixed $value * @param mixed $reference + * @param string|null $storeKey will store the result under this alias + * @param string|null $onlyIf will only run computation if the matching + * store key is true + * @param string|null $onlyIfNot will only run computation if the matching + * store key is false + * + * */ - public function push($type, $value, $reference = null) - { - $this->stack[$this->count++] = [ - 'type' => $type, - 'value' => $value, - 'reference' => $reference, - ]; + public function push( + $type, + $value, + $reference = null, + $storeKey = null, + $onlyIf = null, + $onlyIfNot = null + ) { + $stackItem = $this->getStackItem($type, $value, $reference, $storeKey, + $onlyIf, $onlyIfNot); + + $this->stack[$this->count++] = $stackItem; + if ($type == 'Function') { $localeFunction = Calculation::localeFunc($value); if ($localeFunction != $value) { @@ -52,6 +65,35 @@ public function push($type, $value, $reference = null) } } + public function getStackItem( + $type, + $value, + $reference = null, + $storeKey = null, + $onlyIf = null, + $onlyIfNot = null + ) { + $stackItem = [ + 'type' => $type, + 'value' => $value, + 'reference' => $reference, + ]; + + if (isset($storeKey)) { + $stackItem['storeKey'] = $storeKey; + } + + if (isset($onlyIf)) { + $stackItem['onlyIf'] = $onlyIf; + } + + if (isset($onlyIfNot)) { + $stackItem['onlyIfNot'] = $onlyIfNot; + } + + return $stackItem; + } + /** * Pop the last entry from the stack. * diff --git a/src/PhpSpreadsheet/Cell/Cell.php b/src/PhpSpreadsheet/Cell/Cell.php index 416b4a9909..aec7cc0106 100644 --- a/src/PhpSpreadsheet/Cell/Cell.php +++ b/src/PhpSpreadsheet/Cell/Cell.php @@ -267,6 +267,7 @@ public function getCalculatedValue($resetLog = true) } } } catch (Exception $ex) { + throw $ex; // @todo HCK remove if (($ex->getMessage() === 'Unable to access External Workbook') && ($this->calculatedValue !== null)) { return $this->calculatedValue; // Fallback for calculations referencing external files. } From 147bd465837ec5f4804f766238b71a951728f832 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Fri, 30 Nov 2018 12:23:33 +0100 Subject: [PATCH 2/8] Working branch pruning with tests --- .../Calculation/Calculation.php | 73 ++++++++++---- .../Calculation/CalculationTest.php | 99 +++++++++++++++++++ 2 files changed, 153 insertions(+), 19 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index c785abf529..30dd4d79c5 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -2253,6 +2253,7 @@ public static function getInstance(Spreadsheet $spreadsheet = null) public function flushInstance() { $this->clearCalculationCache(); + $this->storeKeyCounter = 0; } /** @@ -3339,14 +3340,27 @@ private function _parseFormula($formula, Cell $pCell = null) $currentCondition = null; $currentOnlyIf = null; $currentOnlyIfNot = null; - if ($expectingConditionMap[$pendingStoreKey] ?? false) { + $previousStoreKey = null; + if ($expectingConditionMap[$pendingStoreKey] ?? false) { // this is a condition $currentCondition = $pendingStoreKey; + $stackDepth = count($pendingStoreKeysStack); + if ($stackDepth > 1) { // nested if + $previousStoreKey = $pendingStoreKeysStack[$stackDepth - 2]; + } } if ($expectingThenMap[$pendingStoreKey] ?? false) { $currentOnlyIf = $pendingStoreKey; + } elseif (isset($previousStoreKey)) { + if ($expectingThenMap[$previousStoreKey] ?? false) { + $currentOnlyIf = $previousStoreKey; + } } if ($expectingElseMap[$pendingStoreKey] ?? false) { $currentOnlyIfNot = $pendingStoreKey; + } elseif (isset($previousStoreKey)) { + if ($expectingElseMap[$previousStoreKey] ?? false) { + $currentOnlyIfNot = $previousStoreKey; + } } $pendingStoreKey = end($pendingStoreKeysStack); @@ -3378,6 +3392,7 @@ private function _parseFormula($formula, Cell $pCell = null) @(self::$operatorAssociativity[$opCharacter] ? self::$operatorPrecedence[$opCharacter] < self::$operatorPrecedence[$o2['value']] : self::$operatorPrecedence[$opCharacter] <= self::$operatorPrecedence[$o2['value']])) { $output[] = $stack->pop(); // Swap operands and higher precedence operators from the stack to the output } + // Finally put our current operator onto the stack // HCK we inject context information $stack->push('Binary Operator', $opCharacter, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); @@ -3392,10 +3407,17 @@ private function _parseFormula($formula, Cell $pCell = null) $output[] = $o2; } $d = $stack->last(2); + + // HCK we decrease the depth whether is it a function call or a + // parenthesis + if (isset($pendingStoreKey)) { + $parenthesisDepthMap[$pendingStoreKey] -= 1; + } + if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $d['value'], $matches)) { // Did this parenthesis just close a function? - + // HCK - if (isset($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == 0) { + if (isset($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == -1) { // we are closing an IF( if ($d['value'] != 'IF(') { return $this->raiseFormulaError('Parser bug we should be in an "IF("'); @@ -3467,12 +3489,6 @@ private function _parseFormula($formula, Cell $pCell = null) if ($argumentCountError) { return $this->raiseFormulaError("Formula Error: Wrong number of arguments for $functionName() function: $argumentCount given, " . $expectedArgumentCountString . ' expected'); } - } else { - // HCK we decrease the depth whether is it a function call or a - // parenthesis - if (isset($pendingStoreKey)) { - $parenthesisDepthMap[$pendingStoreKey] -= 1; - } } ++$index; } elseif ($opCharacter == ',') { // Is this the separator for function arguments? @@ -3542,17 +3558,18 @@ private function _parseFormula($formula, Cell $pCell = null) $parenthesisDepthMap[$pendingStoreKey] += 1; } - $stack->push('Function', $valToUpper); + $stack->push('Function', $valToUpper, null, + $currentCondition, $currentOnlyIf, $currentOnlyIfNot); // tests if the function is closed right after opening $ax = preg_match('/^\s*(\s*\))/ui', substr($formula, $index + $length), $amatch); if ($ax) { - $stack->push('Operand Count for Function ' . $valToUpper . ')', 0); + $stack->push('Operand Count for Function ' . $valToUpper . ')', 0, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); $expectingOperator = true; // HCK (this is a closed function we don't go deeper) $parenthesisDepthMap[$pendingStoreKey] -= 1; } else { - $stack->push('Operand Count for Function ' . $valToUpper . ')', 1); + $stack->push('Operand Count for Function ' . $valToUpper . ')', 1, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); $expectingOperator = false; } $stack->push('Brace', '('); @@ -3739,7 +3756,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $pCellParent = ($pCell !== null) ? $pCell->getParent() : null; $stack = new Stack(); - //var_dump($tokens);die; + $fakedForBranchPruning = []; // Loop through each token in turn foreach ($tokens as $tokenData) { @@ -3751,18 +3768,34 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) if (isset($tokenData['onlyIf'])) { $onlyIfStoreKey = $tokenData['onlyIf']; $this->getValueFromCache($onlyIfStoreKey, $storeValue); - if (isset($storeValue) && $storeValue) { - // ... now ... how do we bypass this ? :/ - // continue; + if (is_array($storeValue)) { + $wrappedItem = end($storeValue); + $storeValue = end($wrappedItem); + } + if (isset($storeValue) && !$storeValue) { + // If branching value is not true, we don't need to compute + if (!isset($fakedForBranchPruning['onlyIf-' . $onlyIfStoreKey])) { + $stack->push('Value', 'Branch pruned (only if ' . $onlyIfStoreKey . ')'); + $fakedForBranchPruning['onlyIf-' . $onlyIfStoreKey] = true; + } + continue; } } if (isset($tokenData['onlyIfNot'])) { $onlyIfNotStoreKey = $tokenData['onlyIfNot']; $this->getValueFromCache($onlyIfNotStoreKey, $storeValue); - if (isset($storeValue) && !$storeValue) { - // ... now ... how do we bypass this ? :/ - // continue; + if (is_array($storeValue)) { + $wrappedItem = end($storeValue); + $storeValue = end($wrappedItem); + } + if (isset($storeValue) && $storeValue) { + // If branching value is true, we don't need to compute + if (!isset($fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey])) { + $stack->push('Value', 'Branch pruned (only if not ' . $onlyIfNotStoreKey . ')'); + $fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey] = true; + } + continue; } } @@ -4138,6 +4171,8 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) var_dump('case N end process stack for ' . $cellID); + var_dump($tokens, $fakedForBranchPruning); + return $output; } diff --git a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php index cd77ef84db..43f3dcf82f 100644 --- a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php @@ -140,4 +140,103 @@ public function testFormulaWithOptionalArgumentsAndRequiredCellReferenceShouldPa $cell->setValue('=OFFSET(D3, -1, -2)'); self::assertEquals(5, $cell->getCalculatedValue(), 'missing arguments should be filled with null'); } + + public function testBranchPruningFormulaParsing() + { + $calculation = Calculation::getInstance(); + + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + + $sheet->fromArray( + [ + ['please +', 'please *', 'increment'], + [1, 1, 1], // sum is 3 + [3, 3, 3], // product is 27 + ] + ); + + $cell = $sheet->getCell('E5'); + + // Very simple formula + $formula = '=IF(A1="please +",B1)'; + $tokens = $calculation->parseFormula($formula); + + $foundEqualAssociatedToStoreKey = false; + $foundConditionalOnB1 = false; + foreach ($tokens as $token) { + $isBinaryOperator = $token['type'] == 'Binary Operator'; + $isEqual = $token['value'] == '='; + $correctStoreKey = ($token['storeKey'] ?? '') == 'storeKey-0'; + $correctOnlyIf = ($token['onlyIf'] ?? '') == 'storeKey-0'; + $isB1Reference = ($token['reference'] ?? '') == 'B1'; + + $foundEqualAssociatedToStoreKey = $foundEqualAssociatedToStoreKey || + ($isBinaryOperator && $isEqual && $correctStoreKey); + + $foundConditionalOnB1 = $foundConditionalOnB1 || + ($isB1Reference && $correctOnlyIf); + } + $this->assertTrue($foundEqualAssociatedToStoreKey); + $this->assertTrue($foundConditionalOnB1); + + + $calculation->flushInstance(); // resets the ids + + // + // Internal opeartio + $formula = '=IF(A1="please +",SUM(B1:B3))+IF(A2="please *",PRODUCT(C1:C3), C1)'; + $tokens = $calculation->parseFormula($formula); + + $plusGotTagged = false; + $productFunctionCorrectlyTagged = false; + foreach ($tokens as $token) { + $isBinaryOperator = $token['type'] == 'Binary Operator'; + $isPlus = $token['value'] == '+'; + $anyStoreKey = isset($token['storeKey']); + $anyOnlyIf = isset($token['onlyIf']); + $anyOnlyIfNot = isset($token['onlyIfNot']); + $plusGotTagged = $plusGotTagged || + ($isBinaryOperator && $isPlus && + ($anyStoreKey || $anyOnlyIfNot || $anyOnlyIf)); + + $isFunction = $token['type'] == 'Function'; + $isProductFunction = $token['value'] == 'PRODUCT('; + $correctOnlyIf = ($token['onlyIf'] ?? '') == 'storeKey-1'; + $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || + ($isFunction && $isProductFunction && $correctOnlyIf); + } + $this->assertFalse($plusGotTagged, + 'chaining IF( should not affect the external operators'); + $this->assertTrue($productFunctionCorrectlyTagged, + 'function nested inside if should be tagged to be processed only if parent branching requires it'); + + $calculation->flushInstance(); // resets the ids + + $formula = '=IF(A1="please +",SUM(B1:B3),1+IF(NOT(A2="please *"),C2-C1,PRODUCT(C1:C3)))'; + $tokens = $calculation->parseFormula($formula); + + $plusCorrectlyTagged = false; + $productFunctionCorrectlyTagged = false; + $notFunctionCorrectlyTagged = false; + foreach($tokens as $token) { + $value = $token['value']; + $isPlus = $value == '+'; + $isProductFunction = $value == 'PRODUCT('; + $isNotFunction = $value == 'NOT('; + $isOnlyIfNotDepth1 = $token['onlyIfNot'] == 'storeKey-1'; + $isStoreKeyDepth1 = $token['storeKey'] == 'storeKey-1'; + $isOnlyIfNotDepth0 = $token['onlyIfNot'] == 'storeKey-0'; + + $plusCorrectlyTagged = $plusCorrectlyTagged || + ($isPlus && $isOnlyIfNotDepth0); + $notFunctionCorrectlyTagged = $notFunctionCorrectlyTagged || + ($isNotFunction && $isOnlyIfNotDepth0 && $isStoreKeyDepth1); + $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || + ($isProductFunction && $isOnlyIfNotDepth1 && !$isStoreKeyDepth1 && !$isOnlyIfNotDepth0); + } + $this->assertTrue($plusCorrectlyTagged); + $this->assertTrue($productFunctionCorrectlyTagged); + $this->assertTrue($notFunctionCorrectlyTagged); + } } From 029bff22e2764a4f88cfec7fe6da2c0cee5d981d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Mon, 3 Dec 2018 19:42:06 +0100 Subject: [PATCH 3/8] Branch bruning extratests --- .../Calculation/Calculation.php | 230 ++++++++++++------ .../Calculation/Token/Stack.php | 15 ++ .../Calculation/CalculationTest.php | 99 ++++++-- 3 files changed, 258 insertions(+), 86 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 30dd4d79c5..36bd40e1c1 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -71,7 +71,9 @@ class Calculation * * @var int */ - private $storeKeyCounter = 0; + private $branchStoreKeyCounter = 0; + + private $branchPruningEnabled = false; /** * List of operators that can be used within formulae @@ -2253,7 +2255,9 @@ public static function getInstance(Spreadsheet $spreadsheet = null) public function flushInstance() { $this->clearCalculationCache(); - $this->storeKeyCounter = 0; + $this->clearBranchStore(); + $this->branchStoreKeyCounter = 0; + } /** @@ -2397,6 +2401,30 @@ public function renameCalculationCacheForWorksheet($fromWorksheetName, $toWorksh } } + /** + * Enable/disable calculation cache. + * + * @param bool $pValue + */ + public function setBranchPruningEnabled($enabled) + { + $this->branchPruningEnabled = $enabled; + } + + public function enableBranchPruning() + { + $this->setBranchPruningEnabled(true); + } + + public function disableBranchPruning() + { + $this->setBranchPruningEnabled(false); + } + + public function clearBranchStore() { + $this->branchStoreKeyCounter = 0; + } + /** * Get the currently defined locale code. * @@ -2718,7 +2746,6 @@ public function calculate(Cell $pCell = null) */ public function calculateCellValue(Cell $pCell = null, $resetLog = true) { - var_dump('calculateCellValue start: Working on ' . $pCell->getCoordinate()); if ($pCell === null) { return null; } @@ -2784,8 +2811,6 @@ public function calculateCellValue(Cell $pCell = null, $resetLog = true) return Functions::NAN(); } - var_dump('calculateCellValue end: Resolving ' . $pCell->getCoordinate() . ' at ' . $result); - return $result; } @@ -2870,8 +2895,16 @@ public function getValueFromCache($cellReference, &$cellValue) if (($this->calculationCacheEnabled) && (isset($this->calculationCache[$cellReference]))) { $this->debugLog->writeDebugLog('Retrieving value for cell ', $cellReference, ' from cache'); // Return the cached result + $cellValue = $this->calculationCache[$cellReference]; + // @todo remove + /*$v = $cellValue; + while (is_array($v)) { + $v = end($v); + } + var_dump('retrieveing ' . $cellReference . ' => ' . $v);*/ + return true; } @@ -2884,6 +2917,11 @@ public function getValueFromCache($cellReference, &$cellValue) */ public function saveValueToCache($cellReference, $cellValue) { + // @todo remove + if ($cellReference == 'Main!AC99') { + var_dump('die'); // die; + } + if ($this->calculationCacheEnabled) { $this->calculationCache[$cellReference] = $cellValue; } @@ -3341,28 +3379,31 @@ private function _parseFormula($formula, Cell $pCell = null) $currentOnlyIf = null; $currentOnlyIfNot = null; $previousStoreKey = null; - if ($expectingConditionMap[$pendingStoreKey] ?? false) { // this is a condition - $currentCondition = $pendingStoreKey; - $stackDepth = count($pendingStoreKeysStack); - if ($stackDepth > 1) { // nested if - $previousStoreKey = $pendingStoreKeysStack[$stackDepth - 2]; + $pendingStoreKey = end($pendingStoreKeysStack); + + if ($this->branchPruningEnabled) { + if ($expectingConditionMap[$pendingStoreKey] ?? false) { // this is a condition + $currentCondition = $pendingStoreKey; + $stackDepth = count($pendingStoreKeysStack); + if ($stackDepth > 1) { // nested if + $previousStoreKey = $pendingStoreKeysStack[$stackDepth - 2]; + } } - } - if ($expectingThenMap[$pendingStoreKey] ?? false) { - $currentOnlyIf = $pendingStoreKey; - } elseif (isset($previousStoreKey)) { - if ($expectingThenMap[$previousStoreKey] ?? false) { - $currentOnlyIf = $previousStoreKey; + if ($expectingThenMap[$pendingStoreKey] ?? false) { + $currentOnlyIf = $pendingStoreKey; + } elseif (isset($previousStoreKey)) { + if ($expectingThenMap[$previousStoreKey] ?? false) { + $currentOnlyIf = $previousStoreKey; + } } - } - if ($expectingElseMap[$pendingStoreKey] ?? false) { - $currentOnlyIfNot = $pendingStoreKey; - } elseif (isset($previousStoreKey)) { - if ($expectingElseMap[$previousStoreKey] ?? false) { - $currentOnlyIfNot = $previousStoreKey; + if ($expectingElseMap[$pendingStoreKey] ?? false) { + $currentOnlyIfNot = $pendingStoreKey; + } elseif (isset($previousStoreKey)) { + if ($expectingElseMap[$previousStoreKey] ?? false) { + $currentOnlyIfNot = $previousStoreKey; + } } } - $pendingStoreKey = end($pendingStoreKeysStack); $opCharacter = $formula[$index]; // Get the first character of the value at the current index position if ((isset(self::$comparisonOperators[$opCharacter])) && (strlen($formula) > $index) && (isset(self::$comparisonOperators[$formula[$index + 1]]))) { @@ -3396,6 +3437,7 @@ private function _parseFormula($formula, Cell $pCell = null) // Finally put our current operator onto the stack // HCK we inject context information $stack->push('Binary Operator', $opCharacter, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); + ++$index; $expectingOperator = false; } elseif ($opCharacter == ')' && $expectingOperator) { // Are we expecting to close a parenthesis? @@ -3410,14 +3452,14 @@ private function _parseFormula($formula, Cell $pCell = null) // HCK we decrease the depth whether is it a function call or a // parenthesis - if (isset($pendingStoreKey)) { + if (!empty($pendingStoreKey)) { $parenthesisDepthMap[$pendingStoreKey] -= 1; } if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $d['value'], $matches)) { // Did this parenthesis just close a function? // HCK - if (isset($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == -1) { + if (!empty($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == -1) { // we are closing an IF( if ($d['value'] != 'IF(') { return $this->raiseFormulaError('Parser bug we should be in an "IF("'); @@ -3494,7 +3536,7 @@ private function _parseFormula($formula, Cell $pCell = null) } elseif ($opCharacter == ',') { // Is this the separator for function arguments? if ( - isset($pendingStoreKey) && + !empty($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == 0 ) { // HCK we must step to the IF next step @@ -3525,16 +3567,16 @@ private function _parseFormula($formula, Cell $pCell = null) return $this->raiseFormulaError('Formula Error: Unexpected ,'); } $d = $stack->pop(); - $stack->push($d['type'], ++$d['value'], $d['reference']); // increment the argument count - $stack->push('Brace', '('); // put the ( back on, we'll need to pop back to it again + $stack->push($d['type'], ++$d['value'], $d['reference'], $d['storeKey'] ?? null, $d['onlyIf'] ?? null, $d['onlyIfNot'] ?? null); // increment the argument count + $stack->push('Brace', '(', null, $d['storeKey'] ?? null, $d['onlyIf'] ?? null, $d['onlyIfNot'] ?? null); // put the ( back on, we'll need to pop back to it again $expectingOperator = false; $expectingOperand = true; ++$index; } elseif ($opCharacter == '(' && !$expectingOperator) { - if (isset($pendingStoreKey)) { // HCK we go deeper + if (!empty($pendingStoreKey)) { // HCK we go deeper $parenthesisDepthMap[$pendingStoreKey] += 1; } - $stack->push('Brace', '('); + $stack->push('Brace', '(', null, $currentCondition, $currentOnlyIf, $currentOnlyIf); ++$index; } elseif ($isOperandOrFunction && !$expectingOperator) { // do we now have a function/variable/number? $expectingOperator = true; @@ -3549,13 +3591,15 @@ private function _parseFormula($formula, Cell $pCell = null) $valToUpper = strtoupper($val); // HCK: here $matches[1] will contain values like IF, and $val IF( $injectStoreKey = null; - if ($valToUpper == 'IF(') { // we handle a new if - $pendingStoreKey = $this->getUnusedStoreKey(); + if ($this->branchPruningEnabled && ($valToUpper == 'IF(')) { // we handle a new if + $pendingStoreKey = $this->getUnusedBranchStoreKey(); $pendingStoreKeysStack[] = $pendingStoreKey; $expectingConditionMap[$pendingStoreKey] = true; $parenthesisDepthMap[$pendingStoreKey] = 0; } else { // this is not a if but we good deeper - $parenthesisDepthMap[$pendingStoreKey] += 1; + if (!empty($pendingStoreKey) && array_key_exists($pendingStoreKey, $parenthesisDepthMap)) { + $parenthesisDepthMap[$pendingStoreKey] += 1; + } } $stack->push('Function', $valToUpper, null, @@ -3567,7 +3611,9 @@ private function _parseFormula($formula, Cell $pCell = null) $expectingOperator = true; // HCK (this is a closed function we don't go deeper) - $parenthesisDepthMap[$pendingStoreKey] -= 1; + if (!empty($pendingStoreKey) && array_key_exists($pendingStoreKey, $parenthesisDepthMap)) { + //$parenthesisDepthMap[$pendingStoreKey] -= 1; + } } else { $stack->push('Operand Count for Function ' . $valToUpper . ')', 1, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); $expectingOperator = false; @@ -3743,13 +3789,12 @@ private static function dataTestReference(&$operandData) */ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) { - - var_dump('begin process stack for ' . $cellID); - if ($tokens == false) { return false; } + var_dump('resolving ' . $cellID); + // If we're using cell caching, then $pCell may well be flushed back to the cache (which detaches the parent cell collection), // so we store the parent cell collection so that we can re-attach it when necessary $pCellWorksheet = ($pCell !== null) ? $pCell->getWorksheet() : null; @@ -3757,51 +3802,86 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $stack = new Stack(); $fakedForBranchPruning = []; + $branchStore = []; + + // var_dump('Tokens: ' . $this->getTokensAsString($tokens)); // Loop through each token in turn foreach ($tokens as $tokenData) { $token = $tokenData['value']; - // HCK skip useless keys + + // HCK skip useless keys, branch pruning in practice $storeKey = $tokenData['storeKey'] ?? null; + $tokenValue = $token; + while (is_array($tokenValue)) { + $tokenValue = array_pop($tokenValue); + } + + // @todo remove useless string generation + /*$stackStr = $stack . ''; + while (strlen($stackStr) < 70) { + $stackStr .= ' '; + }*/ + // var_dump($stackStr . ' ' . $cellID. ' current tok -> ' . $tokenValue); - if (isset($tokenData['onlyIf'])) { + if ($this->branchPruningEnabled && isset($tokenData['onlyIf'])) { $onlyIfStoreKey = $tokenData['onlyIf']; - $this->getValueFromCache($onlyIfStoreKey, $storeValue); + $storeValue = $branchStore[$onlyIfStoreKey] ?? null; if (is_array($storeValue)) { $wrappedItem = end($storeValue); $storeValue = end($wrappedItem); } - if (isset($storeValue) && !$storeValue) { + + if (isset($storeValue) && (($storeValue !== true) || ($storeValue === 'Pruned branch'))) { + // If branching value is not true, we don't need to compute if (!isset($fakedForBranchPruning['onlyIf-' . $onlyIfStoreKey])) { - $stack->push('Value', 'Branch pruned (only if ' . $onlyIfStoreKey . ')'); + $stack->push('Value', 'Pruned branch (only if ' . $onlyIfStoreKey . ') ' . $token); $fakedForBranchPruning['onlyIf-' . $onlyIfStoreKey] = true; } + + if (isset($storeKey)) { + // We are processing an if condition + // We cascade the pruning to the depending branches + $branchStore[$storeKey] = 'Pruned branch'; + $fakedForBranchPruning['onlyIfNot-' . $storeKey] = true; + $fakedForBranchPruning['onlyIf-' . $storeKey] = true; + } + continue; } } - if (isset($tokenData['onlyIfNot'])) { + if ($this->branchPruningEnabled && isset($tokenData['onlyIfNot'])) { $onlyIfNotStoreKey = $tokenData['onlyIfNot']; - $this->getValueFromCache($onlyIfNotStoreKey, $storeValue); + $storeValue = $branchStore[$onlyIfNotStoreKey] ?? null; if (is_array($storeValue)) { $wrappedItem = end($storeValue); $storeValue = end($wrappedItem); } - if (isset($storeValue) && $storeValue) { + if (isset($storeValue) && ($storeValue || ($storeValue === 'Pruned branch'))) { + // If branching value is true, we don't need to compute if (!isset($fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey])) { - $stack->push('Value', 'Branch pruned (only if not ' . $onlyIfNotStoreKey . ')'); + $stack->push('Value', 'Pruned branch (only if not ' . $onlyIfNotStoreKey . ') ' . $token); $fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey] = true; } + + + if (isset($storeKey)) { + // We are processing an if condition + // We cascade the pruning to the depending branches + $branchStore[$storeKey] = 'Pruned branch'; + $fakedForBranchPruning['onlyIfNot-' . $storeKey] = true; + $fakedForBranchPruning['onlyIf-' . $storeKey] = true; + } + + continue; } } - var_dump(''); - var_dump('processing ', $token); - var_dump(''); // if the token is a binary operator, pop the top two values off the stack, do the operation, and push the result back on the stack if (isset(self::$binaryOperators[$token])) { @@ -3833,7 +3913,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) case '=': // Equality case '<>': // Inequality $result = $this->executeBinaryComparisonOperation($cellID, $operand1, $operand2, $token, $stack); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; // Binary Operators case ':': // Range @@ -3889,23 +3969,23 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) break; case '+': // Addition $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'plusEquals', $stack); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; case '-': // Subtraction $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'minusEquals', $stack); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; case '*': // Multiplication $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayTimesEquals', $stack); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; case '/': // Division $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayRightDivide', $stack); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; case '^': // Exponential $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'power', $stack); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; case '&': // Concatenation // If either of the operands is a matrix, we need to treat them both as matrices @@ -3937,7 +4017,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $this->debugLog->writeDebugLog('Evaluation Result is ', $this->showTypeDetails($result)); $stack->push('Value', $result); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } break; case '|': // Intersect $rowIntersect = array_intersect_key($operand1, $operand2); @@ -3982,7 +4062,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } $this->debugLog->writeDebugLog('Evaluation Result is ', $this->showTypeDetails($result)); $stack->push('Value', $result); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } } else { $this->executeNumericBinaryOperation($multiplier, $arg, '*', 'arrayTimesEquals', $stack); } @@ -4056,7 +4136,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } } $stack->push('Value', $cellValue, $cellRef); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $cellValue); } + if (isset($storeKey)) { $branchStore[$storeKey] = $cellValue; } // if the token is a function, pop arguments off the stack, hand them to the function, and push the result back on } elseif (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $token, $matches)) { @@ -4080,7 +4160,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $args = $argArrayVals = []; for ($i = 0; $i < $argCount; ++$i) { $arg = $stack->pop(); - var_dump($arg); $a = $argCount - $i - 1; if (($passByReference) && (isset(self::$phpSpreadsheetFunctions[$functionName]['passByReference'][$a])) && @@ -4104,7 +4183,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } } - var_dump($functionName, 'done if'); // Reverse the order of the arguments krsort($args); @@ -4135,18 +4213,18 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $this->debugLog->writeDebugLog('Evaluation Result for ', self::localeFunc($functionName), '() function call is ', $this->showTypeDetails($result)); } $stack->push('Value', self::wrapResult($result)); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $result); } + if (isset($storeKey)) { $branchStore[$storeKey] = $result; } } } else { // if the token is a number, boolean, string or an Excel error, push it onto the stack if (isset(self::$excelConstants[strtoupper($token)])) { $excelConstant = strtoupper($token); $stack->push('Constant Value', self::$excelConstants[$excelConstant]); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, self::$excelConstants[$excelConstant]); } + if (isset($storeKey)) { $branchStore[$storeKey] = self::$excelConstants[$excelConstant]; } $this->debugLog->writeDebugLog('Evaluating Constant ', $excelConstant, ' as ', $this->showTypeDetails(self::$excelConstants[$excelConstant])); } elseif ((is_numeric($token)) || ($token === null) || (is_bool($token)) || ($token == '') || ($token[0] == '"') || ($token[0] == '#')) { $stack->push('Value', $token); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $token); } + if (isset($storeKey)) { $branchStore[$storeKey] = $token; } // if the token is a named range, push the named range name onto the stack } elseif (preg_match('/^' . self::CALCULATION_REGEXP_NAMEDRANGE . '$/i', $token, $matches)) { $namedRange = $matches[6]; @@ -4156,7 +4234,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $pCell->attach($pCellParent); $this->debugLog->writeDebugLog('Evaluation Result for named range ', $namedRange, ' is ', $this->showTypeDetails($cellValue)); $stack->push('Named Range', $cellValue, $namedRange); - if (isset($storeKey)) { $this->saveValueToCache($storeKey, $cellValue); } + if (isset($storeKey)) { $branchStore[$storeKey] = $cellValue; } } else { return $this->raiseFormulaError("undefined variable '$token'"); } @@ -4169,9 +4247,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $output = $stack->pop(); $output = $output['value']; - var_dump('case N end process stack for ' . $cellID); - - var_dump($tokens, $fakedForBranchPruning); + // var_dump($this->branchPruningEnabled, $output); return $output; } @@ -4349,7 +4425,7 @@ private function executeBinaryComparisonOperation($cellID, $operand1, $operand2, // And push the result onto the stack $stack->push('Value', $result); - return true; + return $result; } /** @@ -4658,9 +4734,21 @@ private function addCellReference(array $args, $passCellReference, $functionCall return $args; } - private function getUnusedStoreKey() { - $storeKeyValue = 'storeKey-' . $this->storeKeyCounter; - $this->storeKeyCounter++; + private function getUnusedBranchStoreKey() { + $storeKeyValue = 'storeKey-' . $this->branchStoreKeyCounter; + $this->branchStoreKeyCounter++; return $storeKeyValue; } + + private function getTokensAsString($tokens) { + $tokensStr = array_map(function($token) { + $value = $token['value'] ?? 'no value'; + while (is_array($value)) { + $value = array_pop($value); + } + return $value; + }, $tokens); + $str = '[ ' . implode(' | ', $tokensStr) . ' ]'; + return $str; + } } diff --git a/src/PhpSpreadsheet/Calculation/Token/Stack.php b/src/PhpSpreadsheet/Calculation/Token/Stack.php index 74d5293e9b..7b28181bd4 100644 --- a/src/PhpSpreadsheet/Calculation/Token/Stack.php +++ b/src/PhpSpreadsheet/Calculation/Token/Stack.php @@ -132,4 +132,19 @@ public function clear() $this->stack = []; $this->count = 0; } + + public function __toString() { + $str = 'Stack: '; + foreach ($this->stack as $index => $item) { + if ($index > $this->count - 1) { + break; + } + $value = $item['value'] ?? 'no value'; + while (is_array($value)) { + $value = array_pop($value); + } + $str .= $value . ' |> ' ; + } + return $str; + } } diff --git a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php index 43f3dcf82f..a3459821df 100644 --- a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php @@ -145,19 +145,6 @@ public function testBranchPruningFormulaParsing() { $calculation = Calculation::getInstance(); - $spreadsheet = new Spreadsheet(); - $sheet = $spreadsheet->getActiveSheet(); - - $sheet->fromArray( - [ - ['please +', 'please *', 'increment'], - [1, 1, 1], // sum is 3 - [3, 3, 3], // product is 27 - ] - ); - - $cell = $sheet->getCell('E5'); - // Very simple formula $formula = '=IF(A1="please +",B1)'; $tokens = $calculation->parseFormula($formula); @@ -180,11 +167,10 @@ public function testBranchPruningFormulaParsing() $this->assertTrue($foundEqualAssociatedToStoreKey); $this->assertTrue($foundConditionalOnB1); - $calculation->flushInstance(); // resets the ids // - // Internal opeartio + // Internal operation $formula = '=IF(A1="please +",SUM(B1:B3))+IF(A2="please *",PRODUCT(C1:C3), C1)'; $tokens = $calculation->parseFormula($formula); @@ -219,11 +205,13 @@ public function testBranchPruningFormulaParsing() $plusCorrectlyTagged = false; $productFunctionCorrectlyTagged = false; $notFunctionCorrectlyTagged = false; + $findOneOperandCountTagged = false; foreach($tokens as $token) { $value = $token['value']; $isPlus = $value == '+'; $isProductFunction = $value == 'PRODUCT('; $isNotFunction = $value == 'NOT('; + $isIfOperand = $token['type'] == 'Operand Count for Function IF()'; $isOnlyIfNotDepth1 = $token['onlyIfNot'] == 'storeKey-1'; $isStoreKeyDepth1 = $token['storeKey'] == 'storeKey-1'; $isOnlyIfNotDepth0 = $token['onlyIfNot'] == 'storeKey-0'; @@ -234,9 +222,90 @@ public function testBranchPruningFormulaParsing() ($isNotFunction && $isOnlyIfNotDepth0 && $isStoreKeyDepth1); $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || ($isProductFunction && $isOnlyIfNotDepth1 && !$isStoreKeyDepth1 && !$isOnlyIfNotDepth0); + $findOneOperandCountTagged = $findOneOperandCountTagged || + ($isIfOperand && $isOnlyIfNotDepth0); } $this->assertTrue($plusCorrectlyTagged); $this->assertTrue($productFunctionCorrectlyTagged); $this->assertTrue($notFunctionCorrectlyTagged); + + + $calculation->flushInstance(); // resets the ids + + $formula = '=IF(AND(TRUE(),A1="please +"),2,3)'; + // this used to raise a parser error, we keep it even though we don't + // test the output + $tokens = $calculation->parseFormula($formula); + + $calculation->flushInstance(); // resets the ids + + $formula = '=IF(A1="flag",IF(A2<10, 0) + IF(A3<10000, 0))'; + $tokens = $calculation->parseFormula($formula); + $properlyTaggedPlus = false; + foreach ($tokens as $token) { + $isPlus = $token['value'] === '+'; + $hasOnlyIf = !empty($token['onlyIf'] ?? null); + + $properlyTaggedPlus = $properlyTaggedPlus || + ($isPlus && $hasOnlyIf); + } + $this->assertTrue($properlyTaggedPlus); + } + + /** + * @param $expectedResult + * @param $dataArray + * @param string $formula + * @param string $cellCoordinates where to put the formula + * @param string[] $shouldBeSetInCacheCells coordinates of cells that must + * be set in cache + * @param string[] $shouldNotBeSetInCacheCells coordinates of cells that must + * not be set in cache because of pruning + * @throws \PhpOffice\PhpSpreadsheet\Exception + * @dataProvider dataProviderBranchPruningFullExecution + */ + public function testBranchPruningFullExecution( + $expectedResult, + $dataArray, + string $formula, + $cellCoordinates, + $shouldBeSetInCacheCells = [], + $shouldNotBeSetInCacheCells = []) + { + $spreadsheet = new Spreadsheet(); + $sheet = $spreadsheet->getActiveSheet(); + + $sheet->fromArray($dataArray); + $cell = $sheet->getCell($cellCoordinates); + $calculation = Calculation::getInstance( + $cell->getWorksheet()->getParent() + ); + + $cell->setValue($formula); + $calculated = $cell->getCalculatedValue(); + $this->assertEquals($expectedResult, $calculated); + + // this mostly to ensure that at least some cells are cached + foreach ($shouldBeSetInCacheCells as $setCell) { + unset($inCache); + $calculation->getValueFromCache('Worksheet!' . $setCell, $inCache); + $this->assertNotEmpty($inCache); + } + + foreach ($shouldNotBeSetInCacheCells as $notSetCell) { + unset($inCache); + $calculation->getValueFromCache('Worksheet!' . $notSetCell, + $inCache); + $this->assertEmpty($inCache); + } + + $calculation->disableBranchPruning(); + $calculated = $cell->getCalculatedValue(); + $this->assertEquals($expectedResult, $calculated); } + + public function dataProviderBranchPruningFullExecution() { + return require 'data/Calculation/Calculation.php'; + } + } From edd2b43f9af75d1b4f533dd7012e00ad5559a67f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Thu, 13 Dec 2018 10:15:33 +0100 Subject: [PATCH 4/8] Other branch pruning tests --- .../Calculation/Calculation.php | 169 +++++++++--------- .../Calculation/Token/Stack.php | 14 +- .../Calculation/CalculationTest.php | 10 +- tests/data/Calculation/Calculation.php | 60 +++++++ 4 files changed, 160 insertions(+), 93 deletions(-) create mode 100644 tests/data/Calculation/Calculation.php diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 36bd40e1c1..f2b12e16e9 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -67,13 +67,13 @@ class Calculation private $calculationCacheEnabled = true; /** - * Used to generate unique store keys + * Used to generate unique store keys. * * @var int */ private $branchStoreKeyCounter = 0; - private $branchPruningEnabled = false; + private $branchPruningEnabled = true; /** * List of operators that can be used within formulae @@ -2257,7 +2257,6 @@ public function flushInstance() $this->clearCalculationCache(); $this->clearBranchStore(); $this->branchStoreKeyCounter = 0; - } /** @@ -2405,6 +2404,7 @@ public function renameCalculationCacheForWorksheet($fromWorksheetName, $toWorksh * Enable/disable calculation cache. * * @param bool $pValue + * @param mixed $enabled */ public function setBranchPruningEnabled($enabled) { @@ -2421,7 +2421,8 @@ public function disableBranchPruning() $this->setBranchPruningEnabled(false); } - public function clearBranchStore() { + public function clearBranchStore() + { $this->branchStoreKeyCounter = 0; } @@ -2772,7 +2773,6 @@ public function calculateCellValue(Cell $pCell = null, $resetLog = true) $cellAddress = array_pop($this->cellStack); $this->spreadsheet->getSheetByName($cellAddress['sheet'])->getCell($cellAddress['cell']); } catch (\Exception $e) { - throw $e; // @todo HCK remove $cellAddress = array_pop($this->cellStack); $this->spreadsheet->getSheetByName($cellAddress['sheet'])->getCell($cellAddress['cell']); @@ -2898,13 +2898,6 @@ public function getValueFromCache($cellReference, &$cellValue) $cellValue = $this->calculationCache[$cellReference]; - // @todo remove - /*$v = $cellValue; - while (is_array($v)) { - $v = end($v); - } - var_dump('retrieveing ' . $cellReference . ' => ' . $v);*/ - return true; } @@ -2917,11 +2910,6 @@ public function getValueFromCache($cellReference, &$cellValue) */ public function saveValueToCache($cellReference, $cellValue) { - // @todo remove - if ($cellReference == 'Main!AC99') { - var_dump('die'); // die; - } - if ($this->calculationCacheEnabled) { $this->calculationCache[$cellReference] = $cellValue; } @@ -3359,7 +3347,7 @@ private function _parseFormula($formula, Cell $pCell = null) $expectingOperand = false; // We use this test in syntax-checking the expression to determine whether an operand // should be null in a function call - // HCK: support for IF branch pruning + // IF branch pruning // currently pending storeKey (last item of the storeKeysStack $pendingStoreKey = null; // stores a list of storeKeys (string[]) @@ -3369,11 +3357,10 @@ private function _parseFormula($formula, Cell $pCell = null) $expectingElseMap = []; // ['storeKey' => true, ...] $parenthesisDepthMap = []; // ['storeKey' => 4, ...] - // The guts of the lexical parser // Loop through the formula extracting each operator and operand in turn while (true) { - // HCK: we adapt the output item to the context (it will + // Branch pruning: we adapt the output item to the context (it will // be used to limit its computation) $currentCondition = null; $currentOnlyIf = null; @@ -3415,7 +3402,6 @@ private function _parseFormula($formula, Cell $pCell = null) if ($opCharacter == '-' && !$expectingOperator) { // Is it a negation instead of a minus? // Put a negation on the stack - // HCK we inject context information $stack->push('Unary Operator', '~', null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); ++$index; // and drop the negation symbol } elseif ($opCharacter == '%' && $expectingOperator) { @@ -3435,7 +3421,6 @@ private function _parseFormula($formula, Cell $pCell = null) } // Finally put our current operator onto the stack - // HCK we inject context information $stack->push('Binary Operator', $opCharacter, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); ++$index; @@ -3450,15 +3435,14 @@ private function _parseFormula($formula, Cell $pCell = null) } $d = $stack->last(2); - // HCK we decrease the depth whether is it a function call or a - // parenthesis + // Branch pruning we decrease the depth whether is it a function + // call or a parenthesis if (!empty($pendingStoreKey)) { $parenthesisDepthMap[$pendingStoreKey] -= 1; } if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $d['value'], $matches)) { // Did this parenthesis just close a function? - // HCK if (!empty($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == -1) { // we are closing an IF( if ($d['value'] != 'IF(') { @@ -3539,7 +3523,7 @@ private function _parseFormula($formula, Cell $pCell = null) !empty($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == 0 ) { - // HCK we must step to the IF next step + // We must go to the IF next argument if ($expectingConditionMap[$pendingStoreKey]) { $expectingConditionMap[$pendingStoreKey] = false; $expectingThenMap[$pendingStoreKey] = true; @@ -3573,7 +3557,7 @@ private function _parseFormula($formula, Cell $pCell = null) $expectingOperand = true; ++$index; } elseif ($opCharacter == '(' && !$expectingOperator) { - if (!empty($pendingStoreKey)) { // HCK we go deeper + if (!empty($pendingStoreKey)) { // Branch pruning: we go deeper $parenthesisDepthMap[$pendingStoreKey] += 1; } $stack->push('Brace', '(', null, $currentCondition, $currentOnlyIf, $currentOnlyIf); @@ -3584,12 +3568,12 @@ private function _parseFormula($formula, Cell $pCell = null) $val = $match[1]; $length = strlen($val); - if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $val, $matches)) { $val = preg_replace('/\s/u', '', $val); if (isset(self::$phpSpreadsheetFunctions[strtoupper($matches[1])]) || isset(self::$controlFunctions[strtoupper($matches[1])])) { // it's a function $valToUpper = strtoupper($val); - // HCK: here $matches[1] will contain values like IF, and $val IF( + // here $matches[1] will contain values like "IF" + // and $val "IF(" $injectStoreKey = null; if ($this->branchPruningEnabled && ($valToUpper == 'IF(')) { // we handle a new if $pendingStoreKey = $this->getUnusedBranchStoreKey(); @@ -3609,11 +3593,6 @@ private function _parseFormula($formula, Cell $pCell = null) if ($ax) { $stack->push('Operand Count for Function ' . $valToUpper . ')', 0, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); $expectingOperator = true; - - // HCK (this is a closed function we don't go deeper) - if (!empty($pendingStoreKey) && array_key_exists($pendingStoreKey, $parenthesisDepthMap)) { - //$parenthesisDepthMap[$pendingStoreKey] -= 1; - } } else { $stack->push('Operand Count for Function ' . $valToUpper . ')', 1, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); $expectingOperator = false; @@ -3643,7 +3622,6 @@ private function _parseFormula($formula, Cell $pCell = null) } } - // HCK patching on context $outputItem = $stack->getStackItem('Cell Reference', $val, $val, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); @@ -3751,7 +3729,6 @@ private function _parseFormula($formula, Cell $pCell = null) } } - while (($op = $stack->pop()) !== null) { // pop everything off the stack and push onto output if ((is_array($op) && $op['value'] == '(') || ($op === '(')) { return $this->raiseFormulaError("Formula Error: Expecting ')'"); // if there are any opening braces on the stack, then braces were unbalanced @@ -3793,38 +3770,23 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) return false; } - var_dump('resolving ' . $cellID); - // If we're using cell caching, then $pCell may well be flushed back to the cache (which detaches the parent cell collection), // so we store the parent cell collection so that we can re-attach it when necessary $pCellWorksheet = ($pCell !== null) ? $pCell->getWorksheet() : null; $pCellParent = ($pCell !== null) ? $pCell->getParent() : null; $stack = new Stack(); + // Stores branches that have been pruned $fakedForBranchPruning = []; + // help us to know when pruning ['branchTestId' => true/false] $branchStore = []; - // var_dump('Tokens: ' . $this->getTokensAsString($tokens)); - // Loop through each token in turn foreach ($tokens as $tokenData) { - $token = $tokenData['value']; - // HCK skip useless keys, branch pruning in practice + // Branch pruning: skip useless resolutions $storeKey = $tokenData['storeKey'] ?? null; - $tokenValue = $token; - while (is_array($tokenValue)) { - $tokenValue = array_pop($tokenValue); - } - - // @todo remove useless string generation - /*$stackStr = $stack . ''; - while (strlen($stackStr) < 70) { - $stackStr .= ' '; - }*/ - // var_dump($stackStr . ' ' . $cellID. ' current tok -> ' . $tokenValue); - if ($this->branchPruningEnabled && isset($tokenData['onlyIf'])) { $onlyIfStoreKey = $tokenData['onlyIf']; $storeValue = $branchStore[$onlyIfStoreKey] ?? null; @@ -3833,7 +3795,9 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $storeValue = end($wrappedItem); } - if (isset($storeValue) && (($storeValue !== true) || ($storeValue === 'Pruned branch'))) { + if (isset($storeValue) && (($storeValue !== true) + || ($storeValue === 'Pruned branch')) + ) { // If branching value is not true, we don't need to compute if (!isset($fakedForBranchPruning['onlyIf-' . $onlyIfStoreKey])) { @@ -3860,7 +3824,9 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $wrappedItem = end($storeValue); $storeValue = end($wrappedItem); } - if (isset($storeValue) && ($storeValue || ($storeValue === 'Pruned branch'))) { + if (isset($storeValue) && ($storeValue + || ($storeValue === 'Pruned branch')) + ) { // If branching value is true, we don't need to compute if (!isset($fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey])) { @@ -3868,7 +3834,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey] = true; } - if (isset($storeKey)) { // We are processing an if condition // We cascade the pruning to the depending branches @@ -3877,12 +3842,10 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $fakedForBranchPruning['onlyIf-' . $storeKey] = true; } - continue; } } - // if the token is a binary operator, pop the top two values off the stack, do the operation, and push the result back on the stack if (isset(self::$binaryOperators[$token])) { // We must have two operands, error if we don't @@ -3913,7 +3876,10 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) case '=': // Equality case '<>': // Inequality $result = $this->executeBinaryComparisonOperation($cellID, $operand1, $operand2, $token, $stack); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; // Binary Operators case ':': // Range @@ -3969,23 +3935,38 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) break; case '+': // Addition $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'plusEquals', $stack); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; case '-': // Subtraction $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'minusEquals', $stack); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; case '*': // Multiplication $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayTimesEquals', $stack); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; case '/': // Division $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'arrayRightDivide', $stack); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; case '^': // Exponential $result = $this->executeNumericBinaryOperation($operand1, $operand2, $token, 'power', $stack); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; case '&': // Concatenation // If either of the operands is a matrix, we need to treat them both as matrices @@ -4017,7 +3998,10 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $this->debugLog->writeDebugLog('Evaluation Result is ', $this->showTypeDetails($result)); $stack->push('Value', $result); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } + break; case '|': // Intersect $rowIntersect = array_intersect_key($operand1, $operand2); @@ -4062,7 +4046,9 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } $this->debugLog->writeDebugLog('Evaluation Result is ', $this->showTypeDetails($result)); $stack->push('Value', $result); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } } else { $this->executeNumericBinaryOperation($multiplier, $arg, '*', 'arrayTimesEquals', $stack); } @@ -4136,10 +4122,20 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) } } $stack->push('Value', $cellValue, $cellRef); - if (isset($storeKey)) { $branchStore[$storeKey] = $cellValue; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $cellValue; + } - // if the token is a function, pop arguments off the stack, hand them to the function, and push the result back on + // if the token is a function, pop arguments off the stack, hand them to the function, and push the result back on } elseif (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $token, $matches)) { + if (($cellID == 'AC99') || (isset($pCell) && $pCell->getCoordinate() == 'AC99')) { + if (defined('RESOLVING')) { + define('RESOLVING2', true); + } else { + define('RESOLVING', true); + } + } + $functionName = $matches[1]; $argCount = $stack->pop(); $argCount = $argCount['value']; @@ -4213,19 +4209,25 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $this->debugLog->writeDebugLog('Evaluation Result for ', self::localeFunc($functionName), '() function call is ', $this->showTypeDetails($result)); } $stack->push('Value', self::wrapResult($result)); - if (isset($storeKey)) { $branchStore[$storeKey] = $result; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $result; + } } } else { // if the token is a number, boolean, string or an Excel error, push it onto the stack if (isset(self::$excelConstants[strtoupper($token)])) { $excelConstant = strtoupper($token); $stack->push('Constant Value', self::$excelConstants[$excelConstant]); - if (isset($storeKey)) { $branchStore[$storeKey] = self::$excelConstants[$excelConstant]; } + if (isset($storeKey)) { + $branchStore[$storeKey] = self::$excelConstants[$excelConstant]; + } $this->debugLog->writeDebugLog('Evaluating Constant ', $excelConstant, ' as ', $this->showTypeDetails(self::$excelConstants[$excelConstant])); } elseif ((is_numeric($token)) || ($token === null) || (is_bool($token)) || ($token == '') || ($token[0] == '"') || ($token[0] == '#')) { $stack->push('Value', $token); - if (isset($storeKey)) { $branchStore[$storeKey] = $token; } - // if the token is a named range, push the named range name onto the stack + if (isset($storeKey)) { + $branchStore[$storeKey] = $token; + } + // if the token is a named range, push the named range name onto the stack } elseif (preg_match('/^' . self::CALCULATION_REGEXP_NAMEDRANGE . '$/i', $token, $matches)) { $namedRange = $matches[6]; $this->debugLog->writeDebugLog('Evaluating Named Range ', $namedRange); @@ -4234,7 +4236,9 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $pCell->attach($pCellParent); $this->debugLog->writeDebugLog('Evaluation Result for named range ', $namedRange, ' is ', $this->showTypeDetails($cellValue)); $stack->push('Named Range', $cellValue, $namedRange); - if (isset($storeKey)) { $branchStore[$storeKey] = $cellValue; } + if (isset($storeKey)) { + $branchStore[$storeKey] = $cellValue; + } } else { return $this->raiseFormulaError("undefined variable '$token'"); } @@ -4247,8 +4251,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $output = $stack->pop(); $output = $output['value']; - // var_dump($this->branchPruningEnabled, $output); - return $output; } @@ -4451,7 +4453,7 @@ private function strcmpLowercaseFirst($str1, $str2) * @param string $matrixFunction * @param mixed $stack * - * @return mixed|bool + * @return bool|mixed */ private function executeNumericBinaryOperation($operand1, $operand2, $operation, $matrixFunction, &$stack) { @@ -4734,21 +4736,26 @@ private function addCellReference(array $args, $passCellReference, $functionCall return $args; } - private function getUnusedBranchStoreKey() { + private function getUnusedBranchStoreKey() + { $storeKeyValue = 'storeKey-' . $this->branchStoreKeyCounter; - $this->branchStoreKeyCounter++; + ++$this->branchStoreKeyCounter; + return $storeKeyValue; } - private function getTokensAsString($tokens) { - $tokensStr = array_map(function($token) { + private function getTokensAsString($tokens) + { + $tokensStr = array_map(function ($token) { $value = $token['value'] ?? 'no value'; while (is_array($value)) { $value = array_pop($value); } + return $value; }, $tokens); $str = '[ ' . implode(' | ', $tokensStr) . ' ]'; + return $str; } } diff --git a/src/PhpSpreadsheet/Calculation/Token/Stack.php b/src/PhpSpreadsheet/Calculation/Token/Stack.php index 7b28181bd4..7592bded8f 100644 --- a/src/PhpSpreadsheet/Calculation/Token/Stack.php +++ b/src/PhpSpreadsheet/Calculation/Token/Stack.php @@ -36,13 +36,11 @@ public function count() * @param mixed $type * @param mixed $value * @param mixed $reference - * @param string|null $storeKey will store the result under this alias - * @param string|null $onlyIf will only run computation if the matching + * @param null|string $storeKey will store the result under this alias + * @param null|string $onlyIf will only run computation if the matching * store key is true - * @param string|null $onlyIfNot will only run computation if the matching + * @param null|string $onlyIfNot will only run computation if the matching * store key is false - * - * */ public function push( $type, @@ -133,7 +131,8 @@ public function clear() $this->count = 0; } - public function __toString() { + public function __toString() + { $str = 'Stack: '; foreach ($this->stack as $index => $item) { if ($index > $this->count - 1) { @@ -143,8 +142,9 @@ public function __toString() { while (is_array($value)) { $value = array_pop($value); } - $str .= $value . ' |> ' ; + $str .= $value . ' |> '; } + return $str; } } diff --git a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php index a3459821df..3b0a51b8d3 100644 --- a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php @@ -206,7 +206,7 @@ public function testBranchPruningFormulaParsing() $productFunctionCorrectlyTagged = false; $notFunctionCorrectlyTagged = false; $findOneOperandCountTagged = false; - foreach($tokens as $token) { + foreach ($tokens as $token) { $value = $token['value']; $isPlus = $value == '+'; $isProductFunction = $value == 'PRODUCT('; @@ -229,7 +229,6 @@ public function testBranchPruningFormulaParsing() $this->assertTrue($productFunctionCorrectlyTagged); $this->assertTrue($notFunctionCorrectlyTagged); - $calculation->flushInstance(); // resets the ids $formula = '=IF(AND(TRUE(),A1="please +"),2,3)'; @@ -261,10 +260,11 @@ public function testBranchPruningFormulaParsing() * be set in cache * @param string[] $shouldNotBeSetInCacheCells coordinates of cells that must * not be set in cache because of pruning + * * @throws \PhpOffice\PhpSpreadsheet\Exception * @dataProvider dataProviderBranchPruningFullExecution */ - public function testBranchPruningFullExecution( + public function testFullExecution( $expectedResult, $dataArray, string $formula, @@ -304,8 +304,8 @@ public function testBranchPruningFullExecution( $this->assertEquals($expectedResult, $calculated); } - public function dataProviderBranchPruningFullExecution() { + public function dataProviderBranchPruningFullExecution() + { return require 'data/Calculation/Calculation.php'; } - } diff --git a/tests/data/Calculation/Calculation.php b/tests/data/Calculation/Calculation.php new file mode 100644 index 0000000000..dca79ed7a0 --- /dev/null +++ b/tests/data/Calculation/Calculation.php @@ -0,0 +1,60 @@ +3,C1,0)', 'E5']; + + $dataArray4 = [ + ['noflag', 0, 0], + [127000, 0, 0], + [ 10000, 0.03, 0], + [ 20000, 0.06, 0], + [ 40000, 0.09, 0], + [ 70000, 0.12, 0], + [ 90000, 0.03, 0] + ]; + $formula2 = '=IF(A1="flag",IF(A2<10, 0) + IF(A3<10000, 0))'; + $set7 = [false, $dataArray4, $formula2, 'E5']; + + $dataArray5 = [ + [1, 2], + [3, 4], + ['=A1+A2', '=SUM(B1:B2)'], + [ 'take A', 0] + ]; + $formula3 = '=IF(A4="take A", A3, B3)'; + $set8 = [4, $dataArray5, $formula3, 'E5', ['A3'], ['B3']]; + + return [$set0, $set1, $set2, $set3, $set4, $set5, $set6, $set7, $set8]; +})(); \ No newline at end of file From 075377c2c99094781499f30b4776e7d930c17118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Fri, 14 Dec 2018 23:20:04 +0100 Subject: [PATCH 5/8] Update changelog close #788: branch pruning --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c6f0414a5..b0836175be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org). - Refactored Matrix Functions to use external Matrix library - Possibility to specify custom colors of values for pie and donut charts - [#768](https://github.com/PHPOffice/PhpSpreadsheet/pull/768) +- Branch pruning around IF function calls to avoid resolution of every branches ### Fixed From 9a9eb878ab88c81a8605733c743b1c1c8ebe4c55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Fri, 14 Dec 2018 23:23:21 +0100 Subject: [PATCH 6/8] Remove debug comment --- src/PhpSpreadsheet/Cell/Cell.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/PhpSpreadsheet/Cell/Cell.php b/src/PhpSpreadsheet/Cell/Cell.php index aec7cc0106..416b4a9909 100644 --- a/src/PhpSpreadsheet/Cell/Cell.php +++ b/src/PhpSpreadsheet/Cell/Cell.php @@ -267,7 +267,6 @@ public function getCalculatedValue($resetLog = true) } } } catch (Exception $ex) { - throw $ex; // @todo HCK remove if (($ex->getMessage() === 'Unable to access External Workbook') && ($this->calculatedValue !== null)) { return $this->calculatedValue; // Fallback for calculations referencing external files. } From 126790052cd7a8c2e11529fc4e134bfa870fea3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A4ntz=20Miccoli?= Date: Fri, 14 Dec 2018 23:49:22 +0100 Subject: [PATCH 7/8] Remove null coalescing operator "??" and other minor changes for compatibility reasons --- .../Calculation/Calculation.php | 26 +++++++++++-------- .../Calculation/Token/Stack.php | 2 +- .../Calculation/CalculationTest.php | 18 ++++++------- tests/data/Calculation/Calculation.php | 6 +++-- 4 files changed, 29 insertions(+), 23 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index f2b12e16e9..e2eefd0a63 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -3369,24 +3369,25 @@ private function _parseFormula($formula, Cell $pCell = null) $pendingStoreKey = end($pendingStoreKeysStack); if ($this->branchPruningEnabled) { - if ($expectingConditionMap[$pendingStoreKey] ?? false) { // this is a condition + // this is a condition ? + if (isset($expectingConditionMap[$pendingStoreKey]) && $expectingConditionMap[$pendingStoreKey]) { $currentCondition = $pendingStoreKey; $stackDepth = count($pendingStoreKeysStack); if ($stackDepth > 1) { // nested if $previousStoreKey = $pendingStoreKeysStack[$stackDepth - 2]; } } - if ($expectingThenMap[$pendingStoreKey] ?? false) { + if (isset($expectingThenMap[$pendingStoreKey]) && $expectingThenMap[$pendingStoreKey]) { $currentOnlyIf = $pendingStoreKey; } elseif (isset($previousStoreKey)) { - if ($expectingThenMap[$previousStoreKey] ?? false) { + if (isset($expectingThenMap[$previousStoreKey]) && $expectingThenMap[$previousStoreKey]) { $currentOnlyIf = $previousStoreKey; } } - if ($expectingElseMap[$pendingStoreKey] ?? false) { + if (isset($expectingElseMap[$pendingStoreKey]) && $expectingElseMap[$pendingStoreKey]) { $currentOnlyIfNot = $pendingStoreKey; } elseif (isset($previousStoreKey)) { - if ($expectingElseMap[$previousStoreKey] ?? false) { + if (isset($expectingElseMap[$previousStoreKey]) && $expectingElseMap[$previousStoreKey]) { $currentOnlyIfNot = $previousStoreKey; } } @@ -3551,8 +3552,11 @@ private function _parseFormula($formula, Cell $pCell = null) return $this->raiseFormulaError('Formula Error: Unexpected ,'); } $d = $stack->pop(); - $stack->push($d['type'], ++$d['value'], $d['reference'], $d['storeKey'] ?? null, $d['onlyIf'] ?? null, $d['onlyIfNot'] ?? null); // increment the argument count - $stack->push('Brace', '(', null, $d['storeKey'] ?? null, $d['onlyIf'] ?? null, $d['onlyIfNot'] ?? null); // put the ( back on, we'll need to pop back to it again + $itemStoreKey = isset($d['storeKey']) ? $d['storeKey'] : null; + $itemOnlyIf = isset($d['onlyIf']) ? $d['onlyIf'] : null; + $itemOnlyIfNot = isset($d['onlyIfNot']) ? $d['onlyIfNot'] : null; + $stack->push($d['type'], ++$d['value'], $d['reference'], $itemStoreKey, $itemOnlyIf, $itemOnlyIfNot); // increment the argument count + $stack->push('Brace', '(', null, $itemStoreKey, $itemOnlyIf, $itemOnlyIfNot); // put the ( back on, we'll need to pop back to it again $expectingOperator = false; $expectingOperand = true; ++$index; @@ -3786,10 +3790,10 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) $token = $tokenData['value']; // Branch pruning: skip useless resolutions - $storeKey = $tokenData['storeKey'] ?? null; + $storeKey = isset($tokenData['storeKey']) ? $tokenData['storeKey'] : null; if ($this->branchPruningEnabled && isset($tokenData['onlyIf'])) { $onlyIfStoreKey = $tokenData['onlyIf']; - $storeValue = $branchStore[$onlyIfStoreKey] ?? null; + $storeValue = isset($branchStore[$onlyIfStoreKey]) ? $branchStore[$onlyIfStoreKey] : null; if (is_array($storeValue)) { $wrappedItem = end($storeValue); $storeValue = end($wrappedItem); @@ -3819,7 +3823,7 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) if ($this->branchPruningEnabled && isset($tokenData['onlyIfNot'])) { $onlyIfNotStoreKey = $tokenData['onlyIfNot']; - $storeValue = $branchStore[$onlyIfNotStoreKey] ?? null; + $storeValue = isset($branchStore[$onlyIfNotStoreKey]) ? $branchStore[$onlyIfNotStoreKey] : null; if (is_array($storeValue)) { $wrappedItem = end($storeValue); $storeValue = end($wrappedItem); @@ -4747,7 +4751,7 @@ private function getUnusedBranchStoreKey() private function getTokensAsString($tokens) { $tokensStr = array_map(function ($token) { - $value = $token['value'] ?? 'no value'; + $value = isset($token['value']) ? $token['value'] : 'no value'; while (is_array($value)) { $value = array_pop($value); } diff --git a/src/PhpSpreadsheet/Calculation/Token/Stack.php b/src/PhpSpreadsheet/Calculation/Token/Stack.php index 7592bded8f..1f052423a2 100644 --- a/src/PhpSpreadsheet/Calculation/Token/Stack.php +++ b/src/PhpSpreadsheet/Calculation/Token/Stack.php @@ -138,7 +138,7 @@ public function __toString() if ($index > $this->count - 1) { break; } - $value = $item['value'] ?? 'no value'; + $value = isset($item['value']) ? $item['value'] : 'no value'; while (is_array($value)) { $value = array_pop($value); } diff --git a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php index 3b0a51b8d3..7fff307509 100644 --- a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php @@ -154,9 +154,9 @@ public function testBranchPruningFormulaParsing() foreach ($tokens as $token) { $isBinaryOperator = $token['type'] == 'Binary Operator'; $isEqual = $token['value'] == '='; - $correctStoreKey = ($token['storeKey'] ?? '') == 'storeKey-0'; - $correctOnlyIf = ($token['onlyIf'] ?? '') == 'storeKey-0'; - $isB1Reference = ($token['reference'] ?? '') == 'B1'; + $correctStoreKey = (isset($token['storeKey']) ? $token['storeKey'] : '') == 'storeKey-0'; + $correctOnlyIf = (isset($token['onlyIf']) ? $token['onlyIf'] : '') == 'storeKey-0'; + $isB1Reference = (isset($token['reference']) ? $token['reference'] : '') == 'B1'; $foundEqualAssociatedToStoreKey = $foundEqualAssociatedToStoreKey || ($isBinaryOperator && $isEqual && $correctStoreKey); @@ -188,7 +188,7 @@ public function testBranchPruningFormulaParsing() $isFunction = $token['type'] == 'Function'; $isProductFunction = $token['value'] == 'PRODUCT('; - $correctOnlyIf = ($token['onlyIf'] ?? '') == 'storeKey-1'; + $correctOnlyIf = (isset($token['onlyIf']) ? $token['onlyIf'] : '') == 'storeKey-1'; $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || ($isFunction && $isProductFunction && $correctOnlyIf); } @@ -212,9 +212,9 @@ public function testBranchPruningFormulaParsing() $isProductFunction = $value == 'PRODUCT('; $isNotFunction = $value == 'NOT('; $isIfOperand = $token['type'] == 'Operand Count for Function IF()'; - $isOnlyIfNotDepth1 = $token['onlyIfNot'] == 'storeKey-1'; - $isStoreKeyDepth1 = $token['storeKey'] == 'storeKey-1'; - $isOnlyIfNotDepth0 = $token['onlyIfNot'] == 'storeKey-0'; + $isOnlyIfNotDepth1 = (array_key_exists('onlyIfNot', $token) ? $token['onlyIfNot'] : null) == 'storeKey-1'; + $isStoreKeyDepth1 = (array_key_exists('storeKey', $token) ? $token['storeKey'] : null) == 'storeKey-1'; + $isOnlyIfNotDepth0 = (array_key_exists('onlyIfNot', $token) ? $token['onlyIfNot'] : null) == 'storeKey-0'; $plusCorrectlyTagged = $plusCorrectlyTagged || ($isPlus && $isOnlyIfNotDepth0); @@ -243,7 +243,7 @@ public function testBranchPruningFormulaParsing() $properlyTaggedPlus = false; foreach ($tokens as $token) { $isPlus = $token['value'] === '+'; - $hasOnlyIf = !empty($token['onlyIf'] ?? null); + $hasOnlyIf = !empty($token['onlyIf']); $properlyTaggedPlus = $properlyTaggedPlus || ($isPlus && $hasOnlyIf); @@ -267,7 +267,7 @@ public function testBranchPruningFormulaParsing() public function testFullExecution( $expectedResult, $dataArray, - string $formula, + $formula, $cellCoordinates, $shouldBeSetInCacheCells = [], $shouldNotBeSetInCacheCells = []) diff --git a/tests/data/Calculation/Calculation.php b/tests/data/Calculation/Calculation.php index dca79ed7a0..d54729f0e5 100644 --- a/tests/data/Calculation/Calculation.php +++ b/tests/data/Calculation/Calculation.php @@ -1,6 +1,6 @@ Date: Sat, 15 Dec 2018 00:29:34 +0100 Subject: [PATCH 8/8] Fix style errors and splitting tests --- .../Calculation/Calculation.php | 15 ++---- .../Calculation/Token/Stack.php | 3 +- .../Calculation/CalculationTest.php | 53 +++++++++++-------- tests/data/Calculation/Calculation.php | 5 +- 4 files changed, 37 insertions(+), 39 deletions(-) diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index e2eefd0a63..30432780c4 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -2256,7 +2256,6 @@ public function flushInstance() { $this->clearCalculationCache(); $this->clearBranchStore(); - $this->branchStoreKeyCounter = 0; } /** @@ -3443,7 +3442,6 @@ private function _parseFormula($formula, Cell $pCell = null) } if (preg_match('/^' . self::CALCULATION_REGEXP_FUNCTION . '$/i', $d['value'], $matches)) { // Did this parenthesis just close a function? - if (!empty($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == -1) { // we are closing an IF( if ($d['value'] != 'IF(') { @@ -3519,9 +3517,7 @@ private function _parseFormula($formula, Cell $pCell = null) } ++$index; } elseif ($opCharacter == ',') { // Is this the separator for function arguments? - - if ( - !empty($pendingStoreKey) && + if (!empty($pendingStoreKey) && $parenthesisDepthMap[$pendingStoreKey] == 0 ) { // We must go to the IF next argument @@ -3590,8 +3586,7 @@ private function _parseFormula($formula, Cell $pCell = null) } } - $stack->push('Function', $valToUpper, null, - $currentCondition, $currentOnlyIf, $currentOnlyIfNot); + $stack->push('Function', $valToUpper, null, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); // tests if the function is closed right after opening $ax = preg_match('/^\s*(\s*\))/ui', substr($formula, $index + $length), $amatch); if ($ax) { @@ -3626,9 +3621,7 @@ private function _parseFormula($formula, Cell $pCell = null) } } - $outputItem = $stack->getStackItem('Cell Reference', $val, - $val, $currentCondition, $currentOnlyIf, - $currentOnlyIfNot); + $outputItem = $stack->getStackItem('Cell Reference', $val, $val, $currentCondition, $currentOnlyIf, $currentOnlyIfNot); $output[] = $outputItem; } else { // it's a variable, constant, string, number or boolean @@ -3802,7 +3795,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) if (isset($storeValue) && (($storeValue !== true) || ($storeValue === 'Pruned branch')) ) { - // If branching value is not true, we don't need to compute if (!isset($fakedForBranchPruning['onlyIf-' . $onlyIfStoreKey])) { $stack->push('Value', 'Pruned branch (only if ' . $onlyIfStoreKey . ') ' . $token); @@ -3831,7 +3823,6 @@ private function processTokenStack($tokens, $cellID = null, Cell $pCell = null) if (isset($storeValue) && ($storeValue || ($storeValue === 'Pruned branch')) ) { - // If branching value is true, we don't need to compute if (!isset($fakedForBranchPruning['onlyIfNot-' . $onlyIfNotStoreKey])) { $stack->push('Value', 'Pruned branch (only if not ' . $onlyIfNotStoreKey . ') ' . $token); diff --git a/src/PhpSpreadsheet/Calculation/Token/Stack.php b/src/PhpSpreadsheet/Calculation/Token/Stack.php index 1f052423a2..61a93ea095 100644 --- a/src/PhpSpreadsheet/Calculation/Token/Stack.php +++ b/src/PhpSpreadsheet/Calculation/Token/Stack.php @@ -50,8 +50,7 @@ public function push( $onlyIf = null, $onlyIfNot = null ) { - $stackItem = $this->getStackItem($type, $value, $reference, $storeKey, - $onlyIf, $onlyIfNot); + $stackItem = $this->getStackItem($type, $value, $reference, $storeKey, $onlyIf, $onlyIfNot); $this->stack[$this->count++] = $stackItem; diff --git a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php index 7fff307509..4ff8465d1b 100644 --- a/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/CalculationTest.php @@ -141,9 +141,10 @@ public function testFormulaWithOptionalArgumentsAndRequiredCellReferenceShouldPa self::assertEquals(5, $cell->getCalculatedValue(), 'missing arguments should be filled with null'); } - public function testBranchPruningFormulaParsing() + public function testBranchPruningFormulaParsingSimpleCase() { $calculation = Calculation::getInstance(); + $calculation->flushInstance(); // resets the ids // Very simple formula $formula = '=IF(A1="please +",B1)'; @@ -166,7 +167,11 @@ public function testBranchPruningFormulaParsing() } $this->assertTrue($foundEqualAssociatedToStoreKey); $this->assertTrue($foundConditionalOnB1); + } + public function testBranchPruningFormulaParsingMultipleIfsCase() + { + $calculation = Calculation::getInstance(); $calculation->flushInstance(); // resets the ids // @@ -189,14 +194,15 @@ public function testBranchPruningFormulaParsing() $isFunction = $token['type'] == 'Function'; $isProductFunction = $token['value'] == 'PRODUCT('; $correctOnlyIf = (isset($token['onlyIf']) ? $token['onlyIf'] : '') == 'storeKey-1'; - $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || - ($isFunction && $isProductFunction && $correctOnlyIf); + $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || ($isFunction && $isProductFunction && $correctOnlyIf); } - $this->assertFalse($plusGotTagged, - 'chaining IF( should not affect the external operators'); - $this->assertTrue($productFunctionCorrectlyTagged, - 'function nested inside if should be tagged to be processed only if parent branching requires it'); + $this->assertFalse($plusGotTagged, 'chaining IF( should not affect the external operators'); + $this->assertTrue($productFunctionCorrectlyTagged, 'function nested inside if should be tagged to be processed only if parent branching requires it'); + } + public function testBranchPruningFormulaParingNestedIfCase() + { + $calculation = Calculation::getInstance(); $calculation->flushInstance(); // resets the ids $formula = '=IF(A1="please +",SUM(B1:B3),1+IF(NOT(A2="please *"),C2-C1,PRODUCT(C1:C3)))'; @@ -216,26 +222,30 @@ public function testBranchPruningFormulaParsing() $isStoreKeyDepth1 = (array_key_exists('storeKey', $token) ? $token['storeKey'] : null) == 'storeKey-1'; $isOnlyIfNotDepth0 = (array_key_exists('onlyIfNot', $token) ? $token['onlyIfNot'] : null) == 'storeKey-0'; - $plusCorrectlyTagged = $plusCorrectlyTagged || - ($isPlus && $isOnlyIfNotDepth0); - $notFunctionCorrectlyTagged = $notFunctionCorrectlyTagged || - ($isNotFunction && $isOnlyIfNotDepth0 && $isStoreKeyDepth1); - $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || - ($isProductFunction && $isOnlyIfNotDepth1 && !$isStoreKeyDepth1 && !$isOnlyIfNotDepth0); - $findOneOperandCountTagged = $findOneOperandCountTagged || - ($isIfOperand && $isOnlyIfNotDepth0); + $plusCorrectlyTagged = $plusCorrectlyTagged || ($isPlus && $isOnlyIfNotDepth0); + $notFunctionCorrectlyTagged = $notFunctionCorrectlyTagged || ($isNotFunction && $isOnlyIfNotDepth0 && $isStoreKeyDepth1); + $productFunctionCorrectlyTagged = $productFunctionCorrectlyTagged || ($isProductFunction && $isOnlyIfNotDepth1 && !$isStoreKeyDepth1 && !$isOnlyIfNotDepth0); + $findOneOperandCountTagged = $findOneOperandCountTagged || ($isIfOperand && $isOnlyIfNotDepth0); } $this->assertTrue($plusCorrectlyTagged); $this->assertTrue($productFunctionCorrectlyTagged); $this->assertTrue($notFunctionCorrectlyTagged); + } + public function testBranchPruningFormulaParsingNoArgumentFunctionCase() + { + $calculation = Calculation::getInstance(); $calculation->flushInstance(); // resets the ids $formula = '=IF(AND(TRUE(),A1="please +"),2,3)'; // this used to raise a parser error, we keep it even though we don't // test the output - $tokens = $calculation->parseFormula($formula); + $calculation->parseFormula($formula); + } + public function testBranchPruningFormulaParsingInequalitiesConditionsCase() + { + $calculation = Calculation::getInstance(); $calculation->flushInstance(); // resets the ids $formula = '=IF(A1="flag",IF(A2<10, 0) + IF(A3<10000, 0))'; @@ -270,16 +280,14 @@ public function testFullExecution( $formula, $cellCoordinates, $shouldBeSetInCacheCells = [], - $shouldNotBeSetInCacheCells = []) - { + $shouldNotBeSetInCacheCells = [] + ) { $spreadsheet = new Spreadsheet(); $sheet = $spreadsheet->getActiveSheet(); $sheet->fromArray($dataArray); $cell = $sheet->getCell($cellCoordinates); - $calculation = Calculation::getInstance( - $cell->getWorksheet()->getParent() - ); + $calculation = Calculation::getInstance($cell->getWorksheet()->getParent()); $cell->setValue($formula); $calculated = $cell->getCalculatedValue(); @@ -294,8 +302,7 @@ public function testFullExecution( foreach ($shouldNotBeSetInCacheCells as $notSetCell) { unset($inCache); - $calculation->getValueFromCache('Worksheet!' . $notSetCell, - $inCache); + $calculation->getValueFromCache('Worksheet!' . $notSetCell, $inCache); $this->assertEmpty($inCache); } diff --git a/tests/data/Calculation/Calculation.php b/tests/data/Calculation/Calculation.php index d54729f0e5..09538c3cd9 100644 --- a/tests/data/Calculation/Calculation.php +++ b/tests/data/Calculation/Calculation.php @@ -1,6 +1,7 @@