sc/inc/queryevaluator.hxx | 9 +- sc/qa/unit/ucalc.cxx | 106 +++++++++++++++++++++++++++++++++ sc/source/core/data/queryevaluator.cxx | 18 +++-- 3 files changed, 120 insertions(+), 13 deletions(-)
New commits: commit 986baafc7856f43fe33a957c2faefdcd89505796 Author: Luboš Luňák <[email protected]> AuthorDate: Sat Dec 4 18:39:22 2021 +0100 Commit: Adolfo Jayme Barrientos <[email protected]> CommitDate: Sun Dec 5 18:09:22 2021 +0100 fix caching of ScQueryItem values for multiple entries (tdf#146037) The code assumed that there would be only one ScQueryEntry. Change-Id: Idf4cc6bdbbf1edad7d13eb9a9643bb67e199dd01 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/126368 Tested-by: Luboš Luňák <[email protected]> Reviewed-by: Luboš Luňák <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/126300 Tested-by: Jenkins Reviewed-by: Adolfo Jayme Barrientos <[email protected]> diff --git a/sc/inc/queryevaluator.hxx b/sc/inc/queryevaluator.hxx index 3caa1c57dfcf..44bf52ec3685 100644 --- a/sc/inc/queryevaluator.hxx +++ b/sc/inc/queryevaluator.hxx @@ -68,10 +68,9 @@ class ScQueryEvaluator std::unique_ptr<bool[]> mpTestDynamic; std::unordered_map<FormulaError, svl::SharedString> mCachedSharedErrorStrings; - std::vector<double> mCachedSortedItemValues; - std::vector<const rtl_uString*> mCachedSortedItemStrings; - bool mCachedSortedItemValuesReady = false; - bool mCachedSortedItemStringsReady = false; + // The "outside" index in these two is the index of ScQueryEntry in ScQueryParam. + std::vector<std::vector<double>> mCachedSortedItemValues; + std::vector<std::vector<const rtl_uString*>> mCachedSortedItemStrings; static bool isPartialTextMatchOp(const ScQueryEntry& rEntry); static bool isTextMatchOp(const ScQueryEntry& rEntry); @@ -109,7 +108,7 @@ class ScQueryEvaluator const ScQueryEntry::Item& rItem); std::pair<bool, bool> processEntry(SCROW nRow, SCCOL nCol, ScRefCellValue& aCell, - const ScQueryEntry& rEntry); + const ScQueryEntry& rEntry, size_t nEntryIndex); public: ScQueryEvaluator(ScDocument& rDoc, const ScTable& rTab, const ScQueryParam& rParam, diff --git a/sc/qa/unit/ucalc.cxx b/sc/qa/unit/ucalc.cxx index e936aada4a51..9a80a42ad4b1 100644 --- a/sc/qa/unit/ucalc.cxx +++ b/sc/qa/unit/ucalc.cxx @@ -169,6 +169,7 @@ public: void testDataArea(); void testAutofilter(); void testAutoFilterTimeValue(); + void testAutofilterOptimizations(); void testTdf76836(); void testTdf76441(); void testTdf142186(); @@ -293,6 +294,7 @@ public: CPPUNIT_TEST(testToggleRefFlag); CPPUNIT_TEST(testAutofilter); CPPUNIT_TEST(testAutoFilterTimeValue); + CPPUNIT_TEST(testAutofilterOptimizations); CPPUNIT_TEST(testTdf76836); CPPUNIT_TEST(testTdf76441); CPPUNIT_TEST(testTdf142186); @@ -3540,6 +3542,110 @@ void Test::testAutoFilterTimeValue() m_pDoc->DeleteTab(0); } +void Test::testAutofilterOptimizations() +{ + m_pDoc->InsertTab( 0, "Test" ); + + constexpr SCCOL nCols = 4; + constexpr SCROW nRows = 200; + m_pDoc->SetString(0, 0, 0, "Column1"); + m_pDoc->SetString(1, 0, 0, "Column2"); + m_pDoc->SetString(2, 0, 0, "Column3"); + m_pDoc->SetString(3, 0, 0, "Column4"); + + // Fill 1st column with 0-199, 2nd with 1-200, 3rd with "1000"-"1199", 4th with "1001-1200" + // (the pairs are off by one to each other to check filtering out a value filters out + // only the relevant column). + for(SCROW i = 0; i < nRows; ++i) + { + m_pDoc->SetValue(0, i + 1, 0, i); + m_pDoc->SetValue(1, i + 1, 0, i+1); + m_pDoc->SetString(2, i + 1, 0, "val" + OUString::number(i+1000)); + m_pDoc->SetString(3, i + 1, 0, "val" + OUString::number(i+1000+1)); + } + + ScDBData* pDBData = new ScDBData("NONAME", 0, 0, 0, nCols, nRows); + m_pDoc->SetAnonymousDBData(0, std::unique_ptr<ScDBData>(pDBData)); + + pDBData->SetAutoFilter(true); + ScRange aRange; + pDBData->GetArea(aRange); + m_pDoc->ApplyFlagsTab( aRange.aStart.Col(), aRange.aStart.Row(), + aRange.aEnd.Col(), aRange.aStart.Row(), + aRange.aStart.Tab(), ScMF::Auto); + + //create the query param + ScQueryParam aParam; + pDBData->GetQueryParam(aParam); + ScQueryEntry& rEntry0 = aParam.GetEntry(0); + rEntry0.bDoQuery = true; + rEntry0.nField = 0; + rEntry0.eOp = SC_EQUAL; + rEntry0.GetQueryItems().resize(nRows); + ScQueryEntry& rEntry1 = aParam.GetEntry(1); + rEntry1.bDoQuery = true; + rEntry1.nField = 1; + rEntry1.eOp = SC_EQUAL; + rEntry1.GetQueryItems().resize(nRows); + ScQueryEntry& rEntry2 = aParam.GetEntry(2); + rEntry2.bDoQuery = true; + rEntry2.nField = 2; + rEntry2.eOp = SC_EQUAL; + rEntry2.GetQueryItems().resize(nRows); + ScQueryEntry& rEntry3 = aParam.GetEntry(3); + rEntry3.bDoQuery = true; + rEntry3.nField = 3; + rEntry3.eOp = SC_EQUAL; + rEntry3.GetQueryItems().resize(nRows); + // Set up autofilter to select all values except one in each column. + // This should only filter out 2nd, 3rd, 6th and 7th rows. + for( int i = 0; i < nRows; ++i ) + { + if(i!= 1) + rEntry0.GetQueryItems()[i].mfVal = i; + if(i!= 2) + rEntry1.GetQueryItems()[i].mfVal = i + 1; + if(i!= 5) + { + rEntry2.GetQueryItems()[i].maString = m_pDoc->GetSharedStringPool().intern("val" + OUString::number(i+1000)); + rEntry2.GetQueryItems()[i].meType = ScQueryEntry::ByString; + } + if(i!= 6) + { + rEntry3.GetQueryItems()[i].maString = m_pDoc->GetSharedStringPool().intern("val" + OUString::number(i+1000+1)); + rEntry3.GetQueryItems()[i].meType = ScQueryEntry::ByString; + } + } + // add queryParam to database range. + pDBData->SetQueryParam(aParam); + + // perform the query. + m_pDoc->Query(0, aParam, true); + + // check that only rows with filtered out values are hidden, and not rows that share + // a value in a different column + SCROW nRow1, nRow2; + CPPUNIT_ASSERT_MESSAGE("row 2 should be visible", !m_pDoc->RowHidden(1, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 3 should be hidden", m_pDoc->RowHidden(2, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 4 should be hidden", m_pDoc->RowHidden(3, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 5 should be visible", !m_pDoc->RowHidden(4, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 6 should be visible", !m_pDoc->RowHidden(5, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 7 should be hidden", m_pDoc->RowHidden(6, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 8 should be hidden", m_pDoc->RowHidden(7, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_MESSAGE("row 9 should be visible", !m_pDoc->RowHidden(8, 0, &nRow1, &nRow2)); + + // Remove filtering. + rEntry0.Clear(); + rEntry1.Clear(); + rEntry2.Clear(); + m_pDoc->Query(0, aParam, true); + CPPUNIT_ASSERT_MESSAGE("All rows should be shown.", !m_pDoc->RowHidden(0, 0, &nRow1, &nRow2)); + CPPUNIT_ASSERT_EQUAL_MESSAGE("All rows should be shown.", SCROW(0), nRow1); + CPPUNIT_ASSERT_EQUAL_MESSAGE("All rows should be shown.", SCROW(MAXROW), nRow2); + + m_pDoc->DeleteTab(0); +} + void Test::testTdf76441() { m_pDoc->InsertTab(0, "Test"); diff --git a/sc/source/core/data/queryevaluator.cxx b/sc/source/core/data/queryevaluator.cxx index f5241839221d..7fe9c4e0e302 100644 --- a/sc/source/core/data/queryevaluator.cxx +++ b/sc/source/core/data/queryevaluator.cxx @@ -690,7 +690,7 @@ std::pair<bool, bool> ScQueryEvaluator::compareByRangeLookup(const ScRefCellValu } std::pair<bool, bool> ScQueryEvaluator::processEntry(SCROW nRow, SCCOL nCol, ScRefCellValue& aCell, - const ScQueryEntry& rEntry) + const ScQueryEntry& rEntry, size_t nEntryIndex) { std::pair<bool, bool> aRes(false, false); const ScQueryEntry::QueryItemsType& rItems = rEntry.GetQueryItems(); @@ -731,16 +731,17 @@ std::pair<bool, bool> ScQueryEvaluator::processEntry(SCROW nRow, SCCOL nCol, ScR { // Sort, cache and binary search for the value in items. // Don't bother comparing approximately. - auto& values = mCachedSortedItemValues; - if (!mCachedSortedItemValuesReady) + if (mCachedSortedItemValues.size() <= nEntryIndex) { + mCachedSortedItemValues.resize(nEntryIndex + 1); + auto& values = mCachedSortedItemValues[nEntryIndex]; values.reserve(rItems.size()); for (const auto& rItem : rItems) if (rItem.meType == ScQueryEntry::ByValue) values.push_back(rItem.mfVal); std::sort(values.begin(), values.end()); - mCachedSortedItemValuesReady = true; } + auto& values = mCachedSortedItemValues[nEntryIndex]; auto it = std::lower_bound(values.begin(), values.end(), value); if (it != values.end() && *it == value) return std::make_pair(true, true); @@ -787,9 +788,10 @@ std::pair<bool, bool> ScQueryEvaluator::processEntry(SCROW nRow, SCCOL nCol, ScR // Sort, cache and binary search for the string in items. // Since each SharedString is identified by pointer value, // sorting by pointer value is enough. - auto& values = mCachedSortedItemStrings; - if (!mCachedSortedItemStringsReady) + if (mCachedSortedItemStrings.size() <= nEntryIndex) { + mCachedSortedItemStrings.resize(nEntryIndex + 1); + auto& values = mCachedSortedItemStrings[nEntryIndex]; values.reserve(rItems.size()); for (const auto& rItem : rItems) { @@ -802,8 +804,8 @@ std::pair<bool, bool> ScQueryEvaluator::processEntry(SCROW nRow, SCCOL nCol, ScR } } std::sort(values.begin(), values.end()); - mCachedSortedItemStringsReady = true; } + auto& values = mCachedSortedItemStrings[nEntryIndex]; const rtl_uString* string = mrParam.bCaseSens ? cellSharedString->getData() : cellSharedString->getDataIgnoreCase(); @@ -917,7 +919,7 @@ bool ScQueryEvaluator::ValidQuery(SCROW nRow, const ScRefCellValue* pCell, else aCell = mrTab.GetCellValue(nCol, nRow); - std::pair<bool, bool> aRes = processEntry(nRow, nCol, aCell, rEntry); + std::pair<bool, bool> aRes = processEntry(nRow, nCol, aCell, rEntry, it - itBeg); if (nPos == -1) {
