Merge pull request #5225 from manisandro/ogr_gdb_subset218

[OGR] Add workaround for OGRSQL not recognizing the column name returned by OGR_L_GetFIDColumn

Author: manisandro

src/providers/ogr/qgsogrfeatureiterator.cpp

@@ -172,23 +172,28 @@ bool QgsOgrFeatureIterator::nextFeatureFilterExpression( QgsFeature& f )
 bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature& feature ) const
 {
   feature.setValid( false );
-  OGRFeatureH fet;
+  OGRFeatureH fet = 0;
   if ( mOrigFidAdded )
   {
     OGR_L_ResetReading( ogrLayer );
-    while (( fet = OGR_L_GetNextFeature( ogrLayer ) ) )
+    OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( ogrLayer );
+    int lastField = OGR_FD_GetFieldCount( fdef ) - 1;
+    if ( lastField >= 0 )
     {
-      if (
+      while (( fet = OGR_L_GetNextFeature( ogrLayer ) ) )
+      {
+        if (
 #if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >= 2000000
-        OGR_F_GetFieldAsInteger64
+          OGR_F_GetFieldAsInteger64
 #else
-        OGR_F_GetFieldAsInteger
+          OGR_F_GetFieldAsInteger
 #endif
-        ( fet, 0 ) == FID_TO_NUMBER( id ) )
-      {
-        break;
+          ( fet, lastField ) == FID_TO_NUMBER( id ) )
+        {
+          break;
+        }
+        OGR_F_Destroy( fet );
       }
-      OGR_F_Destroy( fet );
     }
   }
   else

src/providers/ogr/qgsogrprovider.cpp

@@ -3443,33 +3443,59 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH
       layerName = encoding->fromUnicode( modifiedLayerName );
     }
   }
-  QByteArray sql;
-  if ( subsetString.startsWith( "SELECT ", Qt::CaseInsensitive ) )
-    sql = encoding->fromUnicode( subsetString );
+  OGRLayerH subsetLayer = 0;
+  if ( subsetString.startsWith( QLatin1String( "SELECT " ), Qt::CaseInsensitive ) )
+  {
+    QByteArray sql = encoding->fromUnicode( subsetString );
+
+    QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
+    subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr );
+  }
   else
   {
+    QByteArray sqlPart1 = "SELECT *";
+    QByteArray sqlPart3 = " FROM " + quotedIdentifier( layerName, ogrDriverName )
+                          + " WHERE " + encoding->fromUnicode( subsetString );
+
+    origFidAddAttempted = true;
+
     QByteArray fidColumn = OGR_L_GetFIDColumn( layer );
+    // Fallback to FID if OGR_L_GetFIDColumn returns nothing
+    if ( fidColumn.isEmpty() )
+    {
+      fidColumn = "FID";
+    }
+
+    QByteArray sql = sqlPart1 + ", " + fidColumn + " as orig_ogc_fid" + sqlPart3;
+    QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
+    subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr );
 
-    sql = QByteArray( "SELECT " );
-    if ( !fidColumn.isEmpty() )
+    // See https://lists.osgeo.org/pipermail/qgis-developer/2017-September/049802.html
+    // If execute SQL fails because it did not find the fidColumn, retry with hardcoded FID
+    if ( !subsetLayer )
     {
-      sql += fidColumn + " as orig_ogc_fid, ";
-      origFidAddAttempted = true;
+      QByteArray sql = sqlPart1 + ", " + "FID as orig_ogc_fid" + sqlPart3;
+      QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
+      subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr );
+    }
+    // If that also fails, just continue without the orig_ogc_fid
+    if ( !subsetLayer )
+    {
+      QByteArray sql = sqlPart1 + sqlPart3;
+      QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
+      subsetLayer = GDALDatasetExecuteSQL( ds, sql.constData(), nullptr, nullptr );
+      origFidAddAttempted = false;
     }
-    sql += "* FROM " + quotedIdentifier( layerName, ogrDriverName );
-    sql += " WHERE " + encoding->fromUnicode( subsetString );
   }
 
-  QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
-  OGRLayerH subsetLayer = OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
-
-  // Check if first column is orig_ogc_fid
+  // Check if last column is orig_ogc_fid
   if ( origFidAddAttempted && subsetLayer )
   {
     OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( subsetLayer );
-    if ( OGR_FD_GetFieldCount( fdef ) > 0 )
+    int fieldCount = OGR_FD_GetFieldCount( fdef );
+    if ( fieldCount > 0 )
     {
-      OGRFieldDefnH fldDef = OGR_FD_GetFieldDefn( fdef, 0 );
+      OGRFieldDefnH fldDef = OGR_FD_GetFieldDefn( fdef, fieldCount - 1 );
       origFidAdded = qstrcmp( OGR_Fld_GetNameRef( fldDef ), "orig_ogc_fid" ) == 0;
     }
   }

tests/src/python/test_provider_ogr.py

@@ -17,7 +17,7 @@
 import sys
 import tempfile
 
-from qgis.core import QgsVectorLayer, QgsVectorDataProvider, QgsWKBTypes
+from qgis.core import QgsVectorLayer, QgsVectorDataProvider, QgsWKBTypes, QgsFeature
 from qgis.testing import (
     start_app,
     unittest
@@ -240,5 +240,23 @@ def testGeometryCollection(self):
         os.unlink(datasource)
         self.assertFalse(os.path.exists(datasource))
 
+    def testGdb(self):
+        """ Test opening a GDB database layer"""
+        gdb_path = os.path.join(unitTestDataPath(), 'test_gdb.gdb')
+        for i in range(3):
+            l = QgsVectorLayer(gdb_path + '|layerid=' + str(i), 'test', 'ogr')
+            self.assertTrue(l.isValid())
+
+    def testGdbFilter(self):
+        """ Test opening a GDB database layer with filter"""
+        gdb_path = os.path.join(unitTestDataPath(), 'test_gdb.gdb')
+        l = QgsVectorLayer(gdb_path + '|layerid=1|subset="text" = \'shape 2\'', 'test', 'ogr')
+        self.assertTrue(l.isValid())
+        it = l.getFeatures()
+        f = QgsFeature()
+        while it.nextFeature(f):
+            self.assertTrue(f.attribute("text") == "shape 2")
+
+
 if __name__ == '__main__':
     unittest.main()

tests/src/python/test_provider_ogr_sqlite.py

@@ -175,7 +175,7 @@ def testSubsetStringFids(self):
 
         vl = QgsVectorLayer(tmpfile + "|subset=type=2", 'test', 'ogr')
         self.assertTrue(vl.isValid())
-        self.assertTrue(vl.fields().at(0).name() == "orig_ogc_fid")
+        self.assertTrue(vl.fields().at(vl.fields().count() - 1).name() == "orig_ogc_fid")
 
         req = QgsFeatureRequest()
         req.setFilterExpression("value=16")
@@ -186,7 +186,7 @@ def testSubsetStringFids(self):
 
         # Check that subset string is correctly set on reload
         vl.reload()
-        self.assertTrue(vl.fields().at(0).name() == "orig_ogc_fid")
+        self.assertTrue(vl.fields().at(vl.fields().count() - 1).name() == "orig_ogc_fid")
 
 
 if __name__ == '__main__':