QgsVectorLayerExporter: defer GPKG spatial index creation at file clo…

…sing Should fix the issue raised in https://lists.osgeo.org/pipermail/qgis-developer/2020-October/062422.html The difference with QgsVectorFileWriter is that QgsVectorFileWriter does not close and re-open the layer, so the defered spatial index creation mechanism that is built-in in the GPKG driver works. But in the case of QgsVectorLayerExporter, we close and re-open and thus the spatial index gets created immediately.
qgis · Oct 17, 2020 · d8c6a25 · d8c6a25
1 parent 501c63f
commit d8c6a25
Show file tree

Hide file tree

Showing 3 changed files with 125 additions and 2 deletions.
diff --git a/src/core/qgsvectorlayerexporter.cpp b/src/core/qgsvectorlayerexporter.cpp
@@ -62,11 +62,52 @@ QgsVectorLayerExporter::QgsVectorLayerExporter( const QString &uri,
 {
   mProvider = nullptr;
 
+  QMap<QString, QVariant> modifiedOptions( options );
+
+  // For GPKG/Spatialite, we explicitly ask not to create a spatial index at
+  // layer creation since this would slow down inserts. Defer its creation
+  // to end of exportLayer() or destruction of this object.
+  if ( geometryType != QgsWkbTypes::NoGeometry  &&
+       providerKey == QLatin1String( "ogr" ) &&
+       options.contains( QStringLiteral( "driverName" ) ) &&
+       ( options[ QStringLiteral( "driverName" ) ].toString().compare( QLatin1String( "GPKG" ), Qt::CaseInsensitive ) == 0 ||
+         options[ QStringLiteral( "driverName" ) ].toString().compare( QLatin1String( "SQLite" ), Qt::CaseInsensitive ) == 0 ) )
+  {
+    QStringList modifiedLayerOptions;
+    if ( options.contains( QStringLiteral( "layerOptions" ) ) )
+    {
+      QStringList layerOptions = options.value( QStringLiteral( "layerOptions" ) ).toStringList();
+      for ( const QString &layerOption : layerOptions )
+      {
+        if ( layerOption.compare( QLatin1String( "SPATIAL_INDEX=YES" ), Qt::CaseInsensitive ) == 0 ||
+             layerOption.compare( QLatin1String( "SPATIAL_INDEX=ON" ), Qt::CaseInsensitive ) == 0 ||
+             layerOption.compare( QLatin1String( "SPATIAL_INDEX=TRUE" ), Qt::CaseInsensitive ) == 0 ||
+             layerOption.compare( QLatin1String( "SPATIAL_INDEX=1" ), Qt::CaseInsensitive ) == 0 )
+        {
+          // do nothing
+        }
+        else if ( layerOption.compare( QLatin1String( "SPATIAL_INDEX=NO" ), Qt::CaseInsensitive ) == 0 ||
+                  layerOption.compare( QLatin1String( "SPATIAL_INDEX=OFF" ), Qt::CaseInsensitive ) == 0 ||
+                  layerOption.compare( QLatin1String( "SPATIAL_INDEX=FALSE" ), Qt::CaseInsensitive ) == 0 ||
+                  layerOption.compare( QLatin1String( "SPATIAL_INDEX=0" ), Qt::CaseInsensitive ) == 0 )
+        {
+          mCreateSpatialIndex = false;
+        }
+        else
+        {
+          modifiedLayerOptions << layerOption;
+        }
+      }
+    }
+    modifiedLayerOptions << QStringLiteral( "SPATIAL_INDEX=FALSE" );
+    modifiedOptions[ QStringLiteral( "layerOptions" ) ] = modifiedLayerOptions;
+  }
+
   // create an empty layer
   QString errMsg;
   QgsProviderRegistry *pReg = QgsProviderRegistry::instance();
   mError = pReg->createEmptyLayer( providerKey, uri, fields, geometryType, crs, overwrite, mOldToNewAttrIdx,
-                                   errMsg, !options.isEmpty() ? &options : nullptr );
+                                   errMsg, !modifiedOptions.isEmpty() ? &modifiedOptions : nullptr );
   if ( errorCode() )
   {
     mErrorMessage = errMsg;
@@ -132,6 +173,12 @@ QgsVectorLayerExporter::QgsVectorLayerExporter( const QString &uri,
 QgsVectorLayerExporter::~QgsVectorLayerExporter()
 {
   flushBuffer();
+
+  if ( mCreateSpatialIndex )
+  {
+    createSpatialIndex();
+  }
+
   delete mProvider;
 }
 
@@ -222,6 +269,7 @@ bool QgsVectorLayerExporter::flushBuffer()
 
 bool QgsVectorLayerExporter::createSpatialIndex()
 {
+  mCreateSpatialIndex = false;
   if ( mProvider && ( mProvider->capabilities() & QgsVectorDataProvider::CreateSpatialIndex ) != 0 )
   {
     return mProvider->createSpatialIndex();
@@ -433,7 +481,7 @@ QgsVectorLayerExporter::exportLayer( QgsVectorLayer *layer,
   }
   int errors = writer->errorCount();
 
-  if ( !writer->createSpatialIndex() )
+  if ( writer->mCreateSpatialIndex && !writer->createSpatialIndex() )
   {
     if ( writer->errorCode() && errorMessage )
     {

diff --git a/src/core/qgsvectorlayerexporter.h b/src/core/qgsvectorlayerexporter.h
@@ -164,6 +164,8 @@ class CORE_EXPORT QgsVectorLayerExporter : public QgsFeatureSink
 
     QgsFeatureList mFeatureBuffer;
 
+    bool mCreateSpatialIndex = true;
+
 #ifdef SIP_RUN
     QgsVectorLayerExporter( const QgsVectorLayerExporter &rh );
 #endif

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -1851,6 +1851,79 @@ def testTransactionGroupCrash(self):
         feature.setAttributes([None, 'three'])
         self.assertTrue(vl.addFeature(feature))
 
+    def _testVectorLayerExporterDeferredSpatialIndex(self, layerOptions, expectSpatialIndex):
+        """ Internal method """
+
+        tmpfile = '/vsimem/_testVectorLayerExporterDeferredSpatialIndex.gpkg'
+        options = {}
+        options['driverName'] = 'GPKG'
+        options['layerName'] = 'table1'
+        if layerOptions:
+            options['layerOptions'] = layerOptions
+        exporter = QgsVectorLayerExporter(tmpfile, "ogr", QgsFields(), QgsWkbTypes.Polygon,
+                                          QgsCoordinateReferenceSystem(3111), False, options)
+        self.assertFalse(exporter.errorCode(),
+                         'unexpected export error {}: {}'.format(exporter.errorCode(), exporter.errorMessage()))
+
+        # Check that at that point the rtree is *not* created
+        ds = ogr.Open(tmpfile)
+        sql_lyr = ds.ExecuteSQL("SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'gpkg_extensions'")
+        assert sql_lyr.GetNextFeature() is None
+        ds.ReleaseResultSet(sql_lyr)
+        del ds
+
+        del exporter
+
+        ds = gdal.OpenEx(tmpfile, gdal.OF_VECTOR)
+        if expectSpatialIndex:
+            # Check that at that point the rtree is created
+            sql_lyr = ds.ExecuteSQL("SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'gpkg_extensions'")
+            assert sql_lyr.GetNextFeature() is not None
+            ds.ReleaseResultSet(sql_lyr)
+            sql_lyr = ds.ExecuteSQL("SELECT 1 FROM gpkg_extensions WHERE table_name = 'table1' AND extension_name = 'gpkg_rtree_index'")
+            assert sql_lyr.GetNextFeature() is not None
+            ds.ReleaseResultSet(sql_lyr)
+        else:
+            # Check that at that point the rtree is *still not* created
+            sql_lyr = ds.ExecuteSQL("SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'gpkg_extensions'")
+            assert sql_lyr.GetNextFeature() is None
+            ds.ReleaseResultSet(sql_lyr)
+        return ds
+
+    def testVectorLayerExporterDeferredSpatialIndexNoLayerOptions(self):
+        """ Check that a deferred spatial index is created when no layer creation options is provided """
+
+        ds = self._testVectorLayerExporterDeferredSpatialIndex(None, True)
+        filename = ds.GetDescription()
+        del ds
+        gdal.Unlink(filename)
+
+    def testVectorLayerExporterDeferredSpatialIndexLayerOptions(self):
+        """ Check that a deferred spatial index is created when other layer creations options is provided """
+
+        ds = self._testVectorLayerExporterDeferredSpatialIndex(['GEOMETRY_NAME=my_geom'], True)
+        lyr = ds.GetLayer(0)
+        self.assertEqual(lyr.GetGeometryColumn(), 'my_geom')
+        filename = ds.GetDescription()
+        del ds
+        gdal.Unlink(filename)
+
+    def testVectorLayerExporterDeferredSpatialIndexExplicitSpatialIndexAsked(self):
+        """ Check that a deferred spatial index is created when explicit asked """
+
+        ds = self._testVectorLayerExporterDeferredSpatialIndex(['SPATIAL_INDEX=YES'], True)
+        filename = ds.GetDescription()
+        del ds
+        gdal.Unlink(filename)
+
+    def testVectorLayerExporterDeferredSpatialIndexSpatialIndexDisallowed(self):
+        """ Check that a deferred spatial index is NOT created when explicit disallowed """
+
+        ds = self._testVectorLayerExporterDeferredSpatialIndex(['SPATIAL_INDEX=NO'], False)
+        filename = ds.GetDescription()
+        del ds
+        gdal.Unlink(filename)
+
 
 if __name__ == '__main__':
     unittest.main()