Skip to content

Commit

Permalink
[mssql] Use Filter instead of STIntersects to improve query performance
Browse files Browse the repository at this point in the history
...and refine validity test from 57dc3c7

Using Filter is more performant since it does a bounding box only
check when an appropriate spatial index is available. This matches
the behavior with other providers, where the provider filter only
does a bounding box check and callers must perform the actual
intersection check if required.

While Filter also avoids SQL server closing the iterator upon
encountering invalid geometries, we can't rely on this because SQL
server will transparently fall back to STIntersects if it
decides there's no suitable spatial indexes available, and then
throw an exception on invalid geometries. So we still require
the STISValid check when using Filter.

The extent calculation from 57dc3c7 has been refined to
avoid the very expensive STMakeValid call. Instead we use a (ugly!)
workaround to skip invalid geometries using STIsValid only
inside the min/max x/y aggregates. Note that we can't just
dump a WHERE STIsValid clause in to the statement because SQL
server still throws the exception. Gotta love it!

Fix #15752, #10947
  • Loading branch information
nyalldawson committed Jun 2, 2017
1 parent 039866d commit 62af54e
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 4 deletions.
2 changes: 1 addition & 1 deletion src/providers/mssql/qgsmssqlfeatureiterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ void QgsMssqlFeatureIterator::BuildStatement( const QgsFeatureRequest& request )
<< qgsDoubleToString( request.filterRect().xMinimum() ) << ' ' << qgsDoubleToString( request.filterRect().yMaximum() ) << ", "
<< qgsDoubleToString( request.filterRect().xMinimum() ) << ' ' << qgsDoubleToString( request.filterRect().yMinimum() );

mStatement += QString( " where [%1].STIsValid() = 1 AND [%1].STIntersects([%2]::STGeomFromText('POLYGON((%3))',%4)) = 1" ).arg(
mStatement += QString( " where [%1].STIsValid() = 1 AND [%1].Filter([%2]::STGeomFromText('POLYGON((%3))',%4)) = 1" ).arg(
mSource->mGeometryColName, mSource->mGeometryColType, r, QString::number( mSource->mSRId ) );
filterAdded = true;
}
Expand Down
6 changes: 3 additions & 3 deletions src/providers/mssql/qgsmssqlprovider.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -690,14 +690,14 @@ void QgsMssqlProvider::UpdateStatistics( bool estimate )
if ( estimate )
{
if ( mGeometryColType == "geometry" )
statement = QString( "select min([%1].MakeValid().STPointN(1).STX), min([%1].MakeValid().STPointN(1).STY), max([%1].MakeValid().STPointN(1).STX), max([%1].MakeValid().STPointN(1).STY)" ).arg( mGeometryColName );
statement = QString( "select min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STX else NULL end), min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STY else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STX else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).STY else NULL end)" ).arg( mGeometryColName );
else
statement = QString( "select min([%1].MakeValid().STPointN(1).Long), min([%1].MakeValid().STPointN(1).Lat), max([%1].MakeValid().STPointN(1).Long), max([%1].MakeValid().STPointN(1).Lat)" ).arg( mGeometryColName );
statement = QString( "select min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Long else NULL end), min(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Lat else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Long else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STPointN(1).Lat else NULL end)" ).arg( mGeometryColName );
}
else
{
if ( mGeometryColType == "geometry" )
statement = QString( "select min([%1].MakeValid().STEnvelope().STPointN(1).STX), min([%1].MakeValid().STEnvelope().STPointN(1).STY), max([%1].MakeValid().STEnvelope().STPointN(3).STX), max([%1].MakeValid().STEnvelope().STPointN(3).STY)" ).arg( mGeometryColName );
statement = QString( "select min(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(1).STX else NULL end), min(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(1).STY else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(3).STX else NULL end), max(case when ([%1].STIsValid() = 1) THEN [%1].STEnvelope().STPointN(3).STY else NULL end)" ).arg( mGeometryColName );
else
{
statement = QString( "select [%1]" ).arg( mGeometryColName );
Expand Down
8 changes: 8 additions & 0 deletions tests/src/python/test_provider_mssql.py
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ def testInvalidGeometries(self):
(self.dbconn), "testinvalid", "mssql")
assert(vl.isValid())

self.assertEqual(vl.dataProvider().extent().toString(1), QgsRectangle(173.953, -41.513, 173.967, -41.502).toString(1))

#burn through features - don't want SQL server to trip up on the invalid ones
count = 0
for f in vl.dataProvider().getFeatures():
Expand All @@ -147,6 +149,12 @@ def testInvalidGeometries(self):
self.assertEqual(count, 37)
# sorry... you get NO chance to see these features exist and repair them... because SQL server. Use PostGIS instead and live a happier life!

# with estimated metadata
vl = QgsVectorLayer('%s srid=4167 type=POLYGON estimatedmetadata=true table="qgis_test"."invalid_polys" (ogr_geometry) sql=' %
(self.dbconn), "testinvalid", "mssql")
assert(vl.isValid())
self.assertEqual(vl.dataProvider().extent().toString(1), QgsRectangle(173.954, -41.513, 173.967, -41.502).toString(1))


if __name__ == '__main__':
unittest.main()

0 comments on commit 62af54e

Please sign in to comment.