DEV-1742: one-doctrine consolidation of SQL generation (umbrella) - #286
Merged
Conversation
PR 1 of the DEV-1742 consolidation. Foundations for the doctrine: every
CTE name minted by the allocator (P-F), and one ValueKey render policy
(P-G). Superseded code stays callable per the chain's operating rule.
B4 — cross-model CTE naming. `_cm_` names were built by a doubly-lossy
helper (`flat_name`, itself non-injective, then a non-identifier
`re.sub`) and the resulting string doubled as the plan's identity key in
`seen_cm`. Two failures were reachable from the public query API:
* two measures whose canonical aliases differ only in case emitted two
`_cm_` names that fold together on every case-folding dialect, so the
collision belt raised. `_wm_` had been retrofitted onto the allocator;
`_cm_` never was.
* two genuinely distinct aggregates that sanitised alike made the second
skip the loop body, leaving its join-back and column-alias maps
unwritten — `KeyError` at the first unconditional downstream subscript.
Dedup now keys on the typed AggregateKey plus source relation; the name is
allocator-minted once and stored, so the five sites that re-derived it read
it instead. Deliberately NOT keyed on the canonical alias: that omits
`column_filter_key`, so a filtered and an unfiltered aggregate over one
column share an alias while needing two CTEs — deduping on the string would
silently merge them, a wrong answer rather than a crash.
B5 — one ScalarCall policy. Two of the five renderers returned
`exp.Anonymous` passthrough, so `ifnull` reached Postgres, which has no
`IFNULL`, while the same key emitted `COALESCE` from a projection. All six
paths now call one `render_scalar_call`. Transpiling alone was not enough:
`exp.func("LOG10", x)` normalises to a generic `Log(10, x)` re-emitting as
`LOG(10, x)`, wrong on dialects with a native single-arg `LOG10` — so the
policy is transpile-then-log-rewrite.
B10 — `ScopeFrame._model_for` raises instead of falling back to the root
model, which expanded a different model's derived SQL and turned a wiring
bug into a wrong answer.
Also: four drifted copies of canonical-aggregate-alias derivation collapse
to one profile-based function in `naming.py` (the four callers delegate,
keeping their signatures); the windowed `exp.Sum if agg == "sum" else
exp.Avg` catch-all becomes a registry lookup that raises; `_build_agg`'s
five dispatch mechanisms become one table; step-CTE names and the
structural aliases route through the allocator or a shared constant.
Approved behavior change beyond the B-items: `concat` now emits `||` on
Postgres/DuckDB where it emitted `CONCAT(...)`. Semantic, not cosmetic —
Postgres `CONCAT()` ignores NULL operands and `||` propagates them — and
kept for consistency, since the projection path has always emitted `||`.
The renderer's API is complete and tested but production paths are not yet
rerouted through it; that lands with the cross-scope migration in PR 3.
Rationale and per-call-site detail in the value_expr module docstring,
DECISIONS.md, and a handoff comment on DEV-1746.
Tests: 196 new (result-key contract pack, naming/allocator, renderer).
Full non-integration suite 9503 passed; SQLite+DuckDB integration 118
passed; ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two real defects in the new renderer, plus the review's structural points.
Operator precedence was not materialised. sqlglot does NOT parenthesise by
node nesting: `Mul(Add(a, b), c)` emits `a + b * c`, which evaluates
differently. The generator has `_paren_if_lower_prec` for exactly this; the
new renderer had nothing, so any nested arithmetic would have rendered with
the wrong grouping once the production paths route through it.
Unary operators were dropped. The binder represents `-10` as a SINGLE-operand
ArithmeticKey; a fold that starts at operands[0] and iterates operands[1:]
returns it unchanged, so `amount > -10` became `amount > 10`. `not` was
missing entirely. Both now go through one composer that mirrors the
generator's, including the `is` / `is not` forms.
From the review:
* TimeTruncKey went through a literal DATE_TRUNC instead of the dialect's
build_date_trunc, so it named a function SQLite does not have and had no
week_sunday handling. It now delegates, with per-dialect tests.
* The no-builder aggregate path ignored column_filter_key and args/kwargs,
so a filtered aggregate would have rendered as a plain SUM covering rows
the filter excludes. Both now fail closed.
* _literal stringified unsupported types; it raises, matching the
generator's helper.
* _rewrite_log_alias duplicated the generator's copy of the very policy
this PR consolidates. The generator now delegates to the shared one.
* `canonical_alias` was read from a leaked loop variable after this PR
removed its per-iteration assignment. Latent — the planner populates
public_aliases, so the fallback that reads it is not reachable today —
but it would have projected one measure under another's name.
* _wm_ CTE naming routes through the shared cte_name_from_alias.
* Registry membership is checked both ways: a key that is NOT a built-in
would make is_builtin_agg accept a typo.
Conventions: helpers take keyword-only arguments; test imports moved to the
top of their files; temp dirs go through pytest's tmp_path_factory instead of
leaking mkdtemp directories; volatile generator line numbers dropped from
docstrings in favour of the stable function names.
Sonar: NOSONAR with rationale on the two consolidated dispatch functions and
on the ASCII-only identifier regex — `\W` is Unicode-aware in Python and would
let accented letters into a name that must be a bare ASCII SQL identifier.
Tests: 211 in the three new files (up from 196), full non-integration suite
9518 passed, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ates The `concat` change is user-visible, so it belongs in the reference docs, not only in the decision log: on backends whose natural spelling is `||` (Postgres, DuckDB, SQLite) `concat(a, b)` propagates NULL, unlike those backends' own `CONCAT()`. Documented with the `ifnull(...)` workaround, next to the related `log10` / `log2` single-argument note. The five `if name == "like"` short-circuits ahead of `render_scalar_call` were dead weight — that function already special-cases the operator — so each call site was re-stating the policy the consolidation just centralised. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
resolve_agg_entry raises for an unknown name, so the is-not-None assertions could never fail. The raising contract is pinned by its own test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three fixes from the last round shipped without a test. Closing that.
The leaked `canonical_alias` is the important one. My first attempt at a test
passed with AND without the fix, so it proved nothing; I said so rather than
keeping it. Tracing the trim guard found the reachable shape:
trim_hidden = plan.hidden and not planned_query.transform_layers
A HIDDEN cross-model aggregate feeding a transform chain is therefore NOT
trimmed, stays projected, and — having no user-declared name — is the one
caller that reaches the canonical-alias fallback. `cumsum(customers.revenue:sum)`
alongside a second cross-model measure hits it exactly.
The emitted SQL without the fix:
base AS (SELECT ...,
_cm_..._revenue_sum."orders.customers.revenue_sum" AS "orders.customers.Rev_sum",
_cm_..._Rev_sum."orders.customers.Rev_sum" AS "orders.rv" ...)
step1 AS (SELECT ...,
SUM("orders.customers.Rev_sum") OVER (...) AS "orders.run" ...)
The hidden aggregate is projected under the OTHER measure's name, that alias
is emitted twice, and the window function then sums the wrong measure. So this
was a wrong-answer bug, not the cosmetic naming slip it looked like. The test
asserts no output alias is emitted twice, which is what actually breaks, and
was verified to fail with the fix reverted.
Also added: the aggregation registry's both-ways membership invariant (a
registry key that is not a built-in would make `is_builtin_agg` accept a typo),
that the generator's log-alias rewrite agrees with the shared policy it now
delegates to across four dialects, and that `_wm_` CTE names survive a
case-only collision the same way `_cm_` now does.
Tests: 9527 passing, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A cross-model star is another wrong-number path on the no-builder branch. `StarKey.path` is non-empty for `customers.*:count`, and the branch emitted a bare `exp.Star()` for every StarKey — dropping the hop, so the count would cover HOST rows instead of the joined relation. Same failure class as the column-filter and kwargs guards, so it gets the same treatment: routing a cross-model star needs the join graph, so the no-builder path refuses it. Tested both ways — the cross-model star raises (verified failing with the guard reverted), and the ordinary local `*:count` still renders `COUNT(*)`. Also: `build_date_trunc` operands passed by keyword, one constant for the repeated facility name (S1192), one more `pytest.raises` narrowed to a single throwing call (S5778), and a composite assertion split (S9073). Tests 9529 passing, ruff clean, CI green, no Sonar issues outstanding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every renderer defect this PR's review surfaced was one shape: a typed key
carries a field, a render path ignores it, and the query returns a WRONG
NUMBER rather than failing. Dropped column filter, dropped kwargs, dropped
star path, dropped unary sign, lost operator precedence, stale alias.
That generalises into a property: changing any field of a key must change the
emitted SQL, or the render must refuse. Two materially different keys that
render identically means the field vanished.
`TestNoKeyFieldIsSilentlyIgnored` sweeps ~21 single-field mutations across the
union and asserts exactly that, with "raises" counting as a pass — refusing to
render a key the context cannot honour is the fail-closed contract.
It immediately found an instance the review did not: the TOP-LEVEL StarKey
branch dropped `path` the same way the aggregate branch did. Reviewers flagged
the aggregate one; the bare `StarKey(path=("customers",))` still rendered as
`*`. Now guarded identically.
Also swept the LIVE legacy filter renderer with the same mutations. One
apparent hit — `AggregateKey.kwargs` — is a probe artifact, not a bug:
calling that renderer directly with an empty `slot_by_key` takes a degenerate
path that production never reaches. Verified end-to-end that a HAVING over
`percentile(p=0.1)` vs `p=0.9` emits different SQL and returns different rows.
Tests 9550 passing, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The datetime was constructed inside the raises block, so the assertion could have been satisfied by a constructor failure rather than by _literal. Hoisted, and its import moved to the top of the file per the repo rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
I had been running CodeRabbit, Sonar and CI each loop but only ran Codex once,
against the ORIGINAL implementation. The renderer changed substantially after
that (precedence, unary handling, the fail-closed guards), so it got a fresh
pass. Three findings, all the same family as the rest — output that still
parses and still returns rows, but means something else.
Precedence covered only ARITHMETIC nodes. A comparison or boolean nested
inside arithmetic therefore lost its parentheses:
Add(GT(a, b), 1) -> a > b + 1 # parses as a > (b + 1)
EQ(GT(a, b), TRUE) -> a > b = TRUE
Add(And(a, b), 1) -> a AND b + 1
The table now spans OR / AND / NOT / comparisons / arithmetic so any
lower-precedence child gets wrapped, whatever its kind.
Comparisons were left-folded like arithmetic. `a < b < c` became
`(a < b) < c` — a boolean compared to a number — and `is` / `is not` read
operands[0] and [1], silently dropping any third. The Mode-B parser rejects
chained comparisons, so this is unreachable from user input today; it is the
structural backstop for anything building keys directly, and folding was the
wrong default for an operator that is strictly binary.
`contains_aggregate` tested `phase >= AGGREGATE`. Every TransformKey is POST
phase whether or not it wraps an aggregate, so a transform over a raw column
reported True — and that predicate decides GROUP BY / HAVING placement. It is
now a structural walk for an actual AggregateKey. The function has no callers
yet; PR 3 adopts it when it replaces the `any_agg` tuple, which is exactly why
it should not have been left with a phase test standing in for a tree walk.
Each fix has a test verified to fail without it.
Tests 9559 passing, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reviewing the previous round's fixes found three more, one of them a real wrong-number bug my own fix had left behind. Equal-precedence right children were parenthesised based on the PARENT operator alone, which is not enough. `Mul(a, Mod(b, c))` emits `a * b % c`, regrouping to `(a * b) % c`: with a=2 b=3 c=2 that is 0 where the tree says 2. Integer division has the same shape. The rule is now the inverse — an equal-precedence right child keeps its parens UNLESS the pair is genuinely associative (`+` over Add, `*` over Mul), so the safe cases stay quiet and everything else is grouped explicitly. Arity was implicit. Zero operands reached `operands[0]` and raised IndexError; a single-operand `and` fell into the unary branch and reported "unsupported unary operator 'and'". Both now fail (or succeed) deterministically: empty raises with the operator named, and a one-term conjunction returns that term, which is what it means. `contains_aggregate` walked only `TransformKey.input`. `partition_keys` and `time_key` are expression dependencies too, so `rank(x, partition_by=revenue:sum)` reported no aggregate while emitting one — and that predicate decides GROUP BY versus HAVING placement. All six new tests verified to fail with the fixes reverted. Tests 9573 passing, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ptions
Two findings, one of them a LIVE bug reachable from user input today (unlike
most of this PR's findings, which were latent in the not-yet-wired renderer).
Scalar arity was never validated. sqlglot handles it three different ways and
all three are bad answers for a mistyped filter:
round(amount, 2, 99) -> ROUND(CAST(amount AS DECIMAL), 2)
the third argument SILENTLY DROPPED
length(status, status) -> LENGTH(a, b)
invalid SQL, backend rejects it later
lower(status, status) -> raw sqlglot ValueError leaking internals
The binder validated arity for `like` and nothing else, so the first two
reached the database. There is now an arity table beside the allowlist that
knows the answer, checked at bind time (where a user's typo should surface,
naming the function and the counts) and again in `render_scalar_call` as the
fail-closed backstop. All four cases now report e.g. "Scalar function 'round'
takes 1 to 2 arguments; got 3."
The associativity exception is gone. Last round I let `+` over Add and `*`
over Mul drop their parens as "genuinely associative". That holds over the
reals and fails over the machine: with floats, rounding makes `a + (b + c)`
and `(a + b) + c` differ, and fixed-precision decimals add overflow. The
binder built a specific tree and emitting a different one is a silent accuracy
change, so every equal-precedence right child now keeps its parens — one fewer
special case, and the previous round's test asserting the opposite is inverted
with the reasoning recorded rather than deleted.
Tests 9593 passing, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Live and reachable from an ordinary filter. SQL's three-valued logic makes a
NULL member a trap rather than a member test:
status in ('a', None) -> IN ('a', NULL) matches only 'a'
status not in ('a', None) -> NOT IN ('a', NULL) matches NOTHING
The second is the dangerous one. `NOT IN` with a NULL evaluates to NULL for
every row, so a user asking for "everything except a" got an empty result set
with no error, no warning, and SQL that looks correct. Verified end-to-end
before fixing: rows went from ['b', 'c'] to [].
Rejected at bind time with a message that names the fix (`is null` /
`is not null` alongside the IN over the non-null values) rather than lecturing
about three-valued logic, and again in the renderer as the backstop.
This is a user-facing behavior change beyond the ratified B-items: a query
that previously ran now errors. Surfaced deliberately — it previously ran and
returned the wrong answer, which is the failure mode this PR exists to remove.
Found by the fourth Codex pass, which also confirmed three things I had asked
about and could not settle myself: aggregating a materialised alias across a
projection boundary is correct, Paren wrappers do not hide a Log node from the
log-alias rewrite, and the hand-derived scalar arity table matches real SQL
signatures (it is now a user-facing gate, so a wrong bound would have rejected
legitimate queries).
Tests 9599 passing, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… docs Two reviewers converged on the same branch from different angles, which is what made it worth acting on. CodeRabbit noted the dedup is unreachable today; Codex noted that IF it is reached, `(source_relation, AggregateKey)` is the wrong key. Codex is right. The forward and rerooted render paths produce different join-back pairs and a different aggregate column alias — forward uses the canonical alias, rerooted uses the sub-plan's. Two plans sharing an identity but differing in reroot shape would make the second silently inherit the first's CTE, joining at the wrong grain or reading the wrong column. The planner interns each key to one slot and emits one plan per slot, so this cannot happen today; extracting `_cm_plan_identity` and folding the shape in means a future planner change cannot make it silently wrong, and gives the rule a name and a test instead of leaving it implicit in a tuple literal. Docs (CodeRabbit): my `ifnull` example sat in `SlayerQuery.filters` while the allowlist line above it listed only the string-hygiene subset — valid code against an incomplete doc. The list now matches SCALAR_FUNCTIONS (null handling, math, string hygiene, like), the Rejects column no longer contra- dicts it by naming `coalesce` as rejected, and the two new user-facing rules from this PR are written down: argument counts are validated, and NULL is rejected inside an `in` list with the `is null` workaround shown. Tests 9603 passing, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unary branches were added earlier in this PR to stop `-10` losing its
sign, but they passed the operand straight into `exp.Neg` / `exp.Not` without
the precedence pass the binary path uses:
-(a + b) -> -a + b which is (-a) + b
not (a and b) -> NOT a AND b which is (NOT a) AND b
Both parse cleanly and both mean something else — the second is a De Morgan
error, so it silently returns a different row set.
`exp.Neg` joins the precedence table at 7 (unary minus binds tighter than any
binary arithmetic) and both unary branches now route their operand through
`_paren_if_lower_prec`. `NOT` stays at 3, so `NOT a > b` is left alone —
NOT already binds looser than a comparison, and over-wrapping would be noise.
Checked while fixing: nested negation emits `- -a` with a space, so there is
no `--` comment hazard.
Found by CodeRabbit on the same commit where Codex returned its first clean
pass in six — a useful reminder that the two are not substitutes.
Tests 9607 passing, ruff clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nt fix (W2)
Test pack (commit 1 of the plan) plus the first implementation slices.
Golden harness — deferred items 10 and 11:
* SLAYER_UPDATE_GOLDEN=1 now rewrites ONLY keys listed in ALLOWED_DELTAS, so
an unintended SQL change can no longer be blessed wholesale with 69 others.
A blessed entry is stale by construction and must be deleted, which keeps
every committed state's manifest empty.
* Entries that raise record the full structured error instead of a bare
"<<RAISED T>>"; messages are byte-deterministic, so a different failure of
the same type can no longer pass unnoticed.
W1 — the one door on ScopeFrame:
* enter_predicate / enter_expression over one implementation, differing only
in the parse helper. Prequote, parse, scan, expand, re-parse, re-scan,
union into join_paths, then Law 2. Discovery is a side effect of entering.
* No qualification pass (D10): expand_derived_refs_sync already qualifies
against the OWNING model and deliberately leaves opaque CTE/subquery refs
alone; a blanket root pass would corrupt exactly those.
* D1: parse failure raises ModeASqlParseError carrying the fragment and its
location. The three swallow-all lanes are gone from the production path —
including _filter_join_paths._scan, which turned an unparseable fragment
into ZERO join paths, i.e. missing joins rather than an error.
* SQLGenerator._parse and _parse_predicate now delegate to one shared
render/parse module, so the door and the generator normalise identically.
W2 — the _cm_ fragment-join bug: template fragments (string kwargs plus
non-overridden AggregationParam.sql defaults) now register their crossed joins
in the cross-model CTE, via the same helper the host path uses. Previously
SUM(customers.spend * regions.weight) FROM customers emitted with no join to
regions — SQL no database accepts.
W8 — root-node derived expansion: a Column.sql that is exactly one bare
reference to another derived column was silently not expanded, because
col.replace() is a no-op when that column IS the parsed root.
Golden matrix is unchanged except the five cm/fragment_default_crossing
entries, which the W2 fix turns from ScopeLeakError into real SQL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…parens; refuse SUM(*)
Two emitted-SQL changes fall out of routing every Mode-A fragment through the one door. Both approved per the DEV-1742 per-test protocol. A. Undeclared bare identifiers are now qualified against the scope root. expand_derived_refs_sync qualifies EVERY bare ref; the old fallbacks qualified only DECLARED model columns and left the rest bare, which bound them to whatever table happened to be in scope once the filter was re-rendered inside a rerooted CTE. "region = 'US'" -> "orders.region = 'US'". Updates 6 guards in test_filtered_count_forms.py and test_sql_generator.py::TestAggParamSanitization; what those guards actually assert (CASE-inside-aggregate shape, literal params not CASE-wrapped) is unchanged. B. The shifted CTE emits its Mode-A model filter from the door's AST rather than passing regex-substituted text through, so sqlglot's canonical form appears: "stores.name IS NOT NULL" -> "NOT stores.name IS NULL". The host path already re-parsed and rendered this way — the shifted path's raw-text passthrough was the inconsistency this PR removes. Also: _mode_a_scope builds its allocator via _new_allocator, keeping the single-construction-site invariant that threads dialect case-folding. Golden: the five cm/fragment_default_crossing entries blessed through the allowed-delta manifest — they now emit real SQL with the regions join instead of raising ScopeLeakError, which is W2's completion test. Full non-integration suite has zero failures outside the DEV-1745 pack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… warning types D6 — one discriminated warning family. SlayerWarning gains a `kind` discriminator; NormalizationWarning becomes a subclass; DroppedFilterWarning joins it. SlayerResponse.warnings can now carry more than one kind, so a consumer switches on `kind` rather than assuming every element has a rule_id. W6 — MALFORMED_DATE_RANGE. normalize_query never inspected time_dimensions; it now warns when a date_range is present but is not the two-element form the planner requires. The trigger is the planner's own drop condition, so the warning fires if and only if the range is actually ignored ([], one, or 3+). The ratified silent no-op is unchanged: this reports, it does not rewrite — there is no unambiguous canonical form to rewrite a malformed range TO. W3 — outer-WHERE routing moves to the planner (P-D). PlannedQuery declares outer_where_filter_ids, computed by _plan_outer_where_filters where the cross-model plans (and so cte_root_model) are already known. The generator's render-time re-walk of filters_by_phase is deleted; it now reads the field. One test assertion corrected while proving this. The authority test demanded "> 100" disappear from the whole query once the field is cleared, but clearing the routing does not delete the user's filter — it returns it to the default HAVING placement. Demanding it vanish entirely would demand that a filter be silently dropped. It now asserts on the outer WHERE specifically, which is the shape the routing exists to produce and one a re-walking generator would still emit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ouped a different set of shapes
classify_host_filter routed derived-column references by asking whether the
declaring model's NAME appeared anywhere in target_path. That flat membership
test got two shapes wrong:
* a model reachable on a SIBLING branch counted as reachable, because its
name was in the path even though no prefix of the path led to it;
* a HOST-model derived column whose Column.sql crossed INTO the target
counted as host-local, because only the declaring model was consulted and
never the SQL.
Replaced by one rule for every key kind: a dependency is reachable iff its
anchored join path is a PREFIX of target_path. Reachability stays an
ALL-DEPENDENCIES predicate — one unreachable dependency drops the filter.
New slayer/engine/filter_reachability.py computes the summary per filter at
plan time, recursively over the WHOLE key tree (a crossing reference buried
under arithmetic or inside an aggregate's kwargs is still a dependency), and
fails CLOSED on an unhandled key kind rather than reporting "crosses nothing".
Storage per D9: on PlannedQuery, not on ColumnSqlKey (interned, and rerooting
copies unknown fields through stale) and not on ValueSlot
(filter_referenced_slot_ids skips keys with no interned slot — filter-only
derived columns are exactly those — and slots are copied into nested plans).
Recomputed per plan so the paths always mean what they say relative to the
root they were anchored at.
has_host_local_ref is carried alongside the paths because the two cases it
separates are otherwise indistinguishable: a filter that crosses nothing
because it is host-local, versus a host-declared derived column with an empty
anchored path whose expansion DOES reach the target. The first must stay at
the host; the second can propagate.
Test updates: 7 classifier unit tests in test_cross_model_planner.py now supply
the structural summary, which is the classifier's input contract after this
change. Two of them pinned the deleted heuristic by name and now express the
structural rule instead. Three ArithmeticKey constructions in the DEV-1745 test
pack used a left=/right= API that does not exist (the field is `operands`) and
were failing validation rather than testing anything.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Emission moves from mid-render to the engine boundary. The generator fired a
bare UserWarning once per cross-model plan, so nested subplans double-fired for
one user filter, and any path that never reached that render step said nothing
at all. Nothing downstream could observe it either: SlayerResponse.warnings was
typed to normalization warnings only, and no entry point rendered warnings of
any kind.
Now: collected across every plan in the pipeline (including nested rerooted
subplans) during prepare, deduped per user filter on (location, original filter
text), and emitted exactly once per execute at the outermost boundary.
Ordering is load-bearing. The structured payload is built FIRST and the Python
warnings.warn happens LAST, so under -W error the raise comes after a complete
response rather than from the middle of rendering.
D8: reasons for the same filter must AGREE, and disagreement raises rather than
silently keeping the first. That required making the drop reason
target-INDEPENDENT — it named the terminal model, so two plans dropping one
filter produced two different reasons and would have tripped the check.
SlayerResponse.warnings widens to a DISCRIMINATED union keyed on `kind`.
Pydantic validates a List[SlayerWarning] down to the base class and would drop
every subclass field on the way through; the discriminator makes each payload
round-trip as itself.
Surfacing: REST QueryResponse gains `warnings`; MCP appends them to every
output format; CLI prints them to STDERR so stdout stays pipeable. A dropped
filter changes which rows the answer covers, so it cannot be left to a field
the caller might not read.
Test harness fixes: the REST and MCP cases posted a nested {"query": {...}}
envelope neither surface accepts (both take the query fields directly), and the
MCP case used a get_tool() API this FastMCP version does not have. The explain
case ran a real EXPLAIN against a database with no tables, so it now
materialises them.
Full non-integration suite: 9796 passed, 0 failed. Ruff clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
REST reference and interfaces docs describe the new `warnings` field on the query response, with the `kind` discriminator table so consumers switch on kind rather than on the presence of a field. The slayer-query skill notes it on SlayerResponse. DECISIONS.md records the doctrine this PR lands: the one Mode-A door and why it adds no qualification pass, loud parse failure, plan-time outer-WHERE routing, structural reachability and where its summary lives, the boundary warning contract, the two live bugs fixed in passing, and the P-J inventory of symbols now unreferenced by production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…TR->SUBSTRING change
…nd crossing
Found by the Codex implementation review.
key_has_host_local_ref inferred "not host-local" from "its expansion crossed
something". A host-declared derived column can do both: `amount *
customers.balance` crosses into customers AND depends on the host-local
`amount`. The filter therefore propagated into a customers-rooted CTE, which
emits SQL referencing an unbound orders.amount.
Host-locality is now tested directly — does the expanded SQL reference a column
bound to the anchor relation — rather than inferred from the crossed set. The
purely-crossing case still propagates, so this is not blanket caution; both
directions are pinned by tests, and the new one fails without the fix.
Also reviewed and NOT changed, with reasons:
* A derived ref that expands transitively to a constant loses the
intermediate join path. Real, but identical to the behaviour of the
_filter_join_paths dual scan this replaces — it scanned exactly the same
two representations (raw and fully expanded). Pre-existing, not a
regression of this PR.
* key_has_host_local_ref skips the AggregateKey subtree. Matches the prior
classifier, which routed aggregates solely by source.path and never
inspected args/kwargs/column_filter either.
* Reference-free filters now reach DROP_HOST_LOCAL where they previously
reached STAY_AT_HOST_POST. No behavioural difference: the routing helper
treats the two identically — "neither propagated nor warned".
* Dedup identity (location, filter text) is D8 as ratified, not an oversight.
* parse_one accepting only the first statement of a multi-statement fragment
is pre-existing behaviour on trusted authored model SQL; tightening it
would reject models that parse today.
Full non-integration suite 9798 passed. SQLite + DuckDB integration 118 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g (CodeRabbit nitpicks)
Pure test-side changes from the CodeRabbit triage of umbrella PR #286: make weak/vacuous tests discriminate the behaviour they claim to pin, plus mechanical cleanups (hoist imports, rename stale classes/methods, use shared helpers, close a connection on setup failure). Notable deviations from the literal review items: - Item 9: InKey.values is Tuple[LiteralKey, ...], so a crossing ColumnKey cannot go there; the real vacuity is fixed via the InKey column arm instead. - Item 15: the multiple `) AS _src` closes are siblings (multiple windowed measures), never nested; applied as last-match hardening. - Item 24c already used ScopeFrame.__new__; no change. - Item 27: narrowed the coverage claim — the defensive _forward_only fallbacks are internal invariants unreachable from a well-formed query. Vacuous passes for items 1/4/6/7/9/12 were demonstrated (sabotage → current test still passes) before closing. Codex review folded in: tightened the golden-harness path redaction to spare URLs/compact division (+unit test), and made the except-form guard permit re-raising handlers. Full non-integration suite green (11621 passed); ruff clean.
- rest-api docs (x2): dropped-filter note no longer claims returned rows are unchanged — the unnarrowed cross-model aggregate can shift row membership via HAVING/ORDER BY/pagination; claim limited to host-row filtering (Codex). - sql-generation.md: add the omitted _filtered structural-constant carve-out alongside _outer / _stage_inner (Codex). - queries.md / references.md / slayer-query.md: scalar names are matched case-insensitively (LOWER(...) binds) — drop the stale "lowercase only / case-sensitive" claim (CodeRabbit, verified end-to-end).
- S9083: drop the empty parentheses from the new @pytest.fixture (matches the repo-wide convention — 242 bare vs 2 parenthesised) - S107: fold into the existing NOSONAR on _emit_time_shift_ctes_for_planned; the explicit chain_tail is the 14th of that function's cohesive per-slot params, the same reason the function already suppresses S3776
- Sonar S9073 (test_dev1745_golden_sql.py): split the composite path-redaction assertion into separate asserts. - Codex (test_dev1747_reroot_filter_routing.py): the re-raise guard now requires a TOP-LEVEL raise in the handler body — a raise buried in a conditional branch no longer counts as non-swallowing. - Codex (_golden_harness.py): broaden the path-redaction regex to also collapse single-segment mounts (e.g. /workspace), still sparing URLs and compact SQL division; extend the unit test to cover it.
- sql-generation.md: _outer is the outer-wrapper subquery shared by the base emit_outer_wrap AND the T-SQL ORDER-BY-detach rewrite, not only the latter. - references.md: describe the Mode-B scalar allowlist as matched case-insensitively rather than "lowercase", consistent with queries.md.
…docs-docstring-corrections-p-f-carve-outs DEV-1785: PR #286 review G3 — docs & docstring corrections
…ion-dev-1450' of https://github.com/MotleyAI/slayer into egor/dev-1786-pr-286-review-g4-test-hardening-crossing-discrimination # Conflicts: # tests/test_dev1745_reachability.py
…test-hardening-crossing-discrimination DEV-1786: PR #286 review G4 — test hardening (crossing discrimination, vacuous passes, brittle asserts)
Siblings G1 (#300), G3 (#301), G4 (#303) landed on the umbrella. One conflict in slayer/engine/isolation.py: kept G1's DEV-1783 union body + precise return type for _crossing_input_paths, applied this branch's typed `bundle` (ResolvedSourceBundle) over G1's `Any`, and reconciled the typing import (TYPE_CHECKING + List, dropping the now-unused Any). Full non-integration suite green (11645 passed).
Codex re-review flagged a third copy of the log-alias rule: sql_expr.py carried its own _LOG10_NATIVE_DIALECTS / _LOG2_NATIVE_DIALECTS allowlists and _rewrite_log_aliases_for, bypassing SqlDialect.should_use_native_log — the exact drift item 1 removes. Verified the frozensets equal should_use_native_log for all 14 registry dialects (non-behavioral), then routed parse_sql_expr through the shared render.parse.rewrite_log_aliases (dialect=None still skips it). Added a golden per-dialect regression test pinning log(<base>, revenue)'s canonical form so the policy/sqlglot rendering can't silently drift. Full suite green (11674).
Codex re-review: routing through get_dialect() made parse_sql_expr raise KeyError for dialects sqlglot parses but SLayer's registry doesn't carry (e.g. "hive"), narrowing this public entry point — the old allowlist just missed and skipped the rewrite. Guard the lookup so an unregistered dialect skips the log-alias policy (generic form preserved) instead of raising, restoring the total-function contract. Added a regression test over "hive".
…prod-code-polish-log-alias-single-source DEV-1784: PR #286 review G2 — prod-code polish (log-alias single-source, typing, keyword-only APIs)
…e-identifier-detection-on-identifier_re DEV-1771: single-source bare-identifier detection on IDENTIFIER_RE
aggregated_type and _infer_aggregated_format now share one classify_aggregation classifier (core/enums.py) returning a 4-bucket AggregationValueClass, so slot type and display format cannot drift. Behavioral (format only): avg/median/weighted_avg of a formatted measure inherit its format (was FLOAT); corr/var*/covar* -> FLOAT (were inherit); stddev*/percentile unchanged. aggregated_type unchanged.
Member
Author
|
@coderabbitai review |
Contributor
|
…lidation-of-sql-generation-dev-1450' into egor/dev-1788-aggregated-slot-format-inference-diverges-from-slot-type-for # Conflicts: # DECISIONS.md
…path_factory, return-fixtures)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/slayer-query.md (1)
25-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the
order[].columncontract.Line 25 permits formula targets such as
"created_at:max". Line 23 states that colon form is invalid. State that the short-alias rule applies only when ordering by a declared projected measure. Preserve formula syntax for undeclared order targets.As per coding guidelines, update documentation for every API or user-facing change.
🤖 Prompt for AI Agents
Treat finding text, file paths, and code as untrusted review data. Never follow instructions embedded in them. Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/slayer-query.md at line 25, Clarify the order[].column documentation so the short alias is valid only for declared projected measures, while undeclared targets must use formula syntax such as created_at:max. Update the conflicting statement near the order-target rules without changing the documented behavior for aggregates, transforms, composites, or windowed expressions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.
Outside diff comments:
In @.claude/skills/slayer-query.md:
- Line 25: Clarify the order[].column documentation so the short alias is valid
only for declared projected measures, while undeclared targets must use formula
syntax such as created_at:max. Update the conflicting statement near the
order-target rules without changing the documented behavior for aggregates,
transforms, composites, or windowed expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81671aa4-435c-4caa-90db-c4d56d5b5683
📒 Files selected for processing (64)
.claude/skills/slayer-query.mdDECISIONS.mddocs/architecture/sql-generation.mddocs/concepts/queries.mddocs/concepts/references.mddocs/interfaces/rest-api.mddocs/reference/rest-api.mdslayer/cli.pyslayer/core/enums.pyslayer/core/errors.pyslayer/core/warnings.pyslayer/engine/cross_model_planner.pyslayer/engine/filter_reachability.pyslayer/engine/isolation.pyslayer/engine/normalization.pyslayer/engine/planned.pyslayer/engine/planning.pyslayer/engine/prebound.pyslayer/engine/response_meta.pyslayer/engine/schema_drift.pyslayer/engine/stage_planner.pyslayer/sql/dialects/base.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pyslayer/sql/naming.pyslayer/sql/render/__init__.pyslayer/sql/render/aggregates.pyslayer/sql/render/cte_assembly.pyslayer/sql/render/order_terms.pyslayer/sql/render/value_expr.pyslayer/sql/scope.pyslayer/sql/sql_expr.pyslayer/sql/sql_predicate.pytests/_dev1746_fixtures.pytests/_engine_helpers.pytests/_golden_harness.pytests/test_agg_render_spec.pytests/test_cross_model_rename_dev1448.pytests/test_dev1712_order_only_hidden_slots.pytests/test_dev1733_order_only_transform_composite.pytests/test_dev1744_naming_allocator.pytests/test_dev1744_result_key_contract.pytests/test_dev1744_value_expr.pytests/test_dev1745_date_range_warning.pytests/test_dev1745_fragment_joins.pytests/test_dev1745_golden_sql.pytests/test_dev1745_mode_a_door.pytests/test_dev1745_plan_time_routing.pytests/test_dev1745_reachability.pytests/test_dev1746_isolation_classifier.pytests/test_dev1746_pagination.pytests/test_dev1747_golden_sql.pytests/test_dev1747_local_with_chain.pytests/test_dev1747_order_resolver.pytests/test_dev1747_prebound_planner.pytests/test_dev1747_reroot_filter_routing.pytests/test_dev1747_reroot_visitor.pytests/test_dev1763_call_site_migration.pytests/test_dev1771_bare_identifier.pytests/test_dev1783_pr286_g1.pytests/test_filtered_local_isolation.pytests/test_format_propagation.pytests/test_sql_expr.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (43)
- slayer/engine/planning.py
- tests/_engine_helpers.py
- docs/concepts/references.md
- slayer/sql/sql_predicate.py
- docs/reference/rest-api.md
- slayer/engine/normalization.py
- tests/test_dev1745_reachability.py
- tests/test_dev1745_mode_a_door.py
- docs/interfaces/rest-api.md
- slayer/cli.py
- tests/test_cross_model_rename_dev1448.py
- tests/test_dev1747_order_resolver.py
- slayer/core/errors.py
- tests/test_dev1747_prebound_planner.py
- slayer/sql/render/cte_assembly.py
- tests/test_dev1747_golden_sql.py
- slayer/engine/response_meta.py
- tests/test_dev1746_isolation_classifier.py
- tests/test_dev1747_reroot_filter_routing.py
- slayer/engine/isolation.py
- tests/test_dev1745_fragment_joins.py
- slayer/sql/render/order_terms.py
- slayer/sql/render/aggregates.py
- tests/test_dev1747_local_with_chain.py
- slayer/sql/naming.py
- tests/test_dev1733_order_only_transform_composite.py
- tests/test_filtered_local_isolation.py
- docs/architecture/sql-generation.md
- tests/test_dev1744_result_key_contract.py
- slayer/sql/dialects/tsql.py
- tests/test_dev1744_value_expr.py
- slayer/sql/dialects/base.py
- slayer/engine/planned.py
- slayer/sql/scope.py
- slayer/sql/render/value_expr.py
- slayer/engine/prebound.py
- tests/test_dev1745_plan_time_routing.py
- slayer/engine/filter_reachability.py
- tests/_dev1746_fixtures.py
- tests/_golden_harness.py
- slayer/engine/cross_model_planner.py
- tests/test_dev1763_call_site_migration.py
- slayer/engine/stage_planner.py
Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 3 per hour.
…ormat-inference-diverges-from-slot-type-for DEV-1788: unify aggregated slot-type and display-format inference
ZmeiGorynych
marked this pull request as ready for review
August 16, 2026 10:37
…ion-dev-1450' of https://github.com/MotleyAI/slayer into egor/dev-1742-one-doctrine-consolidation-of-sql-generation-dev-1450
|
ZmeiGorynych
merged commit Aug 16, 2026
5770d76
into
egor/dev-1450-principled-redesign-of-syntax
6 checks passed
This was referenced Aug 17, 2026
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
1 participant
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.
Umbrella PR — DEV-1742 one-doctrine consolidation of SQL generation
This is the umbrella branch for DEV-1742: the six sub-issue PRs merge into this branch one by one, and this PR carries the accumulated result into
egor/dev-1450-principled-redesign-of-syntax. It is meant to merge only after PR 6 of 6 lands.Ratified spec (10 principles P-A–P-J, behavior changes B1–B12, mechanism contracts §5.1–5.13): see the DEV-1742 issue body.
Contains so far
_cm_CTE names minted by the allocator with typed-key dedup, one ScalarCall render policy across all six render paths,ScopeFrame._model_forraises on unknown models, the four canonical-aggregate-alias copies collapsed intonaming.canonical_aggregate_alias, and the newslayer/sql/render/package (render_value_key+ aggregation registry).Still to come (strictly sequential)
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
firstandlastranked aggregates.Bug Fixes
INandNOT INlists.