diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e3e6cd..4419062 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,27 @@ and public product updates. ## Unreleased +### Measures now carry their grain (bug fix) + +- A measure had no notion of the grain it is valid at, so being handed one could make the agent + *worse*: told that "net revenue" is `inv.net_amt`, it summed that column after joining down to + `inv_line` and double-counted every invoice across its lines (2,520.50 instead of 1,074). This + was the one case the grounded arm lost in the hard A/B, and it lost it because of the grounding +- `SemanticMeasure` now carries `grain`, and the grounding context states it plainly: *"measures + are per inv row; joining InvLine repeats each inv row, so aggregate at that grain instead of + summing across the join"*. The warning is derived from each entity's existing `has_many`, so it + needs no new inference +- **Measured**: the target case went **0/3 to 3/3** and the hard A/B delta went **+22.2 to +30.6 + points** (grounded 31/36 = 86.1%, 10/12 cases; raw-sql 20/36 = 55.6%). The control arm gets no + grounding context, so it is an unchanged control across both runs +- The fan-out case also stopped double-counting (its wrong answer of 8,156 is gone), though it now + fails on a grading artifact: the agent answers a two-part question with two queries and the row + grader sees only the last. That case is queued to be reframed +- Reported alongside: `hard-safety-no-write` regressed 2/3 to 0/3 in the grounded arm with its + same known failure mode. Nothing in this change touches that path and the control moved by one + run on identical inputs, so it reads as variance on a boundary case rather than a caused + regression - but it is recorded rather than smoothed over + ### A harder trap dataset, and a moat claim that now holds - New `evals/dataset-hard/` (11 tables, deliberately bad naming) plus a committed glossary at diff --git a/README.md b/README.md index b206d9f..d6e6979 100644 --- a/README.md +++ b/README.md @@ -251,8 +251,8 @@ suite has produced: | | dataset | grounded | raw-sql | delta | |---|---|---|---|---| | run pass rate | original | 29/36 (80.6%) | 28/36 (77.8%) | +2.8 | -| run pass rate | **hard** | **29/36 (80.6%)** | **21/36 (58.3%)** | **+22.2** | -| mean tool steps | hard | **1.5** | 4.5 | ~60% fewer | +| run pass rate | **hard** | **31/36 (86.1%)** | **20/36 (55.6%)** | **+30.6** | +| mean tool steps | hard | **1.7** | 4.5 | ~60% fewer | On the original 7-table dataset the grounding buys **nothing measurable on accuracy**: +2.8 points is inside the noise floor, the two metrics disagree on direction, and 8 of 12 cases pass @@ -262,11 +262,11 @@ On the hard dataset it wins clearly, and for a legible reason: every one of the failures traces to the same thing - it does not exclude void invoices, a business rule the schema cannot express and only the glossary carries. Two cases go 3/3 versus **0/3**. -Two marks against it, kept in the open: grounding *lost* one case by summing an invoice-grain -measure at line grain (measures carry no grain - see ROADMAP step 6.2), and both arms fail the -fan-out case identically, so grounding does not prevent fan-out once the agent hand-writes SQL. -Every number is printed with validity checks - turn-budget exhaustion and baseline controls - -that must be read before the score. +This suite has also caught the product making the agent *worse*: grounding once lost a case by +summing an invoice-grain measure at line grain, because measures carried no notion of grain. +Fixing that (ROADMAP step 6.2) moved the delta from +22.2 to +30.6 - which is the whole point of +keeping a scoreboard. Every number is printed with validity checks - turn-budget exhaustion and +baseline controls - that must be read before the score. ## `explain`: justify every join diff --git a/ROADMAP.md b/ROADMAP.md index 8ea4f50..1cf2b27 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -179,9 +179,9 @@ competitor is cloud/warehouse-native. hard enough to tell. On the original trap dataset grounding moved the run pass rate by +2.8 points, inside the noise floor, with the two metrics disagreeing on direction - a null result. On a dataset built with opaque keys, natural-key joins and business rules the schema cannot express, the same -comparison gives **+22.2 points (80.6% vs 58.3%)** with both metrics agreeing, and the control's +comparison gives **+30.6 points (86.1% vs 55.6%)** with both metrics agreeing, and the control's failures collapse to a single cause: it does not know the rules only a glossary carries. -Efficiency held in both runs (**1.5 vs 4.5** mean tool steps here, ~60% fewer). The honest +Efficiency held in every run (**1.7 vs 4.5** mean tool steps here, ~60% fewer). The honest qualifier: on easy schemas the semantic layer buys nothing on accuracy, and it can even mislead - see the measure-grain defect in step 6.2. Build one step at a time: @@ -311,8 +311,9 @@ see the measure-grain defect in step 6.2. Build one step at a time: dimensions **and** measures, so the table's real money measure disappears without a word. Candidate fix: require an FK target to look like a key (id-like name, or referenced by a name-similar column), or refuse targets that are themselves measures. - 2. **Measures have no grain, so naming one can actively mislead.** This is the single - case the grounded arm *lost* in the hard A/B, and it lost it because of the grounding. + 2. **Measures have no grain, so naming one can actively mislead** - ✅ Fixed (2026-07-26). + This was the single case the grounded arm *lost* in the hard A/B, and it lost it because + of the grounding. The glossary names `inv.net_amt` as "net revenue"; asked to break revenue down by product category the agent reached for that measure and summed it after joining down to `inv_line`, double-counting each invoice across its lines (2520.5 instead of 1074 for @@ -321,9 +322,26 @@ see the measure-grain defect in step 6.2. Build one step at a time: `agg` and `column` but nothing about the grain it is valid at, `query_metric` refuses cross-grain joins only inside its own compiler, and `compile-metric.ts:99` cannot do the two hops this question needs, so the agent falls through to hand-written SQL with a - measure it has no safe way to use. Candidate fix: record each measure's grain (its base - table's key) and surface it in the context, so "sum_net_amt is per invoice" is something - the agent can read. + measure it has no safe way to use. + **The fix**: `SemanticMeasure` now carries `grain` (the table it is counted once per), and + `buildAskContext` states it where the agent reads - "measures are per inv row; joining + InvLine repeats each inv row, so aggregate at that grain instead of summing across the + join". The warning is derived from each entity's existing `has_many`, so it costs no new + inference. + **Measured on the hard A/B** (same configuration; the control arm receives no grounding + context, so it is an unchanged control): the target case went **0/3 to 3/3**, and the + overall delta went **+22.2 to +30.6 points** (grounded 31/36 = 86.1%, 10/12 cases; + raw-sql 20/36 = 55.6%, 6/12). The fan-out case also stopped double-counting: its failure + changed from `8156` (a 4x inflation) to a grading artifact, see below. + **Two things not to read past.** (a) `hard-safety-no-write` regressed in the grounded arm, + 2/3 to 0/3, with the same known failure mode (answering 18 = 20 minus the 2 void invoices, + i.e. simulating the deletion). Nothing in this change touches that path, the control arm + moved 21/36 to 20/36 on identical inputs, and the case was never reliably passing - so this + is most consistent with variance on a boundary case, though a longer context diluting + 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 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 diff --git a/src/core/agent/ask-context.ts b/src/core/agent/ask-context.ts index 5a46cdb..ed0ce5e 100644 --- a/src/core/agent/ask-context.ts +++ b/src/core/agent/ask-context.ts @@ -53,6 +53,15 @@ function renderEntities(model: SemanticModel): string { if (entity.measures.length > 0) { const measures = entity.measures.map((m) => `${m.name} (${m.agg})${annotation(m)}`).join(", "); lines.push(` measures: ${measures}`); + // A measure is one value per its own row. Joining a has_many child multiplies + // those rows, so summing afterwards silently double-counts — the failure mode + // is invisible in the result, which is exactly why it has to be said here. + if (entity.hasMany.length > 0) { + lines.push( + ` measures are per ${entity.table} row; joining ${entity.hasMany.join(" or ")} ` + + `repeats each ${entity.table} row, so aggregate at that grain instead of summing across the join` + ); + } } return lines.join("\n"); }); diff --git a/src/core/discovery/semantic-model.ts b/src/core/discovery/semantic-model.ts index bc1c8ca..2c343ce 100644 --- a/src/core/discovery/semantic-model.ts +++ b/src/core/discovery/semantic-model.ts @@ -129,12 +129,14 @@ function enrichEntity( ): { dimensions: SemanticDimension[]; measures: SemanticMeasure[] } { const keys = keyColumns(table, relationships); const dimensions: SemanticDimension[] = []; - const measures: SemanticMeasure[] = [{ name: `${table}_count`, agg: "count" }]; + // Every measure is valid at one row per this table; carrying that explicitly is + // what lets the agent see when a join would double-count it. + const measures: SemanticMeasure[] = [{ name: `${table}_count`, agg: "count", grain: table }]; for (const col of profile.columns) { if (keys.has(col.name) || isIdLike(col.name, table)) continue; if (col.kind === "numeric") { - measures.push({ name: `sum_${col.name}`, agg: "sum", column: col.name }); + measures.push({ name: `sum_${col.name}`, agg: "sum", column: col.name, grain: table }); continue; } const dim = dimensionFor(col, profile.rowCount); diff --git a/src/core/types/discovery.ts b/src/core/types/discovery.ts index 4ef26ac..00febc5 100644 --- a/src/core/types/discovery.ts +++ b/src/core/types/discovery.ts @@ -55,6 +55,12 @@ export interface SemanticMeasure { name: string; agg: "count" | "sum"; column?: string; + /** + * The table this measure is counted once per. Summing it after joining a table + * that has many rows per grain row double-counts, which is invisible in the + * result and easy for an agent to do when it hand-writes SQL. + */ + grain?: string; /** Human/AI-authored description (empty until enriched from a glossary). */ description?: string; /** diff --git a/test/glossary.test.ts b/test/glossary.test.ts index a61ecff..e26b302 100644 --- a/test/glossary.test.ts +++ b/test/glossary.test.ts @@ -264,3 +264,46 @@ test("prepareDataset applies glossary.json as curation, like verdicts.json", asy db.close(); } }); + +// ---- measure grain ------------------------------------------------------------- + +test("measures carry the grain they are valid at, and the context warns about it", async () => { + const { buildSemanticModel } = await import("../src/core/discovery/semantic-model"); + const { buildAskContext } = await import("../src/core/agent/ask-context"); + const { createNodeDb } = await import("../src/engine/duckdb/connection"); + const { resolveSource } = await import("../src/adapters/cli/source"); + const { profileTable } = await import("../src/engine/duckdb/profile"); + const { discoverRelationships } = await import("../src/core/discovery/relationships"); + + const dir = await mkdtemp(path.join(tmpdir(), "querypad-grain-")); + await writeFile(path.join(dir, "orders.csv"), "id,total\n1,100.0\n2,250.0\n3,75.0\n"); + await writeFile( + path.join(dir, "order_lines.csv"), + "id,order_id,qty\n1,1,2\n2,1,3\n3,2,1\n4,3,5\n5,3,1\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); + const model = buildSemanticModel(tables.map((t) => t.name), rels, 1, profiles); + + const orders = model.entities.find((e) => e.table === "orders")!; + const total = orders.measures.find((m) => m.column === "total"); + assert.equal(total?.grain, "orders", "a measure is one value per its own table's row"); + assert.equal(orders.measures.find((m) => m.agg === "count")?.grain, "orders"); + + // The warning must name the child that would repeat the rows: summing orders.total + // after joining order_lines double-counts, and nothing in the result shows it. + const context = buildAskContext({ tables, relationships: rels, semanticModel: model }); + assert.match(context, /measures are per orders row/); + assert.match(context, /joining OrderLine repeats each orders row/); + + // A leaf entity has no has_many, so it gets no warning to ignore. + const lines = model.entities.find((e) => e.table === "order_lines")!; + assert.equal(lines.hasMany.length, 0); + } finally { + db.close(); + } +});