Merge pull request #300 from ropensci/fix-sql

Fix sql

NEWS.md

@@ -3,11 +3,12 @@
 ### MAJOR CHANGES
 
 * Bump minimum R version from 3.5.0 to 3.6.0 since that's a requirement for one of our indirect dependencies (i.e. [evaluate](https://cran.r-project.org/package=evaluate)). 
+* Adjusted the SQL syntax used inside `oe_get_network` so that the queries are compatible with GDAL 3.10 ([#298](https://github.com/ropensci/osmextract/issues/291)). 
 
 ### MINOR CHANGES
 
 * Updated the `osmconf.ini` file to be in synch with the GDAL version.
-* Added `oneway` as column by default when using `get_network(mode = "driving")`, which indicates if a link represents an uni-directional road ([#296](https://github.com/ropensci/osmextract/issues/296))
+* Added `oneway` as column by default when using `oe_get_network(mode = "driving")`, which indicates if a link represents an uni-directional road ([#296](https://github.com/ropensci/osmextract/issues/296))
 
 # osmextract 0.5.1
 

R/get-network.R

@@ -144,6 +144,14 @@ oe_get_network = function(
 # accepted but have support among many mappers. More precise options are defined
 # by other tags. See also: https://wiki.openstreetmap.org/wiki/Bicycle#Bicycle_Restrictions
 
+# WARNING: Starting from GDAL 3.10, an expression like "foo NOT IN ('bar')"
+# evaluates as false, while previously it would evaluate as true. Therefore, in
+# the following code, when building the -where clause, we need to be explicit
+# and include possible NULL values in the expression. See also the discussion in
+# #298 and the fix implemented by @rouault. See also
+# https://github.com/OSGeo/gdal/blob/779871e56134111d61f1fe2859b8d19f8f04fcdf/MIGRATION_GUIDE.TXT#L4
+# for official docs.
+
 # A cycling mode of transport includes the following scenarios:
 # - highway IS NOT NULL (since usually that means that's not a road);
 # - highway NOT IN ('abandoned', 'bus_guideway', 'byway', 'construction', 'corridor',
@@ -163,21 +171,21 @@ load_options_cycling = function(place) {
     "-where", "
     (highway IS NOT NULL)
     AND
-    (highway NOT IN (
+    (highway IS NULL OR highway NOT IN (
     'abandoned', 'bus_guideway', 'byway', 'construction', 'corridor', 'elevator',
     'fixme', 'escalator', 'gallop', 'historic', 'no', 'planned', 'platform',
     'proposed', 'raceway', 'steps'
     ))
     AND
-    (highway NOT IN ('motorway', 'motorway_link', 'footway', 'bridleway',
+    (highway IS NULL OR highway NOT IN ('motorway', 'motorway_link', 'footway', 'bridleway',
     'pedestrian') OR bicycle IN ('yes', 'designated', 'permissive', 'destination')
     )
     AND
-    (access NOT IN ('private', 'no'))
+    (access IS NULL OR access NOT IN ('private', 'no'))
     AND
-    (bicycle NOT IN ('private', 'no', 'use_sidepath', 'restricted'))
+    (bicycle IS NULL OR bicycle NOT IN ('private', 'no', 'use_sidepath', 'restricted'))
     AND
-    (service NOT ILIKE 'private%')
+    (service IS NULL OR service NOT ILIKE 'private%')
     "
     )
   )
@@ -203,17 +211,17 @@ load_options_walking = function(place) {
     "-where", "
     (highway IS NOT NULL)
     AND
-    (highway NOT IN ('abandoned', 'bus_guideway', 'byway', 'construction', 'corridor', 'elevator',
+    (highway IS NULL OR highway NOT IN ('abandoned', 'bus_guideway', 'byway', 'construction', 'corridor', 'elevator',
     'fixme', 'escalator', 'gallop', 'historic', 'no', 'planned', 'platform', 'proposed', 'raceway',
     'motorway', 'motorway_link'))
     AND
-    (highway <> 'cycleway' OR foot IN ('yes', 'designated', 'permissive', 'destination'))
+    (highway IS NULL OR highway <> 'cycleway' OR foot IN ('yes', 'designated', 'permissive', 'destination'))
     AND
-    (access NOT IN ('private', 'no'))
+    (access IS NULL OR access NOT IN ('private', 'no'))
     AND
-    (foot NOT IN ('private', 'no', 'use_sidepath', 'restricted'))
+    (foot IS NULL OR foot NOT IN ('private', 'no', 'use_sidepath', 'restricted'))
     AND
-    (service NOT ILIKE 'private%')
+    (service IS NULL OR service NOT ILIKE 'private%')
     "
     )
   )
@@ -236,16 +244,16 @@ load_options_driving = function(place) {
     "-where", "
     (highway IS NOT NULL)
     AND
-    (highway NOT IN (
+    (highway IS NULL OR highway NOT IN (
     'abandoned', 'bus_guideway', 'byway', 'construction', 'corridor', 'elevator',
     'fixme', 'escalator', 'gallop', 'historic', 'no', 'planned', 'platform',
     'proposed', 'cycleway', 'pedestrian', 'bridleway', 'path', 'footway',
     'steps'
     ))
     AND
-    (access NOT IN ('private', 'no'))
+    (access IS NULL OR access NOT IN ('private', 'no'))
     AND
-    (service NOT ILIKE 'private%')
+    (service IS NULL OR service NOT ILIKE 'private%')
     "
     )
   )

tests/testthat/test-get-network.R

@@ -10,6 +10,41 @@ test_that("oe_get_network: simplest examples work", {
   expect_error(oe_get_network("ITS Leeds", quiet = TRUE), NA)
 })
 
+test_that("oe_get_network: -where clause has the same behaviour as the corresponding R code", {
+  # See the discussion in #298 for more details about this test. The basic idea
+  # is to test that the output of SQL code is the same as analogous R
+  # expressions run on the same .osm.pbf file.
+  withr::local_envvar(
+    .new = list(
+      "OSMEXT_DOWNLOAD_DIRECTORY" = tempdir(),
+      "TESTTHAT" = "true"
+    )
+  )
+  its_pbf <- setup_pbf()
+
+  its <- oe_read(its_pbf,
+    layer = "lines",
+    extra_tags = c("access", "service", "oneway"), quiet = TRUE
+  )
+  # Replicate the SQL conditions for driving mode using regular R code
+  idx_R <- with(
+    its,
+    !is.na(highway) &
+    # NB: %in% automatically sets NA %in% ('bar') as FALSE
+    (is.na(highway) | highway %!in% c(
+      'abandoned', 'bus_guideway', 'byway', 'construction', 'corridor', 'elevator',
+      'fixme', 'escalator', 'gallop', 'historic', 'no', 'planned', 'platform',
+      'proposed', 'cycleway', 'pedestrian', 'bridleway', 'path', 'footway',
+      'steps'
+    )) &
+    (is.na(access) | access %!in% c('private', 'no')) &
+    (is.na(service) | !grepl('^private', service, ignore.case = TRUE))
+  )
+
+  its_driving <- oe_get_network("ITS Leeds", mode = "driving", quiet = TRUE)
+  expect_true(nrow(its_driving) == sum(idx_R))
+})
+
 test_that("oe_get_network: options in ... work correctly", {
   withr::local_envvar(
     .new = list(