Cache unique values when creating features #9203

elpaso · 2019-02-19T09:06:16Z

Cache unique values when creating features

Fixes #21305 - pasting features is very slow

Aggressively optimize createFeature for speed
and introduces createFeatures for bulk creation.

Fixes qgis#21305 - pasting features is very slow Aggressively optimize createFeature for speed and introduces createFeatures for bulk creation.

elpaso · 2019-02-19T09:08:04Z

@nyalldawson @m-kuhn I'd appreciate your review on this one.

Speed gain is really huge and it makes pasting features usable again.

src/core/qgsvectorlayerutils.h

m-kuhn · 2019-02-19T10:54:49Z

I like the idea

The only thing I'm not sure about is if it's worth mentioning a performance factor, there's probably a linear dependency between the speed gain and the number of features :)

elpaso · 2019-02-19T10:58:04Z

@m-kuhn thanks for the review, how do you feel about backporting, is that ok?

m-kuhn · 2019-02-19T11:04:23Z

I would give it a considerable amount of testing time before backporting given that there's a risk of side effects.

gioman · 2019-02-19T11:12:03Z

I would give it a considerable amount of testing time before backporting given that there's a risk of side effects.

@m-kuhn I will do my best to test it, anyway this possibly solve a very bad regression (and hopefully also fixes the other issue about opening time of attribute tables with many columns, that is QGIS 3 has become unbearably slow).

Fixes qgis#21311

I've talked to Even for this one

src/providers/ogr/qgsogrprovider.cpp

Co-Authored-By: elpaso <elpaso@itopen.it>

src/providers/ogr/qgsogrprovider.cpp

Co-Authored-By: elpaso <elpaso@itopen.it>

elpaso · 2019-02-19T14:38:54Z

@m-kuhn sql is QByteArray

m-kuhn · 2019-02-19T14:40:08Z

oops

m-kuhn · 2019-02-19T17:44:53Z

What I wonder more is if it's appropriate to trigger a createFeature() on every paste operation.

Shouldn't the logic be

If a pasted feature misses an attribute (really misses, not if it's NULL) then get a default value for this attribute
Then check unique constraints and if some unique constraints fail, try to get a unique attribute

Or is this not the same kind of pasting we are talking about?

elpaso · 2019-02-19T18:02:10Z

What I wonder more is if it's appropriate to trigger a createFeature() on every paste operation.

Shouldn't the logic be
* If a pasted feature misses an attribute (really misses, not if it's NULL) then get a default value for this attribute

* Then check unique constraints and if some unique constraints fail, try to get a unique attribute
Or is this not the same kind of pasting we are talking about?

I'm not sure I follow you: this is exactly what happens inside the monster method createFeature/s: all constraints are checked, all defaults are evaluated and the last step tries to create a unique value if the unique constraint is still not honored.

I'm currently working on a fix for this last step: we should use the cached unique values instead of fetching from the provider (because the provider does not have all the values from the possibly previously other created features).

Btw, I'm happy to talk/chat to you about this: maybe easier to explain.

elpaso · 2019-02-19T18:06:33Z

@m-kuhn see my last commit for the unique values generation (now using the cached unique values) and the new test that failed with the previous implementation.

src/core/qgsvectorlayerutils.cpp

m-kuhn · 2019-02-20T07:20:17Z

src/core/qgsvectorlayerutils.cpp

-      v = attributes.value( idx );
-    }
+      // 3. provider side default value clause
+      // note - not an else if deliberately. Users may return null from a default value expression to fallback to provider defaults


Do we really want that? NULL to fallback to provider defaults... different discussion.

Actually the check below is for isValid() and the comment here says NULL

Yeah, different discussion (that we need to do sooner or later), I called this monster method for a reason, I don't entirely get the logic here since there are a lot of different permutations of use cases.

m-kuhn · 2019-02-20T07:23:12Z

@elpaso I see how it works now, looks reasonable. Very good!
I just left some comments which I think already applied to the code before, would be good to get @nyalldawson 's eyes there too, if I'm not mistaken he's the original author of that.

Co-Authored-By: elpaso <elpaso@itopen.it>

nirvn · 2019-02-20T07:25:04Z

Is the plan for this to be merged into 3.6.0, or is it better to aim at 3.6.1 to give it more testing time?

elpaso · 2019-02-20T07:28:12Z

@nirvn I'd merge into 3.6: there is quite a good test coverage on this area and without this patch pasting a bunch of features with a "slow" provider makes QGIS completely unresponsive for minutes.

I removed the backporting tag, but if it was my decision I would backport it.

m-kuhn · 2019-02-20T07:29:39Z

I agree, merging for 3.6.0 is a good idea, it looks "safe enough" and more testing is welcome. For backporting I think the question is not if but when.

nirvn · 2019-02-20T07:30:26Z

Ok. If you guys think it's safe enough, I'm happy with that :)

This reverts commit f4d7e73.

isValid is not enough because fields are initialized with QVariant(field.type()) which is valid but null. Fixes qgis#21304 With test

elpaso · 2019-02-20T09:14:35Z

@m-kuhn is this what you meant with checking for nulls? bf1575f

I added a test for bug https://issues.qgis.org/issues/21304

…sting features

m-kuhn · 2019-02-20T09:24:04Z

Codewise, v.isNull() implicitly also checks for !v.isValid() so the extra check there is no longer required if this change is done.

However, I was more questioning this functionality in general. I.e. I believe a QGIS configured default value should be able to overwrite a database default value with a NULL value.

Maybe we need to add a new INVALID value that can be used by default values so we no longer abuse NULL.

But I'd really appreciate a statement by @nyalldawson concerning this before deciding for a path.

gioman · 2019-02-21T09:47:09Z

For backporting I think the question is not if but when.

so this will not be backported? I understand playing safe but please also consider the bad regression (for a pretty common operation) we will eventually ship into the release that will put to bed 2.18.

m-kuhn · 2019-02-21T09:49:05Z

so this will not be backported?

on the contrary, it will be backported when we have enough test results to be sure it doesn't introduce any regressions.

gioman · 2019-02-21T09:50:39Z

on the contrary,

sorry I expressed myself badly... I was thinking (in time) for the upcoming release.

m-kuhn · 2019-02-21T09:55:26Z

@gioman @elpaso do you feel confident for an express backport on this?
I'm not putting a veto on it, just thought another couple of weeks for testing this might be good, but if you feel it's appropriate to move ahead, please proceed.

nyalldawson · 2019-02-21T09:55:40Z

My vote would go to playing it safe and waiting till 3.4.6 here -- I understand it's a major regression, but this code is quite complex and there's a chance a worse regression may have been introduced!

gioman · 2019-02-21T09:56:57Z

My vote would go to playing it safe and waiting till 3.4.6 here -- I understand it's a major regression, but this code is quite complex and there's a chance a worse regression may have been introduced!

@nyalldawson @elpaso @m-kuhn I understand, no worries. Thanks everyone for the great bug fixing effort!

elpaso added 2 commits February 19, 2019 08:26

Cache unique values in createFeature (~10x faster)

f2e745e

Cache unique values when creating features

ba3d9ed

Fixes qgis#21305 - pasting features is very slow Aggressively optimize createFeature for speed and introduces createFeatures for bulk creation.

elpaso added Bugfix Needs Backporting labels Feb 19, 2019

elpaso added this to the 3.6.0 milestone Feb 19, 2019

elpaso added 2 commits February 19, 2019 10:51

Rename QgsFeaturesData to QgsFeatureData and fix test case

490c1fe

Fix docs for createFeatures

348e683

m-kuhn reviewed Feb 19, 2019

View reviewed changes

src/core/qgsvectorlayerutils.h Outdated Show resolved Hide resolved

m-kuhn approved these changes Feb 19, 2019

View reviewed changes

elpaso added 5 commits February 19, 2019 14:46

Typo

7fa9bc1

Fix unique values on GPKG pk

1689c93

Fixes qgis#21311

Fix renamed QgsVectorLayerUtils.QgsFeatureData

818611c

Fix unique values on SQLite (OGR) pk

5d7a7ac

Explain why the workaround for GPKG/SQLite is necessary

f454827

I've talked to Even for this one

m-kuhn reviewed Feb 19, 2019

View reviewed changes

src/providers/ogr/qgsogrprovider.cpp Outdated Show resolved Hide resolved

Update src/providers/ogr/qgsogrprovider.cpp

adba08c

Co-Authored-By: elpaso <elpaso@itopen.it>

m-kuhn reviewed Feb 19, 2019

View reviewed changes

src/providers/ogr/qgsogrprovider.cpp Show resolved Hide resolved

Update src/providers/ogr/qgsogrprovider.cpp

7e008b2

Co-Authored-By: elpaso <elpaso@itopen.it>

Revert QStringLiteral

11c9ce0

Fix unique values when generating a set of features

d239ea2

elpaso removed the Needs Backporting label Feb 20, 2019

m-kuhn reviewed Feb 20, 2019

View reviewed changes

src/core/qgsvectorlayerutils.cpp Show resolved Hide resolved

m-kuhn reviewed Feb 20, 2019

View reviewed changes

Update src/core/qgsvectorlayerutils.cpp

f4d7e73

Co-Authored-By: elpaso <elpaso@itopen.it>

elpaso added 4 commits February 20, 2019 08:42

Revert "Update src/core/qgsvectorlayerutils.cpp"

443d860

This reverts commit f4d7e73.

Moved comment

4c8280a

Prevent crash when fields are deleted from the vector options dialog

76c6ec8

Also check for nulls when applying defaults

bf1575f

isValid is not enough because fields are initialized with QVariant(field.type()) which is valid but null. Fixes qgis#21304 With test

Test for regression qgis#21304 "default value" is not applied when pa…

2604fc7

…sting features

Removed redundant check for isValid

4250ef2

elpaso merged commit 17280c3 into qgis:master Feb 21, 2019

elpaso deleted the bugfix-21305-paste-slow-no-provider branch February 21, 2019 07:31

elpaso mentioned this pull request Feb 21, 2019

Postgis: cache information about enum fields #9219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache unique values when creating features #9203

Cache unique values when creating features #9203

elpaso commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

gioman commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

elpaso commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn Feb 20, 2019

m-kuhn Feb 20, 2019

elpaso Feb 20, 2019

m-kuhn commented Feb 20, 2019

nirvn commented Feb 20, 2019

elpaso commented Feb 20, 2019

m-kuhn commented Feb 20, 2019

nirvn commented Feb 20, 2019

elpaso commented Feb 20, 2019

m-kuhn commented Feb 20, 2019

gioman commented Feb 21, 2019

m-kuhn commented Feb 21, 2019

gioman commented Feb 21, 2019

m-kuhn commented Feb 21, 2019

nyalldawson commented Feb 21, 2019

gioman commented Feb 21, 2019

Cache unique values when creating features #9203

Cache unique values when creating features #9203

Conversation

elpaso commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

gioman commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

m-kuhn commented Feb 19, 2019

elpaso commented Feb 19, 2019

elpaso commented Feb 19, 2019

m-kuhn Feb 20, 2019

Choose a reason for hiding this comment

m-kuhn Feb 20, 2019

Choose a reason for hiding this comment

elpaso Feb 20, 2019

Choose a reason for hiding this comment

m-kuhn commented Feb 20, 2019

nirvn commented Feb 20, 2019

elpaso commented Feb 20, 2019

m-kuhn commented Feb 20, 2019

nirvn commented Feb 20, 2019

elpaso commented Feb 20, 2019

m-kuhn commented Feb 20, 2019

gioman commented Feb 21, 2019

m-kuhn commented Feb 21, 2019

gioman commented Feb 21, 2019

m-kuhn commented Feb 21, 2019

nyalldawson commented Feb 21, 2019

gioman commented Feb 21, 2019