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.

Sign up for GitHub

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

Jump to bottom

Fix PostGIS geography extent computation #57514

Merged
merged 11 commits into from
Jun 22, 2024
Merged

Fix PostGIS geography extent computation #57514

merged 11 commits into from
Jun 22, 2024

Conversation

strk
Copy link
Contributor

@strk strk commented May 22, 2024

Closes GH-30294

@strk strk added Bug Either a bug report, or a bug fix. Let's hope for the latter! PostGIS data provider labels May 22, 2024
@github-actions github-actions bot added this to the 3.38.0 milestone May 22, 2024
@strk strk mentioned this pull request May 22, 2024
Closed
benoitdm-oslandia
benoitdm-oslandia approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@benoitdm-oslandia benoitdm-oslandia left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions GitHub Actions
Copy link

github-actions bot commented May 22, 2024 •
edited
Loading

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit f5fc285)

@strk
Copy link
Contributor Author

strk commented May 22, 2024

It's actually not good as it returns an extent of 0.8130174279212951,-0.1733461171388630 : 0.9706265926361080,0.2985342144966130 but it seems to be a PostGIS issue with estimated extent, which I've filed as https://trac.osgeo.org/postgis/ticket/5734

@benoitdm-oslandia
Copy link
Contributor

strange behavior! That is really strange no one has ever noticed this issue....

@strk
Copy link
Contributor Author

strk commented May 22, 2024 •
edited
Loading

Indeed, I guess we could resort back to give full planet extent for estimation and use a cast to geometry for full computation, until the issue is fixed upstream

@strk
Copy link
Contributor Author

strk commented May 22, 2024

With 69fffa0 we never estimate but always compute, as if stats were never gathered. It works correctly for me

@strk strk force-pushed the issue30294-geography-extent branch from 69fffa0 to 17f7e1e Compare May 22, 2024 19:19
@strk strk force-pushed the issue30294-geography-extent branch from 17f7e1e to d9cbc1a Compare May 23, 2024 08:57
benoitdm-oslandia
benoitdm-oslandia approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@benoitdm-oslandia benoitdm-oslandia left a comment

Choose a reason for hiding this comment

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

LGTM!

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 7, 2024

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 Jun 7, 2024
@strk strk added Requires Tests! Waiting on the submitter to add unit tests before eligible for merging and removed stale Uh oh! Seems this work is abandoned, and the PR is about to close. labels Jun 7, 2024
@strk strk force-pushed the issue30294-geography-extent branch 3 times, most recently from a8468d3 to d702056 Compare June 13, 2024 12:01
@strk strk force-pushed the issue30294-geography-extent branch from d702056 to efae21a Compare June 17, 2024 21:51
@strk strk force-pushed the issue30294-geography-extent branch from aa5e200 to 7acbaf7 Compare June 18, 2024 10:41
@elpaso
Copy link
Contributor

elpaso commented Jun 21, 2024

This PR is flagged with require tests. I agree that a test would be nice.

@strk strk mentioned this pull request Jun 21, 2024
Closed
2 tasks
@strk strk force-pushed the issue30294-geography-extent branch from 7acbaf7 to dcf1e6c Compare June 21, 2024 09:19
@strk
Copy link
Contributor Author

strk commented Jun 21, 2024

Test added

m-kuhn
m-kuhn approved these changes Jun 21, 2024
View reviewed changes
Copy link
Member

@m-kuhn m-kuhn left a comment

Choose a reason for hiding this comment

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

One minor change requested, then it's good to go

@Gustry Gustry removed the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Jun 21, 2024
@strk strk mentioned this pull request Jun 21, 2024
Closed
@strk
Copy link
Contributor Author

strk commented Jun 21, 2024

I confirm the new test is effective at catching the bug as I tried it against 3.34 and got this:

AssertionError: <QgsRectangle: -180 -90, 180 90> != <QgsRectangle: -10 -60, 10 60>

@qgis qgis deleted a comment from github-actions bot Jun 21, 2024
@nyalldawson nyalldawson reopened this Jun 21, 2024
strk added 11 commits June 22, 2024 05:36
@strk
Fix PostGIS geography extent computation
0c36c42 
Do not estimate extent with geography spatial type, because
ST_EstimatedExtent returns bogus data, see:
https://trac.osgeo.org/postgis/ticket/5734

Closes qgisGH-30294
@strk
Refactor QgsPostgresProvider::extent3D
efaebe7 
- Accept empty (not-null) extent as a valid answer
- Drop not more needed workarounds for corner cases
- Make the code hopefully more readble
- Fix a spurious COMMIT out of transaction
@strk
PostGIS-3.5.0 fixed estimated extent for geography, use it !
ec1a6b3
@strk
Extrapolate extent estimation and extraction to private functions
b517ac4 
Also const-correct QgsPostgresProvider::setExtent
@strk
Add test for estimated geography extent
41aa83f
@strk
Do not translate debug messages
af8d588
@strk
Have testGeographyExtent cleanup after itself
0be9b92
@strk
Use assertAlmostEqual for easier debugging
4167713
@strk
Add debug line for extent estimation
ab52c62
@strk
Comment computeExtent3D and estimateExtent private methods
ab30ff4
@strk
Fix syntax
f5fc285
@strk strk force-pushed the issue30294-geography-extent branch from 85190c7 to f5fc285 Compare June 22, 2024 03:37
@strk strk merged commit 0873689 into qgis:master Jun 22, 2024
30 checks passed
@strk strk deleted the issue30294-geography-extent branch June 22, 2024 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-kuhn m-kuhn m-kuhn approved these changes

@benoitdm-oslandia benoitdm-oslandia benoitdm-oslandia approved these changes

@nyalldawson nyalldawson Awaiting requested review from nyalldawson

Assignees
No one assigned
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! PostGIS data provider
Projects
None yet
Milestone
3.38.0
Development

Successfully merging this pull request may close these issues.

Extent of data fetched from GEOGRAPHY PostGIS column can not be changed
6 participants
@strk @benoitdm-oslandia @elpaso @m-kuhn @nyalldawson @Gustry