Code Review Practices

-- goal: monthly revenue per product (net of refunds)
SELECT
  DATE_TRUNC('month', o.order_date) AS month,
  p.product_name,
  SUM(o.amount) AS revenue
FROM orders o
JOIN products p ON o.product_id = p.id
WHERE o.status = 'completed'
GROUP BY 1,2
ORDER BY 1,2;

WITH revenue AS (
  SELECT DATE_TRUNC('month', order_date AT TIME ZONE 'UTC') AS month,
         product_id,
         SUM(amount) AS gross
  FROM orders
  WHERE status = 'completed'
  GROUP BY 1,2
), refunds AS (
  SELECT DATE_TRUNC('month', refund_date AT TIME ZONE 'UTC') AS month,
         product_id,
         SUM(refund_amount) AS refunded
  FROM order_refunds
  GROUP BY 1,2
)
SELECT r.month,
       p.product_name,
       COALESCE(r.gross,0) - COALESCE(f.refunded,0) AS net_revenue
FROM revenue r
LEFT JOIN refunds f USING (month, product_id)
LEFT JOIN products p ON p.id = r.product_id
ORDER BY 1,2;

{
  "mark": "line",
  "encoding": {
    "x": {"field": "date", "type": "temporal"},
    "y": {"field": "revenue", "type": "quantitative"},
    "color": {"field": "region", "type": "nominal"}
  },
  "data": {"name": "monthly_revenue"}
}

{
  "mark": {"type": "line", "interpolate": "monotone"},
  "encoding": {
    "x": {"field": "date", "type": "temporal", "timeUnit": "yearmonth", "title": "Month"},
    "y": {"field": "revenue", "type": "quantitative", "aggregate": "sum",
           "title": "Revenue (USD)", "axis": {"format": "$~s"}},
    "color": {"field": "region", "type": "nominal", "title": "Region"},
    "tooltip": [
      {"field": "date", "type": "temporal", "timeUnit": "yearmonth"},
      {"field": "region", "type": "nominal"},
      {"field": "revenue", "type": "quantitative", "aggregate": "sum", "format": ".2s"}
    ]
  },
  "data": {"name": "monthly_revenue"}
}

def add_conversion_rate(df):
    # df has clicks and purchases
    df['conversion_rate'] = df['purchases'] / df['clicks']
    return df

def add_conversion_rate(df):
    """Add conversion_rate = purchases / clicks (0 when clicks == 0).
    Expects numeric columns: clicks, purchases."""
    clicks = df['clicks'].fillna(0)
    purchases = df['purchases'].fillna(0)
    df['conversion_rate'] = purchases.div(clicks.replace(0, float('inf'))).fillna(0.0)
    return df

• Read PR title/description; run locally or open preview
• Skim files changed; identify high-risk areas first
• Open the checklist and keep it beside you

• Comment on behavior and outcomes, not just syntax
• Be specific: point to lines, suggest concrete changes
• Batch nits; propose automated lint/pre-commit for repetitive items

• Leave a summary comment: what you checked and remaining risks
• Approve with required changes or request updates
• Celebrate improvements; note follow-ups as tasks

diff --git a/metrics/revenue.sql b/metrics/revenue.sql
--- a/metrics/revenue.sql
+++ b/metrics/revenue.sql
@@
-SELECT DATE_TRUNC('month', order_date) AS month,
-       SUM(amount) AS revenue
-FROM orders
-WHERE status = 'completed'
-GROUP BY 1
+SELECT DATE_TRUNC('month', order_date) AS month,
+       SUM(amount) AS revenue
+FROM orders o
+LEFT JOIN order_refunds r ON r.order_id = o.id
+WHERE o.status = 'completed'
+GROUP BY 1

• Check correctness: refunds joined but never used?
• Timezones and net vs gross definition
• Performance and join type

Example comments:

• (Lines +2,+3) We join refunds but never subtract refund amounts; this still reports gross revenue. Suggest SELECT SUM(o.amount) - COALESCE(SUM(r.refund_amount),0) AS net_revenue.
• (Line +1) Specify timezone in DATE_TRUNC to avoid cross-region drift.
• (Join) LEFT JOIN may duplicate orders if multiple refunds per order; aggregate refunds in a CTE before joining to prevent double-subtracting.
• (Filter) Ensure refunded orders still counted as completed when appropriate; clarify business rule in PR description.
• (Perf) Add predicate on r.refund_date to limit scanned partitions.
• (Validation) Add a check comparing net_revenue <= gross to catch sign issues.
• (Docs) Update metric definition to explicitly say “net of refunds.”

Update revenue stuff and fix the chart. Also changed some names.

Sample improved description:

• Title: Net Revenue metric and monthly trend chart
• Context: Finance requested net revenue (gross - refunds) for monthly reporting.
• Changes: Updated metrics/revenue.sql to subtract refunds (see CTE refunds), adjusted chart spec to show monthly sum by region, added tooltip/axis labels.
• Screenshots: Attached before/after chart images with sample month.
• Risk/Impact: Slight metric decrease vs previous reports; dashboards depending on revenue will update automatically.
• Test plan: Queried last 6 months; verified net_revenue <= gross; compared two random orders manually.
• Rollout/Owner: Merge after approval; I will monitor dashboard refresh next morning.