Skip to content

fix(naming): stop the de-novo loader forking the identity of already-… #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JamesKane merged 1 commit into from
Jul 13, 2026
Merged

fix(naming): stop the de-novo loader forking the identity of already-… #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions rust/crates/du-db/src/denovo.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,36 @@ fn confidence(support: Option<i32>) -> &'static str {
}
}

/// Named Y catalog rows that carry **no `hs1` coordinate**. This is a load-blocking
/// precondition, not a statistic: [`resolve_variant`] / [`prefill_vcache`] reuse a
/// catalog row only by matching `coordinates @> {'hs1': …}`, so a named variant that
/// hasn't been lifted to hs1 yet is *invisible* to the match — and the loader quietly
/// mints a second, coordinate-named row for a marker YBrowse already named.
///
/// The failure is silent and permanent: the duplicate is what gets linked to the branch,
/// so the marker's real name is stranded on an unlinked catalog row and the branch row
/// lands in the curator naming queue as if it were a novel discovery. Getting this wrong
/// once produced ~130k such duplicates (ybrowse-ingest → de-novo load → variant-coord-lift,
/// when the lift has to come *second*).
pub async fn unlifted_y_catalog_count(pool: &PgPool) -> Result<i64, DbError> {
Ok(sqlx::query_scalar(
"SELECT count(*) FROM core.variant \
WHERE defining_haplogroup_id IS NULL \
AND canonical_name IS NOT NULL AND canonical_name NOT LIKE 'chr%:%' \
AND NOT (coordinates ? 'hs1') \
AND (coordinates->'GRCh38'->>'contig' = 'chrY' \
OR coordinates->'GRCh37'->>'contig' = 'chrY')",
)
.fetch_one(pool)
.await?)
}

/// Load a de-novo tree document as the tree foundation. Assumes `tree.*` is
/// already cleared (greenfield). Commits the topology, then recomputes backbone
/// and bumps the tree revision.
///
/// A Y load **refuses to start** unless the Y catalog is fully hs1-lifted — see
/// [`unlifted_y_catalog_count`]. Run `du-jobs run-once variant-coord-lift` first.
pub async fn load(pool: &PgPool, doc: &DenovoTree, keep_private: bool) -> Result<LoadReport, DbError> {
let dna: DnaType = match doc.haplogroup_type.as_str() {
"Y_DNA" => DnaType::YDna,
Expand All @@ -177,6 +204,18 @@ pub async fn load(pool: &PgPool, doc: &DenovoTree, keep_private: bool) -> Result
let dna_label = pg_enum_label(&dna)?;
let mut rep = LoadReport::default();

if matches!(dna, DnaType::YDna) {
let unlifted = unlifted_y_catalog_count(pool).await?;
if unlifted > 0 {
return Err(DbError::Conflict(format!(
"{unlifted} named Y catalog variants have no hs1 coordinate — the de-novo \
catalog match is hs1-only, so loading now would mint a duplicate \
coordinate-named row for each of them instead of reusing its name. \
Run `du-jobs run-once variant-coord-lift` first, then re-run this load."
)));
}
}

let mut tx = pool.begin().await?;

// 1. Resolve every defining SNP to a core.variant id (catalog reuse or mint),
Expand Down
240 changes: 203 additions & 37 deletions rust/crates/du-db/src/naming.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,12 +188,16 @@ pub fn established_name(aliases: &Value) -> Option<String> {
.map(str::to_string)
}

/// **Adopt an established name**: ratify a variant's existing ISOGG/YBrowse name
/// (a non-DU `common_names` alias) as its canonical name instead of minting a new
/// `DU` identifier. This is the "named by definition" path — when a matching
/// locus + mutation state already has a name in the source set, the authority
/// reuses it. Sets `canonical_name` to that name and marks `NAMED`. Errors if
/// the variant is already named or carries no established name.
/// **Adopt an established name**: ratify the name this variant already has — an
/// ISOGG/YBrowse `common_names` alias, or the name of a variant at the same locus +
/// mutation state — as its canonical name, instead of minting a new `DU` identifier.
/// This is the "named by definition" path: a marker the source set already named must
/// not get a second, DecodingUs-invented identity. Sets `canonical_name` and marks
/// `NAMED`. Errors if the variant is already named or no name is available to adopt.
///
/// Both name sources are resolved here, in [`adoptable_name`]'s order — the site twin
/// is not a fallback nicety but the *only* source for a de-novo coordinate row, which
/// carries no aliases of its own.
pub async fn adopt_established_name(pool: &PgPool, id: i64) -> Result<String, DbError> {
let mut tx = pool.begin().await?;
let row: Option<(Option<String>, Value)> = sqlx::query_as(
Expand All @@ -210,8 +214,19 @@ pub async fn adopt_established_name(pool: &PgPool, id: i64) -> Result<String, Db
if real_named {
return Err(DbError::Conflict("variant already has a canonical name".into()));
}
let name = established_name(&aliases)
.ok_or_else(|| DbError::Conflict("variant has no established name to adopt".into()))?;
let name = match established_name(&aliases) {
Some(n) => n,
None => sqlx::query_as::<_, (i64, String)>(SITE_TWIN_SQL)
.bind(id)
.fetch_all(&mut *tx)
.await?
.into_iter()
.next()
.map(|(_, n)| n)
.ok_or_else(|| {
DbError::Conflict("variant has no established name to adopt".into())
})?,
};
// Canonical identity is (name + defining branch) — the recurrence model that
// replaces ISOGG's L270.1/L270.2 suffixing: the same SNP name is canonical on
// each branch it defines, scoped by `defining_haplogroup_id`. A clash only
Expand Down Expand Up @@ -243,34 +258,185 @@ pub async fn adopt_established_name(pool: &PgPool, id: i64) -> Result<String, Db
Ok(name)
}

/// **Dedup check** before naming: other *named* variants at the same locus **and
/// mutation state** — contig + position + ancestral + derived — on this variant's
/// preferred build (`hs1` first, else `GRCh38`). Matching by locus alone is wrong:
/// two distinct SNPs can share a position with different alleles (e.g. Z12236 A>C
/// vs Y17125 A>G), so a same-position-different-allele variant is NOT a duplicate.
/// Other **named** variants at the same locus **and mutation state** — contig +
/// position + ancestral + derived — on this variant's preferred build (`hs1` first,
/// else `GRCh38`). Matching by locus alone is wrong: two distinct SNPs can share a
/// position with different alleles (e.g. Z12236 A>C vs Y17125 A>G), so a
/// same-position-different-allele variant is NOT a duplicate.
///
/// Coordinate placeholders (`chrY:…`) are excluded: a placeholder is not a name, and
/// without this filter the authority reports "a named variant already exists at this
/// locus: `chrY:10249542C>G`" — contradicting every other path in this module, which
/// treats that string as *unnamed* (see [`is_placeholder_name`]).
///
/// The ordering **is the adoption preference** — the head of this list is what
/// [`adopt_established_name`] ratifies: the catalog representative first, then an
/// external name over one of our own `DU` mints, then alphabetical for determinism.
const SITE_TWIN_SQL: &str = "WITH b AS ( \
SELECT CASE WHEN coordinates ? 'hs1' THEN 'hs1' \
WHEN coordinates ? 'GRCh38' THEN 'GRCh38' END AS build \
FROM core.variant WHERE id = $1), \
me AS ( \
SELECT b.build, \
v.coordinates->b.build->>'contig' AS c, \
v.coordinates->b.build->>'position' AS p, \
v.coordinates->b.build->>'ancestral' AS a, \
v.coordinates->b.build->>'derived' AS d \
FROM core.variant v, b WHERE v.id = $1) \
SELECT v.id, v.canonical_name FROM core.variant v, me \
WHERE v.id <> $1 AND v.canonical_name IS NOT NULL \
AND v.canonical_name NOT LIKE 'chr%:%' \
AND me.build IS NOT NULL \
AND me.c IS NOT NULL AND me.p IS NOT NULL \
AND v.coordinates->me.build->>'contig' = me.c \
AND v.coordinates->me.build->>'position' = me.p \
AND v.coordinates->me.build->>'ancestral' IS NOT DISTINCT FROM me.a \
AND v.coordinates->me.build->>'derived' IS NOT DISTINCT FROM me.d \
ORDER BY v.catalog_representative DESC, (v.canonical_name LIKE 'DU%'), v.canonical_name \
LIMIT 10";

/// **Dedup check** before naming — see [`SITE_TWIN_SQL`].
pub async fn dedup_by_site(pool: &PgPool, id: i64) -> Result<Vec<(i64, String)>, DbError> {
Ok(sqlx::query_as(
"WITH b AS ( \
SELECT CASE WHEN coordinates ? 'hs1' THEN 'hs1' \
WHEN coordinates ? 'GRCh38' THEN 'GRCh38' END AS build \
FROM core.variant WHERE id = $1), \
me AS ( \
SELECT b.build, \
v.coordinates->b.build->>'contig' AS c, \
v.coordinates->b.build->>'position' AS p, \
v.coordinates->b.build->>'ancestral' AS a, \
v.coordinates->b.build->>'derived' AS d \
FROM core.variant v, b WHERE v.id = $1) \
SELECT v.id, v.canonical_name FROM core.variant v, me \
WHERE v.id <> $1 AND v.canonical_name IS NOT NULL AND me.build IS NOT NULL \
AND me.c IS NOT NULL AND me.p IS NOT NULL \
AND v.coordinates->me.build->>'contig' = me.c \
AND v.coordinates->me.build->>'position' = me.p \
AND v.coordinates->me.build->>'ancestral' IS NOT DISTINCT FROM me.a \
AND v.coordinates->me.build->>'derived' IS NOT DISTINCT FROM me.d \
ORDER BY v.canonical_name LIMIT 10",
)
.bind(id)
.fetch_all(pool)
.await?)
Ok(sqlx::query_as(SITE_TWIN_SQL).bind(id).fetch_all(pool).await?)
}

/// The name this variant would **adopt**, from the two places a real name can live:
/// its own established (ISOGG/YBrowse) alias, else a named variant at the same locus +
/// mutation state ([`dedup_by_site`]).
///
/// The second source is the one that matters in practice. The de-novo loader mints its
/// coordinate-named rows with **no aliases at all**, so the alias source never fires for
/// them — a de-novo branch row's real name lives only on the separate catalog row at the
/// same site. Consulting aliases alone made the UI offer no way to act on its own "a named
/// variant already exists here" warning.
///
/// Resolution order must stay in step with [`adopt_established_name`], which decides what
/// the button actually writes.
pub async fn adoptable_name(
pool: &PgPool,
id: i64,
aliases: &Value,
) -> Result<Option<String>, DbError> {
if let Some(n) = established_name(aliases) {
return Ok(Some(n));
}
Ok(dedup_by_site(pool, id).await?.into_iter().next().map(|(_, n)| n))
}

/// Outcome of [`reconcile_placeholder_names`].
#[derive(Debug, Default)]
pub struct NameReconcile {
/// Coordinate-placeholder rows examined.
pub scanned: i64,
/// Of those, ones that have a named variant at the same hs1 site + mutation state.
pub matched: i64,
/// Rows folded onto that existing name (`NAMED`).
pub adopted: i64,
/// Matched but **not** written: the name is already canonical for that same branch.
/// That is a real duplicate or an unmodelled recurrence — a merge-review question,
/// not something to resolve by overwriting. Left in the queue deliberately.
pub conflicted: i64,
}

/// One batch of the reconcile below: take the next `$1` placeholder rows after id `$2`,
/// find each one's named twin at the same hs1 site + mutation state, and adopt that name
/// where doing so doesn't collide with the `(name + defining branch)` unique key.
///
/// Scoped to **branch-defining** rows — the same population as the `needs_name` queue, so
/// what this clears is exactly what a curator would otherwise have to clear by hand. The
/// loader also leaves behind coordinate-named rows that define *no* branch and are linked
/// to nothing (94.6k of them at last count, ~67k of which duplicate an already-named
/// catalog row). Those are catalog dead weight, not naming work: they never surface in the
/// queue, and resolving them means merging rows ([`crate::merge`]), not renaming one — a
/// rename would just collide with the catalog row on `(name, NULL)`. Left alone here
/// deliberately; folding them is a separate cleanup.
///
/// The `ok` CTE deduplicates *within* the batch as well as against the table — two
/// placeholder rows on one branch can resolve to the same name when the catalog itself
/// carries that name at more than one site, and the second write would violate
/// `variant_canonical_name_key`.
const RECONCILE_BATCH_SQL: &str = "WITH win AS ( \
SELECT v.id, v.defining_haplogroup_id AS branch, v.coordinates->'hs1' AS hs1 \
FROM core.variant v \
WHERE v.canonical_name LIKE 'chr%:%' AND v.coordinates ? 'hs1' \
AND v.defining_haplogroup_id IS NOT NULL AND v.id > $2 \
ORDER BY v.id LIMIT $1), \
cand AS ( \
SELECT DISTINCT ON (w.id) w.id, w.branch, n.canonical_name AS name \
FROM win w \
JOIN core.variant n \
ON n.id <> w.id \
AND n.canonical_name IS NOT NULL \
AND n.canonical_name NOT LIKE 'chr%:%' \
AND n.coordinates ? 'hs1' \
AND n.coordinates->'hs1'->>'contig' = w.hs1->>'contig' \
AND n.coordinates->'hs1'->>'position' = w.hs1->>'position' \
AND n.coordinates->'hs1'->>'ancestral' = w.hs1->>'ancestral' \
AND n.coordinates->'hs1'->>'derived' = w.hs1->>'derived' \
ORDER BY w.id, n.catalog_representative DESC, (n.canonical_name LIKE 'DU%'), n.canonical_name), \
ok AS ( \
SELECT DISTINCT ON (c.name, COALESCE(c.branch, -1)) c.id, c.name \
FROM cand c \
WHERE NOT EXISTS ( \
SELECT 1 FROM core.variant o \
WHERE o.canonical_name = c.name \
AND COALESCE(o.defining_haplogroup_id, -1) = COALESCE(c.branch, -1)) \
ORDER BY c.name, COALESCE(c.branch, -1), c.id), \
upd AS ( \
UPDATE core.variant v \
SET canonical_name = ok.name, naming_status = 'NAMED', updated_at = now() \
FROM ok WHERE v.id = ok.id \
RETURNING v.id) \
SELECT (SELECT max(id) FROM win) AS cursor, \
(SELECT count(*) FROM win) AS scanned, \
(SELECT count(*) FROM cand) AS matched, \
(SELECT count(*) FROM upd) AS adopted";

/// **Fold coordinate-placeholder variants onto the name they already have.**
///
/// The de-novo loader mints a `chrY:10249542C>G` row whenever its hs1 catalog match
/// misses, and that row — not the named catalog row — is what gets linked to the branch.
/// Every such row then surfaces in the curator naming queue as if it were a novel
/// discovery, where the only offered action (Mint DU name) would give a marker YBrowse
/// already named a second, conflicting identity.
///
/// This job resolves them the way a curator would: adopt the existing name. It is the bulk
/// form of [`adopt_established_name`] and uses the same site match and same preference
/// order, so a row cleared here and a row cleared by the button get the same name.
///
/// Idempotent and safe to re-run: an adopted row no longer matches `chr%:%`, so it drops
/// out of the scan. Worth running **recurrently**, not just once — a YBrowse ingest that
/// names a marker after a tree load creates exactly this state again, and the fix is the
/// same fold. Rows whose name is already taken on their branch are counted in
/// `conflicted` and left alone for merge review.
///
/// Batched by id cursor so it never long-locks the catalog.
pub async fn reconcile_placeholder_names(
pool: &PgPool,
batch: i64,
) -> Result<NameReconcile, DbError> {
let batch = batch.clamp(1, 50_000);
let mut rep = NameReconcile::default();
let mut cursor: i64 = 0;
loop {
let (next, scanned, matched, adopted): (Option<i64>, i64, i64, i64) =
sqlx::query_as(RECONCILE_BATCH_SQL)
.bind(batch)
.bind(cursor)
.fetch_one(pool)
.await?;
rep.scanned += scanned;
rep.matched += matched;
rep.adopted += adopted;
rep.conflicted += matched - adopted;
// `win` empty ⇒ no placeholder rows past the cursor ⇒ done. Advancing the cursor
// past every *scanned* row (not just adopted ones) is what makes this terminate:
// conflicted rows are never written, so a cursor keyed on progress-made would
// re-select them forever.
match next {
Some(id) => cursor = id,
None => break,
}
}
Ok(rep)
}
Loading
Loading