Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix missing features when combining FilterExpression requests with
subsets of attributes and the filter expression requires fields
which are not included in the attribute subset

Note: I've only fixed the providers which implement the vector
provider conformance unit tests. Other providers (eg GRASS) should
also implement a similar fix.

(cherry-picked from 402ee9d)
  • Loading branch information
nyalldawson committed Apr 4, 2016
1 parent 86f0c5f commit 8c9255e
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 2 deletions.
13 changes: 13 additions & 0 deletions src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp
Expand Up @@ -138,6 +138,19 @@ QgsDelimitedTextFeatureIterator::QgsDelimitedTextFeatureIterator( QgsDelimitedTe
mLoadGeometry = false;
}

// ensure that all attributes required for expression filter are being fetched
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && request.filterType() == QgsFeatureRequest::FilterExpression )
{
QgsAttributeList attrs = request.subsetOfAttributes();
Q_FOREACH ( const QString& field, request.filterExpression()->referencedColumns() )
{
int attrIdx = mSource->mFields.fieldNameIndex( field );
if ( !attrs.contains( attrIdx ) )
attrs << attrIdx;
}
mRequest.setSubsetOfAttributes( attrs );
}

QgsDebugMsg( QString( "Iterator is scanning file: " ) + ( mMode == FileScan ? "Yes" : "No" ) );
QgsDebugMsg( QString( "Iterator is loading geometries: " ) + ( mLoadGeometry ? "Yes" : "No" ) );
QgsDebugMsg( QString( "Iterator is testing geometries: " ) + ( mTestGeometry ? "Yes" : "No" ) );
Expand Down
15 changes: 14 additions & 1 deletion src/providers/mssql/qgsmssqlfeatureiterator.cpp
Expand Up @@ -77,7 +77,20 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
mAttributesToFetch.append( mFidCol );

bool subsetOfAttributes = mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes;
Q_FOREACH ( int i, subsetOfAttributes ? mRequest.subsetOfAttributes() : mSource->mFields.allAttributesList() )
QgsAttributeList attrs = subsetOfAttributes ? mRequest.subsetOfAttributes() : mSource->mFields.allAttributesList();

// ensure that all attributes required for expression filter are being fetched
if ( subsetOfAttributes && request.filterType() == QgsFeatureRequest::FilterExpression )
{
Q_FOREACH ( const QString& field, request.filterExpression()->referencedColumns() )
{
int attrIdx = mSource->mFields.fieldNameIndex( field );
if ( !attrs.contains( attrIdx ) )
attrs << attrIdx;
}
}

Q_FOREACH ( int i, attrs )
{
QString fieldname = mSource->mFields.at( i ).name();
if ( mSource->mFidColName == fieldname )
Expand Down
12 changes: 12 additions & 0 deletions src/providers/ogr/qgsogrfeatureiterator.cpp
Expand Up @@ -63,6 +63,18 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource* source, bool
mFetchGeometry = ( !mRequest.filterRect().isNull() ) || !( mRequest.flags() & QgsFeatureRequest::NoGeometry );
QgsAttributeList attrs = ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes ) ? mRequest.subsetOfAttributes() : mSource->mFields.allAttributesList();

// ensure that all attributes required for expression filter are being fetched
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && request.filterType() == QgsFeatureRequest::FilterExpression )
{
Q_FOREACH ( const QString& field, request.filterExpression()->referencedColumns() )
{
int attrIdx = mSource->mFields.fieldNameIndex( field );
if ( !attrs.contains( attrIdx ) )
attrs << attrIdx;
}
mRequest.setSubsetOfAttributes( attrs );
}

// make sure we fetch just relevant fields
// unless it's a VRT data source filtered by geometry as we don't know which
// attributes make up the geometry and OGR won't fetch them to evaluate the
Expand Down
13 changes: 13 additions & 0 deletions src/providers/oracle/qgsoraclefeatureiterator.cpp
Expand Up @@ -44,10 +44,23 @@ QgsOracleFeatureIterator::QgsOracleFeatureIterator( QgsOracleFeatureSource* sour
mAttributeList = mRequest.subsetOfAttributes();
if ( mAttributeList.isEmpty() )
mAttributeList = mSource->mFields.allAttributesList();

// ensure that all attributes required for expression filter are being fetched
if ( mRequest.filterType() == QgsFeatureRequest::FilterExpression )
{
Q_FOREACH ( const QString& field, mRequest.filterExpression()->referencedColumns() )
{
int attrIdx = mSource->mFields.fieldNameIndex( field );
if ( !mAttributeList.contains( attrIdx ) )
mAttributeList << attrIdx;
}
}

}
else
mAttributeList = mSource->mFields.allAttributesList();


QString whereClause;

if ( !mRequest.filterRect().isNull() && !mSource->mGeometryColumn.isNull() && mSource->mHasSpatialIndex )
Expand Down
13 changes: 13 additions & 0 deletions src/providers/postgres/qgspostgresfeatureiterator.cpp
Expand Up @@ -89,6 +89,19 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
}
else if ( request.filterType() == QgsFeatureRequest::FilterExpression )
{
// ensure that all attributes required for expression filter are being fetched
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
{
QgsAttributeList attrs = mRequest.subsetOfAttributes();
Q_FOREACH ( const QString& field, request.filterExpression()->referencedColumns() )
{
int attrIdx = mSource->mFields.fieldNameIndex( field );
if ( !attrs.contains( attrIdx ) )
attrs << attrIdx;
}
mRequest.setSubsetOfAttributes( attrs );
}

if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )
{
//IMPORTANT - this MUST be the last clause added!
Expand Down
13 changes: 13 additions & 0 deletions src/providers/spatialite/qgsspatialitefeatureiterator.cpp
Expand Up @@ -83,6 +83,19 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
//IMPORTANT - this MUST be the last clause added!
else if ( request.filterType() == QgsFeatureRequest::FilterExpression )
{
// ensure that all attributes required for expression filter are being fetched
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && request.filterType() == QgsFeatureRequest::FilterExpression )
{
QgsAttributeList attrs = request.subsetOfAttributes();
Q_FOREACH ( const QString& field, request.filterExpression()->referencedColumns() )
{
int attrIdx = mSource->mFields.fieldNameIndex( field );
if ( !attrs.contains( attrIdx ) )
attrs << attrIdx;
}
mRequest.setSubsetOfAttributes( attrs );
}

if ( QSettings().value( "/qgis/compileExpressions", true ).toBool() )
{
QgsSpatiaLiteExpressionCompiler compiler = QgsSpatiaLiteExpressionCompiler( source );
Expand Down
12 changes: 11 additions & 1 deletion src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp
Expand Up @@ -80,12 +80,22 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
mFields = mSource->provider()->fields();
if ( request.flags() & QgsFeatureRequest::SubsetOfAttributes )
{

// copy only selected fields
Q_FOREACH ( int idx, request.subsetOfAttributes() )
{
mAttributes << idx;
}

// ensure that all attributes required for expression filter are being fetched
if ( request.filterType() == QgsFeatureRequest::FilterExpression )
{
Q_FOREACH ( const QString& field, request.filterExpression()->referencedColumns() )
{
int attrIdx = mFields.fieldNameIndex( field );
if ( !mAttributes.contains( attrIdx ) )
mAttributes << attrIdx;
}
}
}
else
{
Expand Down
4 changes: 4 additions & 0 deletions tests/src/python/providertestbase.py
Expand Up @@ -21,6 +21,10 @@ def assert_query(self, provider, expression, expected):
result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression))])
assert set(expected) == result, 'Expected {} and got {} when testing expression "{}"'.format(set(expected), result, expression)

# Also check that filter works when referenced fields are not being retrieved by request
result = set([f['pk'] for f in provider.getFeatures(QgsFeatureRequest().setFilterExpression(expression).setSubsetOfAttributes([0]))])
assert set(expected) == result, 'Expected {} and got {} when testing expression "{}" using empty attribute subset'.format(set(expected), result, expression)

'''
This is a collection of tests for vector data providers and kept generic.
To make use of it, subclass it and set self.provider to a provider you want to test.
Expand Down

0 comments on commit 8c9255e

Please sign in to comment.