Skip to content

Commit

Permalink
Fix some providers not requesting all required attributes needed
Browse files Browse the repository at this point in the history
for client side order by clauses
  • Loading branch information
nyalldawson committed May 25, 2017
1 parent aa376c2 commit 47ffb58
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 12 deletions.
12 changes: 12 additions & 0 deletions src/providers/delimitedtext/qgsdelimitedtextfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,18 @@ QgsDelimitedTextFeatureIterator::QgsDelimitedTextFeatureIterator( QgsDelimitedTe
attributeIndexes += attrs.toSet();
mRequest.setSubsetOfAttributes( attributeIndexes.toList() );
}
// also need attributes required by order by
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
{
QgsAttributeList attrs = request.subsetOfAttributes();
Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
{
int attrIndex = mSource->mFields.lookupField( attr );
if ( !attrs.contains( attrIndex ) )
attrs << attrIndex;
}
mRequest.setSubsetOfAttributes( attrs );
}

QgsDebugMsg( QString( "Iterator is scanning file: " ) + ( mMode == FileScan ? "Yes" : "No" ) );
QgsDebugMsg( QString( "Iterator is loading geometries: " ) + ( mLoadGeometry ? "Yes" : "No" ) );
Expand Down
13 changes: 13 additions & 0 deletions src/providers/ogr/qgsogrfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,19 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
attrs = attributeIndexes.toList();
mRequest.setSubsetOfAttributes( attrs );
}
// also need attributes required by order by
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
{
QSet<int> attributeIndexes;
Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
{
attributeIndexes << mSource->mFields.lookupField( attr );
}
attributeIndexes += attrs.toSet();
attrs = attributeIndexes.toList();
mRequest.setSubsetOfAttributes( attrs );
}

if ( request.filterType() == QgsFeatureRequest::FilterExpression && request.filterExpression()->needsGeometry() )
{
mFetchGeometry = true;
Expand Down
13 changes: 13 additions & 0 deletions src/providers/postgres/qgspostgresfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,19 @@ QgsPostgresFeatureIterator::QgsPostgresFeatureIterator( QgsPostgresFeatureSource
mOrderByCompiled = false;
}

// ensure that all attributes required for order by are fetched
if ( !mOrderByCompiled && mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
{
QgsAttributeList attrs = mRequest.subsetOfAttributes();
Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
{
int attrIndex = mSource->mFields.lookupField( attr );
if ( !attrs.contains( attrIndex ) )
attrs << attrIndex;
}
mRequest.setSubsetOfAttributes( attrs );
}

if ( !mOrderByCompiled )
limitAtProvider = false;

Expand Down
13 changes: 12 additions & 1 deletion src/providers/spatialite/qgsspatialitefeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
}
}


whereClause = whereClauses.join( QStringLiteral( " AND " ) );

// Setup the order by
Expand Down Expand Up @@ -175,6 +174,18 @@ QgsSpatiaLiteFeatureIterator::QgsSpatiaLiteFeatureIterator( QgsSpatiaLiteFeature
if ( !mOrderByCompiled )
limitAtProvider = false;

// also need attributes required by order by
if ( !mOrderByCompiled && mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
{
QSet<int> attributeIndexes;
Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
{
attributeIndexes << mSource->mFields.lookupField( attr );
}
attributeIndexes += mRequest.subsetOfAttributes().toSet();
mRequest.setSubsetOfAttributes( attributeIndexes.toList() );
}

// preparing the SQL statement
bool success = prepareStatement( whereClause, limitAtProvider ? mRequest.limit() : -1, orderByParts.join( QStringLiteral( "," ) ) );
if ( !success && useFallbackWhereClause )
Expand Down
11 changes: 11 additions & 0 deletions src/providers/virtual/qgsvirtuallayerfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,17 @@ QgsVirtualLayerFeatureIterator::QgsVirtualLayerFeatureIterator( QgsVirtualLayerF
mAttributes << attrIdx;
}
}

// also need attributes required by order by
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
{
Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
{
int attrIdx = mSource->mFields.lookupField( attr );
if ( !mAttributes.contains( attrIdx ) )
mAttributes << attrIdx;
}
}
}
else
{
Expand Down
15 changes: 15 additions & 0 deletions src/providers/wfs/qgswfsfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,21 @@ QgsWFSFeatureIterator::QgsWFSFeatureIterator( QgsWFSFeatureSource *source,
}
}

// also need attributes required by order by
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && !mRequest.orderBy().isEmpty() )
{
Q_FOREACH ( const QString &attr, mRequest.orderBy().usedAttributes() )
{
int idx = dataProviderFields.indexFromName( attr );
if ( idx >= 0 && !cacheSubSet.contains( idx ) )
cacheSubSet.append( idx );

idx = mShared->mFields.indexFromName( attr );
if ( idx >= 0 && !mSubSetAttributes.contains( idx ) )
mSubSetAttributes.append( idx );
}
}

if ( mFetchGeometry )
{
int hexwkbGeomIdx = dataProviderFields.indexFromName( QgsWFSConstants::FIELD_HEXWKB_GEOM );
Expand Down
28 changes: 17 additions & 11 deletions tests/src/python/featuresourcetestbase.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,19 +270,25 @@ def runGetFeatureTests(self, source):
self.assert_query(source, 'intersects($geometry,geom_from_gml( \'<gml:Polygon srsName="EPSG:4326"><gml:outerBoundaryIs><gml:LinearRing><gml:coordinates>-72.2,66.1 -65.2,66.1 -65.2,72.0 -72.2,72.0 -72.2,66.1</gml:coordinates></gml:LinearRing></gml:outerBoundaryIs></gml:Polygon>\'))', [1, 2])

# combination of an uncompilable expression and limit
feature = next(self.source.getFeatures(QgsFeatureRequest().setFilterExpression('pk=4')))
context = QgsExpressionContext()
scope = QgsExpressionContextScope()
scope.setVariable('parent', feature)
context.appendScope(scope)

request = QgsFeatureRequest()
request.setExpressionContext(context)
request.setFilterExpression('"pk" = attribute(@parent, \'pk\')')
request.setLimit(1)
# TODO - move this test to FeatureSourceTestCase
# it's currently added in ProviderTestCase, but tests only using a QgsVectorLayer getting features,
# i.e. not directly requesting features from the provider. Turns out the WFS provider fails this
# and should be fixed - then we can enable this test at the FeatureSourceTestCase level

values = [f['pk'] for f in self.source.getFeatures(request)]
self.assertEqual(values, [4])
#feature = next(self.source.getFeatures(QgsFeatureRequest().setFilterExpression('pk=4')))
#context = QgsExpressionContext()
#scope = QgsExpressionContextScope()
#scope.setVariable('parent', feature)
#context.appendScope(scope)

#request = QgsFeatureRequest()
#request.setExpressionContext(context)
#request.setFilterExpression('"pk" = attribute(@parent, \'pk\')')
#request.setLimit(1)

#values = [f['pk'] for f in self.source.getFeatures(request)]
#self.assertEqual(values, [4])

def testGetFeaturesExp(self):
self.runGetFeatureTests(self.source)
Expand Down
4 changes: 4 additions & 0 deletions tests/src/python/test_provider_wfs.py
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,10 @@ def tearDownClass(cls):
shutil.rmtree(cls.basetestpath, True)
cls.vl = None # so as to properly close the provider and remove any temporary file

def testWkbType(self):
"""N/A for WFS provider"""
pass

def testInconsistentUri(self):
"""Test a URI with a typename that doesn't match a type of the capabilities"""

Expand Down

0 comments on commit 47ffb58

Please sign in to comment.