diff --git a/CHANGELOG.md b/CHANGELOG.md index 8db5013..dd49932 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ and public product updates. ## Unreleased +### An ambiguous join is refused instead of guessed (bug fix) + +- `ensureJoin` matched on the table pair rather than the column, so a table with two foreign keys + into the same target - billing region and shipping region - silently joined on whichever edge + sorted first. The result looked perfectly ordinary while answering a question the user had not + asked +- It now refuses and names both candidate keys, so the agent can write the SQL it actually means. + That matches how the compiler already handles fan-out: refuse rather than guess +- Both engine suites are unchanged (18/18, 25/25); a regression test pins a fixture where both + edges are inferred at 93% + ### Plan-first turn for multi-part questions - Asked for revenue *and* ticket counts per customer, the agent wrote one query joining both diff --git a/ROADMAP.md b/ROADMAP.md index 610683d..84125cd 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -369,8 +369,12 @@ see the measure-grain defect in step 6.2. Build one step at a time: has zero renames. Unique names rather than refuse-on-ambiguity - which would have matched the compiler's usual style - because a catalog keyed by name cannot hold duplicates and `resolve_terms` was offering two identical-looking entries pointing at different tables. - The remaining half (`ensureJoin` matching on the table pair rather than the column, so two - FKs into one target pick whichever edge sorts first) is still open. + **The other half is also fixed**: `ensureJoin` matched on the table pair rather than the + column, so two FKs into one target (billing vs shipping region) silently joined on + whichever edge sorted first - answering a question the user did not ask, with a result that + looks perfectly ordinary. It now refuses and names both candidate keys, matching how the + compiler already handles fan-out: refuse rather than guess. Reproduced on a fixture where + both edges are inferred at 93%, and both engine suites are unchanged (18/18, 25/25). **Re-measured after the fix**: the hard A/B is unchanged at **+30.6** (grounded 31/36, raw-sql 20/36) across two runs at different code states, which is a useful reproducibility signal for the harness itself. diff --git a/src/core/discovery/compile-metric.ts b/src/core/discovery/compile-metric.ts index 546e517..906e71f 100644 --- a/src/core/discovery/compile-metric.ts +++ b/src/core/discovery/compile-metric.ts @@ -86,11 +86,19 @@ export function compileMetric( // Ensure the fact table can reach `target` via a single many-to-one hop (no fan-out). const ensureJoin = (target: string, what: string): string | null => { if (target === base || joins.has(target)) return null; - const forward = relationships.find((r) => r.from.table === base && r.to.table === target); - if (forward) { + const forward = relationships.filter((r) => r.from.table === base && r.to.table === target); + // More than one key into the same table (billing vs shipping region) means the + // grouping is genuinely ambiguous. Picking the first edge would answer a question + // the user did not ask, and the result would look perfectly ordinary. + if (forward.length > 1) { + const keys = forward.map((r) => `${base}.${r.from.column}`).join(" or "); + return `Cannot use "${what}": ${base} joins ${target} through more than one key (${keys}), so this grouping is ambiguous. Write the SQL directly and pick the one you mean.`; + } + if (forward.length === 1) { + const edge = forward[0]; joins.set( target, - `JOIN ${qi(target)} ON ${qi(base)}.${qi(forward.from.column)} = ${qi(target)}.${qi(forward.to.column)}` + `JOIN ${qi(target)} ON ${qi(base)}.${qi(edge.from.column)} = ${qi(target)}.${qi(edge.to.column)}` ); return null; } diff --git a/test/discovery.test.ts b/test/discovery.test.ts index b4eb8a6..f7ff172 100644 --- a/test/discovery.test.ts +++ b/test/discovery.test.ts @@ -209,3 +209,52 @@ test("colliding measure names are table-qualified on every side", async () => { db.close(); } }); + +test("two keys into the same table make a grouping ambiguous, and it is refused", async () => { + const { buildSemanticModel } = await import("../src/core/discovery/semantic-model"); + const { compileMetric } = await import("../src/core/discovery/compile-metric"); + const { mkdtemp, writeFile } = await import("node:fs/promises"); + const { tmpdir } = await import("node:os"); + const { resolveSource } = await import("../src/adapters/cli/source"); + + const dir = await mkdtemp(path.join(tmpdir(), "querypad-ambig-join-")); + // `tier` repeats, so it survives the categorical ratio rule and is groupable. + await writeFile( + path.join(dir, "regions.csv"), + "id,name,tier\n1,West,core\n2,East,core\n3,North,edge\n4,South,edge\n5,Central,core\n6,Far,edge\n" + ); + await writeFile( + path.join(dir, "orders.csv"), + "id,billing_region_id,shipping_region_id,amount\n" + + "1,1,2,10.0\n2,1,3,20.0\n3,2,1,30.0\n4,3,4,40.0\n5,4,2,50.0\n6,5,6,60.0\n" + ); + + const db = await createNodeDb(); + try { + const { tables } = await resolveSource({ folder: dir }).load(db.runner); + const profiles = await Promise.all(tables.map((t) => profileTable(t, db.runner, 1))); + const rels = await discoverRelationships(profiles, db.runner); + // Both foreign keys are real and both are inferred. + const keys = rels.map(relationshipKey); + assert.ok(keys.includes("orders.billing_region_id->regions.id"), keys.join(",")); + assert.ok(keys.includes("orders.shipping_region_id->regions.id"), keys.join(",")); + + const model = buildSemanticModel(tables.map((t) => t.name), rels, 1, profiles); + const compiled = compileMetric(model, rels, { + metric: "orders_count", + dimensions: ["tier"], + }); + + // Silently joining on whichever edge sorted first would answer a question the + // user did not ask, and the result would look perfectly ordinary. + assert.equal(compiled.ok, false, "an ambiguous grouping must not compile"); + if (!compiled.ok) { + assert.match(compiled.error, /more than one key/); + // The refusal has to name both candidates, or the agent cannot act on it. + assert.match(compiled.error, /billing_region_id/); + assert.match(compiled.error, /shipping_region_id/); + } + } finally { + db.close(); + } +});