Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wfs] Fix background cache iterator when using with both subset string and filter on fid(s) #34009

Merged
merged 3 commits into from Mar 17, 2020

Conversation

@troopa81
Copy link
Contributor

troopa81 commented Jan 24, 2020

#Description

In QgsBackgroundCacheFeatureIterator we combine request and side filter expression request (when a subsetfilter is set on layer) but it removes the fid filter on input request if there is one. This PR fixes this.

Checklist

  • Commit messages are descriptive and explain the rationale for changes
  • Commits which fix bugs include Fixes #11111 at the bottom of the commit message
  • I have read the QGIS Coding Standards and this PR complies with them
  • New unit tests have been added for core changes
  • I have run the scripts/prepare-commit.sh script before each commit
  • I have evaluated whether it is appropriate for this PR to be backported, backport requests are left as label or comment
@roya0045

This comment has been minimized.

Copy link
Contributor

roya0045 commented Jan 24, 2020

I'm confused as to how the subset string is distinct from an expression. Though usually expressions and fids filter are exclusive (see #9915 )

@m-kuhn m-kuhn changed the title Fix backgroundcachefeatureiterator when there is a sidefilterexpression and filter fids [wfs] Fix backgroundcachefeatureiterator when there is a sidefilterexpression and filter fids Jan 25, 2020
Copy link
Member

m-kuhn left a comment

Looks good, thanks 👍

Just one small thing, using variable names from inside a provider in a title for a pull request (like the "clientSideFilterExpression" mangled to sidefilterexpression) is normally not very helpful to get started. It's better to add some bigger picture topics to the title (subset filter, wfs, fid filter) to help reviewers filter what they want to look into.

@nyalldawson nyalldawson reopened this Jan 27, 2020
@nyalldawson

This comment has been minimized.

Copy link
Contributor

nyalldawson commented Jan 27, 2020

@rouault how's this look to you?

@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 28, 2020

Pending my remark, and resolving issues raised in other providers by the added test, LGTM

@troopa81 troopa81 changed the title [wfs] Fix backgroundcachefeatureiterator when there is a sidefilterexpression and filter fids [wfs] Fix background cache iterator when using with both subset string and filter on fid(s) Jan 28, 2020
@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Jan 28, 2020

Just one small thing, using variable names from inside a provider in a title for a pull request (like the "clientSideFilterExpression" mangled to sidefilterexpression) is normally not very helpful to get started. It's better to add some bigger picture topics to the title (subset filter, wfs, fid filter) to help reviewers filter what they want to look into.

True. Renamed

@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Jan 28, 2020

I'm confused as to how the subset string is distinct from an expression. Though usually expressions and fids filter are exclusive

Actually, this is not exactly the same thing than combining filter request (from what I understand), it's a first filtering on subset string, then you filter on fid based on the result of first filtering. For this provider, it could be managed by a filter AND combination (if such request combination was possible), for the others it's quite different (traduce with where clause for PG provider for instance).

@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Jan 28, 2020

Pending my remark, and resolving issues raised in other providers by the added test, LGTM

I fixed it, waiting for travis now

Copy link
Member

m-kuhn left a comment

Something more I wonder on the conceptual side.
Does WFS support an optimize get feature by fid(s) request?

Right now - if I understand the approach correctly - a get feature by fid (e.g. identify feature) will download the complete subset (potentially thousands of features) and filter locally for a single fid. It seems the inverse approach would be way more efficient.

@rouault

This comment has been minimized.

Copy link
Contributor

rouault commented Jan 28, 2020

Does WFS support an optimize get feature by fid(s) request?

It does. But... you need to provide a gml:id as input, not a QGIS feature id, and there's no universal way of creating a bijection between both. So, a QGIS get feature by fid request can indeed download the whole dataset from the server. But if you identify a feature already downloaded from the server, it will start by querying the temporary Spatialite database first, so things are not that bad in practice for interactive scenarios.

@troopa81 troopa81 force-pushed the troopa81:fix_oapif_filter_fid branch from 0f94125 to 631ab93 Feb 3, 2020
@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Feb 3, 2020

I revert my modification on delimited provider, I had a bad understanding of the real issue (just a mix up between pk and id)

@stale

This comment has been minimized.

Copy link

stale bot commented Feb 17, 2020

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 stale bot added the stale label Feb 17, 2020
@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Feb 17, 2020

Don't stale

@stale stale bot removed the stale label Feb 17, 2020
@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Feb 28, 2020

Pending my remark, and resolving issues raised in other providers by the added test, LGTM

@rouault was ok for merging, is it ok for you @m-kuhn ?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Feb 28, 2020

Is there any drawback on inverting the approach (i.e. doing the fid request towards the backend and the subset filtering as post processing step)?

I think it would be much more future proof if the fid->pk lookup situation improves in the future.

Julien Cabieces and others added 3 commits Jan 24, 2020
@troopa81 troopa81 force-pushed the troopa81:fix_oapif_filter_fid branch from 631ab93 to 3d3af4c Feb 28, 2020
@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Feb 28, 2020

Is there any drawback on inverting the approach (i.e. doing the fid request towards the backend and the subset filtering as post processing step)?

No, I think its better indeed. I changed it

@github-actions github-actions bot added this to the 3.14.0 milestone Mar 4, 2020
@troopa81 troopa81 closed this Mar 11, 2020
@troopa81 troopa81 reopened this Mar 11, 2020
@troopa81

This comment has been minimized.

Copy link
Contributor Author

troopa81 commented Mar 17, 2020

@m-kuhn Does it look like ready for merging with the last modification?

@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Mar 17, 2020

Yes, sorry I must have gotten sidetracked last time I looked at it.

@m-kuhn m-kuhn merged commit 4f0f4a8 into qgis:master Mar 17, 2020
4 checks passed
4 checks passed
Backport
Details
Create issue on doc repo for labeled issue
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
qgis.QGIS #20200311.22 succeeded
Details
@m-kuhn

This comment has been minimized.

Copy link
Member

m-kuhn commented Mar 17, 2020

Thanks a lot

@qgis-bot

This comment has been minimized.

Copy link

qgis-bot commented Mar 17, 2020

The backport to release-3_10 failed:

The process 'git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_10 release-3_10
# Navigate to the new working tree
cd .worktrees/backport-release-3_10
# Create a new branch
git switch --create backport-34009-to-release-3_10
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4f0f4a86537d07fe7f7a90b21194c33ec56a57e4
# Push it to GitHub
git push --set-upstream origin backport-34009-to-release-3_10
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_10

Then, create a pull request where the base branch is release-3_10 and the compare/head branch is backport-34009-to-release-3_10.

@qgis-bot

This comment has been minimized.

Copy link

qgis-bot commented Mar 17, 2020

The backport to release-3_12 failed:

The process 'git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_12 release-3_12
# Navigate to the new working tree
cd .worktrees/backport-release-3_12
# Create a new branch
git switch --create backport-34009-to-release-3_12
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick 4f0f4a86537d07fe7f7a90b21194c33ec56a57e4
# Push it to GitHub
git push --set-upstream origin backport-34009-to-release-3_12
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_12

Then, create a pull request where the base branch is release-3_12 and the compare/head branch is backport-34009-to-release-3_12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.