Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Implement _geoBoundingBox filter #672

Closed
wants to merge 12 commits into from
Closed

Implement _geoBoundingBox filter #672

wants to merge 12 commits into from

Conversation

gmourier
Copy link
Member

@gmourier gmourier commented Oct 28, 2022 •
edited
Loading

Pull Request

Related issue

Fixes #2761

What does this PR do?

  • Implements _geoBoundingBox((lat, lng), (lat, lng)) filter

PR checklist

Please check if your PR fulfills the following requirements:

  • Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)?
  • Have you read the contributing guidelines?
  • Have you made sure that the title is accurate and descriptive of the changes?

Thank you so much for contributing to Meilisearch!

@gmourier gmourier changed the title Implement geo bounding box Implement _geoBoundingBox Oct 28, 2022
@gmourier gmourier changed the title Implement _geoBoundingBox Implement _geoBoundingBox filter Oct 28, 2022
@irevoire irevoire added enhancement New feature or request no breaking The related changes are not breaking (DB nor API) hacktoberfest-accepted labels Oct 28, 2022
@irevoire irevoire requested a review from loiclec October 31, 2022 10:47
loiclec
loiclec previously approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@loiclec loiclec left a comment

Choose a reason for hiding this comment

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

Great PR, thank you :)
I was a bit worried about some implementation details when crossing the line of 180deg longitude and about some validation logic, but it looks like everything is handled properly 😄

filter-parser/src/lib.rs Outdated Show resolved Hide resolved
milli/src/search/facet/filter.rs Show resolved Hide resolved
filter-parser/src/error.rs Outdated Show resolved Hide resolved
filter-parser/src/value.rs Outdated Show resolved Hide resolved
@gmourier gmourier requested review from loiclec and removed request for irevoire November 1, 2022 08:57
@curquiza curquiza mentioned this pull request Nov 16, 2022
Closed
3 tasks
@curquiza
Copy link
Member

@gmourier sorry, we took time to prioritize the review of your PR. Could you rebase it please?

@curquiza curquiza mentioned this pull request Nov 23, 2022
Closed
2 tasks
@curquiza
Copy link
Member

curquiza commented Jan 3, 2023

bors try

bors bot added a commit that referenced this pull request Jan 3, 2023
@bors
Try #672:
18a9252
@bors
Copy link
Contributor

bors bot commented Jan 3, 2023

try

Build failed:

@curquiza curquiza mentioned this pull request Jan 19, 2023
Merged
@curquiza
Copy link
Member

Closed in favor of meilisearch/meilisearch#3405

@curquiza curquiza closed this Jan 19, 2023
bors bot added a commit to meilisearch/meilisearch that referenced this pull request Feb 6, 2023
@bors @gmourier
@dureuill @irevoire
Merge #3405
2438d8f 
3405: Implement geo bounding box r=curquiza a=curquiza

Following meilisearch/milli#672 (work from `@gmourier)`

Fixes #2761

Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com>
Co-authored-by: Louis Dureuil <louis@meilisearch.com>
Co-authored-by: Tamo <tamo@meilisearch.com>
bors bot added a commit to meilisearch/meilisearch that referenced this pull request Feb 7, 2023
@bors @gmourier
@dureuill @irevoire
Merge #3405
97fd9ac 
3405: Implement geo bounding box r=irevoire a=curquiza

Following meilisearch/milli#672 (work from `@gmourier)`

Fixes #2761

Co-authored-by: Guillaume Mourier <guillaume@meilisearch.com>
Co-authored-by: Louis Dureuil <louis@meilisearch.com>
Co-authored-by: Tamo <tamo@meilisearch.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@loiclec loiclec Awaiting requested review from loiclec

Assignees
No one assigned
Labels
enhancement New feature or request no breaking The related changes are not breaking (DB nor API)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gmourier @curquiza @loiclec @irevoire