docs(all): реструктурирована документация — docs/internal/ заменён на design/, reference/, archive/, plans/
- Зачем: - docs/internal/ превратился в свалку: дизайн-документы, ревью, планы и справочники лежали вперемешку. - архивные планы были неотличимы от живых документов. - Что: - docs/internal/ удалён; файлы распределены по docs/design/, docs/reference/, docs/archive/, docs/plans/, docs/assignment/. - educational-tasks.md убран из корня в архив (устарел). - обновлены все перекрёстные ссылки в AGENTS.md, TODO.md, README.md, docs/README.md и внутри design/reference/. - актуализированы architecture_review.md (статус DM-слоя), db_schema.md (DM-слой), TESTING.md, dag_execution_order.md, pxf_bookings.md. - добавлены заглушки docs/assignment/README.md и docs/plans/README.md. - Проверка: - make test && make lint - rg 'docs/internal' --glob '!docs/archive/*' — должно быть пусто.
This commit is contained in:
@@ -0,0 +1,149 @@
|
||||
# Ревью решения (образец для студентов): `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 там, где её не будет.
|
||||
Это педагогически опасно: студент запоминает неверную модель, а потом “не понимает”, почему запросы медленные.
|
||||
|
||||
Что сделано:
|
||||
- DDL-комментарии приведены к честной формулировке “ключ выбран так-то, но JOIN по другим ключам может требовать motion”.
|
||||
- Исправлены места, где co-location заявлялась ошибочно (в т.ч. `routes`, `flights`, `segments`, `boarding_passes`).
|
||||
|
||||
Файлы: `sql/stg/routes_ddl.sql`, `sql/stg/flights_ddl.sql`, `sql/stg/segments_ddl.sql`, `sql/stg/boarding_passes_ddl.sql`.
|
||||
|
||||
### 2.2. DQ-проверки ссылочной целостности: “текущий батч” vs “вся история” (статус: исправлено)
|
||||
|
||||
Часть DQ-скриптов проверяет наличие “родительских” записей в таблице **без фильтра `_load_id`**.
|
||||
При append-only истории это может скрыть проблемы текущей загрузки:
|
||||
родитель был загружен в прошлом батче → проверка пройдёт, даже если текущий батч родителя не загрузил.
|
||||
|
||||
Что сделано:
|
||||
- `routes_dq.sql`: проверка airports/airplanes стала батч-строгой (`_load_id = текущий батч`).
|
||||
- `seats_dq.sql`: проверка airplanes стала батч-строгой (`_load_id = текущий батч`).
|
||||
- `flights_dq.sql`: проверка routes стала батч-строгой (`_load_id = текущий батч`).
|
||||
|
||||
Примечание (почему не везде `_load_id = текущий батч`):
|
||||
- Если дочерняя таблица грузится инкрементом, то ссылки могут указывать на “исторические” записи,
|
||||
загруженные в предыдущих батчах → для таких связей корректнее проверять “существует в STG вообще”.
|
||||
- Для `boarding_passes` (full snapshot) ссылки на `tickets/segments` также проверяются по STG-истории,
|
||||
потому что `tickets/segments` не перезагружаются полным снэпшотом каждый запуск.
|
||||
|
||||
### 2.3. Smoke-тесты DAG’ов (статус: исправлено)
|
||||
|
||||
Что сделано:
|
||||
- Тесты усилены: теперь проверяются ключевые зависимости графа через `get_direct_relatives(upstream=False)` и “барьеры” через `get_flat_relatives(upstream=False)`.
|
||||
|
||||
### 2.4. Документация по DAG (статус: синхронизировано)
|
||||
|
||||
Что сделано:
|
||||
- `docs/bookings_to_gp_stage.md` обновлён так, чтобы отражать текущий набор таблиц и шагов пайплайна.
|
||||
|
||||
---
|
||||
|
||||
## 3) Рекомендации по качеству и читаемости (Clean Code для SQL и DAG)
|
||||
|
||||
### 3.1. “Empty window” в инкременте: договориться о политике (fail vs skip) (статус: исправлено)
|
||||
|
||||
Раньше поведение было разным:
|
||||
- часть DQ-скриптов падала, если в окне инкремента 0 строк;
|
||||
- `boarding_passes_dq.sql` делал `RAISE NOTICE` и `RETURN`.
|
||||
|
||||
Обе стратегии допустимы, но в учебном решении важно выбрать одну и объяснить:
|
||||
- **Fail** полезен, когда “ожидаем данные в каждом запуске” (например, учебный генератор должен добавлять день);
|
||||
- **Skip** полезен, когда “окно может быть пустым и это нормально”.
|
||||
|
||||
Выбранная политика для учебного стенда:
|
||||
- Для инкрементальных таблиц (`bookings`, `tickets`, `flights`, `segments`) “пустое окно” **допустимо**:
|
||||
DQ логирует `NOTICE` и завершает проверку, не падая.
|
||||
- Для snapshot-справочников (`airports`, `airplanes`, `routes`, `seats`) пустой источник считаем ошибкой:
|
||||
это почти всегда признак проблем с PXF/источником.
|
||||
|
||||
### 3.2. Комментарии в `*_load.sql`: точнее формулировать “идемпотентность”, а не “дедупликацию источника” (статус: исправлено)
|
||||
|
||||
Типовой паттерн:
|
||||
```sql
|
||||
WHERE NOT EXISTS (
|
||||
SELECT 1 FROM stg.table WHERE _load_id = '{{ run_id }}' AND key = ext.key
|
||||
);
|
||||
```
|
||||
|
||||
Это в первую очередь защита от повторного запуска того же таска в рамках одного `_load_id` (retry),
|
||||
а не “лечение” дублей в источнике.
|
||||
|
||||
Что сделано:
|
||||
- В `sql/stg/*_load.sql` комментарии приведены к формулировке “идемпотентность при повторном запуске/ретрае”.
|
||||
|
||||
### 3.3. Проверка составных ключей: избегать склейки строк (статус: исправлено)
|
||||
|
||||
Паттерн вида `COUNT(DISTINCT col1 || '|' || col2)` теоретически может давать коллизии (если в данных встречается разделитель).
|
||||
В учебном стенде риск небольшой, но как “эталон” лучше показывать более безопасный подход.
|
||||
|
||||
Что сделано:
|
||||
- Заменили склейку строк на `COUNT(DISTINCT md5(ROW(col1, col2)::text))` в DQ‑скриптах для составных ключей.
|
||||
Такой подход сохраняет DV‑стиль и убирает неоднозначность разделителей.
|
||||
|
||||
---
|
||||
|
||||
## 4) Практические примеры “как улучшить”
|
||||
|
||||
### 4.1. Батч-строгая ссылочная целостность (пример подхода)
|
||||
|
||||
Если таблицы грузятся как snapshot в рамках батча, проверки можно сделать батч-строгими:
|
||||
“в текущем батче ссылки указывают на строки текущего батча”.
|
||||
|
||||
Идея (пример для routes → airports):
|
||||
```sql
|
||||
LEFT JOIN stg.airports AS a
|
||||
ON r.departure_airport = a.airport_code
|
||||
AND a._load_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(upstream=False)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 5) Чек-лист “готово как эталон”
|
||||
|
||||
- [x] В DDL-комментариях нет неверных обещаний про co-location/уникальность ключей.
|
||||
- [x] Для DQ определена и описана политика “0 строк”: где fail, где skip.
|
||||
- [x] DQ ссылочной целостности не маскирует проблемы текущего батча (batch-строгие проверки там, где это уместно).
|
||||
- [x] `docs/bookings_to_gp_stage.md` соответствует фактическому DAG.
|
||||
- [x] Smoke-тесты проверяют хотя бы критические зависимости графа.
|
||||
Reference in New Issue
Block a user