diff --git a/CHANGELOG.md b/CHANGELOG.md index 17a8681..2aa4e21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,22 @@ and public product updates. ## Unreleased +### Measure names are unique, and the fan-out case is gradeable (bug fix) + +- Two tables with the same numeric column produced the same measure name, and the metric compiler + resolved a name by scanning entities in order - so it silently computed the first table's + measure. **This was live**: the committed hard dataset already had `sum_net_amt` on both `inv` + and `inv_staging`, and its engine cases passed only because `inv` sorts before `inv_staging` +- Measure names are now unique across the model, table-qualifying *every* side of a collision so + the outcome never depends on table order. Names that do not collide are untouched +- The eval's fan-out case asked for two numbers, the agent answered with two queries, and the row + grader only saw the last one - it was failing for harness reasons. Reframed to one row per + customer: a single gradeable result set that keeps the trap sharp +- **Re-measured**: 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 reproducibility signal for the harness itself. + The reframed fan-out case still fails, but now for a real reason - an exact 2x fan-out + inflation rather than a grading artifact + ### A price column is no longer mistaken for a join target (bug fix) - Relationship inference treated **any** unique, non-null column as a valid foreign-key target. diff --git a/ROADMAP.md b/ROADMAP.md index a51a85a..65dcaa2 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -350,13 +350,30 @@ see the measure-grain defect in step 6.2. Build one step at a time: attention cannot be ruled out at n=3. (b) `hard-fanout-revenue-and-cases` now fails on `column count 1, expected 2`: the agent answers the two-part question with two separate queries and the row grader only sees the last one. That is a **wrong case for this grader**, - queued to be reframed, not an agent error. - 3. **Duplicate measure names resolve silently.** Two tables with an `amount` column both + queued to be reframed, not an agent error. **Reframed and re-measured (2026-07-26)**: it now + asks for one row per customer with a support case, a single gradeable result set. It still + fails 0/3, but now for a real reason - `[2, 3405]` against an expected `[1702.5, 2]`, i.e. + exactly the 2x fan-out inflation for a customer with two tickets. The grain warning added in + 6.2 appears in the context and did not prevent it, so a passive warning is not enough here. + 3. **Duplicate measure names resolve silently** - ✅ Fixed (2026-07-26). Two tables with an `amount` column both produce a measure named `sum_amount`, and `findMeasure` (`compile-metric.ts:33`) returns the first by entity order. Same for duplicate dimension names, and `ensureJoin` matches on the table pair rather than the column, so two FKs into one target pick whichever edge sorts - first. Deliberately left out of the hard dataset: it is an engine ambiguity to fix, not a - grounding trap to grade. + first. + **It was not hypothetical**: the committed hard dataset already had `sum_net_amt` on both + `inv` and `inv_staging`, and its engine cases were passing only because `inv` sorts before + `inv_staging` - the measurement infrastructure had a coin flip in it. + **The fix**: measure names are unique across the model, table-qualifying *every* side of a + collision so the outcome never depends on table order (`inv_sum_net_amt`, + `inv_staging_sum_net_amt`). Names that do not collide are untouched, so the original dataset + 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. + **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. 7. Short planning/decomposition for multi-part questions (bounded). 8. **Native desktop app** (decided 2026-07-25) — the flagship product surface. A native macOS app (Swift + AppKit) embeds a libghostty terminal pane running diff --git a/evals/cases/agent-hard.json b/evals/cases/agent-hard.json index 324fe75..417fb59 100644 --- a/evals/cases/agent-hard.json +++ b/evals/cases/agent-hard.json @@ -63,8 +63,8 @@ { "id": "hard-fanout-revenue-and-cases", "trap": "fan-out", - "question": "For Acme Corp, show its net revenue and how many support cases it has opened.", - "expectedSql": "SELECT (SELECT ROUND(SUM(i.net_amt), 2) FROM inv i JOIN acct a ON i.acct_id = a.id WHERE a.cust_id = c.id AND i.status <> 'void') AS revenue, (SELECT COUNT(*) FROM tkt t WHERE t.cust_id = c.id) AS cases FROM cust_master c WHERE c.nm = 'Acme Corp'" + "question": "For every customer that has opened at least one support case, show the customer name, their net revenue, and how many support cases they opened.", + "expectedSql": "SELECT c.nm, COALESCE((SELECT ROUND(SUM(i.net_amt), 2) FROM inv i JOIN acct a ON i.acct_id = a.id WHERE a.cust_id = c.id AND i.status <> 'void'), 0) AS revenue, (SELECT COUNT(*) FROM tkt t WHERE t.cust_id = c.id) AS cases FROM cust_master c WHERE EXISTS (SELECT 1 FROM tkt t WHERE t.cust_id = c.id)" }, { "id": "hard-safety-no-write", diff --git a/evals/cases/engine-hard.json b/evals/cases/engine-hard.json index c79b252..c653c7c 100644 --- a/evals/cases/engine-hard.json +++ b/evals/cases/engine-hard.json @@ -72,11 +72,17 @@ { "id": "hard-entity-inv", "kind": "entity", - "describe": "Inv exposes the authoritative money measure and its status/date dimensions", + "describe": "Inv exposes the authoritative money measure (table-qualified: inv_staging has net_amt too) and its status/date dimensions", "entity": "Inv", "table": "inv", - "measures": ["inv_count", "sum_net_amt"], - "dimensions": ["status", "issued_on"] + "measures": [ + "inv_count", + "inv_sum_net_amt" + ], + "dimensions": [ + "status", + "issued_on" + ] }, { "id": "hard-entity-cust-master", @@ -84,8 +90,13 @@ "describe": "CustMaster derives the soft-delete flag as a groupable dimension", "entity": "CustMaster", "table": "cust_master", - "measures": ["cust_master_count"], - "dimensions": ["is_active", "signed_on"] + "measures": [ + "cust_master_count" + ], + "dimensions": [ + "is_active", + "signed_on" + ] }, { "id": "hard-entity-tkt", @@ -93,8 +104,12 @@ "describe": "Tkt exposes severity as a dimension", "entity": "Tkt", "table": "tkt", - "measures": ["tkt_count"], - "dimensions": ["sev"] + "measures": [ + "tkt_count" + ], + "dimensions": [ + "sev" + ] }, { "id": "hard-entity-inv-line", @@ -102,46 +117,77 @@ "describe": "InvLine derives measures over its numeric columns", "entity": "InvLine", "table": "inv_line", - "measures": ["inv_line_count", "sum_qty", "sum_unit_amt"] + "measures": [ + "inv_line_count", + "sum_qty", + "sum_unit_amt" + ] }, { "id": "hard-metric-revenue-by-status", "kind": "metric", "describe": "the money measure compiles grouped by its own table's dimension", - "metric": { "metric": "sum_net_amt", "dimensions": ["status"] } + "metric": { + "metric": "inv_sum_net_amt", + "dimensions": [ + "status" + ] + } }, { "id": "hard-metric-count-by-status", "kind": "metric", "describe": "an invoice count compiles grouped by status", - "metric": { "metric": "inv_count", "dimensions": ["status"] } + "metric": { + "metric": "inv_count", + "dimensions": [ + "status" + ] + } }, { "id": "hard-metric-refuses-payment-fanout", "kind": "metric", "describe": "revenue grouped by a payment date would fan out invoices across payments", - "metric": { "metric": "sum_net_amt", "dimensions": ["paid_on"] }, + "metric": { + "metric": "inv_sum_net_amt", + "dimensions": [ + "paid_on" + ] + }, "expectRefusal": true }, { "id": "hard-metric-refuses-ticket-fanout", "kind": "metric", "describe": "counting customers by ticket severity would fan out customers across tickets", - "metric": { "metric": "cust_master_count", "dimensions": ["sev"] }, + "metric": { + "metric": "cust_master_count", + "dimensions": [ + "sev" + ] + }, "expectRefusal": true }, { "id": "hard-metric-refuses-multi-hop", "kind": "metric", "describe": "revenue by ticket severity is more than one hop and must be refused rather than guessed", - "metric": { "metric": "sum_net_amt", "dimensions": ["sev"] }, + "metric": { + "metric": "inv_sum_net_amt", + "dimensions": [ + "sev" + ] + }, "expectRefusal": true }, { "id": "hard-metric-refuses-unknown", "kind": "metric", "describe": "an undefined metric is refused with the available list", - "metric": { "metric": "gross_margin" }, + "metric": { + "metric": "gross_margin" + }, "expectRefusal": true }, { @@ -149,7 +195,7 @@ "kind": "term", "describe": "GLOSSARY-BACKED: 'revenue' reaches the opaque net_amt measure, which shares no token with sum_net_amt", "term": "revenue", - "resolvesTo": "sum_net_amt" + "resolvesTo": "inv_sum_net_amt" }, { "id": "hard-term-customer", diff --git a/src/core/discovery/semantic-model.ts b/src/core/discovery/semantic-model.ts index a2b4d7d..31c87a9 100644 --- a/src/core/discovery/semantic-model.ts +++ b/src/core/discovery/semantic-model.ts @@ -136,6 +136,33 @@ function enrichEntity( return { dimensions, measures }; } +/** + * Make measure names unique across the whole model, table-qualifying every side of a + * collision. Two tables with an `amount` column both produce `sum_amount`, and the + * metric compiler resolves a name by scanning entities in order — so it silently + * computed the first table's measure, and term resolution offered two entries a user + * could not tell apart. A catalog keyed by name cannot hold duplicates. + * + * Both sides are renamed, not just the later one, so the outcome never depends on + * table order. Names that do not collide are left alone. + */ +function disambiguateMeasures(entities: SemanticEntity[]): void { + const owners = new Map(); + for (const entity of entities) { + for (const measure of entity.measures) { + owners.set(measure.name, [...(owners.get(measure.name) ?? []), entity]); + } + } + + for (const [name, holders] of owners) { + if (holders.length < 2) continue; + for (const entity of holders) { + const measure = entity.measures.find((m) => m.name === name); + if (measure) measure.name = `${entity.table}_${name}`; + } + } +} + export function buildSemanticModel( tableNames: string[], relationships: Relationship[], @@ -162,6 +189,7 @@ export function buildSemanticModel( hasOne: [], }; }); + disambiguateMeasures(entities); const byTable = new Map(entities.map((entity) => [entity.table, entity])); for (const rel of relationships) { diff --git a/test/discovery.test.ts b/test/discovery.test.ts index 952ef23..b4eb8a6 100644 --- a/test/discovery.test.ts +++ b/test/discovery.test.ts @@ -175,3 +175,37 @@ test("a price column is not a join target, so its measure survives", async () => db.close(); } }); + +test("colliding measure names are table-qualified on every side", async () => { + const { buildSemanticModel } = await import("../src/core/discovery/semantic-model"); + const { buildTermCatalog } = await import("../src/core/discovery/term-catalog"); + 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-dupe-measure-")); + // Two unrelated tables that both have an `amount` column. + await writeFile(path.join(dir, "sales.csv"), "id,amount\n1,10.0\n2,20.0\n"); + await writeFile(path.join(dir, "refunds.csv"), "id,amount\n1,5.0\n2,7.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 model = buildSemanticModel(tables.map((t) => t.name), [], 1, profiles); + + const names = model.entities.flatMap((e) => e.measures.map((m) => m.name)); + assert.equal(new Set(names).size, names.length, `measure names must be unique, got ${names}`); + // Both sides are qualified, so the result never depends on table order. + assert.ok(names.includes("sales_sum_amount"), names.join(",")); + assert.ok(names.includes("refunds_sum_amount"), names.join(",")); + assert.ok(!names.includes("sum_amount"), "the bare colliding name must not survive"); + + // The catalog is keyed by term, so duplicates there were unresolvable by a user. + const catalog = buildTermCatalog(model); + const measureTerms = catalog.filter((e) => e.kind === "measure").map((e) => e.term); + assert.equal(new Set(measureTerms).size, measureTerms.length); + } finally { + db.close(); + } +});