Pull Requests And Code Reviews

Why this matters

In Analytics Engineering, pull requests (PRs) and code reviews protect data quality, reduce incidents, and spread knowledge across the team. You use PRs to propose changes to SQL models, dbt projects, metrics logic, documentation, and orchestration files. Reviews catch breaking changes early, verify performance, and ensure naming, testing, and lineage are consistent.

• Real task: propose a new dim_customers model and get feedback before it reaches production.
• Real task: review a teammate’s PR to ensure joins won’t duplicate rows and tests cover edge cases.
• Real task: fix a failing CI check by updating schema tests and rerunning pipelines before merge.

Who this is for

• Analytics Engineers working with Git-based workflows (dbt, SQL, Python transformations).
• BI Developers and Data Analysts contributing to shared analytics repositories.
• Data Engineers collaborating on warehouse models and tests.

Prerequisites

• Basic Git: clone, branch, commit, push, pull.
• Comfort with SQL and your analytics stack (e.g., dbt, warehouse).
• Understanding of your team’s branching and deployment strategy.

Concept explained simply

A pull request is a proposal to merge your branch into a target branch (usually main). It packages your changes with context so others can review, run checks, and decide when it’s safe to merge.

git checkout -b feat/dim-customers-normalized
# make changes
git add .
git commit -m "feat(dbt): add normalized email to dim_customers and tests"
git push -u origin feat/dim-customers-normalized
# open a PR on your platform (Draft if not ready)

Worked examples

Example 1: Authoring a PR for a dbt model change

1. Create a small branch: one logical change only.
2. Write good commits: imperative, scoped messages.
3. Open a PR with a template covering context, impact, test plan, risks, and rollout.
4. Mark as Draft if CI or validation isn’t done yet.
5. Address feedback promptly; push updates; re-request review.
6. Squash-merge after approvals and green checks; delete the branch.

Title: feat(dbt): normalize customer emails in dim_customers

Context
- Adds email_normalized using lower(trim(email)) to improve deduping.
- Updates schema tests: unique + not_null on customer_id and email_normalized.

Impact
- Affects downstream Customer 360 dashboard metrics that group by email.
- Expect minor shift in counts due to normalization; documented below.

Test plan
- Ran `dbt build -s dim_customers` locally.
- Compared row counts before/after: delta +0.2% (expected from dedupe).
- Verified no dupe groups by customer_id/email_normalized.

Performance
- Added index suggestion in model config (warehouse-dependent).

Risks & rollback
- If unexpected drop in counts >1%, revert via squash commit rollback.

Release notes
- New column: email_normalized (string). Update queries to use it.

Example 2: Reviewing SQL changes

1. Scan the diff for SELECT *, cartesian joins, and non-deterministic logic.
2. Check tests: unique, not_null, and relationship tests updated?
3. Validate performance: filters pushed down? unnecessary CTE expansion?
4. Think lineage: will this break downstream dashboards or contracts?
5. Leave specific, actionable comments with suggestions.

Example 3: Fixing failing checks and conflicts

1. Pull latest main and rebase/merge to resolve conflicts.
2. Run lint/tests locally. Fix style and schema test failures.
3. Push updates; ensure CI passes and summarize fixes in a short comment.

Author checklist

• Is the PR focused on one logical change and under ~300 lines of diff?
• Clear title and description: context, impact, test plan, risks, rollback.
• Tests updated: not_null, unique, relationships, freshness (if relevant).
• No SELECT *; explicit columns and stable schema.
• Performance considered: filters, joins, incremental strategy as needed.
• Documentation updated (model docs, column descriptions).

Reviewer checklist

• Does the change preserve correctness and avoid duplication?
• Are edge cases handled (nulls, late-arriving data, time zones)?
• Do tests cover the new logic and guard against regressions?
• Is the change minimal and readable? Any dead code or unused columns?
• Any data contract impacts communicated (column add/remove/rename)?
• CI green and local validation steps reproduced when risky?

Exercises

Do these before the quick test. Tip: the quick test at the bottom is available to everyone; log in to save your progress.

--- a/models/stg_orders.sql
+++ b/models/stg_orders.sql
@@
- select *
- from raw.orders o
- left join raw.customers c on o.customer_id = c.customer_id
- where order_date >= current_date - 90
+ select
+   o.order_id,
+   o.customer_id,
+   o.amount,
+   o.order_date,
+   c.email,
+   c.signup_date
+ from raw.orders o
+ left join raw.customers c on o.customer_id = c.customer_id and c.is_active = true
+ where date(o.order_date) >= current_date - 90

Common mistakes and self-check

• Oversized PRs: hard to review and risky to merge. Self-check: can you split it into smaller, independent PRs?
• Poor descriptions: reviewers lack context. Self-check: would a new teammate understand the why, what, and how to validate?
• No tests updated: logic changes without guards. Self-check: list tests that should fail if a regression occurs.
• SELECT * and unstable schemas: downstream breakages. Self-check: enumerate exact output columns.
• Ignoring performance: slow joins or missing filters. Self-check: estimate row counts and filter selectivity.
• Approving without running risky checks locally: fragile merges. Self-check: run the critical model and compare key metrics.

Practical projects

• Set up a PR template with sections: Context, Impact, Test Plan, Risks, Rollback, Release Notes.
• Create CODEOWNERS or review rules for critical models to enforce expert reviews.
• Add CI steps to run dbt build on changed models and surface test results in the PR.
• Simulate an incident: introduce a failing test in a branch, catch it via PR checks, and fix before merge.

Learning path

1. Branching and committing effectively.
2. Creating PRs with strong descriptions and small scope.
3. Code review heuristics for analytics (joins, tests, performance, lineage).
4. CI checks for dbt/SQL and reading test outputs.
5. Merge strategies and rollback tactics.
6. Advanced: review ownership, templates, and automated validations.

Mini challenge

Open a Draft PR for an in-progress model and ask one targeted question in the description (e.g., “Is this join key correct for late-arriving updates?”). Convert to Ready for review only after tests pass.

Next steps

• Integrate data quality checks into PR CI for changed models.
• Adopt squash merges for clean history and easier rollbacks.
• Run lightweight release notes from merged PRs to inform BI stakeholders.

Quick Test

This test is available to everyone. Log in to save your progress and see your history.