<div dir="ltr">This is a summery list of the intended changes to be made to the QgsOgrProvider class which is based on the Proposal 9 of QGIS Grant Applications - August 2016.<div><br><div><a href="https://qgisblog.files.wordpress.com/2016/09/qgisgrantapplicationssummary-september2016.pdf">https://qgisblog.files.wordpress.com/2016/09/qgisgrantapplicationssummary-september2016.pdf</a><br></div><div><br></div></div><div>Despite the fact that this Proposal did not receive the needed votes to be accepted, the need still exist to correct, what I believe to be, critical flaws in the present implementation.</div><div><br></div><div>For this reason I have continued working on this and have prepared a workable solution of the known problems which can be viewed here:</div><div><br></div><div><a href="https://github.com/mj10777/QGIS/tree/master_2_ogrprovider">https://github.com/mj10777/QGIS/tree/master_2_ogrprovider</a><br></div><div><br></div><div>(unfortunately I created this about 2 hours before 'master_2' died, so this should be considered as a 'release-2_18' branch)</div><div><br></div><div>---</div><div>Main problems:</div><div><br></div><div>Problem 01:</div><div><br></div><div>The retrieval logic for Ogr-Layers changed between gdal 1.* and gdal 2.*.</div><div>This was reported on the 31.03.2015:</div><div><a href="https://hub.qgis.org/issues/12479">https://hub.qgis.org/issues/12479</a><br></div><div><br></div><div>Unfortunately the suggested changes were not implemented, so that at present, when using gdal 2.*, will not return the same list as gdal 1.*</div><div>- this geometries contained in a table that has more than 1 geometry will not turn up (mainly spatialite)</div><div><br></div><div>The table containing these geometries will show up, but cannot be loaded.</div><div><br></div><div>To make matter worse, corrections (30.06.2016) made to resolve:</div><div><a href="https://hub.qgis.org/issues/15168">https://hub.qgis.org/issues/15168</a><br></div><div><br></div><div>removed the 'layername' parameter from the sent uri, making it impossible to use 'GetLayerByName', which MUST be used after the first table that contains more than 1 geometry</div><div>- GetLayer (with id) can be used up to this point, but not afterwords</div><div><br></div><div>To retrieve a Layer, where there are duplicate names in the Datasource (issue: 15168) (mainly in none database Datasources such as KML, GML)</div><div>- only GetLayer (with id) can be used to retrieve the layer correctly</div><div><br></div><div>So the need exists to determine how the Layer should be opened while reading the whole Datasource in QgsOgrProvider::subLayers()</div><div>- if it a duplicate 'layername' : GetLayer (with id)</div><div>- otherwise GetLayerByName</div><div><br></div><div>--</div><div>Problem 02:</div><div><br></div><div>Since 2015, new code has been added implementing gdal 2.* specific functions, which will only be included when compiled/built against gdal 2.*.</div><div><br></div><div>Thus, when QGIS is complied against gdal 2.*, symbols/functions are added</div><div>- that doe not exist in gdal 1.*.</div><div><br></div><div>Nothing was undertaken to check which gdal version is being used when QGIS is running, causing an 'symbol lookup error' for:</div><div>- libqgis_core.so: OGR_F_GetFieldAsInteger64, OGR_F_SetFieldInteger64 and OGR_G_ExportToIsoWkb</div><div>- libogrprovider.so: OGR_GT_Flatten, OGR_GT_HasZ and OGR_GT_HasM</div><div><br></div><div>causing QGIS to crash.</div><div><br></div><div><div>--</div><div>Problem 03:</div></div><div><br></div><div>The maintaining of the code in QgsOgrProvider has become difficult.</div><div><br></div><div>The need exists to consolidate/isolate crucial functions (such a Open, Sublayer) in a way that</div><div>- a backup solution is offered to reliably compleate the task</div><div>- build/runtime version specific code (including deprecated functions)</div><div><br></div><div>are done in 'Wrapper' functions that deal with these problems once and then correctly.</div><div><br></div><div>Sample: the logic to determine the Geometry type is done in multiple places</div><div>- getOgrGeomType and in SubLayers (where with gdal 2.* an incorrect result will be returned and 3 '#if defined(GDAL_COMPUTE_VERSION)')</div><div><br></div><div>Such tasks should be consolidated in extra functions so that any maintainer of QgsOgrProvider would simply use them without having to deal version specific conditions.</div><div><br></div><div>These tasks should be added to the already existing class 'QgsOgrProviderUtils'.</div><div><br></div><div><div>--</div><div>Major changes:</div></div><div><br></div><div>QgsOgrProvider::open<br></div><div>- now uses QgsOgrProvider::OGRGetLayerWrapper to open the dataset, replacing the logic of</div><div>-- either using GetLayerByName OR GetLayer (with id)</div><div><br></div><div>This is done 3 times, but also done in the same way in QgsOgrProvider::repack() and QgsOgrFeatureIterator::QgsOgrFeatureIterator</div><div><br></div><div>Based on the sent Url, OGRGetLayerWrapper will attempt to open the Datasource as declared in the Url.</div><div>- during subLayers() checking was done if GetLayerByName OR GetLayer (with id) is needed</div><div>-- for this the ogr-specific sublayer string will send an extra parameter 'ogrgettype=0|1' with this information</div><div><br></div><div>If the given method fails, the other method will be attempted</div><div>- if that fails the DataSource will be read calling subLayers()</div><div><br></div><div>The sublayer string, that successfully opened the Layer will be stored in the class member mSubLayerString.</div><div>- when QgsOgrFeatureIterator later runs it will retrieve this value from its copy of QgsOgrProvider (mSource)</div><div>--> and load the layer without further checking</div><div> ogrLayer = QgsOgrProviderUtils::OGRGetSubLayerStringWrapper( mConn->ds, mSource-> mSubLayerString )</div><div><br></div><div>The ''ogrgettype=0|1" will be a part of the Uri that will be stored in a QGIS project</div><div>- so that when loading a project, no further checking is needed</div><div><br></div><div>Since the parameters in the Uri are 'optional', older projects not containing 'ogrgettype=0|1" will not fail</div><div>- when changes to the properties of a Project-Layer are made, the 'ogrgettype=0|1" will be appended to the Uri string</div><div><br></div><div>QgsSublayersDialog will not show the ogrgettype received from subLayers() in the dialog.<br></div><div>- typedef struct LayerDefinition has been adapted to store the ogrgettype value in ' int getType'</div><div><br></div><div>QgisApp::askUserForOGRSublayers<br></div><div>- has been adapted to build the uri with ogrgettype (as well as geometrytype and featurescount)</div><div>- layername has also been (re-)added [was removed last June]</div><div><br></div><div><br></div><div>--<br></div><div><div>Status:</div></div><div><br></div><div>All of the above have been implemented.</div><div><br></div><div>There are areas where a policy decisions should be made before a pull request should be made.</div><div><br></div><div>A python test script has been created (tests/src/python/test_provider_ogr_general.py) to test the different conditions</div><div>- together with test data - some of which are the original gdal/ogr autotest databases</div><div><br></div><div>Altogether 6 tests</div><div>1) Spatialite SpatialView - write support</div><div>- a special gdal 2.2.0 version that supports writable SpatialViews runs correctly</div><div>-- checks CREATE, UPDATE and DELETE TRIGGERs exist and run correctly </div><div>- other gdal versions report that  writable SpatialViews are not supported</div><div>2) Spatialite SpatialTable with more than 1 geometry</div><div>- created with gdal autotest/auto/ogr_sql_sqlite.py spatialite_8()</div><div>--> tests that the results returned  from gdal 1.* and 2.* are the same</div><div>3) Spatialite SpatialTables with XYZM values</div><div>- created with gdal autotest/auto/ogr_sql_sqlite.py spatialite_5()<br></div><div>--> checks that expected values fro different Z/M/ZM values are correctly retrieved</div><div>---> reports that gdal 1.* does not support M/ZM tables</div><div>4) GML 2 MultiPolygon with InternalRings</div><div>- created from a historically correct complicated geometry from spatialite Database<br></div><div>-- loops through Polygons, Exterior/InteriorRing calculating area</div><div>--> comparing the values with the original spatialite result</div><div>--> that the area sum of the Polygons is the same as that as the MultiPolygon</div><div>--> that the area of the Polygon + the area of the InteriorRing are the same as the ExteriorRing</div><div>5) Sample kmz file showing layers with unicode Chinese characters, some of which a duplicate and some with a geometry field</div><div>- included in bug report 15168</div><div>-- testing Layers without a geometry definition will not be listed</div><div>-- testing that the retrieved layers with duplicate names</div><div>--> comparing the amount of points and that the first and last points are different.</div><div>6) sqlite Integer64</div><div>- created [as 'sqlite_test.db'] and filled with gdal autotest/ogr/ogr_sql_sqlite.py ogr_sqlite_1, 2, 11, 12, 13, 14, 15 and 16<br></div><div>-- checking/updating values in Table 'tpoly' which contains 2 fields with BIGINT (Integer64) </div><div><br></div><div>Windows:</div><div>No test have been done with Windows to insure that no 'symbol lookup error' occur.</div><div><br></div><div>Tested combinations (linux only):</div><div>- original 'release-2_18' (2016-10-25) built with gdal 2.2.0</div><div>-- running with gdal 2.2.0: brings expected problems with test for tables that has more than 1 geometry</div><div>-- running with gdal 1.11.2: receives (as expected) 'symbol lookup error'<br></div><div><br></div><div>- branch 'master_2_ogrprovider'  built with gdal 2.2.0<br></div><div>-- running with gdal 2.2.0: all test run correctly<br></div><div>-- running with gdal 1.11.2: all test run correctly, reporting that M and Integer64 values are not supported - without 'symbol lookup error' </div><div><br></div><div><div>- branch 'master_2_ogrprovider'  built with gdal 1.11.2<br></div><div>-- running with gdal 2.2.0: all test run correctly, but </div><div>--> Integer64 values are returned as a string<br></div><div>--> Z/N/ZM Geometries Types are shown and correct values retrieved<br></div><div>-- running with gdal 1.11.2: all test run correctly, reporting that M and Integer64 values are not supported </div></div><div><br></div><div>Documentation:</div><div>The new and changed functions in 'QgsOgrProviderUtils' and 'QgsOgrProvider' have been documented</div><div>- explaining 'the reason why' thing are being done in a certain way</div><div><br></div><div><div>--</div><div>Policy decisions:</div></div><div><br></div><div>With the exception of  'OGR_G_ExportToIsoWkb' (that contains M value support)</div><div>- all other functions that cause a 'symbol lookup error' can be emulated</div><div><br></div><div>When using gdal 1.*, the result of the 'Integer64' fields are incorrect</div><div>- all values > MAX_INT32 will overflow</div><div>So when built with gdal 2.* and running with gdal 1.*</div><div>- the reading and writing of 'Integer64' fields should not be permitted to avoid the falsification of data</div><div><br></div><div>The same is true for QgsGmlStreamingParser::endElement, where 'OGR_G_ExportToIsoWkb' is used.</div><div><br></div><div>For QGIS 2.*: the support for gdal 1.* and 2.* should continue as is.</div><div><br></div><div>For QGIS 3.0: gdal 1.* support should be removed.<br></div><div><br></div><div>Reasons:</div><div>As gdal evolves, it will become more difficult to 'emulate' OGR functions that do not exist in gdal 1.*</div><div>- the inclusion of which would cause a 'symbol lookup error' </div><div>A proper support for M-Values will probably become more important in the future than it is now.</div><div><br></div><div>---</div><div><br></div><div>(sorry if this is a bit long winded, but one has to start somewhere)</div><div><br></div><div>Mark Johnson, Berlin Germany</div></div>