From 762d539d5263059d2795ad774114440bdb7d0d86 Mon Sep 17 00:00:00 2001 From: Dmitry Dementev Date: Sun, 18 Jan 2026 21:22:00 +0300 Subject: [PATCH] =?UTF-8?q?=D0=A0=D0=B5=D0=B2=D1=8C=D1=8E=20=D1=80=D0=B5?= =?UTF-8?q?=D1=88=D0=B5=D0=BD=D0=B8=D1=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/bookings_to_gp_stage.md | 7 ++ docs/internal/bookings_stg_code_review.md | 147 ++++++++++++++++++++++ docs/internal/db_schema.md | 1 + 3 files changed, 155 insertions(+) create mode 100644 docs/internal/bookings_stg_code_review.md diff --git a/docs/bookings_to_gp_stage.md b/docs/bookings_to_gp_stage.md index e07d4e9..eb6ea76 100644 --- a/docs/bookings_to_gp_stage.md +++ b/docs/bookings_to_gp_stage.md @@ -12,6 +12,8 @@ - Сверяет количество строк между источником (за окно инкремента) и загруженным батчем. - Загружает инкремент в `stg.tickets` через внешнюю таблицу `stg.tickets_ext`, используя PXF. - Запускает DQ‑проверки для `stg.tickets` (количество, ссылочная целостность, обязательные поля). +- Загружает справочники (full load): `stg.airports`, `stg.airplanes`, `stg.routes`, `stg.seats` + DQ. +- Загружает транзакции: `stg.flights` (инкремент), `stg.segments` (инкремент), `stg.boarding_passes` (full snapshot) + DQ. ## Что должно быть готово перед запуском @@ -113,3 +115,8 @@ LIMIT 10; - Ошибки про `stg.bookings_ext`/`stg.bookings`: не применён DDL → запустите `bookings_stg_ddl` или `make ddl-gp`. - Ошибки PXF (`protocol "pxf" does not exist`, connection refused): перезапустите `greenplum` и повторите DDL. Для технических деталей см. `docs/internal/pxf_bookings.md`. + +## Рекомендации по качеству решения + +Ревью решения и список улучшений, которые делают пайплайн более “эталонным” для обучения: +`docs/internal/bookings_stg_code_review.md`. diff --git a/docs/internal/bookings_stg_code_review.md b/docs/internal/bookings_stg_code_review.md new file mode 100644 index 0000000..3caffa7 --- /dev/null +++ b/docs/internal/bookings_stg_code_review.md @@ -0,0 +1,147 @@ +# Ревью решения (образец для студентов): `bookings-db` → `stg` в Greenplum + +Этот документ фиксирует рекомендации по улучшению учебного решения ETL (Airflow + Greenplum + PXF) +на основе ревью изменений ветки `chore/bookings-etl` (добавление полного STG слоя и пайплайна загрузки). + +Цель ревью — сделать решение **безоговорочно рекомендуемым** к изучению начинающими: +понятным, предсказуемым, с корректной терминологией и честными инженерными компромиссами. + +--- + +## 1) Сильные стороны решения (что уже хорошо и стоит сохранить) + +1. **Единый “шаблон” по таблицам** в `sql/stg/`: + - `{table}_ddl.sql` — создаёт `*_ext` и внутреннюю таблицу; + - `{table}_load.sql` — загружает данные; + - `{table}_dq.sql` — валидирует качество и останавливает пайплайн при проблемах. + + Это отличная учебная структура: студент быстро понимает, “где что лежит” и как добавлять новые таблицы. + +2. **DAG как оркестратор, SQL как логика**: + - `airflow/dags/bookings_to_gp_stage.py` и `airflow/dags/bookings_stg_ddl.py` используют `PostgresOperator` + и читают SQL с диска через `template_searchpath="/sql"`. + Это соответствует “канонической” модели: Airflow управляет шагами, а трансформации живут в SQL. + +3. **Понятные сообщения при падении DQ** (в большинстве скриптов): студенту легче дебажить. + +--- + +## 2) Критичные замечания (исправить перед тем, как показывать как эталон) + +### 2.1. Некорректные утверждения про MPP и co-location (вводят студентов в заблуждение) + +В Greenplum производительность JOIN сильно зависит от распределения данных по сегментам. +Если ключ распределения двух таблиц совпадает с ключом JOIN — часто удаётся обойтись без перераспределения данных (motion). + +Проблема: в некоторых DDL-комментариях сейчас обещается co-location там, где его не будет. +Это педагогически опасно: студент запоминает неверную модель, а потом “не понимает”, почему запросы медленные. + +Примеры мест, которые стоит скорректировать: +- `sql/stg/flights_ddl.sql`: `stg.flights` распределена по `flight_id`, а `stg.boarding_passes` — по `ticket_no`, + поэтому “co-location flights и boarding_passes при JOIN по flight_id” не выполняется. +- `sql/stg/routes_ddl.sql`: распределение `stg.routes` по `route_no` не даёт co-location с `stg.airports` (которая по `airport_code`) + и `stg.airplanes` (которая по `airplane_code`) при типичных JOIN’ах. + +Рекомендация: либо исправить распределение (если это действительно важно для учебного кейса), +либо **честно переписать комментарии**: “ключ выбран так-то, но JOIN по другим ключам может требовать motion”. + +### 2.2. DQ-проверки ссылочной целостности иногда “смотрят в историю”, а не в текущий батч + +Часть DQ-скриптов проверяет наличие “родительских” записей в таблице **без фильтра `batch_id`**. +При append-only истории это может скрыть проблемы текущей загрузки: +родитель был загружен в прошлом батче → проверка пройдёт, даже если текущий батч родителя не загрузил. + +Пример: +- `sql/stg/routes_dq.sql` проверяет airports/airplanes без ограничения на `batch_id`. + +Рекомендация: для snapshot-таблиц (справочники и boarding_passes) использовать батч-строгую проверку: +“в текущем `batch_id` все ссылки указывают на строки текущего `batch_id`”. +Это лучше учит идее “консистентность батча” и упрощает отладку. + +### 2.3. Smoke-тесты DAG’ов есть, но почти не проверяют граф + +В `tests/test_dags_smoke.py` новые тесты в основном проверяют “таски существуют” через `dag.has_task(...)`. +Как учебный пример теста это слабовато: студент видит тест, но не понимает, что именно он защищает. + +Рекомендация: тестировать зависимости так же, как это уже сделано для `csv_to_greenplum` +(через `dag.get_task(...).get_direct_relatives("downstream")`). + +### 2.4. Документация по DAG отстаёт от реальной логики + +`docs/bookings_to_gp_stage.md` описывает только загрузку `bookings` и `tickets`, +но DAG теперь загружает ещё 7 таблиц (справочники и транзакции). + +Рекомендация: обновить документ, чтобы студент мог запустить пайплайн “по инструкции” без сюрпризов. + +--- + +## 3) Рекомендации по качеству и читаемости (Clean Code для SQL и DAG) + +### 3.1. “Empty window” в инкременте: договориться о политике (fail vs skip) + +Сейчас поведение разное: +- `bookings_dq.sql` / `tickets_dq.sql` / `flights_dq.sql` / `segments_dq.sql` падают, если в окне инкремента 0 строк; +- `boarding_passes_dq.sql` делает `RAISE NOTICE` и `RETURN`. + +Обе стратегии допустимы, но в учебном решении важно выбрать одну и объяснить: +- **Fail** полезен, когда “ожидаем данные в каждом запуске” (например, учебный генератор должен добавлять день); +- **Skip** полезен, когда “окно может быть пустым и это нормально”. + +Рекомендация: зафиксировать политику в документации и сделать поведение единообразным по группам таблиц. + +### 3.2. Комментарии в `*_load.sql`: точнее формулировать “идемпотентность”, а не “дедупликацию источника” + +Типовой паттерн: +```sql +WHERE NOT EXISTS ( + SELECT 1 FROM stg.table WHERE batch_id = '{{ run_id }}' AND key = ext.key +); +``` + +Это в первую очередь защита от повторного запуска того же таска в рамках одного `batch_id` (retry), +а не “лечение” дублей в источнике. + +Рекомендация: заменить формулировку комментариев на “идемпотентность для текущего батча”. + +### 3.3. Проверка составных ключей: избегать склейки строк + +Паттерн вида `COUNT(DISTINCT col1 || '|' || col2)` теоретически может давать коллизии (если в данных встречается разделитель). +В учебном стенде риск небольшой, но как “эталон” лучше показывать более безопасный подход: +- если поддерживается: `COUNT(DISTINCT (col1, col2))`; +- либо использовать стабильную сериализацию, где коллизии исключены (например, `md5(...)` от безопасной структуры). + +--- + +## 4) Практические примеры “как улучшить” + +### 4.1. Батч-строгая ссылочная целостность (пример подхода) + +Если таблицы грузятся как snapshot в рамках батча, проверки можно сделать батч-строгими: +“в текущем батче ссылки указывают на строки текущего батча”. + +Идея (пример для routes → airports): +```sql +LEFT JOIN stg.airports AS a + ON r.departure_airport = a.airport_code + AND a.batch_id = v_batch_id +``` + +### 4.2. Smoke-тест реального графа (минимальный полезный уровень) + +Вместо “таски существуют” лучше проверять ключевые зависимости: +```python +tickets_dq = dag.get_task("check_tickets_dq") +airports_load = dag.get_task("load_airports_to_stg") +assert airports_load in tickets_dq.get_direct_relatives("downstream") +``` + +--- + +## 5) Чек-лист “готово как эталон” + +- [ ] В DDL-комментариях нет неверных обещаний про co-location/уникальность ключей. +- [ ] Для DQ определена и описана политика “0 строк”: где fail, где skip. +- [ ] DQ ссылочной целостности не маскирует проблемы текущего батча (batch-строгие проверки там, где это уместно). +- [ ] `docs/bookings_to_gp_stage.md` соответствует фактическому DAG. +- [ ] Smoke-тесты проверяют хотя бы критические зависимости графа. + diff --git a/docs/internal/db_schema.md b/docs/internal/db_schema.md index 9d3693b..614f7c9 100644 --- a/docs/internal/db_schema.md +++ b/docs/internal/db_schema.md @@ -402,6 +402,7 @@ graph LR ## Связанные документы - [`docs/internal/bookings_stg_design.md`](bookings_stg_design.md) — Детальный дизайн STG слоя для bookings +- [`docs/internal/bookings_stg_code_review.md`](bookings_stg_code_review.md) — Ревью решения и рекомендации по улучшению - [`docs/internal/bookings_tz.md`](bookings_tz.md) — Работа с часовыми поясами в источнике - [`docs/internal/pxf_bookings.md`](pxf_bookings.md) — Настройка PXF для чтения из bookings-db - [`TESTING.md`](../../TESTING.md) — Пошаговый чек-лист для тестирования стенда