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

Fix extent computation in QgsSpatiaLiteProvider #54917

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

strk
Copy link
Contributor

@strk strk commented Oct 11, 2023

Trying to debug #54646 (comment) I was lost in this function and thought I'd rewrite it.

@github-actions github-actions bot added this to the 3.34.0 milestone Oct 11, 2023
@strk strk force-pushed the spatialite-provider branch from eb7eaea to af585e8 Compare October 11, 2023 14:48
@strk strk force-pushed the spatialite-provider branch from af585e8 to 99aff8d Compare October 11, 2023 14:55
@strk strk changed the title Simplify QgsSpatiaLiteProvider::getTableSummary implementation Fix extent computation in QgsSpatiaLiteProvider Oct 11, 2023
@strk strk force-pushed the spatialite-provider branch 3 times, most recently from 3de2cd0 to 2cc125d Compare October 11, 2023 16:01
@strk strk added Bug Either a bug report, or a bug fix. Let's hope for the latter! Spatialite data provider labels Oct 11, 2023
@strk strk requested a review from lnicola October 11, 2023 16:16
@lnicola
Copy link
Contributor

lnicola commented Oct 11, 2023

Looks like I had an unsubmitted comment for some reason. Looks fine, I guess, but I really think moving to the standard query API is a better idea. But even without that, this looks like an improvement and probably fixes the mentioned issue.

@strk strk force-pushed the spatialite-provider branch from 2cc125d to cee6e6c Compare October 11, 2023 21:55
@strk
Copy link
Contributor Author

strk commented Oct 11, 2023

The basic fix here, I believe, is handling the NULL min/max values of extent when number of rows is zero.
In that case, without this change, you get a 0.0 values instead of NULL, so a 0,0,0,0 QgsRectangle instead of a Null rectangle.
Nobody notices until you stop considering 0,0,0,0 not-null ( as in #54646 )

@strk
Copy link
Contributor Author

strk commented Oct 11, 2023

If the empty string check is not good (CI will tell, I guess?) we can consider using proper null checking. This may be useful in that case: https://stackoverflow.com/questions/8961457/how-to-check-a-value-in-a-sqlite-column-is-null-or-not-with-c-api

Extent should be set to null if there are no rows or geometric
field or computed min/max envelope ordinates are null.

Also makes the implementation more readable (hopefully).
@strk strk force-pushed the spatialite-provider branch from 4b78fe6 to 03fe615 Compare October 12, 2023 08:30
@strk strk enabled auto-merge (rebase) October 12, 2023 11:58
@strk
Copy link
Contributor Author

strk commented Oct 13, 2023

@elpaso I've addressed all your suggestions, are we ready to go now ? I need one approval to proceed

@strk strk merged commit 32d07b1 into qgis:master Oct 13, 2023
@strk strk deleted the spatialite-provider branch October 13, 2023 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Spatialite data provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants