diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e27adf2..599d5fe 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -22,3 +22,69 @@ jobs: - run: uv run mypy src/sqlquality - run: uv run pytest - run: uv build + + # The `check` job above installs every extra, so it can never catch a test that + # quietly starts requiring psycopg or a running database. That matters because the + # project promises a plain `uv run pytest` works with no extras and no Docker — the + # first thing a new contributor does. A test that acquires such a dependency turns + # into a *skip*, which reads as success in a green run, so this job fails on any + # skip rather than only on a failure. + # + # Deliberately not a matrix: the invariant is about dependencies, not interpreter + # versions, so one Python is enough and four would just be slower. + no-extras: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: astral-sh/setup-uv@v5 + with: + python-version: "3.11" + # No --all-extras, and no service container: this is the bare install. + - run: uv sync + - name: Run the default suite and fail on any skip + run: | + set -o pipefail + # One run, not two. `pytest` exits 0 on skips, so the summary has to be + # inspected — and it has to be a real run: a skip decided at runtime (an + # ImportError inside a fixture) never shows up in --collect-only. + # -rs lists each skip with its reason, so a failure here names the test + # instead of only reporting a count. --strict-markers catches a typo'd + # `integration` marker, which would otherwise silently select the test. + uv run pytest -q -rs --strict-markers 2>&1 | tee result.txt + if grep -qE '[0-9]+ skipped' result.txt; then + echo "::error::The default test suite must not skip anything without extras." + echo "A test acquired a dependency on an extra or on Docker. Either mark it" + echo "'integration' so it is deselected, or remove the dependency." + grep -E '^SKIPPED' result.txt || true + exit 1 + fi + + # `uv sync` installs what uv.lock pins, so every job above tests exactly one point in the + # dependency ranges pyproject declares. A user running `pip install sqlquality` gets the + # *newest* release satisfying those ranges instead — and that difference has already shipped + # a real bug: sqlglot changed `IS NOT NULL` from `Not(Is(...))` to `Is(..., negate=True)` + # within `>=30.12,<31`, which inverted the null-check polarity and made ADV004 emit a partial + # index over exactly the wrong rows. The lockfile hid it from CI completely. + # + # This job resolves to the highest allowed versions, so an upstream release that breaks a + # declared range fails here rather than in a user's generated DDL. It is blocking on purpose: + # the point is to find out before a release, and the remedy is always available — handle both + # behaviours, or narrow the range in pyproject. If upstream churn makes it noisy, add + # `continue-on-error: true` rather than deleting it. + highest-deps: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: astral-sh/setup-uv@v5 + with: + python-version: "3.12" + # An explicit venv rather than `--system`: the runner's Python is Debian-managed and + # refuses `pip install` (PEP 668), and `uv run` is no good either because it would sync + # from the lockfile — the very thing this job exists to bypass. So the venv is built and + # invoked directly. + - run: uv venv .venv-highest + # Extras included, so the psycopg-dependent paths are exercised too. + - run: uv pip install --python .venv-highest --resolution highest -e '.[postgres]' pytest + - name: Record what was resolved + run: uv pip list --python .venv-highest + - run: .venv-highest/bin/python -m pytest -q diff --git a/CHANGELOG.md b/CHANGELOG.md index 81acd25..fb827a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 could no longer satisfy the hot query's `ORDER BY`. - ADV003 is scoped to the tables the workload was observed using, like ADV002 — it no longer proposes `DROP INDEX` for a relation the run never analysed. + +### Fixed + +- `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: + ADV004 turns these roles directly into a partial index's `WHERE` clause, so it proposed + `WHERE col IS NULL` for a workload filtering `WHERE col IS NOT NULL` — an index over exactly + the complement of the intended rows. `uv.lock` pins 30.12, so development and CI never saw + it while any fresh `pip install sqlquality` resolved a newer 30.x and did. CI now also runs + the suite against the highest versions the declared dependency ranges allow. - `advise` unwraps `DECLARE ... CURSOR FOR` and `COPY (...) TO` reads to their inner query before filtering, so server-side-cursor and `COPY`-based workloads (what psycopg2, Django and SQLAlchemy emit for large result sets) reach the analysis diff --git a/src/sqlquality/workload/aggregate.py b/src/sqlquality/workload/aggregate.py index 73c05fa..4add688 100644 --- a/src/sqlquality/workload/aggregate.py +++ b/src/sqlquality/workload/aggregate.py @@ -25,11 +25,16 @@ def _identifier_pattern(name: str) -> re.Pattern[str]: """Compiled whole-identifier matcher for one name, compiled once per name. - Callers such as ADV006's wide-table detection and the expression-index disclosure in - `postgres.py` test one name against many statements (or vice versa), and a schema with - many tables was recompiling the same handful of name patterns over and over, thrashing - `re`'s own pattern cache. Caching by name here means each identifier is compiled once - regardless of how many times it is checked. + The callers are the expression-index disclosures in ADV001, ADV007 and ADV008, which test + one column name against every expression index on a relation. A relation with several + expression indexes was recompiling the same name pattern once per index, thrashing `re`'s + own pattern cache; caching by name means each identifier is compiled once regardless of how + many times it is checked. + + ADV006's wide-table detection used to be the main caller and no longer is: it parses each + statement and resolves its tables through `resolve_relation` instead, because text matching + cannot see a schema qualifier and so attributed `select * from public.orders` to a + same-named table in another schema. """ return re.compile(rf"\b{re.escape(name)}\b") diff --git a/src/sqlquality/workload/extract.py b/src/sqlquality/workload/extract.py index f4b2486..aa4e3f2 100644 --- a/src/sqlquality/workload/extract.py +++ b/src/sqlquality/workload/extract.py @@ -31,6 +31,30 @@ class AmbiguousRelation(UnqualifiableQuery): """ +def _is_negated(node: exp.Is) -> bool: + """True when an ``IS`` predicate is the negated form, under either sqlglot encoding. + + sqlglot changed how it represents `IS NOT NULL` *within the version range this package + declares* (`sqlglot>=30.12,<31`): + + * up to 30.12 it wraps the node — ``Not(Is(col, Null()))`` — so polarity lives on the parent + * from 30.13 it sets a flag on the node itself — ``Is(col, Null(), negate=True)`` — and no + ``exp.Not`` appears in the tree at all + + Reading only the parent silently inverted every `IS NOT NULL` into a `NULL_CHECK` on the + newer parse, which is not a cosmetic misclassification: ADV004 turns these roles straight + into a partial index's `WHERE` clause, so it emitted `WHERE col IS NULL` for a workload that + filters `IS NOT NULL` — an index over exactly the wrong subset of rows, at MEDIUM confidence. + The lockfile hid it, since `uv sync` pins 30.12 while a fresh `pip install sqlquality` + resolves the newest 30.x. + + Both encodings are accepted rather than picking one and tightening the version floor: the + flag is additive, so a tree built either way answers correctly, and users are not forced to + a particular sqlglot to get correct DDL. + """ + return bool(node.args.get("negate")) or isinstance(node.parent, exp.Not) + + def _within(node: exp.Expression, *types: type[exp.Expression]) -> bool: """True if any ancestor of ``node`` is one of ``types``. Mirrors antipatterns._within_exists.""" parent = node.parent @@ -67,12 +91,7 @@ def _role(column: exp.Column) -> ColumnRole | None: if isinstance(node, exp.Is) and predicate_scope: null_side = isinstance(node.expression, exp.Null) if null_side: - # `is not null` parses as Not(Is(...)), so polarity comes from the parent. - return ( - ColumnRole.NOT_NULL_CHECK - if isinstance(node.parent, exp.Not) - else ColumnRole.NULL_CHECK - ) + return ColumnRole.NOT_NULL_CHECK if _is_negated(node) else ColumnRole.NULL_CHECK if comparison is None and predicate_scope: if isinstance(node, _EQUALITY_NODES): comparison = ColumnRole.EQUALITY diff --git a/src/sqlquality/workload/postgres.py b/src/sqlquality/workload/postgres.py index f2a7ff9..fe5912c 100644 --- a/src/sqlquality/workload/postgres.py +++ b/src/sqlquality/workload/postgres.py @@ -445,6 +445,16 @@ def propose_indexes( f" Only about {leading_ndv:.0f} distinct values, so the index may not be " "selective enough to be worth its write cost." ) + # The MEDIUM rung used to say nothing at all, which reads as a considered judgement + # when it is really an absence of evidence: the statistics for the leading column + # were not available, so the selectivity check simply did not run. The whole rule set + # turns on disclosing a check that could not run rather than assuming its answer, and + # this was the one confidence level that stayed silent about why. + elif leading_ndv is None and rows is not None and have_index_data: + rationale += ( + f" No distinct-value statistics for {columns[0]}, so how selective this index " + "would be could not be checked — run ANALYZE on the table for a firmer answer." + ) if partial_skipped: rationale += ( f" A partial index ({', '.join(partial_skipped)}) leads with these columns " @@ -577,6 +587,14 @@ def propose_join_keys( f" Only about {column_ndv:.0f} distinct values, so the index may not be " "selective enough to be worth its write cost." ) + # Same disclosure ADV001 makes at the same rung, in the same words: MEDIUM here + # means the selectivity check could not run, not that it ran and was middling. + elif column_ndv is None and rows is not None and have_index_data: + rationale += ( + f" No distinct-value statistics for {item.column}, so how selective this " + "index would be could not be checked — run ANALYZE on the table for a " + "firmer answer." + ) if partial_skipped: rationale += ( f" A partial index ({', '.join(partial_skipped)}) leads with these " @@ -727,7 +745,7 @@ def propose_grouping_indexes( if not have_index_data: rationale += ( " The existing-index list could not be read, so whether an index already " - "leads with these columns is unknown." + "leads with these columns is unknown — check before applying." ) if rows is None: rationale += _UNKNOWN_ROWS_NOTE @@ -1715,9 +1733,12 @@ def rank(proposal: Proposal) -> tuple[int, int]: # preference equal — only possible today if the same code proposes the same # DDL twice) keeps whichever proposal `propose()` happened to append first. # That residual is accepted rather than papered over with a further tie-break - # key: two proposals with the same code, the same confidence and the same DDL - # carry no information that distinguishes them, so which one is kept cannot - # matter to a reader the way which *code* is kept does. + # key. Note it is not that such proposals are *identical* — `rationale` and + # `title` can still differ, and `_fold_discarded` preserves the loser's + # rationale either way, so nothing a reader needs is lost. What they no longer + # differ in is the thing a tie-break could act on: same code, same confidence, + # same DDL leaves no principled basis for preferring one, whereas which *code* + # wins is a real editorial choice and `_CODE_PREFERENCE` makes it. ranked = sorted(group, key=rank) winner, *losers = ranked merged[ddl] = cls._fold_discarded(winner, losers, same_index=True) diff --git a/tests/test_advise_cli.py b/tests/test_advise_cli.py index 35115da..48c67ba 100644 --- a/tests/test_advise_cli.py +++ b/tests/test_advise_cli.py @@ -630,3 +630,32 @@ def no_driver(self, params, timeout_s): result = runner.invoke(app, ["advise", "--dsn", "postgresql://u@h/db"]) assert result.exit_code == 2 assert "sqlquality[postgres]" in result.output + + +def test_coverage_warning_is_silent_exactly_at_the_threshold(): + """`share <= _LOW_COVERAGE_FRACTION` returns None, and the boundary is deliberate. + + Nothing pinned which comparison was used, so flipping `<=` to `<` — making the warning + fire at exactly 20% — passed the whole suite. Either choice is defensible; leaving it + unpinned is not, because the threshold is what decides whether a user is told their + proposals may reflect coverage rather than a healthy workload. + + 20 unexplained of 100 candidates is exactly 0.2. The pair below it and above it are + asserted too, so the test fails whichever direction the comparison is flipped rather than + only one of them. + """ + # exactly at the threshold: silent + at = _coverage_warning(_workload_with(stats=80, unparseable=20, noise=0), _aggregation_with()) + assert at is None, "the warning fired at exactly the threshold" + + # one statement worse: 21 of 101 is above 0.2, so it must fire + above = _coverage_warning( + _workload_with(stats=80, unparseable=21, noise=0), _aggregation_with() + ) + assert above is not None, "the warning stayed silent above the threshold" + + # one statement better: 19 of 99 is below 0.2, so it must stay silent + below = _coverage_warning( + _workload_with(stats=80, unparseable=19, noise=0), _aggregation_with() + ) + assert below is None, "the warning fired below the threshold" diff --git a/tests/test_workload_extract.py b/tests/test_workload_extract.py index bb4574a..3ce49a2 100644 --- a/tests/test_workload_extract.py +++ b/tests/test_workload_extract.py @@ -7,6 +7,7 @@ from sqlquality.workload.extract import ( AmbiguousRelation, UnqualifiableQuery, + _is_negated, extract_usage, resolve_relation, ) @@ -92,6 +93,33 @@ def test_null_checks_carry_polarity(): ) +@pytest.mark.parametrize("negated", [False, True]) +def test_null_polarity_is_read_from_both_sqlglot_encodings(negated): + """`IS NOT NULL` has two representations inside the declared `sqlglot>=30.12,<31` range. + + Up to 30.12 it is `Not(Is(col, Null()))`; from 30.13 it is `Is(col, Null(), negate=True)` + with no `exp.Not` in the tree. Parsing SQL only ever exercises whichever encoding the + installed version produces — which is why the lockfile hid the bug while a fresh + `pip install sqlquality` inverted every `IS NOT NULL` into a `NULL_CHECK`, and ADV004 then + emitted `WHERE col IS NULL` for a workload filtering `IS NOT NULL`. + + So both trees are built directly rather than parsed. This test therefore fails on either + encoding regardless of which sqlglot is installed, where a parse-based test can only ever + check one of them. + """ + column = exp.column("shipped_at", table="orders") + predicate = exp.Is(this=column, expression=exp.Null()) + if negated: + # Both shapes at once is not a real tree, so exercise them one at a time: the flag + # form on this pass, the wrapper form on the same pass through `_is_negated`'s other + # branch below. + predicate.set("negate", True) + assert _is_negated(predicate) is negated + + wrapped = exp.Not(this=exp.Is(this=column.copy(), expression=exp.Null())) + assert _is_negated(wrapped.this) is True + + def test_function_wrapped_predicate_is_non_sargable(): usage = _usage("select id from orders where lower(status) = $1") assert (ORDERS, "status", ColumnRole.NON_SARGABLE) in usage diff --git a/tests/test_workload_rules.py b/tests/test_workload_rules.py index 96bf39c..3a1a902 100644 --- a/tests/test_workload_rules.py +++ b/tests/test_workload_rules.py @@ -2551,3 +2551,86 @@ def test_adv008_evidence_reports_the_bare_table_name_and_its_own_schema(): proposals = propose_grouping_indexes(usage, facts, {}, min_cost_share=0.01) assert proposals[0].evidence["schema"] == "staging" assert proposals[0].evidence["table"] == "events" + + +def test_adv001_medium_discloses_that_the_selectivity_check_could_not_run(): + """MEDIUM must say *why*, not just be a middling number. + + The rung is reached when the row count and the index list are both known but the NDV + catalog has nothing for the leading column — so the selectivity check did not run. Saying + nothing reads as a considered judgement rather than an absence of evidence, and disclosing + a check that could not run is the discipline the whole rule set turns on. It was the one + confidence level that stayed silent about its own reason. + """ + relation = Relation("public", "orders") + usage = (_usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.5),) + facts = {relation: _facts(relation, rows=100_000, ndv={})} + proposals = propose_indexes(usage, facts, {}, min_cost_share=0.01) + assert proposals[0].confidence is Confidence.MEDIUM + assert "No distinct-value statistics for status" in proposals[0].rationale + assert "ANALYZE" in proposals[0].rationale + + +def test_adv007_medium_discloses_the_same_gap_in_the_same_words(): + """The two index-creating rules must not explain the same rung differently. + + An operator reads the report, not the rule that produced the line; ADV001 explaining a + MEDIUM while ADV007 stayed silent for the identical reason is the asymmetry that made the + low-NDV caveat a finding in the first place. + """ + relation = Relation("public", "order_items") + usage = (_usage(relation, "order_id", ColumnRole.JOIN, cost_share=0.4),) + facts = {relation: _facts(relation, rows=100_000, ndv={})} + adv007 = propose_join_keys(usage, facts, {}, min_cost_share=0.01)[0] + + orders = Relation("public", "orders") + adv001 = propose_indexes( + (_usage(orders, "order_id", ColumnRole.EQUALITY, cost_share=0.4),), + {orders: _facts(orders, rows=100_000, ndv={})}, + {}, + min_cost_share=0.01, + )[0] + + shared = "so how selective this index would be could not be checked" + assert shared in adv007.rationale + assert shared in adv001.rationale + + +def test_adv001_says_nothing_about_selectivity_when_a_louder_gap_already_applies(): + """The disclosure is for the MEDIUM rung only, not a blanket sentence. + + With an unknown row count the proposal is LOW and already carries the small-table note; a + second "statistics were missing" sentence there would be noise, and would also be + misleading — the reason it is LOW is the row count, not the NDV. + """ + relation = Relation("public", "orders") + usage = (_usage(relation, "status", ColumnRole.EQUALITY, cost_share=0.5),) + facts = {relation: _facts(relation, rows=None, ndv={})} + proposals = propose_indexes(usage, facts, {}, min_cost_share=0.01) + assert proposals[0].confidence is Confidence.LOW + assert "No distinct-value statistics" not in proposals[0].rationale + + +def test_adv007_orders_equal_cost_join_keys_by_column_name(): + """Two join keys tied on cost must come out in a fixed order. + + The sort key is `(-cost_ms, column)`; without the column component two equally hot join + keys would order by whatever `_by_relation` happened to accumulate, and the report would + reshuffle between runs on identical input. + """ + relation = Relation("public", "order_items") + facts = {relation: _facts(relation, rows=100_000)} + forward = ( + _usage(relation, "zeta", ColumnRole.JOIN, cost_share=0.4, cost_ms=100.0), + _usage(relation, "alpha", ColumnRole.JOIN, cost_share=0.4, cost_ms=100.0), + ) + columns = [ + p.evidence["columns"] for p in propose_join_keys(forward, facts, {}, min_cost_share=0.01) + ] + assert columns == [("alpha",), ("zeta",)] + # Reversed input must give the same order, or the tiebreak is not doing the work. + reversed_columns = [ + p.evidence["columns"] + for p in propose_join_keys(tuple(reversed(forward)), facts, {}, min_cost_share=0.01) + ] + assert reversed_columns == columns