From 09ff551a3031baa52a8c040cdeb504da026a5f66 Mon Sep 17 00:00:00 2001 From: Kiyeon Jeon Date: Sun, 26 Jul 2026 02:46:12 +0900 Subject: [PATCH] fix: an ambiguous join is refused instead of guessed 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. That is the worst shape of bug this engine can have: the answer looks perfectly ordinary while answering a question the user never 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 and unknown metrics: refuse rather than guess. Reproduced on a fixture where both edges are inferred at 93% and the compiler picked billing without a word. Both engine suites are unchanged (18/18, 25/25) - neither committed dataset can trigger it through query_metric, since a lookup table's label is not a groupable dimension, which is also why the eval never caught this one. Found by reading the code path while closing out the defect list, not by measurement. This closes the engine-defect list the hard dataset opened. --- CHANGELOG.md | 11 +++++++ ROADMAP.md | 8 +++-- src/core/discovery/compile-metric.ts | 14 ++++++-- test/discovery.test.ts | 49 ++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 5 deletions(-) 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(); + } +});