Merge pull request #5125 from manisandro/ogr_orig_fid

[2.18][OGR] Attempt to use actual ogr_fid also if subset string is set
qgis · Sep 4, 2017 · c575eb4 · c575eb4
2 parents fa8e1a7 + 8eaeaaa
commit c575eb4
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 8 deletions.
diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -42,6 +42,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
     , ogrLayer( nullptr )
     , mSubsetStringSet( false )
     , mFetchGeometry( false )
+    , mOrigFidAdded( false )
     , mExpressionCompiled( false )
     , mFilterFids( mRequest.filterFids() )
     , mFilterFidsIt( mFilterFids.constBegin() )
@@ -67,7 +68,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
 
   if ( !mSource->mSubsetString.isEmpty() )
   {
-    ogrLayer = QgsOgrProviderUtils::setSubsetString( ogrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString );
+    ogrLayer = QgsOgrProviderUtils::setSubsetString( ogrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString, mOrigFidAdded );
     if ( !ogrLayer )
     {
       return;
@@ -171,7 +172,24 @@ bool QgsOgrFeatureIterator::nextFeatureFilterExpression( QgsFeature& f )
 bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature& feature ) const
 {
   feature.setValid( false );
-  OGRFeatureH fet = OGR_L_GetFeature( ogrLayer, FID_TO_NUMBER( id ) );
+  OGRFeatureH fet;
+  if ( mOrigFidAdded )
+  {
+    OGR_L_ResetReading( ogrLayer );
+    while (( fet = OGR_L_GetNextFeature( ogrLayer ) ) )
+    {
+      if ( OGR_F_GetFieldAsInteger64( fet, 0 ) == id )
+      {
+        break;
+      }
+      OGR_F_Destroy( fet );
+    }
+  }
+  else
+  {
+    fet = OGR_L_GetFeature( ogrLayer, FID_TO_NUMBER( id ) );
+  }
+
   if ( !fet )
   {
     return false;
@@ -296,7 +314,14 @@ void QgsOgrFeatureIterator::getFeatureAttribute( OGRFeatureH ogrFet, QgsFeature
 
 bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature& feature ) const
 {
-  feature.setFeatureId( OGR_F_GetFID( fet ) );
+  if ( mOrigFidAdded )
+  {
+    feature.setFeatureId( OGR_F_GetFieldAsInteger64( fet, 0 ) );
+  }
+  else
+  {
+    feature.setFeatureId( OGR_F_GetFID( fet ) );
+  }
   feature.initAttributes( mSource->mFields.count() );
   feature.setFields( mSource->mFields ); // allow name-based attribute lookups
 

diff --git a/src/providers/ogr/qgsogrfeatureiterator.h b/src/providers/ogr/qgsogrfeatureiterator.h
@@ -78,6 +78,7 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsOgr
     OGRLayerH ogrLayer;
 
     bool mSubsetStringSet;
+    bool mOrigFidAdded;
 
     //! Set to true, if geometry is in the requested columns
     bool mFetchGeometry;

diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -3416,14 +3416,17 @@ OGRwkbGeometryType QgsOgrProvider::ogrWkbSingleFlatten( OGRwkbGeometryType type
 
 OGRLayerH QgsOgrProvider::setSubsetString( OGRLayerH layer, OGRDataSourceH ds )
 {
-  return QgsOgrProviderUtils::setSubsetString( layer, ds, mEncoding, mSubsetString );
+  bool origFidAdded = false;
+  return QgsOgrProviderUtils::setSubsetString( layer, ds, mEncoding, mSubsetString, origFidAdded );
 }
 
-OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString )
+OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString, bool &origFidAdded )
 {
   QByteArray layerName = OGR_FD_GetName( OGR_L_GetLayerDefn( layer ) );
   OGRSFDriverH ogrDriver = OGR_DS_GetDriver( ds );
   QString ogrDriverName = OGR_Dr_GetName( ogrDriver );
+  bool origFidAddAttempted = false;
+  origFidAdded = false;
 
   if ( ogrDriverName == "ODBC" ) //the odbc driver does not like schema names for subset
   {
@@ -3440,12 +3443,33 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH
     sql = encoding->fromUnicode( subsetString );
   else
   {
-    sql = "SELECT * FROM " + quotedIdentifier( layerName, ogrDriverName );
+    QByteArray fidColumn = OGR_L_GetFIDColumn( layer );
+
+    sql = QByteArray( "SELECT " );
+    if ( !fidColumn.isEmpty() )
+    {
+      sql += fidColumn + " as orig_ogc_fid, ";
+      origFidAddAttempted = true;
+    }
+    sql += "* FROM " + quotedIdentifier( layerName, ogrDriverName );
     sql += " WHERE " + encoding->fromUnicode( subsetString );
   }
 
   QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
-  return OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
+  OGRLayerH subsetLayer = OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
+
+  // Check if first column is orig_ogc_fid
+  if ( origFidAddAttempted && subsetLayer )
+  {
+    OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( subsetLayer );
+    if ( OGR_FD_GetFieldCount( fdef ) > 0 )
+    {
+      OGRFieldDefnH fldDef = OGR_FD_GetFieldDefn( fdef, 0 );
+      origFidAdded = qstrcmp( OGR_Fld_GetNameRef( fldDef ), "orig_ogc_fid" ) == 0;
+    }
+  }
+
+  return subsetLayer;
 }
 
 void QgsOgrProvider::open( OpenMode mode )

diff --git a/src/providers/ogr/qgsogrprovider.h b/src/providers/ogr/qgsogrprovider.h
@@ -400,7 +400,7 @@ class QgsOgrProviderUtils
 {
   public:
     static void setRelevantFields( OGRLayerH ogrLayer, int fieldCount, bool fetchGeometry, const QgsAttributeList &fetchAttributes, bool firstAttrIsFid );
-    static OGRLayerH setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString );
+    static OGRLayerH setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString, bool &origFidAdded );
     static QByteArray quotedIdentifier( QByteArray field, const QString& ogrDriverName );
 
     /** Quote a value for placement in a SQL string.

diff --git a/tests/src/python/test_provider_ogr_sqlite.py b/tests/src/python/test_provider_ogr_sqlite.py
@@ -129,6 +129,57 @@ def testFidSupport(self):
         got = [(f.attribute('fid'), f.attribute('intfield')) for f in vl.dataProvider().getFeatures(QgsFeatureRequest().setFilterExpression("fid = 12"))]
         self.assertEqual(got, [(12, 123)])
 
+    def testSubsetStringFids(self):
+        """ tests that feature ids are stable even if a subset string is set """
+
+        tmpfile = os.path.join(self.basetestpath, 'subsetStringFids.sqlite')
+        ds = ogr.GetDriverByName('SQLite').CreateDataSource(tmpfile)
+        lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint, options=['FID=fid'])
+        lyr.CreateField(ogr.FieldDefn('type', ogr.OFTInteger))
+        lyr.CreateField(ogr.FieldDefn('value', ogr.OFTInteger))
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetFID(0)
+        f.SetField(0, 1)
+        f.SetField(1, 11)
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetFID(1)
+        f.SetField(0, 1)
+        f.SetField(1, 12)
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetFID(2)
+        f.SetField(0, 1)
+        f.SetField(1, 13)
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetFID(3)
+        f.SetField(0, 2)
+        f.SetField(1, 14)
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetFID(4)
+        f.SetField(0, 2)
+        f.SetField(1, 15)
+        lyr.CreateFeature(f)
+        f = ogr.Feature(lyr.GetLayerDefn())
+        f.SetFID(5)
+        f.SetField(0, 2)
+        f.SetField(1, 16)
+        lyr.CreateFeature(f)
+        f = None
+        ds = None
+
+        vl = QgsVectorLayer(tmpfile + "|subset=type=2", 'test', 'ogr')
+        self.assertTrue(vl.isValid())
+
+        req = QgsFeatureRequest()
+        req.setFilterExpression("value=16")
+        it = vl.getFeatures(req)
+        f = QgsFeature()
+        self.assertTrue(it.nextFeature(f))
+        self.assertTrue(f.id() == 5)
+
 
 if __name__ == '__main__':
     unittest.main()