DEV-1608: Cube → SLayer ingestion (Stage 1) - #211
Open
ZmeiGorynych wants to merge 10 commits into
Open
Conversation
Add `slayer/cube/` — an offline importer that converts Cube (Cube.js / Cube.dev) YAML data models into persisted SLayer models, mirroring the existing `slayer/dbt/` importer. Exposed via a new `slayer import-cube` CLI. Conversion is fully offline (types come from Cube's declared dimension/ measure types). Everything that can't map cleanly is recorded in a structured `CubeConversionReport` (also written to `cube_import_report.json`) rather than silently dropped. Mapping: - cube → table-owning model; view → facade model (re-exports members as derived columns + local/cross-model ModelMeasures on the join_path root). - measures → Column + ModelMeasure split (count_distinct_approx → count_distinct; conditional filters → Column.filter with a filter-aware dedup key; finite trailing rolling_window → windowed aggregation; calculated number/string/time/boolean measures → ModelMeasure formula). - dimensions → typed columns (case dim → CASE WHEN); joins → join_pairs with member→physical-column resolution (non-equi/non-column ON reported); segments → boolean columns; `extends` flattened (abstract bases hidden). - no-SLayer-home features (pre_aggregations, refresh_key, calendar, hierarchies, drill_members, access_policy, sql_alias, geo, sub_query, custom granularities) reported + stashed under meta.cube_unmapped. - Tesseract features (switch, number_agg, case measures, measure filter) deferred to a follow-up (DEV-1610 tracks native inheritance). Includes a namespace allocator + offline sqlglot/formula validation so a broken member is reported, never crashes whole-model construction. Docs: docs/cube/cube_import.md (+ mkdocs nav). Co-Authored-By: Claude Opus 4.8 (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:
📝 WalkthroughWalkthroughChangesCube Import Pipeline
List Variable Substitution
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Quality-gate blockers: - refs.py: rewrite the join-ON splitter from `\s+AND\s+` to `\bAND\b` (removes the polynomial-backtracking shape Sonar S5852 flagged). - cli.py: NOSONAR(S8707) on the report-file write — the path is an intended, user-specified CLI output location. Codex correctness: - Map Cube `public: false` dimensions/segments to `Column.hidden`, and skip private members when a view uses `includes: "*"`. - SQL-escape default-filter literal values (single quotes doubled). Sonar maintainability: - `*:count` literal → `_STAR_COUNT` constant; drop unnecessary `list()`; `dict.fromkeys`; validate the `mode` arg (was unused); fix a noqa-comment syntax; replace unused test locals with `_`. - Reduce cognitive complexity of `parse_cube_project` / `_strip_jinja_members` / `_member_has_jinja` / `translate_cube_refs` / `_run_import_cube` by extracting helpers. Adds tests for the hidden-mapping, includes-"*" privacy filter, filter escaping, and mode validation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 12
🧹 Nitpick comments (1)
slayer/cli.py (1)
1477-1486: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider isolating
save_modelfailures so the report is still emitted.The converter routes invalid members into the report and aims to never crash, but
storage.save_modelruns save-time validation (e.g. derived-column cycle detection) that can still raise on a converted model. A single failing model here aborts the loop before_write_cube_report/the summary run, leaving partially-saved state and no report — the opposite of the importer's "report, don't crash" contract. Wrapping the per-model save in a try/except that records aparse_error/save issue would keep the run resilient.🤖 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/cli.py` around lines 1477 - 1486, The per-model persistence loop in the import flow should not let a single `storage.save_model` failure abort the entire run, because that prevents `_write_cube_report` and the final summary from running. Update the loop around `run_sync(storage.save_model(model))` to catch save-time exceptions, record the failure as a report issue/`parse_error` on the relevant model or report object, and then continue saving the remaining models. Keep the existing `_print_cube_import_summary` and `_write_cube_report` calls in place so the importer still emits a report even when one model fails.
🤖 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 `@CUBE_IMPORT_SPEC.md`:
- Around line 21-30: Update the Markdown in CUBE_IMPORT_SPEC.md to satisfy the
fence-lint issue by adding explicit language tags to the unlabeled fenced blocks
in the spec and removing the stray standalone fence near the end of the file.
Focus on the fenced sections around the referenced diagram/spec blocks and the
empty fence at the EOF so the rendered document and MD040 checks are both
correct.
In `@docs/cube/cube_import.md`:
- Around line 82-86: The joined-measure example in the Cube import spec uses the
cube measure name instead of the underlying column, which conflicts with the
facade contract. Update the Cube view documentation in the import spec so the
`ModelMeasure` example references `<joinpath>.<underlying_column>:<agg>` rather
than `customers.ltv:sum`, using the relevant Cube import docs section to keep
the terminology aligned with how `join_path` and included measures are
described.
In `@slayer/cli.py`:
- Around line 1507-1510: The report path logic in the import flow is only using
args.storage, so it can miss args.models_dir and place cube_import_report.json
in the wrong location. Update the report-path resolution near the storage_base
handling to follow the same fallback order as _resolve_storage (args.storage,
then args.models_dir, then _STORAGE_DEFAULT), and keep the .db dirname
normalization consistent so the report is written alongside the resolved storage
backend.
In `@slayer/cube/converter.py`:
- Around line 332-365: Keep the measure tracking state in sync with what
actually gets emitted: `_convert_calc_measure()` should record calculated
measures as `kind="calc"` when `_emit_measure()` succeeds, and
`_convert_agg_measure()` should only populate `_MeasureInfo` after a successful
emit. Update `_emit_measure()` to return success/failure so callers like
`_convert_calc_measure()`, `_convert_agg_measure()`, and `_facade_measure()` can
avoid keeping stale entries when validation or emission drops a measure. After
`_validate_offline()`/related pruning, remove any `_measure_info` entries for
measures that were not retained so view re-exports stay consistent with the
source model.
- Around line 416-428: The join conversion logic in _convert_joins currently
allows a ModelJoin to be created even when cj.name does not exist in
self._cubes, because _resolve_join_pairs() can still return pairs based on the
raw target member. Add a target-cube existence check before appending the join:
if cj.name is missing from self._cubes, report a CubeConversionIssue for the
unsupported join and skip it. Keep the fix localized to _convert_joins and, if
needed, _resolve_join_pairs so only joins targeting real cubes are persisted.
- Around line 290-298: The CASE SQL builder is embedding raw labels into
single-quoted SQL, which breaks on apostrophes and other special characters.
Update `_build_case_sql()` in `converter.py` to pass each `when["label"]` and
the `else["label"]` through `_sql_str_literal()` before appending them. Keep the
existing `translate_cube_refs()` flow intact and ensure the `CASE`, `WHEN`,
`THEN`, and `ELSE` fragments still assemble the same way.
In `@slayer/cube/extends.py`:
- Around line 53-65: The cycle handling in `resolve()` for
`flatten_cube_extends()` only stops at the node that detects the loop, so parent
frames still merge that partially resolved object and continue inheritance.
Update the `resolve()` logic to propagate a cycle-failure state back through the
entire call chain for all nodes in the cycle, so none of them get merged, and
apply the same fix in `flatten_view_extends()`; keep `EXTENDS_CYCLE` reporting
on the detected cycle while ensuring `converter.py` receives fully flattened
results with no inherited members from cyclic chains.
In `@slayer/cube/models.py`:
- Around line 102-114: `CubeViewCubeRef` currently models only flat `alias`,
`title`, and `description` fields, but the view spec expects per-member override
payloads and also includes `format` and `meta`; expand this model so it can
preserve the documented override shape instead of dropping it through Pydantic.
Update `CubeViewCubeRef` in `models.py` to add the missing override fields and
represent member-specific overrides in a way the converter can inspect later,
while keeping the existing `join_path`, `includes`, `excludes`, and `prefix`
behavior intact.
In `@slayer/cube/parser.py`:
- Around line 88-102: The _load_yaml helper currently only handles
yaml.YAMLError, so unreadable files can still crash import before a
CubeConversionIssue is recorded. Update _load_yaml to catch OSError from the
open(path, encoding="utf-8") read path and treat it like a non-fatal parse
failure by appending a warning issue with CubeIssueCategory.PARSE_ERROR (or
templating category only if raw text is available and contains_jinja applies),
then return None. Keep the behavior aligned with the existing _load_yaml and
CubeConversionIssue flow so malformed and unreadable YAML are reported the same
way through the parser/CLI contract.
In `@slayer/cube/refs.py`:
- Around line 11-12: The literal-skipping regex in refs.py is too naive and
breaks on SQL strings with doubled single quotes, so placeholders inside valid
literals can be rewritten. Update the literal handling in _LITERAL_RE (and any
related parsing in refs.py) to correctly match SQL string literals with escaped
quotes like 'can''t {CUBE}', and ensure _REF_RE substitution still ignores text
inside those literals. Verify the resulting SQL remains valid for the
rewrite/validation path used by converter.py’s rewrite-and-validate flow so
legitimate Cube models are not downgraded to COMPLEX_SQL.
In `@tests/fixtures/cube_project/model/cubes/malformed.yml`:
- Around line 1-5: The malformed cube fixture is currently failing earlier on
schema validation because it has a sql_table but no name, so it never reaches
the CubeCube/_cube_source NO_SOURCE path in converter.py. Update the fixture to
use a valid cube name and remove the source-related fields (for example, the
sql_table entry) so the converter can instantiate the cube and report
CubeIssueCategory.NO_SOURCE as intended.
In `@tests/test_cube_views.py`:
- Around line 254-256: The test is too permissive because it accepts a
disconnected-view failure when this case is specifically about the root cube not
being emitted. Update the assertion in the cube views test to check only for the
root-specific issue category, using the existing report.issues check and the
CubeIssueCategory.AMBIGUOUS_VIEW_ROOT symbol, and remove the fallback to
CubeIssueCategory.DISCONNECTED_VIEW so the test enforces the intended
root-resolution behavior.
---
Nitpick comments:
In `@slayer/cli.py`:
- Around line 1477-1486: The per-model persistence loop in the import flow
should not let a single `storage.save_model` failure abort the entire run,
because that prevents `_write_cube_report` and the final summary from running.
Update the loop around `run_sync(storage.save_model(model))` to catch save-time
exceptions, record the failure as a report issue/`parse_error` on the relevant
model or report object, and then continue saving the remaining models. Keep the
existing `_print_cube_import_summary` and `_write_cube_report` calls in place so
the importer still emits a report even when one model fails.
🪄 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: b8dd63f0-00f5-486f-a578-da03875c3ea2
📒 Files selected for processing (27)
CUBE_IMPORT_SPEC.mddocs/cube/cube_import.mdmkdocs.ymlslayer/cli.pyslayer/cube/__init__.pyslayer/cube/converter.pyslayer/cube/extends.pyslayer/cube/models.pyslayer/cube/parser.pyslayer/cube/refs.pyslayer/cube/report.pytests/fixtures/cube_project/model/cubes/customers.ymltests/fixtures/cube_project/model/cubes/events.ymltests/fixtures/cube_project/model/cubes/for_loop.ymltests/fixtures/cube_project/model/cubes/malformed.ymltests/fixtures/cube_project/model/cubes/orders.ymltests/fixtures/cube_project/model/cubes/templated_member.ymltests/fixtures/cube_project/model/views/orders_overview.ymltests/test_cube_boundaries.pytests/test_cube_cli.pytests/test_cube_converter.pytests/test_cube_extends.pytests/test_cube_parser.pytests/test_cube_refs.pytests/test_cube_report.pytests/test_cube_smoke.pytests/test_cube_views.py
- refs.py: extract `_equality_pair` from `parse_join_on` to bring its cognitive complexity under the threshold (Sonar S3776). - CUBE_IMPORT_SPEC.md: add fence languages to code blocks and drop the stray trailing fence (CodeRabbit MD040). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Spell out that a facade re-exports a joined measure via its *underlying column* (`customers.amount:sum`), not the Cube measure name — matching the converter's facade contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`_write_cube_report` now resolves its base via `args.storage or args.models_dir or _STORAGE_DEFAULT`, matching `_resolve_storage`, so a `--models-dir`-only invocation writes the report next to the models as documented. Adds a CLI test for the parity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Correctness:
- extends: detect the full extends cycle up front so no node on the cycle
inherits (previously only the closing frame was un-merged, letting ancestors
still merge a cyclic node). Applies to cubes and views.
- converter: reject joins whose target cube isn't in the project (was
persisting a ModelJoin to a non-existent model); keep _measure_info in sync
with emitted measures (record calc measures as kind="calc", record only on
successful emit, prune after offline validation) so view facades never
re-export a measure the model no longer has.
- refs: doubled-quote-aware SQL string literal regex (`'can''t {CUBE}'` no
longer leaks `{CUBE}` to the translator); escape CASE labels via
_sql_str_literal.
- parser: catch OSError in _load_yaml (broken symlink / permission) and report
it like a malformed file instead of aborting.
- models/converter: CubeViewCubeRef.includes accepts Cube's per-member override
object form (was crashing the view parse); overrides are reported as
unsupported (Stage 1) rather than silently dropped. Dropped the dead flat
title/description fields.
- cli: isolate per-model save_model failures so the report is still written.
Tests for each; also tightened the root-not-emitted view assertion to the
exact AMBIGUOUS_VIEW_ROOT category.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
…ion-in-mode-a-raw-sql-surfaces' into egor/dev-1608-cube-to-slayer-ingestion
…-slayer-ingestion # Conflicts: # mkdocs.yml # slayer/cli.py
Accept list/tuple variable values in _render_variable_value via a new
_render_list_value helper, rendering an injection-safe IN-list body. One
branch at the single choke point covers every consumer (Mode-A engine pass,
Mode-B enrichment, get_column_types defaults probe) with no schema change.
- escape="sql": comma-joined, string elements auto-quoted + quote-doubled
(author writes `col IN ({var})` parens, no per-element quotes).
- escape="python": trailing comma so the Mode-B Python-AST parser reads a
tuple even for a single-element list.
- Empty list raises (IN () is invalid SQL; "no filter" belongs to a sentinel
default). Non-scalar / non-finite elements raise, naming the variable.
- Per-element escaping reuses _escape_string_value, so DEV-1727's
dialect-aware escaping composes automatically.
Tests: unit render (both modes), error cases, all-four Mode-A surfaces,
Mode-A/Mode-B end-to-end against SQLite, injection, precedence layers, and
get_column_types list default. Flipped the old "lists raise" assertion.
Docs: models.md / queries.md (IN example + trailing-comma note), CLAUDE.md,
slayer-query / slayer-models skills, DECISIONS.md (quoting asymmetry +
empty-list rationale).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
slayer/cli.py (1)
1569-1585: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReport save failures with an accurate category and count.
Two points in the save loop:
- A persistence failure is recorded as
CubeIssueCategory.PARSE_ERROR. The JSON report then labels a storage error as a parse error, which misleads anyone triaging the report. Add a dedicated category, for exampleSAVE_FAILED, inslayer/cube/report.py.result.report.model_countis set by the converter before saving. When a save fails, the final line still reports the full model count as done. Track the number of models actually saved and print that.🔧 Proposed change
from slayer.cube.report import CubeConversionIssue, CubeIssueCategory + saved = 0 for model in result.models: try: run_sync(storage.save_model(model)) + saved += 1 except Exception as exc: # noqa: BLE001 — one bad model shouldn't abort the import result.report.add(CubeConversionIssue( - category=CubeIssueCategory.PARSE_ERROR, severity="error", + category=CubeIssueCategory.SAVE_FAILED, severity="error", cube=model.name, message=f"Failed to save model '{model.name}': {exc}")) _print_cube_import_summary(result, include_hidden=args.include_hidden) report_path = _write_cube_report(result, args) print( - f"\nDone: {result.report.model_count} models " + f"\nDone: {saved} of {result.report.model_count} models saved " f"({result.report.hidden_count} hidden, {result.report.view_count} views), " f"{len(result.report.issues)} report issues. Report: {report_path}" )🤖 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/cli.py` around lines 1569 - 1585, Update CubeIssueCategory in the report definitions to add a dedicated SAVE_FAILED category, then use it in the run_sync(storage.save_model(model)) exception path instead of PARSE_ERROR. In the save loop, track a count of models successfully persisted, incrementing only after save_model succeeds, and use that count in the final “Done” summary instead of result.report.model_count.slayer/cube/converter.py (1)
611-642: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport
includesobjects without a validnameas parse errors. Cube requiresname, butCubeViewCubeRefaccepts arbitrary dictionaries.{}is dropped without an issue, and{"alias": ...}reportsmember=None. Reject these entries before conversion or report them as parse errors.🤖 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/cube/converter.py` around lines 611 - 642, Update _include_names to validate dictionary entries before processing overrides: require a non-empty name, and report entries missing or having an invalid name as parse errors instead of dropping them or using member=None. Only apply the existing unsupported-override reporting and exclusion logic after valid names are confirmed.docs/concepts/models.md (1)
391-402: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDo not describe SQL list substitution as injection-safe or dialect-aware.
SQL-mode escaping doubles quotes but leaves backslashes unchanged. On backslash-escaping SQL dialects, a backslash before a quote can break the literal boundary. The supported contract is trusted input with non-dialect-aware escaping.
docs/concepts/models.md#L391-L402: Replace “injection-safe” with the trusted-input contract..claude/skills/slayer-models.md#L91-L91: Add the trusted-input and dialect caveat..claude/skills/slayer-query.md#L79-L79: Add the trusted-input and dialect caveat.CLAUDE.md#L56-L56: Remove the injection-safe claim.DECISIONS.md#L68-L68: State that future dialect-aware escaping will compose through the shared helper.docs/concepts/queries.md#L272-L290: State that quote escaping is not a dialect-independent injection defense.🤖 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 `@docs/concepts/models.md` around lines 391 - 402, Replace the “injection-safe” and dialect-aware SQL list-substitution claims with the trusted-input, non-dialect-aware escaping contract: document that quote doubling leaves backslashes unchanged and may permit literal breakouts on backslash-escaping dialects. Apply this wording to docs/concepts/models.md lines 391-402, .claude/skills/slayer-models.md line 91, .claude/skills/slayer-query.md line 79, and remove the injection-safe claim from CLAUDE.md line 56; update DECISIONS.md line 68 to state that future dialect-aware escaping will compose through the shared helper, and docs/concepts/queries.md lines 272-290 to clarify quote escaping is not a dialect-independent injection defense.Source: Coding guidelines
🧹 Nitpick comments (7)
tests/test_cube_converter.py (2)
317-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this dimension test out of the joins section.
test_case_dimension_label_escapes_quotestests CASE label escaping. It sits after the── 4.4 joins ──header at Line 285, between two join tests. Move it up next totest_case_dimension_becomes_case_when_columnat Line 233.🤖 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 `@tests/test_cube_converter.py` around lines 317 - 327, Move test_case_dimension_label_escapes_quotes from the joins test section to immediately alongside test_case_dimension_becomes_case_when_column in the CASE dimension tests, without changing its implementation or assertions.
243-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the composite assertion.
SonarCloud flags this line. With a single combined assertion, a failure does not show which clause failed. Use separate assertions.
💚 Proposed fix
- assert "CASE WHEN" in sql and "'small'" in sql and "'big'" in sql + assert "CASE WHEN" in sql + assert "'small'" in sql + assert "'big'" in sql🤖 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 `@tests/test_cube_converter.py` at line 243, Split the composite assertion in the cube converter test into separate assertions, independently checking that sql contains “CASE WHEN”, “‘small’”, and “‘big’”.Source: Linters/SAST tools
slayer/cli.py (1)
1569-1569: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGroup the report import with the other function-local imports.
slayer/cube/reportis imported in the middle of the function body. Move it next to the converter and parser imports at Lines 1556-1557 so all deferred imports for this command sit together.As per coding guidelines: "Place imports at the top of files."
🤖 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/cli.py` at line 1569, Move the local import of CubeConversionIssue and CubeIssueCategory from the middle of the function to the existing converter and parser imports near the start of that function, keeping all deferred imports for the command grouped together.Source: Coding guidelines
slayer/cube/converter.py (2)
355-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse keyword arguments for the measure-emission calls.
_emit_measuretakes seven positional parameters. The call sites at Lines 355, 364, and 370 pass them all positionally, so an argument-order mistake stays silent. Convertformula,meas,report, andcube_nameto keyword-only parameters, or pass them by keyword at the call sites.As per coding guidelines: "Use keyword arguments for functions with more than one parameter."
Also applies to: 395-395
🤖 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/cube/converter.py` around lines 355 - 366, Update all visible _emit_measure call sites, including the branches around the measure conversion flow and the additional call near line 395, to pass formula, meas, report, and cube.name as keyword arguments; retain the existing positional arguments only where appropriate and preserve behavior.Source: Coding guidelines
368-372: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRelease the reserved name on the calculated-measure failure path.
_convert_measurereservesfinal_nameat Line 336 for both measure shapes._convert_calc_measurecalls_emit_measurewithnames=None, so a failed calculated measure keeps its allocated name. A later member with the same base name then gets an unnecessary suffix. The aggregate path already releases the name. Passnamesthrough for symmetry.♻️ Proposed change
- def _convert_calc_measure(self, cube, meas, measures, final_name, info, report) -> None: + def _convert_calc_measure(self, cube, meas, measures, names, final_name, info, report) -> None: formula = translate_cube_refs(meas.sql, mode="dsl", cube=cube.name) - if self._emit_measure(measures, None, final_name, formula, meas, report, cube.name, + if self._emit_measure(measures, names, final_name, formula, meas, report, cube.name, result_type=_DIM_TYPE_MAP.get(meas.type)): info[meas.name] = _MeasureInfo(kind="calc", emitted_name=final_name)Update the call site at Line 338 accordingly.
Also applies to: 395-408
🤖 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/cube/converter.py` around lines 368 - 372, Update _convert_calc_measure to accept the reserved names collection and pass it to _emit_measure instead of None, matching the aggregate-measure path so failed calculated measures release final_name and later members can reuse the base name.slayer/cube/extends.py (1)
65-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one shared flattening helper.
flatten_cube_extendsandflatten_view_extendsnow differ only in the merge function and the issue field (cube=vsview=). A single generic helper that takes the merge callable and the issue factory would remove the duplicated cycle reporting, memoization, and resolve closure. This is optional; the current form is correct.Also applies to: 108-126
🤖 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/cube/extends.py` around lines 65 - 90, Optionally consolidate flatten_cube_extends and flatten_view_extends around one generic flattening helper that accepts the type-specific merge callable and issue factory. Move shared cycle detection, issue collection, memoization, and recursive resolve logic into that helper, while preserving cube/view-specific issue fields and existing flattening behavior.tests/test_cube_parser.py (1)
106-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test name with the covered behavior.
The test creates only a hidden directory. It does not create a
targetdirectory, so the "target" part of the name is not covered. Rename the test, or add atarget/YAML file and assert it is skipped.♻️ Suggested rename
-def test_hidden_dirs_and_target_skipped(tmp_path): +def test_hidden_dirs_skipped(tmp_path):🤖 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 `@tests/test_cube_parser.py` around lines 106 - 111, Rename test_hidden_dirs_and_target_skipped to reflect that it only verifies hidden-directory skipping, or add a target directory containing a YAML cube and assert that cube is also excluded; ensure the test name matches the behaviors it actually covers.
🤖 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/cube/parser.py`:
- Around line 90-92: Update _load_yaml’s file-reading exception handling to
catch UnicodeDecodeError alongside OSError, ensuring invalid UTF-8 is converted
into the existing PARSE_ERROR warning path instead of aborting the import.
---
Outside diff comments:
In `@docs/concepts/models.md`:
- Around line 391-402: Replace the “injection-safe” and dialect-aware SQL
list-substitution claims with the trusted-input, non-dialect-aware escaping
contract: document that quote doubling leaves backslashes unchanged and may
permit literal breakouts on backslash-escaping dialects. Apply this wording to
docs/concepts/models.md lines 391-402, .claude/skills/slayer-models.md line 91,
.claude/skills/slayer-query.md line 79, and remove the injection-safe claim from
CLAUDE.md line 56; update DECISIONS.md line 68 to state that future
dialect-aware escaping will compose through the shared helper, and
docs/concepts/queries.md lines 272-290 to clarify quote escaping is not a
dialect-independent injection defense.
In `@slayer/cli.py`:
- Around line 1569-1585: Update CubeIssueCategory in the report definitions to
add a dedicated SAVE_FAILED category, then use it in the
run_sync(storage.save_model(model)) exception path instead of PARSE_ERROR. In
the save loop, track a count of models successfully persisted, incrementing only
after save_model succeeds, and use that count in the final “Done” summary
instead of result.report.model_count.
In `@slayer/cube/converter.py`:
- Around line 611-642: Update _include_names to validate dictionary entries
before processing overrides: require a non-empty name, and report entries
missing or having an invalid name as parse errors instead of dropping them or
using member=None. Only apply the existing unsupported-override reporting and
exclusion logic after valid names are confirmed.
---
Nitpick comments:
In `@slayer/cli.py`:
- Line 1569: Move the local import of CubeConversionIssue and CubeIssueCategory
from the middle of the function to the existing converter and parser imports
near the start of that function, keeping all deferred imports for the command
grouped together.
In `@slayer/cube/converter.py`:
- Around line 355-366: Update all visible _emit_measure call sites, including
the branches around the measure conversion flow and the additional call near
line 395, to pass formula, meas, report, and cube.name as keyword arguments;
retain the existing positional arguments only where appropriate and preserve
behavior.
- Around line 368-372: Update _convert_calc_measure to accept the reserved names
collection and pass it to _emit_measure instead of None, matching the
aggregate-measure path so failed calculated measures release final_name and
later members can reuse the base name.
In `@slayer/cube/extends.py`:
- Around line 65-90: Optionally consolidate flatten_cube_extends and
flatten_view_extends around one generic flattening helper that accepts the
type-specific merge callable and issue factory. Move shared cycle detection,
issue collection, memoization, and recursive resolve logic into that helper,
while preserving cube/view-specific issue fields and existing flattening
behavior.
In `@tests/test_cube_converter.py`:
- Around line 317-327: Move test_case_dimension_label_escapes_quotes from the
joins test section to immediately alongside
test_case_dimension_becomes_case_when_column in the CASE dimension tests,
without changing its implementation or assertions.
- Line 243: Split the composite assertion in the cube converter test into
separate assertions, independently checking that sql contains “CASE WHEN”,
“‘small’”, and “‘big’”.
In `@tests/test_cube_parser.py`:
- Around line 106-111: Rename test_hidden_dirs_and_target_skipped to reflect
that it only verifies hidden-directory skipping, or add a target directory
containing a YAML cube and assert that cube is also excluded; ensure the test
name matches the behaviors it actually covers.
🪄 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: 144ea645-d3d3-49b2-a9b7-b449e4f36097
📒 Files selected for processing (22)
.claude/skills/slayer-models.md.claude/skills/slayer-query.mdCLAUDE.mdDECISIONS.mddocs/concepts/models.mddocs/concepts/queries.mdslayer/cli.pyslayer/core/query.pyslayer/cube/converter.pyslayer/cube/extends.pyslayer/cube/models.pyslayer/cube/parser.pyslayer/cube/refs.pytests/test_cube_cli.pytests/test_cube_converter.pytests/test_cube_extends.pytests/test_cube_parser.pytests/test_cube_refs.pytests/test_cube_views.pytests/test_mode_a_variable_substitution.pytests/test_models.pyzensical.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test_cube_extends.py
- tests/test_cube_refs.py
- slayer/cube/refs.py
- tests/test_cube_cli.py
- slayer/cube/models.py
DEV-1730 (list variables): - Escape control chars (newline/CR/tab/NUL) in the python-mode branch of _escape_string_value so a Mode-B string value round-trips through ast.parse instead of producing a broken multiline literal (Codex). Fixes scalars too. - Drop the "injection-safe" claim from the list-variable docs/skills/DECISIONS and align with the established trusted-input, not-dialect-aware contract (CodeRabbit). DEV-1608 (Cube ingestion, pre-existing on this PR): - parser: catch UnicodeDecodeError alongside OSError so an invalid-UTF-8 file becomes a PARSE_ERROR warning instead of aborting the import (CodeRabbit). - cli/report: add a dedicated SAVE_FAILED category and report the actual saved count in the summary instead of the full model count (CodeRabbit). - converter: report view `includes` entries with no valid `name` as parse errors instead of silently dropping them / reporting member=None; pass `names` through the calc-measure path so a failed measure releases its reserved name; use keyword args at the _emit_measure call sites (CodeRabbit). - tests: split the composite assertion (Sonar python:S9073), move the CASE dimension test out of the joins section, and cover target/ dir skipping. Full non-integration suite green; ruff clean. Co-Authored-By: Claude Fable 5 <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.
Stage 1 of Cube (Cube.js / Cube.dev) YAML ingestion. Adds
slayer/cube/— an offline importer that converts Cube data models into persisted SLayer models, mirroring the existingslayer/dbt/importer, exposed via a newslayer import-cubeCLI. Full spec lives in the DEV-1608 issue body.Approach
CubeConversionReport(written tocube_import_report.json), never silently dropped.Mapping
ModelMeasures anchored on thejoin_pathroot; mirrors the root cube's source mode).Column+ModelMeasuresplit:count_distinct_approx→count_distinct; conditionalfilters→Column.filterwith a filter-aware dedup key; finite trailingrolling_window→windowed aggregation; calculatednumber/string/time/booleanmeasures→ModelMeasureformula.case→CASE WHEN); joins →join_pairswith member→physical-column resolution (non-equi/non-column ON reported); segments → boolean columns;extendsflattened (abstract bases emitted hidden).pre_aggregations,refresh_key,calendar,hierarchies,drill_members,access_policy,sql_alias,geo,sub_query, customgranularities) reported + stashed undermeta.cube_unmapped.switch,number_agg,casemeasures, measurefilter) deferred to a follow-up. Native model inheritance (the future replacement for flatten) is tracked in DEV-1610.A namespace allocator + offline sqlglot/formula validation ensure a broken member is reported rather than crashing whole-model construction.
Tests
ruffclean.Docs
docs/cube/cube_import.md+mkdocs.ymlnav entry.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
slayer import-cubecommand.IN/NOT INexpressions.Documentation
Bug Fixes