Priya Nair·
Refactored a 400-line dbt model into staged CTEs with tests using this review prompt in Cursor. Runtime dropped 60% and the logic is finally readable.
Reviews a dbt SQL model for structure, performance, and testability, then proposes a staged refactor.
dbt Model Refactor Review
You are a senior analytics engineer reviewing a dbt model. I will paste the model SQL and its schema.yml entry (if one exists).
Context:
- Warehouse: {{warehouse}}
- Model materialization: {{materialization}}
- Approximate source table sizes: {{table_sizes}}
- Downstream consumers: {{consumers}}
Review in this order:
1. **Readability audit.** Identify nested subqueries that should be CTEs, CTEs doing more than one job, and column logic duplicated in multiple places. Propose a staging structure: which logic belongs in a staging model, which in intermediate models, and what remains in this mart model. Follow the one-model-one-purpose rule.
2. **Performance review.** Find the expensive patterns: joins before filters, SELECT * pulled through the chain, window functions over unpartitioned data, and repeated scans of the same large source. For each, estimate relative impact (high/medium/low) and show the fix as a SQL snippet.
3. **Correctness risks.** Look for silent fan-out from joins (duplicate key risk), timezone handling in date truncation, and NULL handling in aggregates or COALESCE chains that could hide bad data.
4. **Test plan.** Propose the schema.yml tests this model should have: unique/not_null on the primary key, accepted_values where enums exist, relationships to upstream models, and one custom data test for the trickiest business rule you found in the SQL.
5. **Refactor sequence.** Give me the order of changes as small, independently shippable steps so I never break downstream consumers mid-refactor.
Do not rewrite the entire model in one blob — I want the staged plan.
Model SQL:
{{model_sql}}
Ergebnisse
## Review: `fct_subscription_revenue`
### 1. Readability Audit
Three structural issues:
- The 90-line nested subquery aliased `x` computes plan changes — this is a full intermediate model. Extract to `int_subscription_plan_changes`.
- `cte_base` both deduplicates events AND applies business filters. Split: dedup belongs in `stg_billing__events`, filters stay here.
- Currency conversion logic appears **three times** with slightly different rounding. Extract once into `int_revenue_converted`; the inconsistent rounding is almost certainly a live bug (see §3).
**Proposed structure:** `stg_billing__events` → `int_subscription_plan_changes` + `int_revenue_converted` → `fct_subscription_revenue`.
### 2. Performance Review
| Pattern | Impact | Fix |
|---|---|---|
| `events` (2B rows) scanned twice for MRR and churn legs | **High** | Single scan into a CTE, branch after |
| `WHERE event_date >= '2023-01-01'` applied AFTER the join | **High** | Push the filter into the staging CTE before joining |
| `SELECT *` from `stg_customers` (80 cols, 6 used) | **Medium** | Explicit column list |
| Unpartitioned `row_number()` over full event set | **Medium** | `PARTITION BY subscription_id` — also fixes a correctness issue |
Expected effect: source bytes scanned roughly halve; the double scan is your 60% runtime.
### 3. Correctness Risks
- 🚨 **Join fan-out:** `plan_history` join is on `subscription_id` alone but the table has one row per (subscription_id, valid_from). Revenue rows duplicate for any subscription with >1 plan change. Add the date-range condition.
- Inconsistent rounding (2 vs 4 decimals) across the three conversion copies → cents-level drift vs. finance reports.
- `date_trunc('month', event_at)` on a timestamptz — confirm warehouse TZ is UTC or truncate on a converted column.
### 4. Test Plan
```yaml
columns:
- name: revenue_row_id
tests: [unique, not_null]
- name: plan_tier
tests:
- accepted_values: {values: ['free','pro','enterprise']}
tests:
- dbt_utils.expression_is_true:
expression: "mrr_usd >= 0 or event_type = 'refund'"
```
Plus `relationships` from `subscription_id` to `int_subscription_plan_changes`.
### 5. Refactor Sequence
1. Add the fan-out join fix + unique test (bug fix, ship alone)
2. Extract staging dedup
3. Extract `int_revenue_converted`, unify rounding, reconcile vs. finance before merging
4. Extract plan-changes intermediate
5. Column pruning + filter pushdown (pure perf, last)
Modell: Cursor
31 Likes19 SavesScore: 22
2 Kommentare
Luca Brunner·
Point 3 on join fan-out — I have seen that exact bug ship to a board deck. The date-range join condition should be a lint rule.
Ahmed Hassan·
The independently-shippable refactor sequence is the part most reviews skip. Nice.