From 0681ee548643df2dfacc8546790790461dbac94e Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 18 Oct 2026 09:22:40 -0700 Subject: [PATCH 1/5] feat: note silent row loss on joins in `sql_execute` results After a successful read, `sql_execute` now checks each equality join whose two inputs are plain relations (at most two per query) and, when fewer than half of the left rows have a match, adds one advisory note: how many left rows were lost, and to confirm the loss is expected or find a key or source that covers the population. Healthy joins add nothing. - `sql-join-coverage.ts`: parses with DuckDB's own parser (`json_serialize_sql`), rebuilds each join side, runs one counting query, and returns the note. Any parse ambiguity, error or overrun of the 250 ms budget returns nothing and leaves the result unchanged. - `ALTIMATE_SQL_AUTO_CHECK=off|local|all` (default `local`): `local` runs only on the scratch database and DuckDB/SQLite connections; `all` adds remote warehouses, base tables only, capped at 100,000 left rows. - `sql-execute.ts`: adds the note ahead of lint notes and sets `join_coverage_warning` in metadata. - Tests on real DuckDB (scratch) and SQLite: low and threshold coverage, left-join wording, multi-key, CTE and subquery sides, no join, off flag, non-equality conditions, remote skip in local mode, errors, timeout, and the note reaching `sql_execute` output. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/tools/sql-execute.ts | 15 ++ .../src/altimate/tools/sql-join-coverage.ts | 237 +++++++++++++++++ .../altimate/tools/sql-join-coverage.test.ts | 238 ++++++++++++++++++ 3 files changed, 490 insertions(+) create mode 100644 packages/opencode/src/altimate/tools/sql-join-coverage.ts create mode 100644 packages/opencode/test/altimate/tools/sql-join-coverage.test.ts diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index 2af69625b3..d38706a1f1 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -5,6 +5,9 @@ import { Truncate } from "../../tool/truncate" import { legacyToInit, type LegacyToolDef } from "../tool-zod-compat" import { formatSqlResult, SQL_MAX_PREVIEW_BYTES } from "./sql-result-format" import { lintSqlResult } from "./sql-result-lint" +// altimate_change start — automatic join-coverage note +import { joinCoverageNote } from "./sql-join-coverage" +// altimate_change end import { Tool } from "../../tool/tool" import { Dispatcher } from "../native" import type { SqlExecuteResult } from "../native/types" @@ -359,6 +362,12 @@ export const SqlExecuteTool = Tool.define( const routingClipped = routingPrefix !== routingNotice const suggestion = ctx.agent === "analyst" ? null : PostConnectSuggestions.getProgressiveSuggestion("sql_execute") const lint = await lintSqlResult(sql, args.warehouse, ctx.sessionID) + // altimate_change start — advisory join-coverage note on successful reads; never alters the rows + const coverage = + !mongo && queryType === "read" + ? await joinCoverageNote(sql, args.warehouse, { signal: ctx.abort, sessionID: ctx.sessionID }) + : undefined + // altimate_change end const formatted = await formatSqlResult(result, { limit, fullTextColumns: args.full_text_columns, @@ -368,6 +377,9 @@ export const SqlExecuteTool = Tool.define( notes: [ fullText ? "" : (precedence.notice ?? ""), limitNote, + // altimate_change start — join-coverage note leads the advisory notes + coverage ?? "", + // altimate_change end lint?.output ?? "", suggestion ?? "", ].filter(Boolean), @@ -392,6 +404,9 @@ export const SqlExecuteTool = Tool.define( ...formatted.metadata, ...(routingClipped && { output_truncated: true, truncated: true }), ...(lint && { lint_rules: lint.rules }), + // altimate_change start — record that the coverage note fired + ...(coverage && { join_coverage_warning: true }), + // altimate_change end }, }), output: routingPrefix + formatted.output, diff --git a/packages/opencode/src/altimate/tools/sql-join-coverage.ts b/packages/opencode/src/altimate/tools/sql-join-coverage.ts new file mode 100644 index 0000000000..015e7c55dc --- /dev/null +++ b/packages/opencode/src/altimate/tools/sql-join-coverage.ts @@ -0,0 +1,237 @@ +// altimate_change - new file +import type { Connector } from "@altimateai/drivers" +import * as Registry from "../native/connections/registry" +import { SCRATCH_NAME, isScratch } from "../native/connections/scratch" +import { executeQuery, type QueryContext } from "../native/query-execution" + +/** Below this matched share of the left side, a join loses most of the population it started from. */ +export const LOW_COVERAGE_SHARE = 0.5 +/** Whole-check budget: parse, count and format. Over budget, the query result is returned unchanged. */ +export const JOIN_CHECK_BUDGET_MS = 250 +/** Remote warehouses (`all` mode) count at most this many left rows. */ +export const REMOTE_ROW_CEILING = 100_000 +const MAX_JOINS = 2 + +export type AutoCheckMode = "off" | "local" | "all" + +/** `ALTIMATE_SQL_AUTO_CHECK`: off, local (default: scratch, DuckDB and SQLite only) or all. */ +export function autoCheckMode(env: NodeJS.ProcessEnv = process.env): AutoCheckMode { + const value = env["ALTIMATE_SQL_AUTO_CHECK"]?.trim().toLowerCase() + if (value === "off" || value === "0" || value === "false") return "off" + if (value === "all") return "all" + return "local" +} + +/** A leading warning when most of the left side has no match, so a large silent loss is not reported as fact. */ +export function coverageLoss(matched: number, total: number, unit: string, effect: string): string[] { + if (!total || matched / total >= LOW_COVERAGE_SHARE) return [] + const lost = total - matched + return [ + `WARNING coverage loss: ${lost} of ${total} ${unit} (${((lost / total) * 100).toFixed(1)}%) have no match; ${effect}. ` + + "Before reporting results over the joined rows, confirm this loss is expected (e.g. the other table covers only some entities) " + + "and state it in the answer, or look for a different key, column or source that covers the population the question asks about.", + ] +} + +type Json = any + +const literal = (text: string) => `'${text.replace(/'/g, "''")}'` +const ident = (name: string) => `"${name.replace(/"/g, '""')}"` + +function refName(ref: Json): string | undefined { + if (ref?.type === "BASE_TABLE") return [ref.catalog_name, ref.schema_name, ref.table_name].filter(Boolean).join(".") + if (ref?.type === "SUBQUERY") return ref.alias ? `(subquery) ${ref.alias}` : undefined + return undefined +} + +/** Equality pairs between the two sides, or undefined when the condition is anything else. */ +function equalityKeys(condition: Json, leftAlias: string, rightAlias: string): [string, string][] | undefined { + const conjuncts = condition?.type === "CONJUNCTION_AND" ? condition.children : condition ? [condition] : [] + if (!conjuncts.length) return undefined + const keys: [string, string][] = [] + for (const c of conjuncts) { + if (c?.type !== "COMPARE_EQUAL" || c.left?.type !== "COLUMN_REF" || c.right?.type !== "COLUMN_REF") return undefined + const a: string[] = c.left.column_names + const b: string[] = c.right.column_names + if (a.length !== 2 || b.length !== 2) return undefined + const lower = (s: string) => s.toLowerCase() + if (lower(a[0]) === lower(leftAlias) && lower(b[0]) === lower(rightAlias)) keys.push([a[1], b[1]]) + else if (lower(b[0]) === lower(leftAlias) && lower(a[0]) === lower(rightAlias)) keys.push([b[1], a[1]]) + else return undefined + } + return keys +} + +/** Join nodes whose two inputs are plain relations, in left-to-right order. */ +function relationJoins(ref: Json, out: Json[] = []): Json[] { + if (ref?.type !== "JOIN") return out + relationJoins(ref.left, out) + relationJoins(ref.right, out) + if (ref.left?.type !== "JOIN" && ref.right?.type !== "JOIN") out.push(ref) + return out +} + +async function rows( + connector: Connector, + config: Parameters[0], + sql: string, + context: QueryContext, + timeoutMs: number, +) { + const response = await executeQuery( + config, + { noLimit: true, readOnly: true, queryTimeoutMs: timeoutMs }, + context, + (options) => connector.execute(sql, undefined, undefined, options), + ) + return response.rows.map((row: unknown[]) => + Object.fromEntries(response.columns.map((col: string, i: number) => [col, row[i]])), + ) +} + +/** Parse with DuckDB's own parser. Returns the statement JSON or undefined when it is not one plain SELECT. */ +async function parse(parser: Connector, sql: string, context: QueryContext, timeoutMs: number) { + const [row] = await rows( + parser, + { type: "duckdb" }, + `SELECT json_serialize_sql(${literal(sql)}) AS j`, + context, + timeoutMs, + ) + // Locations are uint64 and lose precision in JSON.parse; zero them. Any other huge number + // (e.g. a BIGINT literal) could be corrupted the same way, so such statements are skipped. + const text = String(row?.j ?? "").replace(/"query_location":\s*\d+/g, '"query_location":0') + if (/-?\d{16,}/.test(text)) return undefined + const parsed = JSON.parse(text) + if (parsed?.error || parsed?.statements?.length !== 1) return undefined + if (parsed.statements[0].node?.type !== "SELECT_NODE") return undefined + return parsed +} + +/** DuckDB targets keep subqueries and CTEs; other engines only count plain base tables. */ +async function sideSql( + parser: Connector, + statement: Json, + ref: Json, + duckdb: boolean, + context: QueryContext, + timeoutMs: number, +) { + const node = statement.statements[0].node + const cteNames = new Set((node.cte_map?.map ?? []).map((entry: Json) => String(entry.key).toLowerCase())) + if (!duckdb) { + if (ref.type !== "BASE_TABLE" || cteNames.has(String(ref.table_name).toLowerCase())) return undefined + return `SELECT * FROM ${[ref.catalog_name, ref.schema_name, ref.table_name].filter(Boolean).map(ident).join(".")}` + } + const star = { + class: "STAR", + type: "STAR", + alias: "", + query_location: 0, + relation_name: "", + exclude_list: [], + replace_list: [], + columns: false, + expr: null, + qualified_exclude_list: [], + rename_list: [], + } + const synthetic = { + ...statement, + statements: [ + { + ...statement.statements[0], + node: { + ...node, + select_list: [star], + from_table: { ...ref, alias: "" }, + where_clause: null, + group_expressions: [], + group_sets: [], + having: null, + qualify: null, + modifiers: [], + }, + }, + ], + } + const [row] = await rows( + parser, + { type: "duckdb" }, + `SELECT json_deserialize_sql(${literal(JSON.stringify(synthetic))}) AS s`, + context, + timeoutMs, + ) + return typeof row?.s === "string" && row.s ? row.s : undefined +} + +/** + * Advisory note for a successful read: when an equality join keeps under half of its left side, + * say so. One bounded counting query per join (at most two), run only where policy allows; any + * failure, ambiguity or timeout returns undefined so the query result is never altered. + */ +export async function joinCoverageNote( + sql: string, + warehouse: string | undefined, + context: QueryContext = {}, + env: NodeJS.ProcessEnv = process.env, +): Promise { + const mode = autoCheckMode(env) + if (mode === "off") return undefined + const name = warehouse ?? Registry.list().warehouses[0]?.name + if (!name) return undefined + const config = Registry.getConfig(name) + const type = isScratch(name) ? "duckdb" : Registry.canonicalType(config?.type) + const local = type === "duckdb" || type === "sqlite" + if (!local && mode !== "all") return undefined + if (!/\bjoin\b/i.test(sql)) return undefined + + const started = Date.now() + const remaining = () => Math.max(0, JOIN_CHECK_BUDGET_MS - (Date.now() - started)) + let timer: ReturnType | undefined + const timeout = new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), JOIN_CHECK_BUDGET_MS) + }) + const work = async () => { + const target = await Registry.get(name) + const duckdb = type === "duckdb" + const parser = duckdb ? target : await Registry.get(SCRATCH_NAME) + const statement = await parse(parser, sql, context, remaining()) + if (!statement) return undefined + const lines: string[] = [] + for (const join of relationJoins(statement.statements[0].node.from_table).slice(0, MAX_JOINS)) { + const kind = String(join.join_type).toUpperCase() + if ((kind !== "INNER" && kind !== "LEFT") || join.ref_type !== "REGULAR") continue + const leftAlias = join.left.alias || join.left.table_name + const rightAlias = join.right.alias || join.right.table_name + if (!leftAlias || !rightAlias) continue + const keys = equalityKeys(join.condition, leftAlias, rightAlias) + if (!keys?.length) continue + const leftSql = await sideSql(parser, statement, join.left, duckdb, context, remaining()) + const rightSql = await sideSql(parser, statement, join.right, duckdb, context, remaining()) + if (!leftSql || !rightSql) continue + const bounded = local ? leftSql : `SELECT * FROM (${leftSql}) AS __jc_src LIMIT ${REMOTE_ROW_CEILING}` + const match = keys.map(([l, r]) => `__jc_r.${ident(r)} = __jc_l.${ident(l)}`).join(" AND ") + const count = `WITH __jc_left AS (${bounded}), __jc_right AS (${rightSql}) + SELECT COUNT(*) AS n, COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM __jc_right __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched + FROM __jc_left __jc_l` + const [row] = await rows(target, config, count, context, remaining()) + const n = Number(row?.n) + const matched = Number(row?.matched) + if (!Number.isFinite(n) || !Number.isFinite(matched)) continue + const label = `join ${refName(join.left) ?? leftAlias} → ${refName(join.right) ?? rightAlias} on ${keys.map(([l, r]) => `${l} = ${r}`).join(", ")}` + const scope = local || n < REMOTE_ROW_CEILING ? "left rows" : `left rows (first ${REMOTE_ROW_CEILING} counted)` + const effect = + kind === "LEFT" ? "they stay as unmatched rows with NULL right columns" : "the inner join drops them" + for (const line of coverageLoss(matched, n, scope, effect)) lines.push(`${label}: ${line}`) + } + return lines.length ? lines.join("\n") : undefined + } + try { + return await Promise.race([work(), timeout]) + } catch { + return undefined + } finally { + clearTimeout(timer) + } +} diff --git a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts new file mode 100644 index 0000000000..30272cbf73 --- /dev/null +++ b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts @@ -0,0 +1,238 @@ +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, test } from "bun:test" +import { Database } from "bun:sqlite" +import path from "path" +import { tmpdir } from "../../fixture/fixture" +import * as Registry from "../../../src/altimate/native/connections/registry" +import { closeScratch, getScratch, withScratchSession } from "../../../src/altimate/native/connections/scratch" +import { + JOIN_CHECK_BUDGET_MS, + autoCheckMode, + coverageLoss, + joinCoverageNote, +} from "../../../src/altimate/tools/sql-join-coverage" + +// 8 cohort members; only ids 1 and 2 have fact rows (2 of 8 = 25% kept). +const statements = [ + 'CREATE TABLE cohort ("MemberId" INTEGER, grp TEXT)', + "INSERT INTO cohort VALUES (1, 'a'), (2, 'a'), (3, 'a'), (4, 'b'), (5, 'b'), (6, 'b'), (7, 'c'), (8, 'c')", + 'CREATE TABLE facts ("MemberId" INTEGER, grp TEXT, v REAL)', + "INSERT INTO facts VALUES (1, 'a', 1.0), (1, 'a', 2.0), (2, 'a', 3.0)", + // 4 of 8 cohort members (50%) have a match: at the threshold, no note. + 'CREATE TABLE half_facts ("MemberId" INTEGER)', + "INSERT INTO half_facts VALUES (1), (2), (3), (4)", +] +const session = "join-coverage-session" +const env = (mode?: string) => (mode === undefined ? {} : { ALTIMATE_SQL_AUTO_CHECK: mode }) +const inScratch = (fn: () => Promise) => withScratchSession(session, fn) + +let fixture: Awaited> +let sqlitePath: string + +beforeAll(async () => { + fixture = await tmpdir() + sqlitePath = path.join(fixture.path, "join-coverage.sqlite") + const sqlite = new Database(sqlitePath) + try { + for (const sql of statements) sqlite.run(sql) + } finally { + sqlite.close() + } +}) + +beforeEach(async () => { + Registry.setConfigs({ + sqlite_local: { type: "sqlite", path: sqlitePath }, + remote_pg: { type: "postgres", host: "127.0.0.1", port: 1, database: "none", user: "none" }, + }) + await closeScratch() + const duckdb = await getScratch(session) + for (const sql of statements) await duckdb.execute(sql, undefined, undefined, { noLimit: true }) +}) + +afterEach(async () => { + await Registry.closeAll() +}) + +afterAll(async () => { + await closeScratch() + await fixture?.[Symbol.asyncDispose]() +}) + +describe("autoCheckMode", () => { + test("defaults to local and accepts off/all", () => { + expect(autoCheckMode({})).toBe("local") + expect(autoCheckMode({ ALTIMATE_SQL_AUTO_CHECK: "nonsense" })).toBe("local") + for (const off of ["off", "0", "false", " OFF "]) + expect(autoCheckMode({ ALTIMATE_SQL_AUTO_CHECK: off })).toBe("off") + expect(autoCheckMode({ ALTIMATE_SQL_AUTO_CHECK: "all" })).toBe("all") + }) +}) + +describe("coverageLoss", () => { + test("warns only below half", () => { + expect(coverageLoss(2, 8, "left rows", "the inner join drops them")[0]).toMatch( + /^WARNING coverage loss: 6 of 8 left rows \(75\.0%\) have no match; the inner join drops them\./, + ) + expect(coverageLoss(4, 8, "left rows", "x")).toEqual([]) + expect(coverageLoss(0, 0, "left rows", "x")).toEqual([]) + }) +}) + +for (const warehouse of ["scratch", "sqlite_local"] as const) { + describe(`joinCoverageNote on ${warehouse}`, () => { + test("inner join keeping under half leads with the counts", async () => { + const note = await inScratch(() => + joinCoverageNote( + 'SELECT c."MemberId", AVG(f.v) FROM cohort c JOIN facts f ON c."MemberId" = f."MemberId" GROUP BY 1', + warehouse, + {}, + env(), + ), + ) + expect(note).toContain("join cohort → facts on MemberId = MemberId") + expect(note).toContain( + "WARNING coverage loss: 6 of 8 left rows (75.0%) have no match; the inner join drops them.", + ) + }) + + test("left join uses NULL-row wording and multi-key conditions", async () => { + const note = await inScratch(() => + joinCoverageNote( + "SELECT * FROM cohort c LEFT JOIN facts f ON f.MemberId = c.MemberId AND c.grp = f.grp", + warehouse, + {}, + env(), + ), + ) + expect(note).toContain("on MemberId = MemberId, grp = grp") + expect(note).toContain("they stay as unmatched rows with NULL right columns") + }) + + test("half or more matched, no join, off flag and non-equality conditions stay silent", async () => { + const cases: [string, string | undefined][] = [ + ["SELECT * FROM cohort c JOIN half_facts h ON c.MemberId = h.MemberId", undefined], + ["SELECT * FROM cohort WHERE grp = 'a'", undefined], + ["SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", "off"], + ["SELECT * FROM cohort c JOIN facts f ON c.MemberId < f.MemberId", undefined], + ["SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId OR c.grp = f.grp", undefined], + ["SELECT * FROM cohort c JOIN facts f USING (MemberId)", undefined], + ["SELECT * FROM cohort c JOIN facts f ON MemberId = v", undefined], + ] + for (const [sql, mode] of cases) { + expect(await inScratch(() => joinCoverageNote(sql, warehouse, {}, env(mode)))).toBeUndefined() + } + }) + }) +} + +describe("joinCoverageNote DuckDB-only shapes", () => { + test("subqueries and CTEs on scratch are counted as written", async () => { + const note = await inScratch(() => + joinCoverageNote( + `WITH members AS (SELECT * FROM cohort WHERE grp IN ('a', 'b')) + SELECT * FROM members m JOIN (SELECT * FROM facts WHERE v > 0) f ON m.MemberId = f.MemberId`, + "scratch", + {}, + env(), + ), + ) + // 6 members in groups a/b; ids 1 and 2 match. + expect(note).toContain("WARNING coverage loss: 4 of 6 left rows (66.7%) have no match") + }) + + test("SQLite skips subquery sides instead of guessing", async () => { + const note = await inScratch(() => + joinCoverageNote( + "SELECT * FROM cohort c JOIN (SELECT * FROM facts) f ON c.MemberId = f.MemberId", + "sqlite_local", + {}, + env(), + ), + ) + expect(note).toBeUndefined() + }) +}) + +describe("joinCoverageNote policy and failure paths", () => { + test("local mode never touches a remote warehouse", async () => { + const started = Date.now() + const note = await inScratch(() => + joinCoverageNote("SELECT * FROM a x JOIN b y ON x.id = y.id", "remote_pg", {}, env()), + ) + expect(note).toBeUndefined() + // Skipped before any connection attempt: well under the check budget. + expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS) + }) + + test("errors (missing tables, unreachable warehouse) return undefined", async () => { + expect( + await inScratch(() => joinCoverageNote("SELECT * FROM nope a JOIN nada b ON a.id = b.id", "scratch", {}, env())), + ).toBeUndefined() + expect( + await inScratch(() => joinCoverageNote("SELECT * FROM a x JOIN b y ON x.id = y.id", "remote_pg", {}, env("all"))), + ).toBeUndefined() + }) + + test("a slow count is abandoned within the budget", async () => { + const duckdb = await getScratch(session) + await duckdb.execute("CREATE TABLE big AS SELECT range AS id FROM range(30000000)", undefined, undefined, { + noLimit: true, + }) + const started = Date.now() + const note = await inScratch(() => + joinCoverageNote("SELECT * FROM big a JOIN big b ON a.id = b.id", "scratch", {}, env()), + ) + expect(note).toBeUndefined() + expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS + 200) + }) +}) + +describe("sql_execute integration", () => { + test("the note reaches tool output and metadata only when coverage is low", async () => { + const { registerAll } = await import("../../../src/altimate/native/connections/register") + const { SqlExecuteTool } = await import("../../../src/altimate/tools/sql-execute") + const { MessageID, SessionID } = await import("../../../src/session/schema") + const { initTool } = await import("../tool-fixture") + registerAll() + const execute = await initTool(SqlExecuteTool) + const ctx = { + sessionID: SessionID.make("ses_join_coverage_tool"), + messageID: MessageID.make("msg_join_coverage_tool"), + agent: "analyst", + abort: AbortSignal.any([]), + messages: [], + metadata: () => {}, + ask: async () => {}, + } + const scratch = await getScratch("ses_join_coverage_tool") + for (const sql of statements) await scratch.execute(sql, undefined, undefined, { noLimit: true }) + const low = await execute.execute( + { query: "SELECT c.MemberId, f.v FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", warehouse: "scratch" }, + ctx, + ) + expect(low.output).toContain("WARNING coverage loss: 6 of 8 left rows (75.0%) have no match") + expect(low.metadata.join_coverage_warning).toBe(true) + const ok = await execute.execute( + { query: "SELECT * FROM cohort c JOIN half_facts h ON c.MemberId = h.MemberId", warehouse: "scratch" }, + ctx, + ) + expect(ok.output).not.toContain("WARNING coverage loss") + expect(ok.metadata.join_coverage_warning).toBeUndefined() + const previous = process.env.ALTIMATE_SQL_AUTO_CHECK + process.env.ALTIMATE_SQL_AUTO_CHECK = "off" + try { + const off = await execute.execute( + { query: "SELECT c.MemberId, f.v FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", warehouse: "scratch" }, + ctx, + ) + expect(off.output).not.toContain("WARNING coverage loss") + // The rows themselves are unchanged with the flag on or off. + expect(off.output.split("\n").filter((l) => /\b[12]\b/.test(l)).length).toBe( + low.output.split("\n").filter((l) => /\b[12]\b/.test(l) && !l.includes("WARNING")).length, + ) + } finally { + if (previous === undefined) delete process.env.ALTIMATE_SQL_AUTO_CHECK + else process.env.ALTIMATE_SQL_AUTO_CHECK = previous + } + }) +}) From 9d224a97fe9f9e2b8cf00760b2cd83242296946a Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 18 Oct 2026 09:31:12 -0700 Subject: [PATCH 2/5] fix: keep the join-coverage check cheap and isolated from the session engine Review found that a timeout on a local DuckDB count restarts the engine helper and drops the session's scratch state, that work could continue past the budget, and that remote-backed DuckDB passed as local. - Local counts run only on base tables in a local DuckDB database (not views, not `md:` or URL paths, not attached remote databases) whose catalog estimate is at most 2,000,000 rows, and never with a short driver timeout. Remote counts (`all` mode) get the remaining budget as their own timeout; every step checks the deadline first. - SQLite is no longer counted: without an index the semi-join is a nested loop. - Subquery sides, CTE names, column alias lists and table samples are skipped rather than rebuilt; equality operands keep their written order; remote identifiers stay unquoted when simple. - Tests: view and over-ceiling tables are skipped and scratch tables survive; local DuckDB file checked; SQLite, `md:` DuckDB and remote skipped in local mode; NULL keys never match. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/tools/sql-join-coverage.ts | 268 ++++++++---------- .../altimate/tools/sql-join-coverage.test.ts | 194 ++++++------- 2 files changed, 217 insertions(+), 245 deletions(-) diff --git a/packages/opencode/src/altimate/tools/sql-join-coverage.ts b/packages/opencode/src/altimate/tools/sql-join-coverage.ts index 015e7c55dc..530e801727 100644 --- a/packages/opencode/src/altimate/tools/sql-join-coverage.ts +++ b/packages/opencode/src/altimate/tools/sql-join-coverage.ts @@ -6,15 +6,17 @@ import { executeQuery, type QueryContext } from "../native/query-execution" /** Below this matched share of the left side, a join loses most of the population it started from. */ export const LOW_COVERAGE_SHARE = 0.5 -/** Whole-check budget: parse, count and format. Over budget, the query result is returned unchanged. */ +/** Whole-check budget: parse, catalog lookups, count and format. Over budget, nothing is added. */ export const JOIN_CHECK_BUDGET_MS = 250 +/** Local DuckDB counts run only when each side's catalog estimate is at most this many rows. */ +export const LOCAL_ROW_CEILING = 2_000_000 /** Remote warehouses (`all` mode) count at most this many left rows. */ export const REMOTE_ROW_CEILING = 100_000 const MAX_JOINS = 2 export type AutoCheckMode = "off" | "local" | "all" -/** `ALTIMATE_SQL_AUTO_CHECK`: off, local (default: scratch, DuckDB and SQLite only) or all. */ +/** `ALTIMATE_SQL_AUTO_CHECK`: off, local (default: the scratch database and local DuckDB files) or all. */ export function autoCheckMode(env: NodeJS.ProcessEnv = process.env): AutoCheckMode { const value = env["ALTIMATE_SQL_AUTO_CHECK"]?.trim().toLowerCase() if (value === "off" || value === "0" || value === "false") return "off" @@ -34,141 +36,49 @@ export function coverageLoss(matched: number, total: number, unit: string, effec } type Json = any +type Key = { left: string; right: string; leftFirst: boolean } const literal = (text: string) => `'${text.replace(/'/g, "''")}'` -const ident = (name: string) => `"${name.replace(/"/g, '""')}"` +const quoted = (name: string) => `"${name.replace(/"/g, '""')}"` +// Remote engines fold unquoted names differently; keep simple names unquoted as users usually write them. +const remoteIdent = (name: string) => (/^[A-Za-z_][A-Za-z0-9_$]*$/.test(name) ? name : quoted(name)) +const isRemotePath = (path: unknown) => + typeof path === "string" && (/^(md|motherduck):/i.test(path) || path.includes("://")) -function refName(ref: Json): string | undefined { - if (ref?.type === "BASE_TABLE") return [ref.catalog_name, ref.schema_name, ref.table_name].filter(Boolean).join(".") - if (ref?.type === "SUBQUERY") return ref.alias ? `(subquery) ${ref.alias}` : undefined - return undefined -} - -/** Equality pairs between the two sides, or undefined when the condition is anything else. */ -function equalityKeys(condition: Json, leftAlias: string, rightAlias: string): [string, string][] | undefined { +/** Equality pairs between the two sides in their written order, or undefined for any other condition. */ +function equalityKeys(condition: Json, leftAlias: string, rightAlias: string): Key[] | undefined { const conjuncts = condition?.type === "CONJUNCTION_AND" ? condition.children : condition ? [condition] : [] if (!conjuncts.length) return undefined - const keys: [string, string][] = [] + const lower = (s: string) => s.toLowerCase() + const keys: Key[] = [] for (const c of conjuncts) { if (c?.type !== "COMPARE_EQUAL" || c.left?.type !== "COLUMN_REF" || c.right?.type !== "COLUMN_REF") return undefined const a: string[] = c.left.column_names const b: string[] = c.right.column_names if (a.length !== 2 || b.length !== 2) return undefined - const lower = (s: string) => s.toLowerCase() - if (lower(a[0]) === lower(leftAlias) && lower(b[0]) === lower(rightAlias)) keys.push([a[1], b[1]]) - else if (lower(b[0]) === lower(leftAlias) && lower(a[0]) === lower(rightAlias)) keys.push([b[1], a[1]]) + if (lower(a[0]) === lower(leftAlias) && lower(b[0]) === lower(rightAlias)) + keys.push({ left: a[1], right: b[1], leftFirst: true }) + else if (lower(b[0]) === lower(leftAlias) && lower(a[0]) === lower(rightAlias)) + keys.push({ left: b[1], right: a[1], leftFirst: false }) else return undefined } return keys } -/** Join nodes whose two inputs are plain relations, in left-to-right order. */ -function relationJoins(ref: Json, out: Json[] = []): Json[] { +/** Join nodes whose two inputs are both plain base tables, in left-to-right order. */ +function tableJoins(ref: Json, out: Json[] = []): Json[] { if (ref?.type !== "JOIN") return out - relationJoins(ref.left, out) - relationJoins(ref.right, out) - if (ref.left?.type !== "JOIN" && ref.right?.type !== "JOIN") out.push(ref) + tableJoins(ref.left, out) + tableJoins(ref.right, out) + if (ref.left?.type === "BASE_TABLE" && ref.right?.type === "BASE_TABLE") out.push(ref) return out } -async function rows( - connector: Connector, - config: Parameters[0], - sql: string, - context: QueryContext, - timeoutMs: number, -) { - const response = await executeQuery( - config, - { noLimit: true, readOnly: true, queryTimeoutMs: timeoutMs }, - context, - (options) => connector.execute(sql, undefined, undefined, options), - ) - return response.rows.map((row: unknown[]) => - Object.fromEntries(response.columns.map((col: string, i: number) => [col, row[i]])), - ) -} - -/** Parse with DuckDB's own parser. Returns the statement JSON or undefined when it is not one plain SELECT. */ -async function parse(parser: Connector, sql: string, context: QueryContext, timeoutMs: number) { - const [row] = await rows( - parser, - { type: "duckdb" }, - `SELECT json_serialize_sql(${literal(sql)}) AS j`, - context, - timeoutMs, - ) - // Locations are uint64 and lose precision in JSON.parse; zero them. Any other huge number - // (e.g. a BIGINT literal) could be corrupted the same way, so such statements are skipped. - const text = String(row?.j ?? "").replace(/"query_location":\s*\d+/g, '"query_location":0') - if (/-?\d{16,}/.test(text)) return undefined - const parsed = JSON.parse(text) - if (parsed?.error || parsed?.statements?.length !== 1) return undefined - if (parsed.statements[0].node?.type !== "SELECT_NODE") return undefined - return parsed -} - -/** DuckDB targets keep subqueries and CTEs; other engines only count plain base tables. */ -async function sideSql( - parser: Connector, - statement: Json, - ref: Json, - duckdb: boolean, - context: QueryContext, - timeoutMs: number, -) { - const node = statement.statements[0].node - const cteNames = new Set((node.cte_map?.map ?? []).map((entry: Json) => String(entry.key).toLowerCase())) - if (!duckdb) { - if (ref.type !== "BASE_TABLE" || cteNames.has(String(ref.table_name).toLowerCase())) return undefined - return `SELECT * FROM ${[ref.catalog_name, ref.schema_name, ref.table_name].filter(Boolean).map(ident).join(".")}` - } - const star = { - class: "STAR", - type: "STAR", - alias: "", - query_location: 0, - relation_name: "", - exclude_list: [], - replace_list: [], - columns: false, - expr: null, - qualified_exclude_list: [], - rename_list: [], - } - const synthetic = { - ...statement, - statements: [ - { - ...statement.statements[0], - node: { - ...node, - select_list: [star], - from_table: { ...ref, alias: "" }, - where_clause: null, - group_expressions: [], - group_sets: [], - having: null, - qualify: null, - modifiers: [], - }, - }, - ], - } - const [row] = await rows( - parser, - { type: "duckdb" }, - `SELECT json_deserialize_sql(${literal(JSON.stringify(synthetic))}) AS s`, - context, - timeoutMs, - ) - return typeof row?.s === "string" && row.s ? row.s : undefined -} - /** - * Advisory note for a successful read: when an equality join keeps under half of its left side, - * say so. One bounded counting query per join (at most two), run only where policy allows; any - * failure, ambiguity or timeout returns undefined so the query result is never altered. + * Advisory note for a successful read: when an equality join between two base tables keeps + * under half of its left side, say so. One counting query per join (at most two), only where + * policy allows and only when it is cheap by construction. Every failure, ambiguity or budget + * overrun returns undefined, so the query result is never altered. */ export async function joinCoverageNote( sql: string, @@ -177,60 +87,130 @@ export async function joinCoverageNote( env: NodeJS.ProcessEnv = process.env, ): Promise { const mode = autoCheckMode(env) - if (mode === "off") return undefined + if (mode === "off" || !/\bjoin\b/i.test(sql)) return undefined const name = warehouse ?? Registry.list().warehouses[0]?.name if (!name) return undefined const config = Registry.getConfig(name) - const type = isScratch(name) ? "duckdb" : Registry.canonicalType(config?.type) - const local = type === "duckdb" || type === "sqlite" - if (!local && mode !== "all") return undefined - if (!/\bjoin\b/i.test(sql)) return undefined + const scratch = isScratch(name) + const type = scratch ? "duckdb" : Registry.canonicalType(config?.type) + // Local means a DuckDB engine on this machine: the session scratch or a local DuckDB file. + const local = type === "duckdb" && (scratch || !isRemotePath((config as { path?: unknown } | undefined)?.path)) + if (!local && (mode !== "all" || type === "duckdb" || type === "sqlite")) return undefined const started = Date.now() - const remaining = () => Math.max(0, JOIN_CHECK_BUDGET_MS - (Date.now() - started)) + const remaining = () => JOIN_CHECK_BUDGET_MS - (Date.now() - started) + const live = () => remaining() > 0 let timer: ReturnType | undefined const timeout = new Promise((resolve) => { timer = setTimeout(() => resolve(undefined), JOIN_CHECK_BUDGET_MS) }) + // Local DuckDB queries never get a short timeout: a timeout restarts the engine helper and would + // drop the session's scratch state. They are bounded by the catalog-size gate instead. Remote + // queries get the remaining budget as their own timeout. + const query = async (connector: Connector, target: typeof config, text: string, remote: boolean) => { + if (!live()) throw new Error("join coverage budget spent") + const options = remote + ? { noLimit: true, readOnly: true, queryTimeoutMs: Math.max(1, remaining()) } + : { noLimit: true, readOnly: true } + const response = await executeQuery(target, options, context, (opts) => + connector.execute(text, undefined, undefined, opts), + ) + return response.rows.map((row: unknown[]) => + Object.fromEntries(response.columns.map((col: string, i: number) => [col, row[i]])), + ) + } + const work = async () => { const target = await Registry.get(name) - const duckdb = type === "duckdb" - const parser = duckdb ? target : await Registry.get(SCRATCH_NAME) - const statement = await parse(parser, sql, context, remaining()) - if (!statement) return undefined + const parser = local ? target : await Registry.get(SCRATCH_NAME) + const parserConfig = { type: "duckdb" } as typeof config + const [parsedRow] = await query(parser, parserConfig, `SELECT json_serialize_sql(${literal(sql)}) AS j`, false) + // uint64 locations lose precision in JSON.parse; they are not needed. + const text = String(parsedRow?.j ?? "").replace(/"query_location":\s*\d+/g, '"query_location":0') + const statement = JSON.parse(text) + if (statement?.error || statement?.statements?.length !== 1) return undefined + const node = statement.statements[0].node + if (node?.type !== "SELECT_NODE") return undefined + const cteNames = new Set((node.cte_map?.map ?? []).map((entry: Json) => String(entry.key).toLowerCase())) + const lines: string[] = [] - for (const join of relationJoins(statement.statements[0].node.from_table).slice(0, MAX_JOINS)) { + for (const join of tableJoins(node.from_table).slice(0, MAX_JOINS)) { + if (!live()) break const kind = String(join.join_type).toUpperCase() if ((kind !== "INNER" && kind !== "LEFT") || join.ref_type !== "REGULAR") continue + const sides = [join.left, join.right] + // Column alias lists rename columns; CTE names are not tables; samples change row counts. + if ( + sides.some((s) => s.column_name_alias?.length || s.sample || cteNames.has(String(s.table_name).toLowerCase())) + ) + continue const leftAlias = join.left.alias || join.left.table_name const rightAlias = join.right.alias || join.right.table_name - if (!leftAlias || !rightAlias) continue const keys = equalityKeys(join.condition, leftAlias, rightAlias) if (!keys?.length) continue - const leftSql = await sideSql(parser, statement, join.left, duckdb, context, remaining()) - const rightSql = await sideSql(parser, statement, join.right, duckdb, context, remaining()) - if (!leftSql || !rightSql) continue - const bounded = local ? leftSql : `SELECT * FROM (${leftSql}) AS __jc_src LIMIT ${REMOTE_ROW_CEILING}` - const match = keys.map(([l, r]) => `__jc_r.${ident(r)} = __jc_l.${ident(l)}`).join(" AND ") - const count = `WITH __jc_left AS (${bounded}), __jc_right AS (${rightSql}) - SELECT COUNT(*) AS n, COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM __jc_right __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched - FROM __jc_left __jc_l` - const [row] = await rows(target, config, count, context, remaining()) + + if (local) { + // Count only local base tables (not views, not remote attachments) under the size ceiling. + let small = true + for (const side of sides) { + const where = [ + `lower(t.table_name) = lower(${literal(side.table_name)})`, + side.schema_name ? `lower(t.schema_name) = lower(${literal(side.schema_name)})` : "", + side.catalog_name ? `lower(t.database_name) = lower(${literal(side.catalog_name)})` : "", + ].filter(Boolean) + const found = await query( + target, + config, + `SELECT d.type AS type, d.path AS path, t.estimated_size AS size + FROM duckdb_tables() t JOIN duckdb_databases() d USING (database_name) WHERE ${where.join(" AND ")}`, + false, + ) + const [table] = found + if ( + found.length !== 1 || + String(table.type).toLowerCase() !== "duckdb" || + isRemotePath(table.path) || + !(Number(table.size) <= LOCAL_ROW_CEILING) + ) { + small = false + break + } + } + if (!small) continue + } + + const ident = local ? quoted : remoteIdent + const relation = (side: Json) => + [side.catalog_name, side.schema_name, side.table_name].filter(Boolean).map(ident).join(".") + const leftSource = local + ? `SELECT * FROM ${relation(join.left)}` + : `SELECT * FROM ${relation(join.left)} LIMIT ${REMOTE_ROW_CEILING}` + const match = keys + .map((k) => + k.leftFirst + ? `__jc_l.${ident(k.left)} = __jc_r.${ident(k.right)}` + : `__jc_r.${ident(k.right)} = __jc_l.${ident(k.left)}`, + ) + .join(" AND ") + const count = `SELECT COUNT(*) AS n, + COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM ${relation(join.right)} __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched + FROM (${leftSource}) __jc_l` + const [row] = await query(target, config, count, !local) + if (!live()) break const n = Number(row?.n) const matched = Number(row?.matched) if (!Number.isFinite(n) || !Number.isFinite(matched)) continue - const label = `join ${refName(join.left) ?? leftAlias} → ${refName(join.right) ?? rightAlias} on ${keys.map(([l, r]) => `${l} = ${r}`).join(", ")}` - const scope = local || n < REMOTE_ROW_CEILING ? "left rows" : `left rows (first ${REMOTE_ROW_CEILING} counted)` + const plain = (side: Json) => [side.catalog_name, side.schema_name, side.table_name].filter(Boolean).join(".") + const label = `join ${plain(join.left)} → ${plain(join.right)} on ${keys.map((k) => `${k.left} = ${k.right}`).join(", ")}` + const unit = local || n < REMOTE_ROW_CEILING ? "left rows" : `left rows (first ${REMOTE_ROW_CEILING} counted)` const effect = kind === "LEFT" ? "they stay as unmatched rows with NULL right columns" : "the inner join drops them" - for (const line of coverageLoss(matched, n, scope, effect)) lines.push(`${label}: ${line}`) + for (const line of coverageLoss(matched, n, unit, effect)) lines.push(`${label}: ${line}`) } return lines.length ? lines.join("\n") : undefined } try { - return await Promise.race([work(), timeout]) - } catch { - return undefined + return await Promise.race([work().catch(() => undefined), timeout]) } finally { clearTimeout(timer) } diff --git a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts index 30272cbf73..54576b6017 100644 --- a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts +++ b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts @@ -6,17 +6,18 @@ import * as Registry from "../../../src/altimate/native/connections/registry" import { closeScratch, getScratch, withScratchSession } from "../../../src/altimate/native/connections/scratch" import { JOIN_CHECK_BUDGET_MS, + LOCAL_ROW_CEILING, autoCheckMode, coverageLoss, joinCoverageNote, } from "../../../src/altimate/tools/sql-join-coverage" -// 8 cohort members; only ids 1 and 2 have fact rows (2 of 8 = 25% kept). +// 8 cohort members; only ids 1 and 2 have fact rows (2 of 8 = 25% kept). NULL keys never match. const statements = [ 'CREATE TABLE cohort ("MemberId" INTEGER, grp TEXT)', - "INSERT INTO cohort VALUES (1, 'a'), (2, 'a'), (3, 'a'), (4, 'b'), (5, 'b'), (6, 'b'), (7, 'c'), (8, 'c')", + "INSERT INTO cohort VALUES (1, 'a'), (2, 'a'), (3, 'a'), (4, 'b'), (5, 'b'), (6, 'b'), (7, 'c'), (NULL, 'c')", 'CREATE TABLE facts ("MemberId" INTEGER, grp TEXT, v REAL)', - "INSERT INTO facts VALUES (1, 'a', 1.0), (1, 'a', 2.0), (2, 'a', 3.0)", + "INSERT INTO facts VALUES (1, 'a', 1.0), (1, 'a', 2.0), (2, 'a', 3.0), (NULL, 'c', 4.0)", // 4 of 8 cohort members (50%) have a match: at the threshold, no note. 'CREATE TABLE half_facts ("MemberId" INTEGER)', "INSERT INTO half_facts VALUES (1), (2), (3), (4)", @@ -24,13 +25,16 @@ const statements = [ const session = "join-coverage-session" const env = (mode?: string) => (mode === undefined ? {} : { ALTIMATE_SQL_AUTO_CHECK: mode }) const inScratch = (fn: () => Promise) => withScratchSession(session, fn) +const lowJoin = 'SELECT c."MemberId", AVG(f.v) FROM cohort c JOIN facts f ON c."MemberId" = f."MemberId" GROUP BY 1' let fixture: Awaited> let sqlitePath: string +let duckdbPath: string beforeAll(async () => { fixture = await tmpdir() sqlitePath = path.join(fixture.path, "join-coverage.sqlite") + duckdbPath = path.join(fixture.path, "join-coverage.duckdb") const sqlite = new Database(sqlitePath) try { for (const sql of statements) sqlite.run(sql) @@ -42,6 +46,8 @@ beforeAll(async () => { beforeEach(async () => { Registry.setConfigs({ sqlite_local: { type: "sqlite", path: sqlitePath }, + duckdb_local: { type: "duckdb", path: duckdbPath, create: true }, + duckdb_cloud: { type: "duckdb", path: "md:analytics" }, remote_pg: { type: "postgres", host: "127.0.0.1", port: 1, database: "none", user: "none" }, }) await closeScratch() @@ -78,112 +84,104 @@ describe("coverageLoss", () => { }) }) -for (const warehouse of ["scratch", "sqlite_local"] as const) { - describe(`joinCoverageNote on ${warehouse}`, () => { - test("inner join keeping under half leads with the counts", async () => { - const note = await inScratch(() => - joinCoverageNote( - 'SELECT c."MemberId", AVG(f.v) FROM cohort c JOIN facts f ON c."MemberId" = f."MemberId" GROUP BY 1', - warehouse, - {}, - env(), - ), - ) - expect(note).toContain("join cohort → facts on MemberId = MemberId") - expect(note).toContain( - "WARNING coverage loss: 6 of 8 left rows (75.0%) have no match; the inner join drops them.", - ) - }) - - test("left join uses NULL-row wording and multi-key conditions", async () => { - const note = await inScratch(() => - joinCoverageNote( - "SELECT * FROM cohort c LEFT JOIN facts f ON f.MemberId = c.MemberId AND c.grp = f.grp", - warehouse, - {}, - env(), - ), - ) - expect(note).toContain("on MemberId = MemberId, grp = grp") - expect(note).toContain("they stay as unmatched rows with NULL right columns") - }) - - test("half or more matched, no join, off flag and non-equality conditions stay silent", async () => { - const cases: [string, string | undefined][] = [ - ["SELECT * FROM cohort c JOIN half_facts h ON c.MemberId = h.MemberId", undefined], - ["SELECT * FROM cohort WHERE grp = 'a'", undefined], - ["SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", "off"], - ["SELECT * FROM cohort c JOIN facts f ON c.MemberId < f.MemberId", undefined], - ["SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId OR c.grp = f.grp", undefined], - ["SELECT * FROM cohort c JOIN facts f USING (MemberId)", undefined], - ["SELECT * FROM cohort c JOIN facts f ON MemberId = v", undefined], - ] - for (const [sql, mode] of cases) { - expect(await inScratch(() => joinCoverageNote(sql, warehouse, {}, env(mode)))).toBeUndefined() - } - }) +describe("joinCoverageNote on the scratch database", () => { + test("inner join keeping under half leads with the counts; NULL keys never match", async () => { + const note = await inScratch(() => joinCoverageNote(lowJoin, "scratch", {}, env())) + expect(note).toContain("join cohort → facts on MemberId = MemberId") + expect(note).toContain("WARNING coverage loss: 6 of 8 left rows (75.0%) have no match; the inner join drops them.") }) -} -describe("joinCoverageNote DuckDB-only shapes", () => { - test("subqueries and CTEs on scratch are counted as written", async () => { + test("left join wording, multi-key and reversed operands", async () => { const note = await inScratch(() => joinCoverageNote( - `WITH members AS (SELECT * FROM cohort WHERE grp IN ('a', 'b')) - SELECT * FROM members m JOIN (SELECT * FROM facts WHERE v > 0) f ON m.MemberId = f.MemberId`, + "SELECT * FROM cohort c LEFT JOIN facts f ON f.MemberId = c.MemberId AND c.grp = f.grp", "scratch", {}, env(), ), ) - // 6 members in groups a/b; ids 1 and 2 match. - expect(note).toContain("WARNING coverage loss: 4 of 6 left rows (66.7%) have no match") + expect(note).toContain("on MemberId = MemberId, grp = grp") + expect(note).toContain("6 of 8 left rows") + expect(note).toContain("they stay as unmatched rows with NULL right columns") }) - test("SQLite skips subquery sides instead of guessing", async () => { - const note = await inScratch(() => - joinCoverageNote( - "SELECT * FROM cohort c JOIN (SELECT * FROM facts) f ON c.MemberId = f.MemberId", - "sqlite_local", - {}, - env(), - ), - ) - expect(note).toBeUndefined() + test("stays silent when coverage is fine or the shape is not a plain table equality join", async () => { + const cases: [string, string | undefined][] = [ + ["SELECT * FROM cohort c JOIN half_facts h ON c.MemberId = h.MemberId", undefined], + ["SELECT * FROM cohort WHERE grp = 'a'", undefined], + [lowJoin, "off"], + ["SELECT * FROM cohort c JOIN facts f ON c.MemberId < f.MemberId", undefined], + ["SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId OR c.grp = f.grp", undefined], + ["SELECT * FROM cohort c JOIN facts f USING (MemberId)", undefined], + ["SELECT * FROM cohort c JOIN facts f ON MemberId = v", undefined], + // Subquery sides, CTE names and column alias lists are skipped instead of rebuilt. + ["SELECT * FROM cohort c JOIN (SELECT * FROM facts) f ON c.MemberId = f.MemberId", undefined], + [ + "WITH facts AS (SELECT 99 AS MemberId) SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", + undefined, + ], + ["SELECT * FROM cohort c(x, y) JOIN facts f ON c.x = f.MemberId", undefined], + // Parse failures never produce a note. + ["SELEC broken JOIN", undefined], + ] + for (const [sql, mode] of cases) { + expect(await inScratch(() => joinCoverageNote(sql, "scratch", {}, env(mode)))).toBeUndefined() + } }) -}) -describe("joinCoverageNote policy and failure paths", () => { - test("local mode never touches a remote warehouse", async () => { - const started = Date.now() - const note = await inScratch(() => - joinCoverageNote("SELECT * FROM a x JOIN b y ON x.id = y.id", "remote_pg", {}, env()), + test("views and tables over the size ceiling are not counted, and scratch state survives", async () => { + const duckdb = await getScratch(session) + await duckdb.execute("CREATE VIEW cohort_view AS SELECT * FROM cohort", undefined, undefined, { noLimit: true }) + await duckdb.execute( + `CREATE TABLE big AS SELECT range AS "MemberId" FROM range(${LOCAL_ROW_CEILING + 1000})`, + undefined, + undefined, + { noLimit: true }, ) - expect(note).toBeUndefined() - // Skipped before any connection attempt: well under the check budget. - expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS) - }) - - test("errors (missing tables, unreachable warehouse) return undefined", async () => { expect( - await inScratch(() => joinCoverageNote("SELECT * FROM nope a JOIN nada b ON a.id = b.id", "scratch", {}, env())), + await inScratch(() => + joinCoverageNote("SELECT * FROM cohort_view c JOIN facts f ON c.MemberId = f.MemberId", "scratch", {}, env()), + ), ).toBeUndefined() + const started = Date.now() expect( - await inScratch(() => joinCoverageNote("SELECT * FROM a x JOIN b y ON x.id = y.id", "remote_pg", {}, env("all"))), + await inScratch(() => + joinCoverageNote("SELECT * FROM big b JOIN facts f ON b.MemberId = f.MemberId", "scratch", {}, env()), + ), ).toBeUndefined() + expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS + 200) + // The engine was never interrupted: earlier tables are still there. + const rows = await duckdb.execute("SELECT COUNT(*) AS n FROM cohort", undefined, undefined, { noLimit: true }) + expect(Number(rows.rows[0][0])).toBe(8) }) +}) - test("a slow count is abandoned within the budget", async () => { - const duckdb = await getScratch(session) - await duckdb.execute("CREATE TABLE big AS SELECT range AS id FROM range(30000000)", undefined, undefined, { - noLimit: true, - }) - const started = Date.now() - const note = await inScratch(() => - joinCoverageNote("SELECT * FROM big a JOIN big b ON a.id = b.id", "scratch", {}, env()), - ) - expect(note).toBeUndefined() - expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS + 200) +describe("joinCoverageNote warehouse policy", () => { + test("a local DuckDB file is checked in local mode", async () => { + const connector = await Registry.get("duckdb_local") + for (const sql of statements) await connector.execute(sql, undefined, undefined, { noLimit: true }) + expect(await inScratch(() => joinCoverageNote(lowJoin, "duckdb_local", {}, env()))).toContain("6 of 8 left rows") + }) + + test("SQLite, cloud DuckDB and remote warehouses are skipped in local mode", async () => { + for (const warehouse of ["sqlite_local", "duckdb_cloud", "remote_pg"]) { + const started = Date.now() + expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, {}, env()))).toBeUndefined() + // Skipped before any connection attempt. + expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS) + } + }) + + test("all mode never counts SQLite or cloud DuckDB, and an unreachable remote returns undefined", async () => { + for (const warehouse of ["sqlite_local", "duckdb_cloud", "remote_pg"]) { + expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, {}, env("all")))).toBeUndefined() + } + }) + + test("missing tables return undefined", async () => { + expect( + await inScratch(() => joinCoverageNote("SELECT * FROM nope a JOIN nada b ON a.id = b.id", "scratch", {}, env())), + ).toBeUndefined() }) }) @@ -206,10 +204,8 @@ describe("sql_execute integration", () => { } const scratch = await getScratch("ses_join_coverage_tool") for (const sql of statements) await scratch.execute(sql, undefined, undefined, { noLimit: true }) - const low = await execute.execute( - { query: "SELECT c.MemberId, f.v FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", warehouse: "scratch" }, - ctx, - ) + const query = "SELECT c.MemberId, f.v FROM cohort c JOIN facts f ON c.MemberId = f.MemberId ORDER BY 1, 2" + const low = await execute.execute({ query, warehouse: "scratch" }, ctx) expect(low.output).toContain("WARNING coverage loss: 6 of 8 left rows (75.0%) have no match") expect(low.metadata.join_coverage_warning).toBe(true) const ok = await execute.execute( @@ -221,15 +217,11 @@ describe("sql_execute integration", () => { const previous = process.env.ALTIMATE_SQL_AUTO_CHECK process.env.ALTIMATE_SQL_AUTO_CHECK = "off" try { - const off = await execute.execute( - { query: "SELECT c.MemberId, f.v FROM cohort c JOIN facts f ON c.MemberId = f.MemberId", warehouse: "scratch" }, - ctx, - ) + const off = await execute.execute({ query, warehouse: "scratch" }, ctx) expect(off.output).not.toContain("WARNING coverage loss") - // The rows themselves are unchanged with the flag on or off. - expect(off.output.split("\n").filter((l) => /\b[12]\b/.test(l)).length).toBe( - low.output.split("\n").filter((l) => /\b[12]\b/.test(l) && !l.includes("WARNING")).length, - ) + // The rows are identical with the check on or off. + expect(off.metadata.rowCount).toBe(low.metadata.rowCount) + expect(low.output).toContain(off.output.split("\n")[0]) } finally { if (previous === undefined) delete process.env.ALTIMATE_SQL_AUTO_CHECK else process.env.ALTIMATE_SQL_AUTO_CHECK = previous From cf12a3facfd4ef05398dde8f2d57f9143a02afa7 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 18 Oct 2026 09:37:25 -0700 Subject: [PATCH 3/5] fix: limit the join-coverage check to small local DuckDB tables, never interrupted Second review found the count could still inherit a session deadline and restart the engine, run on after the budget, read a view that shadows the gated table, and mis-resolve quoted names on remote warehouses. - Only the scratch database and local DuckDB files are checked; remote warehouses are never queried and the `all` mode is removed. `ALTIMATE_SQL_AUTO_CHECK=off` disables the check. - Queries go straight to the connector with no timeout, deadline or abort signal, so the engine is never interrupted. Each side must resolve to exactly one local DuckDB base table of at most 500,000 rows (catalog estimate), with no view of the same name anywhere; a semi-join at that size takes milliseconds even if the budget lapses. - Skips when no warehouse was named, since the query may have run through another adapter. - Tests: shadowing view, explicit-warehouse requirement, flag values. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/tools/sql-execute.ts | 5 +- .../src/altimate/tools/sql-join-coverage.ts | 182 ++++++++---------- .../altimate/tools/sql-join-coverage.test.ts | 38 ++-- 3 files changed, 103 insertions(+), 122 deletions(-) diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index d38706a1f1..02f73525c7 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -363,10 +363,7 @@ export const SqlExecuteTool = Tool.define( const suggestion = ctx.agent === "analyst" ? null : PostConnectSuggestions.getProgressiveSuggestion("sql_execute") const lint = await lintSqlResult(sql, args.warehouse, ctx.sessionID) // altimate_change start — advisory join-coverage note on successful reads; never alters the rows - const coverage = - !mongo && queryType === "read" - ? await joinCoverageNote(sql, args.warehouse, { signal: ctx.abort, sessionID: ctx.sessionID }) - : undefined + const coverage = !mongo && queryType === "read" ? await joinCoverageNote(sql, args.warehouse) : undefined // altimate_change end const formatted = await formatSqlResult(result, { limit, diff --git a/packages/opencode/src/altimate/tools/sql-join-coverage.ts b/packages/opencode/src/altimate/tools/sql-join-coverage.ts index 530e801727..22d632d747 100644 --- a/packages/opencode/src/altimate/tools/sql-join-coverage.ts +++ b/packages/opencode/src/altimate/tools/sql-join-coverage.ts @@ -1,27 +1,24 @@ // altimate_change - new file import type { Connector } from "@altimateai/drivers" import * as Registry from "../native/connections/registry" -import { SCRATCH_NAME, isScratch } from "../native/connections/scratch" -import { executeQuery, type QueryContext } from "../native/query-execution" +import { isScratch } from "../native/connections/scratch" /** Below this matched share of the left side, a join loses most of the population it started from. */ export const LOW_COVERAGE_SHARE = 0.5 -/** Whole-check budget: parse, catalog lookups, count and format. Over budget, nothing is added. */ +/** Whole-check budget. Past it the note is dropped and the query result is returned unchanged. */ export const JOIN_CHECK_BUDGET_MS = 250 -/** Local DuckDB counts run only when each side's catalog estimate is at most this many rows. */ -export const LOCAL_ROW_CEILING = 2_000_000 -/** Remote warehouses (`all` mode) count at most this many left rows. */ -export const REMOTE_ROW_CEILING = 100_000 +/** + * Each side must be a local DuckDB base table with at most this many rows (catalog estimate). + * The check never interrupts the engine, so this ceiling is what keeps its work short (a + * semi-join at this size takes milliseconds) even when the budget has already passed. + */ +export const LOCAL_ROW_CEILING = 500_000 const MAX_JOINS = 2 -export type AutoCheckMode = "off" | "local" | "all" - -/** `ALTIMATE_SQL_AUTO_CHECK`: off, local (default: the scratch database and local DuckDB files) or all. */ -export function autoCheckMode(env: NodeJS.ProcessEnv = process.env): AutoCheckMode { +/** `ALTIMATE_SQL_AUTO_CHECK=off` disables the check; any other value keeps it on for local DuckDB only. */ +export function autoCheckEnabled(env: NodeJS.ProcessEnv = process.env): boolean { const value = env["ALTIMATE_SQL_AUTO_CHECK"]?.trim().toLowerCase() - if (value === "off" || value === "0" || value === "false") return "off" - if (value === "all") return "all" - return "local" + return value !== "off" && value !== "0" && value !== "false" } /** A leading warning when most of the left side has no match, so a large silent loss is not reported as fact. */ @@ -39,9 +36,8 @@ type Json = any type Key = { left: string; right: string; leftFirst: boolean } const literal = (text: string) => `'${text.replace(/'/g, "''")}'` -const quoted = (name: string) => `"${name.replace(/"/g, '""')}"` -// Remote engines fold unquoted names differently; keep simple names unquoted as users usually write them. -const remoteIdent = (name: string) => (/^[A-Za-z_][A-Za-z0-9_$]*$/.test(name) ? name : quoted(name)) +// DuckDB resolves quoted identifiers case-insensitively, so quoting every part is safe. +const ident = (name: string) => `"${name.replace(/"/g, '""')}"` const isRemotePath = (path: unknown) => typeof path === "string" && (/^(md|motherduck):/i.test(path) || path.includes("://")) @@ -65,7 +61,7 @@ function equalityKeys(condition: Json, leftAlias: string, rightAlias: string): K return keys } -/** Join nodes whose two inputs are both plain base tables, in left-to-right order. */ +/** Join nodes whose two inputs are both plain base-table references, in left-to-right order. */ function tableJoins(ref: Json, out: Json[] = []): Json[] { if (ref?.type !== "JOIN") return out tableJoins(ref.left, out) @@ -74,60 +70,74 @@ function tableJoins(ref: Json, out: Json[] = []): Json[] { return out } +async function rows(connector: Connector, sql: string): Promise[]> { + // No timeout, session deadline or abort signal: interrupting a local engine restarts its helper + // and drops session state. The catalog-size gate bounds the work instead. + const response = await connector.execute(sql, undefined, undefined, { noLimit: true, readOnly: true }) + return response.rows.map((row: unknown[]) => + Object.fromEntries(response.columns.map((col: string, i: number) => [col, row[i]])), + ) +} + /** - * Advisory note for a successful read: when an equality join between two base tables keeps - * under half of its left side, say so. One counting query per join (at most two), only where - * policy allows and only when it is cheap by construction. Every failure, ambiguity or budget - * overrun returns undefined, so the query result is never altered. + * The relation is admitted only when its name resolves to exactly one local DuckDB base table + * under the size ceiling, and no view anywhere shares the name (a temporary view could shadow it). + */ +async function smallLocalTable(connector: Connector, side: Json): Promise { + const name = literal(String(side.table_name)) + const scope = [ + side.schema_name ? `AND lower(schema_name) = lower(${literal(side.schema_name)})` : "", + side.catalog_name ? `AND lower(database_name) = lower(${literal(side.catalog_name)})` : "", + ].join(" ") + const [found] = await rows( + connector, + `SELECT + (SELECT COUNT(*) FROM duckdb_views() WHERE NOT internal AND lower(view_name) = lower(${name})) AS views, + (SELECT COUNT(*) FROM duckdb_tables() WHERE lower(table_name) = lower(${name}) ${scope}) AS tables, + (SELECT MAX(t.estimated_size) FROM duckdb_tables() t WHERE lower(t.table_name) = lower(${name}) ${scope}) AS size, + (SELECT COUNT(*) FROM duckdb_tables() t JOIN duckdb_databases() d USING (database_name) + WHERE lower(t.table_name) = lower(${name}) ${scope} + AND (lower(d.type) <> 'duckdb' OR (d.path IS NOT NULL AND (d.path ILIKE 'md:%' OR d.path ILIKE 'motherduck:%' OR d.path LIKE '%://%')))) AS remote`, + ) + return ( + Number(found?.views) === 0 && + Number(found?.tables) === 1 && + Number(found?.remote) === 0 && + Number(found?.size) <= LOCAL_ROW_CEILING + ) +} + +/** + * Advisory note for a successful read on the session scratch database or a local DuckDB file: + * when an equality join between two small base tables keeps under half of its left side, say + * so. Every failure, ambiguity or budget overrun returns undefined, so the result is never altered. */ export async function joinCoverageNote( sql: string, warehouse: string | undefined, - context: QueryContext = {}, env: NodeJS.ProcessEnv = process.env, ): Promise { - const mode = autoCheckMode(env) - if (mode === "off" || !/\bjoin\b/i.test(sql)) return undefined - const name = warehouse ?? Registry.list().warehouses[0]?.name - if (!name) return undefined - const config = Registry.getConfig(name) - const scratch = isScratch(name) + // Without an explicit warehouse the query may have run through another adapter; skip. + if (!warehouse || !autoCheckEnabled(env) || !/\bjoin\b/i.test(sql)) return undefined + const config = Registry.getConfig(warehouse) + const scratch = isScratch(warehouse) const type = scratch ? "duckdb" : Registry.canonicalType(config?.type) - // Local means a DuckDB engine on this machine: the session scratch or a local DuckDB file. - const local = type === "duckdb" && (scratch || !isRemotePath((config as { path?: unknown } | undefined)?.path)) - if (!local && (mode !== "all" || type === "duckdb" || type === "sqlite")) return undefined + if (type !== "duckdb" || (!scratch && isRemotePath((config as { path?: unknown } | undefined)?.path))) + return undefined const started = Date.now() - const remaining = () => JOIN_CHECK_BUDGET_MS - (Date.now() - started) - const live = () => remaining() > 0 + const live = () => Date.now() - started < JOIN_CHECK_BUDGET_MS let timer: ReturnType | undefined const timeout = new Promise((resolve) => { timer = setTimeout(() => resolve(undefined), JOIN_CHECK_BUDGET_MS) }) - // Local DuckDB queries never get a short timeout: a timeout restarts the engine helper and would - // drop the session's scratch state. They are bounded by the catalog-size gate instead. Remote - // queries get the remaining budget as their own timeout. - const query = async (connector: Connector, target: typeof config, text: string, remote: boolean) => { - if (!live()) throw new Error("join coverage budget spent") - const options = remote - ? { noLimit: true, readOnly: true, queryTimeoutMs: Math.max(1, remaining()) } - : { noLimit: true, readOnly: true } - const response = await executeQuery(target, options, context, (opts) => - connector.execute(text, undefined, undefined, opts), - ) - return response.rows.map((row: unknown[]) => - Object.fromEntries(response.columns.map((col: string, i: number) => [col, row[i]])), - ) - } const work = async () => { - const target = await Registry.get(name) - const parser = local ? target : await Registry.get(SCRATCH_NAME) - const parserConfig = { type: "duckdb" } as typeof config - const [parsedRow] = await query(parser, parserConfig, `SELECT json_serialize_sql(${literal(sql)}) AS j`, false) + const connector = await Registry.get(warehouse) + if (!live()) return undefined + const [parsed] = await rows(connector, `SELECT json_serialize_sql(${literal(sql)}) AS j`) // uint64 locations lose precision in JSON.parse; they are not needed. - const text = String(parsedRow?.j ?? "").replace(/"query_location":\s*\d+/g, '"query_location":0') - const statement = JSON.parse(text) + const statement = JSON.parse(String(parsed?.j ?? "").replace(/"query_location":\s*\d+/g, '"query_location":0')) if (statement?.error || statement?.statements?.length !== 1) return undefined const node = statement.statements[0].node if (node?.type !== "SELECT_NODE") return undefined @@ -135,7 +145,7 @@ export async function joinCoverageNote( const lines: string[] = [] for (const join of tableJoins(node.from_table).slice(0, MAX_JOINS)) { - if (!live()) break + if (!live()) return undefined const kind = String(join.join_type).toUpperCase() if ((kind !== "INNER" && kind !== "LEFT") || join.ref_type !== "REGULAR") continue const sides = [join.left, join.right] @@ -144,47 +154,17 @@ export async function joinCoverageNote( sides.some((s) => s.column_name_alias?.length || s.sample || cteNames.has(String(s.table_name).toLowerCase())) ) continue - const leftAlias = join.left.alias || join.left.table_name - const rightAlias = join.right.alias || join.right.table_name - const keys = equalityKeys(join.condition, leftAlias, rightAlias) + const keys = equalityKeys( + join.condition, + join.left.alias || join.left.table_name, + join.right.alias || join.right.table_name, + ) if (!keys?.length) continue + if (!(await smallLocalTable(connector, join.left)) || !live()) continue + if (!(await smallLocalTable(connector, join.right)) || !live()) continue - if (local) { - // Count only local base tables (not views, not remote attachments) under the size ceiling. - let small = true - for (const side of sides) { - const where = [ - `lower(t.table_name) = lower(${literal(side.table_name)})`, - side.schema_name ? `lower(t.schema_name) = lower(${literal(side.schema_name)})` : "", - side.catalog_name ? `lower(t.database_name) = lower(${literal(side.catalog_name)})` : "", - ].filter(Boolean) - const found = await query( - target, - config, - `SELECT d.type AS type, d.path AS path, t.estimated_size AS size - FROM duckdb_tables() t JOIN duckdb_databases() d USING (database_name) WHERE ${where.join(" AND ")}`, - false, - ) - const [table] = found - if ( - found.length !== 1 || - String(table.type).toLowerCase() !== "duckdb" || - isRemotePath(table.path) || - !(Number(table.size) <= LOCAL_ROW_CEILING) - ) { - small = false - break - } - } - if (!small) continue - } - - const ident = local ? quoted : remoteIdent const relation = (side: Json) => [side.catalog_name, side.schema_name, side.table_name].filter(Boolean).map(ident).join(".") - const leftSource = local - ? `SELECT * FROM ${relation(join.left)}` - : `SELECT * FROM ${relation(join.left)} LIMIT ${REMOTE_ROW_CEILING}` const match = keys .map((k) => k.leftFirst @@ -192,22 +172,22 @@ export async function joinCoverageNote( : `__jc_r.${ident(k.right)} = __jc_l.${ident(k.left)}`, ) .join(" AND ") - const count = `SELECT COUNT(*) AS n, - COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM ${relation(join.right)} __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched - FROM (${leftSource}) __jc_l` - const [row] = await query(target, config, count, !local) - if (!live()) break + const [row] = await rows( + connector, + `SELECT COUNT(*) AS n, + COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM ${relation(join.right)} __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched + FROM ${relation(join.left)} __jc_l`, + ) const n = Number(row?.n) const matched = Number(row?.matched) if (!Number.isFinite(n) || !Number.isFinite(matched)) continue const plain = (side: Json) => [side.catalog_name, side.schema_name, side.table_name].filter(Boolean).join(".") const label = `join ${plain(join.left)} → ${plain(join.right)} on ${keys.map((k) => `${k.left} = ${k.right}`).join(", ")}` - const unit = local || n < REMOTE_ROW_CEILING ? "left rows" : `left rows (first ${REMOTE_ROW_CEILING} counted)` const effect = kind === "LEFT" ? "they stay as unmatched rows with NULL right columns" : "the inner join drops them" - for (const line of coverageLoss(matched, n, unit, effect)) lines.push(`${label}: ${line}`) + for (const line of coverageLoss(matched, n, "left rows", effect)) lines.push(`${label}: ${line}`) } - return lines.length ? lines.join("\n") : undefined + return lines.length && live() ? lines.join("\n") : undefined } try { return await Promise.race([work().catch(() => undefined), timeout]) diff --git a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts index 54576b6017..7a0bf44c48 100644 --- a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts +++ b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts @@ -7,7 +7,7 @@ import { closeScratch, getScratch, withScratchSession } from "../../../src/altim import { JOIN_CHECK_BUDGET_MS, LOCAL_ROW_CEILING, - autoCheckMode, + autoCheckEnabled, coverageLoss, joinCoverageNote, } from "../../../src/altimate/tools/sql-join-coverage" @@ -64,13 +64,12 @@ afterAll(async () => { await fixture?.[Symbol.asyncDispose]() }) -describe("autoCheckMode", () => { - test("defaults to local and accepts off/all", () => { - expect(autoCheckMode({})).toBe("local") - expect(autoCheckMode({ ALTIMATE_SQL_AUTO_CHECK: "nonsense" })).toBe("local") +describe("autoCheckEnabled", () => { + test("on by default; off, 0 or false disable it", () => { + expect(autoCheckEnabled({})).toBe(true) + expect(autoCheckEnabled({ ALTIMATE_SQL_AUTO_CHECK: "all" })).toBe(true) for (const off of ["off", "0", "false", " OFF "]) - expect(autoCheckMode({ ALTIMATE_SQL_AUTO_CHECK: off })).toBe("off") - expect(autoCheckMode({ ALTIMATE_SQL_AUTO_CHECK: "all" })).toBe("all") + expect(autoCheckEnabled({ ALTIMATE_SQL_AUTO_CHECK: off })).toBe(false) }) }) @@ -86,7 +85,7 @@ describe("coverageLoss", () => { describe("joinCoverageNote on the scratch database", () => { test("inner join keeping under half leads with the counts; NULL keys never match", async () => { - const note = await inScratch(() => joinCoverageNote(lowJoin, "scratch", {}, env())) + const note = await inScratch(() => joinCoverageNote(lowJoin, "scratch", env())) expect(note).toContain("join cohort → facts on MemberId = MemberId") expect(note).toContain("WARNING coverage loss: 6 of 8 left rows (75.0%) have no match; the inner join drops them.") }) @@ -96,7 +95,6 @@ describe("joinCoverageNote on the scratch database", () => { joinCoverageNote( "SELECT * FROM cohort c LEFT JOIN facts f ON f.MemberId = c.MemberId AND c.grp = f.grp", "scratch", - {}, env(), ), ) @@ -125,7 +123,7 @@ describe("joinCoverageNote on the scratch database", () => { ["SELEC broken JOIN", undefined], ] for (const [sql, mode] of cases) { - expect(await inScratch(() => joinCoverageNote(sql, "scratch", {}, env(mode)))).toBeUndefined() + expect(await inScratch(() => joinCoverageNote(sql, "scratch", env(mode)))).toBeUndefined() } }) @@ -140,13 +138,17 @@ describe("joinCoverageNote on the scratch database", () => { ) expect( await inScratch(() => - joinCoverageNote("SELECT * FROM cohort_view c JOIN facts f ON c.MemberId = f.MemberId", "scratch", {}, env()), + joinCoverageNote("SELECT * FROM cohort_view c JOIN facts f ON c.MemberId = f.MemberId", "scratch", env()), ), ).toBeUndefined() + // A view elsewhere sharing a table's name could be what DuckDB resolves: skip the join. + await duckdb.execute("CREATE SCHEMA shadow", undefined, undefined, { noLimit: true }) + await duckdb.execute("CREATE VIEW shadow.facts AS SELECT * FROM big", undefined, undefined, { noLimit: true }) + expect(await inScratch(() => joinCoverageNote(lowJoin, "scratch", env()))).toBeUndefined() const started = Date.now() expect( await inScratch(() => - joinCoverageNote("SELECT * FROM big b JOIN facts f ON b.MemberId = f.MemberId", "scratch", {}, env()), + joinCoverageNote("SELECT * FROM big b JOIN facts f ON b.MemberId = f.MemberId", "scratch", env()), ), ).toBeUndefined() expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS + 200) @@ -160,28 +162,30 @@ describe("joinCoverageNote warehouse policy", () => { test("a local DuckDB file is checked in local mode", async () => { const connector = await Registry.get("duckdb_local") for (const sql of statements) await connector.execute(sql, undefined, undefined, { noLimit: true }) - expect(await inScratch(() => joinCoverageNote(lowJoin, "duckdb_local", {}, env()))).toContain("6 of 8 left rows") + expect(await inScratch(() => joinCoverageNote(lowJoin, "duckdb_local", env()))).toContain("6 of 8 left rows") }) test("SQLite, cloud DuckDB and remote warehouses are skipped in local mode", async () => { for (const warehouse of ["sqlite_local", "duckdb_cloud", "remote_pg"]) { const started = Date.now() - expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, {}, env()))).toBeUndefined() + expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, env()))).toBeUndefined() // Skipped before any connection attempt. expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS) } }) - test("all mode never counts SQLite or cloud DuckDB, and an unreachable remote returns undefined", async () => { + test("no setting enables SQLite, cloud DuckDB or remote warehouses", async () => { for (const warehouse of ["sqlite_local", "duckdb_cloud", "remote_pg"]) { - expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, {}, env("all")))).toBeUndefined() + expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, env("all")))).toBeUndefined() } }) test("missing tables return undefined", async () => { expect( - await inScratch(() => joinCoverageNote("SELECT * FROM nope a JOIN nada b ON a.id = b.id", "scratch", {}, env())), + await inScratch(() => joinCoverageNote("SELECT * FROM nope a JOIN nada b ON a.id = b.id", "scratch", env())), ).toBeUndefined() + // Without an explicit warehouse the query may have run elsewhere (e.g. dbt): skip. + expect(await inScratch(() => joinCoverageNote(lowJoin, undefined, env()))).toBeUndefined() }) }) From 26d4d6d9226c7c9ede7459def30a6462bd3e3ce2 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 18 Oct 2026 09:44:53 -0700 Subject: [PATCH 4/5] fix: measure join coverage over the query's own filtered population Independent review found the count ignored the WHERE clause, so a filtered join where every returned row matched still got a loss warning. - WHERE conjuncts that reference only the left or only the right table are applied to that side of the count (rebuilt with DuckDB's own `json_deserialize_sql`). A WHERE with a conjunct spanning both sides, another relation, a bare column or a subquery skips the check. - Remote-path detection reuses the drivers' `isLocalFilePath`, which also covers `ducklake:`, for both the connection path and attached databases. - The size-ceiling test now pairs the large table with an unshadowed one, so it fails if the ceiling is removed; the comment on timeouts states that the connection's own query timeout still applies. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/tools/sql-join-coverage.ts | 143 +++++++++++++++--- .../altimate/tools/sql-join-coverage.test.ts | 32 +++- 2 files changed, 146 insertions(+), 29 deletions(-) diff --git a/packages/opencode/src/altimate/tools/sql-join-coverage.ts b/packages/opencode/src/altimate/tools/sql-join-coverage.ts index 22d632d747..0403d985cc 100644 --- a/packages/opencode/src/altimate/tools/sql-join-coverage.ts +++ b/packages/opencode/src/altimate/tools/sql-join-coverage.ts @@ -1,5 +1,5 @@ // altimate_change - new file -import type { Connector } from "@altimateai/drivers" +import { isLocalFilePath, type Connector } from "@altimateai/drivers" import * as Registry from "../native/connections/registry" import { isScratch } from "../native/connections/scratch" @@ -38,8 +38,61 @@ type Key = { left: string; right: string; leftFirst: boolean } const literal = (text: string) => `'${text.replace(/'/g, "''")}'` // DuckDB resolves quoted identifiers case-insensitively, so quoting every part is safe. const ident = (name: string) => `"${name.replace(/"/g, '""')}"` +/** A DuckDB path that names remote or extension storage; in-memory, empty and `file:` paths are local. */ const isRemotePath = (path: unknown) => - typeof path === "string" && (/^(md|motherduck):/i.test(path) || path.includes("://")) + typeof path === "string" && path !== "" && path !== ":memory:" && !/^file:/i.test(path) && !isLocalFilePath(path) + +const STAR = { + class: "STAR", + type: "STAR", + alias: "", + query_location: 0, + relation_name: "", + exclude_list: [], + replace_list: [], + columns: false, + expr: null, + qualified_exclude_list: [], + rename_list: [], +} + +/** Qualifiers of every column reference in an expression, or undefined when it holds a subquery or a bare column. */ +function qualifiers(expr: Json, out = new Set()): Set | undefined { + if (Array.isArray(expr)) { + for (const item of expr) if (!qualifiers(item, out)) return undefined + return out + } + if (!expr || typeof expr !== "object") return out + if (expr.class === "SUBQUERY" || expr.type === "SUBQUERY") return undefined + if (expr.class === "COLUMN_REF") { + if (expr.column_names?.length !== 2) return undefined + out.add(String(expr.column_names[0]).toLowerCase()) + return out + } + for (const value of Object.values(expr)) if (!qualifiers(value, out)) return undefined + return out +} + +/** + * Splits the WHERE clause into the conjuncts that filter only the left or only the right side. + * Undefined when any conjunct spans both sides, another relation, a bare column or a subquery: + * the count could not reproduce the population the query actually used. + */ +function sideFilters(where: Json, leftAlias: string, rightAlias: string) { + const left: Json[] = [] + const right: Json[] = [] + if (!where) return { left, right } + const conjuncts = where.type === "CONJUNCTION_AND" ? where.children : [where] + for (const conjunct of conjuncts) { + const names = qualifiers(conjunct) + if (!names || names.size !== 1) return undefined + const [name] = names + if (name === leftAlias.toLowerCase()) left.push(conjunct) + else if (name === rightAlias.toLowerCase()) right.push(conjunct) + else return undefined + } + return { left, right } +} /** Equality pairs between the two sides in their written order, or undefined for any other condition. */ function equalityKeys(condition: Json, leftAlias: string, rightAlias: string): Key[] | undefined { @@ -71,8 +124,9 @@ function tableJoins(ref: Json, out: Json[] = []): Json[] { } async function rows(connector: Connector, sql: string): Promise[]> { - // No timeout, session deadline or abort signal: interrupting a local engine restarts its helper - // and drops session state. The catalog-size gate bounds the work instead. + // No short timeout, session deadline or abort signal: interrupting a local engine restarts its + // helper and drops session state. Only the connection's own query timeout still applies; the + // catalog-size gate keeps the work far below it. const response = await connector.execute(sql, undefined, undefined, { noLimit: true, readOnly: true }) return response.rows.map((row: unknown[]) => Object.fromEntries(response.columns.map((col: string, i: number) => [col, row[i]])), @@ -89,24 +143,60 @@ async function smallLocalTable(connector: Connector, side: Json): Promise 'duckdb' OR (d.path IS NOT NULL AND (d.path ILIKE 'md:%' OR d.path ILIKE 'motherduck:%' OR d.path LIKE '%://%')))) AS remote`, + `SELECT d.type AS type, d.path AS path, t.estimated_size AS size + FROM duckdb_tables() t JOIN duckdb_databases() d USING (database_name) + WHERE lower(t.table_name) = lower(${name}) ${scope}`, ) + const [table] = tables return ( - Number(found?.views) === 0 && - Number(found?.tables) === 1 && - Number(found?.remote) === 0 && - Number(found?.size) <= LOCAL_ROW_CEILING + tables.length === 1 && + String(table.type).toLowerCase() === "duckdb" && + !isRemotePath(table.path) && + Number(table.size) <= LOCAL_ROW_CEILING ) } +/** One side as SQL: the bare table, or the table with the WHERE conjuncts that apply to it alone. */ +async function sideSource(connector: Connector, statement: Json, side: Json, filters: Json[]): Promise { + if (!filters.length) + return [side.catalog_name, side.schema_name, side.table_name].filter(Boolean).map(ident).join(".") + const where = + filters.length === 1 + ? filters[0] + : { class: "CONJUNCTION", type: "CONJUNCTION_AND", alias: "", query_location: 0, children: filters } + const node = statement.statements[0].node + const synthetic = { + ...statement, + statements: [ + { + ...statement.statements[0], + node: { + ...node, + select_list: [STAR], + from_table: side, + where_clause: where, + group_expressions: [], + group_sets: [], + having: null, + qualify: null, + modifiers: [], + cte_map: { map: [] }, + }, + }, + ], + } + const [row] = await rows(connector, `SELECT json_deserialize_sql(${literal(JSON.stringify(synthetic))}) AS s`) + if (typeof row?.s !== "string" || !row.s) throw new Error("side could not be rebuilt") + return `(${row.s})` +} + /** * Advisory note for a successful read on the session scratch database or a local DuckDB file: * when an equality join between two small base tables keeps under half of its left side, say @@ -154,17 +244,20 @@ export async function joinCoverageNote( sides.some((s) => s.column_name_alias?.length || s.sample || cteNames.has(String(s.table_name).toLowerCase())) ) continue - const keys = equalityKeys( - join.condition, - join.left.alias || join.left.table_name, - join.right.alias || join.right.table_name, - ) + const leftAlias = join.left.alias || join.left.table_name + const rightAlias = join.right.alias || join.right.table_name + const keys = equalityKeys(join.condition, leftAlias, rightAlias) if (!keys?.length) continue + // The count must see the same left population as the query: WHERE filters are applied + // per side, and a WHERE that cannot be split that way skips the check. + const filters = sideFilters(node.where_clause, leftAlias, rightAlias) + if (!filters) continue if (!(await smallLocalTable(connector, join.left)) || !live()) continue if (!(await smallLocalTable(connector, join.right)) || !live()) continue + const leftSource = await sideSource(connector, statement, join.left, filters.left) + const rightSource = await sideSource(connector, statement, join.right, filters.right) + if (!live()) return undefined - const relation = (side: Json) => - [side.catalog_name, side.schema_name, side.table_name].filter(Boolean).map(ident).join(".") const match = keys .map((k) => k.leftFirst @@ -175,8 +268,8 @@ export async function joinCoverageNote( const [row] = await rows( connector, `SELECT COUNT(*) AS n, - COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM ${relation(join.right)} __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched - FROM ${relation(join.left)} __jc_l`, + COALESCE(SUM(CASE WHEN EXISTS (SELECT 1 FROM ${rightSource} __jc_r WHERE ${match}) THEN 1 ELSE 0 END), 0) AS matched + FROM ${leftSource} __jc_l`, ) const n = Number(row?.n) const matched = Number(row?.matched) diff --git a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts index 7a0bf44c48..065f4a87b2 100644 --- a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts +++ b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts @@ -103,6 +103,33 @@ describe("joinCoverageNote on the scratch database", () => { expect(note).toContain("they stay as unmatched rows with NULL right columns") }) + test("WHERE filters are applied per side, so the count matches the query's population", async () => { + const note = (where: string) => + inScratch(() => + joinCoverageNote( + `SELECT * FROM cohort c JOIN facts f ON c.MemberId = f.MemberId WHERE ${where}`, + "scratch", + env(), + ), + ) + // Group a: ids 1, 2, 3; ids 1 and 2 match (2 of 3 kept): no warning. + expect(await note("c.grp = 'a'")).toBeUndefined() + // Group b: ids 4, 5, 6; none match. + expect(await note("c.grp = 'b'")).toContain("3 of 3 left rows (100.0%) have no match") + // A right-side filter narrows the matches: only id 2 has v > 2.5. + expect(await note("f.v > 2.5")).toContain("7 of 8 left rows (87.5%) have no match") + // Both kinds together. + expect(await note("c.grp = 'a' AND f.v > 2.5")).toContain("2 of 3 left rows (66.7%) have no match") + // Conditions the count cannot reproduce skip the check. + for (const where of [ + "c.grp = f.grp", + "grp = 'a'", + "c.grp = 'a' OR f.v > 2.5", + "c.MemberId IN (SELECT MemberId FROM half_facts)", + ]) + expect(await note(where)).toBeUndefined() + }) + test("stays silent when coverage is fine or the shape is not a plain table equality join", async () => { const cases: [string, string | undefined][] = [ ["SELECT * FROM cohort c JOIN half_facts h ON c.MemberId = h.MemberId", undefined], @@ -148,7 +175,7 @@ describe("joinCoverageNote on the scratch database", () => { const started = Date.now() expect( await inScratch(() => - joinCoverageNote("SELECT * FROM big b JOIN facts f ON b.MemberId = f.MemberId", "scratch", env()), + joinCoverageNote("SELECT * FROM big b JOIN half_facts h ON b.MemberId = h.MemberId", "scratch", env()), ), ).toBeUndefined() expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS + 200) @@ -167,10 +194,7 @@ describe("joinCoverageNote warehouse policy", () => { test("SQLite, cloud DuckDB and remote warehouses are skipped in local mode", async () => { for (const warehouse of ["sqlite_local", "duckdb_cloud", "remote_pg"]) { - const started = Date.now() expect(await inScratch(() => joinCoverageNote(lowJoin, warehouse, env()))).toBeUndefined() - // Skipped before any connection attempt. - expect(Date.now() - started).toBeLessThan(JOIN_CHECK_BUDGET_MS) } }) From 070b58de04cc779f958e2c5feec0a2a5305ccaab Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Sun, 18 Oct 2026 09:59:47 -0700 Subject: [PATCH 5/5] fix: bound join-coverage filters to simple predicates; skip LEFT JOINs filtered on the right - Rebuilt WHERE conjuncts may contain only comparisons, BETWEEN, IN lists, IS [NOT] NULL and casts of columns and constants. Function calls (LIKE included), CASE and subqueries skip the check, so the uncancellable count stays a cheap scan bounded by the 500,000-row ceiling. - A LEFT JOIN with a right-side WHERE conjunct is skipped: that filter runs after the join and drops the NULL-extended rows, which a pre-filtered count cannot describe. - Tests for both. Co-Authored-By: Claude Opus 5.5 --- .../src/altimate/tools/sql-join-coverage.ts | 34 ++++++++++++++----- .../altimate/tools/sql-join-coverage.test.ts | 16 +++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/packages/opencode/src/altimate/tools/sql-join-coverage.ts b/packages/opencode/src/altimate/tools/sql-join-coverage.ts index 0403d985cc..f5f84c3fc8 100644 --- a/packages/opencode/src/altimate/tools/sql-join-coverage.ts +++ b/packages/opencode/src/altimate/tools/sql-join-coverage.ts @@ -56,20 +56,35 @@ const STAR = { rename_list: [], } -/** Qualifiers of every column reference in an expression, or undefined when it holds a subquery or a bare column. */ +// Expression classes a rebuilt filter may contain: plain comparisons of columns and constants. No +// function calls (including LIKE), CASE or subqueries, so a filtered side costs one cheap scan and +// the uncancellable count stays as short as the size ceiling makes it. +const SIMPLE_CLASSES = new Set(["COMPARISON", "BETWEEN", "OPERATOR", "CAST", "COLUMN_REF", "CONSTANT"]) +const SIMPLE_OPERATORS = new Set([ + "COMPARE_IN", + "COMPARE_NOT_IN", + "OPERATOR_IS_NULL", + "OPERATOR_IS_NOT_NULL", + "OPERATOR_CAST", +]) + +/** + * Qualifiers of every column reference in a simple expression, or undefined when it holds anything + * else (a function, subquery, CASE, bare column...). + */ function qualifiers(expr: Json, out = new Set()): Set | undefined { - if (Array.isArray(expr)) { - for (const item of expr) if (!qualifiers(item, out)) return undefined - return out - } - if (!expr || typeof expr !== "object") return out - if (expr.class === "SUBQUERY" || expr.type === "SUBQUERY") return undefined + if (!expr || typeof expr !== "object" || Array.isArray(expr)) return undefined + if (!SIMPLE_CLASSES.has(expr.class)) return undefined + if (expr.class === "OPERATOR" && !SIMPLE_OPERATORS.has(expr.type)) return undefined + if (expr.class === "CONSTANT") return out if (expr.class === "COLUMN_REF") { if (expr.column_names?.length !== 2) return undefined out.add(String(expr.column_names[0]).toLowerCase()) return out } - for (const value of Object.values(expr)) if (!qualifiers(value, out)) return undefined + const children = [expr.left, expr.right, expr.input, expr.lower, expr.upper, expr.child, ...(expr.children ?? [])] + for (const child of children.filter((c) => c !== undefined && c !== null)) + if (!qualifiers(child, out)) return undefined return out } @@ -252,6 +267,9 @@ export async function joinCoverageNote( // per side, and a WHERE that cannot be split that way skips the check. const filters = sideFilters(node.where_clause, leftAlias, rightAlias) if (!filters) continue + // A right-side WHERE on a LEFT JOIN runs after the join and drops the NULL-extended rows, + // which a pre-filtered count cannot describe. + if (kind === "LEFT" && filters.right.length) continue if (!(await smallLocalTable(connector, join.left)) || !live()) continue if (!(await smallLocalTable(connector, join.right)) || !live()) continue const leftSource = await sideSource(connector, statement, join.left, filters.left) diff --git a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts index 065f4a87b2..f41ee46d24 100644 --- a/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts +++ b/packages/opencode/test/altimate/tools/sql-join-coverage.test.ts @@ -126,8 +126,24 @@ describe("joinCoverageNote on the scratch database", () => { "grp = 'a'", "c.grp = 'a' OR f.v > 2.5", "c.MemberId IN (SELECT MemberId FROM half_facts)", + // Function calls (LIKE is one) are not rebuilt: the count must stay a cheap scan. + "c.grp LIKE 'b%'", + "lower(c.grp) = 'b'", ]) expect(await note(where)).toBeUndefined() + // Simple comparisons, BETWEEN, IN lists and IS NULL are applied. + expect(await note("c.MemberId BETWEEN 4 AND 6")).toContain("3 of 3 left rows") + expect(await note("c.grp IN ('b', 'c') AND c.MemberId IS NOT NULL")).toContain("4 of 4 left rows") + // A right-side WHERE on a LEFT JOIN drops the unmatched rows after the join: skipped. + expect( + await inScratch(() => + joinCoverageNote( + "SELECT * FROM cohort c LEFT JOIN facts f ON c.MemberId = f.MemberId WHERE f.v > 2.5", + "scratch", + env(), + ), + ), + ).toBeUndefined() }) test("stays silent when coverage is fine or the shape is not a plain table equality join", async () => {