Merge pull request #5224 from manisandro/ogr_gdb_subset

[OGR] Add workaround for OGRSQL not recognizing the column name returned by OGR_L_GetFIDColumn
qgis · Sep 21, 2017 · 54ea029 · 54ea029
2 parents 06dc966 + a67194d
commit 54ea029
Show file tree

Hide file tree

Showing 56 changed files with 70 additions and 22 deletions.
diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -196,17 +196,22 @@ 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 ( OGR_F_GetFieldAsInteger64( fet, 0 ) == id )
+      while ( ( fet = OGR_L_GetNextFeature( ogrLayer ) ) )
       {
-        break;
+        if ( OGR_F_GetFieldAsInteger64( fet, lastField ) == id )
+        {
+          break;
+        }
+        OGR_F_Destroy( fet );
       }
-      OGR_F_Destroy( fet );
     }
   }
   else

diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -3537,33 +3537,59 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, GDALDatasetH ds
       layerName = encoding->fromUnicode( modifiedLayerName );
     }
   }
-  QByteArray sql;
+  OGRLayerH subsetLayer = 0;
   if ( subsetString.startsWith( QLatin1String( "SELECT " ), Qt::CaseInsensitive ) )
-    sql = encoding->fromUnicode( subsetString );
+  {
+    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, mGDALDriverName )
+                          + " 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, mGDALDriverName );
-    sql += " WHERE " + encoding->fromUnicode( subsetString );
   }
 
-  QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
-  OGRLayerH subsetLayer = GDALDatasetExecuteSQL( 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;
     }
   }

diff --git a/tests/src/python/test_provider_ogr.py b/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
@@ -238,6 +238,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()
diff --git a/tests/src/python/test_provider_ogr_sqlite.py b/tests/src/python/test_provider_ogr_sqlite.py
@@ -245,7 +245,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")
@@ -256,7 +256,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__':

diff --git a/tests/testdata/test_gdb.gdb/a00000001.TablesByName.atx b/tests/testdata/test_gdb.gdb/a00000001.TablesByName.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000001.gdbindexes b/tests/testdata/test_gdb.gdb/a00000001.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000001.gdbtable b/tests/testdata/test_gdb.gdb/a00000001.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000001.gdbtablx b/tests/testdata/test_gdb.gdb/a00000001.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000002.gdbtable b/tests/testdata/test_gdb.gdb/a00000002.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000002.gdbtablx b/tests/testdata/test_gdb.gdb/a00000002.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000003.gdbindexes b/tests/testdata/test_gdb.gdb/a00000003.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000003.gdbtable b/tests/testdata/test_gdb.gdb/a00000003.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000003.gdbtablx b/tests/testdata/test_gdb.gdb/a00000003.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000004.CatItemsByPhysicalName.atx b/tests/testdata/test_gdb.gdb/a00000004.CatItemsByPhysicalName.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000004.CatItemsByType.atx b/tests/testdata/test_gdb.gdb/a00000004.CatItemsByType.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000004.FDO_UUID.atx b/tests/testdata/test_gdb.gdb/a00000004.FDO_UUID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000004.gdbindexes b/tests/testdata/test_gdb.gdb/a00000004.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000004.gdbtable b/tests/testdata/test_gdb.gdb/a00000004.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000004.gdbtablx b/tests/testdata/test_gdb.gdb/a00000004.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000004.spx b/tests/testdata/test_gdb.gdb/a00000004.spx
diff --git a/tests/testdata/test_gdb.gdb/a00000005.CatItemTypesByName.atx b/tests/testdata/test_gdb.gdb/a00000005.CatItemTypesByName.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000005.CatItemTypesByParentTypeID.atx b/tests/testdata/test_gdb.gdb/a00000005.CatItemTypesByParentTypeID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000005.CatItemTypesByUUID.atx b/tests/testdata/test_gdb.gdb/a00000005.CatItemTypesByUUID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000005.gdbindexes b/tests/testdata/test_gdb.gdb/a00000005.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000005.gdbtable b/tests/testdata/test_gdb.gdb/a00000005.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000005.gdbtablx b/tests/testdata/test_gdb.gdb/a00000005.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000006.CatRelsByDestinationID.atx b/tests/testdata/test_gdb.gdb/a00000006.CatRelsByDestinationID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000006.CatRelsByOriginID.atx b/tests/testdata/test_gdb.gdb/a00000006.CatRelsByOriginID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000006.CatRelsByType.atx b/tests/testdata/test_gdb.gdb/a00000006.CatRelsByType.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000006.FDO_UUID.atx b/tests/testdata/test_gdb.gdb/a00000006.FDO_UUID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000006.gdbindexes b/tests/testdata/test_gdb.gdb/a00000006.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000006.gdbtable b/tests/testdata/test_gdb.gdb/a00000006.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000006.gdbtablx b/tests/testdata/test_gdb.gdb/a00000006.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByBackwardLabel.atx b/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByBackwardLabel.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByDestItemTypeID.atx b/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByDestItemTypeID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByForwardLabel.atx b/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByForwardLabel.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByName.atx b/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByName.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByOriginItemTypeID.atx b/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByOriginItemTypeID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByUUID.atx b/tests/testdata/test_gdb.gdb/a00000007.CatRelTypesByUUID.atx
diff --git a/tests/testdata/test_gdb.gdb/a00000007.gdbindexes b/tests/testdata/test_gdb.gdb/a00000007.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000007.gdbtable b/tests/testdata/test_gdb.gdb/a00000007.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000007.gdbtablx b/tests/testdata/test_gdb.gdb/a00000007.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000009.gdbindexes b/tests/testdata/test_gdb.gdb/a00000009.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a00000009.gdbtable b/tests/testdata/test_gdb.gdb/a00000009.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a00000009.gdbtablx b/tests/testdata/test_gdb.gdb/a00000009.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a00000009.spx b/tests/testdata/test_gdb.gdb/a00000009.spx
diff --git a/tests/testdata/test_gdb.gdb/a0000000a.gdbindexes b/tests/testdata/test_gdb.gdb/a0000000a.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a0000000a.gdbtable b/tests/testdata/test_gdb.gdb/a0000000a.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a0000000a.gdbtablx b/tests/testdata/test_gdb.gdb/a0000000a.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a0000000a.spx b/tests/testdata/test_gdb.gdb/a0000000a.spx
diff --git a/tests/testdata/test_gdb.gdb/a0000000b.gdbindexes b/tests/testdata/test_gdb.gdb/a0000000b.gdbindexes
diff --git a/tests/testdata/test_gdb.gdb/a0000000b.gdbtable b/tests/testdata/test_gdb.gdb/a0000000b.gdbtable
diff --git a/tests/testdata/test_gdb.gdb/a0000000b.gdbtablx b/tests/testdata/test_gdb.gdb/a0000000b.gdbtablx
diff --git a/tests/testdata/test_gdb.gdb/a0000000b.spx b/tests/testdata/test_gdb.gdb/a0000000b.spx
diff --git a/tests/testdata/test_gdb.gdb/gdb b/tests/testdata/test_gdb.gdb/gdb
diff --git a/tests/testdata/test_gdb.gdb/timestamps b/tests/testdata/test_gdb.gdb/timestamps