airflow-greenplum/docs/internal/bookings_stg_code_review.md

Ревью решения (образец для студентов): 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-скриптов проверяет наличие “родительских” записей в таблице без фильтра batch_id. При append-only истории это может скрыть проблемы текущей загрузки: родитель был загружен в прошлом батче → проверка пройдёт, даже если текущий батч родителя не загрузил.

Что сделано для справочников (snapshot), которые загружаются каждый запуск:

• routes_dq.sql: проверка airports/airplanes стала батч-строгой (batch_id = текущий батч).
• seats_dq.sql: проверка airplanes стала батч-строгой (batch_id = текущий батч).
• flights_dq.sql: проверка routes стала батч-строгой (batch_id = текущий батч).

Почему не всё делаем батч-строго:

• Для инкрементальных таблиц (например, segments) ссылки могут указывать на данные, загруженные в предыдущих батчах → там корректнее проверять “существует в STG вообще”, а не “существует в текущем батче”.

2.3. Smoke-тесты DAG’ов (статус: исправлено)

Что сделано:

• Тесты усилены: теперь проверяются ключевые зависимости графа через get_direct_relatives("downstream").

2.4. Документация по DAG (статус: синхронизировано базово)

Что сделано:

• docs/bookings_to_gp_stage.md обновлён так, чтобы отражать текущий набор таблиц и шагов пайплайна.

---

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: точнее формулировать “идемпотентность”, а не “дедупликацию источника”

Типовой паттерн:

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):

LEFT JOIN stg.airports AS a
  ON r.departure_airport = a.airport_code
 AND a.batch_id = v_batch_id

4.2. Smoke-тест реального графа (минимальный полезный уровень)

Вместо “таски существуют” лучше проверять ключевые зависимости:

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-тесты проверяют хотя бы критические зависимости графа.