fix: a unique price column is not a foreign-key target - #22
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
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 got "explained" as a foreign key into it. Measured at 81% / 68% / 54% on a realistic fixture when the hard eval dataset was first built - which is how this was found.
The damage is worse than a wrong edge in the graph. Both endpoints of a relationship are excluded from the semantic model, so a phantom edge silently deletes the table's real measure:
Nothing in the output says why. On the hard dataset this removed
sum_net_amtfrom the invoice table entirely.What
A non-id target is 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 join target.isIdLikemoves tosignals.tsas the single shared definition. The two halves of the engine had quietly disagreed:semantic-model.tsalready carried the comment "a unique, non-null column likeamountis a real measure, not a key" - and inference did not know that.Verification
sku -> skujoin survives at 100%, and both measures come back. A regression test pins exactly this.npm run eval:engine18/18 andnpm run eval:engine:hard25/25, both unchanged - so no real edge was lost, including the three natural-key joins the hard dataset depends on.npm run checkexit 0;npm run test:cli142 pass / 0 failROADMAP step 6.1. Found by building a harder eval dataset, which is the second engine defect that work has surfaced.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CKZ9UVgw2Mhiu6T5YAG7k6