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

_geoBoundingBox filter returns no results when lat and lng are indexed as strings #3973

Closed
martinmaillard opened this issue Aug 4, 2023 · 6 comments · Fixed by #3986
Closed
Labels
bug Something isn't working as expected filtering Everything related to the filtering features geo search Everything related to Geo search stuff v1.3.1 PRs/issues solved in v1.3.1 released on 2023-08-14
Milestone

Comments

@martinmaillard
Copy link

martinmaillard commented Aug 4, 2023

Describe the bug

When indexing documents with a _geo key containing lat and lng as string values, the _geoBoundingBox filter never returns any resuls. Reindexing the same documents with lat and lng as float values solves the issue.

I'm reporting this as a bug because the documentation states that string values are valid for lat and lng.

When using JSON and NDJSON, _geo must contain an object with two keys: lat and lng. Both fields must contain either a floating point number or a string indicating, respectively, latitude and longitude:

Note that _geoRadius works as expected.

To Reproduce
Steps to reproduce the behavior:

  1. Index the following documents: [{"id": 1, "_geo": {"lng": "6", "lat": "46"}}, {"id": 2, "_geo": {"lng": 6, "lat": 46}]
  2. Query the index with the following filter: {"filter": "_geoBoundingBox([47, 7], [44, 4])"}
  3. Only the document with id=2 is returned

Expected behavior
Both documents should be returned

Meilisearch version:
Tested with 1.2 and 1.3


Comment from the team:

This issue was an indexing issue. The fix will be shipped in a patch version which means you don't need a dump to use it.
For all the new documents you send, it'll take effect immediately BUT, if your database already contains geo documents with their coordinates as string, you won't be able to query them until you re-index them by removing and then adding again _geo as a faceted attribute (i.e.: removing it from both the sortable and filterable attributes).
You can also delete and send back all the affected documents if that's easier for you.

@irevoire irevoire added the support Issues related to support questions label Aug 7, 2023
@irevoire
Copy link
Member

irevoire commented Aug 7, 2023

Hey @martinmaillard.

It seems like you mixed up a bit the order of your latitude/longitude.

As you can see in the documentation, the _geoBoundingBox function must receive the latitude and then the longitude, i.e.: _geoBoundingBox([{lat}, {lng}], [{lat}, {lng}]).
And as stated later:

The first array indicates the geographic coordinates of the top right corner of the rectangular area. The second array indicates the coordinates of the bottom left corner of the rectangular area.

Following these indications, this means the top right corner should be expressed as [47, 7], and your bottom left corner as [44, 6].
Finally, for your documents, the final filter you wanted to send was this one: { "filter": "_geoBoundingBox([47,7], [44,6])" }.

@irevoire irevoire added geo search Everything related to Geo search stuff filtering Everything related to the filtering features labels Aug 7, 2023
@martinmaillard
Copy link
Author

Hi @irevoire,

Sorry, I don't understand your message. Where did I mix up lat and lng? The filter you wrote looks exactly like mine, except it has a lng of 6 instead of 4, which shouldn't change anything in this case.

Am I missing something?

@irevoire irevoire added bug Something isn't working as expected and removed support Issues related to support questions labels Aug 7, 2023
@irevoire
Copy link
Member

irevoire commented Aug 7, 2023

Oops, yes, you’re right. Sorry, I totally misunderstood your issue 🤦

The bug is related to the fact that the _geoBoundingBox is kind of a hack around the filtering system of meilisearch.
But since the range doesn’t work on strings with the meilisearch filter, the _geoBoundingBox can't work with the strings either.
This issue is not trivial to fix.

@martinmaillard
Copy link
Author

Ok, I see. Thanks for looking into it :)

Maybe the fix is to change the documentation of _geo to make it clear that lat and lng can't be strings at the moment. That would probably save time for some people.

@irevoire irevoire added spike A spike is where we need to do investigation before we could estimate the effort to fix and removed spike A spike is where we need to do investigation before we could estimate the effort to fix labels Aug 8, 2023
@irevoire
Copy link
Member

irevoire commented Aug 8, 2023

Hey @martinmaillard, just to keep you updated.
I chatted about this issue with the team, and we found a cool solution without any downside. It's implemented and will be released in the next patch version of meilisearch (probably by the end of the week or next week).
Also, I’ll update your initial message so it's easier for the rest of the team to follow what's going on!
Thanks for the report 🎉

@curquiza curquiza linked a pull request Aug 8, 2023 that will close this issue
@curquiza curquiza added this to the v1.3.1 milestone Aug 8, 2023
meili-bors bot added a commit that referenced this issue Aug 9, 2023
3986: Fix geo bounding box with strings r=ManyTheFish a=irevoire

# Pull Request

When sending a document with one geofield of type string (i.e.: `{ "_geo": { "lat": 12, "lng": "13" }}`), the geobounding box would exclude this document.

This PR fixes this issue by automatically parsing the string value in case we're working on a geofield.

## Related issue
Fixes #3973

## What does this PR do?
- Automatically parse the facet value iif we're working on a geofield.
- Make insta works with snapshots in loops or closure executed multiple times. (you may need to update your cli if it panics after this PR: `cargo install cargo-insta`).
- Add one integration test in milli and in meilisearch to ensure it works forever.
- Add three snapshots for the dump that mysteriously disappeared I don't know how


Co-authored-by: Tamo <tamo@meilisearch.com>
@curquiza
Copy link
Member

curquiza commented Aug 10, 2023

Fixed by #3986 without downside
Will be released today in v1.3.1
Thank for the report and the time, it's been a while we have issues like this and we try to understand the problem. Thank you! 🙏

@meili-bot meili-bot added the v1.3.1 PRs/issues solved in v1.3.1 released on 2023-08-14 label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working as expected filtering Everything related to the filtering features geo search Everything related to Geo search stuff v1.3.1 PRs/issues solved in v1.3.1 released on 2023-08-14
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants