diff --git a/server/src/nest/maps/maps.helpers.ts b/server/src/nest/maps/maps.helpers.ts index 8f2b90746..035371cc4 100644 --- a/server/src/nest/maps/maps.helpers.ts +++ b/server/src/nest/maps/maps.helpers.ts @@ -103,15 +103,20 @@ export { haversineMetres }; * and for most of the rest it returned the shops AROUND the landmark rather * than nothing, which is worse than nothing because it looks like an answer. * - * Two candidate signals for "the index does not have this one" were measured - * and both failed. The index's own score does not separate the cases (a - * threshold of 0.75 catches 19 of 62 misses and throws away 6 of 66 hits), and - * name overlap against the result set does not either (57 false positives, - * because "Hase Station" shares a word with "Hase-dera"). So this does not - * classify. It shows both, and lets the name decide the order: + * Three orderings were tried against that corpus, and the two clever ones lost. * - * 1. results whose name plausibly matches what was typed, index first - * 2. everything else, index first + * A score threshold cannot tell the cases apart: at 0.75 it catches 19 of 62 + * misses and throws away 6 of 66 hits. Promoting results whose name matches + * what was typed is worse than it sounds, because "Hase Station" shares a word + * with "Hase-dera" while OpenStreetMap answers in the local language and + * returns the correct temple under a name that shares nothing at all: the rule + * pushed seven weak index matches above the right answer, which landed at + * position eight. + * + * So this does not rank. It alternates, index first, and lets each source's own + * ordering stand. An exact business match still opens the list, because the + * index put it first; a landmark the index does not carry is second rather than + * eleventh. Neither source has to be judged by the other's yardstick. * * A place both sources know is kept once, as the index's copy, because that is * the one carrying a stable id, contact details and hours. @@ -119,7 +124,6 @@ export { haversineMetres }; export function mergeSearchResults( fromIndex: Record[], fromOsm: Record[], - query: string, limit = 10, ): Record[] { const sameThing = (a: Record, b: Record): boolean => { @@ -139,15 +143,13 @@ export function mergeSearchResults( }; const extra = fromOsm.filter((o) => !fromIndex.some((i) => sameThing(i, o))); - const matches = (p: Record): boolean => - typeof p.name === 'string' && namesOverlap(query, p.name); - return [ - ...fromIndex.filter(matches), - ...extra.filter(matches), - ...fromIndex.filter((p) => !matches(p)), - ...extra.filter((p) => !matches(p)), - ].slice(0, limit); + const out: Record[] = []; + for (let i = 0; i < Math.max(fromIndex.length, extra.length) && out.length < limit; i++) { + if (i < fromIndex.length) out.push(fromIndex[i]); + if (i < extra.length && out.length < limit) out.push(extra[i]); + } + return out; } export function namesOverlap(a: string, b: string): boolean { diff --git a/server/src/nest/maps/maps.service.ts b/server/src/nest/maps/maps.service.ts index 651c6c1d6..2ffb9818d 100644 --- a/server/src/nest/maps/maps.service.ts +++ b/server/src/nest/maps/maps.service.ts @@ -1656,7 +1656,7 @@ export class MapsService { return [] as Record[]; }), ]); - const places = mergeSearchResults(found.map(toPlaceRecord), osm, query); + const places = mergeSearchResults(found.map(toPlaceRecord), osm); if (places.length > 0) { return { places, diff --git a/server/tests/unit/nest/maps.merge-search.test.ts b/server/tests/unit/nest/maps.merge-search.test.ts index 130427196..9f7f057fc 100644 --- a/server/tests/unit/nest/maps.merge-search.test.ts +++ b/server/tests/unit/nest/maps.merge-search.test.ts @@ -20,20 +20,21 @@ const p = (name: string, lat: number, lng: number, source: string) => ({ }); describe('mergeSearchResults', () => { - it('MAPS-MERGE-001: lifts the landmark above the shops that surround it', () => { - // The Hase-dera case. The index has the coffee shop next door and not the - // temple; OpenStreetMap has the temple. - const index = [p('Uni Coffee Roastery', 35.3128, 139.534, 'trek-places')]; - const osm = [p('Hase-dera', 35.3125, 139.5335, 'openstreetmap')]; - const out = mergeSearchResults(index, osm, 'Hase-dera'); - expect(out.map((r) => r.name)).toEqual(['Hase-dera', 'Uni Coffee Roastery']); + it('MAPS-MERGE-001: puts the landmark second, not eleventh', () => { + // The Hase-dera case. The index has the shops around the temple and not the + // temple; OpenStreetMap has the temple, under its Japanese name. + const index = Array.from({ length: 10 }, (_, i) => + p(`Shop ${i}`, 35.31 + i / 1000, 139.53, 'trek-places'), + ); + const osm = [p('長谷寺', 35.3125, 139.5335, 'openstreetmap')]; + const out = mergeSearchResults(index, osm); + expect(out[1].name).toBe('長谷寺'); }); it('MAPS-MERGE-002: leaves a good index match at the top', () => { const index = [p("L'Osteria Rostock", 54.0879, 12.1408, 'trek-places')]; - const osm = [p('Osteria Steinstrasse', 54.09, 12.15, 'openstreetmap')]; - const out = mergeSearchResults(index, osm, "L'Osteria"); - expect(out[0].name).toBe("L'Osteria Rostock"); + const osm = [p('Steinstrasse', 54.09, 12.15, 'openstreetmap')]; + expect(mergeSearchResults(index, osm)[0].name).toBe("L'Osteria Rostock"); }); it('MAPS-MERGE-003: keeps one copy of a place both sources know', () => { @@ -41,39 +42,35 @@ describe('mergeSearchResults', () => { // details and hours. const index = [p("L'Osteria", 54.0879, 12.1408, 'trek-places')]; const osm = [p("L'Osteria", 54.08792, 12.14082, 'openstreetmap')]; - const out = mergeSearchResults(index, osm, "L'Osteria"); + const out = mergeSearchResults(index, osm); expect(out).toHaveLength(1); expect(out[0].source).toBe('trek-places'); }); it('MAPS-MERGE-004: two neighbouring shops both survive', () => { - // 60 m is the dedupe distance: close enough to be one shop, far enough - // apart that the bakery beside it is still its own result. + // 60 m is the dedupe distance, and the names have to agree as well: a + // temple and the coffee shop at its gate are closer than that. const index = [p('Bakery A', 54.0879, 12.1408, 'trek-places')]; const osm = [p('Bakery B', 54.0889, 12.1408, 'openstreetmap')]; - expect(mergeSearchResults(index, osm, 'Bakery')).toHaveLength(2); + expect(mergeSearchResults(index, osm)).toHaveLength(2); }); it('MAPS-MERGE-005: a source that answered nothing changes nothing', () => { const index = [p("L'Osteria", 54.0879, 12.1408, 'trek-places')]; - expect(mergeSearchResults(index, [], "L'Osteria")).toEqual(index); - expect(mergeSearchResults([], index, "L'Osteria")).toEqual(index); + expect(mergeSearchResults(index, [])).toEqual(index); + expect(mergeSearchResults([], index)).toEqual(index); }); - it('MAPS-MERGE-006: name matches come first, index before OpenStreetMap', () => { - const index = [p('Something Else', 1, 1, 'trek-places'), p('Kegon Falls', 1.5, 1.5, 'trek-places')]; - const osm = [p('Kegon Waterfall', 2, 2, 'openstreetmap'), p('Unrelated', 3, 3, 'openstreetmap')]; - expect(mergeSearchResults(index, osm, 'Kegon Falls').map((r) => r.name)).toEqual([ - 'Kegon Falls', - 'Kegon Waterfall', - 'Something Else', - 'Unrelated', - ]); + it('MAPS-MERGE-006: alternates, index first, and keeps each order intact', () => { + const index = [p('I1', 1, 1, 'trek-places'), p('I2', 2, 2, 'trek-places')]; + const osm = [p('O1', 3, 3, 'openstreetmap'), p('O2', 4, 4, 'openstreetmap')]; + expect(mergeSearchResults(index, osm).map((r) => r.name)).toEqual(['I1', 'O1', 'I2', 'O2']); }); - it('MAPS-MERGE-007: honours the cap', () => { + it('MAPS-MERGE-007: honours the cap without cutting mid-pair', () => { const many = Array.from({ length: 12 }, (_, i) => p(`Place ${i}`, 50 + i, 10 + i, 'trek-places')); - expect(mergeSearchResults(many, many, 'Place', 10)).toHaveLength(10); + const other = Array.from({ length: 12 }, (_, i) => p(`Other ${i}`, 20 + i, 30 + i, 'openstreetmap')); + expect(mergeSearchResults(many, other, 5)).toHaveLength(5); }); it('MAPS-MERGE-008: a result without coordinates is kept, not deduped away', () => { @@ -81,6 +78,6 @@ describe('mergeSearchResults', () => { // the row would lose a result; treating it as "not near anything" keeps it. const index = [p('A', 54, 12, 'trek-places')]; const osm = [{ name: 'B', lat: null, lng: null, source: 'openstreetmap' }]; - expect(mergeSearchResults(index, osm, 'A')).toHaveLength(2); + expect(mergeSearchResults(index, osm)).toHaveLength(2); }); });