DEV-1608: Cube → SLayer ingestion (Stage 1)#211
Open
ZmeiGorynych wants to merge 6 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
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds an offline Cube import pipeline that parses Cube YAML projects, flattens inheritance, converts cubes and views into ChangesCube→SLayer Import Pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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>
|
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
slayer import-cubeto convert Cube YAML cubes and views into persisted SLayer models and generate a JSON conversion report.extendsflattening, view facade model generation, join/segment/default-filter mapping, and structured “report-and-skip” conversion with deferred Stage 2 handling.