Skip to content

Commit

Permalink
Merge pull request #43083 from troopa81/fix_pg_inor_text
Browse files Browse the repository at this point in the history 
[Postgres] Use IN clause instead of OR for whereClause on text primary keys
  • Loading branch information
@m-kuhn
m-kuhn committed May 7, 2021
2 parents ef9dd06 + 9832e56 commit 8651055
Show file tree
Hide file tree
Showing 2 changed files with 162 additions and 23 deletions.
51 changes: 28 additions & 23 deletions src/providers/postgres/qgspostgresprovider.cpp
View file
Expand Up @@ -591,6 +591,29 @@ QString QgsPostgresUtils::whereClause( QgsFeatureId featureId, const QgsFields &

QString QgsPostgresUtils::whereClause( const QgsFeatureIds &featureIds, const QgsFields &fields, QgsPostgresConn *conn, QgsPostgresPrimaryKeyType pkType, const QList<int> &pkAttrs, const std::shared_ptr<QgsPostgresSharedData> &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 : std::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:
Expand All @@ -616,34 +639,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 : std::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 : std::as_const( featureIds ) )
Expand Down
134 changes: 134 additions & 0 deletions tests/src/providers/testqgspostgresprovider.cpp
View file
Expand Up @@ -14,6 +14,8 @@
***************************************************************************/
#include "qgstest.h"
#include <QObject>
#include <QRegularExpression>
#include <QRegularExpressionMatch>

#include <qgspostgresprovider.h>
#include <qgspostgresconn.h>
Expand All @@ -38,6 +40,7 @@ class TestQgsPostgresProvider: public QObject
void decodeJsonbMap();
void testDecodeDateTimes();
void testQuotedValueBigInt();
void testWhereClauseFids();
};


Expand Down Expand Up @@ -295,5 +298,136 @@ void TestQgsPostgresProvider::testQuotedValueBigInt()
QCOMPARE( QgsPostgresUtils::whereClause( 1LL, fields, NULL, QgsPostgresPrimaryKeyType::PktFidMap, pkAttrs, std::shared_ptr<QgsPostgresSharedData>( 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<int> 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<QgsPostgresSharedData>( 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<QgsPostgresSharedData>( 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<QgsPostgresSharedData>( 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<QgsPostgresSharedData>( 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<QgsPostgresSharedData>( sdata ) ),
QStringList() << "\"fld_int\"=42 AND \"fld\"::text='QGIS ''Rocks''!'"
<< "\"fld_int\"=43 AND \"fld\"::text='PostGIS too!'" );
}

QGSTEST_MAIN( TestQgsPostgresProvider )
#include "testqgspostgresprovider.moc"

0 comments on commit 8651055

Please sign in to comment.