fix(advise): inverted IS NOT NULL on current sqlglot, and the CI gaps that hid it

.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

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

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")
 

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

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)

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"

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