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

Add GeoDistanceCondition #363

Merged
merged 19 commits into from
Sep 13, 2024
Merged

Conversation

ker0x
Copy link
Contributor

@ker0x ker0x commented Feb 20, 2024

This PR introduce GeoDistanceCondition and a new. GeoPointField for latitude and longitude coordinates:

ToDos

@alexander-schranz alexander-schranz added Adapter: Meilisearch Meilisearch Adapter related issue Adapter: Loupe Loupe Adapter related issue labels Feb 20, 2024
@alexander-schranz
Copy link
Member

Thx for the pull request at current early state of the library we will not merge things into the core which are not part of all Adapters. Still we can keep this pull request open.

It would be great if you could create an issue for this also and if you have some time add links to the specific search engines documentation. I may would create 2 issues for GeoDistance and GeoBoundingBox as it are 2 seperate features/conditions and we may can only implement the once which are currently supported by all engines.

What we require is a list like this one:

#343 (comment)

@alexander-schranz alexander-schranz marked this pull request as draft February 20, 2024 16:56
@alexander-schranz alexander-schranz added the features New feature or request label Feb 20, 2024
@alexander-schranz alexander-schranz added help wanted Extra attention is needed SEAL Core Seal Core related issue labels Feb 20, 2024
@ker0x ker0x force-pushed the feature/geo-conditions branch 2 times, most recently from 33350c3 to 9516c97 Compare March 12, 2024 08:31
@alexander-schranz alexander-schranz added Adapter: Elasticsearch Elasticsearch Adapter releated issue Adapter: Opensearch Opensearch Adapter related issue Adapter: Algolia Algolia Adapter related issue Adapter: Solr Solr Adapter related issue Adapter: Typesense Typesense Adapter related issue Adapter: RediSearch RediSearch Adapter related issue labels Mar 22, 2024
@alexander-schranz
Copy link
Member

@ker0x do you know if there is some GeoDistance Algorithm we could just copy paste into the Memory adapter here?

For Opensearch I think the Elasticsearch implementation mostly can be copied?

@alexander-schranz alexander-schranz force-pushed the feature/geo-conditions branch 2 times, most recently from b06a7cf to 99ae5a9 Compare August 9, 2024 13:01
@alexander-schranz
Copy link
Member

alexander-schranz commented Aug 9, 2024

I think we need also introduce a new Type for this .:

return new Index('blog', [
    'id' => new Field\IdentifierField('id'),
    'geo' => new Field\GeoPointField('geo', filterable: true),
]);

This would be the first type without a map to a native PHP type so we need force how such information are converted into PHP raw format e.g.:

'geo' => [
    'latitude' => 12.3,
    'longitude' => 43.8,
],

Basically we could use lat and lon but personally not a fan of using abbreviations.

We may need to have a look how different engine store it.

@alexander-schranz
Copy link
Member

I added GeoPointField and added already a implementation for Memory Adapter and Meilisearch. Started with Loupe but did not yet found out why it doesn't show any results there 🤔

@ker0x
Copy link
Contributor Author

ker0x commented Sep 1, 2024

Hello @alexander-schranz

Sorry for the lack of feedback, I haven't had too much time available lately to continue working on this feature.

However, I've just added GeoDistanceCondition for Opensearch and fix GeoDistanceCondition for Solr and GeoBoundingBoxCondition for Opensearch.

@alexander-schranz
Copy link
Member

No problem. Thank you for all your work you already did put into it. I pushed an update as I accidently added a regression, the Github CI is still a little bit flacky (checking the false commit out) but Meilisearch and Memory Adapter are now green with the GeoDistance test.

@alexander-schranz
Copy link
Member

Thx to @Toflar we solved the issues on Loupe side.
I pushed updates for Algolia and also Elasticsearch and Opensearch.
Typesense was also good to implement.

Missing for the GeoDistanceCondition is so only Solr and Redisearch now to update to the GeoPointField Type and make tests green.

@alexander-schranz
Copy link
Member

Not sure if I did something wrong but there seems be an issue with the result returned by Redisearch: RediSearch/RediSearch#5020.

@alexander-schranz
Copy link
Member

alexander-schranz commented Sep 12, 2024

Found the issue on my side lat <-> lon. So only Solr is last for the GeoDistanceCondition tests :)

@alexander-schranz
Copy link
Member

As it seems the GeoDistanceCondition is now already finished I think we should move the GeoBoundingBoxCondition in its own pull request so we can merge this one:

Reapply GeoBoundingBoxCondition patch
diff --git a/docs/search-and-filters/index.rst b/docs/search-and-filters/index.rst
index b353b39..7861024 100644
--- a/docs/search-and-filters/index.rst
+++ b/docs/search-and-filters/index.rst
@@ -204,6 +204,22 @@ The ``GeoDistanceCondition`` is used to filter results within a radius by specif
 
 The field is required to be marked as ``filterable`` in the index configuration.
 
+GeoBoundingBoxCondition
+~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``GeoBoundingBoxCondition`` is used to filter results within a bounding box by specifying a min latitude, min longitude, max latitude and max longitude.
+
+.. code-block:: php
+
+    <?php
+
+    use Schranz\Search\SEAL\Search\Condition;
+
+    $result = $this->engine->createSearchBuilder()
+        ->addIndex('restaurants')
+        ->addFilter(new Condition\GeoBoundingBoxCondition('location', 45.494181, 9.214024, 45.449484, 9.179175))
+        ->getResult();
+
 
 The field is required to be marked as ``filterable`` in the index configuration.
 
diff --git a/packages/seal-algolia-adapter/src/AlgoliaSearcher.php b/packages/seal-algolia-adapter/src/AlgoliaSearcher.php
index e5441bb..ee38380 100644
--- a/packages/seal-algolia-adapter/src/AlgoliaSearcher.php
+++ b/packages/seal-algolia-adapter/src/AlgoliaSearcher.php
@@ -106,6 +106,9 @@ final class AlgoliaSearcher implements SearcherInterface
                     ),
                     'aroundRadius' => $filter->distance,
                 ],
+                $filter instanceof Condition\GeoBoundingBoxCondition => $geoFilters = [
+                    'insideBoundingBox' => [[$filter->minLatitude, $filter->minLongitude, $filter->maxLatitude, $filter->maxLongitude]],
+                ],
                 default => throw new \LogicException($filter::class . ' filter not implemented.'),
             };
         }
diff --git a/packages/seal-meilisearch-adapter/src/MeilisearchSearcher.php b/packages/seal-meilisearch-adapter/src/MeilisearchSearcher.php
index 9b4657e..f99702f 100644
--- a/packages/seal-meilisearch-adapter/src/MeilisearchSearcher.php
+++ b/packages/seal-meilisearch-adapter/src/MeilisearchSearcher.php
@@ -92,6 +92,13 @@ final class MeilisearchSearcher implements SearcherInterface
                     $this->escapeFilterValue($filter->longitude),
                     $this->escapeFilterValue($filter->distance),
                 ),
+                $filter instanceof Condition\GeoBoundingBoxCondition => $filters[] = \sprintf(
+                    '_geoBoundingBox([%s, %s], [%s, %s])',
+                    $this->escapeFilterValue($filter->minLatitude),
+                    $this->escapeFilterValue($filter->minLongitude),
+                    $this->escapeFilterValue($filter->maxLatitude),
+                    $this->escapeFilterValue($filter->maxLongitude),
+                ),
                 default => throw new \LogicException($filter::class . ' filter not implemented.'),
             };
         }
diff --git a/packages/seal-opensearch-adapter/src/OpensearchSearcher.php b/packages/seal-opensearch-adapter/src/OpensearchSearcher.php
index 0acc16a..3380ad4 100644
--- a/packages/seal-opensearch-adapter/src/OpensearchSearcher.php
+++ b/packages/seal-opensearch-adapter/src/OpensearchSearcher.php
@@ -90,6 +90,16 @@ final class OpensearchSearcher implements SearcherInterface
                         'lon' => $filter->longitude,
                     ],
                 ],
+                $filter instanceof Condition\GeoBoundingBoxCondition => $query['bool']['filter']['geo_bounding_box'][$this->getFilterField($search->indexes, $filter->field)] = [
+                    'top_left' => [
+                        'lat' => $filter->minLatitude,
+                        'lon' => $filter->minLongitude,
+                    ],
+                    'bottom_right' => [
+                        'lat' => $filter->maxLatitude,
+                        'lon' => $filter->maxLongitude,
+                    ],
+                ],
                 default => throw new \LogicException($filter::class . ' filter not implemented.'),
             };
         }
diff --git a/packages/seal/src/Search/Condition/GeoBoundingBoxCondition.php b/packages/seal/src/Search/Condition/GeoBoundingBoxCondition.php
new file mode 100644
index 0000000..654a493
--- /dev/null
+++ b/packages/seal/src/Search/Condition/GeoBoundingBoxCondition.php
@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the Schranz Search package.
+ *
+ * (c) Alexander Schranz <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Schranz\Search\SEAL\Search\Condition;
+
+class GeoBoundingBoxCondition
+{
+    public function __construct(
+        public readonly string $field,
+        public readonly float $minLatitude,
+        public readonly float $minLongitude,
+        public readonly float $maxLatitude,
+        public readonly float $maxLongitude,
+    ) {
+    }
+}

@ker0x ker0x changed the title Add GeoDistanceCondition and GeoBoundingBoxCondition Add GeoDistanceCondition Sep 13, 2024
@alexander-schranz alexander-schranz force-pushed the feature/geo-conditions branch 2 times, most recently from e9211cf to e57d576 Compare September 13, 2024 07:47
@alexander-schranz alexander-schranz marked this pull request as ready for review September 13, 2024 10:45
@alexander-schranz alexander-schranz merged commit 180cc20 into PHP-CMSIG:0.5 Sep 13, 2024
30 of 32 checks passed
@alexander-schranz
Copy link
Member

@ker0x Merged. Thank you for your help here and kicking this off. If you want you can open the merge request for the GeoBoundingBoxCondition working branch.

@ker0x ker0x deleted the feature/geo-conditions branch September 14, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Adapter: Algolia Algolia Adapter related issue Adapter: Elasticsearch Elasticsearch Adapter releated issue Adapter: Loupe Loupe Adapter related issue Adapter: Meilisearch Meilisearch Adapter related issue Adapter: Opensearch Opensearch Adapter related issue Adapter: RediSearch RediSearch Adapter related issue Adapter: Solr Solr Adapter related issue Adapter: Typesense Typesense Adapter related issue features New feature or request help wanted Extra attention is needed SEAL Core Seal Core related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants