perf(statistics): scale large aggregate buckets (gates F8 mace all-time)

.planning/phases/01-game-type-aware-statistics-parity/PERF-REVIEW.md

@@ -0,0 +1,119 @@
+# Review — branch fix/perf-large-bucket-identity vs master (ebdc4ea)
+
+**Scope:** Single commit 34aae9d — behavior-preserving perf fix for statistics recalc.
+Files changed: `repository.ts`, migration `0010_...sql`, 4 test files (+2 new).
+Focus: byte-identity of aggregate output across all three changes (F3 identity index, F4 set-based resolve, F5 batched inserts).
+
+**Gates:**
+- API contract: ✅ No route or schema touched — internal repository functions only. No OpenAPI impact.
+- Layer/DI: ✅ All changes stay inside `src/modules/statistics/repository/`. No cross-module import changes, no DI wiring changed.
+- Migration: ✅ `IF NOT EXISTS`, functional index only, additive, 0009 untouched.
+
+---
+
+## Blockers 🔴
+
+_none_
+
+---
+
+## High 🟠
+
+_none_
+
+---
+
+## Medium 🟡
+
+### 1. `repository.ts:919` [correctness] — `display_name` indexed unconditionally for ALL loaded identities, including steam-id-only matches
+
+`buildPlayerIdentityIndex` calls `pushTo(byLowerName, identity.display_name.toLowerCase(), entry)` for every row, regardless of how `loadPlayerIdentities` fetched it. A row fetched only because `psi.steam_id = any($steamIds)` (no display_name or nickname match against observed names) gets bucketed under its display_name in `byLowerName`. When a player's `player.n` happens to equal that display_name, it surfaces as a candidate — `playerIdentityMatchPriority` then returns `DISPLAY_NAME_MATCH_PRIORITY` and the row gets selected.
+
+**This is not a regression:** the old `bestPlayerIdentity` O(n) scan ran `playerIdentityMatchPriority` against ALL loaded identities including steam-id-fetched ones, so the display_name match would have fired there too. Behavior is identical.
+
+The risk is that the comment in the JSDoc ("candidates are only the rows indexed under the player's steam id and lower(name)") implies the index is narrow, but it's actually wider than stated — every row's display_name is indexed regardless of whether it is an observed name. If `loadPlayerIdentities` later changes to fetch additional rows (e.g. by relation), the index could surface false candidates where the old scan would too — so parity is still preserved, but the index is silently broader than its documentation claims.
+
+**Fix:** Narrow the JSDoc claim or, if tighter correctness guarantees are desired in future, track which keys were "observed names" separately. No code change required for current behavior.
+
+### 2. `repository.ts:1369` [correctness] — `JSON.stringify(row.stats)` changes serialization path for jsonb
+
+Old per-row insert passed `row.stats` (a plain TS object) directly as a pg parameter. pg serializes TS objects to JSON automatically when the column is `jsonb`. New insert does `JSON.stringify(row.stats)` and passes the string; pg accepts pre-serialized JSON strings for jsonb columns.
+
+Both produce the same stored JSONB, **but** if `row.stats` ever contains a value that `JSON.stringify` handles differently from pg's own object serialization (e.g. a `Date` property — `JSON.stringify` produces `"2024-..."` while pg may coerce differently, or a `toJSON()` override), the results would diverge. Current `stats` type (`PlayerAggregateRow.stats`) has only `number` and `version: 1` fields, so no divergence today.
+
+The same applies to `JSON.stringify(row.inputs)` for bounty (line 1465) — `BountyPointRow.inputs` has `number` and literal fields only, so no divergence today.
+
+**Fix:** Document the pre-serialization assumption in a comment, or assert in the test that the round-tripped JSON equals the original object. Low urgency given current types. Also applies at `repository.ts:1388` (squad stats) and `repository.ts:1465` (bounty inputs).
+
+### 3. `repository.ts` [correctness] — `ensureNameFallbackIdentities` OR-split drops the `cp.id = pn.player_id` join in the nickname EXISTS, widening the match set
+
+Old query:
+```sql
+left join player_nicknames pn on pn.player_id = cp.id
+where lower(cp.display_name) = lower(occ.name)
+   or (lower(pn.nickname) = lower(occ.name) and ...)
+```
+The `cp.id = pn.player_id` join scoped the nickname side to nicknames belonging to the same canonical player as the display_name side. The new second EXISTS:
+```sql
+exists (select 1 from player_nicknames pn where lower(pn.nickname) = lower(occ.name) and ...)
+```
+has no join to `canonical_players` — it matches any nickname row across all players.
+
+**This is semantically equivalent for the function's purpose:** the question asked is "does this (name, ts) occurrence already have SOME canonical identity?" not "is it the same canonical player for both halves?" The widened second EXISTS can only ever mark MORE occurrences as already-covered (potentially preventing duplicate canonical_player creation), never fewer. So it is at least as conservative as the old query.
+
+**However**, if a nickname exists that matches `occ.name` in the window but belongs to a player whose display_name is unrelated, the old query would NOT have matched that occurrence (because the outer EXISTS required a single cp row satisfying BOTH display_name OR nickname via the join), while the new EXISTS DOES match it. This means the new code could skip creating a canonical_player for a name that the old code would have created one for.
+
+**Worked example:** Suppose no canonical_player has `lower(display_name) = 'ace'`, but a player_nicknames row exists with `nickname = 'ace'` in the active window. Old code: `exists (select 1 from cp left join pn on pn.player_id = cp.id where lower(cp.display_name) = 'ace' or (lower(pn.nickname) = 'ace' and ...))`. The first half of the OR is never satisfied (no cp has display_name 'ace'). For the second half to fire, there must be a `cp` row such that `pn.player_id = cp.id AND lower(pn.nickname) = 'ace'` — this EXISTS fires if the player with nickname 'ace' exists in canonical_players (which they do by assumption). So the old query ALSO finds this match. **The semantic widening is a non-issue in practice** because `player_nicknames.player_id` always references a `canonical_players.id` (FK), so any pn row's `player_id` always has a matching cp row — the old LEFT JOIN was redundant for the OR's right side.
+
+The comment in the code states "identical to the original OR" which is correct given the FK constraint. But the analysis relies on the FK being enforced — worth a brief comment noting this.
+
+**Fix:** Add a comment noting that the second EXISTS omits the cp join because `player_nicknames.player_id` is an FK to `canonical_players.id`, making the join redundant for EXISTS purposes.
+
+---
+
+## Low 🔵
+
+### 4. `repository.ts:886-927` [naming] — `IndexedIdentity` interface is unexported but `PlayerIdentityIndex` and `buildPlayerIdentityIndex` are exported; `IndexedIdentity` appears in the public `PlayerIdentityIndex` type indirectly only through the `Map` value type
+
+`PlayerIdentityIndex` exposes `Map<string, IndexedIdentity[]>` in its public interface, meaning callers who import `PlayerIdentityIndex` will encounter `IndexedIdentity` in inferred types but cannot name it. `IndexedIdentity` should either be exported alongside the other two or the maps should be typed with an inline type (the intent is the latter). Minor since no external consumer currently destructures the maps.
+
+### 5. `tests/batched-write.test.ts:474` [tests] — `countQueries(client, "select occ.idx")` is a prefix match but the actual SQL starts with multiple leading newlines
+
+`ScriptedClient.query` trims SQL (`sql.trim()`) before storing in `queries`. The actual query SQL added in this diff starts with `\n      select occ.idx` — after trim it starts with `select occ.idx`. `countQueries` uses `startsWith("select occ.idx")`. This is correct; the trim makes it safe. No bug, but a future SQL reformatting (e.g. adding a CTE before the SELECT) would silently break the count assertion without a compile error. Note for future maintainability.
+
+### 6. `insert-assertions.ts:20-33` [tests] — `bountyInsertRows` re-zips with unchecked array lengths
+
+The function destructures `[rotationId, gameType, playerIds, points, inputs]` and zips on `inputs.map((inputJson, row) => [..., playerIds[row], points[row], ...])`. If `playerIds` or `points` is shorter than `inputs` (a param count regression), `playerIds[row]` silently returns `undefined` instead of failing fast. The test then asserts on the re-zipped row, and the undefined would propagate without a clear assertion failure pointing at the mismatch.
+
+**Fix:** assert `playerIds.length === inputs.length && points.length === inputs.length` before the zip, or use `zip`-style utility that throws on length mismatch.
+
+---
+
+## Non-Findings Checked
+
+- **Identity tie-break (first-in-array on equal priority):** The new `toSorted` secondary key `left.candidate.order - right.candidate.order` (ascending order = earlier array element wins) correctly reproduces the stable sort behavior of the old code. The `order` field is set from `identities.entries()` (0-based insertion order), so the tie-break is byte-identical. ✓
+
+- **Inactive nicknames not matching:** `buildPlayerIdentityIndex` indexes ALL nicknames (active or not) under `byLowerName`. When an inactive nickname is a candidate, `playerIdentityMatchPriority` returns `undefined` (nickname window check fails, display_name check also fails for a name-mismatch) and the candidate is excluded via `flatMap`. The test at `identity-index.test.ts:725` (player `eid:4, n:"Char"` — expired nickname) confirms this. ✓
+
+- **`ensureNameFallbackIdentities` timestamp parameter:** `occ.ts` is correctly bound to `$2::timestamptz[]` (the `replayTimestamp` per occurrence), and the correlated nickname window check `pn.observed_from <= occ.ts` is a proper lateral reference. PostgreSQL allows outer-query column references in EXISTS subqueries. ✓
+
+- **Batched unnest column order vs INSERT column list:** All three new unnest inserts specify explicit column lists in the INSERT clause (`insert into player_stats (rotation_id, player_id, stats, game_type, is_show)`) and the SELECT uses named aliases from the `as data(...)` clause — no positional dependency that could silently swap columns. ✓
+
+- **Empty arrays / empty loop equivalence:** All three batched inserts are guarded by `.length > 0`; an empty input produces no INSERT, exactly matching the old empty `for` loop. ✓
+
+- **`player_id` nullable in `commander_side_stats`:** `aggregates.map((row) => row.playerId)` produces `(string | undefined)[]`; the cast `$3::uuid[]` accepts NULL positionally via unnest. The test `commander.test.ts:593` confirms `[null]` is the first element when commander has no resolved identity. ✓
+
+- **`display_name` non-nullable in `PlayerIdentityRow`:** TypeScript types `display_name: string` (not `string | null`), so `identity.display_name.toLowerCase()` in `buildPlayerIdentityIndex` is safe — no null-dereference risk. ✓
+
+- **Migration 0009 untouched:** Diff touches only 0010 (new file). Migration sequence is intact. ✓
+
+---
+
+## Verdict
+
+**APPROVE**
+
+No blockers, no high findings. The three medium findings (1, 2, 3) are documentation/future-proofing concerns — the code is correct for all current inputs. Finding 3 (OR-split FK analysis) is the highest-risk item and is proven equivalent given the FK constraint. The identity index, tie-break, and batched inserts are all byte-identical to the original for every reachable input.
+
+Mandatory before merge: none.
+Optional follow-up: findings 1, 2, 3 (comment clarity), 6 (test robustness).

.planning/quick/PERF-large-bucket-identity-SUMMARY.md

@@ -0,0 +1,88 @@
+# PERF — Large-bucket identity / recalc scaling (F8 gate)
+
+**Branch:** `fix/perf-large-bucket-identity`
+**Commit:** `34aae9d` (single atomic commit)
+**Status:** complete — full `pnpm verify` green (100% coverage, OpenAPI diff empty)
+
+Behavior-preserving performance fixes for the F8 large all-time buckets (gates the
+~20,735-replay `mace` all-time recalc in one pass). Addresses 01-REVIEW findings 3, 4, 5.
+No aggregate output changes: `player_stats` / `squad_stats` / `commander_side_stats` /
+`bounty_points` and the fallback `canonical_players` / `player_nicknames` rows are byte-identical.
+
+## Finding → Fix
+
+### Finding 3 — O(1) identity resolution (`repository.ts`)
+- **Before:** `bestPlayerIdentity` `.flatMap`+`.toSorted` over the ENTIRE identities array per
+  player per replay → O(replays × players × |identities|).
+- **Fix:** `buildPlayerIdentityIndex` builds, once per bucket, a `bySteamId` + `byLowerName`
+  lookup (each identity indexed under its `steam_id`, `lower(display_name)`, and `lower(nickname)`,
+  preserving original array order via a stored `order`). `bestPlayerIdentityIndexed` scores only
+  the few candidates reachable from a player's steam id / lower(name) using the **unchanged**
+  `playerIdentityMatchPriority`, then selects highest priority with first-original-array-order
+  tie-break — replicating the prior stable `.toSorted((l,r)=>r.priority-l.priority)[0]` exactly.
+- Applied to both the player path (`resolvedPlayers`) and the commander path (`commanderIdentities`).
+- **Candidate-completeness proof:** `playerIdentityMatchPriority` returns a priority only via
+  steam_id==sid, lower(nickname)==lower(name), or lower(display_name)==lower(name) — all three are
+  exactly the index keys, so no matching identity is ever missed.
+
+### Finding 4 — fallback resolve + missing index (`repository.ts`, migration 0010)
+- **New index:** `0010_canonical_players_display_name_lower_index.sql` →
+  `create index if not exists idx_canonical_players_display_name_lower on canonical_players (lower(display_name))`
+  (mirrors the existing `idx_player_nicknames_nickname` on `lower(nickname)`). Confirmed applied
+  in the live dev DB. Index-only, no behavior change.
+- **Resolve restructure:** the per-occurrence correlated `where exists (cp LEFT JOIN pn WHERE
+  displayMatch OR nickMatch)` is rewritten into ONE set-based pass with two index-sargable EXISTS
+  halves (`exists(cp where lower(display_name)=lower(name))` OR `exists(pn where lower(nickname)=
+  lower(name) AND window)`). Predicate is provably identical (the LEFT JOIN's nickMatch only reads
+  `pn` columns, and every `pn` belongs to a `cp`, so the two independent EXISTS = the original OR).
+  Statement count unchanged (one query); output ordinalities unchanged.
+- **`loadPlayerIdentities` fan-out:** left as-is. The N×M `cp ⨯ steam_ids ⨯ nicknames` rows are now
+  consumed in O(1) by the index, so the only residual cost is data transfer, bounded by distinct
+  observed names per bucket. Deduplicating in SQL would change row identity/order and risk
+  cross-player tie-break drift — **not** behavior-preserving — so it is deliberately not changed.
+
+### Finding 5 — batch the replace inserts (`repository.ts`)
+- `replaceAggregateRows` / `replaceCommanderRows` / `replaceBountyRows` converted from one
+  `await client.query(insert ...)` per row to a single multi-row
+  `insert ... select * from unnest(...)` per table (the F7/FW2 pattern).
+- Byte-identical: each `unnest` column carries the same per-row scalar in the same order;
+  constant `rotation_id` / `game_type` passed as reused scalars; `jsonb` columns (`stats`,
+  `inputs`) JSON-serialized per element exactly as the single-param insert serialized them;
+  `numeric(12,2)` `points` stored identically; empty arrays produce zero rows (matching the
+  empty loop). Per-table delete predicates unchanged.
+
+## How byte-identity is proven
+1. **Real-pg parity harness unchanged & green** (`postgres.test.ts`, 185 integration tests):
+   per-replay vs set-based, per-type, all-time, and `is_show` comparisons all still pass against
+   the new batched inserts and O(1) resolution — the strongest oracle.
+2. **`identity-index.test.ts` (new):** cross-checks `bestPlayerIdentityIndexed` against a verbatim
+   re-implementation of the prior per-player scan on an adversarial seeded corpus (steam match,
+   active/expired nickname windows, duplicate display names for the tie-break, nickname-beats-
+   display, steam-beats-name, no-match) across a 4-point timestamp sweep.
+3. **`batched-write.test.ts` (new):** ScriptedClient query-count assertions — exactly ONE insert
+   per aggregate table (not per-row) and exactly ONE `select occ.idx` set-based fallback resolve
+   regardless of occurrence count; batched commander/bounty params carry all rows.
+4. Updated `commander.test.ts` / `bounty.test.ts` assertions to the batched param shape
+   (`bountyInsertRows` re-zips the batch back to per-row tuples) — strengthened with explicit
+   "one insert per scope" length checks, not weakened.
+
+## Verification
+`pnpm verify` (PG/RabbitMQ/MinIO up): format, lint, typecheck, 617 unit + 185 integration tests,
+OpenAPI contract diff EMPTY, ops checks, **100% coverage** (stmts/branches/funcs/lines).
+
+## Files
+- `src/infra/db/migrations/0010_canonical_players_display_name_lower_index.sql` (new index)
+- `src/modules/statistics/repository/repository.ts` (findings 3/4/5)
+- `src/modules/statistics/repository/tests/identity-index.test.ts` (new)
+- `src/modules/statistics/repository/tests/batched-write.test.ts` (new)
+- `src/modules/statistics/repository/tests/insert-assertions.ts` (new helper)
+- `src/modules/statistics/repository/tests/{bounty,commander}.test.ts` (assertions updated)
+
+## Deviations
+None beyond the documented scope. No architectural changes; `loadPlayerIdentities` fan-out left
+intact for behavior-preservation (documented above). Out-of-scope items (classification, bucketing,
+is_show formula, legacy-export shape, OpenAPI) untouched.
+
+## Operational note
+Migration 0010 must be applied on deploy (`pnpm db:migrate`). It is index-only and additive; no
+recalc is required by this change (unlike 0009).

src/infra/db/migrations/0010_canonical_players_display_name_lower_index.sql

@@ -0,0 +1,16 @@
+-- 0010_canonical_players_display_name_lower_index: index lower(canonical_players.display_name).
+--
+-- The name-fallback identity resolve (statistics recalc, `ensureNameFallbackIdentities`) and the
+-- `loadPlayerIdentities` join both probe `lower(canonical_players.display_name)` against the set of
+-- observed parser names for a recalc bucket. A functional index on lower(nickname) already exists
+-- (`idx_player_nicknames_nickname`, migration 0001), but the matching index on
+-- lower(canonical_players.display_name) was never added — so the display_name half of every probe
+-- is a sequential scan. On the F8 all-time `mace` bucket (~20735 replays in one recalc pass) that
+-- is a seq scan per distinct observed name, the recalc hot spot the parity-driver flagged.
+--
+-- This is a behavior-preserving performance index only: it changes query plans, never results.
+-- Run inside the migrate.ts transaction wrapper, so a plain (non-CONCURRENTLY) build is used; the
+-- table is small relative to the recalc scan it accelerates. Idempotent via `if not exists`.
+
+create index if not exists idx_canonical_players_display_name_lower
+  on canonical_players (lower(display_name));