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

[WIP] Add support for Postgis Geometry data types #130

Closed
wants to merge 23 commits into from

Conversation

bettdouglas
Copy link

@bettdouglas bettdouglas commented Jun 8, 2020

This PR enables support of postgis geometries which is provided via an extension.
PostGIS is an extension that enables performing storage of spatial objects, enables manipulation of the objects as well as doing spatial analytics like finding features which are inside a specific region, e.g find all shops inside New York, find 20 closest shops near me, etc. PostGIS Webiste

This PR hopes to add support for this datatype.
It requires a dependency on dart_jts which has ability to handle postgis data types as Plain Dart Objects.

Since postgis is an extension, the datatype will not be available in databases where the extension has not been created/activated via CREATE EXTENSION postgis;

Due to that, there are some caveats to implementing this library here though as discussed by @isoos in #120 since the datatype's oid is dynamic, not static like other officially supported types.

A workaround has been suggested here #120 (comment)

This is the issue which needs reviewing as well as modifying the implementation in order to get the oid of the object as well as testing.

Kindly review. Looking foward to your feedback

@bettdouglas bettdouglas changed the title Add support for Postgis Geometry data types [WIP] Add support for Postgis Geometry data types Jun 9, 2020
@bettdouglas bettdouglas closed this Jun 9, 2020
@bettdouglas bettdouglas reopened this Jun 9, 2020
@bettdouglas bettdouglas closed this Jun 9, 2020
@bettdouglas bettdouglas reopened this Jun 9, 2020
utf8.encode(
value.toText(),
),
); //TODO: Convert to Uint8List. Need help here. Using toText() in raw sql inserts it well.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please help me to understand this TODO: it seems that it is already implemented. Is there something missing?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, this should have been added to the PostgresTextEncoder. I've added it in the last commit

Copy link
Author

Choose a reason for hiding this comment

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

b0c853a adds conversion of geometry objects from dart to string using toText() which returns the text in which postgis can understand

uuid
uuid,

/// Must be a [List] of [int] currently in Ewkb(Extended Well-Known Binary) format
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please link to the spec/documentation of this format.

Copy link
Author

@bettdouglas bettdouglas Jun 9, 2020

Choose a reason for hiding this comment

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

https://postgis.net/docs/using_postgis_dbmanagement.html#OpenGISWKBWKT
I'll add it into the code and push the change

Copy link
Author

Choose a reason for hiding this comment

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

e23a3c6 I hope that helps

@@ -305,5 +326,7 @@ class PostgresBinaryDecoder extends Converter<Uint8List, dynamic> {
1184: PostgreSQLDataType.timestampWithTimezone,
2950: PostgreSQLDataType.uuid,
3802: PostgreSQLDataType.json,
46315: PostgreSQLDataType.geometry, /// TODO: Oid Changes on different databases after running `CREATE EXTENSION postgis`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should go into a separate PR. Do you mind if I create a stub and you can review it if it will work here?

Copy link
Author

@bettdouglas bettdouglas Jun 9, 2020

Choose a reason for hiding this comment

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

I'd really appreciate the assistance on that. Sure that would be a good way to progress

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have an idea for implementing this with a kind of pluggable interface, which not only solves this obstacle, but also it could make sure we don't necessarily bind the postgres package to an API that is changing under us (package:dart_jts). Please be a bit patient while I do the necessary preparation work, I'll keep you posted on it.

Copy link
Author

Choose a reason for hiding this comment

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

I'd love to see how that will work. Thanks for your assistance

Copy link
Collaborator

Choose a reason for hiding this comment

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

This rewrite / internal refactoring seems to be a much harder problem than I first anticipated. We need to define custom encoders and decoders, and pass them through every little place that may use them. Not sure when I will be ready with that... :(

Copy link
Author

@bettdouglas bettdouglas Jul 3, 2020

Choose a reason for hiding this comment

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

Okay, I understand. I made some hacky solution where I send a query to read the datatypes when opening the connection,it's working well and all the other tests still work. If you'd love to check it out, and give some feedback...its on oid-connection-test branch.
The major change is that the typeMap is made dynamic and it has to be injected into the Query object.

I've used it on a variety of images including docker images and they seem to be working okay. The only caveat is that if someone run's CREATE extension custom-extension after connecting to the database, the typeMap will need to be updated to reflect this as the typeMap will not have the new types

Some tests are failing though.

Copy link
Author

Choose a reason for hiding this comment

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

here's a screenshot of failing tests and passing ones since Travis CI cannot run

https://imgur.com/Zj8qqTa

Copy link
Author

@bettdouglas bettdouglas Jul 5, 2020

Choose a reason for hiding this comment

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

@isoos let me just close this into separate PR.s

@@ -305,5 +326,7 @@ class PostgresBinaryDecoder extends Converter<Uint8List, dynamic> {
1184: PostgreSQLDataType.timestampWithTimezone,
2950: PostgreSQLDataType.uuid,
3802: PostgreSQLDataType.json,
46315: PostgreSQLDataType.geometry, /// TODO: Oid Changes on different databases after running `CREATE EXTENSION postgis`
46971: PostgreSQLDataType.geometry /// `SELECT oid, typarray FROM pg_type WHERE typname in ('geometry','geography')`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@bettdouglas: If you have two databases on the same server, you create the postgis extension on both, will this be one entry or two for 'geometry'?

Copy link
Author

Choose a reason for hiding this comment

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

So, postgis has two general types which is 'geography' and 'geometry'. When you create the extension on one database, the two types('geography' and 'geometry') will be registered to the pg_type table. Running the create extension postgis again replicates the same behavior but doesn't affect types registered in other databases.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In other words, it will have two new records per DB, and their OIDs will be different. It does complicate things, as we'll need a per-DB query (or another way to detect the type, which won't be cheap).

Copy link
Author

Choose a reason for hiding this comment

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

That would be kind of expensive,but i assume since the behaviour is different per database, and since every connection from dart has to specifies a specific database, the on connection query to get oids would just include one additional type 'geography' apart from 'geometry', or maybe I'm getting this wrong. or How were you planning to implement this?

@bettdouglas
Copy link
Author

Closed due to #130 (comment) to separate the concerns

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.

2 participants