init
This commit is contained in:
tao.chen
@@ -0,0 +1,124 @@
|
||||
---
|
||||
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
|
||||
Reference in New Issue
Block a user