skunk icon indicating copy to clipboard operation
skunk copied to clipboard

Add PostGIS support.

Open cranst0n opened this issue 4 years ago • 6 comments

Preliminary crack at adding PostGIS codecs, using the PostGIS JDBC driver types. I based of this stuff on the way PostGIS support is done in doobie.

Note that this changes the Docker image to add the PostGIS extension, which may have CI consequences I can't foresee, but it works on my machine :sunglasses:.

cranst0n avatar Aug 27 '21 17:08 cranst0n

I'm reluctant to depend on JDBC for these data types. How hard would it be to implement them and their parsers in Scala?

tpolecat avatar Aug 27 '21 17:08 tpolecat

Yep understandable. I think re-implementing the parsing shouldn't be too awful, at least for the basic types. I'll try throwing something minimal together and get a better feel for how much will be involved, then update this.

cranst0n avatar Aug 27 '21 18:08 cranst0n

Also a compile error for 2.12, looks like.

tpolecat avatar Aug 27 '21 19:08 tpolecat

@tpolecat I've reworked it and wanted to get an initial opinion from you if this direction seems okay to you. I'm jumping in the deep on PostGIS. Seems like being able to parse Well Known Binary (WKB) and/or Well Known Text (WKT) may be a solution.

cranst0n avatar Aug 30 '21 15:08 cranst0n

I will try to have a look at this soon. Thanks!

tpolecat avatar Aug 30 '21 18:08 tpolecat

So I'm almost apologetic about this, but the scope of this expanded far more than I originally intended. Some of this probably isn't necessary to take on in skunk and could be broken into its own library, but ultimately made it easier to create tests to build my own confidence. Plus is gave me an excuse to finally play around with cats-parse.

Definitely looking for feedback, the good, the bad and the ugly. Willing to massage this into a form you're comfortable with. Or even blow it up and take it back to basics.

cranst0n avatar Sep 03 '21 02:09 cranst0n

Very interesting! Is this PR still under consideration?

k0ala avatar Dec 07 '23 17:12 k0ala

If someone wants to get this in to a mergeable state again, for sure.

mpilquist avatar Dec 07 '23 17:12 mpilquist

Codecov Report

Attention: 18 lines in your changes are missing coverage. Please review.

Comparison is base (1482bd4) 84.45% compared to head (f6204fa) 85.59%.

Files Patch % Lines
modules/postgis/src/main/scala/geometry.scala 80.35% 11 Missing :warning:
modules/postgis/src/main/scala/ewkb/codecs.scala 93.65% 4 Missing :warning:
modules/postgis/src/main/scala/ewkt/parser.scala 97.46% 2 Missing :warning:
modules/postgis/src/main/scala/ewkb/domain.scala 98.03% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #534      +/-   ##
==========================================
+ Coverage   84.45%   85.59%   +1.14%     
==========================================
  Files         129      135       +6     
  Lines        1769     2034     +265     
  Branches      177      228      +51     
==========================================
+ Hits         1494     1741     +247     
- Misses        275      293      +18     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Dec 08 '23 20:12 codecov-commenter

Did my best to resurrect this. Appears to be passing CI. Let me know if there's any changes/enhancements you'd like to see.

cranst0n avatar Dec 08 '23 20:12 cranst0n

Thanks @cranst0n!

mpilquist avatar Jan 10 '24 13:01 mpilquist