From 6a88bfcaeb10effa700db22adb79bbb1e04f20ca Mon Sep 17 00:00:00 2001 From: Matthias Kuhn Date: Wed, 30 May 2018 16:44:03 +0200 Subject: [PATCH 1/4] Cast left node to text on string comparison --- src/core/qgssqlexpressioncompiler.cpp | 20 +++++++++++ src/core/qgssqlexpressioncompiler.h | 34 +++++++++++++++++++ src/core/qgssqliteexpressioncompiler.cpp | 5 +++ src/core/qgssqliteexpressioncompiler.h | 1 + .../qgspostgresexpressioncompiler.cpp | 5 +++ .../postgres/qgspostgresexpressioncompiler.h | 1 + 6 files changed, 66 insertions(+) diff --git a/src/core/qgssqlexpressioncompiler.cpp b/src/core/qgssqlexpressioncompiler.cpp index 5fcc179124d..bc151113a6c 100644 --- a/src/core/qgssqlexpressioncompiler.cpp +++ b/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 ); diff --git a/src/core/qgssqlexpressioncompiler.h b/src/core/qgssqlexpressioncompiler.h index 02d1aab76f2..ac63aa72441 100644 --- a/src/core/qgssqlexpressioncompiler.h +++ b/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. diff --git a/src/core/qgssqliteexpressioncompiler.cpp b/src/core/qgssqliteexpressioncompiler.cpp index afdabda8364..ee9cd426cdf 100644 --- a/src/core/qgssqliteexpressioncompiler.cpp +++ b/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 diff --git a/src/core/qgssqliteexpressioncompiler.h b/src/core/qgssqliteexpressioncompiler.h index 6e6103d1743..f9d83301c9c 100644 --- a/src/core/qgssqliteexpressioncompiler.h +++ b/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; }; diff --git a/src/providers/postgres/qgspostgresexpressioncompiler.cpp b/src/providers/postgres/qgspostgresexpressioncompiler.cpp index 038a29a2c82..582ebcbdb1b 100644 --- a/src/providers/postgres/qgspostgresexpressioncompiler.cpp +++ b/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() ) diff --git a/src/providers/postgres/qgspostgresexpressioncompiler.h b/src/providers/postgres/qgspostgresexpressioncompiler.h index 32104823efc..34d9430d7e8 100644 --- a/src/providers/postgres/qgspostgresexpressioncompiler.h +++ b/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; From dbe4186a9c6d9820b26dbe63ada5445d2c38374b Mon Sep 17 00:00:00 2001 From: Matthias Kuhn Date: Thu, 31 May 2018 14:04:23 +0200 Subject: [PATCH 2/4] Add tests for compiling string operators --- .../auto_generated/qgsfeatureiterator.sip.in | 17 ++++++++++++++ src/core/qgsfeatureiterator.cpp | 5 +++++ src/core/qgsfeatureiterator.h | 18 +++++++++++++++ .../postgres/qgspostgresfeatureiterator.cpp | 3 +++ .../qgsspatialitefeatureiterator.cpp | 1 + tests/src/python/providertestbase.py | 22 +++++++++++++++++++ 6 files changed, 66 insertions(+) diff --git a/python/core/auto_generated/qgsfeatureiterator.sip.in b/python/core/auto_generated/qgsfeatureiterator.sip.in index 6f50f128122..1abf5a8d663 100644 --- a/python/core/auto_generated/qgsfeatureiterator.sip.in +++ b/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: diff --git a/src/core/qgsfeatureiterator.cpp b/src/core/qgsfeatureiterator.cpp index 96683a9f5f5..e1044fef6e8 100644 --- a/src/core/qgsfeatureiterator.cpp +++ b/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 ); diff --git a/src/core/qgsfeatureiterator.h b/src/core/qgsfeatureiterator.h index 7f36c440d62..ad8526d9d91 100644 --- a/src/core/qgsfeatureiterator.h +++ b/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; diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp index 5da7b2cffc8..709cc3f3e59 100644 --- a/src/providers/postgres/qgspostgresfeatureiterator.cpp +++ b/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() ) diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp index 763165b99fa..ccbe88fc6fd 100644 --- a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp +++ b/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 ) diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py index deb3e31d602..f2c70e74012 100644 --- a/tests/src/python/providertestbase.py +++ b/tests/src/python/providertestbase.py @@ -817,3 +817,25 @@ 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) + """ + vl = self.getEditableLayer() + for expression in ( + '5 LIKE \'5\'', + '5 ILIKE \'5\'', + '15 NOT LIKE \'5\'', + '15 NOT ILIKE \'5\'', + '5 ~ \'5\''): + iterator = vl.dataProvider().getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'5\'')) + count = len([f for f in iterator]) + self.assertEqual(count, 5) + self.assertFalse(iterator.compileFailed()) + self.enableCompiler() + iterator = vl.dataProvider().getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'5\'')) + self.assertEqual(count, 5) + self.assertFalse(iterator.compileFailed()) + self.disableCompiler() From d1eabe1c2b3babeddc3996a05c6261175596c250 Mon Sep 17 00:00:00 2001 From: Matthias Kuhn Date: Thu, 31 May 2018 18:26:43 +0200 Subject: [PATCH 3/4] Make compiled tests more stable we do not want to ignore AttributeErrors for those --- tests/src/python/providertestbase.py | 15 +++++++-------- tests/src/python/test_provider_db2.py | 1 + tests/src/python/test_provider_mssql.py | 1 + tests/src/python/test_provider_oracle.py | 1 + tests/src/python/test_provider_postgres.py | 2 ++ tests/src/python/test_provider_shapefile.py | 1 + tests/src/python/test_provider_spatialite.py | 1 + 7 files changed, 14 insertions(+), 8 deletions(-) diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py index f2c70e74012..a4930ebb3bc 100644 --- a/tests/src/python/providertestbase.py +++ b/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) diff --git a/tests/src/python/test_provider_db2.py b/tests/src/python/test_provider_db2.py index 68566a47a2f..32b5ba91579 100644 --- a/tests/src/python/test_provider_db2.py +++ b/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) diff --git a/tests/src/python/test_provider_mssql.py b/tests/src/python/test_provider_mssql.py index d841c112991..cdae4842b2d 100644 --- a/tests/src/python/test_provider_mssql.py +++ b/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) diff --git a/tests/src/python/test_provider_oracle.py b/tests/src/python/test_provider_oracle.py index bda24a7a48d..0fed49e6237 100644 --- a/tests/src/python/test_provider_oracle.py +++ b/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) diff --git a/tests/src/python/test_provider_postgres.py b/tests/src/python/test_provider_postgres.py index b34861d2bcc..d0978ddda34 100644 --- a/tests/src/python/test_provider_postgres.py +++ b/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) diff --git a/tests/src/python/test_provider_shapefile.py b/tests/src/python/test_provider_shapefile.py index d97e377fa94..d833c8d6402 100644 --- a/tests/src/python/test_provider_shapefile.py +++ b/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) diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py index bae1499490d..aa66c15ef87 100644 --- a/tests/src/python/test_provider_spatialite.py +++ b/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) From 48bbd2460c793d11f97452759315309100f5e19c Mon Sep 17 00:00:00 2001 From: Matthias Kuhn Date: Thu, 31 May 2018 18:26:18 +0200 Subject: [PATCH 4/4] Run string comparison test on all providers --- tests/src/python/providertestbase.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/tests/src/python/providertestbase.py b/tests/src/python/providertestbase.py index a4930ebb3bc..16bc24e847c 100644 --- a/tests/src/python/providertestbase.py +++ b/tests/src/python/providertestbase.py @@ -822,19 +822,18 @@ class ProviderTestCase(FeatureSourceTestCase): Test if string comparisons with numbers are cast by the expression compiler (or work fine without doing anything :P) """ - vl = self.getEditableLayer() for expression in ( '5 LIKE \'5\'', '5 ILIKE \'5\'', '15 NOT LIKE \'5\'', '15 NOT ILIKE \'5\'', '5 ~ \'5\''): - iterator = vl.dataProvider().getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'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()) - self.enableCompiler() - iterator = vl.dataProvider().getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'5\'')) - self.assertEqual(count, 5) - self.assertFalse(iterator.compileFailed()) - self.disableCompiler() + if self.enableCompiler(): + iterator = self.source.getFeatures(QgsFeatureRequest().setFilterExpression('5 LIKE \'5\'')) + self.assertEqual(count, 5) + self.assertFalse(iterator.compileFailed()) + self.disableCompiler()