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 <erack@redhat.com>
Tested-by: Jenkins
(cherry picked from commit b60b6bfaafa1315e07108dba50f016975b619c59)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/122735
Reviewed-by: Xisco Fauli <xiscofauli@libreoffice.org>

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 9f9c697e709e..72f5d3448fd6 100644
--- a/sc/source/core/data/column3.cxx
+++ b/sc/source/core/data/column3.cxx
@@ -2551,7 +2551,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 554b69b90203..14a9c7f35884 100644
--- a/sc/source/core/data/table3.cxx
+++ b/sc/source/core/data/table3.cxx
@@ -2445,6 +2445,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,
@@ -2453,23 +2464,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);
@@ -2483,51 +2513,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:
             {
@@ -3223,11 +3270,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;
 
@@ -3269,7 +3320,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();
@@ -3281,7 +3332,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 )
         {
@@ -3312,7 +3363,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)
@@ -3330,7 +3381,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)
     {
@@ -3741,7 +3792,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() :