Postgis 3

[etiennebr]

This is the draft PR to adapt sf to postgis 3 and proj 6.

• I added github actions, because after talking with Jim at RStudio conf, I realized it would be much easier to test multiple databases such as postgis version 2 and 3. We could get rid of travis if you feel like it, or keep both for now. Github actions are relatively faster (see jim's talk for details).
• I used st_write's new arguments logic. Also, if you're ok with it, I'd like to deprecate delete_layer in favor of overwrite.

There are many commits, if you prefer I could reorganize the commits while I rebase to make it easier to review.

I still need to provide a fix to override missing local srids (#1234 (comment)).

[Robinlovelace]

Apologies for off-topic comment by a quick look at this suggests you solved a problem I was having with GH Actions: r-lib/usethis#1005 great work 🎉

Do you plan to create a placeholder repo that demonstrates these actions, e.g. called sf_actions that could be like https://github.com/edzer/sf_dep/ ? That could be useful for others. Heads-up @Nowosad and also links to the question of creating test environments. I think Docker images with different versions of upstream dependencies would be ideal and think that rocker/geospatial could be a good place for these to live for the community but unsure how to integrate in GH actions. See rocker-org/geospatial#31 and geocompx/geocompr#476 for details.

[edzer]

I still see this:

* checking R code for possible problems ... NOTE
find_database_srtext: no visible binding for global variable ‘srid’
Undefined global functions or variables:
  srid
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... OK
* checking for missing documentation entries ... OK
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... WARNING
Undocumented arguments in documentation object 'find_database_srid'
  ‘crs_local’

Undocumented arguments in documentation object 'find_database_srtext'
  ‘conn’ ‘crs_local’ ‘wkt’

Functions with \usage entries need to have the appropriate \alias
entries, and all their arguments documented.
The \usage entries must correspond to syntactically valid R code.
See chapter ‘Writing R documentation files’ in the ‘Writing R

could you address this? Will submit to CRAN today or tomorrow.

[etiennebr]

@Robinlovelace, good to hear. It's on a separate branch, so I could certainly create a repo out of the github actions.

@edzer, I'm on it. The PR is still in draft mode though, but I'll try to resolve as much checks as I can before the end of the day. I saw you made some commits, I'll have a look too.

[edzer]

Great! I resolved the conflicts and the warnings; I also tried to get postgis on travis to work, which doesn't look like it's going to work (requiring GEOS >= 3.6, apparently not avail on travis), but now see that you got postgis tests going on this new workflows thing, right?

[etiennebr]

Yes, I also tried to have the tests work with travis, but in the end it was much simpler to make them work on github actions/workflow, and adding more databases will be also relatively simple. We could transition the whole tests suite to github. It also allows tests on mac and windows (which I suspended since they don't support postgres services for now).

[etiennebr]

@edzer, found this note.

sf/R/wkb.R

Lines 325 to 330 in 051b204

	if (EWKB)
	writeBin(createType(class(x), endian, TRUE), rc, size = 1L, endian = endian)
	else
	writeBin(createType(class(x)), rc, size = 4L, endian = endian)
	# TODO (?): write SRID in case of EWKB?
	# write out x:

Do you plan to fix EWKB for R st_as_binary? I think it could solve all remaining problems :)

[edzer]

@etiennebr, that would be great.

Writing the SRID of EWKB happens and has always happened here:

sf/src/wkb.cpp

Lines 609 to 610 in 051b204

	if (EWKB && srid != 0)
	add_int(os, srid);

The section you point to is the R code which is only used when calling st_as_binary(..., pureR = TRUE) which nobody ever uses; it is there only to allow double checking that C++ and R give the same result. And perhaps for educational purposes...?

[edzer]

So you're writing documentation for a function that is not exported, right? If so, why? I'd suggest to leave out the #', maybe use only #, so that no documentation objects are created.

[etiennebr]

Yeah, it feels awkward to write a .Rd, but I still felt like minimal documentation would be useful. I'll use #, then.

[etiennebr]

Thanks for the pointer. I modified the CPL_write_wkb function to override srid. I think it's the best solution, but as a result I also need to update lwgeom even if the argument is optional --I'm not familiar with the way the sf_RcppExport.h works, I suppose you might have a better solution.

All checks pass locally on my side (after re-building lwgeom).

[edzer]

Can't we pass the (new) srid through the input field of the crs in the sfc object, so that we can keep the C++ interface as it was?

Maybe you are right, I just want to understand what the implication is of working with a postgis db; any srid that you're writing in the EWKB has to be valid (as of: present) in the context of the database you're writing to, right?

[etiennebr]

Thanks for looking at this. I tried to pass it through the srid attribute as it was, but when the srid is written to the EWKB, it goes through epsg_from_crs and it can't find the srid locally and makes it 0, so I need a way to bypass this.

Postgis maintains a spatial_ref_sys table, somewhat equivalent to the local list of projections. Since postgis stores the srid for every geometry, it just stores the srid and uses the spatial_ref_sys to look up the projection definition. It has four other columns auth_name, auth_srid, srtext and proj4text. So in theory, we also need to carry those around. But in practice, currently, there are no conflicting srid. In the future, the database could handle an authority that would create it's own 4326 by assigning a different database srid.

The table can also be edited by the user to add custom projections, or be of a different version than the local list of projections An R user could also use a WKT unavailable in the list of local projections (so no local srid, but still has a valid WKT).

[edzer]

I don't like to extend CPL_write_wkb with an argument if we don't have to. Can we use the input field, and make the assumptions that

• if epsg_from_crs succeeds, the returned SRID is good
• else, if input carries a string of the form AUTH:SRID, like EPSG:4326, and that succesful parsing an integer after the : must be good
• else we use 0

or do I overlook something?

[etiennebr]

I must say I currently have an awkward work schedule with the pandemic, so it's very likely I'm not coming with the best solutions :). I think your logic would work in most cases (in fact I don't think using custom projection is common, so we're talking about small edge cases). The improbable scenario where two identical srids refer to different WKT would still be incorrect. I don't know if this could happen with different proj/postgis versions, but it could happen when using custom projections.

I'm thinking that maybe CPL_write_wkb should not validate the projection and rather write whatever srid passed (by reading the crs). We could then expose internally epsg_from_crs (it's useful for other purposes anyway) and make sure to call it before we call CPL_write_wkb Maybe this would make for a cleaner interface too.

[edzer]

I agree that asking GDAL to get it is maybe not the good thing here. OK, I'll write it such that it tries to read the SRID from the input field of the crs, and writes 0 on failure. Would that currently work with your tests?

[etiennebr]

Excellent, I think it should work.