diff options
author | Eike Rathke <erack@redhat.com> | 2021-09-28 00:19:47 +0200 |
---|---|---|
committer | Eike Rathke <erack@redhat.com> | 2021-09-28 02:25:22 +0200 |
commit | b60b6bfaafa1315e07108dba50f016975b619c59 (patch) | |
tree | c0f39a3277f08e02481a94f8507187563ebb6a4c /sc | |
parent | 540ae5590f775a04168453f8c61a839303f598ef (diff) |
Fix regression of a series of commits that, intended for filter
queries, unconditionally round numeric values as shown under
ScTable::ValidQuery() and compareByValue() without having taken
into account that the same query and compare functions are used by
the interpreter for all functions that use query criteria,
possibly delivering completely wrong results including in
backports to 7.2.0.0
commit f6b143a57d9bd8f5d7b29febcb4e01ee1eb2ff1d
CommitDate: Wed Jul 7 17:44:46 2021 +0200
tdf#142910 sc filter: fix "greater than" or "smaller than" etc
commit 51375b48378915b6e95c1ac26b2ccf8e39880f7e
CommitDate: Tue Sep 21 11:06:35 2021 +0200
tdf#144253 tdf#144324 sc filter: use formatted values in filters
Several related and intertwined commits in filter context make
assumptions about these queries always being executed rounded, so
the only clean solution is to make that depend on the
ScQueryEntry::Item being passed. Its mbRoundForFilter value is set
to true for all items of all queries executed via ScTable::Query()
and ScTable::GetFilteredFilterEntries(). It might be not all are
necessary (or some even still harmful?) and unnecessarily
obtaining number formats and calling RoundValueAsShown() is still
a bottle neck for those, but that should be addressed and reworked
independently. The important part is calculations work as before.
Also, moved obtaining number formats for calling RoundValueAsShown()
into logic that calls them only if necessary.
Note the TODO in compareByValue() about suspicious rounding of
rItem.mfVal in filter context that is to be addressed.
Change-Id: Ieb178ad1ea15a635caeb1ba698c2f4b7ad676d57
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/122729
Reviewed-by: Eike Rathke <erack@redhat.com>
Tested-by: Jenkins
Diffstat (limited to 'sc')
-rw-r--r-- | sc/inc/queryentry.hxx | 9 | ||||
-rw-r--r-- | sc/source/core/data/column3.cxx | 2 | ||||
-rw-r--r-- | sc/source/core/data/table3.cxx | 121 | ||||
-rw-r--r-- | sc/source/core/tool/queryentry.cxx | 3 |
4 files changed, 94 insertions, 41 deletions
diff --git a/sc/inc/queryentry.hxx b/sc/inc/queryentry.hxx index 94ea761c1239..9b0b1cd98124 100644 --- a/sc/inc/queryentry.hxx +++ b/sc/inc/queryentry.hxx @@ -44,13 +44,14 @@ struct SC_DLLPUBLIC ScQueryEntry struct SAL_DLLPRIVATE Item { - QueryType meType; - double mfVal; + QueryType meType; + double mfVal; svl::SharedString maString; + Color maColor; bool mbMatchEmpty; - Color maColor; + bool mbRoundForFilter; - Item() : meType(ByValue), mfVal(0.0), mbMatchEmpty(false) {} + Item() : meType(ByValue), mfVal(0.0), mbMatchEmpty(false), mbRoundForFilter(false) {} bool operator== (const Item& r) const; }; diff --git a/sc/source/core/data/column3.cxx b/sc/source/core/data/column3.cxx index 60934b5ba8a0..79f001c12c91 100644 --- a/sc/source/core/data/column3.cxx +++ b/sc/source/core/data/column3.cxx @@ -2554,7 +2554,7 @@ class FilterEntriesHandler pFormatter->GetInputLineString(fVal, nIndex, aStr); } // store the formatted/rounded value for filtering - if (nFormat && !bDate) + if ((nFormat % SV_COUNTRY_LANGUAGE_OFFSET) != 0 && !bDate) mrFilterEntries.push_back(ScTypedStrData(aStr, fVal, rColumn.GetDoc().RoundValueAsShown(fVal, nFormat), ScTypedStrData::Value, bDate)); else mrFilterEntries.push_back(ScTypedStrData(aStr, fVal, fVal, ScTypedStrData::Value, bDate)); diff --git a/sc/source/core/data/table3.cxx b/sc/source/core/data/table3.cxx index 83d2a9cfa99f..a68145207588 100644 --- a/sc/source/core/data/table3.cxx +++ b/sc/source/core/data/table3.cxx @@ -2444,6 +2444,17 @@ public: return mrTab.HasStringData(nCol, nRow); } + sal_uInt32 getNumFmt( SCCOL nCol, SCROW nRow, const ScInterpreterContext* pContext ) + { + sal_uInt32 nNumFmt = (pContext ? + mrTab.GetNumberFormat(*pContext, ScAddress(nCol, nRow, mrTab.GetTab())) : + mrTab.GetNumberFormat(nCol, nRow)); + if (nNumFmt && (nNumFmt % SV_COUNTRY_LANGUAGE_OFFSET) == 0) + // Any General of any locale is irrelevant for rounding. + nNumFmt = 0; + return nNumFmt; + } + std::pair<bool,bool> compareByValue( const ScRefCellValue& rCell, SCCOL nCol, SCROW nRow, const ScQueryEntry& rEntry, const ScQueryEntry::Item& rItem, @@ -2452,23 +2463,42 @@ public: bool bOk = false; bool bTestEqual = false; double nCellVal; - double fRoundedValue = rItem.mfVal; - sal_uInt32 nNumFmt = pContext ? mrTab.GetNumberFormat(*pContext, ScAddress(nCol, nRow, mrTab.GetTab())) : - mrTab.GetNumberFormat(nCol, nRow); + double fQueryVal = rItem.mfVal; + // Defer all number format detection to as late as possible as it's a + // bottle neck, even if that complicates the code. Also do not + // unnecessarily call ScDocument::RoundValueAsShown() for the same + // reason. + sal_uInt32 nNumFmt = NUMBERFORMAT_ENTRY_NOT_FOUND; if (!rCell.isEmpty()) { switch (rCell.meType) { case CELLTYPE_VALUE : - nCellVal = mrDoc.RoundValueAsShown(rCell.mfValue, nNumFmt, pContext); + nCellVal = rCell.mfValue; break; case CELLTYPE_FORMULA : - nCellVal = mrDoc.RoundValueAsShown(rCell.mpFormula->GetValue(), nNumFmt, pContext); + nCellVal = rCell.mpFormula->GetValue(); break; default: nCellVal = 0.0; } + if (rItem.mbRoundForFilter && nCellVal != 0.0) + { + nNumFmt = getNumFmt( nCol, nRow, pContext); + if (nNumFmt) + { + switch (rCell.meType) + { + case CELLTYPE_VALUE : + case CELLTYPE_FORMULA : + nCellVal = mrDoc.RoundValueAsShown(nCellVal, nNumFmt, pContext); + break; + default: + assert(!"can't be"); + } + } + } } else nCellVal = mrTab.GetValue(nCol, nRow); @@ -2482,51 +2512,68 @@ public: if (rItem.meType == ScQueryEntry::ByDate) { - SvNumberFormatter* pFormatter = pContext ? pContext->GetFormatTable() : mrDoc.GetFormatTable(); - const SvNumberformat* pEntry = pFormatter->GetEntry(nNumFmt); - if (pEntry) + if (nNumFmt == NUMBERFORMAT_ENTRY_NOT_FOUND) + nNumFmt = getNumFmt( nCol, nRow, pContext); + if (nNumFmt) { - SvNumFormatType nNumFmtType = pEntry->GetType(); - /* NOTE: Omitting the check for absence of - * css::util::NumberFormat::TIME would include also date+time formatted - * values of the same day. That may be desired in some - * cases, querying all time values of a day, but confusing - * in other cases. A user can always setup a standard - * filter query for x >= date AND x < date+1 */ - if ((nNumFmtType & SvNumFormatType::DATE) && !(nNumFmtType & SvNumFormatType::TIME)) + SvNumberFormatter* pFormatter = pContext ? pContext->GetFormatTable() : mrDoc.GetFormatTable(); + const SvNumberformat* pEntry = pFormatter->GetEntry(nNumFmt); + if (pEntry) { - // The format is of date type. Strip off the time - // element. - nCellVal = ::rtl::math::approxFloor(nCellVal); + SvNumFormatType nNumFmtType = pEntry->GetType(); + /* NOTE: Omitting the check for absence of + * css::util::NumberFormat::TIME would include also date+time formatted + * values of the same day. That may be desired in some + * cases, querying all time values of a day, but confusing + * in other cases. A user can always setup a standard + * filter query for x >= date AND x < date+1 */ + if ((nNumFmtType & SvNumFormatType::DATE) && !(nNumFmtType & SvNumFormatType::TIME)) + { + // The format is of date type. Strip off the time + // element. + nCellVal = ::rtl::math::approxFloor(nCellVal); + } } } } - else if (nNumFmt) - fRoundedValue = mrDoc.RoundValueAsShown(rItem.mfVal, nNumFmt, pContext); + else if (rItem.mbRoundForFilter && fQueryVal != 0.0) + { + /* TODO: shouldn't rItem.mfVal (which fQueryVal is) already had + * been stored as rounded in all cases if needed so this extra + * rounding is superfluous? Or rather, if not, then rounding it + * here may produce different roundings for different cell number + * formats, which is odd. This all looks suspicious and the + * intention of tdf#142910 commit + * f6b143a57d9bd8f5d7b29febcb4e01ee1eb2ff1d isn't quite clear. */ + if (nNumFmt == NUMBERFORMAT_ENTRY_NOT_FOUND) + nNumFmt = getNumFmt( nCol, nRow, pContext); + if (nNumFmt) + fQueryVal = mrDoc.RoundValueAsShown(fQueryVal, nNumFmt, pContext); + } switch (rEntry.eOp) { case SC_EQUAL : - bOk = ::rtl::math::approxEqual(nCellVal, fRoundedValue); + bOk = ::rtl::math::approxEqual(nCellVal, fQueryVal); break; case SC_LESS : - bOk = (nCellVal < fRoundedValue) && !::rtl::math::approxEqual(nCellVal, fRoundedValue); + bOk = (nCellVal < fQueryVal) && !::rtl::math::approxEqual(nCellVal, fQueryVal); break; case SC_GREATER : - bOk = (nCellVal > fRoundedValue) && !::rtl::math::approxEqual(nCellVal, fRoundedValue); + bOk = (nCellVal > fQueryVal) && !::rtl::math::approxEqual(nCellVal, fQueryVal); break; case SC_LESS_EQUAL : - bOk = (nCellVal < fRoundedValue) || ::rtl::math::approxEqual(nCellVal, fRoundedValue); + bOk = (nCellVal < fQueryVal) || ::rtl::math::approxEqual(nCellVal, fQueryVal); if ( bOk && mpTestEqualCondition ) - bTestEqual = ::rtl::math::approxEqual(nCellVal, fRoundedValue); + bTestEqual = ::rtl::math::approxEqual(nCellVal, fQueryVal); break; case SC_GREATER_EQUAL : - bOk = (nCellVal > fRoundedValue) || ::rtl::math::approxEqual( nCellVal, fRoundedValue); + bOk = (nCellVal > fQueryVal) || ::rtl::math::approxEqual( nCellVal, fQueryVal); if ( bOk && mpTestEqualCondition ) - bTestEqual = ::rtl::math::approxEqual(nCellVal, fRoundedValue); + bTestEqual = ::rtl::math::approxEqual(nCellVal, fQueryVal); break; case SC_NOT_EQUAL : - bOk = !::rtl::math::approxEqual(nCellVal, fRoundedValue); + bOk = !::rtl::math::approxEqual(nCellVal, fQueryVal); break; default: { @@ -3222,11 +3269,15 @@ bool CanOptimizeQueryStringToNumber( const SvNumberFormatter* pFormatter, sal_uI class PrepareQueryItem { const ScDocument& mrDoc; + const bool mbRoundForFilter; public: - explicit PrepareQueryItem(const ScDocument& rDoc) : mrDoc(rDoc) {} + explicit PrepareQueryItem(const ScDocument& rDoc, bool bRoundForFilter) : + mrDoc(rDoc), mbRoundForFilter(bRoundForFilter) {} void operator() (ScQueryEntry::Item& rItem) { + rItem.mbRoundForFilter = mbRoundForFilter; + if (rItem.meType != ScQueryEntry::ByString && rItem.meType != ScQueryEntry::ByDate) return; @@ -3268,7 +3319,7 @@ public: } }; -void lcl_PrepareQuery( const ScDocument* pDoc, ScTable* pTab, ScQueryParam& rParam ) +void lcl_PrepareQuery( const ScDocument* pDoc, ScTable* pTab, ScQueryParam& rParam, bool bRoundForFilter ) { bool bTopTen = false; SCSIZE nEntryCount = rParam.GetEntryCount(); @@ -3280,7 +3331,7 @@ void lcl_PrepareQuery( const ScDocument* pDoc, ScTable* pTab, ScQueryParam& rPar continue; ScQueryEntry::QueryItemsType& rItems = rEntry.GetQueryItems(); - std::for_each(rItems.begin(), rItems.end(), PrepareQueryItem(*pDoc)); + std::for_each(rItems.begin(), rItems.end(), PrepareQueryItem(*pDoc, bRoundForFilter)); if ( !bTopTen ) { @@ -3311,7 +3362,7 @@ void lcl_PrepareQuery( const ScDocument* pDoc, ScTable* pTab, ScQueryParam& rPar void ScTable::PrepareQuery( ScQueryParam& rQueryParam ) { - lcl_PrepareQuery(&rDocument, this, rQueryParam); + lcl_PrepareQuery(&rDocument, this, rQueryParam, false); } SCSIZE ScTable::Query(const ScQueryParam& rParamOrg, bool bKeepSub) @@ -3329,7 +3380,7 @@ SCSIZE ScTable::Query(const ScQueryParam& rParamOrg, bool bKeepSub) SCROW nOutRow = 0; SCROW nHeader = aParam.bHasHeader ? 1 : 0; - lcl_PrepareQuery(&rDocument, this, aParam); + lcl_PrepareQuery(&rDocument, this, aParam, true); if (!aParam.bInplace) { @@ -3740,7 +3791,7 @@ void ScTable::GetFilteredFilterEntries( ScQueryParam aParam( rParam ); aParam.RemoveEntryByField(nCol); - lcl_PrepareQuery(&rDocument, this, aParam); + lcl_PrepareQuery(&rDocument, this, aParam, true); for ( SCROW j = nRow1; j <= nRow2; ++j ) { if (ValidQuery(j, aParam)) diff --git a/sc/source/core/tool/queryentry.cxx b/sc/source/core/tool/queryentry.cxx index c94c6225f625..d66382d2e2b8 100644 --- a/sc/source/core/tool/queryentry.cxx +++ b/sc/source/core/tool/queryentry.cxx @@ -34,7 +34,8 @@ bool ScQueryEntry::Item::operator== (const Item& r) const { - return meType == r.meType && mfVal == r.mfVal && maString == r.maString && mbMatchEmpty == r.mbMatchEmpty; + return meType == r.meType && mfVal == r.mfVal && maString == r.maString && mbMatchEmpty == r.mbMatchEmpty + && mbRoundForFilter == r.mbRoundForFilter; } ScQueryEntry::ScQueryEntry() : |