From 562d1cc90ea7990b077a52858020dcbb5d1aed77 Mon Sep 17 00:00:00 2001 From: Kiyeon Jeon Date: Sun, 26 Jul 2026 01:27:46 +0900 Subject: [PATCH 1/2] fix: measure names are unique, and the fan-out case is gradeable Two cleanups that close out the defect list the hard dataset surfaced. 1. Colliding measure names (ROADMAP 6.3). Two tables with the same numeric column both produced the same measure name, and compileMetric resolved a name by scanning entities in order - so it silently computed the first table's measure. This 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. Measure names are now 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; the original dataset has zero renames. Unique names rather than refuse-on-ambiguity, which would have matched the compiler's usual style: a catalog keyed by name cannot hold duplicates, and resolve_terms was offering two identical-looking entries pointing at different tables, which no user could disambiguate. 2. The fan-out case asked for two numbers, the agent answered with two queries, and the row grader only ever saw the last one - so it failed for harness reasons rather than agent ones. It now asks for one row per customer with a support case: a single gradeable result set that keeps the trap sharp. The naive double join still inflates Acme to 8156 vs 2039 and Umbrella to 4515 vs 2257.50. --- evals/cases/agent-hard.json | 4 +- evals/cases/engine-hard.json | 76 ++++++++++++++++++++++------ src/core/discovery/semantic-model.ts | 28 ++++++++++ test/discovery.test.ts | 34 +++++++++++++ 4 files changed, 125 insertions(+), 17 deletions(-) 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(); + } +}); From 8bf529b75d5670b3416e96bc8a27e490ea51c2ee Mon Sep 17 00:00:00 2001 From: Kiyeon Jeon Date: Sun, 26 Jul 2026 01:40:53 +0900 Subject: [PATCH 2/2] docs: record the measure-ambiguity fix and the re-measured A/B The hard A/B is unchanged at +30.6 (grounded 31/36, raw-sql 20/36) across two runs at different code states - a useful reproducibility signal for the harness. The reframed fan-out case still fails 0/3, but now for a real reason: it returns [2, 3405] against an expected [1702.5, 2], which is exactly the 2x inflation for a customer with two tickets. Before the reframe it failed on 'column count 1, expected 2', a grading artifact. So the reframe worked - the case now measures what it was built to measure - and the underlying fan-out failure is genuine. Worth recording for step 7: the grain warning added in 6.2 is present in the context for this exact entity and did not prevent the double count. A passive warning is not sufficient here. --- CHANGELOG.md | 16 ++++++++++++++++ ROADMAP.md | 25 +++++++++++++++++++++---- 2 files changed, 37 insertions(+), 4 deletions(-) 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