Merge pull request 'fix(tradein/dedup): drop geo-guard in fias-pass — same-fias identity outranks proximity (#2187)' (#2195) from fix/tradein-dedup-fias-pass-no-geoguard into main

tradein-mvp/backend/app/services/house_dedup_merge.py

@@ -23,11 +23,14 @@ WHAT this is:
     «улица Вайнера, 66» share one cluster_key. Rows whose canon is NULL/blank are never clustered
     (cluster_key NULL → ignored). Only canons shared by >1 house_id form a cluster.
 
-    GEO GUARD (anti over-merge): because the canon strips город/район, two different buildings
-    with the same street+number in different region-66 towns (e.g. «Ленина 5») would share a
-    canon. To avoid merging them, within a canon-cluster a house is a LOSER only if it is within
-    250 m of the keeper (ST_DistanceSphere on the WGS84 geom). Same-canon houses >250 m away — or
-    with NULL geom on either side — are left as separate rows (conservative; never over-merges).
+    GEO GUARD (anti over-merge, CANON PASS ONLY — #2187): because the canon strips город/район,
+    two different buildings with the same street+number in different region-66 towns (e.g. «Ленина
+    5») would share a canon. To avoid merging them, within a canon-cluster a house is a LOSER only
+    if it is within 250 m of the keeper (ST_DistanceSphere on the WGS84 geom). Same-canon houses
+    >250 m away — or with NULL geom on either side — are left as separate rows (conservative). The
+    FIAS pass deliberately SKIPS this guard: a shared ФИАС/ГАР UUID IS the building identity and
+    strictly outranks geo-proximity, so same-fias rows merge even with NULL geom on a side or
+    >250 m apart (the geom-first keeper rule simultaneously repairs the broken coordinate).
 
 TWO PASSES (2026-07-02 follow-up): the SAME cluster→keeper→re-point→carry-identity→delete
     pipeline now runs TWICE inside one transaction, parametrised by the cluster-key expression
@@ -40,8 +43,10 @@ TWO PASSES (2026-07-02 follow-up): the SAME cluster→keeper→re-point→carry-
       2. CANON pass — the canonical-address clustering above, now with a CROSS-FIAS GUARD: within a
          canon cluster a loser is NOT merged when it AND the keeper both carry a non-null but
          DIFFERENT house_fias_id — provably different buildings the canon collapsed (the
-         slash-collapse class). Critical anti-over-merge fix. The 250 m geo guard applies to both
-         passes (after the backfill, fias rows almost always have geom → conservative).
+         slash-collapse class). Critical anti-over-merge fix. The 250 m geo guard applies to the
+         CANON pass ONLY (#2187): the FIAS pass merges on UUID identity regardless of geom — a
+         боевой прогон left 130 same-fias groups (335 houses, 1 529 listings) split because the
+         guard blocked them (NULL geom on a side, or >250 m from a broken coordinate).
 
 IDENTITY CARRY-OVER (each pass, BEFORE deleting losers): the keeper's NULL identity / geo-QC
     fields are filled from its losers with COALESCE semantics (keeper value wins; donor = the
@@ -161,14 +166,35 @@ _CANON_KEY_EXPR = """
 """
 
 
-def _mapping_sql(cluster_key_case: str) -> str:
+def _mapping_sql(cluster_key_case: str, *, apply_geo_guard: bool = True) -> str:
     """Render the loser→keeper mapping SQL for one pass, given its cluster-key CASE expression.
 
     Only cluster keys shared by >1 house_id form a cluster; the keeper is rn=1 per cluster, losers
-    are rn>1. The final mapping is filtered by the 250 m geo guard AND the CROSS-FIAS guard (both
-    no-ops for the fias pass, where every clustered row shares one fias by construction).
+    are rn>1. The CROSS-FIAS guard always applies (a no-op for the fias pass, where every clustered
+    row shares one fias by construction).
+
+    apply_geo_guard (#2187): the 250 m ST_DistanceSphere guard is emitted ONLY when True.
+      - CANON pass → True: the canon strips город/район, so same-street-number buildings in
+        different region-66 towns share a canon; the guard stops the cross-town over-merge.
+      - FIAS pass → False: a shared ФИАС/ГАР UUID IS the building identity and strictly outranks
+        proximity, so same-fias rows merge even with NULL geom on a side or >250 m apart (the
+        geom-first keeper rule simultaneously repairs the broken coordinate).
     `cluster_key_case` is a STATIC module constant (never runtime data) — no value injection.
     """
+    geo_guard = (
+        """
+      -- GEO GUARD (canon pass only — #2187). tradein_canon_addr strips город/район, so two
+      -- different buildings sharing a street+number canon («Ленина 5» in different region-66
+      -- towns) collapse to one cluster_key. A loser merges only when geographically next to the
+      -- keeper (<=250 m — covers one building's geocode spread, prod: Мраморская 34к4 dupes at
+      -- 222 m; region-66 towns are km+ apart → 250 m is safe from cross-town). >250 m, or NULL
+      -- geom on either side, → left as separate rows (conservative — never over-merges).
+      AND keeper_geom IS NOT NULL
+      AND loser_geom IS NOT NULL
+      AND ST_DistanceSphere(loser_geom, keeper_geom) <= 250"""
+        if apply_geo_guard
+        else ""
+    )
     return f"""
     CREATE TEMP TABLE _1772_dup_mapping ON COMMIT DROP AS
     WITH clustered AS (
@@ -223,25 +249,14 @@ def _mapping_sql(cluster_key_case: str) -> str:
         JOIN houses h ON h.id = dh.id
         LEFT JOIN listing_counts lc ON lc.house_id = dh.id
     )
-    -- Geo guard against cross-town over-merge: tradein_canon_addr strips город/район, so two
-    -- different buildings sharing a street+number canon (e.g. «Ленина 5» in different towns)
-    -- collapse to one cluster_key. A loser is only merged when it is geographically next to the
-    -- keeper (<=250 m — покрывает geocode-разброс одного здания, прод: дубли Мраморская 34к4 в
-    -- 222 m; города региона-66 в км+ друг от друга → 250 m безопасно от cross-town). Houses in
-    -- the same canon-cluster but >250 m away, or with NULL geom on either side, are left as
-    -- separate rows (conservative — never over-merges). Cluster_key также требует цифру в canon
-    -- (номер дома) — исключает вырожденные canon вида «екатеринбург»/«сооружение».
     -- CROSS-FIAS guard (#1772 follow-up): never merge two rows that BOTH carry a non-null but
-    -- DIFFERENT house_fias_id — they are provably different buildings the cluster key happened to
-    -- collapse (canon slash-collapse «Сулимова, 32»/«Сулимова, 3/2»). No-op for the fias pass (one
-    -- fias per cluster) and for canon clusters where at most one side has a fias.
+    -- DIFFERENT house_fias_id — provably different buildings the cluster key collapsed (canon
+    -- slash-collapse «Сулимова, 32»/«Сулимова, 3/2»). No-op for the fias pass (one fias per
+    -- cluster) and for canon clusters where at most one side carries a fias.
     SELECT id AS loser_id, keeper_id, norm_address
     FROM ranked
     WHERE rn > 1
-      AND id <> keeper_id
-      AND keeper_geom IS NOT NULL
-      AND loser_geom IS NOT NULL
-      AND ST_DistanceSphere(loser_geom, keeper_geom) <= 250
+      AND id <> keeper_id{geo_guard}
       AND NOT (
           NULLIF(loser_fias, '') IS NOT NULL
           AND NULLIF(keeper_fias, '') IS NOT NULL
@@ -250,10 +265,12 @@ def _mapping_sql(cluster_key_case: str) -> str:
     """
 
 
-# Canon-pass mapping — kept as a module constant for backward-compatible introspection/tests.
+# Canon-pass mapping — geo guard ON (cross-town over-merge protection for street+number canons).
 _BUILD_MAPPING_SQL = text(_mapping_sql(_CANON_KEY_EXPR))
-# Fias-pass mapping — same pipeline, clustered by the ФИАС building UUID (runs first).
-_BUILD_MAPPING_SQL_FIAS = text(_mapping_sql(_FIAS_KEY_EXPR))
+# Fias-pass mapping — same pipeline, clustered by the ФИАС building UUID (runs first). Geo guard
+# OFF (#2187): a shared ГАР UUID IS the building identity and outranks proximity — same-fias rows
+# merge even with NULL geom or >250 m apart (the geom-first keeper rule fixes broken coords).
+_BUILD_MAPPING_SQL_FIAS = text(_mapping_sql(_FIAS_KEY_EXPR, apply_geo_guard=False))
 
 # Each step keys off _1772_dup_mapping → empty mapping ⇒ 0 rows touched ⇒ idempotent no-op.
 _STEPS: list[tuple[str, str]] = [

tradein-mvp/backend/tests/test_house_dedup_merge.py

@@ -256,21 +256,61 @@ def test_fias_pass_clusters_by_house_fias_id() -> None:
     assert "'addr:'" not in flat
 
 
+def test_fias_pass_drops_geo_guard_canon_pass_keeps_it() -> None:
+    """#2187: ФИАС identity strictly outranks geo-proximity, so the fias-pass mapping must NOT
+    carry the 250 m distance guard (same-fias rows merge even with NULL geom on a side or >250 m
+    apart), while the canon-pass mapping MUST keep it (cross-town over-merge protection)."""
+    fias = _flat(_FIAS_MAPPING_SQL)
+    canon = _flat(_MAPPING_SQL)
+    # canon pass keeps the full geo guard (distance + NULL-geom safety on both sides).
+    assert "ST_DistanceSphere(loser_geom, keeper_geom) <= 250" in canon
+    assert "keeper_geom IS NOT NULL" in canon
+    assert "loser_geom IS NOT NULL" in canon
+    # fias pass drops the distance guard AND the NULL-geom exclusions entirely.
+    assert "ST_DistanceSphere" not in fias
+    assert "loser_geom IS NOT NULL" not in fias
+    assert "keeper_geom IS NOT NULL" not in fias
+    # the cross-fias anti-over-merge guard is untouched in the canon pass.
+    assert "lower(loser_fias) <> lower(keeper_fias)" in canon
+
+
+def test_mapping_sql_geo_guard_param_toggles_only_distance_filter() -> None:
+    """_mapping_sql(apply_geo_guard=...) toggles ONLY the 250 m distance filter; the cross-fias
+    guard is emitted regardless, and the default is True (canon-safe)."""
+    with_guard = _flat(hdm._mapping_sql(hdm._CANON_KEY_EXPR, apply_geo_guard=True))
+    without_guard = _flat(hdm._mapping_sql(hdm._CANON_KEY_EXPR, apply_geo_guard=False))
+    assert "ST_DistanceSphere" in with_guard
+    assert "ST_DistanceSphere" not in without_guard
+    # default = True (the canon pass must never lose its guard by omission).
+    assert "ST_DistanceSphere" in _flat(hdm._mapping_sql(hdm._CANON_KEY_EXPR))
+    # cross-fias guard present in BOTH renderings (independent of the geo guard).
+    assert "lower(loser_fias) <> lower(keeper_fias)" in with_guard
+    assert "lower(loser_fias) <> lower(keeper_fias)" in without_guard
+
+
 def test_both_passes_share_one_pipeline_no_copy_paste() -> None:
-    """The two mappings differ ONLY in the cluster-key expression (parametrised, no copy-paste)."""
+    """The two mappings differ ONLY in the cluster key + the (canon-only) geo guard — no copy-paste.
+
+    #2187: the 250 m distance guard is emitted for the canon pass ONLY; the fias pass drops it (a
+    shared ГАР UUID IS the building identity and outranks proximity). Everything else — the CTE
+    skeleton, keeper rule, per-partition geom/fias exposure and the cross-fias guard — is identical.
+    """
     canon = _flat(_MAPPING_SQL)
     fias = _flat(_FIAS_MAPPING_SQL)
     assert "'addr:'" in canon and "'addr:'" not in fias
     assert "'fias:'" in fias and "'fias:'" not in canon
-    # both carry the same downstream skeleton (keeper rule, geo guard, cross-fias guard).
+    # both carry the same downstream skeleton (keeper rule, per-partition geom, cross-fias guard).
     for token in (
         "clusters_with_count",
         "listing_counts",
         "ranked AS",
         "AS keeper_geom",
-        "ST_DistanceSphere(loser_geom, keeper_geom) <= 250",
+        "lower(loser_fias) <> lower(keeper_fias)",
     ):
         assert token in canon and token in fias
+    # the 250 m distance guard is CANON-ONLY (#2187) — fias identity outranks proximity.
+    assert "ST_DistanceSphere(loser_geom, keeper_geom) <= 250" in canon
+    assert "ST_DistanceSphere" not in fias
 
 
 def test_cross_fias_guard_blocks_slash_collapse_over_merge() -> None:
@@ -924,3 +964,77 @@ def test_real_fias_pass_cross_guard_and_identity_carryover() -> None:
         db.execute(_t("DELETE FROM houses WHERE id BETWEEN 900020 AND 900025"))
         db.commit()
         db.close()
+
+
+@pytest.mark.skipif(_live_session() is None, reason="no reachable Postgres test DB")
+def test_real_fias_pass_ignores_geo_guard() -> None:
+    """End-to-end on a real DB for #2187 — the FIAS pass drops the 250 m geo guard:
+
+      A. same house_fias_id, loser has NULL geom (no lat/lon)  → MERGED (fias identity outranks
+         the missing coordinate; the geom-first keeper survives + the loser's listing re-points).
+      B. same house_fias_id, ~5 km apart (>250 m)              → MERGED (fias identity outranks
+         proximity — a scattered/broken coordinate no longer blocks the merge).
+      C. same canon, NO fias, ~5 km apart (>250 m)             → NOT merged (the canon pass STILL
+         enforces the geo guard — the fix is fias-only, canon behaviour is unchanged).
+
+    A/B use distinct synthetic streets so ONLY the shared fias groups them (the canon never does);
+    C shares one address so only the canon pass can group it. Streets «*2187» avoid prod aliases.
+    Latitude delta 0.045° at ~56.84° ≈ 5 km (>250 m).
+    """
+    from sqlalchemy import text as _t
+
+    db = _live_session()
+    assert db is not None
+    try:
+        db.execute(
+            _t(
+                "INSERT INTO houses "
+                "(id, source, ext_house_id, address, lat, lon, house_fias_id) VALUES "
+                # A — same fias, loser NULL geom → fias pass merges despite the missing coordinate
+                "(900030,'avito','EXT-2187-A-K','ФиасГеоA2187, 1', 56.84000,60.60000,'F-A-2187'),"
+                "(900031,'cian', 'EXT-2187-A-L','ФиасГеоAL2187, 2',NULL,    NULL,    'F-A-2187'),"
+                # B — same fias, ~5 km apart (>250 m) → fias pass merges despite the distance
+                "(900032,'avito','EXT-2187-B-K','ФиасГеоB2187, 3', 56.84000,60.60000,'F-B-2187'),"
+                "(900033,'cian', 'EXT-2187-B-L','ФиасГеоBL2187, 4',56.88500,60.60000,'F-B-2187'),"
+                # C — same canon, NO fias, ~5 km apart → canon pass STILL blocks (guard unchanged)
+                "(900034,'avito','EXT-2187-C-1','КанонГео2187, 5', 56.84000,60.60000,NULL),"
+                "(900035,'cian', 'EXT-2187-C-2','КанонГео2187, 5', 56.88500,60.60000,NULL)"
+            )
+        )
+        # A loser gets a listing so we prove the re-point still fires with a NULL-geom loser.
+        db.execute(
+            _t(
+                "INSERT INTO listings "
+                "(id, source, source_url, source_id, dedup_hash, price_rub, house_id_fk) VALUES "
+                "(910031,'cian','http://t/2187/al','L-2187-AL','dh-2187-al',6000000,900031)"
+            )
+        )
+        db.commit()
+
+        out = hdm.merge_duplicate_houses(db, dry_run=False)
+        assert out["losers_deleted"] >= 2  # A + B both merged by the fias pass
+
+        ids = [
+            r.id
+            for r in db.execute(
+                _t("SELECT id FROM houses WHERE id BETWEEN 900030 AND 900035 ORDER BY id")
+            ).all()
+        ]
+        # A: same-fias, loser NULL geom → MERGED (the geo guard would have blocked on NULL geom).
+        assert 900030 in ids and 900031 not in ids, "same-fias NULL-geom loser must merge"
+        # B: same-fias, >250 m apart → MERGED (the geo guard would have blocked on distance).
+        assert 900032 in ids and 900033 not in ids, "same-fias >250 m apart must merge"
+        # C: same-canon, no fias, >250 m apart → NOT merged (canon geo guard unchanged by #2187).
+        assert 900034 in ids and 900035 in ids, "canon-only pair >250 m apart must NOT merge"
+
+        # A: the NULL-geom loser's listing re-pointed onto the surviving geom-first keeper.
+        repointed = db.execute(_t("SELECT house_id_fk FROM listings WHERE id = 910031")).scalar()
+        assert repointed == 900030
+    finally:
+        db.rollback()
+        db.execute(_t("DELETE FROM listings WHERE id = 910031"))
+        db.execute(_t("DELETE FROM house_sources WHERE house_id BETWEEN 900030 AND 900035"))
+        db.execute(_t("DELETE FROM house_address_aliases WHERE house_id BETWEEN 900030 AND 900035"))
+        db.execute(_t("DELETE FROM houses WHERE id BETWEEN 900030 AND 900035"))
+        db.commit()
+        db.close()