Skip to content

Commit

Permalink
[OGR] Attempt to use actual ogr_fid also if subset string is set
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@manisandro
manisandro committed Sep 4, 2017
1 parent f51244c commit 217e700
Show file tree
Hide file tree
Showing 5 changed files with 109 additions and 8 deletions.
31 changes: 28 additions & 3 deletions src/providers/ogr/qgsogrfeatureiterator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
, mConn( nullptr )
, ogrLayer( nullptr )
, mSubsetStringSet( false )
, mOrigFidAdded( false )
, mFetchGeometry( false )
, mExpressionCompiled( false )
, mFilterFids( mRequest.filterFids() )
Expand All @@ -69,7 +70,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;
Expand Down Expand Up @@ -197,7 +198,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;
Expand Down Expand Up @@ -323,7 +341,14 @@ void QgsOgrFeatureIterator::getFeatureAttribute( OGRFeatureH ogrFet, QgsFeature

bool QgsOgrFeatureIterator::readFeature( OGRFeatureH fet, QgsFeature &feature ) const
{
feature.setId( OGR_F_GetFID( fet ) );
if ( mOrigFidAdded )
{
feature.setId( OGR_F_GetFieldAsInteger64( fet, 0 ) );
}
else
{
feature.setId( OGR_F_GetFID( fet ) );
}
feature.initAttributes( mSource->mFields.count() );
feature.setFields( mSource->mFields ); // allow name-based attribute lookups

Expand Down
1 change: 1 addition & 0 deletions src/providers/ogr/qgsogrfeatureiterator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsOgr
OGRLayerH ogrLayer;

bool mSubsetStringSet;
bool mOrigFidAdded;

//! Set to true, if geometry is in the requested columns
bool mFetchGeometry;
Expand Down
32 changes: 28 additions & 4 deletions src/providers/ogr/qgsogrprovider.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3454,14 +3454,17 @@ OGRwkbGeometryType QgsOgrProvider::ogrWkbSingleFlatten( OGRwkbGeometryType type

OGRLayerH QgsOgrProvider::setSubsetString( OGRLayerH layer, OGRDataSourceH ds )
{
return QgsOgrProviderUtils::setSubsetString( layer, ds, textEncoding(), mSubsetString );
bool origFidAdded = false;
return QgsOgrProviderUtils::setSubsetString( layer, ds, textEncoding(), 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 == QLatin1String( "ODBC" ) ) //the odbc driver does not like schema names for subset
{
Expand All @@ -3478,12 +3481,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 )
Expand Down
2 changes: 1 addition & 1 deletion src/providers/ogr/qgsogrprovider.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,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.
Expand Down
51 changes: 51 additions & 0 deletions tests/src/python/test_provider_ogr_sqlite.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,57 @@ def testDefaultValues(self):
self.assertTrue(vl.dataProvider().defaultValue(5).secsTo(QTime.currentTime()) < 1)
self.assertTrue(vl.dataProvider().defaultValue(6).secsTo(QDateTime.currentDateTime()) < 1)

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()

5 comments on commit 217e700

@nirvn
Copy link
Contributor

@nirvn nirvn commented on 217e700 Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manisandro , this led to a significant regression whereas editing a sqlite (through OGR provider) layer with a filter already present messes up the field positioning and data. It should IMHO be reverted ASAP.

@manisandro
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rouault Do you see another option other than introducing the logic to hide the orig_ogr_fid field in QgsFields?

@nirvn
Copy link
Contributor

@nirvn nirvn commented on 217e700 Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the issue link: https://issues.qgis.org/issues/17122

@manisandro
Copy link
Member Author

@manisandro manisandro commented on 217e700 Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look at adding code to filter orig_ogr_fid when writing to the dataset today, if that does not work I'll revert. Actually I misunderstood the issue and it's not a matter of writing. Looking into it now.

@manisandro
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#5181

Please sign in to comment.