diff --git a/CHANGELOG.md b/CHANGELOG.md index fb827a6..85085b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,40 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 could no longer satisfy the hot query's `ORDER BY`. - ADV003 is scoped to the tables the workload was observed using, like ADV002 — it no longer proposes `DROP INDEX` for a relation the run never analysed. +- Optional dbt enrichment for `advise`: `--project-dir` (reads + `/target/manifest.json`) or `--manifest ` layers dbt model metadata + onto the same analysis, and every manifest-free `advise` invocation is proven + byte-identical (stdout, markdown, DDL and stderr) to a run with no dbt support at all. + **ADV302** rewrites an index-creating proposal for a `table`-, `incremental`- or + `materialized_view`-materialized dbt model into a commented `indexes:` config block + instead of raw DDL, because a normal `dbt run` (or `--full-refresh`) drops and rebuilds + those relations and the DDL would not survive it; on a `view` the *DDL* is dropped and + explained while the proposal stays at LOW confidence, since a view has no storage to index + but "this index cannot apply here" is still the finding. ADV302 is a rewrite, not a + proposal code: the original rule keeps its code, confidence and cost share, so no proposal + ever carries `code: "ADV302"` — filter on `evidence.dbt_index_config` instead — and + `advise` prints a stderr line saying how many proposals it rewrote, since the terminal + table row is otherwise unchanged. Several index proposals for one model merge into a single + `indexes:` block, because dbt reads one `indexes` key per config and two blocks pasted into + one config are a duplicate YAML key whose loser is silently discarded. Wherever ADV302 + declines and leaves executable DDL in place (a partial index, an unrecognised + materialization, no plain column list, a non-btree access method), the warning is written + into the `--ddl` script above the statement, not only into the rationale. A `DROP INDEX` + proposal (ADV002, ADV003) on a dbt-managed relation is the same hazard pointing the other + way — if the index is declared in the model's `indexes:` config, `dbt run` recreates it and + the drop silently reverts — so those keep their DDL and gain a warning, in the rationale and + in the DDL script, that the config entry has to be removed too. A manifest whose + `adapter_type` is neither postgres nor redshift (or is absent), or whose schema is not v12, + is disclosed on stderr: a foreign adapter means dbt is not building the relations `advise` + introspected at all, so every match is a name coincidence. **ADV301** proposes + materializing a `view`-backed model that carries a hot share of workload cost, capped at + MEDIUM. **ADV303** flags a dbt model within reach of the manifest that the analyzed + workload never touched and that no other model, snapshot or dbt exposure declares as a + consumer, capped at LOW. Matching a model to a relation is on the exact `(schema, table)` + pair with no bare-name fallback, since a dbt project's target schema routinely differs + from the schema being introspected; a relation two different models both claim is + dropped from matching (not guessed at) and counted in the CLI disclosure and the JSON + payload's `dbt.dropped_collisions`. ### Fixed diff --git a/README.md b/README.md index 1e1c59c..5df5a8a 100644 --- a/README.md +++ b/README.md @@ -268,8 +268,9 @@ optimizations — indexes to add, indexes to drop, partial indexes, non-sargable predicates, and hot `SELECT *`. Output is an advisory report plus a DDL file for you to review. **`advise` never writes to your database and never executes DDL.** -Only **Postgres** is implemented today; Redshift, Snowflake and dbt enrichment are -designed but not built — see [Limitations](#limitations). +Only **Postgres** is implemented today; Redshift and Snowflake are designed but not built +— see [Limitations](#limitations). An optional dbt manifest enriches the same analysis — +see [dbt enrichment](#dbt-enrichment-optional) below. ```console $ sqlquality advise --dsn postgresql://readonly@db.internal/analytics @@ -312,10 +313,12 @@ missing driver degrades with an install hint instead of a traceback. | `--profile` | — | dbt profile name, read from `profiles.yml`. | | `--target` | — | dbt target within the profile. | | `--profiles-dir` | `~/.dbt` | Directory holding `profiles.yml`. | +| `--project-dir` | — | dbt project dir; reads `target/manifest.json` to enrich proposals (optional). See [dbt enrichment](#dbt-enrichment-optional). | +| `--manifest` | — | Path to a dbt `manifest.json`. Overrides `--project-dir`. | | `--schema` | `public` | Schema to introspect. Repeat for several: `--schema public --schema sales`. See Limitations for the ambiguity caveat. | | `--since` | — | Window, e.g. `7d`. **Not honored on Postgres** — see Prerequisites below. | | `--limit` | `500` | Max query-history rows to read. | -| `--min-cost-share` | `0.01` | Suppress proposals below this share of workload cost. Applies to the **cost-weighted** rules (ADV001, ADV004, ADV005, ADV006, ADV007, ADV008); the index-hygiene rules **ADV002 and ADV003 carry no cost evidence and are always reported**, whatever the threshold. | +| `--min-cost-share` | `0.01` | Suppress proposals below this share of workload cost. Applies to the **cost-weighted** rules (ADV001, ADV004, ADV005, ADV006, ADV007, ADV008, ADV301 — the last only with `--project-dir`/`--manifest`); the index-hygiene rules **ADV002 and ADV003**, and **ADV303** (its evidence is absence, not cost, so there is no share to threshold), carry no cost evidence and are reported whatever the threshold. ADV303 has its own non-threshold suppression: it emits nothing at all when no query usage could be extracted, since then every model would look untouched by definition. | | `--keep-literals` | off | Do **not** redact literal values from query text. | | `--timeout` | `30` | Statement timeout in seconds (rejected outside 1–3600). | | `--dry-run` | off | Print every statement the adapter would issue, then exit 0 **without connecting**. | @@ -386,6 +389,19 @@ statement is not valid SQL to copy out and run. | ADV006 | Hot `SELECT *` on a wide table (≥15 columns) | cost share, column count | | ADV007 | Add index on a hot join key with no existing index leading with it | cost share, NDV, row estimate, absence of a covering index | | ADV008 | Composite index for a hot `GROUP BY`, column order inferred from cost, capped at MEDIUM | cost share, row estimate, absence of a covering index | +| ADV301¹ | Materialize a `view`-backed dbt model that carries a hot share of workload cost, capped at MEDIUM | cost share, dbt model | +| ADV303¹ | A dbt model within reach of the manifest that the analyzed workload never touched and no other model, snapshot or exposure declares as a consumer, capped at LOW | dbt model | + +¹ Only fires with `--project-dir` or `--manifest` loaded — see [dbt enrichment](#dbt-enrichment-optional). + +**ADV302 is not in that table, because it is not a proposal code.** It is a *rewrite* +applied to another rule's proposal — ADV001, ADV004, ADV007 or ADV008 keeps its own code, +confidence and cost share, and only its `ddl` and `rationale` change. So no proposal ever +carries `code: "ADV302"`, and a `--json` consumer filtering on that code sees zero rows on +every run; filter on `evidence.dbt_index_config == true` instead (present, and `true`, only +on a proposal whose DDL was replaced by a dbt config block). The terminal table shows the +original rule's row unchanged, so `advise` prints a line on stderr saying how many proposals +ADV302 rewrote. See [dbt enrichment](#dbt-enrichment-optional). **Confidence model**, mechanical rather than judgment-based: @@ -455,7 +471,8 @@ DROP INDEX "public"."idx_orders_customer_ref"; ``` `--json` emits the same evidence as a structured payload (`analyzed`, `degraded`, -`engine`, `proposals`, `redacted`, `skipped`, `window`). This is the first proposal from +`engine`, `proposals`, `redacted`, `skipped`, `window`, plus `dbt` when — and only when — a +manifest was loaded). This is the first proposal from the run above — the real payload lists all five under `proposals`: ```console @@ -543,6 +560,105 @@ exposes only rows for tables the current role owns or can select from — a role table access silently sees no statistics ``` +#### dbt enrichment (optional) + +Passing `--project-dir` (reads `/target/manifest.json`) or `--manifest +` layers dbt model metadata onto the same analysis. Neither is required: every +`advise` invocation without one behaves exactly as documented above, and that no-manifest +path is proven byte-identical (stdout, markdown, DDL and stderr) to a run with no dbt +support at all — dbt is enrichment layered on top of an engine-agnostic core, never a +requirement of it. + +**Why ADV302 exists.** The rules above propose DDL from query cost and catalog metadata +with no idea whether the table they're indexing is dbt-managed — and if it is, that +matters. dbt's `table` materialization drops and recreates its relation on *every* +`dbt run`, so a raw `CREATE INDEX` applied once is silently gone the next time dbt runs. +`incremental` differs only in degree: a normal run keeps the relation, but +`dbt run --full-refresh` rebuilds it the same way. `materialized_view` behaves like +`incremental` — refreshed in place on a normal run, rebuilt on `--full-refresh` or a config +change dbt can't apply in place. A plain `view` has no storage of its own at all, so it +cannot carry an index. Confidently advising DDL that a routine `dbt run` silently erases is +worse than advising nothing, which is what **ADV302** exists to prevent: with a manifest +loaded, an index-creating proposal for a `table`-, `incremental`- or +`materialized_view`-materialized relation is rewritten into a commented dbt `indexes:` +config block you paste into that model's own config instead of DDL you'd apply once and +lose; on a `view` the **DDL** is dropped and explained instead (there is no relation to +index) while the proposal itself stays, downgraded to LOW — "this index cannot apply here" +is the finding; on any other or absent materialization the DDL is left untouched, since +unrecognised is not the same as known-safe. A partial (`WHERE`-restricted) index has no +config-block equivalent — dbt's `indexes` config carries no predicate — so that proposal is +disclosed as not expressible rather than silently dropping the predicate. + +**One model, one `indexes:` block.** dbt reads a single `indexes` key per model config, so +when a run recommends several indexes for the same model they are merged into one block, +carried by the highest-ranked of those proposals; each of the others points at it by code +instead of emitting a block of its own. Two standalone blocks pasted under one `config:` are +a duplicate YAML mapping key, and PyYAML — dbt's own parser — resolves that by silently +keeping one and discarding the other recommended index, with no error. + +**Whenever a statement is left executable for a dbt-managed relation** — the partial-index, +unrecognised-materialization, no-column-list and non-btree paths above — the warning is +written into the `--ddl` script itself, as comment lines directly above the statement, not +only into the `rationale`. The DDL script carries no rationales, and it is the artifact a +human actually applies. + +**A `DROP INDEX` proposal on a dbt-managed relation is the same hazard pointing the other +way.** ADV002 and ADV003 read the catalog, not the manifest, so they will propose dropping an +index that the model's `indexes:` config still declares — and the next `dbt run` puts it +straight back, after which the tool proposes the same drop again. Those proposals keep their +DDL (dropping a genuinely unused index is still right, and dbt's `indexes` config cannot +express a removal) and gain a warning, in the rationale *and* in the `--ddl` file, that the +config entry has to be removed as well or the drop will not stick. + +**`advise` checks the manifest against the connection**, the same two checks `check` makes on +the same file: it warns when the manifest is not a v12 schema, and when its `adapter_type` is +neither `postgres` nor `redshift`. The second matters more than the missing `indexes:` config +key would suggest: a Snowflake or BigQuery manifest paired with a Postgres connection means +dbt is not building the relations `advise` just introspected *at all*, so every match is a +name coincidence and all three dbt rules are wrong — ADV302's premise that a `dbt run` +rebuilds the relation included. A manifest recording **no** `adapter_type` warns too, since +`dbt compile` always writes one and the honest statement is that the pairing could not be +checked. `advise` warns rather than suppressing: the mismatch is something to fix in your +invocation, and dropping all dbt output silently would hide it. + +**The block is rebuilt from the proposal's column list, not from its DDL**, and always as +`type: btree`. That is faithful for every rule shipping today — each emits a plain btree over +a column list with no `USING`, expression, `DESC`/`NULLS` or opclass — and a proposal naming a +non-btree access method declines the rewrite rather than being flattened into a btree. +Ordering, opclasses and expression indexes are *not* detected: a future rule emitting one +would need this reconstruction extended alongside it. + +Two more proposals only fire with a manifest loaded — see the proposal table above for +ADV301 and ADV303. Both are capped below HIGH, for the same reason ADV302's rewrite trusts +the manifest as of whenever `dbt compile` last ran: a model's materialization or its +consumers can change without a fresh compile, so a stale manifest degrades to a wrong (but +traceable — the disclosed materialization or lack of a consumer names why) recommendation +rather than a silent one. + +**Matching is exact, deliberately.** A model's `relation_name` is dropped down to its +`(schema, table)` pair (dbt writes a `catalog.schema.table` name; the database part is +discarded, since `advise` connects to one database at a time) and matched against the +relation each proposal already carries — **there is no bare-table-name fallback**. A dbt +project's target schema (`dev`, `main`, a CI schema, ...) routinely differs from the schema +`advise` introspects in production, so matching on the table name alone would risk +attributing a production table's proposal to an unrelated development model — and ADV302 +would then rewrite that table's DDL on the strength of a wrong guess. If two *different* +models both build the same `(schema, table)` pair (legitimate when a project targets more +than one database), `advise` cannot tell which is live: that relation is dropped from +matching entirely — not guessed at — and counted, both in the CLI's `dbt enrichment from +...` disclosure line and in the JSON payload's `dbt.dropped_collisions`. + +```console +$ sqlquality advise --dsn postgresql://readonly@db.internal/analytics --project-dir ./my_dbt_project +engine: postgres (credentials from --dsn) +dbt enrichment from my_dbt_project/target/manifest.json (42 model(s)) +... +``` + +A manifest that is missing, unreadable or malformed degrades to "no enrichment" plus a +line on stderr — `advise` never aborts an otherwise-successful run over an optional input, +since by the time the manifest loads the whole catalog analysis has already run. + ### check (the CI gate) Scores each changed model on both a candidate and a baseline dbt manifest, and gates @@ -897,6 +1013,28 @@ LLM suggestions unavailable: The 'anthropic' package is required for AnthropicPr Qualify the table in the query, or run `advise` once per `--schema`, to recover it. Generated DDL is qualified with the schema it was read from, so it does not depend on the applying session's `search_path`. -- **Redshift, Snowflake and dbt enrichment are designed but not implemented.** `advise` - supports Postgres only today; passing another `--engine` fails with a clear error - rather than silently degrading. +- **Redshift and Snowflake are designed but not implemented.** `advise` supports Postgres + only today; passing another `--engine` fails with a clear error rather than silently + degrading. Optional dbt enrichment (`--project-dir`/`--manifest`, see + [dbt enrichment](#dbt-enrichment-optional)) is implemented for Postgres. +- **dbt enrichment trusts the manifest as of its last `dbt compile`.** ADV302 rewrites DDL + based on a model's materialization as the manifest records it; a materialization changed + without a fresh `dbt compile` produces a stale — but traceable, since the disclosed + materialization names its own source — rewrite. Nothing verifies the manifest against + the live relation. +- **A manifest for another warehouse is warned about, not rejected.** `advise` connects to + Postgres; a manifest whose `adapter_type` is something else (or absent) gets a stderr + warning and enrichment still runs, so a project whose manifest and target database disagree + gets dbt proposals built on `(schema, table)` name coincidences. dbt's `indexes` model + config is likewise implemented by the postgres and redshift adapters only, and ADV302's + rewrite is not translated per adapter. +- **ADV302 reconstructs the index from the proposal's column list.** The emitted block is + always `type: btree` over that column list; column *ordering* is preserved but opclasses, + `DESC`/`NULLS` and expression indexes are not expressible, and a non-btree access method + declines the rewrite rather than being silently flattened. That last check is textual (no + rule records an access method in evidence), so a column whose *name* contains a `USING` + clause declines a rewrite that would have been fine — the safe direction. +- **ADV303 only looks at a model's immediate consumers.** A dead model feeding another dead + model is not reported until the downstream one is gone, so a fully dead chain unwinds one + model per run, from its leaf. Conservative by construction: it never flags a model that + something declares a dependency on. diff --git a/docs/superpowers/plans/2026-07-27-advise-dbt-enrichment.md b/docs/superpowers/plans/2026-07-27-advise-dbt-enrichment.md new file mode 100644 index 0000000..dc93374 --- /dev/null +++ b/docs/superpowers/plans/2026-07-27-advise-dbt-enrichment.md @@ -0,0 +1,860 @@ +# Advise dbt Enrichment Implementation Plan (Batch 3a) + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When a dbt manifest is available, `advise` stops proposing DDL that dbt will destroy, and adds three proposals that are only computable by joining workload cost to the model graph. + +**Architecture:** dbt stays **optional enrichment layered on top of an engine-agnostic core** — no adapter learns anything about dbt. `PostgresWorkloadAdapter.propose()` returns proposals as it does today; a new engine-neutral `workload/dbt.py` then transforms them and appends ADV301–ADV303. The seam is one call in `cli.py` between `propose()` and the renderers, so the same enrichment will apply to the Redshift adapter in Batch 3b without change. + +**Tech Stack:** Python 3.11+, existing `DbtProject` (manifest v12 reader), sqlglot 30.x, typer, rich, pytest. + +## Global Constraints + +Every task's requirements implicitly include this section. + +- **All four CI gates must pass before every commit:** `uv run ruff check .`, `uv run ruff format --check .`, `uv run mypy src/sqlquality`, `uv run pytest -q`. There are now also `no-extras` and `highest-deps` CI jobs; don't break either. +- **`uv run pytest` must report `N passed, M deselected`, never `skipped`.** Integration tests are marked `integration`. +- **dbt is optional, never required.** Every existing invocation of `advise` must behave **identically** when no manifest is supplied. This is a hard requirement, not a nicety: the project's positioning is that the dbt-free path is first-class. +- **No adapter may import from `workload/dbt.py`.** Enrichment is applied above the adapter layer. If you find yourself needing adapter knowledge inside the enrichment, stop and report it. +- **sqlquality never executes user SQL.** DDL is written to a file for human review. +- **No credential and no user literal may reach stdout, stderr, a report, or an exception message.** +- **Confidence never overstates evidence.** A check that could not run is disclosed, not assumed. +- **A test that passes with the production change reverted is not a test.** Every test must be observed to FAIL against a deliberate mutation of the line it claims to pin, and the mutation reported. Use `PYTHONDONTWRITEBYTECODE=1` and purge `__pycache__` around each. +- **Where a test asserts over a set, it must discriminate for every member.** Batch 2 produced **seven** findings of the shape "asserts over a set, checks one member". Do not add an eighth. +- Public identifiers get a docstring saying *why*, matching the surrounding module's density. + +## Facts established by probing before this plan was written + +Do not re-derive these; do not contradict them. + +- `ModelNode` already carries `unique_id`, `name`, `resource_type`, `materialized`, `compiled_code`, `relation_name`, `depends_on`, `config`. No new manifest parsing is needed for this plan. +- `relation_name` is a **quoted three-part** string: `'"dev"."main"."stg_orders"'`. The `schema` field on the raw node is `None` in real fixtures, so `relation_name` is the only reliable source of the schema. +- `tests/fixtures/manifest_v12.json` already contains what this plan needs: `model.demo.stg_orders` materialized as `view`, `model.demo.orders` materialized as `table` **with an `indexes` key already in its config**, and non-model resources (`seed`, `test`) that must not be treated as models. +- `original_file_path` and `patch_path` are `None` in that fixture, so a model's source file is **not** always knowable. Every message must degrade gracefully. +- `advise` has **no** manifest option today; `check` derives one from `--project-dir` as `project_dir / "target" / "manifest.json"`. +- `resolve_connection` already recognises the `redshift` and `snowflake` engines, but `get_workload_adapter` only registers `postgres`, so `--engine redshift` fails today with a clear `ValueError`. That is Batch 3b's problem, not this plan's. + +## Why ADV302 is the point of this plan + +dbt's `table` materialization **drops and recreates** the relation on every `dbt run`. So a `CREATE INDEX` that `advise` currently emits for a dbt-managed table is destroyed by the next build — the tool is confidently telling an operator to do something that silently reverts. dbt's postgres adapter accepts an `indexes` config instead, which it applies after each build. The fixture already has a model using it. + +`incremental` differs and the difference matters: dbt does not recreate the relation on a normal run, so an index **survives** until someone runs `--full-refresh`. And a `view` cannot carry an index at all, which makes any index proposal against one meaningless rather than merely fragile. + +--- + +### Task 1: load a manifest for `advise`, and map relations to models + +**Files:** +- Create: `src/sqlquality/workload/dbt.py` +- Create: `tests/test_workload_dbt.py` +- Modify: `src/sqlquality/cli.py` (`advise` gains `--project-dir` / `--manifest`) + +**Interfaces:** +- Produces: + - `sqlquality.workload.dbt.parse_relation_name(relation_name: str) -> Relation | None` + - `sqlquality.workload.dbt.DbtContext` — `@dataclass(frozen=True)` holding + `models: dict[Relation, ModelNode]`, with: + - `classmethod from_project(project: DbtProject) -> DbtContext` + - `model_for(relation: Relation) -> ModelNode | None` + - `sqlquality.workload.dbt.load_dbt_context(project_dir: Path | None, manifest: Path | None) -> tuple[DbtContext | None, str | None]` + returning `(context, disclosure)` — `(None, None)` when neither option was given. +- Consumes: `DbtProject`, `ModelNode`, `Relation`. + +**The matching rule, and why it declines rather than guesses.** `relation_name` gives +`database.schema.table`; a `Relation` has only `(schema, table)`. Match on the last two parts and +ignore the database, because `advise` connects to one database at a time. Do **not** fall back to +matching on the bare table name when the schemas differ: dbt's `main`/`dev` targets routinely +differ from a production schema, and a bare-name match would attribute a production table to an +unrelated dev model and then rewrite its DDL. An unmatched relation simply gets no enrichment. + +- [ ] **Step 1: Write the failing tests** + +```python +import json +from pathlib import Path + +import pytest + +from sqlquality.dbtproject import DbtProject +from sqlquality.models import Relation +from sqlquality.workload.dbt import DbtContext, load_dbt_context, parse_relation_name + +FIXTURE = Path(__file__).parent / "fixtures" / "manifest_v12.json" + + +def _project() -> DbtProject: + return DbtProject.from_path(FIXTURE) + + +@pytest.mark.parametrize( + "raw,expected", + [ + ('"dev"."main"."stg_orders"', Relation("main", "stg_orders")), + ('"main"."orders"', Relation("main", "orders")), + ("dev.main.orders", Relation("main", "orders")), + ('"dev"."main"."Weird.Name"', Relation("main", "Weird.Name")), + ], +) +def test_parse_relation_name_takes_the_last_two_parts(raw, expected): + assert parse_relation_name(raw) == expected + + +@pytest.mark.parametrize("raw", ["", "orders", '"orders"', " "]) +def test_parse_relation_name_declines_what_it_cannot_qualify(raw): + """A one-part name has no schema, and inventing one would mis-attribute.""" + assert parse_relation_name(raw) is None + + +def test_context_indexes_models_by_relation(): + context = DbtContext.from_project(_project()) + node = context.model_for(Relation("main", "stg_orders")) + assert node is not None + assert node.unique_id == "model.demo.stg_orders" + assert node.materialized == "view" + + +def test_context_excludes_non_model_resources(): + """A seed and a test are not models: proposing a materialization change for a dbt + test, or rewriting DDL because a seed shares a name, would both be nonsense.""" + context = DbtContext.from_project(_project()) + assert context.model_for(Relation("main", "raw_orders")) is None + for node in context.models.values(): + assert node.resource_type == "model" + + +def test_context_does_not_match_on_a_bare_table_name(): + """dbt's target schema routinely differs from the introspected one. Matching `orders` + in schema `public` to a model in schema `main` would attribute a production table to an + unrelated dev model and then rewrite its DDL.""" + context = DbtContext.from_project(_project()) + assert context.model_for(Relation("public", "orders")) is None + assert context.model_for(Relation("main", "orders")) is not None + + +def test_load_returns_nothing_when_no_option_is_given(): + assert load_dbt_context(None, None) == (None, None) + + +def test_load_reads_an_explicit_manifest_and_discloses_the_source(): + context, disclosure = load_dbt_context(None, FIXTURE) + assert context is not None + assert disclosure is not None and str(FIXTURE) in disclosure + + +def test_load_reports_a_missing_manifest_without_raising(tmp_path): + """A bad manifest path must degrade to 'no enrichment', not abort a run that already + did all the catalog work — the same reasoning as the report-write failure path.""" + context, disclosure = load_dbt_context(None, tmp_path / "nope.json") + assert context is None + assert disclosure is not None and "nope.json" in disclosure + + +def test_load_reports_unparseable_json_without_raising(tmp_path): + bad = tmp_path / "manifest.json" + bad.write_text("{not json", encoding="utf-8") + context, disclosure = load_dbt_context(None, bad) + assert context is None + assert disclosure is not None +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `uv run pytest tests/test_workload_dbt.py -x -q` + +Expected: FAIL — `ModuleNotFoundError: No module named 'sqlquality.workload.dbt'`. + +- [ ] **Step 3: Implement the module** + +```python +"""Optional dbt enrichment for `advise`. + +dbt is *layered on top of* the engine-agnostic core, never underneath it: no workload adapter +imports this module, and every `advise` run behaves identically without a manifest. The +project's positioning is that the dbt-free path is first-class, so enrichment has to be +additive by construction rather than by discipline. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass +from pathlib import Path + +from sqlquality.dbtproject import DbtProject, DbtProjectError, ModelNode +from sqlquality.models import Relation + +#: Splits a dbt `relation_name` on unquoted dots. dbt quotes each part, so a dot *inside* a +#: quoted identifier (`"Weird.Name"`) must not split — hence matching quoted segments first. +_PART = re.compile(r'"((?:[^"]|"")*)"|([^.]+)') + + +def parse_relation_name(relation_name: str) -> Relation | None: + """`(schema, table)` from a dbt `relation_name`, or None if it cannot be qualified. + + dbt writes a quoted three-part name — `'"dev"."main"."stg_orders"'` — and the raw node's + own `schema` field is `None` in practice, so this string is the only reliable source. The + database part is dropped because `advise` connects to one database at a time. + + A name with fewer than two parts returns None rather than a guess: a `Relation` needs a + schema, and inventing one is how a production table gets attributed to an unrelated model. + """ + parts = [ + (quoted if quoted is not None else bare).replace('""', '"') + for quoted, bare in _PART.findall(relation_name.strip()) + ] + parts = [p for p in parts if p] + if len(parts) < 2: + return None + return Relation(schema=parts[-2], table=parts[-1]) + + +@dataclass(frozen=True) +class DbtContext: + """dbt models indexed by the relation they build, for joining against workload facts.""" + + models: dict[Relation, ModelNode] + + @classmethod + def from_project(cls, project: DbtProject) -> DbtContext: + """Index every *model* by its relation. + + Only `resource_type == "model"` is indexed. Seeds, tests and snapshots also occupy + relations, but "materialize this dbt test as a table" and "express this seed's index + as dbt config" are both nonsense, and a seed sharing a name with a model's relation + would otherwise silently win the mapping. + """ + models: dict[Relation, ModelNode] = {} + for uid in project.model_ids(): + node = project.node(uid) + if node.resource_type != "model" or not node.relation_name: + continue + relation = parse_relation_name(node.relation_name) + if relation is not None: + models[relation] = node + return cls(models=models) + + def model_for(self, relation: Relation) -> ModelNode | None: + """The model building this exact relation, matching schema *and* table. + + Deliberately no bare-table-name fallback: dbt's `main`/`dev` target schemas routinely + differ from the schema `advise` introspects, so a name-only match would attribute a + production table to an unrelated development model — and then, via ADV302, rewrite + that table's DDL on the strength of it. + """ + return self.models.get(relation) + + +def load_dbt_context( + project_dir: Path | None, manifest: Path | None +) -> tuple[DbtContext | None, str | None]: + """Load a manifest if one was requested, returning `(context, disclosure)`. + + Never raises. A manifest that is missing, unreadable or malformed degrades to "no + enrichment" plus a line for the user, because by the time this runs the whole catalog + analysis has already happened — aborting would throw away real work over an optional + input. Same reasoning as the report-write failure path in `cli.py`. + """ + if manifest is None and project_dir is None: + return None, None + path = manifest if manifest is not None else (project_dir or Path()) / "target" / "manifest.json" + try: + project = DbtProject.from_path(path) + except (OSError, ValueError, DbtProjectError) as exc: + # `ValueError` covers `json.JSONDecodeError`, and `DbtProjectError` is a ValueError + # subclass — both listed so the intent survives a refactor of either. + return None, f"dbt enrichment unavailable: could not read {path}: {exc}" + context = DbtContext.from_project(project) + return context, f"dbt enrichment from {path} ({len(context.models)} model(s))" +``` + +- [ ] **Step 4: Wire the CLI options** + +Add to `advise`, alongside the existing dbt-flavoured options: + +```python + project_dir: Path | None = typer.Option( + None, + "--project-dir", + help="dbt project dir; reads target/manifest.json to enrich proposals (optional).", + ), + manifest: Path | None = typer.Option( + None, "--manifest", help="Path to a dbt manifest.json. Overrides --project-dir." + ), +``` + +In the body, after `proposals = adapter.propose(...)`, load the context and echo the +disclosure to stderr. Do not apply enrichment yet — Tasks 2-4 add the rules and Task 5 wires +them. For this task it is enough that the options parse, the manifest loads, and the +disclosure prints. + +- [ ] **Step 5: Run the tests** + +Run: `uv run pytest tests/test_workload_dbt.py -q` then `uv run pytest -q` + +Expected: PASS. The full suite must be unchanged in count except for the new tests. + +- [ ] **Step 6: Prove the no-guess rule discriminates** + +Add a bare-name fallback to `model_for` — `self.models.get(relation) or next((n for r, n in +self.models.items() if r.table == relation.table), None)` — and run +`tests/test_workload_dbt.py`. Expected: `test_context_does_not_match_on_a_bare_table_name` +FAILS. Restore. Report the mutation. + +- [ ] **Step 7: Commit** + +```bash +git add src/sqlquality/workload/dbt.py src/sqlquality/cli.py tests/test_workload_dbt.py +git commit -m "feat(advise): load an optional dbt manifest and index models by relation" +``` + +--- + +### Task 2: ADV302 — stop proposing DDL that dbt will destroy + +**Files:** +- Modify: `src/sqlquality/workload/dbt.py` +- Test: `tests/test_workload_dbt.py` + +**Interfaces:** +- Consumes: `DbtContext` (Task 1), `Proposal`, `Relation`. +- Produces: + `enrich_proposals(proposals: list[Proposal], context: DbtContext) -> list[Proposal]` — + rewrites index-creating proposals whose relation is a dbt model, and emits ADV302 notes. + Returns the list unchanged when `context` has no matching model. + +**This is the correctness fix.** A `CREATE INDEX` on a `table`-materialized dbt model is +destroyed by the next `dbt run`. The three materializations differ and the difference is the +whole rule: + +| materialized | what happens to a raw `CREATE INDEX` | what to say | +|---|---|---| +| `table` | dropped on **every** `dbt run` | express as an `indexes` config entry; raw DDL will not survive | +| `incremental` | survives a normal run, lost on `--full-refresh` | express as config so a full refresh keeps it | +| `view` | cannot exist at all | the proposal is not applicable; a view has no storage to index | +| anything else / absent | unknown | say the materialization is unrecognised and leave the DDL alone | + +- [ ] **Step 1: Write the failing tests** + +```python +from sqlquality.models import Confidence, Proposal +from sqlquality.workload.dbt import enrich_proposals + + +def _index_proposal(relation, columns=("status",), code="ADV001"): + quoted = ", ".join(f'"{c}"' for c in columns) + return Proposal( + code=code, + title=f"Add index on {relation}({', '.join(columns)})", + rationale="hot predicate.", + evidence={ + "schema": relation.schema, + "table": relation.table, + "columns": tuple(columns), + "cost_share": 0.5, + }, + confidence=Confidence.HIGH, + ddl=f'CREATE INDEX ON "{relation.schema}"."{relation.table}" ({quoted});', + ) + + +def test_adv302_replaces_raw_ddl_for_a_table_model_with_a_dbt_config_block(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + [out] = enrich_proposals([_index_proposal(relation)], context) + assert out.ddl is not None + assert "CREATE INDEX" not in out.ddl + assert "indexes" in out.ddl + assert "columns" in out.ddl and "status" in out.ddl + assert "dbt run" in out.rationale + + +def test_adv302_keeps_the_relation_and_columns_it_was_given(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + [out] = enrich_proposals([_index_proposal(relation, ("status", "created_at"))], context) + assert "status" in out.ddl and "created_at" in out.ddl + assert out.evidence["dbt_model"] == "model.demo.orders" + assert out.evidence["dbt_materialized"] == "table" + + +def test_adv302_says_a_view_cannot_be_indexed_at_all(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "stg_orders") # materialized: view + [out] = enrich_proposals([_index_proposal(relation)], context) + assert out.ddl is None, "a view has no storage to index, so there is no DDL to run" + assert "view" in out.rationale + assert out.confidence is Confidence.LOW + + +def test_adv302_distinguishes_incremental_from_table(): + """An index survives a normal incremental run and is lost on --full-refresh. Saying + 'every dbt run drops it' would be false, and false in the direction that makes an + operator distrust a correct proposal.""" + project = _project_with_materialization("model.demo.orders", "incremental") + context = DbtContext.from_project(project) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"))], context) + assert "full-refresh" in out.rationale or "full refresh" in out.rationale + assert "every dbt run" not in out.rationale + + +def test_adv302_leaves_an_unrecognised_materialization_alone_and_says_so(): + project = _project_with_materialization("model.demo.orders", "exotic") + context = DbtContext.from_project(project) + original = _index_proposal(Relation("main", "orders")) + [out] = enrich_proposals([original], context) + assert out.ddl == original.ddl, "unknown materialization must not have its DDL rewritten" + assert "exotic" in out.rationale + + +def test_adv302_does_not_touch_a_relation_dbt_does_not_manage(): + context = DbtContext.from_project(_project()) + original = _index_proposal(Relation("public", "orders")) + assert enrich_proposals([original], context) == [original] + + +def test_adv302_does_not_rewrite_a_drop_index_proposal(): + """Dropping an index dbt never created is a perfectly ordinary thing to do, and there is + no `indexes` config that expresses a removal.""" + context = DbtContext.from_project(_project()) + drop = Proposal( + code="ADV002", + title="Drop unused index idx_cold on main.orders", + rationale="no scans.", + evidence={"schema": "main", "table": "orders", "index": "idx_cold"}, + confidence=Confidence.MEDIUM, + ddl='DROP INDEX "main"."idx_cold";', + ) + [out] = enrich_proposals([drop], context) + assert out.ddl == drop.ddl + + +def test_adv302_does_not_rewrite_an_advisory_proposal_with_no_ddl(): + context = DbtContext.from_project(_project()) + advisory = Proposal( + code="ADV005", + title="Non-sargable predicate on main.orders.status", + rationale="wrapped in a function.", + evidence={"schema": "main", "table": "orders", "column": "status"}, + confidence=Confidence.HIGH, + ddl=None, + ) + [out] = enrich_proposals([advisory], context) + assert out.ddl is None + # It should still be attributed to the model, so the reader knows where to fix it. + assert out.evidence["dbt_model"] == "model.demo.orders" +``` + +Add the helper the two materialization tests need, next to `_project`: + +```python +def _project_with_materialization(uid: str, materialized: str) -> DbtProject: + """The fixture manifest with one model's materialization changed. + + Edits a deep copy rather than a second fixture file: the point of variation is one field, + and a whole extra manifest would drift from the real one. + """ + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["nodes"][uid]["config"]["materialized"] = materialized + return DbtProject.from_manifest(raw) +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `uv run pytest tests/test_workload_dbt.py -x -q` + +Expected: FAIL — `ImportError: cannot import name 'enrich_proposals'`. + +- [ ] **Step 3: Implement `enrich_proposals`** + +Key points to get right, stated because each is a way to get it wrong: + +- Detect an index-creating proposal by its **DDL prefix** (`CREATE INDEX`), not by rule code: + ADV001, ADV007 and ADV008 all create indexes and Batch 3b will add more, so a code list + would silently miss the next one. +- Preserve a `WHERE` clause if present. ADV004's partial index cannot be expressed by dbt's + `indexes` config, which has no predicate field — so that proposal must be **disclosed as + not expressible**, keeping its DDL and saying dbt will drop it. Do not silently emit a + config block that loses the predicate. +- The generated config block is YAML for a human to paste, so it goes in `ddl` (the reviewable + script) commented as configuration, not as an executable statement. `render_ddl` comments + every non-DDL line already; make sure what you emit survives that renderer unchanged. + +```python +#: dbt materializations whose relation is rebuilt, and what that does to a raw index. +_REBUILD = { + "table": "every `dbt run` drops and recreates this relation, so a raw CREATE INDEX is lost", + "incremental": ( + "a normal `dbt run` keeps this relation, but `dbt run --full-refresh` rebuilds it and a " + "raw CREATE INDEX is lost" + ), +} +``` + +Emit, for a rebuilt materialization, a `ddl` value of the form: + +``` +-- ADV302: express this as dbt config, not DDL. Add to the model's config block: +-- indexes: +-- - columns: ['status', 'created_at'] +-- type: btree +``` + +and append to the rationale which materialization it is and what happens. Add +`"dbt_model"`, `"dbt_materialized"` and `"dbt_index_config"` to `evidence`. + +- [ ] **Step 4: Run the tests** + +Run: `uv run pytest tests/test_workload_dbt.py -q` then `uv run pytest -q` + +Expected: PASS. + +- [ ] **Step 5: Prove the materialization branches discriminate individually** + +Three separate mutations, three results — this rule's whole value is that it distinguishes +cases, so a test that passes for two of three is the shape this project has been bitten by +seven times: + +1. Make `incremental` share `table`'s wording. Expected: the incremental test FAILS. +2. Make `view` fall through to the rebuild branch. Expected: the view test FAILS. +3. Make an unrecognised materialization rewrite the DDL anyway. Expected: the exotic test FAILS. + +Restore each. Report all three. + +- [ ] **Step 6: Commit** + +```bash +git add src/sqlquality/workload/dbt.py tests/test_workload_dbt.py +git commit -m "feat(advise): ADV302 -- express index proposals as dbt config, not doomed DDL" +``` + +--- + +### Task 3: ADV301 — a hot model materialized as a view + +**Files:** +- Modify: `src/sqlquality/workload/dbt.py` +- Test: `tests/test_workload_dbt.py` + +**Interfaces:** +- Produces: + `propose_materialization(aggregation: Aggregation, context: DbtContext, *, min_cost_share: float) -> list[Proposal]` + emitting code `"ADV301"`. +- Consumes: `Aggregation` (`usage`, `tables`), `DbtContext`. + +A `view` re-executes its query on every read. When the workload shows a view-materialized model +carrying a hot share of cost, materializing it as a `table` or `incremental` trades build time +for read time. This is only computable by joining cost to the model graph, which is the point. + +Confidence ceiling is **MEDIUM**, never HIGH, and there is deliberately no HIGH branch: the +trade depends on how often the model is rebuilt versus read, and on freshness requirements — +neither visible from query history. Follow ADV008's precedent and say so in the docstring so a +later reader does not add the missing rung for symmetry. + +- [ ] **Step 1: Write the failing tests** + +```python +def _aggregation(*usages, total=1000.0): + return Aggregation( + usage=tuple(usages), + total_cost_ms=total, + skipped_unqualifiable=0, + tables=frozenset(u.relation for u in usages), + ) + + +def test_adv301_proposes_materializing_a_hot_view(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "stg_orders") # view + usage = _usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.4) + proposals = propose_materialization(_aggregation(usage), context, min_cost_share=0.01) + assert [p.code for p in proposals] == ["ADV301"] + assert proposals[0].confidence is Confidence.MEDIUM + assert proposals[0].evidence["dbt_model"] == "model.demo.stg_orders" + assert proposals[0].ddl is None, "changing a materialization is a config edit, not DDL" + + +def test_adv301_is_silent_for_a_model_already_materialized_as_a_table(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.9) + assert propose_materialization(_aggregation(usage), context, min_cost_share=0.01) == [] + + +def test_adv301_respects_the_cost_share_threshold(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "stg_orders"), "status", ColumnRole.EQUALITY, cost_share=0.001) + assert propose_materialization(_aggregation(usage), context, min_cost_share=0.01) == [] + + +def test_adv301_never_reaches_high_confidence(): + """The build-vs-read trade is not visible from query history, so HIGH would be a claim + about a schedule this tool cannot see.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "stg_orders"), "status", ColumnRole.EQUALITY, cost_share=0.99) + [out] = propose_materialization(_aggregation(usage), context, min_cost_share=0.01) + assert out.confidence is Confidence.MEDIUM + + +def test_adv301_ignores_relations_dbt_does_not_manage(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("public", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.9) + assert propose_materialization(_aggregation(usage), context, min_cost_share=0.01) == [] + + +def test_adv301_reports_one_proposal_per_relation_not_per_column(): + """Two hot columns on one view are one materialization decision.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "stg_orders") + proposals = propose_materialization( + _aggregation( + _usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.4), + _usage(relation, "created_at", ColumnRole.RANGE, cost_share=0.3), + ), + context, + min_cost_share=0.01, + ) + assert len(proposals) == 1 + assert proposals[0].evidence["cost_share"] == 0.4, "the max, not the sum" +``` + +- [ ] **Step 2: Run to verify they fail.** Expected: `NameError: propose_materialization`. +- [ ] **Step 3: Implement it.** One proposal per relation, `cost_share` as the max over that + relation's usage (summing double-counts — see `ColumnUsage.cost_share`), sorted by + relation for canonical output. +- [ ] **Step 4: Run the tests.** Expected: PASS. +- [ ] **Step 5: Prove the max-not-sum choice discriminates.** Change `max` to `sum` and confirm + `test_adv301_reports_one_proposal_per_relation_not_per_column` FAILS. Restore, report. +- [ ] **Step 6: Commit** + +```bash +git commit -am "feat(advise): ADV301 -- materialize a hot view-backed dbt model" +``` + +--- + +### Task 4: ADV303 — a dbt model the workload never touched + +**Files:** +- Modify: `src/sqlquality/workload/dbt.py` +- Test: `tests/test_workload_dbt.py` + +**Interfaces:** +- Produces: + `propose_unused_models(aggregation: Aggregation, context: DbtContext, workload: Workload) -> list[Proposal]` + emitting `"ADV303"`. + +A model that costs a build every night and that nothing queries is worth knowing about. But the +evidence here is **absence**, which is much weaker than presence, so this rule needs the loudest +caveat in the codebase and a hard confidence cap of **LOW**: + +- the window may simply not cover the reader (a monthly report, a BI tool with its own cache); +- `--limit` truncates query history, so a cold-but-used model can look unused; +- a model consumed only by *other models* is used, just not by ad-hoc queries — so a model with + dbt children must be excluded outright, not merely downgraded. + +The last point is a correctness gate, not a caveat: excluding models with children is what stops +this rule proposing the deletion of every staging model in a project. + +- [ ] **Step 1: Write the failing tests** + +```python +def test_adv303_flags_a_model_no_query_touched(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + codes = {p.code for p in proposals} + assert codes == {"ADV303"} + flagged = {p.evidence["dbt_model"] for p in proposals} + assert "model.demo.customer_orders" in flagged + + +def test_adv303_excludes_a_model_that_other_models_depend_on(): + """A staging model consumed by a downstream model is used. Without this gate the rule + proposes deleting every staging model in the project.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + flagged = {p.evidence["dbt_model"] for p in proposals} + assert "model.demo.stg_orders" not in flagged, "stg_orders feeds orders" + + +def test_adv303_is_capped_at_low_confidence(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + for proposal in propose_unused_models(_aggregation(usage), context, _workload()): + assert proposal.confidence is Confidence.LOW + + +def test_adv303_states_the_window_caveat_and_the_limit_caveat(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + [first, *_] = propose_unused_models(_aggregation(usage), context, _workload()) + assert "window" in first.rationale + assert "--limit" in first.rationale + + +def test_adv303_emits_nothing_when_every_model_was_touched(): + context = DbtContext.from_project(_project()) + usages = [ + _usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.1) + for relation in context.models + ] + assert propose_unused_models(_aggregation(*usages), context, _workload()) == [] + + +def test_adv303_carries_no_ddl(): + """Deleting a model is a repository change with review implications; the tool must not + hand over a statement that does it.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + for proposal in propose_unused_models(_aggregation(usage), context, _workload()): + assert proposal.ddl is None +``` + +`_workload()` is a minimal `Workload`; reuse the helper style already in +`tests/test_workload_aggregate.py`. + +- [ ] **Step 2: Run to verify they fail.** +- [ ] **Step 3: Implement.** Use `DbtProject.model_children` — the `DbtContext` will need to + carry the child relation, so extend it to keep `child_count: dict[Relation, int]` or hold + the `DbtProject`. Prefer holding what you need rather than the whole project, and say why + in the docstring. +- [ ] **Step 4: Run the tests.** +- [ ] **Step 5: Prove the children gate discriminates.** Remove it and confirm + `test_adv303_excludes_a_model_that_other_models_depend_on` FAILS. Restore, report. +- [ ] **Step 6: Commit** + +```bash +git commit -am "feat(advise): ADV303 -- a dbt model the analysed workload never touched" +``` + +--- + +### Task 5: wire enrichment into the command, and keep the dbt-free path identical + +**Files:** +- Modify: `src/sqlquality/cli.py` +- Modify: `src/sqlquality/report.py` +- Test: `tests/test_advise_cli.py` + +**Interfaces:** +- Produces: `advise` applying `enrich_proposals` and appending ADV301/ADV303 when a context + loaded; `advise_payload` and `render_advise_markdown` carrying the dbt disclosure. + +**The constraint that matters most in this task.** Every `advise` invocation without a manifest +must produce **byte-identical** output to `main` before this branch. Prove it, do not assert it: +capture a run's full stdout, JSON, markdown and DDL on `main`, then on the branch with no dbt +options, and diff them. + +- [ ] **Step 1: Write the failing tests** + +```python +def test_no_manifest_means_no_behaviour_change(tmp_path, monkeypatch): + """The dbt-free path is first-class, so enrichment must be additive by construction.""" + without = _run_advise(tmp_path, extra=[]) + assert "dbt" not in without.stdout.lower() + payload = json.loads(without.stdout) + for proposal in payload["proposals"]: + assert "dbt_model" not in proposal["evidence"] + assert payload["dbt"] is None + + +def test_a_manifest_is_disclosed_on_stderr(): + result = runner.invoke(app, ["advise", ..., "--manifest", str(FIXTURE), "--json"]) + assert "dbt enrichment from" in result.stderr + + +def test_an_unreadable_manifest_does_not_fail_the_run(tmp_path): + """Exit 0 with a disclosure — the catalog work already happened, and dbt is optional.""" + result = runner.invoke(app, ["advise", ..., "--manifest", str(tmp_path / "no.json")]) + assert result.exit_code == 0 + assert "dbt enrichment unavailable" in result.stderr + + +def test_the_payload_records_which_manifest_was_used(): + payload = ... # a --json run with --manifest + assert payload["dbt"]["manifest"].endswith("manifest_v12.json") + assert payload["dbt"]["models"] >= 1 + + +def test_adv301_and_adv303_only_appear_with_a_manifest(): + with_dbt = {p["code"] for p in _payload(extra=["--manifest", str(FIXTURE)])["proposals"]} + without = {p["code"] for p in _payload(extra=[])["proposals"]} + assert not ({"ADV301", "ADV303"} & without) + # And at least one of them appears with the manifest, or this test proves nothing. + assert {"ADV301", "ADV303"} & with_dbt +``` + +- [ ] **Step 2: Run to verify they fail.** +- [ ] **Step 3: Wire it.** Load the context, `enrich_proposals` the adapter's output, extend + with ADV301/ADV303, then re-sort with the adapter's ranking key so ordering stays + canonical. Pass the disclosure into both renderers and add a `"dbt"` key to the payload + (`None` when absent). +- [ ] **Step 4: Prove the no-manifest path is byte-identical** + +```bash +git stash && git checkout main +# run advise against the integration fixture, capturing stdout/json/markdown/ddl +git checkout - && git stash pop +# run the same invocation with no dbt options, and diff every artifact +``` + +Record the diff (which must be empty) in your report. If it is not empty, that is a defect in +this task, not an acceptable change. + +- [ ] **Step 5: Run the whole suite and all four gates.** +- [ ] **Step 6: Commit** + +--- + +### Task 6: prove it against a real project, and document it + +**Files:** +- Modify: `tests/integration/` (a live run with a manifest) +- Modify: `README.md`, `CHANGELOG.md` +- Modify: `docs/superpowers/specs/2026-07-26-advise-workload-analysis-design.md` + +- [ ] **Step 1: Add a live test** that runs `advise` against the seeded Postgres **with** a + manifest whose `relation_name` schemas match the seeded schemas, and asserts that an + index proposal for a dbt-managed table comes out as a config block rather than + `CREATE INDEX`. Include a non-vacuity guard: assert the un-enriched run *did* produce a + `CREATE INDEX` for that relation first, or the test proves nothing. +- [ ] **Step 2: Run the integration suite.** + +Note: host port 55432 collides with an unrelated container on at least one dev machine, in +which case `docker compose up` does not bind and the suite silently talks to whatever else is +listening. If the seeded assertions behave oddly, check `docker ps --filter publish=55432` +before assuming a code defect. + +- [ ] **Step 3: Confirm `uv run pytest` still reports zero skips**, and that the `no-extras` + and `highest-deps` jobs would still pass (the new module must not import psycopg). +- [ ] **Step 4: Document.** README section on dbt enrichment including the ADV302 rationale + (raw DDL on a dbt-managed table does not survive `dbt run`); ADV301/302/303 in the rule + table; `--min-cost-share`'s help text if ADV301 is cost-weighted; CHANGELOG entries; and + a spec deviation recording that matching is on the qualified `(schema, table)` pair with + no bare-name fallback, and why. +- [ ] **Step 5: All four gates, then commit.** + +--- + +## Self-Review + +**Spec coverage.** The three approved dbt deliverables map to Tasks 2, 3 and 4; Task 1 is the +shared foundation, Task 5 the wiring, Task 6 the proof and docs. Snowflake is deliberately out +of scope (deferred pending an account); Redshift is Batch 3b. + +**Placeholder scan.** Task 5's test bodies use `...` for the connection arguments of an +`advise` invocation, because that harness already exists in `tests/test_advise_cli.py` and +copying it here would drift from it. Every other step carries real code. The implementer must +read that file and match its existing stub-adapter fixture rather than invent a second one — +note that fixture's `fake_connect` was recently fixed to stop hard-coding `schemas`, so it is +the current one to follow. + +**Type consistency.** `Relation`, `Proposal`, `Aggregation`, `Workload`, `ModelNode` and +`DbtProject` are all pre-existing and used with their current field names. `DbtContext` gains a +child-count map in Task 4; that is the one shape that changes mid-plan, and Task 4 says so. + +**Known risk this plan accepts.** ADV302 rewrites DDL based on a manifest that may be stale — +someone can change a model's materialization without re-running `dbt compile`. The rule +discloses the materialization it read, so a wrong rewrite is traceable to a stale manifest +rather than invisible. Guarding harder would mean verifying the live relation against the +manifest, which is Batch 3b territory at best. diff --git a/docs/superpowers/specs/2026-07-26-advise-workload-analysis-design.md b/docs/superpowers/specs/2026-07-26-advise-workload-analysis-design.md index 72e3654..5f71bb5 100644 --- a/docs/superpowers/specs/2026-07-26-advise-workload-analysis-design.md +++ b/docs/superpowers/specs/2026-07-26-advise-workload-analysis-design.md @@ -3,11 +3,16 @@ Date: 2026-07-26 Status: Postgres (steps 1–4 below) shipped in `sqlquality advise`, now including ADV007 (join keys), ADV008 (`GROUP BY`), multi-schema `(schema, table)` keying, and -`DECLARE`/`COPY` unwrapping (Batch 2, 2026-07-27); Redshift, Snowflake and dbt enrichment -(steps 5–7) remain design-only, not yet implemented. See -`docs/superpowers/plans/2026-07-26-advise-postgres.md` for the implementation plan and its -"Deviations from the spec" section, reconciled into this document below, and "Deviations -from the spec (Batch 2)" further down for what changed after the initial ship. +`DECLARE`/`COPY` unwrapping (Batch 2, 2026-07-27); optional dbt enrichment (ADV301–ADV303, +`--project-dir`/`--manifest`) shipped Batch 3a, 2026-07-28, with a code reassignment from +this document's original dbt section — see "Deviations from the spec (Batch 3a: dbt +enrichment)" below. Redshift and Snowflake (steps 5–6) remain design-only, not yet +implemented. See `docs/superpowers/plans/2026-07-26-advise-postgres.md` for the Postgres +implementation plan and its own "Deviations from the spec" section, reconciled into this +document below; `docs/superpowers/plans/2026-07-27-advise-dbt-enrichment.md` for the dbt +enrichment plan; "Deviations from the spec (Batch 2)" further down for what changed after +the initial Postgres ship; and "Deviations from the spec (Batch 3a: dbt enrichment)" for +what changed while building dbt enrichment. ## Summary @@ -77,7 +82,9 @@ src/sqlquality/workload/ postgres.py introspection SQL + index rules + DDL rendering redshift.py introspection SQL + table-design rules + DDL rendering snowflake.py introspection SQL + clustering rules + DDL rendering - dbtenrich.py ADV301-303, active only when --manifest is supplied + dbt.py ADV301-303, active only when --project-dir/--manifest is supplied + (shipped filename; see Batch 3a deviation #2 below for why this module + is imported from cli.py only, never from an adapter) ``` `extract.py`, `aggregate.py` and `fingerprint.py` are engine-agnostic and hold the bulk of @@ -500,13 +507,145 @@ Clustering carries ongoing credit cost, so ADV201 and ADV203 must state that the recommendation itself has a price — unlike an index, it is not a one-time cost. `SYSTEM$CLUSTERING_INFORMATION` consumes compute and is therefore not called by default. -### dbt enrichment (`--manifest`) +### dbt enrichment (`--project-dir` / `--manifest`) + +Shipped in Batch 3a (2026-07-28), with a code reassignment from what this section +originally specified — see "Deviations from the spec (Batch 3a: dbt enrichment)" below for +why. | Code | Proposal | Note | |---|---|---| -| ADV301 | Hot table maps to a model materialized as `view` → propose `table` or `incremental` | cost share attributed to the model | -| ADV302 | Model never referenced in the window → dead-model candidate | permanently LOW confidence: BI tools, longer windows and downstream-only models all hide usage | -| ADV303 | Recurring join path across a large cost share, all tables mapping to models → propose a mart | fingerprint count | +| ADV301 | Hot table maps to a model materialized as `view` → propose `table` or `incremental` | cost share attributed to the model; capped at MEDIUM | +| ADV302 | An index-creating proposal for a `table`/`incremental`/`materialized_view` dbt model is rewritten into a config block instead of DDL that a normal (or `--full-refresh`) `dbt run` would destroy; on a `view` the *DDL* is dropped and explained and the proposal survives at LOW. Not a proposal code: the original rule keeps its own code — see deviation 5 | dbt materialization, columns | +| ADV303 | Model never referenced in the analyzed window, and no other model, snapshot or dbt exposure declares it as a consumer → dead-model candidate. Only *immediate* consumers count, so a dead chain unwinds one model per run from its leaf | permanently LOW confidence: BI tools, longer windows, `--limit` truncation and downstream-only models all hide usage | + +The originally-specified "recurring join path across models → propose a mart" rule is out +of scope for Batch 3a; nothing in the shipped code claims that code or that behavior. + +## Deviations from the spec (Batch 3a: dbt enrichment) + +Found and agreed while implementing the dbt enrichment this document's "dbt enrichment" +subsection above originally specified. + +1. **Code reassignment: ADV302 is the DDL-correctness rewrite, not "dead-model + candidate."** The spec as written gave ADV301 the hot-view-materialization proposal, + ADV302 the dead-model proposal, and ADV303 a join-path/mart proposal. While scoping the + implementation it became clear the highest-value rule — the one that justified doing + dbt enrichment *before* Redshift/Snowflake in this batch — is neither of those: it is + recognizing that a raw `CREATE INDEX` proposal for a dbt-managed `table` (or + `incremental`, or `materialized_view`) relation is *actively wrong* advice, because + `dbt run` drops and recreates that relation (or, for `incremental`, `--full-refresh` + does), silently destroying the index the next time the pipeline runs. Every other + dbt-enrichment behavior is *additive* (a proposal that would not otherwise exist); + this one is *corrective* (a proposal the tool already made, made safe). That + asymmetry — correctness fix vs. new proposal — is why it took the lower, more + prominent number: **ADV302** is the rewrite/config-block rule, and the dead-model + rule this section originally called ADV302 shipped as **ADV303** instead. ADV301 + (materialize a hot view) is unchanged from the original spec. The join-path/mart rule + originally slotted at ADV303 was dropped from this batch's scope entirely (see the + table above) rather than renumbered again, since a fourth code with no implementation + behind it would just be a dangling promise. +2. **dbt is layered strictly on top of the engine-agnostic core — no adapter imports + it.** `sqlquality.workload.dbt` is imported from exactly one place, `cli.py`, which + calls `enrich_proposals`/`propose_materialization`/`propose_unused_models` once, after + `adapter.propose()` has already returned and been re-sorted with the adapter's own + ranking key — `adapter.ranking_key`, a public hook on the `WorkloadAdapter` ABC that each + adapter may override, resolved off the instance the CLI resolved. (It first shipped as + `PostgresWorkloadAdapter._ranking_key` reached into directly from `cli.py`, which made + this claim false for any second engine: it would have got Postgres's ordering on the dbt + path and its own everywhere else. Corrected before merge; the CLI now imports no adapter + at all.) `PostgresWorkloadAdapter` (and, when it ships, the Redshift adapter) has + no knowledge that dbt enrichment exists. This was a design goal restated in the + architecture section above, not a deviation from it — recorded here because it was + verified by grep (`sqlquality.workload.dbt` appears nowhere under `workload/postgres.py` + or any other adapter) at the end of every task in this batch, not merely assumed. +3. **Matching a dbt model to a relation is on the qualified `(schema, table)` pair, with + deliberately no bare-table-name fallback.** `DbtContext.model_for` looks up + `self.models.get(relation)` and nothing else. A dbt project's target schema — `dev`, + `main`, a CI schema, whatever `profiles.yml` names — routinely differs from the schema + `advise` introspects in production. A name-only match (ignore the manifest's schema, + match on table name alone) would therefore attribute a production table's proposal to + whatever development-schema model happens to share its table name, and ADV302 would + then rewrite that production table's DDL into a config block on the strength of a + guess about the wrong model's materialization — the exact class of silent + misattribution this whole batch exists to avoid, not introduce. The cost of this + strictness: a relation that two *different* models both build (legitimate when a + project targets more than one database, since dbt's `relation_name` carries a + database segment this project drops) cannot be disambiguated from the manifest alone, + so it is dropped from the index entirely rather than resolved by dict-insertion-order + luck, and counted in `DbtContext.dropped_collisions` — surfaced in both the CLI's `dbt + enrichment from ...` disclosure line and the JSON payload's `dbt.dropped_collisions`. +4. **The no-manifest path is byte-identical to a build with no dbt support at all, proven + by measurement, not asserted.** Neither `--project-dir` nor `--manifest` given means + `load_dbt_context` returns `(None, None)` and none of the enrichment functions run, so + `advise` without a manifest is, by construction, the same code path as before this + batch. This was verified, not just argued: `stdout` (`--json`), the markdown report, + the `--ddl` file and `stderr` were each diffed byte-for-byte against `main` twice — + once with a stubbed adapter (deterministic, no live DB) and once against a live, + seeded Postgres — and all four came back an empty diff both times. Getting to a + literal empty diff required one interface decision: `advise_payload`'s `dbt` key is + *omitted from the JSON payload entirely* when no manifest loaded, rather than emitted + as `"dbt": null` — the latter is a schema addition relative to `main` that would have + made "byte-identical" true only with an asterisk. A consumer that wants the key + unconditionally still has `payload.get("dbt")`. Guarded by a test that renders all four + artifacts from a fixed, non-vacuous workload and asserts every dbt-conditional element is + absent from each — a diff against another commit is not something the suite can do, but + the absence property is, and the byte-identity claim was otherwise unprotected against + regression. +5. **ADV302 is a rewrite, not a proposal `code`.** No `Proposal` ever carries + `code="ADV302"`: `_enrich_one` replaces `ddl` and amends `rationale`, and the original + rule (ADV001/ADV004/ADV007/ADV008) keeps its code, confidence and cost share, since the + evidence for the index is unchanged — only the delivery mechanism is. Consequences, + recorded because they surprise a consumer: a `--json` filter on `code == "ADV302"` matches + nothing on every run (filter on `evidence.dbt_index_config` instead), and the terminal + table row is identical to the same proposal from a dbt-free run, so `advise` prints a + stderr line naming how many proposals were rewritten. +6. **One model gets one `indexes:` block.** dbt reads a single `indexes` key per model + config, so two standalone blocks pasted into one config are a duplicate YAML mapping key + and PyYAML — dbt's parser — silently keeps one, discarding the other recommended index. + Multiple index proposals per relation is the ordinary case (the collapse layer never folds + non-prefix column lists, and deliberately preserves same-set-different-order pairs), so + `enrich_proposals` merges every index for one model into the block carried by the + highest-ranked of those proposals; the others point at it by code. +7. **ADV302's `indexes:` config shape is postgres/redshift-specific, and is disclosed rather + than suppressed elsewhere.** dbt implements the `indexes` model config only on those two + adapters. `advise` warns on stderr when the manifest's `adapter_type` is something else, + and when the manifest is not a v12 schema — the same two checks `check` makes on the same + file. It still emits the rewrite: the alternative is raw DDL the same rebuild destroys, so + declining would leave the operator less informed, and `advise` connects only to Postgres + today, so this configuration is already a mismatch worth naming rather than working + around. +8. **A caveat that qualifies an executable statement is written into the `--ddl` script.** + `render_ddl` emits only the code/confidence header, the title and the DDL — never + `rationale` — so ADV302's decline paths (partial index, unrecognised materialization, no + plain column list, non-btree access method) produced a file holding a config block that + explained raw DDL does not survive `dbt run` and, below it, a bare `CREATE INDEX` on that + same dbt-managed table. `Proposal` grew an optional `note` field that `render_ddl` emits as + comment lines above the statement; it is deliberately absent from the JSON payload and the + markdown report, both of which already carry `rationale`, so the pre-dbt payload shape is + unchanged. +9. **A `DROP INDEX` for a dbt-managed relation is disclosed, not exempted.** The rewrite + branch originally exempted drops outright, reasoning that "dbt never created this index, so + dropping it is ordinary." That is false in exactly the case ADV302 exists for: if the index + *is* declared in the model's `indexes:` config, the next `dbt run` recreates it — the + operator drops it, dbt puts it back, and ADV002/ADV003 propose the same drop again next run. + The same silently-reverting advice, pointing the other way, and reachable through the + ordinary rules rather than only in principle. The proposal is kept (dropping a genuinely + unused index is still right, and dbt's `indexes` config cannot express a removal) and gains + the warning in both `rationale` and `note`, so the operator gets both halves of the + instruction. The property the DDL script now holds is stated statement-wise, not + relation-wise: no executable line for a dbt-managed relation without an adjacent warning. + The test that first checked this filtered blocks on the *table* name and so skipped every + drop, since a `DROP INDEX` names an index. +10. **A manifest inconsistent with the connection is warned about, and "absent" warns too.** + `advise` connects to Postgres; an `adapter_type` outside `{postgres, redshift}` means dbt + is not building the relations just introspected at all, so every `(schema, table)` match is + a name coincidence and ADV301/ADV302/ADV303 are all wrong — a stronger statement than + "ADV302's `indexes` config key may not exist there," which was the first wording. A + manifest recording *no* `adapter_type` warns as well, deliberately: `dbt compile` always + writes one, and warning on "different" while staying silent on "unknown" would make silence + mean either "consistent" or "unchecked". Warned rather than suppressed, since the fix is in + the user's invocation and dropping all dbt output would hide it. ## Confidence model @@ -515,7 +654,7 @@ Mechanical, derived from inputs rather than judgment: - **HIGH** — cost share above threshold, **and** supporting catalog stats present, **and** the current physical state confirmed to lack the proposal. - **MEDIUM** — cost evidence solid, but a catalog input is missing or stale. -- **LOW** — absence-based (ADV302) or thin evidence. +- **LOW** — absence-based (ADV303) or thin evidence. Every proposal renders its inputs inline: cost share, calls, distinct fingerprints, row estimate, NDV, and current index/DISTKEY/clustering state. A reader must be able to diff --git a/src/sqlquality/cli.py b/src/sqlquality/cli.py index f12b3ca..af98327 100644 --- a/src/sqlquality/cli.py +++ b/src/sqlquality/cli.py @@ -46,6 +46,14 @@ from sqlquality.workload.aggregate import aggregate, star_tables from sqlquality.workload.base import MAX_TIMEOUT_S, MIN_TIMEOUT_S from sqlquality.workload.connection import ConnectionResolutionError, resolve_connection +from sqlquality.workload.dbt import ( + describe_rewrites, + enrich_proposals, + load_dbt_context, + propose_materialization, + propose_unused_models, + resolve_manifest_path, +) from sqlquality.workload.fingerprint import ingest console = Console() @@ -724,6 +732,14 @@ def advise( profiles_dir: Path | None = typer.Option( None, "--profiles-dir", help="Directory holding profiles.yml (default: ~/.dbt)." ), + project_dir: Path | None = typer.Option( + None, + "--project-dir", + help="dbt project dir; reads target/manifest.json to enrich proposals (optional).", + ), + manifest: Path | None = typer.Option( + None, "--manifest", help="Path to a dbt manifest.json. Overrides --project-dir." + ), schema: list[str] = typer.Option( ["public"], "--schema", @@ -744,9 +760,12 @@ def advise( # share they do not have would be inventing evidence. help=( "Suppress proposals below this share of workload cost. Applies to the " - "cost-weighted rules (ADV001, ADV004, ADV005, ADV006, ADV007, ADV008); the " - "index-hygiene rules ADV002 and ADV003 carry no cost evidence and are always " - "reported." + "cost-weighted rules (ADV001, ADV004, ADV005, ADV006, ADV007, ADV008, ADV301 " + "-- the last only with --project-dir/--manifest); the index-hygiene rules " + "ADV002 and ADV003, and ADV303 (its evidence is absence, not cost, so there is " + "no share to threshold), carry no cost evidence and are reported whatever the " + "threshold. ADV303 has its own non-threshold suppression: it emits nothing when " + "no query usage could be extracted at all." ), ), keep_literals: bool = typer.Option( @@ -840,6 +859,54 @@ def advise( ) proposals = adapter.propose(aggregation, facts, workload, min_cost_share=min_cost_share) + # Optional dbt enrichment: neither option given means (None, None) and nothing below + # fires, so every existing `advise` invocation behaves identically without a manifest — + # that identity is proved byte-for-byte in tests/test_advise_cli.py and is the + # constraint this whole block exists to honour. + dbt_context, dbt_disclosure = load_dbt_context(project_dir, manifest) + if dbt_disclosure is not None: + typer.echo(dbt_disclosure, err=True) + + dbt_payload: dict | None = None + if dbt_context is not None: + # Rewrite index-creating proposals for dbt-managed relations (ADV302), then add + # ADV301 (materialize a hot view) and ADV303 (a model the workload never touched). + # `enrich_proposals` and both `propose_*` calls return an *unsorted-relative-to- + # each-other* concatenation, so the combined list is re-sorted with the adapter's + # own ranking key: without this, a proposal `enrich_proposals` downgrades (e.g. a + # view it strips DDL from, dropping it to LOW) would keep its old, now-wrong + # position, and the terminal table, the markdown and the DDL file would each see a + # different order depending on which pass touched them last. + proposals = enrich_proposals(proposals, dbt_context) + proposals = proposals + propose_materialization( + aggregation, dbt_context, min_cost_share=min_cost_share + ) + proposals = proposals + propose_unused_models(aggregation, dbt_context, workload) + # `adapter.ranking_key`, not one specific adapter's: ordering is each adapter's own + # responsibility, and reaching into `PostgresWorkloadAdapter` here meant a future + # engine would silently get Postgres's ordering on the dbt path while keeping its + # own everywhere else. + proposals = sorted(proposals, key=adapter.ranking_key) + + # ADV302 is a rewrite, not a proposal code: an enriched row in the table above is + # byte-identical to the same proposal from a dbt-free run, and the terminal never + # prints `rationale`. Without this line a terminal-only user cannot tell that + # enrichment fired at all. + rewrite_note = describe_rewrites(proposals) + if rewrite_note is not None: + typer.echo(rewrite_note, err=True) + + # The same resolution `load_dbt_context` itself used, so the path disclosed here is + # exactly the one it loaded — one shared function rather than a second copy of the + # precedence, which could be (and was) changed in one place only. + resolved_manifest = resolve_manifest_path(project_dir, manifest) + assert resolved_manifest is not None # dbt_context is only ever set when one was given + dbt_payload = { + "manifest": str(resolved_manifest), + "models": len(dbt_context.models), + "dropped_collisions": dbt_context.dropped_collisions, + } + payload = advise_payload( proposals, workload, @@ -847,6 +914,7 @@ def advise( engine=params.engine, redacted=not keep_literals, degraded=adapter.degraded, + dbt=dbt_payload, ) # Both writes happen after the whole analysis, so an unwritable path would otherwise # discard the work *and* exit 1 — the code the epilog reserves for "findings or gate @@ -872,6 +940,7 @@ def advise( engine=params.engine, redacted=not keep_literals, degraded=adapter.degraded, + dbt=dbt_payload, ) try: markdown.write_text(markdown_text, encoding="utf-8") diff --git a/src/sqlquality/dbtproject.py b/src/sqlquality/dbtproject.py index 4821734..5ac658d 100644 --- a/src/sqlquality/dbtproject.py +++ b/src/sqlquality/dbtproject.py @@ -83,6 +83,19 @@ def model_parents(self, uid: str) -> list[str]: def model_children(self, uid: str) -> list[str]: return sorted(c for c in self._child_map.get(uid, []) if self._is_model(c)) + def child_ids(self, uid: str) -> list[str]: + """Every declared consumer of `uid`, with no resource-type filter at all. + + `model_children` narrows to models only, which is right for its own callers (the + model DAG). A caller asking "is anything declared to consume this?" wants the + opposite: a snapshot or an exposure is a real, dbt-declared consumer — an exposure + exists specifically to say "a BI dashboard reads this" — and `model_children`'s + filter would make either invisible. Returns raw `child_map` unique_ids so a caller + can apply its own notion of "consumer" (see `workload.dbt.propose_unused_models`, + which excludes tests from that notion but nothing else). + """ + return sorted(self._child_map.get(uid, [])) + def compiled_sql(self, uid: str) -> str: node = self.node(uid) if not node.compiled_code: diff --git a/src/sqlquality/models.py b/src/sqlquality/models.py index 23c7545..2fd93ec 100644 --- a/src/sqlquality/models.py +++ b/src/sqlquality/models.py @@ -218,6 +218,26 @@ class Proposal: evidence: dict[str, object] confidence: Confidence ddl: str | None = None + #: A caveat that must travel *with the statement*, rendered as comment lines directly + #: above `ddl` in the DDL script. + #: + #: This exists because `rationale` does not reach the DDL script at all — only the code, + #: confidence, cost share and title do. So a caveat that only lives in `rationale` is + #: invisible to precisely the person acting on the statement, which is how a `--ddl` file + #: came to hold a dbt config block explaining that raw DDL is destroyed by `dbt run` and, + #: below it, a raw `CREATE INDEX` on that same dbt-managed table. Anything an operator + #: must know *before running this statement* belongs here as well as in `rationale`. + #: + #: Deliberately a separate field rather than comment lines prepended to `ddl`: prepending + #: makes `ddl` multi-line but not *fully* commented, which routes it into `render_ddl`'s + #: NOT-RENDERED fallback and prints a reason ("an identifier contains a line break") that + #: is false for it. Keeping `ddl` a pure statement also keeps it engine-agnostic — how a + #: note is rendered is the adapter's business, not the rule's. + #: + #: Deliberately *not* part of the JSON payload or the markdown report: both already carry + #: `rationale`, which says the same thing in prose, and adding a key that is `None` on + #: every dbt-free run would break the byte-identity of the pre-dbt payload for no gain. + note: str | None = None def analyzed_query_groups(workload: Workload, aggregation: Aggregation) -> int: diff --git a/src/sqlquality/report.py b/src/sqlquality/report.py index 3cc8c05..e49994a 100644 --- a/src/sqlquality/report.py +++ b/src/sqlquality/report.py @@ -143,9 +143,22 @@ def advise_payload( engine: str, redacted: bool, degraded: list[tuple[str, str]], + dbt: dict | None = None, ) -> dict: - """JSON-serializable summary of an advise run.""" - return { + """JSON-serializable summary of an advise run. + + The `"dbt"` key is *omitted entirely* when `dbt` is `None` (the default, and what every + existing caller before this key existed still gets) rather than present with a `None` + value: the dbt-free path is first-class, and this is what lets a no-manifest `advise` + invocation stay byte-identical to the payload from before dbt enrichment existed, not + merely equal apart from one known extra key. A consumer wanting the manifest count + unconditionally can still do `payload.get("dbt")`, which behaves the same either way. + When a manifest did load, the caller (`cli.advise`) is responsible for handing in a + plain, JSON-serializable dict (a `Path` or a `Relation` is not — `cli.advise` already + stringifies the manifest path before building this dict), since this function does not + itself normalize it the way `_jsonable` normalizes proposal evidence. + """ + payload = { "engine": engine, "redacted": redacted, "window": workload.window_description, @@ -180,6 +193,9 @@ def advise_payload( for p in proposals ], } + if dbt is not None: + payload["dbt"] = dbt + return payload def _jsonable(value: object) -> object: @@ -199,8 +215,14 @@ def render_advise_markdown( engine: str, redacted: bool, degraded: list[tuple[str, str]], + dbt: dict | None = None, ) -> str: - """Render advise proposals as markdown (suitable for a ticket or PR comment).""" + """Render advise proposals as markdown (suitable for a ticket or PR comment). + + `dbt` defaults to `None` — every existing caller omits it — so a no-manifest run + renders exactly the markdown it always has; the section below only appears when a + manifest actually loaded. + """ lines = [ f"# sqlquality advise — {_md_escape(engine)}", "", @@ -235,6 +257,15 @@ def render_advise_markdown( lines.append(f"- `{_md_escape(capability)}`: {_md_escape(reason)}") lines.append("") + if dbt is not None: + lines.append("## dbt enrichment") + lines.append("") + lines.append(f"- manifest: `{_md_escape(dbt['manifest'])}`") + lines.append(f"- models indexed: {dbt['models']}") + if dbt.get("dropped_collisions"): + lines.append(f"- cross-database collisions dropped: {dbt['dropped_collisions']}") + lines.append("") + if not proposals: lines.append("No proposals — nothing in the analyzed workload met the thresholds.") return "\n".join(lines) + "\n" diff --git a/src/sqlquality/workload/base.py b/src/sqlquality/workload/base.py index 09f4c1c..0ffea88 100644 --- a/src/sqlquality/workload/base.py +++ b/src/sqlquality/workload/base.py @@ -17,12 +17,14 @@ from sqlquality.models import ( Aggregation, + Confidence, ConnectionParams, Proposal, Relation, TableFacts, Workload, WorkloadFetch, + cost_share_of, ) #: Executes one parameterized introspection statement and returns its rows. @@ -61,6 +63,36 @@ def __init__(self) -> None: #: Schema(s) to introspect. The CLI overwrites this from --schema before connect(). self.schemas: tuple[str, ...] = ("public",) + #: Highest confidence first, then largest cost share — the reading order a human wants. + _CONFIDENCE_ORDER = {Confidence.HIGH: 0, Confidence.MEDIUM: 1, Confidence.LOW: 2} + + @classmethod + def ranking_key(cls, proposal: Proposal) -> tuple[int, float, str, str]: + """Canonical presentation order for proposals *this* adapter produced. + + Public, and on the ABC, because ordering is each adapter's own responsibility and a + caller outside the adapter legitimately needs it: `cli.advise` re-sorts after the + optional dbt enrichment layer appends ADV301/ADV303 and downgrades some proposals, + and without a hook it reached into one specific adapter's private classmethod — so a + future engine would silently have got Postgres's ordering on the dbt path only, while + keeping its own everywhere else. An adapter whose proposals want a different reading + order overrides this; the default is the ordering every adapter has wanted so far. + + Highest confidence first, then largest cost share, then a canonical tiebreak so + equal-confidence equal-cost proposals do not reorder between runs and make the CLI's + tests flaky. + + `cost_share_of` rather than `float(evidence.get(...))`: bool is an int subclass, so a + stray True became -1.0 and sorted a fabricated share ahead of a genuinely hot + proposal, at the top of the list the CLI presents as "read this first". + """ + return ( + cls._CONFIDENCE_ORDER[proposal.confidence], + -(cost_share_of(proposal.evidence) or 0.0), + proposal.code, + proposal.title, + ) + @abstractmethod def introspection_sql(self) -> list[IntrospectionStatement]: """Every statement this adapter can run. Backs --dry-run.""" diff --git a/src/sqlquality/workload/dbt.py b/src/sqlquality/workload/dbt.py new file mode 100644 index 0000000..8662b97 --- /dev/null +++ b/src/sqlquality/workload/dbt.py @@ -0,0 +1,1048 @@ +"""Optional dbt enrichment for `advise`. + +dbt is *layered on top of* the engine-agnostic core, never underneath it: no workload adapter +imports this module, and every `advise` run behaves identically without a manifest. The +project's positioning is that the dbt-free path is first-class, so enrichment has to be +additive by construction rather than by discipline. +""" + +from __future__ import annotations + +import dataclasses +import re +from collections import defaultdict +from dataclasses import dataclass, field +from pathlib import Path + +from sqlquality.dbtproject import DbtProject, DbtProjectError, ModelNode +from sqlquality.models import Aggregation, ColumnUsage, Confidence, Proposal, Relation, Workload + + +def _split_relation_parts(text: str) -> list[str] | None: + """Tokenize a dot-separated, optionally double-quoted identifier list. + + A hand-written scanner rather than a regex `findall`: `findall` silently *skips* any + character that matches neither alternative — which is exactly how a stray unescaped + quote, an empty quoted segment (`""`), or a trailing dot with nothing after it used to + disappear instead of being rejected, shifting every later part one slot to the left (a + `catalog.schema` name silently misread as `schema.table`). This scanner instead returns + `None` the moment the input can't be tiled into parts with nothing left over, so a + malformed name always declines rather than mis-parsing. + """ + parts: list[str] = [] + i, n = 0, len(text) + while i < n: + if text[i] == '"': + j = i + 1 + buf: list[str] = [] + closed = False + while j < n: + if text[j] == '"': + if j + 1 < n and text[j + 1] == '"': + # A doubled quote is SQL's escape for a literal `"` inside a + # quoted identifier — not the end of the segment. + buf.append('"') + j += 2 + continue + closed = True + j += 1 + break + buf.append(text[j]) + j += 1 + if not closed: + return None # unterminated quote + part = "".join(buf) + i = j + else: + start = i + while i < n and text[i] not in '."': + i += 1 + part = text[start:i] + if i < n and text[i] == '"': + return None # a quote appearing mid-bare-segment is not a valid name + if not part: + return None # an empty segment (`""` or two dots in a row) can't be qualified + parts.append(part) + if i < n: + if text[i] != ".": + return None + i += 1 + if i >= n: + return None # a trailing dot with nothing after it is malformed + return parts + + +def parse_relation_name(relation_name: str) -> Relation | None: + """`(schema, table)` from a dbt `relation_name`, or None if it cannot be qualified. + + dbt writes a quoted three-part name — `'"dev"."main"."stg_orders"'` — and the raw node's + own `schema` field is `None` in practice, so this string is the only reliable source. The + database part is dropped because `advise` connects to one database at a time. + + A name with fewer than two parts, or one that cannot be cleanly tokenized (an empty + segment, an unterminated quote, a trailing dot), returns None rather than a guess: a + `Relation` needs an exact schema, and inventing one — or shifting onto the wrong part + because a malformed segment silently vanished — is how a production table gets + attributed to an unrelated model. + """ + text = relation_name.strip() + if not text: + return None + parts = _split_relation_parts(text) + if parts is None or len(parts) < 2: + return None + return Relation(schema=parts[-2], table=parts[-1]) + + +def _is_test_id(unique_id: str) -> bool: + """Whether `unique_id` names a dbt test, by dbt's own unique_id convention. + + A test's unique_id is always `test...` — this is dbt's own + naming scheme, the same one that makes `unique_id.startswith("model.")` reliable + elsewhere in this module. Checking the id rather than resolving a node is deliberate: + an exposure or a source referenced in `child_map` may not even have an entry in + `manifest["nodes"]` (both live in their own top-level manifest sections), so a lookup + through `DbtProject.node` would raise for exactly the consumers this check must not + reject. + """ + return unique_id.split(".", 1)[0] == "test" + + +@dataclass(frozen=True) +class DbtContext: + """dbt models indexed by the relation they build, for joining against workload facts.""" + + models: dict[Relation, ModelNode] + #: Relations dropped because two *different* models claimed the same (schema, table) — + #: see `from_project`. Surfaced so the CLI disclosure can tell a user "we found nothing" + #: apart from "we found two candidates and refused to guess." + dropped_collisions: int = 0 + #: How many other *declared consumers* — anything in the manifest's child_map except a + #: test — the model building this relation has, keyed the same way as `models`. ADV303 + #: needs this to exclude a model that only looks unused because nothing but another + #: dbt-declared consumer reads it. + #: + #: Deliberately **not** `DbtProject.model_children`'s count: that method filters to + #: `resource_type == "model"`, which is correct for its own callers (the model DAG) but + #: wrong here — a snapshot or an exposure is a real, dbt-declared consumer (an exposure + #: exists specifically to say "a BI dashboard reads this"), and a model whose only child + #: is one of those is used, just not by another model. `from_project` instead counts + #: `DbtProject.child_ids`, which reads the manifest's child_map with no resource-type + #: filter, and excludes only `test.*` ids: a `not_null` test is an assertion *about* a + #: model, not a consumer *of* it, so it must not count toward "something reads this." + #: + #: Holding the whole `DbtProject` just to ask this on demand would let dbt-shaped + #: knowledge (unique_ids, the child map) leak past this module's boundary into whatever + #: calls `DbtContext.model_for` today. Carrying only the count keeps `DbtContext` a plain + #: fact about relations, the same shape `model_for` already promises. + consumer_count: dict[Relation, int] = field(default_factory=dict) + + @classmethod + def from_project(cls, project: DbtProject) -> DbtContext: + """Index every model by the relation it builds. + + `project.model_ids()` already filters to `resource_type == "model"`, so seeds, + tests and snapshots never reach this loop — there is nothing left here to + re-check that against. A model with no `relation_name` *is* skipped, and that + guard is reachable: an ephemeral materialization is inlined as a CTE and never + occupies a physical relation, so dbt leaves its `relation_name` unset. + + Two different models can each build a relation with the same `(schema, table)` + in two different databases — `'"prod"."main"."orders"'` and + `'"stage"."main"."orders"'` both key `Relation("main", "orders")` once the + database is dropped. `advise` connects to one database at a time, so there is no + way to tell which model is the right one. Rather than let dict insertion order + silently pick a winner, a colliding relation is dropped from the index entirely + (and counted in `dropped_collisions`): an unmatched relation reads as "we + couldn't tell," not as a guess that a later rule then rewrites DDL on the + strength of. + """ + candidates: dict[Relation, ModelNode] = {} + collided: set[Relation] = set() + for uid in project.model_ids(): + node = project.node(uid) + if not node.relation_name: + continue + relation = parse_relation_name(node.relation_name) + if relation is None: + continue + existing = candidates.get(relation) + if existing is not None and existing.unique_id != node.unique_id: + collided.add(relation) + continue + candidates[relation] = node + models = {r: n for r, n in candidates.items() if r not in collided} + consumer_count = { + r: sum(1 for cid in project.child_ids(n.unique_id) if not _is_test_id(cid)) + for r, n in models.items() + } + return cls(models=models, dropped_collisions=len(collided), consumer_count=consumer_count) + + def model_for(self, relation: Relation) -> ModelNode | None: + """The model building this exact relation, matching schema *and* table. + + Deliberately no bare-table-name fallback: dbt's `main`/`dev` target schemas routinely + differ from the schema `advise` introspects, so a name-only match would attribute a + production table to an unrelated development model — and then, via ADV302, rewrite + that table's DDL on the strength of it. + """ + return self.models.get(relation) + + +def resolve_manifest_path(project_dir: Path | None, manifest: Path | None) -> Path | None: + """The manifest path `load_dbt_context` will read, or None if neither option was given. + + An explicit `--manifest` wins; otherwise `--project-dir/target/manifest.json`; otherwise + neither was given. One function with two callers, deliberately: `load_dbt_context` + resolves the path it loads, and `cli.advise` reports which path *was* loaded in its JSON + payload. Those were two independent copies of this same precedence, and swapping the + order in either copy alone left the whole suite green — so the payload could name a + manifest that was never read. + """ + if manifest is not None: + return manifest + if project_dir is not None: + return project_dir / "target" / "manifest.json" + return None + + +#: dbt adapters that could plausibly be building the relations `advise` introspects. `advise` +#: connects to Postgres only, and redshift is its derivative (same `CREATE INDEX`, same +#: `indexes` model config), so a manifest naming either is consistent with the connection. +#: Anything else names a different warehouse entirely — see `_manifest_warnings`. +_CONSISTENT_ADAPTERS = frozenset({"postgres", "redshift"}) + + +def _manifest_warnings(project: DbtProject) -> list[str]: + """The two manifest checks `check` makes and the dbt `advise` path did not. + + `check` warns on a non-v12 `dbt_schema_version` and resolves its dialect from + `adapter_type`; `advise` read neither, so it silently accepted a v10/v11 manifest whose + node shapes it reads as if they were v12, and said nothing at all when the manifest + described a different warehouse from the one it had just connected to. + + **What a foreign `adapter_type` actually means.** Not merely "ADV302 might emit a config + key that adapter lacks" — the deeper problem is that dbt is then not building the Postgres + relations `advise` just introspected *at all*. A Snowflake manifest paired with a Postgres + connection means every `(schema, table)` match is a coincidence of naming: ADV302's + premise (a `dbt run` rebuilds this relation, so raw DDL does not survive) is false, + ADV301 attributes Postgres cost to a model that builds a Snowflake table, and ADV303 calls + a Postgres relation an unused dbt model. So the warning is about the pairing, not about one + config key. + + Warn rather than suppress. Two commands reading the same file and disagreeing about + whether it is even the right shape is what a user of both would not expect, and the + mismatch is something the user has to fix in their invocation — silently dropping all dbt + output would hide the very thing they need told. For ADV302 specifically, its alternative + to a config block is raw DDL that a rebuild destroys, so declining the rewrite would leave + the operator *less* informed, not more. + + **An absent `adapter_type` warns too, deliberately.** `dbt compile` always writes one, so + absence means a hand-written or truncated manifest, and the honest statement is that the + pairing cannot be checked rather than that it is fine. Warning on "different" while + staying silent on "unknown" would make silence mean two different things. `check` makes + the same distinction, disclosing "manifest adapter_type absent or unrecognized" rather + than assuming. + + Both values are `isinstance`-guarded because `metadata` is a section some other tool + wrote: a non-string version would otherwise raise from the `in` test, and this function + runs where a raise degrades the whole enrichment. + """ + warnings: list[str] = [] + schema_version = project.schema_version() + if not isinstance(schema_version, str) or "/v12" not in schema_version: + found = schema_version if schema_version else "(absent)" + warnings.append( + f"warning: manifest dbt_schema_version is {found}, expected a v12 schema — " + "dbt enrichment may be unreliable" + ) + adapter_type = project.adapter_type() + if not isinstance(adapter_type, str) or not adapter_type: + warnings.append( + "warning: manifest records no adapter_type, so it cannot be confirmed that these " + "models build the relations being introspected — dbt enrichment assumes they do" + ) + elif adapter_type not in _CONSISTENT_ADAPTERS: + warnings.append( + f"warning: manifest adapter_type is {adapter_type} but advise connects to " + "postgres, so these models do not build the relations being introspected — every " + "dbt match is a name coincidence and ADV301/ADV302/ADV303 will be wrong" + ) + return warnings + + +def load_dbt_context( + project_dir: Path | None, manifest: Path | None +) -> tuple[DbtContext | None, str | None]: + """Load a manifest if one was requested, returning `(context, disclosure)`. + + Never raises. A manifest that is missing, unreadable or malformed degrades to "no + enrichment" plus a line for the user, because by the time this runs the whole catalog + analysis has already happened — aborting would throw away real work over an optional + input. Same reasoning as the report-write failure path in `cli.py`. + """ + path = resolve_manifest_path(project_dir, manifest) + if path is None: + return None, None + try: + project = DbtProject.from_path(path) + context = DbtContext.from_project(project) + warnings = _manifest_warnings(project) + except DbtProjectError as exc: + # The expected failure mode: `DbtProject.from_path` already wraps a missing file + # or unparseable JSON into a `DbtProjectError` whose own message names `path`, so + # nothing is added here — doing so would print the path twice. + return None, f"dbt enrichment unavailable: {exc}" + except Exception as exc: + # The wide net, deliberately: manifest.json is a file some *other* tool wrote, and + # a structurally-valid-JSON-but-wrong-shaped manifest (`{"nodes": null}`, a node + # that isn't an object, a non-string `relation_name`, ...) can raise almost + # anything — AttributeError from a misplaced `.get()`, TypeError from iterating + # `None` — well past the narrow set `DbtProject` raises on purpose. This runs + # after the whole catalog analysis, so the cost of a miss here is an aborted run + # that already did the real work; the cost of this wide a net is at most + # swallowing a bug in our own parsing, which the disclosure below still surfaces. + return None, f"dbt enrichment unavailable: could not read {path}: {exc}" + disclosure = f"dbt enrichment from {path} ({len(context.models)} model(s)" + if context.dropped_collisions: + disclosure += f", {context.dropped_collisions} cross-database collision(s) dropped" + disclosure += ")" + for warning in warnings: + disclosure += f"\n{warning}" + return context, disclosure + + +#: dbt materializations whose relation is rebuilt out from under a raw `CREATE INDEX`, and +#: what that rebuild does to it. `view` and anything unrecognised are handled separately — +#: a view has no relation to index at all, and an unrecognised materialization is unknown +#: rather than known-safe, so neither belongs in a table keyed by "known to be rebuilt." +#: `ephemeral` never reaches this table either, but for a different reason: an ephemeral +#: model is inlined as a CTE and so has no `relation_name`, which means it never gets far +#: enough through `DbtContext.from_project`/`model_for` to reach a proposal at all. +_REBUILD = { + "table": "every `dbt run` drops and recreates this relation, so a raw CREATE INDEX is lost", + "incremental": ( + "a normal `dbt run` keeps this relation, but `dbt run --full-refresh` rebuilds it and a " + "raw CREATE INDEX is lost" + ), + #: A dbt `materialized_view` model (dbt-core 1.6+) also accepts an `indexes` config, and + #: like `incremental` it is not rebuilt on *every* run: a normal `dbt run` refreshes it + #: in place. It is rebuilt when `dbt run --full-refresh` runs, or when a configuration + #: change forces dbt to drop and recreate it rather than refresh in place — either way a + #: raw CREATE INDEX outside dbt's config does not survive that path. + "materialized_view": ( + "a normal `dbt run` refreshes this materialized view in place, but `dbt run " + "--full-refresh` (or a config change dbt can't apply in place) drops and recreates " + "it, and a raw CREATE INDEX is lost" + ), +} + +#: `CREATE INDEX` or `CREATE UNIQUE INDEX`, optionally followed by `CONCURRENTLY` — matched +#: as a prefix, so whatever comes after (`CONCURRENTLY`, `ON`, ...) is irrelevant here. +_INDEX_CREATE_RE = re.compile(r"(?i)^CREATE\s+(?:UNIQUE\s+)?INDEX\b") +_UNIQUE_INDEX_RE = re.compile(r"(?i)^CREATE\s+UNIQUE\s+INDEX\b") +#: `DROP INDEX`, optionally `CONCURRENTLY` / `IF EXISTS` — matched as a prefix, so whatever +#: follows is irrelevant. The DDL script's own header recommends `CONCURRENTLY` for a live +#: table, so a proposal that used it must not stop being recognised as index-dropping. +_INDEX_DROP_RE = re.compile(r"(?i)^DROP\s+INDEX\b") + +#: A `USING ` clause naming anything but btree. `\S` after the lookahead is load +#: bearing: without it, `\s+` backtracks so that `USING btree` (two spaces) satisfies a +#: bare `(?!btree\b)` one space in, and a plain btree index would be refused. +_NON_BTREE_RE = re.compile(r"(?i)\bUSING\s+(?!btree\b)\S") + + +def _is_index_creating(ddl: str | None) -> bool: + """An index-creating proposal, detected by its DDL prefix rather than its rule code. + + ADV001, ADV007 and ADV008 all emit `CREATE INDEX` today and Batch 3b adds more; a + hardcoded set of codes would silently stop matching the day a new rule ships. Also + matches `CREATE UNIQUE INDEX` (dbt's `indexes` config has a `unique` field for exactly + this) and tolerates an operator-facing `CONCURRENTLY` in between — the DDL script's own + header recommends `CONCURRENTLY` for a live table, so a proposal that used it must not + silently stop being recognised as index-creating. + """ + return ddl is not None and _INDEX_CREATE_RE.match(ddl.lstrip()) is not None + + +def _is_index_dropping(ddl: str) -> bool: + """A `DROP INDEX` proposal (ADV002, ADV003), detected by prefix like its create-side twin. + + Kept separate from `_is_index_creating` rather than folded into one "touches an index" + check: the two need opposite advice. A create is *replaced* by dbt config; a drop cannot + be — dbt's `indexes` config has no way to express a removal — so a drop keeps its DDL and + gains a warning that the config entry has to go too. + """ + return _INDEX_DROP_RE.match(ddl.lstrip()) is not None + + +def _is_unique_index(ddl: str) -> bool: + """Whether `ddl` is a `CREATE UNIQUE INDEX`, which dbt's config expresses as `unique: true`.""" + return _UNIQUE_INDEX_RE.match(ddl.lstrip()) is not None + + +def _names_a_non_btree_method(ddl: str) -> bool: + """Whether `ddl` asks for an access method the config reconstruction cannot express. + + The config block is rebuilt from `evidence["columns"]` and hardcodes `type: btree`; the + DDL text itself is discarded. That is faithful for every rule shipping today — all of + them emit a plain btree over a column list, with no `USING`, no expression, no + `DESC`/`NULLS`/opclass — but the day a rule proposes `USING gin`, a silent rewrite to + `type: btree` would hand back a *different index* than the one the evidence justified. + So a non-btree access method declines the rewrite and discloses instead. + + Textual, unlike `_is_partial_index`, which keys on `guard_column`/`guard_predicate` in + evidence. That asymmetry is forced rather than chosen: no rule records an access method in + its evidence, so there is nothing structural to key on here today. The cost is a possible + over-trigger — **not** on a column merely named `USING`, since quoting puts a `"` where + `\\s+` needs whitespace and `"USING"` therefore does not match, but on one whose name + *contains* the whole clause, like `"USING gin"`. That declines a rewrite which would have + been fine — the conservative direction, since a missed detection instead quietly changes + the recommended index. Ordering, opclasses and expression indexes are *not* detectable + this way and remain a documented limitation of the reconstruction rather than a guard. + """ + return _NON_BTREE_RE.search(ddl) is not None + + +def _is_partial_index(proposal: Proposal) -> bool: + """A WHERE-restricted proposal (ADV004's partial index), detected structurally. + + A substring search for `"WHERE"` in the DDL is foolable two ways: a column genuinely + named `WHERE` (quoted, so syntactically a plain identifier) makes an ordinary index + proposal look partial, and a lowercase `where` — plausible from a future engine's rule, + even though every rule here emits uppercase today — would not match at all, silently + dropping a real predicate into a config block that has nowhere to put it. ADV004 + already carries `guard_column`/`guard_predicate` in its own evidence for exactly this + proposal shape, so keying on their presence is structural rather than textual: it + cannot be spoofed by an identifier and cannot miss on casing. + """ + return "guard_column" in proposal.evidence or "guard_predicate" in proposal.evidence + + +def _relation_of(proposal: Proposal) -> Relation | None: + """The relation a proposal is about, from its own evidence — every rule stores one.""" + schema = proposal.evidence.get("schema") + table = proposal.evidence.get("table") + if isinstance(schema, str) and isinstance(table, str): + return Relation(schema, table) + return None + + +def _comment_block(lines: list[str]) -> str: + """Render `lines` as a `--`-commented block, safe even if a line's *content* smuggles + a raw newline. + + `parse_relation_name` accepts a newline inside a quoted identifier — dbt's own + `relation_name` field can carry one — and the column/table names this module + interpolates ultimately come from a live catalog, which permits the same thing. + Wrapping an interpolated column list in `list(...)` already neutralizes that: a + built-in `list` has no `__str__` of its own, so formatting it falls back to `__repr__`, + which escapes an embedded `\\n` in each contained string into the two literal + characters `\\` and `n` before this function ever sees it — `repr()`'s own escaping is + what does that work, not the value's *outer* formatting conversion, so an explicit + `!r` on top of an already-listed value adds nothing. Splitting each logical line again + here is a second, independent defense: it protects a future caller that interpolates a + raw value without going through a list's own escaping, so nothing reaching here can + produce an output line lacking a leading `--` even then. `render_ddl` defends the same + hazard the same way for raw DDL; this is that defense's equivalent for a generated + config block. + """ + out: list[str] = [] + for line in lines: + physical = line.splitlines() or [""] + out.extend(f"-- {p}" for p in physical) + return "\n".join(out) + + +def _dbt_attribution(model: ModelNode) -> str: + """Which model this is and how it is built, as one operator-facing phrase. + + An absent materialization says so once, in words. Interpolating it directly rendered + "materialized as `None`" — a Python literal leaking into a sentence an operator reads — + and `materialized=""` rendered "materialized as ``", an empty code span, in both cases + *beside* a second spelling of the same fact ("materialization '(absent)'") supplied by + the caller. One fact, one phrase, and callers no longer restate it. + """ + if not model.materialized: + return f"`{model.unique_id}`, with no materialization recorded in the manifest" + return f"`{model.unique_id}`, materialized as `{model.materialized}`" + + +def _dbt_ddl_note(model: ModelNode, reason: str) -> str: + """A `Proposal.note` for a statement that stays executable on a dbt-managed relation. + + ADV302 declines to rewrite on several paths (a partial index, an unrecognised + materialization, no plain column list, a non-btree access method) and each leaves real, + runnable DDL in place. The explanation for that used to live only in `rationale`, which + never reaches the `--ddl` file — so that file could hold a config block explaining that + raw DDL is destroyed by `dbt run` and, a few lines below, a bare `CREATE INDEX` on that + same dbt-managed table. This is the disclosure that travels with the statement instead. + + No backticks and no markdown: this is rendered into a SQL script as `--` comment lines, + where markdown emphasis is noise. Pre-wrapped rather than one long line for the same + reason — `_comment_lines` prefixes each physical line and wraps nothing. + """ + return ( + f"dbt WARNING: this relation is built by dbt model {model.unique_id}\n" + f"({_built_as(model)}), so the statement below is not durable. {reason}\n" + "Reapply it by hand after any rebuild, or it silently disappears." + ) + + +def _built_as(model: ModelNode) -> str: + return model.materialized if model.materialized else "materialization not recorded" + + +def _dbt_drop_note(model: ModelNode) -> str: + """A `Proposal.note` for a `DROP INDEX` on a relation dbt manages. + + The mirror image of the bug ADV302 exists to fix. A dropped index that the model's + `indexes:` config still declares is put straight back by the next `dbt run`, so the + statement below silently reverts and this tool proposes the same drop again next time — + unless the config entry goes too. Conditional on the config declaring it, which is why it + is worded as a condition rather than a verdict: nothing here can read the model's `.yml`, + only the manifest's materialization. + """ + return ( + f"dbt WARNING: this relation is built by dbt model {model.unique_id}\n" + f"({_built_as(model)}). If this index is declared in that model's indexes\n" + "config, the next dbt run recreates it: remove the config entry as well,\n" + "or the drop below does not stick and will be proposed again next run." + ) + + +@dataclass(frozen=True) +class _IndexEntry: + """One `- columns: [...]` item in a dbt model's `indexes` config list. + + Frozen and comparable so two proposals that reduce to the same index (same columns, same + uniqueness) contribute one entry to the merged block rather than two identical ones. + """ + + columns: tuple[str, ...] + unique: bool + + def render(self) -> list[str]: + # `!r` is deliberately absent: `list(columns)` has no `__str__` of its own, so plain + # `{list(...)}` formatting already falls back to `__repr__` and gets the same + # per-element escaping `!r` would have asked for explicitly — see `_comment_block`. + lines = [f" - columns: {list(self.columns)}", " type: btree"] + if self.unique: + lines.append(" unique: true") + return lines + + +def enrich_proposals(proposals: list[Proposal], context: DbtContext) -> list[Proposal]: + """Rewrite index-creating proposals whose relation dbt manages; attribute the rest. + + A `CREATE INDEX` (or `CREATE UNIQUE INDEX`, optionally `CONCURRENTLY`) proposal on a + `table`-, `incremental`- or `materialized_view`-materialized dbt model is expressed + instead as a config block a human can paste into that model's `.yml`, since the raw + DDL is destroyed the next time dbt rebuilds the relation. A `view` cannot carry an + index at all, so the *DDL* is dropped and explained — the proposal itself survives at + LOW, since "this index cannot apply here" is the finding. An unrecognised (or absent) + materialization is left alone — unknown is not the same as known-safe, so the DDL is + not touched on a guess. + + **One model gets exactly one `indexes` block, however many proposals it collects.** + This is the reason for the two passes below and not an optimization. `indexes` is a + single YAML mapping key, so two standalone blocks pasted into one model's config are a + duplicate key and PyYAML — dbt's own parser — silently keeps the last: the other + recommended index is discarded with no error at all. Two survivors per relation is the + *normal* case, not an edge case, because the adapter's collapse layer never folds + non-prefix column lists and deliberately preserves same-set-different-order pairs. So + the first (highest-ranked) proposal for a model carries the complete merged block, and + every later one for that same model is rewritten to point at it rather than emit a + second block. Per-model rather than per-relation only in spelling: `DbtContext` indexes + one model per relation. + + Everything else passes through with only its evidence enriched: a `DROP INDEX` + proposal is ordinary regardless of dbt (dbt never created the index, so there is + nothing for its `indexes` config to un-express), and an advisory proposal with no DDL + has nothing to rewrite either. Both are still attributed to the model they concern, so + a reader knows where to make the fix. + + A proposal whose relation dbt does not manage — or that carries no `(schema, table)` + evidence at all — is returned completely unchanged. + + Output order is the input order. The caller re-sorts by the adapter's own ranking key + after appending ADV301/ADV303, and this function must not pre-empt that. + """ + # Pass 1: decide every proposal that can be decided alone, and collect the index entries + # of the ones that cannot — a config block cannot be rendered until every proposal for + # that model has been seen. + models: list[ModelNode | None] = [] + decided: list[Proposal | None] = [] + entries: list[_IndexEntry | None] = [] + for proposal in proposals: + relation = _relation_of(proposal) + model = context.model_for(relation) if relation is not None else None + models.append(model) + if model is None: + decided.append(proposal) + entries.append(None) + continue + finished, entry = _classify(proposal, model) + decided.append(finished) + entries.append(entry) + + merged: dict[str, list[_IndexEntry]] = {} + owner: dict[str, int] = {} + for position, (model, entry) in enumerate(zip(models, entries)): + if model is None or entry is None: + continue + block = merged.setdefault(model.unique_id, []) + if entry not in block: + block.append(entry) + owner.setdefault(model.unique_id, position) + + # Pass 2: render the config block once per model. + out: list[Proposal] = [] + for position, proposal in enumerate(proposals): + finished = decided[position] + if finished is not None: + out.append(finished) + continue + model = models[position] + entry = entries[position] + assert model is not None and entry is not None # the only shape pass 1 leaves undecided + if owner[model.unique_id] == position: + out.append(_as_config_block(proposal, model, merged[model.unique_id])) + else: + out.append( + _deferred_to_block(proposal, model, proposals[owner[model.unique_id]], entry) + ) + return out + + +def _dbt_evidence(proposal: Proposal, model: ModelNode) -> dict[str, object]: + """`proposal.evidence` plus the model attribution — as a *copy*. + + Copied, not mutated in place: enrichment is a transformation over proposals the adapter + already produced, and quietly editing the caller's dict would make the same proposal + object read differently depending on whether enrichment ran. + """ + evidence = dict(proposal.evidence) + evidence["dbt_model"] = model.unique_id + evidence["dbt_materialized"] = model.materialized + return evidence + + +def _classify(proposal: Proposal, model: ModelNode) -> tuple[Proposal | None, _IndexEntry | None]: + """Either the finished proposal, or the index entry it contributes to a merged block. + + Exactly one of the two is non-None. Returning `(None, entry)` means "this one becomes + dbt config, but which text it gets depends on the other proposals for this model", which + only `enrich_proposals`'s second pass can know. + """ + evidence = _dbt_evidence(proposal, model) + + if not _is_index_creating(proposal.ddl): + # Not rewritten: there is no `indexes` config entry that expresses a *removal*, and + # an advisory proposal has no DDL to rewrite. But "dbt never created this index, so a + # drop is ordinary" — the original reasoning here — is false in exactly the case this + # module exists for. If the index *is* declared in the model's `indexes:` config, the + # next `dbt run` recreates it: the operator drops it, dbt puts it back, and the next + # run of this tool proposes dropping it again. That is the same silently-reverting + # advice ADV302 was built to eliminate, pointing the other way, so it is disclosed — + # in the rationale *and*, since `render_ddl` never emits a rationale, in a note beside + # the statement itself. + # + # Not suppressed: dropping a genuinely unused index is still the right call, and the + # operator needs both halves of the instruction, not neither. + if proposal.ddl is None: + # Nothing is emitted into the DDL script, so there is no statement to warn + # beside, and `rationale` already reaches every surface that shows this proposal. + return dataclasses.replace(proposal, evidence=evidence), None + if _is_index_dropping(proposal.ddl): + rationale = ( + f"{proposal.rationale} This relation is a dbt model " + f"({_dbt_attribution(model)}): if this index is declared in that model's " + "`indexes:` config, `dbt run` recreates it, so the config entry has to be " + "removed as well or the drop will not stick — and this proposal will come " + "back on the next run." + ) + return ( + dataclasses.replace( + proposal, rationale=rationale, evidence=evidence, note=_dbt_drop_note(model) + ), + None, + ) + # Some other statement for a relation dbt owns — no rule emits one today, and + # `_is_index_creating` matches by DDL prefix specifically so this stays covered when + # one does. Unknown shape, so the note claims only what is certainly true. + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}), " + "which dbt rebuilds on its own schedule, so this statement is not expressed as " + "dbt config and may not outlive the next rebuild." + ) + note = _dbt_ddl_note( + model, + "dbt rebuilds this relation on its own schedule, and this statement is not\n" + "expressed as dbt config.", + ) + return dataclasses.replace( + proposal, rationale=rationale, evidence=evidence, note=note + ), None + + ddl = proposal.ddl + assert ddl is not None # _is_index_creating(None) is False, so this branch guarantees it + materialized = model.materialized + + if materialized == "view": + rationale = ( + f"{proposal.rationale} This relation is a dbt view ({_dbt_attribution(model)}): " + "a view has no storage of its own to index, so this proposal does not apply." + ) + return ( + dataclasses.replace( + proposal, + ddl=None, + rationale=rationale, + confidence=Confidence.LOW, + evidence=evidence, + ), + None, + ) + + if materialized not in _REBUILD: + why = ( + f"but materialization '{materialized}' is unrecognised" + if materialized + else "but an unrecorded materialization is unknown rather than known-safe" + ) + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}), " + f"{why}, so the DDL below is left as-is rather than rewritten on a guess." + ) + note = _dbt_ddl_note( + model, + "sqlquality does not recognise that materialization, so it cannot tell whether\n" + "a dbt run destroys this index.", + ) + return dataclasses.replace( + proposal, rationale=rationale, evidence=evidence, note=note + ), None + + # `table`, `incremental` or `materialized_view`: the relation genuinely gets rebuilt, + # so a raw CREATE INDEX is lost sooner or later. A partial index (ADV004's + # WHERE-restricted proposal) has no dbt `indexes`-config equivalent — that config has + # no predicate field — so it must be disclosed as not expressible rather than silently + # rewritten into a config block that quietly drops the WHERE clause and turns a + # correct proposal into a wrong one. + if _is_partial_index(proposal): + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}): " + f"{_REBUILD[materialized]}. dbt's `indexes` config has no predicate field, so this " + "partial index cannot be expressed as config — it will be dropped on the next " + "rebuild unless you reapply the DDL above by hand afterward." + ) + note = _dbt_ddl_note( + model, + "dbt's indexes config has no predicate field, so this partial index cannot be\n" + "expressed as config.", + ) + return dataclasses.replace( + proposal, rationale=rationale, evidence=evidence, note=note + ), None + + columns = proposal.evidence.get("columns") + if ( + not isinstance(columns, (tuple, list)) + or not columns + or not all(isinstance(c, str) for c in columns) + ): + # No plain column list to express as config — leave the DDL untouched rather than + # invent one, and *say so*. Unreachable from today's rules, all of which populate + # `columns`; reachable by design, because `_is_index_creating` matches on the DDL + # prefix precisely so a future index-creating rule is covered without being + # enumerated here. This path used to decline in complete silence — no rationale + # amendment, executable DDL kept — which is the one outcome this module exists to + # prevent, so the disclosure matters more here than on the paths that are exercised. + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}): " + f"{_REBUILD[materialized]}. This proposal carries no plain column list, so it " + "cannot be expressed as dbt `indexes` config and the DDL below is left as-is — " + "reapply it by hand after each rebuild." + ) + note = _dbt_ddl_note( + model, + "This proposal carries no plain column list, so it cannot be expressed as\n" + "dbt indexes config.", + ) + return dataclasses.replace( + proposal, rationale=rationale, evidence=evidence, note=note + ), None + + if _names_a_non_btree_method(ddl): + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}): " + f"{_REBUILD[materialized]}. This index names a non-btree access method, which the " + "config block below cannot express without changing the index it recommends, so " + "the DDL is left as-is — reapply it by hand after each rebuild." + ) + note = _dbt_ddl_note( + model, + "This index names a non-btree access method, which dbt indexes config as\n" + "reconstructed here cannot express.", + ) + return dataclasses.replace( + proposal, rationale=rationale, evidence=evidence, note=note + ), None + + return None, _IndexEntry(columns=tuple(columns), unique=_is_unique_index(ddl)) + + +def _as_config_block(proposal: Proposal, model: ModelNode, entries: list[_IndexEntry]) -> Proposal: + """Rewrite `proposal`'s DDL as the one `indexes` block covering this whole model. + + The block names the model. It used to say only "the model's config block", so two + different relations recommending the same column list rendered byte-identical `ddl` — + distinguishable in the DDL file only by the title comment above it, and in the JSON + payload by nothing at all. It also no longer says "above": in markdown the fenced DDL is + *below* the rationale, in the DDL file the rationale is absent entirely, and in JSON + there is no spatial relation to be right or wrong about. + """ + lines = [ + "ADV302: express this as dbt config, not DDL — raw DDL does not survive a rebuild.", + f"Add to the config of dbt model {model.unique_id}:", + ] + if len(entries) > 1: + lines += [ + f"(all {len(entries)} indexes this run recommends for that model, in ONE block on", + "purpose: an `indexes` mapping key can appear once, so two blocks pasted into the", + "same config silently keep only the last)", + ] + lines.append(" indexes:") + for entry in entries: + lines.extend(entry.render()) + config_ddl = _comment_block(lines) + evidence = _dbt_evidence(proposal, model) + # A flag, not the block itself. This key used to hold a byte-identical copy of `ddl`, + # which earned nothing and smeared the markdown Evidence line — evidence renders as flat + # `k=v` pairs, so a multi-line value lands inline with literal `\n` escapes mid-sentence. + # As a flag it is the one thing a consumer cannot get elsewhere: ADV302 is never a + # proposal `code`, so `code == "ADV302"` matches nothing and this is how a `--json` + # consumer filters for "the DDL here is dbt config, not runnable SQL". + evidence["dbt_index_config"] = True + # `or ""` is for the type checker only: `_classify` returns an entry — the sole way to + # reach this function — exactly when `materialized` is a key of `_REBUILD`. + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}): " + f"{_REBUILD[model.materialized or '']}. Add the config block this proposal carries in " + f"place of its DDL to `{model.unique_id}` instead of running that DDL directly; " + "`dbt run` applies it." + ) + if len(entries) > 1: + rationale += ( + f" That block covers all {len(entries)} indexes this run recommends for the model, " + "because dbt reads only one `indexes` key per config." + ) + return dataclasses.replace(proposal, ddl=config_ddl, rationale=rationale, evidence=evidence) + + +def _deferred_to_block( + proposal: Proposal, model: ModelNode, owner: Proposal, entry: _IndexEntry +) -> Proposal: + """Point a second index proposal for one model at that model's single config block. + + Emitting its own standalone block instead is the silent-data-loss bug this exists to + prevent: two `indexes` keys in one config, and dbt keeps the last. + """ + lines = [ + f"ADV302: the index on {list(entry.columns)} for dbt model {model.unique_id}", + f"is already included in the single dbt config block reported under {owner.code}:", + # `_comment_block` splits each logical line again, so an interpolated title carrying + # a raw newline still cannot produce an uncommented output line. + owner.title, + "Paste that one block. A second `indexes` key in the same config would be a", + "duplicate YAML mapping key, and dbt would silently keep only one of them.", + ] + evidence = _dbt_evidence(proposal, model) + evidence["dbt_index_config_reported_with"] = owner.code + rationale = ( + f"{proposal.rationale} This relation is a dbt model ({_dbt_attribution(model)}): " + f"{_REBUILD[model.materialized or '']}. This index is already part of the single dbt " + f"`indexes` config block reported under {owner.code} for this model — dbt reads one " + "`indexes` key per config, so both indexes have to be expressed in that one block " + "rather than in a block of their own." + ) + return dataclasses.replace( + proposal, ddl=_comment_block(lines), rationale=rationale, evidence=evidence + ) + + +def describe_rewrites(proposals: list[Proposal]) -> str | None: + """One line saying ADV302 fired, or None when it did not. + + ADV302 is never a proposal `code` — it is a rewrite applied to another rule's proposal — + so `code == "ADV302"` matches nothing and an enriched terminal row is byte-identical to + the same proposal from a dbt-free run: same code, same confidence, same cost share, same + title. The terminal never prints `rationale`, where the whole disclosure lives, so + without this line a user who reads only the terminal cannot tell enrichment happened. + Counted off the two evidence flags rather than by searching the DDL text for "ADV302", + which would depend on the wording of a string meant for humans. + """ + rewritten = sum(1 for p in proposals if p.evidence.get("dbt_index_config") is True) + merged = sum(1 for p in proposals if "dbt_index_config_reported_with" in p.evidence) + if not rewritten and not merged: + return None + line = ( + f"ADV302 expressed {rewritten + merged} index proposal(s) as dbt `indexes` config: " + "their DDL is a config block to add to the model, not runnable SQL" + ) + if merged: + line += ( + f" ({merged} folded into another proposal's block, since dbt reads one `indexes` " + "key per model config)" + ) + return line + + +def propose_materialization( + aggregation: Aggregation, context: DbtContext, *, min_cost_share: float +) -> list[Proposal]: + """ADV301 — a dbt model materialized as a `view` that carries a hot share of workload cost. + + A view re-executes its defining query on every read, so any cost saved by a `table` or + `incremental` build is instead paid, in full, every time something reads it. When the + workload shows a view carrying a hot share of cost, that repeated cost is exactly what a + materialization change would trade for a heavier (and scheduled) `dbt run`. This is only + computable by joining workload cost — which this tool has — to the model graph's + materialization — which only the manifest has; neither alone is enough. + + Confidence is capped at MEDIUM and there is deliberately no HIGH branch, mirroring + ADV008's precedent for the same shape of gap: whether the trade actually pays off depends + on how often the model is *rebuilt* versus how often it is *read*, and on how fresh the + data needs to be — neither is visible from query history. Claiming HIGH would be a claim + about a build schedule this tool cannot see. Do not add a HIGH branch here for symmetry + with ADV001; the missing rung is deliberate, not an oversight. + + One proposal per relation, not per column: two hot columns on the same view are one + materialization decision, not two. `cost_share` is the *max* over that relation's usage, + not the sum — `ColumnUsage.cost_share` is deliberately not a partition (see its own + docstring), so a query hot on two columns of the same view would otherwise be counted + twice, exactly the double-count ADV001 and ADV008 already avoid the same way. + + Only `materialized == "view"` qualifies, and excluding `materialized_view` needs no + separate branch: the equality check already excludes it by construction, and correctly + so — unlike a plain view, a materialized view refreshes its *stored* result rather than + re-executing its query on every read, so it has already made the build-time-for-read-time + trade this rule proposes; recommending it again would be pointless. Contrast ADV302, + which *does* need an explicit `materialized_view` branch, because it answers a different + question (does a rebuild destroy an index) that a materialized view answers the same way + a table does. + """ + by_relation: dict[Relation, list[ColumnUsage]] = defaultdict(list) + for item in aggregation.usage: + by_relation[item.relation].append(item) + + proposals: list[Proposal] = [] + for relation in sorted(by_relation): + model = context.model_for(relation) + if model is None or model.materialized != "view": + continue + cost_share = max(item.cost_share for item in by_relation[relation]) + if cost_share < min_cost_share: + continue + proposals.append( + Proposal( + code="ADV301", + title=f"Materialize {relation} instead of a view", + rationale=( + f"This dbt model ({_dbt_attribution(model)}) carries a hot share of " + f"workload cost ({cost_share:.1%}) but, as a view, re-executes its " + "defining query on every read. Materializing it as `table` or " + "`incremental` trades that repeated read cost for a build cost paid on " + "each `dbt run` instead — worth it only if the model is read far more " + "often than it is rebuilt, and if it does not need to reflect every " + "write immediately. Neither is visible from query history, which is why " + "this is capped at MEDIUM: it names the trade, not a verdict on it." + ), + evidence={ + "schema": relation.schema, + "table": relation.table, + "dbt_model": model.unique_id, + "cost_share": cost_share, + }, + confidence=Confidence.MEDIUM, + ddl=None, + ) + ) + return proposals + + +def propose_unused_models( + aggregation: Aggregation, context: DbtContext, workload: Workload +) -> list[Proposal]: + """ADV303 — a dbt model within reach of the manifest that the analyzed workload never + touched. + + A model that costs a build every night and that nothing queries is worth knowing about, + but the evidence here is *absence*, which is far weaker than presence, so this carries + the loudest caveat in this module and a hard confidence cap of LOW rather than a + downgrade for each individual caveat: + + * the analyzed window may simply not cover this model's reader — a monthly report, a + BI tool with its own cache, an ad-hoc job that only runs quarterly; + * `--limit` truncates the query history handed to `advise`, so a cold-but-genuinely-used + model can look exactly like an unused one within the slice this tool actually saw. + + A model with a declared consumer is excluded outright — not merely downgraded — because + that is a correctness gate, not a caveat: a staging model consumed only by another model, + a snapshot, or a dbt exposure (which exists specifically to declare "a BI dashboard reads + this") *is* used, just not by an ad-hoc query, and without this exclusion the rule would + propose deleting every staging model in a well-formed project. See + `DbtContext.consumer_count` for what counts as a consumer and why a test does not. + + This only looks at a model's *immediate* consumers, not the whole downstream chain: if + dead model A feeds dead model B, B being unused does not get attributed back to A — A is + judged solely on its own `consumer_count`, which B's presence still satisfies. This is + conservative by construction (it never flags something it shouldn't) but it also means a + fully dead sub-DAG is only ever reported from its leaf, not from its root — cascading + would require knowing that every consumer along the chain is itself unused, which this + rule does not attempt. + + An `Aggregation` with no usage at all is refused rather than treated as evidence: every + relation in `context.models` would trivially be "untouched" (none can be in + `aggregation.tables`, which is built only from usage), so an empty or fully-unparseable + workload must not read as proof that every childless model is unused. + """ + if not aggregation.usage: + return [] + proposals: list[Proposal] = [] + for relation in sorted(context.models): + if relation in aggregation.tables: + continue + if context.consumer_count.get(relation, 0) > 0: + continue + model = context.models[relation] + rationale = ( + f"No query in the analyzed workload ({workload.window_description}) referenced " + f"this dbt model ({_dbt_attribution(model)}). This is evidence of absence, not " + "proof of it: the window may simply not cover this model's reader — a monthly " + "report, a BI tool with its own cache, a quarterly job — and `--limit` truncates " + "the query history this tool actually saw, so a cold-but-used model can look " + "unused within that slice. A model with a declared consumer — another model, a " + "snapshot, or a dbt exposure — is excluded from this rule outright rather than " + "merely downgraded, because it is used, just not by an ad-hoc query. That " + "exclusion is not transitive: only a model's immediate consumers are considered, " + "so a dead chain surfaces one model per run, from its leaf — if this model feeds " + "another that turns out to be dead too, that one is only reported after this one " + "is gone." + ) + proposals.append( + Proposal( + code="ADV303", + title=f"{relation} is a dbt model the analyzed workload never touched", + rationale=rationale, + evidence={ + "schema": relation.schema, + "table": relation.table, + "dbt_model": model.unique_id, + }, + confidence=Confidence.LOW, + ddl=None, + ) + ) + return proposals diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index fe5912c..e9d06f4 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -1218,6 +1218,25 @@ def _comment_lines(text: str) -> list[str]: return [f"-- {line}" for line in text.splitlines()] +def _is_fully_commented(ddl: str) -> bool: + """True when every physical line of `ddl` already begins with `--`. + + `render_ddl`'s line-break guard exists to catch an *identifier* whose embedded newline + would otherwise leave part of a raw statement looking like a bare, executable line. A + `ddl` value that is already a `--`-commented disclosure on every line — for instance, a + config-block proposal something upstream of this adapter generated instead of raw + DDL — is categorically not that hazard: it is already inert on every line, so it can be + emitted verbatim (with the usual code/confidence header) rather than routed through the + NOT-RENDERED fallback, which would double-comment every line and print a reason ("an + identifier contains a line break") that is simply false for this kind of proposal. This + check is about the *shape* of the text alone, so it names nothing about dbt or any + other specific caller — any adapter-agnostic multi-line, pre-commented `ddl` gets the + same treatment. + """ + lines = ddl.splitlines() + return bool(lines) and all(line.startswith("--") for line in lines) + + class PostgresWorkloadAdapter(WorkloadAdapter): engine = "postgres" @@ -1588,9 +1607,6 @@ def fetch_indexes( ) return {relation: tuple(indexes) for relation, indexes in result.items()} - #: Highest confidence first, then largest cost share — the reading order a human wants. - _CONFIDENCE_ORDER = {Confidence.HIGH: 0, Confidence.MEDIUM: 1, Confidence.LOW: 2} - #: Which rule's rationale to keep when two rules propose byte-identical DDL at equal #: confidence. Lower wins. The order is by how directly the evidence supports *this* #: index: a filter predicate (ADV001) is the most direct reason to build a B-tree, a @@ -1970,24 +1986,7 @@ def propose( proposals = self._dedupe_by_ddl(proposals) proposals = self._collapse_index_prefixes(proposals) proposals = self._disclose_column_set_overlaps(proposals) - return sorted(proposals, key=self._ranking_key) - - @classmethod - def _ranking_key(cls, proposal: Proposal) -> tuple[int, float, str, str]: - """Highest confidence first, then largest cost share — the reading order a human - wants — with a canonical tiebreak so equal-confidence equal-cost proposals do not - reorder between runs and make the CLI's tests flaky. - - `cost_share_of` rather than `float(evidence.get(...))`: bool is an int subclass, so - a stray True became -1.0 and sorted a fabricated share ahead of a genuinely hot - proposal, at the top of the list the CLI presents as "read this first". - """ - return ( - cls._CONFIDENCE_ORDER[proposal.confidence], - -(cost_share_of(proposal.evidence) or 0.0), - proposal.code, - proposal.title, - ) + return sorted(proposals, key=self.ranking_key) def render_ddl(self, proposals: list[Proposal]) -> str: """A commented, reviewable script. sqlquality never executes this.""" @@ -2006,7 +2005,9 @@ def render_ddl(self, proposals: list[Proposal]) -> str: for proposal in proposals: if not proposal.ddl: continue - if "\n" in proposal.ddl or "\r" in proposal.ddl: + if ("\n" in proposal.ddl or "\r" in proposal.ddl) and not _is_fully_commented( + proposal.ddl + ): # An identifier containing a line break cannot be emitted as a single-line # statement. Quoting already makes it *semantically* safe — psql parses the # whole thing as one quoted identifier, so nothing extra executes — but the @@ -2015,9 +2016,16 @@ def render_ddl(self, proposals: list[Proposal]) -> str: # instead would emit DDL targeting an object that does not exist. So it is # commented out in full with the reason, rather than rendered wrong or # silently dropped. + # + # This is skipped when `_is_fully_commented` already holds: a `ddl` that is + # every-line-`--`-commented is not an identifier smuggling a line break, it + # is an intentional multi-line disclosure, and running it through this + # fallback would double-comment it and print a reason that is false for it. body.append("-- NOT RENDERED: an identifier in this proposal contains a line") body.append("-- break, so it cannot be emitted as a single-line statement.") body.append("-- Verify the name and apply this by hand:") + if proposal.note: + body.extend(_comment_lines(proposal.note)) body.extend(_comment_lines(proposal.ddl)) body.append("") continue @@ -2025,6 +2033,12 @@ def render_ddl(self, proposals: list[Proposal]) -> str: share_text = f", {share:.1%} of workload cost" if share is not None else "" body.append(f"-- {proposal.code} [{proposal.confidence.value}{share_text}]") body.extend(_comment_lines(proposal.title)) + # `note` before the statement, not after: this script's whole purpose is to be + # read top-to-bottom before anything is run, and `rationale` — where every other + # caveat lives — never reaches this file at all. A caveat printed below the + # statement it qualifies is a caveat an operator reads after pasting it. + if proposal.note: + body.extend(_comment_lines(proposal.note)) body.append(proposal.ddl) body.append("") if not body: diff --git a/tests/integration/test_advise_live.py b/tests/integration/test_advise_live.py index 4dddb69..c18bce5 100644 --- a/tests/integration/test_advise_live.py +++ b/tests/integration/test_advise_live.py @@ -247,3 +247,134 @@ def test_never_analysed_join_key_still_proposes_at_low_confidence(seeded): assert order_items_proposals, "ADV007 did not fire for public.order_items" assert order_items_proposals[0]["evidence"]["row_estimate"] is None assert order_items_proposals[0]["confidence"] == "low", order_items_proposals[0] + + +def _adv001_for(payload: dict, *, schema: str, table: str) -> dict | None: + for p in payload["proposals"]: + if p["code"] == "ADV001" and p["evidence"].get("schema") == schema: + if p["evidence"].get("table") == table: + return p + return None + + +def test_adv302_rewrites_a_real_index_proposal_into_dbt_config(seeded, tmp_path): + """ADV302's whole reason to exist, proven on live data rather than a fixture. + + A raw `CREATE INDEX` on a dbt `table`-materialized relation does not survive the next + `dbt run` (it drops and recreates the relation), so ADV302 rewrites that proposal into + a config block instead of doomed DDL. `tests/fixtures/manifest_v12.json` cannot prove + this live: its `relation_name`s are all schema `"main"`, which this seeded database + never has (`public`/`staging`), and relation matching is on the qualified + `(schema, table)` pair with no bare-name fallback -- so a manifest built from schemas + that do not match what got seeded would match nothing, and the test would pass while + proving nothing. This manifest declares `public.orders` instead, which conftest.py's + `seeded` fixture actually creates. + + Non-vacuity guard first: the *un-enriched* run must really emit `CREATE INDEX` for + `public.orders` (ADV001, on the hot `status` predicate -- see conftest.py's seeded + workload). Without this half, the enriched assertion below would pass just as well if + the workload simply produced no proposal for that relation at all. + """ + dsn, _schema = seeded + + bare = _run_advise(seeded, schemas=("public", "staging")) + bare_orders = _adv001_for(bare, schema="public", table="orders") + assert bare_orders is not None, ( + f"no un-enriched ADV001 for public.orders; got " + f"{[(p['code'], p['evidence'].get('schema'), p['evidence'].get('table')) for p in bare['proposals']]}" + ) + assert bare_orders["ddl"] is not None + assert bare_orders["ddl"].upper().lstrip().startswith("CREATE INDEX"), bare_orders + assert "dbt_index_config" not in bare_orders["evidence"], bare_orders + + manifest = { + "metadata": { + "dbt_schema_version": "https://schemas.getdbt.com/dbt/manifest/v12.json", + "adapter_type": "postgres", + }, + "nodes": { + "model.live_it.orders": { + "unique_id": "model.live_it.orders", + "name": "orders", + "resource_type": "model", + "config": {"materialized": "table"}, + "compiled_code": "select * from {{ source('raw', 'orders') }}", + # The database part ("analytics") is deliberately NOT what conftest.py's + # `seeded` fixture actually connects to -- parse_relation_name drops it, + # since `advise` connects to one database at a time. Only the + # (schema, table) pair below has to match what got seeded. + "relation_name": '"analytics"."public"."orders"', + "depends_on": {"macros": [], "nodes": []}, + } + }, + "sources": {}, + "parent_map": {"model.live_it.orders": []}, + "child_map": {"model.live_it.orders": []}, + } + manifest_path = tmp_path / "manifest.json" + manifest_path.write_text(json.dumps(manifest), encoding="utf-8") + + result = runner.invoke( + app, + [ + "advise", + "--dsn", + dsn, + "--schema", + "public", + "--schema", + "staging", + "--json", + "--min-cost-share", + "0.0", + "--manifest", + str(manifest_path), + ], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + assert payload["dbt"]["models"] == 1, payload.get("dbt") + assert payload["dbt"]["dropped_collisions"] == 0, payload.get("dbt") + + enriched = _adv001_for(payload, schema="public", table="orders") + assert enriched is not None, "the dbt-managed public.orders proposal disappeared entirely" + assert not (enriched["ddl"] or "").upper().lstrip().startswith("CREATE INDEX"), enriched + assert enriched["evidence"]["dbt_model"] == "model.live_it.orders" + assert enriched["evidence"]["dbt_materialized"] == "table" + + # This live workload really does produce more than one index proposal for `public.orders` + # -- a hot predicate and a hot join key -- which is the ordinary case, not an edge one. + # dbt reads one `indexes` key per model config, so the whole run must contain exactly one + # `indexes:` block for the model: two standalone blocks pasted into one config are a + # duplicate YAML mapping key, and PyYAML (dbt's own parser) silently keeps just one of + # them, discarding the other recommended index with no error at all. + for_model = [ + p for p in payload["proposals"] if p["evidence"].get("dbt_model") == "model.live_it.orders" + ] + assert len(for_model) >= 2, ( + "this test's whole point is multiple proposals for one dbt model; got " + f"{[(p['code'], p['title']) for p in for_model]}" + ) + owners = [p for p in for_model if p["evidence"].get("dbt_index_config") is True] + assert len(owners) == 1, [p["code"] for p in owners] + blocks = [ + line + for p in for_model + for line in (p["ddl"] or "").splitlines() + if line.removeprefix("--").strip() == "indexes:" + ] + assert len(blocks) == 1, f"one model, one `indexes:` block, got {len(blocks)}" + + [owner] = owners + assert "indexes:" in owner["ddl"], owner + assert "model.live_it.orders" in owner["ddl"], "the block names the model to paste it into" + # Every index recommended for the model is inside that one block, this ADV001's included. + columns = [p["evidence"]["columns"] for p in for_model if p["evidence"].get("columns")] + assert columns, for_model + for column_list in columns: + assert str(list(column_list)) in owner["ddl"], (column_list, owner["ddl"]) + for other in for_model: + if other is owner: + continue + assert other["evidence"]["dbt_index_config_reported_with"] == owner["code"] + assert "already included in the single dbt config block" in other["ddl"] diff --git a/tests/test_advise_cli.py b/tests/test_advise_cli.py index 48c67ba..ec90630 100644 --- a/tests/test_advise_cli.py +++ b/tests/test_advise_cli.py @@ -1,3 +1,4 @@ +import dataclasses import json from pathlib import Path @@ -10,7 +11,7 @@ _validate_schemas, app, ) -from sqlquality.models import Aggregation, QueryStat, Relation, Workload +from sqlquality.models import Aggregation, Confidence, Proposal, QueryStat, Relation, Workload from sqlquality.report import advise_payload runner = CliRunner() @@ -511,6 +512,302 @@ def test_empty_workload_exits_0(monkeypatch): assert json.loads(result.stdout)["proposals"] == [] +DBT_FIXTURE = Path(__file__).parent / "fixtures" / "manifest_v12.json" + + +def test_project_dir_loads_a_manifest_and_discloses_only_on_stderr(monkeypatch, tmp_path): + """--project-dir must actually reach `load_dbt_context` — replacing `target/manifest.json` + with garbage used to leave every test in this module green, because nothing exercised the + option at all. The disclosure it produces must land on stderr: stdout has to stay valid + JSON under --json, since that is what a later task diffs byte-for-byte against `main`. + """ + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + target = tmp_path / "target" + target.mkdir() + (target / "manifest.json").write_text(DBT_FIXTURE.read_text(encoding="utf-8"), encoding="utf-8") + + result = runner.invoke( + app, + ["advise", "--dsn", "postgresql://u@h/db", "--project-dir", str(tmp_path), "--json"], + ) + assert result.exit_code == 0 + assert "dbt enrichment" in result.stderr + assert "dbt enrichment" not in result.stdout + payload = json.loads(result.stdout) # stdout must still be pure, parseable JSON + assert payload["proposals"] == [] + # The payload's manifest path must resolve via --project-dir/target/manifest.json — + # the same precedence load_dbt_context itself used to load this file. + assert payload["dbt"]["manifest"] == str(target / "manifest.json") + assert payload["dbt"]["models"] == 3 + + +def test_project_dir_with_a_broken_manifest_does_not_abort_the_run(monkeypatch, tmp_path): + """A garbage `target/manifest.json` must degrade to 'no enrichment', not crash a run + that already did the whole catalog analysis.""" + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + target = tmp_path / "target" + target.mkdir() + (target / "manifest.json").write_text("{not json", encoding="utf-8") + + result = runner.invoke( + app, ["advise", "--dsn", "postgresql://u@h/db", "--project-dir", str(tmp_path)] + ) + assert result.exit_code == 0 + assert "dbt enrichment unavailable" in result.stderr + assert "Traceback" not in result.output + + +def test_manifest_option_loads_and_discloses_the_source(monkeypatch): + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + result = runner.invoke( + app, ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(DBT_FIXTURE)] + ) + assert result.exit_code == 0 + assert str(DBT_FIXTURE) in result.stderr + + +def test_no_dbt_option_means_no_disclosure_anywhere(monkeypatch): + """The no-manifest path is every existing `advise` invocation. A later task proves + byte-identical output against `main` by diffing artifacts, so nothing dbt-shaped may + appear anywhere in the output without either --project-dir or --manifest. + """ + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + result = runner.invoke(app, ["advise", "--dsn", "postgresql://u@h/db"]) + assert result.exit_code == 0 + assert "dbt enrichment" not in result.output + + +def test_no_manifest_means_no_behaviour_change(monkeypatch): + """The dbt-free path is first-class, so enrichment must be additive by construction. + + This is a unit-level pin of the same constraint the task proves by diffing a whole run's + artifacts against `main`: with neither `--project-dir` nor `--manifest`, no proposal may + carry dbt evidence and the payload must carry no `"dbt"` key at all — not even one set + to `None` — so the payload stays byte-identical to what `main` produced before this key + existed, rather than merely equal apart from one known extra key. + """ + _stub_adapter( + monkeypatch, + { + "pg_stat_statements": [ + ("select id from orders where status = $1 and created_at > $2", 100, 5000.0, 10), + ], + "pg_stat_database": [("2026-07-01",)], + "information_schema.columns": WIDE_COLUMNS, + "pg_total_relation_size": [("public", "orders", 5_000_000, 10**8)], + "pg_stats": [("public", "orders", "status", 5000.0)], + "pg_index": [], + }, + ) + result = runner.invoke(app, ["advise", "--dsn", "postgresql://u@h/db", "--json"]) + assert result.exit_code == 0 + assert "dbt" not in result.stderr.lower() + payload = json.loads(result.stdout) + assert payload["proposals"], "the scenario must produce at least one proposal to test" + for proposal in payload["proposals"]: + assert "dbt_model" not in proposal["evidence"] + assert "dbt" not in payload + + +def test_an_unreadable_manifest_via_the_flag_does_not_fail_the_run(monkeypatch, tmp_path): + """Exit 0 with a disclosure — the catalog work already happened, and dbt is optional. + + Distinct from `test_project_dir_with_a_broken_manifest_does_not_abort_the_run`: that one + exercises a malformed *file* reached via `--project-dir`; this one exercises `--manifest` + naming a path that does not exist at all. + """ + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + missing = tmp_path / "no.json" + result = runner.invoke( + app, ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(missing), "--json"] + ) + assert result.exit_code == 0 + assert "dbt enrichment unavailable" in result.stderr + payload = json.loads(result.stdout) + assert "dbt" not in payload + + +def test_the_payload_records_which_manifest_was_used(monkeypatch): + """The mirror image of `test_no_manifest_means_no_behaviour_change`: with a manifest, + the `"dbt"` key must be present (not merely non-`None` — `"dbt" in payload` is the + actual claim, since the no-manifest test now pins its *absence*) and carry the + manifest path, model count and collision count.""" + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + result = runner.invoke( + app, + ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(DBT_FIXTURE), "--json"], + ) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + assert "dbt" in payload + assert payload["dbt"]["manifest"] == str(DBT_FIXTURE) + # The fixture carries exactly 3 models: stg_orders, orders, customer_orders. + assert payload["dbt"]["models"] == 3 + assert payload["dbt"]["dropped_collisions"] == 0 + + +def test_the_json_payload_is_serializable_with_a_manifest_loaded(monkeypatch): + """`--manifest` is parsed by typer as a `Path`, and `json.dumps` cannot encode one. + + If `cli.advise` ever handed a raw `Path` into `dbt_payload["manifest"]` instead of + `str(resolved_manifest)`, `json.dumps(payload, ...)` would raise `TypeError` — *after* + the whole catalog analysis had already run, the same late-failure shape the write- + failure handlers elsewhere in this module exist to avoid. Asserting `isinstance(..., + str)` pins the actual hazard directly, rather than only failing coincidentally were a + future encoder ever more lenient than the stdlib's about non-`str` dict values. + """ + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + result = runner.invoke( + app, + ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(DBT_FIXTURE), "--json"], + ) + assert result.exit_code == 0 + assert "Traceback" not in result.output + payload = json.loads(result.stdout) + assert isinstance(payload["dbt"]["manifest"], str) + json.dumps(payload) # must not raise + + +def test_the_payload_reports_a_nonzero_dropped_collision_count(monkeypatch, tmp_path): + """`dropped_collisions` must reflect `DbtContext.dropped_collisions`, not a hardcoded 0. + + Two models here build the same `(schema, table)` in two different databases — the + cross-database collision `DbtContext.from_project` refuses to guess at (see + workload/dbt.py). Task 1 counts it precisely so a user can learn a relation was + silently dropped from the index; this pins that the count actually reaches the CLI + payload rather than a value that happens to already be right for the shared fixture, + which has zero collisions and so cannot catch a hardcoded 0. + """ + manifest = { + "metadata": { + "dbt_schema_version": "https://schemas.getdbt.com/dbt/manifest/v12.json", + "adapter_type": "postgres", + }, + "nodes": { + "model.demo.a": { + "unique_id": "model.demo.a", + "name": "a", + "resource_type": "model", + "config": {"materialized": "table"}, + "compiled_code": "select 1", + "relation_name": '"prod"."main"."orders"', + "depends_on": {"macros": [], "nodes": []}, + }, + "model.demo.b": { + "unique_id": "model.demo.b", + "name": "b", + "resource_type": "model", + "config": {"materialized": "table"}, + "compiled_code": "select 1", + "relation_name": '"stage"."main"."orders"', + "depends_on": {"macros": [], "nodes": []}, + }, + }, + "sources": {}, + "parent_map": {"model.demo.a": [], "model.demo.b": []}, + "child_map": {"model.demo.a": [], "model.demo.b": []}, + } + manifest_path = tmp_path / "manifest.json" + manifest_path.write_text(json.dumps(manifest), encoding="utf-8") + + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + result = runner.invoke( + app, + ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(manifest_path), "--json"], + ) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + assert payload["dbt"]["dropped_collisions"] == 1 + assert payload["dbt"]["models"] == 0 + + +def test_adv301_and_adv303_only_appear_with_a_manifest(monkeypatch): + """ADV301/ADV303 need the model graph a manifest carries, so neither may appear without + one — and at least one must appear with the fixture manifest, or this test proves + nothing about the wiring at all. + + `customer_orders` in the fixture manifest has no declared consumer, so + `propose_unused_models` (ADV303) flags it as soon as the workload has *any* usage at + all — regardless of which schema that usage is in, since the negative check is simply + "not in aggregation.tables". The workload below queries `public.orders`, wholly + unrelated to the fixture's `main` schema, so ADV303 firing here is attributable only to + the manifest being loaded, not to any accidental overlap with the query below. + """ + rows = { + "pg_stat_statements": [ + ("select id from orders where status = $1", 5, 100.0, 5), + ], + "pg_stat_database": [("2026-07-01",)], + "information_schema.columns": [ + ("public", "orders", "id", "integer"), + ("public", "orders", "status", "text"), + ], + "pg_total_relation_size": [("public", "orders", 5_000_000, 10**8)], + "pg_stats": [("public", "orders", "status", 5000.0)], + "pg_index": [], + } + _stub_adapter(monkeypatch, rows) + without = runner.invoke(app, ["advise", "--dsn", "postgresql://u@h/db", "--json"]) + _stub_adapter(monkeypatch, rows) + with_dbt = runner.invoke( + app, + ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(DBT_FIXTURE), "--json"], + ) + assert without.exit_code == 0 + assert with_dbt.exit_code == 0 + without_codes = {p["code"] for p in json.loads(without.stdout)["proposals"]} + with_codes = {p["code"] for p in json.loads(with_dbt.stdout)["proposals"]} + assert not ({"ADV301", "ADV303"} & without_codes), without_codes + assert {"ADV301", "ADV303"} & with_codes, with_codes + + +def test_enrichment_output_is_resorted_by_the_adapters_ranking_key(monkeypatch): + """After enriching and extending with ADV301/ADV303, the combined list must be re-sorted + by `PostgresWorkloadAdapter._ranking_key`, not left in call order — otherwise the + terminal table, the markdown and the DDL file could each disagree on the order. + + The base adapter is stubbed to return one LOW-confidence proposal; `propose_materialization` + is stubbed to contribute one HIGH-confidence proposal. Concatenation in call order would + put the LOW proposal first; the ranking key puts HIGH first. Only a real re-sort produces + the HIGH-first order asserted below. + """ + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + + low = Proposal( + code="ADV999", + title="low one", + rationale="r", + evidence={}, + confidence=Confidence.LOW, + ddl=None, + ) + high = Proposal( + code="ADV001", + title="high one", + rationale="r", + evidence={}, + confidence=Confidence.HIGH, + ddl=None, + ) + + monkeypatch.setattr( + "sqlquality.workload.postgres.PostgresWorkloadAdapter.propose", + lambda self, *a, **k: [low], + ) + monkeypatch.setattr("sqlquality.cli.enrich_proposals", lambda proposals, context: proposals) + monkeypatch.setattr("sqlquality.cli.propose_materialization", lambda *a, **k: [high]) + monkeypatch.setattr("sqlquality.cli.propose_unused_models", lambda *a, **k: []) + + result = runner.invoke( + app, + ["advise", "--dsn", "postgresql://u@h/db", "--manifest", str(DBT_FIXTURE), "--json"], + ) + assert result.exit_code == 0 + payload = json.loads(result.stdout) + codes = [p["code"] for p in payload["proposals"]] + assert codes == ["ADV001", "ADV999"], codes + + def test_ddl_and_markdown_files_are_written(monkeypatch, tmp_path): _stub_adapter( monkeypatch, @@ -659,3 +956,503 @@ def test_coverage_warning_is_silent_exactly_at_the_threshold(): _workload_with(stats=80, unparseable=19, noise=0), _aggregation_with() ) assert below is None, "the warning fired below the threshold" + + +#: A workload whose hot predicate and hot join key both land on `public.orders`, plus a join +#: key on an unrelated `public.payments`. Two survivors on one relation is the *normal* shape +#: — the adapter's collapse layer never folds non-prefix column lists — and `payments` is the +#: control: nothing dbt-managed, so nothing about it may change. +TWO_INDEXES_ON_ORDERS_ROWS = { + "pg_stat_statements": [ + ("select id from orders where status = $1 and created_at > $2", 100, 5000.0, 10), + ( + "select o.id from orders o join payments p on p.customer_id = o.customer_id", + 80, + 4000.0, + 8, + ), + ], + "pg_stat_database": [("2026-07-01",)], + "information_schema.columns": [ + ("public", "orders", "id", "integer"), + ("public", "orders", "status", "text"), + ("public", "orders", "created_at", "timestamp"), + ("public", "orders", "customer_id", "integer"), + ("public", "payments", "id", "integer"), + ("public", "payments", "customer_id", "integer"), + ], + "pg_total_relation_size": [ + ("public", "orders", 5_000_000, 10**8), + ("public", "payments", 5_000_000, 10**8), + ], + "pg_stats": [ + ("public", "orders", "status", 5000.0), + ("public", "orders", "customer_id", 5000.0), + ], + #: An index on the dbt-managed relation that the workload never scans, so ADV002 fires + #: through the real rules and the run contains a genuine `DROP INDEX` for a dbt model. + "pg_index": [ + ( + "public", + "orders", + "idx_orders_cold", + "id", + 1, + False, # unique + False, # primary + 0, # scans + 8192, + False, # partial + None, # predicate + False, # expressions + "CREATE INDEX idx_orders_cold ON public.orders USING btree (id)", + ) + ], +} + + +def _orders_manifest(tmp_path, materialized="table"): + """A manifest declaring `public.orders` — the schema the stubbed workload really uses. + + `tests/fixtures/manifest_v12.json` cannot serve here: its `relation_name`s are all schema + `main`, matching is on the qualified `(schema, table)` pair with no bare-name fallback, so + a fixture built from a schema the workload never touches would match nothing and every + assertion below would pass while proving nothing. The database part is deliberately not + the connected one — `parse_relation_name` drops it. + """ + manifest = { + "metadata": { + "dbt_schema_version": "https://schemas.getdbt.com/dbt/manifest/v12.json", + "adapter_type": "postgres", + }, + "nodes": { + "model.demo.orders": { + "unique_id": "model.demo.orders", + "name": "orders", + "resource_type": "model", + "config": {"materialized": materialized}, + "compiled_code": "select 1", + "relation_name": '"analytics"."public"."orders"', + "depends_on": {"macros": [], "nodes": []}, + } + }, + "sources": {}, + "parent_map": {"model.demo.orders": []}, + "child_map": {"model.demo.orders": []}, + } + path = tmp_path / "manifest.json" + path.write_text(json.dumps(manifest), encoding="utf-8") + return path + + +def test_adv302_rewrites_an_index_proposal_into_dbt_config_through_the_cli(monkeypatch, tmp_path): + """ADV302's entire CLI wiring, pinned in the suite CI actually runs. + + Nothing in the default suite pinned that `advise` calls `enrich_proposals` at all: + replacing that one line with `pass` — which disables the branch's headline feature + completely — left all 665 tests passing. The only guard was + `tests/integration/test_advise_live.py`, and `pyproject.toml` sets + `addopts = "-m 'not integration'"` while `ci.yml` provisions no Postgres, so CI never runs + it: ADV302 could have been deleted from the CLI with every check green. This is the third + instance of that defect class on this branch, so it is pinned here — no Docker, no extras, + no live database, because the `no-extras` CI job depends on that. + """ + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--manifest", + str(_orders_manifest(tmp_path)), + "--json", + ], + ) + assert result.exit_code == 0, result.output + proposals = json.loads(result.stdout)["proposals"] + orders = [p for p in proposals if p["evidence"].get("table") == "orders"] + assert orders, f"the scenario must produce a proposal for public.orders: {proposals}" + rewritten = [p for p in orders if p["evidence"].get("dbt_index_config") is True] + assert rewritten, f"no index proposal was expressed as dbt config: {orders}" + for proposal in orders: + assert not (proposal["ddl"] or "").upper().lstrip().startswith("CREATE INDEX"), proposal + assert proposal["evidence"]["dbt_model"] == "model.demo.orders" + for proposal in rewritten: + assert "ADV302" in proposal["ddl"], proposal + # The control: `payments` is not dbt-managed, so its proposal must be untouched. + [payments] = [p for p in proposals if p["evidence"].get("table") == "payments"] + assert payments["ddl"] == 'CREATE INDEX ON "public"."payments" ("customer_id");' + assert "dbt_model" not in payments["evidence"] + + +def test_two_index_proposals_for_one_dbt_model_yield_one_config_block_through_the_cli( + monkeypatch, tmp_path +): + """The end-to-end form of the duplicate-YAML-key data loss. + + Two ordinary proposals on one dbt-managed relation each emitted a complete, standalone + `indexes:` block. Pasted under one model's `config:` that is a duplicate mapping key, and + PyYAML — dbt's own parser — silently keeps the last: the other recommended index is + discarded with no error. Asserted on the `--ddl` artifact because that is the file a human + copies from. + """ + import yaml + + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + ddl_path = tmp_path / "out.sql" + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--manifest", + str(_orders_manifest(tmp_path)), + "--ddl", + str(ddl_path), + ], + ) + assert result.exit_code == 0, result.output + script = ddl_path.read_text(encoding="utf-8") + lines = [ln.removeprefix("--").strip() for ln in script.splitlines()] + assert lines.count("indexes:") == 1, f"one model, one `indexes:` block:\n{script}" + + # Un-comment from the `indexes:` line to the end of that comment run — exactly the region + # a human copies into a model config — keeping the original indentation, which is what + # makes it YAML at all. + body_lines: list[str] = [] + started = False + for raw in script.splitlines(): + if not raw.startswith("--"): + if started: + break + continue + content = raw[3:] if raw.startswith("-- ") else raw[2:] + if content.strip() == "indexes:": + started = True + if started: + body_lines.append(content) + body = "\n".join(body_lines) + parsed = yaml.safe_load(body) + assert [entry["columns"] for entry in parsed["indexes"]] == [ + ["status", "created_at"], + ["customer_id"], + ], f"both recommended indexes must survive in the one block:\n{body}" + + +def _warned_statements(script: str) -> dict[str, bool]: + """`{statement: whether its block carries a dbt warning}` for every statement in a script. + + Keyed by the statement itself rather than by the relation named in it, because the two are + not the same thing: `DROP INDEX "public"."idx_orders_cold";` names an index, so filtering + blocks on the *table* name skipped every drop — which is how a bare `DROP INDEX` for a + dbt-managed relation sat in the same file that declared that relation dbt-managed. + """ + result: dict[str, bool] = {} + for block in script.split("\n\n"): + lines = block.splitlines() + statements = [ln for ln in lines if ln.strip() and not ln.startswith("--")] + if not statements: + continue + warned = any("dbt WARNING" in ln for ln in lines) + for statement in statements: + result[statement] = warned + return result + + +def test_the_ddl_file_warns_beside_every_statement_it_keeps_for_a_dbt_relation( + monkeypatch, tmp_path +): + """The constraint: the `--ddl` file must never carry an executable statement for a + dbt-managed relation without an adjacent comment saying dbt will destroy it. + + `render_ddl` emits only the code/confidence header, the title and the DDL — `rationale`, + where every ADV302 disclosure used to live, never reaches this file. So one file held a + config block explaining that raw DDL is destroyed by `dbt run` and, below it, a bare + `CREATE INDEX` on that same dbt-managed table. Here the manifest declares an + *unrecognised* materialization, which is a real end-to-end decline path: the DDL is + deliberately kept, so the warning has to be in the file. + """ + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + ddl_path = tmp_path / "out.sql" + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--manifest", + str(_orders_manifest(tmp_path, materialized="exotic")), + "--json", + "--ddl", + str(ddl_path), + ], + ) + assert result.exit_code == 0, result.output + script = ddl_path.read_text(encoding="utf-8") + payload = json.loads(result.stdout) + + # Matched by statement text against the payload, deliberately **not** by looking for the + # relation's name in the DDL: `DROP INDEX "public"."idx_orders_cold";` names the *index*, + # so a `'"public"."orders"' in block` filter silently skipped every drop — the exact shape + # of statement that turned out to be missing its warning. + warned = _warned_statements(script) + dbt_statements = { + p["ddl"] for p in payload["proposals"] if "dbt_model" in p["evidence"] and p["ddl"] + } + plain_statements = { + p["ddl"] for p in payload["proposals"] if "dbt_model" not in p["evidence"] and p["ddl"] + } + assert dbt_statements, f"the scenario must keep executable DDL for a dbt model:\n{script}" + assert plain_statements, "and at least one statement for a relation dbt does not manage" + assert any(s.upper().startswith("CREATE INDEX") for s in dbt_statements), dbt_statements + assert any(s.upper().startswith("DROP INDEX") for s in dbt_statements), ( + f"ADV002 must fire for the dbt-managed relation: {dbt_statements}" + ) + for statement in dbt_statements: + assert warned.get(statement) is True, f"no dbt warning beside: {statement}\n{script}" + # Discriminating in the other direction: a renderer that warned on everything would + # satisfy the loop above and mean nothing. + for statement in plain_statements: + assert warned.get(statement) is False, f"spurious dbt warning beside: {statement}" + + +def test_the_ddl_file_carries_a_warning_on_every_adv302_decline_shape(monkeypatch, tmp_path): + """The same constraint over all four decline paths at once, including the two no live + workload reaches (a proposal with no plain column list, and a non-btree access method). + + `propose` is stubbed here precisely because the point is coverage of the *shapes* ADV302 + declines on rather than of the rules that produce them: `_is_index_creating` matches by + DDL prefix specifically so future rules are covered without being enumerated, so these + paths must hold for a proposal shape, not for today's four rule codes. + """ + from sqlquality.workload.postgres import PostgresWorkloadAdapter + + def _p(code, ddl, extra=None): + evidence = { + "schema": "public", + "table": "orders", + "columns": ("status",), + "cost_share": 0.5, + } + evidence.update(extra or {}) + return Proposal( + code=code, + title=f"{code} on public.orders", + rationale="r.", + evidence=evidence, + confidence=Confidence.HIGH, + ddl=ddl, + ) + + no_columns = _p("ADV008", 'CREATE INDEX ON "public"."orders" (lower("email"));') + no_columns = dataclasses.replace( + no_columns, evidence={k: v for k, v in no_columns.evidence.items() if k != "columns"} + ) + stubbed = [ + _p( + "ADV004", + 'CREATE INDEX ON "public"."orders" ("region") WHERE "deleted" IS NULL;', + {"guard_column": "deleted", "guard_predicate": "IS NULL"}, + ), + no_columns, + _p("ADV009", 'CREATE INDEX ON "public"."orders" USING gin ("payload");'), + _p("ADV001", 'CREATE INDEX ON "public"."orders" ("status");'), + ] + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + monkeypatch.setattr(PostgresWorkloadAdapter, "propose", lambda self, *a, **k: list(stubbed)) + + ddl_path = tmp_path / "out.sql" + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--manifest", + str(_orders_manifest(tmp_path)), + "--ddl", + str(ddl_path), + ], + ) + assert result.exit_code == 0, result.output + script = ddl_path.read_text(encoding="utf-8") + + warned = _warned_statements(script) + assert len(warned) == 3, f"three declines keep their DDL; ADV001 becomes config:\n{script}" + for statement, has_warning in warned.items(): + assert has_warning, f"no dbt warning beside: {statement}\n{script}" + assert "ADV004" in script and "ADV009" in script and "ADV008" in script + # And the one that *was* rewritten carries no bare statement at all. + assert "-- ADV001 [high" in script + assert " indexes:" in script.replace("--", "") + + +def test_the_terminal_says_adv302_fired(monkeypatch, tmp_path): + """ADV302 is never a proposal `code`, so an enriched row in the terminal table is + byte-identical to the same proposal from a dbt-free run — same code, confidence, cost + share and title — and the terminal never prints `rationale`. Without a line of its own a + terminal user cannot tell enrichment happened at all. + + On stderr, like every other disclosure this command makes, so stdout stays pure JSON. + """ + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--manifest", + str(_orders_manifest(tmp_path)), + "--json", + ], + ) + assert result.exit_code == 0, result.output + assert "ADV302" in result.stderr + assert "ADV302" not in result.stdout.split('"proposals"')[0] + json.loads(result.stdout) # stdout is still pure JSON + + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + without = runner.invoke(app, ["advise", "--dsn", "postgresql://u@h/db", "--json"]) + assert "ADV302" not in without.stderr, "no manifest, no rewrite, no line" + + +def test_an_explicit_manifest_wins_over_project_dir_in_both_the_load_and_the_payload( + monkeypatch, tmp_path +): + """The precedence existed twice — in `load_dbt_context` and in the CLI's payload builder — + and swapping it in *either* copy alone left the whole suite green, because no test passed + both flags. The payload could therefore name a manifest that was never read. + + Both manifests are valid and differ in model count, so the payload's `models` proves which + file was actually loaded rather than merely which path was formatted into a string. + """ + project_dir = tmp_path / "proj" + (project_dir / "target").mkdir(parents=True) + (project_dir / "target" / "manifest.json").write_text( + DBT_FIXTURE.read_text(encoding="utf-8"), encoding="utf-8" + ) # 3 models + explicit = _orders_manifest(tmp_path) # 1 model + + _stub_adapter(monkeypatch, {"pg_stat_statements": [], "pg_stat_database": [("2026-07-01",)]}) + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--project-dir", + str(project_dir), + "--manifest", + str(explicit), + "--json", + ], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + assert payload["dbt"]["manifest"] == str(explicit) + assert payload["dbt"]["models"] == 1, "the *explicit* manifest is the one that was read" + assert str(explicit) in result.stderr + + +def test_the_resort_uses_the_resolved_adapters_ranking_key(monkeypatch, tmp_path): + """The re-sort after enrichment must go through the adapter it resolved. + + It reached `PostgresWorkloadAdapter._ranking_key` — a private classmethod of one specific + adapter, from the engine-agnostic CLI — while the resolved adapter instance was in scope, + so a future engine would silently have got Postgres's ordering on the dbt path only. + Overriding the public hook must change the order the CLI emits. + """ + from sqlquality.workload.postgres import PostgresWorkloadAdapter + + class _TitleRankingAdapter(PostgresWorkloadAdapter): + """Stands in for a second engine: same rules, its own reading order.""" + + @classmethod + def ranking_key(cls, proposal): + return (proposal.title, proposal.code) + + # A *subclass*, resolved the way the CLI resolves any adapter. Patching + # `PostgresWorkloadAdapter.ranking_key` itself could not discriminate: the un-fixed code + # named that same class, so the override would have been picked up either way. + monkeypatch.setattr( + "sqlquality.cli.get_workload_adapter", lambda engine: _TitleRankingAdapter() + ) + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--manifest", + str(_orders_manifest(tmp_path)), + "--json", + ], + ) + assert result.exit_code == 0, result.output + titles = [p["title"] for p in json.loads(result.stdout)["proposals"]] + assert titles == sorted(titles), f"the overridden ranking key must be the one used: {titles}" + + +def test_the_no_manifest_run_contains_no_dbt_conditional_element_anywhere(monkeypatch, tmp_path): + """A regression guard for the branch's headline compatibility claim. + + "The no-manifest path is byte-identical to `main`" was established by a one-off manual + diff of all four artifacts; nothing in the suite performed it, so the claim was + unprotected. A diff against another commit is not something a unit test can do, but the + equivalent property is: on a run that produces real proposals and writes every artifact, + no dbt-conditional element may appear in any of them. + """ + _stub_adapter(monkeypatch, TWO_INDEXES_ON_ORDERS_ROWS) + ddl_path = tmp_path / "out.sql" + md_path = tmp_path / "out.md" + result = runner.invoke( + app, + [ + "advise", + "--dsn", + "postgresql://u@h/db", + "--json", + "--ddl", + str(ddl_path), + "--markdown", + str(md_path), + ], + ) + assert result.exit_code == 0, result.output + payload = json.loads(result.stdout) + assert payload["proposals"], "a vacuous run would satisfy every assertion below" + assert ddl_path.read_text(encoding="utf-8").count("CREATE INDEX ON") == 3, ( + "two indexes on orders and one on payments — the count pins that the artifacts are " + "non-vacuous, since a run with no DDL would satisfy every absence check below" + ) + assert "dbt" not in payload + + surfaces = { + "stdout": result.stdout, + "stderr": result.stderr, + "ddl": ddl_path.read_text(encoding="utf-8"), + "markdown": md_path.read_text(encoding="utf-8"), + } + # Every dbt-conditional element this branch can emit, each checked against every + # surface — a single "dbt" substring check would pass while ADV301 leaked. + forbidden = [ + "dbt", + "ADV301", + "ADV302", + "ADV303", + "indexes:", + "dbt_model", + "dbt_materialized", + "dbt_index_config", + "materialized as", + "manifest", + ] + for name, text in surfaces.items(): + for token in forbidden: + assert token not in text, f"{token!r} leaked into {name} on a dbt-free run" + for proposal in payload["proposals"]: + assert not any(k.startswith("dbt") for k in proposal["evidence"]), proposal diff --git a/tests/test_report_markdown.py b/tests/test_report_markdown.py index b3da15a..e36d732 100644 --- a/tests/test_report_markdown.py +++ b/tests/test_report_markdown.py @@ -143,6 +143,61 @@ def test_payload_is_json_serializable(): json.dumps(_payload()) +def test_payload_omits_the_dbt_key_when_absent(): + """Every existing caller omits `dbt`; the key must not appear at all — not even set to + `None` — so a no-manifest payload stays byte-identical to what callers got before this + key existed, rather than merely equal apart from one known extra key. A consumer that + wants the value unconditionally can still do `payload.get("dbt")`, which behaves the + same either way.""" + assert "dbt" not in _payload() + + +def test_payload_carries_the_dbt_disclosure_when_given(): + dbt = {"manifest": "/proj/target/manifest.json", "models": 3, "dropped_collisions": 1} + payload = advise_payload( + PROPOSALS, + WORKLOAD, + AGGREGATION, + engine="postgres", + redacted=True, + degraded=[], + dbt=dbt, + ) + assert payload["dbt"] == dbt + + +def test_markdown_omits_the_dbt_section_when_absent(): + """The no-manifest markdown must not mention dbt at all — this is the same additive-by- + construction constraint `advise` proves byte-for-byte against `main`, pinned here at the + renderer's own level.""" + md = render_advise_markdown( + PROPOSALS, WORKLOAD, AGGREGATION, engine="postgres", redacted=True, degraded=[] + ) + assert "dbt" not in md.lower() + + +def test_markdown_renders_the_dbt_disclosure_when_given(): + dbt = {"manifest": "/proj/target/manifest.json", "models": 3, "dropped_collisions": 2} + md = render_advise_markdown( + PROPOSALS, WORKLOAD, AGGREGATION, engine="postgres", redacted=True, degraded=[], dbt=dbt + ) + assert "## dbt enrichment" in md + assert "/proj/target/manifest.json" in md + assert "models indexed: 3" in md + assert "cross-database collisions dropped: 2" in md + + +def test_markdown_omits_the_collision_line_when_there_were_none(): + """A truthy-count check, not `"dropped_collisions" in dbt`: the key is always present + (see cli.advise), so a bare membership check would print "dropped: 0" on every run.""" + dbt = {"manifest": "/proj/target/manifest.json", "models": 1, "dropped_collisions": 0} + md = render_advise_markdown( + PROPOSALS, WORKLOAD, AGGREGATION, engine="postgres", redacted=True, degraded=[], dbt=dbt + ) + assert "## dbt enrichment" in md + assert "collisions dropped" not in md + + def test_markdown_shows_confidence_and_cost_share(): md = render_advise_markdown( PROPOSALS, WORKLOAD, AGGREGATION, engine="postgres", redacted=True, degraded=[] diff --git a/tests/test_workload_dbt.py b/tests/test_workload_dbt.py new file mode 100644 index 0000000..44f5152 --- /dev/null +++ b/tests/test_workload_dbt.py @@ -0,0 +1,1513 @@ +import dataclasses +import json +from pathlib import Path + +import pytest + +from sqlquality.dbtproject import DbtProject +from sqlquality.models import ( + Aggregation, + ColumnRole, + ColumnUsage, + Confidence, + Proposal, + Relation, + Workload, +) +from sqlquality.workload.dbt import ( + DbtContext, + _comment_block, + _manifest_warnings, + describe_rewrites, + enrich_proposals, + load_dbt_context, + parse_relation_name, + propose_materialization, + propose_unused_models, + resolve_manifest_path, +) + +FIXTURE = Path(__file__).parent / "fixtures" / "manifest_v12.json" + + +def _project() -> DbtProject: + return DbtProject.from_path(FIXTURE) + + +def _project_with_materialization(uid: str, materialized: str) -> DbtProject: + """The fixture manifest with one model's materialization changed. + + Edits a deep copy rather than a second fixture file: the point of variation is one field, + and a whole extra manifest would drift from the real one. + """ + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["nodes"][uid]["config"]["materialized"] = materialized + return DbtProject.from_manifest(raw) + + +def _index_proposal(relation, columns=("status",), code="ADV001"): + quoted = ", ".join(f'"{c}"' for c in columns) + return Proposal( + code=code, + title=f"Add index on {relation}({', '.join(columns)})", + rationale="hot predicate.", + evidence={ + "schema": relation.schema, + "table": relation.table, + "columns": tuple(columns), + "cost_share": 0.5, + }, + confidence=Confidence.HIGH, + ddl=f'CREATE INDEX ON "{relation.schema}"."{relation.table}" ({quoted});', + ) + + +@pytest.mark.parametrize( + "raw,expected", + [ + ('"dev"."main"."stg_orders"', Relation("main", "stg_orders")), + ('"main"."orders"', Relation("main", "orders")), + ("dev.main.orders", Relation("main", "orders")), + ('"dev"."main"."Weird.Name"', Relation("main", "Weird.Name")), + ], +) +def test_parse_relation_name_takes_the_last_two_parts(raw, expected): + assert parse_relation_name(raw) == expected + + +@pytest.mark.parametrize("raw", ["", "orders", '"orders"', " "]) +def test_parse_relation_name_declines_what_it_cannot_qualify(raw): + """A one-part name has no schema, and inventing one would mis-attribute.""" + assert parse_relation_name(raw) is None + + +def test_parse_relation_name_strips_surrounding_whitespace(): + assert parse_relation_name(' "main"."orders" ') == Relation("main", "orders") + + +def test_parse_relation_name_unescapes_a_doubled_quote(): + """`""` inside a quoted identifier is SQL's escape for a literal `"`, not a delimiter — + `"ord""ers"` names the table `ord"ers`, not two segments.""" + assert parse_relation_name('"main"."ord""ers"') == Relation("main", 'ord"ers') + + +@pytest.mark.parametrize( + "raw", + [ + '"db".""."t"', # an empty quoted segment can't be a schema + '"db"."sch".', # trailing dot: something was supposed to follow and didn't + '"a"."b', # unterminated quote on the last segment + # Garbage immediately after a closing quote, with no dot between. This is the shape + # the scanner's own docstring is about and the one the parametrization was missing: + # without the "the char after a quoted segment must be a dot" reject, this parses as + # three parts `a`, `b`, `y` -- the `x` vanishes and every later part shifts one slot, + # so `Relation("b", "y")` is returned for a name that names neither. + '"a"."b"xy', + ], +) +def test_parse_relation_name_declines_a_malformed_segment_rather_than_shifting(raw): + """A regex `findall` used to silently *skip* a character it couldn't match — so each of + these had a part disappear instead of being rejected, and the remaining parts shifted + one slot, misreading a `catalog.schema` pair as `schema.table`. Declining is correct: + inventing a schema from a name we couldn't fully tokenize is how a production table + gets attributed to an unrelated model.""" + assert parse_relation_name(raw) is None + + +def test_context_indexes_models_by_relation(): + context = DbtContext.from_project(_project()) + node = context.model_for(Relation("main", "stg_orders")) + assert node is not None + assert node.unique_id == "model.demo.stg_orders" + assert node.materialized == "view" + + +def test_context_excludes_non_model_resources(): + """A seed and a test are not models: proposing a materialization change for a dbt + test, or rewriting DDL because a seed shares a name, would both be nonsense. + + `DbtProject.model_ids()` is what actually guarantees this — it already filters to + `resource_type == "model"` before `DbtContext.from_project` ever sees a unique_id, so + there is no reachable guard left in this module to pin. This asserts the guarantee + itself: no seed or test unique_id ever reaches `models`. + + The exact set, not just the two negatives: with only negative assertions this test passed + against an empty model index, so it could not distinguish "seeds and tests are excluded" + from "nothing is indexed at all" — and it read as coverage for the former. + """ + context = DbtContext.from_project(_project()) + unique_ids = {node.unique_id for node in context.models.values()} + assert unique_ids == { + "model.demo.stg_orders", + "model.demo.orders", + "model.demo.customer_orders", + }, "the three models, and only them — the fixture also carries a seed and a test" + assert context.model_for(Relation("main", "raw_orders")) is None # the seed's relation + + +def test_context_skips_a_model_with_no_relation_name(): + """An ephemeral materialization is inlined as a CTE and never occupies a physical + relation, so dbt leaves its `relation_name` unset — unlike the resource-type check, + this guard *is* reachable: removing it would call `parse_relation_name(None)` and + raise, rather than just index one extra model.""" + manifest = { + "nodes": { + "model.demo.ephemeral_thing": { + "resource_type": "model", + "config": {"materialized": "ephemeral"}, + "relation_name": None, + }, + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + assert context.models == {} + + +def test_context_does_not_match_on_a_bare_table_name(): + """dbt's target schema routinely differs from the introspected one. Matching `orders` + in schema `public` to a model in schema `main` would attribute a production table to an + unrelated dev model and then rewrite its DDL.""" + context = DbtContext.from_project(_project()) + assert context.model_for(Relation("public", "orders")) is None + assert context.model_for(Relation("main", "orders")) is not None + + +def test_context_declines_a_cross_database_collision(): + """Two different databases can each have a `main.orders` — `'"prod"."main"."orders"'` + and `'"stage"."main"."orders"'` both key `Relation("main", "orders")` once the database + is dropped. `advise` connects to one database at a time, so there is no way to tell + which model actually built the relation it introspected — guessing is exactly the + failure `model_for`'s no-bare-name-fallback rule exists to prevent, applied to a + different source of ambiguity. The relation must be dropped, not resolved to whichever + unique_id happened to sort last.""" + manifest = { + "nodes": { + "model.demo.orders_prod": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"prod"."main"."orders"', + }, + "model.demo.orders_stage": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"stage"."main"."orders"', + }, + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + assert context.model_for(Relation("main", "orders")) is None + assert context.dropped_collisions == 1 + + +def test_load_returns_nothing_when_no_option_is_given(): + assert load_dbt_context(None, None) == (None, None) + + +def test_load_reads_an_explicit_manifest_and_discloses_the_source(): + context, disclosure = load_dbt_context(None, FIXTURE) + assert context is not None + assert disclosure is not None and str(FIXTURE) in disclosure + + +def test_load_reports_a_missing_manifest_without_raising(tmp_path): + """A bad manifest path must degrade to 'no enrichment', not abort a run that already + did all the catalog work — the same reasoning as the report-write failure path.""" + context, disclosure = load_dbt_context(None, tmp_path / "nope.json") + assert context is None + assert disclosure is not None and "nope.json" in disclosure + + +def test_load_reports_unparseable_json_without_raising(tmp_path): + bad = tmp_path / "manifest.json" + bad.write_text("{not json", encoding="utf-8") + context, disclosure = load_dbt_context(None, bad) + assert context is None + assert disclosure is not None + + +@pytest.mark.parametrize( + "manifest_json", + [ + "[]", + '"s"', + "42", + "null", + '{"nodes": "abc"}', + '{"nodes": null}', + '{"nodes": {"model.x": "abc"}}', + '{"nodes": {"model.x": {"resource_type": "model", "relation_name": 42, "config": {}}}}', + ], + ids=[ + "top-level-list", + "top-level-string", + "top-level-int", + "top-level-null", + "nodes-is-a-string", + "nodes-is-null", + "node-value-is-not-an-object", + "relation_name-is-not-a-string", + ], +) +def test_load_survives_a_wrong_shaped_manifest_without_raising(tmp_path, manifest_json): + """Each of these is *valid JSON* but the wrong shape for a manifest, and each raises a + different AttributeError/TypeError deep inside DbtProject or DbtContext — reproduced + end-to-end, `advise --manifest ...` used to exit 1 with a traceback for every one of + them, *after* the whole catalog analysis had already run. A narrow except list missed + all of these; each case must independently stay green.""" + bad = tmp_path / "manifest.json" + bad.write_text(manifest_json, encoding="utf-8") + context, disclosure = load_dbt_context(None, bad) + assert context is None + assert disclosure is not None + + +def test_adv302_replaces_raw_ddl_for_a_table_model_with_a_dbt_config_block(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + [out] = enrich_proposals([_index_proposal(relation)], context) + assert out.ddl is not None + assert "CREATE INDEX" not in out.ddl + assert "indexes" in out.ddl + assert "columns" in out.ddl and "status" in out.ddl + assert "type: btree" in out.ddl + assert "dbt run" in out.rationale + + +def test_adv302_keeps_the_relation_and_columns_it_was_given(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + [out] = enrich_proposals([_index_proposal(relation, ("status", "created_at"))], context) + assert "status" in out.ddl and "created_at" in out.ddl + assert out.evidence["dbt_model"] == "model.demo.orders" + assert out.evidence["dbt_materialized"] == "table" + + +def test_adv302_says_a_view_cannot_be_indexed_at_all(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "stg_orders") # materialized: view + [out] = enrich_proposals([_index_proposal(relation)], context) + assert out.ddl is None, "a view has no storage to index, so there is no DDL to run" + # The substantive claim, not just the word "view" — the generic attribution string + # `(materialized as `view`)` alone would already satisfy a bare `"view" in rationale` + # even if this sentence were stripped or replaced with something generic. + assert "has no storage of its own to index" in out.rationale + assert out.confidence is Confidence.LOW + + +def test_adv302_distinguishes_incremental_from_table(): + """An index survives a normal incremental run and is lost on --full-refresh. Saying + 'every dbt run drops it' would be false, and false in the direction that makes an + operator distrust a correct proposal.""" + project = _project_with_materialization("model.demo.orders", "incremental") + context = DbtContext.from_project(project) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"))], context) + assert "full-refresh" in out.rationale or "full refresh" in out.rationale + assert "every dbt run" not in out.rationale + + +def test_adv302_leaves_an_unrecognised_materialization_alone_and_says_so(): + project = _project_with_materialization("model.demo.orders", "exotic") + context = DbtContext.from_project(project) + original = _index_proposal(Relation("main", "orders")) + [out] = enrich_proposals([original], context) + assert out.ddl == original.ddl, "unknown materialization must not have its DDL rewritten" + assert "exotic" in out.rationale + + +def test_adv302_does_not_touch_a_relation_dbt_does_not_manage(): + context = DbtContext.from_project(_project()) + original = _index_proposal(Relation("public", "orders")) + assert enrich_proposals([original], context) == [original] + + +def test_adv302_does_not_rewrite_a_drop_index_proposal(): + """Dropping an index dbt never created is a perfectly ordinary thing to do, and there is + no `indexes` config that expresses a removal. + + `evidence` deliberately includes a `columns` tuple, the same shape ADV001/007/008 + carry: without it, a broken `_is_index_creating` that wrongly called this DROP + "index-creating" would still slip through the *separate* "no columns to express as + config" bail-out and leave `ddl` untouched by accident — passing this test for the + wrong reason instead of actually exercising the DDL-prefix guard it claims to pin. + """ + context = DbtContext.from_project(_project()) + drop = Proposal( + code="ADV002", + title="Drop unused index idx_cold on main.orders", + rationale="no scans.", + evidence={ + "schema": "main", + "table": "orders", + "index": "idx_cold", + "columns": ("status",), + }, + confidence=Confidence.MEDIUM, + ddl='DROP INDEX "main"."idx_cold";', + ) + [out] = enrich_proposals([drop], context) + assert out.ddl == drop.ddl + assert "indexes:" not in out.ddl + + +def test_adv302_does_not_rewrite_an_advisory_proposal_with_no_ddl(): + context = DbtContext.from_project(_project()) + advisory = Proposal( + code="ADV005", + title="Non-sargable predicate on main.orders.status", + rationale="wrapped in a function.", + evidence={"schema": "main", "table": "orders", "column": "status"}, + confidence=Confidence.HIGH, + ddl=None, + ) + [out] = enrich_proposals([advisory], context) + assert out.ddl is None + # It should still be attributed to the model, so the reader knows where to fix it. + assert out.evidence["dbt_model"] == "model.demo.orders" + + +def test_adv302_discloses_a_partial_index_as_not_expressible_rather_than_dropping_the_where(): + """ADV004's partial index has a WHERE clause dbt's `indexes` config has no field for. + Silently emitting a config block would lose the predicate and turn a correct proposal + into a wrong one, so it must keep its raw DDL and say dbt will drop it instead.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + partial = Proposal( + code="ADV004", + title=f"Partial index on {relation}(status) WHERE deleted_at IS NULL", + rationale="hot predicate, restricted by a null check.", + evidence={ + "schema": relation.schema, + "table": relation.table, + "columns": ("status",), + "guard_column": "deleted_at", + "guard_predicate": "IS NULL", + "cost_share": 0.5, + }, + confidence=Confidence.MEDIUM, + ddl=( + f'CREATE INDEX ON "{relation.schema}"."{relation.table}" ' + '("status") WHERE "deleted_at" IS NULL;' + ), + ) + [out] = enrich_proposals([partial], context) + assert out.ddl == partial.ddl, "the WHERE clause must survive, not be silently dropped" + assert "WHERE" in out.ddl + assert "dbt" in out.rationale and "drop" in out.rationale.lower() + + +def test_adv302_does_not_mistake_a_column_named_where_for_a_partial_index(): + """A substring search for "WHERE" in the DDL is foolable by a column literally named + `WHERE`: quoted, it is a perfectly ordinary identifier, but `CREATE INDEX ON t + ("WHERE")` contains the substring anyway. Detection keys on ADV004's own + `guard_column`/`guard_predicate` evidence instead, which this proposal does not carry, + so it must be rewritten normally rather than disclosed as an inexpressible partial + index.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + [out] = enrich_proposals([_index_proposal(relation, ("WHERE",))], context) + assert out.ddl is not None + assert "CREATE INDEX" not in out.ddl, "a column named WHERE must not block the rewrite" + assert "indexes" in out.ddl + + +def test_adv302_detects_a_lowercase_where_partial_index_via_evidence_not_text(): + """Every rule in this codebase emits an uppercase `WHERE` today, but detection must not + depend on that: keying on ADV004's `guard_column`/`guard_predicate` evidence catches a + lowercase `where` (plausible from a future engine's rule) exactly the same as an + uppercase one, whereas a text search for `"WHERE"` would silently miss it and drop the + predicate into a config block that has nowhere to put it.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + partial = Proposal( + code="ADV004", + title=f"Partial index on {relation}(status) where deleted_at is null", + rationale="hot predicate, restricted by a null check.", + evidence={ + "schema": relation.schema, + "table": relation.table, + "columns": ("status",), + "guard_column": "deleted_at", + "guard_predicate": "is null", + "cost_share": 0.5, + }, + confidence=Confidence.MEDIUM, + ddl=( + f'CREATE INDEX ON "{relation.schema}"."{relation.table}" ' + '("status") where "deleted_at" is null;' + ), + ) + [out] = enrich_proposals([partial], context) + assert out.ddl == partial.ddl, "a lowercase predicate must still be disclosed, not dropped" + + +def test_adv302_expresses_a_unique_index_with_dbts_unique_config_field(): + """`CREATE UNIQUE INDEX` is still index-creating DDL a table rebuild destroys, and + dbt's `indexes` config has a `unique` field for exactly this — so it must be rewritten + (not silently skipped) and the uniqueness preserved rather than dropped.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + unique = Proposal( + code="ADV001", + title=f"Add unique index on {relation}(email)", + rationale="hot predicate.", + evidence={"schema": relation.schema, "table": relation.table, "columns": ("email",)}, + confidence=Confidence.HIGH, + ddl=f'CREATE UNIQUE INDEX ON "{relation.schema}"."{relation.table}" ("email");', + ) + [out] = enrich_proposals([unique], context) + assert out.ddl is not None + assert "CREATE UNIQUE INDEX" not in out.ddl + assert "unique: true" in out.ddl + + +def test_adv302_recognises_create_index_concurrently_as_index_creating(): + """The DDL script's own header recommends CONCURRENTLY for a live table, so a + proposal that used it must not silently stop being detected as index-creating — + that is exactly the silent-skip the DDL-prefix requirement exists to prevent.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + concurrent = Proposal( + code="ADV001", + title=f"Add index on {relation}(status)", + rationale="hot predicate.", + evidence={"schema": relation.schema, "table": relation.table, "columns": ("status",)}, + confidence=Confidence.HIGH, + ddl=f'CREATE INDEX CONCURRENTLY ON "{relation.schema}"."{relation.table}" ("status");', + ) + [out] = enrich_proposals([concurrent], context) + assert out.ddl is not None + assert "CREATE INDEX" not in out.ddl + assert "indexes" in out.ddl + + +def test_adv302_rewrites_a_materialized_view_instead_of_calling_it_unrecognised(): + """`materialized_view` has been a real dbt materialization since dbt-core 1.6 and + supports an `indexes` config exactly like `table`/`incremental` — calling it + "unrecognised" is safe but wrong, since the operator is left with DDL a rebuild or + full refresh destroys.""" + project = _project_with_materialization("model.demo.orders", "materialized_view") + context = DbtContext.from_project(project) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"))], context) + assert out.ddl is not None + assert "CREATE INDEX" not in out.ddl + assert "indexes" in out.ddl + assert "unrecognised" not in out.rationale + assert "materialized view" in out.rationale + + +def test_adv302_config_block_survives_a_newline_in_a_column_name(): + """A newline inside a quoted identifier parses successfully (parse_relation_name + accepts one in a relation name), and a column introspected from a live catalog can + carry the same thing. The generated config block goes straight into the --ddl file as + `--`-commented lines, so an embedded raw newline there must not let the second half of + the line break out of the comment — the same hazard render_ddl already defends against + for raw DDL, reproduced for enrich_proposals' own generated block. + """ + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") # materialized: table + hostile_columns = ("sta\ntus -- DROP TABLE users;",) + [out] = enrich_proposals([_index_proposal(relation, hostile_columns)], context) + assert out.ddl is not None + for line in out.ddl.splitlines(): + assert line.startswith("--"), f"bare line outside comment mode: {line!r}" + # The hostile text must not appear as a live, uncommented statement anywhere. + executable = [ln for ln in out.ddl.splitlines() if not ln.startswith("--")] + assert not any("DROP TABLE users" in ln for ln in executable) + # Pins the specific mechanism: `list(columns)` formatting escapes the embedded + # newline into the two literal characters `\` and `n` — this is what actually + # neutralizes the hazard here, not `_comment_block`'s resplitting (see + # `test_comment_block_defends_a_raw_newline_smuggled_past_the_repr_escaping` for + # that defense pinned in isolation). + assert "\\n" in out.ddl + + +def test_comment_block_defends_a_raw_newline_smuggled_past_the_repr_escaping(): + """`_comment_block` is `enrich_proposals`' own equivalent of `render_ddl`'s per-line + comment guard. The end-to-end hazard test above never actually exercises this + function's own resplitting, because `list(columns)` formatting already escapes an + embedded newline before `_comment_block` ever sees it — so this pins the second, + independent defense directly: a raw `\\n` inside one logical line, bypassing any + repr-based escaping entirely, must still not produce a bare physical line.""" + rendered = _comment_block(["safe line", "unsafe\nline -- DROP TABLE users;", "also safe"]) + lines = rendered.splitlines() + assert len(lines) == 4 # three logical lines, one of which splits into two physical ones + for line in lines: + assert line.startswith("--"), f"bare line outside comment mode: {line!r}" + + +def _usage(relation, column, role, cost_share=0.5, cost_ms=50.0, fps=("fp1",)): + """Mirrors `tests/test_workload_rules.py`'s helper of the same name — not imported + across test modules, because that file's helper is private to it. `fps` defaults to a + single shared fingerprint, so usages co-occur unless a test deliberately gives them + disjoint sets; irrelevant to the dbt rules below (neither checks co-occurrence) but kept + for parity with the original.""" + return ColumnUsage( + relation=relation, + column=column, + role=role, + calls=10, + cost_ms=cost_ms, + cost_share=cost_share, + fingerprint_ids=frozenset(fps), + ) + + +def _aggregation(*usages, total=1000.0): + return Aggregation( + usage=tuple(usages), + total_cost_ms=total, + skipped_unqualifiable=0, + tables=frozenset(u.relation for u in usages), + ) + + +def test_adv301_proposes_materializing_a_hot_view(): + context = DbtContext.from_project(_project()) + relation = Relation("main", "stg_orders") # view + usage = _usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.4) + proposals = propose_materialization(_aggregation(usage), context, min_cost_share=0.01) + assert [p.code for p in proposals] == ["ADV301"] + assert proposals[0].confidence is Confidence.MEDIUM + assert proposals[0].evidence["dbt_model"] == "model.demo.stg_orders" + assert proposals[0].ddl is None, "changing a materialization is a config edit, not DDL" + + +def test_adv301_is_silent_for_a_model_already_materialized_as_a_table(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.9) + assert propose_materialization(_aggregation(usage), context, min_cost_share=0.01) == [] + + +def test_adv301_respects_the_cost_share_threshold(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "stg_orders"), "status", ColumnRole.EQUALITY, cost_share=0.001) + assert propose_materialization(_aggregation(usage), context, min_cost_share=0.01) == [] + + +def test_adv301_never_reaches_high_confidence(): + """The build-vs-read trade is not visible from query history, so HIGH would be a claim + about a schedule this tool cannot see.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "stg_orders"), "status", ColumnRole.EQUALITY, cost_share=0.99) + [out] = propose_materialization(_aggregation(usage), context, min_cost_share=0.01) + assert out.confidence is Confidence.MEDIUM + + +def test_adv301_ignores_relations_dbt_does_not_manage(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("public", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.9) + assert propose_materialization(_aggregation(usage), context, min_cost_share=0.01) == [] + + +def test_adv301_reports_one_proposal_per_relation_not_per_column(): + """Two hot columns on one view are one materialization decision.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "stg_orders") + proposals = propose_materialization( + _aggregation( + _usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.4), + _usage(relation, "created_at", ColumnRole.RANGE, cost_share=0.3), + ), + context, + min_cost_share=0.01, + ) + assert len(proposals) == 1 + assert proposals[0].evidence["cost_share"] == 0.4, "the max, not the sum" + + +def _workload() -> Workload: + """A minimal Workload, in the style of `tests/test_workload_aggregate.py`'s `_workload` + helper (not imported across test modules — see that file's own helper). ADV303 only + reads `window_description` off it, so an empty `stats` tuple is enough.""" + return Workload(stats=(), window_description="the last 7 days") + + +def test_adv303_flags_a_model_no_query_touched(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + codes = {p.code for p in proposals} + assert codes == {"ADV303"} + flagged = {p.evidence["dbt_model"] for p in proposals} + assert "model.demo.customer_orders" in flagged + + +def test_adv303_excludes_a_model_that_other_models_depend_on(): + """A staging model consumed by a downstream model is used. Without this gate the rule + proposes deleting every staging model in the project.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + flagged = {p.evidence["dbt_model"] for p in proposals} + assert "model.demo.stg_orders" not in flagged, "stg_orders feeds orders" + + +def test_adv303_is_capped_at_low_confidence(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + assert proposals, "the loop below is vacuous otherwise — this must pin a non-empty result" + for proposal in proposals: + assert proposal.confidence is Confidence.LOW + + +def test_adv303_states_the_window_caveat_and_the_limit_caveat(): + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + [first, *_] = propose_unused_models(_aggregation(usage), context, _workload()) + assert "window" in first.rationale + assert "--limit" in first.rationale + + +def test_adv303_emits_nothing_when_every_model_was_touched(): + context = DbtContext.from_project(_project()) + usages = [ + _usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.1) + for relation in context.models + ] + assert propose_unused_models(_aggregation(*usages), context, _workload()) == [] + + +def test_adv303_carries_no_ddl(): + """Deleting a model is a repository change with review implications; the tool must not + hand over a statement that does it.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + assert proposals, "the loop below is vacuous otherwise — this must pin a non-empty result" + for proposal in proposals: + assert proposal.ddl is None + + +def _unrelated_usage() -> ColumnUsage: + """A usage on a relation outside every fixture project used here, just enough to keep + `Aggregation.usage` non-empty so the "nothing was analysed" bail-out (see + `test_adv303_emits_nothing_when_no_usage_was_extracted`) does not swallow a test that + is not testing that guard.""" + return _usage(Relation("other", "noise"), "id", ColumnRole.EQUALITY, cost_share=0.01) + + +def test_adv303_emits_nothing_when_no_usage_was_extracted(): + """An `Aggregation` with no usage at all means nothing was analysed — every relation in + `context.models` would trivially look "untouched" by definition (none of them can be in + `aggregation.tables`, which is built only from usage), so an empty workload or a fully + unparseable one must not read as evidence that every childless model is unused.""" + context = DbtContext.from_project(_project()) + assert propose_unused_models(_aggregation(), context, _workload()) == [] + + +def test_adv303_excludes_a_model_with_exactly_one_model_child(): + """`> 0` and `> 1` both leave every other test green if the only fixture with children + happens to have two of them (`stg_orders` feeds both `orders` and `customer_orders`). + This is the commonest real shape — one staging model feeding one downstream model — so + it needs its own fixture to be pinned at all. + + `orphan` is in the fixture so the assertion discriminates: with only `parent` and `child` + the expected result is the empty set, which is also what a rule that flagged *nothing* + produces — so the test passed without the exclusion it claims to pin ever being reached. + """ + manifest = { + "nodes": { + "model.demo.parent": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."parent"', + }, + "model.demo.child": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."child"', + }, + "model.demo.orphan": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."orphan"', + }, + }, + "child_map": { + "model.demo.parent": ["model.demo.child"], + "model.demo.child": [], + "model.demo.orphan": [], + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + usage = _usage(Relation("main", "child"), "status", ColumnRole.EQUALITY, cost_share=0.5) + proposals = propose_unused_models(_aggregation(usage), context, _workload()) + flagged = {p.evidence["dbt_model"] for p in proposals} + assert flagged == {"model.demo.orphan"}, "parent has exactly one model child; orphan has none" + + +def test_adv303_excludes_a_model_whose_only_child_is_a_snapshot(): + """An exposure/snapshot is a real, dbt-declared consumer that `model_children` cannot + see because it filters to `resource_type == 'model'`. ADV303 must read the manifest's + raw child_map (via `DbtProject.child_ids`) instead, or it would propose deleting a model + dbt itself documents as being snapshotted. + + `orphan` is in the fixture for the same reason as in the one-model-child test: without a + model this rule *does* flag, the expected result is the empty set and the test passes + against a rule that flags nothing at all. + """ + manifest = { + "nodes": { + "model.demo.raw": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."raw"', + }, + "model.demo.orphan": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."orphan"', + }, + "snapshot.demo.raw_snapshot": { + "resource_type": "snapshot", + "config": {"materialized": "snapshot"}, + "relation_name": '"dev"."main"."raw_snapshot"', + }, + }, + "child_map": { + "model.demo.raw": ["snapshot.demo.raw_snapshot"], + "model.demo.orphan": [], + "snapshot.demo.raw_snapshot": [], + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + proposals = propose_unused_models(_aggregation(_unrelated_usage()), context, _workload()) + flagged = {p.evidence["dbt_model"] for p in proposals} + assert flagged == {"model.demo.orphan"}, "a snapshot is a declared consumer; orphan has none" + + +def test_adv303_excludes_a_model_whose_only_child_is_an_exposure(): + """An exposure exists in dbt specifically to declare "a BI dashboard / a downstream + tool reads this" — a mart whose only declared consumer is an exposure is exactly the + case this rule must not flag. Exposures live outside `nodes` in a real manifest, so + this only works because `child_ids` reads `child_map` directly rather than resolving + each child through `DbtProject.node`. + + `orphan` is in the fixture so the expected result is a non-empty set: as an + absence-only assertion this passed against a rule that flagged nothing. + """ + manifest = { + "nodes": { + "model.demo.mart": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."mart"', + }, + "model.demo.orphan": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."orphan"', + }, + }, + "child_map": { + "model.demo.mart": ["exposure.demo.dashboard"], + "model.demo.orphan": [], + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + proposals = propose_unused_models(_aggregation(_unrelated_usage()), context, _workload()) + flagged = {p.evidence["dbt_model"] for p in proposals} + assert flagged == {"model.demo.orphan"}, "an exposure is a declared consumer; orphan has none" + + +def test_adv303_does_not_count_a_test_as_a_consumer(): + """A `not_null` test is an assertion about a model, not a consumer of it: it does not + read the model's output for any purpose downstream would recognise, so a model whose + only child is a test is still unused and must be flagged, unlike a snapshot or an + exposure.""" + manifest = { + "nodes": { + "model.demo.mart": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."mart"', + }, + "test.demo.not_null_mart_id.abc123": { + "resource_type": "test", + "config": {"materialized": "test"}, + "relation_name": None, + }, + }, + "child_map": {"model.demo.mart": ["test.demo.not_null_mart_id.abc123"]}, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + proposals = propose_unused_models(_aggregation(_unrelated_usage()), context, _workload()) + flagged = {p.evidence["dbt_model"] for p in proposals} + assert flagged == {"model.demo.mart"} + + +def test_adv303_orders_proposals_canonically_by_relation(): + """Canonical output order is by relation, not by dbt unique_id or manifest insertion + order — chosen so the two orders disagree: `DbtProject.model_ids()` already sorts by + unique_id, so removing `propose_unused_models`'s own `sorted(context.models)` would + still pass by accident unless a model's unique_id order disagrees with its relation's + (schema, table) order, as it deliberately does here.""" + manifest = { + "nodes": { + "model.demo.a_second": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."zzz_relation"', + }, + "model.demo.b_first": { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."aaa_relation"', + }, + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + proposals = propose_unused_models(_aggregation(_unrelated_usage()), context, _workload()) + assert [p.evidence["table"] for p in proposals] == ["aaa_relation", "zzz_relation"] + + +def test_adv301_orders_proposals_canonically_by_relation(): + """Same canonical-order requirement as ADV303, pinned independently for + `propose_materialization`: without its own `sorted(by_relation)`, output would follow + the order usages were supplied in, not relation order.""" + manifest = { + "nodes": { + "model.demo.z_model": { + "resource_type": "model", + "config": {"materialized": "view"}, + "relation_name": '"dev"."main"."z_model"', + }, + "model.demo.a_model": { + "resource_type": "model", + "config": {"materialized": "view"}, + "relation_name": '"dev"."main"."a_model"', + }, + }, + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + proposals = propose_materialization( + _aggregation( + _usage(Relation("main", "z_model"), "status", ColumnRole.EQUALITY, cost_share=0.5), + _usage(Relation("main", "a_model"), "status", ColumnRole.EQUALITY, cost_share=0.5), + ), + context, + min_cost_share=0.01, + ) + assert [p.evidence["table"] for p in proposals] == ["a_model", "z_model"] + + +# --- ADV302: the generated config block ----------------------------------------------- + + +def _config_mapping(ddl: str) -> dict: + """The `indexes:` mapping inside a generated ADV302 block, parsed as YAML. + + The block exists to be pasted into a dbt model's `.yml`, and nothing asserted it was + valid YAML at all — so `list(columns)` → `tuple(columns)` survived mutation, emitting + `columns: ('status',)`, which PyYAML (dbt's own parser) rejects outright. Stripping the + `--` comment prefixes and dropping the prose above `indexes:` is exactly what a human + copying the block into a model config does. + """ + import yaml + + lines = [ln[3:] if ln.startswith("-- ") else ln[2:] for ln in ddl.splitlines()] + start = next(i for i, ln in enumerate(lines) if ln.strip() == "indexes:") + parsed = yaml.safe_load("\n".join(lines[start:])) + assert isinstance(parsed, dict), parsed + return parsed + + +def _indexes_key_lines(proposals) -> list[str]: + """Every emitted line that opens an `indexes:` mapping, across a whole run. + + Counting this is the direct form of the property: dbt reads one `indexes` key per model + config, so a second one for the same model is a duplicate YAML key and is silently + dropped. + """ + return [ + ln + for p in proposals + for ln in (p.ddl or "").splitlines() + if ln.removeprefix("--").strip() == "indexes:" + ] + + +def test_adv302_config_block_is_valid_yaml(): + """The block's only purpose is to be pasted into a `.yml`, and no test parsed it.""" + context = DbtContext.from_project(_project()) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"), ("status",))], context) + assert _config_mapping(out.ddl) == {"indexes": [{"columns": ["status"], "type": "btree"}]} + + +def test_adv302_merges_every_index_for_one_model_into_a_single_config_block(): + """Two proposals on one dbt model must produce ONE `indexes:` block, not two. + + Two standalone blocks pasted under a single model's `config:` are a duplicate YAML + mapping key, and PyYAML — dbt's parser — keeps only the last, silently discarding the + other recommended index with no error. Two survivors per relation is the *normal* case: + the adapter's collapse layer never folds non-prefix column lists, and deliberately + preserves same-set-different-order pairs. + """ + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + first = _index_proposal(relation, ("status", "created_at"), code="ADV001") + second = _index_proposal(relation, ("customer_id",), code="ADV007") + + out = enrich_proposals([first, second], context) + + assert [p.code for p in out] == ["ADV001", "ADV007"], "input order must be preserved" + assert len(_indexes_key_lines(out)) == 1, ( + "one model, one `indexes:` block — a second one is a duplicate YAML key that dbt " + "silently resolves by keeping only one of them" + ) + [owner] = [p for p in out if p.evidence.get("dbt_index_config") is True] + assert _config_mapping(owner.ddl) == { + "indexes": [ + {"columns": ["status", "created_at"], "type": "btree"}, + {"columns": ["customer_id"], "type": "btree"}, + ] + }, "both recommended indexes, in input (ranked) order, in one block" + [deferred] = [p for p in out if "dbt_index_config_reported_with" in p.evidence] + assert deferred.code == "ADV007" + assert deferred.evidence["dbt_index_config_reported_with"] == "ADV001" + assert deferred.ddl is not None + assert "CREATE INDEX" not in deferred.ddl, "still not doomed DDL" + assert "customer_id" in deferred.ddl, "must say which index it is" + assert "ADV001" in deferred.ddl, "and where the block carrying it is" + assert "ADV001" in deferred.rationale + + +def test_adv302_merges_three_indexes_and_keeps_a_unique_one_distinct(): + """A merged block must not flatten the per-entry fields: three entries, one unique.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + plain_a = _index_proposal(relation, ("status",), code="ADV001") + plain_b = _index_proposal(relation, ("customer_id",), code="ADV007") + unique = _index_proposal(relation, ("order_key",), code="ADV008") + unique = dataclasses.replace( + unique, ddl='CREATE UNIQUE INDEX ON "main"."orders" ("order_key");' + ) + + out = enrich_proposals([plain_a, plain_b, unique], context) + + assert len(_indexes_key_lines(out)) == 1 + [owner] = [p for p in out if p.evidence.get("dbt_index_config") is True] + assert _config_mapping(owner.ddl) == { + "indexes": [ + {"columns": ["status"], "type": "btree"}, + {"columns": ["customer_id"], "type": "btree"}, + {"columns": ["order_key"], "type": "btree", "unique": True}, + ] + } + + +def test_adv302_does_not_repeat_an_identical_entry_in_the_merged_block(): + """Two proposals reducing to the same index contribute one entry, not two identical ones.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + out = enrich_proposals( + [ + _index_proposal(relation, ("status",), code="ADV001"), + _index_proposal(relation, ("status",), code="ADV008"), + ], + context, + ) + [owner] = [p for p in out if p.evidence.get("dbt_index_config") is True] + assert _config_mapping(owner.ddl) == {"indexes": [{"columns": ["status"], "type": "btree"}]} + + +def test_adv302_keeps_one_block_per_model_when_two_models_are_involved(): + """Merging is per model, not per run: two dbt models get one block each.""" + context = DbtContext.from_project(_project()) + out = enrich_proposals( + [ + _index_proposal(Relation("main", "orders"), ("status",), code="ADV001"), + _index_proposal(Relation("main", "customer_orders"), ("status",), code="ADV007"), + ], + context, + ) + assert len(_indexes_key_lines(out)) == 2, "two models, two blocks — neither collides" + assert all(p.evidence.get("dbt_index_config") is True for p in out) + + +def test_adv302_config_block_names_its_model_so_two_relations_never_render_the_same_text(): + """Two relations recommending the same column list used to render byte-identical `ddl` + *and* identical evidence — distinguishable in the DDL file only by the title comment + above them, and in the JSON payload by nothing at all. The block says which model to + paste it into, so it has to name that model.""" + context = DbtContext.from_project(_project()) + [orders] = enrich_proposals( + [_index_proposal(Relation("main", "orders"), ("customer_id",))], context + ) + [customer_orders] = enrich_proposals( + [_index_proposal(Relation("main", "customer_orders"), ("customer_id",))], context + ) + assert orders.ddl != customer_orders.ddl + assert "model.demo.orders" in orders.ddl + assert "model.demo.customer_orders" in customer_orders.ddl + + +def test_adv302_never_claims_the_config_block_is_above_anything(): + """ "Add the config block above" was wrong in every surface: in markdown the fenced DDL is + *below* the rationale, in the DDL file the rationale is absent entirely, and in JSON there + is no spatial relation at all. The block replaces the DDL, so it is never "above" it.""" + context = DbtContext.from_project(_project()) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"))], context) + assert "block above" not in out.rationale + assert "above" not in out.ddl + + +def test_adv302_evidence_carries_a_flag_not_a_copy_of_the_ddl(): + """`dbt_index_config` used to hold a byte-identical copy of `ddl`. It earned nothing and + smeared the markdown Evidence line, which renders evidence as flat `k=v` pairs — so a + multi-line value landed inline with literal `\\n` escapes mid-sentence. As a flag it is + the one thing a consumer cannot get elsewhere: ADV302 is never a proposal `code`, so + `code == "ADV302"` matches nothing and this is the only way to filter for it.""" + context = DbtContext.from_project(_project()) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"))], context) + assert out.evidence["dbt_index_config"] is True + assert "\n" not in str(out.evidence["dbt_index_config"]) + + +def test_adv302_does_not_mark_a_plain_index_as_unique(): + """`_is_unique_index` → `return True` survived: every block would gain `unique: true`, + recommending a uniqueness *constraint* on a column with no evidence of being unique. + `"unique: true"` was asserted once, only for a genuinely unique index; nothing asserted + its absence.""" + context = DbtContext.from_project(_project()) + [out] = enrich_proposals([_index_proposal(Relation("main", "orders"))], context) + assert "unique" not in out.ddl + assert "unique" not in _config_mapping(out.ddl)["indexes"][0] + + +@pytest.mark.parametrize( + "ddl", + [ + # A future `CREATE TABLE`/`CREATE VIEW` rule must not be rewritten into an + # `indexes:` block: narrowing the regex to `^CREATE\\b` survived mutation, and the + # only negatives tested were `DROP INDEX` and `ddl=None`. + 'CREATE TABLE "main"."orders_new" AS SELECT * FROM "main"."orders";', + 'CREATE VIEW "main"."orders_v" AS SELECT * FROM "main"."orders";', + # `INDEX` must be a whole word: dropping the `\\b` accepted this. + 'CREATE INDEXES ON "main"."orders" ("status");', + # `.match()` → `.search()` survived: an index-creating phrase anywhere in a + # statement that is not itself index-creating must not qualify it. + 'CREATE TABLE "main"."t" AS SELECT 1; -- next step: CREATE INDEX ON "main"."t" (a)', + ], +) +def test_adv302_only_rewrites_a_statement_that_starts_by_creating_an_index(ddl): + """`evidence` deliberately carries a valid `columns` tuple, so a broken prefix check + cannot be caught by the separate "no columns to express" bail-out instead — the same + reasoning as the DROP INDEX test above.""" + context = DbtContext.from_project(_project()) + proposal = Proposal( + code="ADV999", + title="something other than an index", + rationale="r.", + evidence={"schema": "main", "table": "orders", "columns": ("status",)}, + confidence=Confidence.MEDIUM, + ddl=ddl, + ) + [out] = enrich_proposals([proposal], context) + assert out.ddl == ddl, "only index creation becomes an `indexes:` config block" + assert "indexes" not in (out.ddl or "") + assert out.evidence["dbt_model"] == "model.demo.orders", "still attributed, just not rewritten" + + +@pytest.mark.parametrize("guard", ["guard_column", "guard_predicate"]) +def test_adv302_treats_either_guard_fact_alone_as_a_partial_index(guard): + """`_is_partial_index` is a disjunction and every fixture supplied *both* facts, so + dropping either alternative survived. A partial index rewritten into a config block that + has no predicate field silently drops the WHERE clause and turns a correct proposal into + a wrong one, so each alternative has to hold on its own.""" + context = DbtContext.from_project(_project()) + proposal = _index_proposal(Relation("main", "orders"), ("region",), code="ADV004") + proposal = dataclasses.replace( + proposal, + evidence={**proposal.evidence, guard: "deleted" if guard == "guard_column" else "IS NULL"}, + ddl='CREATE INDEX ON "main"."orders" ("region") WHERE "deleted" IS NULL;', + ) + [out] = enrich_proposals([proposal], context) + assert out.ddl == proposal.ddl, "a partial index must keep its DDL, not lose its WHERE" + assert "no predicate field" in out.rationale + assert out.note is not None and "dbt WARNING" in out.note + + +@pytest.mark.parametrize( + "columns", + [ + None, # the key is absent entirely + (), # present but empty + ("status", 3), # present but not all strings + "status", # a bare string is iterable, and `list("status")` would emit characters + ], +) +def test_adv302_declines_and_discloses_when_there_is_no_plain_column_list(columns): + """The whole `columns` validation was unpinned — replacing it with `if False:` survived. + + Unreachable from today's rules, all of which populate `columns`; reachable *by design*, + because `_is_index_creating` matches on the DDL prefix precisely so a future + index-creating rule is covered without being enumerated. It used to decline in complete + silence: DDL left executable, rationale entirely unamended, only `evidence` quietly + gaining the dbt keys — the one outcome this module exists to prevent. + """ + context = DbtContext.from_project(_project()) + proposal = _index_proposal(Relation("main", "orders")) + evidence = dict(proposal.evidence) + if columns is None: + del evidence["columns"] + else: + evidence["columns"] = columns + proposal = dataclasses.replace(proposal, evidence=evidence) + + [out] = enrich_proposals([proposal], context) + + assert out.ddl == proposal.ddl, "no invented column list" + assert "indexes" not in out.ddl + assert "no plain column list" in out.rationale, "the decline must be disclosed" + assert out.note is not None and "dbt WARNING" in out.note + + +def test_adv302_declines_a_non_btree_access_method_rather_than_calling_it_btree(): + """The block is rebuilt from `evidence["columns"]` and hardcodes `type: btree`, throwing + the DDL away. Faithful for every rule today, all of which emit a plain btree over a + column list — but a `USING gin` proposal rewritten to `type: btree` hands back a + different index than the evidence justified.""" + context = DbtContext.from_project(_project()) + proposal = dataclasses.replace( + _index_proposal(Relation("main", "orders"), ("payload",)), + ddl='CREATE INDEX ON "main"."orders" USING gin ("payload");', + ) + [out] = enrich_proposals([proposal], context) + assert out.ddl == proposal.ddl + assert "non-btree access method" in out.rationale + assert out.note is not None and "dbt WARNING" in out.note + + +def test_adv302_still_rewrites_an_explicit_using_btree(): + """`USING btree` *is* btree, so it must not trip the non-btree guard. Two spaces + deliberately: a lookahead without the trailing `\\S` lets `\\s+` backtrack one space in and + refuse a plain btree index.""" + context = DbtContext.from_project(_project()) + proposal = dataclasses.replace( + _index_proposal(Relation("main", "orders"), ("status",)), + ddl='CREATE INDEX ON "main"."orders" USING btree ("status");', + ) + [out] = enrich_proposals([proposal], context) + assert _config_mapping(out.ddl) == {"indexes": [{"columns": ["status"], "type": "btree"}]} + + +def test_enrich_proposals_preserves_input_order(): + """`return out[::-1]` survived: every test passed a one-element list. The caller re-sorts + by the adapter's ranking key afterwards, and this function must not pre-empt that.""" + context = DbtContext.from_project(_project()) + proposals = [ + _index_proposal(Relation("main", "orders"), ("status",), code="ADV001"), + _index_proposal(Relation("public", "unmanaged"), ("id",), code="ADV007"), + _index_proposal(Relation("main", "stg_orders"), ("id",), code="ADV008"), + ] + out = enrich_proposals(proposals, context) + assert [p.code for p in out] == ["ADV001", "ADV007", "ADV008"] + + +def test_enrich_proposals_does_not_mutate_the_evidence_it_was_given(): + """`dict(proposal.evidence)` → `proposal.evidence` survived: nothing pinned that the + input proposals come back unchanged. `Proposal` is frozen but `evidence` is a plain dict, + so the freeze does not cover this.""" + context = DbtContext.from_project(_project()) + original = _index_proposal(Relation("main", "orders")) + before = dict(original.evidence) + out = enrich_proposals([original], context) + assert original.evidence == before, "the caller's proposal must be untouched" + assert "dbt_model" not in original.evidence + assert out[0].evidence["dbt_model"] == "model.demo.orders" + + +def test_adv302_view_branch_keeps_the_proposal_and_only_drops_the_ddl(): + """Documented for a while as "the proposal is dropped", which it is not: the finding + ("this index cannot apply here") is worth reporting, so the proposal survives at LOW with + its title and cost share and only its DDL goes.""" + context = DbtContext.from_project(_project()) + original = _index_proposal(Relation("main", "stg_orders")) + [out] = enrich_proposals([original], context) + assert out.ddl is None + assert out.title == original.title + assert out.evidence["cost_share"] == original.evidence["cost_share"] + assert out.confidence is Confidence.LOW + assert out.note is None, "no statement is emitted, so there is nothing to warn beside" + + +@pytest.mark.parametrize("absent", [None, ""]) +def test_adv302_states_an_absent_materialization_once_and_without_a_python_literal(absent): + """It used to say both "materialized as `None`" — a Python literal in an operator-facing + string — and "materialization '(absent)'", two spellings of one fact; `materialized=""` + rendered "materialized as ``", an empty code span, beside the same duplicate.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["nodes"]["model.demo.orders"]["config"]["materialized"] = absent + context = DbtContext.from_project(DbtProject.from_manifest(raw)) + original = _index_proposal(Relation("main", "orders")) + [out] = enrich_proposals([original], context) + + assert out.ddl == original.ddl, "unknown is not known-safe: the DDL is not rewritten" + assert "None" not in out.rationale + assert "``" not in out.rationale + assert "(absent)" not in out.rationale + assert out.rationale.count("materializ") == 2, ( + f"one statement of the fact plus one reason, not two spellings: {out.rationale}" + ) + assert "no materialization recorded in the manifest" in out.rationale + assert out.note is not None and "dbt WARNING" in out.note + + +def test_adv303_discloses_that_dead_chains_unwind_one_model_per_run(): + """The transitive-deadness caveat landed as a docstring only, reaching no user: a fully + dead chain surfaces one model per run and nothing explained why the parent was not + flagged the first time.""" + context = DbtContext.from_project(_project()) + usage = _usage(Relation("main", "orders"), "status", ColumnRole.EQUALITY, cost_share=0.5) + [first, *_] = propose_unused_models(_aggregation(usage), context, _workload()) + assert "not transitive" in first.rationale + assert "leaf" in first.rationale + + +def test_describe_rewrites_is_silent_when_nothing_was_rewritten(): + """The terminal line must not appear on a run where ADV302 did not fire — including a + run with a manifest that simply matched nothing.""" + assert describe_rewrites([]) is None + assert describe_rewrites([_index_proposal(Relation("public", "unmanaged"))]) is None + + +def test_describe_rewrites_reports_both_rewritten_and_folded_proposals(): + """ADV302 is never a proposal `code`, and the terminal never prints `rationale`, so an + enriched row is byte-identical to the same proposal from a dbt-free run. This line is the + only signal a terminal-only user gets that enrichment fired.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + out = enrich_proposals( + [ + _index_proposal(relation, ("status",), code="ADV001"), + _index_proposal(relation, ("customer_id",), code="ADV007"), + ], + context, + ) + line = describe_rewrites(out) + assert line is not None + assert "ADV302" in line + assert "2 index proposal(s)" in line + assert "1 folded" in line + assert "\n" not in line, "one stderr line" + + +def test_resolve_manifest_path_prefers_an_explicit_manifest_over_a_project_dir(): + """One function, because this precedence used to exist twice — in `load_dbt_context` and + again in the CLI's payload builder — and swapping it in either copy alone left the whole + suite green, so the payload could name a manifest that was never read.""" + project_dir = Path("/tmp/proj") + explicit = Path("/tmp/elsewhere/manifest.json") + assert resolve_manifest_path(project_dir, explicit) == explicit + assert resolve_manifest_path(project_dir, None) == project_dir / "target" / "manifest.json" + assert resolve_manifest_path(None, explicit) == explicit + assert resolve_manifest_path(None, None) is None + + +def test_load_warns_when_the_manifest_is_not_a_v12_schema(tmp_path): + """`check` warns on this and the dbt `advise` path checked nothing, so it silently + accepted a v10/v11 manifest whose node shapes it reads as if they were v12.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["metadata"]["dbt_schema_version"] = "https://schemas.getdbt.com/dbt/manifest/v10.json" + path = tmp_path / "manifest.json" + path.write_text(json.dumps(raw), encoding="utf-8") + context, disclosure = load_dbt_context(None, path) + assert context is not None, "a wrong schema version degrades to a warning, not a refusal" + assert "dbt_schema_version" in disclosure + assert "v10" in disclosure + + +def test_load_warns_that_the_indexes_config_is_postgres_specific_on_another_adapter(tmp_path): + """ADV302 rewrites index DDL into dbt's `indexes` model config, which only the postgres + and redshift adapters implement. Against a Snowflake or BigQuery project the rewrite is + advice for a config key that does not exist, presented as a correctness fix — and no + document said so.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["metadata"]["adapter_type"] = "snowflake" + path = tmp_path / "manifest.json" + path.write_text(json.dumps(raw), encoding="utf-8") + context, disclosure = load_dbt_context(None, path) + assert context is not None + assert "snowflake" in disclosure + assert "ADV302" in disclosure + + +@pytest.mark.parametrize("adapter_type", ["postgres", "redshift"]) +def test_load_is_quiet_for_an_adapter_that_has_the_indexes_config(adapter_type): + """The warning must discriminate: firing for postgres too would make it noise, and a + warning nobody can act on is worse than none.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["metadata"]["adapter_type"] = adapter_type + project = DbtProject.from_manifest(raw) + assert _manifest_warnings(project) == [] + + +@pytest.mark.parametrize("metadata", [{}, {"adapter_type": 12, "dbt_schema_version": None}]) +def test_load_survives_metadata_of_the_wrong_shape(tmp_path, metadata): + """`metadata` is a section some other tool wrote, and this runs after the whole catalog + analysis: a non-string version raising from an `in` test would discard real work over an + optional input.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["metadata"] = metadata + path = tmp_path / "manifest.json" + path.write_text(json.dumps(raw), encoding="utf-8") + context, disclosure = load_dbt_context(None, path) + assert context is not None + assert disclosure is not None + assert "3 model(s)" in disclosure + + +def test_the_merged_block_and_its_cross_reference_stay_fully_commented(): + """Invariant 1, over the two texts this rewrite newly interpolates raw values into. + + The deferred block interpolates the *owner proposal's title* and the model's `unique_id`, + both of which can carry a raw newline — a title is built from live catalog identifiers, + and dbt's `relation_name`/`unique_id` permit one too. Neither may produce an output line + without a leading `--`, or the DDL script shows something that reads like a statement. + """ + hostile = 'evil";\nDROP TABLE users; --' + # A newline in the *unique_id* too: a manifest is a file some other tool wrote, this key + # is interpolated raw into both texts, and unlike a column list it does not pass through + # `repr()` escaping on the way. `_comment_block`'s own re-split is the only thing + # standing between it and an uncommented output line. + uid = f"model.demo.{hostile}" + manifest = { + "nodes": { + uid: { + "resource_type": "model", + "config": {"materialized": "table"}, + "relation_name": '"dev"."main"."orders"', + } + } + } + context = DbtContext.from_project(DbtProject.from_manifest(manifest)) + relation = Relation("main", "orders") + first = _index_proposal(relation, (hostile,), code="ADV001") + second = _index_proposal(relation, ("customer_id",), code="ADV007") + + out = enrich_proposals([first, second], context) + + assert out[0].evidence["dbt_index_config"] is True, "the owner block must be exercised" + assert "dbt_index_config_reported_with" in out[1].evidence, "and the deferred one too" + for proposal in out: + lines = [ln for ln in (proposal.ddl or "").splitlines() if ln.strip()] + assert lines, proposal + assert all(ln.startswith("--") for ln in lines), lines + + +def test_a_drop_index_on_a_dbt_relation_warns_that_the_config_entry_must_go_too(): + """The mirror image of the bug ADV302 exists to fix, and it is reachable through the + ordinary rules — ADV002 and ADV003 read the catalog, not the manifest. + + "Dropping an index dbt never created is ordinary" — the original justification for + exempting drops entirely — is false in exactly the case this module cares about. If the + index is declared in the model's `indexes:` config, the next `dbt run` recreates it: the + operator drops it, dbt puts it back, and this tool proposes the same drop again next run. + That is the same silently-reverting advice ADV302 was built to eliminate. The proposal is + *not* suppressed — dropping a genuinely unused index is still right — so the operator has + to be given both halves of the instruction, in the rationale and beside the statement. + """ + context = DbtContext.from_project(_project()) + drop = Proposal( + code="ADV002", + title="Drop unused index idx_cold on main.orders", + rationale="no scans.", + evidence={"schema": "main", "table": "orders", "index": "idx_cold"}, + confidence=Confidence.MEDIUM, + ddl='DROP INDEX "main"."idx_cold";', + ) + [out] = enrich_proposals([drop], context) + + assert out.ddl == drop.ddl, "a genuinely unused index is still worth dropping" + assert out.confidence is drop.confidence + # The DDL script never carries a rationale, so the warning has to be a note as well. + assert out.note is not None + assert "dbt WARNING" in out.note + assert "model.demo.orders" in out.note + assert "indexes" in out.note and "recreates it" in out.note + assert "remove the config entry" in out.note + assert "`indexes:` config" in out.rationale + assert "will not stick" in out.rationale + + +def test_a_drop_index_on_a_relation_dbt_does_not_manage_gets_no_note(): + """Discriminating: a note on every drop would satisfy the test above and mean nothing.""" + context = DbtContext.from_project(_project()) + drop = Proposal( + code="ADV002", + title="Drop unused index idx_cold on public.orders", + rationale="no scans.", + evidence={"schema": "public", "table": "orders", "index": "idx_cold"}, + confidence=Confidence.MEDIUM, + ddl='DROP INDEX "public"."idx_cold";', + ) + assert enrich_proposals([drop], context) == [drop] + + +def test_an_advisory_proposal_for_a_dbt_relation_gets_no_note(): + """`note` renders only beside a statement in the DDL script. A proposal with no DDL + contributes no line to that file, so a note would appear nowhere at all — its `rationale` + already reaches every surface that shows it.""" + context = DbtContext.from_project(_project()) + advisory = Proposal( + code="ADV005", + title="Non-sargable predicate on main.orders.status", + rationale="wrapped in a function.", + evidence={"schema": "main", "table": "orders", "column": "status"}, + confidence=Confidence.HIGH, + ddl=None, + ) + [out] = enrich_proposals([advisory], context) + assert out.note is None + assert out.evidence["dbt_model"] == "model.demo.orders" + + +def test_any_other_kept_statement_for_a_dbt_relation_also_warns(): + """No rule emits one today, and `_is_index_creating` matches by DDL prefix specifically so + a future one stays covered. The constraint is about the *file*: no executable statement for + a dbt-managed relation without an adjacent warning, whatever the statement is.""" + context = DbtContext.from_project(_project()) + proposal = Proposal( + code="ADV999", + title="Cluster main.orders", + rationale="r.", + evidence={"schema": "main", "table": "orders", "columns": ("status",)}, + confidence=Confidence.MEDIUM, + ddl='CLUSTER "main"."orders" USING "idx_status";', + ) + [out] = enrich_proposals([proposal], context) + assert out.ddl == proposal.ddl + assert out.note is not None and "dbt WARNING" in out.note + assert "which dbt rebuilds on its own schedule" in out.rationale + + +def test_load_warns_when_the_manifest_targets_a_different_warehouse(tmp_path): + """A foreign `adapter_type` is not merely "ADV302 might emit a config key that adapter + lacks" — it means dbt is not building the Postgres relations `advise` just introspected at + all, so every `(schema, table)` match is a name coincidence and all three dbt rules are + wrong, ADV302's rebuild premise included.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + raw["metadata"]["adapter_type"] = "snowflake" + path = tmp_path / "manifest.json" + path.write_text(json.dumps(raw), encoding="utf-8") + _context, disclosure = load_dbt_context(None, path) + assert "snowflake" in disclosure + assert "do not build the relations being introspected" in disclosure + + +def test_load_warns_when_the_manifest_records_no_adapter_type(tmp_path): + """Deliberate: `dbt compile` always writes an `adapter_type`, so absence means a + hand-written or truncated manifest. Warning on "different" while staying silent on + "unknown" would make silence mean two things — enrichment consistent with the connection, + or unchecked. `check` draws the same distinction rather than assuming.""" + raw = json.loads(FIXTURE.read_text(encoding="utf-8")) + del raw["metadata"]["adapter_type"] + path = tmp_path / "manifest.json" + path.write_text(json.dumps(raw), encoding="utf-8") + context, disclosure = load_dbt_context(None, path) + assert context is not None, "an unverifiable pairing still enriches, with a warning" + assert "records no adapter_type" in disclosure + # And it must not claim a *schema version* problem the manifest does not have. + assert "dbt_schema_version" not in disclosure diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index 3902d0a..ef11185 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -968,10 +968,25 @@ def test_the_ranking_key_ignores_a_boolean_cost_share(): evidence={"cost_share": 0.5}, confidence=Confidence.HIGH, ) - key = PostgresWorkloadAdapter._ranking_key + key = PostgresWorkloadAdapter.ranking_key assert key(hot) < key(stray) +def test_the_ranking_key_is_public_on_the_adapter_interface(): + """`cli.advise` re-sorts after dbt enrichment and needs *this adapter's* order. + + It used to reach `PostgresWorkloadAdapter._ranking_key` directly — a private classmethod + of one specific adapter, from the engine-agnostic CLI — so a second engine would have + silently got Postgres's ordering on the dbt path and its own everywhere else. Ordering + belongs to the adapter, so the hook has to be on the ABC and public; pinning both here + keeps the CLI from needing an engine-specific import to sort a list. + """ + from sqlquality.workload.base import WorkloadAdapter + + assert "ranking_key" in vars(WorkloadAdapter), "the hook must live on the ABC, not one adapter" + assert PostgresWorkloadAdapter.ranking_key.__func__ is WorkloadAdapter.ranking_key.__func__ + + def test_the_schema_statement_runs_once_per_run(): """CAP_SCHEMA was executed by both fetch_schema and fetch_table_facts. diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index 3a1a902..ebee524 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -15,6 +15,7 @@ from sqlquality.workload.postgres import ( PgIndex, PostgresWorkloadAdapter, + _is_fully_commented, _quote_ident, propose_grouping_indexes, propose_indexes, @@ -1743,6 +1744,17 @@ def test_render_ddl_emits_a_reviewable_commented_script(): assert "review" in script.lower() +def _uncommented(script: str) -> list[str]: + """Every non-blank line of a rendered DDL script that is not a `--` comment. + + The one property the whole script format exists to provide is that nothing unintended is + executable, so tests assert over this list *exactly* rather than over a per-line + "comment or looks like a statement" disjunction — which any injected line ending in a + semicolon satisfies. + """ + return [line for line in script.splitlines() if line.strip() and not line.startswith("--")] + + def test_render_ddl_never_emits_a_bare_uncommented_line(): """The one property this file exists to guarantee: safe to skim, nothing unintended. @@ -1761,12 +1773,10 @@ def test_render_ddl_never_emits_a_bare_uncommented_line(): ), ] script = PostgresWorkloadAdapter().render_ddl(proposals) - for line in script.splitlines(): - if not line.strip(): - continue - assert line.startswith("--") or line.rstrip().endswith(";"), ( - f"bare non-comment, non-statement line in generated script: {line!r}" - ) + # An exact list, not "comment or ends in a semicolon": that disjunction is satisfied by + # *any* bare line ending in `;`, which is precisely the thing being smuggled, so it could + # not distinguish a clean script from one carrying an injected statement. + assert _uncommented(script) == ["CREATE INDEX ON t (c);"], script assert "-- line2 -- injected" in script @@ -1809,6 +1819,40 @@ def test_render_ddl_recommends_concurrently_for_index_creation(): assert "CONCURRENTLY" in script +def test_render_ddl_emits_a_pre_commented_multiline_block_verbatim_with_its_header(): + """A proposal can arrive with `ddl` already a `--`-commented, multi-line disclosure + rather than raw executable DDL — dbt enrichment's ADV302 rewrite is exactly this + shape. Before this test, the line-break guard treated *any* multi-line `ddl` as the + identifier-with-an-embedded-break hazard: it double-commented every line, printed a + false "an identifier ... contains a line break", and — because that whole branch + `continue`s before the header is appended — dropped the `-- ADV001 [confidence]` line + a reader needs to know which rule this is and how confident it was. A `ddl` value that + is already fully commented is not that hazard and must be emitted verbatim, with its + usual header. + """ + config_block = ( + "-- ADV302: express this as dbt config, not DDL. Add to the model's config block:\n" + "-- indexes:\n" + "-- - columns: ['status']\n" + "-- type: btree" + ) + proposals = [ + Proposal( + code="ADV001", + title="Add index on orders(status)", + rationale="hot predicate.", + evidence={"cost_share": 0.5}, + confidence=Confidence.HIGH, + ddl=config_block, + ), + ] + script = PostgresWorkloadAdapter().render_ddl(proposals) + assert "-- ADV001 [high, 50.0% of workload cost]" in script + assert "NOT RENDERED" not in script + assert "-- -- indexes:" not in script, "must not be double-commented" + assert script.count("- columns: ['status']") == 1 + + def test_generated_ddl_quotes_identifiers(): """Unquoted identifiers break on anything needing quotes — mixed case, reserved words.""" relation = Relation("public", "Order") @@ -1857,13 +1901,10 @@ def test_a_newline_in_an_identifier_is_not_rendered_as_a_statement(): ) script = PostgresWorkloadAdapter().render_ddl(proposals) assert "NOT RENDERED" in script - for line in script.splitlines(): - if not line.strip(): - continue - assert line.startswith("--") or line.rstrip().endswith(";"), f"bare line: {line!r}" - # No executable statement mentions the smuggled text — it survives only as a comment. - executable = [ln for ln in script.splitlines() if not ln.startswith("--")] - assert not any("DROP TABLE users" in ln for ln in executable) + # Nothing at all is executable here: the whole statement was commented out, so the + # smuggled text survives only as a comment. Asserting the empty list rather than + # "comment or ends in `;`" — a bare `DROP TABLE users; --` satisfies that disjunction. + assert _uncommented(script) == [], script # The real name is still recoverable, so an operator can see what was anomalous. assert "DROP TABLE users; --" in script @@ -2634,3 +2675,134 @@ def test_adv007_orders_equal_cost_join_keys_by_column_name(): for p in propose_join_keys(tuple(reversed(forward)), facts, {}, min_cost_share=0.01) ] assert reversed_columns == columns + + +def _ddl_proposal( + ddl: str = 'CREATE INDEX ON "main"."orders" ("status");', note: str | None = None +) -> Proposal: + """A minimal DDL-carrying proposal for the renderer tests below.""" + return Proposal( + code="ADV001", + title="Add index on orders(status)", + rationale="hot predicate.", + evidence={"cost_share": 0.5}, + confidence=Confidence.HIGH, + ddl=ddl, + note=note, + ) + + +def test_a_proposal_note_is_emitted_as_comment_lines_above_its_statement(): + """`rationale` never reaches this file — only code, confidence, cost share and title do. + + So a caveat that lives only in `rationale` is invisible to the one person acting on the + statement, which is how a `--ddl` file came to hold a dbt config block explaining that + raw DDL is destroyed by `dbt run` and, below it, a bare `CREATE INDEX` on that same + dbt-managed table. `note` is the field that travels with the statement, and it has to be + *above* it: a caveat printed below is a caveat read after pasting. + """ + script = PostgresWorkloadAdapter().render_ddl( + [ + _ddl_proposal( + note="dbt WARNING: rebuilt by dbt model model.demo.orders.\nReapply by hand." + ) + ] + ) + lines = script.splitlines() + statement = lines.index('CREATE INDEX ON "main"."orders" ("status");') + assert lines[statement - 2] == "-- dbt WARNING: rebuilt by dbt model model.demo.orders." + assert lines[statement - 1] == "-- Reapply by hand." + assert _uncommented(script) == ['CREATE INDEX ON "main"."orders" ("status");'], script + + +def test_a_multiline_note_cannot_break_out_of_comment_mode(): + """A note is interpolated from a manifest's `unique_id`, which can carry a newline. + + Rendered through `_comment_lines`, every physical line gets its own `--`, so a note is + subject to the same guarantee as a title. + """ + script = PostgresWorkloadAdapter().render_ddl( + [_ddl_proposal(note="warning\nDROP TABLE users; -- injected")] + ) + assert "-- DROP TABLE users; -- injected" in script + assert _uncommented(script) == ['CREATE INDEX ON "main"."orders" ("status");'], script + + +def test_a_note_survives_the_not_rendered_fallback(): + """The fallback tells an operator to apply the statement by hand, so a caveat about + whether the statement is even durable belongs there too.""" + script = PostgresWorkloadAdapter().render_ddl( + [_ddl_proposal(ddl='CREATE INDEX ON "main"."or\nders" ("status");', note="dbt WARNING: x")] + ) + assert "NOT RENDERED" in script + assert "-- dbt WARNING: x" in script + assert _uncommented(script) == [], script + + +def test_a_proposal_without_a_note_renders_exactly_as_before(): + """`note` defaults to None and must add nothing at all when unset — every dbt-free run + is this case, and the pre-dbt DDL script has to stay byte-identical.""" + script = PostgresWorkloadAdapter().render_ddl([_ddl_proposal()]) + assert script == PostgresWorkloadAdapter().render_ddl([_ddl_proposal(note=None)]) + assert ( + "-- ADV001 [high, 50.0% of workload cost]\n" + "-- Add index on orders(status)\n" + 'CREATE INDEX ON "main"."orders" ("status");' + ) in script + + +def test_a_carriage_return_in_an_identifier_is_not_rendered_as_a_statement(): + """The line-break guard tests `"\\n" in ddl or "\\r" in ddl` and only the `\\n` half was + pinned: dropping the `\\r` half left every test green while a CR-only break — which + `str.splitlines()` splits on, so the file really does show two physical lines — bypassed + the fallback entirely and emitted something reading like a bare statement.""" + script = PostgresWorkloadAdapter().render_ddl( + [_ddl_proposal(ddl='CREATE INDEX ON "main"."or\rders" ("status");')] + ) + assert "NOT RENDERED" in script + assert _uncommented(script) == [], script + + +def test_is_fully_commented_requires_every_line_to_be_a_comment(): + """The guard that lets a pre-commented multi-line `ddl` skip the NOT-RENDERED fallback. + + It is the check standing between `render_ddl` and its own core promise, and it had no + direct test: `all(...)` → `any(...)` left the whole suite green while + `ddl='-- note\\nDROP TABLE users;'` was emitted verbatim, bare and executable. Each case + below kills a distinct leniency mutation — `any`, `startswith("-")`, tolerating blank + lines, and `.lstrip()`-ing before the check — so the guard cannot be widened silently. + """ + assert _is_fully_commented("-- one line") is True + assert _is_fully_commented("-- indexes:\n-- - columns: ['status']") is True + # `any` instead of `all`: a first line that is a comment must not vouch for the rest. + assert _is_fully_commented("-- note\nDROP TABLE users;") is False + assert _is_fully_commented("DROP TABLE users;\n-- note") is False + # A single dash is not a SQL comment; `startswith("-")` would accept this. + assert _is_fully_commented("- note\n- more") is False + # A blank line is not a comment line. This one is about intent rather than safety — a + # blank line is inert either way, and a raw statement after one is still rejected (the + # next case) — but tolerating it silently widens which `ddl` values skip the fallback, + # and no generated block needs it: `_comment_block` prefixes every line it emits. + assert _is_fully_commented("-- note\n\n-- more") is False + assert _is_fully_commented("-- note\n\nDROP TABLE users;") is False + # An indented comment is not safe to emit verbatim: `psql` is fine with it, but the + # guard's premise is "already inert on every line as written", and `.lstrip()` would + # extend it to text this renderer has no other reason to trust. + assert _is_fully_commented(" -- note") is False + # Nothing at all is not "every line is a comment". + assert _is_fully_commented("") is False + + +def test_a_partially_commented_ddl_is_never_emitted_bare(): + """The invariant `_is_fully_commented` protects, asserted through the renderer. + + A multi-line `ddl` whose first line is a comment and whose later lines are raw + statements is exactly the hazard the fallback exists for, and widening the guard routes + it around the fallback and prints it verbatim. + """ + script = PostgresWorkloadAdapter().render_ddl( + [_ddl_proposal(ddl="-- a note\nDROP TABLE users;")] + ) + assert "NOT RENDERED" in script + assert _uncommented(script) == [], script + assert "-- DROP TABLE users;" in script