Fix fallback orderby and attribute subset

Also properly closes the iterator when a fallback orderby is used
qgis · Dec 22, 2015 · a5f8818 · a5f8818 · SebDieBln · Dec 24, 2015
1 parent 9d81938
commit a5f8818
Show file tree

Hide file tree

Showing 5 changed files with 165 additions and 11 deletions.
diff --git a/python/core/qgsfeaturerequest.sip b/python/core/qgsfeaturerequest.sip
@@ -78,6 +78,30 @@ class QgsFeatureRequest
 
     };
 
+    class OrderBy
+    {
+      public:
+        /**
+         * Serialize to XML
+         */
+        void save( QDomElement& elem ) const;
+
+        /**
+         * Deserialize from XML
+         */
+        void load( const QDomElement& elem );
+
+        /**
+         * Returns a set of used attributes
+         */
+        QSet<QString> usedAttributes() const;
+
+        /**
+         * Dumps the content to an SQL equivalent syntax
+         */
+        QString dump() const;
+    };
+
     /**
      * A special attribute that if set matches all attributes
      */
@@ -175,12 +199,12 @@ class QgsFeatureRequest
     /**
      * Return a list of order by clauses specified for this feature request.
      */
-    QList<QgsFeatureRequest::OrderByClause> orderBys() const;
+    QgsFeatureRequest::OrderBy orderBys() const;
 
     /**
      * Set a list of order by clauses.
      */
-    void setOrderBys(const QList<QgsFeatureRequest::OrderByClause>& orderBys );
+    QgsFeatureRequest& setOrderBys(const QgsFeatureRequest::OrderBy& orderBys );
 
     /** Set the maximum number of features to request.
      * @param limit maximum number of features, or -1 to request all features.

diff --git a/src/core/qgsfeatureiterator.cpp b/src/core/qgsfeatureiterator.cpp
@@ -149,6 +149,12 @@ bool QgsAbstractFeatureIterator::nextFeature( QgsFeature& f )
       ++mFeatureIterator;
       dataOk = true;
     }
+    else
+    {
+      dataOk = false;
+      // don't call close, the provider connection has already been closed
+      mClosed = true;
+    }
   }
   else
   {
@@ -283,6 +289,7 @@ void QgsAbstractFeatureIterator::setupOrderBy( const QList<QgsFeatureRequest::Or
 
     mFeatureIterator = mCachedFeatures.constBegin();
     mUseCachedFeatures = true;
+    mClosed = false;
   }
 }
 

diff --git a/src/core/qgsfeaturerequest.cpp b/src/core/qgsfeaturerequest.cpp
@@ -154,14 +154,15 @@ QgsFeatureRequest& QgsFeatureRequest::addOrderBy( const QString& expression, boo
   return *this;
 }
 
-QList<QgsFeatureRequest::OrderByClause> QgsFeatureRequest::orderBys() const
+QgsFeatureRequest::OrderBy QgsFeatureRequest::orderBys() const
 {
   return mOrderBys;
 }
 
-void QgsFeatureRequest::setOrderBys( const QList<QgsFeatureRequest::OrderByClause>& orderBys )
+QgsFeatureRequest& QgsFeatureRequest::setOrderBys( const QgsFeatureRequest::OrderBy& orderBys )
 {
   mOrderBys = orderBys;
+  return *this;
 }
 
 QgsFeatureRequest& QgsFeatureRequest::setLimit( long limit )
@@ -307,7 +308,77 @@ void QgsFeatureRequest::OrderByClause::setNullsFirst( bool nullsFirst )
   mNullsFirst = nullsFirst;
 }
 
+QString QgsFeatureRequest::OrderByClause::dump() const
+{
+  return QString( "%1 %2 %3" )
+         .arg( mExpression.expression() )
+         .arg( mAscending ? "ASC" : "DESC" )
+         .arg( mNullsFirst ? "NULLS FIRST" : "NULLS LAST" );
+}
+
 QgsExpression QgsFeatureRequest::OrderByClause::expression() const
 {
   return mExpression;
 }
+
+void QgsFeatureRequest::OrderBy::save( QDomElement& elem ) const
+{
+  QDomDocument doc = elem.ownerDocument();
+  QList<OrderByClause>::ConstIterator it;
+  for ( it = constBegin(); it != constEnd(); ++it )
+  {
+    const OrderByClause& clause = *it;
+    QDomElement clauseElem = doc.createElement( "orderByClause" );
+    clauseElem.setAttribute( "asc", clause.ascending() );
+    clauseElem.setAttribute( "nullsFirst", clause.nullsFirst() );
+    clauseElem.appendChild( doc.createTextNode( clause.expression().expression() ) );
+
+    elem.appendChild( clauseElem );
+  }
+}
+
+void QgsFeatureRequest::OrderBy::load( const QDomElement& elem )
+{
+  clear();
+
+  QDomNodeList clauses = elem.childNodes();
+
+  for ( int i = 0; i < clauses.size(); ++i )
+  {
+    QDomElement clauseElem = clauses.at( i ).toElement();
+    QString expression = clauseElem.toText().data();
+    bool asc = clauseElem.attribute( "asc" ).toInt() != 0;
+    bool nullsFirst  = clauseElem.attribute( "nullsFirst" ).toInt() != 0;
+
+    append( OrderByClause( expression, asc, nullsFirst ) );
+  }
+}
+
+QSet<QString> QgsFeatureRequest::OrderBy::usedAttributes() const
+{
+  QSet<QString> usedAttributes;
+
+  QList<OrderByClause>::ConstIterator it;
+  for ( it = constBegin(); it != constEnd(); ++it )
+  {
+    const OrderByClause& clause = *it;
+
+    usedAttributes.unite( clause.expression().referencedColumns().toSet() );
+  }
+
+  return usedAttributes;
+}
+QString QgsFeatureRequest::OrderBy::dump() const
+{
+  QStringList results;
+
+  QList<OrderByClause>::ConstIterator it;
+  for ( it = constBegin(); it != constEnd(); ++it )
+  {
+    const OrderByClause& clause = *it;
+
+    results << clause.dump();
+  }
+
+  return results.join( ", " );
+}
diff --git a/src/core/qgsfeaturerequest.h b/src/core/qgsfeaturerequest.h
@@ -155,12 +155,47 @@ class CORE_EXPORT QgsFeatureRequest
          */
         void setNullsFirst( bool nullsFirst );
 
+        /**
+         * Dumps the content to an SQL equivalent
+         */
+        QString dump() const;
+
       private:
         QgsExpression mExpression;
         bool mAscending;
         bool mNullsFirst;
     };
 
+    /**
+     * Represents a list of OrderByClauses, with the most important first and the least
+     * important last.
+     *
+     * @note added in QGIS 2.14
+     */
+    class OrderBy : public QList<OrderByClause>
+    {
+      public:
+        /**
+         * Serialize to XML
+         */
+        void save( QDomElement& elem ) const;
+
+        /**
+         * Deserialize from XML
+         */
+        void load( const QDomElement& elem );
+
+        /**
+         * Returns a set of used attributes
+         */
+        QSet<QString> usedAttributes() const;
+
+        /**
+         * Dumps the content to an SQL equivalent syntax
+         */
+        QString dump() const;
+    };
+
     /**
      * A special attribute that if set matches all attributes
      */
@@ -277,13 +312,17 @@ class CORE_EXPORT QgsFeatureRequest
 
     /**
      * Return a list of order by clauses specified for this feature request.
+     *
+     * @note added in 2.14
      */
-    QList<OrderByClause> orderBys() const;
+    OrderBy orderBys() const;
 
     /**
      * Set a list of order by clauses.
+     *
+     * @note added in 2.14
      */
-    void setOrderBys( const QList<OrderByClause>& orderBys );
+    QgsFeatureRequest& setOrderBys( const OrderBy& orderBys );
 
     /** Set the maximum number of features to request.
      * @param limit maximum number of features, or -1 to request all features.
@@ -346,7 +385,7 @@ class CORE_EXPORT QgsFeatureRequest
     QgsAttributeList mAttrs;
     QgsSimplifyMethod mSimplifyMethod;
     long mLimit;
-    QList<OrderByClause> mOrderBys;
+    OrderBy mOrderBys;
 };
 
 Q_DECLARE_OPERATORS_FOR_FLAGS( QgsFeatureRequest::Flags )

diff --git a/src/core/qgsvectorlayerfeatureiterator.cpp b/src/core/qgsvectorlayerfeatureiterator.cpp
@@ -105,17 +105,30 @@ QgsVectorLayerFeatureIterator::QgsVectorLayerFeatureIterator( QgsVectorLayerFeat
   if ( mProviderRequest.flags() & QgsFeatureRequest::SubsetOfAttributes )
   {
     // prepare list of attributes to match provider fields
-    QgsAttributeList providerSubset;
+    QSet<int> providerSubset;
     QgsAttributeList subset = mProviderRequest.subsetOfAttributes();
     int nPendingFields = mSource->mFields.count();
-    for ( int i = 0; i < subset.count(); ++i )
+    Q_FOREACH ( int attrIndex, subset )
     {
-      int attrIndex = subset[i];
       if ( attrIndex < 0 || attrIndex >= nPendingFields ) continue;
       if ( mSource->mFields.fieldOrigin( attrIndex ) == QgsFields::OriginProvider )
         providerSubset << mSource->mFields.fieldOriginIndex( attrIndex );
     }
-    mProviderRequest.setSubsetOfAttributes( providerSubset );
+
+    // This is done in order to be prepared to do fallback order bys
+    // and be sure we have the required columns.
+    // TODO:
+    // It would be nicer to first check if we can compile the order by
+    // and only modify the subset if we cannot.
+    if ( !mProviderRequest.orderBys().isEmpty() )
+    {
+      Q_FOREACH ( const QString& attr, mProviderRequest.orderBys().usedAttributes() )
+      {
+        providerSubset << mSource->mFields.fieldNameIndex( attr );
+      }
+    }
+
+    mProviderRequest.setSubsetOfAttributes( providerSubset.toList() );
   }
 
   if ( mProviderRequest.filterType() == QgsFeatureRequest::FilterExpression )