Merge pull request #1031 from lonvia/rework-dedups-filter

Convert deduplicaton filter to predicate

Author: lonvia

src/main/java/de/komoot/photon/GenericSearchHandler.java

@@ -2,6 +2,7 @@
 
 import de.komoot.photon.query.RequestBase;
 import de.komoot.photon.query.RequestFactory;
+import de.komoot.photon.searcher.PhotonResult;
 import de.komoot.photon.searcher.ResultFormatter;
 import de.komoot.photon.searcher.SearchHandler;
 import de.komoot.photon.searcher.StreetDupesRemover;
@@ -10,6 +11,7 @@
 import org.jspecify.annotations.NullMarked;
 
 import java.io.IOException;
+import java.util.Comparator;
 
 @NullMarked
 public class GenericSearchHandler<T extends RequestBase> implements Handler {
@@ -28,17 +30,14 @@ public GenericSearchHandler(RequestFactory<T> requestFactory, SearchHandler<T> r
     public void handle(Context context) {
         final T searchRequest = requestFactory.create(context);
 
-        var results = requestHandler.search(searchRequest);
+        var results = requestHandler.search(searchRequest).stream()
+                .sorted(Comparator.comparingDouble(PhotonResult::getScore).reversed());
 
-        // Further filtering
-        if (searchRequest.getDedupe()){
-            results = new StreetDupesRemover(searchRequest.getLanguage()).execute(results);
+        if (searchRequest.getDedupe()) {
+            results = results.filter(new StreetDupesRemover());
         }
 
-        // Restrict to the requested limit.
-        if (results.size() > searchRequest.getLimit()) {
-            results = results.subList(0, searchRequest.getLimit());
-        }
+        results = results.limit(searchRequest.getLimit());
 
         String debugInfo = null;
         if (searchRequest.getDebug()) {
@@ -48,7 +47,7 @@ public void handle(Context context) {
         try {
             context.status(200)
                     .result(formatter.convert(
-                            results, searchRequest.getLanguage(),
+                            results.toList(), searchRequest.getLanguage(),
                             searchRequest.getReturnGeometry(),
                             searchRequest.getDebug(), debugInfo));
         } catch (IOException e) {

src/main/java/de/komoot/photon/opensearch/OpenSearchSearchHandler.java

@@ -25,8 +25,9 @@ public OpenSearchSearchHandler(OpenSearchClient client, int queryTimeout) {
 
     @Override
     public List<PhotonResult> search(SimpleSearchRequest request) {
-        final int limit = request.getLimit();
-        final int extLimit = limit > 1 ? (int) Math.round(limit * 1.5) : 1;
+        // Have it return more result candidates than results requested,
+        // will be reranked and filtered later.
+        final int extLimit = Math.max(5, (int) Math.round(request.getLimit() * 1.5));
 
         var results = sendQuery(buildQuery(request, false), extLimit);
 

src/main/java/de/komoot/photon/searcher/PhotonResult.java

@@ -15,6 +15,11 @@ public interface PhotonResult {
     @Nullable
     Object get(String key);
 
+    default String getOrDefault(String key, String defValue) {
+        String value = (String) get(key);
+        return value == null ? defValue : value;
+    }
+
     @Nullable
     String getLocalised(String key, String language, String... altNames);
 

src/main/java/de/komoot/photon/searcher/StreetDupesRemover.java

@@ -3,71 +3,47 @@
 import de.komoot.photon.opensearch.DocFields;
 import org.jspecify.annotations.NullMarked;
 
-import java.util.ArrayList;
 import java.util.HashSet;
-import java.util.List;
+import java.util.function.Predicate;
 
 /**
  * Filter out duplicate streets from the list.
  */
 @NullMarked
-public class StreetDupesRemover {
-    private final String language;
-
-    public StreetDupesRemover(String language) {
-        this.language = language;
-    }
-
-    public List<PhotonResult> execute(List<PhotonResult> results) {
-        final List<PhotonResult> filteredItems = new ArrayList<>(results.size());
-        final HashSet<String> keys = new HashSet<>();
-
-        for (PhotonResult result : results) {
-            if ("highway".equals(result.get(DocFields.OSM_KEY))) {
-                // result is a street
-                final String postcode = (String) result.get(DocFields.POSTCODE);
-                final String name = result.getLocalised(DocFields.NAME, language);
-
-                if (postcode != null && name != null) {
-                    // street has a postcode and name
-
-                    // OSM_VALUE is part of key to avoid deduplication of e.g. bus_stops and streets with same name
-                    String key = (String) result.get(DocFields.OSM_VALUE);
-                    if (key == null) {
-                        key = "";
-                    }
-
-                    if (language.equals("nl")) {
-                        final String onlyDigitsPostcode = stripNonDigits(postcode);
-                        key += ":" + onlyDigitsPostcode + ":" + name;
-                    } else {
-                        key += ":" + postcode + ":" + name;
+public class StreetDupesRemover implements Predicate<PhotonResult> {
+    final HashSet<String> keys = new HashSet<>();
+
+    @Override
+    public boolean test(PhotonResult result) {
+        if ("highway".equals(result.get(DocFields.OSM_KEY))) {
+            // result is a street
+            final String name = result.getLocalised(DocFields.NAME, "default");
+
+            if (name != null) {
+                final String countryCode = (String) result.get(DocFields.COUNTRYCODE);
+                String postcode = result.getOrDefault(DocFields.POSTCODE, "");
+
+                final var pcLength = postcode.length();
+                final var spacePos = postcode.indexOf(" ");
+                if (spacePos > 0) {
+                    if ("NL".equals(countryCode)) {
+                        postcode = postcode.substring(0, spacePos);
+                    } else if ("GB".equals(countryCode) && pcLength > spacePos + 2) {
+                        postcode = postcode.substring(0, spacePos + 2);
                     }
-
-                    if (keys.contains(key)) {
-                        // an osm highway object (e.g. street or bus_stop) with this osm_value + name + postcode is already part of the result list
-                        continue;
-                    }
-                    keys.add(key);
                 }
-            }
-            filteredItems.add(result);
-        }
 
-        return filteredItems;
-    }
+                // OSM_VALUE is part of key to avoid deduplication of e.g. bus_stops and streets with same name
+                final String key = String.join(":",
+                        result.getOrDefault(DocFields.OSM_VALUE, ""),
+                        postcode,
+                        name,
+                        countryCode);
 
-    private static String stripNonDigits(
-            final CharSequence input /* inspired by seh's comment */) {
-        final StringBuilder sb = new StringBuilder(
-                input.length() /* also inspired by seh's comment */);
-        for (int i = 0; i < input.length(); i++) {
-            final char c = input.charAt(i);
-            if (c > 47 && c < 58) {
-                sb.append(c);
+                return keys.add(key);
             }
         }
-        return sb.toString();
-    }
 
+        return true;
+    }
 }

src/test/java/de/komoot/photon/searcher/StreetDupesRemoverTest.java

@@ -3,41 +3,52 @@
 import de.komoot.photon.opensearch.DocFields;
 import org.junit.jupiter.api.Test;
 
-import java.util.List;
-
 import static org.assertj.core.api.Assertions.*;
 
 class StreetDupesRemoverTest {
 
     @Test
     void testDeduplicatesStreets() {
-        StreetDupesRemover streetDupesRemover = new StreetDupesRemover("en");
-        var allResults = List.of(
-            createDummyResult("99999", "Main Street", "highway", "Unclassified"),
-            createDummyResult("99999", "Main Street", "highway", "Unclassified"));
-
-        assertThat(streetDupesRemover.execute(allResults))
-                .hasSize(1);
+        assertThat(new StreetDupesRemover())
+                .accepts(highway("99999", "Main Street", "unclassified", "de"))
+                .rejects(highway("99999", "Main Street", "unclassified", "de"))
+                .accepts(highway("99999", "Sub Street", "unclassified", "de"))
+                .accepts(highway("99999", "Main Street", "unclassified", "ch"));
     }
 
     @Test
     void testStreetAndBusStopNotDeduplicated() {
-        StreetDupesRemover streetDupesRemover = new StreetDupesRemover("en");
-        var allResults = List.of(
-            createDummyResult("99999", "Main Street", "highway", "bus_stop"),
-            createDummyResult("99999", "Main Street", "highway", "Unclassified"));
+        assertThat(new StreetDupesRemover())
+                .accepts(highway("99999", "Main Street", "bus_stop", "de"))
+                .accepts(highway("99999", "Main Street", "unclassified", "de"));
+    }
+
+    @Test
+    void testNLStreet() {
+        assertThat(new StreetDupesRemover())
+                .accepts(highway("2345 XZ", "Main Street", "unclassified", "nl"))
+                .accepts(highway("2346 AB", "Main Street", "unclassified", "nl"))
+                .rejects(highway("2345 AB", "Main Street", "unclassified", "nl"))
+                .accepts(highway("2", "Main Street", "unclassified", "nl"));
+    }
+
+    @Test
+    void testGBStreet() {
+        assertThat(new StreetDupesRemover())
+                .accepts(highway("WN8 6LT", "Main Street", "unclassified", "gb"))
+                .accepts(highway("WN8 4LT", "Main Street", "unclassified", "gb"))
+                .rejects(highway("WN8 634", "Main Street", "unclassified", "gb"))
+                .accepts(highway("W", "Main Street", "unclassified", "gb"));
 
-        assertThat(streetDupesRemover.execute(allResults))
-                .hasSize(2);
     }
 
-    private PhotonResult createDummyResult(String postCode, String name, String osmKey,
-                    String osmValue) {
+    private PhotonResult highway(String postCode, String name, String osmValue, String countryCode) {
         return new MockPhotonResult()
                 .put(DocFields.POSTCODE, postCode)
-                .putLocalized(DocFields.NAME, "en", name)
-                .put(DocFields.OSM_KEY, osmKey)
-                .put(DocFields.OSM_VALUE, osmValue);
+                .putLocalized(DocFields.NAME, "default", name)
+                .put(DocFields.OSM_KEY, "highway")
+                .put(DocFields.OSM_VALUE, osmValue)
+                .put(DocFields.COUNTRYCODE, countryCode.toUpperCase());
     }
 
 }