Add curated semantic enrichment to the Jaffle Shop demo#250
Conversation
Auto-ingested demo models were barebones: bare columns, no labels, descriptions, or measures. Mirror the Storyline demo's enrichment, adapted for SLayer's dollars-native loader: - Column labels/descriptions and model descriptions for all 7 models - Currency/percent formats (replacing only the bare auto-ingested INTEGER/FLOAT defaults) - 16 saved measures with labels, descriptions, and result types (total_revenue, avg_order_value, effective_tax_rate, unique_customers, sales_weighted_aov, ...) - A weighted_avg built-in override on orders defaulting the weight to subtotal, as a custom-aggregation example apply_demo_enrichment is additive-only and idempotent (fills only unset fields, merges measures/aggregations by name) and runs on both paths of ensure_demo_datasource — fresh ingest and the reuse fast path (saving only on change), so existing demo installs upgrade on their next --demo startup. The checked-in lightning-talk example store is regenerated with the enrichment (one-time migration; its notebook calls ensure_demo_datasource).
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe Jaffle Shop demo now applies curated, additive semantic enrichment during model ingestion and datasource reuse. Example YAML models and CLI documentation describe the resulting metadata, measures, formats, aggregation, and idempotent behavior. Tests cover enrichment and persistence. Jaffle Shop demo enrichment
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ensure_demo_datasource
participant apply_demo_enrichment
participant ModelStorage
CLI->>ensure_demo_datasource: create demo with optional ingest
ensure_demo_datasource->>apply_demo_enrichment: enrich ingested or reused models
apply_demo_enrichment-->>ensure_demo_datasource: return updated model
ensure_demo_datasource->>ModelStorage: save changed model
ModelStorage-->>CLI: return demo models
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
slayer/demo/jaffle_shop.py (2)
462-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
default_time_dimensionlogic between the caller andapply_demo_enrichment.
ensure_demo_datasource's ingestion loop (lines 858-860) unconditionally setsmodel.default_time_dimensionfromDEFAULT_TIME_DIMENSIONS, andapply_demo_enrichment(lines 462-467) now does the same thing again, guarded by anis Nonecheck. Sinceapply_demo_enrichmentis called right after on the same model, the caller's block is now redundant and the two mechanisms could drift out of sync if one is edited without the other.♻️ Proposed cleanup
for model in models: - if model.name in DEFAULT_TIME_DIMENSIONS: - model.default_time_dimension = DEFAULT_TIME_DIMENSIONS[model.name] apply_demo_enrichment(model)Also applies to: 858-861
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/demo/jaffle_shop.py` around lines 462 - 467, Remove the redundant default_time_dimension assignment from ensure_demo_datasource’s ingestion loop and keep this responsibility in apply_demo_enrichment. Preserve the existing guarded assignment using DEFAULT_TIME_DIMENSIONS in apply_demo_enrichment so each model is enriched consistently in one place.
480-498: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMeasures/aggregations/formats are appended by reference, not copied — shared mutable state across model instances.
DEMO_ENRICHMENTis built once at module import (line 428) and reused for every call.new_measures,new_aggs, andcolumn.format = col_spec.formatall attach the sameModelMeasure/Aggregation/NumberFormatPydantic instances (mutable by default) onto every enriched model. If any code elsewhere ever mutates one of these objects in place (e.g. hoisted-transform naming, per the coding guidelines on hoisted transform aliasing), that mutation would leak across every other model that received the same spec object. Copying defensively avoids this class of bug.🛡️ Proposed fix
- column.format = col_spec.format + column.format = col_spec.format.model_copy() changed = True ... - new_measures = [m for m in spec.measures if m.name not in existing_measures] + new_measures = [m.model_copy() for m in spec.measures if m.name not in existing_measures] ... - new_aggs = [a for a in spec.aggregations if a.name not in existing_aggs] + new_aggs = [a.model_copy() for a in spec.aggregations if a.name not in existing_aggs]Could you confirm (or point to) whether anything downstream ever mutates
ModelMeasure/Aggregation/NumberFormatinstances in place after they're attached to aSlayerModel(e.g. hoisted-transform alias assignment)? If so this becomes a real cross-model contamination bug rather than a defensive-only concern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/demo/jaffle_shop.py` around lines 480 - 498, In the enrichment flow, defensively deep-copy mutable spec objects before attaching them to a SlayerModel. Update new_measures and new_aggs to contain independent copies, and copy col_spec.format before assigning it to column.format in the format-update block. Preserve the existing deduplication and changed-tracking behavior while ensuring no ModelMeasure, Aggregation, or NumberFormat instance is shared across enriched models.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/demo/jaffle_shop.py`:
- Around line 442-500: Reduce cognitive complexity in apply_demo_enrichment by
extracting the column-enrichment loop into a helper and consolidating the named
measures and aggregations merge logic into a reusable helper such as
_merge_named. Preserve the current additive, idempotent behavior and ensure
apply_demo_enrichment still tracks whether any helper changed the model before
returning.
- Around line 442-454: Update apply_demo_enrichment to return False before
looking up or applying DEMO_ENRICHMENT when the model is SQL- or query-backed,
including sql_table, sql, or source_queries configurations. Continue enriching
only table-backed models while preserving the existing idempotent behavior for
eligible models.
---
Nitpick comments:
In `@slayer/demo/jaffle_shop.py`:
- Around line 462-467: Remove the redundant default_time_dimension assignment
from ensure_demo_datasource’s ingestion loop and keep this responsibility in
apply_demo_enrichment. Preserve the existing guarded assignment using
DEFAULT_TIME_DIMENSIONS in apply_demo_enrichment so each model is enriched
consistently in one place.
- Around line 480-498: In the enrichment flow, defensively deep-copy mutable
spec objects before attaching them to a SlayerModel. Update new_measures and
new_aggs to contain independent copies, and copy col_spec.format before
assigning it to column.format in the format-update block. Preserve the existing
deduplication and changed-tracking behavior while ensuring no ModelMeasure,
Aggregation, or NumberFormat instance is shared across enriched models.
🪄 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: 4ea98120-6663-4158-bf1c-8fad9b38dfc5
📒 Files selected for processing (12)
CLAUDE.mddocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/customers.yamldocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/items.yamldocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/orders.yamldocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/products.yamldocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/stores.yamldocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/supplies.yamldocs/examples/09_lightning_talk/slayer_models/models/jaffle_shop/tweets.yamldocs/getting-started/cli.mddocs/reference/cli.mdslayer/demo/jaffle_shop.pytests/test_demo_cli.py
…jects - apply_demo_enrichment skips models not backed by sql_table (DEV-1356 convention: the curated spec assumes the physical jaffle tables) - Extract _enrich_column / _new_named_entries helpers (SonarCloud cognitive-complexity gate) - Deep-copy measures/aggregations/formats when attaching so the module-level DEMO_ENRICHMENT spec can't leak mutations across models - Drop the duplicate default_time_dimension assignment in ensure_demo_datasource (now solely owned by apply_demo_enrichment)
The MBQL source-table browse of orders now returns the 7 physical columns plus the saved measures the demo enrichment adds (the facade catalog exposes measures as queryable fields). Derive the expected count from DEMO_ENRICHMENT instead of hardcoding.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/test_metabase_e2e.py`:
- Line 461: Move the DEMO_ENRICHMENT import out of the test function and place
it with the module-level imports at the top of
tests/integration/test_metabase_e2e.py, leaving the test’s 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: 33b7cd71-fc80-4a3e-9806-95c62d3cd75b
📒 Files selected for processing (1)
tests/integration/test_metabase_e2e.py
|
1 participant
Summary
The stock Jaffle Shop demo ingested barebones models — bare columns, no labels, descriptions, or measures. This PR layers the same kind of curated enrichment the Storyline demo has onto SLayer's bundled demo, adapted for the dollars-native loader (SLayer converts jafgen's cents on load, so no
dollarsaggregation or/ 100.0is needed):subtotal/tax_paid/order_total/price/cost, percent onstores.tax_rate— replacing only the bare auto-ingested INTEGER/FLOAT defaults (DEV-1538 convention), so user-tuned formats survivetype(feeds the Flight wire schema):total_revenue,net_sales,tax_collected,order_count,unique_customers,avg_order_value(withnullifguard),effective_tax_rate,sales_weighted_aovon orders, plus counts/averages on the other modelsweighted_avgbuilt-in override on orders defaulting the weight tosubtotal, so bareorder_total:weighted_avgyields a sales-weighted AOVapply_demo_enrichmentis additive-only and idempotent — labels/descriptions fill only where unset, measures/aggregations merge by name — and runs on both paths ofensure_demo_datasource: fresh ingest and the reuse fast path (saving only when something changed), so pre-enrichment demo installs upgrade on their next--demostartup.The checked-in lightning-talk example store (
docs/examples/09_lightning_talk/slayer_models/) is regenerated with the enrichment — a one-time migration, since its notebook callsensure_demo_datasource; re-runs are no-ops.Testing
tests/test_demo_cli.pycovering idempotency, user-edit preservation, format-override rules, duplicate-measure merging, and fast-path persistence (22 pass)ruff checkcleanSummary by CodeRabbit
New Features
weighted_avgaggregation.Documentation
--ingestbehavior and enrichment details.Tests