fix: stop re-searching Google Maps on every nearby-search request

POI/categories.go

@@ -2,6 +2,7 @@ package POI
 
 import (
 	"fmt"
+	"math"
 	"strings"
 )
 
@@ -107,16 +108,29 @@ func GetPlaceTypes(placeCat PlaceCategory) (placeTypes []LocationType) {
 	return
 }
 
+// AllPlaceCategories enumerates every place category. It is the single source of truth for
+// "what categories exist": ParsePlaceCategory validates against it, and callers that must touch
+// every category's geo bucket (e.g. deleting a place that may be filed under several) iterate
+// it rather than hardcoding a subset — which is how Shopping, Lodging, and Wellness came to be
+// missed by cleanup paths written before they existed.
+var AllPlaceCategories = []PlaceCategory{
+	PlaceCategoryVisit,
+	PlaceCategoryEatery,
+	PlaceCategoryShopping,
+	PlaceCategoryLodging,
+	PlaceCategoryWellness,
+}
+
 // ParsePlaceCategory converts a category string (e.g. from an API request) into a known
 // PlaceCategory, reporting whether it matched. Matching is exact against the canonical
 // category names ("Eatery", "Shopping", "Lodging", "Wellness", "Visit").
 func ParsePlaceCategory(s string) (PlaceCategory, bool) {
-	switch PlaceCategory(s) {
-	case PlaceCategoryVisit, PlaceCategoryEatery, PlaceCategoryShopping, PlaceCategoryLodging, PlaceCategoryWellness:
-		return PlaceCategory(s), true
-	default:
-		return PlaceCategory(""), false
+	for _, cat := range AllPlaceCategories {
+		if PlaceCategory(s) == cat {
+			return cat, true
+		}
 	}
+	return PlaceCategory(""), false
 }
 
 // umbrellaLocationTypes are Google's generic feature types that describe almost
@@ -175,15 +189,64 @@ func PriceyEatery(placeCategory PlaceCategory, priceLevel PriceLevel) bool {
 	return (placeCategory == PlaceCategoryEatery) && (priceLevel >= PriceLevelThree)
 }
 
-// EncodeNearbySearchRedisKey generates a Redis Key for Redis nearby search with place category and price info
-// The key includes the price level info for eatery and no price info for visit
-func EncodeNearbySearchRedisKey(placeCategory PlaceCategory, level PriceLevel) string {
-	keys := []string{"placeIDs", strings.ToLower(string(placeCategory))}
-	// add price levels for eatery category
-	if placeCategory == PlaceCategoryEatery {
-		keys = append(keys, fmt.Sprintf("level%d", level))
+// EncodeNearbySearchRedisKey generates the Redis geo-index key for a category's nearby search.
+//
+// One bucket per category, with no price segment. Eateries used to be split into
+// placeIDs:eatery:level0..4 keyed on each place's own price level, which fragmented the index
+// for no benefit: Google omits price_level for most places (so they collapsed into level0) and
+// only accepts a price filter at level >= 3, so searches for levels 0-2 were identical yet each
+// read back a fifth of the data. Callers that care about price already filter after the read
+// (matching.filterPlacesOnPriceLevel). Redis GEO is a sorted set scored by 52-bit geohash and
+// GEORADIUS probes 9 geohash cells at O(log N + M), so one bucket holds millions of members
+// without degrading — which is how placeIDs:visit has always worked.
+func EncodeNearbySearchRedisKey(placeCategory PlaceCategory) string {
+	return strings.Join([]string{"placeIDs", strings.ToLower(string(placeCategory))}, ":")
+}
+
+// searchCellDegrees sizes the freshness grid to iowrappers.ColdStartSearchRadius (~8 km), the
+// area one cold external search actually populates. A fixed-degree grid narrows in meters as
+// latitude rises, which only shrinks cells — erring toward an extra cold search, never toward
+// claiming coverage we do not have.
+const searchCellDegrees = 0.072
+
+// EncodeSearchCell quantizes coordinates to the freshness grid. This is a cache key, not a
+// spatial index: it is never range-queried, so it needs no neighbor probing or Z-order
+// ordering. The only property that matters is that a cell is no larger than the area a cold
+// search populates.
+func EncodeSearchCell(lat, lng float64) string {
+	return fmt.Sprintf("%d_%d",
+		int(math.Floor(lat/searchCellDegrees)),
+		int(math.Floor(lng/searchCellDegrees)))
+}
+
+// EncodeLastSearchTimeField identifies the external search variant that last covered a cell:
+//
+//	<cell>:<category>          the unfiltered search — every category, and eatery levels 0-2
+//	<cell>:eatery:pricey<N>    the price-filtered 4x-radius search, N in {3,4}
+//
+// Note this is scoped to the SEARCH, not to the bucket. Levels 0-2 share one field because
+// Google is issued an identical unfiltered request for all three, so two of every three
+// fan-outs were redundant. Levels 3-4 keep their own field because PriceyEatery makes Google
+// apply a real price filter at four times the radius: a fresh generic marker must not suppress
+// that search, or expensive places beyond the generic search's reach are never fetched.
+//
+// It is keyed on a location cell rather than country/admin1/city because the buckets it guards
+// are geo indexes read from arbitrary coordinates. A city name has no extent, so it cannot
+// answer "did we populate the area this query covers?" — a request 20 km from a city centroid
+// would read a marker claiming freshness over ground no search had reached.
+func EncodeLastSearchTimeField(placeCategory PlaceCategory, level PriceLevel, lat, lng float64) string {
+	segments := []string{EncodeSearchCell(lat, lng), strings.ToLower(string(placeCategory))}
+	if PriceyEatery(placeCategory, level) {
+		segments = append(segments, fmt.Sprintf("pricey%d", level))
 	}
-	return strings.Join(keys, ":")
+	return strings.Join(segments, ":")
+}
+
+// EncodeBrandLastSearchTimeField is the brand-search equivalent of EncodeLastSearchTimeField.
+// Brand buckets are geo indexes read from precise coordinates too, so they need the same cell
+// scoping.
+func EncodeBrandLastSearchTimeField(keyword string, lat, lng float64) string {
+	return strings.Join([]string{EncodeSearchCell(lat, lng), "brand", NormalizeBrandKey(keyword)}, ":")
 }
 
 // NormalizeBrandKey converts a brand keyword into a stable slug used in Redis keys and

POI/places.go

@@ -144,11 +144,12 @@ const (
 	PriceLevelDefault = 2
 )
 
-// AllPriceLevels enumerates the distinct price buckets an eatery can be filed
-// under. Eateries are partitioned by price on write (EncodeNearbySearchRedisKey
-// appends the level only for the Eatery category), so a merchant/category eatery
-// search — which has no price preference — must union across all of these or it
-// only sees one price tier. Other categories are single-bucket.
+// AllPriceLevels enumerates every price level a place can carry.
+//
+// Eateries used to be partitioned across one geo bucket per level
+// (placeIDs:eatery:level0..4); they no longer are, so this is not a list of buckets. Its
+// remaining use is the migration that unions those retired keys into placeIDs:eatery, which
+// needs to enumerate them. See EncodeNearbySearchRedisKey for why the split was collapsed.
 var AllPriceLevels = []PriceLevel{
 	PriceLevelZero, PriceLevelOne, PriceLevelTwo, PriceLevelThree, PriceLevelFour,
 }
@@ -165,10 +166,26 @@ func (place *Place) GetStatus() BusinessStatus {
 	return place.Status
 }
 
+// DefaultOpeningHours is the placeholder CreatePlace writes for any weekday the source data left
+// blank. Because it is always filled in, a stored place's Hours are never empty and their
+// emptiness cannot be used to detect missing data — use HasRealOpeningHours instead.
+const DefaultOpeningHours = "8:30 am – 9:30 pm"
+
 func (place *Place) GetHour(day Weekday) string {
 	return place.Hours[day]
 }
 
+// HasRealOpeningHours reports whether any weekday carries hours that came from source data
+// rather than the DefaultOpeningHours placeholder.
+func (place *Place) HasRealOpeningHours() bool {
+	for day := DateMonday; day <= DateSunday; day++ {
+		if hour := place.GetHour(day); hour != "" && hour != DefaultOpeningHours {
+			return true
+		}
+	}
+	return false
+}
+
 // KnownClosedOnDay reports whether the place's cached hours explicitly mark it closed on
 // the given weekday, e.g. "Sunday: Closed". Places with unknown or default hours return
 // false — absence of data is not treated as closed.
@@ -365,7 +382,7 @@ func CreatePlace(name, addr, formattedAddr, businessStatus string, locationType
 	// set default
 	for weekday = DateMonday; weekday <= DateSunday; weekday++ {
 		if place.GetHour(weekday) == "" {
-			place.SetHour(weekday, "8:30 am – 9:30 pm")
+			place.SetHour(weekday, DefaultOpeningHours)
 		}
 	}
 

docs/migrations/collapse-eatery-buckets.md

@@ -0,0 +1,166 @@
+# Migration: collapse the eatery price buckets
+
+One-time merge of `placeIDs:eatery:level0..4` into a single `placeIDs:eatery` geo index, so every
+category has exactly one bucket the way `placeIDs:visit` always has.
+
+Two ways to run it:
+
+- **`redis-cli ZUNIONSTORE`** — the pre-deploy path. One command, no application code involved.
+- **`GET /v1/migrate/union-eatery-buckets`** — admin only, dry-run unless `apply=true`. Reports
+  sizes and predicts the result, and is re-runnable. Only available *after* this change ships.
+
+## Ordering
+
+The union must land before the collapsed-key read goes live, or eatery reads hit a key that does
+not exist yet and every request returns empty until organic traffic refills it.
+
+But the HTTP endpoint **is part of the code being deployed**, so it cannot be the pre-deploy step
+— it does not exist until the deploy that introduces it. Use `redis-cli` for that step:
+
+```
+1. redis-cli union  →  2. deploy  →  3. verify (endpoint dry run)  →  4. delete the level* keys
+```
+
+The union is purely additive and invisible to the running code — it creates a key nothing reads
+yet — so step 1 is safe to run at any point beforehand.
+
+If you deploy first by mistake it is recoverable, not fatal: the new write path populates
+`placeIDs:eatery` organically and the union later merges the legacy members in (the target is
+included in the union sources, so nothing is lost). The cost is degraded eatery results until
+each area gets re-searched, plus the Google spend for those searches.
+
+Note what the ordering does **not** buy you: the marker re-key (see below) forces one cold search
+per occupied cell per category regardless of migration order. What running the union first
+protects is the ~8.5k existing eatery members staying readable in the meantime.
+
+## Why the split is going away
+
+Eateries were filed under `placeIDs:eatery:level<N>` keyed on each place's own price level, while
+reads asked for the level the *caller* wanted. Three things made that lossy:
+
+- Google omits `price_level` for most places, so `res.PriceLevel == 0` and they nearly all landed
+  in `level0`.
+- Google only accepts a price filter at level ≥ 3, so searches for levels 0/1/2 issued
+  **identical** requests — three cold fan-outs per area instead of one — then scattered the
+  results across five buckets and each read back its own fifth.
+- Price selection was already happening downstream anyway, in
+  `matching.filterPlacesOnPriceLevel` (`planner/solver.go`).
+
+Redis GEO is a sorted set scored by 52-bit geohash and `GEORADIUS` probes 9 geohash cells at
+O(log N + M), so one bucket holds millions of members without degrading. The split bought nothing
+and cost 5× fragmentation.
+
+## `AGGREGATE MIN` is mandatory
+
+A GEO member's score **is** its 52-bit geohash. `redis.ZStore.Aggregate` defaults to `SUM`, which
+would add the scores of any place present in two source buckets and silently relocate it — in
+practice into the ocean. The migration passes `MIN`, which keeps a real geohash; a place's
+coordinates are identical across buckets, so which one survives does not matter.
+
+`TestUnionEateryPriceBucketsPreservesCoordinates` pins this by seeding one place into two buckets
+and asserting a 100 m `GEORADIUS` around its true coordinates still finds it.
+
+The target key is included in the union sources, so the migration is re-runnable and cannot drop
+members that already-deployed code has written to the collapsed key.
+
+## Steps
+
+```bash
+BASE=https://best-vacation-planner.herokuapp.com
+# Heroku: eval $(heroku config:get REDIS_URL -a <app>) or use `heroku redis:cli -a <app>`
+R="redis-cli -u $REDIS_URL"
+
+# --- 1. Union, before deploying. -------------------------------------------------
+# Record the starting sizes so step 3 has something to check against.
+for L in 0 1 2 3 4; do echo -n "level$L "; $R ZCARD "placeIDs:eatery:level$L"; done
+$R ZCARD placeIDs:eatery   # expected 0 on a first run
+
+# Note the SIX keys: the target is included so the command is idempotent and cannot
+# drop members already written to the collapsed key. AGGREGATE MIN is mandatory.
+$R ZUNIONSTORE placeIDs:eatery 6 \
+  placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \
+  placeIDs:eatery:level3 placeIDs:eatery:level4 placeIDs:eatery AGGREGATE MIN
+
+$R ZCARD placeIDs:eatery   # <= the sum above; lower means a place was in two buckets
+
+# Spot-check that a geohash score survived, against that place's own record.
+# These two must agree to within a few metres.
+ID=$($R ZRANGE placeIDs:eatery 0 0 | head -1)
+$R GEOPOS placeIDs:eatery "$ID"
+$R GET "place_details:place_ID:$ID" | jq '.Location'
+
+# --- 2. Deploy. -----------------------------------------------------------------
+
+# --- 3. Verify. -----------------------------------------------------------------
+# The endpoint exists now. A dry run re-reports the sizes and predicts the same count,
+# which confirms the deployed code resolves the key you just populated.
+curl -s -H "Authorization: Bearer $ADMIN_JWT" \
+  "$BASE/v1/migrate/union-eatery-buckets" | jq
+
+# A non-zero eatery count — which /stats/places could never report before, because it
+# built "placeIDs:eatery", a key nothing wrote.
+curl -s -H "Authorization: Bearer $ADMIN_JWT" "$BASE/stats/places" | jq
+
+# --- 4. Only once the deploy is healthy. ----------------------------------------
+$R DEL placeIDs:eatery:level0 placeIDs:eatery:level1 placeIDs:eatery:level2 \
+       placeIDs:eatery:level3 placeIDs:eatery:level4
+```
+
+To roll back before step 4, restore from the backup taken in step 1 — the `level*` keys are
+untouched until then, so `ZUNIONSTORE` can also simply be re-run.
+
+## Five source keys, not six — and why the target needs a backup
+
+`placeIDs:eatery` **already existed in production** when this ran, holding 7,113 members. It is a
+pre-2023 artefact: the price split was introduced in `20c1eb7` (2023-02-20), and before that
+eateries were written to exactly this un-suffixed name. Nothing has read it since, so it sat as
+dead data for roughly three years — until this change made that name the live read key again.
+
+4,634 of those members existed in no `level*` bucket. Sampling 300 of them:
+
+| | |
+| --- | --- |
+| no `place_details` record (orphans) | 13 (4.3%) |
+| records with **no `Types` field at all** | 286 of 287 |
+| primary type detectably non-Eatery | 1 (a hotel) |
+
+The missing `Types` is what settles it: those records predate the field, and both
+`POI.ReclassifyForCategory` and the purge migration deliberately *keep* no-Types records, so
+neither can audit them. Spot checks turned up a horse racing track filed as an eatery. Merging
+~4,600 unauditable three-year-old records into the live eatery bucket — right after an incident
+about misclassified places in eatery buckets — invites a repeat.
+
+So the pre-deploy union uses **five** source keys and lets `ZUNIONSTORE` overwrite the target,
+producing exactly the 8,500 members production already serves. No coverage regression, because
+those 4,634 were not being served. Their `place_details` records are untouched, so reviving them
+later stays possible as a deliberate decision.
+
+Because that overwrites the target, **back it up first**:
+
+```bash
+$R COPY placeIDs:eatery placeIDs:eatery:pre-collapse-backup   # preserves scores exactly
+```
+
+Use the **six**-key form (target included) only for re-runs *after* the deploy, where the new
+write path is filling the target and an overwrite would drop members it has already written.
+
+## Expect a small number of members in two price buckets
+
+34 members were in more than one `level*` bucket — the same place written under different price
+levels as Google's answer changed over time, which is the fragmentation this change removes. For
+those, `AGGREGATE MIN` picks one of the two real positions, which may differ from the coordinates
+in the `place_details` record by tens of metres. That is pre-existing inconsistency surfacing, not
+migration damage: verified 33 exact matches and one 82 m difference that was present in the source
+buckets beforehand. A `SUM` corruption looks nothing like this — it roughly doubles the score and
+throws the place thousands of kilometres.
+
+`expected_after` is the deduped union size, so it will be **lower** than `source_total` whenever a
+place appears in more than one price bucket. That is the intended outcome, not data loss.
+
+## Marker fields are also changing
+
+The same change re-keys `MapsLastSearchTime` from `<country>:<admin1>:<city>:…` to
+`<cell>:<category>` (see [reclassify-buckets.md](reclassify-buckets.md#steps) for the format).
+Old fields are simply never read again. Expect one cold search per occupied cell per category
+after deploy; the geo buckets are untouched, so reads return the full member set immediately and
+only the markers re-establish. `redis-cli DEL MapsLastSearchTime` afterwards is optional cleanup.