diff --git a/src/main.ts b/src/main.ts index 092acc8..cc246cf 100644 --- a/src/main.ts +++ b/src/main.ts @@ -202,7 +202,7 @@ async function runInCI( } } if (previousRun) { - reportContext.comparison = compareRuns( + reportContext.comparison = await compareRuns( queries, previousRun, config.regressionThreshold, diff --git a/src/reporters/query-shape.test.ts b/src/reporters/query-shape.test.ts new file mode 100644 index 0000000..7e71187 --- /dev/null +++ b/src/reporters/query-shape.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, test } from "vitest"; +import { originFile, originsCompatible, shapeKey } from "./query-shape.ts"; + +describe("shapeKey", () => { + test("a column added to the SELECT list does not change the key", async () => { + const base = await shapeKey( + "SELECT id, user_id FROM renders WHERE user_id = $1 ORDER BY created_at DESC LIMIT $2", + ); + const wider = await shapeKey( + "SELECT id, user_id, session_id, source_payload FROM renders WHERE user_id = $1 ORDER BY created_at DESC LIMIT $2", + ); + expect(base).not.toBeNull(); + expect(wider).toBe(base); + }); + + test("a different WHERE changes the key", async () => { + const a = await shapeKey("SELECT id FROM renders WHERE user_id = $1"); + const b = await shapeKey("SELECT id FROM renders WHERE share_slug = $1"); + expect(a).not.toBe(b); + }); + + test("a different table changes the key", async () => { + const a = await shapeKey("SELECT id FROM renders WHERE id = $1"); + const b = await shapeKey("SELECT id FROM widgets WHERE id = $1"); + expect(a).not.toBe(b); + }); + + test("returns null for an unparseable query rather than throwing", async () => { + expect(await shapeKey("this is not sql")).toBeNull(); + }); +}); + +describe("originFile", () => { + test("reads the first entry of the file tag", () => { + expect( + originFile([{ key: "file", value: "src/db/postgres.ts;src/service.ts" }]), + ).toBe("src/db/postgres.ts"); + }); + + test("is null when there is no file tag", () => { + expect(originFile([{ key: "route", value: "/api/x" }])).toBeNull(); + expect(originFile(undefined)).toBeNull(); + }); +}); + +describe("originsCompatible", () => { + test("both tagged and equal → compatible", () => { + const a = [{ key: "file", value: "src/db/postgres.ts" }]; + const b = [{ key: "file", value: "src/db/postgres.ts" }]; + expect(originsCompatible(a, b)).toBe(true); + }); + + test("both tagged and different → incompatible", () => { + const a = [{ key: "file", value: "src/db/postgres.ts" }]; + const b = [{ key: "file", value: "src/db/other.ts" }]; + expect(originsCompatible(a, b)).toBe(false); + }); + + test("either untagged → compatible", () => { + expect(originsCompatible([{ key: "file", value: "src/db/postgres.ts" }], [])).toBe(true); + expect(originsCompatible(undefined, undefined)).toBe(true); + }); +}); diff --git a/src/reporters/query-shape.ts b/src/reporters/query-shape.ts new file mode 100644 index 0000000..f7ddfa8 --- /dev/null +++ b/src/reporters/query-shape.ts @@ -0,0 +1,58 @@ +import { normalizedFingerprint, type SQLCommenterTag } from "@query-doctor/core"; +import { fingerprint, parse } from "@libpg-query/parser"; + +/** + * A query's shape key: core's normalized fingerprint, which strips bare column + * references from every SELECT list (top-level, CTEs, subqueries, set-op arms) + * so that adding a column to a SELECT does not change the key — while a change + * to the FROM / WHERE / ORDER / LIMIT / aggregates still does. + * + * This is how we recognise "the same query, columns added" across a raw-hash + * change (#3367): a column added to a SELECT changes the exact hash, so without + * a shape key the same call site reads as one `removed` + one `new` query + * instead of one changed query. Returns null when the query can't be parsed — + * it then simply can't be shape-matched and falls back to new/removed. + */ +export async function shapeKey(query: string): Promise { + try { + return await normalizedFingerprint(await parse(query), fingerprint); + } catch { + return null; + } +} + +/** + * The query's own call site: the first entry of the sqlcommenter `file` tag + * (packed most-specific-first, `;`-separated), read the same way test-origin + * detection reads origin. Used to keep shape-matching from merging + * identically-shaped queries issued from different call sites. Null when the + * query carries no `file` tag. + */ +export function originFile( + tags: readonly SQLCommenterTag[] | undefined, +): string | null { + if (!tags) return null; + for (const tag of tags) { + if (tag.key !== "file") continue; + const first = tag.value.split(";")[0]?.trim(); + if (first) return first; + } + return null; +} + +/** + * Whether two queries' origins are compatible for shape-matching. When both + * carry a `file` tag they must name the same call site; when either is untagged + * we allow the match, since the shape key (same table + WHERE/ORDER/LIMIT, + * differing only in bare columns) is already specific enough that a collision is + * the same access pattern. + */ +export function originsCompatible( + a: readonly SQLCommenterTag[] | undefined, + b: readonly SQLCommenterTag[] | undefined, +): boolean { + const fa = originFile(a); + const fb = originFile(b); + if (fa && fb) return fa === fb; + return true; +} diff --git a/src/reporters/site-api.test.ts b/src/reporters/site-api.test.ts index 4776392..2973230 100644 --- a/src/reporters/site-api.test.ts +++ b/src/reporters/site-api.test.ts @@ -11,10 +11,15 @@ import { } from "./site-api.ts"; function makeQuery(hash: string, cost: number = 100): CiQueryPayload { + // Give each distinct hash a distinct table so the query text (and thus its + // shape) is unique per hash — real queries can't share text without sharing a + // hash. This keeps shape-matching (#3367) from conflating unrelated fixtures; + // the changed-query tests below use explicitly same-shape queries. + const table = `t_${hash.replace(/[^a-z0-9]/gi, "_")}`; return { hash, - query: `SELECT * FROM t WHERE id = $1`, - formattedQuery: `SELECT *\nFROM t\nWHERE id = $1`, + query: `SELECT * FROM ${table} WHERE id = $1`, + formattedQuery: `SELECT *\nFROM ${table}\nWHERE id = $1`, optimization: { state: "no_improvement_found", cost, @@ -38,11 +43,11 @@ function makePreviousRun(queries: CiQueryPayload[]): PreviousRun { describe("compareRuns", () => { describe("new query detection via previousRun", () => { - test("when previousRun has no queries, all current queries are new", () => { + test("when previousRun has no queries, all current queries are new", async () => { const queries = [makeQuery("hash-a"), makeQuery("hash-b")]; const previousRun = makePreviousRun([]); - const result = compareRuns(queries, previousRun, 10); + const result = await compareRuns(queries, previousRun, 10); expect(result.newQueries).toHaveLength(2); expect(result.newQueries.map((q) => q.hash)).toEqual([ @@ -53,7 +58,7 @@ describe("compareRuns", () => { expect(result.improved).toHaveLength(0); }); - test("when previousRun contains hashes, only non-seen queries are new", () => { + test("when previousRun contains hashes, only non-seen queries are new", async () => { const queries = [ makeQuery("hash-a"), makeQuery("hash-b"), @@ -64,17 +69,17 @@ describe("compareRuns", () => { makeQuery("hash-c"), ]); - const result = compareRuns(queries, previousRun, 10); + const result = await compareRuns(queries, previousRun, 10); expect(result.newQueries).toHaveLength(1); expect(result.newQueries[0].hash).toBe("hash-b"); }); - test("query in previousRun with increased cost is flagged as regression", () => { + test("query in previousRun with increased cost is flagged as regression", async () => { const currentQueries = [makeQuery("hash-a", 500)]; const previousRun = makePreviousRun([makeQuery("hash-a", 100)]); - const result = compareRuns(currentQueries, previousRun, 10); + const result = await compareRuns(currentQueries, previousRun, 10); expect(result.newQueries).toHaveLength(0); expect(result.regressed).toHaveLength(1); @@ -86,22 +91,22 @@ describe("compareRuns", () => { }); describe("regression threshold", () => { - test("cost change within threshold is not flagged", () => { + test("cost change within threshold is not flagged", async () => { const currentQueries = [makeQuery("hash-a", 115)]; const previousRun = makePreviousRun([makeQuery("hash-a", 100)]); - const result = compareRuns(currentQueries, previousRun, 20); + const result = await compareRuns(currentQueries, previousRun, 20); expect(result.regressed).toHaveLength(0); expect(result.improved).toHaveLength(0); expect(result.newQueries).toHaveLength(0); }); - test("cost decrease beyond threshold is flagged as improved", () => { + test("cost decrease beyond threshold is flagged as improved", async () => { const currentQueries = [makeQuery("hash-a", 30)]; const previousRun = makePreviousRun([makeQuery("hash-a", 100)]); - const result = compareRuns(currentQueries, previousRun, 10); + const result = await compareRuns(currentQueries, previousRun, 10); expect(result.improved).toHaveLength(1); expect(result.improved[0].hash).toBe("hash-a"); @@ -109,11 +114,11 @@ describe("compareRuns", () => { expect(result.improved[0].currentCost).toBe(30); }); - test("acknowledged regression goes to acknowledgedRegressed, not regressed", () => { + test("acknowledged regression goes to acknowledgedRegressed, not regressed", async () => { const currentQueries = [makeQuery("hash-a", 500)]; const previousRun = makePreviousRun([makeQuery("hash-a", 100)]); - const result = compareRuns( + const result = await compareRuns( currentQueries, previousRun, 10, @@ -128,27 +133,27 @@ describe("compareRuns", () => { }); describe("minimumCost filtering", () => { - test("regression where both costs are below minimumCost is skipped", () => { + test("regression where both costs are below minimumCost is skipped", async () => { const currentQueries = [makeQuery("hash-a", 8)]; const previousRun = makePreviousRun([makeQuery("hash-a", 2)]); - const result = compareRuns(currentQueries, previousRun, 10, 50); + const result = await compareRuns(currentQueries, previousRun, 10, 50); expect(result.regressed).toHaveLength(0); }); - test("regression where current cost exceeds minimumCost is reported", () => { + test("regression where current cost exceeds minimumCost is reported", async () => { const currentQueries = [makeQuery("hash-a", 200)]; const previousRun = makePreviousRun([makeQuery("hash-a", 10)]); - const result = compareRuns(currentQueries, previousRun, 10, 50); + const result = await compareRuns(currentQueries, previousRun, 10, 50); expect(result.regressed).toHaveLength(1); }); }); describe("disappeared queries", () => { - test("queries in previousRun but not in current are disappeared", () => { + test("queries in previousRun but not in current are disappeared", async () => { const currentQueries = [makeQuery("hash-a")]; const previousRun = makePreviousRun([ makeQuery("hash-a"), @@ -156,14 +161,14 @@ describe("compareRuns", () => { makeQuery("hash-c"), ]); - const result = compareRuns(currentQueries, previousRun, 10); + const result = await compareRuns(currentQueries, previousRun, 10); expect(result.disappearedHashes).toEqual(["hash-b", "hash-c"]); }); }); describe("mixed scenarios", () => { - test("new, regressed, improved, and disappeared queries together", () => { + test("new, regressed, improved, and disappeared queries together", async () => { const currentQueries = [ makeQuery("hash-a", 500), // regressed (was 100) makeQuery("hash-b", 30), // improved (was 100) @@ -177,7 +182,7 @@ describe("compareRuns", () => { makeQuery("hash-e", 100), // disappeared ]); - const result = compareRuns(currentQueries, previousRun, 10); + const result = await compareRuns(currentQueries, previousRun, 10); expect(result.newQueries).toHaveLength(1); expect(result.newQueries[0].hash).toBe("hash-d"); @@ -195,27 +200,27 @@ describe("compareRuns", () => { tags: [{ key: "file", value: "tests/pg/postgres.test.ts" }], }); - test("a regressed test-origin query is bucketed out of the gate, not regressed", () => { + test("a regressed test-origin query is bucketed out of the gate, not regressed", async () => { const current = [fromTestFile(makeQuery("hash-a", 500))]; const previousRun = makePreviousRun([makeQuery("hash-a", 100)]); - const result = compareRuns(current, previousRun, 10); + const result = await compareRuns(current, previousRun, 10); expect(result.regressed).toHaveLength(0); expect(result.testOriginExcluded.map((q) => q.hash)).toEqual(["hash-a"]); }); - test("a new test-origin query never enters newQueries", () => { + test("a new test-origin query never enters newQueries", async () => { const current = [fromTestFile(makeQuery("hash-new", 200))]; const previousRun = makePreviousRun([]); - const result = compareRuns(current, previousRun, 10); + const result = await compareRuns(current, previousRun, 10); expect(result.newQueries).toHaveLength(0); expect(result.testOriginExcluded.map((q) => q.hash)).toEqual(["hash-new"]); }); - test("production queries in the same run are unaffected", () => { + test("production queries in the same run are unaffected", async () => { const current = [ fromTestFile(makeQuery("hash-test", 500)), // was 100 → excluded makeQuery("hash-prod", 500), // was 100 → regressed @@ -225,22 +230,106 @@ describe("compareRuns", () => { makeQuery("hash-prod", 100), ]); - const result = compareRuns(current, previousRun, 10); + const result = await compareRuns(current, previousRun, 10); expect(result.regressed.map((q) => q.hash)).toEqual(["hash-prod"]); expect(result.testOriginExcluded.map((q) => q.hash)).toEqual(["hash-test"]); }); - test("an untagged query still gates exactly as before", () => { + test("an untagged query still gates exactly as before", async () => { const current = [makeQuery("hash-a", 500)]; const previousRun = makePreviousRun([makeQuery("hash-a", 100)]); - const result = compareRuns(current, previousRun, 10); + const result = await compareRuns(current, previousRun, 10); expect(result.regressed).toHaveLength(1); expect(result.testOriginExcluded).toHaveLength(0); }); }); + + describe("changed-query detection via shape (#3367)", () => { + // The real Nutcracker case: adding columns to a SELECT changes the hash but + // not the query's shape. + const BASE_SQL = + "SELECT id, share_slug, user_id FROM design_renders WHERE user_id = $1 ORDER BY created_at DESC LIMIT $2"; + const WIDER_SQL = + "SELECT id, share_slug, user_id, session_id, source_payload FROM design_renders WHERE user_id = $1 ORDER BY created_at DESC LIMIT $2"; + + const q = ( + hash: string, + sql: string, + cost: number, + file = "src/db/postgres.ts", + ): CiQueryPayload => ({ + hash, + query: sql, + formattedQuery: sql, + optimization: { state: "no_improvement_found", cost, indexesUsed: [] }, + nudges: [], + tags: [{ key: "file", value: file }], + tableReferences: [], + }); + + test("a column added to a SELECT reads as one changed query, not new + removed", async () => { + const current = [q("hash-wide", WIDER_SQL, 100)]; + const previousRun = makePreviousRun([q("hash-base", BASE_SQL, 100)]); + + const result = await compareRuns(current, previousRun, 10); + + expect(result.newQueries).toHaveLength(0); + expect(result.disappearedHashes).toHaveLength(0); + }); + + test("a shape-matched query carries the cost delta against its previous self", async () => { + const current = [q("hash-wide", WIDER_SQL, 500)]; + const previousRun = makePreviousRun([q("hash-base", BASE_SQL, 100)]); + + const result = await compareRuns(current, previousRun, 10); + + expect(result.regressed).toHaveLength(1); + expect(result.regressed[0]).toMatchObject({ + hash: "hash-wide", + previousCost: 100, + currentCost: 500, + }); + expect(result.newQueries).toHaveLength(0); + expect(result.disappearedHashes).toHaveLength(0); + }); + + test("the same shape from a different call site is not merged", async () => { + const current = [q("hash-wide", WIDER_SQL, 100, "src/db/other.ts")]; + const previousRun = makePreviousRun([ + q("hash-base", BASE_SQL, 100, "src/db/postgres.ts"), + ]); + + const result = await compareRuns(current, previousRun, 10); + + expect(result.newQueries.map((x) => x.hash)).toEqual(["hash-wide"]); + expect(result.disappearedHashes).toEqual(["hash-base"]); + }); + + test("when the original query still runs, the widened variant is genuinely new", async () => { + const current = [q("hash-base", BASE_SQL, 100), q("hash-wide", WIDER_SQL, 100)]; + const previousRun = makePreviousRun([q("hash-base", BASE_SQL, 100)]); + + const result = await compareRuns(current, previousRun, 10); + + expect(result.newQueries.map((x) => x.hash)).toEqual(["hash-wide"]); + expect(result.disappearedHashes).toHaveLength(0); + }); + + test("a genuinely different query is not shape-matched", async () => { + const current = [ + q("hash-x", "SELECT id FROM widgets WHERE owner_id = $1", 100), + ]; + const previousRun = makePreviousRun([q("hash-base", BASE_SQL, 100)]); + + const result = await compareRuns(current, previousRun, 10); + + expect(result.newQueries.map((x) => x.hash)).toEqual(["hash-x"]); + expect(result.disappearedHashes).toEqual(["hash-base"]); + }); + }); }); describe("postToSiteApi authentication", () => { @@ -335,22 +424,22 @@ describe("postToSiteApi payload baseBranch", () => { }); describe("classifyIngestFailure", () => { - test("treats no-response and 5xx as transient (recoverable)", () => { + test("treats no-response and 5xx as transient (recoverable)", async () => { expect(classifyIngestFailure(null)).toBe("transient"); expect(classifyIngestFailure(500)).toBe("transient"); expect(classifyIngestFailure(503)).toBe("transient"); }); - test("treats 401/403 as auth (user must fix the token)", () => { + test("treats 401/403 as auth (user must fix the token)", async () => { expect(classifyIngestFailure(401)).toBe("auth"); expect(classifyIngestFailure(403)).toBe("auth"); }); - test("treats 413 as too_large (payload over the size limit)", () => { + test("treats 413 as too_large (payload over the size limit)", async () => { expect(classifyIngestFailure(413)).toBe("too_large"); }); - test("treats other 4xx as a rejected run (contract skew)", () => { + test("treats other 4xx as a rejected run (contract skew)", async () => { expect(classifyIngestFailure(400)).toBe("rejected"); expect(classifyIngestFailure(422)).toBe("rejected"); }); @@ -449,25 +538,25 @@ describe("gateEligibleNewQueries", () => { }; } - test("flags a new query whose recommendation exceeds regressionThreshold", () => { + test("flags a new query whose recommendation exceeds regressionThreshold", async () => { const result = gateEligibleNewQueries([withRecommendation("a", 99)], 90); expect(result.map((q) => q.hash)).toEqual(["a"]); }); - test("with regressionThreshold 0 (flag-all), any real recommendation blocks", () => { + test("with regressionThreshold 0 (flag-all), any real recommendation blocks", async () => { const result = gateEligibleNewQueries([withRecommendation("a", 99)], 0); expect(result.map((q) => q.hash)).toEqual(["a"]); }); - test("ignores a recommendation at or below regressionThreshold", () => { + test("ignores a recommendation at or below regressionThreshold", async () => { const result = gateEligibleNewQueries([withRecommendation("a", 40)], 90); expect(result).toHaveLength(0); }); - test("ignores a query with no index recommendation (e.g. test-only query)", () => { + test("ignores a query with no index recommendation (e.g. test-only query)", async () => { const result = gateEligibleNewQueries( [withRecommendation("a", 99, false)], 90, @@ -476,13 +565,13 @@ describe("gateEligibleNewQueries", () => { expect(result).toHaveLength(0); }); - test("ignores a query with no available improvement", () => { + test("ignores a query with no available improvement", async () => { const result = gateEligibleNewQueries([makeQuery("a")], 90); expect(result).toHaveLength(0); }); - test("acknowledged hashes are exempt", () => { + test("acknowledged hashes are exempt", async () => { const result = gateEligibleNewQueries( [withRecommendation("a", 99), withRecommendation("b", 99)], 90, diff --git a/src/reporters/site-api.ts b/src/reporters/site-api.ts index c97a4b8..3232421 100644 --- a/src/reporters/site-api.ts +++ b/src/reporters/site-api.ts @@ -4,6 +4,7 @@ import * as github from "@actions/github"; import { isTestOriginQuery } from "@query-doctor/core"; import type { ComputedStats, FullSchema, IndexRecommendation, Nudge, SQLCommenterTag, StatisticsMode, TableReference } from "@query-doctor/core"; import { DEFAULT_CONFIG, type AnalyzerConfig } from "../config.ts"; +import { originsCompatible, shapeKey } from "./query-shape.ts"; import type { OptimizedQuery } from "../sql/recent-query.ts"; import type { Op } from "jsondiffpatch/formatters/jsonpatch"; @@ -298,23 +299,79 @@ export function buildQueries( .filter((q) => !ignoredSet.has(q.hash)); } -export function compareRuns( +/** + * Bucket previous queries by shape key. Keys are computed up front (parsing is + * async), then the map is built synchronously so concurrent inserts can't race. + * Unparseable queries carry no key and simply don't participate in shape-matching. + */ +async function buildShapeIndex( + queries: CiQueryPayload[], +): Promise> { + const keyed = await Promise.all( + queries.map(async (q) => ({ q, key: await shapeKey(q.query) })), + ); + const index = new Map(); + for (const { q, key } of keyed) { + if (!key) continue; + const bucket = index.get(key); + if (bucket) bucket.push(q); + else index.set(key, [q]); + } + return index; +} + +/** + * Find a previous query of the same shape and call site for a current query + * whose exact hash didn't match — the changed-query case (#3367). Skips + * previous queries already claimed by another match so each pairs at most once. + */ +async function matchByShape( + current: CiQueryPayload, + prevByShape: Map, + matchedPrevHashes: Set, +): Promise { + const key = await shapeKey(current.query); + if (!key) return null; + const bucket = prevByShape.get(key); + if (!bucket) return null; + return ( + bucket.find( + (prev) => + !matchedPrevHashes.has(prev.hash) && + originsCompatible(current.tags, prev.tags), + ) ?? null + ); +} + +export async function compareRuns( currentQueries: CiQueryPayload[], previousRun: PreviousRun, regressionThreshold: number, minimumCost: number = 0, acknowledgedQueryHashes: string[] = [], -): RunComparison { +): Promise { const prevByHash = new Map(previousRun.queries.map((q) => [q.hash, q])); const currentHashes = new Set(currentQueries.map((q) => q.hash)); const acknowledgedSet = new Set(acknowledgedQueryHashes); + // Shape index over the previous run: a column added to a SELECT changes the + // exact hash but not the query's shape, so an exact-hash miss falls back to + // matching a previous query of the same shape and call site — recognising one + // changed query instead of a spurious removed+new pair (#3367). + const prevByShape = await buildShapeIndex(previousRun.queries); + const matchedPrevHashes = new Set(); + const regressed: RegressedQuery[] = []; const acknowledgedRegressed: RegressedQuery[] = []; const improved: ImprovedQuery[] = []; const newQueries: CiQueryPayload[] = []; const testOriginExcluded: CiQueryPayload[] = []; + // Pair each current query with its previous counterpart in two passes. + // Pass 1 reserves every exact-hash match first, so pass 2's shape-matching + // can't claim a previous query that a later current query matches exactly. + const pairs: Array<[current: CiQueryPayload, prev: CiQueryPayload]> = []; + const pendingShapeMatch: CiQueryPayload[] = []; for (const current of currentQueries) { // A query issued from a test file runs no production path, so it must never // gate the PR (#3199). Bucket it out before any regressed/new/improved @@ -324,11 +381,25 @@ export function compareRuns( testOriginExcluded.push(current); continue; } - const prev = prevByHash.get(current.hash); - if (!prev) { + const exact = prevByHash.get(current.hash); + if (exact) { + matchedPrevHashes.add(exact.hash); + pairs.push([current, exact]); + } else { + pendingShapeMatch.push(current); + } + } + for (const current of pendingShapeMatch) { + const prev = await matchByShape(current, prevByShape, matchedPrevHashes); + if (prev) { + matchedPrevHashes.add(prev.hash); + pairs.push([current, prev]); + } else { newQueries.push(current); - continue; } + } + + for (const [current, prev] of pairs) { const prevCost = getQueryCost(prev); const currentCost = getQueryCost(current); if (prevCost === null || currentCost === null || prevCost === 0) continue; @@ -370,9 +441,13 @@ export function compareRuns( } } + // A previous query counts as disappeared only if nothing in the current run + // claims it — neither an exact-hash match nor a shape match. Excluding + // shape-matched hashes is what stops a column-add from showing up as a phantom + // removal alongside its "new" twin (#3367). const disappearedHashes: string[] = []; for (const [hash] of prevByHash) { - if (!currentHashes.has(hash)) { + if (!currentHashes.has(hash) && !matchedPrevHashes.has(hash)) { disappearedHashes.push(hash); } }