ACE-083: classify traps by what a fan-out can actually inflate, and give the scope walk a scope - #160
Conversation
…nd give the scope walk a scope The fan rule decided from "a one-side column appears in some aggregate" x "one-to-many cardinality" rather than from "a duplication-sensitive rollup of a one-side measure" x "one-to-many cardinality". Three consequences, two costing correct queries a refusal and one letting an inflated one through clean. 1. Fan-immune aggregates no longer feed the fan rule. MIN/MAX, any DISTINCT-qualified aggregate and the boolean folds are idempotent under row duplication, so a fan-out cannot change their value and the query was already right. Decided on the parsed node (type + the distinct argument), never on a name string: this gate reads each statement in its datasource's own grammar, and a name allowlist breaks on the first engine that spells an aggregate differently. SUM, AVG, COUNT(col) and the ordered/array aggregates stay in-scope, with one test each so the exemption cannot widen unnoticed. 2. A one-side column inside an aggregate is no longer confused with the one-side column being aggregated. SUM(order_items.quantity * orders.total_amount) sums one value per many-side row that a per-order rate scales; nothing is duplicated. The genuine trap names no many-side column, so it still fires. 3. The scope walk stops at a CTE body. It was a flat find_all over the whole tree, which broke in both directions: tables leaked OUT of a CTE body, so pre-aggregating a measure in a CTE before joining -- the remediation this guard itself recommends -- was refused; and a CTE alias never resolved, so wrapping a table in a pass-through WITH hid the one side entirely and an inflated total came back clean. A CTE alias now resolves to its underlying table only when the body preserves grain, transitively; a grain-changing CTE is its own entity and does not inherit the source table's cardinality; and a body that joins, unions, or reads a table the model does not declare does not resolve at all. That last case no longer returns a bare allow -- PreFlightResult carries a certainty, and an unprovable lineage says so instead of implying the query was checked. The two callers that ask a coverage question rather than a cardinality one -- the receipt's provenance list and the row-scoping pass -- keep the flat walk under its own name, so neither loses a table. No verdict changed across the safety corpus or the shipped example queries. Spec: ACE-083
There was a problem hiding this comment.
Pull request overview
This PR tightens the SQL pre-flight fan-trap classifier to reduce false refusals by distinguishing duplication-immune aggregates and co-factor usage, and it introduces a scope-aware table walk so CTE bodies don’t leak into outer cardinality analysis while grain-preserving CTE aliases can still resolve transitively.
Changes:
- Exempt duplication-immune aggregates (e.g., MIN/MAX, DISTINCT-qualified, boolean folds) from the fan-trap rule, and avoid treating one-side scalar co-factors as “one-side measures.”
- Add scope-aware table binding for cardinality analysis, including CTE alias resolution only when the CTE preserves grain (with transitive resolution).
- Add a comprehensive test suite covering fan-immune vs fan-sensitive aggregates, co-factor cases, and CTE scoping/grain behavior, plus an “uncertain” certainty mode when lineage can’t be established.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_semantic_model_runtime.py | Adds regression tests for fan-trap precision (immune vs sensitive aggregates), measure-vs-co-factor attribution, and CTE scope/grain handling. |
| packages/agami-core/src/semantic_model/runtime.py | Refines fan-trap detection logic, introduces scope-aware table walking with CTE grain classification, and adds certainty to pre-flight results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _preserves_grain(body: "exp.Select") -> bool: | ||
| """Is `body` one output row per source row? A WHERE or a LIMIT still is; a GROUP BY, a | ||
| SELECT DISTINCT or an aggregate is not. Matched by node type, per `_own_child`.""" | ||
| if _own_child(body, (exp.Group, exp.Distinct)) is not None: | ||
| return False | ||
| return body.find(exp.AggFunc) is None |
| return { | ||
| table | ||
| for _alias, table, grain in _scope_bindings(tree) | ||
| if grain is not None and (grain.opaque or table not in known_tables) | ||
| } |
Converting to draft — the loosening is unbounded in four shapesReview found, and I independently reproduced through
Why each is wrongThe first two — the co-factor rule is too broad. The second two — a grain-changing CTE is assumed to sit at the join key's grain, and never checked against it. Two more, same root
And My error, not the reviewer's findThe non-regression criterion I wrote only guarded Next commit adds that reverse assertion first, then narrows the two rules. |
Spec: ACE-083
The fan-trap detector decides from (a one-side column appears in some aggregate) × (one-to-many cardinality) rather than from (a duplication-sensitive rollup of a one-side measure at one-side grain) × (one-to-many cardinality). Three fixes restore the missing terms. Closes #151.
What was wrong
Measured through
pre_flight_checkagainst the test fixture model:COUNT(DISTINCT orders.id) FROM orders JOIN order_items …fan_trapDISTINCTde-duplicates; a fan-out cannot inflate itMAX(orders.total_amount)over the same shapefan_trapSUM(order_items.quantity * orders.total_amount)fan_trapfan_trapWITH o AS (SELECT * FROM orders) SELECT SUM(o.total_amount) FROM o JOIN order_items …allowThe last two share one root cause:
_tables_in_scopewas a flatfind_all(exp.Table)over the whole tree with no concept of scope, so CTE-body tables leaked into the outer query (false refusals) while CTE aliases never resolved to their underlying table (false allows).Changes
MIN,MAX,DISTINCT-qualified aggregates and boolean folds no longer feed the fan rule. Matched on the sqlglot node type and thedistinctarg, not a name string, so it survives a dialect spelling it differently.SUM,AVG,COUNT(*),COUNT(col),STRING_AGGandARRAY_AGGstill trip it — one test each so the exemption cannot silently widen.AVGis the one worth naming: a fan duplicates rows unevenly, so averages do shift._tables_in_scopeis outer-scope only; a CTE alias resolves to its underlying table only when the CTE preserves grain (noGROUP BY/DISTINCT/ aggregate), transitively; a grain-changing CTE keeps its own grain instead of inheriting the source table's cardinality; anything unclassifiable yieldscertainty="uncertain"rather than a silentallow.This changes what counts as a trap, never what happens once one is found. The
auto_rewritebranch is untouched.Verification
dev.py check: 2304 passed, 95 skipped, 2 xfailed; ruff + gitleaks clean; no vendored-lib drift.Collected-test delta is exactly the new cases: 2377 → 2401.
Red-on-base, measured rather than asserted — base source with the new tests: 16 failed, 8 passed. With the fix: 24 passed. The 8 passing on base are the guardrail assertions (the fan-sensitive aggregates that must still refuse), which are supposed to be green on base.