---
name: sql-review
description: Use when performing a static review of a generated PySpark SQL string before it is executed. Triggers include "review this SQL", "SQL 评审", "静态审查", "risk level", "check join cardinality", "数据膨胀", "重复聚合", "分区裁剪". Consumes a SQL string plus the SQL Context that produced it, and emits a risk-graded review (HIGH / MEDIUM / LOW) with concrete fixes. Does NOT modify the SQL — it reports findings.
---

# SQL Review

## Overview

Static, pre-execution review of a generated PySpark SQL string. The reviewer does **not** rewrite SQL — it emits findings, severity, and suggested fixes. The author of the SQL decides what to act on.

Core principle: **review against the SQL Context, not against vibes.** Every field, alias, and join key referenced in the SQL must exist in the SQL Context that produced it. If it does not, that is itself a `HIGH` finding (the SQL bypassed the contract).

## When to Use

Use when:
- A SQL string has been generated from a `sql_context` and is about to be executed
- The user pastes a SQL string and asks for a review, optimization, or risk assessment
- A code change adds a new `spark.sql(...)` call to a PySpark script

Do NOT use when:
- The SQL is not Spark SQL (e.g. plain Postgres, HiveQL outside Spark) — at minimum replace Spark-specific checks with the relevant engine's equivalents, but flag it
- The SQL is a one-off interactive query that won't be reused — the user can opt out

## Inputs

1. The **SQL string** to review
2. The **SQL Context** that the SQL was generated from (or, if absent, an explicit acknowledgement that the SQL is "untrusted / ad-hoc")

If the SQL Context is missing, raise a single `HIGH` finding: *"SQL has no associated SQL Context; downstream skills cannot guarantee field/join correctness."* Then continue with structural checks.

## Severity Model

| Level | Meaning | Action |
|---|---|---|
| `HIGH` | Will produce wrong numbers, fail to run, or violate policy | Block execution. Must fix. |
| `MEDIUM` | Likely correct, but inefficient, fragile, or risky at scale | Fix before production. OK in dev. |
| `LOW` | Style / best-practice nit | Optional fix. |

Emit findings in this order: `HIGH` → `MEDIUM` → `LOW`. A single `HIGH` finding makes the overall review verdict `FAIL`.

## Workflow

```dot
digraph sql_review {
    "Parse SQL (logical, not just regex)" [shape=box];
    "Cross-check aliases, columns, join keys against SQL Context" [shape=box];
    "Detect multi-statement or forbidden verbs (guardrail)" [shape=box];
    "Analyze join cardinality & inflation risk" [shape=box];
    "Detect repeated aggregation on same measure" [shape=box];
    "Detect full table scan & missing partition filter" [shape=box];
    "Verify GROUP BY matches SELECT" [shape=box];
    "Check null handling on join keys & measures" [shape=box];
    "Check field types vs operations" [shape=box];
    "Aggregate findings into verdict" [shape=box];

    "Parse SQL (logical, not just regex)" -> "Cross-check aliases, columns, join keys against SQL Context";
    "Cross-check aliases, columns, join keys against SQL Context" -> "Detect multi-statement or forbidden verbs (guardrail)";
    "Detect multi-statement or forbidden verbs (guardrail)" -> "Analyze join cardinality & inflation risk";
    "Analyze join cardinality & inflation risk" -> "Detect repeated aggregation on same measure";
    "Detect repeated aggregation on same measure" -> "Detect full table scan & missing partition filter";
    "Detect full table scan & missing partition filter" -> "Verify GROUP BY matches SELECT";
    "Verify GROUP BY matches SELECT" -> "Check null handling on join keys & measures";
    "Check null handling on join keys & measures" -> "Check field types vs operations";
    "Check field types vs operations" -> "Aggregate findings into verdict";
}
```

## Checks (run all)

1. **Reference integrity** — every `<alias>.<column>` in the SQL must exist in the SQL Context. Mismatches are `HIGH`.
2. **Join key match** — every `ON` predicate must match a frozen join from the SQL Context. Invented joins are `HIGH`.
3. **Forbidden verbs / multi-statement** — defer to `pyspark-sql-guardrails`. Any DDL/DML/maintenance verb, or any `;`-stacked statement, is `HIGH`.
4. **Join cardinality risk** — for every join, if the SQL Context marks cardinality as `1:N` or `N:N` and there is no dedup, raise `MEDIUM` (1:N) or `HIGH` (N:N).
5. **Repeated aggregation** — if the same measure is aggregated twice (e.g. once in a subquery, once outside) without intentional re-aggregation, raise `HIGH`.
6. **Partition / time filter** — for any fact table with a known time field, missing `WHERE` on that field is `HIGH` (full scan risk).
7. **GROUP BY completeness** — every selected non-aggregated column must be in `GROUP BY`. Mismatch is `HIGH`.
8. **Null handling** — nullable join keys with `INNER JOIN` silently drop rows (`MEDIUM`); nullable measures aggregated without `COALESCE` can produce `NULL` that breaks downstream `WHERE ... > 0` (`MEDIUM`).
9. **Type mismatch** — string columns used in numeric aggregation without cast (`MEDIUM`); date string compared to `TIMESTAMP` literal (`MEDIUM`).
10. **Spark best practice** — `COUNT(*)` vs `COUNT(1)` is a style nit (`LOW`); `SELECT *` in production is `MEDIUM`; missing `DISTRIBUTE BY` / `CLUSTER BY` for skewed joins is `LOW` to `MEDIUM`.

## Output Contract

```yaml
sql_review:
  verdict: <PASS | FAIL>
  highest_severity: <HIGH | MEDIUM | LOW>
  summary: "<one-sentence summary>"
  findings:
    - id: F1
      severity: HIGH
      check: reference_integrity
      location: "<line/column or SQL fragment>"
      message: "<what is wrong>"
      fix: "<concrete suggested change>"
    - id: F2
      severity: MEDIUM
      check: join_cardinality
      location: "JOIN customer_info"
      message: "1:N join without pre-aggregation; risk of row inflation"
      fix: "Aggregate loan_order by customer_id before joining"
  notes:
    - "SQL Context provided: yes (sql_context.aliases = [...])"
    - "Reviewed by: sql-review v1"
```

## Forbidden Behaviors

| Rationalization | Reality |
|---|---|
| "The SQL looks fine, no need to cross-check the context" | A column can look fine and still not exist in the source table. Cross-check is mandatory. |
| "I'll fix the SQL while reviewing" | The reviewer reports. The author fixes. Separation of concerns. |
| "No SQL Context was provided, so I'll skip field checks" | Missing context is itself a `HIGH` finding. Never silently skip. |
| "It's just a dev query, skip partition check" | The check is cheap. Skipping it produces silent full scans. |
| "1:N joins are normal, no risk" | 1:N is fine *if* the grain is preserved. The reviewer must verify, not assume. |
| "Regex is enough to check GROUP BY" | GROUP BY membership is structural. Use a parser, not regex. |

## Completion Criteria

A review is complete when:
- All 10 checks have been evaluated
- Every `HIGH` finding has a `fix`
- `verdict` is `PASS` iff `highest_severity` is `MEDIUM` or `LOW`
- The output YAML can be pasted into a PR comment without edits