Merge pull request #7612 from elpaso/bugfix-19441-rulerenderer-exp-depth

Bugfix 19441 rulerenderer exp depth
qgis · Aug 17, 2018 · ce87662 · ce87662
2 parents 6d1e420 + a8bc941
commit ce87662
Show file tree

Hide file tree

Showing 4 changed files with 174 additions and 0 deletions.
diff --git a/src/core/symbology/qgsrulebasedrenderer.cpp b/src/core/symbology/qgsrulebasedrenderer.cpp
@@ -409,9 +409,35 @@ bool QgsRuleBasedRenderer::Rule::startRender( QgsRenderContext &context, const Q
   if ( subfilters.length() > 1 || !subfilters.value( 0 ).isEmpty() )
   {
     if ( subfilters.contains( QStringLiteral( "TRUE" ) ) )
+    {
       sf = QStringLiteral( "TRUE" );
+    }
+    // If we have more than 50 rules (to stay on the safe side) make a binary tree or SQLITE will fail,
+    // see: http://issues.qgis.org/issues/19441
+    else if ( subfilters.count() > 50 )
+    {
+      std::function<QString( const QStringList & )>bt = [ &bt ]( const QStringList & subf )
+      {
+        if ( subf.count( ) == 1 )
+        {
+          return subf.at( 0 );
+        }
+        else if ( subf.count( ) == 2 )
+        {
+          return subf.join( QStringLiteral( ") OR (" ) ).prepend( '(' ).append( ')' );
+        }
+        else
+        {
+          int midpos = static_cast<int>( subf.length() / 2 );
+          return QStringLiteral( "(%1) OR (%2)" ).arg( bt( subf.mid( 0, midpos ) ) ).arg( bt( subf.mid( midpos ) ) );
+        }
+      };
+      sf = bt( subfilters );
+    }
     else
+    {
       sf = subfilters.join( QStringLiteral( ") OR (" ) ).prepend( '(' ).append( ')' );
+    }
   }
 
   // Now join the subfilters with their parent (this) based on if

diff --git a/tests/src/core/CMakeLists.txt b/tests/src/core/CMakeLists.txt
@@ -93,6 +93,7 @@ SET(TESTS
  testqgsdxfexport.cpp
  testqgsellipsemarker.cpp
  testqgsexpressioncontext.cpp
+ testqgssqliteexpressioncompiler.cpp
  testqgsexpression.cpp
  testqgsfeature.cpp
  testqgsfields.cpp

diff --git a/tests/src/core/testqgsrulebasedrenderer.cpp b/tests/src/core/testqgsrulebasedrenderer.cpp
@@ -143,6 +143,42 @@ class TestQgsRuleBasedRenderer: public QObject
       delete clone;
     }
 
+    /**
+     * test_many_rules_expression_filter checks that with > 50 rules we have a binary tree (log2(N))
+     */
+    void test_many_rules_expression_filter()
+    {
+
+      QgsVectorLayer *layer = new QgsVectorLayer( QStringLiteral( "point?field=fld:int" ), QStringLiteral( "x" ), QStringLiteral( "memory" ) );
+      QgsRenderContext ctx; // dummy render context
+      ctx.expressionContext().setFields( layer->fields() );
+
+      std::function<QString( const int ruleCount )> makeFilter = [ & ]( const int rc ) -> QString
+      {
+
+        // prepare renderer
+        RRule *rootRule = new RRule( nullptr );
+        for ( int i = 0; i < rc; i++ )
+        {
+          rootRule->appendChild( new RRule( nullptr, 0, 0, QStringLiteral( "%1" ).arg( i ) ) );
+        }
+        QgsRuleBasedRenderer r( rootRule );
+        r.startRender( ctx, layer->fields() );
+        r.stopRender( ctx );
+        return r.filter();
+      };
+
+      QCOMPARE( makeFilter( 1 ), QString( "(0)" ) );
+      QCOMPARE( makeFilter( 2 ), QString( "(0) OR (1)" ) );
+      QCOMPARE( makeFilter( 3 ), QString( "(0) OR (1) OR (2)" ) );
+      QCOMPARE( makeFilter( 10 ), QString( "(0) OR (1) OR (2) OR (3) OR (4) OR (5) OR (6) OR (7) OR (8) OR (9)" ) );
+      QCOMPARE( makeFilter( 51 ), QString( "(((((0) OR ((1) OR (2))) OR ((3) OR ((4) OR (5)))) OR (((6) OR ((7) OR (8))) OR ((9) OR ((10) OR (11))))) OR "
+                                           "((((12) OR ((13) OR (14))) OR ((15) OR ((16) OR (17)))) OR (((18) OR ((19) OR (20))) OR (((21) OR (22)) OR ((23) OR (24)))))) OR "
+                                           "(((((25) OR ((26) OR (27))) OR ((28) OR ((29) OR (30)))) OR (((31) OR ((32) OR (33))) OR (((34) OR (35)) OR ((36) OR (37))))) OR "
+                                           "((((38) OR ((39) OR (40))) OR ((41) OR ((42) OR (43)))) OR (((44) OR ((45) OR (46))) OR (((47) OR (48)) OR ((49) OR (50))))))" ) );
+
+    }
+
   private:
     void xml2domElement( const QString &testFile, QDomDocument &doc )
     {

diff --git a/tests/src/core/testqgssqliteexpressioncompiler.cpp b/tests/src/core/testqgssqliteexpressioncompiler.cpp
@@ -0,0 +1,111 @@
+/***************************************************************************
+
+ testqgssqliteexpressioncompiler.cpp
+
+ ---------------------
+ begin                : 14.8.2018
+ copyright            : (C) 2018 by Alessandro Pasotti
+ email                : elpaso at itopen dot it
+ ***************************************************************************
+ *                                                                         *
+ *   This program is free software; you can redistribute it and/or modify  *
+ *   it under the terms of the GNU General Public License as published by  *
+ *   the Free Software Foundation; either version 2 of the License, or     *
+ *   (at your option) any later version.                                   *
+ *                                                                         *
+ ***************************************************************************/
+#include "qgstest.h"
+
+
+#include <qgsapplication.h>
+//header for class being tested
+#include "qgsexpression.h"
+#include "qgsvectorlayer.h"
+#include "qgsproject.h"
+#include "qgssqliteexpressioncompiler.h"
+
+class TestQgsSQLiteExpressionCompiler: public QObject
+{
+    Q_OBJECT
+
+  public:
+
+    TestQgsSQLiteExpressionCompiler() = default;
+
+    QgsExpression makeExpression( const int length );
+
+
+  private slots:
+
+    void initTestCase();
+    void cleanupTestCase();
+    void testMakeExpression();
+    void testCompiler();
+
+  private:
+
+    QgsVectorLayer *mPointsLayer = nullptr;
+};
+
+
+
+QgsExpression TestQgsSQLiteExpressionCompiler::makeExpression( const int length )
+{
+  QStringList expString;
+  for ( int i = 0; i < length; ++i )
+  {
+    expString.append( QStringLiteral( "(\"Z\" >= %1) AND (\"Bottom\" <= %2)" ).arg( i ).arg( i + 1 ) );
+  }
+  QgsExpression exp( expString.join( QStringLiteral( ") OR (" ) ).prepend( '(' ).append( ')' ) );
+  return exp;
+}
+
+void TestQgsSQLiteExpressionCompiler::initTestCase()
+{
+  //
+  // Runs once before any tests are run
+  //
+  // init QGIS's paths - true means that all path will be inited from prefix
+  QgsApplication::init();
+  QgsApplication::initQgis();
+  // Will make sure the settings dir with the style file for color ramp is created
+  QgsApplication::createDatabase();
+  QgsApplication::showSettings();
+  //create a point layer that will be used in all tests...
+  mPointsLayer = new QgsVectorLayer( QStringLiteral( "Point?crs=epsg:4326&field=Z:integer&field=Bottom:integer" ), QStringLiteral( "test mem layer" ), QStringLiteral( "memory" ) );
+  QgsProject::instance()->addMapLayer( mPointsLayer );
+}
+
+void TestQgsSQLiteExpressionCompiler::cleanupTestCase()
+{
+  QgsApplication::exitQgis();
+}
+
+void TestQgsSQLiteExpressionCompiler::testMakeExpression()
+{
+  QgsExpression exp( makeExpression( 1 ) );
+  QVERIFY( exp.isValid() );
+  QCOMPARE( QString( exp ), QString( "((\"Z\" >= 0) AND (\"Bottom\" <= 1))" ) );
+  QgsExpression exp2( makeExpression( 2 ) );
+  QVERIFY( exp2.isValid() );
+  QCOMPARE( QString( exp2 ), QString( "((\"Z\" >= 0) AND (\"Bottom\" <= 1)) OR ((\"Z\" >= 1) AND (\"Bottom\" <= 2))" ) );
+}
+
+void TestQgsSQLiteExpressionCompiler::testCompiler()
+{
+  QgsSQLiteExpressionCompiler compiler = QgsSQLiteExpressionCompiler( mPointsLayer->fields() );
+  QgsExpression exp( makeExpression( 1 ) );
+  QCOMPARE( compiler.compile( &exp ), QgsSqlExpressionCompiler::Result::Complete );
+  QCOMPARE( compiler.result(), QString( exp ) );
+  exp = makeExpression( 3 );
+  QCOMPARE( compiler.compile( &exp ), QgsSqlExpressionCompiler::Result::Complete );
+  // Check that parenthesis matches
+  QCOMPARE( compiler.result().count( '(' ),  compiler.result().count( ')' ) );
+  QCOMPARE( compiler.result(), QString( "((((\"Z\" >= 0) AND (\"Bottom\" <= 1)) OR ((\"Z\" >= 1) AND (\"Bottom\" <= 2))) OR ((\"Z\" >= 2) AND (\"Bottom\" <= 3)))" ) );
+}
+
+
+
+QGSTEST_MAIN( TestQgsSQLiteExpressionCompiler )
+
+#include "testqgssqliteexpressioncompiler.moc"