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

Added geoNear aggregate stage. #242

Merged
merged 3 commits into from
Jun 19, 2020
Merged

Conversation

Andrewangeta
Copy link
Collaborator

Description

This PR adds support for the $geoNear aggregate stage

Motivation and Context

This adds more missing API to aggregate stages.

Checklist:

  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

@Andrewangeta Andrewangeta requested a review from Joannis May 11, 2020 19:14
@Andrewangeta
Copy link
Collaborator Author

Andrewangeta commented May 11, 2020

Also important to Note that we have 2 separate replaceRoot aggregate stages one prefixed with the $ and one that's not.

@Joannis
Copy link
Member

Joannis commented May 12, 2020

That is problematic API

@Andrewangeta
Copy link
Collaborator Author

The replaceRoot yea

geoNear["minDistance"] = minDistance
}

if let key = key {
Copy link
Member

Choose a reason for hiding this comment

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

You can assign a String? directly to a Document. If it's nil it won't be inserted.

Copy link
Member

Choose a reason for hiding this comment

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

Should simplify this code a LOT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah that’s good to know. Thanks for the tip. Will update.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved

if useLegacy {
geoNear["near"] = coordinates.makePrimitive()
} else {
geoNear["near"] = ["type": "Point", "coordinates": coordinates] as Document
Copy link
Member

Choose a reason for hiding this comment

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

I assume that geoNear only supports a point, right?
Looking at the API a second time makes me wonder whether useLegacy should be a boolean option.
Also, coordinates is either 2 or 3 coordinates if I remember correctly. In which case the [Double] is fine. But I'm wondering if that API could be changed to be less prone to error. A good example of an error that I keep making is the order of latitude and longitude.

I also wonder whether uniqueDocs should be written out fully as uniqueDocuments, irregardless of MongoDB's documentation.

These are not decisions, but something I'd wish to discuss before merging for sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also updated these.

@Andrewangeta Andrewangeta requested a review from Joannis June 18, 2020 22:17
Copy link
Member

@Joannis Joannis left a comment

Choose a reason for hiding this comment

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

Nothing to add anymore

@Andrewangeta Andrewangeta merged commit caa59c3 into master/6.0 Jun 19, 2020
@Joannis Joannis deleted the feature/geo-near-aggregate branch September 3, 2023 13:16
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