fix(tradein-estimator): restore CAST in IS NOT NULL predicates (revert audit #9, prod fix) #518

Merged

lekss361 merged 1 commit from fix/tradein-estimator-restore-cast into main

2026-05-24 12:35:29 +00:00

lekss361 commented

2026-05-24 12:29:25 +00:00

Owner

🚨 Production fix

PR #509 broke /api/v1/trade-in/estimate whenever target_house_type is None (or target_year is None — same risk).

Stack trace (prod 2026-05-24 ~12:25)

sqlalchemy.exc.ProgrammingError: (psycopg.errors.AmbiguousParameter)
 could not determine data type of parameter $4
LINE 24:  WHEN $4 IS NOT NULL
parameters: {'lon': 60.6172026, 'lat': 56.8354334, 'target_year': 1978,
             'target_house_type': None, ...}

$4 is target_house_type. psycopg3 prepared-statement plan step needs the bind type to evaluate IS NOT NULL itself — the CAST(... AS text) in the THEN branch is not enough because it never gets reached during planning.

Root cause

PR #509 removed CAST(:target_year AS integer) and CAST(:target_house_type AS text) from the 4 IS NOT NULL predicates in _fetch_analogs, citing audit finding #9 (perf). The audit's concern was a per-row CAST cost on ≤300 rows — negligible. The inline comment at lines 814–816 of estimator.py already documented why those CASTs were load-bearing:

-- CAST обязателен: target_year / target_house_type приходят NULL
-- без типа → PostgreSQL "could not determine data type of parameter"
-- (AmbiguousParameter). Явный тип снимает неоднозначность.

PR #509 misread the audit; this PR reverts that single piece. THEN-branch CASTs untouched (always present). Other PR #509 changes (IMV atomic link, batched yandex history) stay in place — unrelated.

Diff

Exactly 4 lines edited (×2 in SELECT relevance_score, ×2 in row_number OVER ORDER BY):

WHEN :target_year IS NOT NULL → WHEN CAST(:target_year AS integer) IS NOT NULL
WHEN :target_house_type IS NOT NULL → WHEN CAST(:target_house_type AS text) IS NOT NULL

Verification

0 bare :target_year IS NOT NULL / :target_house_type IS NOT NULL left (grep).
4 CAST forms present (grep count).
AST parse: OK.

Test plan

After deploy: submit /estimate with house_type=null from frontend; expect 200 OK with estimate result.
Submit with both year and house_type null; expect 200 OK.
Submit with both filled; expect same 200 OK (regression check).

## 🚨 Production fix PR #509 broke `/api/v1/trade-in/estimate` whenever `target_house_type` is None (or `target_year` is None — same risk). ### Stack trace (prod 2026-05-24 ~12:25) ``` sqlalchemy.exc.ProgrammingError: (psycopg.errors.AmbiguousParameter) could not determine data type of parameter $4 LINE 24: WHEN $4 IS NOT NULL parameters: {'lon': 60.6172026, 'lat': 56.8354334, 'target_year': 1978, 'target_house_type': None, ...} ``` `$4` is `target_house_type`. psycopg3 prepared-statement plan step needs the bind type to evaluate `IS NOT NULL` itself — the `CAST(... AS text)` in the THEN branch is not enough because it never gets reached during planning. ### Root cause PR #509 removed `CAST(:target_year AS integer)` and `CAST(:target_house_type AS text)` from the 4 `IS NOT NULL` predicates in `_fetch_analogs`, citing audit finding #9 (perf). The audit's concern was a per-row CAST cost on ≤300 rows — negligible. The inline comment at lines 814–816 of `estimator.py` already documented why those CASTs were load-bearing: ``` -- CAST обязателен: target_year / target_house_type приходят NULL -- без типа → PostgreSQL "could not determine data type of parameter" -- (AmbiguousParameter). Явный тип снимает неоднозначность. ``` PR #509 misread the audit; this PR reverts that single piece. THEN-branch CASTs untouched (always present). Other PR #509 changes (IMV atomic link, batched yandex history) stay in place — unrelated. ### Diff Exactly 4 lines edited (×2 in SELECT relevance_score, ×2 in row_number OVER ORDER BY): - `WHEN :target_year IS NOT NULL` → `WHEN CAST(:target_year AS integer) IS NOT NULL` - `WHEN :target_house_type IS NOT NULL` → `WHEN CAST(:target_house_type AS text) IS NOT NULL` ### Verification - 0 bare `:target_year IS NOT NULL` / `:target_house_type IS NOT NULL` left (grep). - 4 CAST forms present (grep count). - AST parse: OK. ## Test plan - [ ] After deploy: submit `/estimate` with `house_type=null` from frontend; expect 200 OK with estimate result. - [ ] Submit with both year and house_type null; expect 200 OK. - [ ] Submit with both filled; expect same 200 OK (regression check).

lekss361 added 1 commit 2026-05-24 12:29:26 +00:00

fix(tradein-estimator): restore CAST in IS NOT NULL predicates (revert audit #9 ) 4a2a804296

PR #509 removed CAST(:target_year AS integer) and CAST(:target_house_type AS text)
from the IS NOT NULL predicates in _fetch_analogs, citing audit finding #9 (perf).
This broke production: psycopg3 prepared statement could not determine $4 type
when target_house_type=None, raising AmbiguousParameter at plan time. The CAST
in the THEN branch is not enough — PG planner needs the type to evaluate
IS NOT NULL itself.

The inline comment at lines 814-816 explicitly documented this requirement;
PR #509 misread the audit finding. Per-row CAST cost on ≤300 rows is negligible
vs production breakage. Restoring CAST forms; THEN-branch CASTs untouched
(they were never removed).

Verified prod stack trace 2026-05-24 ~12:25 (target_year=1978, target_house_type=None).

lekss361 commented

2026-05-24 12:35:18 +00:00

Author

Owner

Deep Code Review — verdict

Summary

Status: APPROVE
Files: 1 (P0 prod hotfix — backend/app/services/estimator.py)
Lines: +4 / -4 · Scope: surgical revert of PR #509 over-cleanup · PR: #518

Correctness 🎯 (the load-bearing point)

The stack trace psycopg.errors.AmbiguousParameter: could not determine data type of parameter $4 / LINE 24: WHEN $4 IS NOT NULL is exactly the failure mode that bare :name IS NOT NULL produces in psycopg v3 prepared-statement plans.

During plan generation psycopg needs to resolve every $N to a Postgres OID. A bare $N IS NOT NULL predicate provides no type context — when the Python value is None, the driver cannot infer it, plan fails before any row is evaluated.
CAST in the THEN branch is not reached during planning — only the predicate position counts.
CAST(NULL AS integer) IS NOT NULL → FALSE, same as NULL IS NOT NULL → FALSE (Postgres IS NOT NULL on a typed NULL is well-defined). No boolean-eval regression.
The inline comment at lines 879–881 of estimator.py already documents this exact gotcha. PR #509 (audit finding #9) removed the CAST despite that warning.

Performance ⚡

No concern. The CAST is on a constant bind parameter (not a column), planner constant-folds it once; per-row cost is zero. The audit's perf concern was misapplied — it would only matter if CAST were applied to year_built / house_type columns.

Project conventions 📋

Matches .claude/rules/backend.md:

В SQL text(...): всегда CAST(:name AS type) — НИКОГДА :name::type.

The revert restores compliance.

Verification

grep counts: 0 bare :target_year IS NOT NULL, 0 bare :target_house_type IS NOT NULL, 2 CAST(:target_year AS integer) IS NOT NULL, 2 CAST(:target_house_type AS text) IS NOT NULL ✅
AST parse: OK ✅
Scope: only _fetch_analogs SQL touched; IMV / yandex history changes from PR #509 untouched ✅
Callers (lines 503/511/523/797) pass int | None / str | None — types match CAST targets ✅

Cross-file impact

None — change is confined to the SQL string literal inside _fetch_analogs. Same call sites, same param names, same param values. Behavior identical when both params are non-NULL (regression-safe).

Recommended follow-up (non-blocking)

Add a regression test in tradein-mvp/backend/tests/test_estimator.py (or equivalent): call _fetch_analogs(..., year_built=None, house_type=None) against a real DB and assert it does not raise AmbiguousParameter. Cheapest insurance against PR #509-style re-occurrence.

Complexity / blast radius

Risk: very low (4-line revert restoring previous behaviour)
Reversibility: trivial
Merge window: prod is currently broken on this path → merge now

Approved. Merging.

## Deep Code Review — verdict ### Summary - Status: APPROVE - Files: 1 (P0 prod hotfix — `backend/app/services/estimator.py`) - Lines: +4 / -4 · Scope: surgical revert of PR #509 over-cleanup · PR: #518 ### Correctness 🎯 (the load-bearing point) The stack trace `psycopg.errors.AmbiguousParameter: could not determine data type of parameter $4 / LINE 24: WHEN $4 IS NOT NULL` is exactly the failure mode that bare `:name IS NOT NULL` produces in psycopg v3 prepared-statement plans. - During plan generation psycopg needs to resolve every `$N` to a Postgres OID. A bare `$N IS NOT NULL` predicate provides **no type context** — when the Python value is `None`, the driver cannot infer it, plan fails before any row is evaluated. - CAST in the THEN branch is **not reached** during planning — only the predicate position counts. - `CAST(NULL AS integer) IS NOT NULL` → `FALSE`, same as `NULL IS NOT NULL` → `FALSE` (Postgres `IS NOT NULL` on a typed NULL is well-defined). No boolean-eval regression. - The inline comment at lines 879–881 of estimator.py already documents this exact gotcha. PR #509 (audit finding #9) removed the CAST despite that warning. ### Performance ⚡ No concern. The CAST is on a **constant bind parameter** (not a column), planner constant-folds it once; per-row cost is zero. The audit's perf concern was misapplied — it would only matter if CAST were applied to `year_built` / `house_type` columns. ### Project conventions 📋 Matches `.claude/rules/backend.md`: > В SQL `text(...)`: всегда `CAST(:name AS type)` — НИКОГДА `:name::type`. The revert restores compliance. ### Verification - grep counts: 0 bare `:target_year IS NOT NULL`, 0 bare `:target_house_type IS NOT NULL`, 2 `CAST(:target_year AS integer) IS NOT NULL`, 2 `CAST(:target_house_type AS text) IS NOT NULL` ✅ - AST parse: OK ✅ - Scope: only `_fetch_analogs` SQL touched; IMV / yandex history changes from PR #509 untouched ✅ - Callers (lines 503/511/523/797) pass `int | None` / `str | None` — types match CAST targets ✅ ### Cross-file impact None — change is confined to the SQL string literal inside `_fetch_analogs`. Same call sites, same param names, same param values. Behavior identical when both params are non-NULL (regression-safe). ### Recommended follow-up (non-blocking) Add a regression test in `tradein-mvp/backend/tests/test_estimator.py` (or equivalent): call `_fetch_analogs(..., year_built=None, house_type=None)` against a real DB and assert it does not raise `AmbiguousParameter`. Cheapest insurance against PR #509-style re-occurrence. ### Complexity / blast radius - Risk: very low (4-line revert restoring previous behaviour) - Reversibility: trivial - Merge window: prod is currently broken on this path → merge now Approved. Merging.