Fix virtual layer FilterRect handling when no uid is defined (fixes #…

…15709) When no uid if defined, features returned have an id defined by an autoincremented integer. So we cannot use a SQL filter here because it would return a subset of features and then an autoincremented id that does not correspond to ids without filters. So in this case, all the features are requested and the rectangle intersection is done by the provider, not by SQLite.
qgis · Oct 26, 2018 · efe4a79 · efe4a79
1 parent 225c922
commit efe4a79
Show file tree

Hide file tree

Showing 3 changed files with 120 additions and 47 deletions.
diff --git a/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp b/src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp
@@ -115,6 +115,14 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
         else // never return a feature if the id is negative
           offset = QStringLiteral( " LIMIT 0" );
       }
+      else if ( !mFilterRect.isNull() &&
+                mRequest.flags() & QgsFeatureRequest::ExactIntersect )
+      {
+        // if an exact intersection is requested, prepare the geometry to intersect
+        QgsGeometry rectGeom = QgsGeometry::fromRect( mFilterRect );
+        mRectEngine.reset( QgsGeometry::createGeometryEngine( rectGeom.constGet() ) );
+        mRectEngine->prepareGeometry();
+      }
     }
 
     if ( request.flags() & QgsFeatureRequest::SubsetOfAttributes )
@@ -246,61 +254,85 @@ bool QgsVirtualLayerFeatureIterator::fetchFeature( QgsFeature &feature )
   {
     return false;
   }
-  if ( mQuery->step() != SQLITE_ROW )
-  {
-    return false;
-  }
 
-  feature.setFields( mSource->mFields, /* init */ true );
-
-  if ( mSource->mDefinition.uid().isNull() &&
-       mRequest.filterType() != QgsFeatureRequest::FilterFid )
-  {
-    // no id column => autoincrement
-    feature.setId( mFid++ );
-  }
-  else
-  {
-    // first column: uid
-    feature.setId( mQuery->columnInt64( 0 ) );
-  }
 
-  int n = mQuery->columnCount();
-  int i = 0;
-  Q_FOREACH ( int idx, mAttributes )
+  bool skipFeature = false;
+  do
   {
-    int type = mQuery->columnType( i + 1 );
-    switch ( type )
+    if ( mQuery->step() != SQLITE_ROW )
     {
-      case SQLITE_INTEGER:
-        feature.setAttribute( idx, mQuery->columnInt64( i + 1 ) );
-        break;
-      case SQLITE_FLOAT:
-        feature.setAttribute( idx, mQuery->columnDouble( i + 1 ) );
-        break;
-      case SQLITE_TEXT:
-      default:
-        feature.setAttribute( idx, mQuery->columnText( i + 1 ) );
-        break;
-    };
-    i++;
-  }
-  if ( n > mAttributes.size() + 1 )
-  {
-    // geometry field
-    QByteArray blob( mQuery->columnBlob( n - 1 ) );
-    if ( blob.size() > 0 )
+      return false;
+    }
+
+    feature.setFields( mSource->mFields, /* init */ true );
+
+    if ( mSource->mDefinition.uid().isNull() &&
+         mRequest.filterType() != QgsFeatureRequest::FilterFid )
     {
-      feature.setGeometry( spatialiteBlobToQgsGeometry( blob.constData(), blob.size() ) );
+      // no id column => autoincrement
+      feature.setId( mFid++ );
     }
     else
     {
-      feature.clearGeometry();
+      // first column: uid
+      feature.setId( mQuery->columnInt64( 0 ) );
+    }
+
+    int n = mQuery->columnCount();
+    int i = 0;
+    Q_FOREACH ( int idx, mAttributes )
+    {
+      int type = mQuery->columnType( i + 1 );
+      switch ( type )
+      {
+        case SQLITE_INTEGER:
+          feature.setAttribute( idx, mQuery->columnInt64( i + 1 ) );
+          break;
+        case SQLITE_FLOAT:
+          feature.setAttribute( idx, mQuery->columnDouble( i + 1 ) );
+          break;
+        case SQLITE_TEXT:
+        default:
+          feature.setAttribute( idx, mQuery->columnText( i + 1 ) );
+          break;
+      };
+      i++;
+    }
+    if ( n > mAttributes.size() + 1 )
+    {
+      // geometry field
+      QByteArray blob( mQuery->columnBlob( n - 1 ) );
+      if ( blob.size() > 0 )
+      {
+        feature.setGeometry( spatialiteBlobToQgsGeometry( blob.constData(), blob.size() ) );
+      }
+      else
+      {
+        feature.clearGeometry();
+      }
+    }
+
+    feature.setValid( true );
+    geometryToDestinationCrs( feature, mTransform );
+
+    // if the FilterRect has not been applied on the query
+    // apply it here by skipping features until they intersect
+    if ( mSource->mDefinition.uid().isNull() && feature.hasGeometry() && mSource->mDefinition.hasDefinedGeometry() && !mFilterRect.isNull() )
+    {
+      if ( mRequest.flags() & QgsFeatureRequest::ExactIntersect )
+      {
+        // using exact test when checking for intersection
+        skipFeature = !mRectEngine->intersects( feature.geometry().constGet() );
+      }
+      else
+      {
+        // check just bounding box against rect when not using intersection
+        skipFeature = !feature.geometry().boundingBox().intersects( mFilterRect );
+      }
     }
   }
+  while ( skipFeature );
 
-  feature.setValid( true );
-  geometryToDestinationCrs( feature, mTransform );
   return true;
 }
 

diff --git a/src/providers/virtual/qgsvirtuallayerfeatureiterator.h b/src/providers/virtual/qgsvirtuallayerfeatureiterator.h
@@ -20,6 +20,7 @@ email                : hugo dot mercier at oslandia dot com
 
 #include "qgsvirtuallayerprovider.h"
 #include "qgsfeatureiterator.h"
+#include "qgsgeometryengine.h"
 
 #include <memory>
 #include <QPointer>
@@ -74,6 +75,7 @@ class QgsVirtualLayerFeatureIterator : public QgsAbstractFeatureIteratorFromSour
     QgsCoordinateTransform mTransform;
     QgsRectangle mFilterRect;
 
+    std::unique_ptr< QgsGeometryEngine > mRectEngine;
 };
 
 #endif
diff --git a/tests/src/python/test_provider_virtual.py b/tests/src/python/test_provider_virtual.py
@@ -866,9 +866,9 @@ def test_joined_layers_conversion(self):
         v1.addJoin(joinInfo2)
         self.assertEqual(len(v1.fields()), 7)
         df = QgsVirtualLayerDefinitionUtils.fromJoinedLayer(v1)
-        self.assertEqual(df.query(), ('SELECT t.geometry, t.rowid AS uid, t."id", t."b_id", t."c_id", t."name", j1."bname" AS "B_bname", j1."bfield" AS "B_bfield", j2."cname" AS "C_cname" FROM "{}" AS t ' +
-                                      'LEFT JOIN "{}" AS j1 ON t."b_id"=j1."id" ' +
-                                      'LEFT JOIN "{}" AS j2 ON t."c_id"=j2."id"').format(v1.id(), v2.id(), v3.id()))
+        self.assertEqual(df.query(), ('SELECT t.geometry, t.rowid AS uid, t."id", t."b_id", t."c_id", t."name", j1."bname" AS "B_bname", j1."bfield" AS "B_bfield", j2."cname" AS "C_cname" FROM "{}" AS t '
+                                      + 'LEFT JOIN "{}" AS j1 ON t."b_id"=j1."id" '
+                                      + 'LEFT JOIN "{}" AS j2 ON t."c_id"=j2."id"').format(v1.id(), v2.id(), v3.id()))
 
         # test NoGeometry joined layers with field names starting with a digit or containing white spaces
         joinInfo3 = QgsVectorLayerJoinInfo()
@@ -938,6 +938,45 @@ def testFieldsWithSpecialCharacters(self):
 
         QgsProject.instance().removeMapLayer(ml)
 
+    def testFiltersWithoutUid(self):
+        ml = QgsVectorLayer("Point?srid=EPSG:4326&field=a:int", "mem_no_uid", "memory")
+        self.assertEqual(ml.isValid(), True)
+        QgsProject.instance().addMapLayer(ml)
+
+        # a memory layer with 10 features
+        ml.startEditing()
+        for i in range(10):
+            f = QgsFeature(ml.fields())
+            f.setGeometry(QgsGeometry.fromWkt('POINT({} 0)'.format(i)))
+            f.setAttributes([i])
+            ml.addFeatures([f])
+        ml.commitChanges()
+
+        df = QgsVirtualLayerDefinition()
+        df.setQuery('select * from mem_no_uid')
+        vl = QgsVectorLayer(df.toString(), "vl", "virtual")
+        self.assertEqual(vl.isValid(), True)
+
+        # make sure the returned id with a filter is the same as
+        # if there is no filter
+        req = QgsFeatureRequest().setFilterRect(QgsRectangle(4.5, -1, 5.5, 1))
+        fids = [f.id() for f in vl.getFeatures(req)]
+        self.assertEqual(fids, [5])
+
+        req = QgsFeatureRequest().setFilterExpression("a = 5")
+        fids = [f.id() for f in vl.getFeatures(req)]
+        self.assertEqual(fids, [5])
+
+        req = QgsFeatureRequest().setFilterFid(5)
+        a = [(f.id(), f['a']) for f in vl.getFeatures(req)]
+        self.assertEqual(a, [(5, 5)])
+
+        req = QgsFeatureRequest().setFilterFids([5, 6, 8])
+        a = [(f.id(), f['a']) for f in vl.getFeatures(req)]
+        self.assertEqual(a, [(5, 5), (6, 6), (8, 8)])
+
+        QgsProject.instance().removeMapLayer(ml)
+
 
 if __name__ == '__main__':
     unittest.main()