Skip to content

Postgres: Support Postgis Point type and related functions. #5602

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

vanniktech
Copy link
Contributor

Still have a bunch of questions and this is not done yet. However, I did manage to publish the postgres dialect into my maven local and use it already. Since my query filtering is dynamic, I currently only need the Geometry Type since I construct the sql to query myself.


Is it correct that I when I make a change to the dialect, I have to ./gradlew publishToMavenLocal? This moves it into the build directory of the repository. So like in sqldelight/dialects/postgresql, I execute ./gradlew publishToMavenLocal and then I can run the integration tests?

How do I run the integration tests though? If I'm in the sqldelight/sqldelight-gradle-plugin/src/test/integration-postgresql directory and I try to run the tests using ./gradlew test, I'm getting:

PostgreSqlTest > testBooleans FAILED
    java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration
        at org.testcontainers.dockerclient.DockerClientProviderStrategy.getFirstValidStrategy(DockerClientProviderStrategy.java:232)
        at org.testcontainers.DockerClientFactory.getOrInitializeStrategy(DockerClientFactory.java:154)
        at org.testcontainers.DockerClientFactory.dockerHostIpAddress(DockerClientFactory.java:344)
        at org.testcontainers.containers.ContainerState.getHost(ContainerState.java:64)
        at org.testcontainers.containers.PostgreSQLContainer.getJdbcUrl(PostgreSQLContainer.java:100)
        at org.testcontainers.containers.JdbcDatabaseContainer.constructUrlForConnection(JdbcDatabaseContainer.java:309)
        at org.testcontainers.containers.JdbcDatabaseContainer.createConnection(JdbcDatabaseContainer.java:269)
        at org.testcontainers.jdbc.ContainerDatabaseDriver.connect(ContainerDatabaseDriver.java:140)
        at java.sql/java.sql.DriverManager.getConnection(DriverManager.java:683)
        at java.sql/java.sql.DriverManager.getConnection(DriverManager.java:253)
        at app.cash.sqldelight.postgresql.integration.PostgreSqlTest.<init>(PostgreSqlTest.kt:24)

What docker do I need to start? Could I also just point it to a local postgres installation on my Mac?

@griffio
Copy link
Contributor

griffio commented Jan 5, 2025

Still have a bunch of questions and this is not done yet. However, I did manage to publish the postgres dialect into my maven local and use it already. Since my query filtering is dynamic, I currently only need the Geometry Type since I construct the sql to query myself.

Is it correct that I when I make a change to the dialect, I have to ./gradlew publishToMavenLocal? This moves it into the build directory of the repository. So like in sqldelight/dialects/postgresql, I execute ./gradlew publishToMavenLocal and then I can run the integration tests?

How do I run the integration tests though? If I'm in the sqldelight/sqldelight-gradle-plugin/src/test/integration-postgresql directory and I try to run the tests using ./gradlew test, I'm getting:

PostgreSqlTest > testBooleans FAILED
    java.lang.IllegalStateException: Previous attempts to find a Docker environment failed. Will not retry. Please see logs and check configuration
        at org.testcontainers.dockerclient.DockerClientProviderStrategy.getFirstValidStrategy(DockerClientProviderStrategy.java:232)
        at org.testcontainers.DockerClientFactory.getOrInitializeStrategy(DockerClientFactory.java:154)
        at org.testcontainers.DockerClientFactory.dockerHostIpAddress(DockerClientFactory.java:344)
        at org.testcontainers.containers.ContainerState.getHost(ContainerState.java:64)
        at org.testcontainers.containers.PostgreSQLContainer.getJdbcUrl(PostgreSQLContainer.java:100)
        at org.testcontainers.containers.JdbcDatabaseContainer.constructUrlForConnection(JdbcDatabaseContainer.java:309)
        at org.testcontainers.containers.JdbcDatabaseContainer.createConnection(JdbcDatabaseContainer.java:269)
        at org.testcontainers.jdbc.ContainerDatabaseDriver.connect(ContainerDatabaseDriver.java:140)
        at java.sql/java.sql.DriverManager.getConnection(DriverManager.java:683)
        at java.sql/java.sql.DriverManager.getConnection(DriverManager.java:253)
        at app.cash.sqldelight.postgresql.integration.PostgreSqlTest.<init>(PostgreSqlTest.kt:24)

What docker do I need to start? Could I also just point it to a local postgres installation on my Mac?

Is it correct that I when I make a change to the dialect, I have to ./gradlew publishToMavenLocal?This moves it into the build directory of the repository. So like in sqldelight/dialects/postgresql, I execute ./gradlew publishToMavenLocal and then I can run the integration tests?

No need to publishToMavenLocal for changes to dialect or running integration tests

How do I run the integration tests though?

Something like - :sqldelight-gradle-plugin:dockerTest --tests "app.cash.sqldelight.dialect.DialectIntegrationTests.integrationTestsPostgreSql"

What docker do I need to start? Could I also just point it to a local postgres installation on my Mac?

Use Docker Desktop or Docker Daemon running locally - the integration tests use TestContainers see app.cash.sqldelight.postgresql.integration.async.PostgreSqlTest

Also, because PostGis is a third party extension - it would need to be installed when the integration tests run as PostGis types do not exist in the docker postgresql database.

@vanniktech
Copy link
Contributor Author

@AlecKazakova how should I continue with this? Is postgis first party support in postgres desired or not? I think it'd be cool.

@vanniktech
Copy link
Contributor Author

Also, because PostGis is a third party extension - it would need to be installed when the integration tests run as PostGis types do not exist in the docker postgresql database.

@griffio where can I do this? I've tried to find some kind of docker configuration file or something but came up emtpy.

@griffio
Copy link
Contributor

griffio commented Jan 6, 2025

Also, because PostGis is a third party extension - it would need to be installed when the integration tests run as PostGis types do not exist in the docker postgresql database.

@griffio where can I do this? I've tried to find some kind of docker configuration file or something but came up emtpy.

It's quite auto-magical -

The jdbc driver url will cause the TestContainer runtime on the classpath to load the docker image jdbc:tc:postgresql:latest - see https://java.testcontainers.org/modules/databases/jdbc/

You must have a Docker daemon running otherwise you get the Previous attempts to find a Docker environment failed

Also you may be able to load the extension here ->

@vanniktech vanniktech force-pushed the postgres-postgis branch 4 times, most recently from ddbb0a1 to 42a02b2 Compare January 11, 2025 10:46
@vanniktech
Copy link
Contributor Author

Okay so this thing is finally green now with all the required tests for the newly added syntax. Without @griffio's help I would have never come this far (because I've never touched those grammar files before).

The Sqldelight postgres dialect now supports Postgis; not the entire syntax is supported but everything you need for inserting points and querying by them. That allows you to basically have locations stored in the database and efficiently query for distances from one location to another. Adding more things is trivial now, since the foundation has been laid out.

I'd argue for having this included in the default postgres grammar because:

  • the added grammar is trivial
  • it's really cool to get out of the box support for this with the sqldelight postgres dialect
  • we don't need to do special dances to integration test this since there's a docker image with postgis!
  • the overload of having this as a separate dialect and published as a separate artifact is way higher than just me being responsible for this now

@vanniktech vanniktech marked this pull request as ready for review January 11, 2025 11:53
@vanniktech vanniktech requested a review from griffio January 11, 2025 11:54
@vanniktech vanniktech changed the title Postgres: Preliminary Postgis support. Postgres: Support Postgis Point type and related functions. Jan 11, 2025
@@ -244,6 +250,12 @@ class PostgreSqlTypeResolver(private val parentResolver: TypeResolver) : TypeRes
is PostgreSqlDoubleColonCastOperatorExpression -> {
(argument.parent.parent as SqlExpr).postgreSqlType()
}
is PostgreSqlGeometrySetsridFunctionStmt -> {
IntermediateType(INTEGER)
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be changed from IntermediateType(INTEGER) -> PostgreSqlType.INTEGER as the parent resolver maps INTEGER to LONG and the function fails at runtime as Postgresql ST_SetSRID as it only takes Int


insertMakePoint:
INSERT INTO locations (name, point)
VALUES ('New York', ST_SetSRID(ST_MakePoint(:x::REAL, :y::REAL), :srid::INTEGER));
Copy link
Contributor

Choose a reason for hiding this comment

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

Test with VALUES ('New York', ST_SetSRID(ST_MakePoint(:x, :y), :srid)); This checks the resolver works without casts - as per fix above changing from LONG -> INT

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change this:

Screenshot 2025-01-13 at 22 49 55

together with:

Screenshot 2025-01-13 at 22 50 03

but this fails for me:

> A failure occurred while executing app.cash.sqldelight.gradle.SqlDelightTask$GenerateInterfaces
   > Failed to compile PostgreSqlInsertStmtImpl(INSERT_STMT): [] :
     INSERT INTO locations (name, point)
     VALUES ('New York', ST_SetSRID(ST_MakePoint(:x::REAL, :y::REAL), :srid))

Copy link
Contributor

@griffio griffio Jan 13, 2025

Choose a reason for hiding this comment

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

I will have a look - with the bind resolver the casting is not required as sets to variables to double for ST_MakePoint and int for ST_SetSRID

INSERT INTO locations (name, point)  
VALUES ('New York', ST_SetSRID(ST_MakePoint(:x, :y), :srid))

This compiles

Copy link
Contributor

Choose a reason for hiding this comment

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

Until we find a better way with function argument resolution - It's best to avoid mixing casts and non casted variables for the ST_* functions - either use all casts or none

Using casts turn the statement into an anonymous FunctionExpression and needs a casted variable to tell it the type - hence the error Cannot bind unknown types or null


insertMakePointM:
INSERT INTO locations (name, pointM)
VALUES ('New York', ST_SetSRID(ST_MakePointM(:x::REAL, :y::REAL, :m::REAL), :srid::INTEGER));
Copy link
Contributor

@griffio griffio Jan 13, 2025

Choose a reason for hiding this comment

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

After testing locally - I think that all the ::REAL must be ::FLOAT

50 | New York | 0101000020E610000000000040628052C0000000E0225B4440 - inserted with REAL
51 | New York | 0101000020E6100000AAF1D24D628052C04260E5D0225B4440 - inserted with FLOAT

It is missing the precision and would cause tests to be invalid

REAL in PostgreSQL is a 4-byte single-precision floating-point number
FLOAT without precision specified defaults to 8-byte double-precision floating-point number
The first insert using REAL (4 bytes) has less precision, which is why you see the coordinates rounded/truncated in the first row. The second insert using FLOAT (8 bytes) maintains more decimal precision, visible in the second row's coordinates.

For geospatial coordinates where precision matters, using FLOAT (double precision) is the better choice since it provides more accurate coordinate storage. This explains why you see slightly different binary representations of the point in the database

Confusingly - REAL is mapped to double in the resolver type code and would work if set by the resolver
When using the cast ::REAL even though JDBC set it as double the value is without the full precision

}

with(database.postgisQueries.selectLocationByDistanceHardcoded(distanceMeters = 0.0).executeAsList()) {
assertThat(size).isEqualTo(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be size 1 but is zero size because of the precision issue with REAL vs FLOAT

Copy link
Contributor

@griffio griffio left a comment

Choose a reason for hiding this comment

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

See comments for some issues

Copy link

@swankjesse swankjesse left a comment

Choose a reason for hiding this comment

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

Looks legit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants