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

Fix the range of latitude/longitude #193

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

Fix the range of latitude/longitude #193

wants to merge 2 commits into from

Conversation

Plopix
Copy link

@Plopix Plopix commented Apr 8, 2015

We need to be worldwide ;-)

@Plopix
Copy link
Author

Plopix commented Apr 8, 2015

In the indexation class, the longitude is first https://github.com/ezsystems/ezfind/blob/master/classes/ezfsolrdocumentfieldgmaplocation.php#L46

@bdunogier
Copy link
Member

I guess that it was wrong all along ?

@andrerom
Copy link
Contributor

andrerom commented Apr 8, 2015

+1, review ping @pspanja @paulborgermans

@Plopix
Copy link
Author

Plopix commented Apr 9, 2015

@bdunogier Only the ezfind extendedattribute filter was wrong. For me the fact that the ezfsolrdocumentfieldgmaplocation indexes long/lat is not an issue if the extended filter works correctly with.

Additionnaly, I think it could be a bad idea to change the order (lon,lat) in the ezfsolrdocumentfieldgmaplocation because it could be an issue for existing project. (they will have to re-index)

Then this fix should be enough but it's just my opinion ;)

Thx!

@Plopix
Copy link
Author

Plopix commented Apr 9, 2015

To my understanding I find out that the sort was also wrong, the geodist function need to know the field/origin.

According to the documentation: https://cwiki.apache.org/confluence/display/solr/Spatial+Search

geodist is a distance function that takes three optional parameters: (sfield,latitude,longitude). You can use the geodist function to sort results by distance or score return results.

I'm not a Solr expert and the feedback of @paulborgermans or @pspanja would be great.

But without the 3 arguments, the results was not correctly sorted.

Thx!

@paulborgermans
Copy link
Contributor

In general, geographical support in ez find is lagging behind Lucene/Solr capabilities and the whole datatype handling should be refactored ...

That said, my opinion on this pull request:

  • the indexing indeed makes the wrong order, so the error should be fixed there, not repeating the "error" here, even though that means the need for re-indexing
  • either the use of the query parameters sfield, pt are to be used or as arguments to geodist() but not both
  • using the filter is actually also a workaround for a missing parameter in schema.xml

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

Successfully merging this pull request may close these issues.

4 participants