diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e1e201..8143d7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added -- **"Prepare database" is now something you can run** — Maintenance → Databases +- **"Prepare database" is now something you can run** — Maintenance → Database has a card that runs the whole pipeline in the right order (fetch FIDE IDs, merge duplicate players, normalise names, deduplicate games, rebuild the position index). It is the same pass that runs by itself after an import, and @@ -17,6 +17,35 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 other work is running queues it rather than refusing. (#255) ### Fixed +- **Duplicate players with the same name are now merged automatically** — + automatic player merging only ever looked at FIDE IDs, so two records for one + person with no FIDE ID between them stayed split forever. That also hid + duplicate *games*: two games are only recognised as the same game when they + name the same two player records, so every copy hanging off the second record + survived deduplication too — the games list showed the same game twice and no + maintenance step could clean it up. Player merging now also folds together + records whose names match once capitals, commas and spacing are ignored — the + same key the importer already identifies people by, so this restores an + invariant rather than guessing. A name held by two *different* FIDE IDs is + left untouched: those are namesakes FIDE distinguishes, and a merge cannot be + undone. The merge keeps whichever record carries the FIDE ID, and refreshes + every affected game count. Since this runs in the pass that follows each + import, affected databases repair themselves on the next import — or straight + away via Maintenance → Players → "Merge duplicate players — automatic". + Duplicates that are spelled differently *and* have no FIDE ID still need the + manual merge. (#266) +- **Merging players now re-opens their games for deduplication** — game + deduplication pairs games on their two player records, so every verdict it + reached while a person's records were split is stale. Its routine pass only + looks at games it has not seen before, so the duplicate copies a merge had + just exposed were never re-examined and survived indefinitely. Merging — by + hand or automatically — now marks the kept player's games for another look. + (#266) +- **Name normalisation runs before player merging, not after** — renaming a + record to its FIDE-canonical spelling can itself produce a duplicate, when the + new name is one another record already holds. With merging running first, each + maintenance pass ended by creating duplicates that only the *next* pass would + clean up. (#266) - **A failed import no longer breaks the maintenance run that follows it** — "Merge duplicate players" (and deduplication) died with *"Cannot create index with outstanding updates"*, because a failure part-way through an import left @@ -27,6 +56,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 run repairs a database left in this state instead of failing on it. (#255) ### Changed +- **Player merging can be previewed before it runs** — "Merge duplicate players + — automatic" has a Preview button that lists every merge it would make (which + record is kept, which are folded in, and whether a FIDE ID or the name alone + linked them) and changes nothing. Merges cannot be undone, so a first run on a + large database is worth looking at first — particularly the ones linked by + name alone, the only ones that could be genuine namesakes. Also available as + `chess-db players dedup --dry-run`. (#266) +- **The Maintenance tools are grouped by what they act on, and say what they + do** — "Deduplication" was games deduplication, "Merge duplicate players" was + the automatic one, and "Merge players" (on a different tab) was the manual + one; all three names were guesses. They are now **"Remove duplicate games"**, + **"Merge duplicate players — automatic"** and **"Merge two players — + manual"**, and the tabs hold what their name says: **Database** (prepare, + remove duplicate games, position index, soft-deleted games) and **Players** + (fetch FIDE IDs, the two merges, normalise names, FIDE list, player reference + file). Each tab opens with a line explaining the order its steps belong in — + in particular that duplicate games are recognised through their two player + records, so merging duplicate players comes first. The automatic merge now + states that it only finds records sharing a FIDE ID, and points at the manual + merge for the rest. (#266) - **Enabling LAN access now says what else is needed** — after installing with "Allow other computers on this network to connect", Windows explains the three remaining steps: read the access token from an elevated PowerShell, enter it in diff --git a/chess-client/src/components/ActivityIndicator.tsx b/chess-client/src/components/ActivityIndicator.tsx index 07a8958..e61aaa4 100644 --- a/chess-client/src/components/ActivityIndicator.tsx +++ b/chess-client/src/components/ActivityIndicator.tsx @@ -38,8 +38,8 @@ function jobLabel(j: Job): string { case "update": return "Scheduled update"; case "fide_refresh": return "Update FIDE player list"; case "index_positions": return "Build position index"; - case "dedup_games": return "Deduplicate games"; - case "dedup_players": return "Merge duplicate players"; + case "dedup_games": return "Remove duplicate games"; + case "dedup_players": return p.dry_run ? "Preview duplicate players" : "Merge duplicate players"; case "cleanup": return "Clean up games"; case "normalise": return "Normalise player names"; case "resolve_fide": return "Fetch missing FIDE IDs"; @@ -53,7 +53,7 @@ function jobLabel(j: Job): string { if (f) return c ? `Import ${f} → ${c}` : `Import ${f}`; return c ? `Import PGN → ${c}` : "Import PGN"; } - case "maintenance_pending": return "Prepare database — resolve · dedup · normalise · index"; + case "maintenance_pending": return "Prepare database — resolve · normalise · dedup · index"; case "players_import": return "Import players"; case "players_export": return "Export players"; case "backup": return p.collection ? `Backup ${p.collection}` : "Backup"; diff --git a/chess-client/src/components/MaintenancePanel.tsx b/chess-client/src/components/MaintenancePanel.tsx index 1d65333..252c7cd 100644 --- a/chess-client/src/components/MaintenancePanel.tsx +++ b/chess-client/src/components/MaintenancePanel.tsx @@ -543,10 +543,10 @@ function PrepareDatabaseSection({ onMutated }: { onMutated?: () => void }) { return (

- Runs every step below in the right order: fetch FIDE IDs, merge duplicate players, - normalise names, deduplicate games, rebuild the position index. This is what runs by - itself after an import — start it here if a run was interrupted, or after restoring - a database. + Runs every maintenance step in the right order: fetch FIDE IDs, normalise names, merge + duplicate players (all on the Players tab), remove duplicate games, rebuild the position + index. This is what runs by itself after an import — start it here if a run was + interrupted, or after restoring a database.

void run()} disabled={state === "starting"}> @@ -570,24 +570,46 @@ function PrepareDatabaseSection({ onMutated }: { onMutated?: () => void }) { ); } -// ── Merge duplicate players (same FIDE ID) ──────────────────────────────────── +// ── Merge duplicate players, automatically (same FIDE ID or same name) ──────── function DedupPlayersSection({ onMutated }: { onMutated?: () => void }) { const progress = useJobProgress("maint-dedup-players"); + // Which of the two buttons is running, so the progress row doesn't claim to be + // merging while it is only previewing. + const [preview, setPreview] = useState(false); useEffect(() => { if (progress.done) onMutated?.(); }, [progress.done]); return ( - +

- Merge player records that share the same FIDE ID (e.g. name variants across sources), - reassigning their games to a single row. Run after fetching FIDE IDs. + Searches the whole database for records that are one person — they share a FIDE ID, or + they share a name once capitals, commas and spacing are ignored — and merges each set + into one, moving all their games with them. Two records with different FIDE IDs + under one name are left alone: FIDE says they are different people. Running “Fetch + missing FIDE IDs” first links more of them. Duplicates spelled differently with no FIDE + ID on either side still need “Merge two players”.

{!progress.running && !progress.done && ( - void progress.run(["players", "dedup"])}> - Merge duplicates - +
+
+ {/* Preview first: merging cannot be undone, so the safe action is + the one offered alongside, not buried behind a checkbox. */} + { setPreview(true); void progress.run(["players", "dedup", "--dry-run"]); }}> + Preview + + { setPreview(false); void progress.run(["players", "dedup"]); }}> + Find and merge duplicates + +
+

+ Preview lists every merge it would make and changes nothing. Merging cannot be undone. +

+
)} {(progress.running || progress.done) && ( - + )} ); @@ -652,9 +674,9 @@ function FideRefreshSection() { ); } -// ── Deduplication section ───────────────────────────────────────────────────── +// ── Remove duplicate games ──────────────────────────────────────────────────── -function DeduplicationSection({ onMutated }: { onMutated?: () => void }) { +function DedupGamesSection({ onMutated }: { onMutated?: () => void }) { const progress = useJobProgress("maint-dedup"); // Full re-checks every game (cleans duplicates an earlier pass missed, e.g. the // same game across TWIC and a Lichess broadcast); incremental only checks games @@ -673,9 +695,11 @@ function DeduplicationSection({ onMutated }: { onMutated?: () => void }) { }, [progress.done]); return ( - +

- Detects and removes duplicate games resulting from overlapping collections. + Finds games stored more than once — the same game from two overlapping sources, or a PGN + imported twice — and keeps the most complete copy. Two games only match when they name the + same two player records, so merge duplicate players (Players tab) first.

{!progress.running && !progress.done && ( <> @@ -700,7 +724,7 @@ function DeduplicationSection({ onMutated }: { onMutated?: () => void }) { ? "Full — re-checks every game (slower). Cleans duplicates an earlier pass missed." : "Incremental — only games added since the last pass (fast); same as the automatic sweep."}

- Run deduplication + Remove duplicate games )} {(progress.running || progress.done) && ( @@ -784,7 +808,7 @@ function NormaliseSection({ onMutated }: { onMutated?: () => void }) {

Update player names to their FIDE-canonical form using the locally-stored FIDE list. This runs instantly — no online lookups. If names don't change, update the FIDE list - first (FIDE player list, above). + first (“FIDE player list”, at the top of this tab).

{!progress.running && !progress.done && ( Normalise names @@ -1040,17 +1064,19 @@ function PurgeSection({ status, onMutated }: { status: StatusInfo | null; onMuta // ── Panel shell ─────────────────────────────────────────────────────────────── -// ── Merge players section ───────────────────────────────────────────────────── +// ── Merge two players, by hand ──────────────────────────────────────────────── function MergePlayersSection({ onMutated }: { onMutated?: () => void }) { const [open, setOpen] = useState(false); return ( - +

- Combine duplicate player records — e.g. a full name (“Karpov, Anatoly”) and a surname-only - entry (“Karpov”) for the same person — into one. All games move to the player you keep. + Pick two records yourself and combine them — e.g. a full name (“Karpov, Anatoly”) and a + surname-only entry (“Karpov”) for the same person. All games move to the record you keep. + This is the way to fix duplicates the automatic merge cannot see: the two spellings differ, + and neither record carries a FIDE ID to link them.

- setOpen(true)}>Merge players… + setOpen(true)}>Choose two players… {open && ( setOpen(false)} onMerged={() => onMutated?.()} /> )} @@ -1063,12 +1089,20 @@ function MergePlayersSection({ onMutated }: { onMutated?: () => void }) { const TABS = [ { id: "sources", label: "Sources" }, - { id: "databases", label: "Databases" }, + { id: "databases", label: "Database" }, { id: "players", label: "Players" }, { id: "others", label: "Others" }, ] as const; type TabId = (typeof TABS)[number]["id"]; +/** One sentence above a tab's cards saying what the tab is for and in which + * order its steps belong — the grouping alone never conveyed that, and the + * Players/Database split only makes sense once you know duplicate games are + * matched through their player records. */ +function TabLead({ children }: { children: React.ReactNode }) { + return

{children}

; +} + function TabBar({ active, onChange }: { active: TabId; onChange: (id: TabId) => void }) { // M3 primary tabs — a row of text labels with an active underline indicator. return ( @@ -1097,8 +1131,9 @@ function TabBar({ active, onChange }: { active: TabId; onChange: (id: TabId) => export default function MaintenancePanel({ onRunWizard, status, onMutated, connection = "connected" }: Props) { // Full-screen, non-modal view (driven by App's `mode` state). Mirrors the home // screen's layout: a centred max-width column on the bg-surface base. The tools - // are grouped into tabs (Databases / Players / Others) to keep each view - // uncluttered; the database overview stays pinned above the tabs. + // are grouped by what they act on — Sources (where games come from), Database + // (the games), Players (who played them), Others (the app itself) — to keep + // each view uncluttered; the database overview stays pinned above the tabs. const [tab, setTab] = useState("sources"); // Inactive tabs are hidden, not unmounted, so a long-running job (e.g. a TWIC @@ -1153,29 +1188,53 @@ export default function MaintenancePanel({ onRunWizard, status, onMutated, conne
- {/* Maintenance tasks in recommended run order — the same identity-first - pipeline the background maintenance runs automatically after imports. - "Prepare database" runs the lot; the rest are the individual steps. */} -
- - - - - - - + {/* Database — what happens to the games themselves. "Prepare database" + runs every step of the maintenance pipeline, here and on the Players + tab, in the right order; the cards are the individual steps. */} +
+ + Work on the stored games: run the whole maintenance pipeline, remove copies of the + same game, rebuild the position index, and empty the recycle bin. Player records are + cleaned up on the Players tab — do that first, since duplicate games are recognised + by their two players. + +
+ + + + +
-
- - + {/* Players — everything that decides WHO a game was played by. In the + order the automatic pipeline runs them, with the manual merge next + to the automatic one it complements. */} +
+ + One person can arrive from several sources under several spellings, leaving one + player record per spelling — and that also hides duplicate games, which are matched + by their two players. These steps identify players and fold the duplicates together, + in the order shown. + + {/* Two columns, in pipeline order: the FIDE list feeds the ID lookup + (row 1), the canonical-name pair renames from it (row 2), and the + merges come last — a rename can itself create a duplicate — with + automatic and manual side by side so the pair reads at a glance + (row 3). */} +
+ + + + + + +
-
diff --git a/chess-client/src/hooks/useJobProgress.ts b/chess-client/src/hooks/useJobProgress.ts index 9380f0a..26ae4a4 100644 --- a/chess-client/src/hooks/useJobProgress.ts +++ b/chess-client/src/hooks/useJobProgress.ts @@ -145,7 +145,12 @@ function planFromArgs(args: string[]): Plan { if (hasFlag(args, "--dry-run")) params.dry_run = true; return { kind: "job", type: "normalise", params }; } - if (a1 === "dedup") return { kind: "job", type: "dedup_players", params: {} }; + if (a1 === "dedup") { + // `--dry-run` reports every planned merge and writes nothing — the + // preview the Maintenance panel offers next to the real run, since a + // player merge cannot be undone. + return { kind: "job", type: "dedup_players", params: { dry_run: hasFlag(args, "--dry-run") } }; + } if (a1 === "resolve-fide") return { kind: "job", type: "resolve_fide", params: {} }; break; } diff --git a/chess-db/src/dedup.rs b/chess-db/src/dedup.rs index ba0ec59..cad0d69 100644 --- a/chess-db/src/dedup.rs +++ b/chess-db/src/dedup.rs @@ -12,34 +12,74 @@ pub fn hard_delete_game(conn: &Connection, id: u32) -> Result<()> { Ok(()) } -pub fn dedup_players(conn: &Connection, reporter: &Reporter) -> Result<()> { - // Fetch every player row that shares a fide_id with another, plus that - // player's most recent game date (for the survivor tiebreaker) — all in ONE - // query. The per-player last-date is computed in a single pass over games - // rather than a whole-table scan per fide_id (the old approach's first cost). - let rows: Vec<(u32, u32, String, bool, Option)> = { +/// One candidate row for the player merge: everything needed to cluster it and +/// to score it as a survivor. +struct PlayerRow { + id: u32, + name: String, + name_normalized: String, + fide_id: Option, + name_normalised: bool, + last_date: Option, +} + +pub fn dedup_players(conn: &Connection, dry_run: bool, reporter: &Reporter) -> Result<()> { + // Fetch every player row that shares a fide_id OR a normalised name with + // another, plus that player's most recent game date (for the survivor + // tiebreaker) — all in ONE query. The per-player last-date is computed in a + // single pass over games rather than a whole-table scan per group (the old + // approach's first cost). + // + // Normalised name is the second key because it is the SAME key the importer + // identifies people by: `get_or_create_player` looks a name up in a cache of + // every existing `name_normalized` before it ever considers a FIDE ID, so + // one normalised name already means one person as far as import is + // concerned. Two rows sharing one are an artefact — most often a rename + // (`players normalise` / `players import` rewrite a row to its FIDE-canonical + // spelling, which can land on a name another row already holds) — and until + // they are merged they also hide duplicate GAMES, since `dedup_games` pairs + // on player ids. `cluster_players` refuses the one case where the rows are + // known to be different people: distinct FIDE IDs. + let rows: Vec = { let mut stmt = conn.prepare( - "WITH dups AS ( + "WITH dup_fide AS ( SELECT fide_id FROM players WHERE fide_id IS NOT NULL GROUP BY fide_id HAVING COUNT(*) > 1 ), + dup_name AS ( + SELECT name_normalized FROM players + WHERE name_normalized IS NOT NULL AND name_normalized <> '' + GROUP BY name_normalized HAVING COUNT(*) > 1 + ), + cand AS ( + SELECT id, name, name_normalized, fide_id, name_normalised + FROM players + WHERE fide_id IN (SELECT fide_id FROM dup_fide) + OR name_normalized IN (SELECT name_normalized FROM dup_name) + ), last AS ( SELECT pid, MAX(date) AS last_date FROM (SELECT white_id AS pid, date FROM games UNION ALL SELECT black_id AS pid, date FROM games) - WHERE pid IN (SELECT id FROM players WHERE fide_id IN (SELECT fide_id FROM dups)) + WHERE pid IN (SELECT id FROM cand) GROUP BY pid ) - SELECT p.fide_id, p.id, p.name, p.name_normalised, l.last_date - FROM players p - JOIN dups d ON d.fide_id = p.fide_id - LEFT JOIN last l ON l.pid = p.id - ORDER BY p.fide_id, p.id", + SELECT c.id, c.name, c.name_normalized, c.fide_id, c.name_normalised, l.last_date + FROM cand c + LEFT JOIN last l ON l.pid = c.id + ORDER BY c.id", )?; stmt.query_map([], |r| { - Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?, r.get(4)?)) + Ok(PlayerRow { + id: r.get(0)?, + name: r.get(1)?, + name_normalized: r.get(2)?, + fide_id: r.get(3)?, + name_normalised: r.get(4)?, + last_date: r.get(5)?, + }) })? .filter_map(|r| r.ok()) .collect() @@ -50,44 +90,18 @@ pub fn dedup_players(conn: &Connection, reporter: &Reporter) -> Result<()> { return Ok(()); } - // Group consecutive rows by fide_id (the query is ORDER BY fide_id), pick a - // survivor per group, and build a flat old_id → survivor_id reassignment map. - // Each player row has exactly one fide_id, so no old_id is another group's - // survivor — the map needs no chaining. - let mut mapping: Vec<(u32, u32)> = Vec::new(); - let mut fide_ids = 0usize; - let mut group: Vec<(u32, String, bool, Option)> = Vec::new(); - let flush = |group: &mut Vec<(u32, String, bool, Option)>, - mapping: &mut Vec<(u32, u32)>, - fide_ids: &mut usize| { - if group.len() >= 2 { - *fide_ids += 1; - let survivor_idx = pick_survivor(group); - let survivor_id = group[survivor_idx].0; - for (id, ..) in group.iter() { - if *id != survivor_id { - mapping.push((*id, survivor_id)); - } - } - } - group.clear(); - }; - - let mut cur_fide: Option = None; - for (fide_id, id, name, normalised, last_date) in rows { - if cur_fide != Some(fide_id) { - flush(&mut group, &mut mapping, &mut fide_ids); - cur_fide = Some(fide_id); - } - group.push((id, name, normalised, last_date)); - } - flush(&mut group, &mut mapping, &mut fide_ids); + let (mapping, groups) = cluster_players(&rows); if mapping.is_empty() { reporter.done("No duplicate players found."); return Ok(()); } + if dry_run { + report_planned_merges(&rows, &mapping, groups, reporter); + return Ok(()); + } + if reporter.is_cancelled() { reporter.done("Cancelled before merging."); return Ok(()); @@ -149,14 +163,173 @@ pub fn dedup_players(conn: &Connection, reporter: &Reporter) -> Result<()> { return Ok(()); } + // Survivors now own their losers' games, so every stored count is understated. + // The pipeline's later `dedup_games` would refresh them, but a merge run on + // its own from the Maintenance page has to leave the numbers right too. + crate::db::queries::recalculate_game_counts(conn)?; + reporter.done(format!( - "Removed {} duplicate player record(s) across {} FIDE ID(s).", + "Removed {} duplicate player record(s) across {} player(s).", mapping.len(), - fide_ids, + groups, )); Ok(()) } +/// Spell out what a real run would merge, without touching anything. A merge +/// cannot be undone, so the preview has to be specific enough to spot a wrong +/// one: every planned merge is listed by name and id, with the FIDE ID that +/// links it — or "by name" when the names alone did, which is the case worth +/// checking, since two people really can share a name. +fn report_planned_merges( + rows: &[PlayerRow], + mapping: &[(u32, u32)], + groups: usize, + reporter: &Reporter, +) { + use std::collections::HashMap; + let by_id: HashMap = rows.iter().map(|r| (r.id, r)).collect(); + + // Group the flat old→new mapping back into one line per survivor. + let mut per_survivor: HashMap> = HashMap::new(); + for (old, new) in mapping { + per_survivor.entry(*new).or_default().push(*old); + } + let mut survivors: Vec = per_survivor.keys().copied().collect(); + survivors.sort_unstable(); + + // Cap the listing: a first run on a large database can plan thousands of + // merges, and flooding the log helps nobody. The counts below are complete. + const MAX_LINES: usize = 50; + let mut by_name_only = 0usize; + for (shown, sid) in survivors.iter().enumerate() { + let losers = &per_survivor[sid]; + let keep = by_id.get(sid); + let fide = keep.and_then(|k| k.fide_id); + // "By name" = nothing in the cluster carries a FIDE ID to justify it. + let linked_by_fide = + fide.is_some() || losers.iter().any(|l| by_id.get(l).and_then(|r| r.fide_id).is_some()); + if !linked_by_fide { + by_name_only += 1; + } + if shown >= MAX_LINES { + continue; + } + let names = losers + .iter() + .map(|l| match by_id.get(l) { + Some(r) => format!("“{}” [{}]", r.name, r.id), + None => format!("[{}]", l), + }) + .collect::>() + .join(", "); + reporter.log(format!( + " keep “{}” [{}] ({}) ← {}", + keep.map(|k| k.name.as_str()).unwrap_or("?"), + sid, + match fide { + Some(f) => format!("FIDE {f}"), + None => "by name".to_string(), + }, + names, + )); + } + if survivors.len() > MAX_LINES { + reporter.log(format!(" …and {} more", survivors.len() - MAX_LINES)); + } + if by_name_only > 0 { + reporter.log(format!( + "{by_name_only} of these are linked by name alone (no FIDE ID on either side) — \ + check those for genuine namesakes.", + )); + } + reporter.done(format!( + "Dry run: {} duplicate player record(s) across {} player(s) would be merged.", + mapping.len(), + groups, + )); +} + +/// Cluster candidate rows into "same person" sets and choose one survivor each. +/// Returns `(old_id → survivor_id for every non-survivor, number of clusters)`. +/// +/// Two rows join a cluster when they share a FIDE ID or share a normalised name; +/// union-find makes the relation transitive, so `A(fide 7) — B(fide 7, "smith +/// john") — C("smith john")` collapses to one survivor without the caller having +/// to chase chains through the mapping. +/// +/// The one refusal: a normalised name held by rows with two or more DIFFERENT +/// FIDE IDs is left alone entirely. Those are namesakes that FIDE itself +/// distinguishes, and a merge cannot be undone. (FIDE IDs never conflict inside a +/// cluster otherwise: unioning by FIDE ID cannot mix two of them, and this guard +/// stops a name from doing so.) +fn cluster_players(rows: &[PlayerRow]) -> (Vec<(u32, u32)>, usize) { + use std::collections::HashMap; + + // Rows with ≥2 distinct FIDE IDs under one normalised name — never merged. + let ambiguous: std::collections::HashSet<&str> = { + let mut seen: HashMap<&str, u32> = HashMap::new(); + let mut bad = std::collections::HashSet::new(); + for r in rows { + let Some(fid) = r.fide_id else { continue }; + match seen.get(r.name_normalized.as_str()) { + Some(&other) if other != fid => { bad.insert(r.name_normalized.as_str()); } + Some(_) => {} + None => { seen.insert(&r.name_normalized, fid); } + } + } + bad + }; + + let mut parent: HashMap = rows.iter().map(|r| (r.id, r.id)).collect(); + let union = |parent: &mut HashMap, a: u32, b: u32| { + let (ra, rb) = (uf_find(parent, a), uf_find(parent, b)); + if ra != rb { + parent.insert(ra, rb); + } + }; + let mut by_fide: HashMap = HashMap::new(); + let mut by_name: HashMap<&str, u32> = HashMap::new(); + for r in rows { + if let Some(fid) = r.fide_id { + match by_fide.get(&fid) { + Some(&first) => union(&mut parent, r.id, first), + None => { by_fide.insert(fid, r.id); } + } + } + if ambiguous.contains(r.name_normalized.as_str()) { + continue; + } + match by_name.get(r.name_normalized.as_str()) { + Some(&first) => union(&mut parent, r.id, first), + None => { by_name.insert(&r.name_normalized, r.id); } + } + } + + // Group by cluster root, then pick each cluster's survivor. + let mut clusters: HashMap> = HashMap::new(); + for r in rows { + clusters.entry(uf_find(&mut parent, r.id)).or_default().push(r); + } + let mut mapping = Vec::new(); + let mut groups = 0usize; + for members in clusters.values() { + if members.len() < 2 { + continue; // a lone row: its partner was the ambiguous-name case + } + groups += 1; + let survivor = pick_survivor(members); + for m in members { + if m.id != survivor { + mapping.push((m.id, survivor)); + } + } + } + // Deterministic order so a cancelled run rebuilds the same map on re-run. + mapping.sort_unstable(); + (mapping, groups) +} + /// The UPDATE/DELETE body of the player merge, run while the games player-column /// indexes are dropped (see dedup_players). Reads the prepared `merge_map` temp /// table. Returns Ok(false) if cancelled between range chunks (merge_map is @@ -198,6 +371,22 @@ fn run_player_merge_updates(conn: &Connection, reporter: &Reporter) -> Result Option { }) } -fn pick_survivor(rows: &[(u32, String, bool, Option)]) -> usize { +/// The id of the row to keep for a cluster. A row carrying the cluster's FIDE ID +/// always wins: the merge only rewrites `games`, so a FIDE-less survivor would +/// drop the ID off the database entirely. Among equals, `name_score` decides, +/// and the lowest id breaks a tie so the choice is stable across runs. +fn pick_survivor(rows: &[&PlayerRow]) -> u32 { rows.iter() - .enumerate() - .max_by_key(|(_, (_, name, normalised, last_date))| { - name_score(name, *normalised, last_date.as_deref()) + .max_by_key(|r| { + ( + r.fide_id.is_some(), + name_score(&r.name, r.name_normalised, r.last_date.as_deref()), + std::cmp::Reverse(r.id), + ) }) - .map(|(i, _)| i) + .map(|r| r.id) .unwrap_or(0) } @@ -1044,7 +1240,7 @@ mod dedup_players_tests { ) .unwrap(); - dedup_players(&conn, &Reporter::silent()).unwrap(); + dedup_players(&conn, false, &Reporter::silent()).unwrap(); // Non-survivors 2 and 4 are gone; 1, 3, 5 remain. let remaining: Vec = { @@ -1071,8 +1267,208 @@ mod dedup_players_tests { (2,'C, D','c d',200,FALSE);", ) .unwrap(); - dedup_players(&conn, &Reporter::silent()).unwrap(); + dedup_players(&conn, false, &Reporter::silent()).unwrap(); let n: i64 = conn.query_row("SELECT COUNT(*) FROM players", [], |r| r.get(0)).unwrap(); assert_eq!(n, 2); } + + fn ids(conn: &Connection) -> Vec { + let mut s = conn.prepare("SELECT id FROM players ORDER BY id").unwrap(); + s.query_map([], |r| r.get(0)).unwrap().filter_map(|r| r.ok()).collect() + } + + /// The gap this key closes: two rows with the same normalised name and no + /// FIDE ID on either. Nothing linked them before, and while they were split + /// `dedup_games` could not see that their games were the same game. + #[test] + fn merges_same_normalised_name_without_fide_ids() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'Sedlak,Marek','sedlak marek',NULL,FALSE), + (2,'Sedlak, Marek','sedlak marek',NULL,FALSE); + INSERT INTO games (id, white_id, black_id, date) VALUES (1, 2, 1, '2020-01-01');", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + + // Rows in a name group differ only in spacing/case — which `name_score` + // scores identically — so the tiebreak decides: the lowest (oldest) id. + assert_eq!(ids(&conn), vec![1]); + let g: (u32, u32) = conn + .query_row("SELECT white_id, black_id FROM games WHERE id=1", [], |r| Ok((r.get(0)?, r.get(1)?))) + .unwrap(); + assert_eq!(g, (1, 1), "both sides reassigned to the survivor"); + } + + /// A rename (`players normalise` / `players import`) can land a FIDE-carrying + /// row on a name a FIDE-less row already holds. The merged row must keep the + /// FIDE ID — the merge only rewrites `games`, so a FIDE-less survivor would + /// drop it from the database. + #[test] + fn survivor_keeps_the_fide_id_when_merging_by_name() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'Sedlak, Marek','sedlak marek',NULL,FALSE), + (2,'Sedlak, Marek','sedlak marek',555,TRUE); + INSERT INTO games (id, white_id, black_id, date) VALUES (1, 1, 2, '2020-01-01');", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + + assert_eq!(ids(&conn), vec![2]); + let fide: Option = conn + .query_row("SELECT fide_id FROM players WHERE id=2", [], |r| r.get(0)) + .unwrap(); + assert_eq!(fide, Some(555)); + } + + /// Namesakes FIDE itself distinguishes are left alone — a merge cannot be + /// undone, so two distinct FIDE IDs under one name veto the whole name group. + #[test] + fn refuses_same_name_with_two_different_fide_ids() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'Smith, John','smith john',111,TRUE), + (2,'Smith, John','smith john',222,TRUE), + (3,'Smith, John','smith john',NULL,FALSE);", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + + assert_eq!(ids(&conn), vec![1, 2, 3], "including the FIDE-less row: which one is it?"); + } + + /// Chained across both keys: A and B share a FIDE ID, B and C share a name. + /// All three are one person and must collapse to a single row. + #[test] + fn chains_fide_id_and_name_links_into_one_cluster() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'Novak,P','novak p',77,FALSE), + (2,'Novak, Peter','novak peter',77,TRUE), + (3,'Novak, Peter','novak peter',NULL,FALSE); + INSERT INTO games (id, white_id, black_id, date) VALUES + (1, 1, 3, '2020-01-01'), + (2, 3, 2, '2021-01-01');", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + + assert_eq!(ids(&conn), vec![2], "the normalised, FIDE-carrying row survives"); + let all: Vec<(u32, u32)> = { + let mut s = conn.prepare("SELECT white_id, black_id FROM games ORDER BY id").unwrap(); + s.query_map([], |r| Ok((r.get(0)?, r.get(1)?))).unwrap().filter_map(|r| r.ok()).collect() + }; + assert_eq!(all, vec![(2, 2), (2, 2)], "every reference chased to the one survivor"); + } + + /// An empty normalised name is not an identity — rows that have one must not + /// all collapse into a single player. + #[test] + fn empty_normalised_name_is_not_a_merge_key() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'?','',NULL,FALSE), + (2,'','',NULL,FALSE);", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + + assert_eq!(ids(&conn), vec![1, 2]); + } + + /// Merging players changes the very key `dedup_games` pairs on, so any + /// "already vetted" verdict reached while the rows were split is stale. The + /// automatic pipeline runs normalise → dedup_players → dedup_games, and its + /// dedup_games is INCREMENTAL: without re-opening the survivor's games, a + /// duplicate pair that was unmatchable in one pass stays vetted and is never + /// looked at again — so the copies survive forever (#266). + #[test] + fn merge_reopens_the_survivors_games_for_the_next_incremental_dedup() { + let conn = setup(); + // Two records for one person (same normalised name), each holding one + // copy of the SAME game. Both games were vetted by an earlier pass that + // could not pair them, because their white_ids differed. + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'Abadjian, Vahram','abadjian vahram',NULL,FALSE), + (2,'Abadjian, Vahram','abadjian vahram',12345,TRUE), + (3,'Krejcar, Walter','krejcar walter',NULL,FALSE); + INSERT INTO games (id, white_id, black_id, date, result, opening_line, move_count, pgn, deduped) VALUES + (1, 1, 3, '2026-02-14', '0-1', 'e4 e5', 4, '[W \"a\"]\n\n1. e4 e5 2. Nf3 Nc6 0-1', TRUE), + (2, 2, 3, '2026-02-14', '0-1', 'e4 e5', 4, '[W \"a\"]\n\n1. e4 e5 2. Nf3 Nc6 0-1', TRUE);", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + // Incremental — exactly what the post-import pipeline runs. + dedup_games(&conn, false, false, &Reporter::silent()).unwrap(); + + let games: i64 = conn.query_row("SELECT COUNT(*) FROM games", [], |r| r.get(0)).unwrap(); + assert_eq!(games, 1, "the copy the merge exposed is removed by the very next pass"); + } + + /// A preview must leave the database exactly as it found it — players, game + /// assignments and the `deduped` flags a real merge would have cleared. + #[test] + fn dry_run_changes_nothing() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised) VALUES + (1,'Abadjian, Vahram','abadjian vahram',NULL,FALSE), + (2,'Abadjian, Vahram','abadjian vahram',12345,TRUE), + (3,'Krejcar, Walter','krejcar walter',NULL,FALSE); + INSERT INTO games (id, white_id, black_id, date, deduped) VALUES + (1, 1, 3, '2026-02-14', TRUE);", + ) + .unwrap(); + + dedup_players(&conn, true, &Reporter::silent()).unwrap(); + + assert_eq!(ids(&conn), vec![1, 2, 3], "no row removed"); + let (w, vetted): (u32, bool) = conn + .query_row("SELECT white_id, deduped FROM games WHERE id=1", [], |r| Ok((r.get(0)?, r.get(1)?))) + .unwrap(); + assert_eq!(w, 1, "no game reassigned"); + assert!(vetted, "dedup flags untouched"); + + // ...and the real run that follows still does the work. + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + assert_eq!(ids(&conn), vec![2, 3]); + } + + /// #205-style: after a merge the survivor owns its losers' games, so the + /// stored counts must be refreshed even when no game dedup follows. + #[test] + fn merge_refreshes_player_game_counts() { + let conn = setup(); + conn.execute_batch( + "INSERT INTO players (id,name,name_normalized,fide_id,name_normalised,game_count) VALUES + (1,'Sedlak, Marek','sedlak marek',NULL,FALSE,1), + (2,'Sedlak, Marek','sedlak marek',555,TRUE,1), + (3,'Horak, Jan','horak jan',666,TRUE,2); + INSERT INTO games (id, white_id, black_id, date) VALUES + (1, 1, 3, '2020-01-01'), + (2, 2, 3, '2021-01-01');", + ) + .unwrap(); + + dedup_players(&conn, false, &Reporter::silent()).unwrap(); + + let gc = |id: u32| -> i64 { + conn.query_row("SELECT game_count FROM players WHERE id=?", duckdb::params![id], |r| r.get(0)).unwrap() + }; + assert_eq!(gc(2), 2, "survivor's count covers both merged rows' games"); + assert_eq!(gc(3), 2, "untouched player's count still right"); + } } diff --git a/chess-db/src/jobs.rs b/chess-db/src/jobs.rs index 8d6a18a..d0f919d 100644 --- a/chess-db/src/jobs.rs +++ b/chess-db/src/jobs.rs @@ -689,8 +689,8 @@ impl JobManager { /// If maintenance is owed and nothing import- or maintenance-class is queued /// or running, enqueue the coalesced pass once and clear the owed flag. The - /// single identity-first pass (#167) is resolve-fide → dedup_players → - /// normalise → dedup_games → index; `dedup_games` is incremental (#—), so + /// single identity-first pass (#167) is resolve-fide → normalise → + /// dedup_players → dedup_games → index; `dedup_games` is incremental (#—), so /// it's affordable after every sync and there's no longer a light variant. If /// the queue hasn't drained yet, re-arm and wait for a later call. Safe to /// call from any thread and as often as you like — the flag is claimed @@ -729,10 +729,16 @@ impl JobManager { // ahead and failing with "No FIDE list loaded" (#206). let c = self.next_cluster_id(); let m = |t: &str, p| self.submit_in_cluster(t.to_string(), p, Some(c.clone())); + // + // `normalise` runs BEFORE `dedup_players`, not after (#266): renaming a + // row to its FIDE-canonical spelling is itself a way to create a + // duplicate — the new name can be one another row already holds — so + // merging has to come after the renames, or the pass ends by creating + // duplicates it will not clean up until the next one. m("fide_refresh", serde_json::json!({ "if_due": true })); m("resolve_fide", serde_json::json!({})); - m("dedup_players", serde_json::json!({})); m("normalise", serde_json::json!({})); + m("dedup_players", serde_json::json!({})); m("dedup_games", serde_json::json!({})); m("index_positions", serde_json::json!({ "fast": true })); } @@ -1383,7 +1389,7 @@ fn run_job( dedup::dedup_games(conn, flag(p, "dry_run"), flag(p, "full"), reporter)?; } "dedup_players" => { - dedup::dedup_players(conn, reporter)?; + dedup::dedup_players(conn, flag(p, "dry_run"), reporter)?; } "cleanup" => { dedup::cleanup_nonstandard(conn, flag(p, "non_standard"), flag(p, "dry_run"), reporter)?; diff --git a/chess-db/src/main.rs b/chess-db/src/main.rs index 097abdc..ff7333e 100644 --- a/chess-db/src/main.rs +++ b/chess-db/src/main.rs @@ -644,8 +644,15 @@ enum GameCommands { enum PlayersCommands { /// Recalculate and store game counts for all players UpdateGameCounts, - /// Merge duplicate player records that share the same FIDE ID - Dedup, + /// Merge duplicate player records: those sharing a FIDE ID, and those sharing + /// a normalised name (the key the importer identifies people by). A name held + /// by two different FIDE IDs is left alone — those are namesakes + Dedup { + /// List every merge that would be made, and change nothing. A merge + /// cannot be undone, so preview a first run on a large database. + #[arg(long)] + dry_run: bool, + }, /// Merge two player records: reassign all games from drop-id to keep-id, then delete drop-id Merge { /// Player ID to keep @@ -1566,9 +1573,9 @@ fn job_spec_for(command: &Commands) -> Option { "resolve_import", json!({ "path": path.to_string_lossy() }), ), - Commands::Players { subcommand: PlayersCommands::Dedup } => ( + Commands::Players { subcommand: PlayersCommands::Dedup { dry_run } } => ( "dedup_players", - json!({}), + json!({ "dry_run": dry_run }), ), Commands::Players { subcommand: PlayersCommands::Import { path } } => ( "players_import", @@ -2279,8 +2286,8 @@ async fn main() -> Result<()> { db::queries::recalculate_game_counts(&conn)?; println!("Done."); } - PlayersCommands::Dedup => { - dedup::dedup_players(&conn, &reporter)?; + PlayersCommands::Dedup { dry_run } => { + dedup::dedup_players(&conn, dry_run, &reporter)?; } PlayersCommands::Merge { keep_id, drop_id, yes } => { do_merge_players(&conn, keep_id, drop_id, yes)?; diff --git a/chess-db/src/serve.rs b/chess-db/src/serve.rs index e73c99b..a0a8db1 100644 --- a/chess-db/src/serve.rs +++ b/chess-db/src/serve.rs @@ -1319,6 +1319,15 @@ async fn merge_players_handler( conn.execute("UPDATE games SET white_id = ? WHERE white_id = ?", duckdb::params![keep_id, drop_id])?; conn.execute("UPDATE games SET black_id = ? WHERE black_id = ?", duckdb::params![keep_id, drop_id])?; conn.execute("DELETE FROM players WHERE id = ?", duckdb::params![drop_id])?; + // Re-open the kept player's games for deduplication: dedup_games + // pairs on white_id AND black_id, so every verdict it reached while + // these two rows were split is stale, and its incremental pass would + // otherwise never revisit them. Merging by hand is exactly how a user + // exposes duplicate games it could not previously see (#266). + conn.execute( + "UPDATE games SET deduped = FALSE WHERE white_id = ? OR black_id = ?", + duckdb::params![keep_id, keep_id], + )?; crate::db::queries::recalculate_game_count_for(conn, keep_id)?; Ok(()) }) @@ -1384,7 +1393,7 @@ async fn create_job_handler( } /// Run the whole maintenance pipeline on demand — the same coalesced, -/// identity-first pass (fide_refresh → resolve_fide → dedup_players → normalise +/// identity-first pass (fide_refresh → resolve_fide → normalise → dedup_players /// → dedup_games → index) that runs by itself after an import. /// /// It was previously reachable only as a side effect of importing, while the @@ -1712,7 +1721,7 @@ async fn setup_start_handler(State(state): State) -> ApiResult