From 55135688e0eb425eb1fbaada31b2bf2b386c339d Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Wed, 30 Oct 2024 11:21:57 +0100 Subject: [PATCH] Collapse OR nodes into IN Fixes #43992 but also implements an optimization of OR chained nodes by turning them into IN conditions when possible. On behalf of: Faunalia --- src/core/expression/qgsexpressionnode.cpp | 1 - src/core/expression/qgsexpressionnodeimpl.cpp | 116 ++++++++++++++++++ src/gui/qgsrelationeditorwidget.cpp | 17 ++- src/gui/qgsrelationeditorwidget.h | 3 + tests/src/core/testqgsexpression.cpp | 34 +++++ tests/src/gui/testqgsrelationeditorwidget.cpp | 28 ++++- 6 files changed, 194 insertions(+), 5 deletions(-) diff --git a/src/core/expression/qgsexpressionnode.cpp b/src/core/expression/qgsexpressionnode.cpp index d54bd9a83de6..35504b860f80 100644 --- a/src/core/expression/qgsexpressionnode.cpp +++ b/src/core/expression/qgsexpressionnode.cpp @@ -97,4 +97,3 @@ void QgsExpressionNode::cloneTo( QgsExpressionNode *target ) const target->parserFirstColumn = parserFirstColumn; target->parserFirstLine = parserFirstLine; } - diff --git a/src/core/expression/qgsexpressionnodeimpl.cpp b/src/core/expression/qgsexpressionnodeimpl.cpp index f2de45374aa0..09e3951833f6 100644 --- a/src/core/expression/qgsexpressionnodeimpl.cpp +++ b/src/core/expression/qgsexpressionnodeimpl.cpp @@ -694,6 +694,122 @@ QgsExpressionNode::NodeType QgsExpressionNodeBinaryOperator::nodeType() const bool QgsExpressionNodeBinaryOperator::prepareNode( QgsExpression *parent, const QgsExpressionContext *context ) { + + // if this is an OR, try to collapse the OR expression into an IN node + if ( mOp == boOr ) + { + + // First step: flatten OR chain and collect values + QMap orValuesMap; + QList orFieldNames; + + // Get a list of all the OR and IN nodes chained together + std::function visitOrNodes = [&visitOrNodes, &orValuesMap, &orFieldNames]( QgsExpressionNode * node ) -> bool + { + if ( QgsExpressionNodeBinaryOperator *op = dynamic_cast( node ) ) + { + if ( op->op() != boOr && op->op() != boEQ ) + { + return false; + } + + if ( op->op() == boEQ ) + { + // If left is a column ref and right is a literal, collect + if ( dynamic_cast( op->opLeft() ) && dynamic_cast( op->opRight() ) ) + { + const QString fieldName = op->opLeft()->dump(); + if ( !orValuesMap.contains( fieldName ) ) + { + orFieldNames.append( fieldName ); + orValuesMap.insert( fieldName, QgsExpressionNode::NodeList() ); + } + orValuesMap[fieldName].append( op->opRight()->clone() ); + return true; + } + return false; + } + + if ( visitOrNodes( op->opLeft() ) && visitOrNodes( op->opRight() ) ) + { + return true; + } + + } + else if ( QgsExpressionNodeInOperator *inOp = dynamic_cast( node ) ) + { + if ( inOp->node()->nodeType() != QgsExpressionNode::ntColumnRef ) + { + return false; + } + + const QString fieldName = inOp->node()->dump(); + + // Check if all nodes are literals + const auto nodes = inOp->list()->list(); + for ( const auto &valueNode : std::as_const( nodes ) ) + { + if ( valueNode->nodeType() != QgsExpressionNode::ntLiteral ) + { + return false; + } + } + + if ( !orValuesMap.contains( fieldName ) ) + { + orFieldNames.append( fieldName ); + orValuesMap.insert( fieldName, *inOp->list()->clone() ); + } + else + { + for ( const auto &valueNode : std::as_const( nodes ) ) + { + orValuesMap[fieldName].append( valueNode->clone() ); + } + } + + return true; + } + return false; + }; + + + // Second step: build the OR chain of IN operators + if ( visitOrNodes( this ) && ! orValuesMap.empty() ) + { + + // Recursively build the OR chain of IN operators + std::function::const_iterator )> buildOrChain = [&buildOrChain, &orValuesMap, &orFieldNames]( QList::const_iterator fieldNameit ) -> QgsExpressionNode * + { + + QgsExpressionNode *currentNode; + auto orValuesIt = orValuesMap.find( *fieldNameit ); + + if ( orValuesIt.value().count() == 1 ) + { + currentNode = new QgsExpressionNodeBinaryOperator( boEQ, new QgsExpressionNodeColumnRef( orValuesIt.key() ), orValuesIt.value().at( 0 )->clone() ); + } + else + { + currentNode = new QgsExpressionNodeInOperator( new QgsExpressionNodeColumnRef( orValuesIt.key() ), orValuesIt.value().clone() ); + } + + if ( fieldNameit + 1 == orFieldNames.cend() ) + { + return currentNode; + } + else + { + QgsExpressionNode *nextNode = buildOrChain( fieldNameit + 1 ); + return new QgsExpressionNodeBinaryOperator( boOr, currentNode, nextNode ); + } + }; + + mCompiledSimplifiedNode.reset( buildOrChain( orFieldNames.cbegin() ) ); + } + + } + bool resL = mOpLeft->prepare( parent, context ); bool resR = mOpRight->prepare( parent, context ); return resL && resR; diff --git a/src/gui/qgsrelationeditorwidget.cpp b/src/gui/qgsrelationeditorwidget.cpp index af67372c847e..cea7be062d0a 100644 --- a/src/gui/qgsrelationeditorwidget.cpp +++ b/src/gui/qgsrelationeditorwidget.cpp @@ -626,8 +626,10 @@ void QgsRelationEditorWidget::updateUiSingleEdit() QgsVectorLayer *layer = nullptr; if ( mNmRelation.isValid() ) { - QgsFeatureIterator it = mRelation.referencingLayer()->getFeatures( request ); QgsFeature fet; + QgsFeatureRequest nmRequest; + + QgsFeatureIterator it = mRelation.referencingLayer()->getFeatures( request ); QStringList filters; while ( it.nextFeature( fet ) ) @@ -636,8 +638,15 @@ void QgsRelationEditorWidget::updateUiSingleEdit() filters << filter.prepend( '(' ).append( ')' ); } - QgsFeatureRequest nmRequest; - nmRequest.setFilterExpression( filters.join( QLatin1String( " OR " ) ) ); + QString reducedExpression; + if ( QgsExpression::attemptReduceToInClause( filters, reducedExpression ) ) + { + nmRequest.setFilterExpression( reducedExpression ); + } + else + { + nmRequest.setFilterExpression( filters.join( QStringLiteral( " OR " ) ) ); + } request = nmRequest; layer = mNmRelation.referencedLayer(); @@ -666,6 +675,8 @@ void QgsRelationEditorWidget::updateUiSingleEdit() updateButtons(); } + + mFeatureRequest = request; } void QgsRelationEditorWidget::updateUiMultiEdit() diff --git a/src/gui/qgsrelationeditorwidget.h b/src/gui/qgsrelationeditorwidget.h index 7d634b92a5a5..43deac8c831a 100644 --- a/src/gui/qgsrelationeditorwidget.h +++ b/src/gui/qgsrelationeditorwidget.h @@ -267,6 +267,9 @@ class GUI_EXPORT QgsRelationEditorWidget : public QgsAbstractRelationEditorWidge bool mAllowAddChildFeatureWithNoGeometry = true; QString mFilterExpression; + // Mainly used for testing purposes + QgsFeatureRequest mFeatureRequest; + QList mMultiEditPreviousSelectedItems; QgsFeatureIds mMultiEdit1NJustAddedIds; diff --git a/tests/src/core/testqgsexpression.cpp b/tests/src/core/testqgsexpression.cpp index 3fd0188f861b..c5805238931d 100644 --- a/tests/src/core/testqgsexpression.cpp +++ b/tests/src/core/testqgsexpression.cpp @@ -5589,6 +5589,40 @@ class TestQgsExpression: public QObject } } + void testCollapseOrValues_data() + { + QTest::addColumn( "expression" ); + QTest::addColumn( "expected" ); + + + QTest::newRow( "simple" ) << QStringLiteral( "field = 'value' OR field = 'value2'" ) << QStringLiteral( "field IN ('value', 'value2')" ); + QTest::newRow( "simple 2" ) << QStringLiteral( "\"field\" = 'value' OR field = 'value2' OR field = 'value3'" ) << QStringLiteral( "field IN ('value', 'value2', 'value3')" ); + QTest::newRow( "simple3 mixed" ) << QStringLiteral( "field = 'value' OR field = 'value2' OR field2 = 'value3'" ) << QStringLiteral( "field IN ('value', 'value2') OR field2 = 'value3'" ); + QTest::newRow( "simple3 mixed 2" ) << QStringLiteral( "field2 = 'value3' OR field = 'value1' OR field = 'value2'" ) << QStringLiteral( "field2 = 'value3' OR field IN ('value1', 'value2')" ); + QTest::newRow( "simple3 mixed 3" ) << QStringLiteral( "field = 'value1' OR field2 = 'value3' OR field = 'value2'" ) << QStringLiteral( "field IN ('value1', 'value2') OR field2 = 'value3'" ); + + // test with IN + QTest::newRow( "simple IN" ) << QStringLiteral( "field IN ('value', 'value2') OR field = 'value3'" ) << QStringLiteral( "field IN ('value', 'value2', 'value3')" ); + QTest::newRow( "simple IN 2" ) << QStringLiteral( "field = 'value' OR field IN ('value2', 'value3')" ) << QStringLiteral( "field IN ('value', 'value2', 'value3')" ); + + // test cases that should not trigger reduction + QTest::newRow( "no reduction 1" ) << QStringLiteral( "field = 'value' OR field2 = 'value2'" ) << QStringLiteral( "field = 'value' OR field2 = 'value2'" ); + // this could theoretically be reduced, but we don't currently support this + QTest::newRow( "no reduction 2" ) << QStringLiteral( "field = 'value' OR field != 'value2' OR field = 'value3'" ) << QStringLiteral( "field = 'value' OR field <> 'value2' OR field = 'value3'" ); + + } + + void testCollapseOrValues() + { + QFETCH( QString, expression ); + QFETCH( QString, expected ); + + QgsExpression exp( expression ); + QgsExpressionContext context; + exp.prepare( &context ); + QCOMPARE( exp.rootNode()->effectiveNode()->dump(), expected ); + } + void testPrecomputedNodesWithIntrospectionFunctions() { QgsFields fields; diff --git a/tests/src/gui/testqgsrelationeditorwidget.cpp b/tests/src/gui/testqgsrelationeditorwidget.cpp index 8136747b9f0c..e14df205107a 100644 --- a/tests/src/gui/testqgsrelationeditorwidget.cpp +++ b/tests/src/gui/testqgsrelationeditorwidget.cpp @@ -41,6 +41,7 @@ class TestQgsRelationEditorWidget : public QObject void testMultiEdit1N(); void testMultiEditNM(); + void testFeatureRequest(); void testUpdateUi(); private: @@ -159,7 +160,7 @@ void TestQgsRelationEditorWidget::init() mLayerJoin->commitChanges(); QgsFeature jft3( mLayerJoin->fields() ); - jft3.setAttribute( QStringLiteral( "pk" ), 102 ); + jft3.setAttribute( QStringLiteral( "pk" ), 103 ); jft3.setAttribute( QStringLiteral( "fk_layer1" ), 0 ); jft3.setAttribute( QStringLiteral( "fk_layer2" ), 11 ); mLayerJoin->startEditing(); @@ -285,6 +286,31 @@ void TestQgsRelationEditorWidget::testMultiEditNM() } +void TestQgsRelationEditorWidget::testFeatureRequest() +{ + + // Init a relation editor widget + QgsRelationEditorWidget relationEditorWidget( QVariantMap(), + new QWidget() ); + relationEditorWidget.setRelations( *mRelation1N, *mRelationNM ); + + QVERIFY( !relationEditorWidget.multiEditModeActive() ); + + QgsFeatureIds featureIds; + QgsFeatureIterator featureIterator = mLayer1->getFeatures(); + QgsFeature feature; + featureIterator.nextFeature( feature ); + relationEditorWidget.setFeature( feature ); + + QgsAttributeEditorContext context; + QgsTrackedVectorLayerTools tools; + context.setVectorLayerTools( &tools ); + relationEditorWidget.setEditorContext( context ); + + relationEditorWidget.updateUiSingleEdit(); + QCOMPARE( relationEditorWidget.mFeatureRequest.filterExpression()->expression(), QStringLiteral( "\"pk\" IN (10,11)" ) ); +} + void TestQgsRelationEditorWidget::testUpdateUi() { // Test that we don't recreate all the widget when only the request have been updated