Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix aggregate function returns incorrect results (LTR backport) #59085

Merged
merged 3 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions python/PyQt6/core/auto_generated/qgsexpressioncontext.sip.in
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,18 @@ if evaluation should be canceled, if set.
.. seealso:: :py:func:`setFeedback`

.. versionadded:: 3.20
%End

QString uniqueHash( bool &ok /Out/, const QSet<QString> &variables = QSet<QString>() ) const;
%Docstring
Returns a unique hash representing the current state of the context.

:param variables: optional names of a subset of variables to include in the hash. If not specified, all variables will be considered.

:return: - calculated hash
- ok: ``True`` if the hash could be generated, or false if e.g. a variable value is of a type which cannot be hashed

.. versionadded:: 3.40
%End

static const QString EXPR_FIELDS;
Expand Down
13 changes: 13 additions & 0 deletions python/core/auto_generated/qgsexpressioncontext.sip.in
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,19 @@ if evaluation should be canceled, if set.
.. seealso:: :py:func:`setFeedback`

.. versionadded:: 3.20
%End

QString uniqueHash( bool &ok /Out/, const QSet<QString> &variables = QSet<QString>() ) const;
%Docstring
Returns a unique hash representing the current state of the context.

:param variables: optional names of a subset of variables to include in the hash. If not specified, all variables will be considered.

:return: - calculated hash
- ok: will be set to ``True`` if the hash could be generated, or false if e.g. a variable value is of a type which cannot be hashed


.. versionadded:: 3.40
%End

static const QString EXPR_FIELDS;
Expand Down
37 changes: 25 additions & 12 deletions src/core/expression/qgsexpressionfunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -714,18 +714,21 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
QgsExpression subExp( subExpression );
QgsExpression filterExp( parameters.filter );

const QSet< QString > filterVars = filterExp.referencedVariables();
const QSet< QString > subExpVars = subExp.referencedVariables();
QSet<QString> allVars = filterVars + subExpVars;

bool isStatic = true;
if ( filterExp.referencedVariables().contains( QStringLiteral( "parent" ) )
|| filterExp.referencedVariables().contains( QString() )
|| subExp.referencedVariables().contains( QStringLiteral( "parent" ) )
|| subExp.referencedVariables().contains( QString() ) )
if ( filterVars.contains( QStringLiteral( "parent" ) )
|| filterVars.contains( QString() )
|| subExpVars.contains( QStringLiteral( "parent" ) )
|| subExpVars.contains( QString() ) )
{
isStatic = false;
}
else
{
const QSet<QString> refVars = filterExp.referencedVariables() + subExp.referencedVariables();
for ( const QString &varName : refVars )
for ( const QString &varName : allVars )
{
const QgsExpressionContextScope *scope = context->activeScopeForVariable( varName );
if ( scope && !scope->isStatic( varName ) )
Expand All @@ -738,15 +741,20 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon

if ( !isStatic )
{
cacheKey = QStringLiteral( "aggfcn:%1:%2:%3:%4:%5%6:%7" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter,
QString::number( context->feature().id() ), QString::number( qHash( context->feature() ) ), orderBy );
bool ok = false;
const QString contextHash = context->uniqueHash( ok, allVars );
if ( ok )
{
cacheKey = QStringLiteral( "aggfcn:%1:%2:%3:%4:%5:%6" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter,
orderBy, contextHash );
}
}
else
{
cacheKey = QStringLiteral( "aggfcn:%1:%2:%3:%4:%5" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter, orderBy );
}

if ( context->hasCachedValue( cacheKey ) )
if ( !cacheKey.isEmpty() && context->hasCachedValue( cacheKey ) )
{
return context->cachedValue( cacheKey );
}
Expand All @@ -757,7 +765,7 @@ static QVariant fcnAggregate( const QVariantList &values, const QgsExpressionCon
subContext.appendScope( subScope );
result = vl->aggregate( aggregate, subExpression, parameters, &subContext, &ok, nullptr, context->feedback(), &aggregateError );

if ( ok )
if ( ok && !cacheKey.isEmpty() )
{
// important -- we should only store cached values when the expression is successfully calculated. Otherwise subsequent
// use of the expression context will happily grab the invalid QVariant cached value without realising that there was actually an error
Expand Down Expand Up @@ -1004,8 +1012,13 @@ static QVariant fcnAggregateGeneric( QgsAggregateCalculator::Aggregate aggregate
QString cacheKey;
if ( !isStatic )
{
cacheKey = QStringLiteral( "agg:%1:%2:%3:%4:%5%6:%7" ).arg( vl->id(), QString::number( aggregate ), subExpression, parameters.filter,
QString::number( context->feature().id() ), QString::number( qHash( context->feature() ) ), orderBy );
bool ok = false;
const QString contextHash = context->uniqueHash( ok, refVars );
if ( ok )
{
cacheKey = QStringLiteral( "agg:%1:%2:%3:%4:%5:%6" ).arg( vl->id(), QString::number( static_cast< int >( aggregate ) ), subExpression, parameters.filter,
orderBy, contextHash );
}
}
else
{
Expand Down
43 changes: 43 additions & 0 deletions src/core/qgsexpressioncontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -765,3 +765,46 @@ QgsFeedback *QgsExpressionContext::feedback() const
{
return mFeedback;
}

QString QgsExpressionContext::uniqueHash( bool &ok, const QSet<QString> &variables ) const
{
QString hash;
ok = true;
const QString delimiter( QStringLiteral( "||~~||" ) );

if ( hasFeature() )
{
hash.append( QString::number( feature().id() ) + delimiter + QString::number( qHash( feature() ) ) + delimiter );
}

QStringList sortedVars = qgis::setToList( variables );
if ( sortedVars.empty() )
sortedVars = variableNames();
std::sort( sortedVars.begin(), sortedVars.end() );

for ( const QString &variableName : std::as_const( sortedVars ) )
{
const QVariant value = variable( variableName );
hash.append( variableName + "=" );
if ( QgsVariantUtils::isNull( value ) )
{
hash.append( delimiter );
}
else if ( value.type() == QVariant::String )
{
hash.append( value.toString() + delimiter );
}
else
{
const QString variantString = value.toString();
if ( variantString.isEmpty() )
{
ok = false;
return QString();
}
hash.append( variantString + delimiter );
}
}

return hash;
}
12 changes: 12 additions & 0 deletions src/core/qgsexpressioncontext.h
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,18 @@ class CORE_EXPORT QgsExpressionContext
*/
QgsFeedback *feedback() const;

/**
* Returns a unique hash representing the current state of the context.
*
* \param ok will be set to TRUE if the hash could be generated, or false if e.g. a variable value is of a type which cannot be hashed
* \param variables optional names of a subset of variables to include in the hash. If not specified, all variables will be considered.
*
* \returns calculated hash
*
* \since QGIS 3.40
*/
QString uniqueHash( bool &ok SIP_OUT, const QSet<QString> &variables = QSet<QString>() ) const;

//! Inbuilt variable name for fields storage
static const QString EXPR_FIELDS;
//! Inbuilt variable name for value original value variable
Expand Down
19 changes: 18 additions & 1 deletion tests/src/core/testqgsexpression.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2634,7 +2634,7 @@ class TestQgsExpression: public QObject
QCOMPARE( res.toInt(), 1 );
}

void test_aggregate_with_variable()
void test_aggregate_with_variable_feature()
{
// this checks that a variable can be non static in a aggregate, i.e. the result will change across the fetched features
// see https://github.com/qgis/QGIS/issues/33382
Expand All @@ -2658,6 +2658,23 @@ class TestQgsExpression: public QObject
}
}

void test_aggregate_with_variables()
{
// this checks that a variable can be non static in a aggregate, i.e. the result will change across the fetched features
// see https://github.com/qgis/QGIS/issues/58221
QgsExpressionContext context;
context.appendScope( QgsExpressionContextUtils::layerScope( mAggregatesLayer ) );
context.lastScope()->setVariable( QStringLiteral( "my_var" ), QStringLiteral( "3" ) );

QgsExpression exp( QString( "aggregate(layer:='aggregate_layer', aggregate:='concatenate_unique', expression:=\"col2\", filter:=\"col1\"=@my_var)" ) );
QString res = exp.evaluate( &context ).toString();
QCOMPARE( res, QStringLiteral( "test333" ) );

context.lastScope()->setVariable( QStringLiteral( "my_var" ), QStringLiteral( "4" ) );
res = exp.evaluate( &context ).toString();
QCOMPARE( res, QStringLiteral( "test" ) );
}

void aggregate_data()
{
QTest::addColumn<QString>( "string" );
Expand Down
38 changes: 38 additions & 0 deletions tests/src/core/testqgsexpressioncontext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ class TestQgsExpressionContext : public QObject
void description();
void readWriteScope();
void layerStores();
void uniqueHash();

private:

Expand Down Expand Up @@ -1043,5 +1044,42 @@ void TestQgsExpressionContext::layerStores()
QCOMPARE( c3.loadedLayerStore(), &store4 );
}

void TestQgsExpressionContext::uniqueHash()
{
QgsExpressionContext context;
bool ok = true;
// the actual hash values aren't important, just that they are unique. Feel free to change the expected results accordingly
QSet< QString > vars;
vars.insert( QStringLiteral( "var1" ) );
vars.insert( QStringLiteral( "var2" ) );
QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "var1=||~~||var2=||~~||" ) );
QVERIFY( ok );

QgsExpressionContextScope *scope1 = new QgsExpressionContextScope();
context.appendScope( scope1 );
scope1->setVariable( QStringLiteral( "var1" ), QStringLiteral( "a string" ) );
scope1->setVariable( QStringLiteral( "var2" ), 5 );
QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "var1=a string||~~||var2=5||~~||" ) );
QVERIFY( ok );

QgsExpressionContextScope *scope2 = new QgsExpressionContextScope();
context.appendScope( scope2 );
scope2->setVariable( QStringLiteral( "var1" ), QStringLiteral( "b string" ) );
QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "var1=b string||~~||var2=5||~~||" ) );
QVERIFY( ok );

QgsFeature feature;
feature.setId( 11 );
feature.setAttributes( QgsAttributes() << 5 << 11 );
context.setFeature( feature );
QCOMPARE( context.uniqueHash( ok, vars ), QStringLiteral( "11||~~||1566||~~||var1=b string||~~||var2=5||~~||" ) );
QVERIFY( ok );

// a value which can't be converted to string
scope2->setVariable( QStringLiteral( "var1" ), QVariant::fromValue( QgsCoordinateReferenceSystem( "EPSG:3857" ) ) );
context.uniqueHash( ok, vars );
QVERIFY( !ok );
}

QGSTEST_MAIN( TestQgsExpressionContext )
#include "testqgsexpressioncontext.moc"
Loading