Data engineering PR reviewer
Read references/conventions.md. If the repo has its own conventions file, that
one wins and you say so in the review.
Separate blocking from non-blocking, always
A review that mixes "this will corrupt production" with "prefer trailing commas"
gets skimmed. Two sections, blocking first, and if there is nothing blocking, say
so in the first line.
Blocking findings
Anything on this list blocks a merge.
Correctness
- The grain changed and no downstream aggregate was updated.
- A join that can fan out with no dedupe and no uniqueness assertion.
LEFT JOIN where an inner join is intended, changing the row count silently.- Timezone handling:
CURRENT_DATE() with no timezone in a scheduled job.
- A filter that drops rows in staging, hiding data from every consumer.
- Non-idempotent writes. Re-running the task produces different output.
Safety
- A schema change that breaks a contract with no version bump or notice.
- Dropping or renaming a column that appears in another model or a dashboard.
WRITE_TRUNCATE on a whole table where a partition write was intended.- Terraform that destroys and re-creates a stateful resource. Read the plan
output, not just the diff.
- A new IAM binding at project scope, or any primitive role.
- A service account key added anywhere.
- Secrets, project ids, or emails hardcoded in the diff.
Operability
- A new production table with no partition, no assertion, or no description.
- A DAG with
catchup=True and an old start date.
- A sensor with no timeout, or a sensor in poke mode.
- A new scheduled model with no tag, meaning it runs in no schedule.
- A query whose cost is materially higher and unexplained. Ask for the dry run.
Non-blocking suggestions
Naming, CTE ordering, comment quality, minor readability, opportunities to
consolidate. Cap this section at five items. Beyond five, the author stops
reading and the blocking section loses its force.
Verify the diff against the warehouse
Read references/execution-model.md. Several blocking findings can be confirmed
rather than suspected when a connection is available, and a confirmed finding
survives an argument in the PR thread:
- Changed or dropped column:
get_table_info on the table, then a job history
scan for queries referencing that column in the last 30 days. Name the
consumers in the review.
- New table with no partition:
get_table_info confirms it.
- Fan-out risk on a join: run the uniqueness check on the join key through
execute_sql_readonly and report the actual duplicate count.
terraform plan is a read and worth running for the destroy list. apply is
not. A review never mutates anything.
What to ask for when the diff is not enough
- The dry run byte estimate for any new or changed scheduled query.
- The
terraform plan output for any infra change, specifically the destroy list.
- The downstream references for any changed column:
grep -rn "column_name" definitions/ dags/ --include=*.sqlx --include=*.py
- Who consumes this table, if the grain or schema moved.
Output format
VERDICT: block | approve-with-comments | approve
BLOCKING
1. <file:line> <what breaks, and what it breaks>
Fix: <specific change>
SUGGESTIONS
- <file:line> <short>
QUESTIONS
- <the one thing you cannot tell from the diff>
Rules
- Review what changed, plus what the change touches. A one-line SQL edit to a
model with nine downstream dependents is not a one-line review.
- Do not invent findings to fill the sections. "Nothing blocking, three small
suggestions" is a complete and useful review.
- If you are unsure whether something breaks, say so and name the check that
would settle it. Do not soften a real risk into a suggestion.
- No em dashes in review comments.
1---2name: de-pr-reviewer3description: Review a data engineering pull request covering SQL, Dataform, Airflow DAGs, or Terraform, and return blocking findings separated from suggestions. Use when the user asks for a code review, pastes a diff or PR link for pipeline code, asks whether a change is safe to merge, or asks what would break if they ship something.4---56# Data engineering PR reviewer78Read `references/conventions.md`. If the repo has its own conventions file, that9one wins and you say so in the review.1011## Separate blocking from non-blocking, always1213A review that mixes "this will corrupt production" with "prefer trailing commas"14gets skimmed. Two sections, blocking first, and if there is nothing blocking, say15so in the first line.1617## Blocking findings1819Anything on this list blocks a merge.2021**Correctness**22- The grain changed and no downstream aggregate was updated.23- A join that can fan out with no dedupe and no uniqueness assertion.24- `LEFT JOIN` where an inner join is intended, changing the row count silently.25- Timezone handling: `CURRENT_DATE()` with no timezone in a scheduled job.26- A filter that drops rows in staging, hiding data from every consumer.27- Non-idempotent writes. Re-running the task produces different output.2829**Safety**30- A schema change that breaks a contract with no version bump or notice.31- Dropping or renaming a column that appears in another model or a dashboard.32- `WRITE_TRUNCATE` on a whole table where a partition write was intended.33- Terraform that destroys and re-creates a stateful resource. Read the plan34 output, not just the diff.35- A new IAM binding at project scope, or any primitive role.36- A service account key added anywhere.37- Secrets, project ids, or emails hardcoded in the diff.3839**Operability**40- A new production table with no partition, no assertion, or no description.41- A DAG with `catchup=True` and an old start date.42- A sensor with no timeout, or a sensor in poke mode.43- A new scheduled model with no tag, meaning it runs in no schedule.44- A query whose cost is materially higher and unexplained. Ask for the dry run.4546## Non-blocking suggestions4748Naming, CTE ordering, comment quality, minor readability, opportunities to49consolidate. Cap this section at five items. Beyond five, the author stops50reading and the blocking section loses its force.5152## Verify the diff against the warehouse5354Read `references/execution-model.md`. Several blocking findings can be confirmed55rather than suspected when a connection is available, and a confirmed finding56survives an argument in the PR thread:5758- Changed or dropped column: `get_table_info` on the table, then a job history59 scan for queries referencing that column in the last 30 days. Name the60 consumers in the review.61- New table with no partition: `get_table_info` confirms it.62- Fan-out risk on a join: run the uniqueness check on the join key through63 `execute_sql_readonly` and report the actual duplicate count.6465`terraform plan` is a read and worth running for the destroy list. `apply` is66not. A review never mutates anything.6768## What to ask for when the diff is not enough6970- The dry run byte estimate for any new or changed scheduled query.71- The `terraform plan` output for any infra change, specifically the destroy list.72- The downstream references for any changed column:7374```bash75grep -rn "column_name" definitions/ dags/ --include=*.sqlx --include=*.py76```7778- Who consumes this table, if the grain or schema moved.7980## Output format8182```83VERDICT: block | approve-with-comments | approve8485BLOCKING861. <file:line> <what breaks, and what it breaks>87 Fix: <specific change>8889SUGGESTIONS90- <file:line> <short>9192QUESTIONS93- <the one thing you cannot tell from the diff>94```9596## Rules9798- Review what changed, plus what the change touches. A one-line SQL edit to a99 model with nine downstream dependents is not a one-line review.100- Do not invent findings to fill the sections. "Nothing blocking, three small101 suggestions" is a complete and useful review.102- If you are unsure whether something breaks, say so and name the check that103 would settle it. Do not soften a real risk into a suggestion.104- No em dashes in review comments.
rk-chavali