Skip to content

Commit 8eaeaaa

Browse files
manisandromanisandro
committedSep 4, 2017
[OGR] Attempt to use actual ogr_fid also if subset string is set
If a subset string is set on an OGR layer, the feature iterator returns features with ids taken from a sequence starting from 0, regardless of the original feature id. This causes a mismatch in the data shown by the identify results table and the attribute widget.
·
final-2_18_28final-2_18_13
1 parent fa8e1a7 commit 8eaeaaa

File tree

5 files changed

+109
-8
lines changed

5 files changed

+109
-8
lines changed
 

‎src/providers/ogr/qgsogrfeatureiterator.cpp

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
4242
, ogrLayer( nullptr )
4343
, mSubsetStringSet( false )
4444
, mFetchGeometry( false )
45+
, mOrigFidAdded( false )
4546
, mExpressionCompiled( false )
4647
, mFilterFids( mRequest.filterFids() )
4748
, mFilterFidsIt( mFilterFids.constBegin() )
@@ -67,7 +68,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
6768

6869
if ( !mSource->mSubsetString.isEmpty() )
6970
{
70-
ogrLayer = QgsOgrProviderUtils::setSubsetString( ogrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString );
71+
ogrLayer = QgsOgrProviderUtils::setSubsetString( ogrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString, mOrigFidAdded );
7172
if ( !ogrLayer )
7273
{
7374
return;
@@ -171,7 +172,24 @@ bool QgsOgrFeatureIterator::nextFeatureFilterExpression( QgsFeature& f )
171172
bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature& feature ) const
172173
{
173174
feature.setValid( false );
174-
OGRFeatureH fet = OGR_L_GetFeature( ogrLayer, FID_TO_NUMBER( id ) );
175+
OGRFeatureH fet;
176+
if ( mOrigFidAdded )
177+
{
178+
OGR_L_ResetReading( ogrLayer );
179+
while (( fet = OGR_L_GetNextFeature( ogrLayer ) ) )
180+
{
181+
if ( OGR_F_GetFieldAsInteger64( fet, 0 ) == id )
182+
{
183+
break;
184+
}
185+
OGR_F_Destroy( fet );
186+
}
187+
}
188+
else
189+
{
190+
fet = OGR_L_GetFeature( ogrLayer, FID_TO_NUMBER( id ) );
191+
}
192+
175193
if ( !fet )
176194
{
177195
return false;
@@ -296,7 +314,14 @@ void QgsOgrFeatureIterator::getFeatureAttribute( OGRFeatureH ogrFet, QgsFeature
296314

297315
bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature& feature ) const
298316
{
299-
feature.setFeatureId( OGR_F_GetFID( fet ) );
317+
if ( mOrigFidAdded )
318+
{
319+
feature.setFeatureId( OGR_F_GetFieldAsInteger64( fet, 0 ) );
320+
}
321+
else
322+
{
323+
feature.setFeatureId( OGR_F_GetFID( fet ) );
324+
}
300325
feature.initAttributes( mSource->mFields.count() );
301326
feature.setFields( mSource->mFields ); // allow name-based attribute lookups
302327

‎src/providers/ogr/qgsogrfeatureiterator.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsOgr
7878
OGRLayerH ogrLayer;
7979

8080
bool mSubsetStringSet;
81+
bool mOrigFidAdded;
8182

8283
//! Set to true, if geometry is in the requested columns
8384
bool mFetchGeometry;

‎src/providers/ogr/qgsogrprovider.cpp

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3416,14 +3416,17 @@ OGRwkbGeometryType QgsOgrProvider::ogrWkbSingleFlatten( OGRwkbGeometryType type
34163416

34173417
OGRLayerH QgsOgrProvider::setSubsetString( OGRLayerH layer, OGRDataSourceH ds )
34183418
{
3419-
return QgsOgrProviderUtils::setSubsetString( layer, ds, mEncoding, mSubsetString );
3419+
bool origFidAdded = false;
3420+
return QgsOgrProviderUtils::setSubsetString( layer, ds, mEncoding, mSubsetString, origFidAdded );
34203421
}
34213422

3422-
OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString )
3423+
OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString, bool &origFidAdded )
34233424
{
34243425
QByteArray layerName = OGR_FD_GetName( OGR_L_GetLayerDefn( layer ) );
34253426
OGRSFDriverH ogrDriver = OGR_DS_GetDriver( ds );
34263427
QString ogrDriverName = OGR_Dr_GetName( ogrDriver );
3428+
bool origFidAddAttempted = false;
3429+
origFidAdded = false;
34273430

34283431
if ( ogrDriverName == "ODBC" ) //the odbc driver does not like schema names for subset
34293432
{
@@ -3440,12 +3443,33 @@ OGRLayerH QgsOgrProviderUtils::setSubsetString( OGRLayerH layer, OGRDataSourceH
34403443
sql = encoding->fromUnicode( subsetString );
34413444
else
34423445
{
3443-
sql = "SELECT * FROM " + quotedIdentifier( layerName, ogrDriverName );
3446+
QByteArray fidColumn = OGR_L_GetFIDColumn( layer );
3447+
3448+
sql = QByteArray( "SELECT " );
3449+
if ( !fidColumn.isEmpty() )
3450+
{
3451+
sql += fidColumn + " as orig_ogc_fid, ";
3452+
origFidAddAttempted = true;
3453+
}
3454+
sql += "* FROM " + quotedIdentifier( layerName, ogrDriverName );
34443455
sql += " WHERE " + encoding->fromUnicode( subsetString );
34453456
}
34463457

34473458
QgsDebugMsg( QString( "SQL: %1" ).arg( encoding->toUnicode( sql ) ) );
3448-
return OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
3459+
OGRLayerH subsetLayer = OGR_DS_ExecuteSQL( ds, sql.constData(), nullptr, nullptr );
3460+
3461+
// Check if first column is orig_ogc_fid
3462+
if ( origFidAddAttempted && subsetLayer )
3463+
{
3464+
OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( subsetLayer );
3465+
if ( OGR_FD_GetFieldCount( fdef ) > 0 )
3466+
{
3467+
OGRFieldDefnH fldDef = OGR_FD_GetFieldDefn( fdef, 0 );
3468+
origFidAdded = qstrcmp( OGR_Fld_GetNameRef( fldDef ), "orig_ogc_fid" ) == 0;
3469+
}
3470+
}
3471+
3472+
return subsetLayer;
34493473
}
34503474

34513475
void QgsOgrProvider::open( OpenMode mode )

‎src/providers/ogr/qgsogrprovider.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ class QgsOgrProviderUtils
400400
{
401401
public:
402402
static void setRelevantFields( OGRLayerH ogrLayer, int fieldCount, bool fetchGeometry, const QgsAttributeList &fetchAttributes, bool firstAttrIsFid );
403-
static OGRLayerH setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString );
403+
static OGRLayerH setSubsetString( OGRLayerH layer, OGRDataSourceH ds, QTextCodec* encoding, const QString& subsetString, bool &origFidAdded );
404404
static QByteArray quotedIdentifier( QByteArray field, const QString& ogrDriverName );
405405

406406
/** Quote a value for placement in a SQL string.

‎tests/src/python/test_provider_ogr_sqlite.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,57 @@ def testFidSupport(self):
129129
got = [(f.attribute('fid'), f.attribute('intfield')) for f in vl.dataProvider().getFeatures(QgsFeatureRequest().setFilterExpression("fid = 12"))]
130130
self.assertEqual(got, [(12, 123)])
131131

132+
def testSubsetStringFids(self):
133+
""" tests that feature ids are stable even if a subset string is set """
134+
135+
tmpfile = os.path.join(self.basetestpath, 'subsetStringFids.sqlite')
136+
ds = ogr.GetDriverByName('SQLite').CreateDataSource(tmpfile)
137+
lyr = ds.CreateLayer('test', geom_type=ogr.wkbPoint, options=['FID=fid'])
138+
lyr.CreateField(ogr.FieldDefn('type', ogr.OFTInteger))
139+
lyr.CreateField(ogr.FieldDefn('value', ogr.OFTInteger))
140+
f = ogr.Feature(lyr.GetLayerDefn())
141+
f.SetFID(0)
142+
f.SetField(0, 1)
143+
f.SetField(1, 11)
144+
lyr.CreateFeature(f)
145+
f = ogr.Feature(lyr.GetLayerDefn())
146+
f.SetFID(1)
147+
f.SetField(0, 1)
148+
f.SetField(1, 12)
149+
lyr.CreateFeature(f)
150+
f = ogr.Feature(lyr.GetLayerDefn())
151+
f.SetFID(2)
152+
f.SetField(0, 1)
153+
f.SetField(1, 13)
154+
lyr.CreateFeature(f)
155+
f = ogr.Feature(lyr.GetLayerDefn())
156+
f.SetFID(3)
157+
f.SetField(0, 2)
158+
f.SetField(1, 14)
159+
lyr.CreateFeature(f)
160+
f = ogr.Feature(lyr.GetLayerDefn())
161+
f.SetFID(4)
162+
f.SetField(0, 2)
163+
f.SetField(1, 15)
164+
lyr.CreateFeature(f)
165+
f = ogr.Feature(lyr.GetLayerDefn())
166+
f.SetFID(5)
167+
f.SetField(0, 2)
168+
f.SetField(1, 16)
169+
lyr.CreateFeature(f)
170+
f = None
171+
ds = None
172+
173+
vl = QgsVectorLayer(tmpfile + "|subset=type=2", 'test', 'ogr')
174+
self.assertTrue(vl.isValid())
175+
176+
req = QgsFeatureRequest()
177+
req.setFilterExpression("value=16")
178+
it = vl.getFeatures(req)
179+
f = QgsFeature()
180+
self.assertTrue(it.nextFeature(f))
181+
self.assertTrue(f.id() == 5)
182+
132183

133184
if __name__ == '__main__':
134185
unittest.main()

0 commit comments

Comments
 (0)
Please sign in to comment.