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

feat: geospatial types support #4208

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

Conversation

Oreilles
Copy link
Contributor

@Oreilles Oreilles commented Sep 6, 2023

Fixes prisma/prisma#1798
Fixes prisma/prisma#2789

This PR implements support for database geometry types. I'll start this message by saying that, although I'd be very proud of making such a contribution to Prisma, I understand that the likeliness of such a massive PR to be accepted is pretty low due to the time I'd take to fully review. How did I end up there ? In the end of last year, I tried to get in touch with the team as I had some experience in the field, and also had the time to work on the feature - but by the time we managed to exchange emails I no longer had enough time to invest in working on this. Last month, I had time and remembered about this project, so I started browing the prisma repo - and realised the ORM core was not actually Typescript, but full Rust, which I had no previous experience with. I questioned my ability to be up to the task, but though: why not have a go at it ? Not having sufficient rust experience, I decided against getting back in touch with the Prisma team and just leverage this project as a learning experience. A month later, although it is very far from being production ready, I believe this branch might deserve some attention which is why I decide to release it. Again, apologies about the size. I worked on this as a learning experience and ended up getting quite ahead of myself.. heh.

Now for the meat of the PR, here's what it includes:

In the psl / query-engine:

  • Two new ScalarTypes: Geometry and GeoJSON. Both types refer to the same underlying database types, they only differ in the geometry serialization format the user wants to use: GeoJSON for GeoJSON, EWKT for Geometry. Why do we need both ? GeoJSON is the format most people wants to use, as they most likely use TypeScript and GeoJSON is the de-facto standard for manipulating geometries on the web. But Ewkt allows for richer information about the raw geometry: it can be used to serialize geometry in any CRS (when GeoJSON is limited by specification to WGS84), and can also represent more geometry types than geojson (CIRCULARSTRING , CURVEPOLYGON etc). As such, only EWKT can be used to safely & exhaustively read database geometries.
  • A new filtering condition: Geometry(Not)Within mapping to st_within
  • A new filtering condition Geometry(Not)Intersects mapping to st_intersects

In quaint:

  • Two new enum Values: Geometry and Geography, which both holds the same type (GeometryValue, a struct with a WKT string and an SRID i32 ). We might have needed only one, but MSSQL required making the two explicitly different.
  • A new function geom_from_text, that maps to the SQL/MM st_geomfromtext.
  • A new function geom_as_text, that maps to st_astext.
  • A new compare function geometry_is_valid that maps to st_isvalid
  • A new compare function geometry_is_empty that maps to st_isempty
  • A new compare function geometry(_not)_within that maps to st_within
  • A new compare function geometry(_not)_intersects that maps to st_intersects
  • A new compare function geometry_type(_not)_equals that maps to st_geometrytype(x) = ...

All in all, what works:

  • GeoJSON & EWKT I/O for CockroachDB, MySQL, MSSQL
  • GeoJSON & EWKT I/O for PostgreSQL (as long as PostGIS is installed and the public schema is exposed to Prisma)
  • GeoJSON & EWKT I/O for SQLite (as long as Spatialite is available and exposed with SPATIALITE_PATH)
  • GeoJSON I/O for MongoDB
  • Within and Intersects operators for all vendors except MongoDB (Mongo has the $geoWithin and $geoIntersects query operators, however they cannot be used with the current connector implementation, since those are Query operators wherehas Prisma uses Aggregation operators. Unfortunately, they're not compatible, and no geospatial filtering operators are available for aggregate queries in MongoDB.
  • Introspection / Schema description for all vendors (both directions)

What hasn't been fully implemented / properly tested:

  • Migrations (particularily casts)
  • Default value handling
  • Index type & operators handling (postgres only ?)

There is still a lot of work to do here, but I was quite happy to get the core features (introspection, IO and filtering) going and though it was worth sharing.

My intent with this PR is to show a possible implementation, start a discussion with the team and community about its viability, potential drawbacks and help making these features reach Prisma in a production ready state. Questions about the implementation details are welcome and encouraged. Please get in touch at aurele.nitoref at icloud if you'd like to discuss this more thoroughly!

@Oreilles Oreilles requested a review from a team as a code owner September 6, 2023 14:44
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jordiup
Copy link

jordiup commented Sep 7, 2023

Absolutely boss 🚀

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 7, 2023

CodSpeed Performance Report

Merging #4208 will not alter performance

Comparing Oreilles:feat/geometry (08c779a) with main (031f4d3)

Summary

✅ 11 untouched benchmarks

@janpio
Copy link
Contributor

janpio commented Sep 7, 2023

Amazing.

So that the tests are automatically run going forward:
Can you quickly do a PR to main where you change line 5 to [![Cargo docs](https://github.com/prisma/prisma-engines/actions/workflows/on-push-to-main.yml/badge.svg)](https://github.com/prisma/prisma-engines/actions/workflows/on-push-to-main.yml)? I can then merge that, and the repository will remember that we trust you and run the tests without me clicking a button. (And that button needs to be fixed anyway 😆)

@Oreilles Oreilles force-pushed the feat/geometry branch 5 times, most recently from adf09c6 to a588e12 Compare September 8, 2023 13:31
@Oreilles
Copy link
Contributor Author

Oreilles commented Sep 8, 2023

So there was quite a lot of things I had overlooked when testing on my machine..! I squashed all the fixes and we now have a mostly green test suite 👍

@janpio
Copy link
Contributor

janpio commented Sep 8, 2023

🚀

Two test failures left for now:

  1. Something with 1 PostgreSQL metrics test, 7 v.s 9 metrics recorded or something. Could your PR cause this? (We recently change something in metrics, so you might want to rebase (Update branch button in GH UI) to get the most recent version of that code).
  2. A lot of unhappiness in the Vitess 8 tests with it not being able to introspect the tables because of permission problems. Strange. That looks a bit ... weird, even flaky maybe? I retriggered a test run there. Maybe it just goes green.

PS: No need from our side to combine your PRs and force push, we only merge via "Squash and merge" which combines all the commits, and with the individual commits of the fixes it might be easier to understand what test failures where fixed where - or introduced.

@Oreilles
Copy link
Contributor Author

Oreilles commented Sep 8, 2023

So the two additional queries breaking the tests are due to:

CREATE EXTENSION IF NOT EXISTS postgis
https://github.com/Oreilles/prisma-engines/blob/a588e1296288b79df0204397a441f2b3d891b9d9/query-engine/connector-test-kit-rs/qe-setup/src/postgres.rs#L42

and

SELECT PostGIS_version()
https://github.com/Oreilles/prisma-engines/blob/a588e1296288b79df0204397a441f2b3d891b9d9/schema-engine/connectors/sql-schema-connector/src/flavour/postgres.rs#L628-L630

Both are needed to run the tests, I updated the counter to 9 to make the test succeed.
( Also rebased on main just in case... )

No clue about the Vitess issue though...

EDIT: I tried running the vitess tests on prisma:main on my machine and the result is the same, which makes me assume that this is not due to a code change in this PR...

EDIT: it's now all fixed 🙌

@di-sukharev
Copy link

How can I help to merge it?

@janpio
Copy link
Contributor

janpio commented Oct 19, 2023

Test it, figure out how it would fit into Prisma, let the author and us know what else is needed to make this a complete feature.

@okonon
Copy link

okonon commented Oct 23, 2023

@janpio @Oreilles thanks for all you do guys. I am at the point in my project to start introducing PostGIS and trying to figure out how to test this PR. I cloned @Oreilles feature branch and compiled debug targets of prisma-engines how do i tie it into existing prisma to run test migration/introspection ? thank you

@RafaelKr
Copy link

@okonon
Copy link

okonon commented Oct 25, 2023

@RafaelKr thanks a lot for that sorry i missed that section.
I was able to test introspection from existing database (prisma db pull):
image
Looks like it is introspecting correctly.

@beardo01
Copy link

beardo01 commented Nov 8, 2023

Adding a comment to voice support for this to be added to Prisma! Looking forward to seeing this merged; keep up the good work.

@toniopelo
Copy link

Definitely would love that to be merged as well 👍

@janpio janpio changed the base branch from main to janpio/oreilles-base-2 November 8, 2023 21:33
@janpio
Copy link
Contributor

janpio commented Nov 8, 2023

Sorry, but this will definitely not be merged as it is only part of the full feature that needs to be implemented.

But we are interested in bringing it there, so let's see what we can do here.

@janpio
Copy link
Contributor

janpio commented Nov 8, 2023

For now I created a branch based on the commit this was forked from, so that there are no conflicts and @Oreilles can keep committing if they want.

I also created a local branch of the code that runs all our CI (Buildkite is not run on external contributions), with the goal of publishing the engines to our infrastructure, so we can build a Prisma CLI and Client with them for further testing.

But unfortunately some tests are failing (results only internally visible): #4427
image
Turns out GitHub Actions do not run for these databases, and seems the current code has some problems - and that is blocking my plan to release these engine files :/

Random example of a SQLite failure, our of many ([2023-11-08T21:25:35Z] test result: FAILED. 622 passed; 970 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.73s):

[2023-11-08T21:25:35Z] thread 'writes::top_level_mutations::delete_many_relations::delete_many_rels::pm_c1_no_children' panicked at 'called `Result::unwrap()` on an `Err` value: Error { kind: QueryError(SqlInputError { error: Error { code: Unknown, extended_code: 1 }, msg: "no such function: InitSpatialMetaData", sql: "SELECT InitSpatialMetaData()", offset: 7 }), original_code: Some("1"), original_message: Some("no such function: InitSpatialMetaData") }', /root/build/query-engine/connector-test-kit-rs/qe-setup/src/sqlite.rs:9:65

MySQL 5.6 only fails 2:

[2023-11-08T21:35:48Z] failures:
[2023-11-08T21:35:48Z]     queries::filters::geometry_filter::geometry_filter_spec::geometric_comparison_filters
[2023-11-08T21:35:48Z]     writes::data_types::native_types::mysql::mysql::native_geojson_srid_geometry_types

5.7 only 1 even:

[2023-11-08T21:39:12Z] failures:
[2023-11-08T21:39:12Z]     writes::data_types::native_types::mysql::mysql::native_geojson_srid_geometry_types

MariaDB the same as 5.6:

[2023-11-08T21:31:19Z] failures:
[2023-11-08T21:31:19Z]     queries::filters::geometry_filter::geometry_filter_spec::geometric_comparison_filters
[2023-11-08T21:31:19Z]     writes::data_types::native_types::mysql::mysql::native_geojson_srid_geometry_types

Trying to get these tests to run on GitHub Actions now. But maybe you have speculative fixes @Oreilles?

@janpio janpio self-assigned this Nov 8, 2023
@Oreilles
Copy link
Contributor Author

Oreilles commented Nov 8, 2023

Interesting, as far as I can remember (almost) all tests were succeeding the last time we discussed the PR, and we did have tests for all MySQL versions... though I might remember wrong. Did you resolve merge conflicts when you merged main, which could explain tests failing afterwards ?

Anyway, I'll have a look as soon as possible. @janpio if that's something you can do, please send me the test cli commands to reproduce the failing tests.

Edit: nevermind, I had not seen that they were already mentioned in your message!

@Oreilles
Copy link
Contributor Author

Yes, the problem most certainly comes from the driver and not from quaint.
Maybe it has to do with the fact that String and Bytes are effectively treated the same way with the driver:

https://github.com/blackbeam/rust_mysql_common/blob/112dba546522600eb1937ddd6ff44d4f216ff11e/src/value/mod.rs#L76-L78

https://github.com/blackbeam/rust_mysql_common/blob/112dba546522600eb1937ddd6ff44d4f216ff11e/src/io.rs#L33-L35

But what's strange is that insertions and equality comparison are working just fine, so binary is accepted as a valid geometry representation. Furthermore, when testing it on a local MariaDB server, this query returns successfully:

SELECT ST_Intersects(
    ST_GeomFromText('POINT(1 2)', 4326),
    UNHEX('E61000000101000000000000000000F03F0000000000000040')
);

Meaning that a binary string is a valid argument to the ST_Intersects / ST_Within functions...

@janpio
Copy link
Contributor

janpio commented Nov 30, 2023

Maybe we have a mechanism to exclude MariaDB from running this test? Totally fine to move forward.

@Oreilles
Copy link
Contributor Author

Alright then, let's skip it for now.

@janpio
Copy link
Contributor

janpio commented Nov 30, 2023

🍾

#4507 led to an integration version 5.7.0-23.integration-geometry-e0f834dc25d3a4b4c821df9810dfd890335c0b86 which got a PR and tests here: prisma/prisma#22173 + https://github.com/prisma/prisma/actions/runs/7049075945?pr=22173 and they all passed

That means, that the engines are now fully working with your work (except the planetscale driver adapter, we can ignore that for now), and it is more about figuring out how to use this new functionality in prisma/prisma. To work on that, you can open a PR into this branch @Oreilles!

@salvinoto
Copy link

Is this PostGIS support for prisma?

@janpio
Copy link
Contributor

janpio commented Dec 1, 2023

@salvinoto Maybe, in the future. For now it is an experiment how a building block could look like that @Oreilles is working on.

@acidjazz
Copy link

acidjazz commented Aug 6, 2024

is anyone using this PR? what is left to get done here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Feature A PR That introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PostGIS/GIS support Geolocation/Spatial types support