Integrate database backend with Search API Location

As far as I know, the Search API Location module doesn't require Solr. Instead, it provides a generic interface for backends to support location searches, which Solr currently supports. (See the documentation.)

So, the issue is the other way round: the database backend would have to support the Search API Location module.

The problem with that is that, while Solr comes with indexing and searching of locations by default, SQL databases in general don't, so it's much harder to implement for the database backend. But, in theory, nothing is stopping us. It's just that someone would need to do it.

@drunken monkey can you provide some hints or guide to achieve the same ? Or may be a skeleton patch with some hints.

Sure, see this patch. It's pretty straight-forward: Everywhere a location field could be used (indexing, conditions, sorts, facets) we need to include extra code to support location fields. (Straight-forward but of course a lot of work, and also probably rather complex.) Or, put another way: just see where the Solr backend checks for the "location" field type or "search_api_location" option – the DB backend will most likely need to act at the same places (plus when building the tables for indexing).

To make things cleaner, if you're actually gonna do this, we'll probably want to put everything related to this into its own "type handler" class and then call that from the backend plugin code, to not mess up that code even more with all these special cases. However, if you get it to work, and some test coverage, I can surely deal with such refactoring.

PS: \Drupal\search_api_db\Plugin\search_api\backend\Database::convert() also needs some special code, forgot that.

@drunken monkey Thanks, Once I completed search_api_location I will surely try to implement it.

any updates here? I would also be very interested having this feature, but unfortunatelly my current understanding of search api and maybe drupal is not sufficient to implement it.

Rerolled #5 and I am go to give it a shot today

Very work-in-progress:
- The location is now indexed and can be therefor be rendered in Views
example: POINT (4.4777 51.9244)

- The views-distance filter shows up in Views, but
* Settings-option for the exposed filter is not showing up in Views. So the exposed filter cannot be configured like this is possible with the Solr-backend
* The code that adds the views-distance filters only works on the getFieldIdentifier (field-name) and not on the field-type.

Forgot use statements in #11

This both seems like you just didn’t switch the field to use the “Location” data type? (Or maybe didn’t clear cache after adding supportsDataType().)
Also, the getSupportedFeatures() change shouldn’t be necessary. That was D7’s way of doing this.

Testing out your latest patch (thanks for the hard work so far!). Found a quick bug (missing 'location' case option in convert method prevented indexing... not sure if this is the best way forward, but it works.

Not sure what more help I can be, barely looked at what is actually happening here in the code, but I'll dig in more later.

just rerolled the patch

MR re-roll from #15.

I do need to make distance sort functioning, will give some try for this R&D ;-)

@ hswong3i: Unfortunately, testing of issue forks doesn’t work in this project – see #3190024: Problem with test dependencies when testing issue forks. Until this is fixed, we should to stick to the old patch-based development workflow in this project.

Re-rolling the patch in #15 to apply on latest search_api (8.x-1.22)

Hello,
I have created a draft merge request to ask of your opinion. This works with MySQL only. Is this approach acceptable?

Here's what I have and have not done so far:
- Tried to add some more code to #21.
- Filtering works using MySQL's ST_Distance_Sphere() function.
- Sorting works using the output from ST_Distance_Sphere().
- I haven't done anything about facets.
- I haven't removed any comment originally added as part of the initial skeleton patch.

Thanks.

Hello, any feedback for #24?

Hi progga, thanks for working on this.
Can you trigger the test-bot to see if there are no regressions?
Also it would be good to add a new tests specifically for this change.

Does this only work with MySql? Or also with related databases like sqlite?

> Can you trigger the test-bot to see if there are no regressions?

Sorry, the test runner is not going very far due to dependency issues unrelated to this change.

> Also it would be good to add a new tests specifically for this change.

Okay, I will try to get something specific to MySQL.

> Does this only work with MySql? Or also with related databases like sqlite?

Here's the situation with spatial distance calculation functions:
- MySQL: The ST_Distance_Sphere() function is readily available. So all is good.
- Postgres: No out-of-the-box equivalent of MySQL's ST_Distance_Sphere() but PostGIS comes with the similar ST_DistanceSphere().
- Sqlite: Again, nothing like MySQL's ST_Distance_Sphere() but the Spatialite extension provides ST_Distance() which is similar.

I have skipped the Postgres and Sqlite implementations for location search as we can't be certain that the PostGis or Spatialite extensions are present alongside Postgres or Sqlite.

> Also it would be good to add a new tests specifically for this change.

I have added a Kernel test for MySQL-based location search.

All tests for the search_api module and its search_api_db submodule passes in my development machine. But test runs are failing in Drupal CI very early due to unrelated composer dependency issues. Not sure what to do about that.

Adding a patch file for the merge request to see if that triggers a good test run.

Adding the patch again for the merge request after declaring the right test dependencies.

Both the merge request and the last patch are failing to cross the codebase build stage of DrupalCI. This is possibly because composer is checking out search_api:8.x-1.x-dev whereas search_api_autocomplete requires search_api:1.x: sudo -u www-data /usr/local/bin/composer require drupal/search_api 8.x-1.x-dev --prefer-stable --no-progress --no-suggest --no-interaction --working-dir /var/www/html

Had it done sudo -u www-data /usr/local/bin/composer require drupal/search_api 1.x-dev --prefer-stable --no-progress --no-suggest --no-interaction --working-dir /var/www/html, all would be good. In fact, this is just what happened during the test run of an older patch that did *not* touch the composer.json file.

I don't know how to resolve the DrupalCI issue. The test runs fine in a fresh Drupal install locally. So requesting review again.

Hello, any feedback for #31?

@ekes showed me this patch, because it introduces a difference between mysql versions this would be complicated to just enable on most backends. I think we would need to introduce a difference between mysql versions and detect the right compatiblity layer based on that.

I think that is good solution but I'm not sure about the implications this all has, I would very much prefer if we could do this simpler, another option I can think about is to have a configuration option in the backend so people can explicitly opt in if they know they have the right configuration options. This feels like a dirtier solution though. I think the best way forward is to ping @drunken monkey on the global drupal slack (in #search).

> because it introduces a difference between mysql versions this would be complicated to just enable on most backends

Thanks for looking into this. The ST_Distance_Sphere() function is available in MySQL 5.7+ and MariaDB 10.2.38+ both of which are older than Drupal 9's minimum MySQL requirement. So I would expect any supported MySQL instance to implement ST_Distance_Sphere().

> I think the best way forward is to ping @drunken monkey on the global drupal slack (in #search).

I will try your suggestion. Thank you :)

@ progga: Thanks a lot for all your work on this!
However, as explained in #19, testing unfortunately doesn’t work for issue forks in this project, so please use patch files instead.

Attached is a overhauled version from me, refactored in a way that I think this would fit best into the module. However, I’m unfortunately pretty busy with bug reports etc. to be able to devote much more time to a feature that apparently relatively few people want (at least not enough to test patches, etc.). Also, as long as the Search API Location module is pretty much unmaintained, and (as far as I’m aware) incompatible with Drupal 10, it wouldn’t make much sense to add support for it. (Especially, we cannot add a dev dependency on it, otherwise tests for this module will break on Drupal 10.)

I don’t have a problem with adding functionality that only works on MySQL, though, or just some versions/configurations of Postgres/SQLite, as long as this is clearly documented. What I would prefer, though, is if as much of the code for this as possible is actually inside the DB compatibility layer, instead of this complicated combination of DB compatibility plugin code preparing the way for a filter/sort placed by the normal backend plugin code. The current structure might make it much harder to later add support for other DBMSs.

Also, while I removed the explicit isLocationAware() method, we might want to still have it to make it possible for a DB compatibility plugin to determine location support based on the specific DB server version or configuration (e.g., installed extensions).

Finally, what I don’t really understand is addRetrievedFieldValues(). How is that related to this issue? If it isn’t, please remove that from the patch.

Oops, here’s the promised patch file.

Work-in-progress.

Work-in-progress.

> testing unfortunately doesn’t work for issue forks in this project, so please use patch files instead.

Testing of patches also breaks when composer.json is touched. I have tried to explain my understanding in #32.

> as long as the Search API Location module is pretty much unmaintained, and (as far as I’m aware) incompatible with Drupal 10, it wouldn’t make much sense to add support for it. (Especially, we cannot add a dev dependency on it, otherwise tests for this module will break on Drupal 10.)

I have dropped search_api_location in favour of a new test module that provides a mock location field type. This saves me from touching composer.json which is allowing the tests to pass.

> I don’t have a problem with adding functionality that only works on MySQL, though, or just some versions/configurations of Postgres/SQLite, as long as this is clearly documented.

Done.

> What I would prefer, though, is if as much of the code for this as possible is actually inside the DB compatibility layer

I have tried to move most location search specific code inside the database compatibility layer.

> Also, while I removed the explicit isLocationAware() method, we might want to still have it

I have revived it as LocationAwareDatabaseInterface::isLocationEnabled().

> Finally, what I don’t really understand is addRetrievedFieldValues(). How is that related to this issue? If it isn’t, please remove that from the patch.

I have renamed it to extractRetrievedFieldValuesWhereAvailable() and updated the function comments to further clarify its usage. It extracts the distance of each result item from the query result and then adds it to the result item. That's because Drupal\search_api\Plugin\views\field\SearchApiFieldTrait::query() has marked it as a "retrieved" field meaning it has been added to the search_api_retrieved_field_values query option. So if a query result row has these properties: item_id, score, a_location_field__distance and $query->getOption('search_api_retrieved_field_values') returns ['a_location_field__distance', 'another_field'], then extractRetrievedFieldValuesWhereAvailable() will include the 'a_location_field__distance' field and its value to the result item. This way the distance of each result item can later be displayed in a View.

Besides a patch, I have also attached an diff with #37 which was your last change. You will notice that I had to swap the *point* MySQL field type with varchar. This is because Drupal\search_api_db\Plugin\search_api\backend\Database::indexItem() is failing to populate Point type MySQL fields. As far as I have seen, Drupal's dynamic Insert queries can't insert into Point type fields. As an example, consider these two failing queries:

\Drupal::service('database')
  ->insert('a_table_with_a_point_field')
  ->fields(['a_point_field'])
  ->values(['a_point_field' => 'POINT(0, 51)'])
  ->execute();

Or its static query equivalent:

\Drupal::service('database')
  ->query('INSERT INTO {a_table_with_a_point_field} (a_point_field) VALUES(:our_point_field)', [
    ':our_point_field' => 'POINT(0, 51)',
  ]);

This is because a point type MySQL field refuses to accept a string value. There is a work-around which is to pass these through a converter:

\Drupal::service('database')
  ->query('INSERT INTO {a_table_with_a_point_field} (a_point_field) VALUES(ST_PointFromText(:our_point_field))', [
    ':our_point_field' => 'POINT(0, 51)',
  ]);

But I cannot think of any way to build queries like the last one in Database::indexItem(). So I had to swap the point field type with varchar even though point would definitely be a better choice performance-wise.

Thanks a lot for your continued work on this! This is now looking pretty good already.
I went through the whole code and corrected some nit-picks, but nothing major.

That we cannot (easily) use the proper MySQL data type is, of course, rather unfortunate, but I can’t really think of a good solution here, either. Just a wild selection of pretty bad ones:

• It would, in theory, be possible to use an INSERT … SELECT … query, where it’s simple to add expressions.
• Doesn’t seem like anything is stopping us from just using $db->query('INSERT INTO …') directly.
• We could probably also override the database class to provide a custom workaround.

As said, all pretty bad. So, probably fine to stick with the “clean” way for the first version and think about improving performance later, if there is demand. (And maybe someone comes up with a better solution.)

Anyways, please give my attached patch revision a try and thorough review, and if everything looks good to you, I can finally commit this.
And, in any case, thanks a lot for your work, once again!

Of course, everyone else is very welcome to test/review, too, to make sure this is a good solution.

I have thoroughly tested #44 and it works okay. Even stumbled upon a new issue with search_ap_location in the process but that's unrelated. So I am okay with #44.

Good to hear, thanks for testing and reporting back.
Committed.
Thanks again, everyone – especially thanks a lot to ndf and progga!

Thank you :)

I'm going to pop this here - for the next time I think about this - I decided to take a look if it was possible for here, and for storing the geofield itself in a native geometry type.

We could probably also override the database class to provide a custom workaround.

Ooof! Drupal doesn't really have a single location with schema of all tables in the db any more. There's no way of registering that placeholders, or replacement values are to be handled differently for different types of field. The INSERT, UPDATE, SELECT queries are written deep in the conversion of their implementation of Drupal\Core\Database\Query\Query to a string. After some head scratching best I could come up with would be to extend each one of these classes, and alter that point such that it would allow changes based on the table field that is being included. But I dare not think how much work that would be, or how careful you'd have to be not to break something!

Amazing! thank you!!!

Any chance of a release soon to get this out there?