Corrections of QgsOgrProvider implementaion of GDAL 2.0

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
What is the status of the proper QgsOgrProvider implementaion of GDAL 2.0, as described at this offered solution:


And was first reported on the 31.03.2015:

One way or another this matter needs to be resolved before an initial release of QGIS 3.

The present master version does not read tables that have more than one geometry correctly.

Mark Johnson, Berlin Germany



_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Even Rouault-2

On lundi 20 mars 2017 11:19:57 CET Mark Johnson wrote:

> https://github.com/qgis/QGIS/pull/3697

 

Mark,

 

Most of my comments in

https://github.com/qgis/QGIS/pull/3697#issuecomment-257366410 and

https://github.com/qgis/QGIS/pull/3697#issuecomment-257571410 still hold true and I'd really like see them taken into account.

 

What has changed since them is that we only support GDAL >= 2.0 in master now, so that can simplify things.

 

Are you willing to rework your PR ? Actually I think it should be completely revisited since looking at the diff of the current PR, I see no use of OGR_F_GetGeomFieldRef() so it seems that you select among the various different geometry fields by relying on the "layer_name(geometry_field_name)" syntax that is implemented in the SQLite driver, but just as a backward compatible way. But this will not work with other drivers.

 

But perhaps I didn't understand the PR. There are so many things in it that it is too hard to review. I think you should target at something that minimizes the number of changes (for example switching from the OGR_DS_ to GDALDataset API doesn't directly seem related to supporting layers with multiple geometries, so that should be left aside).

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
>> I see no use of OGR_F_GetGeomFieldRef() so it seems
that you select among the various different geometry fields by relying on the
"layer_name(geometry_field_name)" syntax

That is caused by the fact that the initial list (QgsOgrProvider::subLayers()) returns only the "layer_name(geometry_field_name)" and id
- whereby, since 30.06.2016, "layer_name(geometry_field_name)" is no longer sent to QgsOgrProvider

>> OGR_DS_ to GDALDataset API doesn't directly seem related to
supporting layers with multiple geometries
only the depreciated functions were changed to the 2.* version.

>> we only support GDAL >= 2.0 in master
and it is clear that if QGIS is called with a gdal 1.* version the Application will crash with a Lookup error?

Mark


_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Even Rouault-2

On lundi 20 mars 2017 20:39:22 CET Mark Johnson wrote:

> >> I see no use of OGR_F_GetGeomFieldRef() so it seems

>

> that you select among the various different geometry fields by relying on

> the

> "layer_name(geometry_field_name)" syntax

>

> That is caused by the fact that the initial list

> (QgsOgrProvider::subLayers()) returns only the

> "layer_name(geometry_field_name)" and id

> - whereby, since 30.06.2016, "layer_name(geometry_field_name)" is no longer

> sent to QgsOgrProvider

>

 

In the provider we could decide to allocate layer ids diffently than OGR .

 

Example if you have a dataset with

- layer1 (geom1, geom2)

- layer2 (geom)

 

Then you would map that to :

- qgis_ogr_provider_layer_id = 1: layer1, geom1

- qgis_ogr_provider_layer_id = 2: layer1, geom2

- qgis_ogr_provider_layer_id = 3: layer2, geom

 

 

> >> we only support GDAL >= 2.0 in master

>

> and it is clear that if QGIS is called with a gdal 1.* version the

> Application will crash with a Lookup error?

 

Yes. And that's perfectly fine. That's only an issue that can happen for us developers playing with different versions, and we know how to solve those issues. Real world users that receive software through binary distributions will never have to deal with such version mismatch, so no need to clutter the code for those situations.

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
In the provider we could decide to allocate layer ids diffently than OGR .

Example if you have a dataset with
- layer1 (geom1, geom2)
- layer2 (geom)

Then you would map that to :
- qgis_ogr_provider_layer_id = 1: layer1, geom1
- qgis_ogr_provider_layer_id = 2: layer1, geom2
- qgis_ogr_provider_layer_id = 3: layer2, geom

Which gdal function would then called for this construction
- the function that needs an id cannot use qgis_ogr_provider_layer_id

Some value must also be stored in the project, that must be valid days or weeks later.

Is there a function that can use the original id and OGR_F_GetGeomFieldRef()?

Mark


_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
Application will crash with a Lookup error? 
Yes. And that's perfectly fine. That's only an issue that can happen for us developers playing
with different versions, and we know how to solve those issues.

 The proposed solution for this constellation for-sees
- at application start: read and store the version numbers
- QgsOgrProvider returns null when version < 2
-- avoiding a crash with a Lookup error [in Linux at least occurs at the first use of an unknown function. Windows??]

Mark

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Nyall Dawson
In reply to this post by mj10777
On 21 March 2017 at 05:39, Mark Johnson <[hidden email]> wrote:
>>> we only support GDAL >= 2.0 in master
> and it is clear that if QGIS is called with a gdal 1.* version the
> Application will crash with a Lookup error?

Hi Mark!

Please keep in mind that's there's some non-negotiables here:

1. No extra code will be allowed in master to handle GDAL < 2.0. QGIS
3.0 has a hard GDAL >= 2.0 requirement, and no code will be accepted
to work around this requirement.

2. 2.18 is a stable branch and only bug fixes which do not affect the
stability of the release will be accepted. As it stands PR #3697
(opened against 2.18) is far too intrusive for inclusion in the stable
release branch.

As Even stated, you'll need to open a series of smaller,
self-contained PRs against the master branch only (keeping in mind the
requirement that all master code is GDAL >= 2.0 only). When these are
accepted it's possible that certain PRs from this series will be
applicable for backporting to 2.x, but that will need to be determined
only after the PRs have been fully reviewed and accepted in to master.

Nyall
_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Even Rouault-2
In reply to this post by mj10777

On lundi 20 mars 2017 22:39:20 CET Mark Johnson wrote:

> > In the provider we could decide to allocate layer ids diffently than OGR .

> >

> >> Example if you have a dataset with

> >

> > - layer1 (geom1, geom2)

> >

> > - layer2 (geom)

> >

> >> Then you would map that to :

> > - qgis_ogr_provider_layer_id = 1: layer1, geom1

> >

> > - qgis_ogr_provider_layer_id = 2: layer1, geom2

> >

> > - qgis_ogr_provider_layer_id = 3: layer2, geom

>

> Which gdal function would then called for this construction

> - the function that needs an id cannot use qgis_ogr_provider_layer_id

>

> Some value must also be stored in the project, that must be valid days or

> weeks later.

 

Good point.

 

There are different issues you've raised and that we might want handle or not:

1) make sure that a URI points to the same layer if the datasource evolves (new layer)

2) be able to deal with the corner cases of layers with same name

 

1) involves that we cannot always rely on the index of the layer in the layer list returned by OGR

2) involves that we cannot always rely on the layer name to identify uniquely a layer (can happen in some drivers like KML)

 

Fun situation...

 

So if we want to address both we need the layer_id and layer_name. I'd say when you build a OGR URI (mostly in QgisApp::askUserForOGRSublayers()), then look at the uniqueness of the layer names. If there's layer name unicity, then use only the layer name. Otherwise fallback to uniquely the layer_id (and well in that case if the datasource changes at some point, we might have an issue indeed)

 

On opening in the povider, if you have both the layer_name and layer_id, then prefer the layer_name as currently :

https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsogrprovider.cpp#L186

 

On top of that, we need to add an extra argument to store the geometry column name in the URI and in the output of subLayers().

With OGR_FD_GetGeomFieldIndex(feat_defn, geom_field_name) you get the index that OGR_F_GetGeomFieldRef(feature, geom_field_index) wil use.

 

All this would impact mainly AnalyzeURI in qgsogrprovider.cpp, QgsOgrProvider::subLayers() and QgisApp::askUserForOGRSublayers()

 

Even

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
Please keep in mind that's there's some non-negotiables here:

One way or another this matter needs to be resolved before an initial release of QGIS 3.

This topic is only about QGIS 3.0 only. 

Mark

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Nyall Dawson
On 21 March 2017 at 08:13, Mark Johnson <[hidden email]> wrote:
>> Please keep in mind that's there's some non-negotiables here:
>
>
>> One way or another this matter needs to be resolved before an initial
>> release of QGIS 3.
>
>
> This topic is only about QGIS 3.0 only.

Isn't this about PR #3697 - which is opened against 2.18? If #3697 is
abandoned, can I close to clean up the PR queue?

Nyall
_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777


There are different issues you've raised and that we might want handle or not:

1) make sure that a URI points to the same layer if the datasource evolves (new layer)

2) be able to deal with the corner cases of layers with same name

 

1) involves that we cannot always rely on the index of the layer in the layer list returned by OGR

2) involves that we cannot always rely on the layer name to identify uniquely a layer (can happen in some drivers like KML)


The proposed solution deals with problem in QgsOgrProvider::subLayers()
- searches for duplicate names
-- an extra parameter tells QgsOgrProvider if the given id or layername should be used
- default layername, otherwise id

All of this is stored in the URI for the project.

This was tested with the KML where the problem was reported
- it was then that the layername was removed (i.e. is no longer sent to QgsOgrProvider by subLayers()) last June

On top of that, we need to add an extra argument to store the geometry column name in the URI and in the output of subLayers().
The layername is also needed for the display in QGIS 

Mark

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
Isn't this about PR #3697 - which is opened against 2.18? If #3697 is
abandoned, can I close to clean up the PR queue?

Since the problem is not resolved, no.

The present QGIS 3.0 cannot table with more than geometry.
- at that time there was no QGIS 3
I changed that to 3.0 this morning.

Mark 

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Even Rouault-2
In reply to this post by mj10777

 

> The proposed solution deals with problem in QgsOgrProvider::subLayers()

> - searches for duplicate names

> -- an extra parameter tells QgsOgrProvider if the given id or layername

> should be used

 

Perhaps we can avoid that extra parameter, and have the layer name prioritary over the layer id when both are found ?

 

> - default layername, otherwise id

>

> All of this is stored in the URI for the project.

>

> This was tested with the KML where the problem was reported

> - it was then that the layername was removed (i.e. is no longer sent to

> QgsOgrProvider by subLayers()) last June

 

Not sure to what you are refering too, since I can see the layerName returned here:

https://github.com/qgis/QGIS/blob/master/src/providers/ogr/qgsogrprovider.cpp#L700

 

>

> On top of that, we need to add an extra argument to store the geometry

>

> > column name in the URI and in the output of subLayers().

>

> The layername is also needed for the display in QGIS

 

The layer name can be stored in subLayers() for conveniency, since it has usually a short life. And for subLayers() / askUserForOGRSublayers() make the layer id what is used.

 

 

--

Spatialys - Geospatial professional services

http://www.spatialys.com


_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777

1. No extra code will be allowed in master to handle GDAL < 2.0. QGIS

3.0 has a hard GDAL >= 2.0 requirement, and no code will be accepted
to work around this requirement.

The original code did many tasks being coded multiple times 
- these common tasks are written once in a extra function that is used when needed.

The gdal portions where depreciated gdal 1* functions where used
- are also in extra functions with a switch statement for the different versions

This was done in preparation for when gdal 1* should expire (i. E. QGIS 3).

All that needs to be done is to remove the gdal 1 portion of the switch where the gdal 1 code is used.

Mark

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Nyall Dawson
On 21 March 2017 at 08:45, Mark Johnson <[hidden email]> wrote:

>>
>> 1. No extra code will be allowed in master to handle GDAL < 2.0. QGIS
>> 3.0 has a hard GDAL >= 2.0 requirement, and no code will be accepted
>> to work around this requirement.
>
>
> The original code did many tasks being coded multiple times
> - these common tasks are written once in a extra function that is used when
> needed.
>
> The gdal portions where depreciated gdal 1* functions where used
> - are also in extra functions with a switch statement for the different
> versions
>
> This was done in preparation for when gdal 1* should expire (i. E. QGIS 3).
>
> All that needs to be done is to remove the gdal 1 portion of the switch
> where the gdal 1 code is used.

Great - looking forward to seeing you push this to the PR! It'll make
the remaining changes much easier to review.

Nyall
_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
The layer name can be stored in subLayers() for conveniency, since it has usually a short life. And for subLayers() / askUserForOGRSublayers() make the layer id what is used.
subLayers is not used when read from a project, only when a new source is added. 

Not sure to what you are refering too, since I can see the layerName returned here:
Between  subLayers() and QgsOgrProvider the data runs through other functions in other classes
- in one the layername is removed

void QgisApp::askUserForOGRSublayers( QgsVectorLayer *layer )
    Q_FOREACH ( const QgsSublayersDialog::LayerDefinition& def, chooseSublayersDialog.selection() )
    {
      QString layerGeometryType = def.type;
 -    QString composedURI = uri + "|layerid=" + QString::number( def.layerId );
 -
 +    QString composedURI = uri + "|layerid=" + QString::number( def.layerId ) + "|layername=" + def.layerName;

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
Great - looking forward to seeing you push this to the PR! It'll make
the remaining changes much easier to review.

Thats the way it was done when submitted..

Mark 

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Nyall Dawson
On 21 March 2017 at 09:03, Mark Johnson <[hidden email]> wrote:
>> Great - looking forward to seeing you push this to the PR! It'll make
>> the remaining changes much easier to review.
>
>
> Thats the way it was done when submitted..

Yet I still see lots of code in that PR like:

if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,0,0)

That's what I'm referring to. All this conditional code needs removal
(unless it's testing for something in GDAL > 2.0 ).

Nyall
_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

mj10777
In reply to this post by mj10777
Yet I still see lots of code in that PR like:

if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,0,0)

That's what I'm referring to. All this conditional code needs removal
(unless it's testing for something in GDAL > 2.0 ).

Mosty in the static functions
- QgsOgrProviderUtils, mostly with a switch

and a few elsewhere.

There are also few for much older versions
#if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >= 1400

They are easy to remove.

There is one (at least) that must remain

#if defined(GDAL_COMPUTE_VERSION) && GDAL_VERSION_NUM >= GDAL_COMPUTE_VERSION(2,1,2)

I count 20, beforehand there were many more.

There also other classes sped around the project that uses OGR
- some of which also have these

Mark

_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Reply | Threaded
Open this post in threaded view
|

Re: Corrections of QgsOgrProvider implementaion of GDAL 2.0

Nyall Dawson
On 21 March 2017 at 09:23, Mark Johnson <[hidden email]> wrote:

>> Yet I still see lots of code in that PR like:
>>
>> if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >=
>> GDAL_COMPUTE_VERSION(2,0,0)
>>
>> That's what I'm referring to. All this conditional code needs removal
>> (unless it's testing for something in GDAL > 2.0 ).
>
>
> Mosty in the static functions
> - QgsOgrProviderUtils, mostly with a switch
>
> and a few elsewhere.
>
> There are also few for much older versions
> #if defined(GDAL_VERSION_NUM) && GDAL_VERSION_NUM >= 1400
>
> They are easy to remove.
>
> There is one (at least) that must remain
>
> #if defined(GDAL_COMPUTE_VERSION) && GDAL_VERSION_NUM >=
> GDAL_COMPUTE_VERSION(2,1,2)
>
> I count 20, beforehand there were many more.
>
> There also other classes sped around the project that uses OGR
> - some of which also have these

Ok - I find this thread very confusing. Does this acknowledgement mean
that you WILL remove these ifdefs from the PR?

Here's the thing: I *want* your work to land in QGIS. It's important
stuff, and needs to be addressed. But in its current form it won't be
merged - it's been independently reviewed by two core developers who
have both requested changes. I hate to see all this effort you've put
in wasted, but you need to rework your approach before this can be
merged.

Nyall
_______________________________________________
Qgis-developer mailing list
[hidden email]
List info: https://lists.osgeo.org/mailman/listinfo/qgis-developer
Unsubscribe: https://lists.osgeo.org/mailman/listinfo/qgis-developer
12