From 31cecdbc51d090badd22a07a75f36bc274c7ff8e Mon Sep 17 00:00:00 2001 From: Matthias Kuhn Date: Fri, 19 Apr 2013 10:51:06 +0200 Subject: [PATCH] Fix attributetable and vectorlayercache * In the attributetable there was a mess with references and pointers, originating from 66fadee8ef. * QgsVectorLayerCache did sometimes cache features which did not contain all information which needs to be cached and therefore corrupting the cache and leading to incomplete cache hits. * Add a unit test for the cache problem * Fix QgsCacheIndexFeatureId * QgsAbstractCacheIndex::getCacheIterator now produces a QgsFeatureIterator (instead of a list of Feature Ids). This allows to combine a mixed response, partly satisfied by the cache and partly by an additional query to the backend. --- src/core/qgscacheindex.h | 8 ++-- src/core/qgscacheindexfeatureid.cpp | 36 ++++++++++++++- src/core/qgscacheindexfeatureid.h | 44 +++++++++++++++++-- src/core/qgsfeaturerequest.cpp | 9 ++++ src/core/qgsfeaturerequest.h | 2 + src/core/qgsvectorlayercache.cpp | 27 +++++++++--- src/core/qgsvectorlayercache.h | 18 +++++++- .../attributetable/qgsattributetablemodel.cpp | 32 ++++++-------- .../attributetable/qgsattributetablemodel.h | 2 +- tests/src/core/testqgsvectorlayercache.cpp | 42 +++++++++++++++--- 10 files changed, 183 insertions(+), 37 deletions(-) diff --git a/src/core/qgscacheindex.h b/src/core/qgscacheindex.h index a8ff3e81029..7b1cdaac38d 100644 --- a/src/core/qgscacheindex.h +++ b/src/core/qgscacheindex.h @@ -19,6 +19,7 @@ #include "qgsfeature.h" // QgsFeatureIds class QgsFeatureRequest; +class QgsFeatureIterator; /** * @brief @@ -60,14 +61,15 @@ class QgsAbstractCacheIndex * and write the list of feature ids of cached features to cachedFeatures. If it is not able * it will return false and the cachedFeatures state is undefined. * - * @param cachedFeatures A reference to {@link QgsFeatureIds}, where a list of ids is written to, - * in case this index is able to answer the request. + * @param featureIterator A reference to a {@link QgsFeatureIterator}. A valid featureIterator will + * be assigned in case this index is able to answer the request and the return + * value is true. * @param featureRequest The feature request, for which this index is queried. * * @return True, if this index holds the information to answer the request. * */ - virtual bool getCachedIds( QgsFeatureIds& cachedFeatures, const QgsFeatureRequest& featureRequest ) = 0; + virtual bool getCacheIterator( QgsFeatureIterator& featureIterator, const QgsFeatureRequest& featureRequest ) = 0; }; #endif // QGSCACHEINDEX_H diff --git a/src/core/qgscacheindexfeatureid.cpp b/src/core/qgscacheindexfeatureid.cpp index f09815c02b5..d368bb161a9 100644 --- a/src/core/qgscacheindexfeatureid.cpp +++ b/src/core/qgscacheindexfeatureid.cpp @@ -14,11 +14,45 @@ ***************************************************************************/ #include "qgscacheindexfeatureid.h" +#include "qgsfeaturerequest.h" +#include "qgscachedfeatureiterator.h" +#include "qgsvectorlayercache.h" -QgsCacheIndexFeatureId::QgsCacheIndexFeatureId( QgsCachedVectorLayer* cachedVectorLayer ) +QgsCacheIndexFeatureId::QgsCacheIndexFeatureId( QgsVectorLayerCache* cachedVectorLayer ) : QgsAbstractCacheIndex() , C( cachedVectorLayer ) { } +void QgsCacheIndexFeatureId::flushFeature(const QgsFeatureId fid) +{ + Q_UNUSED( fid ) +} + +void QgsCacheIndexFeatureId::flush() +{ +} + +void QgsCacheIndexFeatureId::requestCompleted( QgsFeatureRequest featureRequest, QgsFeatureIds fids ) +{ + Q_UNUSED( featureRequest ) + Q_UNUSED( fids ) +} + +bool QgsCacheIndexFeatureId::getCacheIterator( QgsFeatureIterator &featureIterator, const QgsFeatureRequest &featureRequest ) +{ + if( featureRequest.filterType() == QgsFeatureRequest::FilterFid ) + { + if ( C->isFidCached( featureRequest.filterFid() ) ) + { + QgsFeatureIds fids; + fids << featureRequest.filterFid(); + featureIterator = QgsFeatureIterator( new QgsCachedFeatureIterator( C, featureRequest, fids ) ); + return true; + } + } + + return false; +} + diff --git a/src/core/qgscacheindexfeatureid.h b/src/core/qgscacheindexfeatureid.h index 30dda69bbe6..bb1a89f31f0 100644 --- a/src/core/qgscacheindexfeatureid.h +++ b/src/core/qgscacheindexfeatureid.h @@ -18,19 +18,57 @@ #include "qgscacheindex.h" -class QgsCachedVectorLayer; +class QgsVectorLayerCache; class QgsCacheIndexFeatureId : public QgsAbstractCacheIndex { public: - QgsCacheIndexFeatureId( QgsCachedVectorLayer* cachedVectorLayer ); + QgsCacheIndexFeatureId( QgsVectorLayerCache* ); + + /** + * Is called, whenever a feature is removed from the cache. You should update your indexes, so + * they become invalid in case this feature was required to successfuly answer a request. + */ + virtual void flushFeature( const QgsFeatureId fid ); + + /** + * Sometimes, the whole cache changes its state and its easier to just withdraw everything. + * In this case, this method is issued. Be sure to clear all cache information in here. + */ + virtual void flush(); + + /** + * @brief + * Implement this method to update the the indices, in case you need information contained by the request + * to properly index. (E.g. spatial index) + * Does nothing by default + * + * @param featureRequest The feature request that was answered + * @param fids The feature ids that have been returned + */ + virtual void requestCompleted( QgsFeatureRequest featureRequest, QgsFeatureIds fids ); + + /** + * Is called, when a feature request is issued on a cached layer. + * If this cache index is able to completely answer the feature request, it will return true + * and write the list of feature ids of cached features to cachedFeatures. If it is not able + * it will return false and the cachedFeatures state is undefined. + * + * @param cachedFeatures A reference to {@link QgsFeatureIds}, where a list of ids is written to, + * in case this index is able to answer the request. + * @param featureRequest The feature request, for which this index is queried. + * + * @return True, if this index holds the information to answer the request. + * + */ + virtual bool getCacheIterator( QgsFeatureIterator& featureIterator, const QgsFeatureRequest& featureRequest ); signals: public slots: private: - QgsCachedVectorLayer* C; + QgsVectorLayerCache* C; }; #endif // QGSCACHEINDEXFEATUREID_H diff --git a/src/core/qgsfeaturerequest.cpp b/src/core/qgsfeaturerequest.cpp index e7ad3b87dfd..8be0cb1da57 100644 --- a/src/core/qgsfeaturerequest.cpp +++ b/src/core/qgsfeaturerequest.cpp @@ -38,6 +38,15 @@ QgsFeatureRequest::QgsFeatureRequest( const QgsRectangle& rect ) { } +QgsFeatureRequest::QgsFeatureRequest( const QgsFeatureRequest &rh ) +{ + mFlags = rh.mFlags; + mFilter = rh.mFilter; + mFilterRect = rh.mFilterRect; + mFilterFid = rh.mFilterFid; + mAttrs = rh.mAttrs; +} + QgsFeatureRequest& QgsFeatureRequest::setFilterRect( const QgsRectangle& rect ) { diff --git a/src/core/qgsfeaturerequest.h b/src/core/qgsfeaturerequest.h index a791495b160..77965274042 100644 --- a/src/core/qgsfeaturerequest.h +++ b/src/core/qgsfeaturerequest.h @@ -78,6 +78,8 @@ class CORE_EXPORT QgsFeatureRequest //! construct a request with rectangle filter explicit QgsFeatureRequest( const QgsRectangle& rect ); + QgsFeatureRequest( const QgsFeatureRequest& rh ); + FilterType filterType() const { return mFilter; } //! Set rectangle from which features will be taken. Empty rectangle removes the filter. diff --git a/src/core/qgsvectorlayercache.cpp b/src/core/qgsvectorlayercache.cpp index 6336d36e2c2..227ca0755b9 100644 --- a/src/core/qgsvectorlayercache.cpp +++ b/src/core/qgsvectorlayercache.cpp @@ -105,6 +105,11 @@ void QgsVectorLayerCache::setFullCache( bool fullCache ) } } +void QgsVectorLayerCache::addCacheIndex( QgsAbstractCacheIndex* cacheIndex ) +{ + mCacheIndices.append( cacheIndex ); +} + void QgsVectorLayerCache::setCacheAddedAttributes( bool cacheAddedAttributes ) { if ( cacheAddedAttributes ) @@ -251,11 +256,8 @@ QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &fe // Check if an index is able to deliver the requested features foreach ( QgsAbstractCacheIndex *idx, mCacheIndices ) { - QgsFeatureIds featureIds; - - if ( idx->getCachedIds( featureIds, featureRequest ) ) + if ( idx->getCacheIterator( it, featureRequest ) ) { - it = QgsFeatureIterator( new QgsCachedFeatureIterator( this, featureRequest, featureIds ) ); requiresWriterIt = false; break; } @@ -272,12 +274,27 @@ QgsFeatureIterator QgsVectorLayerCache::getFeatures( const QgsFeatureRequest &fe if ( requiresWriterIt && mLayer->dataProvider() ) { // No index was able to satisfy the request - it = QgsFeatureIterator( new QgsCachedFeatureWriterIterator( this, featureRequest ) ); + QgsFeatureRequest myRequest = QgsFeatureRequest( myRequest ); + + // Make sure if we cache the geometry, it gets fetched + if ( mCacheGeometry ) + myRequest.setFlags( featureRequest.flags() & ~QgsFeatureRequest::NoGeometry ); + + // Make sure, all the cached attributes are requested as well + QSet attrs = featureRequest.subsetOfAttributes().toSet() + mCachedAttributes.toSet(); + myRequest.setSubsetOfAttributes( attrs.toList() ); + + it = QgsFeatureIterator( new QgsCachedFeatureWriterIterator( this, myRequest ) ); } return it; } +bool QgsVectorLayerCache::isFidCached( const QgsFeatureId fid ) +{ + return mCache.contains( fid ); +} + bool QgsVectorLayerCache::checkInformationCovered( const QgsFeatureRequest& featureRequest ) { QgsAttributeList requestedAttributes; diff --git a/src/core/qgsvectorlayercache.h b/src/core/qgsvectorlayercache.h index 1bea624261b..f52409794da 100644 --- a/src/core/qgsvectorlayercache.h +++ b/src/core/qgsvectorlayercache.h @@ -139,10 +139,26 @@ class CORE_EXPORT QgsVectorLayerCache : public QObject * * @param cacheIndex The cache index to add. */ - void addCacheIndex( const QgsAbstractCacheIndex& cacheIndex ); + void addCacheIndex( QgsAbstractCacheIndex *cacheIndex ); + /** + * Query this VectorLayerCache for features. + * If the VectorLayerCache (and moreover any of its indices) is able to satisfy + * the request, the returned {@link QgsFeatureIterator} will iterate over cached features. + * If it's not possible to fully satisfy the request from the cache, part or all of the features + * will be requested from the data provider. + * @param featureRequest The request specifying filter and required data. + * @return An iterator over the requested data. + */ QgsFeatureIterator getFeatures( const QgsFeatureRequest& featureRequest ); + /** + * Check if a certain feature id is cached. + * @param fid The feature id to look for + * @return True if this id is in the cache + */ + bool isFidCached(const QgsFeatureId fid ); + /** * Gets the feature at the given feature id. Considers the changed, added, deleted and permanent features * @param featureId The id of the feature to query diff --git a/src/gui/attributetable/qgsattributetablemodel.cpp b/src/gui/attributetable/qgsattributetablemodel.cpp index 0515fecfc16..88022356ee8 100644 --- a/src/gui/attributetable/qgsattributetablemodel.cpp +++ b/src/gui/attributetable/qgsattributetablemodel.cpp @@ -486,31 +486,27 @@ QVariant QgsAttributeTableModel::data( const QModelIndex &index, int role ) cons return QVariant( Qt::AlignLeft ); } - const QVariant* pVal = NULL; + QVariant val; // if we don't have the row in current cache, load it from layer first - if ( mFeat.id() != rowId || !mFeat.isValid() ) + if ( mCachedField == fieldId ) { - if ( mCachedField == fieldId ) + val = mFieldCache[ rowId ]; + } + else + { + if ( mFeat.id() != rowId || !mFeat.isValid() ) { - const QVariant& val = mFieldCache[rowId]; - pVal = &val; + if ( !loadFeatureAtId( rowId ) ) + return QVariant( "ERROR" ); + + if ( mFeat.id() != rowId ) + return QVariant( "ERROR" ); } - else if ( !loadFeatureAtId( rowId ) ) - return QVariant( "ERROR" ); + + val = mFeat.attribute( fieldId ); } - if ( pVal == NULL && mFeat.id() != rowId ) - return QVariant( "ERROR" ); - - if ( !pVal ) - { - const QVariant& val = mFeat.attribute( fieldId ); - pVal =&val; - } - - const QVariant& val = *pVal; - // For sorting return unprocessed value if ( SortRole == role ) { diff --git a/src/gui/attributetable/qgsattributetablemodel.h b/src/gui/attributetable/qgsattributetablemodel.h index 3377fc9570a..b0d1fe75259 100644 --- a/src/gui/attributetable/qgsattributetablemodel.h +++ b/src/gui/attributetable/qgsattributetablemodel.h @@ -261,7 +261,7 @@ class GUI_EXPORT QgsAttributeTableModel: public QAbstractTableModel /** The currently cached column */ int mCachedField; /** Allows to cache one specific column (used for sorting) */ - QMap mFieldCache; + QHash mFieldCache; }; diff --git a/tests/src/core/testqgsvectorlayercache.cpp b/tests/src/core/testqgsvectorlayercache.cpp index 544b296fc75..f6e2db7f753 100644 --- a/tests/src/core/testqgsvectorlayercache.cpp +++ b/tests/src/core/testqgsvectorlayercache.cpp @@ -23,6 +23,8 @@ #include #include #include +#include +#include /** @ingroup UnitTests * This is a unit test for the vector layer cache @@ -36,17 +38,19 @@ class TestVectorLayerCache: public QObject private slots: void initTestCase(); // will be called before the first testfunction is executed. void cleanupTestCase(); // will be called after the last testfunction was executed. - void init() {}; // will be called before each testfunction is executed. - void cleanup() {}; // will be called after every testfunction. + void init(); // will be called before each testfunction is executed. + void cleanup(); // will be called after every testfunction. void testCacheOverflow(); // Test cache will work if too many features to cache them all are present void testCacheAttrActions(); // Test attribute add/ attribute delete void testFeatureActions(); // Test adding/removing features works + void testSubsetRequest(); void onCommittedFeaturesAdded( QString, QgsFeatureList ); private: QgsVectorLayerCache* mVectorLayerCache; + QgsCacheIndexFeatureId* mFeatureIdIndex; QgsVectorLayer* mPointsLayer; QgsFeatureList mAddedFeatures; QMap mTmpFiles; @@ -88,8 +92,19 @@ void TestVectorLayerCache::initTestCase() QFileInfo myPointFileInfo( myPointsFileName ); mPointsLayer = new QgsVectorLayer( myPointFileInfo.filePath(), myPointFileInfo.completeBaseName(), "ogr" ); +} +void TestVectorLayerCache::init() +{ mVectorLayerCache = new QgsVectorLayerCache( mPointsLayer, 10 ); + mFeatureIdIndex = new QgsCacheIndexFeatureId( mVectorLayerCache ); + mVectorLayerCache->addCacheIndex( mFeatureIdIndex ); +} + +void TestVectorLayerCache::cleanup() +{ + delete mVectorLayerCache; + delete mFeatureIdIndex; } //runs after all tests @@ -107,8 +122,6 @@ void TestVectorLayerCache::cleanupTestCase() mAddedFeatures.clear(); } - delete mVectorLayerCache; - mVectorLayerCache = NULL; delete mPointsLayer; mPointsLayer = NULL; @@ -190,9 +203,28 @@ void TestVectorLayerCache::testFeatureActions() // Delete feature... mPointsLayer->startEditing(); QVERIFY( mPointsLayer->deleteFeature( fid ) ); - mPointsLayer->commitChanges(); QVERIFY( false == mVectorLayerCache->featureAtId( fid, f ) ); + mPointsLayer->rollBack(); +} + +void TestVectorLayerCache::testSubsetRequest() +{ + QgsFeature f; + + QgsFields fields = mPointsLayer->pendingFields(); + QStringList requiredFields; + requiredFields << "Class" << "Cabin Crew"; + + mVectorLayerCache->featureAtId( 16, f ); + QVariant a = f.attribute( 3 ); + + QgsFeatureIterator itSubset = mVectorLayerCache->getFeatures( QgsFeatureRequest().setSubsetOfAttributes( requiredFields, fields) ); + while ( itSubset.nextFeature( f ) ) {} + itSubset.close(); + + mVectorLayerCache->featureAtId( 16, f ); + QVERIFY( a == f.attribute( 3 ) ); } void TestVectorLayerCache::onCommittedFeaturesAdded( QString layerId, QgsFeatureList features )