Skip to content

Add Polygon Type To Schema / PolygonContain to Query #3944

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

Merged
merged 16 commits into from
Jul 12, 2017

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Jun 21, 2017

#3942

Adds Mongo and PG support for Polygon Type and PolygonContain query

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #3944 into master will increase coverage by 0.08%.
The diff coverage is 97.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3944      +/-   ##
==========================================
+ Coverage   90.59%   90.68%   +0.08%     
==========================================
  Files         116      116              
  Lines        7793     7889      +96     
==========================================
+ Hits         7060     7154      +94     
- Misses        733      735       +2
Impacted Files Coverage Δ
...rc/Adapters/Storage/Mongo/MongoSchemaCollection.js 96% <100%> (+0.1%) ⬆️
src/Controllers/SchemaController.js 97.08% <100%> (+0.02%) ⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.89% <100%> (+0.82%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 96.37% <100%> (+0.21%) ⬆️
src/Adapters/Storage/Mongo/MongoTransform.js 84.26% <94.59%> (+0.76%) ⬆️
src/RestWrite.js 93.29% <0%> (-0.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d6b4f4...2eaca5d. Read the comment docs.

@dplewis dplewis changed the title Add Polygon Type To Schema Add Polygon Type To Schema / PolygonContains to Query Jun 21, 2017
@dplewis
Copy link
Member Author

dplewis commented Jun 21, 2017

@flovilmart Done, Let me know if I need more tests

@dplewis dplewis changed the title Add Polygon Type To Schema / PolygonContains to Query Add Polygon Type To Schema / PolygonContain to Query Jun 21, 2017
@dplewis
Copy link
Member Author

dplewis commented Jun 22, 2017

Mongo only throws errors if indexes are set.

I found that mongo throws an error for self-intersecting polygons. Is it ok if mongo just throws an error?

Loop is not valid

https://github.com/mongodb/mongo/blob/master/src/mongo/db/geo/geoparser.cpp#L223

I need to check for invalid loops if not users can set invalid polygons and could break once they add indexes later on.

Maybe we can add geoJson validator dependencies to parse-server and sdks. Eventually we will need it for LineString, MultiPoint, MultiLineString,MultiPolygon,GeometryCollection

@flovilmart
Copy link
Contributor

Is it possible to enforce the indexing when adding those columns types the same way we do with 2D indexes?

@dplewis
Copy link
Member Author

dplewis commented Jun 23, 2017

Since $geoIntersect doesn't require a index I can't catch an error to add one like $near.

I could check the schema and if Polygon Type add an index on object creation.

I'm going to write a few more test for postgres. It doesn't throw errors right now but It may for indexes.

@flovilmart flovilmart added this to the 2.6.0 milestone Jun 25, 2017
@dplewis
Copy link
Member Author

dplewis commented Jun 25, 2017

@flovilmart how does this look?

@flovilmart
Copy link
Contributor

I’ll have a deeper look after the 2.5.0 release :)

@flovilmart
Copy link
Contributor

Why the removal of the test,

@dplewis
Copy link
Member Author

dplewis commented Jul 7, 2017

I noticed when a mongoError gets thrown it retries the request a few times. Is that normal?

I added it back

@flovilmart
Copy link
Contributor

noticed when a mongoError gets thrown it retries the request a few times.

The JS SDK is retrying dumbly.

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Overall looks awesome @dplewis ! A few nits here and there before we get that one in!

@@ -25,6 +25,7 @@ function mongoFieldToParseSchemaField(type) {
case 'geopoint': return {type: 'GeoPoint'};
case 'file': return {type: 'File'};
case 'bytes': return {type: 'Bytes'};
case 'polygon': return {type: 'Polygon'};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit alignment

@@ -98,6 +99,7 @@ function parseFieldTypeToMongoFieldType({ type, targetClass }) {
case 'GeoPoint': return 'geopoint';
case 'File': return 'file';
case 'Bytes': return 'bytes';
case 'Polygon': return 'polygon';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit alignment

@@ -182,7 +182,8 @@ export class MongoStorageAdapter {

addFieldIfNotExists(className, fieldName, type) {
return this._schemaCollection()
.then(schemaCollection => schemaCollection.addFieldIfNotExists(className, fieldName, type));
.then(schemaCollection => schemaCollection.addFieldIfNotExists(className, fieldName, type))
.then(() => this.create2dsphereIndex(className, fieldName, type));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename create2dsphereIndex into an umbrella createIndexesIfNeeded? more explicit :)


create2dsphereIndex(className, fieldName, type) {
if (type && type.type === 'Polygon') {
const index = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use:

const index = {
  [fieldName]: '2dsphere'
};

and remove the next line

Copy link
Member Author

Choose a reason for hiding this comment

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

Works nicely. Thanks

@dplewis
Copy link
Member Author

dplewis commented Jul 11, 2017

Thanks for the feedback. Changes have been made

Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

LGTM!

@flovilmart flovilmart merged commit e6cc820 into parse-community:master Jul 12, 2017
@dplewis dplewis deleted the type-polygon branch July 12, 2017 05:19
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