sc/inc/queryentry.hxx | 9 +-
sc/source/core/data/column3.cxx | 2
sc/source/core/data/table3.cxx | 121 ++++++++++++++++++++++++++-----------
sc/source/core/tool/queryentry.cxx | 3
4 files changed, 94 insertions(+), 41 deletions(-)
New commits:
commit dc1642c3225ff4aeada5df749946f406772eb1ff
Author: Eike Rathke <er...@redhat.com>
AuthorDate: Tue Sep 28 00:19:47 2021 +0200
Commit: Adolfo Jayme Barrientos <fit...@ubuntu.com>
CommitDate: Wed Sep 29 06:32:18 2021 +0200
Resolves: tdf#144740 Fix broken compareByValue() query, tdf#142910
tdf#144253
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 <er...@redhat.com>
Tested-by: Jenkins
(cherry picked from commit b60b6bfaafa1315e07108dba50f016975b619c59)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/122736
Reviewed-by: Xisco Fauli <xiscofa...@libreoffice.org>
Reviewed-by: Mike Kaganski <mike.kagan...@collabora.com>
Reviewed-by: Adolfo Jayme Barrientos <fit...@ubuntu.com>
Tested-by: Adolfo Jayme Barrientos <fit...@ubuntu.com>
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 49c646073e27..ab34d8297c27 100644
--- a/sc/source/core/data/column3.cxx
+++ b/sc/source/core/data/column3.cxx
@@ -2500,7 +2500,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 5caea5500fda..08be06aa15b7 100644
--- a/sc/source/core/data/table3.cxx
+++ b/sc/source/core/data/table3.cxx
@@ -2443,6 +2443,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,
@@ -2451,23 +2462,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);
@@ -2481,51 +2511,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:
{
@@ -3164,11 +3211,15 @@ bool CanOptimizeQueryStringToNumber( SvNumberFormatter*
pFormatter, sal_uInt32 n
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;
@@ -3210,7 +3261,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();
@@ -3222,7 +3273,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 )
{
@@ -3253,7 +3304,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)
@@ -3271,7 +3322,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)
{
@@ -3682,7 +3733,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 121c257e0cf6..bb8c44ba4915 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() :
