diff options
author | Dennis Francis <dennis.francis@collabora.co.uk> | 2018-02-07 12:00:47 +0530 |
---|---|---|
committer | Eike Rathke <erack@redhat.com> | 2018-04-28 13:33:55 +0200 |
commit | 67b1c26c27590678ece7bcef763433aedd0b164d (patch) | |
tree | efed6b3c2c2a6db724d0aa60dcde735d5d10ac4a | |
parent | 212807f77b78c69263f8aae51dcdc73e8017c53a (diff) |
tdf#114479: compute implicit sum ranges for ocSumIf,ocAverageIf...
and update the sum-range token in RPN array while creation of
the RPN array itself.
+ Adds unit tests.
+ In ScParallelismTest unit test, enable threading in its setUp()
method and restore the original setting in tearDown().
Change-Id: Iee9b7759210a82950181a418eb92766a6cf891fc
Reviewed-on: https://gerrit.libreoffice.org/49465
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Eike Rathke <erack@redhat.com>
-rw-r--r-- | formula/source/core/api/FormulaCompiler.cxx | 20 | ||||
-rw-r--r-- | include/formula/FormulaCompiler.hxx | 8 | ||||
-rw-r--r-- | sc/CppunitTest_sc_parallelism.mk | 4 | ||||
-rw-r--r-- | sc/inc/compiler.hxx | 5 | ||||
-rw-r--r-- | sc/qa/unit/parallelism.cxx | 79 | ||||
-rw-r--r-- | sc/qa/unit/ucalc.hxx | 2 | ||||
-rw-r--r-- | sc/qa/unit/ucalc_formula.cxx | 126 | ||||
-rw-r--r-- | sc/source/core/data/formulacell.cxx | 2 | ||||
-rw-r--r-- | sc/source/core/tool/compiler.cxx | 120 |
9 files changed, 364 insertions, 2 deletions
diff --git a/formula/source/core/api/FormulaCompiler.cxx b/formula/source/core/api/FormulaCompiler.cxx index d16e08068b94..96c2e166fe00 100644 --- a/formula/source/core/api/FormulaCompiler.cxx +++ b/formula/source/core/api/FormulaCompiler.cxx @@ -17,6 +17,7 @@ * the License at http://www.apache.org/licenses/LICENSE-2.0 . */ #include <sal/macros.h> +#include <sal/alloca.h> #include <formula/FormulaCompiler.hxx> #include <formula/errorcodes.hxx> #include <formula/token.hxx> @@ -1596,7 +1597,21 @@ void FormulaCompiler::Factor() sal_uInt32 nSepCount = 0; if( !bNoParam ) { + bool bDoIICompute = IsIIOpCode(eMyLastOp); + // Array of FormulaToken double pointers to collect the parameters of II opcodes. + FormulaToken*** pArgArray = nullptr; + if (bDoIICompute) + { + pArgArray = static_cast<FormulaToken***>(alloca(sizeof(FormulaToken**)*FORMULA_MAXPARAMSII)); + if (!pArgArray) + bDoIICompute = false; + } + nSepCount++; + + if (bDoIICompute) + pArgArray[nSepCount-1] = pCode - 1; // Add first argument + while ((eOp == ocSep) && (pArr->GetCodeError() == FormulaError::NONE || !mbStopOnError)) { NextToken(); @@ -1605,7 +1620,12 @@ void FormulaCompiler::Factor() if (nSepCount > FORMULA_MAXPARAMS) SetError( FormulaError::CodeOverflow); eOp = Expression(); + if (bDoIICompute && nSepCount <= FORMULA_MAXPARAMSII) + pArgArray[nSepCount - 1] = pCode - 1; // Add rest of the arguments } + if (bDoIICompute) + HandleIIOpCode(eMyLastOp, pArgArray, + std::min(nSepCount, static_cast<sal_uInt32>(FORMULA_MAXPARAMSII))); } if (bBadName) ; // nothing, keep current token for return diff --git a/include/formula/FormulaCompiler.hxx b/include/formula/FormulaCompiler.hxx index 8171b0a831f0..e64bc867a447 100644 --- a/include/formula/FormulaCompiler.hxx +++ b/include/formula/FormulaCompiler.hxx @@ -41,6 +41,7 @@ #define FORMULA_MAXJUMPCOUNT 32 /* maximum number of jumps (ocChoose) */ #define FORMULA_MAXTOKENS 8192 /* maximum number of tokens in formula */ #define FORMULA_MAXPARAMS 255 /* maximum number of parameters per function (byte) */ +#define FORMULA_MAXPARAMSII 8 /* maximum number of parameters for functions that have implicit intersection ranges */ namespace com { namespace sun { namespace star { @@ -66,7 +67,6 @@ struct FormulaArrayStack bool bTemp; }; - typedef std::unordered_map< OUString, OpCode > OpCodeHashMap; typedef std::unordered_map< OUString, OUString > ExternalHashMap; @@ -323,6 +323,12 @@ protected: bool MergeRangeReference( FormulaToken * * const pCode1, FormulaToken * const * const pCode2 ); + // Returns whether the opcode has implicit intersection ranges as parameters. + // This is no-op for this class. + virtual bool IsIIOpCode(OpCode /*nOpCode*/) const { return false; } + // Handles II opcode and passes the parameter array and number of parameters. + virtual void HandleIIOpCode(OpCode /*nOpCode*/, FormulaToken*** /*pppToken*/, sal_uInt8 /*nNumParams*/) {} + OUString aCorrectedFormula; // autocorrected Formula OUString aCorrectedSymbol; // autocorrected Symbol diff --git a/sc/CppunitTest_sc_parallelism.mk b/sc/CppunitTest_sc_parallelism.mk index bffa7b1bf8c2..247bd9adfeb2 100644 --- a/sc/CppunitTest_sc_parallelism.mk +++ b/sc/CppunitTest_sc_parallelism.mk @@ -61,6 +61,10 @@ $(eval $(call gb_CppunitTest_set_include,sc_parallelism,\ $(eval $(call gb_CppunitTest_use_sdk_api,sc_parallelism)) +$(eval $(call gb_CppunitTest_use_custom_headers,sc_parallelism,\ + officecfg/registry \ +)) + $(eval $(call gb_CppunitTest_use_ure,sc_parallelism)) $(eval $(call gb_CppunitTest_use_vcl,sc_parallelism)) diff --git a/sc/inc/compiler.hxx b/sc/inc/compiler.hxx index b763f2d4d326..41c193dfaf4f 100644 --- a/sc/inc/compiler.hxx +++ b/sc/inc/compiler.hxx @@ -472,6 +472,11 @@ private: /// Access the CharTable flags ScCharFlags GetCharTableFlags( sal_Unicode c, sal_Unicode cLast ) { return c < 128 ? pConv->getCharTableFlags(c, cLast) : ScCharFlags::NONE; } + + bool IsIIOpCode(OpCode nOpCode) const override; + void HandleIIOpCode(OpCode nOpCode, formula::FormulaToken*** pppToken, sal_uInt8 nNumParams) override; + bool AdjustSumRangeShape(const ScComplexRefData& rBaseRange, ScComplexRefData& rSumRange); + void CorrectSumRange(const ScComplexRefData& rBaseRange, ScComplexRefData& rSumRange, formula::FormulaToken** ppSumRangeToken); }; #endif diff --git a/sc/qa/unit/parallelism.cxx b/sc/qa/unit/parallelism.cxx index ca6dce5c9f2f..c5c196154123 100644 --- a/sc/qa/unit/parallelism.cxx +++ b/sc/qa/unit/parallelism.cxx @@ -27,9 +27,12 @@ #include <userdat.hxx> #include <formulacell.hxx> #include <formulagroup.hxx> +#include <scopetools.hxx> #include <svx/svdpage.hxx> +#include <officecfg/Office/Calc.hxx> + using namespace css; using namespace css::uno; @@ -39,6 +42,7 @@ public: ScParallelismTest(); virtual void setUp() override; + virtual void tearDown() override; void getNewDocShell(ScDocShellRef& rDocShellRef); @@ -47,6 +51,7 @@ public: void testVLOOKUP(); void testVLOOKUPSUM(); void testSingleRef(); + void testSUMIFImplicitRange(); CPPUNIT_TEST_SUITE(ScParallelismTest); CPPUNIT_TEST(testSUMIFS); @@ -54,12 +59,18 @@ public: CPPUNIT_TEST(testVLOOKUP); CPPUNIT_TEST(testVLOOKUPSUM); CPPUNIT_TEST(testSingleRef); + CPPUNIT_TEST(testSUMIFImplicitRange); CPPUNIT_TEST_SUITE_END(); private: + + bool getThreadingFlag(); + void setThreadingFlag(bool bSet); + ScDocument *m_pDoc; ScDocShellRef m_xDocShell; + bool m_bThreadingFlagCfg; }; ScParallelismTest::ScParallelismTest() @@ -67,6 +78,18 @@ ScParallelismTest::ScParallelismTest() { } +bool ScParallelismTest::getThreadingFlag() +{ + return officecfg::Office::Calc::Formula::Calculation::UseThreadedCalculationForFormulaGroups::get(); +} + +void ScParallelismTest::setThreadingFlag( bool bSet ) +{ + std::shared_ptr<comphelper::ConfigurationChanges> xBatch(comphelper::ConfigurationChanges::create()); + officecfg::Office::Calc::Formula::Calculation::UseThreadedCalculationForFormulaGroups::set(bSet, xBatch); + xBatch->commit(); +} + void ScParallelismTest::setUp() { test::BootstrapFixture::setUp(); @@ -77,6 +100,19 @@ void ScParallelismTest::setUp() m_pDoc = &m_xDocShell->GetDocument(); sc::FormulaGroupInterpreter::disableOpenCL_UnitTestsOnly(); + + m_bThreadingFlagCfg = getThreadingFlag(); + if (!m_bThreadingFlagCfg) + setThreadingFlag(true); +} + +void ScParallelismTest::tearDown() +{ + // Restore threading flag + if (!m_bThreadingFlagCfg) + setThreadingFlag(false); + + test::BootstrapFixture::tearDown(); } void ScParallelismTest::getNewDocShell( ScDocShellRef& rDocShellRef ) @@ -317,6 +353,49 @@ void ScParallelismTest::testSingleRef() m_pDoc->DeleteTab(0); } +// Common test setup steps for testSUMIFImplicitRange*() +void lcl_setupCommon(ScDocument* pDoc, size_t nNumRows, size_t nConstCellValue) +{ + pDoc->SetValue(3, 0, 0, static_cast<double>(nConstCellValue)); // D1 + for (size_t i = 0; i <= (nNumRows*2); ++i) + { + pDoc->SetValue(0, i, 0, static_cast<double>(i)); + pDoc->SetFormula(ScAddress(1, i, 0), + "=A" + OUString::number(i+1), + formula::FormulaGrammar::GRAM_NATIVE_UI); + } +} + +void ScParallelismTest::testSUMIFImplicitRange() +{ + sc::AutoCalcSwitch aACSwitch(*m_pDoc, false); + m_pDoc->InsertTab(0, "1"); + + const size_t nNumRows = 1048; + const size_t nConstCellValue = 20; + lcl_setupCommon(m_pDoc, nNumRows, nConstCellValue); + OUString aSrcRange = "$A$1:$A$" + OUString::number(nNumRows); + OUString aFormula; + for (size_t i = 0; i < nNumRows; ++i) + { + aFormula = "=SUMIF(" + aSrcRange + ";$D$1;$B$1)"; + m_pDoc->SetFormula(ScAddress(2, i, 0), + aFormula, + formula::FormulaGrammar::GRAM_NATIVE_UI); + } + + ScFormulaCell* pCell = m_pDoc->GetFormulaCell(ScAddress(2, 0, 0)); + sc::AutoCalcSwitch aACSwitch2(*m_pDoc, true); + pCell->InterpretFormulaGroup(); // Start calculation on the F.G at C1 + + for (size_t i = 0; i < nNumRows; ++i) + { + OString aMsg = "At row " + OString::number(i); + CPPUNIT_ASSERT_EQUAL_MESSAGE(aMsg.getStr(), nConstCellValue, static_cast<size_t>(m_pDoc->GetValue(2, i, 0))); + } + m_pDoc->DeleteTab(0); +} + CPPUNIT_TEST_SUITE_REGISTRATION(ScParallelismTest); CPPUNIT_PLUGIN_IMPLEMENT(); diff --git a/sc/qa/unit/ucalc.hxx b/sc/qa/unit/ucalc.hxx index 6718e10a4248..f1d6acb476eb 100644 --- a/sc/qa/unit/ucalc.hxx +++ b/sc/qa/unit/ucalc.hxx @@ -143,6 +143,7 @@ public: void testFormulaRefData(); void testFormulaCompiler(); void testFormulaCompilerJumpReordering(); + void testFormulaCompilerImplicitIntersection(); void testFormulaRefUpdate(); void testFormulaRefUpdateRange(); void testFormulaRefUpdateSheets(); @@ -567,6 +568,7 @@ public: CPPUNIT_TEST(testFormulaRefData); CPPUNIT_TEST(testFormulaCompiler); CPPUNIT_TEST(testFormulaCompilerJumpReordering); + CPPUNIT_TEST(testFormulaCompilerImplicitIntersection); CPPUNIT_TEST(testFormulaRefUpdate); CPPUNIT_TEST(testFormulaRefUpdateRange); CPPUNIT_TEST(testFormulaRefUpdateSheets); diff --git a/sc/qa/unit/ucalc_formula.cxx b/sc/qa/unit/ucalc_formula.cxx index 0b375db0dd21..126bed6c9289 100644 --- a/sc/qa/unit/ucalc_formula.cxx +++ b/sc/qa/unit/ucalc_formula.cxx @@ -1105,6 +1105,132 @@ void Test::testFormulaCompilerJumpReordering() } } +void Test::testFormulaCompilerImplicitIntersection() +{ + struct TestCaseFormula + { + OUString aFormula; + ScAddress aCellAddress; + ScRange aSumRange; + bool bStartColRel; // SumRange-StartCol + bool bEndColRel; // SumRange-EndCol + }; + + m_pDoc->InsertTab(0, "Formula"); + sc::AutoCalcSwitch aACSwitch(*m_pDoc, true); // turn auto calc on. + + { + TestCaseFormula aTestCases[] = + { + // Formula, FormulaCellAddress, SumRange with Implicit Intersection + + // Sumrange is single cell, address is abs + { + OUString("=SUMIF($B$2:$B$10;F2;$D$5)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + false, + false + }, + + // Sumrange is single cell, address is relative + { + OUString("=SUMIF($B$2:$B$10;F2;D5)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + true, + true + }, + + // Baserange(abs,abs), Sumrange(abs,abs) + { + OUString("=SUMIF($B$2:$B$10;F2;$D$5:$D$10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + false, + false + }, + + // Baserange(abs,rel), Sumrange(abs,abs) + { + OUString("=SUMIF($B$2:B10;F2;$D$5:$D$10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + false, + false + }, + + // Baserange(rel,abs), Sumrange(abs,abs) + { + OUString("=SUMIF(B2:$B$10;F2;$D$5:$D$10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + false, + false + }, + + // Baserange(rel,rel), Sumrange(abs,abs) + { + OUString("=SUMIF(B2:B10;F2;$D$5:$D$10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + false, + false + }, + + // Baserange(abs,abs), Sumrange(abs,rel) + { + OUString("=SUMIF($B$2:$B$10;F2;$D$5:D10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + false, + true + }, + + // Baserange(abs,abs), Sumrange(rel,abs) + { + OUString("=SUMIF($B$2:$B$10;F2;D5:$D$10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + true, + false + }, + + // Baserange(abs,abs), Sumrange(rel,rel) + { + OUString("=SUMIF($B$2:$B$10;F2;D5:D10)"), + ScAddress(7, 5, 0), + ScRange( ScAddress(3, 4, 0), ScAddress(3, 12, 0) ), + true, + true + } + }; + + for (auto& rCase : aTestCases) + { + m_pDoc->SetString(rCase.aCellAddress, rCase.aFormula); + const ScFormulaCell* pCell = m_pDoc->GetFormulaCell(rCase.aCellAddress); + const ScTokenArray* pCode = pCell->GetCode(); + CPPUNIT_ASSERT(pCode); + + sal_uInt16 nLen = pCode->GetCodeLen(); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong RPN token count.", static_cast<sal_uInt16>(4), nLen); + + FormulaToken** ppTokens = pCode->GetCode(); + + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong type of token(first argument to SUMIF)", svDoubleRef, ppTokens[0]->GetType()); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong type of token(third argument to SUMIF)", svDoubleRef, ppTokens[2]->GetType()); + + ScComplexRefData aSumRangeData = *ppTokens[2]->GetDoubleRef(); + ScRange aSumRange = aSumRangeData.toAbs(rCase.aCellAddress); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong sum-range in RPN array", rCase.aSumRange, aSumRange); + + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong IsRel type for start column address in sum-range", rCase.bStartColRel, aSumRangeData.Ref1.IsColRel()); + CPPUNIT_ASSERT_EQUAL_MESSAGE("Wrong IsRel type for end column address in sum-range", rCase.bEndColRel, aSumRangeData.Ref2.IsColRel()); + } + } +} + void Test::testFormulaRefUpdate() { m_pDoc->InsertTab(0, "Formula"); diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx index a8c697f50630..7d06d2961cde 100644 --- a/sc/source/core/data/formulacell.cxx +++ b/sc/source/core/data/formulacell.cxx @@ -4195,7 +4195,7 @@ struct ScDependantsCalculator // Partially from ScGroupTokenConverter::convert in sc/source/core/data/grouptokenconverter.cxx ScRangeList aRangeList; - for (auto p: mrCode.Tokens()) + for (auto p: mrCode.RPNTokens()) { switch (p->GetType()) { diff --git a/sc/source/core/tool/compiler.cxx b/sc/source/core/tool/compiler.cxx index beaf4a69dbbb..69ef96592411 100644 --- a/sc/source/core/tool/compiler.cxx +++ b/sc/source/core/tool/compiler.cxx @@ -5780,4 +5780,124 @@ formula::ParamClass ScCompiler::GetForceArrayParameter( const formula::FormulaTo return ScParameterClassification::GetParameterType( pToken, nParam); } +bool ScCompiler::IsIIOpCode(OpCode nOpCode) const +{ + if (nOpCode == ocSumIf || nOpCode == ocAverageIf) + return true; + + return false; +} + +void ScCompiler::HandleIIOpCode(OpCode nOpCode, FormulaToken*** pppToken, sal_uInt8 nNumParams) +{ + switch (nOpCode) + { + case ocSumIf: + case ocAverageIf: + { + if (nNumParams != 3) + return; + + if (!(pppToken[0] && pppToken[2] && *pppToken[0] && *pppToken[2])) + return; + + if ((*pppToken[0])->GetType() != svDoubleRef) + return; + + const StackVar eSumRangeType = (*pppToken[2])->GetType(); + + if ( eSumRangeType != svSingleRef && eSumRangeType != svDoubleRef ) + return; + + const ScComplexRefData& rBaseRange = *(*pppToken[0])->GetDoubleRef(); + + ScComplexRefData aSumRange; + if (eSumRangeType == svSingleRef) + { + aSumRange.Ref1 = *(*pppToken[2])->GetSingleRef(); + aSumRange.Ref2 = aSumRange.Ref1; + } + else + aSumRange = *(*pppToken[2])->GetDoubleRef(); + + CorrectSumRange(rBaseRange, aSumRange, pppToken[2]); + } + break; + default: + ; + } +} + +static void lcl_GetColRowDeltas(const ScRange& rRange, SCCOL& rXDelta, SCROW& rYDelta) +{ + rXDelta = rRange.aEnd.Col() - rRange.aStart.Col(); + rYDelta = rRange.aEnd.Row() - rRange.aStart.Row(); +} + +bool ScCompiler::AdjustSumRangeShape(const ScComplexRefData& rBaseRange, ScComplexRefData& rSumRange) +{ + ScRange aAbs = rSumRange.toAbs(aPos); + + // Current sum-range end col/row + SCCOL nEndCol = aAbs.aEnd.Col(); + SCROW nEndRow = aAbs.aEnd.Row(); + + // Current behaviour is, we will get a #NAME? for the below case, so bail out. + // Note that sum-range's End[Col,Row] are same as Start[Col,Row] if the original formula + // has a single-ref as the sum-range. + if (!ValidCol(nEndCol) || !ValidRow(nEndRow)) + return false; + + SCCOL nXDeltaSum = 0; + SCROW nYDeltaSum = 0; + + lcl_GetColRowDeltas(aAbs, nXDeltaSum, nYDeltaSum); + + aAbs = rBaseRange.toAbs(aPos); + SCCOL nXDelta = 0; + SCROW nYDelta = 0; + + lcl_GetColRowDeltas(aAbs, nXDelta, nYDelta); + + if (nXDelta == nXDeltaSum && + nYDelta == nYDeltaSum) + return false; // shapes of base-range match current sum-range + + // Try to make the sum-range to take the same shape as base-range, + // by adjusting Ref2 member of rSumRange if the resultant sum-range don't + // go out-of-bounds. + + SCCOL nXInc = nXDelta - nXDeltaSum; + SCROW nYInc = nYDelta - nYDeltaSum; + + // Don't let a valid End[Col,Row] go beyond (MAXCOL,MAXROW) to match + // what happens in ScInterpreter::IterateParametersIf(), but there it also shrinks + // the base-range by the (out-of-bound)amount clipped off the sum-range. + // TODO: Probably we can optimize (from threading perspective) rBaseRange + // by shrinking it here correspondingly (?) + if (nEndCol + nXInc > MAXCOL) + nXInc = MAXCOL - nEndCol; + if (nEndRow + nYInc > MAXROW) + nYInc = MAXROW - nEndRow; + + rSumRange.Ref2.IncCol(nXInc); + rSumRange.Ref2.IncRow(nYInc); + + return true; +} + +void ScCompiler::CorrectSumRange(const ScComplexRefData& rBaseRange, + ScComplexRefData& rSumRange, + FormulaToken** ppSumRangeToken) +{ + if (!AdjustSumRangeShape(rBaseRange, rSumRange)) + return; + + // Replace sum-range token + FormulaToken* pNewSumRangeTok = new ScDoubleRefToken(rSumRange); + (*ppSumRangeToken)->DecRef(); + *ppSumRangeToken = pNewSumRangeTok; + pNewSumRangeTok->IncRef(); +} + /* vim:set shiftwidth=4 softtabstop=4 expandtab: */ |