ropensci / osmextract

Download and import OpenStreetMap data from Geofabrik and other providers
https://docs.ropensci.org/osmextract
GNU General Public License v3.0
170 stars 12 forks source link

Unexpected error when using GDAL 3.10.0rc1 [BUG] #298

Closed rsbivand closed 3 weeks ago

rsbivand commented 3 weeks ago

With released GDAL 3.9.3, no error occurs in oe_get_network: osmextract.Rcheck.GDAL393.zip However, when running the same CMD check with the same platform and the same sf, GDAL 3.10.0rc1 shows a failure with no its_walking features found: osmextract.Rcheck.GDAL3100.zip Please run your own checks with a GDAL 3.10.0 release candidate (currently rc2, probably the last) and see whether this is a problem. @agila5 @edzer

Robinlovelace commented 3 weeks ago

Do you know the cause @agila5? Imagine it's a quick fix..

rsbivand commented 3 weeks ago

My sf::sf_extSoftVersion() on GDAL 3.10.0rc1:

          GEOS           GDAL         proj.4 GDAL_with_GEOS     USE_PROJ_H 
      "3.13.0"       "3.10.0"        "9.5.0"         "true"         "true" 
          PROJ 
       "9.5.0" 

with all else the same (GEOS 3.13.0 and PROJ 9,5.0).

Error unchanged with GDAL 3.10.0rc2.

agila5 commented 3 weeks ago

Do you know the cause @agila5? Imagine it's a quick fix..

No clue, sorry. Will try to build a container with GDAL 3.10.0 (Windows user here 😅) and debug from there.

@rsbivand according to the .Rcheck file attached to the first message, the error occurs here

https://github.com/ropensci/osmextract/blob/2aa83c6822f33d6a75c5ce3711b0841cefff5fa0/R/get-network.R#L98-L103

Could you please run the same checks setting quiet = FALSE in that example? Maybe there is some interesting output. Happy to create a branch of this repo with that change if that helps.

rsbivand commented 3 weeks ago

@agila5 :

 library(osmextract)
Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright.
Check the package website, https://docs.ropensci.org/osmextract/, for more details.
> its_pbf = file.path(tempdir(), "test_its-example.osm.pbf")
> file.copy(
+   from = system.file("its-example.osm.pbf", package = "osmextract"),
+   to = its_pbf,
+   overwrite = TRUE
+ )
[1] TRUE
> its = oe_get(
+   "ITS Leeds", quiet = TRUE, download_directory = tempdir()
+ )
> plot(its["highway"], lwd = 2, key.pos = 4, key.width = lcm(2.75))
> its_walking = oe_get_network(
+   "ITS Leeds", mode = "walking",
+   download_directory = tempdir(), quiet = FALSE
+ )
The input place was matched with: ITS Leeds
The chosen file was already detected in the download directory. Skip downloading.
Starting with the vectortranslate operations on the input file!
0...10...20...30...40...50...60...70...80...90...100 - done.
Finished the vectortranslate operations on the input file!
Reading layer `lines' from data source `/tmp/RtmpAN7Wku/test_its-example.gpkg' using driver `GPKG'
Simple feature collection with 0 features and 12 fields
Bounding box:  xmin: NA ymin: NA xmax: NA ymax: NA
Geodetic CRS:  WGS 84

It feels as though the parsing of the vectortranslate options may have changed.

rouault commented 3 weeks ago

ok, I'm confident I've found the reason for this issue (at least I found one). It is linked to this change in GDAL 3.10.0: "OGRSQL: fix compliance of NOT and IN operators regarding NULL values" So before, an expression like access NOT IN ('private', 'no') would match rows where acces is NULL, which is not SQL92 compliant (SQLite or PostgreSQL wouldn't match such rows). So it has to be rewritten as (access IS NULL OR access NOT IN ('private', 'no')) to get in GDAL 3.10.0 the same output as in 3.9.0 So you have to fix your SQL WHERE clauses. But unfortunately I also discovered that there's an implementation issue on GDAL side where in complex expressions this doesn't work as expected. So I'm on it fixing that, and will need to issue a new RC with the fix.

agila5 commented 3 weeks ago

Thank you very much @rouault for the diagnosis 🙇‍♂️ It took me forever just to install GDAL 3.10.0. I will wait for the new RC and then (hopefully) fix this package.

rouault commented 3 weeks ago

GDAL fix in https://github.com/OSGeo/gdal/pull/11190 (sigh: still again the ever recurring topic of NULL, NaN, empty, ...)

rsbivand commented 3 weeks ago

@agila5 once the SQL is corrected, I could try it on rc3 or patched rc2. On Windows you'd need sf built from source against GDAL built for MXE, quite a lot of work.

agila5 commented 3 weeks ago

Thank you very much @rsbivand! I've implemented the suggest fix in the fix-sql branch: https://github.com/ropensci/osmextract/pull/300

rouault commented 3 weeks ago

GDAL 3.10.0 rc3 is out: https://lists.osgeo.org/pipermail/gdal-dev/2024-November/059749.html

rsbivand commented 3 weeks ago

@agila5 with rc3, sf current HEAD and the fix-sql branch of osmextract, looks OK, I get this CMD check log: 00check.log Please let me know if you need any intermediate output from me.

agila5 commented 3 weeks ago

@agila5 with rc3, sf current HEAD and the fix-sql branch of osmextract, looks OK, I get this CMD check log:

I think that's perfect, thank you very much!

Please let me know if you need any intermediate output from me.

I guess the error is fixed, so I will complete that PR, merge it and close this issue. Thank you very much @rsbivand for reporting it, and thanks @rouault for the fix 🙇‍♂️