From 22078d034e9189b35a780d8c93aa20eaca94f092 Mon Sep 17 00:00:00 2001 From: Kiyeon Jeon Date: Sun, 26 Jul 2026 01:16:48 +0900 Subject: [PATCH] fix: a unique price column is not a foreign-key target Relationship inference treated any unique, non-null column as a valid join target. A small price list has unique prices, so every money column whose values coincided was explained as a foreign key into it - measured at 81/68/54% on a realistic fixture when the hard eval dataset was first built. The damage was worse than a wrong edge. Both endpoints of a relationship are excluded from the semantic model, so a phantom edge silently deleted the table's real measure: the invoice table stopped exposing sum_net_amt at all, with nothing in the output to say why. A non-id target is now credible only when the foreign column names it outright (nameSimilarity >= STRONG_NAME_SIMILARITY), which is what a genuine natural key looks like: sku -> sku, region_cd -> region_cd. Uniqueness alone no longer qualifies a column as a target. isIdLike moves to signals.ts as the single shared definition. The two halves of the engine had disagreed about it - semantic-model.ts already carried the comment 'a unique, non-null column like amount is a real measure, not a key', and inference did not know that. Verified on an isolated fixture (a 4-row price list whose prices coincide with a sales table's line amounts): the phantom edge disappears, the legitimate sku join survives at 100%, and both sum_line_amt and sum_list_amt come back. Both engine suites are unchanged (18/18 and 25/25), so no real edge was lost. --- CHANGELOG.md | 18 ++++++++++ ROADMAP.md | 15 ++++++-- src/core/discovery/relationships.ts | 15 ++++++++ src/core/discovery/semantic-model.ts | 12 +------ src/core/discovery/signals.ts | 10 ++++++ test/discovery.test.ts | 51 ++++++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4419062..17a8681 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,24 @@ and public product updates. ## Unreleased +### 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. + A small price list has unique prices, so every money column whose values happened to coincide + was "explained" as a foreign key into it - measured at 81%, 68% and 54% on a realistic fixture +- The damage was worse than a wrong edge in the graph. Both endpoints of a relationship are + excluded from the semantic model, so a phantom edge **silently deleted the table's real + measure**: an invoice table stopped exposing `sum_net_amt` at all, with nothing in the output + to say why +- A non-id target is now credible only when the foreign column names it outright, which is what + a genuine natural key looks like (`sku -> sku`, `region_cd -> region_cd`). Uniqueness alone no + longer qualifies +- `isIdLike` moved to `signals.ts` as the single shared definition. The two halves of the engine + had disagreed about it: the semantic model already knew that "a unique, non-null column like + `amount` is a real measure, not a key", and inference did not +- Both engine suites are unchanged (18/18 and 25/25), so no real edge was lost, and a regression + test pins the fixture that reproduces it + ### 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 diff --git a/ROADMAP.md b/ROADMAP.md index 1cf2b27..a51a85a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -302,15 +302,24 @@ see the measure-grain defect in step 6.2. Build one step at a time: With the web app retired this is the interactive surface: the coding agent is the UI. 6. **Engine defects surfaced by the hard dataset** - open, and worth fixing before more semantic-layer work. - 1. **Numeric value overlap creates phantom foreign keys.** `isKeyCandidate` + 1. **Numeric value overlap creates phantom foreign keys** - ✅ Fixed (2026-07-26). `isKeyCandidate` (`src/core/discovery/relationships.ts:38`) accepts any column that is unique and non-null in its own table, so a small lookup table's `list_amt` is a valid FK *target*. Every money column whose values coincide with those prices then gets an edge (measured: 81%, 68%, 54% on the hard fixture before it was made realistic). The damage is not just a wrong edge in the graph: `keyColumns` (`semantic-model.ts:75`) excludes both endpoints of every edge from 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. + **The fix**: a non-id target is only credible when the foreign column names it outright + (`nameSimilarity >= STRONG_NAME_SIMILARITY`), which is what a real natural key looks like - + `sku -> sku`, `region_cd -> region_cd`. Uniqueness alone no longer qualifies a column as a + join target. `isIdLike` moved to `signals.ts` and is now the single definition shared by + relationship inference and model derivation, which previously disagreed about it: the + semantic model already knew "a unique, non-null column like `amount` is a real measure, not + a key", and inference did not. + Verified on an isolated fixture (a 4-row price list whose prices coincide with a sales + table's line amounts): the phantom edge disappears, the legitimate `sku` join survives at + 100%, and both `sum_line_amt` and `sum_list_amt` come back. Both engine suites are + unchanged (18/18 and 25/25), so no real edge was lost. 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. diff --git a/src/core/discovery/relationships.ts b/src/core/discovery/relationships.ts index 050d9ac..ff19436 100644 --- a/src/core/discovery/relationships.ts +++ b/src/core/discovery/relationships.ts @@ -5,6 +5,7 @@ import { NAME_SIMILARITY_FLOOR, OVERLAP_FLOOR, STRONG_NAME_SIMILARITY, + isIdLike, cardinalityShapeScore, confidence, isTypeCompatible, @@ -92,6 +93,20 @@ export async function discoverRelationships( ) { continue; } + + // Uniqueness alone does not make a column a key: a small price list has + // unique prices, so every money column that happens to match one of them + // would be "explained" as a foreign key into it. Worse than a wrong edge, + // that silently deletes the measure — both endpoints of a relationship are + // excluded from the semantic model. A non-id target is therefore only + // credible when the foreign column names it outright, which is what a real + // natural key looks like (sku -> sku, region_cd -> region_cd). + if ( + !isIdLike(keyColumn.name, keyProfile.tableName) && + nameScore < STRONG_NAME_SIMILARITY + ) { + continue; + } candidates.push({ keyTable: keyProfile.tableName, keyColumn, diff --git a/src/core/discovery/semantic-model.ts b/src/core/discovery/semantic-model.ts index 2c343ce..a2b4d7d 100644 --- a/src/core/discovery/semantic-model.ts +++ b/src/core/discovery/semantic-model.ts @@ -6,7 +6,7 @@ import type { SemanticMeasure, SemanticModel, } from "../types/discovery"; -import { singularize, splitTokens } from "./signals"; +import { isIdLike, singularize, splitTokens } from "./signals"; /** * Roll inferred relationships into named business entities. Pure — tables + @@ -81,16 +81,6 @@ function keyColumns(table: string, relationships: Relationship[]): Set { return keys; } -/** - * True for id-named columns (surrogate keys). We exclude these from measures/dimensions - * by name rather than by uniqueness — a unique, non-null column like `amount` is a real - * measure, not a key, so summing it is meaningful. - */ -function isIdLike(name: string, table: string): boolean { - const lower = name.toLowerCase(); - return lower === "id" || lower.endsWith("_id") || lower === `${singularize(table)}_id`; -} - /** Derive a dimension candidate from a non-key column, or null if it isn't one. */ function dimensionFor(col: ColumnProfile, rowCount: number): SemanticDimension | null { if (col.kind === "date") { diff --git a/src/core/discovery/signals.ts b/src/core/discovery/signals.ts index d60b3c3..50a77e4 100644 --- a/src/core/discovery/signals.ts +++ b/src/core/discovery/signals.ts @@ -82,6 +82,16 @@ export function nameSimilarity( } /** Are two column kinds joinable at all? */ +/** + * True for id-named columns (surrogate keys). Deliberately name-based, not + * uniqueness-based: a unique, non-null column like `amount` is a real measure, not a + * key, so summing it is meaningful and joining on it is not. + */ +export function isIdLike(name: string, table: string): boolean { + const lower = name.toLowerCase(); + return lower === "id" || lower.endsWith("_id") || lower === `${singularize(table)}_id`; +} + export function isTypeCompatible(a: ProfileColumnKind, b: ProfileColumnKind): boolean { if (a === b) return true; // numbers stored as text vs numeric still routinely join after casting diff --git a/test/discovery.test.ts b/test/discovery.test.ts index 3bb663e..952ef23 100644 --- a/test/discovery.test.ts +++ b/test/discovery.test.ts @@ -124,3 +124,54 @@ test("inspect fixtures yields exactly the two true relationships", async () => { db.close(); } }); + +// ---- non-id join targets ------------------------------------------------------- + +test("a price column is not a join target, so its measure survives", async () => { + const { mkdtemp, writeFile } = await import("node:fs/promises"); + const { tmpdir } = await import("node:os"); + const { resolveSource } = await import("../src/adapters/cli/source"); + const { buildSemanticModel } = await import("../src/core/discovery/semantic-model"); + + const dir = await mkdtemp(path.join(tmpdir(), "querypad-fk-target-")); + // A small price list has unique prices, so it passes the unique + non-null test + // that used to be the whole definition of a join target. + await writeFile( + path.join(dir, "price_list.csv"), + "id,sku,list_amt\n1,A,10.00\n2,B,20.00\n3,C,30.00\n4,D,40.00\n" + ); + // line_amt holds those same values by coincidence. It is a measure, not a key. + await writeFile( + path.join(dir, "sales.csv"), + "id,sku,line_amt\n1,A,10.00\n2,B,20.00\n3,A,10.00\n4,C,30.00\n5,D,40.00\n6,B,20.00\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 keys = rels.map(relationshipKey); + + // The natural key still joins: the foreign column names the target outright. + assert.ok(keys.includes("sales.sku->price_list.sku"), `expected the sku join, got ${keys}`); + // The money columns must not be mistaken for one, at any confidence. + assert.ok( + !keys.includes("sales.line_amt->price_list.list_amt"), + `value overlap alone must not make a price a join target, got ${keys}` + ); + + // The damage this prevents: both endpoints of a relationship are dropped from the + // semantic model, so a phantom edge silently deletes the table's real measure. + const model = buildSemanticModel(tables.map((t) => t.name), rels, 1, profiles); + const sale = model.entities.find((e) => e.table === "sales")!; + const priceList = model.entities.find((e) => e.table === "price_list")!; + assert.ok(sale.measures.some((m) => m.column === "line_amt"), "sales must keep sum_line_amt"); + assert.ok( + priceList.measures.some((m) => m.column === "list_amt"), + "price_list must keep sum_list_amt" + ); + } finally { + db.close(); + } +});