S9: align docs, skills, and notebooks with v2/v3 schema (closes #53)#80
Merged
Conversation
Grep-and-update of every user-facing surface that drifted across the v1→v2 (PR #61) and v2→v3 (PR #77) schema cuts, plus PR #79's cache-policy change. - Concepts/getting-started/interfaces/reference docs: replace v1 `fields:` with `measures:`, `model:` with `source_model:`, separate `dimensions:`+`measures:` YAML lists with unified `columns:` (and a named-formula `measures:` library), `Field`/`ColumnRef`/`Dimension` Pydantic class refs in prose with the v3 equivalents. - `*:count` result-key correction: spec said `orders.count` but the engine emits `orders._count` (verified `slayer/core/formula.py:88`, `slayer/engine/enrichment.py`, integration tests). CLAUDE.md and 5 docs updated to match the code. - v3 specifics: `dry_run`/`explain` reframed as `engine.execute()` kwargs (no longer query body fields); `SlayerQuery` `extra="forbid"` noted in forward-tolerance section; query-backed model cache claim corrected to "save paths only". - Style: replaced Python class constructor query examples in skills/README with dict syntax (per `docs/CLAUDE.md`). - Stale CLI flag: `--models-dir` → `--storage`. Added missing `--demo` row to `slayer serve`/`slayer mcp` flag tables. - `docs/examples/01_dynamic/dynamic.md`: `revenue:quantile(0.25)` → `revenue:percentile(p=0.25)` (the actual built-in). - Help topic `08_models.md`: replaced split Dimensions/Measures sections with a unified Columns section + a separate Named-Formula Measures section; corrected the result-column-naming table. - Runnable Python examples (`examples/embedded/{run,verify}.py`, `examples/verify_common.py`): rewritten to v3 (`SlayerQuery(measures=...)`, drop `Field` import, dict-form dimensions/order, fix `name="rank"` shadowing the reserved transform name). Verified end-to-end on bundled SQLite — 41/41 verify checks pass. - `docs/examples/06_multistage_queries/multistage_queries_nb.ipynb`: same ModelExtension `dimensions` → `columns` fix as the sibling .md. Verification: ruff clean; 1264 unit tests pass; embedded run.py executes all 12 demo queries without error; verify.py is 41/41 green. 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 updates documentation, examples, help topics, tests, and example scripts to the v3 query/schema: use ChangesDocs / Examples / CLI migration (unified DAG)
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine as SlayerQueryEngine
participant Cache as ModelCache
participant DB as Database
participant Storage
Client->>Engine: execute(query, variables=?, dry_run=? / explain=?)
Engine->>Cache: read columns/backing_query_sql (may be stale)
Engine->>DB: run SQL plan (no writes)
DB-->>Engine: result rows
Engine-->>Client: result (keys e.g., orders._count)
Client->>Engine: create_model_from_query(query, save=True) / save_model(...)
Engine->>DB: run save-time dry-run (introspect columns)
DB-->>Engine: result columns
Engine->>Cache: update columns/backing_query_sql
Engine->>Storage: persist model + cached columns/backing_query_sql
Storage-->>Engine: ack
Engine-->>Client: saved model metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
examples/mysql/README.md (1)
18-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
poetry runforverify.pyin this example too.
python verify.pymay run with the wrong interpreter/dependencies. Switching topoetry run python verify.pymatches the repo’s documented command conventions.Based on learnings, “Use `poetry run` for all Python commands.”✅ Proposed fix
- python verify.py + poetry run python verify.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/mysql/README.md` around lines 18 - 22, Update the example command so it uses the project's Poetry-managed environment: replace the bare "python verify.py" invocation with "poetry run python verify.py" in the README snippet and any related explanatory sentence, ensuring the instruction and the example command both reflect "poetry run" to guarantee the correct interpreter and dependencies are used.examples/clickhouse/README.md (1)
18-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate ClickHouse example verification command to
poetry run.To avoid interpreter/dependency mismatch, align the verification step with Poetry usage:
poetry run python verify.py.Based on learnings, “Use `poetry run` for all Python commands.”✅ Proposed fix
- python verify.py + poetry run python verify.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/clickhouse/README.md` around lines 18 - 21, Replace the bare "python verify.py" invocation in the ClickHouse example README code block with "poetry run python verify.py" so the verification step uses the project's Poetry-managed environment; find the code block containing the exact string "python verify.py" and update it to "poetry run python verify.py".examples/postgres/README.md (1)
43-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate example verification to use
poetry run.The examples use
python verify.py, but repo conventions requirepoetry runfor Python commands. This helps ensure the correct Poetry virtualenv and dependencies are used when users run the example.Based on learnings, “Use `poetry run` for all Python commands.”✅ Proposed fix
- python verify.py + poetry run python verify.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/postgres/README.md` around lines 43 - 45, Replace the example command that runs verify.py so it uses Poetry's virtualenv; update the README code block invoking "python verify.py" to run via Poetry (e.g., use "poetry run python verify.py") so the verify.py script executes with the project's Poetry-managed environment and dependencies.slayer/help/topics/09_extending.md (1)
11-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't document
ModelExtension.filtersuntil the type supports it.
slayer/core/query.py:229-240still definesModelExtensionwith onlysource_name,columns,measures, andjoins. As written, this example/field list is advertising an API shape that the runtime cannot currently validate reliably, so users following it will either get a validation error or have the filter silently ignored. Please either add the field in code or remove it from the docs/examples copied from this pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slayer/help/topics/09_extending.md` around lines 11 - 29, The docs list a `filters` field for ModelExtension but the runtime type lacks it; update the ModelExtension definition in slayer/core/query.py (the ModelExtension dataclass/typing around lines where ModelExtension is defined) to include an optional filters: Optional[List[str]] (or list[str] = None) and add handling/validation where ModelExtension instances are parsed/validated (the code paths that construct/validate ModelExtension) so filters are accepted and applied rather than ignored; alternatively, if you prefer not to change runtime, remove `filters` from the example in the docs to match the existing ModelExtension shape.docs/concepts/queries.md (1)
59-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix stale
OrderItem.columnexample (*:count→count).
docs/concepts/queries.mdshows anOrderItemexample with"column": "*:count", but elsewhere in the PR/docs (and in the engine aliasing behavior),order[].columnis documented as the short measure alias (e.g.,"count"/"revenue_sum"), while*:countis an input measure formula that maps to the internalorders._countresult key.This mismatch can cause users to copy/paste an
orderexample that doesn’t resolve the intended alias.✅ Proposed doc fix
```json -{"column": "*:count", "direction": "desc"} +{"column": "count", "direction": "desc"}</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/concepts/queries.mdaround lines 59 - 66, The OrderItem example uses a
stale measure alias ":count" for order[].column; update the example so
order[].column shows the short measure alias "count" instead of ":count" (i.e.,
replace the string "*:count" with "count" in the OrderItem JSON example) and
ensure any other occurrences of order[].column examples in the same doc use the
short aliases like "count" or "revenue_sum" to match the engine aliasing
behavior.</details> </blockquote></details> </blockquote></details>🧹 Nitpick comments (2)
examples/verify_common.py (1)
107-175: ⚡ Quick winExtract the repeated count literals.
"*:count"is hard-coded across several payloads here. Hoisting it, and optionally a tiny helper for"{model}._count", would clear the current Sonar duplication warning and make the next naming tweak a one-line edit instead of another sweep.Possible cleanup
+COUNT_MEASURE = "*:count" + + +def count_key(model_name): + return f"{model_name}._count" + ... { "source_model": "orders", - "measures": ["*:count"], + "measures": [COUNT_MEASURE], }, ) - check(f"total orders = {TOTAL_ORDERS}", result["data"][0]["orders._count"] == TOTAL_ORDERS) + check( + f"total orders = {TOTAL_ORDERS}", + result["data"][0][count_key("orders")] == TOTAL_ORDERS, + )Also applies to: 250-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/verify_common.py` around lines 107 - 175, Multiple payloads repeat the literal "*:count" and repeated access to "{model}._count"; define a single constant (e.g., COUNT_MEASURE = "*:count") and a small helper function (e.g., def count_field(model): return f"{model}._count") then replace all occurrences in the API payloads and subsequent result checks to use COUNT_MEASURE and count_field("orders") / count_field("products") / count_field("customers") (and similarly for dynamic checks using STATUS_COUNTS, TOTAL_ORDERS, etc.) so future renames are a one-line change and duplication is removed.examples/embedded/verify.py (1)
84-289: ⚡ Quick winExtract the shared count literals in this script.
The v3-specific
"*:count"and"orders._count"strings are repeated throughout these checks, which is why the current Sonar run is failing here. A couple of module constants/helpers would make future schema renames much safer and keep this verifier easier to update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/embedded/verify.py` around lines 84 - 289, The script repeats the v3-specific literals "*:count" and "orders._count"; define module-level constants (e.g., COUNT_MEASURE = "*:count" and COUNT_COLUMN = "orders._count") and replace every direct use of those string literals in SlayerQuery measures, column membership checks (e.g., "orders._count" in result.columns), dict comprehensions (keys like r["orders._count"]), comparisons, and any order specifications that reference the count with these constants so future schema renames only need updating in one place; search for occurrences of "*:count" and "orders._count" in this file (including uses in result.data[...] and result.columns checks) and swap them to use the new constants.🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed. Inline comments: In `@docs/concepts/ingestion.md`: - Line 58: The concepts page still describes auto-ingestion producing "dimensions" plus one measure per non-ID column; update docs/concepts/ingestion.md to describe the v3 schema (columns and named-formulas) instead: replace references to "dimensions" and the old measure-per-column model with clear language about producing a columns array (with types/roles) and named-formulas for derived measures, update the CLI example/context around "slayer ingest --datasource my_postgres --schema public --storage ./slayer_data" and any examples to show the new columns/named-formulas shape, and grep the repo for old terms like "dimensions" or "measure" to update all occurrences to the v3 terminology. In `@docs/interfaces/cli.md`: - Line 17: Update the documented default for the CLI flag `--storage` so it consistently reflects the platform-appropriate path (use the same language as the earlier line: "~/.local/share/slayer on Linux, ~/Library/Application Support/slayer on macOS, %LOCALAPPDATA%\\slayer on Windows; can be overridden with $SLAYER_STORAGE") instead of "./slayer_data"; specifically change the `--storage` default entry in the `slayer query` section and the other occurrences referenced (the table entries around the earlier sections mentioned) so all mentions match the platform-specific default and the $SLAYER_STORAGE override text. In `@docs/interfaces/mcp.md`: - Line 16: The docs/interfaces/mcp.md page is only partially migrated to the v3 MCP schema; update the entire page to eliminate references to the old model contract (e.g., remove or replace `dimensions` in summaries/create flows, old measure metadata shapes, and `add_measures`/`add_dimensions` usage), update the Tools Reference and launch examples to the v3 schema (match the CLI examples like the new "claude mcp add slayer" usage), and audit linked docs (CLAUDE.md, docs/, .claude/skills/, docs/configuration/) to ensure all examples, field names, and invocation patterns reflect MCP v3 consistently. In `@docs/reference/mcp.md`: - Line 84: The docs row for edit_model says it upserts columns but the "Customize a model" walkthrough still uses the deprecated dimensions and remove["dimensions"] parameters; update the walkthrough and any examples on this page to use the columns-based contract (replace occurrences of dimensions and remove["dimensions"] with columns and remove["columns"], and adjust any example payloads or descriptive text to the new field names) so the edit_model examples and narrative consistently reference edit_model, columns, and remove["columns"] throughout; grep the file for "dimensions" and "remove[\"dimensions\"]" and make the replacements in the "Customize a model" example and any other mentions on the page. In `@examples/embedded/run.py`: - Line 66: The demo prints are using the internal alias `orders._count` instead of the public result key `orders.count`; update all occurrences of the lookup (e.g., the print call that currently references row['orders._count']) to use row['orders.count'] so the example reads from the user-facing result.data shape; apply this change to every similar print/lookup instance noted (lines referencing 'orders._count') so the embedded example uses the public COUNT column key. --- Outside diff comments: In `@docs/concepts/queries.md`: - Around line 59-66: The OrderItem example uses a stale measure alias "*:count" for order[].column; update the example so order[].column shows the short measure alias "count" instead of "*:count" (i.e., replace the string "*:count" with "count" in the OrderItem JSON example) and ensure any other occurrences of order[].column examples in the same doc use the short aliases like "count" or "revenue_sum" to match the engine aliasing behavior. In `@examples/clickhouse/README.md`: - Around line 18-21: Replace the bare "python verify.py" invocation in the ClickHouse example README code block with "poetry run python verify.py" so the verification step uses the project's Poetry-managed environment; find the code block containing the exact string "python verify.py" and update it to "poetry run python verify.py". In `@examples/mysql/README.md`: - Around line 18-22: Update the example command so it uses the project's Poetry-managed environment: replace the bare "python verify.py" invocation with "poetry run python verify.py" in the README snippet and any related explanatory sentence, ensuring the instruction and the example command both reflect "poetry run" to guarantee the correct interpreter and dependencies are used. In `@examples/postgres/README.md`: - Around line 43-45: Replace the example command that runs verify.py so it uses Poetry's virtualenv; update the README code block invoking "python verify.py" to run via Poetry (e.g., use "poetry run python verify.py") so the verify.py script executes with the project's Poetry-managed environment and dependencies. In `@slayer/help/topics/09_extending.md`: - Around line 11-29: The docs list a `filters` field for ModelExtension but the runtime type lacks it; update the ModelExtension definition in slayer/core/query.py (the ModelExtension dataclass/typing around lines where ModelExtension is defined) to include an optional filters: Optional[List[str]] (or list[str] = None) and add handling/validation where ModelExtension instances are parsed/validated (the code paths that construct/validate ModelExtension) so filters are accepted and applied rather than ignored; alternatively, if you prefer not to change runtime, remove `filters` from the example in the docs to match the existing ModelExtension shape. --- Nitpick comments: In `@examples/embedded/verify.py`: - Around line 84-289: The script repeats the v3-specific literals "*:count" and "orders._count"; define module-level constants (e.g., COUNT_MEASURE = "*:count" and COUNT_COLUMN = "orders._count") and replace every direct use of those string literals in SlayerQuery measures, column membership checks (e.g., "orders._count" in result.columns), dict comprehensions (keys like r["orders._count"]), comparisons, and any order specifications that reference the count with these constants so future schema renames only need updating in one place; search for occurrences of "*:count" and "orders._count" in this file (including uses in result.data[...] and result.columns checks) and swap them to use the new constants. In `@examples/verify_common.py`: - Around line 107-175: Multiple payloads repeat the literal "*:count" and repeated access to "{model}._count"; define a single constant (e.g., COUNT_MEASURE = "*:count") and a small helper function (e.g., def count_field(model): return f"{model}._count") then replace all occurrences in the API payloads and subsequent result checks to use COUNT_MEASURE and count_field("orders") / count_field("products") / count_field("customers") (and similarly for dynamic checks using STATUS_COUNTS, TOTAL_ORDERS, etc.) so future renames are a one-line change and duplication is removed.🪄 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:
fd10e898-a7ee-4aac-a197-8c2bda69b9cf📒 Files selected for processing (37)
.claude/skills/slayer-models.md.claude/skills/slayer-query.mdCLAUDE.mdREADME.mddocs/concepts/formulas.mddocs/concepts/ingestion.mddocs/concepts/models.mddocs/concepts/queries.mddocs/concepts/terminology.mddocs/development.mddocs/examples/01_dynamic/dynamic.mddocs/examples/02_sql_vs_dsl/sql_vs_dsl.mddocs/examples/06_multistage_queries/multistage_queries.mddocs/examples/06_multistage_queries/multistage_queries_nb.ipynbdocs/examples/07_aggregations/aggregations.mddocs/getting-started/cli.mddocs/getting-started/python.mddocs/getting-started/rest-api.mddocs/index.mddocs/interfaces/cli.mddocs/interfaces/mcp.mddocs/interfaces/rest-api.mddocs/reference/mcp.mddocs/reference/python-client.mddocs/reference/rest-api.mdexamples/clickhouse/README.mdexamples/embedded/run.pyexamples/embedded/verify.pyexamples/mysql/README.mdexamples/postgres/README.mdexamples/verify_common.pyslayer/help/topics/00_intro.mdslayer/help/topics/03_aggregations.mdslayer/help/topics/06_filters.mdslayer/help/topics/08_models.mdslayer/help/topics/09_extending.mdslayer/help/topics/10_workflow.md
…tes)
Group 1 — finish v3 migration in 4 partially-updated reference pages:
- `docs/concepts/ingestion.md`: Step 3 now describes ingestion as
generating one Column per non-joined source-table column (with `type`
inferred), explicit "no auto-generated measures", and `count_col`
rename. Top-line and Step 1 prose also reframed around columns instead
of dimensions/measures. (CodeRabbit r3178094069.)
- `docs/interfaces/cli.md`: `slayer query` `--storage` default now reads
"platform-appropriate path" to match the other two flag tables on the
same page. (CodeRabbit r3178094070.)
- `docs/interfaces/mcp.md` Tools Reference: `models_summary` and
`inspect_model` rows now say "columns and measures (named formulas)"
/ "column-level filters"; `create_model` row uses the v3
`columns`+`measures` shape; `edit_model` row replaces the dead
`add_measures`/`add_dimensions`/`remove (list)` description with the
actual signature (upserts `columns`/`measures`/`aggregations`/`joins`,
`remove` is a dict keyed by entity type). Verified against
`slayer/mcp/server.py:1664-1681`. (CodeRabbit r3178094071.)
- `docs/reference/mcp.md` "Customize a model" walkthrough: rewritten to
the v3 shape — `columns=[...]` (not `dimensions=`), `measures=[...]`
using the named-formula shape (`{formula, name, label}`, not
`{name, sql}`), and `remove={"columns": [...]}` (not
`{"measures": [...]}`). (CodeRabbit r3178094073.)
Group 2 — hoist repeated string literals to module constants
(Sonar python:S1192, 6 critical issues + 1 overlapping CodeRabbit nit):
- `examples/embedded/run.py`: `COUNT_MEASURE = "*:count"` (12 uses),
`QUANTITY_SUM_MEASURE = "quantity:sum"` (4 uses).
- `examples/embedded/verify.py`: `COUNT_MEASURE` (14 uses),
`COUNT_KEY = "orders._count"` (9 uses),
`CUMSUM_CHANGE_KEY = "orders.cumsum_change"` (4 uses).
- `examples/verify_common.py`: `COUNT_MEASURE` (8 uses).
(One CodeRabbit thread on `examples/embedded/run.py:66` arguing that
`*:count` should produce `orders.count` rather than `orders._count` was
classified INVALID — pinned by integration tests
`test_integration_duckdb.py:106`, `test_integration_postgres.py:114`,
and `slayer/core/formula.py:88`. Reply posted on the thread; this is
the spec-vs-code drift the parent commit `c15ada4` already corrected.)
Verification: ruff clean; embedded `verify.py` is 41/41 green.
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)
examples/embedded/verify.py (1)
77-81: ⚡ Quick winUse keyword args in the new
check()calls.These added calls pass two positional arguments, which conflicts with the repo’s Python calling convention. Please switch them to
check(name=..., condition=...)here and in the other newly addedcheck(...)calls below.♻️ Suggested change
- check("orders has quantity column", "quantity" in column_names) + check(name="orders has quantity column", condition="quantity" in column_names) join_targets = [j.target_model for j in orders_model.joins] - check("orders joins to customers", "customers" in join_targets) - check("orders joins to products", "products" in join_targets) + check(name="orders joins to customers", condition="customers" in join_targets) + check(name="orders joins to products", condition="products" in join_targets)As per coding guidelines:
**/*.py: Use keyword arguments for functions with more than 1 parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/embedded/verify.py` around lines 77 - 81, The three new calls to check (the function check) use positional args; change them to keyword args using name= for the descriptive string and condition= for the boolean expression — e.g., replace the call that checks quantity in column_names (column_names = [c.name for c in orders_model.columns]) with check(name=..., condition="quantity" in column_names), and similarly update the two join checks that inspect join_targets (join_targets = [j.target_model for j in orders_model.joins]) to check(name=..., condition="customers" in join_targets) and check(name=..., condition="products" in join_targets); also apply the same keyword-arg style to any other newly added check(...) calls elsewhere in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/ingestion.md`:
- Around line 23-24: The docs are inconsistent about FK cycle handling: the
earlier paragraph claims ingestion raises RollupGraphError on cycles while the
later "Cycle Handling" section says it logs a warning and falls back; pick the
correct behavior (either raising RollupGraphError or logging a warning and
falling back) and make both places consistent by updating the sentence that
mentions RollupGraphError and the "Cycle Handling" section text to match the
chosen behavior; ensure you reference the same outcome and any error name
(RollupGraphError) or the fallback/logging behavior consistently and adjust
examples/implications (e.g., transitive closure usage) accordingly.
In `@docs/interfaces/mcp.md`:
- Line 72: Update the docs for the models_summary endpoint to stop claiming "No
types" unconditionally: change the wording around `models_summary` to state that
column types are omitted only in the Markdown `format` (default) but are
included in the JSON `format`, or remove the blanket "No types" phrase;
reference `models_summary`, the `format` param (values `"markdown"` and
`"json"`), and `inspect_model` for users who need per-field detail.
In `@docs/reference/mcp.md`:
- Line 81: The docs entry for models_summary is misleading about types; update
the description for the models_summary endpoint/parameter so it clarifies that
Markdown output omits column types but JSON output does include column `type`
values (or remove the blanket “No types” claim); reference the `models_summary`
name and its `format` parameter (default "markdown", also "json") and ensure the
sentence explicitly states the difference between `format="markdown"` and
`format="json"` regarding types.
---
Nitpick comments:
In `@examples/embedded/verify.py`:
- Around line 77-81: The three new calls to check (the function check) use
positional args; change them to keyword args using name= for the descriptive
string and condition= for the boolean expression — e.g., replace the call that
checks quantity in column_names (column_names = [c.name for c in
orders_model.columns]) with check(name=..., condition="quantity" in
column_names), and similarly update the two join checks that inspect
join_targets (join_targets = [j.target_model for j in orders_model.joins]) to
check(name=..., condition="customers" in join_targets) and check(name=...,
condition="products" in join_targets); also apply the same keyword-arg style to
any other newly added check(...) calls elsewhere in this file.
🪄 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: 6319aa1f-b7ae-442e-89ac-85de2ff46e6d
📒 Files selected for processing (7)
docs/concepts/ingestion.mddocs/interfaces/cli.mddocs/interfaces/mcp.mddocs/reference/mcp.mdexamples/embedded/run.pyexamples/embedded/verify.pyexamples/verify_common.py
✅ Files skipped from review due to trivial changes (1)
- docs/interfaces/cli.md
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/verify_common.py
Group 1 — doc accuracy fixes (3 CodeRabbit threads):
- `docs/concepts/ingestion.md:23`: previous wording claimed ingestion
raises `RollupGraphError` on FK cycles, but the user-visible behavior
(verified `slayer/engine/ingestion.py:664-668`) is that
`ingest_datasource` catches the internal raise from `_check_acyclic`,
logs a warning, and falls back to simple models. Reworded the Step 1
paragraph to match the existing "Cycle Handling" section.
(CodeRabbit r3178115700.)
- `docs/interfaces/mcp.md:72` and `docs/reference/mcp.md:81`:
`models_summary` row claimed "No types" unconditionally, but JSON
output (`format="json"`) does include column `type` per
`slayer/mcp/server.py:989`. Now scopes the Markdown-vs-JSON
difference explicitly. (CodeRabbit r3178115702 / r3178115705.)
Group 2 — convention compliance (1 nitpick):
- `examples/embedded/verify.py`: rewrote all 39 `check(...)` call
sites from positional `check("name", expr)` to kwarg form
`check(name="name", condition=expr)` to satisfy the global rule
"Use keyword arguments for functions with more than 1 parameter".
Done file-wide for consistency, not just the lines added in
c15ada4. (CodeRabbit nitpick pullrequestreview-4216307406.)
Verification: ruff clean; embedded `verify.py` is 41/41 green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/interfaces/mcp.md (1)
160-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the “Customize a model” workflow to v3 keys.
Line 163, Line 164, and Line 165 still use deprecated
add_measures,add_dimensions, and list-styleremove, while Line 75 documents the currentcolumns/measuresupsert andremove: {"columns": [...], ...}contract. Please migrate this snippet to the same v3 shape to avoid copy/paste breakage.Based on learnings: "When renaming a field, adding a parameter, or changing response structure, grep all docs and skills for the old name and update every occurrence."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/interfaces/mcp.md` around lines 160 - 166, The snippet for edit_model uses deprecated keys add_measures, add_dimensions, and list-style remove; update the example to the v3 contract by using the edit_model call with upsert-style keys (e.g., "columns" and "measures" for adding/updating columns/measures) and change remove to an object like remove: {"columns": [...], "measures": [...]}; ensure the example still calls edit_model(model_name="orders") and replace add_measures/add_dimensions with measures=[{...}] and columns=[{...}] (or the exact v3 naming used elsewhere) and convert the remove list into remove: {"measures": ["old_measure"], ...} so it matches the documented v3 shape.
🧹 Nitpick comments (1)
examples/embedded/verify.py (1)
157-262: 💤 Low valueOptional: Consider hoisting other duplicated column keys for consistency.
For full consistency with the constant pattern established at lines 24-27, you could also hoist:
"orders.avg_qty"(lines 157, 158, 230)"orders.cumulative"(lines 173, 174)"orders.prev"(lines 187, 188)"orders.rnk"(lines 212, 213)"orders.running"(lines 247, 249)"orders.latest"(lines 260, 262)However, since each appears only 2-3 times and is localized to specific test blocks, this is optional and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/embedded/verify.py` around lines 157 - 262, Several column name strings are duplicated across the verification blocks; hoist them into named constants for consistency (similar to COUNT_KEY/COUNT_MEASURE). Add constants like ORDERS_AVG_QTY, ORDERS_CUMULATIVE, ORDERS_PREV, ORDERS_RNK, ORDERS_RUNNING, ORDERS_LATEST and replace occurrences of the literal strings "orders.avg_qty", "orders.cumulative", "orders.prev", "orders.rnk", "orders.running", and "orders.latest" in the checks and result indexing within the SlayerQuery/engine.execute_sync blocks and subsequent check calls so the tests reference the constants (keep COUNT_KEY, COUNT_MEASURE, TOTAL_ORDERS usage unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/embedded/verify.py`:
- Around line 24-27: Add a new constant CHG_KEY = "orders.chg" alongside
COUNT_MEASURE/COUNT_KEY/CUMSUM_CHANGE_KEY, then replace the three literal uses
with the constant: where the code accesses result.data[0][CHG_KEY] and
result.data[1][CHG_KEY] and the membership check CHG_KEY in result.columns;
ensure you update imports/exports if necessary and run the linter to confirm the
Sonar S1192 duplication warning is resolved.
---
Duplicate comments:
In `@docs/interfaces/mcp.md`:
- Around line 160-166: The snippet for edit_model uses deprecated keys
add_measures, add_dimensions, and list-style remove; update the example to the
v3 contract by using the edit_model call with upsert-style keys (e.g., "columns"
and "measures" for adding/updating columns/measures) and change remove to an
object like remove: {"columns": [...], "measures": [...]}; ensure the example
still calls edit_model(model_name="orders") and replace
add_measures/add_dimensions with measures=[{...}] and columns=[{...}] (or the
exact v3 naming used elsewhere) and convert the remove list into remove:
{"measures": ["old_measure"], ...} so it matches the documented v3 shape.
---
Nitpick comments:
In `@examples/embedded/verify.py`:
- Around line 157-262: Several column name strings are duplicated across the
verification blocks; hoist them into named constants for consistency (similar to
COUNT_KEY/COUNT_MEASURE). Add constants like ORDERS_AVG_QTY, ORDERS_CUMULATIVE,
ORDERS_PREV, ORDERS_RNK, ORDERS_RUNNING, ORDERS_LATEST and replace occurrences
of the literal strings "orders.avg_qty", "orders.cumulative", "orders.prev",
"orders.rnk", "orders.running", and "orders.latest" in the checks and result
indexing within the SlayerQuery/engine.execute_sync blocks and subsequent check
calls so the tests reference the constants (keep COUNT_KEY, COUNT_MEASURE,
TOTAL_ORDERS usage unchanged).
🪄 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: f31bd0f7-e22c-4f6c-ba4e-c07f910e8ad5
📒 Files selected for processing (4)
docs/concepts/ingestion.mddocs/interfaces/mcp.mddocs/reference/mcp.mdexamples/embedded/verify.py
Add `CHG_KEY = "orders.chg"` alongside the existing constants block; replaces 3 duplicated occurrences at lines 200, 202, 249. Verification: ruff clean; embedded `verify.py` is 41/41 green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/embedded/verify.py (1)
158-159: ⚡ Quick winHoist repeated result-column literals to constants.
"orders.avg_qty"and"orders.cumulative"are repeated in assertions. Consolidating them (likeCHG_KEY) keeps this verifier consistent and addresses the active Sonar duplication findings.Suggested fix
COUNT_MEASURE = "*:count" COUNT_KEY = "orders._count" CUMSUM_CHANGE_KEY = "orders.cumsum_change" CHG_KEY = "orders.chg" +AVG_QTY_KEY = "orders.avg_qty" +CUMULATIVE_KEY = "orders.cumulative" @@ - check(name="avg_qty column exists", condition="orders.avg_qty" in result.columns) - all_positive = all(row["orders.avg_qty"] > 0 for row in result.data) + check(name="avg_qty column exists", condition=AVG_QTY_KEY in result.columns) + all_positive = all(row[AVG_QTY_KEY] > 0 for row in result.data) @@ - check(name="cumsum column exists", condition="orders.cumulative" in result.columns) - check(name=f"cumsum final = {TOTAL_ORDERS}", condition=result.data[-1]["orders.cumulative"] == TOTAL_ORDERS) - cumvals = [r["orders.cumulative"] for r in result.data] + check(name="cumsum column exists", condition=CUMULATIVE_KEY in result.columns) + check(name=f"cumsum final = {TOTAL_ORDERS}", condition=result.data[-1][CUMULATIVE_KEY] == TOTAL_ORDERS) + cumvals = [r[CUMULATIVE_KEY] for r in result.data] @@ - check(name="expression column exists", condition="orders.avg_qty" in result.columns) + check(name="expression column exists", condition=AVG_QTY_KEY in result.columns)Also applies to: 174-176, 231-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/embedded/verify.py` around lines 158 - 159, Hoist repeated column-key string literals into named constants (e.g., AVG_QTY_KEY = "orders.avg_qty" and CUMULATIVE_KEY = "orders.cumulative") and replace all usages in this verifier: the check(...) calls and the row lookups such as all(row["orders.avg_qty"] > 0 for row in result.data), plus the other occurrences referenced (lines ~174-176 and ~231). Update references to use the new constants (AVG_QTY_KEY, CUMULATIVE_KEY) so the verifier no longer duplicates the literal strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/embedded/verify.py`:
- Line 85: The check currently dereferences regions_model.sql_table and can
raise AttributeError if storage.get_model("regions") returned None; update the
condition used in check (call site: check(name="regions has no rollup (sql_table
set)", condition=...)) to guard regions_model first (e.g., ensure regions_model
is not None before accessing sql_table) or use a safe accessor (like
getattr(regions_model, "sql_table", None) is not None) so the verification
records a failed check instead of raising.
---
Nitpick comments:
In `@examples/embedded/verify.py`:
- Around line 158-159: Hoist repeated column-key string literals into named
constants (e.g., AVG_QTY_KEY = "orders.avg_qty" and CUMULATIVE_KEY =
"orders.cumulative") and replace all usages in this verifier: the check(...)
calls and the row lookups such as all(row["orders.avg_qty"] > 0 for row in
result.data), plus the other occurrences referenced (lines ~174-176 and ~231).
Update references to use the new constants (AVG_QTY_KEY, CUMULATIVE_KEY) so the
verifier no longer duplicates the literal strings.
🪄 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: 81a1b278-def4-4609-9537-4490917bc965
📒 Files selected for processing (1)
examples/embedded/verify.py
DEV-1319 audited the repo for residual v1 schema wording. The bulk was addressed in c15ada4 / 5a2a135 / 3c6cbd3 / 3b14178; this commit closes the three remaining items: - `slayer/core/formula.py:1`: docstring "Formula parser for SLayer fields." → "...for SLayer measure formulas." (`FieldSpec` type alias itself is left alone — it's a real class, not a v1 vestige). - `slayer/help/topics/10_workflow.md:53-54`: the `edit_model` upsert example used `{"name": "margin", "sql": "revenue - cost"}` under `measures` — that's the v1 ModelMeasure-with-row-level-sql shape that no longer validates (`ModelMeasure` v3 has `formula`, no `sql`). Split into two bullets: row-level fields go through `columns` (with required `type`), saved aggregated formulas go through `measures` (with `formula`, not `sql`). - `tests/test_mcp_server.py` 5 sites in `TestCreateModel` / `test_create_from_query_*`: payloads used `"fields"` (v1) inside the `query` arg. They worked via the v1→v2 migration and `extra="allow"` on the request shell, but the tests aren't *about* migration — they're about `create_model(query=...)` routing. Renamed to `"measures"` for clarity. Also dropped a stray `"dry_run": True` from the cache-population test (v3 strips it via migration anyway, and the test isn't exercising dry-run behaviour). Items left intentionally untouched per the DEV-1319 audit: - `tests/test_api_server.py:211` (`test_request_legacy_fields_payload_migrates`) — explicit forward-compat / migration coverage. - `tests/test_migrations.py` — migration coverage, must keep v1 literals. - `slayer/storage/v2_migration.py`, `v3_migration.py` — the converters. - `docs/blog_0_2_release.md` — historical 0.2-era post. Verification: ruff clean; 1264 unit tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- `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 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 #53. Grep-and-update of every user-facing surface that drifted across:
dimensions+measuresmerged intocolumns;measuresrepurposed as named formulas;SlayerQuery.fields→measures;SlayerQuery.model→source_model.dry_run/explainremoved fromSlayerQuery(nowengine.execute()kwargs);SlayerQueryv3 setsextra="forbid".engine.execute.Findings came from three parallel
Exploreagent passes plus an independentcodexreview, then deduplicated and verified against the actual code (slayer/core/{models,query,formula}.py,slayer/engine/enrichment.py, integration tests). One spec-vs-code drift surfaced and was reconciled in favour of the code (see below).Notable corrections
*:countresult-key:CLAUDE.mdand 5 docs claimedorders.count, but the engine emitsorders._count— pinned byslayer/core/formula.py:88,slayer/engine/enrichment.py, and integration teststest_integration_duckdb.py:106,126,.... Fixed the docs to match the code.docs/examples/01_dynamic/dynamic.md:revenue:quantile(0.25)was never a valid built-in. Changed torevenue:percentile(p=0.25).docs/concepts/models.mdforward-tolerance: reframed for v3'sSlayerQueryextra="forbid"..claude/skills/slayer-models.md: cache-refresh claim was correct ~3 weeks ago but stale after PR Remove read-path cache writes for query-backed models (closes #74) #79; corrected to "save paths only".08_models.md: splitDimensions/Measuressections collapsed into one unifiedColumnssection, with a separateNamed-Formula Measuressection.docs/CLAUDE.md):.claude/skills/slayer-query.mdandREADME.mdno longer use Python class constructors (ColumnRef(...),Field(...)); they're now JSON/dict syntax.--models-dir→--storageeverywhere user-facing. Added missing--demorow toslayer serve/slayer mcpflag tables indocs/interfaces/cli.md.examples/embedded/{run,verify}.py,examples/verify_common.py): rewritten to v3 —SlayerQuery(measures=[...]), dict-form dimensions/order, dropped the deadFieldimport, fixedname="rank"shadowing the reserved transform.multistage_queries_nb.ipynbhad the same v1dimensionsinsideModelExtensionas its sibling.md— fixed both.Test plan
poetry run ruff check slayer/ tests/ examples/— clean.poetry run pytest -m "not integration"— 1264 passed, 0 failed in 4m 9s.poetry run python examples/embedded/run.py— all 12 demo queries execute without error.poetry run python examples/embedded/verify.py— 41 PASS, 0 FAIL.grep -rnE '"fields":|fields=\[' docs/ .claude/skills/ README.md slayer/help/topics/ examples/grep -rn '"model":' docs/ README.md examples/*/README.md(excludingdata_source/source_model)grep -rnE 'orders\.count\b' docs/ README.md .claude/skills/ CLAUDE.md slayer/help/topics/ examples/grep -rn -- '--models-dir' docs/ examples/*/README.md README.md(excluding the deprecation note)postgres,mysql,clickhouse) not exercised locally —verify_common.pywas rewritten to v3, but spinning up Docker Compose stacks was out of scope.Out of scope (worth a follow-up)
tests/test_format_propagation.py:1303-1304mocks useorders.count— self-consistent mock data, doesn't break tests, but contradicts the convention.examples/{clickhouse,mysql,postgres}/start.shstill pass--models-dir(deprecated flag still works at runtime).time_shift(*:count, -1)produces an off-by-one for some calendar months in the embedded demo data — engine quirk, not docs.🤖 Generated with Claude Code
Summary by CodeRabbit