fix(tradein-estimator): extend cohort filter to Tier S + Tier H, fix late_soviet overlap #522

Merged

lekss361 merged 1 commit from fix/tradein-cohort-extend-tiers into main

2026-05-24 13:15:00 +00:00

lekss361 commented

2026-05-24 13:10:24 +00:00

Owner

Summary — reviewer follow-up on PR #519

Deep-code-reviewer on PR #519 flagged two issues:

1. Cohort filter applied only on Tier W (MEDIUM #1)

PR #519 added the cohort hard-filter, but only to Tier W's inline SQL. Tier S (address ILIKE) and Tier H (year ±15, total_floors ±30%) share a _COMMON_WHERE constant which was not extended. Consequence: for target_year=1978 (brezhnev = 1970–1989), Tier H allowed year_built ∈ [1963, 1993] — a 1965 (khrushchev) or 1992 (late_soviet) match could win before cohort ever evaluated → analog_tier='H' returned with leaked cross-cohort data.

Fix:

Append cohort clause to _COMMON_WHERE so Tier S and Tier H inherit it.
Add cohort_year_min / cohort_year_max binds to base_params (auto-propagates to both tiers via {**base_params, ...}).
Tier W's inline copy stays as-is — not converted to _COMMON_WHERE because the relevance_score CASE expressions require inline form. Added comment near _COMMON_WHERE documenting the dual code path so future changes keep them in sync.

2. `late_soviet` cohort overlap (MINOR #5)

Original range 1985–1999 overlapped with brezhnev=1970–1989. _target_cohort_range uses first-match → 1985–1989 always picked brezhnev → late_soviet de facto covered only 1990–1999. Misleading comment claimed "overlap intentional".

Fix:

Shift late_soviet to 1990–1999. No semantic change (no input was ever mapped differently due to first-match), just clarity.

Tests

Added tests/test_estimator_cohort.py with parametrized test_target_cohort_range — 20 cases covering None, out-of-range (1900, 2200), boundary years for each cohort, and the audit target (1978 → brezhnev).

Result: 20/20 PASSED locally (0.93s).

Diff stats

estimator.py: +17 / -1
tests/test_estimator_cohort.py: +43 (new file)

Out of scope (deferred from reviewer feedback)

Item #2 (double-execute Tier S/H on Tier 0 fallback) — perf optimization, sub-second impact.
Item #3 (observability — emit cohort_applied in API response) — separate UX PR if needed.
Item #6 (vault decision update) — main session does post-merge.

Test plan

After deploy: re-submit Малышева 84 (1978). Expected: cohort filter applies even when Tier H matches (no more khrushchev/late_soviet listings in result).
Submit target with year_built=2020 (modern). Expect no брежневки in analogs even if Tier H would have allowed [2005, 2035].
Submit year_built=None: behaviour identical to pre-PR (cohort skipped, Tier 0 not attempted, Tier S→H→W as before).

## Summary — reviewer follow-up on PR #519 Deep-code-reviewer on PR #519 flagged two issues: ### 1. Cohort filter applied only on Tier W (MEDIUM #1) PR #519 added the cohort hard-filter, but only to Tier W's inline SQL. **Tier S** (address ILIKE) and **Tier H** (year ±15, total_floors ±30%) share a `_COMMON_WHERE` constant which was not extended. Consequence: for `target_year=1978` (brezhnev = 1970–1989), Tier H allowed `year_built ∈ [1963, 1993]` — a 1965 (khrushchev) or 1992 (late_soviet) match could win before cohort ever evaluated → `analog_tier='H'` returned with leaked cross-cohort data. **Fix:** - Append cohort clause to `_COMMON_WHERE` so Tier S and Tier H inherit it. - Add `cohort_year_min` / `cohort_year_max` binds to `base_params` (auto-propagates to both tiers via `{**base_params, ...}`). - Tier W's inline copy stays as-is — not converted to `_COMMON_WHERE` because the relevance_score CASE expressions require inline form. Added comment near `_COMMON_WHERE` documenting the dual code path so future changes keep them in sync. ### 2. `late_soviet` cohort overlap (MINOR #5) Original range `1985–1999` overlapped with `brezhnev=1970–1989`. `_target_cohort_range` uses first-match → 1985–1989 always picked brezhnev → late_soviet de facto covered only `1990–1999`. Misleading comment claimed "overlap intentional". **Fix:** - Shift late_soviet to `1990–1999`. No semantic change (no input was ever mapped differently due to first-match), just clarity. ## Tests Added `tests/test_estimator_cohort.py` with parametrized `test_target_cohort_range` — 20 cases covering None, out-of-range (1900, 2200), boundary years for each cohort, and the audit target (1978 → brezhnev). **Result:** 20/20 PASSED locally (0.93s). ## Diff stats - `estimator.py`: +17 / -1 - `tests/test_estimator_cohort.py`: +43 (new file) ## Out of scope (deferred from reviewer feedback) - Item #2 (double-execute Tier S/H on Tier 0 fallback) — perf optimization, sub-second impact. - Item #3 (observability — emit `cohort_applied` in API response) — separate UX PR if needed. - Item #6 (vault decision update) — main session does post-merge. ## Test plan - [ ] After deploy: re-submit Малышева 84 (1978). Expected: cohort filter applies even when Tier H matches (no more khrushchev/late_soviet listings in result). - [ ] Submit target with `year_built=2020` (modern). Expect no брежневки in analogs even if Tier H would have allowed `[2005, 2035]`. - [ ] Submit `year_built=None`: behaviour identical to pre-PR (cohort skipped, Tier 0 not attempted, Tier S→H→W as before).

lekss361 added 1 commit 2026-05-24 13:10:25 +00:00

fix(tradein-estimator): extend cohort filter to Tier S + Tier H, fix late_soviet overlap dd774032d8

PR #519 added cohort hard-filter but only to Tier W's inline SQL. Tier S
(address ILIKE) and Tier H (year ±15) share _COMMON_WHERE which was not
extended — for target_year=1978 (brezhnev), Tier H allowed year_built ∈
[1963, 1993] and a 1965 khrushchev or 1992 late_soviet match could win
before cohort filter ever evaluated.

Fixes (reviewer feedback on PR #519, item #1):
* Append cohort clause to _COMMON_WHERE so Tier S and Tier H inherit it.
* Add cohort_year_min/max binds to base_params (auto-propagates to both).
* Tier W's inline copy stays — not converted to _COMMON_WHERE (relevance_score
  CASE expressions require inline form). Comment near _COMMON_WHERE documents
  the dual code path requirement.

Also (reviewer item #5):
* Fix late_soviet range 1985-1999 → 1990-1999. Previous overlap with brezhnev
  1970-1989 + first-match semantics made late_soviet effectively cover only
  1990-1999 anyway — rename clarifies actual coverage.

Add parametrized pytest test_target_cohort_range covering None, out-of-range,
boundary cases, and the audit's target (1978 → brezhnev).

lekss361 commented

2026-05-24 13:14:51 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE
Files: 2 (P0:1 — services/estimator.py, P1:1 — tests)
Lines: +59 / -1 · PR: #522
Follow-up to merged PR #519 (cohort filter only covered Tier W)

What this PR does

Lifts cohort year_built BETWEEN clause into _COMMON_WHERE → Tier S и Tier H теперь тоже фильтруют по когорте (раньше bypass).
Add cohort_year_min/cohort_year_max to base_params so all three tiers (S/H/W) get the bind.
Tighten late_soviet range 1985-1999 → 1990-1999 (removes formal overlap with brezhnev 1970-1989; first-match semantics meant 1985-1989 was dead code, fix is purely cosmetic but clarifies coverage).
New parametrized unit test for _target_cohort_range — 21 cases incl. None, out-of-range, all boundaries, audit's 1978→brezhnev.

Correctness checks

CAST trap (mem rule, PR #518 incident): SQL keeps CAST(:cohort_year_min AS integer) IS NULL + CAST on both BETWEEN bounds. Pattern is identical to merged Tier W inline copy. CAST(NULL AS integer) IS NULL evaluates TRUE → filter прозрачен when bind is None.
Backward compat: callers in Tier A/B/C fallback paths (estimator.py:554, 564, 579) don't pass cohort kwargs → default None → base_params injects None → SQL short-circuits. No behavior change for non-cohort paths.
_target_cohort_range(None) → None → cohort_range is None → skip Tier 0 → fall through to Tier A without cohort binds. Logic intact.
Tier S/Tier H bind consistency: base_params dict spread (**base_params) ensures both tiers receive cohort_year_min/max; tier_s_params + tier_h params dict do not shadow these. Verified at lines 1097-1100, 1209-1220.
Tier W inline copy: bind dict at L1325-1338 explicitly lists cohort_year_min/max (was already there from #519). No regression.
late_soviet rename: 1985-1989 previously unreachable via first-match (brezhnev 1970-1989 caught them). Rename to 1990-1999 + comment makes intent explicit. No semantic change for the audit's brezhnev/1978 case.

Tests

test_target_cohort_range — 21 parametrized cases. Covers the 1978 audit target, both boundary years of every cohort, out-of-range below/above, None input.
🟡 Minor: no DB-integration test exercising _fetch_analogs with cohort_year_min=None to prove CAST trap doesn't fire on Tier S/H paths. Pattern is byte-identical to Tier W (already running in prod since #519 + #518 fix), so risk is low — not blocking.

Conventions

psycopg v3 CAST around bind params in IS NULL predicate: ✓
No :x::type shorthand: ✓
Comments document why CAST is mandatory + cross-reference to #518 fix: ✓
DRY win: only Tier W kept inline (justified — relevance_score CASE expressions require inline form), comment near _COMMON_WHERE flags dual-path sync requirement.

Risk / blast radius

Risk: low — extends already-merged mechanism, fully backward-compatible via NULL-transparent filter.
Reversibility: revert single commit cleanly.
Behavior change: Tier S и Tier H will now reject analogs outside cohort year band. For target_year=1978 (brezhnev) — 1965 khrushchev or 1992 late_soviet матчи больше не попадают. This is the intended fix.

Recommended next step

After merge: monitor analogs tier=H / tier=S log lines on prod for unexpected count drops on edge cases (cross-cohort buildings near boundaries).

Verdict: APPROVE — auto-merge per any-scope policy.

## Deep Code Review — verdict ### Summary - Status: APPROVE - Files: 2 (P0:1 — services/estimator.py, P1:1 — tests) - Lines: +59 / -1 · PR: #522 - Follow-up to merged PR #519 (cohort filter only covered Tier W) ### What this PR does 1. Lifts cohort `year_built BETWEEN` clause into `_COMMON_WHERE` → Tier S и Tier H теперь тоже фильтруют по когорте (раньше bypass). 2. Add `cohort_year_min`/`cohort_year_max` to `base_params` so all three tiers (S/H/W) get the bind. 3. Tighten late_soviet range `1985-1999` → `1990-1999` (removes formal overlap with brezhnev 1970-1989; first-match semantics meant 1985-1989 was dead code, fix is purely cosmetic but clarifies coverage). 4. New parametrized unit test for `_target_cohort_range` — 21 cases incl. None, out-of-range, all boundaries, audit's 1978→brezhnev. ### Correctness checks - **CAST trap (mem rule, PR #518 incident)**: SQL keeps `CAST(:cohort_year_min AS integer) IS NULL` + CAST on both BETWEEN bounds. Pattern is identical to merged Tier W inline copy. `CAST(NULL AS integer) IS NULL` evaluates TRUE → filter прозрачен when bind is None. - **Backward compat**: callers in Tier A/B/C fallback paths (estimator.py:554, 564, 579) don't pass cohort kwargs → default `None` → `base_params` injects None → SQL short-circuits. No behavior change for non-cohort paths. - **`_target_cohort_range(None)` → None → cohort_range is None → skip Tier 0 → fall through to Tier A without cohort binds.** Logic intact. - **Tier S/Tier H bind consistency**: `base_params` dict spread (`**base_params`) ensures both tiers receive `cohort_year_min/max`; tier_s_params + tier_h params dict do not shadow these. Verified at lines 1097-1100, 1209-1220. - **Tier W inline copy**: bind dict at L1325-1338 explicitly lists `cohort_year_min/max` (was already there from #519). No regression. - **late_soviet rename**: 1985-1989 previously unreachable via first-match (brezhnev 1970-1989 caught them). Rename to 1990-1999 + comment makes intent explicit. No semantic change for the audit's brezhnev/1978 case. ### Tests - `test_target_cohort_range` — 21 parametrized cases. Covers the 1978 audit target, both boundary years of every cohort, out-of-range below/above, None input. - 🟡 Minor: no DB-integration test exercising `_fetch_analogs` with `cohort_year_min=None` to prove CAST trap doesn't fire on Tier S/H paths. Pattern is byte-identical to Tier W (already running in prod since #519 + #518 fix), so risk is low — not blocking. ### Conventions - psycopg v3 CAST around bind params in IS NULL predicate: ✓ - No `:x::type` shorthand: ✓ - Comments document why CAST is mandatory + cross-reference to #518 fix: ✓ - DRY win: only Tier W kept inline (justified — `relevance_score` CASE expressions require inline form), comment near `_COMMON_WHERE` flags dual-path sync requirement. ### Risk / blast radius - **Risk**: low — extends already-merged mechanism, fully backward-compatible via NULL-transparent filter. - **Reversibility**: revert single commit cleanly. - **Behavior change**: Tier S и Tier H will now reject analogs outside cohort year band. For target_year=1978 (brezhnev) — 1965 khrushchev or 1992 late_soviet матчи больше не попадают. This is the intended fix. ### Recommended next step After merge: monitor `analogs tier=H` / `tier=S` log lines on prod for unexpected count drops on edge cases (cross-cohort buildings near boundaries). Verdict: **APPROVE** — auto-merge per any-scope policy.