From 6b728eb67850751fc7132458bc68792e163998d4 Mon Sep 17 00:00:00 2001 From: Julien Cabieces Date: Wed, 5 May 2021 11:40:10 +0200 Subject: [PATCH] [Postgres] Use IN clause instead of OR for whereClause on text primary keys --- .../postgres/qgspostgresprovider.cpp | 51 ++++--- .../src/providers/testqgspostgresprovider.cpp | 134 ++++++++++++++++++ 2 files changed, 162 insertions(+), 23 deletions(-) diff --git a/src/providers/postgres/qgspostgresprovider.cpp b/src/providers/postgres/qgspostgresprovider.cpp index cf15c32e0bdf..e89b0b074223 100644 --- a/src/providers/postgres/qgspostgresprovider.cpp +++ b/src/providers/postgres/qgspostgresprovider.cpp @@ -587,6 +587,29 @@ QString QgsPostgresUtils::whereClause( QgsFeatureId featureId, const QgsFields & QString QgsPostgresUtils::whereClause( const QgsFeatureIds &featureIds, const QgsFields &fields, QgsPostgresConn *conn, QgsPostgresPrimaryKeyType pkType, const QList &pkAttrs, const std::shared_ptr &sharedData ) { + auto lookupKeyWhereClause = [ = ] + { + if ( featureIds.isEmpty() ) + return QString(); + + //simple primary key, so prefer to use an "IN (...)" query. These are much faster then multiple chained ...OR... clauses + QString delim; + QString expr = QStringLiteral( "%1 IN (" ).arg( QgsPostgresConn::quotedIdentifier( fields.at( pkAttrs[0] ).name() ) ); + + for ( const QgsFeatureId featureId : qgis::as_const( featureIds ) ) + { + const QVariantList pkVals = sharedData->lookupKey( featureId ); + if ( !pkVals.isEmpty() ) + { + expr += delim + QgsPostgresConn::quotedValue( pkVals.at( 0 ) ); + delim = ','; + } + } + expr += ')'; + + return expr; + }; + switch ( pkType ) { case PktOid: @@ -612,34 +635,16 @@ QString QgsPostgresUtils::whereClause( const QgsFeatureIds &featureIds, const Qg } case PktInt64: case PktUint64: - { - QString expr; + return lookupKeyWhereClause(); - //simple primary key, so prefer to use an "IN (...)" query. These are much faster then multiple chained ...OR... clauses - if ( !featureIds.isEmpty() ) - { - QString delim; - expr = QStringLiteral( "%1 IN (" ).arg( QgsPostgresConn::quotedIdentifier( fields.at( pkAttrs[0] ).name() ) ); - - for ( const QgsFeatureId featureId : qgis::as_const( featureIds ) ) - { - QVariantList pkVals = sharedData->lookupKey( featureId ); - if ( !pkVals.isEmpty() ) - { - QgsField fld = fields.at( pkAttrs[0] ); - expr += delim + pkVals[0].toString(); - delim = ','; - } - } - expr += ')'; - } - - return expr; - } case PktFidMap: case PktTid: case PktUnknown: { + // on simple string primary key we can use IN + if ( pkType == PktFidMap && pkAttrs.count() == 1 && fields.at( pkAttrs[0] ).type() == QVariant::String ) + return lookupKeyWhereClause(); + //complex primary key, need to build up where string QStringList whereClauses; for ( const QgsFeatureId featureId : qgis::as_const( featureIds ) ) diff --git a/tests/src/providers/testqgspostgresprovider.cpp b/tests/src/providers/testqgspostgresprovider.cpp index 2e140ecf7f7e..5a68194e3a2a 100644 --- a/tests/src/providers/testqgspostgresprovider.cpp +++ b/tests/src/providers/testqgspostgresprovider.cpp @@ -14,6 +14,8 @@ ***************************************************************************/ #include "qgstest.h" #include +#include +#include #include #include @@ -38,6 +40,7 @@ class TestQgsPostgresProvider: public QObject void decodeJsonbMap(); void testDecodeDateTimes(); void testQuotedValueBigInt(); + void testWhereClauseFids(); }; @@ -295,5 +298,136 @@ void TestQgsPostgresProvider::testQuotedValueBigInt() QCOMPARE( QgsPostgresUtils::whereClause( 1LL, fields, NULL, QgsPostgresPrimaryKeyType::PktFidMap, pkAttrs, std::shared_ptr( sdata ) ), QString( "\"fld_bigint\"=-9223372036854775800 AND \"fld_text\"::text='QGIS ''Rocks''!' AND \"fld_integer\"=42" ) ); } +void TestQgsPostgresProvider::testWhereClauseFids() +{ + // test the returned where clause according to given feature ids and primary key + + QgsFields fields; + QList pkAttrs; + QString clause; + + std::shared_ptr< QgsPostgresSharedData > sdata( new QgsPostgresSharedData() ); + + QgsField f0, f1, f2, f3; + + // need regular expression to check IN/OR because QgsFeatureIds is a set and ids could come + // in various order + +#define CHECK_IN_CLAUSE(whereClause,expectedValues) \ + { \ + QRegularExpression inRe("\\\"fld\\\" IN \\(([^,]*),([^,]*)\\)"); \ + QVERIFY(inRe.isValid()); \ + QRegularExpressionMatch match = inRe.match( whereClause ); \ + QVERIFY( match.hasMatch() ); \ + QStringList values; \ + values << match.captured(1); \ + values << match.captured(2); \ + std::sort( values.begin(), values.end() ); \ + QCOMPARE( values, expectedValues ); \ + } + + // QRegularExpression inOr("\\(\\\"fld\\\"=([^,]*) OR \\\"fld\\\"=([^,]*)\\)"); + +#define CHECK_OR_CLAUSE(whereClause,expectedValues) \ + { \ + QRegularExpression inOr("\\((.*) OR (.*)\\)"); \ + QVERIFY(inOr.isValid()); \ + QRegularExpressionMatch match = inOr.match( whereClause ); \ + QVERIFY( match.hasMatch() ); \ + QStringList values; \ + values << match.captured(1); \ + values << match.captured(2); \ + std::sort( values.begin(), values.end() ); \ + QCOMPARE( values, expectedValues ); \ + } + + // 4 byte integer -> IN clause + f0.setName( "fld" ); + f0.setType( QVariant::Int ); + f0.setTypeName( "int4" ); + + // for positive integers, fid == the value, there is no map. + fields.append( f0 ); + pkAttrs.append( 0 ); + + sdata->insertFid( 42, QVariantList() << 42 ); + sdata->insertFid( 43, QVariantList() << 43 ); + + CHECK_IN_CLAUSE( QgsPostgresUtils::whereClause( QgsFeatureIds() << 42 << 43, fields, NULL, QgsPostgresPrimaryKeyType::PktInt, pkAttrs, std::shared_ptr( sdata ) ), + QStringList() << "42" << "43" ); + + // 8 byte integer -> IN clause + f1.setName( "fld" ); + f1.setType( QVariant::LongLong ); + f1.setTypeName( "int8" ); + + fields.clear(); + pkAttrs.clear(); + + fields.append( f1 ); + pkAttrs.append( 0 ); + + sdata->clear(); + sdata->insertFid( 1LL, QVariantList() << -9223372036854775800LL ); // way outside int4 range + sdata->insertFid( 2LL, QVariantList() << -9223372036854775801LL ); + + CHECK_IN_CLAUSE( QgsPostgresUtils::whereClause( QgsFeatureIds() << 1LL << 2LL, fields, NULL, QgsPostgresPrimaryKeyType::PktInt64, pkAttrs, std::shared_ptr( sdata ) ), + QStringList() << "-9223372036854775800" << "-9223372036854775801" ); + + // double -> OR clause + f2.setName( "fld" ); + f2.setType( QVariant::Double ); + f2.setTypeName( "float8" ); + + fields.clear(); + pkAttrs.clear(); + + fields.append( f2 ); + pkAttrs.append( 0 ); + + sdata->clear(); + sdata->insertFid( 1LL, QVariantList() << 3.141592741 ); + sdata->insertFid( 2LL, QVariantList() << 6.141592741 ); + + CHECK_OR_CLAUSE( QgsPostgresUtils::whereClause( QgsFeatureIds() << 1LL << 2LL, fields, NULL, QgsPostgresPrimaryKeyType::PktFidMap, pkAttrs, std::shared_ptr( sdata ) ), + QStringList() << "\"fld\"='3.141592741'" << "\"fld\"='6.141592741'" ); + + // text -> IN clause + f3.setName( "fld" ); + f3.setType( QVariant::String ); + f3.setTypeName( "text" ); + + fields.clear(); + pkAttrs.clear(); + + fields.append( f3 ); + pkAttrs.append( 0 ); + + sdata->clear(); + sdata->insertFid( 1LL, QVariantList() << QString( "QGIS 'Rocks'!" ) ); + sdata->insertFid( 2LL, QVariantList() << QString( "PostGIS too!" ) ); + + CHECK_IN_CLAUSE( QgsPostgresUtils::whereClause( QgsFeatureIds() << 1LL << 2LL, fields, NULL, QgsPostgresPrimaryKeyType::PktFidMap, pkAttrs, std::shared_ptr( sdata ) ), + QStringList() << "'PostGIS too!'" << "'QGIS ''Rocks''!'" ); + + // Composite text + int -> OR clause + f0.setName( "fld_int" ); + pkAttrs.clear(); + pkAttrs.append( 0 ); + pkAttrs.append( 1 ); + + fields.clear(); + fields.append( f0 ); + fields.append( f3 ); + + sdata->clear(); + sdata->insertFid( 1LL, QVariantList() << 42 << QString( "QGIS 'Rocks'!" ) ); + sdata->insertFid( 2LL, QVariantList() << 43 << QString( "PostGIS too!" ) ); + + CHECK_OR_CLAUSE( QgsPostgresUtils::whereClause( QgsFeatureIds() << 1LL << 2LL, fields, NULL, QgsPostgresPrimaryKeyType::PktFidMap, pkAttrs, std::shared_ptr( sdata ) ), + QStringList() << "\"fld_int\"=42 AND \"fld\"::text='QGIS ''Rocks''!'" + << "\"fld_int\"=43 AND \"fld\"::text='PostGIS too!'" ); +} + QGSTEST_MAIN( TestQgsPostgresProvider ) #include "testqgspostgresprovider.moc"