Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

No, not without custom code. It's kind of hard to bring Solr to return this.

das-peter’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
3 KB

The attached patch provides together with #1812528: Add proximity/distance information to search results a way to add / calculate the distance to the search results.
If solr 3 is used the coordinates instead the actual distance is added to the search result. We could calculate the distance on our own with those coordinates but I think that belongs into the problem space of the location module (and it's already solved there).
Do we have somewhere a documentation how the correct implementation of search_api_location support has to look like?

das-peter’s picture

Title: proximity field on views items » Solr: Add proximity/distance information to search results

More speaking title.

das-peter’s picture

drunken monkey’s picture

Status: Needs review » Needs work

Thanks, great work!
As already noted in #1812528-15: Add proximity/distance information to search results, this works perfectly but I think the underlying mechanism still should/could be improved.

Do we have somewhere a documentation how the correct implementation of search_api_location support has to look like?

Yes, in the README.txt file of the Search API Location module.

If solr 3 is used the coordinates instead the actual distance is added to the search result. We could calculate the distance on our own with those coordinates but I think that belongs into the problem space of the location module (and it's already solved there).

You also claim that in the inline comments, however, you seem to have forgotten to also add that. I guess there should be an else in the code added in extractResults() that takes care of this.
Also, I think the Solr 3 method should also work without clean identifiers – returning fields also works when they contain colons, as far as I'm aware of.

Lastly:

@@ -920,6 +920,25 @@ class SearchApiSolrService extends SearchApiAbstractService {
+            // Add pseudofield with the distance to the result items.
+            $location_params['sfield'] = $field;
+            $location_params['fl'][] = '_' . $field . '_distance_:geodist()';
+            $location_params['pt'] = $point;

This means, unless I'm mistaken, that this will only work for a single location field, even though all other code would work for multiple fields as well. Why not pass the field and point in the geodist() function call?

das-peter’s picture

Here's an updated patch that takes the new search_api_location approach in account.
The compatibility with multiple spatial infos should be better too.
Attention: Solr3 code isn't tested as I don't have an instance available atm.

drunken monkey’s picture

Status: Needs review » Needs work

Thanks for the improvements!
However, I still have a few comments which need to be addressed:

  1. +++ b/includes/service.inc
    @@ -1047,6 +1067,11 @@ class SearchApiSolrService extends SearchApiAbstractService {
    +      $params['fl'] .= ',' . implode(',', $location_params['fl']);
    

    This should probably have an if (isset($location_params['fl'])). Or, if you are only putting fl into $location_params, just use a $location_fields variable directly.

  2. +++ b/includes/service.inc
    @@ -1184,6 +1211,23 @@ class SearchApiSolrService extends SearchApiAbstractService {
    +              $doc_field = '_' . str_replace(':', '_', $fields[$spatial['field']]) . '_distance_';
    

    Why the str_replace()? I think you're only adding this if the Solr field name doesn't have any colons?

  3. +++ b/includes/service.inc
    @@ -920,6 +920,26 @@ class SearchApiSolrService extends SearchApiAbstractService {
    +            $location_params['fl'][] = $spatial['field'] . '_distance:' . $field;
    

    Solr 3.x doesn't have field aliases, so this will be interpreted as a literal field name (e.g., field_location:latlon_distance:locs_field_location:latlon), which won't be found.
    You should probably just add the field to fl and check for that later when extracting the results.

das-peter’s picture

Status: Needs work » Needs review
FileSize
2.03 KB
3.5 KB

Finally back at work here :)
Thanks for the feedback.

  1. Changed to $location_fields
  2. That's true - but I usually prefer a better safe than sorry approach. Removed though.
  3. Changed to $field - no clue if that works. I don't have a solr3 instance ready to test it.
drunken monkey’s picture

Looks great, thanks again!

Attached is a slightly revised patch. Apart from the mechanism change mentioned in #1812528-31: Add proximity/distance information to search results (using a new response key instead of a magic field), I also removed the adding of the field to fl for Solr 3.x: if the distance data isn't available, the Views field handler (or whatever other consumer) can just get the field value in the usual way. If the user wants it to be retrieved from Solr, they can just enable "Retrieve results data from Solr". I see no need in a special treatment for that.

das-peter’s picture

Status: Needs review » Reviewed & tested by the community

Okay then let's do that. Looks good to me.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

Excellent. Committed.
Thanks again!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.