Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PostGIS support. #534

Merged
merged 3 commits into from
Jan 10, 2024
Merged

Add PostGIS support. #534

merged 3 commits into from
Jan 10, 2024

Conversation

cranst0n
Copy link
Contributor

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 😎.

@cranst0n cranst0n changed the title Add PostGIS support (#116). Add PostGIS support. #116 Aug 27, 2021
@cranst0n cranst0n changed the title Add PostGIS support. #116 Add PostGIS support. Aug 27, 2021
@tpolecat
Copy link
Member

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

@cranst0n
Copy link
Contributor Author

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.

@tpolecat
Copy link
Member

Also a compile error for 2.12, looks like.

build.sbt Outdated
Comment on lines 162 to 164
lazy val postgis = crossProject(JVMPlatform, JSPlatform)
.crossType(CrossType.Pure)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay!!

@cranst0n
Copy link
Contributor Author

cranst0n commented Aug 30, 2021

@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.

@tpolecat
Copy link
Member

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

@cranst0n cranst0n marked this pull request as draft September 2, 2021 20:47
@cranst0n
Copy link
Contributor Author

cranst0n commented Sep 3, 2021

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.

}

def coordinate(implicit byteOrdering: ByteOrdering, ewkb: EWKBType): Scodec[Coordinate] = {
// TODO: Why is this val assignment necessary? Scala 3 complains without it:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, this issue seems to go away when using scala 3.0.2 instead of 3.0.1

@cranst0n cranst0n marked this pull request as ready for review September 8, 2021 12:07
@k0ala
Copy link

k0ala commented Dec 7, 2023

Very interesting! Is this PR still under consideration?

@mpilquist
Copy link
Member

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

@codecov-commenter
Copy link

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 ⚠️
modules/postgis/src/main/scala/ewkb/codecs.scala 93.65% 4 Missing ⚠️
modules/postgis/src/main/scala/ewkt/parser.scala 97.46% 2 Missing ⚠️
modules/postgis/src/main/scala/ewkb/domain.scala 98.03% 1 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cranst0n
Copy link
Contributor Author

cranst0n commented Dec 8, 2023

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.

@mpilquist mpilquist merged commit 6696bcc into typelevel:main Jan 10, 2024
10 checks passed
@mpilquist
Copy link
Member

Thanks @cranst0n!

mpilquist added a commit that referenced this pull request Jan 14, 2024
Backport #534 (PostGIS support) to series/0.6.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants