[Qgis-developer] Correction of QgsOgrProvider implementaion of GDAL 2.0

[Mark Johnson]

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.

https://qgisblog.files.wordpress.com/2016/09/qgisgrantapplicationssummary-september2016.pdf

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.

For this reason I have continued working on this and have prepared a
workable solution of the known problems which can be viewed here:

https://github.com/mj10777/QGIS/tree/master_2_ogrprovider

(unfortunately I created this about 2 hours before 'master_2' died, so this
should be considered as a 'release-2_18' branch)

---
Main problems:

Problem 01:

The retrieval logic for Ogr-Layers changed between gdal 1.* and gdal 2.*.
This was reported on the 31.03.2015:
https://hub.qgis.org/issues/12479

Unfortunately the suggested changes were not implemented, so that at
present, when using gdal 2.*, will not return the same list as gdal 1.*
- this geometries contained in a table that has more than 1 geometry will
not turn up (mainly spatialite)

The table containing these geometries will show up, but cannot be loaded.

To make matter worse, corrections (30.06.2016) made to resolve:
https://hub.qgis.org/issues/15168

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
- GetLayer (with id) can be used up to this point, but not afterwords

To retrieve a Layer, where there are duplicate names in the Datasource
(issue: 15168) (mainly in none database Datasources such as KML, GML)
- only GetLayer (with id) can be used to retrieve the layer correctly

So the need exists to determine how the Layer should be opened while
reading the whole Datasource in QgsOgrProvider::subLayers()
- if it a duplicate 'layername' : GetLayer (with id)
- otherwise GetLayerByName

--
Problem 02:

Since 2015, new code has been added implementing gdal 2.* specific
functions, which will only be included when compiled/built against gdal 2.*.

Thus, when QGIS is complied against gdal 2.*, symbols/functions are added
- that doe not exist in gdal 1.*.

Nothing was undertaken to check which gdal version is being used when QGIS
is running, causing an 'symbol lookup error' for:
- libqgis_core.so: OGR_F_GetFieldAsInteger64, OGR_F_SetFieldInteger64
and OGR_G_ExportToIsoWkb
- libogrprovider.so: OGR_GT_Flatten, OGR_GT_HasZ and OGR_GT_HasM

causing QGIS to crash.

--
Problem 03:

The maintaining of the code in QgsOgrProvider has become difficult.

The need exists to consolidate/isolate crucial functions (such a Open,
Sublayer) in a way that
- a backup solution is offered to reliably compleate the task
- build/runtime version specific code (including deprecated functions)

are done in 'Wrapper' functions that deal with these problems once and then
correctly.

Sample: the logic to determine the Geometry type is done in multiple places
- getOgrGeomType and in SubLayers (where with gdal 2.* an incorrect result
will be returned and 3 '#if defined(GDAL_COMPUTE_VERSION)')

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.

These tasks should be added to the already existing class
'QgsOgrProviderUtils'.

--
Major changes:

QgsOgrProvider::open
- now uses QgsOgrProvider::OGRGetLayerWrapper to open the dataset,
replacing the logic of
-- either using GetLayerByName OR GetLayer (with id)

This is done 3 times, but also done in the same way
in QgsOgrProvider::repack() and QgsOgrFeatureIterator::QgsOgrFeatureIterator

Based on the sent Url, OGRGetLayerWrapper will attempt to open the
Datasource as declared in the Url.
- during subLayers() checking was done if GetLayerByName OR GetLayer (with
id) is needed
-- for this the ogr-specific sublayer string will send an extra parameter
'ogrgettype=0|1' with this information

If the given method fails, the other method will be attempted
- if that fails the DataSource will be read calling subLayers()

The sublayer string, that successfully opened the Layer will be stored in
the class member mSubLayerString.
- when QgsOgrFeatureIterator later runs it will retrieve this value from
its copy of QgsOgrProvider (mSource)
--> and load the layer without further checking
 ogrLayer = QgsOgrProviderUtils::OGRGetSubLayerStringWrapper( mConn->ds,
mSource-> mSubLayerString )

The ''ogrgettype=0|1" will be a part of the Uri that will be stored in a
QGIS project
- so that when loading a project, no further checking is needed

Since the parameters in the Uri are 'optional', older projects not
containing 'ogrgettype=0|1" will not fail
- when changes to the properties of a Project-Layer are made, the
'ogrgettype=0|1" will be appended to the Uri string

QgsSublayersDialog will not show the ogrgettype received
from subLayers() in the dialog.
- typedef struct LayerDefinition has been adapted to store the ogrgettype
value in ' int getType'

QgisApp::askUserForOGRSublayers
- has been adapted to build the uri with ogrgettype (as well as
geometrytype and featurescount)
- layername has also been (re-)added [was removed last June]


--
Status:

All of the above have been implemented.

There are areas where a policy decisions should be made before a pull
request should be made.

A python test script has been created
(tests/src/python/test_provider_ogr_general.py) to test the different
conditions
- together with test data - some of which are the original
gdal/ogr autotest databases

Altogether 6 tests
1) Spatialite SpatialView - write support
- a special gdal 2.2.0 version that supports writable SpatialViews runs
correctly
-- checks CREATE, UPDATE and DELETE TRIGGERs exist and run correctly
- other gdal versions report that  writable SpatialViews are not supported
2) Spatialite SpatialTable with more than 1 geometry
- created with gdal autotest/auto/ogr_sql_sqlite.py spatialite_8()
--> tests that the results returned  from gdal 1.* and 2.* are the same
3) Spatialite SpatialTables with XYZM values
- created with gdal autotest/auto/ogr_sql_sqlite.py spatialite_5()
--> checks that expected values fro different Z/M/ZM values are correctly
retrieved
---> reports that gdal 1.* does not support M/ZM tables
4) GML 2 MultiPolygon with InternalRings
- created from a historically correct complicated geometry from spatialite
Database
-- loops through Polygons, Exterior/InteriorRing calculating area
--> comparing the values with the original spatialite result
--> that the area sum of the Polygons is the same as that as the
MultiPolygon
--> that the area of the Polygon + the area of the InteriorRing are the
same as the ExteriorRing
5) Sample kmz file showing layers with unicode Chinese characters, some of
which a duplicate and some with a geometry field
- included in bug report 15168
-- testing Layers without a geometry definition will not be listed
-- testing that the retrieved layers with duplicate names
--> comparing the amount of points and that the first and last points are
different.
6) sqlite Integer64
- 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
-- checking/updating values in Table 'tpoly' which contains 2 fields with
BIGINT (Integer64)

Windows:
No test have been done with Windows to insure that no 'symbol lookup error'
occur.

Tested combinations (linux only):
- original 'release-2_18' (2016-10-25) built with gdal 2.2.0
-- running with gdal 2.2.0: brings expected problems with test for tables
that has more than 1 geometry
-- running with gdal 1.11.2: receives (as expected) 'symbol lookup error'

- branch 'master_2_ogrprovider'  built with gdal 2.2.0
-- running with gdal 2.2.0: all test run correctly
-- running with gdal 1.11.2: all test run correctly, reporting that M and
Integer64 values are not supported - without 'symbol lookup error'

- branch 'master_2_ogrprovider'  built with gdal 1.11.2
-- running with gdal 2.2.0: all test run correctly, but
--> Integer64 values are returned as a string
--> Z/N/ZM Geometries Types are shown and correct values retrieved
-- running with gdal 1.11.2: all test run correctly, reporting that M and
Integer64 values are not supported

Documentation:
The new and changed functions in 'QgsOgrProviderUtils' and 'QgsOgrProvider'
have been documented
- explaining 'the reason why' thing are being done in a certain way

--
Policy decisions:

With the exception of  'OGR_G_ExportToIsoWkb' (that contains M value
support)
- all other functions that cause a 'symbol lookup error' can be emulated

When using gdal 1.*, the result of the 'Integer64' fields are incorrect
- all values > MAX_INT32 will overflow
So when built with gdal 2.* and running with gdal 1.*
- the reading and writing of 'Integer64' fields should not be permitted to
avoid the falsification of data

The same is true for QgsGmlStreamingParser::endElement, where
'OGR_G_ExportToIsoWkb' is used.

For QGIS 2.*: the support for gdal 1.* and 2.* should continue as is.

For QGIS 3.0: gdal 1.* support should be removed.

Reasons:
As gdal evolves, it will become more difficult to 'emulate' OGR functions
that do not exist in gdal 1.*
- the inclusion of which would cause a 'symbol lookup error'
A proper support for M-Values will probably become more important in the
future than it is now.

---

(sorry if this is a bit long winded, but one has to start somewhere)

Mark Johnson, Berlin Germany
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/qgis-developer/attachments/20161028/4e2c8837/attachment-0001.html>