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 provider] Fix performance issue on initial GetFeature COUNT=1 request with a BBOX (fixes #50528) #50650

Merged
merged 2 commits into from
Nov 18, 2022

Conversation

rouault
Copy link
Contributor

@rouault rouault commented Oct 21, 2022

No description provided.

@github-actions github-actions bot added this to the 3.30.0 milestone Oct 21, 2022
Copy link
Contributor

@pathmapper pathmapper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, do we still need the logic with the large bbox filter? If the first count=1 request is without bbox filter, the first feature (if any) will be returned and a geometry type or NoGeometry will be detected. So mShared->mWKBType == QgsWkbTypes::Unknown will never be true except there is no feature returned, right? And in this case, a count=1 request with large bbox would receive a feature neither...

Also, with this PR, we're running again into the situation which #50237 fixed (if the first feature returned has NoGeometry and all other features are having a distinct geometry type). Not sure if such a case is considered also as mixed geometry feature type (ref #49328). We're running again in the underlying problem, that's not possible to reliable determine the geometry type of a feature type with unknown geometry in DescribeFeatureTypes using a single count=1 GetFeature request.

…hose GetFeature COUNT=1 request without BBOX returns a null geometry, but with a BBOX returns a non-null geometry
@rouault
Copy link
Contributor Author

rouault commented Oct 22, 2022

With this change, do we still need the logic with the large bbox filter? If the first count=1 request is without bbox filter, the first feature (if any) will be returned and a geometry type or NoGeometry will be detected. So mShared->mWKBType == QgsWkbTypes::Unknown will never be true except there is no feature returned, right? And in this case, a count=1 request with large bbox would receive a feature neither...

There was indeed an issue that I've corrected by amending the commit. If the GetFeature without a bbox filter returns a feature without geometry, then it resets mWKBType to QgsWkbTypes::Unknown. I've also changed the logic to not try a GetFeature with a bbox when the geometry type is already known from DescribeFeatureType

Also, with this PR, we're running again into the situation which #50237 fixed (if the first feature returned has NoGeometry and all other features are having a distinct geometry type)

yes, support of mixed geometry types is a hard problem, not addressed by the recent fixes, including that one. The current behaviour is just a heuristics that work when the layer has a mix of features without geometry and features of the same geometry type.

@pathmapper
Copy link
Contributor

LGTM 🎉

support of mixed geometry types is a hard problem, not addressed by the recent fixes, including that one.

Sure, no question 😄

The current behaviour is just a heuristics that work when the layer has a mix of features without geometry and features of the same geometry type.

Thanks for preserving this behaviour.

@pathmapper
Copy link
Contributor

pathmapper commented Nov 3, 2022

Tested with sample project provided in #50528 and confirmed that the PR fixes the issue when opening the project.

@rouault
Copy link
Contributor Author

rouault commented Nov 3, 2022

Tested with sample project provided in #50528 and confirmed that the PR fixes the issue when opening the project.

do you have permissions to approve ? (go in the "File changes" tab and use "Review changes" dropdown combo at the top right)

@pathmapper
Copy link
Contributor

pathmapper commented Nov 3, 2022

We need someone with write access. @elpaso, @pblottiere maybe one of you could chime in?

@github-actions
Copy link

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.

@github-actions github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 18, 2022
@rouault
Copy link
Contributor Author

rouault commented Nov 18, 2022

@elpaso @nyalldawson gentle ping for a review

@github-actions github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Nov 18, 2022
@pathmapper
Copy link
Contributor

Thanks for the review @elpaso

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

Successfully merging this pull request may close these issues.

None yet

4 participants