Merge pull request #5946 from DelazJ/nullMinMax

[needs-docs] Update min and max functions
qgis · Jan 3, 2018 · 6ed078c · 6ed078c
2 parents d047bc8 + cdbb0df
commit 6ed078c
Show file tree

Hide file tree

Showing 4 changed files with 46 additions and 24 deletions.
diff --git a/resources/function_help/json/max b/resources/function_help/json/max
@@ -7,6 +7,7 @@
 	{"arg":"value1", "syntaxOnly": true},
 	{"arg":"value2", "syntaxOnly": true},
 	{"arg":"value", "descOnly": true, "description":"a number"}],
-  "examples": [ { "expression":"max(2,10.2,5.5)", "returns":"10.2"}
-  ]
+  "examples": [
+    { "expression":"max(2,10.2,5.5)", "returns":"10.2"},
+    { "expression":"max(20.5,NULL,6.2)", "returns":"20.5"}]
 }
diff --git a/resources/function_help/json/min b/resources/function_help/json/min
@@ -6,6 +6,6 @@
   "arguments": [ {"arg":"value1", "syntaxOnly": true},
                  {"arg":"value2", "syntaxOnly": true},
                  {"arg":"value", "descOnly": true, "description":"a number"}],
-  "examples": [ { "expression":"min(20.5,10,6.2)", "returns":"6.2"}
-  ]
+  "examples": [ { "expression":"min(20.5,10,6.2)", "returns":"6.2"},
+                { "expression":"min(2,-10.3,NULL)", "returns":"-10.3"}]
 }
diff --git a/src/core/expression/qgsexpressionfunction.cpp b/src/core/expression/qgsexpressionfunction.cpp
@@ -415,38 +415,50 @@ static QVariant fcnExpScale( const QVariantList &values, const QgsExpressionCont
 
 static QVariant fcnMax( const QVariantList &values, const QgsExpressionContext *, QgsExpression *parent, const QgsExpressionNodeFunction * )
 {
-  //initially set max as first value
-  double maxVal = QgsExpressionUtils::getDoubleValue( values.at( 0 ), parent );
-
-  //check against all other values
-  for ( int i = 1; i < values.length(); ++i )
+  QVariant result( QVariant::Double );
+  double maxVal = std::numeric_limits<double>::quiet_NaN();
+  for ( const QVariant &val : values )
   {
-    double testVal = QgsExpressionUtils::getDoubleValue( values[i], parent );
-    if ( testVal > maxVal )
+    double testVal = val.isNull() ? std::numeric_limits<double>::quiet_NaN() : QgsExpressionUtils::getDoubleValue( val, parent );
+    if ( std::isnan( maxVal ) )
     {
       maxVal = testVal;
     }
+    else if ( !std::isnan( testVal ) )
+    {
+      maxVal = std::max( maxVal, testVal );
+    }
   }
 
-  return QVariant( maxVal );
+  if ( !std::isnan( maxVal ) )
+  {
+    result = QVariant( maxVal );
+  }
+  return result;
 }
 
 static QVariant fcnMin( const QVariantList &values, const QgsExpressionContext *, QgsExpression *parent, const QgsExpressionNodeFunction * )
 {
-  //initially set min as first value
-  double minVal = QgsExpressionUtils::getDoubleValue( values.at( 0 ), parent );
-
-  //check against all other values
-  for ( int i = 1; i < values.length(); ++i )
+  QVariant result( QVariant::Double );
+  double minVal = std::numeric_limits<double>::quiet_NaN();
+  for ( const QVariant &val : values )
   {
-    double testVal = QgsExpressionUtils::getDoubleValue( values[i], parent );
-    if ( testVal < minVal )
+    double testVal = val.isNull() ? std::numeric_limits<double>::quiet_NaN() : QgsExpressionUtils::getDoubleValue( val, parent );
+    if ( std::isnan( minVal ) )
     {
       minVal = testVal;
     }
+    else if ( !std::isnan( testVal ) )
+    {
+      minVal = std::min( minVal, testVal );
+    }
   }
 
-  return QVariant( minVal );
+  if ( !std::isnan( minVal ) )
+  {
+    result = QVariant( minVal );
+  }
+  return result;
 }
 
 static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionContext *context, QgsExpression *parent, const QgsExpressionNodeFunction * )
@@ -3937,8 +3949,8 @@ const QList<QgsExpressionFunction *> &QgsExpression::Functions()
     sFunctions << randfFunc;
 
     sFunctions
-        << new QgsStaticExpressionFunction( QStringLiteral( "max" ), -1, fcnMax, QStringLiteral( "Math" ) )
-        << new QgsStaticExpressionFunction( QStringLiteral( "min" ), -1, fcnMin, QStringLiteral( "Math" ) )
+        << new QgsStaticExpressionFunction( QStringLiteral( "max" ), -1, fcnMax, QStringLiteral( "Math" ), QString(), false, QSet<QString>(), false, QStringList(), /* handlesNull = */ true )
+        << new QgsStaticExpressionFunction( QStringLiteral( "min" ), -1, fcnMin, QStringLiteral( "Math" ), QString(), false, QSet<QString>(), false, QStringList(), /* handlesNull = */ true )
         << new QgsStaticExpressionFunction( QStringLiteral( "clamp" ), QgsExpressionFunction::ParameterList() << QgsExpressionFunction::Parameter( QStringLiteral( "min" ) ) << QgsExpressionFunction::Parameter( QStringLiteral( "value" ) ) << QgsExpressionFunction::Parameter( QStringLiteral( "max" ) ), fcnClamp, QStringLiteral( "Math" ) )
         << new QgsStaticExpressionFunction( QStringLiteral( "scale_linear" ), 5, fcnLinearScale, QStringLiteral( "Math" ) )
         << new QgsStaticExpressionFunction( QStringLiteral( "scale_exp" ), 6, fcnExpScale, QStringLiteral( "Math" ) )

diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp
@@ -623,12 +623,21 @@ class TestQgsExpression: public QObject
       QTest::newRow( "round(1234.557,2) - round up" ) << "round(1234.557,2)" << false << QVariant( 1234.56 );
       QTest::newRow( "round(1234.554,2) - round down" ) << "round(1234.554,2)" << false << QVariant( 1234.55 );
       QTest::newRow( "round(1234.6) - round up to int" ) << "round(1234.6)" << false << QVariant( 1235 );
-      QTest::newRow( "round(1234.6) - round down to int" ) << "round(1234.4)" << false << QVariant( 1234 );
+      QTest::newRow( "round(1234.4) - round down to int" ) << "round(1234.4)" << false << QVariant( 1234 );
       QTest::newRow( "max(1)" ) << "max(1)" << false << QVariant( 1. );
       QTest::newRow( "max(1,3.5,-2.1)" ) << "max(1,3.5,-2.1)" << false << QVariant( 3.5 );
+      QTest::newRow( "max(3.5,-2.1,1)" ) << "max(3.5,-2.1,1)" << false << QVariant( 3.5 );
+      QTest::newRow( "max with null value" ) << "max(1,3.5,null)" << false << QVariant( 3.5 );
+      QTest::newRow( "max with null value first" ) << "max(null,-3.5,2)" << false << QVariant( 2. );
+      QTest::newRow( "max with no params" ) << "max()" << false << QVariant( QVariant::Double );
+      QTest::newRow( "max with only null value" ) << "max(null)" << false << QVariant( QVariant::Double );
       QTest::newRow( "min(-1.5)" ) << "min(-1.5)" << false << QVariant( -1.5 );
       QTest::newRow( "min(-16.6,3.5,-2.1)" ) << "min(-16.6,3.5,-2.1)" << false << QVariant( -16.6 );
       QTest::newRow( "min(5,3.5,-2.1)" ) << "min(5,3.5,-2.1)" << false << QVariant( -2.1 );
+      QTest::newRow( "min with null value" ) << "min(5,null,-2.1)" << false << QVariant( -2.1 );
+      QTest::newRow( "min with null value first" ) << "min(null,3.2,6.5)" << false << QVariant( 3.2 );
+      QTest::newRow( "min with no params" ) << "min()" << false << QVariant( QVariant::Double );
+      QTest::newRow( "min with only null value" ) << "min(null)" << false << QVariant( QVariant::Double );
       QTest::newRow( "clamp(-2,1,5)" ) << "clamp(-2,1,5)" << false << QVariant( 1.0 );
       QTest::newRow( "clamp(min:=-2,value:=1,max:=5)" ) << "clamp(min:=-2,value:=1,max:=5)" << false << QVariant( 1.0 );
       QTest::newRow( "clamp(-2,-10,5)" ) << "clamp(-2,-10,5)" << false << QVariant( -2.0 );
@@ -1226,7 +1235,7 @@ class TestQgsExpression: public QObject
         qDebug() << exp.evalErrorString();
       if ( result.type() != expected.type() )
       {
-        qDebug() << "got " << result.typeName() << " instead of " << expected.typeName();
+        qDebug() << "got type " << result.typeName() << "(" << result.type() << ") instead of " << expected.typeName() << "(" << expected.type() << ")";
       }
       //qDebug() << res.type() << " " << result.type();
       //qDebug() << "type " << res.typeName();