Address PR review comments and handle NULLS

Refactor the evaluation code, handle NULLs to
return a boolean when it is possible.
This commit is contained in:
Alessandro Pasotti 2022-03-15 12:06:13 +01:00
parent 739d188852
commit 34f0e59735
6 changed files with 68 additions and 15 deletions

View File

@ -45,6 +45,9 @@ Abstract base class for all nodes that can appear in an expression.
case QgsExpressionNode::ntCondition:
sipType = sipType_QgsExpressionNodeCondition;
break;
case QgsExpressionNode::ntBetweenOperator:
sipType = sipType_QgsExpressionNodeBetweenOperator;
break;
default:
sipType = 0;
break;

View File

@ -2,17 +2,17 @@
"name": "BETWEEN",
"type": "operator",
"groups": ["Operators"],
"description": "Returns 1 if value is within the specified range. To test for exclusion `NOT BETWEEN` can be used.",
"description": "Returns TRUE if value is within the specified range. The range is considered inclusive of the bounds. To test for exclusion `NOT BETWEEN` can be used.",
"arguments": [{
"arg": "lower_bound AND higher_bound",
"description": "range bounds"
}],
"examples": [{
"expression": "'B' BETWEEN 'A' AND 'C'",
"returns": "1"
"returns": "TRUE"
}, {
"expression": "2 BETWEEN 1 AND 3",
"returns": "1"
"returns": "TRUE"
}],
"tags": ["contained", "found"]
}

View File

@ -2,17 +2,17 @@
"name": "NOT BETWEEN",
"type": "operator",
"groups": ["Operators"],
"description": "Returns 1 if value is not within the specified range.",
"description": "Returns TRUE if value is not within the specified range. The range is considered inclusive of the bounds.",
"arguments": [{
"arg": "lower_bound AND higher_bound",
"description": "range bounds"
}],
"examples": [{
"expression": "'B' NOT BETWEEN 'A' AND 'C'",
"returns": "0"
"returns": "FALSE"
}, {
"expression": "1.0 NOT BETWEEN 1.1 AND 1.2",
"returns": "1"
"returns": "TRUE"
}],
"tags": ["contained", "found"]
}

View File

@ -59,6 +59,9 @@ class CORE_EXPORT QgsExpressionNode SIP_ABSTRACT
case QgsExpressionNode::ntCondition:
sipType = sipType_QgsExpressionNodeCondition;
break;
case QgsExpressionNode::ntBetweenOperator:
sipType = sipType_QgsExpressionNodeBetweenOperator;
break;
default:
sipType = 0;
break;
@ -81,7 +84,7 @@ class CORE_EXPORT QgsExpressionNode SIP_ABSTRACT
ntColumnRef, //!< \see QgsExpression::Node::NodeColumnRef
ntCondition, //!< \see QgsExpression::Node::NodeCondition
ntIndexOperator, //!< Index operator
ntBetweenOperator, //!< Between operator
ntBetweenOperator, //!< Between operator \since QGIS 3.26
};

View File

@ -1816,19 +1816,57 @@ bool QgsExpressionNodeBetweenOperator::prepareNode( QgsExpression *parent, const
QVariant QgsExpressionNodeBetweenOperator::evalNode( QgsExpression *parent, const QgsExpressionContext *context )
{
QgsExpressionNodeBinaryOperator lowBound { QgsExpressionNodeBinaryOperator::BinaryOperator::boGE, mNode->clone(), mLowerBound->clone() };
QgsExpressionNodeBinaryOperator highBound { QgsExpressionNodeBinaryOperator::BinaryOperator::boLE, mNode->clone(), mHigherBound->clone() };
const QVariant lowBoundValue = lowBound.eval( parent, context );
const QVariant highBoundValue = highBound.eval( parent, context );
if ( lowBoundValue.isNull() || highBoundValue.isNull() )
const QVariant nodeVal = mNode->eval( parent, context );
if ( nodeVal.isNull() )
{
return QVariant();
}
else
const QgsExpressionNodeLiteral nodeValNode { nodeVal };
QgsExpressionNodeBinaryOperator lowBound { QgsExpressionNodeBinaryOperator::BinaryOperator::boGE, nodeValNode.clone(), mLowerBound->clone() };
const QVariant lowBoundValue = lowBound.eval( parent, context );
const bool lowBoundBool { lowBoundValue.toBool() };
if ( ! lowBoundValue.isNull() && lowBoundBool == mNegate )
{
const bool res { lowBoundValue.toBool() &&highBoundValue.toBool() };
return mNegate ? QVariant( ! res ) : QVariant( res );
return QVariant( false );
}
QgsExpressionNodeBinaryOperator highBound { QgsExpressionNodeBinaryOperator::BinaryOperator::boLE, nodeValNode.clone(), mHigherBound->clone() };
const QVariant highBoundValue = highBound.eval( parent, context );
if ( lowBoundValue.isNull() && highBoundValue.isNull() )
{
return QVariant();
}
const bool highBoundBool { highBoundValue.toBool() };
// We already checked if both are nulls
if ( lowBoundValue.isNull() || highBoundValue.isNull() )
{
// In this case we can return a boolean
if ( ( lowBoundValue.isNull() && ! highBoundBool ) ||
( highBoundValue.isNull() && ! lowBoundBool ) )
{
return QVariant( mNegate );
}
// Indetermined
return QVariant();
}
if ( ! highBoundValue.isNull() && highBoundBool == mNegate )
{
return QVariant( false );
}
const bool res { lowBoundBool &&highBoundBool };
return mNegate ? QVariant( ! res ) : QVariant( res );
}
QString QgsExpressionNodeBetweenOperator::dump() const

View File

@ -2086,6 +2086,7 @@ class TestQgsExpression: public QObject
// Between
QTest::newRow( "between chars" ) << QStringLiteral( "'b' between 'a' AND 'c'" ) << false << QVariant( true );
QTest::newRow( "between chars false" ) << QStringLiteral( "'c' between 'a' AND 'b'" ) << false << QVariant( false );
QTest::newRow( "not between chars" ) << QStringLiteral( "'b' not between 'a' AND 'c'" ) << false << QVariant( false );
QTest::newRow( "between chars bounds" ) << QStringLiteral( "'b' between 'b' AND 'c'" ) << false << QVariant( true );
QTest::newRow( "not between chars bounds" ) << QStringLiteral( "'b' not between 'b' AND 'c'" ) << false << QVariant( false );
@ -2105,6 +2106,14 @@ class TestQgsExpression: public QObject
QTest::newRow( "between nulls 2" ) << QStringLiteral( "'b' between NULL AND NULL" ) << false << QVariant( );
QTest::newRow( "between nulls 3" ) << QStringLiteral( "NULL between 'a' AND 'c'" ) << false << QVariant( );
QTest::newRow( "between nulls 4" ) << QStringLiteral( "'b' between 'a' AND NULL" ) << false << QVariant( );
QTest::newRow( "between nulls 5" ) << QStringLiteral( "NULL between NULL AND NULL" ) << false << QVariant( );
QTest::newRow( "not between nulls " ) << QStringLiteral( "'c' between NULL AND 'b'" ) << false << QVariant( false );
// Test NULL -> TRUE
QTest::newRow( "not between nulls TRUE" ) << QStringLiteral( "'a' not between 'b' AND NULL" ) << false << QVariant( true );
QTest::newRow( "not between nulls TRUE 2" ) << QStringLiteral( "'c' not between NULL AND 'b'" ) << false << QVariant( true );
// Test NULL -> FALSE
QTest::newRow( "between nulls FALSE" ) << QStringLiteral( "'c' between NULL AND 'b'" ) << false << QVariant( false );
QTest::newRow( "between nulls FALSE 2" ) << QStringLiteral( "'a' between 'b' AND NULL" ) << false << QVariant( false );
}
void run_evaluation_test( QgsExpression &exp, bool evalError, QVariant &expected )