Locate map pins by indexed WKT #336
Conversation
This removes the requirement for an entity load of the geo entity. It enables the possibility to show multiple locations per directory item as #333.
|
Question: as it's quite good to get rid of those entity loads, should we write an update hook as well? It's presently just new installs. |
|
This is the same change BHCC is making to support multiple geo locations for each directory entry. |
|
Tested locally and this works for me. To test multiple locations on a directory venue, needed to edit the field to allow mutliple. Got some warnings when enabling the localgov_demo module, like this: Is that something we want to look at? |
If the maps with areas are also changing - they will be I think - then yes... because that's what that is. Realistically different storage/display for the areas would be best; but the quickest (config wise, not perforance) might just be changing the field type to accept something huge |
|
@ekes discussing on Merge Tuesday, might be nice to store as geojson... |
Interestingly because of the way the field is loaded (I'm trying to look if this is leaflet or search_api) it is then grabbing it from the entity. So for the original context of this issue being able to show results from multiple delta location fields, the fact it indexes nothing, isn't an issue 🤯 So it's an improvement. Keeping it as a string also makes sense, because fulltext tokenizes it etc. and puts it into the fulltext index which we for sure don't want. Fixes the original requirement. But it isn't then a performance improvement. Doing it from indexed data, and indexed data that's already formatted to json would be for sure a win, but it's also quite some more work. It needs both making sure that the db (it's not a solr problem) will store the length of data (and not handle it as tokenized text) and be sure that leaflet will get it from the search result directly. |
|
I'm having issues with this branch when I actully try and add a second geo to a directory. The SQL query for the location search gets changed (thanks to dumping a dqm inside search api db). SELECT DISTINCT "t"."item_id" AS "item_id", '1000' AS "score", ST_Distance_Sphere(Point('-0.1400561', '50.8214626'), ST_PointFromText(t.localgov_location)) / 1000 AS "localgov_location__distance"
FROM
{search_api_db_localgov_directories_index_default} "t"
WHERE "t"."localgov_directory_channels" = '21'
HAVING "localgov_location__distance" < '2'But once a second geo is present, it is changed to: SELECT DISTINCT "t"."item_id" AS "item_id", '1000' AS "score", ST_Distance_Sphere(Point('-0.1400561', '50.8214626'), ST_PointFromText(t.localgov_location)) / 1000 AS "localgov_location__distance"
FROM
{search_api_db_localgov_directories_index_default} "t"
LEFT OUTER JOIN {search_api_db_localgov_directories_index_default_localgov_loca} "t_2" ON t.item_id = t_2.item_id
WHERE ("t"."localgov_directory_channels" = '21') AND ("t_2"."value" < '2') |
Possibly related to this https://www.drupal.org/project/search_api/issues/3353060 ? |
|
Thanks @ekes, do we use those patches? I could try on of them and see if it resolves. |
|
I'll be honest I forgot I'd been working on it till you mentioned it there. So not sure if we did start testing them with LGD, the multivalued field only being an issue here more recently. Testing them, feeding back on the issue, and answering any of Thomas' questions that we can would seem a very good _thing_™ at this point. Sorry I'd forgotten about them. |
|
This is working in terms of the general query, but breaks when facets are present (so no map display as that one has facets whereas I temp took them off the proximity search). 
However the query has been updated correcty SELECT DISTINCT "t"."item_id" AS "item_id", '1000' AS "score", ST_Distance_Sphere(Point('-0.1400561', '50.8214626'), ST_PointFromText(t_2.value)) / 1000 AS "localgov_location__distance"
FROM
{search_api_db_localgov_directories_index_default} "t"
LEFT OUTER JOIN {search_api_db_localgov_directories_index_default_localgov_loca} "t_2" ON t.item_id = t_2.item_id
WHERE "t"."localgov_directory_channels" = '21'
HAVING "localgov_location__distance" < '2'I'll do a bit more digging and update the d.o issue. |
|
Discovery written up and patch added here https://www.drupal.org/project/search_api/issues/3353060#comment-15459294 |
|
Next problem, with the proximity search, now we inlcude the map pins for all locations, the map will show items with pins outside the area if they exist with a location inside the area. Ideally a search should only show map pins inside the area. In the below example we have an entry where it has a location in Brighton and London, so it shows both even with a search for Brighton. 
|
Suggest we make a follow-up issue for this one. It won't be possible to remove them from the search results, because they come from a result - basing it on an index of locations with attached services would be solve it for the map, but would make it different from the results below which are directory entry based, so not work in this case. So either filtering the results post query, or centring the map in JS on the search point leaving the additional result outside the zoom, seem the two best options. |
|
Still hoping for some testing from @stephen-cox and me! @finnlewis Note: we are using this at Greenwich as a patch. |
|
@andybroomfield confirmed that this is working at BHCC @stephen-cox was going to test this... might you have a chance to review this week? |
|
Heres the actual errors from running PHPunit against this branch locally
|
|
Just mentioned this at Merge Tuesday. @andybroomfield is using this at BHCC and we are using this at Greenwich. So at least 2 councils are using this successfully. Let's get it merged! |
| @@ -24,6 +26,15 @@ function localgov_directories_location_install($is_syncing) { | |||
| return; | |||
| } | |||
|
|
|||
| $index = SearchIndex::load(Directory::DEFAULT_INDEX); | |||
| $location_field = new SearchIndexField($index, Directory::LOCATION_FIELD_WKT); | |||
There was a problem hiding this comment.
Hello Ekes, Okay, don't worry. I can see it is of
I am trying to understand the purpose behind this new localgov_location_wkt Search index field. Is it there to avoid the unwanted left join issue that Andy has pointed out?string type. Which suggests that it is for displaying the map popups only.
|
@Adnan-cds want to do a little more testing before we merge this. The test view was timing out hitting 128M of PHP memory, 256M was OK. |
| table: search_api_entity_geo_entity | ||
| field: location | ||
| relationship: localgov_location | ||
| search_api_rendered_item: |
There was a problem hiding this comment.
I don't see any use of this Search: Rendered item field anywhere in this View's display. If we drop this field, the embed_map display loads about a thousand map pins without any memory exhaustion issues in our site. So I would request that this search_api_rendered_item field be dropped.
I am still running into memory exhaustion issues around 1500 map pins after dropping the search_api_rendered_item field. But I don't yet see any easy way out of that.
There was a problem hiding this comment.
@ekes fancy adding this change?
There was a problem hiding this comment.
I'm almost tempted to go the other way. If we're not including a freetext search based on rendered item in the default view maybe we should be?
|
Hello, Ekes, Finn, I have suggested a small config change above which cuts memory usage somewhat. I don't have anything more to propose. Thanks. |
|
Note: @millnut suggested we add to documentation to recommend using Apache Solr for any directories with more than 500 / 1000 pins. |
|
\o/ schema errors fixed. |
This removes the requirement for an entity load of the geo entity.
It enables the possibility to show multiple locations per directory item as #333.