Skip to content

Commit

Permalink
[OGR provider] Revise significantly the way we handle subset filter t…
Browse files Browse the repository at this point in the history 
…o avoid issues with FID (fixes #20136)

Some rationale on this change...

Previously when applying a "regular" subset string, ie. one that is only the
content of a where clause, we issued a full "SELECT * FROM layer WHERE subsetstring",
resulting in a OGR SQL layer. The caveat of that is that most OGR drivers
will have issues retaining the original FID. A hack consisting in adding a
{original_fid_name} as orig_ogc_fid to the select columns was introduced in
4ce2cf1
to try to retain the original FID, but this added a lot of complexity. And
actually, in the case of the OGR GPKG driver, it caused it to still be confused
when analyzing the column definition of the resulting layer, since it sees
2 FID columns despite the renaming (one included in the '*' wildcard, and the
one of orig_ogc_fid), which caused it to use sequential FID numbering (the
driver when seeing more than once a column that is the FID column assumes that
some cross join is done, and thus that FID are unreliable)

A simpler and more robust (crossing fingers!) approach in that case is
just to use OGR_L_SetAttributeFilter() instead of GDALDatasetExecuteSQL().
Some care must be taken to cancel the filter when removing the subset
filter, or in QgsOgrFeatureIterator when combining with the filter
expression coming from the request, but besides that, this is more
straightforward, and actually solves #20136
  • Loading branch information
@rouault
rouault committed Oct 18, 2018
1 parent 7241025 commit 391ec8a
Show file tree
Hide file tree
Showing 7 changed files with 147 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 391ec8a

Please sign in to comment.