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

Sorting #1

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

Sorting #1

wants to merge 5 commits into from

Conversation

OKNoah
Copy link

@OKNoah OKNoah commented Jun 29, 2018

No description provided.

mojodna and others added 2 commits June 28, 2018 06:56
This includes support for both geometry and geography columns as well as
GiST indices (used when passing the `spatial: true`). Client
representations use GeoJSON (existing MySQL and MS SQL drivers use WKT
(well-known text)) for compatibility with geospatial libraries such as
Turf, JSTS, etc.
@mojodna
Copy link
Owner

mojodna commented Jun 29, 2018

Good call on looking into how custom ORDER BYs would work...

I think adding distance (etc.) fields is a generally bad idea (sorry!), since it starts us down the path of needing to add custom support for every single predicate (not just spatial) that could potentially be used in sorting a result set.

My preference would be for something along the lines of this, where ORDER BY clauses can be bound to query parameters:

const posts1 = await connection.manager
    .createQueryBuilder(Post, "post")
    .orderBy({
      "ST_Distance(post.geog, ST_GeometryFromGeoJSON(:origin))": {
          origin, // a GeoJSON object; serialized to a string by typeorm (not because of any column information, but because it is an object)
          order: "ASC"
        }
    })
    .getMany();

This would allow for PostGIS-facilitated geometry operations, e.g. ST_Distance(post.geog, ST_Buffer(ST_GeometryFromGeoJSON(:origin), 0.2)), or sorting based on joined fields, e.g. ST_Distance(post.geog, states.centroid). It would also work for non-PostGIS operations (and be generalizable to other databases).

Unrelated: why are you using a geography field in place of geometry?

@OKNoah
Copy link
Author

OKNoah commented Jun 29, 2018

@mojodna My opinion: the user shouldn't have to know PostGIS to sort things by distance. Just the same as they don't have to know other SQL to use TypeORM.

Regarding geography vs. geometry, I'm not sure I know the exact difference. My use case is measuring distance between two geographical locations, so I assumed geography was best. Perhaps there needs to be simply .orderByDistance? Would appreciate your thoughts.

@mojodna
Copy link
Owner

mojodna commented Jun 29, 2018

I see where you're coming from. My feeling is that it leads down a long road where sorting by distance isn't specifically privileged. Users shouldn't have to know PostGIS to sort things by area, length, similarity, number of vertices... If we can provide clean ways for people to abstract these types of sorting criteria and provide documentation for e.g. distance (with the proper PostGIS function incantations), that might be best.

geography does sound right for your use-case, since it allows meters to be used in place of degrees for unit measurements. The reason it's not the typical go-to is that PostGIS supports geography in substantially fewer functions than geometry: https://postgis.net/docs/using_postgis_dbmanagement.html#PostGIS_Geography

@OKNoah
Copy link
Author

OKNoah commented Jun 29, 2018

@mojodna Unfortunately I don't know quite enough about PostGIS to come up with an API for it all. The most common requests I see are for ordering by distance, and finding results within a certain area. Maybe it's justifiable to add functions/arguments for those two use cases? Anything else would be left up to queries like yours. Just a thought. But I may use your fork and above example myself for now.

@mojodna
Copy link
Owner

mojodna commented Jun 29, 2018

Yep, that'd make sense, I'd just want to draw a line for maintainers to be able to say "no" to an unreasonable proliferation of criteria while providing sufficient escape hatches to do weird, unexpected things.

I'll see if I can sketch something out that allows parameters to be bound to ORDER BY clauses.

@OKNoah
Copy link
Author

OKNoah commented Jun 29, 2018

.orderBy({ "ST_Distance(post.geog, ST_GeometryFromGeoJSON(:origin))": { origin, // a GeoJSON object; serialized to a string by typeorm (not because of any column information, but because it is an object) order: "ASC" } })

I don't think you can do :origin in .orderBy currently :/

@mojodna
Copy link
Owner

mojodna commented Jun 29, 2018

I didn't think so either and started down the path of adding parameters to OrderByCondition. However, when using .setParameters() (the generalized function matching .where()'s 2nd arg), it looks like it does work!

const posts1 = await connection.manager
    .createQueryBuilder(Post, "post")
    .where("ST_Distance(post.geom, ST_GeomFromGeoJSON(:origin)) > 0")
    .orderBy({
        "ST_Distance(post.geom, ST_GeomFromGeoJSON(:origin))": {
            order: "ASC",
            nulls: "NULLS FIRST"
        }
    })
    .setParameters({ origin: JSON.stringify(origin) })
    .getMany();
const posts2 = await connection.manager
    .createQueryBuilder(Post, "post")
    .orderBy("ST_Distance(post.geom, ST_GeomFromGeoJSON(:origin))", "DESC")
    .setParameters({ origin: JSON.stringify(origin) })
    .getMany();

Having to stringify geometry objects and wrap them in ST_GeomFromGeoJSON is a bit janky, so I'm definitely on board with introducing some mix-in functions for QueryBuilder like .orderByDistanceFrom(geom) that calls .addOrderBy() appropriately.

I looked into automatically stringifying objects when passed to setParameters, but there's no way to distinguish them from Dates. Automatically adding ST_GeomFromGeoJSON works when inserting (because we know the column type), but since these are essentially "virtual" columns, we don't know enough about them.

@OKNoah
Copy link
Author

OKNoah commented Jun 29, 2018

Thanks, did not know about .setParameters. New to TypeORM if you can't tell.

@mojodna
Copy link
Owner

mojodna commented Jun 29, 2018

Same here ;-) It's been good exploring it with you!

@mojodna
Copy link
Owner

mojodna commented Jun 29, 2018

Have you figured out how to npm install a fork of a TypeScript project? This failed for me:

npm install --save "github:mojodna/typeorm#postgis"

@OKNoah
Copy link
Author

OKNoah commented Jun 30, 2018

It doesn't build when you do that. I had to change the package.json and publish it.

https://github.com/OKNoah/typeorm/blob/e65ef1bcb30184e1c618e4870fa7701310a49064/package.json

Note I put a prepublish script, set private to false and added a main

It's on npm as typeorm-spatial, which includes my distance thing.

@OKNoah
Copy link
Author

OKNoah commented Jul 1, 2018

@mojodna Hope you're enjoying your weekend, you deserve to on account of your capable hands' work on this!

I have found another interesting project that goes hand-in-hand with PostGIS/geog/geom in postgres: https://github.com/pramsey/pgsql-postal, which brings libpostal functions to pgsql. Those functions are things like postal_normalize('412 first ave, victoria, bc') which would result in data like 412 1st avenue victoria british columbia through a field that looks like (I think)

{"city": "victoria", "road": "first ave", "state": "bc", "house_number": "412"}

Caveat: It requires a few GB of (machine learning?) data and high RAM usage (possibly negated by including postgres config stuff). But where one or both of us to create install arguments, or env variables to disables or enable this functionality, it could be merged. Should that not be possible, would co-maintaining (you should take ownership) of typeorm-spatial which includes these feature, be of any interest?

Combined, it makes an even greeter suite of spatial and address functions right within postgres.

@mojodna
Copy link
Owner

mojodna commented Jul 2, 2018

libpostal is pretty sweet. Fortunately, Paul's bindings expose it as a set of Postgres functions that return standard types, so I don't think there's a need to do anything within typeorm to specifically support it.

As for maintaining typeorm-spatial, I'm hoping we can get the changes merged in ASAP (I pushed a few more today that address updates, constraints, and table migrations) so it can be mainlined with no need to maintain a fork (I'm already a bit over-stretched on project maintenance).

@OKNoah
Copy link
Author

OKNoah commented Jul 2, 2018

@mojodna I'm sure it will make it into a release soon, it's working well for me. If this isn't a maxim already, it should be: "you you want to get a busy project maintained, get a busy project maintainer to maintain it".

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