Skip re-querying geometry type for Oracle provider #34738

tomass · 2020-02-27T18:14:46Z

Skip geometry type re-querying for Oracle as per #34737

gioman · 2020-02-27T19:09:33Z

This probably also fixes #34328

troopa81 · 2020-03-11T07:59:16Z

src/providers/oracle/qgsoracleprovider.cpp

@@ -2614,6 +2614,15 @@ bool QgsOracleProvider::getGeometryDetails()
    return true;
  }

+  // Do not re-check geometry type if it is already known (f.e. when re-loading a project)
+  if ( mRequestedGeomType != QgsWkbTypes::Unknown && mSrid > 0 )


You can store different type of geometry in an Oracle table, so we need to filter only the geometries we request if it differs from the one detected (see). For your information, another PR related to geometry type.

I think your modification will break this because you overwrite mDetectedGeomType.

IMPORTANT: I'm not an expert/developer.
My impression is this code is executed once when we create or re-open an oracle layer (when opening a project). Not for a specific feature, but for the whole layer.
mDetectedGeomType is the type of geometry we've already chosen/found when creating a layer. So styling, labelling etc. is already set-up according to this geometry type.
When re-opening the layer again, this routine re-checks the type of geometry (and in oracle, which does not hold geometry type in meta-tables, this is an expensive action). And it is hard to say what would happen if geometry type would change say from polygon to point... all symbolisation/labelling settings will be partly incorrect/irrelevant.
If I'm not mistaken, I'm overwriting mDetectedGeomType with type which was found in previous sessions. So if a user has added a layer with type polygon, I would use polygon, if a user added a layer with line, I would use line etc.

Can you think of some use case/test I could try to test your version?

P.S. Regarding #34358 we actually are very happy that QGIS ignores the difference between polygon and multipolygon as these are all polygons in general (same symbolisation etc.).

There is one provider for each layer, created when you create the layer (open the project for instance).

mDetectedGeomType is the geometry detected from the database, mRequestedGeomType is the one specified by the user (specified in uri you could see on tooltip).

Can you think of some use case/test I could try to test your version?

You can have a table with mixed polygons and multipolygons and want to display only polygons. That's the way other providers work in QGIS, and that's the desired fix of the PR #34358.

P.S. Regarding #34358 we actually are very happy that QGIS ignores the difference between polygon and multipolygon as these are all polygons in general (same symbolisation etc.).

I understand your issue here. @SebastienPeillet It will indeed break the workflow of people relying on the fact that geomtype and multigeomtype are mixed now but I don't see a way to not break this with your PR. My guess is that you should have either 2 layers or convert to multi the single geometry with a view.

Can you think of some use case/test I could try to test your version?
You can have a table with mixed polygons and multipolygons and want
to display only polygons.

mRequestedGeomType would be the type I chose when creating the layer, right? And unless I edit xml manually it will be one of the types which actually exist in the table (at the time of adding a layer).
When I'm overwriting mDetectedType with mRequestedGeomType I simply say "let's ASSUME the table still has the same geometry type". That is I assume table contents has not changed in principle (then I move a project to production environment that should always be the case).
Therefore unless table contents have changed considerably, mRequestedGeomType and mDetectedType should be the same?

Another example with polygon vs multipolygon. Say I chose polygon, so requestedtype is polygon. Now if I have estimated data on and the first fetched geometry from mixed type table is multipolygon - detectedtype would be multipolygon, should QGIS really start reshuffling configuration? That multipolygon could be one record in a million polygon table... In this case re-checking would be not only unnecessary, but even unwanted/harmful? Or am I missing something?
(You can enable row movement in Oracle and then rows could be moved/rowid changed impacting which are returned as first row even in the same db session)

P.S. The code you link to uses mRequestedType, so my proposed change should not have impact on that?

That is I assume table contents has not changed in principle

You cannot be sure, you could have added any geometry type to your table and you want to filter only the type you select at first. If detectedGeomType == requestedGeomType, filtering is not possible anymore.

the first fetched geometry from mixed type table is multipolygon - detectedtype would be multipolygon

Right! that's why I told you to try the GROUP BY. But I can't see a better solution when you use metadata

P.S. The code you link to uses mRequestedType, so my proposed change should not have impact on that?

You mean mRequestedGeomType, the impact will be the one described above, no filtering.

We could maybe skip the detection of geom type if estimated meta data is true and the mRequestedGeomType is set. And let the mDetectedGeomType to Unknow. So the filter on geometry will be enabled from featureiterator.

But we can't force mHasSpatialIndex and disable mSrid detection.

It should not break anything, hoping that I don't miss a corner case.

Does it sounds good to you?

I agree on skipping geom type testing when estimated meta data is true.

But SRID would have already been detected upon adding the layer. That is why I have a condition mSrid > 0. Why re-query?

Spatial index might be a separate issue (I'm ok with removing it from this PR and opening a new one specially for spacial index). The reason I'm setting it to true is that at least when using a view metadata shows there is no spatial index (as views do not have any indexes on them), but there actually is a spatial view on geometry table used in that view.
This index thing should probably be assumed to true when we're using a view, not a table.
Otherwise some other non trivial queries are done in loadField function which again slows down opening of project in QGIS Server (actually further/future idea is to skip loadField on project re-opening as well, as all fields are already saved in project file).

But SRID would have already been detected upon adding the layer. That is why I have a condition mSrid > 0. Why re-query?

Because it could be different from the one requested. When you add a layer to a project, you just add a configuration that point on a table, QGIS needs to check these parameters to be sure everything is fine and avoid issues later.

This is the way all providers are designed and I'm pretty sure that skipping all of this checks because of loading time won't be accepted and merged.

IMHO I think you should focus more on how to speed up the different request behind this checks instead of trying to modify QGIS. But maybe other developers won't feel the same.

SRID is fetched from metadata, so it's value will not change unless somebody actually decides to change data schema on-line. Therefore in normally designed system this should never happen.
I would say that QGIS should read all required metadata upon adding the layer, save it in the project and then simply use it (or refresh upon explicit request by the user). The only queries done on QGIS Server should be feature fetch queries. This would also increase all benchmarks of QGIS Server ;-)
But of course I'm not in a position to manage/rule here.
Therefore ok, I will only leave a code to skip geometry re-checking and will try to formulate further improvements in a separate PR (or should I first ask in developers mailing list?).
Thank you for your help!

I think that if you are willing to skip all check at layer loading (reading the project), you should ask on developers mailing list. I'm pretty sure people won't agree with this, but it's always good to have others opinion.

Thank you for your help!

You're welcome

troopa81 · 2020-03-23T07:55:34Z

src/providers/oracle/qgsoracleprovider.cpp

@@ -2614,6 +2615,16 @@ bool QgsOracleProvider::getGeometryDetails()
    return true;
  }

+  // Do not re-check geometry type if it is already known (f.e. when re-loading a project)
+  // and trust project data checkbox is enabled in project properties
+  if ( QgsProject::instance()->trustLayerMetadata() && mRequestedGeomType != QgsWkbTypes::Unknown && mSrid > 0 )


This way the parameter will apply to all layers. I think a better way will be to have a per layer parameter setting in the layer uri. Look checkPrimaryKeyUnicity in postgres provider for instance

„Trust layer“ option is on project level in QGIS GUI, so there is no way not to apply it to all layers. Or am I missing something?

You have to propagate it to the layer URI. Take a look on how it's done on checkPrimaryKeyUnicity in postgres provider for instance

OK, I will try to understand how to do that.
But can you explain the idea why does it have to be done that way?

For one - it's not appropriate to use QgsProject::instance() in providers or core library. This breaks QGIS server and other circumstances where multiple projects are loaded in QGIS.

Can you suggest any other way to get the value of "Trust layer" option value?
Or copying the "Trust layer" value to a different level - namely layer - is the only way to go?

can someone explain me, how I can add this code in mine qgis?

troopa81 · 2020-03-23T09:10:55Z

But can you explain the idea **why** does it have to be done that way?

to have a per layer parameter

tomass · 2020-03-23T09:32:03Z

to have a per layer parameter

I'm lost. There is no way to hand-pick layers where this option is on or off. Why add additional complexity which cannot be used in practice? If somebody will decide/want to move this option from project to layer level in GUI, maybe then this option will be saved in layer? Currently all layers will always be the same - all off or all on, then what is a point of per layer saving in this PR?

My thinking comes from the maxima: less code is always better.

tomass · 2020-03-29T13:20:18Z

I've checked the code in postgres provider:
The value of project level property of trust project is copied as is to multiple places in lower level - to multiple layer uri's. Then those multiple values in layer uri are used in provider code.

I do not understand how/when would this behave differently than current implementation in this PR of taking the value directly from project properties without copying it to multiple places/layers?

P.S. The purpose of this PR is not to change the position/level of trust project option.

stale · 2020-04-12T13:55:02Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

tomass · 2020-04-12T17:36:06Z

What is the status of this? Should I just wait?

troopa81 · 2020-04-14T08:17:35Z

What is the status of this? Should I just wait?

Sorry, I forgot answering to you. Like I said before you can't access this parameter directly from project, because it has to be a per layer parameter. There is layers you want to trust and other you don't want to.

tomass · 2020-04-14T08:39:11Z

There is layers you want to trust and other you don't want to.

The setting is on project level. There is no way you can specify which layer you trust or not.
Am I missing something?

troopa81 · 2020-04-14T09:42:30Z

The setting is on project level. There is no way you can specify which layer you trust or not.
Am I missing something?

For now trust parameter:

forces to read extent from the project
(In PG only) set check unicity default value to false, but can be changed later by the user

Trust parameter is never accessed from any provider, and I think it will be better to continue this way.

If I recall correctly, you want to avoid geometry type detection, so maybe we should add a detect geometry type boolean on project which value defaults to false if trust is enabled (like check unicity)

@pblottiere @elpaso Any thoughts on this

tomass · 2020-04-14T09:58:20Z

Just a note: I'm not a developer so I'm already stretching myself out here. Maybe additional/other new functionality could be moved to other PR's and done by real developers? The point of this PR was to fix performance blocker on Oracle, not to re-arrange the settings mechanism.

troopa81 · 2020-04-14T12:00:14Z

Maybe additional/other new functionality could be moved to other PR's and done by real developers?

I understand your point, the 2 seems pretty much linked IMHO.

tomass · 2020-04-14T12:26:01Z

... the 2 seems pretty much linked IMHO.

I for one would vote for NOT moving this setting to layer as by my practice this is an attitude to IT, not technical detail of a specific layer.

stale · 2020-04-28T12:54:45Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2020-07-05T07:25:44Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2020-07-12T08:25:41Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

tomass · 2020-07-12T18:47:00Z

I've added more changes (duplicating the trustLayerMetainfo setting to layer uri), but this PR was closed in the meantime. Should I create a new PR?

tomass · 2020-07-12T20:22:34Z

The code now repeats trustLayerMetadata='1' 2n times for a project with n layers (the actual setting is 7th line in a project file ).
Another thing, it only works for new layers. If I open existing project - the property is not propagated to all Oracle layers and I do not know why.

stale · 2020-08-02T20:22:43Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2020-09-05T19:37:32Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

…ct settings

tomass · 2020-09-07T11:53:43Z

Some help is needed. I've added logic to push trustMetadata value to layer uri:

when creating a new layer
when changing the check box state in project settings.

There is a problem with updating layer uri after project settings have been updated. There is a loop through all oracle layers, local uri object is updated with updated value, but setUri line in qgsproject.cpp does not update the layer uri and I'm unable to see why and therefore find a solution.

// copy the value of this project level setting to layer uri
QgsDataSourceUri uri = vl->dataProvider()->dataSourceUri();
uri.removeParam( QStringLiteral( "trustLayerMetadata" ) );
uri.setParam( QStringLiteral( "trustLayerMetadata" ), trust ? QLatin1String( "1" ) : QLatin1String( "0" ) );
vl->dataProvider()->setUri( uri ); // <--- this does not actually set/change the uri

tomass · 2020-09-27T06:45:07Z

If I see it correctly, #34358 implements this without copying the value to layer uri.

stale · 2020-10-11T11:40:16Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

stale · 2020-11-07T21:58:16Z

While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist.

Skip re-querying geometry type

3fdba0f

gioman mentioned this pull request Feb 27, 2020

Project loading became slow when it contains Oracle layers #34328

Open

astyle

7ebd576

github-actions bot added this to the 3.14.0 milestone Mar 4, 2020

troopa81 requested changes Mar 11, 2020

View reviewed changes

tomass added 3 commits March 21, 2020 15:06

Merge remote-tracking branch 'upstream/master'

221bf13

Skip re-querying only if trust project checkbox is enabled

192149f

fix

462f19f

troopa81 requested changes Mar 23, 2020

View reviewed changes

tomass added 2 commits March 25, 2020 21:14

Merge remote-tracking branch 'upstream/master'

103ce6c

Merge remote-tracking branch 'upstream/master'

caa8ab5

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Apr 12, 2020

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Apr 12, 2020

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Apr 28, 2020

Merge remote-tracking branch 'upstream/master'

279429d

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label May 1, 2020

Merge remote-tracking branch 'upstream/master'

14d6bd3

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 5, 2020

stale bot closed this Jul 12, 2020

tomass added 2 commits July 12, 2020 18:06

Excessive duplication of parameter value to layer level

1138aa9

Fix errors

0f5b2f6

gioman reopened this Jul 12, 2020

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jul 12, 2020

Merge remote-tracking branch 'upstream/master'

0a87d99

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 2, 2020

Merge remote-tracking branch 'upstream/master'

d18b1d2

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Aug 3, 2020

Merge remote-tracking branch 'upstream/master'

f8ea6d4

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 5, 2020

Remove name duplication

32c3a95

stale bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Sep 6, 2020

tomass added 4 commits September 7, 2020 12:02

Remove changes to table model and try changin uri when changing proje…

a74006d

…ct settings

This column is no longer used

438aaff

Fix identation

5b43d17

Fix identation

1744140

stale bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Oct 11, 2020

stale bot closed this Nov 7, 2020

Skip re-querying geometry type for Oracle provider #34738

Skip re-querying geometry type for Oracle provider #34738

Conversation

tomass commented Feb 27, 2020

gioman commented Feb 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troopa81 commented Mar 23, 2020 via email

tomass commented Mar 23, 2020

tomass commented Mar 29, 2020

stale bot commented Apr 12, 2020

tomass commented Apr 12, 2020

troopa81 commented Apr 14, 2020

tomass commented Apr 14, 2020

troopa81 commented Apr 14, 2020

tomass commented Apr 14, 2020

troopa81 commented Apr 14, 2020

tomass commented Apr 14, 2020

stale bot commented Apr 28, 2020

stale bot commented Jul 5, 2020

stale bot commented Jul 12, 2020

tomass commented Jul 12, 2020

tomass commented Jul 12, 2020

stale bot commented Aug 2, 2020

stale bot commented Sep 5, 2020

tomass commented Sep 7, 2020

tomass commented Sep 27, 2020

stale bot commented Oct 11, 2020

stale bot commented Nov 7, 2020