Fix a bunch of MS SQL provider issues #8411
Conversation
These include: - fetching incomplete (corrupted) geometries - fetching incomplete text data (e.g. WKT for CRS) Also fixes a bug with empty attribute table for some ms sql layers
There is no direct relationship between postgis SRIDs and SRIDs used in any other database. If you imported data with GDAL, things may work, because GDAL prefers to use equivalent SRIDs to EPSG IDs / PostGIS SRIDs. If you imported data with QGIS, that will not work. QGIS MS SQL provider happily uses internal SRS IDs from srs.db for SRIDs. That should be probably modified to use GDAL's logic: 1. preferably use EPSG ID as SRID 2. if taken, find a safe SRID
They will just have a generic vector icon
| if ( typeList.isEmpty() ) | ||
| { | ||
| // this suggests that retrieval of geometry type and CRS failed if no results were returned | ||
| // for examle due to invalid geometries in the table (WHAAAT?) |
There was a problem hiding this comment.
Welcome to hell. Sql server engineers have no clue about the requirements of spatial data users.
Isn't this working already? By default all our queries on mssql should pass through a manual make valid step to avoid this issue. I'm keen to hear if we've missed any.
I've hit this too, but just didn't know enough about SQL server crs handling to tackle before freeze. This is the biggest issue with the provider that I'm currently aware of. Fixes look good to me. |
Sounds like a good plan, especially since there's no dbmanager plugin for mssql. In fact... it might be a great time to start a interface class for browser db items with just a capability flag for this action? If you start I know there's definitely other devs who'll take this and run with it (myself included)! |
It looks like addFeatures() does not try to validate geometries, while changeGeometryValues() does. Looking back into the history, the provider originally had MakeValid() in addFeatures() too, but it was removed in 62f4534 ... now with the new option for invalid geometry handling it may be a good idea to reintroduce MakeValid... |
I meant that half-jokingly... Fixing invalid geometries should not be a common task and in fact AFAIK it is just mssql that goes crazy when it gets invalid features. Currently we have inconsistent behavior - features are validated on UPDATE, but not on CREATE. Looking into GDAL code, it validates both on CREATE and UPDATE. Keen to hear what @NathanW2 's issue was. Another problem is that MS SQL has obviously different rules about valid geometries, so a valid geometry according to QGIS may be invalid according to MS SQL (this happened with multiple layers for me). We would need some provider specific pre-commit hooks to warn user and abort if needed, but for now I think we need to force validation of geometries (on create/update). |
|
So the issue with make valid on sql server is sometimes it can make geometry collections, or null geometry objects etc. I mainly removed it because we had clients entering data in QGIS and it would sometimes make "invalid" geometries that would end up breaking other things or never to be seen in QGIS again to be fixed. If we can include validation in the QGIS side as a first pass this should be ok at least as a first gate check. |
Thanks to @m-kuhn 's work in QGIS 3.4 it is possible to add "is valid" geometry checks for digitizing - so if you create a butterfly polygon (with self-intersecting exterior ring) you will get this reported. So this should address common digitizing errors. The tricky bit is that MSSQL has different definition of what is valid, so a valid geometry in QGIS may be still considered invalid by MSSQL - that's why I think we need to have |
|
@wonder-sk do you have particulars about the difference of ms sql notion of isvalid to the geos notion of isvalid? |
|
@m-kuhn no details but I have some geometries that are valid in QGIS but they're not in MSSQL. I haven't investigated further as they contain many vertices. See attached layer in https://issues.qgis.org/issues/20122 By the way ESRI world also has different definition of valid polygons different from OGC (regarding how polygon with hole touching external ring should be represented) |
|
Maybe we need a table 🙄
|
This essentially reverts 62f4534 Rationale: - even valid geometries according to GEOS may be considered as invalid by MS SQL so there is no way of knowing that a geometry may be fail to be added - change of geometries applies MakeValid() so it is consistent again - GDAL driver also applies MakeValid() on all added/changed geometries - QGIS since 3.4 has optional geometry checks for validity etc. so some truly invalid geometries may be fixed before submitted to the provider See also qgis#8411
This essentially reverts 62f4534 Rationale: - even valid geometries according to GEOS may be considered as invalid by MS SQL so there is no way of knowing that a geometry may be fail to be added - change of geometries applies MakeValid() so it is consistent again - GDAL driver also applies MakeValid() on all added/changed geometries - QGIS since 3.4 has optional geometry checks for validity etc. so some truly invalid geometries may be fixed before submitted to the provider See also #8411
This essentially reverts 62f4534 Rationale: - even valid geometries according to GEOS may be considered as invalid by MS SQL so there is no way of knowing that a geometry may be fail to be added - change of geometries applies MakeValid() so it is consistent again - GDAL driver also applies MakeValid() on all added/changed geometries - QGIS since 3.4 has optional geometry checks for validity etc. so some truly invalid geometries may be fixed before submitted to the provider See also qgis#8411 (cherry picked from commit b1fd7b5)
This essentially reverts 62f4534 Rationale: - even valid geometries according to GEOS may be considered as invalid by MS SQL so there is no way of knowing that a geometry may be fail to be added - change of geometries applies MakeValid() so it is consistent again - GDAL driver also applies MakeValid() on all added/changed geometries - QGIS since 3.4 has optional geometry checks for validity etc. so some truly invalid geometries may be fixed before submitted to the provider See also #8411 (cherry picked from commit b1fd7b5)
Testing MS SQL Server container running on Linux, I have quickly run into a combination of bugs in MS SQL provider that made it unusable in my quick tests (import a layer, display it on the map).
The bugs fixed in this PR:
There are still few things that make MS SQL provider annoying, but at least I they are not complete blockers:
update [my_table] set [geom] = [geom].MakeValid() where [geom].STIsValid()=0... maybe something we could have available on right-click in browser :-)