Merge pull request #7118 from m-kuhn/stringCompOperators

Cast left node to text on string comparison

python/core/auto_generated/qgsfeatureiterator.sip.in

@@ -65,6 +65,14 @@ after a timeout to give the system some time to stay responsive).
 If you want to check if the iterator successfully completed, better use :py:func:`QgsFeatureIterator.isClosed()`
 
 .. versionadded:: 3.0
+%End
+
+    bool compileFailed() const;
+%Docstring
+Indicator if there was an error when sending the compiled query to the server.
+This indicates that there is something wrong with the expression compiler.
+
+.. versionadded:: 3.2
 %End
 
   protected:
@@ -144,6 +152,7 @@ Remove reference, delete if refs == 0
 
 
 
+
     virtual bool prepareSimplification( const QgsSimplifyMethod &simplifyMethod );
 %Docstring
 Setup the simplification of geometries to fetch using the specified simplify method
@@ -251,6 +260,14 @@ Returns the status of expression compilation for filter expression requests.
 .. versionadded:: 2.16
 %End
 
+    bool compileFailed() const;
+%Docstring
+Indicator if there was an error when sending the compiled query to the server.
+This indicates that there is something wrong with the expression compiler.
+
+.. versionadded:: 3.2
+%End
+
 
   protected:
 

src/core/qgsfeatureiterator.cpp

@@ -147,6 +147,11 @@ void QgsAbstractFeatureIterator::deref()
     delete this;
 }
 
+bool QgsAbstractFeatureIterator::compileFailed() const
+{
+  return mCompileFailed;
+}
+
 bool QgsAbstractFeatureIterator::prepareSimplification( const QgsSimplifyMethod &simplifyMethod )
 {
   Q_UNUSED( simplifyMethod );

src/core/qgsfeatureiterator.h

@@ -83,6 +83,14 @@ class CORE_EXPORT QgsAbstractFeatureIterator
       return mValid;
     }
 
+    /**
+     * Indicator if there was an error when sending the compiled query to the server.
+     * This indicates that there is something wrong with the expression compiler.
+     *
+     * \since QGIS 3.2
+     */
+    bool compileFailed() const;
+
   protected:
 
     /**
@@ -173,6 +181,8 @@ class CORE_EXPORT QgsAbstractFeatureIterator
     //! Status of compilation of filter expression
     CompileStatus mCompileStatus = NoCompilation;
 
+    bool mCompileFailed = false;
+
     //! Setup the simplification of geometries to fetch using the specified simplify method
     virtual bool prepareSimplification( const QgsSimplifyMethod &simplifyMethod );
 
@@ -323,6 +333,14 @@ class CORE_EXPORT QgsFeatureIterator
      */
     QgsAbstractFeatureIterator::CompileStatus compileStatus() const { return mIter->compileStatus(); }
 
+    /**
+     * Indicator if there was an error when sending the compiled query to the server.
+     * This indicates that there is something wrong with the expression compiler.
+     *
+     * \since QGIS 3.2
+     */
+    bool compileFailed() const { return mIter->compileFailed(); }
+
     friend bool operator== ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 ) SIP_SKIP;
     friend bool operator!= ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 ) SIP_SKIP;
 

src/core/qgssqlexpressioncompiler.cpp

@@ -38,6 +38,18 @@ QString QgsSqlExpressionCompiler::result()
   return mResult;
 }
 
+bool QgsSqlExpressionCompiler::opIsStringComparison( QgsExpressionNodeBinaryOperator::BinaryOperator op )
+{
+  if ( op == QgsExpressionNodeBinaryOperator::BinaryOperator::boILike ||
+       op == QgsExpressionNodeBinaryOperator::BinaryOperator::boLike ||
+       op == QgsExpressionNodeBinaryOperator::BinaryOperator::boNotILike ||
+       op == QgsExpressionNodeBinaryOperator::BinaryOperator::boNotLike ||
+       op == QgsExpressionNodeBinaryOperator::BinaryOperator::boRegexp )
+    return true;
+  else
+    return false;
+}
+
 QString QgsSqlExpressionCompiler::quotedIdentifier( const QString &identifier )
 {
   QString quoted = identifier;
@@ -253,6 +265,9 @@ QgsSqlExpressionCompiler::Result QgsSqlExpressionCompiler::compileNode( const Qg
       QString left;
       Result lr( compileNode( n->opLeft(), left ) );
 
+      if ( opIsStringComparison( n ->op() ) )
+        left = castToText( left );
+
       QString right;
       Result rr( compileNode( n->opRight(), right ) );
 
@@ -409,6 +424,11 @@ QString QgsSqlExpressionCompiler::castToReal( const QString &value ) const
   return QString();
 }
 
+QString QgsSqlExpressionCompiler::castToText( const QString &value ) const
+{
+  return value;
+}
+
 QString QgsSqlExpressionCompiler::castToInt( const QString &value ) const
 {
   Q_UNUSED( value );

src/core/qgssqlexpressioncompiler.h

@@ -20,6 +20,7 @@
 
 #include "qgis_core.h"
 #include "qgsfields.h"
+#include "qgsexpressionnodeimpl.h"
 
 class QgsExpression;
 class QgsExpressionNode;
@@ -80,6 +81,22 @@ class CORE_EXPORT QgsSqlExpressionCompiler
      */
     virtual QString result();
 
+    /**
+     * Returns true if \a op is one of
+     *
+     * - LIKE
+     * - ILIKE
+     * - NOT LIKE
+     * - NOT ILIKE
+     * - ~ (regexp)
+     *
+     * In such cases the left operator will be cast to string to behave equal to
+     * QGIS own expression engine.
+     *
+     * \since QGIS 3.2
+     */
+    bool opIsStringComparison( QgsExpressionNodeBinaryOperator::BinaryOperator op );
+
   protected:
 
     /**
@@ -132,6 +149,23 @@ class CORE_EXPORT QgsSqlExpressionCompiler
      */
     virtual QString castToReal( const QString &value ) const;
 
+    /**
+     * Casts a value to a text result. Subclasses that support casting to text may implement this function
+     * to get equal behavior to the QGIS expression engine when string comparison operators are applied
+     * on non-string data.
+     *
+     * Example:
+     *
+     *     579 LIKE '5%'
+     *
+     * which on a postgres database needs to be
+     *
+     *     579::text LIKE '5%'
+     *
+     * \since QGIS 3.2
+     */
+    virtual QString castToText( const QString &value ) const;
+
     /**
      * Casts a value to a integer result. Subclasses must reimplement this to cast a numeric value to a integer
      * type value so that integer division results in a integer value result instead of real.

src/core/qgssqliteexpressioncompiler.cpp

@@ -111,4 +111,9 @@ QString QgsSQLiteExpressionCompiler::castToInt( const QString &value ) const
   return QStringLiteral( "CAST((%1) AS INTEGER)" ).arg( value );
 }
 
+QString QgsSQLiteExpressionCompiler::castToText( const QString &value ) const
+{
+  return QStringLiteral( "CAST((%1) AS TEXT)" ).arg( value );
+}
+
 ///@endcond

src/core/qgssqliteexpressioncompiler.h

@@ -52,6 +52,7 @@ class CORE_EXPORT QgsSQLiteExpressionCompiler : public QgsSqlExpressionCompiler
     QString sqlFunctionFromFunctionName( const QString &fnName ) const override;
     QString castToReal( const QString &value ) const override;
     QString castToInt( const QString &value ) const override;
+    QString castToText( const QString &value ) const override;
 
 };
 

src/providers/postgres/qgspostgresexpressioncompiler.cpp

@@ -144,6 +144,11 @@ QString QgsPostgresExpressionCompiler::castToInt( const QString &value ) const
   return QStringLiteral( "((%1)::int)" ).arg( value );
 }
 
+QString QgsPostgresExpressionCompiler::castToText( const QString &value ) const
+{
+  return QStringLiteral( "((%1)::text)" ).arg( value );
+}
+
 QgsSqlExpressionCompiler::Result QgsPostgresExpressionCompiler::compileNode( const QgsExpressionNode *node, QString &result )
 {
   switch ( node->nodeType() )

src/providers/postgres/qgspostgresexpressioncompiler.h

@@ -36,6 +36,7 @@ class QgsPostgresExpressionCompiler : public QgsSqlExpressionCompiler
     QStringList sqlArgumentsFromFunctionName( const QString &fnName, const QStringList &fnArgs ) const override;
     QString castToReal( const QString &value ) const override;
     QString castToInt( const QString &value ) const override;
+    QString castToText( const QString &value ) const override;
 
     QString mGeometryColumn;
     QgsPostgresGeometryColumnType mSpatialColType;

src/providers/postgres/qgspostgresfeatureiterator.cpp

@@ -210,7 +210,10 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
       //try with the fallback where clause, e.g., for cases when using compiled expression failed to prepare
       success = declareCursor( fallbackWhereClause, -1, false, orderByParts.join( QStringLiteral( "," ) ) );
       if ( success )
+      {
         mExpressionCompiled = false;
+        mCompileFailed = true;
+      }
     }
 
     if ( !success && !orderByParts.isEmpty() )

src/providers/spatialite/qgsspatialitefeatureiterator.cpp

@@ -213,6 +213,7 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
       //try with the fallback where clause, e.g., for cases when using compiled expression failed to prepare
       mExpressionCompiled = false;
       success = prepareStatement( fallbackWhereClause, -1, orderByParts.join( QStringLiteral( "," ) ) );
+      mCompileFailed = true;
     }
 
     if ( !success )

tests/src/python/providertestbase.py

@@ -52,6 +52,11 @@ class ProviderTestCase(FeatureSourceTestCase):
         cannot be compiled """
         return set()
 
+    def enableCompiler(self):
+        """By default there is no expression compiling available, needs to be overridden in subclass"""
+        print('Provider does not support compiling')
+        return False
+
     def partiallyCompiledFilters(self):
         """ Individual derived provider tests should override this to return a list of expressions which
         should be partially compiled """
@@ -141,14 +146,11 @@ class ProviderTestCase(FeatureSourceTestCase):
             self.runPolyGetFeatureTests(self.poly_provider)
 
     def testGetFeaturesExp(self):
-        try:
-            self.enableCompiler()
+        if self.enableCompiler():
             self.compiled = True
             self.runGetFeatureTests(self.source)
             if hasattr(self, 'poly_provider'):
                 self.runPolyGetFeatureTests(self.poly_provider)
-        except AttributeError:
-            print('Provider does not support compiling')
 
     def testSubsetString(self):
         if not self.source.supportsSubsetString():
@@ -232,11 +234,8 @@ class ProviderTestCase(FeatureSourceTestCase):
         self.runOrderByTests()
 
     def testOrderByCompiled(self):
-        try:
-            self.enableCompiler()
+        if self.enableCompiler():
             self.runOrderByTests()
-        except AttributeError:
-            print('Provider does not support compiling')
 
     def runOrderByTests(self):
         FeatureSourceTestCase.runOrderByTests(self)
@@ -817,3 +816,24 @@ class ProviderTestCase(FeatureSourceTestCase):
             self.assertTrue(vl.dataProvider().deleteAttributes([0]))
             self.assertEqual(vl.dataProvider().minimumValue(0), -200)
             self.assertEqual(vl.dataProvider().maximumValue(0), 400)
+
+    def testStringComparison(self):
+        """
+        Test if string comparisons with numbers are cast by the expression
+        compiler (or work fine without doing anything :P)
+        """
+        for expression in (
+                '5 LIKE \'5\'',
+                '5 ILIKE \'5\'',
+                '15 NOT LIKE \'5\'',
+                '15 NOT ILIKE \'5\'',
+                '5 ~ \'5\''):
+            iterator = self.source.getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'5\''))
+            count = len([f for f in iterator])
+            self.assertEqual(count, 5)
+            self.assertFalse(iterator.compileFailed())
+            if self.enableCompiler():
+                iterator = self.source.getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'5\''))
+                self.assertEqual(count, 5)
+                self.assertFalse(iterator.compileFailed())
+                self.disableCompiler()

tests/src/python/test_provider_db2.py

@@ -58,6 +58,7 @@ class TestPyQgsDb2Provider(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)

tests/src/python/test_provider_mssql.py

@@ -52,6 +52,7 @@ class TestPyQgsMssqlProvider(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)

tests/src/python/test_provider_oracle.py

@@ -85,6 +85,7 @@ class TestPyQgsOracleProvider(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)

tests/src/python/test_provider_postgres.py

@@ -104,6 +104,7 @@ class TestPyQgsPostgresProvider(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)
@@ -1125,6 +1126,7 @@ class TestPyQgsPostgresProviderCompoundKey(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)

tests/src/python/test_provider_shapefile.py

@@ -93,6 +93,7 @@ class TestPyQgsShapefileProvider(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)

tests/src/python/test_provider_spatialite.py

@@ -260,6 +260,7 @@ class TestQgsSpatialiteProvider(unittest.TestCase, ProviderTestCase):
 
     def enableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', True)
+        return True
 
     def disableCompiler(self):
         QgsSettings().setValue('/qgis/compileExpressions', False)