feat(nspd): denorm nspd_parcels + nspd_buildings (#94 PR3 of 4) #222

Merged
lekss361 merged 1 commit from feat/94-pr3-denorm-entities into main 2026-05-16 17:57:00 +00:00
Owner

Summary

Sub-PR 3 of 4 для #94. Денормализует NSPD features из nspd_quarter_dumps.features_json JSONB в отдельные entity таблицы для быстрого lookup.

  • nspd_parcels (PK cad_num) — permitted_use, land_category, cost_value, cost_per_m2, area_sqm, geom (4326)
  • nspd_buildings (PK cad_num) — purpose, floors, floors_underground, year_built, cost_value, build_record_area, geom (4326). Partial index WHERE purpose ILIKE '%многокв%' для МКД-фильтра.
  • ETL nspd_denorm.py: SAVEPOINT per row, ST_Transform 3857→4326, json.dumps geom bind
  • Inline call в harvest_quarter (non-fatal — denorm failure не блокирует harvest)
  • Celery one-shot backfill task для existing dumps
  • Admin endpoint POST /api/v1/admin/etl/nspd-denorm-backfill?limit=N

Roadmap (#94)

  • PR1 (#217): TIER 3 risk zones
  • PR2 (#220): TIER 4 opportunity + red lines
  • PR3 (этот): denorm entity tables
  • PR4: TIER 5 ARN квартирография heatmap (~450 LOC)

Files (6, +932 LOC)

  • data/sql/99_nspd_entities_denorm.sql (NEW, +171) — 2 tables + 6 indexes (idempotent)
  • backend/app/services/scrapers/nspd_denorm.py (NEW, +290)
  • backend/app/workers/tasks/nspd_denorm_backfill.py (NEW, +98)
  • backend/app/workers/tasks/nspd_sync.py (+27 EDIT)
  • backend/app/api/v1/admin_etl.py (+26 EDIT)
  • backend/tests/services/test_nspd_denorm.py (NEW, +322, 19 tests)

Test plan

  • Deploy → migration 99 apply: SELECT * FROM information_schema.tables WHERE table_name IN ('nspd_parcels','nspd_buildings'); → 2 rows
  • Trigger backfill (после следующего harvest cycle для real data):
    curl -X POST "https://gendsgn.ru/api/v1/admin/etl/nspd-denorm-backfill?limit=10" \
      -H "X-Admin-Token: $SCRAPE_ADMIN_TOKEN"
    
  • Property mapping spot-check (post-backfill):
    SELECT count(*), count(permitted_use), count(land_category), count(cost_value) FROM nspd_parcels;
    SELECT count(*), count(purpose), count(floors), count(year_built) FROM nspd_buildings;
    
    Если permitted_use / cost_value сильно NULL — property names в features_json могут быть cn_30/cn_36 (NSPD PKK REST) вместо permitted_use/cost_value (WMS). Тогда нужно adjust mapping.
  • cd backend && uv run pytest tests/services/test_nspd_denorm.py -v → 19/19 pass

Pre-push checks

  • ruff + ruff-format: pass
  • code-reviewer subagent: APPROVE
    • psycopg v3: все CAST(:x AS type), нет :x::type
    • SQL injection: нет f-string в text()
    • SRID consistency: ST_Transform(ST_SetSRID(..., 3857), 4326) правильно
    • SAVEPOINT pattern: with db.begin_nested() per row
    • JSON geom: json.dumps(geom, ensure_ascii=False) (не Python str(dict))
    • Admin auth: AdminTokenAuth dep
    • Migration 99_* после 98_* — sequential order

Known follow-ups

  • Phantom column gate (PR #216) НЕ покрывает new tables (created at migration apply time, dev DB не имеет 99_*). Добавить в test_phantom_columns.py после deploy.
  • Property naming uncertainty — verify через post-deploy SQL spot-check; adjust mapping если features_json содержит cn_* keys
  • Backfill estimate: 1000 quarters × ~30 features → ~30k parcels + ~10k buildings, ~40s

Part of #94

## Summary **Sub-PR 3 of 4** для #94. Денормализует NSPD features из `nspd_quarter_dumps.features_json` JSONB в отдельные entity таблицы для быстрого lookup. - `nspd_parcels` (PK `cad_num`) — permitted_use, land_category, cost_value, cost_per_m2, area_sqm, geom (4326) - `nspd_buildings` (PK `cad_num`) — purpose, floors, floors_underground, year_built, cost_value, build_record_area, geom (4326). Partial index `WHERE purpose ILIKE '%многокв%'` для МКД-фильтра. - ETL `nspd_denorm.py`: SAVEPOINT per row, ST_Transform 3857→4326, json.dumps geom bind - Inline call в `harvest_quarter` (non-fatal — denorm failure не блокирует harvest) - Celery one-shot backfill task для existing dumps - Admin endpoint `POST /api/v1/admin/etl/nspd-denorm-backfill?limit=N` ## Roadmap (#94) - ✅ **PR1** (#217): TIER 3 risk zones - ✅ **PR2** (#220): TIER 4 opportunity + red lines - ⏳ **PR3** (этот): denorm entity tables - ⏳ **PR4**: TIER 5 ARN квартирография heatmap (~450 LOC) ## Files (6, +932 LOC) - `data/sql/99_nspd_entities_denorm.sql` (NEW, +171) — 2 tables + 6 indexes (idempotent) - `backend/app/services/scrapers/nspd_denorm.py` (NEW, +290) - `backend/app/workers/tasks/nspd_denorm_backfill.py` (NEW, +98) - `backend/app/workers/tasks/nspd_sync.py` (+27 EDIT) - `backend/app/api/v1/admin_etl.py` (+26 EDIT) - `backend/tests/services/test_nspd_denorm.py` (NEW, +322, 19 tests) ## Test plan - [ ] Deploy → migration 99 apply: `SELECT * FROM information_schema.tables WHERE table_name IN ('nspd_parcels','nspd_buildings');` → 2 rows - [ ] Trigger backfill (после следующего harvest cycle для real data): ```bash curl -X POST "https://gendsgn.ru/api/v1/admin/etl/nspd-denorm-backfill?limit=10" \ -H "X-Admin-Token: $SCRAPE_ADMIN_TOKEN" ``` - [ ] **Property mapping spot-check** (post-backfill): ```sql SELECT count(*), count(permitted_use), count(land_category), count(cost_value) FROM nspd_parcels; SELECT count(*), count(purpose), count(floors), count(year_built) FROM nspd_buildings; ``` Если `permitted_use` / `cost_value` сильно NULL — property names в features_json могут быть `cn_30`/`cn_36` (NSPD PKK REST) вместо `permitted_use`/`cost_value` (WMS). Тогда нужно adjust mapping. - [ ] `cd backend && uv run pytest tests/services/test_nspd_denorm.py -v` → 19/19 pass ## Pre-push checks - ruff + ruff-format: pass - code-reviewer subagent: APPROVE - psycopg v3: все `CAST(:x AS type)`, нет `:x::type` - SQL injection: нет f-string в `text()` - SRID consistency: `ST_Transform(ST_SetSRID(..., 3857), 4326)` правильно - SAVEPOINT pattern: `with db.begin_nested()` per row - JSON geom: `json.dumps(geom, ensure_ascii=False)` (не Python `str(dict)`) - Admin auth: `AdminTokenAuth` dep - Migration `99_*` после `98_*` — sequential order ## Known follow-ups - Phantom column gate (PR #216) НЕ покрывает new tables (created at migration apply time, dev DB не имеет 99_*). Добавить в `test_phantom_columns.py` после deploy. - Property naming uncertainty — verify через post-deploy SQL spot-check; adjust mapping если features_json содержит `cn_*` keys - Backfill estimate: 1000 quarters × ~30 features → ~30k parcels + ~10k buildings, ~40s Part of #94
lekss361 added 1 commit 2026-05-16 17:20:41 +00:00
- data/sql/99_nspd_entities_denorm.sql: 2 tables (cad_num PK) + 6 indexes
  (quarter_cad, GIST geom, partial WHERE purpose ILIKE '%многокв%')
- nspd_denorm.py: denorm_parcel_feature / denorm_building_feature / denorm_dump
  - SAVEPOINT per row via with db.begin_nested()
  - ST_Transform(ST_SetSRID(ST_GeomFromGeoJSON(:geom), 3857), 4326)
  - json.dumps(geom, ensure_ascii=False) for psycopg bind
  - All CAST(:x AS type) — no :type
- nspd_sync.py: inline denorm after _upsert_dump in harvest_quarter (non-fatal)
- nspd_denorm_backfill.py: Celery one-shot task (per-quarter commit)
- admin_etl.py: POST /api/v1/admin/etl/nspd-denorm-backfill (AdminTokenAuth)
- 19 tests: coerce helpers, denorm parcel/building, aggregate counts

Foundation for downstream features (МКД neighbors lookup, parcel attrs).

Part of #94
Author
Owner

Deep Code Review — verdict

Summary

  • Status: APPROVE (with minor non-blocking notes)
  • Files reviewed: 6 (P0: 1 SQL, P1: 3 backend, P2: 1 worker change, 1 tests)
  • Lines: +932 / -0
  • SHA: db1ff5b
  • PR: #222 (PR3 of 4, builds on merged #217 + #220)

🔴 Critical (BLOCK)

None.

🟠 High

None.

🟡 Medium (worth addressing — non-blocking)

  • backend/app/services/scrapers/nspd_denorm.py:262-280denorm_dump conflates expected skips (no cad_num) with real DB errors into single errors_n counter. Docstring acknowledges это ("пропусков (нет cad_num, DB ошибки)"), но в проде telemetry будет показывать высокий errors_n даже на healthy NSPD data (часть feature'ов в NSPD приходит без cad_num — например, общие fragments quartal'а). Suggested: split errors_nskipped_n (no cad_num) + errors_n (DB failure). Сразу видно если real ETL начнёт ломаться.
  • backend/app/workers/tasks/nspd_denorm_backfill.py:26soft_time_limit=3600 (1h) недостаточно для full backfill at scale. На Свердловской обл. ожидается ~50k+ кварталов × ~100 features = ~5M UPSERTов × ~5-10ms (SAVEPOINT overhead) ≈ 7-14 часов. Сейчас прод-data пустая (0 dumps) — реального impact нет, но при первом full harvest backfill будет фейлиться по soft_time_limit. Suggest либо: (a) поднять до 8h при known full-region run, либо (b) batch-resume — track last quarter_cad, продолжать с него при retry. Можно отдельным PR.
  • backend/app/api/v1/admin_etl.py:80-82limit: int | None = None без upper bound. Случайный POST /api/v1/admin/etl/nspd-denorm-backfill без ?limit= запустит all-quarters scan. Защищено AdminTokenAuth, но defense-in-depth: добавить max cap (например, Query(..., le=100_000)).

🟢 Low / nits

  • data/sql/99_nspd_entities_denorm.sql:160 — partial index WHERE purpose = 'Многоквартирный дом' exact-match. Если НСПД когда-нибудь нормализует строку (whitespace, casing, синонимы "МКД" / "Многоквартирные дома") — индекс станет неэффективен. Сейчас OK, но добавить test/runbook check на values DISTINCT purpose после первого harvest'а.
  • backend/app/services/scrapers/nspd_denorm.py:140-145, 220-226except Exception as e: logger.warning(...) — generic catch без logger.exception. Stack trace потерян, debug по NSPD-edge-case будет сложнее. Suggest logger.warning("...", exc_info=True) или logger.exception(...).

Cross-file impact analysis

  • nspd_sync.py:305-329 — inline denorm правильно не-fatal: outer try/except Exception ловит любую ошибку (включая SessionLocal() failure), inner try/finally гарантирует denorm_db.close(). Harvest продолжается даже если denorm полностью сломан. Good.
  • Миграция order: prod _schema_migrations last = 98_* (verified via MCP), file = 99_* — correct sequential slot, auto-apply через deploy.yml поднимет таблицы перед deploy backend.
  • features_json column в nspd_quarter_dumps существует (verified via MCP: data_type=jsonb, default='[]'::jsonb) — denorm read-path не сломается.
  • Property-key mapping (cad_num, permitted_use, cost_value, area, floors_above_ground, year_built, build_record_area) совпадает с WMS GetFeatureInfo raw response (NSPDFeature.from_raw → pass-through через props = raw.get("properties") or {}). PKK REST cn_30/cn_36 naming здесь не применим — это WMS pipeline.
  • ⚠️ Не верифицировано на реальных данных: nspd_quarter_dumps сейчас пустая (SELECT COUNT(*) = 0 в prod via MCP), поэтому property-key mapping unverifiable end-to-end до первого harvest run. Post-merge: первый harvest_quarter → проверить что nspd_parcels.permitted_use IS NOT NULL для свежей строки.

Conventions / standards

  • psycopg v3: all CAST(:x AS type), никаких :x::type (CAST trap избежан)
  • No f-string SQL, всё parameterized через text() + dict
  • SAVEPOINT pattern with db.begin_nested(): per-row (matches .claude/rules/backend.md)
  • json.dumps(geom, ensure_ascii=False) для geom вместо str(dict) (per CLAUDE.md rule)
  • SQL idempotent: CREATE TABLE IF NOT EXISTS, CREATE INDEX IF NOT EXISTS, ON CONFLICT DO UPDATE
  • BEGIN; ... COMMIT; wrap (per .claude/rules/sql.md)
  • GIST индексы на geom (SRID 4326 consistency), B-tree на quarter_cad/cost_per_m2, partial index на МКД
  • logger.* everywhere, no print()
  • Imports / typing OK (Python 3.12 syntax — dict | None)

Tests coverage

  • 19 unit tests покрывают: coerce helpers (int/numeric, comma decimal, None, invalid), parcel insert path (full feature, no cad_num, zero area cost_per_m2=None, null geometry, DB exception), building insert path, dump aggregation, empty features, unknown layer skip, no-cad_num как error
  • ⚠️ Нет integration test с real PostgreSQL+PostGIS — ST_Transform/ST_SetSRID/ST_GeomFromGeoJSON поведение не верифицировано в unit (мок не выполняет реальный SQL). Acceptable для unit scope; рекомендовано добавить один integration test с docker-compose postgres в future PR (отдельным chore/, не блок).

Positive observations

  • Inline denorm в harvest_quarter правильно non-fatal — bug в denorm не положит harvest pipeline
  • Per-row SAVEPOINT не блокирует batch — одна broken feature не теряет весь quarter
  • ON CONFLICT DO UPDATE для re-harvest idempotency
  • Generic GEOMETRY(Geometry, 4326) (не Polygon) — корректно для NSPD который может вернуть MultiPolygon/Point
  • Подробные COMMENT ON TABLE/COLUMN/INDEX — самодокументирующаяся схема
  • _PARCEL_UPSERT_SQL / _BUILDING_UPSERT_SQL как module-level text() — переиспользуется compiled statement (perf win)

Complexity / blast radius

  • Risk: Low — две новые таблицы (изолированные, нет FK на существующие), inline denorm non-fatal, миграция полностью additive
  • Reversibility: Easy — DROP TABLE nspd_parcels, nspd_buildings CASCADE + удалить запись из _schema_migrations. Никаких изменений existing schema.
  • Merge window: Anytime. Backfill admin endpoint — манual trigger, не auto-cron.
  1. Post-merge: дождаться первого harvest_quarter на реальном квартале → verify SELECT * FROM nspd_parcels WHERE permitted_use IS NOT NULL LIMIT 5; (property-mapping smoke check)
  2. Перед full backfill: запустить с ?limit=10 → измерить avg time per quarter → решить про soft_time_limit
  3. PR4 (последний из split): integration logic / consumers использующие nspd_parcels/buildings

Blocked for auto-merge per .claude/rules/git-pr.md scope policy (touches data/sql/**, backend/app/api/v1/**, backend/app/services/**, backend/app/workers/**). Human merge required.

Verdict: code-quality APPROVE — implementation clean, conventions соблюдены, мелкие nit'ы не блокирующие. Все три medium-замечания — improvements, не bugs. PR3/4 готов к человеческому merge.

<!-- gendesign-review-bot: sha=db1ff5b verdict=approve --> ## Deep Code Review — verdict ### Summary - **Status**: ✅ APPROVE (with minor non-blocking notes) - **Files reviewed**: 6 (P0: 1 SQL, P1: 3 backend, P2: 1 worker change, 1 tests) - **Lines**: +932 / -0 - **SHA**: `db1ff5b` - **PR**: #222 (PR3 of 4, builds on merged #217 + #220) ### 🔴 Critical (BLOCK) _None._ ### 🟠 High _None._ ### 🟡 Medium (worth addressing — non-blocking) - [ ] `backend/app/services/scrapers/nspd_denorm.py:262-280` — `denorm_dump` conflates **expected skips** (no `cad_num`) with **real DB errors** into single `errors_n` counter. Docstring acknowledges это ("пропусков (нет cad_num, DB ошибки)"), но в проде telemetry будет показывать высокий errors_n даже на healthy NSPD data (часть feature'ов в NSPD приходит без cad_num — например, общие fragments quartal'а). Suggested: split `errors_n` → `skipped_n` (no cad_num) + `errors_n` (DB failure). Сразу видно если real ETL начнёт ломаться. - [ ] `backend/app/workers/tasks/nspd_denorm_backfill.py:26` — `soft_time_limit=3600` (1h) недостаточно для full backfill at scale. На Свердловской обл. ожидается ~50k+ кварталов × ~100 features = ~5M UPSERTов × ~5-10ms (SAVEPOINT overhead) ≈ 7-14 часов. **Сейчас прод-data пустая (0 dumps)** — реального impact нет, но при первом full harvest backfill будет фейлиться по soft_time_limit. Suggest либо: (a) поднять до 8h при known full-region run, либо (b) batch-resume — track last `quarter_cad`, продолжать с него при retry. Можно отдельным PR. - [ ] `backend/app/api/v1/admin_etl.py:80-82` — `limit: int | None = None` без upper bound. Случайный `POST /api/v1/admin/etl/nspd-denorm-backfill` без `?limit=` запустит **all-quarters** scan. Защищено `AdminTokenAuth`, но defense-in-depth: добавить max cap (например, `Query(..., le=100_000)`). ### 🟢 Low / nits - [ ] `data/sql/99_nspd_entities_denorm.sql:160` — partial index `WHERE purpose = 'Многоквартирный дом'` exact-match. Если НСПД когда-нибудь нормализует строку (whitespace, casing, синонимы "МКД" / "Многоквартирные дома") — индекс станет неэффективен. Сейчас OK, но добавить test/runbook check на values DISTINCT purpose после первого harvest'а. - [ ] `backend/app/services/scrapers/nspd_denorm.py:140-145, 220-226` — `except Exception as e: logger.warning(...)` — generic catch без `logger.exception`. Stack trace потерян, debug по NSPD-edge-case будет сложнее. Suggest `logger.warning("...", exc_info=True)` или `logger.exception(...)`. ### Cross-file impact analysis - ✅ `nspd_sync.py:305-329` — inline denorm правильно не-fatal: outer `try/except Exception` ловит любую ошибку (включая `SessionLocal()` failure), inner `try/finally` гарантирует `denorm_db.close()`. Harvest продолжается даже если denorm полностью сломан. Good. - ✅ Миграция order: prod `_schema_migrations` last = `98_*` (verified via MCP), file = `99_*` — correct sequential slot, auto-apply через `deploy.yml` поднимет таблицы перед deploy backend. - ✅ `features_json` column в `nspd_quarter_dumps` существует (verified via MCP: `data_type=jsonb`, `default='[]'::jsonb`) — denorm read-path не сломается. - ✅ Property-key mapping (`cad_num`, `permitted_use`, `cost_value`, `area`, `floors_above_ground`, `year_built`, `build_record_area`) совпадает с WMS GetFeatureInfo raw response (NSPDFeature.from_raw → pass-through через `props = raw.get("properties") or {}`). PKK REST `cn_30/cn_36` naming здесь не применим — это WMS pipeline. - ⚠️ **Не верифицировано на реальных данных**: `nspd_quarter_dumps` сейчас пустая (`SELECT COUNT(*) = 0` в prod via MCP), поэтому property-key mapping unverifiable end-to-end до первого harvest run. Post-merge: первый `harvest_quarter` → проверить что `nspd_parcels.permitted_use IS NOT NULL` для свежей строки. ### Conventions / standards - ✅ psycopg v3: all `CAST(:x AS type)`, никаких `:x::type` (CAST trap избежан) - ✅ No f-string SQL, всё parameterized через `text()` + dict - ✅ SAVEPOINT pattern `with db.begin_nested():` per-row (matches `.claude/rules/backend.md`) - ✅ `json.dumps(geom, ensure_ascii=False)` для geom вместо `str(dict)` (per CLAUDE.md rule) - ✅ SQL idempotent: `CREATE TABLE IF NOT EXISTS`, `CREATE INDEX IF NOT EXISTS`, `ON CONFLICT DO UPDATE` - ✅ `BEGIN; ... COMMIT;` wrap (per `.claude/rules/sql.md`) - ✅ GIST индексы на geom (SRID 4326 consistency), B-tree на quarter_cad/cost_per_m2, partial index на МКД - ✅ `logger.*` everywhere, no `print()` - ✅ Imports / typing OK (Python 3.12 syntax — `dict | None`) ### Tests coverage - ✅ 19 unit tests покрывают: coerce helpers (int/numeric, comma decimal, None, invalid), parcel insert path (full feature, no cad_num, zero area cost_per_m2=None, null geometry, DB exception), building insert path, dump aggregation, empty features, unknown layer skip, no-cad_num как error - ⚠️ Нет integration test с real PostgreSQL+PostGIS — `ST_Transform`/`ST_SetSRID`/`ST_GeomFromGeoJSON` поведение не верифицировано в unit (мок не выполняет реальный SQL). Acceptable для unit scope; рекомендовано добавить один integration test с docker-compose postgres в future PR (отдельным chore/, не блок). ### Positive observations - ✅ Inline denorm в `harvest_quarter` правильно non-fatal — bug в denorm не положит harvest pipeline - ✅ Per-row SAVEPOINT не блокирует batch — одна broken feature не теряет весь quarter - ✅ `ON CONFLICT DO UPDATE` для re-harvest idempotency - ✅ Generic `GEOMETRY(Geometry, 4326)` (не Polygon) — корректно для NSPD который может вернуть MultiPolygon/Point - ✅ Подробные `COMMENT ON TABLE/COLUMN/INDEX` — самодокументирующаяся схема - ✅ `_PARCEL_UPSERT_SQL` / `_BUILDING_UPSERT_SQL` как module-level `text()` — переиспользуется compiled statement (perf win) ### Complexity / blast radius - **Risk**: Low — две новые таблицы (изолированные, нет FK на существующие), inline denorm non-fatal, миграция полностью additive - **Reversibility**: Easy — `DROP TABLE nspd_parcels, nspd_buildings CASCADE` + удалить запись из `_schema_migrations`. Никаких изменений existing schema. - **Merge window**: Anytime. Backfill admin endpoint — манual trigger, не auto-cron. ### Recommended next steps 1. Post-merge: дождаться первого `harvest_quarter` на реальном квартале → verify `SELECT * FROM nspd_parcels WHERE permitted_use IS NOT NULL LIMIT 5;` (property-mapping smoke check) 2. Перед full backfill: запустить с `?limit=10` → измерить avg time per quarter → решить про soft_time_limit 3. PR4 (последний из split): integration logic / consumers использующие nspd_parcels/buildings --- ❌ **Blocked for auto-merge** per `.claude/rules/git-pr.md` scope policy (touches `data/sql/**`, `backend/app/api/v1/**`, `backend/app/services/**`, `backend/app/workers/**`). Human merge required. _Verdict: code-quality APPROVE — implementation clean, conventions соблюдены, мелкие nit'ы не блокирующие. Все три medium-замечания — improvements, не bugs. PR3/4 готов к человеческому merge._
lekss361 merged commit 4860cc790a into main 2026-05-16 17:57:00 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: lekss361/gendesign#222
No description provided.