Classifications on joined fields should only consider values which

are matched to layer's features (fix #9051)

src/core/qgsvectorlayer.cpp

@@ -3136,87 +3136,81 @@ void QgsVectorLayer::uniqueValues( int index, QList<QVariant> &uniqueValues, int
   }
 
   QgsFields::FieldOrigin origin = mUpdatedFields.fieldOrigin( index );
-  if ( origin == QgsFields::OriginUnknown )
+  switch ( origin )
   {
-    return;
+    case QgsFields::OriginUnknown:
-  }
+      return;
 
-  if ( origin == QgsFields::OriginProvider ) //a provider field
+    case QgsFields::OriginProvider: //a provider field
-  {
-    mDataProvider->uniqueValues( index, uniqueValues, limit );
-
-    if ( mEditBuffer )
     {
-      QSet<QString> vals;
+      mDataProvider->uniqueValues( index, uniqueValues, limit );
-      Q_FOREACH ( const QVariant& v, uniqueValues )
-      {
-        vals << v.toString();
-      }
 
-      QMapIterator< QgsFeatureId, QgsAttributeMap > it( mEditBuffer->changedAttributeValues() );
+      if ( mEditBuffer )
-      while ( it.hasNext() && ( limit < 0 || uniqueValues.count() < limit ) )
       {
-        it.next();
+        QSet<QString> vals;
-        QVariant v = it.value().value( index );
+        Q_FOREACH ( const QVariant& v, uniqueValues )
-        if ( v.isValid() )
         {
-          QString vs = v.toString();
+          vals << v.toString();
-          if ( !vals.contains( vs ) )
+        }
+
+        QMapIterator< QgsFeatureId, QgsAttributeMap > it( mEditBuffer->changedAttributeValues() );
+        while ( it.hasNext() && ( limit < 0 || uniqueValues.count() < limit ) )
+        {
+          it.next();
+          QVariant v = it.value().value( index );
+          if ( v.isValid() )
           {
-            vals << vs;
+            QString vs = v.toString();
-            uniqueValues << v;
+            if ( !vals.contains( vs ) )
+            {
+              vals << vs;
+              uniqueValues << v;
+            }
           }
         }
       }
-    }
 
-    return;
-  }
-  else if ( origin == QgsFields::OriginJoin )
-  {
-    int sourceLayerIndex;
-    const QgsVectorJoinInfo* join = mJoinBuffer->joinForFieldIndex( index, mUpdatedFields, sourceLayerIndex );
-    Q_ASSERT( join );
-
-    QgsVectorLayer *vl = dynamic_cast<QgsVectorLayer*>( QgsMapLayerRegistry::instance()->mapLayer( join->joinLayerId ) );
-
-    if ( vl )
-      vl->dataProvider()->uniqueValues( sourceLayerIndex, uniqueValues, limit );
-
-    return;
-  }
-  else if ( origin == QgsFields::OriginEdit || origin == QgsFields::OriginExpression )
-  {
-    // the layer is editable, but in certain cases it can still be avoided going through all features
-    if ( origin == QgsFields::OriginEdit && mEditBuffer->mDeletedFeatureIds.isEmpty() && mEditBuffer->mAddedFeatures.isEmpty() && !mEditBuffer->mDeletedAttributeIds.contains( index ) && mEditBuffer->mChangedAttributeValues.isEmpty() )
-    {
-      mDataProvider->uniqueValues( index, uniqueValues, limit );
       return;
     }
 
-    // we need to go through each feature
+    case QgsFields::OriginEdit:
-    QgsAttributeList attList;
+      // the layer is editable, but in certain cases it can still be avoided going through all features
-    attList << index;
+      if ( mEditBuffer->mDeletedFeatureIds.isEmpty() &&
-
+           mEditBuffer->mAddedFeatures.isEmpty() &&
-    QgsFeatureIterator fit = getFeatures( QgsFeatureRequest()
+           !mEditBuffer->mDeletedAttributeIds.contains( index ) &&
-                                          .setFlags( QgsFeatureRequest::NoGeometry )
+           mEditBuffer->mChangedAttributeValues.isEmpty() )
-                                          .setSubsetOfAttributes( attList ) );
-
-    QgsFeature f;
-    QVariant currentValue;
-    QHash<QString, QVariant> val;
-    while ( fit.nextFeature( f ) )
-    {
-      currentValue = f.attribute( index );
-      val.insert( currentValue.toString(), currentValue );
-      if ( limit >= 0 && val.size() >= limit )
       {
-        break;
+        mDataProvider->uniqueValues( index, uniqueValues, limit );
+        return;
       }
-    }
+      FALLTHROUGH
+      //we need to go through each feature
+    case QgsFields::OriginJoin:
+    case QgsFields::OriginExpression:
+    {
+      QgsAttributeList attList;
+      attList << index;
 
-    uniqueValues = val.values();
+      QgsFeatureIterator fit = getFeatures( QgsFeatureRequest()
-    return;
+                                            .setFlags( QgsFeatureRequest::NoGeometry )
+                                            .setSubsetOfAttributes( attList ) );
+
+      QgsFeature f;
+      QVariant currentValue;
+      QHash<QString, QVariant> val;
+      while ( fit.nextFeature( f ) )
+      {
+        currentValue = f.attribute( index );
+        val.insert( currentValue.toString(), currentValue );
+        if ( limit >= 0 && val.size() >= limit )
+        {
+          break;
+        }
+      }
+
+      uniqueValues = val.values();
+      return;
+    }
   }
 
   Q_ASSERT_X( false, "QgsVectorLayer::uniqueValues()", "Unknown source of the field!" );
@@ -3230,58 +3224,52 @@ QVariant QgsVectorLayer::minimumValue( int index )
   }
 
   QgsFields::FieldOrigin origin = mUpdatedFields.fieldOrigin( index );
-  if ( origin == QgsFields::OriginUnknown )
-  {
-    return QVariant();
-  }
 
-  if ( origin == QgsFields::OriginProvider ) //a provider field
+  switch ( origin )
   {
-    return mDataProvider->minimumValue( index );
+    case QgsFields::OriginUnknown:
-  }
+      return QVariant();
-  else if ( origin == QgsFields::OriginJoin )
-  {
-    int sourceLayerIndex;
-    const QgsVectorJoinInfo* join = mJoinBuffer->joinForFieldIndex( index, mUpdatedFields, sourceLayerIndex );
-    Q_ASSERT( join );
 
-    QgsVectorLayer* vl = dynamic_cast<QgsVectorLayer*>( QgsMapLayerRegistry::instance()->mapLayer( join->joinLayerId ) );
+    case QgsFields::OriginProvider: //a provider field
-    Q_ASSERT( vl );
-
-    return vl->minimumValue( sourceLayerIndex );
-  }
-  else if ( origin == QgsFields::OriginEdit || origin == QgsFields::OriginExpression )
-  {
-    // the layer is editable, but in certain cases it can still be avoided going through all features
-    if ( origin == QgsFields::OriginEdit &&
-         mEditBuffer->mDeletedFeatureIds.isEmpty() &&
-         mEditBuffer->mAddedFeatures.isEmpty() && !
-         mEditBuffer->mDeletedAttributeIds.contains( index ) &&
-         mEditBuffer->mChangedAttributeValues.isEmpty() )
-    {
       return mDataProvider->minimumValue( index );
-    }
 
-    // we need to go through each feature
+    case QgsFields::OriginEdit:
-    QgsAttributeList attList;
-    attList << index;
-
-    QgsFeatureIterator fit = getFeatures( QgsFeatureRequest()
-                                          .setFlags( QgsFeatureRequest::NoGeometry )
-                                          .setSubsetOfAttributes( attList ) );
-
-    QgsFeature f;
-    double minimumValue = std::numeric_limits<double>::max();
-    double currentValue = 0;
-    while ( fit.nextFeature( f ) )
     {
-      currentValue = f.attribute( index ).toDouble();
+      // the layer is editable, but in certain cases it can still be avoided going through all features
-      if ( currentValue < minimumValue )
+      if ( mEditBuffer->mDeletedFeatureIds.isEmpty() &&
+           mEditBuffer->mAddedFeatures.isEmpty() && !
+           mEditBuffer->mDeletedAttributeIds.contains( index ) &&
+           mEditBuffer->mChangedAttributeValues.isEmpty() )
       {
-        minimumValue = currentValue;
+        return mDataProvider->minimumValue( index );
       }
     }
-    return QVariant( minimumValue );
+    FALLTHROUGH
+    // no choice but to go through all features
+    case QgsFields::OriginExpression:
+    case QgsFields::OriginJoin:
+    {
+      // we need to go through each feature
+      QgsAttributeList attList;
+      attList << index;
+
+      QgsFeatureIterator fit = getFeatures( QgsFeatureRequest()
+                                            .setFlags( QgsFeatureRequest::NoGeometry )
+                                            .setSubsetOfAttributes( attList ) );
+
+      QgsFeature f;
+      double minimumValue = std::numeric_limits<double>::max();
+      double currentValue = 0;
+      while ( fit.nextFeature( f ) )
+      {
+        currentValue = f.attribute( index ).toDouble();
+        if ( currentValue < minimumValue )
+        {
+          minimumValue = currentValue;
+        }
+      }
+      return QVariant( minimumValue );
+    }
   }
 
   Q_ASSERT_X( false, "QgsVectorLayer::minimumValue()", "Unknown source of the field!" );
@@ -3296,58 +3284,49 @@ QVariant QgsVectorLayer::maximumValue( int index )
   }
 
   QgsFields::FieldOrigin origin = mUpdatedFields.fieldOrigin( index );
-  if ( origin == QgsFields::OriginUnknown )
+  switch ( origin )
   {
-    return QVariant();
+    case QgsFields::OriginUnknown:
-  }
+      return QVariant();
 
-  if ( origin == QgsFields::OriginProvider ) //a provider field
+    case QgsFields::OriginProvider: //a provider field
-  {
-    return mDataProvider->maximumValue( index );
-  }
-  else if ( origin == QgsFields::OriginJoin )
-  {
-    int sourceLayerIndex;
-    const QgsVectorJoinInfo* join = mJoinBuffer->joinForFieldIndex( index, mUpdatedFields, sourceLayerIndex );
-    Q_ASSERT( join );
-
-    QgsVectorLayer* vl = dynamic_cast<QgsVectorLayer*>( QgsMapLayerRegistry::instance()->mapLayer( join->joinLayerId ) );
-    Q_ASSERT( vl );
-
-    return vl->maximumValue( sourceLayerIndex );
-  }
-  else if ( origin == QgsFields::OriginEdit || origin == QgsFields::OriginExpression )
-  {
-    // the layer is editable, but in certain cases it can still be avoided going through all features
-    if ( origin == QgsFields::OriginEdit &&
-         mEditBuffer->mDeletedFeatureIds.isEmpty() &&
-         mEditBuffer->mAddedFeatures.isEmpty() &&
-         !mEditBuffer->mDeletedAttributeIds.contains( index ) &&
-         mEditBuffer->mChangedAttributeValues.isEmpty() )
-    {
       return mDataProvider->maximumValue( index );
-    }
 
-    // we need to go through each feature
+    case QgsFields::OriginEdit:
-    QgsAttributeList attList;
+      // the layer is editable, but in certain cases it can still be avoided going through all features
-    attList << index;
+      if ( mEditBuffer->mDeletedFeatureIds.isEmpty() &&
-
+           mEditBuffer->mAddedFeatures.isEmpty() &&
-    QgsFeatureIterator fit = getFeatures( QgsFeatureRequest()
+           !mEditBuffer->mDeletedAttributeIds.contains( index ) &&
-                                          .setFlags( QgsFeatureRequest::NoGeometry )
+           mEditBuffer->mChangedAttributeValues.isEmpty() )
-                                          .setSubsetOfAttributes( attList ) );
-
-    QgsFeature f;
-    double maximumValue = -std::numeric_limits<double>::max();
-    double currentValue = 0;
-    while ( fit.nextFeature( f ) )
-    {
-      currentValue = f.attribute( index ).toDouble();
-      if ( currentValue > maximumValue )
       {
-        maximumValue = currentValue;
+        return mDataProvider->maximumValue( index );
       }
+
+      FALLTHROUGH
+      //no choice but to go through each feature
+    case QgsFields::OriginJoin:
+    case QgsFields::OriginExpression:
+    {
+      QgsAttributeList attList;
+      attList << index;
+
+      QgsFeatureIterator fit = getFeatures( QgsFeatureRequest()
+                                            .setFlags( QgsFeatureRequest::NoGeometry )
+                                            .setSubsetOfAttributes( attList ) );
+
+      QgsFeature f;
+      double maximumValue = -std::numeric_limits<double>::max();
+      double currentValue = 0;
+      while ( fit.nextFeature( f ) )
+      {
+        currentValue = f.attribute( index ).toDouble();
+        if ( currentValue > maximumValue )
+        {
+          maximumValue = currentValue;
+        }
+      }
+      return QVariant( maximumValue );
     }
-    return QVariant( maximumValue );
   }
 
   Q_ASSERT_X( false, "QgsVectorLayer::maximumValue()", "Unknown source of the field!" );

tests/src/python/test_qgsvectorlayer.py

@@ -59,6 +59,21 @@ def createLayerWithOnePoint():
     return layer
 
 
+def createLayerWithTwoPoints():
+    layer = QgsVectorLayer("Point?field=fldtxt:string&field=fldint:integer",
+                           "addfeat", "memory")
+    pr = layer.dataProvider()
+    f = QgsFeature()
+    f.setAttributes(["test", 123])
+    f.setGeometry(QgsGeometry.fromPoint(QgsPoint(100, 200)))
+    f2 = QgsFeature()
+    f2.setAttributes(["test2", 457])
+    f2.setGeometry(QgsGeometry.fromPoint(QgsPoint(100, 200)))
+    assert pr.addFeatures([f, f2])
+    assert layer.pendingFeatureCount() == 2
+    return layer
+
+
 def createJoinLayer():
     joinLayer = QgsVectorLayer(
         "Point?field=x:string&field=y:integer&field=z:integer",
@@ -70,8 +85,14 @@ def createJoinLayer():
     f2 = QgsFeature()
     f2.setAttributes(["bar", 456, 654])
     f2.setGeometry(QgsGeometry.fromPoint(QgsPoint(2, 2)))
-    assert pr.addFeatures([f1, f2])
+    f3 = QgsFeature()
-    assert joinLayer.pendingFeatureCount() == 2
+    f3.setAttributes(["qar", 457, 111])
+    f3.setGeometry(QgsGeometry.fromPoint(QgsPoint(2, 2)))
+    f4 = QgsFeature()
+    f4.setAttributes(["a", 458, 19])
+    f4.setGeometry(QgsGeometry.fromPoint(QgsPoint(2, 2)))
+    assert pr.addFeatures([f1, f2, f3, f4])
+    assert joinLayer.pendingFeatureCount() == 4
     return joinLayer
 
 
@@ -1083,6 +1104,24 @@ class TestQgsVectorLayer(unittest.TestCase):
         self.assertEqual(f2[2], "foo")
         self.assertEqual(f2[3], 321)
 
+    def test_JoinStats(self):
+        """ test calculating min/max/uniqueValues on joined field """
+        joinLayer = createJoinLayer()
+        layer = createLayerWithTwoPoints()
+        QgsMapLayerRegistry.instance().addMapLayers([joinLayer, layer])
+
+        join = QgsVectorJoinInfo()
+        join.targetFieldName = "fldint"
+        join.joinLayerId = joinLayer.id()
+        join.joinFieldName = "y"
+        join.memoryCache = True
+        layer.addJoin(join)
+
+        # stats on joined fields should only include values present by join
+        self.assertEqual(layer.minimumValue(3), 111)
+        self.assertEqual(layer.maximumValue(3), 321)
+        self.assertEqual(set(layer.uniqueValues(3)), set([111, 321]))
+
     def test_InvalidOperations(self):
         layer = createLayerWithOnePoint()