Skip to content

Better models doc#138

Merged
AivanF merged 2 commits into
mainfrom
aivan/2026-05-21-better-models-doc
May 21, 2026
Merged

Better models doc#138
AivanF merged 2 commits into
mainfrom
aivan/2026-05-21-better-models-doc

Conversation

@AivanF
Copy link
Copy Markdown
Collaborator

@AivanF AivanF commented May 21, 2026

Less text yet more helpful details

Summary by CodeRabbit

  • Documentation
    • Restructured and clarified model concepts with new overview and quick-reference tables
    • Added guidance on column/SQL expression behaviors, window-function handling, and JSON extraction semantics
    • Enhanced metrics docs to distinguish measures from aggregations and clarify naming/expansion rules
    • Expanded joins, model filters, and query-backed model guidance (staging, caching, and naming conventions)
    • Added schema versioning notes and migration/compatibility guidance

Review Change Stack

@AivanF AivanF requested a review from ZmeiGorynych May 21, 2026 12:08
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 145c9a89-ae1a-4e88-95d1-742dcfd2e8c2

📥 Commits

Reviewing files that changed from the base of the PR and between e8f290a and 789506e.

📒 Files selected for processing (1)
  • docs/concepts/models.md

📝 Walkthrough

Walkthrough

Rewrites and reorganizes docs/concepts/models.md: adds a Models overview and fields table, clarifies Column.sql rules and JSON semantics, disambiguates measures vs aggregations, updates joins and model-filter semantics with __ aliases, and expands query-backed model behavior, naming, caching, and schema-versioning guidance.

Changes

Models concept documentation

Layer / File(s) Summary
Models overview and column structure
docs/concepts/models.md
Introduced a structured "Models" overview with a "Fields at a glance" reference table, formalized "Source modes," and reorganized the "Columns" section to document filtered-column behavior and joined-column dot references.
Column.sql semantics and measures/aggregations reference
docs/concepts/models.md
Added "Column.sql" guidance covering window-function rejection, SQL expression conventions (bare vs qualified names, __ alias usage), and SQLite JSON extraction semantics. Reworked measures content with "Measures vs aggregations", a "Measures (named formulas)" schema table and measure namespace/expansion rules; rebuilt "Aggregations" with built-in references, *:count constraints, first/last semantics, and custom aggregation details including {expr} templates and meta.
Joins and model filters
docs/concepts/models.md
Clarified joins to emphasize cross-model measures and path-based __ table aliases for multi-hop/diamond joins. Replaced model-filter guidance with Mode A-only SQL rules, rejecting DSL constructs and documenting automated conversion of multi-hop joined references to __ with warnings.
Query-backed models, naming conventions, and schema versioning
docs/concepts/models.md
Expanded query-backed model docs: stage DAG constraints, two usage modes, {var} precedence (runtime → stage → outer → model defaults), caching rules for columns and backing_query_sql (user-supplied cached fields rejected), query-derived naming with __, explicit measure name override behavior, result-column key formatting, and schema-versioning/v2→v3 notes plus guidance for keeping persisted models synced with live schema.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • MotleyAI/slayer#81: Documentation-only updates related to model/measure summaries and returned columns/measures/joins.
  • MotleyAI/slayer#25: Related docs changes around model/measure syntax and cross-model aggregation naming (revenue:sum, *:count).
  • MotleyAI/slayer#24: Overlapping updates clarifying measures vs aggregations and query-time aggregation syntax.

Suggested reviewers

  • ZmeiGorynych

Poem

🐰 I hopped through lines of docs today,
Rearranged the fields along the way,
Measures, joins, and SQL rules aligned,
Query stages mapped and names defined,
A tidy garden of models—bright and gay.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Better models doc' is vague and generic, using non-descriptive terms that don't convey specific information about what was improved in the models documentation. Consider a more specific title that highlights the key improvement, such as 'Reorganize models documentation for clarity and modularity' or 'Restructure models doc with tables and expanded guidance'.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch aivan/2026-05-21-better-models-doc

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread docs/concepts/models.md Outdated
# Models

A model maps a database table (or SQL subquery) to queryable **columns** and **measures**. Models are defined as YAML files or created via the API/MCP.
A model is SLayer's view of a database table. It declares the columns, named metric formulas, joins, and always-applied filters that queries can build on. Models are defined as YAML (one file per model under `models/<data_source>/`) or created via API/MCP — the two paths produce the same persisted object.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning that this could also be based on a sql query

Comment thread docs/concepts/models.md
filter: "status = 'completed'"
```

`active_revenue:sum` then generates `SUM(CASE WHEN status = 'active' THEN amount END)`. The filter does nothing when the column is used as a group-by dimension — it fires only inside aggregations.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that true? I don't think we want it to be have like that, no?
For me, a filter is just syntactic sugar for case when

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when a Column.filter column is used as a dimension, the filter is silently dropped. EnrichedDimension doesn't even carry a filter_sql field (see slayer/engine/enriched.py:31), so by construction it can't propagate. The dimension build path (generator.py:1313) just resolves dim.sql raw.

Dry-run proof:

-- Model: column `active_status` has sql=status, filter="status = 'active'"
-- Query: dimensions=["active_status"]
SELECT
  orders.status AS "orders.active_status"
FROM orders AS orders
GROUP BY
  orders.status

No CASE WHEN, no WHERE. Every status value shows up.

If you want, we can change that behaviour, but let's make it in a separate PR as I now just wanna quickly update the doc.

Comment thread docs/concepts/models.md Outdated
| `string` | count, count_distinct, first, last, min, max |
| `boolean` | count, count_distinct, sum, min, max, first, last |
| `date` / `time` | count, count_distinct, first, last, min, max |
Bare column names (`"amount"`) auto-qualify against the model's table at query time. For cross-column arithmetic in the same model, prefix with the model name: `"orders.amount * orders.quantity"`. For joined columns inside a model's own `sql`, use the `__` alias form (`customers__regions.name`) — see [Joins](#joins).
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary, source model is assumed

@sonarqubecloud
Copy link
Copy Markdown

@AivanF AivanF merged commit a8b8f2c into main May 21, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants