diff --git a/CHANGELOG.md b/CHANGELOG.md index 85085b3..ec7762e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- `sqlquality advise --engine redshift` — reads Redshift query history + (`sys_query_history`) and catalog metadata (`svv_columns`, `svv_table_info`, + `svv_alter_table_recommendations`) over a read-only connection and proposes SORTKEY + (ADV101), DISTKEY (ADV102) and DISTSTYLE ALL (ADV103) changes, VACUUM/ANALYZE + maintenance (ADV104), and relays Amazon Redshift Advisor's own recommendations verbatim + (ADV105), attributed as Advisor's rather than sqlquality's. Redshift has no indexes, so + none of ADV001–ADV008 apply; ADV101/102/103 each rewrite the entire table (no + `CONCURRENTLY` escape exists on Redshift), which the generated `--ddl` script's header + says loudly, and which caps those three rules at MEDIUM confidence — Redshift exposes no + per-column NDV to judge predicate selectivity or distribution skew. ADV104 is the + exception: its evidence is a direct catalog measurement and its remediation does not + rewrite anything, so it is the only Redshift rule that reaches HIGH. A dbt-managed + Redshift model's table-rewrite proposal is disclosed (not silently applied) via the same + dbt-enrichment path Postgres's index proposals use. **Redshift's introspection SQL has + not been executed against a live cluster** — see the README's `advise` section for what + is and is not verified, and run `--dry-run` before pointing this at production. - `sqlquality advise` — reads Postgres query history (`pg_stat_statements`) and catalog metadata over a read-only connection and proposes indexes, index removals, partial indexes, sargability fixes and `SELECT *` cleanups (ADV001–ADV008), with a `--json` @@ -72,6 +88,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- dbt enrichment now discloses itself in the terminal on **every** engine. The stderr + disclosure line counted only ADV302's config-block rewrite, which no Redshift proposal can + reach (nothing Redshift emits is a `CREATE INDEX`), so a `--project-dir` run on Redshift + warned in `rationale` and in the `--ddl` note that `dbt run` may undo an hours-long + full-table rewrite while the terminal row stayed byte-identical to a dbt-free run. Any + proposal whose DDL cannot be expressed as dbt config is now counted and reported too. - `IS NOT NULL` predicates were classified as `IS NULL` when sqlglot 30.13 or newer was installed, because that release moved the negation from a wrapping `Not` node onto a `negate` flag on the `Is` node itself. Both encodings are now read. This was not cosmetic: diff --git a/README.md b/README.md index 5df5a8a..bd09e5d 100644 --- a/README.md +++ b/README.md @@ -268,9 +268,30 @@ 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 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. +**Postgres** and **Redshift** are implemented; Snowflake is designed but not built — see +[Limitations](#limitations). An optional dbt manifest enriches the same analysis — see +[dbt enrichment](#dbt-enrichment-optional) below. + +**What is proven for Redshift, and what is not — read this before pointing `--engine +redshift` at a production cluster.** There is no Redshift container available for +development, and Postgres — where every other engine's introspection SQL gets exercised +during tests — does not implement Redshift's `svv_*`/`sys_*` system views at all, so +nothing in this adapter can be run against a real Redshift cluster before release. What +*is* verified: the **connection path** (Redshift speaks the PostgreSQL wire protocol, so +the read-only session, the statement timeout and secret scrubbing are exercised live +against a real Postgres server); every introspection **statement's syntax**, checked with +sqlglot's `redshift` dialect; and every statement's **bindability** — that its parameters +can actually be prepared and sent over the wire — proven live against stand-in tables +shaped like the real views. What is **not** verified: the **column names and the +semantics of the resulting proposals**. Those come from AWS's published system-view +documentation, not from an observed row, and have never been executed against a live +cluster. A wrong column name degrades one capability (recorded in `degraded`, never a +crash — see the Redshift section below), but it can still mean thin or wrong evidence. +Run `sqlquality advise --engine redshift --dry-run` first: it prints every statement this +adapter can issue, with no connection at all, so you can review it — or hand it to a DBA +— before `advise` ever touches your cluster. If you run this against a real cluster, +please [open an issue](https://github.com/hanslemm/sqlquality/issues) with what you found; +the first user with a cluster is part of closing this gap, not just a consumer of it. ```console $ sqlquality advise --dsn postgresql://readonly@db.internal/analytics @@ -308,7 +329,7 @@ missing driver degrades with an install hint instead of a traceback. | Flag | Default | Effect | |---|---|---| -| `--engine` | inferred | `postgres` is the only engine implemented today. | +| `--engine` | inferred | `postgres` or `redshift`. See the Redshift section below for what is and is not proven on that engine. | | `--dsn` | — | Database URL. Overrides `SQLQUALITY_DSN`. | | `--profile` | — | dbt profile name, read from `profiles.yml`. | | `--target` | — | dbt target within the profile. | @@ -317,7 +338,7 @@ missing driver degrades with an install hint instead of a traceback. | `--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. | +| `--limit` | `500` | Max query-history rows to read. **On Redshift this counts *executions*, not query groups** — see the Redshift section below. | | `--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). | @@ -347,7 +368,12 @@ SELECT stats_reset ``` (truncated here; the real output also lists the `schema`, `table_facts`, `ndv` and -`indexes` capabilities). `--json` is honored on `--dry-run` too, so the statement list can +`indexes` capabilities). `sqlquality advise --engine redshift --dry-run` works exactly the +same way — no credentials needed, no connection made — and is the recommended way to +review Redshift's introspection SQL yourself (or hand it to a DBA) before trusting it with +a real cluster; see the note at the top of the [Redshift +section](#redshift---engine-redshift) for what is and is not verified about it. `--json` +is honored on `--dry-run` too, so the statement list can be diffed or fed into review tooling. **Data protection.** Query history routinely contains personal data inside predicates @@ -423,6 +449,94 @@ ADV302 rewrote. See [dbt enrichment](#dbt-enrichment-optional). Every proposal's evidence renders inline (cost share, calls, fingerprints, row estimate, NDV, existing index state) so it can be judged from the report alone. +#### Redshift (`--engine redshift`) + +Redshift has no indexes at all, so none of ADV001–ADV008 apply. Its physical-design +levers are different, and so is the blast radius: **ADV101, ADV102 and ADV103 each +rewrite the entire table.** Redshift copies every row, holds a lock on the table for the +whole rewrite, and needs roughly the table's own size again in free disk space while it +runs — on a large table that is hours, not seconds. Unlike a Postgres `CREATE INDEX +CONCURRENTLY`, **there is no concurrent-build escape on Redshift**: none of the three can +be applied alongside normal traffic. Schedule them for a maintenance window; do not run +them ad hoc. `--ddl`'s generated script says this loudly, at the top of the file, not only +beside each individual statement. + +| Code | Proposal | Table rewrite? | Evidence | +|---|---|---|---| +| ADV101 | `ALTER TABLE ... ALTER SORTKEY`: sort the table on its hottest range/equality predicate column, capped at MEDIUM | **Yes** | cost share, current sort key, `stats_off` staleness | +| ADV102 | `ALTER TABLE ... ALTER DISTKEY`: distribute the table on its hottest join predicate column, capped at MEDIUM | **Yes** | cost share, current distribution style, `skew_rows`, `stats_off` | +| ADV103 | `ALTER TABLE ... ALTER DISTSTYLE ALL`: replicate a small (≤1,000,000-row), frequently-joined dimension to every node, capped at MEDIUM | **Yes** | cost share, row estimate, current distribution style | +| ADV104 | `VACUUM` (unsorted region ≥20%) and/or `ANALYZE` (stale statistics ≥20%), each its own proposal | **No** — reclaims sort order or refreshes statistics in place; no exclusive lock for its duration | `unsorted`, `stats_off` (direct catalog measurements) | +| ADV105 | Amazon Redshift Advisor's own SORTKEY/DISTSTYLE recommendation, relayed verbatim | Whatever Advisor recommends — read its own `note` | attributed as Advisor's, not sqlquality's | + +**ADV101, ADV102 and ADV103 can never reach HIGH confidence, by design, not merely by +current implementation.** Whether a SORTKEY, DISTKEY or DISTSTYLE ALL change is actually +worth its rewrite depends on the predicate's selectivity and the table's distribution +skew — and Redshift exposes no per-column distinct-value statistics (no `pg_stats +.n_distinct` equivalent) to measure either. Claiming HIGH would assert something about +data distribution this tool cannot see, while recommending a statement that rewrites the +whole table. ADV104 is the exception: `unsorted`/`stats_off` are direct catalog +measurements, not an inference about data this tool cannot see, and its remediation does +not rewrite anything — so it is also the only Redshift rule that can reach HIGH. + +**ADV105 is Redshift Advisor's own recommendation, never sqlquality's inference — and it +says so everywhere a reader might look.** Its title, rationale, evidence +(`evidence.source == "amazon_redshift_advisor"`) and `note` all attribute it explicitly, +and the DDL script marks its header line `(Amazon Redshift Advisor — not sqlquality)` on +top of that — someone skimming only header lines, never the prose, still cannot mistake +an Advisor statement for one this tool generated. When ADV101/102/103 and an Advisor row +agree on the same table and category, the sqlquality proposal's rationale says so as an +added sentence; the two stay separate proposals rather than merging, so it is always +clear which conclusion is whose. When ADV103 (DISTSTYLE ALL) and ADV102 (DISTKEY) both +fire for the same table, only ADV103 survives — replicating to every node already removes +redistribution for every join, which strictly subsumes any single-column DISTKEY choice — +and the surviving proposal says so. + +A relation with a hot predicate but absent from Redshift's own physical-design catalog +(`svv_table_info`) gets **no** ADV101/102/103 proposal at all, and the run discloses how +many relations this affected (`reduced coverage — physical_facts_gap: ...`) rather than +silently dropping them: that absence cannot, by itself, tell an external Spectrum table +(which cannot carry a SORTKEY/DISTKEY/DISTSTYLE) apart from a genuinely empty local one, +and proposing a rewrite for something that might not even support one is worse than +proposing nothing. + +**The workload can come back silently partial — grant `SYSLOG ACCESS UNRESTRICTED` first.** +`advise` reads `sys_query_history`, and without that privilege Redshift does not deny the +read: it returns **only the connecting user's own queries**. There is no error, no denied +capability and nothing in `degraded` — a cluster whose whole workload is invisible to your +read-only role looks exactly like a quiet cluster with little traffic, and every proposal is +then built from one user's slice of it. Grant it before your first run: + +```sql +ALTER USER SYSLOG ACCESS UNRESTRICTED; -- superuser-only +``` + +`--dry-run` prints this same warning beside the statement it applies to, and the hint is +also recorded in `degraded` **if** the read is refused outright — but the failure described +here is precisely the one that is never refused, so the hint alone is not disclosure. This +is the same class of trap as Postgres's `pg_stats`, and unlike a missing grant it costs you +coverage rather than a capability. + +**`--limit` means executions on Redshift, not query groups.** `sys_query_history` is one +row per *execution*, where Postgres's `pg_stat_statements` is already aggregated per +normalised statement — so `--limit 500` reads the 500 most expensive **executions**, and 500 +executions of one bad query is a legal outcome that leaves every other statement unseen. +The `window:` line names what was actually read ("the 500 most expensive successful queries +…"); raise `--limit` if the coverage line shows fewer query groups than you expect. + +**dbt interaction.** [ADV302](#dbt-enrichment-optional) rewrites `CREATE INDEX` +proposals into dbt `indexes:` config, which has no Redshift equivalent (SORTKEY/DISTKEY +have no comparable dbt config key modeled by this tool). Rather than leave a dbt-managed +Redshift model's table-rewrite proposal silently unwarned — which would be worse than the +Postgres case ADV302 exists to fix, since the wasted work is hours rather than seconds — +`enrich_proposals`'s existing generic path (built for any DDL that is not `CREATE INDEX` +or `DROP INDEX`) already recognises ADV101–105 and attaches a warning to both the +proposal's `rationale` **and** its `--ddl` `note`: that the relation is dbt-managed, that +the statement is not expressed as dbt config, and that it may not survive the model's next +rebuild. This is not the same thing as the `adapter_type` mismatch warning: a Redshift dbt +project correctly records `adapter_type: redshift`, so that check does not fire — this is +a separate, always-on warning specific to table-rewrite and maintenance statements. + **How overlapping proposals are reconciled.** The rules above are evaluated independently, but their output is not shipped independently: two of them can reach the same index from different evidence, and following both would mean creating a redundant pair that ADV003 then @@ -1013,10 +1127,47 @@ 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 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. +- **Snowflake is designed but not implemented.** `advise` supports `postgres` and + `redshift` today; passing `--engine snowflake` (or anything else unrecognised) fails + with a clear error rather than silently degrading. +- **Redshift's catalog SQL has not been executed against a live Redshift cluster.** See + the prominent note at the top of the [Redshift section](#redshift---engine-redshift): + the connection path is verified live (Redshift speaks the Postgres wire protocol), every + statement's syntax and bindability are verified live, but the column names and the + resulting proposals' semantics come from AWS documentation, not an observed row. Run + `--dry-run` first and review before connecting to a production cluster. +- **Redshift declares no NDV and no index capability**, deliberately: Redshift exposes no + `pg_stats.n_distinct` equivalent, and it has no indexes at all — its levers are SORTKEY, + DISTKEY/DISTSTYLE and VACUUM/ANALYZE staleness. This is why ADV101/102/103 can never + reach HIGH confidence (see the [Redshift section](#redshift---engine-redshift)), not a + gap left for a later release. +- **A relation absent from `svv_table_info` is ambiguous, not conclusive.** Redshift omits + both external (Spectrum) tables and genuinely empty local tables from that view, and + nothing else this adapter reads can tell the two apart — so a relation missing from it + gets no ADV101/102/103 proposal at all rather than a guess either way, and the run + discloses how many relations this affected. +- **Redshift's workload is silently partial without `SYSLOG ACCESS UNRESTRICTED`.** + `sys_query_history` returns only the connecting user's own queries to a role lacking that + privilege, and it does so with **no error at all** — so a cluster whose traffic your + read-only role cannot see is indistinguishable from a quiet one, and every `cost_share` is + computed over one user's slice. This is the one Redshift failure mode with no signal + anywhere in the run; see the [Redshift section](#redshift---engine-redshift) for the grant. +- **`--limit` counts executions on Redshift and query groups on Postgres.** + `sys_query_history` is per-execution; `pg_stat_statements` is pre-aggregated per normalised + statement. So on Redshift `--limit 500` means "the 500 most expensive executions", and 500 + executions of a single bad query is a legal outcome that hides every other statement. The + `window:` line always says which was read. +- **Identifier case and attached comments can split one Redshift statement into several + query groups.** `sys_query_history` stores the *verbatim* text the client sent, unlike + `pg_stat_statements`, which Postgres has already parsed and re-serialised (identifiers + folded to lowercase) before storing. So two executions of what is semantically one + statement still fingerprint separately when they differ only in identifier case or in an + attached comment — an ORM query tag, for instance. That inflates the number of query groups + the window's total cost is spread over, which shrinks every `cost_share` and makes + `--min-cost-share` correspondingly stricter, in the same way the `cost_share` and PL/pgSQL + caveats above do. Not "fixed" by case-folding before fingerprinting: nothing there can tell + an unquoted (case-insensitive) identifier from a deliberately quoted, case-sensitive one, so + a general fold risks collapsing a real distinction instead of a spurious one. - **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 diff --git a/docs/superpowers/plans/2026-07-28-advise-redshift.md b/docs/superpowers/plans/2026-07-28-advise-redshift.md new file mode 100644 index 0000000..b7fcc25 --- /dev/null +++ b/docs/superpowers/plans/2026-07-28-advise-redshift.md @@ -0,0 +1,467 @@ +# Advise Redshift Adapter Implementation Plan (Batch 3b) + +> **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:** `sqlquality advise --engine redshift` reads Redshift query history and catalog metadata over a read-only connection and proposes **distribution and sort key** changes — not indexes, which Redshift does not have. + +**Architecture:** A new `RedshiftWorkloadAdapter` implementing the existing `WorkloadAdapter` ABC. Everything above the adapter is reused unchanged: ingest, redaction, the `Relation`-keyed rollup, the proposal collapse, the report renderers, and the dbt enrichment layer from Batch 3a. Only the four things an adapter owns are new — its driver session, its introspection statements, its proposal rules, and its DDL syntax. + +**Tech Stack:** Python 3.11+, psycopg 3 (Redshift speaks the PostgreSQL wire protocol), sqlglot 30.x with its `redshift` dialect, typer, rich, pytest. + +## Global Constraints + +Every task's requirements implicitly include this section. + +- **All four CI gates before every commit:** `uv run ruff check .`, `uv run ruff format --check .`, `uv run mypy src/sqlquality`, `uv run pytest -q`. The `no-extras` and `highest-deps` jobs must also stay green. +- **`uv run pytest` must report `N passed, M deselected`, never `skipped`.** +- **sqlquality never executes user SQL.** `advise` opens a read-only session with a statement timeout and runs only the statements the adapter declares. Generated DDL goes to a file for human review. +- **No credential and no user literal reaches stdout, stderr, a report, or an exception message.** Reuse `workload/secrets.py`'s `secrets_for`/`scrub` — do not write a second credential path. +- **One missing grant costs one capability, never the run.** Record it in `self.degraded` and continue. +- **Confidence never overstates evidence.** A check that could not run is disclosed, not assumed. +- **No existing Postgres behaviour may change.** The `postgres` engine's output must be byte-identical; a shared helper may be extracted, but not altered in effect. +- **A test that passes with the production change reverted is not a test.** Mutate the line each test claims to pin and report the failure. Where a test asserts over a set, it must discriminate for **every** member — this feature has produced **eight** findings of that shape, most recently a wiring call that could be deleted with all 665 tests green. + +## The honesty constraint that shapes this whole plan + +**None of the catalog SQL in this plan can be executed against a real Redshift cluster during development.** There is no Redshift container, and its `svv_*` / `sys_*` / `stl_*` catalogs do not exist in Postgres. That matters more here than usual: on this feature's three predecessor branches, *every* silent-suppression bug was found by running against a live database, and none by a fixture — `reltuples = -1` suppressing all proposals, redaction dismembering `$N`, a `toplevel` filter producing confidently-wrong advice, an `IS NOT NULL` polarity inversion that shipped to PyPI. + +So this plan buys back what verification it can, and is explicit about the rest: + +1. **The connection path IS testable**, because Redshift speaks the PostgreSQL wire protocol through psycopg. The read-only session, the statement timeout, secret scrubbing and per-capability degradation are all exercised against the existing Postgres container. +2. **Every statement is syntax-validated** by parsing it with sqlglot's `redshift` dialect in a unit test. That cannot catch a wrong column name, but it catches the malformed-statement class, which is otherwise invisible until a user runs it. +3. **Every statement's select-list arity is pinned against its Python unpacking.** Batch 2 shipped a column-count mismatch that no fixture caught; a test comparing the two is cheap and closes it. +4. **The limitation is documented prominently, not buried** — README and `--dry-run` both say the introspection SQL has not been executed against a live cluster, and invite the user to run `--dry-run` output by hand. + +**Column names and catalog shapes in this plan come from AWS documentation, not from observation.** Encode them as module-level constants with that provenance in a comment, and make every row unpacking defensive in the way `postgres.py` already is (`_as_int`, `_as_float`, and the `reltuples`-style sentinel translation). Where this plan states a column name, treat it as *to be confirmed by the first user with a cluster*, and make the failure mode a recorded `degraded` entry rather than a traceback. + +## Why the rules are not the Postgres rules renamed + +**Redshift has no indexes.** ADV001–ADV008 are inapplicable and must not be inherited. Redshift's physical-design levers are different, and so is the blast radius: + +| lever | what it does | how bad is the DDL | +|---|---|---| +| SORTKEY | zone maps let a scan skip whole blocks for range/equality predicates | `ALTER TABLE … ALTER SORTKEY` rewrites the table | +| DISTKEY | co-locates join keys so a join needs no redistribution | `ALTER TABLE … ALTER DISTKEY` rewrites the table | +| DISTSTYLE ALL | replicates a small dimension to every node | rewrite, and costs storage per node | +| VACUUM / ANALYZE | reclaims sort order and refreshes statistics | no rewrite, but VACUUM is heavy | + +**Every one of the first three rewrites the entire table.** That is categorically more dangerous than `CREATE INDEX CONCURRENTLY`, and the generated script must say so far more loudly than the Postgres one does. This is the single most important difference between the two adapters. + +The existing offline `RedshiftAdapter` (a `PerfAdapter`, unrelated to `WorkloadAdapter`) already encodes the domain vocabulary this plan needs — `DS_BCAST_INNER`, `DS_DIST_BOTH`, `DS_DIST_ALL_INNER` as redistribution markers, and `keys.py`'s `join_key_columns` / `filter_columns`. Read both before starting; reuse the concepts and the finding codes' reasoning, but note that adapter works from **one SQL file**, whereas this one works from an **aggregated workload**. + +## Capabilities + +Mirror `postgres.py`'s structure exactly — a `SQL: dict[str, str]`, a `_HINTS` dict, and `_run()` recording degradation rather than raising. + +| capability | source | purpose | +|---|---|---| +| `CAP_WORKLOAD` | `sys_query_history` | query text, execution time, call counts | +| `CAP_SCHEMA` | `svv_redshift_columns` | `{schema: {table: {column: type}}}` for `qualify()` | +| `CAP_TABLE_FACTS` | `svv_table_info` | rows, size, `unsorted`, `stats_off`, `diststyle`, `sortkey1`, `skew_rows` | +| `CAP_ADVISOR` | `svv_alter_table_recommendations` | Redshift's *own* recommendations | + +There is deliberately **no** `CAP_NDV` and **no** `CAP_INDEXES`: Redshift exposes no per-column distinct-value statistic comparable to `pg_stats.n_distinct`, and it has no indexes. Do not fabricate either — a rule that needs NDV must disclose that it could not check selectivity, exactly as ADV001 now does on Postgres. + +**`sys_query_history` is the current, documented view and is what this plan targets.** The older `stl_query` + `svl_statementtext` pair has a shape hazard worth knowing about even though we are not using it: `svl_statementtext` **chunks query text across multiple rows** ordered by a `sequence` column, so reading it without reassembly silently truncates every query at ~200 characters. If a later task needs a fallback for older clusters, that reassembly is the thing to get right, and a truncated-text test is the thing that would catch it. + +--- + +### Task 1: the adapter skeleton, its statements, and `--dry-run` + +**Files:** +- Create: `src/sqlquality/workload/redshift.py` +- Modify: `src/sqlquality/workload/__init__.py` (register the engine) +- Create: `tests/test_workload_redshift.py` + +**Interfaces:** +- Produces: `RedshiftWorkloadAdapter` with `engine = "redshift"`, a `SQL` dict over the four + capabilities above, `_HINTS` for each, `introspection_sql()`, and `_run()`. Registered in + `_ADAPTERS` so `get_workload_adapter("redshift")` returns it. +- Consumes: `WorkloadAdapter`, `IntrospectionStatement`, `Querier`, `MIN_TIMEOUT_S`/`MAX_TIMEOUT_S`. + +`connect()`, `fetch_workload`, `fetch_schema`, `fetch_table_facts`, `propose` and `render_ddl` may +raise `NotImplementedError` in this task **only if** a test pins that they do — an adapter that +half-exists and returns empty results silently is worse than one that says it is unfinished. Later +tasks fill them in. + +- [ ] **Step 1: Write the failing tests** + +```python +import sqlglot +import pytest + +from sqlquality.workload import get_workload_adapter +from sqlquality.workload.redshift import ( + CAP_ADVISOR, + CAP_SCHEMA, + CAP_TABLE_FACTS, + CAP_WORKLOAD, + RedshiftWorkloadAdapter, +) + +EXPECTED_CAPABILITIES = {CAP_WORKLOAD, CAP_SCHEMA, CAP_TABLE_FACTS, CAP_ADVISOR} + + +def test_registry_returns_the_redshift_adapter(): + adapter = get_workload_adapter("redshift") + assert adapter.engine == "redshift" + + +def test_every_capability_has_a_statement_and_a_hint(): + statements = RedshiftWorkloadAdapter().introspection_sql() + assert {s.capability for s in statements} == EXPECTED_CAPABILITIES + for statement in statements: + assert statement.sql.strip() + assert statement.privilege_hint.strip() + + +@pytest.mark.parametrize("capability", sorted(EXPECTED_CAPABILITIES)) +def test_every_statement_parses_as_redshift_sql(capability): + """Syntax validation is the one correctness check available without a cluster. + + The catalog SQL in this adapter cannot be executed during development — there is no + Redshift container and `svv_*`/`sys_*` do not exist in Postgres. Parsing each statement + with sqlglot's redshift dialect cannot catch a wrong column name, but it catches a + malformed statement, which would otherwise be invisible until a user ran it. + """ + sql = RedshiftWorkloadAdapter.SQL[capability] + # `%s` placeholders are libpq's, not SQL — sqlglot cannot parse them, so they become + # bind markers for the purposes of this check. + parsed = sqlglot.parse_one(sql.replace("%s", "?"), dialect="redshift") + assert parsed is not None + + +@pytest.mark.parametrize("capability", sorted(EXPECTED_CAPABILITIES)) +def test_no_statement_writes(capability): + """Same guard the Postgres adapter carries, for the same reason.""" + forbidden = ("insert", "update", "delete", "create", "drop", "alter", "truncate", + "grant", "revoke", "vacuum", "analyze") + lowered = RedshiftWorkloadAdapter.SQL[capability].lower() + import re + found = {verb for verb in forbidden if re.search(rf"\b{verb}\b", lowered)} + assert not found, f"{capability} contains write verb(s): {sorted(found)}" + + +def test_there_is_no_ndv_or_index_capability(): + """Redshift exposes no `pg_stats.n_distinct` equivalent and has no indexes. + + Declaring either capability would invite a rule to assume evidence that cannot exist. + """ + capabilities = {s.capability for s in RedshiftWorkloadAdapter().introspection_sql()} + assert not any("ndv" in c or "index" in c for c in capabilities) + + +def test_unimplemented_methods_say_so_rather_than_returning_empty(): + """A half-built adapter that returns nothing looks exactly like a healthy cluster with + no workload, which is the worst possible failure mode for this command.""" + adapter = RedshiftWorkloadAdapter() + with pytest.raises(NotImplementedError): + adapter.fetch_schema(("public",)) +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `uv run pytest tests/test_workload_redshift.py -x -q` + +Expected: FAIL — `ModuleNotFoundError: No module named 'sqlquality.workload.redshift'`. + +- [ ] **Step 3: Write the module** + +Mirror `postgres.py`'s shape. Each statement gets a comment recording that its column names come +from AWS documentation and are **unconfirmed against a live cluster**. Start from: + +```python +CAP_WORKLOAD = "workload" +CAP_SCHEMA = "schema" +CAP_TABLE_FACTS = "table_facts" +CAP_ADVISOR = "advisor" +``` + +and statements of this shape — adjust only if you find a documented reason to, and say what: + +```python + # Column names below come from AWS's Redshift system-view documentation and have NOT + # been executed against a live cluster (see the module docstring). Every consumer + # unpacks defensively and a denied or malformed statement is recorded in `degraded` + # rather than raised, so a wrong name costs one capability instead of the run. + CAP_WORKLOAD: """ + SELECT query_text, elapsed_time + FROM sys_query_history + WHERE database_name = current_database() + AND status = 'success' + ORDER BY elapsed_time DESC + LIMIT %s + """, +``` + +`_HINTS` must name the real grant each view needs — `sys_query_history` shows only the current +user's queries without `SYSLOG ACCESS UNRESTRICTED`, which is exactly the kind of partial-result +trap the Postgres hints already warn about for `pg_stats`. + +- [ ] **Step 4: Register the engine** + +Add `"redshift": RedshiftWorkloadAdapter` to `_ADAPTERS` in `workload/__init__.py`. + +- [ ] **Step 5: Run the tests, then the suite and all four gates.** + +- [ ] **Step 6: Prove the syntax check discriminates** + +Introduce a deliberate syntax error into one statement (`SELECT FROM WHERE`). Expected: that +capability's `test_every_statement_parses_as_redshift_sql` case FAILS **and the other three still +pass**, so the parametrisation discriminates per member rather than as a block. Restore, and report +which case failed. + +- [ ] **Step 7: Commit** + +```bash +git add src/sqlquality/workload/redshift.py src/sqlquality/workload/__init__.py \ + tests/test_workload_redshift.py +git commit -m "feat(advise): a Redshift workload adapter skeleton with syntax-checked statements" +``` + +--- + +### Task 2: connect over the Postgres wire protocol, proven against a real server + +**Files:** +- Modify: `src/sqlquality/workload/redshift.py` +- Modify: `tests/test_workload_redshift.py` +- Modify: `tests/integration/` (a live connection test) + +**Interfaces:** +- Produces: `RedshiftWorkloadAdapter.connect(params, timeout_s)`. + +**This is the one part of the adapter that can be verified for real.** Redshift speaks the +PostgreSQL wire protocol, so `connect()` can and must be exercised against the existing +`postgres:16` container: the read-only session, the clamped statement timeout, secret scrubbing on +a bad password, and a clear install hint when psycopg is missing. + +Extract the shared logic rather than copying it — `postgres.py`'s `connect` already does exactly +this, and two copies of a credential-handling path is how one of them drifts. **But the Postgres +adapter's behaviour must not change**: prove that by running the existing Postgres tests unchanged. + +Redshift-specific difference to preserve: Redshift does not support `SET default_transaction_read_only` +in all configurations. Establish read-only intent the way that works there, and if the statement is +refused, record it as a degradation and say plainly in the hint that the session could not be +proven read-only — do **not** silently continue as if it had succeeded, because "we never write" is +this tool's central promise. + +- [ ] **Step 1: Write the failing tests** — including a live one: + +```python +@pytest.mark.integration +def test_redshift_adapter_connects_over_the_postgres_wire_protocol(live_dsn): + """Redshift speaks the PostgreSQL protocol, so the session setup is genuinely testable. + + The catalog statements are not — `svv_*` does not exist here — so this test deliberately + covers connect() only, and asserts nothing about introspection. + """ + adapter = RedshiftWorkloadAdapter() + params = ConnectionParams(engine="redshift", dsn=live_dsn, fields={}, source="test") + adapter.connect(params, timeout_s=30) + assert adapter._query is not None + rows = adapter._query("SELECT 1", ()) + assert rows == [(1,)] + + +@pytest.mark.integration +def test_a_wrong_password_leaks_nothing(live_dsn): + adapter = RedshiftWorkloadAdapter() + bad = live_dsn.replace(":sqlquality@", ":wr0ng-p4ss@") + params = ConnectionParams(engine="redshift", dsn=bad, fields={}, source="test") + with pytest.raises(ConnectionError) as exc: + adapter.connect(params, timeout_s=5) + assert "wr0ng-p4ss" not in str(exc.value) + assert "wr0ng-p4ss" not in repr(exc.value) +``` + +- [ ] **Step 2–5:** run red, extract the shared session helper, implement, run green, and confirm + `tests/test_workload_postgres.py` and `tests/test_workload_secrets.py` pass **unchanged**. +- [ ] **Step 6: Prove the scrubbing test discriminates** by removing the `scrub(...)` call and + confirming the wrong-password test fails with the password visible. Restore, report. +- [ ] **Step 7: Commit.** + +--- + +### Task 3: workload and schema fetch + +**Files:** modify `redshift.py`, `tests/test_workload_redshift.py`. + +**Interfaces:** `fetch_workload(since, limit) -> WorkloadFetch`, `fetch_schema(schemas) -> dict`. + +Two things to get right, both of which the Postgres adapter learned the hard way: + +- **The window description must be honest.** `sys_query_history` *does* carry timestamps, unlike + `pg_stat_statements` — so unlike Postgres, `--since` can be honoured. If you honour it, say so in + `window_description`; if you do not, say that instead. Do not describe a window you did not apply. +- **`sys_query_history` returns one row per execution, not per normalised statement.** Postgres's + `pg_stat_statements` pre-aggregates by fingerprint; Redshift does not. So `calls` is 1 per row and + the aggregation happens in `ingest()` via fingerprinting — which already sums `calls` and + `total_time_ms` per fingerprint. Confirm that is what happens rather than assuming, and add a test + with two executions of the same statement asserting they collapse to one `QueryStat` with + `calls == 2`. + +- [ ] Tests, red, implement, green, mutation-prove the aggregation claim, commit. + +--- + +### Task 4: table facts, and the sentinels Redshift uses + +**Files:** modify `redshift.py`, `tests/test_workload_redshift.py`. + +**Interfaces:** `fetch_table_facts(schemas, relations) -> dict[Relation, TableFacts]`, plus a +Redshift-specific `RedshiftTableFacts` (or extra fields carried in the adapter) for `unsorted`, +`stats_off`, `diststyle`, `sortkey1` and `skew_rows`, which `TableFacts` does not model. + +`TableFacts` is engine-neutral and must stay so — do not add Redshift columns to it. Hold the +Redshift-specific physical facts in the adapter, keyed by `Relation`, the way `postgres.py` holds +`PgIndex`. + +**The sentinel lesson applies here.** Batch 1 lost a day to `pg_class.reltuples = -1` meaning "never +analysed" being read as "tiny table", which silently suppressed every proposal. Redshift's +`svv_table_info` has its own version of this: `tbl_rows` and `size` are only meaningful once the +table has been analysed, and `stats_off` is precisely the column that says how stale they are. +**Translate unknown to `None` at the boundary** and let the rules disclose it, rather than letting a +zero or a `-1` read as a fact. Add a test per sentinel you handle. + +- [ ] Tests, red, implement, green, mutation-prove each sentinel translation, commit. + +--- + +### Task 5: ADV101 (SORTKEY) and ADV102 (DISTKEY) + +**Files:** modify `redshift.py`, create `tests/test_workload_redshift_rules.py`. + +**Interfaces:** +- `propose_sortkey(usage, facts, physical, *, min_cost_share) -> list[Proposal]` — `"ADV101"` +- `propose_distkey(usage, facts, physical, *, min_cost_share) -> list[Proposal]` — `"ADV102"` + +**ADV101 — SORTKEY from hot range and equality predicates.** Redshift's zone maps store min/max per +block, so a scan can skip blocks entirely when the predicate column is the sort key. The candidate +is the hottest `RANGE`/`EQUALITY` column, and time-series columns are the canonical win. + +Suppress when the table's existing `sortkey1` already **is** that column — the equivalent of +`_covered`, and the same silent-suppression trap: if the sort key could not be read, the claim +"the table is not sorted on this column" is unknowable and confidence caps at LOW with the gap +stated. + +**ADV102 — DISTKEY from hot join keys.** A join whose two sides are not distributed on the join key +forces redistribution — `DS_BCAST_INNER` or the heavier `DS_DIST_BOTH`, which the offline +`RedshiftAdapter` already names. The candidate is the hottest `JOIN`-role column. + +**Both cap at MEDIUM, and there is deliberately no HIGH branch.** Follow ADV008's precedent and say +so in the docstring so a later reader does not add the missing rung. The reasons are specific: +- Redshift exposes no per-column NDV, so distribution **skew** — the thing that makes a DISTKEY + choice good or catastrophic — cannot be predicted. `svv_table_info.skew_rows` describes the + *current* distribution, not the proposed one. +- A SORTKEY change is only worth its rewrite if the predicate is selective, and selectivity is + exactly what cannot be measured without NDV. + +Claiming HIGH would assert something about data distribution this tool cannot see, while +recommending DDL that **rewrites the entire table**. + +- [ ] Tests per rule, red, implement, green, mutate each confidence rung and each suppression gate + independently, commit. + +--- + +### Task 6: ADV103 (DISTSTYLE ALL), ADV104 (VACUUM/ANALYZE), ADV105 (Redshift's own advice) + +**Files:** modify `redshift.py`, modify `tests/test_workload_redshift_rules.py`. + +- **ADV103** — a small, frequently-joined dimension is a candidate for `DISTSTYLE ALL`, which + replicates it to every node and removes redistribution entirely. Gate on a row-count **ceiling** + (the inverse of Postgres's floor) and on the table actually being joined in the workload. Disclose + the cost: storage is multiplied by the node count, and every write is amplified. +- **ADV104** — `svv_table_info.unsorted` above a threshold, or `stats_off` above one, means the + table needs `VACUUM` or `ANALYZE`. This is the one Redshift rule whose remediation does **not** + rewrite the table, so it is the only one that can reasonably reach HIGH — and it is also the + cheapest thing a user can act on, so it belongs near the top of a report. +- **ADV105** — surface `svv_alter_table_recommendations`, which is **Redshift Advisor's own + output**. Present it as the engine's opinion, clearly attributed, alongside ours. Where Advisor + and one of our rules agree, say so — that agreement is the strongest evidence this adapter can + produce, since it is the only signal in the whole plan that comes from the cluster itself rather + than from our inference. Never present an Advisor row as our own conclusion. + +- [ ] Tests per rule, red, implement, green, mutation-prove, commit. + +--- + +### Task 7: `render_ddl` — the dangerous-DDL header, and wiring + +**Files:** modify `redshift.py`, modify `tests/test_workload_redshift_rules.py`, possibly `cli.py`. + +**Interfaces:** `RedshiftWorkloadAdapter.render_ddl(proposals) -> str`, `propose(...)` composing all +five rules and returning them collapsed and ranked. + +**The header must be much louder than the Postgres one.** Every ADV101/102/103 statement rewrites +the whole table: it holds a lock, consumes disk for a full copy, and on a large table takes hours. +The Postgres header recommends `CONCURRENTLY`; there is no such escape here. Say plainly that these +are table rewrites, that they should be scheduled, and that `ADV104`'s `VACUUM`/`ANALYZE` are the +only statements in the file that are not. + +Reuse, do not reimplement: the proposal collapse, `_ranking_key` (now public on the ABC after Batch +3a's F7), `cost_share_of`, and `_is_fully_commented`'s protection that no emitted line is ever +executable-looking. Confirm by test that a hostile identifier cannot produce a bare executable line +in a Redshift script either. + +**Check the dbt enrichment interaction.** Batch 3a's `enrich_proposals` keys on a `CREATE INDEX` +DDL prefix, so it will not touch Redshift's `ALTER TABLE` statements — meaning a dbt-managed +Redshift model currently gets a table-rewrite proposal with **no** warning that `dbt run` will undo +it. Decide deliberately: either extend the enrichment to recognise Redshift's statements, or +document that dbt enrichment covers Postgres index proposals only. State the reasoning either way, +and note that Batch 3a's ADV302 already warns about `adapter_type` mismatches, which is adjacent +but not the same thing. + +- [ ] Tests, red, implement, green, mutation-prove the header and the collapse reuse, commit. + +--- + +### Task 8: prove what can be proven, and document what cannot + +**Files:** `tests/integration/`, `README.md`, `CHANGELOG.md`, the design spec. + +- [ ] **Step 1: Live tests** for everything reachable — `connect()` against the Postgres container, + the read-only session, the timeout clamp, secret scrubbing, and `--dry-run` printing all four + statements with no connection at all. +- [ ] **Step 2: Confirm the whole default suite stays green** with zero skips, and that the Postgres + adapter's output is unchanged (run its tests unmodified). +- [ ] **Step 3: Document the verification gap prominently.** README must say, in the `advise` + section rather than a footnote, that Redshift's introspection SQL is syntax-checked and + shape-tested but **has not been executed against a live cluster**, that column names come from + AWS documentation, and that `advise --engine redshift --dry-run` prints every statement for a + user to run by hand and report back. A user pointing this at a production cluster deserves to + know which parts are proven. +- [ ] **Step 4: Document the rules** — ADV101–105 in the rule table, with the table-rewrite warning + attached to the three that rewrite, and the Advisor attribution for ADV105. +- [ ] **Step 5: CHANGELOG and a spec deviation** recording that Redshift declares no NDV or index + capability, why HIGH is unreachable for ADV101/102/103, and that the connection path is + verified while the catalog path is not. +- [ ] **Step 6: All four gates plus the integration suite, then commit.** + +--- + +## Self-Review + +**Spec coverage.** ADV101–105 map to Tasks 5 and 6; the adapter contract to Tasks 1–4; DDL and +wiring to Task 7; proof and docs to Task 8. Snowflake remains out of scope pending an account. + +**Placeholder scan.** Tasks 3–6 give test *intent* and the specific mutation to run rather than +full test bodies, deliberately: the column names those tests assert on are unverified, so pinning +exact fixtures in the plan would harden guesses into requirements. Each of those tasks names what +must be pinned and what must be mutated; the implementer writes fixtures matching whatever the +statements end up selecting. Tasks 1 and 2 — where the assertions are about *our* code rather than +Redshift's schema — carry real test code. + +**Type consistency.** `Relation`, `TableFacts`, `Proposal`, `Aggregation`, `Workload`, +`ConnectionParams`, `IntrospectionStatement` and `Querier` are all pre-existing and used with +current field names. `TableFacts` is deliberately *not* extended; Redshift's physical facts live in +an adapter-local structure, mirroring `PgIndex`. + +**The biggest risk this plan accepts.** The catalog SQL may simply be wrong — a misremembered column +name produces an adapter that degrades on every capability and proposes nothing. The mitigations are +that each failure is a recorded `degraded` entry naming the statement rather than a traceback, that +`--dry-run` lets a user check every statement before connecting, and that the README says plainly +which parts are unverified. That is honest, but it is weaker than every predecessor batch, and the +first user with a cluster should be treated as part of the verification loop rather than as a +consumer of a finished feature. 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 5f71bb5..059582d 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 @@ -468,6 +468,10 @@ records why they changed. ### Redshift +Shipped in Batch 3b (2026-07-28 to 2026-07-31), with rule renumbering, capability and +workload-path differences from what this subsection originally specified — see +"Deviations from the spec (Batch 3b: Redshift adapter)" below for what changed and why. + Workload from `SYS_QUERY_HISTORY` when available, falling back to `STL_QUERY` plus `SVL_STATEMENTTEXT` reassembled in `sequence` order. The fallback path is subject to `STL_QUERY.querytxt` truncation, which the report discloses. Facts from `SVV_TABLE_INFO` @@ -647,6 +651,106 @@ subsection above originally specified. 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. +## Deviations from the spec (Batch 3b: Redshift adapter) + +Found and agreed while implementing `--engine redshift` (Tasks 1–8 of Batch 3b). The +"Redshift" subsection above, written before any of this was built, described a different +shape in several places; recorded here rather than silently edited in place, so a reader +comparing spec to shipped code can see what changed and why. + +1. **Rule numbering and content do not match the original table.** The spec assigned + ADV101 to DISTKEY, ADV102 to SORTKEY, ADV103 to VACUUM/ANALYZE, ADV104 to disk-based + spill attribution, and ADV105 to DISTSTYLE ALL. The shipped adapter instead assigns + **ADV101 to SORTKEY, ADV102 to DISTKEY, ADV103 to DISTSTYLE ALL, ADV104 to + VACUUM/ANALYZE**, and ADV105 to relaying Redshift Advisor's own recommendation verbatim + (`svv_alter_table_recommendations`) — a proposal the spec did not separately number at + all, only mentioning Advisor agreement as a confidence input to the other rules. The + reordering followed the same logic Batch 3a used for ADV302 (see that section's + deviation 1): SORTKEY is listed first because it is the more commonly reached-for lever + (zone maps apply to any hot range/equality predicate; DISTKEY only helps a join), and + Advisor's relay earned its own code once it became clear the honest way to present the + cluster's own opinion is as a separate, clearly-attributed proposal — never merged into + an ADV101/102/103 proposal as though sqlquality had produced it (see `propose_advisor` + and `_disclose_advisor_agreement` in `redshift.py`). +2. **No `STL_QUERY`/`SVL_STATEMENTTEXT` fallback, and no disk-spill attribution + (`SVL_QUERY_SUMMARY`).** The spec's workload path falls back to reassembling + `STL_QUERY.querytxt` when `SYS_QUERY_HISTORY` is unavailable, and its ADV104 attributes + disk-based spill to the query group that caused it. Neither shipped: `SYS_QUERY_HISTORY` + alone is what Task 1 implemented, kept single-path so the one statement per capability + claim (`introspection_sql()` returning exactly `--dry-run`'s output) stays true, and + `SVL_QUERY_SUMMARY`-based spill attribution was descoped as a distinct capability this + plan's four (`CAP_WORKLOAD`, `CAP_SCHEMA`, `CAP_TABLE_FACTS`, `CAP_ADVISOR`) do not + cover. Both are legitimate future work, not abandoned by oversight — recorded here so a + later batch does not have to rediscover that the fallback and the spill rule were never + built rather than quietly removed. +3. **`svv_columns`, not the spec's `SVV_REDSHIFT_COLUMNS`.** `svv_columns` additionally + covers external (Spectrum) tables, which a query joining one needs to qualify; `SVV_ + REDSHIFT_COLUMNS` covers only local tables and would have silently dropped any statement + touching a Spectrum table as unqualifiable. Accepted as a Task 1 deviation and re-affirmed + through every later task — see deviation 4 below for the consequence this creates. +4. **Deliberately no `CAP_NDV`, no `CAP_INDEXES`.** Redshift exposes no equivalent of + `pg_stats.n_distinct` — there is no per-column distinct-value statistic anywhere in its + system catalog — and it has no indexes at all, so an "existing index" capability would + have nothing to model. Declaring either capability anyway would invite a rule to assume + evidence that structurally cannot exist on this engine: a `CAP_NDV`-shaped capability + that always came back empty is indistinguishable, to a rule reading it, from "every + column has terrible selectivity," which is a confident, wrong signal rather than an + honest absence. Redshift's physical-design levers — SORTKEY, DISTKEY/DISTSTYLE, and + VACUUM/ANALYZE staleness — are read entirely through `CAP_TABLE_FACTS` and `CAP_ADVISOR` + instead (see `RedshiftTableFacts` and the module docstring in `redshift.py`). +5. **Why ADV101, ADV102 and ADV103 can never reach HIGH — a direct consequence of + deviation 4, not a separate design choice.** Every one of the three proposes a + full-table rewrite (`ALTER SORTKEY`/`ALTER DISTKEY`/`ALTER DISTSTYLE ALL`), and whether + that rewrite is actually worth its cost depends on the underlying predicate's + selectivity (SORTKEY) or the table's distribution skew (DISTKEY, DISTSTYLE ALL) — + exactly the two things `CAP_NDV`'s absence means this adapter cannot measure. Claiming + HIGH would assert something about data distribution the tool cannot see while + recommending the single most expensive class of statement in this whole feature. + Structurally enforced, not merely undocumented: no `Confidence.HIGH` literal appears in + `propose_sortkey`, `propose_distkey` or `propose_diststyle_all`, and agreement with an + Amazon Redshift Advisor recommendation (`_disclose_advisor_agreement`) adds a rationale + sentence, never raises the cap. ADV104 (VACUUM/ANALYZE) is the one rule that reaches + HIGH, because its evidence (`unsorted`, `stats_off`) is a direct catalog measurement + about the table's own current state, not an inference about data this tool cannot see, + and its remediation does not rewrite anything. +6. **The connection path is verified live; the catalog path is not, and the honesty gap + between them is the central risk this batch accepts.** Redshift speaks the PostgreSQL + wire protocol through psycopg, so `connect()` — the read-only session, the statement + timeout clamp, and secret scrubbing — is exercised for real against the same + `postgres:16` container every other engine's tests use (Task 2; live-tested further in + Task 8, including the timeout clamp actually taking effect on the server). Every + introspection statement is additionally syntax-checked with sqlglot's `redshift` + dialect and proven *bindable* — that its parameters can be prepared and sent over the + wire — against same-shaped stand-in tables built specifically because Postgres's + analyzer resolves table references before parameter types, so testing an unbindable + statement against a genuinely missing view produces only `UndefinedTable` regardless of + whether the bug being tested for is even present (see + `tests/integration/test_redshift_introspection_bindable_live.py`'s module docstring). + None of that proves the **column names or the resulting proposals' semantics** are + correct: `svv_*`/`sys_*` do not exist in Postgres, there is no Redshift container + available for development, and every column name in this adapter comes from AWS's + published system-view documentation rather than an observed row. A wrong name degrades + exactly one capability (`self.degraded` names the statement, never a traceback) rather + than the whole run, `--dry-run` lets a user inspect every statement before ever + connecting, and the README says which half of this is proven — but that is honestly + weaker than every other adapter this project has shipped, and the first user who points + this at a real cluster is part of closing that gap, not merely a consumer of a finished + feature. +7. **A relation absent from `svv_table_info` cannot be told apart from a Spectrum + (external) table — an ambiguity this adapter declines rather than guesses through.** + AWS documents `svv_table_info` as omitting both external (Spectrum) tables, which + cannot carry a SORTKEY, DISTKEY or DISTSTYLE at all, and genuinely empty local tables. + Nothing else this adapter reads distinguishes the two cases, and Tasks 5–6 considered + adding a further introspection capability to do so out of scope (`propose()`'s inputs + were fixed by that point in the plan). So a relation carrying a hot RANGE/EQUALITY/JOIN + predicate but absent from `physical_facts` gets **no** ADV101/102/103 proposal at all — + proposing a rewrite for something that might not even support one would be worse than + proposing nothing — and the count of relations this affected is disclosed through the + same `self.degraded` channel a denied capability uses + (`DEGRADATION_PHYSICAL_FACTS_GAP`), not silently absorbed. This is recorded as an open + gap, not a solved one: a future task with a real cluster to test against could add a + `svv_external_tables` cross-reference to resolve it. + ## Confidence model Mechanical, derived from inputs rather than judgment: diff --git a/src/sqlquality/cli.py b/src/sqlquality/cli.py index af98327..75387e5 100644 --- a/src/sqlquality/cli.py +++ b/src/sqlquality/cli.py @@ -761,11 +761,15 @@ def advise( help=( "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 when " - "no query usage could be extracted at all." + "-- the last only with --project-dir/--manifest -- and, on Redshift, ADV101 " + "SORTKEY, ADV102 DISTKEY, ADV103 DISTSTYLE ALL); the index-hygiene rules " + "ADV002 and ADV003, ADV303 (its evidence is absence, not cost, so there is " + "no share to threshold), and, on Redshift, ADV104 VACUUM/ANALYZE (its evidence " + "is a catalog measurement about the table itself, not the workload) and ADV105 " + "(Redshift Advisor's own recommendations, not ours 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( diff --git a/src/sqlquality/workload/__init__.py b/src/sqlquality/workload/__init__.py index 9f47cce..c3a78bc 100644 --- a/src/sqlquality/workload/__init__.py +++ b/src/sqlquality/workload/__init__.py @@ -4,9 +4,11 @@ from sqlquality.workload.base import WorkloadAdapter from sqlquality.workload.postgres import PostgresWorkloadAdapter +from sqlquality.workload.redshift import RedshiftWorkloadAdapter _ADAPTERS: dict[str, type[WorkloadAdapter]] = { "postgres": PostgresWorkloadAdapter, + "redshift": RedshiftWorkloadAdapter, } diff --git a/src/sqlquality/workload/dbt.py b/src/sqlquality/workload/dbt.py index 8662b97..53e83c2 100644 --- a/src/sqlquality/workload/dbt.py +++ b/src/sqlquality/workload/dbt.py @@ -511,6 +511,39 @@ def _dbt_drop_note(model: ModelNode) -> str: ) +def _prepend_note(existing: str | None, dbt_note: str) -> str: + """`dbt_note` after whatever `existing` note the proposal already carried, not in place + of it. + + Both the drop-warning and the generic "not expressed as config" paths used to build + `note` as `_dbt_drop_note(model)` / `_dbt_ddl_note(model, reason)` alone — replacing + `proposal.note` outright rather than appending to it. That was silent and harmless for + every rule this module was written against, because no Postgres rule sets `note` before + enrichment ever sees it (`CREATE INDEX`/`DROP INDEX` proposals arrive with `note=None`). + It stopped being harmless the moment a rule with a *real*, already-meaningful `note` + could reach either path: Redshift's ADV105 sets `note` to its own Amazon-Redshift-Advisor + attribution ("this statement was generated by Redshift itself and is relayed + verbatim") — exactly the sentence the task that added this module's dbt-interaction + review required to survive in the rendered DDL file. Overwriting it with only the dbt + warning would have silently discarded that attribution the instant a dbt-managed + relation also carried an Advisor recommendation. Preserving it first, and appending the + dbt warning after a blank line, keeps both true statements next to the one DDL line they + both concern instead of one silently replacing the other. + + Order and non-duplication are both pinned (`test_prepend_note_keeps_the_existing_note + _first_and_emits_the_dbt_note_once`, `test_prepend_note_is_idempotent`): the existing + note comes first because it is the proposal's own statement about its own DDL and the dbt + warning is a caveat *on* it, and `dbt_note` is emitted exactly once — a note already + carrying this warning is returned unchanged rather than accumulating a second copy, so a + second enrichment pass over an already-enriched proposal cannot double it. + """ + if not existing: + return dbt_note + if dbt_note in existing: + return existing + return f"{existing}\n\n{dbt_note}" + + @dataclass(frozen=True) class _IndexEntry: """One `- columns: [...]` item in a dbt model's `indexes` config list. @@ -664,23 +697,42 @@ def _classify(proposal: Proposal, model: ModelNode) -> tuple[Proposal | None, _I ) return ( dataclasses.replace( - proposal, rationale=rationale, evidence=evidence, note=_dbt_drop_note(model) + proposal, + rationale=rationale, + evidence=evidence, + note=_prepend_note(proposal.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. + # Some other statement for a relation dbt owns — no rule emitted one when this + # branch was written, and `_is_index_creating` matches by DDL prefix specifically so + # this stays covered when one does. It is no longer hypothetical: Redshift's + # ADV101-103 (ALTER SORTKEY/DISTKEY/DISTSTYLE, table rewrites), ADV104 + # (VACUUM/ANALYZE) and ADV105 (Advisor's own DDL, relayed verbatim) all land here, + # since none of them is a CREATE/DROP INDEX. See + # `test_dbt_warning_is_attached_to_a_redshift_rewrite_proposal` in + # `tests/test_workload_redshift_rules.py` — this generic path was previously + # unexercised by any test. 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.", + note = _prepend_note( + proposal.note, + _dbt_ddl_note( + model, + "dbt rebuilds this relation on its own schedule, and this statement is not\n" + "expressed as dbt config.", + ), ) + # A flag, for the same reason `dbt_index_config` is one: the warning above lives in + # `rationale` and `note`, and the terminal prints neither, so an enriched row is + # byte-identical to the same proposal from a dbt-free run. `describe_rewrites` reads + # this to give a terminal-only user the one signal that enrichment fired at all — + # which on Redshift, where *every* proposal lands in this branch (nothing it emits + # is a CREATE/DROP INDEX), was otherwise the whole disclosure going missing. + evidence["dbt_ddl_not_expressed_as_config"] = True return dataclasses.replace( proposal, rationale=rationale, evidence=evidence, note=note ), None @@ -873,30 +925,54 @@ def _deferred_to_block( 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. + """One line saying what dbt enrichment actually did to this run's proposals, or None + when it did nothing. + + Neither thing enrichment does is visible in a terminal row. ADV302 is never a proposal + `code` — it is a rewrite applied to another rule's proposal — so `code == "ADV302"` + matches nothing, and the generic "not expressed as dbt config" warning lives only in + `rationale` and `note`. The terminal prints neither, so an enriched row is byte-identical + to the same proposal from a dbt-free run: same code, same confidence, same cost share, + same title. Without this line a user who reads only the terminal cannot tell enrichment + happened. + + **Both kinds are counted, not only ADV302's.** Counting the config-block rewrite alone + made this function return `None` for every Redshift run: nothing Redshift emits is a + `CREATE INDEX`, so every Redshift proposal for a dbt-managed relation takes + `_classify`'s generic path instead — it gets the warning that `dbt run` may undo an + hours-long full-table rewrite, and then the terminal said nothing at all. That is + verbatim the failure mode this function exists to prevent, on the engine where the + wasted work is hours rather than seconds. + + Counted off evidence flags rather than by searching the DDL or rationale 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" + unexpressed = sum( + 1 for p in proposals if p.evidence.get("dbt_ddl_not_expressed_as_config") is True ) - if merged: - line += ( - f" ({merged} folded into another proposal's block, since dbt reads one `indexes` " - "key per model config)" + if not rewritten and not merged and not unexpressed: + return None + clauses: list[str] = [] + if rewritten or merged: + clause = ( + 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: + clause += ( + f" ({merged} folded into another proposal's block, since dbt reads one `indexes` " + "key per model config)" + ) + clauses.append(clause) + if unexpressed: + clauses.append( + f"{unexpressed} proposal(s) target a dbt-managed relation and cannot be expressed " + "as dbt config: the statement is still runnable, but the next `dbt run` may undo " + "it — see each proposal's note in --ddl, or its rationale in --markdown/--json" ) - return line + return "; ".join(clauses) def propose_materialization( diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index e9d06f4..90f4f1a 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -35,7 +35,16 @@ WorkloadAdapter, ) from sqlquality.workload.fingerprint import FLAG_LEADING_WILDCARD_LIKE, FLAG_SELECT_STAR -from sqlquality.workload.secrets import clamp_timeout_ms, scrub, secrets_for +from sqlquality.workload.secrets import secrets_for +from sqlquality.workload.session import ( + LIBPQ_FIELD_MAP, + LIBPQ_PASSTHROUGH_FIELDS, + READ_ONLY_SQL, + dropped_libpq_fields, + import_psycopg, + open_session, + translate_libpq_fields, +) CAP_WORKLOAD = "workload" CAP_STATS_RESET = "stats_reset" @@ -67,44 +76,6 @@ CAP_INDEXES: "reads pg_index and pg_stat_user_indexes; world-readable unless revoked", } -#: dbt profiles.yml field names -> libpq connection keywords. -_PG_FIELD_MAP = { - "dbname": "dbname", - "database": "dbname", - "host": "host", - "port": "port", - "user": "user", - "username": "user", - "password": "password", -} -#: profiles.yml keys forwarded to libpq unchanged, because the name already *is* the libpq -#: keyword. The TLS group is here for a security reason, not a completeness one: a profile -#: saying `sslmode: verify-full` that silently connects under libpq's default `prefer` -#: performs no certificate verification at all, and the user is never told. For a tool -#: pitched as safe to point at production that is the wrong way to fail. -_PG_PASSTHROUGH_FIELDS = frozenset( - {"sslmode", "sslcert", "sslkey", "sslrootcert", "connect_timeout"} -) - - -def _pg_fields(fields: dict[str, str]) -> dict[str, str]: - """Translate profiles.yml keys to libpq keywords, dropping anything unrecognized.""" - translated = {_PG_FIELD_MAP[k]: v for k, v in fields.items() if k in _PG_FIELD_MAP} - translated.update({k: v for k, v in fields.items() if k in _PG_PASSTHROUGH_FIELDS}) - return translated - - -def _dropped_pg_fields(fields: dict[str, str]) -> tuple[str, ...]: - """profiles.yml keys this adapter cannot forward, by name. - - Names only, never values: one of them could be a secret (`sslpassword`), and this text - goes to stderr and from there into CI logs. - """ - return tuple( - sorted(k for k in fields if k not in _PG_FIELD_MAP and k not in _PG_PASSTHROUGH_FIELDS) - ) - - #: Characters of hex kept from the fingerprint digest. 12 is 48 bits — ample for telling #: apart the few hundred query groups one run reads, and short enough to sit in a table cell. _FINGERPRINT_ID_LEN = 12 @@ -1368,17 +1339,11 @@ def _run(self, capability: str, params: tuple[object, ...]) -> list[tuple[object return [] def connect(self, params: ConnectionParams, timeout_s: int) -> None: - try: - import psycopg - except ImportError as exc: - raise ImportError( - "Postgres support requires psycopg. " - "Install it with: pip install 'sqlquality[postgres]'" - ) from exc + psycopg = import_psycopg("Postgres", "postgres") # Silence is the failure mode being fixed here: a dropped `sslmode` downgrades the - # connection with no signal at all. Key names only — see _dropped_pg_fields. - dropped = _dropped_pg_fields(params.fields) + # connection with no signal at all. Key names only — see `dropped_libpq_fields`. + dropped = dropped_libpq_fields(params.fields, LIBPQ_FIELD_MAP, LIBPQ_PASSTHROUGH_FIELDS) if dropped: print( f"warning: ignoring connection setting(s) not supported by the Postgres " @@ -1390,41 +1355,31 @@ def connect(self, params: ConnectionParams, timeout_s: int) -> None: # than trusted. secrets = secrets_for(params) - failure: str | None = None - try: + # `read_only_required=True`: on Postgres `SET default_transaction_read_only` is + # expected to succeed unconditionally, so its failure is indistinguishable from + # any other setup failure and aborts the connection exactly like one. See + # `open_session` and `RedshiftWorkloadAdapter.connect` for the engine where a + # refusal is instead a recorded degradation. + query, _degradation = open_session( + psycopg=psycopg, # Inside the scrubbing envelope: psycopg raises from make_conninfo on an - # unusable keyword, and that message can quote the offending value — which for - # the `password` keyword is the password. - conninfo = params.dsn or psycopg.conninfo.make_conninfo(**_pg_fields(params.fields)) - connection = psycopg.connect(conninfo, autocommit=True) - with connection.cursor() as cursor: - # Belt and braces: the session cannot write even if a statement tried to. - cursor.execute("SET default_transaction_read_only = on") - # set_config() rather than `SET`, because Postgres does not accept bind - # parameters in a SET statement and string-building one with a caller value - # is the wrong habit to establish in the one place we talk to a database. - cursor.execute( - "SELECT set_config('statement_timeout', %s, false)", - ( - f"{clamp_timeout_ms(timeout_s, minimum=MIN_TIMEOUT_S, maximum=MAX_TIMEOUT_S)}ms", - ), + # unusable keyword, and that message can quote the offending value — which + # for the `password` keyword is the password. + conninfo_factory=lambda: ( + params.dsn + or psycopg.conninfo.make_conninfo( + **translate_libpq_fields( + params.fields, LIBPQ_FIELD_MAP, LIBPQ_PASSTHROUGH_FIELDS + ) ) - except Exception as exc: - failure = scrub(str(exc), secrets) - if failure is not None: - # Raised after the handler, and scrubbed: Task 6 established that a dependency's - # exception text is exactly where this class of leak hides, and that leaving the - # handler is the only way to keep the original out of __context__. - # No "Could not connect" prefix here — the CLI adds it. Prefixing at both - # layers printed "Could not connect: Could not connect: ..." on the most - # common failure a user hits. - raise ConnectionError(failure) - - def query(sql: str, bind: tuple[object, ...]) -> list[tuple[object, ...]]: - with connection.cursor() as cur: - cur.execute(sql, bind) - return list(cur.fetchall()) - + ), + secrets=secrets, + timeout_s=timeout_s, + min_timeout_s=MIN_TIMEOUT_S, + max_timeout_s=MAX_TIMEOUT_S, + read_only_sql=READ_ONLY_SQL, + read_only_required=True, + ) self._query = query def fetch_workload(self, since: timedelta | None, limit: int) -> WorkloadFetch: diff --git a/src/sqlquality/workload/redshift.py b/src/sqlquality/workload/redshift.py new file mode 100644 index 0000000..2d3c2f7 --- /dev/null +++ b/src/sqlquality/workload/redshift.py @@ -0,0 +1,1604 @@ +"""Redshift workload adapter: sys_/svv_ system-view introspection, connect() is real. + +**Provenance warning.** None of the SQL in this module has been executed against a live +Redshift cluster: there is no Redshift container available for development, and Postgres — +where every other adapter's SQL gets exercised during tests — does not implement `svv_*` or +`sys_*` views at all, so nothing here can be run locally either. Every statement's column +names come from AWS's published Redshift system-view documentation, not from an observed +row. That is exactly why every consumer of these rows (added in later tasks) is required to +unpack them defensively, and why `_run` records a denied or malformed statement as one entry +in `self.degraded` rather than letting the exception propagate: a wrong column name should +cost this run exactly one capability, never the whole run. The one correctness check +available without a cluster for *column names* is syntax — see +`tests/test_workload_redshift.py`, which parses every statement with sqlglot's `redshift` +dialect. + +That is a narrower boundary than it first looks, and framing it any wider cost a day: a +statement's *parameters* are bound by the driver over the wire, which has nothing to do +with whether the table behind `FROM` is Redshift-only — a parameter psycopg cannot type +fails identically whether it is headed at `svv_table_info` or `pg_class`. So every one of +this module's four statements is also executed, with representative binds, against a +same-named, same-shaped throwaway table created in the same `postgres:16` the rest of the +suite runs against — see `tests/integration/test_redshift_introspection_bindable_live.py`, +and its docstring for why a *real* stand-in table is required: Postgres's analyzer +resolves table references before parameter types, so running a statement against the +genuinely missing view always fails with `UndefinedTable` regardless of whether its +parameters would otherwise bind, which cannot discriminate anything. A parameter-binding +failure (`IndeterminateDatatype`, previously reproducible on every default `advise` run — +see `CAP_WORKLOAD`'s comment) is exactly the class of bug this closes locally. + +**Deliberately no `CAP_NDV`, no `CAP_INDEXES`.** Redshift exposes no equivalent of +`pg_stats.n_distinct`, and it has no indexes at all — its physical-design levers are +SORTKEY, DISTKEY/DISTSTYLE, and VACUUM/ANALYZE staleness. Declaring either capability here +would invite a later rule to assume evidence that cannot exist on this engine. Those levers +are read through `CAP_ADVISOR` below and turned into proposals (ADV101-ADV105) by a later +task; this module is the skeleton — the capability set, the statements, and registration. + +Every method beyond `introspection_sql()` and `connect()` raises `NotImplementedError` here +on purpose. A `fetch_*` method that silently returned empty results would be +indistinguishable from a healthy cluster running no workload at all, which is a worse +failure mode than an explicit "not implemented yet" — see `WorkloadAdapter`'s docstring on +`--dry-run`, the thing Task 1 delivered. + +`connect()` is the exception, and deliberately so: Redshift speaks the PostgreSQL wire +protocol through psycopg, so it is the one part of this adapter genuinely exercisable +against the `postgres:16` container the rest of the suite already runs against — see +`tests/integration/test_redshift_connect_live.py`. Its session setup is shared with +`PostgresWorkloadAdapter.connect` via `workload/session.py`, and the one behavioral +difference — Redshift refusing `SET default_transaction_read_only` is a recorded +degradation rather than a hard failure — is documented on `connect()` itself. +""" + +from __future__ import annotations + +import re +import sys +from collections.abc import Mapping, Sequence +from dataclasses import dataclass, replace +from datetime import datetime, timedelta, timezone + +from sqlquality.models import ( + Aggregation, + ColumnRole, + ColumnUsage, + ConnectionParams, + Confidence, + Proposal, + RawQueryRow, + Relation, + TableFacts, + Workload, + WorkloadFetch, + cost_share_of, +) +from sqlquality.workload.base import ( + MAX_TIMEOUT_S, + MIN_TIMEOUT_S, + IntrospectionStatement, + Querier, + WorkloadAdapter, +) +from sqlquality.workload.postgres import ( + _by_relation, + _comment_lines, + _is_fully_commented, + _sentences, +) +from sqlquality.workload.secrets import secrets_for +from sqlquality.workload.session import ( + LIBPQ_FIELD_MAP, + LIBPQ_PASSTHROUGH_FIELDS, + READ_ONLY_SQL, + dropped_libpq_fields, + import_psycopg, + open_session, + translate_libpq_fields, +) + +CAP_WORKLOAD = "workload" +CAP_SCHEMA = "schema" +CAP_TABLE_FACTS = "table_facts" +CAP_ADVISOR = "advisor" + +#: Pseudo-capability name `connect()` uses to record a read-only degradation in +#: `self.degraded` — not one of `introspection_sql()`'s four statements, since arming +#: read-only intent happens once at connect time rather than per fetch. Named distinctly +#: from every real `CAP_*` so a report reader cannot mistake it for a denied SELECT. +DEGRADATION_READ_ONLY = "read_only" + +#: Pseudo-capability name `propose()` uses to disclose how many relations it declined to +#: propose SORTKEY/DISTKEY/DISTSTYLE ALL for because they were absent from +#: `physical_facts` — see `RedshiftTableFacts`'s docstring and `_skipped_for_physical_gap`. +#: That absence cannot distinguish a Spectrum (external) table, which cannot carry any of +#: these levers, from a genuinely empty local table, so this rule set declines rather than +#: guesses — but declining silently is indistinguishable from a bug that dropped the +#: relation by accident. Reported through `self.degraded`, the same channel the read-only +#: degradation uses, so it reaches the same stderr line, JSON payload and markdown report a +#: denied capability would. +DEGRADATION_PHYSICAL_FACTS_GAP = "physical_facts_gap" + +#: Redshift's dbt adapter accepts the same core libpq keywords Postgres does, so field +#: translation uses the one shared table in `session.py` (`LIBPQ_FIELD_MAP` / +#: `LIBPQ_PASSTHROUGH_FIELDS`) rather than a second, Redshift-named copy of the same +#: data — exactly the drift the brief for this adapter warned against. IAM-based fields +#: (`cluster_identifier`, `iam`, `region`) are not psycopg keywords and are deliberately +#: not in that table; a profile using them falls through to `dropped_libpq_fields` and is +#: named on stderr rather than silently dropped. + +#: What to tell the user when a capability's statement is refused. These strings are what +#: someone hands their DBA, so each one names the actual failure mode rather than a generic +#: "requires access" — in particular `CAP_WORKLOAD`'s partial-result trap, which is exactly +#: the kind the Postgres adapter already warns about for `pg_stats`: a role without the +#: right grant does not get denied, it gets a workload that looks thin or empty. +_HINTS = { + CAP_WORKLOAD: ( + "reads sys_query_history, which without the SYSLOG ACCESS UNRESTRICTED privilege " + "shows only the connecting user's own queries — a role lacking it sees a workload " + "that looks merely small, not one that was denied, so the gap has no error to " + "notice. Grant with ALTER USER SYSLOG ACCESS UNRESTRICTED (superuser-only)." + ), + CAP_SCHEMA: ( + "reads svv_columns; like information_schema, it returns only columns of tables the " + "current user can already see, so a partial result means missing table privileges " + "rather than a missing grant on this view itself" + ), + CAP_TABLE_FACTS: ( + "reads svv_table_info, which is superuser-only unless the connecting role has an " + "explicit SELECT grant on it; rows are also limited to tables the current user has " + "been granted access to, so an unexpectedly short (or empty) result reads as a " + "small schema rather than a denial — there is no error to distinguish the two" + ), + CAP_ADVISOR: ( + "reads svv_alter_table_recommendations, Amazon Redshift Advisor's own SORTKEY/" + "DISTSTYLE recommendations; visible only for tables the current user can access, " + "and only after Advisor has run its analysis — a fresh or lightly queried cluster " + "can return nothing here even with every grant in place" + ), +} + + +def _as_int(value: object) -> int: + """Coerce a driver row value to int. See `postgres.py`'s identical helper — Querier + rows are `tuple[object, ...]`, so this coercion is unavoidably unchecked and lives in + one auditable place rather than at a dozen call sites.""" + return int(value) # type: ignore[call-overload] + + +def _as_float(value: object) -> float: + """Coerce a driver row value to float. See `_as_int`.""" + return float(value) # type: ignore[arg-type] + + +#: `svv_table_info.size` is documented in 1 MB blocks; `TableFacts.size_bytes` is bytes. +_MB_BYTES = 1024 * 1024 + + +def _row_estimate(tbl_rows: object) -> int | None: + """`svv_table_info.tbl_rows`, with a NULL row translated to unknown. + + A review of this module's first version gated this on `stats_off` too — reading + `stats_off = 100` as Redshift's equivalent of `pg_class.reltuples = -1`, Postgres's + "never analyzed" sentinel that silently suppressed every proposal for a table. That + premise was inverted and is corrected here: AWS documents `tbl_rows` as the table's + actual row count and `stats_off` as a 0-100 *staleness percentage* for the planner + statistics, not an "ever analyzed" flag — neither `tbl_rows` nor `size` is itself + derived from ANALYZE, so nulling them out on a high `stats_off` discarded accurate + facts about a merely-stale table and then claimed the row count "could not be + checked" when it plainly could. `stats_off` is still real evidence — see + `RedshiftTableFacts.stats_off` — it is disclosed as a staleness caveat by a later + task's rules, not used here to erase a fact this column was never responsible for. + A NULL `tbl_rows` (the row was never populated at all) is the one genuine unknown. + """ + return None if tbl_rows is None else _as_int(tbl_rows) + + +def _size_bytes(size: object) -> int | None: + """`svv_table_info.size` (1 MB blocks) converted to bytes, or unknown if NULL. + + See `_row_estimate` for why this is no longer gated on `stats_off`. + """ + return None if size is None else _as_int(size) * _MB_BYTES + + +@dataclass(frozen=True) +class RedshiftTableFacts: + """Redshift's own physical-design facts, which `TableFacts` deliberately does not + model — see that dataclass's docstring: it is engine-neutral, and SORTKEY/DISTKEY/ + staleness are Redshift-specific levers. Held in the adapter, keyed by `Relation`, the + way `postgres.py` holds `PgIndex`. + + `stats_off` is a 0-100 staleness *percentage* for this table's planner statistics — 0 + is current, 100 is maximally stale — **not** a flag for "never analyzed" and not a + reason to distrust `tbl_rows`/`size` on the engine-neutral `TableFacts` this adapter + also builds: AWS documents both of those as physical facts about the table itself, + not values ANALYZE produces. A later task's rules should disclose `stats_off` as a + caveat ("statistics are N% stale") rather than treat it as a reason to null out a row + estimate or size that was never derived from statistics in the first place. + + Every field is `None` only when its own source value was SQL NULL. + + A relation absent entirely from the dict this is stored in (`RedshiftWorkloadAdapter + .physical_facts`) is a *distinct* condition from every field here being `None`: absence + means the relation never appeared in `svv_table_info` at all. **This is not, by + itself, proof of a Spectrum (external) table** — AWS also omits genuinely *empty* + tables from `svv_table_info`, so a later task proposing SORTKEY/DISTKEY from this + absence needs an additional signal (e.g. cross-referencing `svv_external_tables`) to + tell the two cases apart; recorded here as a carry-forward, not solved by this task. + """ + + unsorted: float | None + stats_off: float | None + diststyle: str | None + sortkey1: str | None + skew_rows: float | None + + +@dataclass(frozen=True) +class RedshiftAdvisorRow: + """One row of `svv_alter_table_recommendations` — Amazon Redshift Advisor's own + output, not this adapter's inference. See `propose_advisor` (ADV105): it is presented + as the engine's opinion, attributed as such, and never folded into an ADV101/102/103 + proposal as though sqlquality had produced it itself. + + `rec_type` and `recommended_ddl` are read defensively like every other column in this + module (see the module docstring's provenance warning): AWS documents `type` as + naming either a sort-key or a distribution-style recommendation, but that has not been + observed against a live cluster, so `_advisor_category` treats anything it does not + recognize as unclassified rather than raising or guessing. + """ + + relation: Relation + rec_type: str + current_ddl: str | None + recommended_ddl: str | None + + +#: Ceiling, not floor: unlike Postgres's `MIN_ROWS_FOR_INDEX`, a DISTSTYLE ALL candidate +#: must be a *small* dimension. Above this row count, replicating the whole table to every +#: node multiplies its storage per node and amplifies every write against it — worse than +#: the redistribution it would remove. A heuristic, not a documented Redshift limit; AWS +#: publishes no specific number, only the directional guidance that ALL suits a "small" +#: table. +MAX_ROWS_FOR_DISTSTYLE_ALL = 1_000_000 + +#: `svv_table_info.unsorted`/`.stats_off` are 0-100 percentages (see `RedshiftTableFacts`). +#: At or above this, the table has drifted far enough from sorted/analyzed that flagging +#: VACUUM/ANALYZE is worth an operator's attention. A heuristic threshold — Redshift's own +#: documentation gives directional guidance ("run VACUUM as the unsorted region grows"), +#: not a specific number. +UNSORTED_PCT_THRESHOLD = 20.0 +STATS_OFF_PCT_THRESHOLD = 20.0 + +#: Matches `KEY(column)`, whether bare or nested inside `AUTO(...)` — the shapes +#: `svv_table_info.diststyle` takes when the table has an explicit distribution key. Not +#: verified against a live cluster (see the module docstring); parsing is defensive and +#: case-insensitive, matching this whole module's discipline for column *values* it cannot +#: exercise locally. +_DISTSTYLE_KEY_RE = re.compile(r"KEY\(\s*([^)]+?)\s*\)", re.IGNORECASE) + +#: The three shapes an ADV101/102/103 proposal or an Advisor row can agree on. Keyed by +#: `Proposal.code` so `_disclose_advisor_agreement` can look a proposal's category up +#: without re-deriving it from evidence. +_CATEGORY_SORTKEY = "sortkey" +_CATEGORY_DISTKEY = "distkey" +_CATEGORY_DISTSTYLE_ALL = "diststyle_all" +_PROPOSAL_CATEGORY = { + "ADV101": _CATEGORY_SORTKEY, + "ADV102": _CATEGORY_DISTKEY, + "ADV103": _CATEGORY_DISTSTYLE_ALL, +} + + +def _diststyle_key_column(diststyle: str) -> str | None: + """The column name inside `KEY(column)` (or `AUTO(KEY(column))`), or `None`. + + `svv_table_info` carries no separate "DISTKEY column" field — the column name is + embedded in `diststyle`'s text, in one of AWS's documented shapes ('KEY(col)', 'EVEN', + 'ALL', or any of those wrapped in `AUTO(...)`). Parsed rather than string-matched + verbatim so `propose_distkey` can tell "already distributed on this column" apart from + "distributed on a different one" without a second introspection round trip. + """ + match = _DISTSTYLE_KEY_RE.search(diststyle) + return match.group(1).strip() if match else None + + +def _diststyle_is_all(diststyle: str) -> bool: + """True for Redshift's own `ALL` or `AUTO(ALL)` diststyle text. + + Checked as "names ALL and no KEY column" rather than an exact match against the finite + AWS-documented value set, because `AUTO(...)` wraps any of the other shapes too and + telling those apart needs `_diststyle_key_column`'s parsing either way. + """ + return "ALL" in diststyle.upper() and _diststyle_key_column(diststyle) is None + + +def _quote_ident(name: str) -> str: + """Quote an identifier, doubling any embedded double quote. + + See `postgres.py`'s identical helper. Duplicated rather than imported: DDL quoting is + each adapter's own small, self-contained concern, and it is not on the reuse list this + module was given (`_by_relation`, `_is_prefix`, `cost_share_of`, `ranking_key`, the + proposal-collapse machinery) — those are the pieces of shared *logic*, not this + engine-agnostic one-liner. + """ + return '"' + name.replace('"', '""') + '"' + + +def _qualified(schema: str, name: str) -> str: + """`"schema"."name"`, both parts quoted. See `postgres.py`'s identical helper.""" + return f"{_quote_ident(schema)}.{_quote_ident(name)}" + + +def propose_sortkey( + usage: Sequence[ColumnUsage], + facts: Mapping[Relation, TableFacts], + physical: Mapping[Relation, RedshiftTableFacts], + *, + min_cost_share: float, +) -> list[Proposal]: + """ADV101 — a SORTKEY candidate from the table's hottest RANGE/EQUALITY predicate. + + Redshift's zone maps store a min/max per 1MB block for the sort key column, so a scan + can skip whole blocks when the predicate is on that column. A time-series column under + a range predicate is the canonical win, but a hot equality predicate benefits the same + way, so both roles are pooled into one candidate list. + + **Confidence is capped at MEDIUM and there is deliberately no HIGH branch — do not add + one for symmetry with a Postgres index rule.** A SORTKEY change is only worth its + rewrite if the predicate is *selective*, and selectivity is exactly what cannot be + measured without per-column NDV, which Redshift does not expose at all (see the module + docstring's `CAP_NDV` note). Claiming HIGH here would assert something about data + distribution this tool cannot see, while recommending `ALTER TABLE ... ALTER SORTKEY`, + which rewrites the entire table. See `propose_distkey` for the DISTKEY-specific version + of the same argument, and ADV008 in `postgres.py` for the precedent this follows. + + Suppressed when the table's existing `sortkey1` already *is* the candidate column — the + SORTKEY equivalent of `postgres.py`'s `_covered`. If `sortkey1` itself could not be + read (its source value was SQL NULL), the claim "the table is not already sorted on + this column" is unknowable, so confidence drops to LOW and the rationale names the gap + — the same trap `_covered`'s docstring describes for an unreadable index catalog. + + A relation entirely absent from `physical` — as opposed to present with `sortkey1 is + None` — is a different, and materially worse, gap: `svv_table_info` omits both external + (Spectrum) tables, which cannot carry a SORTKEY at all, and genuinely empty local + tables, and nothing available anywhere in this adapter distinguishes the two (see + `RedshiftTableFacts`'s docstring). Proposing a table rewrite for something that might + not even support one is worse than proposing nothing, so this rule does not propose for + it at all — a documented gap, not a silent one, and not a guess either way. + + `facts` is accepted but not read: this rule's absence-of-evidence gate is entirely + `physical`'s (`RedshiftTableFacts` carries `sortkey1`; the engine-neutral `TableFacts` + row estimate has nothing this rule needs), kept in the signature so every ADV10x + proposal function takes the same four-argument shape from `propose()`'s call sites. + """ + proposals: list[Proposal] = [] + for relation, items in sorted(_by_relation(usage).items()): + candidates = sorted( + (i for i in items if i.role in (ColumnRole.RANGE, ColumnRole.EQUALITY)), + key=lambda i: (-i.cost_ms, i.column), + ) + if not candidates: + continue + best = candidates[0] + if best.cost_share < min_cost_share: + continue + + phys = physical.get(relation) + if phys is None: + # Cannot tell a Spectrum table from a genuinely empty one — see this + # function's own docstring. Not proposed, and not a silent skip: the reasoning + # lives above rather than in a per-run message, the same discipline this + # module already uses for every other documented gap. + continue + + if phys.sortkey1 is not None and phys.sortkey1 == best.column: + continue + + if phys.sortkey1 is None: + confidence = Confidence.LOW + rationale = ( + f"{best.column} carries the table's hottest range/equality predicate. " + "The table's existing sort key could not be read, so whether it is " + "already sorted on this column is unknown — confirm before applying." + ) + else: + confidence = Confidence.MEDIUM + rationale = ( + f"{best.column} carries the table's hottest range/equality predicate, and " + f"the table is currently sorted on {phys.sortkey1!r}, not this column. " + "Zone maps let a scan skip whole 1MB blocks when the predicate matches the " + "sort key, which the current sort key cannot provide for this predicate." + ) + rationale += ( + " Confidence is capped at MEDIUM: a SORTKEY change only repays the rewrite if " + "this predicate is selective, and Redshift exposes no per-column " + "distinct-value statistics to check that." + ) + if phys.stats_off is not None and phys.stats_off > 0: + rationale += ( + f" This table's planner statistics are {phys.stats_off:.0f}% stale " + "(stats_off) — treat any row-count-based reasoning elsewhere in this " + "report with that in mind." + ) + + proposals.append( + Proposal( + code="ADV101", + title=f"Consider SORTKEY on {relation}({best.column})", + rationale=rationale, + evidence={ + "schema": relation.schema, + "table": relation.table, + "column": best.column, + "role": best.role.value, + "cost_share": best.cost_share, + "calls": best.calls, + "current_sortkey1": phys.sortkey1, + "stats_off": phys.stats_off, + }, + confidence=confidence, + ddl=( + f"ALTER TABLE {_qualified(relation.schema, relation.table)} " + f"ALTER SORTKEY ({_quote_ident(best.column)});" + ), + note=( + "ALTER SORTKEY rewrites the entire table: Redshift copies every row, " + "holding a lock for the duration, and needs roughly the table's own " + "size again in free disk space while the rewrite runs. There is no " + "CONCURRENTLY equivalent — unlike a Postgres index, this cannot be " + "built alongside normal traffic. Run it in a maintenance window and " + "confirm free disk space first." + ), + ) + ) + return proposals + + +def propose_distkey( + usage: Sequence[ColumnUsage], + facts: Mapping[Relation, TableFacts], + physical: Mapping[Relation, RedshiftTableFacts], + *, + min_cost_share: float, +) -> list[Proposal]: + """ADV102 — a DISTKEY candidate from the table's hottest JOIN predicate. + + A join whose two sides are not distributed on the join key forces Redshift to + redistribute rows across the cluster before the join can run — `DS_BCAST_INNER` or the + heavier `DS_DIST_BOTH`, the same redistribution markers the offline `RedshiftAdapter` + names from an EXPLAIN plan (`sqlquality/adapters/redshift.py`). Distributing both sides + on the join key removes that step entirely. + + **Confidence is capped at MEDIUM, deliberately, with no HIGH branch** — see + `propose_sortkey`'s docstring for the shared reasoning, and do not add a HIGH branch + here either. The DISTKEY-specific version of it: `svv_table_info.skew_rows` describes + the table's *current* distribution, not the skew the proposed key would produce, and + Redshift exposes no per-column NDV to predict it — a bad DISTKEY choice does not merely + cost a slower scan, it can concentrate the whole table onto one node, which is exactly + the failure mode this rule cannot see coming. + + Suppressed when the table is already distributed on the candidate column — parsed out + of `svv_table_info.diststyle`'s `KEY(column)` (or `AUTO(KEY(column))`) text, since that + view carries no separate DISTKEY column (see `_diststyle_key_column`) — or when it is + already `DISTSTYLE ALL`, which already avoids redistribution entirely and is a strictly + better outcome than any single-column DISTKEY could offer (see `propose_diststyle_all`, + which proposes moving *to* ALL under its own, narrower gate). + + See `propose_sortkey` for the absence-from-`physical` handling: identical reasoning, + identical outcome — no proposal, not a guess. `facts` is accepted but not read, for the + same interface-symmetry reason `propose_sortkey` gives. + """ + proposals: list[Proposal] = [] + for relation, items in sorted(_by_relation(usage).items()): + candidates = sorted( + (i for i in items if i.role is ColumnRole.JOIN), + key=lambda i: (-i.cost_ms, i.column), + ) + if not candidates: + continue + best = candidates[0] + if best.cost_share < min_cost_share: + continue + + phys = physical.get(relation) + if phys is None: + continue + + diststyle = phys.diststyle + if diststyle is not None: + if _diststyle_is_all(diststyle): + continue + existing_key = _diststyle_key_column(diststyle) + if existing_key is not None and existing_key == best.column: + continue + + if diststyle is None: + confidence = Confidence.LOW + rationale = ( + f"{best.column} carries the table's hottest join predicate. The table's " + "current distribution style could not be read, so whether it is already " + "distributed on this column is unknown — confirm before applying." + ) + else: + confidence = Confidence.MEDIUM + rationale = ( + f"{best.column} carries the table's hottest join predicate, and the " + f"table's current distribution style is {diststyle!r}, not keyed on this " + "column. A join whose sides are not co-located on the join key forces " + "Redshift to redistribute rows across the cluster before it can complete " + "the join." + ) + rationale += ( + " Confidence is capped at MEDIUM: distribution skew is what makes a DISTKEY " + "choice good or catastrophic, and Redshift exposes no per-column " + "distinct-value statistics to predict it." + ) + if phys.skew_rows is not None: + rationale += ( + f" This table's current skew_rows is {phys.skew_rows:.2f}, but that " + "describes its *existing* distribution, not the skew this DISTKEY would " + "produce, which cannot be predicted from it." + ) + if phys.stats_off is not None and phys.stats_off > 0: + rationale += ( + f" This table's planner statistics are {phys.stats_off:.0f}% stale " + "(stats_off) — treat any row-count-based reasoning elsewhere in this " + "report with that in mind." + ) + + proposals.append( + Proposal( + code="ADV102", + title=f"Consider DISTKEY on {relation}({best.column})", + rationale=rationale, + evidence={ + "schema": relation.schema, + "table": relation.table, + "column": best.column, + "role": best.role.value, + "cost_share": best.cost_share, + "calls": best.calls, + "current_diststyle": diststyle, + "skew_rows": phys.skew_rows, + "stats_off": phys.stats_off, + }, + confidence=confidence, + ddl=( + f"ALTER TABLE {_qualified(relation.schema, relation.table)} " + f"ALTER DISTKEY {_quote_ident(best.column)};" + ), + note=( + "ALTER DISTKEY rewrites the entire table: Redshift redistributes and " + "copies every row across every node, holding a lock for the duration, " + "and needs roughly the table's own size again in free disk space while " + "the rewrite runs. There is no CONCURRENTLY equivalent. Run it in a " + "maintenance window and confirm free disk space first." + ), + ) + ) + return proposals + + +def propose_diststyle_all( + usage: Sequence[ColumnUsage], + facts: Mapping[Relation, TableFacts], + physical: Mapping[Relation, RedshiftTableFacts], + *, + min_cost_share: float, + max_rows: int = MAX_ROWS_FOR_DISTSTYLE_ALL, +) -> list[Proposal]: + """ADV103 — DISTSTYLE ALL for a small, frequently-joined dimension. + + Replicating a small table to every node removes redistribution for every join against + it, at every future query, rather than co-locating on one join key at a time the way + `propose_distkey` does — the natural proposal for a dimension joined from several + directions, where no single DISTKEY could serve every join. + + Gated on a row-count *ceiling*, the inverse of the floor Postgres's index rules use + (`MIN_ROWS_FOR_INDEX` in `postgres.py`): an index below that floor is wasted write + overhead, but DISTSTYLE ALL above this ceiling is wasted — and amplified — storage and + write cost. The failure direction inverts along with the gate. + + **Confidence is capped at MEDIUM, deliberately, with no HIGH branch** — the same + reasoning `propose_sortkey` and `propose_distkey` give: this rule's ceiling is a + row-count heuristic, not a measurement of the storage or write cost this table will + actually incur once replicated, and Redshift's lack of per-column NDV means no sharper + number is available either. + + Every proposal states the cost plainly, at every confidence rung: storage is + multiplied by the cluster's node count, and every write against this table is now + replicated to all of them too. + """ + proposals: list[Proposal] = [] + for relation, items in sorted(_by_relation(usage).items()): + joins = [i for i in items if i.role is ColumnRole.JOIN] + if not joins: + continue + cost_share = max(i.cost_share for i in joins) + if cost_share < min_cost_share: + continue + + table_facts = facts.get(relation) + rows = table_facts.row_estimate if table_facts else None + phys = physical.get(relation) + if phys is None: + continue + if rows is not None and rows > max_rows: + continue + + diststyle = phys.diststyle + if diststyle is not None and _diststyle_is_all(diststyle): + continue + + if rows is None and diststyle is None: + confidence = Confidence.LOW + rationale = ( + f"{relation} is joined by queries carrying a hot share of workload cost. " + f"Its row count could not be verified against this rule's " + f"{max_rows:,}-row ceiling, and its current distribution style could not " + "be read either, so whether it is already DISTSTYLE ALL is unknown — " + "confirm both before applying." + ) + elif rows is None: + confidence = Confidence.LOW + rationale = ( + f"{relation} is joined by queries carrying a hot share of workload cost, " + f"and its current distribution style is {diststyle!r}, not ALL. Its row " + f"count could not be verified against this rule's {max_rows:,}-row " + "ceiling for a 'small' dimension — confirm it before applying." + ) + elif diststyle is None: + confidence = Confidence.LOW + rationale = ( + f"{relation} is joined by queries carrying a hot share of workload cost " + f"and has an estimated {rows:,} rows, at or under this rule's " + f"{max_rows:,}-row ceiling for a 'small' dimension. Its current " + "distribution style could not be read, so whether it is already " + "DISTSTYLE ALL is unknown — confirm before applying." + ) + else: + confidence = Confidence.MEDIUM + rationale = ( + f"{relation} is joined by queries carrying a hot share of workload cost, " + f"has an estimated {rows:,} rows (at or under this rule's " + f"{max_rows:,}-row ceiling for a 'small' dimension), and its current " + f"distribution style is {diststyle!r}, not ALL. Replicating it to every " + "node removes redistribution for every join against it, not just one " + "column's worth." + ) + rationale += ( + " Confidence is capped at MEDIUM for the same reason ADV101/ADV102 are: this " + "rule's row-count ceiling is a heuristic, not a measurement of the storage and " + "write cost this table will actually incur once replicated." + ) + rationale += ( + " DISTSTYLE ALL multiplies this table's storage by the cluster's node count, " + "and every INSERT/UPDATE/DELETE against it is now replicated to every node too " + "— confirm both are acceptable before applying." + ) + if phys.stats_off is not None and phys.stats_off > 0: + rationale += ( + f" This table's planner statistics are {phys.stats_off:.0f}% stale " + "(stats_off) — treat the row estimate above with that in mind." + ) + + proposals.append( + Proposal( + code="ADV103", + title=f"Consider DISTSTYLE ALL on {relation}", + rationale=rationale, + evidence={ + "schema": relation.schema, + "table": relation.table, + "cost_share": cost_share, + "calls": max(i.calls for i in joins), + "row_estimate": rows, + "current_diststyle": diststyle, + "stats_off": phys.stats_off, + }, + confidence=confidence, + ddl=( + f"ALTER TABLE {_qualified(relation.schema, relation.table)} " + "ALTER DISTSTYLE ALL;" + ), + note=( + "ALTER DISTSTYLE ALL rewrites the entire table: Redshift copies every " + "row to every node, holding a lock for the duration, and needs disk " + "space on every node for the copy. There is no CONCURRENTLY " + "equivalent. After this runs, storage for this table is multiplied by " + "the node count and every write to it is replicated to every node — " + "confirm both are acceptable, and run this in a maintenance window." + ), + ) + ) + return proposals + + +def propose_maintenance( + physical: Mapping[Relation, RedshiftTableFacts], + facts: Mapping[Relation, TableFacts], + *, + unsorted_threshold: float = UNSORTED_PCT_THRESHOLD, + stats_off_threshold: float = STATS_OFF_PCT_THRESHOLD, +) -> list[Proposal]: + """ADV104 — VACUUM (unsorted region) and ANALYZE (stale statistics), from direct + measurement. + + The one Redshift rule in this adapter whose remediation does not rewrite the table: + VACUUM reclaims sort order in place and ANALYZE only refreshes planner statistics. That + is also why it is the only one that can reasonably reach HIGH — `unsorted` and + `stats_off` are direct measurements Redshift already computed (see + `RedshiftTableFacts`'s docstring), not an inference this rule makes about data it + cannot see, which is what ADV101-103's MEDIUM cap is about. It is also the cheapest + thing an operator can act on, which is why the default ranking (highest confidence + first, see `WorkloadAdapter.ranking_key`) puts it near the top of a report without this + rule needing to do anything special. + + `stats_off` is a staleness *percentage*, not Postgres's never-analyzed sentinel — see + `RedshiftTableFacts`'s docstring and `_row_estimate`'s. This is the rule that turns it + into a proposal in its own right, rather than merely a caveat riding along with + someone else's evidence, which ADV101-103 each still disclose it as. + + No cost-share gating: unlike ADV101-103, this rule's evidence is a catalog measurement + about the table's own physical state, not about how the workload uses it, so + `--min-cost-share` cannot filter it — the same reasoning `cli.py`'s help text already + gives for ADV002 and ADV003 on the Postgres side. + + A relation whose `unsorted`/`stats_off` value is itself unmeasured (SQL NULL) yields no + proposal for that specific check: there is no measurement to disclose a gap about, and + "maybe you should VACUUM" without one would be exactly the confident-but-wrong claim + this whole rule set exists to avoid making about something else. + """ + proposals: list[Proposal] = [] + for relation in sorted(physical): + phys = physical[relation] + table_facts = facts.get(relation) + rows = table_facts.row_estimate if table_facts else None + + if phys.unsorted is not None and phys.unsorted >= unsorted_threshold: + proposals.append( + Proposal( + code="ADV104", + title=f"Run VACUUM on {relation}", + rationale=( + f"{phys.unsorted:.0f}% of {relation} is in the unsorted region " + f"(svv_table_info.unsorted), at or above this rule's " + f"{unsorted_threshold:.0f}% threshold. VACUUM reclaims sort order " + "so zone maps and merge joins can work again; unlike a SORTKEY or " + "DISTKEY change, it does not rewrite the table's distribution or " + "column definitions, only its physical row order." + ), + evidence={ + "schema": relation.schema, + "table": relation.table, + "unsorted": phys.unsorted, + "row_estimate": rows, + }, + confidence=Confidence.HIGH, + ddl=f"VACUUM {_qualified(relation.schema, relation.table)};", + note=( + "VACUUM is heavy: it reads and rewrites the unsorted portion of " + "the table and competes with other cluster activity for I/O. It " + "does not need a maintenance-window lock the way ALTER " + "SORTKEY/DISTKEY/DISTSTYLE do, but it can still run for a long " + "time on a large table — consider VACUUM SORT ONLY if reclaiming " + "deleted-row space is not also needed." + ), + ) + ) + + if phys.stats_off is not None and phys.stats_off >= stats_off_threshold: + proposals.append( + Proposal( + code="ADV104", + title=f"Run ANALYZE on {relation}", + rationale=( + f"{relation}'s planner statistics are {phys.stats_off:.0f}% stale " + "(svv_table_info.stats_off), at or above this rule's " + f"{stats_off_threshold:.0f}% threshold. ANALYZE refreshes them; it " + "does not rewrite the table at all." + ), + evidence={ + "schema": relation.schema, + "table": relation.table, + "stats_off": phys.stats_off, + "row_estimate": rows, + }, + confidence=Confidence.HIGH, + ddl=f"ANALYZE {_qualified(relation.schema, relation.table)};", + note=( + "ANALYZE reads a sample of the table to refresh planner " + "statistics; it takes no exclusive lock and does not rewrite any " + "row, but it is still real I/O against the cluster." + ), + ) + ) + return proposals + + +def _advisor_category(row: RedshiftAdvisorRow) -> str | None: + """Best-effort classification of one Advisor row into the same category + `propose_sortkey`/`propose_distkey`/`propose_diststyle_all` each propose in, so + `_disclose_advisor_agreement` can detect agreement. `None` when neither `rec_type` nor + `recommended_ddl` can be read as one of them — an unclassified row is still surfaced by + `propose_advisor`, it simply cannot be cross-referenced against our own rules. + + This classification drives *only* the agreement disclosure, never `propose_advisor`'s + own confidence (always HIGH — see its docstring) and never ADV101/102/103's confidence + cap: it is a guess about which of our rules an Advisor row corresponds to, not a fact + either rule's confidence should turn on. + """ + rec_type = row.rec_type.lower() + if "sort" in rec_type: + return _CATEGORY_SORTKEY + if "dist" in rec_type: + ddl = (row.recommended_ddl or "").upper() + return _CATEGORY_DISTSTYLE_ALL if "ALL" in ddl else _CATEGORY_DISTKEY + return None + + +def propose_advisor(rows: Sequence[RedshiftAdvisorRow]) -> list[Proposal]: + """ADV105 — surface Amazon Redshift Advisor's own recommendations, clearly attributed. + + This is the one signal in this whole adapter that comes from the cluster's own + analysis rather than from sqlquality's inference over the workload — see the module + docstring's `CAP_ADVISOR` note and the plan's "why the rules are not the Postgres + rules renamed" section. Confidence is HIGH unconditionally: unlike ADV101-103, this + proposal makes no claim of our own about distribution skew or predicate selectivity — + it relays a conclusion Redshift's own optimizer already reached, which sqlquality did + not derive and has not independently verified. + + `note` says so explicitly, in the field `render_ddl` (Task 7) prints directly above the + DDL: this is the one proposal in the whole adapter whose DDL sqlquality did not + generate, and that must stay visible to whoever is about to run it, not only to whoever + reads `rationale`. + + Never folded into an ADV101/102/103 proposal here or elsewhere — see + `_disclose_advisor_agreement`, which appends a sentence to a *matching* proposal's + rationale instead of merging the two into one object, so a reader can always tell which + conclusion is ours and which is Advisor's. + """ + proposals: list[Proposal] = [] + for row in sorted(rows, key=lambda r: (r.relation.schema, r.relation.table, r.rec_type)): + relation = row.relation + rationale = ( + "Amazon Redshift Advisor (svv_alter_table_recommendations) recommends this " + "change based on its own analysis of the cluster — sqlquality did not " + "generate or verify this recommendation, it only relays it. Advisor's " + "analysis may reflect an earlier snapshot of this table's usage, so confirm " + "it still applies before acting on it." + ) + if row.current_ddl: + rationale += f" Current: {row.current_ddl}." + if row.recommended_ddl: + rationale += f" Recommended: {row.recommended_ddl}." + + proposals.append( + Proposal( + code="ADV105", + title=( + f"Amazon Redshift Advisor recommends a {row.rec_type} change for {relation}" + ), + rationale=rationale, + evidence={ + "schema": relation.schema, + "table": relation.table, + # Machine-readable attribution, alongside the prose in `rationale` and + # `note`: a caller rendering evidence as bare `k=v` pairs (the report's + # own discipline — see `cost_share_of`'s docstring) still gets a + # signal that this proposal's source is Advisor, not this adapter's + # own inference. + "source": "amazon_redshift_advisor", + "recommendation_type": row.rec_type, + "current_ddl": row.current_ddl, + "recommended_ddl": row.recommended_ddl, + }, + confidence=Confidence.HIGH, + ddl=row.recommended_ddl, + note=( + "Source: Amazon Redshift Advisor, not sqlquality's own analysis — " + "this statement was generated by Redshift itself and is relayed " + "verbatim. Review it exactly as you would a recommendation found " + "directly on the Redshift console." + ), + ) + ) + return proposals + + +def _disclose_advisor_agreement( + proposals: list[Proposal], advisor_rows: Sequence[RedshiftAdvisorRow] +) -> list[Proposal]: + """Append a sentence to an ADV101/102/103 proposal when Advisor independently + recommends the same category of change for the same relation. + + Agreement is scoped to (relation, category) — not to the exact column or DDL text — + because Advisor's DDL and ours are generated independently and are not expected to be + byte-identical; a column-level match would silently miss genuine agreement over a + cosmetic difference in how the two describe it. This is the strongest evidence this + adapter can produce, since Advisor's signal comes from the cluster itself rather than + from sqlquality's inference — see the module docstring and `propose_advisor`. + + Disclosed as an extra sentence in the existing proposal's rationale — never by raising + its `Confidence` past the documented MEDIUM cap (see `propose_sortkey`, + `propose_distkey`, `propose_diststyle_all`), and never by merging the Advisor row into + this proposal, which stays a separate ADV105 entry so a reader can always tell whose + conclusion is whose. + """ + agreeing: set[tuple[Relation, str]] = set() + for row in advisor_rows: + category = _advisor_category(row) + if category is not None: + agreeing.add((row.relation, category)) + + if not agreeing: + return proposals + + updated: list[Proposal] = [] + for proposal in proposals: + category = _PROPOSAL_CATEGORY.get(proposal.code) + relation = Relation( + schema=str(proposal.evidence.get("schema")), + table=str(proposal.evidence.get("table")), + ) + if category is not None and (relation, category) in agreeing: + proposal = replace( + proposal, + rationale=proposal.rationale + + " Amazon Redshift Advisor independently recommends the same kind of " + "change for this table (see its own ADV105 proposal for the exact " + "wording) — agreement between the two is the strongest evidence this " + "adapter can produce, since Advisor's signal comes from the cluster " + "itself.", + ) + updated.append(proposal) + return updated + + +def _relation_of(proposal: Proposal) -> Relation: + """The relation an ADV101/102/103 proposal is about, recovered from its own evidence. + + Every one of those three proposals' `evidence` carries plain `schema`/`table` strings + (not a `Relation`, which is not JSON-safe) — this is the one place that reconstitutes + it, so `_collapse_diststyle_all_over_distkey` and `_disclose_advisor_agreement` do not + each re-derive the same lookup with their own, possibly diverging, logic. + """ + return Relation( + schema=str(proposal.evidence.get("schema")), table=str(proposal.evidence.get("table")) + ) + + +def _collapse_diststyle_all_over_distkey(proposals: list[Proposal]) -> list[Proposal]: + """Suppress ADV102 (DISTKEY) for a relation where ADV103 (DISTSTYLE ALL) also fired, + and say why in the survivor. + + `DISTSTYLE ALL` replicates the whole table to every node, which removes redistribution + for *every* join against it — a strictly better outcome than co-locating on any single + join key, which is exactly what ADV102 offers and what its own docstring already says + about ADV103. Emitting both, silently, for the same relation recommends two conflicting + hours-long rewrites: an operator following both runs one full-table rewrite and then + another that undoes it. This is the same defect class Batch 2 shipped for + Postgres — ADV001 and ADV007 proposing a `(a, b)` index alongside an `(a)` index ADV003 + would then advise dropping — except the relationship here is *subsumption of strategy*, + not a column-prefix, so the fix cannot be `postgres.py`'s `_collapse_index_prefixes` or + `_dedupe_by_ddl` as-is: both compare `columns` tuples on a `PgIndex`-shaped proposal, + which has no meaning for "one distribution strategy makes another redundant." What *is* + reused from `postgres.py` is the shape of the fix and `_sentences`, the same + fold-and-attribute discipline `_fold_discarded` uses: keep the stronger proposal, drop + the weaker one, and fold every one of its distinguishing sentences into the survivor + rather than discarding them along with the whole object — so a caveat that only existed + on the discarded ADV102 (its `skew_rows`/`stats_off` disclosures, say) still reaches the + operator instead of vanishing with it. + + A relation with only one of the two present is untouched: this function only ever + removes an ADV102 proposal, and only when an ADV103 proposal for the *same* relation + also survived this run. + """ + diststyle_all_relations = {_relation_of(p) for p in proposals if p.code == "ADV103"} + if not diststyle_all_relations: + return proposals + + kept: list[Proposal] = [] + withheld_by_relation: dict[Relation, Proposal] = {} + for proposal in proposals: + if proposal.code == "ADV102" and _relation_of(proposal) in diststyle_all_relations: + withheld_by_relation[_relation_of(proposal)] = proposal + continue + kept.append(proposal) + + if not withheld_by_relation: + return kept + + updated: list[Proposal] = [] + for proposal in kept: + withheld = ( + withheld_by_relation.get(_relation_of(proposal)) if proposal.code == "ADV103" else None + ) + if withheld is not None: + seen = set(_sentences(proposal.rationale)) + fresh = [s for s in _sentences(withheld.rationale) if s not in seen] + column = withheld.evidence.get("column") + addition = ( + f" ADV102 also proposed a DISTKEY on {column} for this table at " + f"{withheld.confidence.value} confidence, withheld here: DISTSTYLE ALL " + "already removes redistribution entirely for every join against this " + "table, which strictly subsumes any single-column DISTKEY choice — " + "applying both would mean one full-table rewrite undoing another." + ) + if fresh: + addition += " " + " ".join(fresh) + proposal = replace(proposal, rationale=proposal.rationale + addition) + updated.append(proposal) + return updated + + +def _skipped_for_physical_gap( + usage: Sequence[ColumnUsage], physical: Mapping[Relation, RedshiftTableFacts] +) -> int: + """How many relations carrying a hot RANGE/EQUALITY/JOIN predicate were declined by + ADV101/102/103 because they are absent from `physical`. + + Counted, not silently absorbed: `RedshiftTableFacts`'s docstring explains why that + absence cannot, by itself, distinguish a Spectrum (external) table — which cannot carry + a SORTKEY, DISTKEY or DISTSTYLE at all — from a genuinely empty local table, and why + `propose_sortkey`/`propose_distkey`/`propose_diststyle_all` each decline rather than + guess for such a relation. A decision to decline is still a decision an operator is + entitled to see; this is what lets `propose()` disclose it through `self.degraded` + rather than leaving the relation to vanish with no count, no degradation entry and no + stderr line — the same silent-omission failure mode this feature keeps closing + elsewhere. + + Scoped to relations that actually carried a candidate role for at least one of the + three rules — a relation with, say, only `GROUP`/`SORT` usage was never going to get a + SORTKEY/DISTKEY/DISTSTYLE ALL proposal even with `physical` present, so counting it + here would overstate the gap this rule set actually left. + """ + candidate_roles = (ColumnRole.RANGE, ColumnRole.EQUALITY, ColumnRole.JOIN) + return sum( + 1 + for relation, items in _by_relation(usage).items() + if relation not in physical and any(item.role in candidate_roles for item in items) + ) + + +class RedshiftWorkloadAdapter(WorkloadAdapter): + engine = "redshift" + + SQL: dict[str, str] = { + # Column names below come from AWS's Redshift system-view documentation and have + # NOT been executed against a live cluster (see the module docstring). Every + # consumer unpacks defensively and a denied or malformed statement is recorded in + # `degraded` rather than raised, so a wrong name costs one capability instead of + # the run. + # + # `status = 'success'` excludes failed and cancelled statements, which carry no + # useful cost signal and would otherwise dilute cost_share with executions that + # never finished. `database_name = current_database()` scopes to the connected + # database exactly as the Postgres adapter's CAP_WORKLOAD scopes to + # `current_database()` via `pg_database`. + # + # `elapsed_time` is documented as microseconds, unlike `pg_stat_statements + # .total_exec_time`'s milliseconds — fetch_workload() divides by 1000. + # + # `(CAST(%s AS timestamptz) IS NULL OR start_time >= %s)`, not a bare + # `start_time >= %s`: unlike `pg_stat_statements`, which carries no per-statement + # timestamp at all, `sys_query_history.start_time` genuinely lets `--since` be + # honoured here — see fetch_workload()'s docstring and its honest + # `window_description` either way. The same bind value is passed twice (`None` + # when `--since` was not given) so one static, syntax-checkable statement serves + # both cases rather than two near-duplicate strings that could drift apart. + # + # The explicit `CAST(... AS timestamptz)` is load-bearing, not decoration. A bare + # `%s IS NULL` gives the driver no other typed operand in that branch of the OR to + # infer a type from, and every run *without* `--since` binds `None` there — + # reproduced live against `postgres:16` through the identical psycopg wire path: + # `IndeterminateDatatype: could not determine data type of parameter $1`. `_run` + # then swallows it into `degraded`, so the run reported a zero-query workload — + # exactly the "healthy cluster, no traffic" failure mode this module's own + # docstring says it exists to prevent, for the *default* invocation with no + # `--since` at all. See `tests/integration/test_redshift_introspection_bindable_live + # .py`, which executes every one of this adapter's four statements against + # `postgres:16` with representative binds specifically to catch this class of bug + # — a statement that cannot even be prepared — locally, rather than assuming + # bindability is untestable just because the tables underneath are Redshift-only. + CAP_WORKLOAD: """ + SELECT query_text, elapsed_time + FROM sys_query_history + WHERE database_name = current_database() + AND status = 'success' + AND (CAST(%s AS timestamptz) IS NULL OR start_time >= %s) + ORDER BY elapsed_time DESC + LIMIT %s + """, + # svv_columns is Redshift's own columns view (distinct from information_schema, + # which Redshift also exposes but which AWS documents less completely for this + # engine). No reserved words here, unlike CAP_TABLE_FACTS below. It also includes + # external (Spectrum) tables, unlike CAP_TABLE_FACTS's svv_table_info — see + # fetch_schema()'s docstring. + CAP_SCHEMA: """ + SELECT schema_name, table_name, column_name, data_type + FROM svv_columns + WHERE schema_name = ANY(%s) + """, + # svv_table_info's own column names are the reserved words "schema" and "table" — + # both stay double-quoted so the statement parses at all; dropping either quote + # breaks the statement (verified with sqlglot's redshift dialect — see + # test_every_statement_parses_as_redshift_sql). `tbl_rows` and `size` are the row + # estimate and size-in-MB columns per AWS's documentation; `unsorted`, `stats_off`, + # `diststyle`, `sortkey1` and `skew_rows` are the physical-design evidence ADV103 + # (DISTSTYLE ALL) and ADV104 (VACUUM/ANALYZE) need — see `RedshiftTableFacts`. + # `stats_off` is a staleness *percentage*, not a never-analyzed flag — see + # `RedshiftTableFacts`'s docstring — and does not gate `tbl_rows`/`size`. + CAP_TABLE_FACTS: """ + SELECT "schema", "table", tbl_rows, size, unsorted, stats_off, diststyle, + sortkey1, skew_rows + FROM svv_table_info + WHERE "schema" = ANY(%s) AND "table" = ANY(%s) + """, + # svv_alter_table_recommendations is Redshift Advisor's own view of ALTER TABLE + # ... ALTER DISTSTYLE / ALTER SORTKEY recommendations, which is what "advisor" names + # here — the source for ADV101-ADV105 in a later task. Column names are AWS's + # documented ones for this view; unlike svv_table_info none of them are reserved + # words. + CAP_ADVISOR: """ + SELECT database_name, schema_name, table_name, type, current_ddl, recommended_ddl + FROM svv_alter_table_recommendations + WHERE schema_name = ANY(%s) AND table_name = ANY(%s) + """, + } + + def __init__(self, querier: Querier | None = None) -> None: + super().__init__() + self._query = querier + #: CAP_SCHEMA rows per schema tuple. Both fetch_schema and fetch_table_facts need + #: them, and running the statement twice did twice the catalog work and — worse — + #: would append two identical entries to `degraded` when it was denied. Mirrors + #: `PostgresWorkloadAdapter`'s identical cache. + self._schema_cache: dict[tuple[str, ...], list[tuple[object, ...]]] = {} + #: Redshift-specific physical facts from the most recent `fetch_table_facts` call, + #: keyed the same way as its `TableFacts` return value — see `RedshiftTableFacts`. + #: A later task's SORTKEY/DISTKEY rules read this directly rather than through a + #: second introspection round trip, since one CAP_TABLE_FACTS query already carries + #: both the engine-neutral and the Redshift-specific columns. + self.physical_facts: dict[Relation, RedshiftTableFacts] = {} + + def introspection_sql(self) -> list[IntrospectionStatement]: + return [ + IntrospectionStatement(capability=cap, sql=sql.strip(), privilege_hint=_HINTS[cap]) + for cap, sql in self.SQL.items() + ] + + def _run(self, capability: str, params: tuple[object, ...]) -> list[tuple[object, ...]]: + """Run one introspection statement, recording degradation rather than raising. + + A single missing grant must cost only that capability — never the whole run. See + `PostgresWorkloadAdapter._run`, which this mirrors exactly. + """ + if self._query is None: + raise RuntimeError("connect() must be called before fetching") + try: + return self._query(self.SQL[capability], params) + except Exception as exc: # driver-specific; we only need the message + self.degraded.append((capability, f"{exc} — {_HINTS[capability]}")) + return [] + + def connect(self, params: ConnectionParams, timeout_s: int) -> None: + """Open a read-only session over the PostgreSQL wire protocol. + + Redshift speaks libpq through psycopg exactly as Postgres does, so the whole + session-setup mechanism — driver import, conninfo construction inside the + scrubbing envelope, the statement timeout, secret scrubbing on a driver failure — + is shared with `PostgresWorkloadAdapter.connect` via `open_session`; see + `session.py`'s module docstring for why it lives there rather than being copied. + + The one thing this adapter does differently is what a refused + `SET default_transaction_read_only = on` means: Redshift does not accept that + statement in every configuration, and unlike Postgres its refusal here does not + abort the connection. It is recorded in `self.degraded` instead, so the report + says plainly that the session could not be proven read-only — continuing + silently as though the statement had succeeded would misstate the one guarantee + this tool exists to keep. The connection is still safe to use regardless: this + adapter only ever issues the four `SELECT` statements in `SQL` above, pinned by + `test_no_statement_writes`. + """ + psycopg = import_psycopg("Redshift", "warehouse") + + # Silence is the failure mode being fixed here: a dropped `sslmode` downgrades + # the connection with no signal at all. Key names only — see + # `dropped_libpq_fields`. + dropped = dropped_libpq_fields(params.fields, LIBPQ_FIELD_MAP, LIBPQ_PASSTHROUGH_FIELDS) + if dropped: + print( + f"warning: ignoring connection setting(s) not supported by the Redshift " + f"adapter: {', '.join(dropped)}. Pass --dsn if you need them.", + file=sys.stderr, + ) + + # Everything we know to be secret, so a driver exception can be proven clean + # rather than trusted. + secrets = secrets_for(params) + + # `read_only_required=False`: a refusal here is recorded as a degradation, not + # raised — see this method's own docstring and `open_session`'s parameter of the + # same name. + query, degradation = open_session( + psycopg=psycopg, + # Inside the scrubbing envelope: psycopg raises from make_conninfo on an + # unusable keyword, and that message can quote the offending value — which + # for the `password` keyword is the password. + conninfo_factory=lambda: ( + params.dsn + or psycopg.conninfo.make_conninfo( + **translate_libpq_fields( + params.fields, LIBPQ_FIELD_MAP, LIBPQ_PASSTHROUGH_FIELDS + ) + ) + ), + secrets=secrets, + timeout_s=timeout_s, + min_timeout_s=MIN_TIMEOUT_S, + max_timeout_s=MAX_TIMEOUT_S, + read_only_sql=READ_ONLY_SQL, + read_only_required=False, + ) + self._query = query + if degradation is not None: + self.degraded.append((DEGRADATION_READ_ONLY, degradation)) + + def fetch_workload(self, since: timedelta | None, limit: int) -> WorkloadFetch: + """Raw query-history rows plus an honest description of the window they cover. + + Unlike `pg_stat_statements`, `sys_query_history` carries a `start_time` per + execution — so unlike `PostgresWorkloadAdapter.fetch_workload`, `--since` genuinely + can be honoured here, and `window_description` says so plainly either way, the same + discipline the Postgres adapter uses to say the opposite. The statement is also + `ORDER BY elapsed_time DESC LIMIT n`, though, so what is actually returned is *the + n most expensive successful queries* since that cutoff (or overall, with no + `--since`) — not literally everything since then. `window_description` says so + explicitly rather than implying full coverage, because `cost_share` denominators + throughout the rest of this run are computed over exactly that truncated set. + + `sys_query_history` returns one row per *execution*, not per normalised statement — + `pg_stat_statements` pre-aggregates by fingerprint, this view does not. So `calls` + is always 1 on the `RawQueryRow`s built here; the collapse into one `QueryStat` per + fingerprint, with `calls` and `total_time_ms` summed, happens in `ingest()` — see + `tests/test_workload_redshift.py`'s test pinning that two executions of the same + statement actually do collapse, rather than assuming it. + + That collapse is keyed on `ingest()`'s redacted, re-serialised SQL text, which is + sensitive to exactly how the raw text was written — and `sys_query_history` stores + the *verbatim* text the client sent, unlike `pg_stat_statements`, which Postgres has + already parsed and re-serialised (identifiers folded to lowercase) before storing. + So two executions that are, semantically, one statement can still fingerprint as two + separate `QueryStat`s here if they differ only in identifier case or in an attached + comment (an ORM query tag, for instance) — deliberately left undisclosed-but-real + rather than "fixed" by normalising identifiers before `ingest()` runs, since a + general case-fold cannot tell an unquoted identifier (case-insensitive) apart from a + deliberately-quoted, case-sensitive one without risking folding a real distinction + away. Pinned by + `test_identifier_case_and_comments_can_still_split_one_statement_into_two_stats`. + The consequence is real, not merely cosmetic: splitting one statement's cost across + two `QueryStat`s inflates the number of groups the workload's total cost is spread + over, which shrinks every `cost_share` and makes `--min-cost-share` correspondingly + stricter for the affected statement. + """ + cutoff = None if since is None else datetime.now(timezone.utc) - since + rows = self._run(CAP_WORKLOAD, (cutoff, cutoff, limit)) + if cutoff is not None: + window = ( + f"the {limit} most expensive successful queries since {cutoff.isoformat()} " + "in sys_query_history (--since is honoured: sys_query_history carries a " + "per-execution start_time, unlike pg_stat_statements)" + ) + else: + window = ( + f"the {limit} most expensive successful queries recorded in " + "sys_query_history (no --since filter applied)" + ) + return WorkloadFetch( + rows=tuple( + RawQueryRow( + sql=str(sql), + calls=1, + # elapsed_time is documented in microseconds; total_time_ms wants + # milliseconds. A NULL elapsed_time (not documented as possible for a + # 'success' row, but nothing here can prove it can't happen) must not + # raise past this point: an uncaught TypeError here would crash the + # whole run for one malformed row, exactly the failure `_run`'s + # try/except exists to prevent for a denied statement — that guarantee + # is worthless if a single bad row can still take down the run one + # level up. Treated as zero cost rather than dropping the row, so the + # call is still counted; zero cost is honestly conservative, since + # `total_time_ms` has no `None`/unknown state to fall back to. + total_time_ms=(_as_float(elapsed) / 1000.0) if elapsed is not None else 0.0, + ) + for sql, elapsed in rows + ), + window_description=window, + ) + + def _schema_rows(self, schemas: tuple[str, ...]) -> list[tuple[object, ...]]: + """CAP_SCHEMA rows, fetched at most once per schema tuple. See `_schema_cache`.""" + if schemas not in self._schema_cache: + self._schema_cache[schemas] = self._run(CAP_SCHEMA, (list(schemas),)) + return self._schema_cache[schemas] + + def fetch_schema(self, schemas: tuple[str, ...]) -> dict: + """Nested schema mapping for sqlglot qualify(): {schema: {table: {column: type}}}. + + `svv_columns` includes external (Spectrum) tables; `svv_table_info` — what + `fetch_table_facts` reads — does not (an external table cannot carry SORTKEY or + DISTSTYLE, so Redshift never lists one there). So this map can, correctly, carry a + relation `fetch_table_facts` never returns a fact for: a query joining an external + table still needs its columns to qualify, or the whole statement is dropped as + unqualifiable — see `fetch_table_facts`'s docstring for the consequence on the + other side of that gap. + + Nested rather than flat for the same reason `PostgresWorkloadAdapter.fetch_schema` + is: a flat map cannot tell two same-named tables in different schemas apart. + """ + schema: dict[str, dict[str, dict[str, str]]] = {} + for schema_name, table, column, data_type in self._schema_rows(schemas): + schema.setdefault(str(schema_name), {}).setdefault(str(table), {})[str(column)] = str( + data_type + ) + return schema + + def fetch_table_facts( + self, schemas: tuple[str, ...], relations: frozenset[Relation] + ) -> dict[Relation, TableFacts]: + """Row estimates, sizes and columns — plus Redshift's own physical-design facts, + stashed on `self.physical_facts` for a later task (see `RedshiftTableFacts`). + + Deliberately does **not** create an entry for every relation in `relations`, unlike + `PostgresWorkloadAdapter.fetch_table_facts`. `svv_columns` includes external + (Spectrum) tables and `svv_table_info` does not (see `fetch_schema`'s docstring), + so a relation named in `relations` can legitimately never appear in this method's + `svv_table_info` rows at all — forcing an entry anyway, every field `None`, would + make that indistinguishable from a real table this method genuinely has no facts + for. A relation's simple absence from the returned dict (and from + `self.physical_facts`) is the signal a later task's rules must check for instead. + + **That absence is not, by itself, proof of a Spectrum table** — AWS also omits + genuinely *empty* tables from `svv_table_info` — so a later task proposing + SORTKEY/DISTKEY from this absence needs an additional signal to tell the two cases + apart; see `RedshiftTableFacts`'s docstring. Recorded as a carry-forward, not + solved here. + """ + wanted = sorted({relation.table for relation in relations}) + columns: dict[Relation, list[str]] = {} + for schema_name, table, column, _type in self._schema_rows(schemas): + relation = Relation(schema=str(schema_name), table=str(table)) + if relation in relations: + columns.setdefault(relation, []).append(str(column)) + + facts: dict[Relation, TableFacts] = {} + physical: dict[Relation, RedshiftTableFacts] = {} + for ( + schema_name, + table, + tbl_rows, + size, + unsorted, + stats_off, + diststyle, + sortkey1, + skew_rows, + ) in self._run(CAP_TABLE_FACTS, (list(schemas), wanted)): + relation = Relation(schema=str(schema_name), table=str(table)) + # Same over-fetch guard `PostgresWorkloadAdapter.fetch_table_facts` documents: + # the table parameter is bare names, so a same-named table in a *different* + # requested schema that is not itself in `relations` can come back too. + if relation not in relations: + continue + facts[relation] = TableFacts( + relation=relation, + row_estimate=_row_estimate(tbl_rows), + size_bytes=_size_bytes(size), + columns=tuple(columns.get(relation, ())), + ) + physical[relation] = RedshiftTableFacts( + unsorted=_as_float(unsorted) if unsorted is not None else None, + stats_off=_as_float(stats_off) if stats_off is not None else None, + diststyle=str(diststyle) if diststyle is not None else None, + sortkey1=str(sortkey1) if sortkey1 is not None else None, + skew_rows=_as_float(skew_rows) if skew_rows is not None else None, + ) + self.physical_facts = physical + return facts + + def _advisor_rows( + self, schemas: tuple[str, ...], relations: frozenset[Relation] + ) -> list[RedshiftAdvisorRow]: + """CAP_ADVISOR rows for the given relations — ADV105's raw material. + + Same over-fetch guard as `fetch_table_facts`: the statement filters on bare table + names, so a same-named table in a different requested schema can come back too and + must be dropped here rather than misattributed to a relation that never asked for + it. + """ + wanted = sorted({relation.table for relation in relations}) + rows: list[RedshiftAdvisorRow] = [] + for ( + _database_name, + schema_name, + table, + rec_type, + current_ddl, + recommended_ddl, + ) in self._run(CAP_ADVISOR, (list(schemas), wanted)): + relation = Relation(schema=str(schema_name), table=str(table)) + if relation not in relations: + continue + rows.append( + RedshiftAdvisorRow( + relation=relation, + rec_type=str(rec_type), + current_ddl=str(current_ddl) if current_ddl is not None else None, + recommended_ddl=(str(recommended_ddl) if recommended_ddl is not None else None), + ) + ) + return rows + + def propose( + self, + aggregation: Aggregation, + facts: dict[Relation, TableFacts], + workload: Workload, + *, + min_cost_share: float, + ) -> list[Proposal]: + """ADV101-105 — see each `propose_*` function's own docstring for its rule. + + `workload` is accepted (the ABC requires it uniformly across engines) but unused + here: unlike Postgres's ADV005/ADV006, no Redshift rule in this task reads raw + query text — every one of ADV101-104 works from `aggregation.usage` and + `self.physical_facts`, and ADV105 works from Advisor's own catalog rows. + + Two passes run after the four rules produce their raw proposals, in this order: + `_collapse_diststyle_all_over_distkey` first, so a withheld ADV102's rationale is + already folded into ADV103 by the time `_disclose_advisor_agreement` looks for a + `(relation, category)` match — an ADV102 dropped by the collapse must not also be + the one Advisor agreement gets attached to, since it no longer exists in the + returned list at all. **That order is load-bearing and pinned** by + `test_propose_collapses_before_disclosing_agreement`: reversed, the agreement + sentence lands on the ADV102 the collapse is about to withhold, the collapse then + folds it into the surviving ADV103, and ADV103 ends up claiming Advisor agrees with + a `DISTSTYLE ALL` recommendation Advisor never made — presenting Advisor's opinion + as ours, which is the one outcome ADV105's whole design exists to prevent. + + Advisor rows are fetched for `aggregation.tables` *and* every relation `facts` + covers, deliberately. `cli.py` fetches facts for `aggregation.tables | + star_tables(...)`, so a relation reached only by a `SELECT *` has table facts — + and therefore can earn an ADV104 proposal — while never appearing in + `aggregation.tables`. Scoping ADV105 to `aggregation.tables` alone made it + narrower than ADV104 on exactly those relations: no Advisor relay and no agreement + disclosure for a table this run is otherwise happy to propose maintenance on. The + union keeps the two rules' reach consistent, and `_advisor_rows` still drops any + row whose relation was not asked for. + """ + physical = self.physical_facts + skipped = _skipped_for_physical_gap(aggregation.usage, physical) + if skipped: + self.degraded.append( + ( + DEGRADATION_PHYSICAL_FACTS_GAP, + f"{skipped} relation(s) with a hot range/equality/join predicate were " + "not considered for SORTKEY/DISTKEY/DISTSTYLE ALL: absent from " + "svv_table_info, which cannot distinguish a Spectrum (external) table " + "— unable to carry any of these — from a genuinely empty local table, " + "so this rule set declines rather than guesses for them", + ) + ) + proposals = [ + *propose_sortkey(aggregation.usage, facts, physical, min_cost_share=min_cost_share), + *propose_distkey(aggregation.usage, facts, physical, min_cost_share=min_cost_share), + *propose_diststyle_all( + aggregation.usage, facts, physical, min_cost_share=min_cost_share + ), + *propose_maintenance(physical, facts), + ] + proposals = _collapse_diststyle_all_over_distkey(proposals) + advisor_rows = self._advisor_rows(self.schemas, aggregation.tables | frozenset(facts)) + proposals = proposals + propose_advisor(advisor_rows) + proposals = _disclose_advisor_agreement(proposals, advisor_rows) + return sorted(proposals, key=self.ranking_key) + + def render_ddl(self, proposals: list[Proposal]) -> str: + """A commented, reviewable script. sqlquality never executes this. + + **The header is far louder than `PostgresWorkloadAdapter.render_ddl`'s, and that is + the single most important thing about this method.** Every ADV101 (`ALTER + SORTKEY`), ADV102 (`ALTER DISTKEY`) and ADV103 (`ALTER DISTSTYLE ALL`) statement + this adapter can emit rewrites the entire table: Redshift copies every row, holds a + lock on the table for the whole rewrite, and needs roughly the table's own size + again in free disk space while it runs — on a large table that is hours, not + seconds. Postgres's header can recommend `CONCURRENTLY` for exactly this kind of + risk; there is no such escape on Redshift, so the header says plainly that these + three should be scheduled for a maintenance window rather than run ad hoc, and + names `ADV104` (`VACUUM`/`ANALYZE`) as the one statement in this file that is *not* + a rewrite. Each individual proposal's own `note` (see `propose_sortkey`, + `propose_distkey`, `propose_diststyle_all`, `propose_maintenance`) repeats the + specific cost right above its own statement — the header exists for the reader who + only skims the top of a long file before scrolling. + + **ADV105 is attributed here, in the file itself, not only in the report.** Its DDL + is Amazon Redshift Advisor's own recommendation, verbatim — never sqlquality's + inference — so every ADV105 statement's header line below carries an explicit + `(Amazon Redshift Advisor — not sqlquality)` marker in addition to its own `note` + (already worded to say the same thing), so a reader skimming only the one-line + headers, not every note, still cannot mistake an Advisor statement for one this + tool generated. + + Reuses rather than reimplements: `cost_share_of` (bool-safe cost-share formatting) + and `_is_fully_commented`'s line-break guard, both imported from `models.py` and + `postgres.py` respectively — the same protection that guarantees a hostile + identifier (one carrying an embedded newline, `\\r`, or a `--`/`;` sequence) cannot + produce a bare, executable-looking line in this script either. See + `test_render_ddl_never_emits_a_bare_uncommented_line` in + `tests/test_workload_redshift_rules.py`. + """ + header = [ + "-- Generated by `sqlquality advise` — REVIEW BEFORE RUNNING.", + "-- sqlquality does not execute this script and has not validated it against", + "-- your workload's write patterns. Each statement is advisory.", + "--", + "-- DANGER: ADV101 (ALTER SORTKEY), ADV102 (ALTER DISTKEY) and ADV103 (ALTER", + "-- DISTSTYLE ALL) below EACH REWRITE THE ENTIRE TABLE. Redshift copies every", + "-- row, holds a lock on the table for the whole rewrite, and needs roughly", + "-- the table's own size again in free disk space while it runs — on a large", + "-- table that is HOURS, not seconds. There is NO CONCURRENTLY EQUIVALENT on", + "-- Redshift: unlike a Postgres index, none of these three can be built or", + "-- applied alongside normal traffic. SCHEDULE each one for a maintenance", + "-- window; do not run it ad hoc.", + "--", + "-- ADV104 (VACUUM / ANALYZE) is the ONLY statement below that is not a full", + "-- table rewrite: no exclusive lock for its duration, no full-table copy —", + "-- safe to run without scheduling a maintenance window.", + "--", + "-- ADV105 statements are Amazon Redshift Advisor's OWN recommendations, not", + "-- sqlquality's — each one is marked below and its own note repeats this.", + "", + ] + body: list[str] = [] + for proposal in proposals: + if not proposal.ddl: + continue + if ("\n" in proposal.ddl or "\r" in proposal.ddl) and not _is_fully_commented( + proposal.ddl + ): + # Identical guard to `PostgresWorkloadAdapter.render_ddl`: an identifier + # carrying a literal line break is already semantically safe once quoted + # (the whole thing parses as one identifier), but a raw newline would still + # leave a bare-looking line in a file meant to be safe to skim. Comment the + # whole thing out instead of guessing at a truncation. + 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 + share = cost_share_of(proposal.evidence) + share_text = f", {share:.1%} of workload cost" if share is not None else "" + # ADV105's DDL is Redshift Advisor's, never sqlquality's own — see this + # method's own docstring. Marked in the one-line header every statement gets, + # not only in `note` below, so a reader skimming headers alone still sees it. + attribution = ( + " (Amazon Redshift Advisor — not sqlquality)" if proposal.code == "ADV105" else "" + ) + body.append( + f"-- {proposal.code} [{proposal.confidence.value}{share_text}]{attribution}" + ) + 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` never reaches this + # file at all. See `PostgresWorkloadAdapter.render_ddl` for the same discipline. + if proposal.note: + body.extend(_comment_lines(proposal.note)) + body.append(proposal.ddl) + body.append("") + if not body: + body = ["-- No DDL proposals — every finding is advisory-only.", ""] + return "\n".join(header + body) diff --git a/src/sqlquality/workload/session.py b/src/sqlquality/workload/session.py new file mode 100644 index 0000000..bde51c7 --- /dev/null +++ b/src/sqlquality/workload/session.py @@ -0,0 +1,179 @@ +"""Shared psycopg session setup for wire-protocol adapters (Postgres, Redshift). + +Both engines speak libpq through psycopg, so opening a session — importing the driver +with an install hint, building conninfo inside the scrubbing envelope, arming a +statement timeout, and handing back a `Querier` — is exactly the same work for both. +Extracted here so there is one place to audit rather than two that can silently drift, +the same reasoning that consolidated credential scrubbing itself into `secrets.py` — +see that module's own docstring. + +The one thing that is *not* shared is what "read-only" means to each engine. Postgres's +`SET default_transaction_read_only = on` is expected to succeed unconditionally, so its +refusal aborts the whole connection like any other setup failure. Redshift refuses that +same statement in some configurations, and there a refusal degrades the session instead +of aborting it — see `open_session`'s `read_only_required` parameter and `redshift.py`'s +`connect()` for why silently continuing as though it had succeeded is not an option. +""" + +from __future__ import annotations + +from collections.abc import Callable, Mapping, Sequence +from typing import Any + +from sqlquality.workload.base import Querier +from sqlquality.workload.secrets import clamp_timeout_ms, scrub + +#: The statement establishing read-only intent for a libpq-speaking session. Available +#: unconditionally on Postgres; Redshift refuses it in some configurations — see +#: `open_session`'s `read_only_required` parameter for how each engine treats a refusal. +READ_ONLY_SQL = "SET default_transaction_read_only = on" + +#: dbt profiles.yml field names -> libpq connection keywords, shared by every +#: libpq-speaking adapter. One definition, not one per adapter: Postgres's and +#: Redshift's dbt profiles use the identical field names for the keywords that matter +#: here, and two copies of this table is exactly the kind of credential-handling drift +#: this module's own docstring exists to prevent. If an engine ever needs a genuinely +#: different mapping, that is a reason to pass a different table into +#: `translate_libpq_fields`/`dropped_libpq_fields`, not to fork this one. +LIBPQ_FIELD_MAP = { + "dbname": "dbname", + "database": "dbname", + "host": "host", + "port": "port", + "user": "user", + "username": "user", + "password": "password", +} +#: profiles.yml keys forwarded to libpq unchanged, because the name already *is* the +#: libpq keyword. The TLS group is here for a security reason, not a completeness one: a +#: profile saying `sslmode: verify-full` that silently connects under libpq's default +#: `prefer` performs no certificate verification at all, and the user is never told. For +#: a tool pitched as safe to point at production that is the wrong way to fail. +LIBPQ_PASSTHROUGH_FIELDS = frozenset( + {"sslmode", "sslcert", "sslkey", "sslrootcert", "connect_timeout"} +) + + +def import_psycopg(engine_label: str, extra: str) -> Any: + """Import psycopg, or raise an ImportError naming the extra to install. + + Every caller imports inside its own `connect()`, never at module scope: psycopg is + an optional extra, and a module-scope import here would break the `no-extras` CI + job, which never installs it. + """ + try: + import psycopg + except ImportError as exc: + raise ImportError( + f"{engine_label} support requires psycopg. " + f"Install it with: pip install 'sqlquality[{extra}]'" + ) from exc + return psycopg + + +def translate_libpq_fields( + fields: Mapping[str, str], field_map: Mapping[str, str], passthrough: frozenset[str] +) -> dict[str, str]: + """Translate profiles.yml keys to libpq keywords, dropping anything unrecognized.""" + translated = {field_map[k]: v for k, v in fields.items() if k in field_map} + translated.update({k: v for k, v in fields.items() if k in passthrough}) + return translated + + +def dropped_libpq_fields( + fields: Mapping[str, str], field_map: Mapping[str, str], passthrough: frozenset[str] +) -> tuple[str, ...]: + """profiles.yml keys this adapter cannot forward, by name. + + Names only, never values: one of them could be a secret (`sslpassword`), and this + text goes to stderr and from there into CI logs. + """ + return tuple(sorted(k for k in fields if k not in field_map and k not in passthrough)) + + +def open_session( + *, + psycopg: Any, + conninfo_factory: Callable[[], str], + secrets: Sequence[str], + timeout_s: int, + min_timeout_s: int, + max_timeout_s: int, + read_only_sql: str, + read_only_required: bool, +) -> tuple[Querier, str | None]: + """Connect, arm read-only intent and a statement timeout, and hand back a Querier. + + ``min_timeout_s``/``max_timeout_s`` are parameters rather than a module-level import + of `workload.base`'s constants, so each caller passes (and therefore keeps using, + rather than merely importing) the one shared pair `base.py` defines — the same + "defined once so two layers cannot drift apart" reasoning `base.py` gives for their + existence at all. + + ``conninfo_factory`` is called *inside* the scrubbing envelope deliberately: + building a conninfo from caller-supplied fields (``psycopg.conninfo.make_conninfo``) + can itself raise, and that message can quote the offending value — for a `password` + keyword, the password. Calling it here rather than before this function is what + keeps that failure covered. + + ``read_only_required`` decides what a failed ``read_only_sql`` means: + + - Required (Postgres): the statement always succeeds, so a failure is + indistinguishable from any other setup failure and aborts the connection like one. + - Not required (Redshift): the statement is refused in some configurations, so its + failure is caught right here, scrubbed, and returned as a degradation message + instead of raised. The connection still proceeds — the adapter calling this only + ever issues SELECT statements regardless of whether this belt-and-braces guard + could be armed. + + On any other failure, ``ConnectionError`` is raised *after* this function's own + `except` clause has already exited — not from inside it — so the driver's original + exception can never survive as `__context__`. That severance is deliberate: an + unscrubbed driver exception reachable via `__context__` would defeat the whole + scrubbing exercise the moment anything printed a traceback. + """ + failure: str | None = None + degradation: str | None = None + connection: Any = None + try: + conninfo = conninfo_factory() + connection = psycopg.connect(conninfo, autocommit=True) + with connection.cursor() as cursor: + if read_only_required: + # Belt and braces: the session cannot write even if a statement tried + # to. A failure here is a setup failure like any other and is handled + # by the broad `except` below. + cursor.execute(read_only_sql) + else: + try: + cursor.execute(read_only_sql) + except Exception as exc: # driver-specific; only the message matters + degradation = scrub( + "the session could not be proven read-only: the server refused " + f"{read_only_sql!r} ({exc}). Belt-and-braces read-only could not " + "be applied. This does not mean the session might write — this " + "adapter only ever issues the SELECT statements in its own " + "introspection SQL — but this extra safeguard could not be armed.", + secrets, + ) + # set_config() rather than `SET`, because bind parameters are not accepted + # in a SET statement and string-building one with a caller-controlled value + # is the wrong habit to establish in the one place we talk to a database. + cursor.execute( + "SELECT set_config('statement_timeout', %s, false)", + (f"{clamp_timeout_ms(timeout_s, minimum=min_timeout_s, maximum=max_timeout_s)}ms",), + ) + except Exception as exc: + failure = scrub(str(exc), secrets) + if failure is not None: + # Raised after the handler, and scrubbed: see this function's own docstring — + # leaving the handler is the only way to keep the original exception out of + # __context__. No "Could not connect" prefix here; the CLI adds it. + raise ConnectionError(failure) + + def query(sql: str, bind: tuple[object, ...]) -> list[tuple[object, ...]]: + with connection.cursor() as cur: + cur.execute(sql, bind) + return list(cur.fetchall()) + + return query, degradation diff --git a/tests/integration/test_redshift_connect_live.py b/tests/integration/test_redshift_connect_live.py new file mode 100644 index 0000000..bba6083 --- /dev/null +++ b/tests/integration/test_redshift_connect_live.py @@ -0,0 +1,103 @@ +"""Redshift's connect(), exercised against a real server. + +Redshift speaks the PostgreSQL wire protocol, so `connect()` — the session setup, the +clamped statement timeout, and secret scrubbing on a bad password — is genuinely +testable against the same `postgres:16` container the rest of this package uses. The +catalog SQL (`svv_*`/`sys_*`) is not: those views do not exist in Postgres, so nothing +here calls `fetch_workload`, `fetch_schema`, `fetch_table_facts`, or `propose`. +""" + +from __future__ import annotations + +from urllib.parse import urlsplit, urlunsplit + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.base import MAX_TIMEOUT_S +from sqlquality.workload.redshift import RedshiftWorkloadAdapter + + +def _with_wrong_password(dsn: str) -> str: + """Swap whatever password a DSN carries for a wrong one, however it is shaped. + + A DSN-shaped string substitution (``dsn.replace(":sqlquality@", ":wr0ng-p4ss@")``) is + a silent no-op whenever the real password is not literally ``sqlquality`` — exactly + the normal case here: host port 55432 is frequently held by an unrelated container on + this machine, so this suite is routinely run against a custom + ``SQLQUALITY_TEST_DSN`` with different credentials. Parsing the DSN's authority + component and re-encoding it with a substituted password works for whatever DSN is + handed in, not only the one hardcoded default. + """ + parts = urlsplit(dsn) + user = parts.username or "" + host = parts.hostname or "" + port = f":{parts.port}" if parts.port is not None else "" + userinfo = f"{user}:wr0ng-p4ss@" if user else "wr0ng-p4ss@" + netloc = f"{userinfo}{host}{port}" + return urlunsplit((parts.scheme, netloc, parts.path, parts.query, parts.fragment)) + + +@pytest.mark.integration +def test_redshift_adapter_connects_over_the_postgres_wire_protocol(live_dsn): + """Redshift speaks the PostgreSQL protocol, so the session setup is genuinely testable. + + The catalog statements are not — `svv_*` does not exist here — so this test deliberately + covers connect() only, and asserts nothing about introspection. + """ + adapter = RedshiftWorkloadAdapter() + params = ConnectionParams(engine="redshift", dsn=live_dsn, fields={}, source="test") + adapter.connect(params, timeout_s=30) + assert adapter._query is not None + rows = adapter._query("SELECT 1", ()) + assert rows == [(1,)] + + +@pytest.mark.integration +def test_a_wrong_password_leaks_nothing(live_dsn): + adapter = RedshiftWorkloadAdapter() + bad = _with_wrong_password(live_dsn) + assert bad != live_dsn, "the password substitution must actually change the DSN" + params = ConnectionParams(engine="redshift", dsn=bad, fields={}, source="test") + with pytest.raises(ConnectionError) as exc: + adapter.connect(params, timeout_s=5) + assert "wr0ng-p4ss" not in str(exc.value) + assert "wr0ng-p4ss" not in repr(exc.value) + + +@pytest.mark.integration +def test_the_clamped_timeout_actually_reaches_the_server(live_dsn): + """The unit test (`test_an_out_of_range_timeout_is_clamped_before_it_reaches_the_ + session` in `tests/test_workload_redshift.py`) only proves the clamped value is *sent* + to a fake driver's `execute()`. This proves the server actually *applies* it: an + out-of-range `timeout_s` must still leave `statement_timeout` set to the clamped + `MAX_TIMEOUT_S`, not to whatever out-of-range value was requested, on a connection that + genuinely round-trips through Postgres's wire protocol. + """ + adapter = RedshiftWorkloadAdapter() + params = ConnectionParams(engine="redshift", dsn=live_dsn, fields={}, source="test") + adapter.connect(params, timeout_s=99_999) + assert adapter._query is not None + # Postgres reformats a round-number-of-seconds duration into the largest whole unit + # that expresses it exactly — 3600s displays as "1h", not "3600s" — so this asserts on + # the semantic value via `pg_catalog`'s own extraction rather than the display string + # `SHOW` returns. + rows = adapter._query( + "SELECT extract(epoch FROM current_setting('statement_timeout')::interval)", () + ) + assert rows == [(float(MAX_TIMEOUT_S),)] + + +@pytest.mark.integration +def test_a_real_postgres_accepts_the_read_only_statement_without_degradation(live_dsn): + """Against the real `postgres:16` container `SET default_transaction_read_only` always + succeeds, so the belt-and-braces guard reports no degradation. The refusal path itself + (`test_a_refused_read_only_statement_degrades_rather_than_aborts` in + `tests/test_workload_redshift.py`) cannot be exercised live — Postgres always accepts + this statement — so it is covered at unit level with a fake driver that refuses it, + which is the situation this adapter is actually built to handle on a real cluster. + """ + adapter = RedshiftWorkloadAdapter() + params = ConnectionParams(engine="redshift", dsn=live_dsn, fields={}, source="test") + adapter.connect(params, timeout_s=30) + assert adapter.degraded == [] diff --git a/tests/integration/test_redshift_introspection_bindable_live.py b/tests/integration/test_redshift_introspection_bindable_live.py new file mode 100644 index 0000000..aae5f89 --- /dev/null +++ b/tests/integration/test_redshift_introspection_bindable_live.py @@ -0,0 +1,132 @@ +"""Prove every Redshift introspection statement is *bindable*, not merely syntax-checked. + +The unit suite only parses these statements with sqlglot, which can catch a malformed +statement but not a parameter the driver cannot bind — and whether a statement's parameters +can be bound at all is the driver's job, not Redshift's. A bind failure happens identically +whether the FROM clause names `svv_table_info` or `pg_class`. + +**Why this creates throwaway stand-in tables instead of just running the real statements.** +The first version of this file ran each statement as-is against `postgres:16`, on the +theory that the expected, accepted failure would be `UndefinedTable` (the view genuinely +does not exist here) with a parameter-binding failure (`IndeterminateDatatype`) being the +one thing that must not happen instead. Verified empirically that this does not +discriminate anything: Postgres's analyzer resolves table references before parameter +types, so a statement whose FROM-clause table does not exist *always* fails with +`UndefinedTable`, regardless of whether its parameters would otherwise bind — reintroducing +the exact bug this file exists to catch (a bare `(%s IS NULL OR start_time >= %s)`) still +produced only `UndefinedTable` against a real, missing `sys_query_history`. So this instead +creates a same-named, same-shaped (but empty) real table for each capability, which lets +Postgres's analyzer get past the FROM clause and actually resolve every parameter's type — +at which point the *bindable* form succeeds outright (no exception, zero rows) and the +*unbindable* form still fails with `IndeterminateDatatype`, exactly as it does for a +freshly-loaded relation on a real Redshift cluster. See `CAP_WORKLOAD`'s comment in +`redshift.py` for the production bug this reproduces and fixes. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from datetime import datetime, timezone + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.redshift import ( + CAP_ADVISOR, + CAP_SCHEMA, + CAP_TABLE_FACTS, + CAP_WORKLOAD, + RedshiftWorkloadAdapter, +) + +#: One throwaway real table per capability's FROM clause, shaped to match every column +#: each statement selects or filters on — just enough for Postgres's analyzer to resolve +#: the statement fully, including its parameters. Types are the obvious Postgres +#: equivalent of AWS's documented Redshift column types; the *rows* are irrelevant (every +#: table stays empty), only whether the statement can be prepared and run against them. +_SHIM_DDL = { + "sys_query_history": """ + CREATE TABLE sys_query_history ( + query_text text, elapsed_time bigint, database_name text, + status text, start_time timestamptz + ) + """, + "svv_columns": """ + CREATE TABLE svv_columns ( + schema_name text, table_name text, column_name text, data_type text + ) + """, + "svv_table_info": """ + CREATE TABLE svv_table_info ( + "schema" text, "table" text, tbl_rows bigint, size bigint, + unsorted float8, stats_off float8, diststyle text, sortkey1 text, + skew_rows float8 + ) + """, + "svv_alter_table_recommendations": """ + CREATE TABLE svv_alter_table_recommendations ( + database_name text, schema_name text, table_name text, type text, + current_ddl text, recommended_ddl text + ) + """, +} + +#: Representative binds for each capability, matching what its own fetch_* method passes. +#: CAP_WORKLOAD gets two entries — with and without a `--since` cutoff — since those are +#: two different parameter shapes over the wire and the bug this file guards against only +#: reproduced in the no-`--since` (both-NULL) shape. +_BINDS: dict[str, list[tuple[object, ...]]] = { + CAP_WORKLOAD: [ + (None, None, 10), + (datetime.now(timezone.utc), datetime.now(timezone.utc), 10), + ], + CAP_SCHEMA: [(["public"],)], + CAP_TABLE_FACTS: [(["public"], ["orders"])], + CAP_ADVISOR: [(["public"], ["orders"])], +} + + +@pytest.fixture(scope="module") +def shim_tables(live_dsn: str) -> Iterator[None]: + """Create, then drop, one throwaway real table per statement's FROM clause. See the + module docstring for why a real table (rather than the statement as-is against a + genuinely missing relation) is required to make this check discriminating at all. + """ + import psycopg + + with psycopg.connect(live_dsn, autocommit=True) as conn: + with conn.cursor() as cur: + for name, ddl in _SHIM_DDL.items(): + cur.execute(f"DROP TABLE IF EXISTS {name}") + cur.execute(ddl) + try: + yield + finally: + with psycopg.connect(live_dsn, autocommit=True) as conn: + with conn.cursor() as cur: + for name in _SHIM_DDL: + cur.execute(f"DROP TABLE IF EXISTS {name}") + + +@pytest.fixture +def adapter(live_dsn: str, shim_tables: None) -> RedshiftWorkloadAdapter: + a = RedshiftWorkloadAdapter() + a.connect(ConnectionParams(engine="redshift", dsn=live_dsn, fields={}, source="--dsn"), 30) + return a + + +@pytest.mark.parametrize("capability", sorted(_BINDS)) +def test_every_statement_binds_its_parameters(adapter: RedshiftWorkloadAdapter, capability: str): + """Every one of this adapter's four statements must actually run, with representative + binds, against a table shaped like the view it targets — proving its parameters bind + over the wire, which is the driver's job and has nothing to do with which system view + sits behind the FROM clause. + + Calls `adapter._query` directly, bypassing `_run`'s try/except: this test needs to see + a real bind failure if one occurs, not have it swallowed into `degraded` the way a + production run correctly does. `connect(autocommit=True)` (see `session.py`) means a + failed statement does not abort a shared transaction, so every bind in the list runs + independently on the same connection. + """ + for params in _BINDS[capability]: + adapter._query(RedshiftWorkloadAdapter.SQL[capability], params) diff --git a/tests/test_advise_cli.py b/tests/test_advise_cli.py index ec90630..5a1bc42 100644 --- a/tests/test_advise_cli.py +++ b/tests/test_advise_cli.py @@ -1456,3 +1456,156 @@ def test_the_no_manifest_run_contains_no_dbt_conditional_element_anywhere(monkey 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 + + +def test_redshift_read_only_degradation_reaches_stderr_end_to_end(monkeypatch): + """The carried-forward item from Tasks 2-4: `connect()`'s read-only degradation was + recorded correctly from the start, but could never reach a user because `propose()` + raised `NotImplementedError` before `cli.py` ever got to the loop that prints + `adapter.degraded` to stderr — see `RedshiftWorkloadAdapter.propose`'s docstring and + `tests/test_workload_redshift.py`'s `test_the_read_only_degradation_survives_past_ + fetch_workload`, which proved only that `fetch_workload` no longer crashed, and said so + explicitly rather than claiming the full run worked. + + Now that ADV101-105 make `propose()` a real method, a full `advise` run against a + Redshift adapter whose `connect()` recorded a read-only degradation must complete and + print it — this is the first test that exercises `cli.py` end to end for that engine + rather than stopping at `fetch_workload`. + """ + from sqlquality.workload.redshift import DEGRADATION_READ_ONLY, RedshiftWorkloadAdapter + + def fake_connect(self, params, timeout_s): + def query(sql, bind): + return [] + + self._query = query + self.degraded.append( + ( + DEGRADATION_READ_ONLY, + "the session could not be proven read-only (belt-and-braces guard refused) — ***", + ) + ) + + monkeypatch.setattr(RedshiftWorkloadAdapter, "connect", fake_connect) + result = runner.invoke(app, ["advise", "--engine", "redshift", "--dsn", "postgresql://u@h/db"]) + assert result.exit_code == 0, result.output + assert f"reduced coverage — {DEGRADATION_READ_ONLY}:" in result.stderr + assert "could not be proven read-only" in result.stderr + + +def test_redshift_advise_run_with_no_degradation_prints_none(monkeypatch): + """Guards the test above: a clean `connect()` must not print a `reduced coverage` line + at all, so the assertion above is attributable to the recorded degradation, not to + `cli.py` always printing something regardless of `adapter.degraded`'s contents.""" + from sqlquality.workload.redshift import RedshiftWorkloadAdapter + + def fake_connect(self, params, timeout_s): + self._query = lambda sql, bind: [] + + monkeypatch.setattr(RedshiftWorkloadAdapter, "connect", fake_connect) + result = runner.invoke(app, ["advise", "--engine", "redshift", "--dsn", "postgresql://u@h/db"]) + assert result.exit_code == 0, result.output + assert "reduced coverage" not in result.stderr + + +def _redshift_dbt_run(monkeypatch, *, extra_args=()): + """A full `advise --engine redshift` run over a fake querier, on the dbt fixture's + `main.orders` (a `table`-materialized model), returning the CliRunner result. + + The rows are shaped for the real statements: one hot range predicate on + `main.orders.created_at` so ADV101 fires, and `svv_table_info` facts saying the table is + sorted on a *different* column so the proposal is not suppressed. + """ + from sqlquality.workload.redshift import ( + CAP_ADVISOR, + CAP_SCHEMA, + CAP_TABLE_FACTS, + CAP_WORKLOAD, + RedshiftWorkloadAdapter, + ) + + rows = { + CAP_WORKLOAD: [("select id from main.orders where created_at > '2026-01-01'", 5_000_000)], + CAP_SCHEMA: [ + ("main", "orders", "id", "integer"), + ("main", "orders", "created_at", "timestamp"), + ], + CAP_TABLE_FACTS: [("main", "orders", 10_000, 50, 0.0, 0.0, "EVEN", "id", 0.0)], + CAP_ADVISOR: [], + } + + def fake_connect(self, params, timeout_s): + def query(sql, bind): + for capability, canned in rows.items(): + if RedshiftWorkloadAdapter.SQL[capability] in sql: + return canned + return [] + + self._query = query + + monkeypatch.setattr(RedshiftWorkloadAdapter, "connect", fake_connect) + return runner.invoke( + app, + [ + "advise", + "--engine", + "redshift", + "--dsn", + "postgresql://u@h/db", + "--schema", + "main", + *extra_args, + ], + ) + + +def test_redshift_dbt_enrichment_is_disclosed_in_the_terminal_end_to_end(monkeypatch): + """dbt enrichment must not be invisible to a terminal-only user on Redshift. + + `describe_rewrites` counted only ADV302's config-block rewrite, and no Redshift proposal + can reach that path — nothing this adapter emits is a `CREATE INDEX` — so the stderr line + never appeared: the terminal row for an enriched ADV101 was byte-identical to the same + proposal from a dbt-free run (same code, same confidence, same cost share, same title), + while the warning that `dbt run` will undo an hours-long full-table rewrite sat in + `rationale` and in the `--ddl` note, neither of which the terminal prints. This pins the + whole chain end to end — the evidence flag, the count, and `cli.py`'s echo — rather than + only the counting function, because each link in it has been broken separately before. + """ + result = _redshift_dbt_run(monkeypatch, extra_args=("--manifest", str(DBT_FIXTURE))) + assert result.exit_code == 0, result.output + assert "ADV101" in result.stdout, "a vacuous run would satisfy the assertion below" + assert "cannot be expressed as dbt config" in result.stderr + assert "may undo it" in result.stderr + + +def test_redshift_run_without_a_manifest_says_nothing_about_dbt(monkeypatch): + """Control for the test above: the same run with no manifest must print no enrichment + line at all, so the disclosure is attributable to dbt enrichment rather than to + `cli.py` always printing something.""" + result = _redshift_dbt_run(monkeypatch) + assert result.exit_code == 0, result.output + assert "ADV101" in result.stdout + assert "dbt" not in result.stderr + assert "cannot be expressed" not in result.stderr + + +def test_redshift_dry_run_prints_all_four_statements_and_never_connects(monkeypatch): + """Task 8's proof that a user can inspect Redshift's introspection SQL — every column + name unverified against a live cluster (see `redshift.py`'s module docstring) — before + trusting it with a real connection. Mirrors `test_dry_run_prints_statements_and_never_ + connects` above, for the engine whose SQL genuinely needs this escape hatch most. + """ + + def explode(*args, **kwargs): + raise AssertionError("--dry-run must not connect") + + monkeypatch.setattr("sqlquality.workload.redshift.RedshiftWorkloadAdapter.connect", explode) + result = runner.invoke(app, ["advise", "--engine", "redshift", "--dry-run"]) + assert result.exit_code == 0, result.output + for marker in ("sys_query_history", "svv_columns", "svv_table_info", "svv_alter_table"): + assert marker in result.stdout + + +def test_redshift_dry_run_needs_no_credentials(): + result = runner.invoke(app, ["advise", "--engine", "redshift", "--dry-run"]) + assert result.exit_code == 0, result.output diff --git a/tests/test_workload_dbt.py b/tests/test_workload_dbt.py index 44f5152..e888250 100644 --- a/tests/test_workload_dbt.py +++ b/tests/test_workload_dbt.py @@ -1294,6 +1294,91 @@ def test_describe_rewrites_reports_both_rewritten_and_folded_proposals(): assert "\n" not in line, "one stderr line" +def _non_index_proposal(relation, *, code="ADV101", note=None): + """A proposal whose DDL is neither `CREATE INDEX` nor `DROP INDEX` — the shape every + Redshift rule emits (`ALTER TABLE ... ALTER SORTKEY`, `VACUUM`, Advisor's own DDL), and + the only shape that reaches `_classify`'s generic path.""" + return Proposal( + code=code, + title=f"Consider SORTKEY on {relation}(created_at)", + rationale="hot range predicate.", + evidence={"schema": relation.schema, "table": relation.table, "cost_share": 0.5}, + confidence=Confidence.MEDIUM, + ddl=f'ALTER TABLE "{relation.schema}"."{relation.table}" ALTER SORTKEY ("created_at");', + note=note, + ) + + +def test_describe_rewrites_reports_a_statement_that_cannot_be_expressed_as_dbt_config(): + """Counting only ADV302's config-block rewrite made this function return `None` for every + Redshift run — nothing Redshift emits is a `CREATE INDEX`, so all of ADV101-105 take + `_classify`'s generic path instead. Enrichment fired, the warning went into `rationale` + and `note`, and the terminal row stayed byte-identical to a dbt-free run: exactly the + failure mode this line exists to prevent, on the engine where the undone work is hours + of full-table rewrite rather than seconds of index build. + """ + context = DbtContext.from_project(_project()) + out = enrich_proposals([_non_index_proposal(Relation("main", "orders"))], context) + line = describe_rewrites(out) + assert line is not None + assert "1 proposal(s)" in line + assert "cannot be expressed as dbt config" in line + assert "next `dbt run` may undo it" in line + assert "\n" not in line, "one stderr line" + + +def test_describe_rewrites_is_still_silent_for_an_unmanaged_non_index_proposal(): + """Control for the test above: the new count must depend on `DbtContext.model_for` + actually matching, not merely on a proposal whose DDL is not an index.""" + context = DbtContext.from_project(_project()) + out = enrich_proposals([_non_index_proposal(Relation("public", "unmanaged"))], context) + assert describe_rewrites(out) is None + + +def test_describe_rewrites_reports_both_kinds_in_one_line(): + """A mixed run (a Postgres index proposal and a non-index one for the same dbt model) + must disclose both, since they call for different actions: one DDL is config to paste, + the other is runnable SQL that may not survive the next rebuild.""" + context = DbtContext.from_project(_project()) + relation = Relation("main", "orders") + out = enrich_proposals([_index_proposal(relation), _non_index_proposal(relation)], context) + line = describe_rewrites(out) + assert line is not None + assert "ADV302 expressed 1 index proposal(s)" in line + assert "1 proposal(s) target a dbt-managed relation" in line + + +def test_prepend_note_keeps_the_existing_note_first_and_emits_the_dbt_note_once(): + """Order and non-duplication, neither of which was pinned: losing the existing note was + caught by two tests, but swapping the concatenation order and emitting the dbt warning + twice both left the whole suite green. The existing note is the proposal's own statement + about its own DDL — ADV105's "this DDL came from Redshift, not sqlquality" — and the dbt + warning is a caveat on it, so it belongs after, once. + """ + context = DbtContext.from_project(_project()) + existing = "Source: Amazon Redshift Advisor, not sqlquality's own analysis." + (out,) = enrich_proposals( + [_non_index_proposal(Relation("main", "orders"), note=existing)], context + ) + note = out.note or "" + assert note.startswith(existing) + assert "dbt WARNING" in note + assert note.index(existing) < note.index("dbt WARNING") + assert note.count("dbt WARNING") == 1 + + +def test_prepend_note_is_idempotent(): + """Enriching an already-enriched proposal must not stack a second copy of the same dbt + warning onto its note. Not reachable from `cli.py`, which enriches once — pinned because + the fix's own name and docstring claim it cannot duplicate a note, and because a note + that grows on every pass is the kind of thing only a test notices.""" + context = DbtContext.from_project(_project()) + (once,) = enrich_proposals([_non_index_proposal(Relation("main", "orders"))], context) + (twice,) = enrich_proposals([once], context) + assert twice.note == once.note + assert (twice.note or "").count("dbt WARNING") == 1 + + 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 diff --git a/tests/test_workload_postgres.py b/tests/test_workload_postgres.py index ef11185..d46b8c5 100644 --- a/tests/test_workload_postgres.py +++ b/tests/test_workload_postgres.py @@ -229,6 +229,29 @@ def _canned(rows_by_capability): ) +@pytest.mark.parametrize("capability", sorted(EXPECTED_CAPABILITIES)) +def test_a_denied_capability_records_its_privilege_hint_in_degraded(capability): + """The recorded degradation must carry the capability's privilege hint, not only the + driver's message. + + These hints are what someone hands their DBA — the `pg_stat_statements` and + `pg_read_all_stats` prerequisites are useless if the one place a denial is reported + omits them. Dropping the hint from the message left the whole suite green on both + adapters, so the mitigation could be disconnected in silence. The expected text is read + back out of `introspection_sql()` rather than duplicated here, so this cannot drift + from the hint a `--dry-run` prints. + """ + hints = {s.capability: s.privilege_hint for s in PostgresWorkloadAdapter().introspection_sql()} + marker = PostgresWorkloadAdapter.SQL[capability] + adapter = PostgresWorkloadAdapter(querier=FakeQuerier({}, fail_markers=(marker,))) + adapter._run(capability, ()) + assert len(adapter.degraded) == 1 + recorded_capability, reason = adapter.degraded[0] + assert recorded_capability == capability + assert "permission denied" in reason, "the driver's own message must survive too" + assert hints[capability] in reason + + def test_fetch_workload_maps_rows_and_reports_the_window(): querier = FakeQuerier( { @@ -785,6 +808,12 @@ def test_profile_tls_settings_are_forwarded_to_the_driver(monkeypatch): dsn=None, fields={ "host": "db", + # `database` is dbt's spelling and must be translated to libpq's `dbname`. + # Passing it here rather than `dbname` covers the aliasing half of the map: with + # neither key present, scrambling `dbname`/`database` in the shared + # `LIBPQ_FIELD_MAP` left every Postgres test green and only Redshift's noticed, + # which put single-adapter coverage on a credential-path table both engines share. + "database": "mydb", "user": "hans", "password": "hunter2", "sslmode": "verify-full", @@ -797,6 +826,8 @@ def test_profile_tls_settings_are_forwarded_to_the_driver(monkeypatch): ) PostgresWorkloadAdapter().connect(params, 30) conninfo = seen["conninfo"] + assert "dbname=mydb" in conninfo + assert "database=" not in conninfo, "dbt's `database` must be translated, not forwarded" assert "sslmode=verify-full" in conninfo assert "sslrootcert=/etc/ssl/ca.crt" in conninfo assert "sslcert=/etc/ssl/client.crt" in conninfo diff --git a/tests/test_workload_redshift.py b/tests/test_workload_redshift.py new file mode 100644 index 0000000..ddaf2a3 --- /dev/null +++ b/tests/test_workload_redshift.py @@ -0,0 +1,1101 @@ +import re +import sys +import types +from datetime import timedelta +from pathlib import Path + +import sqlglot +import pytest + +from sqlquality.models import ConnectionParams, Relation +from sqlquality.workload import get_workload_adapter +from sqlquality.workload.base import MAX_TIMEOUT_S +from sqlquality.workload.fingerprint import ingest +from sqlquality.workload.redshift import ( + CAP_ADVISOR, + CAP_SCHEMA, + CAP_TABLE_FACTS, + CAP_WORKLOAD, + DEGRADATION_READ_ONLY, + RedshiftWorkloadAdapter, +) +from sqlquality.workload.session import READ_ONLY_SQL + +EXPECTED_CAPABILITIES = {CAP_WORKLOAD, CAP_SCHEMA, CAP_TABLE_FACTS, CAP_ADVISOR} + + +def test_registry_returns_the_redshift_adapter(): + adapter = get_workload_adapter("redshift") + assert adapter.engine == "redshift" + + +def test_every_capability_has_a_statement_and_a_hint(): + statements = RedshiftWorkloadAdapter().introspection_sql() + assert {s.capability for s in statements} == EXPECTED_CAPABILITIES + for statement in statements: + assert statement.sql.strip() + assert statement.privilege_hint.strip() + + +@pytest.mark.parametrize("capability", sorted(EXPECTED_CAPABILITIES)) +def test_every_statement_parses_as_redshift_sql(capability): + """Syntax validation is the one correctness check available without a cluster. + + The catalog SQL in this adapter cannot be executed during development — there is no + Redshift container and `svv_*`/`sys_*` do not exist in Postgres. Parsing each statement + with sqlglot's redshift dialect cannot catch a wrong column name, but it catches a + malformed statement, which would otherwise be invisible until a user ran it. + """ + sql = RedshiftWorkloadAdapter.SQL[capability] + # `%s` placeholders are libpq's, not SQL — sqlglot cannot parse them, so they become + # bind markers for the purposes of this check. + parsed = sqlglot.parse_one(sql.replace("%s", "?"), dialect="redshift") + assert parsed is not None + + +@pytest.mark.parametrize("capability", sorted(EXPECTED_CAPABILITIES)) +def test_no_statement_writes(capability): + """Same guard the Postgres adapter carries, for the same reason.""" + forbidden = ( + "insert", + "update", + "delete", + "create", + "drop", + "alter", + "truncate", + "grant", + "revoke", + "vacuum", + "analyze", + ) + lowered = RedshiftWorkloadAdapter.SQL[capability].lower() + found = {verb for verb in forbidden if re.search(rf"\b{verb}\b", lowered)} + assert not found, f"{capability} contains write verb(s): {sorted(found)}" + + +def _normalized(sql: str) -> str: + """`sql` with every run of whitespace collapsed, so a reformat of the statement cannot + break a predicate assertion that is really about the predicate.""" + return " ".join(sql.lower().split()) + + +def test_workload_statement_is_scoped_to_the_current_database(): + """The same guard `tests/test_workload_postgres.py` carries under this exact name, for + the same claim — it was not carried across when this engine was added, and deleting the + predicate left the whole suite green. + + Without the scope, an `advise` run against a shared cluster ingests *other databases'* + query history: their relation names are attributed to the connected database's schema + map, and proposals — including full-table rewrites — are generated for tables the + session was never pointed at. + """ + sql = _normalized(RedshiftWorkloadAdapter.SQL[CAP_WORKLOAD]) + assert "sys_query_history" in sql + assert "database_name = current_database()" in sql + + +def test_workload_statement_counts_only_successful_executions(): + """`sys_query_history` records failed and cancelled executions alongside successful + ones, and unlike `pg_stat_statements` it is per-execution rather than pre-aggregated. + Without this filter, work that never completed lands in the `cost_share` denominator + and dilutes every proposal's share — and an aborted statement's elapsed time measures + how long it ran before dying, not the cost of the query it was trying to be. + """ + sql = _normalized(RedshiftWorkloadAdapter.SQL[CAP_WORKLOAD]) + assert "status = 'success'" in sql + + +@pytest.mark.parametrize("capability", sorted(EXPECTED_CAPABILITIES)) +def test_a_denied_capability_records_its_privilege_hint_in_degraded(capability): + """The recorded degradation must carry the capability's privilege hint, not only the + driver's message. + + These four hint strings are this adapter's stated mitigation for column names it cannot + verify against a cluster ("a wrong name costs one capability, recorded in `degraded` + naming the statement"), and they are what someone hands their DBA. Dropping the hint + from the message left 904 tests green, so the mitigation could be disconnected in + silence. The expected text is read back out of `introspection_sql()` rather than + duplicated here, so this cannot drift from the hint a `--dry-run` prints. + """ + hints = {s.capability: s.privilege_hint for s in RedshiftWorkloadAdapter().introspection_sql()} + marker = RedshiftWorkloadAdapter.SQL[capability] + adapter = RedshiftWorkloadAdapter(querier=FakeQuerier({}, fail_markers=(marker,))) + adapter._run(capability, ()) + assert len(adapter.degraded) == 1 + recorded_capability, reason = adapter.degraded[0] + assert recorded_capability == capability + assert "permission denied" in reason, "the driver's own message must survive too" + assert hints[capability] in reason + + +def test_the_acquired_redshift_limitations_are_documented_for_users(): + """Three limitations this engine *acquires* must be documented where a user reads, not + only in a docstring or a privilege hint. + + The first is the sharp one: `_HINTS[CAP_WORKLOAD]` describes the `SYSLOG ACCESS + UNRESTRICTED` partial-workload trap precisely, and names it as the dangerous class + *because* it produces no error — but that string only ever reaches a user through + `--dry-run` or a failure, and this failure never happens. A hint that can only be + delivered by an event that cannot occur is not disclosure. The other two are the + per-execution meaning of `--limit` and the fingerprint split, whose siblings ("`cost_share` + is not a partition", the PL/pgSQL double-count) are already in README `## Limitations`. + Each claim is asserted separately, so documenting one and omitting another cannot pass. + """ + readme = (Path(__file__).resolve().parents[1] / "README.md").read_text(encoding="utf-8") + assert "SYSLOG ACCESS UNRESTRICTED" in readme + assert "only the connecting user's own queries" in readme + assert "counts executions on Redshift and query groups on Postgres" in readme + assert "Identifier case and attached comments can split one Redshift statement" in readme + + +def test_there_is_no_ndv_or_index_capability(): + """Redshift exposes no `pg_stats.n_distinct` equivalent and has no indexes. + + Declaring either capability would invite a rule to assume evidence that cannot exist. + """ + capabilities = {s.capability for s in RedshiftWorkloadAdapter().introspection_sql()} + assert not any("ndv" in c or "index" in c for c in capabilities) + + +# Every `WorkloadAdapter` method this feature once left deliberately unbuilt — including +# `render_ddl`, the last one — is now implemented. See git history for the +# `UNIMPLEMENTED`/`test_unimplemented_methods_say_so_rather_than_returning_empty` machinery +# this section used to carry: it named each unbuilt method individually (rather than +# discovering them by reflection) precisely so implementing the last one forced a visible, +# reviewable deletion here rather than an empty parametrisation quietly passing forever. + + +class FakeQuerier: + """Returns canned rows per capability, keyed by a distinctive SQL substring. + + Mirrors `tests/test_workload_postgres.py`'s `FakeQuerier` exactly — same dispatch, same + shape — so the two adapters' fetch tests read the same way. + """ + + def __init__(self, rows_by_marker, fail_markers=()): + self.rows_by_marker = rows_by_marker + self.fail_markers = fail_markers + self.calls = [] + + def __call__(self, sql, params): + self.calls.append((sql, params)) + for marker in self.fail_markers: + if marker in sql: + raise RuntimeError(f"permission denied for {marker}") + for marker, rows in self.rows_by_marker.items(): + if marker in sql: + return rows + return [] + + +def _canned(rows_by_capability): + """A FakeQuerier addressed by capability constant rather than a raw SQL substring.""" + return FakeQuerier( + { + RedshiftWorkloadAdapter.SQL[capability]: rows + for capability, rows in rows_by_capability.items() + } + ) + + +def test_fetch_workload_maps_rows_and_reports_no_filter_applied(): + querier = _canned({CAP_WORKLOAD: [("select id from orders where status = 'x'", 25_000)]}) + fetch = RedshiftWorkloadAdapter(querier=querier).fetch_workload(None, 500) + assert fetch.rows[0].sql == "select id from orders where status = 'x'" + # One row per execution, not pre-aggregated — see the aggregation test below. + assert fetch.rows[0].calls == 1 + # elapsed_time is documented in microseconds; total_time_ms wants milliseconds. + assert fetch.rows[0].total_time_ms == pytest.approx(25.0) + assert "no --since filter" in fetch.window_description + assert "sys_query_history" in fetch.window_description + # The statement is ORDER BY ... LIMIT n, so what was actually fetched is a truncated + # top-n, not "everything" — the window text must say so, not merely name the source + # view. See test_fetch_workload_window_names_the_truncation below for the full pin. + assert "500" in fetch.window_description + + +def test_fetch_workload_window_is_honest_that_since_is_honoured(): + """The opposite discipline from Postgres's identically-named test: `sys_query_history` + *does* carry a per-execution timestamp, so `--since` genuinely can be honoured, and the + window text must say so rather than staying silent or (worse) copying Postgres's + disclaimer that it cannot be. + """ + querier = _canned({CAP_WORKLOAD: []}) + fetch = RedshiftWorkloadAdapter(querier=querier).fetch_workload(timedelta(days=7), 500) + lowered = fetch.window_description.lower() + assert "since" in lowered + assert "honoured" in lowered + assert "not supported" not in lowered + + +def test_fetch_workload_window_names_the_truncation_not_full_coverage(): + """`ORDER BY elapsed_time DESC LIMIT n` combined with a `--since` filter means the + window actually covers is "the n most expensive queries since T", not "everything + since T" — and `cost_share` denominators throughout the rest of the run are computed + over exactly that truncated set. A window sentence that only names the cutoff (and + not the limit) reads as full coverage, which overstates what was actually analysed. + """ + querier = _canned({CAP_WORKLOAD: []}) + fetch = RedshiftWorkloadAdapter(querier=querier).fetch_workload(timedelta(days=7), 42) + assert "42" in fetch.window_description + assert "most expensive" in fetch.window_description.lower() + + +def test_fetch_workload_since_is_actually_bound_into_the_statement(): + """Guards the claim the test above makes in prose: `--since` must change what the + statement is run with, not just what the sentence says. Without this, a + `window_description` claiming the window was honoured while the query ran with no + cutoff at all would still pass every other test here. + """ + querier = FakeQuerier({RedshiftWorkloadAdapter.SQL[CAP_WORKLOAD]: []}) + RedshiftWorkloadAdapter(querier=querier).fetch_workload(timedelta(days=1), 10) + assert len(querier.calls) == 1 + _sql, params = querier.calls[0] + cutoff, cutoff_again, limit = params + assert cutoff is not None + assert cutoff == cutoff_again + assert limit == 10 + + +def test_fetch_workload_without_since_binds_no_cutoff(): + """The control for the test above: no `--since` must mean no cutoff bound in either + placeholder, not merely a friendlier sentence with a filter silently applied anyway. + """ + querier = FakeQuerier({RedshiftWorkloadAdapter.SQL[CAP_WORKLOAD]: []}) + RedshiftWorkloadAdapter(querier=querier).fetch_workload(None, 10) + _sql, params = querier.calls[0] + cutoff, cutoff_again, limit = params + assert cutoff is None + assert cutoff_again is None + assert limit == 10 + + +def test_two_executions_of_the_same_statement_collapse_to_one_query_stat_via_ingest(): + """Task 3's central aggregation claim, confirmed rather than assumed: unlike + `pg_stat_statements`, `sys_query_history` returns one row per *execution*, so + `fetch_workload` emits `calls=1` on every row (pinned below). The collapse into one + `QueryStat` per fingerprint, with `calls` and `total_time_ms` summed, is not this + adapter's job at all — it happens in the engine-agnostic `ingest()` — and this test is + what actually proves that happens for Redshift's rows, rather than assuming `ingest()`'s + Postgres behaviour carries over unchanged. + """ + querier = _canned( + { + CAP_WORKLOAD: [ + ("select id from orders where status = 'a'", 120_000), + ("select id from orders where status = 'b'", 80_000), + ] + } + ) + fetch = RedshiftWorkloadAdapter(querier=querier).fetch_workload(None, 500) + assert len(fetch.rows) == 2 + assert all(row.calls == 1 for row in fetch.rows) + + workload = ingest(fetch, "redshift") + assert len(workload.stats) == 1 + stat = workload.stats[0] + assert stat.calls == 2 + assert stat.total_time_ms == pytest.approx(200.0) + + +def test_identifier_case_and_comments_can_still_split_one_statement_into_two_stats(): + """A deliberate, documented exposure rather than a silent bug: `sys_query_history` + stores the *verbatim* text a client sent — unlike `pg_stat_statements`, which Postgres + has already parsed and re-serialised (identifiers folded to lowercase) before storing. + Two executions that are semantically one statement, differing only in identifier case + or in an attached comment, therefore fingerprint as two separate `QueryStat`s in + `ingest()`. This is not "fixed" here — a general identifier case-fold cannot tell a + case-insensitive unquoted identifier apart from a deliberately-quoted, case-sensitive + one without risking folding away a real distinction — so this test pins the current, + accepted behaviour rather than a silently-changed one. See `fetch_workload`'s docstring + for the `cost_share`/`--min-cost-share` consequence this has. + """ + querier = _canned( + { + CAP_WORKLOAD: [ + ("select id from orders where status = 'a'", 100_000), + ("SELECT ID FROM ORDERS WHERE STATUS = 'b'", 100_000), + ("/* app=foo */ select id from orders where status = 'c'", 100_000), + ] + } + ) + fetch = RedshiftWorkloadAdapter(querier=querier).fetch_workload(None, 500) + workload = ingest(fetch, "redshift") + # Three distinct QueryStats, not one — the collapse test above pins the case ingest() + # *does* unify; this pins the case it deliberately does not. + assert len(workload.stats) == 3 + + +def test_a_null_elapsed_time_does_not_crash_the_whole_run(): + """A malformed or NULL `elapsed_time` must cost this run nothing more than one row, + never the whole `fetch_workload` call — the same "one missing grant, one capability" + guarantee `_run` gives a denied statement, which is worthless if a single bad row can + still take the run down one level higher up. Coerced defensively the way + `postgres.py`'s `_as_float` call sites are, rather than raising a bare `TypeError` out + of a generator expression no caller wraps in a try/except. + """ + querier = _canned( + { + CAP_WORKLOAD: [ + ("select id from orders where status = 'a'", None), + ("select id from customers where status = 'b'", 50_000), + ] + } + ) + fetch = RedshiftWorkloadAdapter(querier=querier).fetch_workload(None, 500) + assert len(fetch.rows) == 2 + by_sql = {row.sql: row for row in fetch.rows} + assert by_sql["select id from orders where status = 'a'"].total_time_ms == 0.0 + assert by_sql["select id from customers where status = 'b'"].total_time_ms == pytest.approx( + 50.0 + ) + + +def test_fetch_schema_builds_a_sqlglot_schema_mapping(): + querier = _canned( + { + CAP_SCHEMA: [ + ("public", "orders", "id", "integer"), + ("public", "orders", "status", "character varying"), + ("public", "customers", "id", "integer"), + ] + } + ) + schema = RedshiftWorkloadAdapter(querier=querier).fetch_schema(("public",)) + assert schema == { + "public": { + "orders": {"id": "integer", "status": "character varying"}, + "customers": {"id": "integer"}, + }, + } + + +def test_fetch_schema_is_nested_by_schema(): + rows = { + CAP_SCHEMA: [ + ("sales", "orders", "id", "integer"), + ("sales", "orders", "status", "text"), + ("staging", "orders", "id", "integer"), + ] + } + adapter = RedshiftWorkloadAdapter(querier=_canned(rows)) + assert adapter.fetch_schema(("sales", "staging")) == { + "sales": {"orders": {"id": "integer", "status": "text"}}, + "staging": {"orders": {"id": "integer"}}, + } + + +def test_schema_rows_are_fetched_at_most_once_per_schema_tuple(): + """`_schema_cache`'s whole justification: `fetch_schema` and `fetch_table_facts` both + need CAP_SCHEMA rows, and querying twice for the same `schemas` tuple would do twice + the catalog work and — worse — record a denied grant in `degraded` twice for the same + missing privilege. Pins the call count directly rather than trusting the docstring. + """ + rows = { + CAP_SCHEMA: [("public", "orders", "id", "integer")], + CAP_TABLE_FACTS: [("public", "orders", 10, 1, 0.0, 0.0, "EVEN", "id", 0.0)], + } + querier = _canned(rows) + adapter = RedshiftWorkloadAdapter(querier=querier) + adapter.fetch_schema(("public",)) + adapter.fetch_table_facts(("public",), frozenset({Relation("public", "orders")})) + schema_calls = [ + call for call in querier.calls if call[0] == RedshiftWorkloadAdapter.SQL[CAP_SCHEMA] + ] + assert len(schema_calls) == 1, "CAP_SCHEMA was queried more than once for the same schemas" + + # A second, distinct schema tuple is a real cache miss, not suppressed entirely. + adapter.fetch_schema(("staging",)) + schema_calls = [ + call for call in querier.calls if call[0] == RedshiftWorkloadAdapter.SQL[CAP_SCHEMA] + ] + assert len(schema_calls) == 2 + + +def test_table_facts_do_not_alias_across_schemas(): + rows = { + CAP_SCHEMA: [("sales", "orders", "id", "integer"), ("staging", "orders", "id", "integer")], + CAP_TABLE_FACTS: [ + ("sales", "orders", 50_000, 1024, 5.0, 0.0, "EVEN", "id", 0.1), + ("staging", "orders", 7, 1, 0.0, 0.0, "KEY(id)", "id", 0.0), + ], + } + adapter = RedshiftWorkloadAdapter(querier=_canned(rows)) + facts = adapter.fetch_table_facts( + ("sales", "staging"), + frozenset({Relation("sales", "orders"), Relation("staging", "orders")}), + ) + assert facts[Relation("sales", "orders")].row_estimate == 50_000 + assert facts[Relation("staging", "orders")].row_estimate == 7 + assert facts[Relation("sales", "orders")].size_bytes == 1024 * 1024 * 1024 + assert facts[Relation("staging", "orders")].size_bytes == 1 * 1024 * 1024 + + +def test_fetch_table_facts_does_not_leak_a_same_named_table_from_another_schema(): + """Same over-fetch guard `PostgresWorkloadAdapter.fetch_table_facts` documents: the + table parameter is bare names, so a same-named table in a schema that was requested but + is not itself in `relations` can come back too. It must not appear in the result. + """ + rows = { + CAP_SCHEMA: [("sales", "orders", "id", "integer")], + CAP_TABLE_FACTS: [ + ("sales", "orders", 100, 1, 0.0, 0.0, "EVEN", "id", 0.0), + ("staging", "orders", 9_999, 50, 0.0, 0.0, "EVEN", "id", 0.0), + ], + } + adapter = RedshiftWorkloadAdapter(querier=_canned(rows)) + facts = adapter.fetch_table_facts( + ("sales", "staging"), frozenset({Relation("sales", "orders")}) + ) + assert list(facts) == [Relation("sales", "orders")] + + +def test_a_null_tbl_rows_reads_as_an_unknown_row_estimate(): + """Sentinel 1: `tbl_rows` itself coming back SQL NULL — the row was never populated at + all — must read as unknown, not as zero.""" + rows = { + CAP_SCHEMA: [("public", "orders", "id", "integer")], + CAP_TABLE_FACTS: [("public", "orders", None, 100, 0.0, 0.0, "EVEN", "id", 0.0)], + } + facts = RedshiftWorkloadAdapter(querier=_canned(rows)).fetch_table_facts( + ("public",), frozenset({Relation("public", "orders")}) + ) + assert facts[Relation("public", "orders")].row_estimate is None + # size is a separate sentinel (see below) and must be unaffected by this one. + assert facts[Relation("public", "orders")].size_bytes == 100 * 1024 * 1024 + + +def test_a_null_size_reads_as_an_unknown_size_bytes(): + """Sentinel 2: `size` coming back SQL NULL must read as unknown, independently of + `tbl_rows`.""" + rows = { + CAP_SCHEMA: [("public", "orders", "id", "integer")], + CAP_TABLE_FACTS: [("public", "orders", 500, None, 0.0, 0.0, "EVEN", "id", 0.0)], + } + facts = RedshiftWorkloadAdapter(querier=_canned(rows)).fetch_table_facts( + ("public",), frozenset({Relation("public", "orders")}) + ) + assert facts[Relation("public", "orders")].size_bytes is None + assert facts[Relation("public", "orders")].row_estimate == 500 + + +def test_stats_off_100_does_not_suppress_a_real_row_estimate_or_size(): + """The inverted-premise bug a review caught in this module's first version: `stats_off` + is a 0-100 *staleness percentage* for planner statistics, not a "never analyzed" flag — + AWS documents `tbl_rows` and `size` as physical facts about the table, not values + ANALYZE produces. Gating them on `stats_off = 100` discarded accurate facts for a + merely-stale table and then claimed the row count "could not be checked" when it + plainly could — the opposite of Redshift's answer to Postgres's genuine + `pg_class.reltuples = -1` sentinel. `stats_off` is still real evidence (see + `test_physical_facts_are_stashed_on_the_adapter_keyed_by_relation`), just not a reason + to null out these two columns. + """ + rows = { + CAP_SCHEMA: [("public", "orders", "id", "integer")], + CAP_TABLE_FACTS: [("public", "orders", 3, 1, 0.0, 100.0, "EVEN", "id", 0.0)], + } + facts = RedshiftWorkloadAdapter(querier=_canned(rows)).fetch_table_facts( + ("public",), frozenset({Relation("public", "orders")}) + ) + assert facts[Relation("public", "orders")].row_estimate == 3 + assert facts[Relation("public", "orders")].size_bytes == 1 * 1024 * 1024 + + +def test_a_null_stats_off_does_not_suppress_a_real_row_estimate(): + """`stats_off` itself coming back NULL — its staleness is simply unknown — must not + suppress `tbl_rows`/`size` either, for the same reason a *known* `stats_off` no longer + does: those two columns were never derived from ANALYZE in the first place, so nothing + about `stats_off` — known, unknown, or maximally stale — is a reason to null them out. + """ + rows = { + CAP_SCHEMA: [("public", "orders", "id", "integer")], + CAP_TABLE_FACTS: [("public", "orders", 42, 7, None, None, None, None, None)], + } + facts = RedshiftWorkloadAdapter(querier=_canned(rows)).fetch_table_facts( + ("public",), frozenset({Relation("public", "orders")}) + ) + assert facts[Relation("public", "orders")].row_estimate == 42 + assert facts[Relation("public", "orders")].size_bytes == 7 * 1024 * 1024 + + +def test_physical_facts_are_stashed_on_the_adapter_keyed_by_relation(): + rows = { + CAP_SCHEMA: [("public", "orders", "id", "integer")], + CAP_TABLE_FACTS: [ + ("public", "orders", 1000, 10, 12.5, 3.0, "KEY(customer_id)", "created_at", 0.4) + ], + } + adapter = RedshiftWorkloadAdapter(querier=_canned(rows)) + adapter.fetch_table_facts(("public",), frozenset({Relation("public", "orders")})) + physical = adapter.physical_facts[Relation("public", "orders")] + assert physical.unsorted == 12.5 + assert physical.stats_off == 3.0 + assert physical.diststyle == "KEY(customer_id)" + assert physical.sortkey1 == "created_at" + assert physical.skew_rows == 0.4 + + +def test_a_relation_absent_from_svv_table_info_is_absent_from_the_facts_dict(): + """`svv_columns` carries external (Spectrum) tables; `svv_table_info` does not. A + Spectrum relation must be simply missing from both results — not present with every + field forced to `None` — so a later task's SORTKEY/DISTKEY rules can tell + "structurally cannot have one" apart from "not analysed yet." See + `fetch_table_facts`'s docstring. + """ + rows = { + CAP_SCHEMA: [ + ("public", "orders", "id", "integer"), + ("spectrum", "events", "id", "integer"), + ], + CAP_TABLE_FACTS: [("public", "orders", 1000, 10, 0.0, 0.0, "EVEN", "id", 0.0)], + } + adapter = RedshiftWorkloadAdapter(querier=_canned(rows)) + facts = adapter.fetch_table_facts( + ("public", "spectrum"), + frozenset({Relation("public", "orders"), Relation("spectrum", "events")}), + ) + assert Relation("public", "orders") in facts + assert Relation("spectrum", "events") not in facts + assert Relation("spectrum", "events") not in adapter.physical_facts + # The columns are still there for qualification purposes (see fetch_schema). + schema = adapter.fetch_schema(("public", "spectrum")) + assert "events" in schema["spectrum"] + + +def _select_list(sql: str) -> str: + """The text between `SELECT` and the first `FROM`. See the identical helper in + `tests/test_workload_postgres.py`.""" + match = re.search(r"select\s+(.*?)\s+from\b", sql, re.IGNORECASE | re.DOTALL) + assert match, f"no SELECT ... FROM found in statement: {sql!r}" + return match.group(1) + + +def _select_list_columns(sql: str) -> list[str]: + """The SELECT list's column names, in the order the *statement* lists them. + + Double quotes are stripped: `svv_table_info`'s `"schema"` and `"table"` are reserved + words that must stay quoted in the SQL, but the name being pinned is the same either + way. A comma nested inside parentheses does not split a column — none of today's + select lists has one, but a naive `split(",")` would silently miscount if one ever + appeared. + """ + depth = 0 + names = [""] + for ch in _select_list(sql): + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if ch == "," and depth == 0: + names.append("") + else: + names[-1] += ch + return [name.strip().strip('"') for name in names] + + +#: One canned value per SELECT-list column *name*, per capability, deliberately +#: distinguishable from every other value in the same row — including from the ones of the +#: same SQL type, which is the whole point (see +#: `test_select_list_columns_land_in_the_field_their_consumer_reads`). `tuple(range(width))` +#: was positionally indistinguishable, so it pinned the column *count* and not their +#: *positions*, and swapping two same-typed columns in the SQL text left the suite green. +_CANNED_COLUMN_VALUES: dict[str, dict[str, object]] = { + CAP_WORKLOAD: { + "query_text": "select id from stand_in where status = 'x'", + "elapsed_time": 7_000, + }, + CAP_SCHEMA: { + "schema_name": "sch", + "table_name": "tbl", + "column_name": "col", + "data_type": "integer", + }, + CAP_TABLE_FACTS: { + "schema": "sch", + "table": "tbl", + "tbl_rows": 33, + "size": 44, + "unsorted": 11.0, + "stats_off": 22.0, + "diststyle": "KEY(dk_col)", + "sortkey1": "sk_col", + "skew_rows": 55.0, + }, + CAP_ADVISOR: { + "database_name": "db", + "schema_name": "sch", + "table_name": "tbl", + "type": "sort key", + "current_ddl": "CURRENT: ALTER TABLE x;", + "recommended_ddl": "RECOMMENDED: ALTER TABLE y;", + }, +} + +_CANNED_RELATION = Relation(schema="sch", table="tbl") + + +def _positional_row(capability: str) -> tuple[object, ...]: + """One row for `capability`, ordered by the *statement's own* SELECT list. + + Values are keyed by column name and then placed in the order the SQL text puts those + names in, which is what makes a swap of two columns in the statement change what the + consumer receives: the value for `stats_off` moves to `unsorted`'s position, and the + assertions below then read `unsorted == 22.0` instead of `11.0`. + """ + values = _CANNED_COLUMN_VALUES[capability] + columns = _select_list_columns(RedshiftWorkloadAdapter.SQL[capability]) + assert sorted(columns) == sorted(values), ( + f"{capability}'s SELECT list is {columns}, which no longer matches the columns this " + f"test pins ({sorted(values)}) — add the new column and assert where it lands, " + "rather than widening the fixture until it stops complaining" + ) + return tuple(values[column] for column in columns) + + +def _check_workload_columns(adapter: RedshiftWorkloadAdapter) -> None: + (row,) = adapter.fetch_workload(None, 10).rows + assert row.sql == "select id from stand_in where status = 'x'" + # elapsed_time is microseconds; total_time_ms is milliseconds. + assert row.total_time_ms == pytest.approx(7.0) + + +def _check_schema_columns(adapter: RedshiftWorkloadAdapter) -> None: + assert adapter.fetch_schema(("sch",)) == {"sch": {"tbl": {"col": "integer"}}} + + +def _check_table_facts_columns(adapter: RedshiftWorkloadAdapter) -> None: + facts = adapter.fetch_table_facts(("sch",), frozenset({_CANNED_RELATION})) + assert facts[_CANNED_RELATION].row_estimate == 33 + assert facts[_CANNED_RELATION].size_bytes == 44 * 1024 * 1024 + physical = adapter.physical_facts[_CANNED_RELATION] + # `unsorted` and `stats_off` are both 0-100 float percentages, so nothing but this + # assertion can tell them apart — and swapping them inverts ADV104's remediation + # (VACUUM where ANALYZE was needed) at the one confidence rung this adapter reaches + # HIGH on. Same for the `diststyle`/`sortkey1` pair, which are both text and gate + # ADV101's and ADV102's suppression. + assert physical.unsorted == 11.0 + assert physical.stats_off == 22.0 + assert physical.diststyle == "KEY(dk_col)" + assert physical.sortkey1 == "sk_col" + assert physical.skew_rows == 55.0 + + +def _check_advisor_columns(adapter: RedshiftWorkloadAdapter) -> None: + (row,) = adapter._advisor_rows(("sch",), frozenset({_CANNED_RELATION})) + assert row.relation == _CANNED_RELATION + assert row.rec_type == "sort key" + assert row.current_ddl == "CURRENT: ALTER TABLE x;" + assert row.recommended_ddl == "RECOMMENDED: ALTER TABLE y;" + + +@pytest.mark.parametrize( + ("capability", "check"), + [ + (CAP_WORKLOAD, _check_workload_columns), + (CAP_SCHEMA, _check_schema_columns), + (CAP_TABLE_FACTS, _check_table_facts_columns), + (CAP_ADVISOR, _check_advisor_columns), + ], + ids=["workload", "schema", "table_facts", "advisor"], +) +def test_select_list_columns_land_in_the_field_their_consumer_reads(capability, check): + """Every statement's SELECT list is pinned by *position*, not merely by arity. + + Batch 2 shipped a column-count mismatch between a statement's SELECT list and its + Python unpacking that no fixture caught, and the guard added for it derived only the row + *width* from the SQL text (`tuple(range(width))`) — so it pinned the count while leaving + every column's position unverified. Swapping two same-typed columns in the statement + left the whole suite green: `unsorted`/`stats_off` inverts ADV104's remediation for + every table on the cluster, `diststyle`/`sortkey1` inverts ADV101's and ADV102's + suppression gates, `table_name`/`column_name` mis-keys the schema map, and + `query_text`/`elapsed_time` mis-reads the whole workload. + + The fixture row is built by looking each canned value up *by column name* and then + placing it at the position the SQL text gives that name — so a swap in the SQL moves + the values with it and the assertions below read the wrong field. Arity is still pinned + too, and by name rather than by number: `_positional_row` requires the statement's + SELECT list to be exactly the set of columns this test knows where to expect. + + One parametrized case per capability, including `CAP_ADVISOR` — whose consumer is + `_advisor_rows` — so a mismatch in one cannot hide behind the other three passing. + """ + querier = _canned({capability: [_positional_row(capability)]}) + check(RedshiftWorkloadAdapter(querier=querier)) + + +class _FakeCursor: + """Enough of a psycopg cursor for connect()'s session-setup statements. + + `fail_on` lets a test make exactly one statement (identified by its SQL text) raise, + which is how the read-only degradation path is exercised without a live cluster: a + real Redshift refusing `SET default_transaction_read_only` looks, from here, like a + cursor.execute() that raises on that one statement and succeeds on every other. + """ + + def __init__( + self, + log: list[tuple] | None = None, + *, + fail_on: frozenset[str] = frozenset(), + fail_message: str = "ERROR: {sql!r} is not supported on this cluster", + ): + self.executed: list[tuple] = [] + self._log = log if log is not None else [] + self._fail_on = fail_on + self._fail_message = fail_message + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def execute(self, sql, params=None): + self.executed.append((sql, params)) + self._log.append((sql, params)) + if sql in self._fail_on: + raise RuntimeError(self._fail_message.format(sql=sql)) + + def fetchall(self): + return [] + + +class _FakeConnection: + def __init__( + self, + *, + fail_on: frozenset[str] = frozenset(), + fail_message: str = "ERROR: {sql!r} is not supported on this cluster", + ) -> None: + self.cursors: list[_FakeCursor] = [] + self.log: list[tuple] = [] + self._fail_on = fail_on + self._fail_message = fail_message + + def cursor(self): + cursor = _FakeCursor(self.log, fail_on=self._fail_on, fail_message=self._fail_message) + self.cursors.append(cursor) + return cursor + + +def _install_fake_psycopg( + monkeypatch, + seen: dict, + *, + fail_on: frozenset[str] = frozenset(), + fail_message: str = "ERROR: {sql!r} is not supported on this cluster", +): + """A psycopg that records the conninfo it was handed and connects successfully.""" + + module = types.ModuleType("psycopg") + + def connect(conninfo, **kwargs): + seen["conninfo"] = conninfo + seen["connection"] = _FakeConnection(fail_on=fail_on, fail_message=fail_message) + return seen["connection"] + + module.connect = connect # type: ignore[attr-defined] + module.conninfo = types.SimpleNamespace( # type: ignore[attr-defined] + make_conninfo=lambda **kw: " ".join(f"{k}={v}" for k, v in kw.items()) + ) + monkeypatch.setitem(sys.modules, "psycopg", module) + return module + + +def test_connect_without_psycopg_installed_raises_a_helpful_import_error(monkeypatch): + import builtins + + real_import = builtins.__import__ + + def no_psycopg(name, *args, **kwargs): + if name == "psycopg": + raise ImportError("No module named 'psycopg'") + return real_import(name, *args, **kwargs) + + monkeypatch.setattr(builtins, "__import__", no_psycopg) + adapter = RedshiftWorkloadAdapter() + params = ConnectionParams( + engine="redshift", dsn="postgresql://u@h/db", fields={}, source="--dsn" + ) + with pytest.raises(ImportError) as exc: + adapter.connect(params, 30) + assert "sqlquality[warehouse]" in str(exc.value) + # Names the calling engine, not merely the extra: `import_psycopg("Redshift", ...)` + # could be miscopied to `import_psycopg("Postgres", ...)` at the Redshift call site + # and every existing assertion here would still pass. + assert "Redshift" in str(exc.value) + assert "Postgres" not in str(exc.value) + + +def test_connect_arms_a_statement_timeout_before_the_querier_is_usable(monkeypatch): + """Mirrors the Postgres unit test of the same shape: session setup must precede any + later query, and the relative order is only observable on the connection-wide log.""" + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + adapter = RedshiftWorkloadAdapter() + adapter.connect( + ConnectionParams(engine="redshift", dsn="postgresql:///x", fields={}, source="--dsn"), 30 + ) + + setup = seen["connection"].log[:] + assert setup == [ + (READ_ONLY_SQL, None), + ("SELECT set_config('statement_timeout', %s, false)", ("30000ms",)), + ], setup + assert adapter.degraded == [] + + adapter._query("SELECT 1", ()) + assert seen["connection"].log[:2] == setup + assert seen["connection"].log[2] == ("SELECT 1", ()) + + +def test_an_out_of_range_timeout_is_clamped_before_it_reaches_the_session(monkeypatch): + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + RedshiftWorkloadAdapter().connect( + ConnectionParams(engine="redshift", dsn="postgresql:///x", fields={}, source="--dsn"), 7200 + ) + assert seen["connection"].log[1][1] == (f"{MAX_TIMEOUT_S * 1000}ms",) + + +def test_a_refused_read_only_statement_degrades_rather_than_aborts(monkeypatch): + """The Redshift-specific difference from Postgres: `SET default_transaction_read_only` + is not accepted in every configuration. A refusal must not be silently treated as + success — the whole "sqlquality never writes" promise rests on the operator being + told, not on the tool assuming the best.""" + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen, fail_on=frozenset({READ_ONLY_SQL})) + adapter = RedshiftWorkloadAdapter() + adapter.connect( + ConnectionParams(engine="redshift", dsn="postgresql:///x", fields={}, source="--dsn"), 30 + ) + + # The connection still succeeds and the statement timeout is still armed — + # a refused read-only guard is not a reason to abandon the rest of setup. + assert adapter._query is not None + assert seen["connection"].log[1] == ( + "SELECT set_config('statement_timeout', %s, false)", + ("30000ms",), + ) + + assert len(adapter.degraded) == 1 + capability, reason = adapter.degraded[0] + assert capability == DEGRADATION_READ_ONLY + assert "could not be proven read-only" in reason + # Not "we might write" — the four SELECT-only statements this adapter issues are a + # separate, already-pinned guarantee (test_no_statement_writes). What is missing is + # the extra belt-and-braces defense, and the message must say which. + assert "belt-and-braces" in reason.lower() + + +def test_the_read_only_degradation_survives_past_fetch_workload(monkeypatch): + """A carried-forward item from Task 2: the read-only degradation above was recorded + correctly, but it could never reach a user, because `cli.py` calls `fetch_workload()` + immediately after `connect()` and that call raised `NotImplementedError` — an unhandled + exception that crashed the whole run before it ever reached the loop that prints + `adapter.degraded` to stderr. `fetch_workload` is now a real method, so that call + succeeds instead of raising, and `degraded` survives to be printed later. + + This does not exercise `cli.py` end to end — `propose()` is still `NotImplementedError` + until Task 5/6 builds it, so a full `advise` run cannot complete yet — but it proves + the specific failure this task closes: the read-only warning is no longer lost between + `connect()` and the rest of the run. + """ + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen, fail_on=frozenset({READ_ONLY_SQL})) + adapter = RedshiftWorkloadAdapter() + adapter.connect( + ConnectionParams(engine="redshift", dsn="postgresql:///x", fields={}, source="--dsn"), 30 + ) + assert len(adapter.degraded) == 1 # the read-only degradation recorded by connect() + + fetch = adapter.fetch_workload(None, 10) # must not raise, and must not touch degraded + assert fetch.rows == () + assert len(adapter.degraded) == 1 + assert adapter.degraded[0][0] == DEGRADATION_READ_ONLY + + +def test_the_read_only_degradation_message_is_scrubbed(monkeypatch): + """The one path that puts raw driver text into user-facing output. + + `self.degraded` is exactly what `cli.py` prints to stderr and embeds in the JSON and + markdown reports, so a secret reaching this message is a real leak, not a theoretical + one — unlike the connect-failure path, which is at least caught by a `ConnectionError` + the caller might choose not to print. The fake driver's refusal is made to quote the + password verbatim, the way a permission-denied message naming the failed session + setting sometimes echoes surrounding context; `scrub()` must still remove it. + """ + seen: dict = {} + _install_fake_psycopg( + monkeypatch, + seen, + fail_on=frozenset({READ_ONLY_SQL}), + fail_message="ERROR: {sql!r} refused for connection password=hunter2", + ) + adapter = RedshiftWorkloadAdapter() + adapter.connect( + ConnectionParams( + engine="redshift", + dsn=None, + fields={"host": "db", "user": "hans", "password": "hunter2"}, + source="profiles.yml", + ), + 30, + ) + assert len(adapter.degraded) == 1 + _capability, reason = adapter.degraded[0] + assert "hunter2" not in reason + assert "***" in reason + + +def test_a_successful_read_only_statement_reports_no_degradation(monkeypatch): + """Guards the guard above: without a forced failure, the same setup reports nothing, + so the degradation in the previous test is attributable to the refusal, not to + `connect()` always degrading regardless of what the driver does.""" + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + adapter = RedshiftWorkloadAdapter() + adapter.connect( + ConnectionParams(engine="redshift", dsn="postgresql:///x", fields={}, source="--dsn"), 30 + ) + assert adapter.degraded == [] + + +def test_connect_scrubs_a_password_from_a_driver_failure(monkeypatch): + """psycopg is not believed to echo a password, but the auth-failure path cannot be + exercised without a live server, so the secret is scrubbed rather than trusted — + the same reasoning `test_workload_postgres.py`'s identical test gives.""" + fake_psycopg = types.ModuleType("psycopg") + + def explode(conninfo, **kwargs): + raise RuntimeError(f"connection failed for conninfo {conninfo}") + + fake_psycopg.connect = explode # type: ignore[attr-defined] + fake_psycopg.conninfo = types.SimpleNamespace( # type: ignore[attr-defined] + make_conninfo=lambda **kw: " ".join(f"{k}={v}" for k, v in kw.items()) + ) + monkeypatch.setitem(sys.modules, "psycopg", fake_psycopg) + + params = ConnectionParams( + engine="redshift", + dsn=None, + fields={"host": "db", "user": "hans", "password": "hunter2"}, + source="profiles.yml", + ) + with pytest.raises(ConnectionError) as exc: + RedshiftWorkloadAdapter().connect(params, 30) + assert "hunter2" not in str(exc.value) + assert "***" in str(exc.value) + assert exc.value.__cause__ is None + assert exc.value.__context__ is None + + +def test_a_conninfo_build_failure_is_scrubbed_like_a_connect_failure(monkeypatch): + """make_conninfo runs inside the scrubbing envelope: it can itself raise on an + unusable keyword, and that message can quote the offending value.""" + + module = types.ModuleType("psycopg") + + def never_called(conninfo, **kwargs): + raise AssertionError("must not reach connect() when the conninfo cannot be built") + + def explode(**kwargs): + raise RuntimeError(f"invalid connection option: {sorted(kwargs.items())}") + + module.connect = never_called # type: ignore[attr-defined] + module.conninfo = types.SimpleNamespace(make_conninfo=explode) # type: ignore[attr-defined] + monkeypatch.setitem(sys.modules, "psycopg", module) + + params = ConnectionParams( + engine="redshift", + dsn=None, + fields={"host": "db", "user": "hans", "password": "hunter2"}, + source="profiles.yml", + ) + with pytest.raises(ConnectionError) as exc: + RedshiftWorkloadAdapter().connect(params, 30) + assert "hunter2" not in str(exc.value) + assert "***" in str(exc.value) + assert exc.value.__context__ is None + + +def test_dropped_profile_keys_are_named_on_stderr(monkeypatch, capsys): + """A key we cannot forward must be reported, not discarded in silence.""" + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + params = ConnectionParams( + engine="redshift", + dsn=None, + fields={ + "host": "db", + "user": "hans", + "password": "hunter2", + "cluster_identifier": "my-cluster", + "iam": "true", + }, + source="profiles.yml", + ) + RedshiftWorkloadAdapter().connect(params, 30) + warning = capsys.readouterr().err + assert "cluster_identifier" in warning + assert "iam" in warning + # Key names only — never a value, since one of them could be a secret. + assert "hunter2" not in warning + assert "my-cluster" not in warning + + +def test_forwarded_and_mapped_keys_are_not_reported_as_dropped(monkeypatch, capsys): + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + params = ConnectionParams( + engine="redshift", + dsn=None, + fields={"host": "db", "dbname": "x", "user": "u", "password": "p", "sslmode": "require"}, + source="profiles.yml", + ) + RedshiftWorkloadAdapter().connect(params, 30) + assert capsys.readouterr().err == "" + + +def test_profile_fields_are_translated_and_forwarded_to_the_driver(monkeypatch): + """Pins the actual conninfo content, not just the dropped-keys warning. + + A previous version of this suite recorded `seen["conninfo"]` and never asserted on + it, so scrambling the field map's targets, dropping the `database`/`username` + aliases, or cutting the TLS passthrough set down to just `sslmode` all left the whole + suite green. This test mirrors Postgres's + `test_profile_tls_settings_are_forwarded_to_the_driver` for exactly that reason: the + TLS group is not cosmetic — a profile saying `sslmode: verify-full` that silently + connects under libpq's default `prefer` performs no certificate verification at all, + and the user is never told. + """ + seen: dict = {} + _install_fake_psycopg(monkeypatch, seen) + params = ConnectionParams( + engine="redshift", + dsn=None, + fields={ + "host": "db", + "database": "mydb", # alias for dbname + "username": "hans", # alias for user + "password": "hunter2", + "sslmode": "verify-full", + "sslrootcert": "/etc/ssl/ca.crt", + "sslcert": "/etc/ssl/client.crt", + "sslkey": "/etc/ssl/client.key", + "connect_timeout": "10", + }, + source="profiles.yml", + ) + RedshiftWorkloadAdapter().connect(params, 30) + conninfo = seen["conninfo"] + assert "host=db" in conninfo + assert "dbname=mydb" in conninfo + assert "user=hans" in conninfo + assert "password=hunter2" in conninfo + assert "sslmode=verify-full" in conninfo + assert "sslrootcert=/etc/ssl/ca.crt" in conninfo + assert "sslcert=/etc/ssl/client.crt" in conninfo + assert "sslkey=/etc/ssl/client.key" in conninfo + assert "connect_timeout=10" in conninfo diff --git a/tests/test_workload_redshift_rules.py b/tests/test_workload_redshift_rules.py new file mode 100644 index 0000000..b049782 --- /dev/null +++ b/tests/test_workload_redshift_rules.py @@ -0,0 +1,1576 @@ +"""ADV101-105: Redshift's own physical-design proposals (SORTKEY, DISTKEY, DISTSTYLE ALL, +VACUUM/ANALYZE, and Redshift Advisor's own recommendations). + +Every one of ADV101-103 recommends DDL that rewrites the whole table, and Redshift exposes +no per-column NDV to predict distribution skew or predicate selectivity — see the plan's +"why the rules are not the Postgres rules renamed" section and each `propose_*` function's +own docstring in `redshift.py`. That is why each of the three tests its own MEDIUM cap +independently: a mutant that quietly added a HIGH branch must fail here, not just in prose. +""" + +from __future__ import annotations + +import pytest + +from sqlquality.dbtproject import ModelNode +from sqlquality.models import ( + Aggregation, + ColumnRole, + ColumnUsage, + Confidence, + Proposal, + Relation, + TableFacts, + Workload, +) +from sqlquality.workload.dbt import DbtContext, enrich_proposals +from sqlquality.workload.redshift import ( + CAP_ADVISOR, + DEGRADATION_PHYSICAL_FACTS_GAP, + MAX_ROWS_FOR_DISTSTYLE_ALL, + RedshiftAdvisorRow, + RedshiftTableFacts, + RedshiftWorkloadAdapter, + _advisor_category, + _collapse_diststyle_all_over_distkey, + _diststyle_is_all, + _diststyle_key_column, + _disclose_advisor_agreement, + _quote_ident, + _skipped_for_physical_gap, + propose_advisor, + propose_diststyle_all, + propose_distkey, + propose_maintenance, + propose_sortkey, +) + +R = Relation(schema="public", table="orders") +R2 = Relation(schema="public", table="customers") + + +def _usage( + relation: Relation, + column: str, + role: ColumnRole, + *, + cost_ms: float = 100.0, + cost_share: float = 0.5, + calls: int = 10, +) -> ColumnUsage: + return ColumnUsage( + relation=relation, + column=column, + role=role, + calls=calls, + cost_ms=cost_ms, + cost_share=cost_share, + ) + + +def _facts(relation: Relation, *, row_estimate: int | None = 1_000) -> dict[Relation, TableFacts]: + return { + relation: TableFacts( + relation=relation, row_estimate=row_estimate, size_bytes=1000, columns=() + ) + } + + +def _phys( + *, + unsorted: float | None = None, + stats_off: float | None = None, + diststyle: str | None = None, + sortkey1: str | None = None, + skew_rows: float | None = None, +) -> RedshiftTableFacts: + return RedshiftTableFacts( + unsorted=unsorted, + stats_off=stats_off, + diststyle=diststyle, + sortkey1=sortkey1, + skew_rows=skew_rows, + ) + + +# --------------------------------------------------------------------------- +# ADV101 — propose_sortkey +# --------------------------------------------------------------------------- + + +def test_sortkey_proposes_at_medium_when_the_table_is_sorted_on_a_different_column(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + p = proposals[0] + assert p.code == "ADV101" + assert p.confidence is Confidence.MEDIUM + assert p.ddl == 'ALTER TABLE "public"."orders" ALTER SORTKEY ("created_at");' + assert "created_at" in p.title + assert "'status'" in p.rationale + + +def test_sortkey_is_suppressed_when_already_the_sort_key(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="created_at")} + assert propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_sortkey_drops_to_low_when_the_existing_sort_key_could_not_be_read(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1=None)} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + assert proposals[0].confidence is Confidence.LOW + assert "could not be read" in proposals[0].rationale + + +def test_sortkey_never_reaches_high_even_with_a_dominant_cost_share(): + """Pins the deliberate MEDIUM cap: no input, however hot, should ever produce HIGH.""" + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.99)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.01) + assert proposals[0].confidence is Confidence.MEDIUM + + +def test_sortkey_medium_cap_rationale_states_the_reason_not_just_the_word(): + """The MEDIUM rung's explanation is the operator's only reason a whole-table rewrite + is offered at less than HIGH, and it is also what stops a later reader adding the + missing HIGH branch "for symmetry" — pin the substance, not just that the word + "MEDIUM" appears somewhere in the rationale. + """ + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + rationale = proposals[0].rationale + assert "a SORTKEY change only repays the rewrite if this predicate is selective" in rationale + assert "Redshift exposes no per-column" in rationale + assert "distinct-value statistics" in rationale + + +def test_sortkey_no_proposal_when_relation_absent_from_physical_facts(): + """Cannot tell a Spectrum table from a genuinely empty one, so no proposal at all — + see `propose_sortkey`'s docstring. Not a silent skip: the reasoning is documented, + but the runtime outcome is that this relation gets nothing from this rule. + """ + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + assert propose_sortkey(usage, _facts(R), {}, min_cost_share=0.1) == [] + + +def test_sortkey_suppressed_below_min_cost_share(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.05)] + physical = {R: _phys(sortkey1="status")} + assert propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_sortkey_candidate_is_the_hottest_range_or_equality_column_not_join_or_group(): + usage = [ + _usage(R, "join_col", ColumnRole.JOIN, cost_ms=999.0, cost_share=0.9), + _usage(R, "group_col", ColumnRole.GROUP, cost_ms=999.0, cost_share=0.9), + _usage(R, "cold_range", ColumnRole.RANGE, cost_ms=10.0, cost_share=0.2), + _usage(R, "hot_equality", ColumnRole.EQUALITY, cost_ms=500.0, cost_share=0.6), + ] + physical = {R: _phys(sortkey1=None)} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + assert proposals[0].evidence["column"] == "hot_equality" + + +def test_sortkey_discloses_stats_off_as_a_caveat_when_present(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status", stats_off=42.0)} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + assert "42%" in proposals[0].rationale + assert "stats_off" in proposals[0].rationale + + +def test_sortkey_omits_stats_off_caveat_when_it_is_zero(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status", stats_off=0.0)} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + assert "stats_off" not in proposals[0].rationale + + +def test_sortkey_note_discloses_the_full_table_rewrite(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + note = proposals[0].note or "" + assert "rewrites the entire table" in note + assert "CONCURRENTLY" in note + + +# --------------------------------------------------------------------------- +# ADV102 — propose_distkey +# --------------------------------------------------------------------------- + + +def test_distkey_proposes_at_medium_when_distributed_on_a_different_column(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="KEY(order_id)")} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + p = proposals[0] + assert p.code == "ADV102" + assert p.confidence is Confidence.MEDIUM + assert p.ddl == 'ALTER TABLE "public"."orders" ALTER DISTKEY "customer_id";' + + +def test_distkey_suppressed_when_already_the_distribution_key(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="KEY(customer_id)")} + assert propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_distkey_suppressed_when_already_distributed_on_the_key_inside_auto(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="AUTO(KEY(customer_id))")} + assert propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_distkey_suppressed_when_diststyle_is_already_all(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="ALL")} + assert propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_distkey_suppressed_when_diststyle_is_auto_all(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="AUTO(ALL)")} + assert propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_distkey_not_suppressed_by_even_diststyle(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="EVEN")} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + assert proposals[0].confidence is Confidence.MEDIUM + + +def test_distkey_drops_to_low_when_diststyle_could_not_be_read(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle=None)} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + assert proposals[0].confidence is Confidence.LOW + assert "could not be read" in proposals[0].rationale + + +def test_distkey_never_reaches_high_even_with_a_dominant_cost_share(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.99)] + physical = {R: _phys(diststyle="EVEN")} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.01) + assert proposals[0].confidence is Confidence.MEDIUM + + +def test_distkey_medium_cap_rationale_states_the_reason_not_just_the_word(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="EVEN")} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + rationale = proposals[0].rationale + assert "distribution skew is what makes a DISTKEY choice good or catastrophic" in rationale + assert "Redshift exposes no per-column" in rationale + assert "distinct-value statistics" in rationale + + +def test_distkey_no_proposal_when_relation_absent_from_physical_facts(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + assert propose_distkey(usage, _facts(R), {}, min_cost_share=0.1) == [] + + +def test_distkey_suppressed_below_min_cost_share(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.05)] + physical = {R: _phys(diststyle="EVEN")} + assert propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) == [] + + +def test_distkey_candidate_is_the_hottest_join_column_not_range_or_equality(): + usage = [ + _usage(R, "range_col", ColumnRole.RANGE, cost_ms=999.0, cost_share=0.9), + _usage(R, "cold_join", ColumnRole.JOIN, cost_ms=10.0, cost_share=0.2), + _usage(R, "hot_join", ColumnRole.JOIN, cost_ms=500.0, cost_share=0.6), + ] + physical = {R: _phys(diststyle="EVEN")} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert len(proposals) == 1 + assert proposals[0].evidence["column"] == "hot_join" + + +def test_distkey_discloses_skew_rows_as_a_caveat_when_present(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="EVEN", skew_rows=3.5)} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert "3.50" in proposals[0].rationale + assert "current" in proposals[0].rationale.lower() + + +def test_distkey_note_discloses_the_full_table_rewrite(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="EVEN")} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + note = proposals[0].note or "" + assert "rewrites the entire table" in note + assert "CONCURRENTLY" in note + + +def test_distkey_omits_skew_rows_caveat_when_absent(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="EVEN", skew_rows=None)} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert "skew_rows" not in proposals[0].rationale + + +def test_distkey_omits_stats_off_caveat_when_it_is_zero(): + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R: _phys(diststyle="EVEN", stats_off=0.0)} + proposals = propose_distkey(usage, _facts(R), physical, min_cost_share=0.1) + assert "stats_off" not in proposals[0].rationale + + +# --------------------------------------------------------------------------- +# ADV103 — propose_diststyle_all +# --------------------------------------------------------------------------- + + +def test_diststyle_all_proposes_at_medium_when_small_and_joined_and_not_already_all(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert len(proposals) == 1 + p = proposals[0] + assert p.code == "ADV103" + assert p.confidence is Confidence.MEDIUM + assert p.ddl == 'ALTER TABLE "public"."customers" ALTER DISTSTYLE ALL;' + + +def test_diststyle_all_suppressed_when_already_all(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="ALL")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert proposals == [] + + +def test_diststyle_all_suppressed_when_over_the_row_ceiling(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=10_000_000), physical, min_cost_share=0.1 + ) + assert proposals == [] + + +def test_diststyle_all_not_suppressed_at_exactly_the_ceiling(): + """The gate is `rows > max_rows`, not `>=` — a table exactly at the ceiling is still a + candidate. A mutant flipping the comparison direction or operator must fail this.""" + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000_000), physical, min_cost_share=0.1, max_rows=1_000_000 + ) + assert len(proposals) == 1 + + +def test_diststyle_all_requires_the_table_to_be_joined_in_the_workload(): + usage = [_usage(R2, "status", ColumnRole.EQUALITY, cost_share=0.9)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert proposals == [] + + +def test_diststyle_all_suppressed_below_min_cost_share(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.05)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert proposals == [] + + +def test_diststyle_all_no_proposal_when_relation_absent_from_physical_facts(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + proposals = propose_diststyle_all(usage, _facts(R2, row_estimate=1_000), {}, min_cost_share=0.1) + assert proposals == [] + + +def test_diststyle_all_low_when_both_row_count_and_diststyle_are_unreadable(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle=None)} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=None), physical, min_cost_share=0.1 + ) + assert len(proposals) == 1 + assert proposals[0].confidence is Confidence.LOW + assert "row count could not be verified" in proposals[0].rationale + assert "distribution style could not be read" in proposals[0].rationale + + +def test_diststyle_all_low_when_only_row_count_is_unreadable(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=None), physical, min_cost_share=0.1 + ) + assert len(proposals) == 1 + assert proposals[0].confidence is Confidence.LOW + assert "row count could not be verified" in proposals[0].rationale + assert "EVEN" in proposals[0].rationale + + +def test_diststyle_all_low_when_only_diststyle_is_unreadable(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle=None)} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert len(proposals) == 1 + assert proposals[0].confidence is Confidence.LOW + assert "distribution style could not be read" in proposals[0].rationale + assert "1,000 rows" in proposals[0].rationale + + +def test_diststyle_all_never_reaches_high(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.99)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=100), physical, min_cost_share=0.01 + ) + assert proposals[0].confidence is Confidence.MEDIUM + + +def test_diststyle_all_medium_cap_rationale_states_the_reason_not_just_the_word(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + rationale = proposals[0].rationale + assert "this rule's row-count ceiling is a heuristic" in rationale + assert ( + "not a measurement of the storage and write cost this table will actually incur " + "once replicated" in rationale + ) + + +def test_diststyle_all_discloses_write_amplification_and_storage_cost(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + rationale = proposals[0].rationale + assert "storage" in rationale.lower() + assert "node count" in rationale + assert "replicated to every node" in rationale + + +def test_diststyle_all_note_discloses_the_full_table_rewrite(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + note = proposals[0].note or "" + assert "rewrites the entire table" in note + assert "CONCURRENTLY" in note + + +def test_diststyle_all_discloses_stats_off_as_a_caveat_when_present(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN", stats_off=33.0)} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert "33%" in proposals[0].rationale + assert "stats_off" in proposals[0].rationale + + +def test_diststyle_all_omits_stats_off_caveat_when_it_is_zero(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN", stats_off=0.0)} + proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert "stats_off" not in proposals[0].rationale + + +# --------------------------------------------------------------------------- +# ADV104 — propose_maintenance +# --------------------------------------------------------------------------- + + +def test_maintenance_proposes_vacuum_at_high_when_unsorted_meets_the_threshold(): + physical = {R: _phys(unsorted=20.0)} + proposals = propose_maintenance(physical, _facts(R)) + assert len(proposals) == 1 + p = proposals[0] + assert p.code == "ADV104" + assert p.confidence is Confidence.HIGH + assert p.ddl == 'VACUUM "public"."orders";' + assert "VACUUM" in p.title + + +def test_maintenance_no_vacuum_below_the_unsorted_threshold(): + physical = {R: _phys(unsorted=19.99)} + assert propose_maintenance(physical, _facts(R)) == [] + + +def test_maintenance_no_vacuum_when_unsorted_is_unmeasured(): + physical = {R: _phys(unsorted=None)} + assert propose_maintenance(physical, _facts(R)) == [] + + +def test_maintenance_proposes_analyze_at_high_when_stats_off_meets_the_threshold(): + physical = {R: _phys(stats_off=20.0)} + proposals = propose_maintenance(physical, _facts(R)) + assert len(proposals) == 1 + p = proposals[0] + assert p.code == "ADV104" + assert p.confidence is Confidence.HIGH + assert p.ddl == 'ANALYZE "public"."orders";' + assert "ANALYZE" in p.title + + +def test_maintenance_no_analyze_below_the_stats_off_threshold(): + physical = {R: _phys(stats_off=19.99)} + assert propose_maintenance(physical, _facts(R)) == [] + + +def test_maintenance_no_analyze_when_stats_off_is_unmeasured(): + physical = {R: _phys(stats_off=None)} + assert propose_maintenance(physical, _facts(R)) == [] + + +def test_maintenance_proposes_both_independently_for_the_same_relation(): + physical = {R: _phys(unsorted=50.0, stats_off=60.0)} + proposals = propose_maintenance(physical, _facts(R)) + assert len(proposals) == 2 + ddls = {p.ddl for p in proposals} + assert ddls == {'VACUUM "public"."orders";', 'ANALYZE "public"."orders";'} + assert all(p.confidence is Confidence.HIGH for p in proposals) + + +def test_maintenance_ignores_relations_with_neither_measurement_stale(): + physical = {R: _phys(unsorted=1.0, stats_off=1.0)} + assert propose_maintenance(physical, _facts(R)) == [] + + +def test_maintenance_not_gated_by_cost_share_or_workload_usage(): + """ADV104's evidence is a catalog measurement, not workload cost — there is no + `min_cost_share` parameter at all, and it must fire from `physical` facts alone even + with no usage/workload data in view.""" + physical = {R: _phys(unsorted=99.0)} + proposals = propose_maintenance(physical, {}) + assert len(proposals) == 1 + assert proposals[0].evidence["row_estimate"] is None + + +# --------------------------------------------------------------------------- +# ADV105 — propose_advisor +# --------------------------------------------------------------------------- + + +def test_advisor_proposal_is_always_high_confidence(): + row = RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl="ALTER TABLE x;" + ) + proposals = propose_advisor([row]) + assert proposals[0].confidence is Confidence.HIGH + + +def test_advisor_proposal_carries_advisors_own_ddl_verbatim(): + row = RedshiftAdvisorRow( + relation=R, + rec_type="sort key", + current_ddl="ALTER TABLE public.orders ALTER SORTKEY NONE;", + recommended_ddl='ALTER TABLE public.orders ALTER SORTKEY ("created_at");', + ) + proposals = propose_advisor([row]) + p = proposals[0] + assert p.ddl == 'ALTER TABLE public.orders ALTER SORTKEY ("created_at");' + assert "Current:" in p.rationale + assert "Recommended:" in p.rationale + + +def test_advisor_proposal_note_attributes_the_ddl_to_redshift_not_sqlquality(): + row = RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl="ALTER TABLE x;" + ) + proposals = propose_advisor([row]) + note = proposals[0].note or "" + assert "Amazon Redshift Advisor" in note + assert "not sqlquality's own analysis" in note + + +def test_advisor_proposal_title_attributes_to_advisor_not_sqlquality(): + """`title` is the one field the terminal table always renders — pin it directly, + since `note` alone (already covered above) does not stop `title` or `rationale` from + independently being rewritten to claim this recommendation as sqlquality's own.""" + row = RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl="ALTER TABLE x;" + ) + proposals = propose_advisor([row]) + title = proposals[0].title + assert "Amazon Redshift Advisor" in title + assert "recommends" in title + assert "sqlquality recommends" not in title.lower() + + +def test_advisor_proposal_rationale_attributes_to_advisor_not_sqlquality(): + row = RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl="ALTER TABLE x;" + ) + proposals = propose_advisor([row]) + rationale = proposals[0].rationale + assert rationale.startswith("Amazon Redshift Advisor") + assert "sqlquality did not generate or verify this recommendation" in rationale + assert "sqlquality recommends" not in rationale.lower() + + +def test_advisor_proposal_evidence_carries_a_machine_readable_source(): + """A caller rendering evidence as bare `k=v` pairs (this report's own discipline) still + needs a signal that this proposal's source is Advisor, not this adapter's inference — + prose alone (`title`/`rationale`/`note`) is invisible to that renderer.""" + row = RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl="ALTER TABLE x;" + ) + proposals = propose_advisor([row]) + assert proposals[0].evidence["source"] == "amazon_redshift_advisor" + + +def test_advisor_proposal_handles_missing_current_and_recommended_ddl(): + row = RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl=None + ) + proposals = propose_advisor([row]) + p = proposals[0] + assert p.ddl is None + assert "Current:" not in p.rationale + assert "Recommended:" not in p.rationale + + +def test_advisor_produces_one_proposal_per_row_in_deterministic_order(): + """Rows are fed in the *opposite* of sorted order — "orders" before "customers" — so + a mutant that deleted the `sorted(...)` call in `propose_advisor` and simply iterated + `rows` as given would still fail this assertion. Feeding already-sorted input here + previously let that mutant survive.""" + rows = [ + RedshiftAdvisorRow(relation=R, rec_type="a", current_ddl=None, recommended_ddl=None), + RedshiftAdvisorRow(relation=R2, rec_type="b", current_ddl=None, recommended_ddl=None), + ] + proposals = propose_advisor(rows) + # Sorted by (schema, table, rec_type) — both share schema "public", so table order + # alone decides: "customers" sorts before "orders". + assert [p.evidence["table"] for p in proposals] == ["customers", "orders"] + + +# --------------------------------------------------------------------------- +# `_advisor_category` and diststyle parsing helpers +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("diststyle", "expected"), + [ + ("KEY(customer_id)", "customer_id"), + ("AUTO(KEY(customer_id))", "customer_id"), + ("EVEN", None), + ("ALL", None), + ("AUTO(ALL)", None), + ], +) +def test_diststyle_key_column_parsing(diststyle, expected): + assert _diststyle_key_column(diststyle) == expected + + +def test_diststyle_key_column_parsing_is_case_insensitive(): + """Pins `_DISTSTYLE_KEY_RE`'s `re.IGNORECASE` flag: nothing in this adapter has ever + observed a live cluster's actual casing for this text (see the module docstring's + provenance warning), so the parser must not silently assume upper case. + """ + assert _diststyle_key_column("key(customer_id)") == "customer_id" + assert _diststyle_key_column("Auto(Key(customer_id))") == "customer_id" + + +@pytest.mark.parametrize( + ("diststyle", "expected"), + [ + ("ALL", True), + ("AUTO(ALL)", True), + ("EVEN", False), + ("KEY(customer_id)", False), + ("AUTO(KEY(customer_id))", False), + ], +) +def test_diststyle_is_all_parsing(diststyle, expected): + assert _diststyle_is_all(diststyle) is expected + + +def test_diststyle_is_all_requires_no_key_column_even_when_all_is_a_substring(): + """Pins the second conjunct of `_diststyle_is_all` — `and _diststyle_key_column(...) + is None` — which a real `svv_table_info.diststyle` value never exercises (`ALL` and + `KEY(...)` are mutually exclusive shapes), so nothing in the ordinary parametrize table + above can tell a version missing this conjunct apart from one that has it. A synthetic + diststyle whose key column name itself contains the substring "ALL" forces the two + checks apart: the naive `"ALL" in diststyle.upper()` alone would say True here, and + only the second conjunct correctly says this is a keyed style, not DISTSTYLE ALL. + """ + assert _diststyle_is_all("KEY(all_customers)") is False + + +@pytest.mark.parametrize( + ("rec_type", "recommended_ddl", "expected"), + [ + ("sort key", None, "sortkey"), + ("Sort Key", None, "sortkey"), + ("distribution style", "ALTER TABLE x ALTER DISTKEY y;", "distkey"), + ("distribution style", "ALTER TABLE x ALTER DISTSTYLE ALL;", "diststyle_all"), + ("something else entirely", None, None), + ], +) +def test_advisor_category_classification(rec_type, recommended_ddl, expected): + row = RedshiftAdvisorRow( + relation=R, rec_type=rec_type, current_ddl=None, recommended_ddl=recommended_ddl + ) + assert _advisor_category(row) == expected + + +# --------------------------------------------------------------------------- +# `_quote_ident` — identifier quoting used by every ADV101/102/103/104 statement +# --------------------------------------------------------------------------- + + +def test_quote_ident_doubles_an_embedded_double_quote(): + """Unquoted, or naively quoted, this would either produce invalid DDL or let an + identifier break out of its quoting — the same reasoning `postgres.py`'s identical + helper documents. Nothing in this adapter's own proposal tests happened to exercise a + quote-containing identifier, so the doubling itself was unpinned here.""" + assert _quote_ident('weird"col') == '"weird""col"' + assert _quote_ident("plain") == '"plain"' + + +# --------------------------------------------------------------------------- +# _disclose_advisor_agreement +# --------------------------------------------------------------------------- + + +def test_agreement_is_disclosed_on_the_matching_sortkey_proposal(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + advisor_rows = [ + RedshiftAdvisorRow(relation=R, rec_type="sort key", current_ddl=None, recommended_ddl=None) + ] + updated = _disclose_advisor_agreement(proposals, advisor_rows) + assert len(updated) == 1 + assert "Advisor independently recommends" in updated[0].rationale + # Agreement must not raise confidence past the documented cap. + assert updated[0].confidence is Confidence.MEDIUM + + +def test_agreement_does_not_fire_for_an_unrelated_relation(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + advisor_rows = [ + RedshiftAdvisorRow(relation=R2, rec_type="sort key", current_ddl=None, recommended_ddl=None) + ] + updated = _disclose_advisor_agreement(proposals, advisor_rows) + assert "Advisor independently recommends" not in updated[0].rationale + + +def test_agreement_does_not_fire_for_a_different_category(): + """A distribution-style Advisor row must not be read as agreeing with a SORTKEY + proposal on the same relation.""" + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + advisor_rows = [ + RedshiftAdvisorRow( + relation=R, + rec_type="distribution style", + current_ddl=None, + recommended_ddl="ALTER TABLE x ALTER DISTKEY y;", + ) + ] + updated = _disclose_advisor_agreement(proposals, advisor_rows) + assert "Advisor independently recommends" not in updated[0].rationale + + +def test_agreement_fires_for_diststyle_all_but_not_distkey_proposal(): + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN")} + distkey_proposals = propose_distkey(usage, _facts(R2), physical, min_cost_share=0.1) + diststyle_proposals = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + advisor_rows = [ + RedshiftAdvisorRow( + relation=R2, + rec_type="distribution style", + current_ddl=None, + recommended_ddl="ALTER TABLE x ALTER DISTSTYLE ALL;", + ) + ] + updated_distkey = _disclose_advisor_agreement(distkey_proposals, advisor_rows) + updated_diststyle = _disclose_advisor_agreement(diststyle_proposals, advisor_rows) + assert "Advisor independently recommends" not in updated_distkey[0].rationale + assert "Advisor independently recommends" in updated_diststyle[0].rationale + + +def test_agreement_is_a_noop_with_no_advisor_rows(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + proposals = propose_sortkey(usage, _facts(R), physical, min_cost_share=0.1) + updated = _disclose_advisor_agreement(proposals, []) + assert updated == proposals + + +def test_agreement_does_not_touch_advisor_proposals_or_maintenance_proposals(): + """Agreement disclosure only rewrites ADV101/102/103 rationale — ADV104 and ADV105 + proposals must pass through unmodified, since they carry no `_PROPOSAL_CATEGORY`.""" + maintenance = propose_maintenance({R: _phys(unsorted=50.0)}, _facts(R)) + advisor = propose_advisor( + [ + RedshiftAdvisorRow( + relation=R, rec_type="sort key", current_ddl=None, recommended_ddl=None + ) + ] + ) + advisor_rows = [ + RedshiftAdvisorRow(relation=R, rec_type="sort key", current_ddl=None, recommended_ddl=None) + ] + updated = _disclose_advisor_agreement(maintenance + advisor, advisor_rows) + assert updated == maintenance + advisor + + +# --------------------------------------------------------------------------- +# _collapse_diststyle_all_over_distkey — ADV103 subsumes ADV102 for the same relation +# --------------------------------------------------------------------------- + + +def _distkey_proposal(relation=R2, column="id", confidence=Confidence.MEDIUM): + usage = [_usage(relation, column, ColumnRole.JOIN, cost_share=0.5)] + physical = {relation: _phys(diststyle="EVEN")} + proposals = propose_distkey(usage, _facts(relation), physical, min_cost_share=0.1) + assert len(proposals) == 1 + assert proposals[0].confidence is confidence + return proposals[0] + + +def _diststyle_all_proposal(relation=R2): + usage = [_usage(relation, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {relation: _phys(diststyle="EVEN")} + proposals = propose_diststyle_all( + usage, _facts(relation, row_estimate=1_000), physical, min_cost_share=0.1 + ) + assert len(proposals) == 1 + return proposals[0] + + +def test_collapse_drops_distkey_when_diststyle_all_fires_for_the_same_relation(): + distkey = _distkey_proposal() + diststyle_all = _diststyle_all_proposal() + collapsed = _collapse_diststyle_all_over_distkey([distkey, diststyle_all]) + codes = [p.code for p in collapsed] + assert codes == ["ADV103"] + + +def test_collapse_discloses_the_withheld_distkey_in_the_survivors_rationale(): + distkey = _distkey_proposal(column="customer_id") + diststyle_all = _diststyle_all_proposal() + collapsed = _collapse_diststyle_all_over_distkey([distkey, diststyle_all]) + rationale = collapsed[0].rationale + assert "ADV102 also proposed a DISTKEY on customer_id" in rationale + assert "withheld" in rationale + assert "strictly subsumes any single-column DISTKEY choice" in rationale + + +def test_collapse_folds_the_withheld_distkeys_own_caveats_into_the_survivor(): + """The fold — `if fresh: addition += ...` — is the substance of the collapse, not a + flourish, and deleting it left the whole suite green because the existing disclosure + test asserts only the fixed `addition` string. + + ADV102 is the *only* proposal that carries the `skew_rows` caveat and its own + DISTKEY-specific MEDIUM-cap explanation. Once it is withheld, the surviving ADV103 is + the sole remaining proposal for a full-table rewrite of this relation — so without the + fold the operator loses both the reason the withheld strategy was withheld and the one + measurement that speaks to how risky a distribution change on this table is. + """ + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + physical = {R2: _phys(diststyle="EVEN", skew_rows=3.75)} + (distkey,) = propose_distkey(usage, _facts(R2), physical, min_cost_share=0.1) + (diststyle_all,) = propose_diststyle_all( + usage, _facts(R2, row_estimate=1_000), physical, min_cost_share=0.1 + ) + # Control: these two sentences exist on ADV102 and on nothing else, so finding them on + # the survivor can only be the fold's doing. + assert "skew_rows is 3.75" in distkey.rationale + assert "skew_rows" not in diststyle_all.rationale + assert "good or catastrophic" not in diststyle_all.rationale + + (survivor,) = _collapse_diststyle_all_over_distkey([distkey, diststyle_all]) + assert survivor.code == "ADV103" + assert "skew_rows is 3.75" in survivor.rationale + assert "not the skew this DISTKEY would produce" in survivor.rationale + assert "good or catastrophic" in survivor.rationale + + +def test_collapse_leaves_distkey_alone_when_diststyle_all_does_not_fire_for_it(): + """A relation with only ADV102 (e.g. it failed ADV103's row-count ceiling) must be + untouched — this function only ever removes an ADV102 that has a matching ADV103 for + the *same* relation.""" + distkey_r = _distkey_proposal(relation=R) + diststyle_all_r2 = _diststyle_all_proposal(relation=R2) + collapsed = _collapse_diststyle_all_over_distkey([distkey_r, diststyle_all_r2]) + codes = {p.code for p in collapsed} + assert codes == {"ADV102", "ADV103"} + + +def test_collapse_is_a_noop_with_no_diststyle_all_proposals(): + distkey = _distkey_proposal() + collapsed = _collapse_diststyle_all_over_distkey([distkey]) + assert collapsed == [distkey] + + +def test_collapse_leaves_diststyle_all_alone_with_no_matching_distkey(): + diststyle_all = _diststyle_all_proposal() + collapsed = _collapse_diststyle_all_over_distkey([diststyle_all]) + assert collapsed == [diststyle_all] + + +# --------------------------------------------------------------------------- +# _skipped_for_physical_gap — Spectrum/empty-table ambiguity, counted rather than silent +# --------------------------------------------------------------------------- + + +def test_skipped_for_physical_gap_counts_a_relation_with_a_candidate_role_and_no_facts(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + assert _skipped_for_physical_gap(usage, {}) == 1 + + +def test_skipped_for_physical_gap_counts_join_and_equality_roles_too(): + usage = [ + _usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5), + _usage(R2, "status", ColumnRole.EQUALITY, cost_share=0.5), + ] + assert _skipped_for_physical_gap(usage, {}) == 2 + + +def test_skipped_for_physical_gap_ignores_a_relation_with_facts_present(): + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + physical = {R: _phys(sortkey1="status")} + assert _skipped_for_physical_gap(usage, physical) == 0 + + +def test_skipped_for_physical_gap_ignores_a_relation_with_no_candidate_role(): + """A relation with only GROUP/SORT usage was never going to get a SORTKEY/DISTKEY/ + DISTSTYLE ALL proposal even with facts present, so it must not inflate this count.""" + usage = [_usage(R, "category", ColumnRole.GROUP, cost_share=0.5)] + assert _skipped_for_physical_gap(usage, {}) == 0 + + +# --------------------------------------------------------------------------- +# RedshiftWorkloadAdapter.propose() — dispatcher wiring +# --------------------------------------------------------------------------- + + +class _AdvisorQuerier: + """A minimal `Querier` that answers only CAP_ADVISOR, for testing `propose()`'s own + wiring rather than the pure rule functions above (already covered directly).""" + + def __init__(self, rows=(), *, fail=False): + self._rows = rows + self._fail = fail + self.calls = [] + + def __call__(self, sql, params): + self.calls.append((sql, params)) + if self._fail: + raise RuntimeError("permission denied for svv_alter_table_recommendations") + return self._rows + + +def _bare_aggregation(usage, tables) -> Aggregation: + return Aggregation( + usage=tuple(usage), total_cost_ms=0.0, skipped_unqualifiable=0, tables=frozenset(tables) + ) + + +def test_propose_no_longer_raises_not_implemented_error(): + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier()) + proposals = adapter.propose( + _bare_aggregation([], []), + {}, + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + assert proposals == [] + + +def test_propose_wires_sortkey_and_maintenance_and_sorts_by_confidence(): + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier()) + adapter.physical_facts = { + R: _phys(sortkey1="status", unsorted=50.0), + } + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + proposals = adapter.propose( + _bare_aggregation(usage, [R]), + _facts(R), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + codes = [p.code for p in proposals] + assert "ADV101" in codes + assert "ADV104" in codes + # HIGH-confidence ADV104 (VACUUM) must sort ahead of MEDIUM-confidence ADV101. + assert codes.index("ADV104") < codes.index("ADV101") + + +def test_propose_fetches_advisor_rows_scoped_to_the_requested_relations(): + querier = _AdvisorQuerier(rows=[("db", "public", "orders", "sort key", None, "ALTER TABLE x;")]) + adapter = RedshiftWorkloadAdapter(querier=querier) + adapter.schemas = ("public",) + proposals = adapter.propose( + _bare_aggregation([], [R]), + {}, + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + assert len(proposals) == 1 + assert proposals[0].code == "ADV105" + assert len(querier.calls) == 1 + _sql, params = querier.calls[0] + assert params == (["public"], ["orders"]) + + +def test_propose_drops_advisor_rows_for_relations_not_in_scope(): + """Over-fetch guard: the statement filters on bare table names, so a same-named table + in a schema that was not requested must not leak into the returned proposals.""" + querier = _AdvisorQuerier( + rows=[("db", "other_schema", "orders", "sort key", None, "ALTER TABLE x;")] + ) + adapter = RedshiftWorkloadAdapter(querier=querier) + proposals = adapter.propose( + _bare_aggregation([], [R]), + {}, + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + assert proposals == [] + + +def test_propose_records_a_denied_advisor_capability_in_degraded_rather_than_raising(): + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier(fail=True)) + proposals = adapter.propose( + _bare_aggregation([], [R]), + {}, + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + assert proposals == [] + assert len(adapter.degraded) == 1 + assert adapter.degraded[0][0] == CAP_ADVISOR + + +def test_propose_disclosed_agreement_survives_the_full_dispatcher(): + querier = _AdvisorQuerier(rows=[("db", "public", "orders", "sort key", None, "ALTER TABLE x;")]) + adapter = RedshiftWorkloadAdapter(querier=querier) + adapter.schemas = ("public",) + adapter.physical_facts = {R: _phys(sortkey1="status")} + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + proposals = adapter.propose( + _bare_aggregation(usage, [R]), + _facts(R), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + by_code = {p.code: p for p in proposals} + assert "ADV101" in by_code and "ADV105" in by_code + assert "Advisor independently recommends" in by_code["ADV101"].rationale + # The Advisor row must still stand as its own, separate, unmodified proposal. + assert by_code["ADV105"].confidence is Confidence.HIGH + + +def test_propose_wires_distkey_when_the_table_is_too_large_for_diststyle_all(): + """Isolates ADV102 in the full dispatcher: the table is over ADV103's row-count + ceiling, so ADV103 does not fire and cannot mask ADV102's own wiring — removing + `propose_distkey(...)` from `propose()` must redden this test on its own.""" + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier()) + adapter.physical_facts = {R: _phys(diststyle="EVEN")} + usage = [_usage(R, "customer_id", ColumnRole.JOIN, cost_share=0.5)] + proposals = adapter.propose( + _bare_aggregation(usage, [R]), + _facts(R, row_estimate=MAX_ROWS_FOR_DISTSTYLE_ALL + 1), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + codes = [p.code for p in proposals] + assert codes == ["ADV102"] + + +def test_propose_wires_diststyle_all_and_suppresses_distkey_for_the_same_relation(): + """Isolates ADV103 in the full dispatcher, and proves the collapse (finding 4) runs + end to end: a small, hot-join dimension makes both ADV102 and ADV103 fire from their + own rule functions, but only ADV103 must survive `propose()`. Removing + `propose_diststyle_all(...)` from `propose()` reddens this test two ways — "ADV103" no + longer appears, and "ADV102" reappears because nothing is left to suppress it.""" + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier()) + adapter.physical_facts = {R2: _phys(diststyle="EVEN")} + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + proposals = adapter.propose( + _bare_aggregation(usage, [R2]), + _facts(R2, row_estimate=1_000), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + codes = [p.code for p in proposals] + assert codes == ["ADV103"] + assert "ADV102 also proposed a DISTKEY" in proposals[0].rationale + + +def test_propose_collapses_before_disclosing_agreement(): + """`propose()`'s documented pass order is load-bearing: reversing it fabricates an + Advisor agreement Advisor never made. + + The fixture is the worst case for the reversal — a small, hot-join dimension, so ADV102 + and ADV103 both fire for the same relation and the collapse withholds ADV102, plus one + Advisor row whose category is `distkey` (a distribution-key recommendation, not + `DISTSTYLE ALL`). Shipped order: the collapse runs first, ADV102 is gone, and the + agreement finds no ADV102 to attach to — ADV103's category is `diststyle_all`, which + Advisor did *not* recommend, so no agreement is claimed. Reversed: the agreement + sentence lands on ADV102, the collapse then folds ADV102's fresh sentences (including + that one) into the surviving ADV103, and ADV103 claims Advisor agrees with a `DISTSTYLE + ALL` change Advisor never recommended. Presenting Advisor's opinion as ours is the one + thing ADV105's whole design exists to prevent, and it is invisible to every other test + because both orders produce the same *set* of codes. + """ + querier = _AdvisorQuerier( + rows=[ + ( + "db", + R2.schema, + R2.table, + "distribution key", + None, + f'ALTER TABLE "{R2.schema}"."{R2.table}" ALTER DISTKEY "id";', + ) + ] + ) + adapter = RedshiftWorkloadAdapter(querier=querier) + adapter.schemas = (R2.schema,) + adapter.physical_facts = {R2: _phys(diststyle="EVEN")} + usage = [_usage(R2, "id", ColumnRole.JOIN, cost_share=0.5)] + proposals = adapter.propose( + _bare_aggregation(usage, [R2]), + _facts(R2, row_estimate=1_000), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + by_code = {p.code: p for p in proposals} + # The collapse ran: ADV102 is withheld, ADV103 survives, ADV105 relays Advisor's row. + assert set(by_code) == {"ADV103", "ADV105"} + assert "ADV102 also proposed a DISTKEY" in by_code["ADV103"].rationale + # And the surviving proposal claims no agreement, because there is none to claim. + assert "independently recommends the same kind of change" not in by_code["ADV103"].rationale + + +def test_propose_fetches_advisor_rows_for_relations_reached_only_through_table_facts(): + """ADV105's scope must not be narrower than ADV104's. + + `cli.py` fetches table facts for `aggregation.tables | star_tables(...)`, so a relation + reached only by a `SELECT *` arrives in `facts`/`physical_facts` without ever appearing + in `aggregation.tables` — and ADV104 (which iterates `physical_facts`) will happily + propose VACUUM/ANALYZE for it. Scoping the Advisor query to `aggregation.tables` alone + meant that same relation could never get an ADV105 relay or an agreement disclosure, so + the cluster's own opinion about it was dropped on the floor. The two rules now see the + same set. + """ + querier = _AdvisorQuerier( + rows=[("db", R2.schema, R2.table, "sort key", None, "ALTER TABLE x;")] + ) + adapter = RedshiftWorkloadAdapter(querier=querier) + adapter.schemas = (R2.schema,) + adapter.physical_facts = {R2: _phys(unsorted=50.0)} + proposals = adapter.propose( + # R2 is deliberately absent from `aggregation.tables` — it is only in `facts`. + _bare_aggregation([], []), + _facts(R2), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + codes = {p.code for p in proposals} + assert "ADV104" in codes, "control: ADV104 already reaches a star-only relation" + assert "ADV105" in codes + _sql, params = querier.calls[0] + assert params == ([R2.schema], [R2.table]) + + +def test_propose_discloses_the_physical_facts_gap_in_degraded(): + """Finding 5: a relation with a hot predicate but no `physical_facts` entry must not + simply vanish — `propose()` must count it and disclose the count through the same + `self.degraded` channel a denied capability uses, so it reaches the coverage line, the + JSON payload and the markdown report.""" + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier()) + adapter.physical_facts = {} + usage = [ + _usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5), + _usage(R2, "id", ColumnRole.JOIN, cost_share=0.5), + ] + adapter.propose( + _bare_aggregation(usage, [R, R2]), + {}, + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + gaps = [reason for cap, reason in adapter.degraded if cap == DEGRADATION_PHYSICAL_FACTS_GAP] + assert len(gaps) == 1 + assert "2 relation(s)" in gaps[0] + assert "Spectrum" in gaps[0] + + +def test_propose_does_not_disclose_a_physical_facts_gap_when_there_is_none(): + """Guards the test above: with `physical_facts` covering every relation involved, no + `DEGRADATION_PHYSICAL_FACTS_GAP` entry should appear at all.""" + adapter = RedshiftWorkloadAdapter(querier=_AdvisorQuerier()) + adapter.physical_facts = {R: _phys(sortkey1="status")} + usage = [_usage(R, "created_at", ColumnRole.RANGE, cost_share=0.5)] + adapter.propose( + _bare_aggregation(usage, [R]), + _facts(R), + Workload(stats=(), window_description="w"), + min_cost_share=0.1, + ) + assert not any(cap == DEGRADATION_PHYSICAL_FACTS_GAP for cap, _ in adapter.degraded) + + +# --------------------------------------------------------------------------- +# render_ddl (Task 7) — a far louder header than Postgres's, ADV105 attribution in the +# file itself, and the same hostile-identifier protection Postgres's render_ddl gives. +# --------------------------------------------------------------------------- + + +def _rewrite_proposal(code="ADV101", ddl='ALTER TABLE "public"."orders" ALTER SORTKEY ("c");'): + return Proposal( + code=code, + title=f"Consider a change on {R}", + rationale="hot predicate", + evidence={"schema": R.schema, "table": R.table, "cost_share": 0.5}, + confidence=Confidence.MEDIUM, + ddl=ddl, + note="ALTER SORTKEY rewrites the entire table: ... There is no CONCURRENTLY equivalent.", + ) + + +def _advisor_render_proposal(): + return Proposal( + code="ADV105", + title="Amazon Redshift Advisor recommends a sort key change", + rationale="Amazon Redshift Advisor recommends this change based on its own analysis.", + evidence={"schema": R.schema, "table": R.table, "source": "amazon_redshift_advisor"}, + confidence=Confidence.HIGH, + ddl='ALTER TABLE "public"."orders" ALTER SORTKEY ("created_at");', + note=( + "Source: Amazon Redshift Advisor, not sqlquality's own analysis — this " + "statement was generated by Redshift itself and is relayed verbatim." + ), + ) + + +def _maintenance_render_proposal(): + return Proposal( + code="ADV104", + title=f"Run VACUUM on {R}", + rationale="unsorted region is large", + evidence={"schema": R.schema, "table": R.table}, + confidence=Confidence.HIGH, + ddl='VACUUM "public"."orders";', + note="VACUUM is heavy ... it does not need a maintenance-window lock.", + ) + + +def test_render_ddl_header_calls_out_adv101_102_103_as_full_table_rewrites(): + script = RedshiftWorkloadAdapter().render_ddl([]) + assert "ADV101" in script + assert "ADV102" in script + assert "ADV103" in script + assert "REWRITE THE ENTIRE TABLE" in script + + +def test_render_ddl_header_says_there_is_no_concurrently_equivalent(): + """Postgres's header can recommend CONCURRENTLY for exactly this hazard; Redshift's + header must say plainly that no such escape exists here.""" + script = RedshiftWorkloadAdapter().render_ddl([]) + assert "CONCURRENTLY" in script + assert "NO CONCURRENTLY EQUIVALENT" in script + + +def test_render_ddl_header_says_schedule_a_maintenance_window(): + """Pinned as one contiguous phrase, not merely "the word 'maintenance' occurs + somewhere" — the ADV104 sentence a few lines down also says "maintenance window" for + the opposite reason (it does *not* need one), so a weaker check would still pass with + the DANGER section's own "schedule a maintenance window" instruction deleted.""" + script = RedshiftWorkloadAdapter().render_ddl([]) + assert "SCHEDULE each one for a maintenance" in script + assert "window; do not run it ad hoc" in script + + +def test_render_ddl_header_names_adv104_as_the_one_non_rewrite(): + script = RedshiftWorkloadAdapter().render_ddl([]) + assert "ADV104" in script + # The header's ADV104 sentence must appear near language distinguishing it from a + # rewrite, not merely mention the code in passing. + header = script.split("-- ADV101")[0] if "-- ADV101" in script else script + assert ( + "not a" in header.lower() + or "not the" in header.lower() + or "only statement" in header.lower() + ) + + +def test_render_ddl_header_is_louder_than_postgres_header(): + """The one property this whole task exists to deliver: the Redshift header must say + materially more than Postgres's does about the danger of the DDL below it.""" + from sqlquality.workload.postgres import PostgresWorkloadAdapter + + postgres_header = PostgresWorkloadAdapter().render_ddl([]) + redshift_header = RedshiftWorkloadAdapter().render_ddl([]) + for word in ("REWRITE", "HOURS", "DANGER", "SCHEDULE"): + assert word not in postgres_header + assert word in redshift_header + + +def test_render_ddl_emits_the_statement_with_code_confidence_and_cost_share(): + script = RedshiftWorkloadAdapter().render_ddl([_rewrite_proposal()]) + assert 'ALTER TABLE "public"."orders" ALTER SORTKEY ("c");' in script + assert "-- ADV101 [medium, 50.0% of workload cost]" in script + + +def test_render_ddl_prints_the_proposals_note_above_its_statement(): + proposal = _rewrite_proposal() + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert script.index("-- ALTER SORTKEY rewrites the entire table") < script.index(proposal.ddl) + + +def test_render_ddl_skips_a_proposal_with_no_ddl(): + proposal = Proposal( + code="ADV104", + title="advisory only", + rationale="r", + evidence={}, + confidence=Confidence.HIGH, + ddl=None, + ) + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert "advisory only" not in script + + +def test_render_ddl_with_no_proposals_still_explains_itself(): + script = RedshiftWorkloadAdapter().render_ddl([]) + assert "No DDL proposals" in script + + +def test_render_ddl_tolerates_a_missing_or_non_numeric_cost_share(): + for evidence in ({}, {"cost_share": "nope"}, {"cost_share": True}): + proposal = Proposal( + code="ADV104", + title="t", + rationale="r", + evidence=evidence, + confidence=Confidence.HIGH, + ddl='VACUUM "public"."orders";', + ) + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert "-- ADV104 [high]" in script + assert "%" not in script.split("-- ADV104")[1].split("\n")[0] + + +def _uncommented(script: str) -> list[str]: + """Every non-blank line that is not a `--` comment. See the identical helper in + `tests/test_workload_rules.py` — the one property this whole file exists to prove.""" + return [line for line in script.splitlines() if line.strip() and not line.startswith("--")] + + +def test_render_ddl_never_emits_a_bare_uncommented_line_for_a_hostile_identifier(): + """The same protection `PostgresWorkloadAdapter.render_ddl` gives, reused rather than + reimplemented: a title or ddl carrying an embedded newline, `\\r`, `--` or `;` must never + leave a bare, executable-looking line in a Redshift script either.""" + hostile = 'ALTER TABLE "public"."orders"\nDROP TABLE users; --\n ALTER SORTKEY ("c");' + proposal = Proposal( + code="ADV101", + title="line1\nline2 -- injected; DROP TABLE users;\r", + rationale="r", + evidence={"schema": "public", "table": "orders", "cost_share": 0.4}, + confidence=Confidence.MEDIUM, + ddl=hostile, + ) + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert "NOT RENDERED" in script + assert _uncommented(script) == [], script + + +def test_render_ddl_emits_a_pre_commented_multiline_block_verbatim(): + """`_is_fully_commented`, reused rather than reimplemented: a `ddl` that already reads + as a `--`-commented, multi-line disclosure on every line is not the identifier-with-a- + line-break hazard and must be emitted as-is, with its usual header.""" + block = "-- ADV999: some future multi-line disclosure\n-- line two\n-- line three" + proposal = Proposal( + code="ADV999", + title="hypothetical config-shaped proposal", + rationale="r", + evidence={"cost_share": 0.3}, + confidence=Confidence.HIGH, + ddl=block, + ) + script = RedshiftWorkloadAdapter().render_ddl([proposal]) + assert "NOT RENDERED" not in script + assert "-- -- ADV999" not in script, "must not be double-commented" + assert script.count("some future multi-line disclosure") == 1 + + +def test_render_ddl_marks_adv105_in_its_own_header_line_not_only_in_note(): + """ADV105's DDL is Redshift Advisor's, not sqlquality's, and that must be visible in the + per-statement header line itself — someone skimming only header lines, never the full + `note` text, must still be able to tell an Advisor statement apart from an ADV101/102/103 + statement sqlquality inferred.""" + script = RedshiftWorkloadAdapter().render_ddl([_advisor_render_proposal()]) + header_line = next(line for line in script.splitlines() if line.startswith("-- ADV105 [")) + assert "Advisor" in header_line + assert "not sqlquality" in header_line + + +def test_render_ddl_does_not_mark_non_advisor_codes_with_the_advisor_attribution(): + """Guards the test above: the attribution marker must be specific to ADV105, not a + blanket suffix every statement gets regardless of its actual source.""" + script = RedshiftWorkloadAdapter().render_ddl([_rewrite_proposal(code="ADV101")]) + header_line = next(line for line in script.splitlines() if line.startswith("-- ADV101 [")) + assert "Advisor" not in header_line + + +def test_render_ddl_full_script_with_all_five_codes_marks_only_advisor(): + """An end-to-end sanity check with one proposal per code: the rewrite trio, the + maintenance rule, and Advisor together in one script, each still individually correct.""" + proposals = [ + _rewrite_proposal(code="ADV101"), + _rewrite_proposal(code="ADV102", ddl='ALTER TABLE "public"."orders" ALTER DISTKEY "c";'), + _rewrite_proposal(code="ADV103", ddl='ALTER TABLE "public"."orders" ALTER DISTSTYLE ALL;'), + _maintenance_render_proposal(), + _advisor_render_proposal(), + ] + script = RedshiftWorkloadAdapter().render_ddl(proposals) + for code in ("ADV101", "ADV102", "ADV103", "ADV104", "ADV105"): + assert f"-- {code} [" in script + advisor_lines = [line for line in script.splitlines() if "Advisor" in line and "[" in line] + assert len(advisor_lines) == 1 + assert advisor_lines[0].startswith("-- ADV105") + + +# --------------------------------------------------------------------------- +# dbt enrichment interaction (Task 7) — a Redshift table-rewrite proposal on a dbt-managed +# relation. Batch 3a's `enrich_proposals` keys its rewrite-to-config path on a `CREATE +# INDEX` DDL prefix, which never matches Redshift's `ALTER TABLE` statements — but its +# generic fallback branch (built, per its own docstring, for exactly "some other statement +# for a relation dbt owns") already recognises anything that is neither `CREATE INDEX` nor +# `DROP INDEX` and attaches a dbt warning to both `rationale` and `note`. That branch had +# zero test coverage before this task — no Postgres rule's DDL ever reached it — so these +# tests are the first proof it actually behaves as its docstring claims for Redshift's +# proposals, and a regression test for the note-clobbering bug this task found and fixed in +# `sqlquality/workload/dbt.py` (`_prepend_note`). +# --------------------------------------------------------------------------- + + +def _dbt_context_for(relation: Relation, *, materialized: str = "table") -> DbtContext: + model = ModelNode( + unique_id=f"model.proj.{relation.table}", + name=relation.table, + resource_type="model", + materialized=materialized, + compiled_code=None, + relation_name=f"{relation.schema}.{relation.table}", + depends_on=[], + config={}, + ) + return DbtContext(models={relation: model}, dropped_collisions=0) + + +def test_dbt_warning_is_attached_to_a_redshift_rewrite_proposal_on_a_dbt_managed_relation(): + """A dbt-managed Redshift model must not get a silent table-rewrite proposal: the next + `dbt run` can undo hours of rewrite work with no warning at all if this path is broken. + """ + proposal = _rewrite_proposal(code="ADV101") + context = _dbt_context_for(R) + (enriched,) = enrich_proposals([proposal], context) + assert enriched.ddl == proposal.ddl # the statement itself is untouched, only disclosed + assert "dbt model" in enriched.rationale + assert "may not outlive the next rebuild" in enriched.rationale + assert "dbt WARNING" in (enriched.note or "") + assert "reapply" in (enriched.note or "").lower() + + +def test_dbt_enrichment_leaves_a_non_dbt_relation_proposal_untouched(): + """Control for the test above: a relation dbt does not manage must not be warned about + at all — this is not a blanket suffix, it depends on `DbtContext.model_for` actually + finding a match.""" + proposal = _rewrite_proposal(code="ADV101") + context = _dbt_context_for(R2) # a *different* relation + (enriched,) = enrich_proposals([proposal], context) + assert enriched.rationale == proposal.rationale + assert enriched.note == proposal.note + + +def test_dbt_enrichment_preserves_the_advisor_attribution_note_for_a_dbt_managed_relation(): + """Regression test for a note-clobbering bug this task found: `enrich_proposals`'s + generic fallback used to build `note` as only the dbt warning, discarding ADV105's own + Amazon-Redshift-Advisor attribution outright. Since `render_ddl` never emits `rationale`, + that attribution existed *only* in `note` — so a dbt-managed relation that Advisor also + has a recommendation for would have shipped a DDL script whose only remaining trace of + the statement's source was a per-statement header marker, with the descriptive sentence + silently gone. Both must survive: the dbt warning is additive, not a replacement. + """ + proposal = _advisor_render_proposal() + context = _dbt_context_for(R) + (enriched,) = enrich_proposals([proposal], context) + assert "Amazon Redshift Advisor" in (enriched.note or "") + assert "not sqlquality's own analysis" in (enriched.note or "") + assert "dbt WARNING" in (enriched.note or "") + + +def test_dbt_enrichment_preserved_note_still_renders_safely_in_the_ddl_script(): + """The two notes concatenated by `_prepend_note` must still pass through `render_ddl`'s + hostile-identifier guard cleanly — this is a integration check that the fix does not + itself introduce a bare line.""" + proposal = _advisor_render_proposal() + context = _dbt_context_for(R) + (enriched,) = enrich_proposals([proposal], context) + script = RedshiftWorkloadAdapter().render_ddl([enriched]) + assert _uncommented(script) == [enriched.ddl] + + +def test_dbt_enrichment_preserves_an_existing_note_on_a_drop_index_proposal_too(): + """`_prepend_note` fixes the same note-clobbering shape on the `DROP INDEX` branch of + `dbt.py`'s `_classify`, not only the generic fallback ADV104/ADV105 land in. No Redshift + rule emits `DROP INDEX` and no Postgres rule sets `note` before reaching that branch + today, so this uses a synthetic, engine-agnostic `Proposal` to prove the fix directly + rather than leaving that branch's preservation unverified — a fix with no test pinning + it is exactly the kind of thing this feature's mutation-testing discipline exists to + catch.""" + proposal = Proposal( + code="ADV002", + title="Unused index", + rationale="never scanned", + evidence={"schema": R.schema, "table": R.table}, + confidence=Confidence.HIGH, + ddl='DROP INDEX "public"."idx_unused";', + note="a pre-existing caveat that must not be discarded", + ) + context = _dbt_context_for(R) + (enriched,) = enrich_proposals([proposal], context) + assert "a pre-existing caveat that must not be discarded" in (enriched.note or "") + assert "dbt WARNING" in (enriched.note or "") diff --git a/tests/test_workload_session.py b/tests/test_workload_session.py new file mode 100644 index 0000000..5cc789c --- /dev/null +++ b/tests/test_workload_session.py @@ -0,0 +1,226 @@ +"""Unit tests for `workload/session.py`'s `open_session`, the mechanism both the Postgres +and Redshift adapters' `connect()` share. + +`test_workload_postgres.py` and `test_workload_redshift.py` each exercise this through +their own adapter and are not to be modified for this extraction, but neither happens to +construct a fake driver where the read-only statement itself fails *and* the caller asked +for `read_only_required=True` — Postgres's fake cursor never fails on that statement, and +Redshift's tests only exercise `read_only_required=False`. That combination is exactly the +Postgres invariant this module must keep ("a refused read-only statement aborts the +connection like any other setup failure"), so it is pinned here directly against the +shared helper rather than left to be an accident of which adapter happens to test it. +""" + +from __future__ import annotations + +import sys +import types + +import pytest + +from sqlquality.models import ConnectionParams +from sqlquality.workload.postgres import PostgresWorkloadAdapter +from sqlquality.workload.session import READ_ONLY_SQL, open_session + + +class _FakeCursor: + def __init__(self, log: list[tuple], *, fail_on: frozenset[str] = frozenset()) -> None: + self._log = log + self._fail_on = fail_on + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def execute(self, sql, params=None): + self._log.append((sql, params)) + if sql in self._fail_on: + raise RuntimeError(f"ERROR: {sql!r} refused") + + def fetchall(self): + return [] + + +class _FakeConnection: + def __init__(self, *, fail_on: frozenset[str] = frozenset()) -> None: + self.log: list[tuple] = [] + self._fail_on = fail_on + + def cursor(self): + return _FakeCursor(self.log, fail_on=self._fail_on) + + +def _fake_psycopg(seen: dict, *, fail_on: frozenset[str] = frozenset()): + module = types.ModuleType("psycopg") + + def connect(conninfo, **kwargs): + seen["conninfo"] = conninfo + seen["connection"] = _FakeConnection(fail_on=fail_on) + return seen["connection"] + + module.connect = connect # type: ignore[attr-defined] + return module + + +def test_a_required_read_only_statement_that_fails_aborts_the_connection(): + """The Postgres invariant: `read_only_required=True` means a refusal is a setup + failure like any other, not a degradation. Neither adapter's own test suite happens + to force the read-only statement itself to fail under `required=True` — Postgres's + fake cursor always lets it through, and Redshift's tests only exercise + `required=False` — so this is the one place that combination is pinned. + """ + seen: dict = {} + psycopg = _fake_psycopg(seen, fail_on=frozenset({READ_ONLY_SQL})) + with pytest.raises(ConnectionError) as exc: + open_session( + psycopg=psycopg, + conninfo_factory=lambda: "postgresql:///x", + secrets=(), + timeout_s=30, + min_timeout_s=1, + max_timeout_s=3600, + read_only_sql=READ_ONLY_SQL, + read_only_required=True, + ) + assert "refused" in str(exc.value) + # The statement_timeout statement must never run once the required setup step failed. + assert seen["connection"].log == [(READ_ONLY_SQL, None)] + + +def test_a_non_required_read_only_statement_that_fails_degrades_instead(): + """The Redshift invariant, direct on the helper: the same failure, under + `read_only_required=False`, is caught, scrubbed, and returned — not raised — and + setup continues to arm the statement timeout regardless.""" + seen: dict = {} + psycopg = _fake_psycopg(seen, fail_on=frozenset({READ_ONLY_SQL})) + query, degradation = open_session( + psycopg=psycopg, + conninfo_factory=lambda: "postgresql:///x", + secrets=(), + timeout_s=30, + min_timeout_s=1, + max_timeout_s=3600, + read_only_sql=READ_ONLY_SQL, + read_only_required=False, + ) + assert degradation is not None + assert "could not be proven read-only" in degradation + assert seen["connection"].log == [ + (READ_ONLY_SQL, None), + ("SELECT set_config('statement_timeout', %s, false)", ("30000ms",)), + ] + assert query is not None + + +def test_a_required_read_only_statement_that_succeeds_reports_no_degradation(): + seen: dict = {} + psycopg = _fake_psycopg(seen) + query, degradation = open_session( + psycopg=psycopg, + conninfo_factory=lambda: "postgresql:///x", + secrets=(), + timeout_s=30, + min_timeout_s=1, + max_timeout_s=3600, + read_only_sql=READ_ONLY_SQL, + read_only_required=True, + ) + assert degradation is None + assert query is not None + + +def test_conninfo_factory_runs_inside_the_scrubbing_envelope(): + """A `conninfo_factory` that raises must be scrubbed exactly like a connect failure — + proving it runs *inside* `open_session`'s try, not before the call.""" + module = types.ModuleType("psycopg") + module.connect = lambda *a, **k: (_ for _ in ()).throw( # pragma: no cover - unreachable + AssertionError("must not reach psycopg.connect") + ) + + def exploding_factory() -> str: + raise RuntimeError("invalid option: password=hunter2") + + with pytest.raises(ConnectionError) as exc: + open_session( + psycopg=module, + conninfo_factory=exploding_factory, + secrets=("hunter2",), + timeout_s=30, + min_timeout_s=1, + max_timeout_s=3600, + read_only_sql=READ_ONLY_SQL, + read_only_required=True, + ) + assert "hunter2" not in str(exc.value) + assert "***" in str(exc.value) + assert exc.value.__context__ is None + + +def test_postgres_adapter_aborts_when_its_read_only_statement_is_refused(monkeypatch): + """Pins `postgres.py`'s own call site, not just the shared helper. + + `postgres.py`'s `connect()` passes `read_only_required=True` as a literal at its one + call to `open_session`. Nothing forces that literal to stay `True`: flip it to + `False` and every existing Postgres unit test still passes, because none of them + makes the read-only statement itself fail — the fake cursor those tests use always + lets it through. Under that flip, Postgres would continue *silently* on a refused + read-only statement, discarding the returned degradation, which is exactly the "we + never write" promise on the engine that actually ships. This goes through + `PostgresWorkloadAdapter.connect()` itself (via a `psycopg` planted in `sys.modules`, + since that is how the adapter imports it) rather than through `open_session` + directly, so a regression at the call site — not just in the helper — fails it. + """ + + class _FailingCursor: + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def execute(self, sql, params=None): + if sql == READ_ONLY_SQL: + raise RuntimeError("ERROR: read-only not supported in this configuration") + + def fetchall(self): + return [] + + class _FailingConnection: + def cursor(self): + return _FailingCursor() + + module = types.ModuleType("psycopg") + module.connect = lambda conninfo, **kwargs: _FailingConnection() # type: ignore[attr-defined] + module.conninfo = types.SimpleNamespace( # type: ignore[attr-defined] + make_conninfo=lambda **kw: " ".join(f"{k}={v}" for k, v in kw.items()) + ) + monkeypatch.setitem(sys.modules, "psycopg", module) + + adapter = PostgresWorkloadAdapter() + params = ConnectionParams(engine="postgres", dsn="postgresql:///x", fields={}, source="--dsn") + with pytest.raises(ConnectionError) as exc: + adapter.connect(params, 30) + assert "read-only" in str(exc.value) + # Not silently continued: no querier was ever installed. + assert adapter._query is None + + +def test_the_timeout_is_clamped_with_the_caller_supplied_bounds(): + seen: dict = {} + psycopg = _fake_psycopg(seen) + open_session( + psycopg=psycopg, + conninfo_factory=lambda: "postgresql:///x", + secrets=(), + timeout_s=99999, + min_timeout_s=1, + max_timeout_s=120, + read_only_sql=READ_ONLY_SQL, + read_only_required=True, + ) + assert seen["connection"].log[1] == ( + "SELECT set_config('statement_timeout', %s, false)", + ("120000ms",), + )