Исправлены критические замечания
This commit is contained in:
@@ -26,52 +26,46 @@
|
||||
|
||||
---
|
||||
|
||||
## 2) Критичные замечания (исправить перед тем, как показывать как эталон)
|
||||
## 2) Критичные замечания (статус на текущий момент)
|
||||
|
||||
### 2.1. Некорректные утверждения про MPP и co-location (вводят студентов в заблуждение)
|
||||
### 2.1. Некорректные утверждения про MPP и co-location (статус: исправлено)
|
||||
|
||||
В Greenplum производительность JOIN сильно зависит от распределения данных по сегментам.
|
||||
Если ключ распределения двух таблиц совпадает с ключом JOIN — часто удаётся обойтись без перераспределения данных (motion).
|
||||
|
||||
Проблема: в некоторых DDL-комментариях сейчас обещается co-location там, где его не будет.
|
||||
Раньше в некоторых 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’ах.
|
||||
Что сделано:
|
||||
- DDL-комментарии приведены к честной формулировке “ключ выбран так-то, но JOIN по другим ключам может требовать motion”.
|
||||
- Исправлены места, где co-location заявлялась ошибочно (в т.ч. `routes`, `flights`, `segments`, `boarding_passes`).
|
||||
|
||||
Рекомендация: либо исправить распределение (если это действительно важно для учебного кейса),
|
||||
либо **честно переписать комментарии**: “ключ выбран так-то, но JOIN по другим ключам может требовать motion”.
|
||||
Файлы: `sql/stg/routes_ddl.sql`, `sql/stg/flights_ddl.sql`, `sql/stg/segments_ddl.sql`, `sql/stg/boarding_passes_ddl.sql`.
|
||||
|
||||
### 2.2. DQ-проверки ссылочной целостности иногда “смотрят в историю”, а не в текущий батч
|
||||
### 2.2. DQ-проверки ссылочной целостности: “текущий батч” vs “вся история” (статус: зафиксировано и частично усилено)
|
||||
|
||||
Часть DQ-скриптов проверяет наличие “родительских” записей в таблице **без фильтра `batch_id`**.
|
||||
При append-only истории это может скрыть проблемы текущей загрузки:
|
||||
родитель был загружен в прошлом батче → проверка пройдёт, даже если текущий батч родителя не загрузил.
|
||||
|
||||
Пример:
|
||||
- `sql/stg/routes_dq.sql` проверяет airports/airplanes без ограничения на `batch_id`.
|
||||
Что сделано для справочников (snapshot), которые загружаются каждый запуск:
|
||||
- `routes_dq.sql`: проверка airports/airplanes стала батч-строгой (`batch_id = текущий батч`).
|
||||
- `seats_dq.sql`: проверка airplanes стала батч-строгой (`batch_id = текущий батч`).
|
||||
- `flights_dq.sql`: проверка routes стала батч-строгой (`batch_id = текущий батч`).
|
||||
|
||||
Рекомендация: для snapshot-таблиц (справочники и boarding_passes) использовать батч-строгую проверку:
|
||||
“в текущем `batch_id` все ссылки указывают на строки текущего `batch_id`”.
|
||||
Это лучше учит идее “консистентность батча” и упрощает отладку.
|
||||
Почему не всё делаем батч-строго:
|
||||
- Для инкрементальных таблиц (например, `segments`) ссылки могут указывать на данные,
|
||||
загруженные в предыдущих батчах → там корректнее проверять “существует в STG вообще”, а не “существует в текущем батче”.
|
||||
|
||||
### 2.3. Smoke-тесты DAG’ов есть, но почти не проверяют граф
|
||||
### 2.3. Smoke-тесты DAG’ов (статус: исправлено)
|
||||
|
||||
В `tests/test_dags_smoke.py` новые тесты в основном проверяют “таски существуют” через `dag.has_task(...)`.
|
||||
Как учебный пример теста это слабовато: студент видит тест, но не понимает, что именно он защищает.
|
||||
Что сделано:
|
||||
- Тесты усилены: теперь проверяются ключевые зависимости графа через `get_direct_relatives("downstream")`.
|
||||
|
||||
Рекомендация: тестировать зависимости так же, как это уже сделано для `csv_to_greenplum`
|
||||
(через `dag.get_task(...).get_direct_relatives("downstream")`).
|
||||
### 2.4. Документация по DAG (статус: синхронизировано базово)
|
||||
|
||||
### 2.4. Документация по DAG отстаёт от реальной логики
|
||||
|
||||
`docs/bookings_to_gp_stage.md` описывает только загрузку `bookings` и `tickets`,
|
||||
но DAG теперь загружает ещё 7 таблиц (справочники и транзакции).
|
||||
|
||||
Рекомендация: обновить документ, чтобы студент мог запустить пайплайн “по инструкции” без сюрпризов.
|
||||
Что сделано:
|
||||
- `docs/bookings_to_gp_stage.md` обновлён так, чтобы отражать текущий набор таблиц и шагов пайплайна.
|
||||
|
||||
---
|
||||
|
||||
@@ -139,9 +133,8 @@ assert airports_load in tickets_dq.get_direct_relatives("downstream")
|
||||
|
||||
## 5) Чек-лист “готово как эталон”
|
||||
|
||||
- [ ] В DDL-комментариях нет неверных обещаний про co-location/уникальность ключей.
|
||||
- [x] В DDL-комментариях нет неверных обещаний про co-location/уникальность ключей.
|
||||
- [ ] Для DQ определена и описана политика “0 строк”: где fail, где skip.
|
||||
- [ ] DQ ссылочной целостности не маскирует проблемы текущего батча (batch-строгие проверки там, где это уместно).
|
||||
- [ ] `docs/bookings_to_gp_stage.md` соответствует фактическому DAG.
|
||||
- [ ] Smoke-тесты проверяют хотя бы критические зависимости графа.
|
||||
|
||||
- [x] DQ ссылочной целостности не маскирует проблемы текущего батча (batch-строгие проверки там, где это уместно).
|
||||
- [x] `docs/bookings_to_gp_stage.md` соответствует фактическому DAG.
|
||||
- [x] Smoke-тесты проверяют хотя бы критические зависимости графа.
|
||||
|
||||
Reference in New Issue
Block a user