Move dry_run/explain off SlayerQuery (v3 schema, closes #71)#77
Merged
Conversation
These were execution-mode flags persisted into query bodies, where
they survived round-trips and broke query-backed model execution
(every load short-circuited to dry_run if the saved stage had it).
They are now engine.execute() kwargs only.
Schema: SlayerQuery and SlayerModel both bump to v3.
- v3_migration.py registers _query_v2_to_v3 (drops dry_run/explain
with one logger.warning + DeprecationWarning per migrated query,
identified by name/source_model) and _model_v2_to_v3 (walks
source_queries entries through the SlayerQuery migration chain so
nested stale flags get stripped too).
- SlayerQuery v3 sets extra="forbid" so typos raise.
Surface routing: REST QueryRequest, MCP query() tool, CLI flags, and
Python client method signatures all unchanged. Internal routing
changes only — they pop the flags before constructing the SlayerQuery
and pass them as engine kwargs.
Engine: execute()/execute_sync() gain dry_run/explain kwargs and a
str input shorthand (engine.execute("model_name")).
Tests: 12 new tests in tests/test_migrations.py — migration cases,
extra=forbid behaviour, engine kwargs across all input shapes, and
end-to-end stale v2 YAML regression with dry_run nested in
source_queries.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR removes persistent ChangesSchema Versioning & Execution Flag Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server as API/CLI/MCP
participant Engine as SlayerQueryEngine
participant Storage
Client->>API_Server: POST /query (payload + dry_run/explain)
API_Server->>Engine: engine.execute(query=..., dry_run=bool, explain=bool)
Engine->>Storage: if name -> load model YAML (migrate v2→v3 at load)
Storage-->>Engine: migrated model/query (no dry_run/explain)
Engine->>Engine: build SQL / EXPLAIN / short-circuit on dry_run
Engine-->>API_Server: SlayerResponse (sql, data, explain)
API_Server-->>Client: HTTP response with SQL / rows / explain
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 54 minutes and 1 second. Comment |
![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 (2)
docs/concepts/models.md (1)
264-273:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwitch this example to dict syntax.
This page is language-agnostic docs, so the
SlayerQuery(...)constructor should be shown as a plain query dict instead.As per coding guidelines, use JSON/dict syntax for all query objects in docs and examples — not Python class constructors.
Proposed replacement
engine.create_model_from_query( - query=SlayerQuery( - source_model="orders", - time_dimensions=[...], - measures=["*:count", "amount:sum"], - ), + query={ + "source_model": "orders", + "time_dimensions": [...], + "measures": ["*:count", "amount:sum"], + }, name="monthly_summary", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/concepts/models.md` around lines 264 - 273, Replace the Python-specific SlayerQuery(...) constructor usage in the example passed to engine.create_model_from_query with a plain JSON/dict-style query object (i.e., use a dict for the query argument instead of the SlayerQuery class) so the docs remain language-agnostic; update the call to engine.create_model_from_query(query=..., name="monthly_summary") where the query previously referenced SlayerQuery to instead use a dict/JSON structure with the same keys (source_model, time_dimensions, measures) and values.slayer/engine/query_engine.py (1)
174-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard the new list input against empties.
engine.execute([])currently raisesIndexErroratqueries[-1]. Since list input is now a supported public shape, this should fail fast with a clearerValueErrorinstead of crashing.🔧 Proposed fix
if isinstance(query, list): + if not query: + raise ValueError("query list must not be empty") queries = [SlayerQuery.model_validate(q) if isinstance(q, dict) else q for q in query] query = queries[-1]🤖 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 174 - 177, When handling list inputs in query_engine.execute (the branch that checks isinstance(query, list)), guard against an empty list before accessing queries[-1]; if the incoming list is empty raise a clear ValueError (e.g. "empty query list not allowed") instead of letting an IndexError occur. Keep the existing behavior of validating dict entries with SlayerQuery.model_validate and assigning query = queries[-1] for non-empty lists, and ensure named_queries is still initialized afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@docs/concepts/models.md`:
- Around line 264-273: Replace the Python-specific SlayerQuery(...) constructor
usage in the example passed to engine.create_model_from_query with a plain
JSON/dict-style query object (i.e., use a dict for the query argument instead of
the SlayerQuery class) so the docs remain language-agnostic; update the call to
engine.create_model_from_query(query=..., name="monthly_summary") where the
query previously referenced SlayerQuery to instead use a dict/JSON structure
with the same keys (source_model, time_dimensions, measures) and values.
In `@slayer/engine/query_engine.py`:
- Around line 174-177: When handling list inputs in query_engine.execute (the
branch that checks isinstance(query, list)), guard against an empty list before
accessing queries[-1]; if the incoming list is empty raise a clear ValueError
(e.g. "empty query list not allowed") instead of letting an IndexError occur.
Keep the existing behavior of validating dict entries with
SlayerQuery.model_validate and assigning query = queries[-1] for non-empty
lists, and ensure named_queries is still initialized afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77bbc92a-146a-48f7-9e1a-3a006205db1c
📒 Files selected for processing (12)
CLAUDE.mddocs/concepts/models.mdslayer/api/server.pyslayer/cli.pyslayer/client/slayer_client.pyslayer/core/models.pyslayer/core/query.pyslayer/engine/query_engine.pyslayer/mcp/server.pyslayer/storage/migrations.pyslayer/storage/v3_migration.pytests/test_migrations.py
# Conflicts: # CLAUDE.md # slayer/api/server.py # slayer/cli.py # slayer/engine/query_engine.py # slayer/mcp/server.py # tests/test_migrations.py
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)
slayer/engine/query_engine.py (1)
245-248:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject empty query lists before indexing the final stage.
execute()now accepts list input, butqueries[-1]will still raise an internalIndexErrorfor[]. Please fail fast with a clearValueErrorso callers get a deterministic validation error instead of a traceback.🔧 Suggested fix
if isinstance(query, list): + if not query: + raise ValueError("query list must not be empty") queries = [SlayerQuery.model_validate(q) if isinstance(q, dict) else q for q in query] query = queries[-1]🤖 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 245 - 248, The code accepts list input but immediately uses queries[-1], which crashes on an empty list; update the execute() handling where it builds queries (using SlayerQuery.model_validate and variables query/queries/named_queries) to first check if the incoming list is empty and raise a clear ValueError (e.g. "query list must not be empty") before creating 'queries' or indexing queries[-1], ensuring callers receive a deterministic validation error instead of an IndexError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@slayer/engine/query_engine.py`:
- Around line 245-248: The code accepts list input but immediately uses
queries[-1], which crashes on an empty list; update the execute() handling where
it builds queries (using SlayerQuery.model_validate and variables
query/queries/named_queries) to first check if the incoming list is empty and
raise a clear ValueError (e.g. "query list must not be empty") before creating
'queries' or indexing queries[-1], ensuring callers receive a deterministic
validation error instead of an IndexError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d30778b-96a8-4f0a-8bb2-0d2220d67f6a
📒 Files selected for processing (10)
CLAUDE.mddocs/concepts/models.mdslayer/api/server.pyslayer/cli.pyslayer/core/models.pyslayer/core/query.pyslayer/engine/query_engine.pyslayer/mcp/server.pytests/test_migrations.pytests/test_query_backed_models.py
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- docs/concepts/models.md
🚧 Files skipped from review as they are similar to previous changes (3)
- slayer/core/models.py
- slayer/mcp/server.py
- slayer/cli.py
- Lines 699/768 (python:S2068): test datasource never connects (dry_run) - Line 786 (python:S7493): hermetic test I/O matches existing pattern Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Use mig.CURRENT_VERSIONS["SlayerModel"] instead of the hard-coded 2; v3 schema bump on main means re-saved YAML stamps version=3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sonar's Python suppression syntax requires alphanumeric-only rule keys inside the parens — the leading 'python:' caused S7632 to flag all three of yesterday's NOSONAR comments as malformed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reuse the existing _build_engine_with_orders helper from the dry_run kwarg tests instead of re-creating the postgres datasource and orders model inline. Removes ~14 duplicated lines flagged by SonarCloud's new_duplicated_lines_density gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # tests/test_query_backed_models.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/test_ingestion_jaffle_shop.py (1)
204-204: 💤 Low valueMove the migration import to module scope.
This is a Python file, and the repo guideline is to keep imports at the top of the file. The test doesn't gain anything from a function-local import here.
As per coding guidelines, "Place imports at the top of files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_ingestion_jaffle_shop.py` at line 204, Move the local import "from slayer.storage import migrations as mig" out of the test body and place it at module scope (top of the file) so imports follow the project's guidelines; locate the test that currently does the function-local import and replace it with a top-level import for the symbol "mig" to avoid function-scoped imports and improve readability.
🤖 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.md`:
- Line 55: The doc line incorrectly states the cache refresh happens on every
engine.execute; update the wording to say cache refresh and persistence occur
only on save-time paths (engine.save_model and its helper
_validate_and_populate_cache) and not during engine.execute (real, dry-run,
explain); keep the note that engine.save_model rejects user-supplied
columns/backing_query_sql for query-backed models and that the cache is
persisted only when values have changed, and mention SlayerModel.query_variables
and engine.create_model_from_query as relevant symbols.
---
Nitpick comments:
In `@tests/integration/test_ingestion_jaffle_shop.py`:
- Line 204: Move the local import "from slayer.storage import migrations as mig"
out of the test body and place it at module scope (top of the file) so imports
follow the project's guidelines; locate the test that currently does the
function-local import and replace it with a top-level import for the symbol
"mig" to avoid function-scoped imports and improve readability.
🪄 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: f4387f60-c8ef-4006-9562-4b611d098234
📒 Files selected for processing (6)
CLAUDE.mddocs/concepts/models.mdslayer/engine/query_engine.pytests/integration/test_ingestion_jaffle_shop.pytests/test_migrations.pytests/test_query_backed_models.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/concepts/models.md
- tests/test_query_backed_models.py
- tests/test_migrations.py
- CLAUDE.md: narrow the cache-refresh wording — engine.execute never writes to storage after #74 (only save paths refresh the cache). - tests/integration/test_ingestion_jaffle_shop.py: hoist the function-local migrations import to module scope per CLAUDE.md imports-at-the-top rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
6 tasks
ZmeiGorynych
added a commit
that referenced
this pull request
May 3, 2026
- `examples/embedded/verify.py:84-86`: guard `regions_model` before dereferencing `.sql_table`. Mirrors the existing `orders_model` check pattern; turns a potential `AttributeError` into a recorded failed check. (CodeRabbit r3178151...) - `examples/embedded/verify.py`: hoist `AVG_QTY_KEY = "orders.avg_qty"` and `CUMULATIVE_KEY = "orders.cumulative"` (3 uses each, the two remaining S1192 hits). (Sonar AZ3t50XxIhX985J8J4BV + AZ3t50XxIhX985J8J4BU; CR nitpick pullrequestreview-4216330491.) The other 4 column keys CR mentioned earlier are below Sonar's threshold (2 uses each) and stay inline. - `pyproject.toml`: 0.3.2 → 0.4.0 to flag the BC-breaking v3 schema cut (PR #77 dropped `dry_run`/`explain` from `SlayerQuery` and added `extra="forbid"`). `slayer/__init__.py:__version__` is still `"0.1.0rc2"` and predates this PR — flagged but left alone. Verification: ruff clean; embedded `verify.py` is 42/42 green (one new check from the regions guard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 6, 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 #71.
dry_runandexplainwere execution-mode flags persisted intoSlayerQuerybodies, where they survived round-trips and broke query-backed model execution (every load short-circuited to dry_run if the saved stage had it). They are nowengine.execute()kwargs only.SlayerQueryandSlayerModelboth go tov3.slayer/storage/v3_migration.pyregisters_query_v2_to_v3(dropsdry_run/explainwith onelogger.warning+ oneDeprecationWarningper migrated query, identifying it by name/source_model) and_model_v2_to_v3(walkssource_queriesentries through the SlayerQuery chain so nested stale flags get stripped).extra=\"forbid\"onSlayerQueryv3 so typos raise.QueryRequest, MCPquery()tool, CLI flags, Python client methods): wire formats unchanged. Internal routing changes only — they pop the flags before constructing theSlayerQueryand thread them through engine kwargs.execute()/execute_sync()gaindry_run/explainkwargs (apply to all input shapes includinglist— the kwarg targets the single resulting SQL statement). Also adds astrinput shorthand (engine.execute(\"model_name\")) for the regression test in Move dry_run/explain off SlayerQuery — they're execution flags, not query shape #71's acceptance criteria.Breaking changes
SlayerQuery(unknown_field=...)now raisesValidationError(extra=forbid). The deprecateddry_run/explainthemselves are intercepted by the migration and emit a DeprecationWarning rather than raising — soft landing for callers porting away from v2.logger.warningper nested SlayerQuery whose v2 YAML carrieddry_run/explain. Re-saving normalizes the on-disk YAML.Test plan
poetry run pytest -m \"not integration\"— 1063 passed (12 new).poetry run pytest tests/integration/test_integration.py tests/integration/test_integration_duckdb.py -m integration— 84 passed.poetry run ruff check slayer/ tests/— clean.New tests in
tests/test_migrations.py:extra=\"forbid\"regression: typo field raises; directSlayerQuery(dry_run=True)warns rather than raises.dry_run: truenested insidesource_queriesis migrated on load, the warning identifies the inner query by name, andengine.execute(\"name\")actually executes (not short-circuits).🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor
Tests