Skip to content

Commit

Permalink
Merge pull request #8216 from rouault/fix_20136
Browse files Browse the repository at this point in the history 
[OGR provider] Revise significantly the way we handle subset filter to avoid issues with FID (fixes #20136)
  • Loading branch information
@m-kuhn
m-kuhn committed Oct 19, 2018
2 parents f23b09b + 6745eec commit 763fa42
Show file tree
Hide file tree
Showing 7 changed files with 192 additions and 149 deletions.
88 changes: 31 additions & 57 deletions src/providers/ogr/qgsogrfeatureiterator.cpp
View file
Expand Up @@ -51,7 +51,7 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
mRequest.setTimeout( -1 );
if ( mSharedDS )
{
mOgrLayer = mSharedDS->createSQLResultLayer( mSource->mEncoding, mSource->mLayerName, mSource->mLayerIndex );
mOgrLayer = mSharedDS->getLayerFromNameOrIndex( mSource->mLayerName, mSource->mLayerIndex );
if ( !mOgrLayer )
{
return;
Expand Down Expand Up @@ -81,13 +81,9 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool

if ( !mSource->mSubsetString.isEmpty() )
{
mOgrOrigLayer = mOgrLayer;
mOgrLayerWithFid = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, QString(), true, &mOrigFidAdded );
mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString, true, &mOrigFidAdded );

OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( mOgrLayer );
QByteArray fidColumn( OGR_L_GetFIDColumn( mOgrLayer ) );
mFirstFieldIsFid = !fidColumn.isEmpty() && OGR_FD_GetFieldIndex( fdef, fidColumn ) < 0;
mOgrLayerOri = mOgrLayer;
mOgrLayer = QgsOgrProviderUtils::setSubsetString( mOgrLayer, mConn->ds, mSource->mEncoding, mSource->mSubsetString );
// If the mSubsetString was a full SELECT ...., then mOgrLayer will be a OGR SQL layer != mOgrLayerOri

mFieldsWithoutFid.clear();
for ( int i = ( mFirstFieldIsFid ) ? 1 : 0; i < mSource->mFields.size(); i++ )
Expand Down Expand Up @@ -122,8 +118,6 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
( mSource->mOgrGeometryTypeFilter != wkbUnknown );

QgsAttributeList attrs = ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes ) ? mRequest.subsetOfAttributes() : mSource->mFields.allAttributesList();
if ( mOrigFidAdded )
attrs << mSource->mFields.count();

// ensure that all attributes required for expression filter are being fetched
if ( mRequest.flags() & QgsFeatureRequest::SubsetOfAttributes && request.filterType() == QgsFeatureRequest::FilterExpression )
Expand Down Expand Up @@ -158,23 +152,23 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
// filter if we choose to ignore them (fixes #11223)
if ( ( mSource->mDriverName != QLatin1String( "VRT" ) && mSource->mDriverName != QLatin1String( "OGR_VRT" ) ) || mFilterRect.isNull() )
{
QgsOgrProviderUtils::setRelevantFields( mOgrLayer, mSource->mFields.count() + ( mOrigFidAdded ? 1 : 0 ), mFetchGeometry, attrs, mSource->mFirstFieldIsFid );
if ( mOgrLayerWithFid )
QgsOgrProviderUtils::setRelevantFields( mOgrLayerWithFid, mSource->mFields.count() + ( mOrigFidAdded ? 1 : 0 ), mFetchGeometry, attrs, mSource->mFirstFieldIsFid );
QgsOgrProviderUtils::setRelevantFields( mOgrLayer, mSource->mFields.count(), mFetchGeometry, attrs, mSource->mFirstFieldIsFid, mSource->mSubsetString );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
QgsOgrProviderUtils::setRelevantFields( mOgrLayerOri, mSource->mFields.count(), mFetchGeometry, attrs, mSource->mFirstFieldIsFid, mSource->mSubsetString );
}

// spatial query to select features
if ( !mFilterRect.isNull() )
{
OGR_L_SetSpatialFilterRect( mOgrLayer, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
if ( mOgrLayerWithFid )
OGR_L_SetSpatialFilterRect( mOgrLayerWithFid, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilterRect( mOgrLayerOri, mFilterRect.xMinimum(), mFilterRect.yMinimum(), mFilterRect.xMaximum(), mFilterRect.yMaximum() );
}
else
{
OGR_L_SetSpatialFilter( mOgrLayer, nullptr );
if ( mOgrLayerWithFid )
OGR_L_SetSpatialFilter( mOgrLayerWithFid, nullptr );
if ( mOgrLayerOri && mOgrLayerOri != mOgrLayer )
OGR_L_SetSpatialFilter( mOgrLayerOri, nullptr );
}

if ( request.filterType() == QgsFeatureRequest::FilterExpression
Expand All @@ -194,21 +188,27 @@ QgsOgrFeatureIterator::QgsOgrFeatureIterator( QgsOgrFeatureSource *source, bool
if ( result == QgsSqlExpressionCompiler::Complete || result == QgsSqlExpressionCompiler::Partial )
{
QString whereClause = compiler->result();
if ( !mSource->mSubsetString.isEmpty() && mOgrLayer == mOgrLayerOri )
{
whereClause = QStringLiteral( "(" ) + mSource->mSubsetString +
QStringLiteral( ") AND (" ) + whereClause +
QStringLiteral( ")" );
}
if ( OGR_L_SetAttributeFilter( mOgrLayer, mSource->mEncoding->fromUnicode( whereClause ).constData() ) == OGRERR_NONE )
{
//if only partial success when compiling expression, we need to double-check results using QGIS' expressions
mExpressionCompiled = ( result == QgsSqlExpressionCompiler::Complete );
mCompileStatus = ( mExpressionCompiled ? Compiled : PartiallyCompiled );
}
}
else
else if ( mSource->mSubsetString.isEmpty() )
{
OGR_L_SetAttributeFilter( mOgrLayer, nullptr );
}

delete compiler;
}
else
else if ( mSource->mSubsetString.isEmpty() )
{
OGR_L_SetAttributeFilter( mOgrLayer, nullptr );
}
Expand All @@ -235,19 +235,7 @@ bool QgsOgrFeatureIterator::fetchFeatureWithId( QgsFeatureId id, QgsFeature &fea
{
feature.setValid( false );
gdal::ogr_feature_unique_ptr fet;
if ( mOrigFidAdded )
{
// if we have been using the original fid as feature IDs (e.g. as a result of a subset
// string in place), then we need to request features from the original, unfiltered
// OGR layer. Otherwise the feature IDs we request will correspond to features in the
// filtered layer, not the original layer!
OGR_L_ResetReading( mOgrLayerWithFid );
fet.reset( OGR_L_GetFeature( mOgrLayerWithFid, FID_TO_NUMBER( id ) ) );
}
else
{
fet.reset( OGR_L_GetFeature( mOgrLayer, FID_TO_NUMBER( id ) ) );
}
fet.reset( OGR_L_GetFeature( mOgrLayer, FID_TO_NUMBER( id ) ) );

if ( !fet )
{
Expand Down Expand Up @@ -375,11 +363,7 @@ bool QgsOgrFeatureIterator::close()
{
iteratorClosed();

if ( mOgrLayer )
{
mSharedDS->releaseResultSet( mOgrLayer );
mOgrLayer = nullptr;
}
mOgrLayer = nullptr;
mSharedDS.reset();
mClosed = true;
return true;
Expand All @@ -396,13 +380,14 @@ bool QgsOgrFeatureIterator::close()
resetReading();
}

if ( mOgrOrigLayer )
if ( mOgrLayerOri )
{
GDALDatasetReleaseResultSet( mConn->ds, mOgrLayer );
GDALDatasetReleaseResultSet( mConn->ds, mOgrLayerWithFid );
mOgrLayer = mOgrOrigLayer;
mOgrOrigLayer = nullptr;
mOgrLayerWithFid = nullptr;
if ( mOgrLayer != mOgrLayerOri )
{
GDALDatasetReleaseResultSet( mConn->ds, mOgrLayer );
}
mOgrLayer = nullptr;
mOgrLayerOri = nullptr;
}

if ( mConn )
Expand Down Expand Up @@ -438,19 +423,7 @@ void QgsOgrFeatureIterator::getFeatureAttribute( OGRFeatureH ogrFet, QgsFeature

bool QgsOgrFeatureIterator::readFeature( gdal::ogr_feature_unique_ptr fet, QgsFeature &feature ) const
{
if ( mOrigFidAdded )
{
OGRFeatureDefnH fdef = OGR_L_GetLayerDefn( mOgrLayer );
int lastField = OGR_FD_GetFieldCount( fdef ) - 1;
if ( lastField >= 0 )
feature.setId( OGR_F_GetFieldAsInteger64( fet.get(), lastField ) );
else
feature.setId( OGR_F_GetFID( fet.get() ) );
}
else
{
feature.setId( OGR_F_GetFID( fet.get() ) );
}
feature.setId( OGR_F_GetFID( fet.get() ) );
feature.initAttributes( mSource->mFields.count() );
feature.setFields( mSource->mFields ); // allow name-based attribute lookups

Expand Down Expand Up @@ -508,7 +481,8 @@ bool QgsOgrFeatureIterator::readFeature( gdal::ogr_feature_unique_ptr fet, QgsFe
else
{
// all attributes
for ( int idx = 0; idx < mSource->mFields.count(); ++idx )
const auto fieldCount = mSource->mFields.count();
for ( int idx = 0; idx < fieldCount; ++idx )
{
getFeatureAttribute( fet.get(), feature, idx );
}
Expand Down
7 changes: 2 additions & 5 deletions src/providers/ogr/qgsogrfeatureiterator.h
View file
Expand Up @@ -79,11 +79,8 @@ class QgsOgrFeatureIterator : public QgsAbstractFeatureIteratorFromSource<QgsOgr
void getFeatureAttribute( OGRFeatureH ogrFet, QgsFeature &f, int attindex ) const;

QgsOgrConn *mConn = nullptr;
OGRLayerH mOgrLayer = nullptr;
OGRLayerH mOgrOrigLayer = nullptr;
OGRLayerH mOgrLayerWithFid = nullptr;

bool mOrigFidAdded = false;
OGRLayerH mOgrLayer = nullptr; // when mOgrLayerUnfiltered != null and mOgrLayer != mOgrLayerUnfiltered, this is a SQL layer
OGRLayerH mOgrLayerOri = nullptr; // only set when there's a mSubsetString. In which case this a regular OGR layer. Potentially == mOgrLayer

//! Sets to true, if geometry is in the requested columns
bool mFetchGeometry = false;
Expand Down

0 comments on commit 763fa42

Please sign in to comment.