S2a: query-backed models as a first-class source mode (#58)#67
Merged
Conversation
Make ``source_queries`` a peer of ``sql_table`` / ``sql`` on ``SlayerModel``
and surface the query-backed source mode end-to-end across engine, MCP,
REST, and CLI.
Engine + data model
- New SlayerModel fields: ``query_variables`` (defaults for ``{var}``
placeholders) and ``backing_query_sql`` (engine-managed cached rendered
SQL). ``source_queries`` is typed ``List[SlayerQuery]`` via a before-
validator that auto-parses dicts.
- Validators: source-mode exclusivity (exactly one source); empty
``source_queries=[]`` rejected; non-final stages must have a ``name``;
no duplicate stage names.
- ``_query_as_model`` / ``_resolve_model`` / ``_resolve_model_inner``
thread ``outer_vars``, ``runtime_kwarg``, ``dry_run_placeholders``
through nested resolution. Variable precedence: ``runtime kwarg >
stage > outer query > model defaults``. Save-time validation
substitutes literal ``'0'`` for unresolved placeholders.
- ``engine.execute(str, variables=...)`` runs a query-backed model by
name. ``engine.save_model(model)`` is the engine-side save helper that
rejects user-supplied ``columns`` / ``backing_query_sql`` for query-
backed models, runs save-time validation, and refreshes the cache.
- Cache refreshes on every execute path (real, dry-run, explain) with
write-if-changed semantics.
Surfaces
- MCP: ``inspect_model`` adds a ``backing_query`` section + ``source_type``
+ (with ``show_sql``) ``backing_query_sql``; ``models_summary`` reports
``source_type``; ``create_model`` accepts ``variables=`` and a list-of-
stages ``query=`` payload; ``query`` tool dispatches to ``execute(str)``
for query-backed model names with no overrides; ``edit_model`` accepts
``source_queries`` / ``query_variables`` and routes through
``engine.save_model``.
- REST: ``POST/PUT /models`` route through ``engine.save_model``;
``POST /query`` accepts ``{"name": "<model>", "variables": {...}}``
for run-by-name and ``variables=`` on the normal body shape.
- CLI: ``slayer query`` accepts a model name (heuristic: doesn't start
with ``{`` / ``[`` / ``@``); new ``--variables KEY=VALUE`` (repeatable)
and ``--variables-json`` flags; ``slayer models create`` routes
through ``engine.save_model``.
Tests
- 18 new model-validator tests in ``tests/test_models.py``.
- New ``tests/test_query_backed_models.py`` (20 tests) covering
``execute(str)``, variable precedence, cache refresh, save-time
placeholder fill, ModelExtension regressions over query-backed and
named-query stages.
- New tests in ``tests/test_mcp_server.py`` and ``tests/test_api_server.py``
for the new MCP / REST surface.
- ``tests/test_migrations.py`` updated to add ``sql_table`` fixtures and
switch ``source_queries`` assertions to attribute access (entries are
now parsed into ``SlayerQuery`` instances).
Docs / skills
- ``CLAUDE.md``, ``docs/concepts/models.md``, ``docs/concepts/queries.md``,
``docs/interfaces/{cli,mcp,rest-api}.md``, ``.claude/skills/slayer-{overview,
models,query}.md`` updated for source modes, ``query_variables``,
run-by-name, variable precedence, and the cache policy.
Out of scope per the design pass on #58: dbt-import emission, standalone
``slayer models inspect`` CLI, CLI YAML model files for ``slayer query``.
Variable-handling regime tracked separately in #65.
Final state: 1102 unit tests + 59 SQLite integration tests pass; ruff
clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR makes query-backed models a first-class SlayerModel source mode ( ChangesQuery-backed models & execution
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Engine
participant Storage
participant SQL
rect rgba(100, 149, 237, 0.5)
note over User,SQL: Save query-backed model (create_model_from_query)
User->>CLI: slayer models create model.yaml (includes source_queries, variables)
CLI->>Engine: save_model(SlayerModel{source_queries, query_variables})
Engine->>Engine: dry-run render with placeholder fill (missing vars -> '0')
Engine->>SQL: render probe query to derive columns/backing_query_sql
Engine->>Storage: save(model with backing_query_sql, columns, query_variables)
Storage-->>CLI: persisted
end
rect rgba(144, 238, 144, 0.5)
note over User,SQL: Execute by name with runtime variables
User->>CLI: slayer query my_model --variables foo=bar
CLI->>Engine: execute("my_model", variables={foo: "bar"})
Engine->>Storage: load("my_model") -> SlayerModel(source_queries, query_variables)
Engine->>Engine: merge variables (runtime > stage > model.query_variables)
Engine->>SQL: execute rendered final-stage SQL with merged variables
SQL-->>Engine: rows
Engine-->>CLI: results
end
rect rgba(255, 192, 203, 0.5)
note over User,Engine: Inspect model (introspection)
User->>CLI: slayer models inspect my_model --show-sql
CLI->>Engine: inspect_model("my_model", show_sql=True)
Engine->>Storage: load("my_model")
Engine->>Engine: build backing_query info (variables, required_variables, stages)
Engine->>SQL: optionally render SQL with placeholder fill for show_sql
Engine-->>CLI: backing-query report + SQL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
slayer/engine/query_engine.py (2)
567-609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve inline query-backed models before returning them.
The
SlayerModeland validated-dict branches return the model as-is, sosource_model=<SlayerModel source_queries=[...]>never goes through_query_as_model(). Those inline models still do not have an executablesql/sql_table, so enrichment/SQL generation will fail once they are used directly. Inline query-backed models need the same expansion path as stored ones.Suggested fix
elif isinstance(query_model, SlayerModel): - return query_model + if query_model.source_queries: + stages = list(query_model.source_queries) + merged_outer = {**query_model.query_variables, **(outer_vars or {})} + return await self._query_as_model( + inner_query=stages[-1], + named_queries={q.name: q for q in stages[:-1] if q.name}, + override_name=query_model.name, + _resolving=_resolving, + outer_vars=merged_outer, + runtime_kwarg=runtime_kwarg, + dry_run_placeholders=dry_run_placeholders, + ) + return query_model @@ else: model = SlayerModel.model_validate(query_model) - return model + return await self._resolve_query_model( + query_model=model, + named_queries=named_queries, + _resolving=_resolving, + outer_vars=outer_vars, + runtime_kwarg=runtime_kwarg, + dry_run_placeholders=dry_run_placeholders, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/engine/query_engine.py` around lines 567 - 609, The code returns inline SlayerModel instances (and dict-validated SlayerModel) directly from _resolve_model, so models with source_queries never get expanded via _query_as_model/_resolve_query_model and thus lack sql/sql_table; update the SlayerModel branch and the dict branch that validates to SlayerModel to detect query-backed models (e.g., check model.source_name or model.source_queries) and route them through the same expansion path used for stored models—call self._query_as_model(...) or await self._resolve_query_model(...) with the same parameters (named_queries, _resolving, outer_vars, runtime_kwarg, dry_run_placeholders) instead of returning the raw SlayerModel, ensuring ModelMeasure/ModelJoin/ModelExtension handling remains unchanged.
815-832:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftResolve join targets through the same query-backed path.
This helper still fetches non-named targets with
storage.get_model(), so a saved query-backed join target never gets expanded into a virtual model and falls through with neithersql_tablenorsql. The named-query branch also dropsruntime_kwarganddry_run_placeholders, so joined stages can ignore runtime precedence and miss save-time placeholder filling. Route both branches through_resolve_model(...)with the active variable context instead of bypassing it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/engine/query_engine.py` around lines 815 - 832, The _resolve_join_target helper currently bypasses the normal model resolution by calling storage.get_model for non-named targets and by using _query_as_model for named queries (losing runtime_kwarg and dry_run_placeholders); change it to route both named and non-named targets through _resolve_model using the active variable context (query.variables) and passing through runtime_kwargs and dry_run_placeholders so saved query-backed models are expanded into virtual models and runtime precedence / placeholder filling is preserved; update references inside _resolve_join_target to call _resolve_model(...) instead of storage.get_model(...) or _query_as_model(...) and ensure it returns the same tuple shape (sql_table/sql or sql string, model) as before.
🧹 Nitpick comments (1)
tests/test_mcp_server.py (1)
1324-1348: ⚡ Quick winMake this source-mode switch test deterministic.
This assertion passes on almost any validation failure and never verifies the stored model state, so regressions in the actual switch-to-query-backed flow can slip through unnoticed. Use a non-cyclic backing query here and assert that the saved model has
source_queriesset andsql_tablecleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_server.py` around lines 1324 - 1348, The test is non-deterministic because it uses a self-referencing (cyclic) source_query and only checks that some error text appears; instead create a non-cyclic backing model and assert the persisted model state after the edit: before calling edit_model save a separate backing model (e.g., name "orders_source") via storage.save_model(SlayerModel(...)), call _call(... "edit_model" ...) with source_queries referencing "orders_source", then load the saved model from storage (use the storage load/get API used elsewhere in tests) and assert that the model's source_queries is set and that sql_table has been cleared/removed (e.g., None or empty) to verify the switch-to-query-backed flow; keep references to test_edit_set_source_queries_makes_model_query_backed, storage.save_model, _call, SlayerModel, source_queries, and sql_table so you can locate and update the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/slayer-models.md:
- Around line 57-74: The docs file mixes v2 source-mode terminology with legacy
v1 examples which will confuse users; update the
`.claude/skills/slayer-models.md` content to be internally consistent by
removing or converting legacy v1 references (dimensions/measures, row-level
measures) to the v2 model vocabulary (SlayerModel source modes like `sql_table`,
`sql`, `source_queries`) and ensure all examples and YAML snippets align with
the new API (e.g., `create_model_from_query`, `model.source_queries`,
`model.query_variables`, `model.columns`, `model.backing_query_sql`, and
`source_model` usage); also add a short migration note explaining that `columns`
and `backing_query_sql` are engine-managed and cannot be supplied when saving
query-backed models and update any ingestion bullets/YAML examples to reflect
query-backed model shapes and variable precedence semantics.
In @.claude/skills/slayer-query.md:
- Around line 71-76: The examples show synchronous calls to the async method
SlayerQueryEngine.execute which can cause coroutine misuse; update the snippets
to use await when calling SlayerQueryEngine.execute (e.g., await
engine.execute(...)) or explicitly show the synchronous alternative
SlayerQueryEngine.execute_sync(...) in sync examples, and adjust surrounding
text to indicate the async/sync variant being demonstrated.
In `@docs/concepts/models.md`:
- Around line 290-339: Update the canonical "Model Fields Reference" section to
include the three new query-backed fields: source_queries (list of SlayerQuery
stages, describe required stage name for multi-stage queries), query_variables
(dict of model default values, explain precedence and that unresolved
placeholders default to '0' at save-time validation), and backing_query_sql
(string, read-only/engine-managed cached rendered SQL); also mark model.columns
as a cached/engine-managed discoverability snapshot, state that user-supplied
columns or backing_query_sql are rejected at save with a clear error, and ensure
descriptions/types and read-only/write behavior match the earlier "Two ways to
use a saved query" / "What gets cached" text.
- Around line 276-287: The example uses a Python constructor (SlayerQuery) which
reduces portability; replace the SlayerQuery(...) argument passed to
engine.create_model_from_query with a plain dict/JSON object using the same keys
(e.g., "source_model", "measures", "dimensions", "time_dimensions") and keep
"variables" as a dict literal; update the call site
(engine.create_model_from_query) to pass that dict for the query parameter and
preserve name/description/variables values.
In `@docs/interfaces/rest-api.md`:
- Around line 48-49: Update the "Run-by-name" sentence to clarify that while
query-defining fields are disallowed, the request body may still include
`variables` plus the optional flags `dry_run` and `explain`; change the phrasing
in docs/interfaces/rest-api.md for the "Run-by-name" entry and make the
analogous edit where the same rule is described in docs/concepts/queries.md so
both say "provide `name` and (optionally) `variables`; `dry_run` and `explain`
are also permitted — only query-defining fields (e.g., the actual query/prompt)
are disallowed."
In `@slayer/api/server.py`:
- Around line 81-105: The run-by-name branch currently ignores the
dry_run/explain flags because it always calls engine.execute(request.name, ...);
change it to respect those flags by computing dry_or_explain from
request.dry_run/request.explain before invoking the engine and then call
engine.execute with those flags (e.g. engine.execute(request.name,
variables=..., dry_run=request.dry_run, explain=request.explain)) or, if your
engine API exposes a separate method for plan-only/explain, call that method
when dry_run or explain is true; update the code around request.name, the
existing dry_or_explain variable, and the engine.execute call so named runs do
not perform a full execution when dry_run or explain are set.
In `@slayer/cli.py`:
- Around line 410-414: The block that parses CLI input via
_json.loads(args.variables_json) can raise a JSON decoding exception and
currently emits a traceback; catch the JSON parsing error
(ValueError/JSONDecodeError) around the call in the has_json branch and instead
raise SystemExit with a clear user-facing message like "--variables-json
contains invalid JSON: <error message>"; keep the existing check that parsed is
a dict and raise SystemExit("--variables-json must decode to a JSON object.") if
not.
- Around line 461-477: When handling args.dry_run or args.explain for a named
model (query_input) you currently synthesize a SlayerQuery wrapper
unconditionally, which incorrectly allows table-backed models; before building
the wrapper (before calling SlayerQuery.model_validate and engine.execute_sync),
resolve the named model and verify it is query-backed (e.g., call whatever
model-resolve/metadata API exists on the engine such as engine.get_model /
engine.resolve_model or an is_query_backed helper) and if the model is not
query-backed raise the same validation/error path as plain named execution; only
synthesize the SlayerQuery and call SlayerQuery.model_validate +
engine.execute_sync when that check passes.
In `@slayer/core/models.py`:
- Around line 223-245: The validator function _coerce_source_queries currently
raises TypeError for non-list inputs and invalid list items; change those raises
to ValueError so Pydantic v2 wraps them into validation errors: replace the
TypeError raised when v is not a list and the TypeError raised for invalid list
elements inside the loop (including the index-based message) with ValueError,
keeping the same messages and behavior; update references in this function only
(function name: _coerce_source_queries, and uses of SlayerQuery.model_validate)
so other validators remain consistent.
In `@slayer/engine/query_engine.py`:
- Around line 674-690: The current flow renders a virtual model using the live
runtime_kwarg and then calls _refresh_cache_after_resolution(model, virtual),
which causes backing_query_sql to be persisted with request-specific variables;
change this so persisted SQL stays canonical: when calling _query_as_model here
(inside the if model.source_queries block) avoid passing caller-specific runtime
variables into the rendering used for cache refresh—either call _query_as_model
twice (one full render for execution with runtime_kwarg, and a second render
with runtime_kwarg=None or dry placeholders only) or pass a flag into
_refresh_cache_after_resolution to skip writing backing_query_sql when
runtime_kwarg is present; update _refresh_cache_after_resolution (or its caller)
so it only persists backing_query_sql derived from model defaults/placeholder
fill and not from request/runtime_kwarg values.
In `@slayer/mcp/server.py`:
- Around line 551-557: The required_variables computation omits stage-scoped
defaults, causing placeholders satisfied by a stage's own variables to be
flagged as required; when iterating model.source_queries (where you call
extract_placeholder_names(q) and append q.model_dump(...)), merge each stage's
q.variables keys into the set of defaulted variables before computing required =
sorted(all_placeholders - set(model.query_variables.keys())), e.g., accumulate
keys from q.variables into the default set (alongside model.query_variables) so
required_variables reflects both global query_variables and per-stage
q.variables.
- Around line 778-785: The shortcut branch using no_overrides + await
storage.get_model(source_model) bypasses the normal SlayerQuery path and calls
engine.execute(source_model, variables=variables) without propagating dry_run or
explain, so plan-only requests may execute the query; modify this branch to
either construct and dispatch a SlayerQuery object (including dry_run and
explain) instead of calling engine.execute with just source_model, or extend the
engine.execute overload to accept and forward dry_run and explain (and
variables) — specifically update the code around no_overrides /
storage.get_model / target.source_queries to call the same code path used for
named queries (create SlayerQuery with source_model, variables, dry_run, explain
and pass it to the engine) so the planning flags are honored and result.sql is
returned without executing the backing query.
- Around line 1615-1621: The code currently wipes validated.columns when
validated.source_queries is true (in the branch using validated.model_copy),
which silently discards client column edits; instead, detect when a query-backed
save is being attempted with columns provided and reject it by raising an
explicit error (e.g., HTTPException/ValueError) referencing the same check
(validated.source_queries) rather than blanking via validated.model_copy, or if
you prefer to allow the request to succeed, strip columns before building the
recorded changes so no "created/updated column ..." entries are appended; update
the branch around validated.source_queries and the validated.model_copy call to
either raise an error when validated.columns is non-empty or remove columns from
the change-tracking payload before persisting.
---
Outside diff comments:
In `@slayer/engine/query_engine.py`:
- Around line 567-609: The code returns inline SlayerModel instances (and
dict-validated SlayerModel) directly from _resolve_model, so models with
source_queries never get expanded via _query_as_model/_resolve_query_model and
thus lack sql/sql_table; update the SlayerModel branch and the dict branch that
validates to SlayerModel to detect query-backed models (e.g., check
model.source_name or model.source_queries) and route them through the same
expansion path used for stored models—call self._query_as_model(...) or await
self._resolve_query_model(...) with the same parameters (named_queries,
_resolving, outer_vars, runtime_kwarg, dry_run_placeholders) instead of
returning the raw SlayerModel, ensuring ModelMeasure/ModelJoin/ModelExtension
handling remains unchanged.
- Around line 815-832: The _resolve_join_target helper currently bypasses the
normal model resolution by calling storage.get_model for non-named targets and
by using _query_as_model for named queries (losing runtime_kwarg and
dry_run_placeholders); change it to route both named and non-named targets
through _resolve_model using the active variable context (query.variables) and
passing through runtime_kwargs and dry_run_placeholders so saved query-backed
models are expanded into virtual models and runtime precedence / placeholder
filling is preserved; update references inside _resolve_join_target to call
_resolve_model(...) instead of storage.get_model(...) or _query_as_model(...)
and ensure it returns the same tuple shape (sql_table/sql or sql string, model)
as before.
---
Nitpick comments:
In `@tests/test_mcp_server.py`:
- Around line 1324-1348: The test is non-deterministic because it uses a
self-referencing (cyclic) source_query and only checks that some error text
appears; instead create a non-cyclic backing model and assert the persisted
model state after the edit: before calling edit_model save a separate backing
model (e.g., name "orders_source") via storage.save_model(SlayerModel(...)),
call _call(... "edit_model" ...) with source_queries referencing
"orders_source", then load the saved model from storage (use the storage
load/get API used elsewhere in tests) and assert that the model's source_queries
is set and that sql_table has been cleared/removed (e.g., None or empty) to
verify the switch-to-query-backed flow; keep references to
test_edit_set_source_queries_makes_model_query_backed, storage.save_model,
_call, SlayerModel, source_queries, and sql_table so you can locate and update
the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad6187fb-91f0-4033-95dc-ba4f113916e9
📒 Files selected for processing (20)
.claude/skills/slayer-models.md.claude/skills/slayer-overview.md.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/models.mddocs/concepts/queries.mddocs/interfaces/cli.mddocs/interfaces/mcp.mddocs/interfaces/rest-api.mdslayer/api/server.pyslayer/cli.pyslayer/core/models.pyslayer/core/query.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/test_api_server.pytests/test_mcp_server.pytests/test_migrations.pytests/test_models.pytests/test_query_backed_models.py
Member
Author
Review fix-up summary (responding to review at #67 (review) + SonarCloud quality gate)13 inline CodeRabbit comments answered on their threads. This top-level reply covers the 2 outside-diff items, the 1 nitpick, and every SonarQube finding. CodeRabbit — outside-diff
CodeRabbit — nitpick
SonarQube — fixed (issues should auto-close on next scan)
SonarQube — accepted with rationale (intentionally not refactored / framework-mandated)These I'll mark as Accept / False Positive in SonarCloud and have left a
All 1120 unit tests + 121 integration tests (SQLite 59 / Postgres 37 / DuckDB 25) pass. Ruff clean. |
Eight logical batches covering all 13 actionable CodeRabbit comments + 2
outside-diff items + 1 nitpick + the SonarQube quality-gate findings.
Correctness:
- Run-by-name plumbing now honors dry_run/explain across REST, MCP, CLI;
engine.execute(str, ...) and _execute_by_name take the new kwargs and
the CLI drops its SlayerQuery synthesis trick so the "must be query-
backed" guard fires uniformly (Batch 1).
- Inline SlayerModel(source_queries=[...]) is now expanded via a new
_expand_query_backed_model helper from both _resolve_query_model and
_resolve_model_inner; _resolve_join_target uses the cached
backing_query_sql when present and falls back to a one-shot expansion
to avoid enrich-time recursion (Batch 2).
- backing_query_sql cache stays canonical: when runtime variables are
present, a second placeholder-fill render produces the cache value so
per-request data never persists. edit_model rejects user-supplied
columns on query-backed models with an explicit error; required_variables
now folds per-stage q.variables into the defaulted set (Batch 3).
- _coerce_source_queries raises ValueError instead of TypeError so
Pydantic v2 wraps it into a ValidationError (Batch 4).
- CLI --variables-json catches JSONDecodeError and exits with a clean
message (Batch 4).
SonarQube:
- _backing_query_markdown_section refactored into _render_field_value /
_render_stage_field_list / _render_source_model / _render_stage helpers
(S3776 cognitive complexity 59 -> well below 15) (Batch 6).
- log statements that include user-supplied model names sanitize CR/LF
before logging (S5145, Batch 7).
- /query, POST /models, PUT /models/{name} now declare responses={400}
for OpenAPI completeness (S8415, Batch 8).
- 7 deferred sites annotated with NOSONAR + per-site rationale (Batches
6, 7); marked accept/false-positive in SonarCloud.
Docs:
- slayer-models / slayer-query skills migrated from v1 dimensions/measures
to v2 source-mode + columns vocabulary; clarified async execute usage.
- docs/concepts/models.md gains source_queries/query_variables/
backing_query_sql in the Model Fields Reference; SlayerQuery() example
switched to the dict form (Batch 5).
- docs/concepts/queries.md and docs/interfaces/rest-api.md clarify that
run-by-name allows variables/dry_run/explain (Batch 1).
Tests: +14 new tests across the 8 batches. All 1120 unit tests pass;
SQLite (59), Postgres (37), DuckDB (25) integration suites green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
slayer/engine/query_engine.py (1)
616-635:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
backing_query_sqlcanonical when outer-query variables shadow model defaults.This still writes request-specific SQL into the persisted cache when values arrive through
outer_varsinstead ofruntime_kwarg. A query like{"source_model": "saved_qb", "variables": {"tenant": "A"}}will refreshbacking_query_sqlwith tenantA, causing the same inspect/export leak and cache churn as the earlier runtime-only bug. Please trigger the canonical second render whenever the effective outer vars differ frommodel.query_variables, not only whenruntime_kwargis set.Suggested fix
- if refresh_cache and not dry_run_placeholders: - # Render a canonical version for the cache when runtime variables - # were in play; otherwise the virtual already IS canonical. - if runtime_kwarg: + if refresh_cache and not dry_run_placeholders: + # Any caller-provided outer/runtime vars make `virtual.sql` + # request-specific. Persist only the canonical render based on + # model defaults + placeholder fill. + needs_canonical = bool(runtime_kwarg) or merged_outer != dict(model.query_variables) + if needs_canonical: try: canonical = await self._query_as_model( inner_query=stages[-1], named_queries=named_q, override_name=model.name, _resolving=set(), # fresh — different render outer_vars=dict(model.query_variables), runtime_kwarg=None, dry_run_placeholders=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/engine/query_engine.py` around lines 616 - 635, The cache-refresh path only triggers a canonical re-render when runtime_kwarg is set, but it must also run when incoming outer_vars shadow model defaults; update the conditional so that if refresh_cache and not dry_run_placeholders and (runtime_kwarg is truthy OR outer_vars is provided and outer_vars != model.query_variables) you perform the canonical render: call _query_as_model with inner_query=stages[-1], named_queries=named_q, override_name=model.name, _resolving=set(), outer_vars=dict(model.query_variables), runtime_kwarg=None, dry_run_placeholders=True (same as the runtime_kwarg branch), then pass the resulting canonical to _refresh_cache_after_resolution (otherwise use virtual) — i.e., detect shadowing via outer_vars != model.query_variables and treat it like the runtime_kwarg case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/engine/query_engine.py`:
- Around line 924-942: The join-target resolution currently always uses
target.backing_query_sql or falls back to _query_as_model with only
target.query_variables, which ignores the enclosing query's variables; change
the logic in the block handling storage-backed targets so that if the enclosing
query has variables (e.g., self.query.variables or the current query's
variables), you prefer a fresh expansion via _query_as_model (even if
backing_query_sql exists) and call _query_as_model with outer_vars equal to the
merged dict of target.query_variables updated by the enclosing query's variables
(caller variables should override target defaults), i.e., pass
outer_vars={**target.query_variables, **enclosing_query_variables} to
_query_as_model; keep using target.model_copy(update={"sql":
target.backing_query_sql}) only when there are no enclosing variables.
In `@tests/test_mcp_server.py`:
- Around line 367-375: The test currently guards the assertion with if
parsed.get("backing_query_sql") which hides regressions; change it to require
the field unconditionally: after calling result = await _call(mcp_server,
name="inspect_model", arguments={... "show_sql": True}), parse JSON into parsed
and assert that "backing_query_sql" is in parsed (i.e.
parsed.get("backing_query_sql") is not None) and that
parsed["backing_query_sql"].lower() contains "amount"; remove the conditional so
the test fails when the warmed cache field is missing.
---
Duplicate comments:
In `@slayer/engine/query_engine.py`:
- Around line 616-635: The cache-refresh path only triggers a canonical
re-render when runtime_kwarg is set, but it must also run when incoming
outer_vars shadow model defaults; update the conditional so that if
refresh_cache and not dry_run_placeholders and (runtime_kwarg is truthy OR
outer_vars is provided and outer_vars != model.query_variables) you perform the
canonical render: call _query_as_model with inner_query=stages[-1],
named_queries=named_q, override_name=model.name, _resolving=set(),
outer_vars=dict(model.query_variables), runtime_kwarg=None,
dry_run_placeholders=True (same as the runtime_kwarg branch), then pass the
resulting canonical to _refresh_cache_after_resolution (otherwise use virtual) —
i.e., detect shadowing via outer_vars != model.query_variables and treat it like
the runtime_kwarg case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bb3dbff-9e5b-4ff8-aa0e-e4f5379b3c14
📒 Files selected for processing (16)
.claude/skills/slayer-models.md.claude/skills/slayer-query.mddocs/concepts/models.mddocs/concepts/queries.mddocs/interfaces/rest-api.mdslayer/api/server.pyslayer/cli.pyslayer/core/models.pyslayer/core/query.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/test_api_server.pytests/test_cli.pytests/test_mcp_server.pytests/test_models.pytests/test_query_backed_models.py
✅ Files skipped from review due to trivial changes (3)
- docs/interfaces/rest-api.md
- docs/concepts/queries.md
- tests/test_api_server.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .claude/skills/slayer-query.md
- slayer/core/query.py
- slayer/core/models.py
- slayer/cli.py
- tests/test_models.py
- slayer/api/server.py
- slayer/mcp/server.py
Addresses follow-up review on commit ed43392. CodeRabbit (Major) — slayer/engine/query_engine.py:942: stored query- backed join targets were resolved either from cached backing_query_sql or from _query_as_model with outer_vars=target.query_variables, so a target like {"filters": ["amount > {threshold}"]} was stuck on cached or default values and ignored the enclosing query's runtime variables. Fix: extracted _render_query_backed_join_target which merges target.query_variables with the outer query's variables (outer wins) and passes them as both outer_vars and runtime_kwarg to _query_as_model. The cache fast-path is only taken when the enclosing query has no variables. Added a class-level _join_target_resolving set on the engine to break loops on cyclic schemas (the call stack crosses _enrich invocations, so a closure-local set isn't sufficient). CodeRabbit (Minor) — tests/test_mcp_server.py:375: hardened test_json_show_sql_includes_backing_query_sql to assert the field is present + non-empty + contains the column reference. The previous "if parsed.get(...)" guard let regressions in the cache-refresh path slip through. SonarQube — slayer/engine/query_engine.py:901 (S3776 cognitive complexity 21/15 on _enrich): NOSONAR-annotated with rationale (the function orchestrates resolve-callback closures + cross-model post-processing; splitting hides the closure variables threaded through enrich_query). Marked Accept in SonarCloud. New regression test: tests/test_query_backed_models.py::TestJoinTargetIsQueryBacked::test_join_target_with_variables_uses_runtime_value All 1129 unit tests pass; SQLite + Postgres + DuckDB + Jaffle Shop integration suites (185 tests) green. Ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_query_backed_models.py (1)
309-310: ⚡ Quick winMove these test imports to module scope.
This new file introduces several inline imports even though the repo standard keeps Python imports at the top of the module. Please hoist
DataType,Column,ModelJoin,SlayerSQLClient, andslayer.engine.query_engineinto the top-level import block.As per coding guidelines, "
**/*.py: Place imports at the top of files`."Also applies to: 588-588, 633-633, 693-693, 727-727
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_query_backed_models.py` around lines 309 - 310, Several tests in tests/test_query_backed_models.py perform inline imports (DataType, Column, ModelJoin, SlayerSQLClient, and slayer.engine.query_engine) inside test functions; move these imports to the module scope by hoisting the statements into the file's top-level import block so they are imported once for the module rather than inside individual tests, ensuring DataType, Column, ModelJoin, SlayerSQLClient, and slayer.engine.query_engine are imported at the top consistent with project import guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/engine/query_engine.py`:
- Around line 200-204: The instance-scoped set _join_target_resolving must not
be shared across concurrent requests because it causes cross-request re-entry
detection in _resolve_join_target/_enrich; change this to a request-scoped guard
(e.g., a ContextVar or an explicit set passed through the resolution call chain)
so each request has its own in-flight set; update usages of
_join_target_resolving in _resolve_join_target and any callers (including
_enrich) to read/modify the request-local set instead of the engine attribute,
and remove or deprecate the engine-level attribute to avoid accidental shared
access.
- Around line 864-905: The refresh path in _refresh_cache_after_resolution only
compares/updates columns and backing_query_sql, leaving stored_model.data_source
stale; update the early-return condition to also compare
stored_model.data_source with virtual.data_source and include "data_source":
virtual.data_source in the model_copy update so the persisted model's
data_source is overwritten when the resolved virtual model changes; touch the
same comparison/update logic in _refresh_cache_after_resolution (function names:
_refresh_cache_after_resolution, stored_model, virtual) to ensure
get_column_types() opens the correct backend.
- Around line 610-640: The cache-refresh path must treat non-default outer_vars
like runtime_kwarg to avoid request-specific SQL in backing_query_sql; modify
the branch inside the refresh_cache block so the condition currently checking
only runtime_kwarg also triggers when outer_vars is provided (i.e., if
runtime_kwarg or outer_vars), and when true re-render canonical via
_query_as_model with outer_vars=dict(model.query_variables) and
_resolving=set(), then call _refresh_cache_after_resolution(model, canonical) as
before; otherwise (no runtime_kwarg and no outer_vars) keep the existing
behavior of refreshing with virtual.
---
Nitpick comments:
In `@tests/test_query_backed_models.py`:
- Around line 309-310: Several tests in tests/test_query_backed_models.py
perform inline imports (DataType, Column, ModelJoin, SlayerSQLClient, and
slayer.engine.query_engine) inside test functions; move these imports to the
module scope by hoisting the statements into the file's top-level import block
so they are imported once for the module rather than inside individual tests,
ensuring DataType, Column, ModelJoin, SlayerSQLClient, and
slayer.engine.query_engine are imported at the top consistent with project
import guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1303a01e-4e33-43bb-8fa5-1b8006c77d0f
📒 Files selected for processing (3)
slayer/engine/query_engine.pytests/test_mcp_server.pytests/test_query_backed_models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_mcp_server.py
…fresh) Addresses 3 follow-up CodeRabbit findings on commit 2e44024. CodeRabbit Critical — slayer/engine/query_engine.py:204 + 981-1005: The recursion guard for query-backed join targets was an instance set (self._join_target_resolving) shared across concurrent requests through the same engine. Switched to a contextvars.ContextVar with a small _get_join_target_resolving() helper that lazy-inits a fresh set on first access in the current asyncio context — each task now gets its own in-flight set automatically. CodeRabbit Major — slayer/engine/query_engine.py:610-640: The cache-refresh canonical re-render only triggered when runtime_kwarg was truthy. When an enclosing SlayerQuery carried its own variables (propagated via outer_vars), they could be persisted into backing_query_sql, causing cache churn and per-request data leakage on inspect/export. Condition now triggers whenever merged_outer differs from model.query_variables OR runtime_kwarg is set. CodeRabbit Major — slayer/engine/query_engine.py:864-905: data_source was preserved from the stored model (model.data_source or virtual.data_source), so editing a query-backed model so its final stage now resolves through a different upstream datasource left the persisted data_source stale — get_column_types() then opened the wrong client. _validate_and_populate_cache now overwrites with virtual.data_source; _refresh_cache_after_resolution adds data_source to both the change detection and the model_copy update. New regression tests: - TestJoinTargetIsQueryBacked::test_join_target_resolving_set_is_per_context - TestBackingQuerySQLCacheHygiene::test_outer_query_variables_do_not_leak_into_persisted_sql - TestBackingQuerySQLCacheHygiene::test_data_source_refreshed_when_backing_query_changes All 1132 unit tests + 185 integration tests pass. Ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@slayer/engine/query_engine.py`:
- Around line 323-332: The current logic only sets updates["dry_run"] and
updates["explain"] when the caller-provided dry_run/explain are truthy, which
prevents clearing persisted True flags; change the block that builds updates so
it unconditionally assigns the caller values (i.e., set updates["dry_run"] =
dry_run and updates["explain"] = explain) instead of guarding on truthiness,
then call main_query.model_copy(update=updates) as before; locate this change
around the variables merged, updates dict and the model_copy call in the
function where main_query is updated.
- Around line 535-554: In get_column_types, when you call self._resolve_model
for query-backed models (the model variable is replaced), recompute the
datasource/client from the resolved model before building the probe query and
choosing the dialect; specifically after the await
self._resolve_model(model_name=model_name) assignment, re-evaluate the
datasource (the variable previously used to call self._dialect_for_type and any
DB client selection) from the resolved model so that _build_type_probe_query,
_enrich, _dialect_for_type and SQLGenerator use the correct datasource for the
expanded virtual model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50c58c05-a0c5-424c-90dc-288ce95e552e
📒 Files selected for processing (2)
slayer/engine/query_engine.pytests/test_query_backed_models.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_query_backed_models.py
…d model CodeRabbit Major (PR #67 thread on commit dcb8960, line 554): when a query-backed model has a stale or blank stored data_source, get_column_types() picked the wrong backend (or returned {}) because _resolve_datasource was called before _resolve_model expanded the model. Fix: - Hoist the source_queries expansion to run before datasource resolution and the SQL-client open. The datasource is then derived from the freshly-resolved virtual model. - Move the probeable-columns short-circuit past the expansion too — it was bailing out on the stored model's empty columns before the engine even got to populate them from the inner query. Filed #71 to cover the dry_run/explain-on-SlayerQuery design issue flagged by the same review (deferring; root cause is schema-level). New regression test: - TestQueryBackedColumnTypes::test_get_column_types_uses_resolved_datasource saves a query-backed model via raw storage.save_model so its data_source is blank and columns are empty, then verifies the type probe runs against the right backend. All 1133 unit tests + 185 integration tests pass. Ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 3, 2026
…der fill) Codex review of commit 73f69b0 surfaced two PR-scope issues; the other 4 findings are deferred to follow-up issues #71/#73/#74. Minor — slayer/api/server.py:89: REST run-by-name disallowed-field check forgot whole_periods_only. A request with `{"name": "qb", "whole_periods_only": true}` was silently accepted and the flag was ignored. Added request.whole_periods_only to the disallowed list. Minor — slayer/engine/query_engine.py:528: get_column_types() expanded query-backed models without dry_run_placeholders=True. Type probing has no caller-supplied variables, so a model with a required-but-undefaulted {var} placeholder failed at SQL-gen and the function returned {} — inspect_model / type metadata silently disappeared for models that are otherwise executable with runtime variables. Pass dry_run_placeholders=True so the canonical placeholder fill (literal '0') applies, matching the save-time validation path. Deferred (filed as separate issues; see Codex review comments on PR): - #71 — move dry_run/explain off SlayerQuery (root cause of the asymmetric-override bug; CodeRabbit + Codex both flagged it). - #73 — resolution context refactor: variable precedence is re-derived ad-hoc at cross-model / rerooted / join-target call sites and each gets a different subset wrong (Codex findings 1+2+3). - #74 — lost-update race in _refresh_cache_after_resolution: read-path side effect saves entire model copy, racing concurrent edits (Codex finding 4). New regression tests: - test_post_query_run_by_name_rejects_whole_periods_only - test_get_column_types_with_required_unbound_variable All 1135 unit tests + 185 integration tests pass. Ruff clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 3, 2026
This was referenced May 10, 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.
Summary
Closes #58. Makes
source_queriesa first-class source mode onSlayerModel, peer tosql_tableandsql, and surfaces query-backed models end-to-end across engine, MCP, REST, and CLI.query_variables,backing_query_sql), source-mode exclusivity + stage-name validators, andsource_queries: List[SlayerQuery]typing with auto-parsing of dicts.engine.execute(str, variables=...)runs a query-backed model by name;engine.save_model(model)is the engine save helper that rejects user-supplied cache fields and refreshescolumns+backing_query_sqlon every execute (real / dry-run / explain) with write-if-changed.runtime kwarg > stage > outer query > model defaults. Save-time validation substitutes literal'0'for unresolved{var}placeholders.inspect_modeladds abacking_querysection andsource_type;create_modelacceptsvariables=and list-of-stages;querytool dispatches toexecute(str)for query-backed names;edit_modelhandlessource_queries/query_variables. RESTPOST /queryaccepts{"name": "<model>", "variables": {...}}. CLIslayer query <model_name>and--variables KEY=VALUE/--variables-jsonflags added.Out of scope per the design pass: dbt-import emission, a standalone
slayer models inspectCLI, and CLI YAML model files forslayer query. Variable-handling regime tracked separately in #65.Design pass and answer-summary posted as a comment on #58.
Test plan
poetry run pytest tests/ -m "not integration"— 1102 passedpoetry run pytest tests/integration/test_integration.py -m integration— 59 passed (SQLite)poetry run ruff check slayer/ tests/— cleanslayer models create query-backed.yaml, thenslayer query <name> --variables k=vPOST /modelswithsource_queriesandPOST /querywith{"name": "..."}inspect_modelshowsbacking_querysection;create_modelwithquery=populates the cache🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests