Skip to content

Commit

Permalink
Merge pull request #36866 from elpaso/bugfix-gh36167-slow-creation-on…
Browse files Browse the repository at this point in the history 
…-joins

Optimize unique values checks in QgsVectorLayerUtils
  • Loading branch information
@elpaso
elpaso committed Jun 2, 2020
2 parents 2f9e57d + 7e3340a commit ebc5995
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 46 deletions.
127 changes: 81 additions & 46 deletions src/core/qgsvectorlayerutils.cpp
View file
Expand Up @@ -165,6 +165,24 @@ bool QgsVectorLayerUtils::valueExists( const QgsVectorLayer *layer, int fieldInd
if ( fieldIndex < 0 || fieldIndex >= fields.count() )
return false;

// If it's a joined field search the value in the source layer
if ( fields.fieldOrigin( fieldIndex ) == QgsFields::FieldOrigin::OriginJoin )
{
int srcFieldIndex;
const QgsVectorLayerJoinInfo *joinInfo { layer->joinBuffer()->joinForFieldIndex( fieldIndex, fields, srcFieldIndex ) };
if ( ! joinInfo )
{
return false;
}
fieldIndex = srcFieldIndex;
layer = joinInfo->joinLayer();
if ( ! layer )
{
return false;
}
fields = layer->fields();
}

QString fieldName = fields.at( fieldIndex ).name();

// build up an optimised feature request
Expand Down Expand Up @@ -395,6 +413,8 @@ bool QgsVectorLayerUtils::validateAttribute( const QgsVectorLayer *layer, const
}
}

bool notNullConstraintViolated { false };

if ( constraints.constraints() & QgsFieldConstraints::ConstraintNotNull
&& ( strength == QgsFieldConstraints::ConstraintStrengthNotSet || strength == constraints.constraintStrength( QgsFieldConstraints::ConstraintNotNull ) )
&& ( origin == QgsFieldConstraints::ConstraintOriginNotSet || origin == constraints.constraintOrigin( QgsFieldConstraints::ConstraintNotNull ) ) )
Expand All @@ -414,30 +434,37 @@ bool QgsVectorLayerUtils::validateAttribute( const QgsVectorLayer *layer, const
if ( value.isNull() )
{
errors << QObject::tr( "value is NULL" );
notNullConstraintViolated = true;
}
}
}

if ( constraints.constraints() & QgsFieldConstraints::ConstraintUnique
&& ( strength == QgsFieldConstraints::ConstraintStrengthNotSet || strength == constraints.constraintStrength( QgsFieldConstraints::ConstraintUnique ) )
&& ( origin == QgsFieldConstraints::ConstraintOriginNotSet || origin == constraints.constraintOrigin( QgsFieldConstraints::ConstraintUnique ) ) )
// if a NOT NULL constraint is violated we don't need to check for UNIQUE
if ( ! notNullConstraintViolated )
{
bool exempt = false;
if ( fields.fieldOrigin( attributeIndex ) == QgsFields::OriginProvider
&& constraints.constraintOrigin( QgsFieldConstraints::ConstraintNotNull ) == QgsFieldConstraints::ConstraintOriginProvider )
{
int providerIdx = fields.fieldOriginIndex( attributeIndex );
exempt = layer->dataProvider()->skipConstraintCheck( providerIdx, QgsFieldConstraints::ConstraintUnique, value );
}

if ( !exempt )
if ( constraints.constraints() & QgsFieldConstraints::ConstraintUnique
&& ( strength == QgsFieldConstraints::ConstraintStrengthNotSet || strength == constraints.constraintStrength( QgsFieldConstraints::ConstraintUnique ) )
&& ( origin == QgsFieldConstraints::ConstraintOriginNotSet || origin == constraints.constraintOrigin( QgsFieldConstraints::ConstraintUnique ) ) )
{
bool alreadyExists = QgsVectorLayerUtils::valueExists( layer, attributeIndex, value, QgsFeatureIds() << feature.id() );
valid = valid && !alreadyExists;
bool exempt = false;
if ( fields.fieldOrigin( attributeIndex ) == QgsFields::OriginProvider
&& constraints.constraintOrigin( QgsFieldConstraints::ConstraintNotNull ) == QgsFieldConstraints::ConstraintOriginProvider )
{
int providerIdx = fields.fieldOriginIndex( attributeIndex );
exempt = layer->dataProvider()->skipConstraintCheck( providerIdx, QgsFieldConstraints::ConstraintUnique, value );
}

if ( alreadyExists )
if ( !exempt )
{
errors << QObject::tr( "value is not unique" );

bool alreadyExists = QgsVectorLayerUtils::valueExists( layer, attributeIndex, value, QgsFeatureIds() << feature.id() );
valid = valid && !alreadyExists;

if ( alreadyExists )
{
errors << QObject::tr( "value is not unique" );
}
}
}
}
Expand Down Expand Up @@ -472,7 +499,26 @@ QgsFeatureList QgsVectorLayerUtils::createFeatures( const QgsVectorLayer *layer,
QgsFields fields = layer->fields();

// Cache unique values
QMap<int, QSet<QVariant>> uniqueValueCaches;
QMap<int, QSet<QVariant>> uniqueValueCache;

auto checkUniqueValue = [ & ]( const int fieldIdx, const QVariant & value )
{
if ( ! uniqueValueCache.contains( fieldIdx ) )
{
// If the layer is filtered, get unique values from an unfiltered clone
if ( ! layer->subsetString().isEmpty() )
{
std::unique_ptr<QgsVectorLayer> unfilteredClone { layer->clone( ) };
unfilteredClone->setSubsetString( QString( ) );
uniqueValueCache[ fieldIdx ] = unfilteredClone->uniqueValues( fieldIdx );
}
else
{
uniqueValueCache[ fieldIdx ] = layer->uniqueValues( fieldIdx );
}
}
return uniqueValueCache[ fieldIdx ].contains( value );
};

for ( const auto &fd : qgis::as_const( featuresData ) )
{
Expand All @@ -496,27 +542,11 @@ QgsFeatureList QgsVectorLayerUtils::createFeatures( const QgsVectorLayer *layer,
v = fd.attributes().value( idx );
}

// Cache unique values
if ( hasUniqueConstraint && ! uniqueValueCaches.contains( idx ) )
{
// If the layer is filtered, get unique values from an unfiltered clone
if ( ! layer->subsetString().isEmpty() )
{
std::unique_ptr<QgsVectorLayer> unfilteredClone { layer->clone( ) };
unfilteredClone->setSubsetString( QString( ) );
uniqueValueCaches[ idx ] = unfilteredClone->uniqueValues( idx );
}
else
{
uniqueValueCaches[ idx ] = layer->uniqueValues( idx );
}
}

// 2. client side default expression
// note - deliberately not using else if!
QgsDefaultValue defaultValueDefinition = layer->defaultValueDefinition( idx );
if ( ( v.isNull() || ( hasUniqueConstraint
&& uniqueValueCaches[ idx ].contains( v ) )
&& checkUniqueValue( idx, v ) )
|| defaultValueDefinition.applyOnUpdate() )
&& defaultValueDefinition.isValid() )
{
Expand All @@ -529,7 +559,7 @@ QgsFeatureList QgsVectorLayerUtils::createFeatures( const QgsVectorLayer *layer,
// 3. provider side default value clause
// note - not an else if deliberately. Users may return null from a default value expression to fallback to provider defaults
if ( ( v.isNull() || ( hasUniqueConstraint
&& uniqueValueCaches[ idx ].contains( v ) ) )
&& checkUniqueValue( idx, v ) ) )
&& fields.fieldOrigin( idx ) == QgsFields::OriginProvider )
{
int providerIndex = fields.fieldOriginIndex( idx );
Expand All @@ -544,7 +574,7 @@ QgsFeatureList QgsVectorLayerUtils::createFeatures( const QgsVectorLayer *layer,
// 4. provider side default literal
// note - deliberately not using else if!
if ( ( v.isNull() || ( checkUnique && hasUniqueConstraint
&& uniqueValueCaches[ idx ].contains( v ) ) )
&& checkUniqueValue( idx, v ) ) )
&& fields.fieldOrigin( idx ) == QgsFields::OriginProvider )
{
int providerIndex = fields.fieldOriginIndex( idx );
Expand All @@ -563,21 +593,26 @@ QgsFeatureList QgsVectorLayerUtils::createFeatures( const QgsVectorLayer *layer,
v = fd.attributes().value( idx );
}

// last of all... check that unique constraints are respected
// we can't handle not null or expression constraints here, since there's no way to pick a sensible
// value if the constraint is violated
if ( checkUnique && hasUniqueConstraint )
// last of all... check that unique constraints are respected if the value is valid
if ( v.isValid() )
{
if ( uniqueValueCaches[ idx ].contains( v ) )
// we can't handle not null or expression constraints here, since there's no way to pick a sensible
// value if the constraint is violated
if ( checkUnique && hasUniqueConstraint )
{
if ( checkUniqueValue( idx, v ) )
{
// unique constraint violated
QVariant uniqueValue = QgsVectorLayerUtils::createUniqueValueFromCache( layer, idx, uniqueValueCache[ idx ], v );
if ( uniqueValue.isValid() )
v = uniqueValue;
}
}
if ( hasUniqueConstraint )
{
// unique constraint violated
QVariant uniqueValue = QgsVectorLayerUtils::createUniqueValueFromCache( layer, idx, uniqueValueCaches[ idx ], v );
if ( uniqueValue.isValid() )
v = uniqueValue;
uniqueValueCache[ idx ].insert( v );
}
}
if ( hasUniqueConstraint )
uniqueValueCaches[ idx ].insert( v );
newFeature.setAttribute( idx, v );
}
result.append( newFeature );
Expand Down
38 changes: 38 additions & 0 deletions tests/src/python/test_qgsvectorlayerutils.py
View file
Expand Up @@ -31,6 +31,7 @@
QgsMemoryProviderUtils,
QgsWkbTypes,
QgsCoordinateReferenceSystem,
QgsVectorLayerJoinInfo,
NULL
)
from qgis.testing import start_app, unittest
Expand Down Expand Up @@ -93,6 +94,43 @@ def test_value_exists(self):
self.assertFalse(QgsVectorLayerUtils.valueExists(layer, 1, 125, [99999, 3]))
self.assertFalse(QgsVectorLayerUtils.valueExists(layer, 1, 125, [2, 4, 5, 3]))

def test_value_exists_joins(self):
"""Test that unique values in fields from joined layers, see GH #36167"""

p = QgsProject()
main_layer = QgsVectorLayer("Point?field=fid:integer",
"main_layer", "memory")
self.assertTrue(main_layer.isValid())
# Attr layer is joined with layer on fk ->
attr_layer = QgsVectorLayer("Point?field=id:integer&field=fk:integer",
"attr_layer", "memory")
self.assertTrue(attr_layer.isValid())

p.addMapLayers([main_layer, attr_layer])
join_info = QgsVectorLayerJoinInfo()
join_info.setJoinLayer(attr_layer)
join_info.setJoinFieldName('fk')
join_info.setTargetFieldName('fid')
join_info.setUsingMemoryCache(True)
main_layer.addJoin(join_info)
main_layer.updateFields()
join_buffer = main_layer.joinBuffer()
self.assertTrue(join_buffer.containsJoins())
self.assertEqual(main_layer.fields().names(), ['fid', 'attr_layer_id'])

f = QgsFeature(main_layer.fields())
f.setAttributes([1])
main_layer.dataProvider().addFeatures([f])

f = QgsFeature(attr_layer.fields())
f.setAttributes([1, 1])
attr_layer.dataProvider().addFeatures([f])

self.assertTrue(QgsVectorLayerUtils.valueExists(main_layer, 0, 1))
self.assertTrue(QgsVectorLayerUtils.valueExists(main_layer, 1, 1))
self.assertFalse(QgsVectorLayerUtils.valueExists(main_layer, 0, 2))
self.assertFalse(QgsVectorLayerUtils.valueExists(main_layer, 1, 2))

def test_validate_attribute(self):
""" test validating attributes against constraints """
layer = createLayerWithOnePoint()
Expand Down

0 comments on commit ebc5995

Please sign in to comment.