search

qgis / QGIS

QGIS is a free, open source, cross platform (lin/win/mac) geographical information system (GIS)
https://qgis.org
GNU General Public License v2.0
10.65k stars 3.01k forks source link

Change Way SDO_RELATE is used in QGIS spatial queries #24285

Open qgib opened 7 years ago

qgib commented 7 years ago

Author Name: Simon Greener (Simon Greener) Original Redmine Issue: 16375

Redmine category:data_provider/oracle

Background.

I executed an "identify" in QGIS against a single mapped object that comes from an Oracle table with sdo_geometry column.

Using database SQL Trace, I dumped the SQL that QGIS executes and I got the following (which I have compared to the C++ source code):

SELECT "GEOM","ROWID","FID","FID_INT","FID_NUM9","FID_NUM8","FID_NUM7", "AN_ATTRIBUTE" FROM "QGIS"."FOO" "FEATUREREQUEST" WHERE sdo_filter("GEOM", mdsys.sdo_geometry(2003,2872,NULL,mdsys.sdo_elem_info_array(1,1003,3), mdsys.sdo_ordinate_array(5992381.68001308757811785, 2112468.83721287222579122,5992389.06444318685680628, 2112476.22164297243580222)))='TRUE' AND sdo_relate("GEOM", mdsys.sdo_geometry(2003,2872,NULL,mdsys.sdo_elem_info_array(1,1003,3), mdsys.sdo_ordinate_array(5992381.68001308757811785, 2112468.83721287222579122,5992389.06444318685680628, 2112476.22164297243580222)),'mask=ANYINTERACT')='TRUE'

Looking at qgsoracleconn.cpp there's an hasSpatial() method which runs:

"SELECT 1 FROM v$option WHERE parameter='Spatial' AND value='TRUE'"

This is then used at line 96 of qgsoraclefeatureiterator.cpp where it says:

// sdo_relate requires Spatial

If the connection has Spatial, it appends this to the query where clause:

AND sdo_relate(%1,%2,'mask=ANYINTERACT')='TRUE'

Now, SDO_RELATE has always been a Location spatial filter, and so is free for all Oracle users. Only SDO_GEOM.RELATE requires Spatial in 11g but not 12c.

Additionally, SDO_RELATE is a secondary filter that executes a Primary filter (SDO_FILTER) internally to generate a candidate set of possible objects that are sent to the internal Secondary filter which does the topological analysis. Even if SDO_RELATE was a licensed filter, it does not need to be used in conjunction with SDO_FILTER. Additionally, the Oracle Query Optimizer is still not smart enough to realise that the SDO_FILTER is not needed so the use of it with SDO_RELATE doubles the primary filtering. So the above query should always be running SDO_RELATE… ANYINTERACT and leaving out the SDO_FILTER part. But, also, the "mask=ANYINTERACT" should be a bind variable as it could be set to "mask=CONTAINS" etc.

In summary (the sentences in bold are the requested functional changes):

  1. SDO_RELATE is a free function and not subject to Spatial licensing
    • the hasSpatial() test in the C++ is invalid and should be removed;
  2. SDO_GEOM.RELATE is a spatial function up till 12c and free afterwards
    • AFAIK QGIS currently doesn't use SDO_GEOM.RELATE;
  3. Because SDO_RELATE executes its own primary filtering, SDO_FILTER is not needed
    • Modify the C++ to remove SDO_FILTER when SDO_RELATE is needed.
  4. SDO_FILTER mask parameter should be a bind variable.

regards Simon

qgib commented 7 years ago

Author Name: Simon Greener (Simon Greener)

Reference for licensing.

11.2 [[https://docs.oracle.com/cd/E18283_01/appdev.112/e11830/sdo_locator.htm#i632018]]

12.2 [[http://docs.oracle.com/database/122/SPATL/oracle-locator.htm#SPATL340]]

Simon

qgib commented 7 years ago

Author Name: Giovanni Manghi (@gioman)

qgib commented 7 years ago

Author Name: Giovanni Manghi (@gioman)

I executed an "identify" in QGIS against a single mapped object that comes from an Oracle table with sdo_geometry column.

Using database SQL Trace, I dumped the SQL that QGIS executes and I got the following (which I have compared to the C++ source code):

SELECT "GEOM","ROWID","FID","FID_INT","FID_NUM9","FID_NUM8","FID_NUM7", "AN_ATTRIBUTE" FROM "QGIS"."FOO" "FEATUREREQUEST" WHERE sdo_filter("GEOM", mdsys.sdo_geometry(2003,2872,NULL,mdsys.sdo_elem_info_array(1,1003,3), mdsys.sdo_ordinate_array(5992381.68001308757811785, 2112468.83721287222579122,5992389.06444318685680628, 2112476.22164297243580222)))='TRUE' AND sdo_relate("GEOM", mdsys.sdo_geometry(2003,2872,NULL,mdsys.sdo_elem_info_array(1,1003,3), mdsys.sdo_ordinate_array(5992381.68001308757811785, 2112468.83721287222579122,5992389.06444318685680628, 2112476.22164297243580222)),'mask=ANYINTERACT')='TRUE'

Looking at qgsoracleconn.cpp there's an hasSpatial() method which runs:

"SELECT 1 FROM v$option WHERE parameter='Spatial' AND value='TRUE'"

This is then used at line 96 of qgsoraclefeatureiterator.cpp where it says:

// sdo_relate requires Spatial

If the connection has Spatial, it appends this to the query where clause:

AND sdo_relate(%1,%2,'mask=ANYINTERACT')='TRUE'

Now, SDO_RELATE has always been a Location spatial filter, and so is free for all Oracle users. Only SDO_GEOM.RELATE requires Spatial in 11g but not 12c.

Additionally, SDO_RELATE is a secondary filter that executes a Primary filter (SDO_FILTER) internally to generate a candidate set of possible objects that are sent to the internal Secondary filter which does the topological analysis. Even if SDO_RELATE was a licensed filter, it does not need to be used in conjunction with SDO_FILTER. Additionally, the Oracle Query Optimizer is still not smart enough to realise that the SDO_FILTER is not needed so the use of it with SDO_RELATE doubles the primary filtering. So the above query should always be running SDO_RELATE… ANYINTERACT and leaving out the SDO_FILTER part. But, also, the "mask=ANYINTERACT" should be a bind variable as it could be set to "mask=CONTAINS" etc.

In summary (the sentences in bold are the requested functional changes):

  1. SDO_RELATE is a free function and not subject to Spatial licensing
    • the hasSpatial() test in the C++ is invalid and should be removed;
  2. SDO_GEOM.RELATE is a spatial function up till 12c and free afterwards
    • AFAIK QGIS currently doesn't use SDO_GEOM.RELATE;
  3. Because SDO_RELATE executes its own primary filtering, SDO_FILTER is not needed
    • Modify the C++ to remove SDO_FILTER when SDO_RELATE is needed.
  4. SDO_FILTER mask parameter should be a bind variable.

regards Simon to Background.

I executed an "identify" in QGIS against a single mapped object that comes from an Oracle table with sdo_geometry column.

Using database SQL Trace, I dumped the SQL that QGIS executes and I got the following (which I have compared to the C++ source code):

SELECT "GEOM","ROWID","FID","FID_INT","FID_NUM9","FID_NUM8","FID_NUM7", "AN_ATTRIBUTE" FROM "QGIS"."FOO" "FEATUREREQUEST" WHERE sdo_filter("GEOM", mdsys.sdo_geometry(2003,2872,NULL,mdsys.sdo_elem_info_array(1,1003,3), mdsys.sdo_ordinate_array(5992381.68001308757811785, 2112468.83721287222579122,5992389.06444318685680628, 2112476.22164297243580222)))='TRUE' AND sdo_relate("GEOM", mdsys.sdo_geometry(2003,2872,NULL,mdsys.sdo_elem_info_array(1,1003,3), mdsys.sdo_ordinate_array(5992381.68001308757811785, 2112468.83721287222579122,5992389.06444318685680628, 2112476.22164297243580222)),'mask=ANYINTERACT')='TRUE'

Looking at qgsoracleconn.cpp there's an hasSpatial() method which runs:

"SELECT 1 FROM v$option WHERE parameter='Spatial' AND value='TRUE'"

This is then used at line 96 of qgsoraclefeatureiterator.cpp where it says:

// sdo_relate requires Spatial

If the connection has Spatial, it appends this to the query where clause:

AND sdo_relate(%1,%2,'mask=ANYINTERACT')='TRUE'

Now, SDO_RELATE has always been a Location spatial filter, and so is free for all Oracle users. Only SDO_GEOM.RELATE requires Spatial in 11g but not 12c.

Additionally, SDO_RELATE is a secondary filter that executes a Primary filter (SDO_FILTER) internally to generate a candidate set of possible objects that are sent to the internal Secondary filter which does the topological analysis. Even if SDO_RELATE was a licensed filter, it does not need to be used in conjunction with SDO_FILTER. Additionally, the Oracle Query Optimizer is still not smart enough to realise that the SDO_FILTER is not needed so the use of it with SDO_RELATE doubles the primary filtering. So the above query should always be running SDO_RELATE… ANYINTERACT and leaving out the SDO_FILTER part. But, also, the "mask=ANYINTERACT" should be a bind variable as it could be set to "mask=CONTAINS" etc.

In summary (the sentences in bold are the requested functional changes):

  1. SDO_RELATE is a free function and not subject to Spatial licensing
    • the hasSpatial() test in the C++ is invalid and should be removed;
  2. SDO_GEOM.RELATE is a spatial function up till 12c and free afterwards
    • AFAIK QGIS currently doesn't use SDO_GEOM.RELATE;
  3. Because SDO_RELATE executes its own primary filtering, SDO_FILTER is not needed
    • Modify the C++ to remove SDO_FILTER when SDO_RELATE is needed.
  4. SDO_FILTER mask parameter should be a bind variable.

regards Simon

qgib commented 7 years ago

Author Name: Jürgen Fischer (@jef-n)

paulwittle commented 5 years ago

I note this change has not been made yet but I think there is a bigger issue with this SQL in Oracle. The ROWID is not 100% reliable as a primary key as previous ROWID values can be reused if data is deleted and re-imported. It can also change dynamically if you have a partitioned table.

The result is that we are finding the basic select tool in QGIS 3.4.7 results in the wrong features being selected. I have tried configuring the layer with a primary key but it still seems to be using ROWID. We have also run the SQL in FME and checked the result and it comes back as a rectangle of data so we have concluded that QGIS must be incorrectly applying the results. I will post a image demonstrating the issue in a separate bug report but we are talking about large Oracle datasets.