148 lines
10 KiB
Markdown
148 lines
10 KiB
Markdown
# Ревью решения (образец для студентов): `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-тесты проверяют хотя бы критические зависимости графа.
|
||
|