DEV-1643: Import OSI (Open Semantic Interchange) configs into SLayer#223
Open
ZmeiGorynych wants to merge 25 commits into
Open
DEV-1643: Import OSI (Open Semantic Interchange) configs into SLayer#223ZmeiGorynych wants to merge 25 commits into
ZmeiGorynych wants to merge 25 commits into
Conversation
Add `slayer import-osi` to convert OSI semantic-model configs (YAML/JSON, file or directory) into SLayer models, mirroring import-dbt's CLI-only shape (parse -> convert -> save_model per model -> printed report). OSI maps directly to SLayer, not via the dbt intermediate. - slayer/osi/: models (schema port), source (source parsing + routing stub), expression (sqlglot -> colon formula, derived-column materialization), converter (OsiToSlayerConverter) - Live table + query-source introspection for real column types; OSI overlays labels/descriptions/is_time/ai_context/primary keys; unique_keys + custom_extensions -> meta - Relationships -> LEFT ModelJoin (composite-safe); metrics -> ModelMeasure anchored via shared join_graph.min_hops_root (also now used by recommend_root_model); orphan COUNT(*) with no unique fact table errors - ModelJoin gains optional description/meta (additive, no version bump) - Extract ConversionResult/ConversionWarning to slayer/ingest_report.py, re-exported from slayer/dbt/converter.py 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:
📝 WalkthroughWalkthroughAdds OSI import support across parsing, conversion, reporting, and CLI wiring, with new OSI schema/source/expression modules, live-introspection model conversion, shared conversion results, updated join selection, docs, fixtures, and tests. ChangesOSI Import Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as slayer import-osi
participant Parser as parse_osi_path
participant Converter as OsiToSlayerConverter
participant Expr as convert_expression
participant Storage as storage.save_model
CLI->>Parser: parse OSI path
Parser-->>CLI: list[OSIDocument]
CLI->>Converter: convert()
Converter->>Converter: build models from datasets
Converter->>Converter: build ModelJoin from relationships
Converter->>Expr: convert metric expression
Expr-->>Converter: ExprResult(formula, materialized)
Converter-->>CLI: ConversionResult
CLI->>Storage: save_model per model
CLI-->>CLI: print report and tally
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- converter: qualified metric refs now verify the column exists on the qualified model (Codex) — SUM(orders.no_such_col) clean-fails instead of importing a query-time-broken measure; test added - converter: reuse core.refs.IDENTIFIER_RE (S6353); drop dead sm_of_dataset dict; extract _build_measures_for and _overlay_one_field to cut cognitive complexity (S3776) - expression: math.isclose for the median 0.5 check (S1244); extract _residual_violation (S3776); list-comprehension instead of list(gen) (S7504) - parser: resolve() the caller-supplied path before filesystem access (S8707) - test: hoist arg construction out of pytest.raises (S5778) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- converter: a derived OSI field whose name matches a physical column now overlays its expression onto that column instead of being silently dropped - expression/converter: materialized hidden-column names are reserved against existing columns (name_taken callback) so a metric never aggregates a colliding pre-existing column - converter: enforce SLayer namespace invariants before the post-construction measure append — duplicate metric names and metric-vs-column collisions clean-fail instead of persisting a model that fails to load - converter: relationship join columns are validated to exist on both models; a typo clean-fails instead of emitting a query-time-broken join - tests for all four Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- converter: bare-alias fields that shadow a physical column now REPLACE it (my prior fix only covered the derived-expression branch) - converter: materialized hidden-column names are reserved against columns AND measures (SLayer shares one namespace), not columns alone - converter: building a ModelMeasure is guarded — a metric named after a reserved transform (cumsum, ...) clean-fails instead of crashing the import, and leaves no orphan hidden columns - converter: an unqualified metric column present on multiple datasets is ambiguous and clean-fails instead of binding by dataset order - tests for all four 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: 5
🧹 Nitpick comments (3)
slayer/osi/parser.py (2)
28-39: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSuffix filter not applied for single-file input.
_collect_filesonly filters by_SUFFIXESwhen walking a directory (Line 37); whenpath.is_file()is true (Line 29-30) any file is accepted regardless of extension. Callingparse_osi_pathdirectly on a non-YAML/JSON file (e.g..txt) will still attempt to parse it as YAML, producing a confusing warning/failure instead of a clear "unsupported file type" message. Directory scanning and single-file scanning should apply the same suffix policy.🛡️ Proposed fix for consistent suffix handling
def _collect_files(path: Path) -> list[Path]: if path.is_file(): - return [path] + return [path] if path.name.endswith(_SUFFIXES) else []Also applies to: 42-44
🤖 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/osi/parser.py` around lines 28 - 39, _collect_files currently returns any single file without checking _SUFFIXES, so update the path.is_file() branch to apply the same suffix gate used in the os.walk loop. Make parse_osi_path’s input handling consistent for both files and directories by rejecting unsupported single-file inputs before parsing, using the _collect_files helper or equivalent suffix check so non-YAML/JSON files are excluded with the same policy.
32-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDirectory traversal order is not fully deterministic across nested subdirectories.
Filenames within a directory are sorted (Line 34), but
dirsorder fromos.walkis left unsorted, so multi-directory imports may collect files in a platform-dependent order. Given the importer's report ordering and model save order likely matter for reproducibility, consider sortingdirsin place as well.♻️ Suggested tweak
for root, dirs, names in os.walk(path): dirs[:] = [d for d in dirs if not d.startswith(".")] + dirs.sort() for name in sorted(names):🤖 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/osi/parser.py` around lines 32 - 39, The file collection logic in the parser’s directory walk is still nondeterministic because os.walk visits nested subdirectories in platform-dependent order. Update the traversal in the parser function that builds the files list to sort the dirs sequence in place before continuing the walk, while keeping the existing filename filtering and sorting behavior, so imports from multiple subdirectories are collected consistently across runs.slayer/osi/models.py (1)
9-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring inconsistency:
extra="allow"contradicts "extra=ignore everywhere" claim.The module docstring states "extra="ignore" everywhere (forward/back-compat across spec versions)." but
OSIAIContextObject(Line 36) setsmodel_config = ConfigDict(extra="allow"). With class-levelextra="allow", unknown keys become real attributes and are included bymodel_dump()(not silently dropped like the rest of the models), soai_context_to_dict()will leak arbitrary vendor-supplied keys through into the normalized dict. If this is intentional (to preserve extra AI-context metadata), update the docstring to call out the exception explicitly; if not intentional, change it toextra="ignore"for consistency with the rest of the file.📝 Proposed docstring fix
Deviations from the reference package: -- ``extra="ignore"`` everywhere (forward/back-compat across spec versions). +- ``extra="ignore"`` everywhere, except ``OSIAIContextObject`` which uses + ``extra="allow"`` so vendor-specific ai_context keys survive into + ``ai_context_to_dict()`` output.Also applies to: 33-41
🤖 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/osi/models.py` around lines 9 - 14, The module docstring and OSIAIContextObject configuration are inconsistent: the docstring claims extra="ignore" everywhere, but OSIAIContextObject uses extra="allow". Decide whether OSIAIContextObject should preserve unknown AI-context fields or drop them; if preserving them is intentional, update the docstring to explicitly note the exception, and if not, change OSIAIContextObject.model_config to match the rest of the models. Keep the wording aligned with ai_context_to_dict and model_dump behavior so the normalization rules are clear.
🤖 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/cli.py`:
- Around line 1545-1554: The _run_import_osi command currently lets
FileNotFoundError from parse_osi_path bubble up as a traceback when the OSI path
is invalid. Wrap the parse_osi_path(args.osi_path) call in a try/except
FileNotFoundError, emit a friendly message that includes the bad args.osi_path,
and exit with a nonzero status so it matches the clean handling used elsewhere
in _run_import_osi.
In `@slayer/osi/converter.py`:
- Around line 557-566: The hidden derived columns created in
_materialize_columns are still hardcoded to DataType.DOUBLE, which causes
incorrect casts for non-numeric materialized operands used by MIN/MAX/COUNT
DISTINCT. Update _materialize_columns to use the operand’s real/expected type
instead of always forcing DOUBLE, ideally by extending MaterializedColumn to
carry that type (or a no-explicit-type sentinel) and threading it through when
constructing the Column, matching the fix in _resolve_field_column.
- Around line 310-321: The derived-field branch in the converter currently
hardcodes new non-time fields to DataType.DOUBLE, which breaks non-numeric
expressions when Column.sql is later cast by the SQL generator. Update the logic
in the derived-field handling path (where field.dimension,
by_name.get(field.name), and Column(...) are used) so brand-new derived columns
infer or probe the expression’s वास्तविक type instead of assuming DOUBLE; if the
field is a time dimension keep TIMESTAMP, otherwise use a type derived from the
expression or an explicit schema hint so text-like expressions aren’t cast
incorrectly.
- Line 28: Replace the cross-module use of the private helper
`_get_column_types_sync` in `slayer/osi/converter.py` with a public API from
`slayer.sql.client` or add a dedicated exported wrapper for OSI to call instead.
Update the converter logic that currently depends on `_get_column_types_sync` so
it uses the stable public entrypoint and no longer imports the underscored
symbol directly. Make sure any related call sites in the converter path that
rely on this helper are switched over consistently.
In `@slayer/osi/source.py`:
- Line 19: Update `_looks_like_query` so quoted identifiers are ignored before
applying `_SELECT_RE`, since the current regex can misclassify values like
quoted `select` identifiers as SQL queries. Reuse the existing quote-aware
approach already used by `_has_top_level_space` and `_split_identifier` by
introducing or applying `_strip_quoted_spans` to blank out double-quoted spans
before the `re.compile` match, and keep the rest of the query-detection logic in
`osi/source.py` unchanged.
---
Nitpick comments:
In `@slayer/osi/models.py`:
- Around line 9-14: The module docstring and OSIAIContextObject configuration
are inconsistent: the docstring claims extra="ignore" everywhere, but
OSIAIContextObject uses extra="allow". Decide whether OSIAIContextObject should
preserve unknown AI-context fields or drop them; if preserving them is
intentional, update the docstring to explicitly note the exception, and if not,
change OSIAIContextObject.model_config to match the rest of the models. Keep the
wording aligned with ai_context_to_dict and model_dump behavior so the
normalization rules are clear.
In `@slayer/osi/parser.py`:
- Around line 28-39: _collect_files currently returns any single file without
checking _SUFFIXES, so update the path.is_file() branch to apply the same suffix
gate used in the os.walk loop. Make parse_osi_path’s input handling consistent
for both files and directories by rejecting unsupported single-file inputs
before parsing, using the _collect_files helper or equivalent suffix check so
non-YAML/JSON files are excluded with the same policy.
- Around line 32-39: The file collection logic in the parser’s directory walk is
still nondeterministic because os.walk visits nested subdirectories in
platform-dependent order. Update the traversal in the parser function that
builds the files list to sort the dirs sequence in place before continuing the
walk, while keeping the existing filename filtering and sorting behavior, so
imports from multiple subdirectories are collected consistently across runs.
🪄 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: 20291414-c408-44a7-988e-e792ed62e792
📒 Files selected for processing (29)
CLAUDE.mddocs/interfaces/cli.mddocs/osi/osi_import.mddocs/reference/cli.mdslayer/cli.pyslayer/core/models.pyslayer/dbt/converter.pyslayer/engine/join_graph.pyslayer/engine/query_engine.pyslayer/ingest_report.pyslayer/osi/__init__.pyslayer/osi/converter.pyslayer/osi/expression.pyslayer/osi/models.pyslayer/osi/parser.pyslayer/osi/source.pytests/fixtures/osi/aicontext_string.yamltests/fixtures/osi/malformed.yamltests/fixtures/osi/shop.jsontests/fixtures/osi/shop.yamltests/fixtures/osi/unknown_version.yamltests/test_cli_import_osi.pytests/test_ingest_report_reexport.pytests/test_osi_converter.pytests/test_osi_expression.pytests/test_osi_modeljoin_fields.pytests/test_osi_parser.pytests/test_osi_source.pyzensical.toml
…tence) - converter: derived field expressions now validate their (unqualified) column references exist on the table (_missing_expr_columns), consistent with the bare-field / metric / relationship checks - expression: a materialized aggregate operand with any unresolved column now clean-fails instead of discarding the None owner and materializing SQL that references the missing column - tests for both Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dation) - converter: derived field expressions that fail to parse now clean-fail (_missing_expr_columns returns None on ParseError) instead of importing a column with invalid SQL - converter: self-qualified column refs (<dataset>.col) in derived field expressions are validated for existence too; genuinely cross-model refs stay deferred to query-time join resolution - tests for both Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A derived field expression may reference a joined model via <alias>.<col> / <a>__<b>.<col>. SLayer's Column.sql resolver does NOT clean-fail a bad such ref (it leaves an unknown alias untouched, or rewrites a known-model/unknown-column ref into SQL that errors at the DB at query time). So the importer now runs a post-join validation pass (_validate_cross_model_field_refs) that walks each cross-model ref through the join graph and drops+reports any column whose ref names a model with no join path or a nonexistent target column — clean-failing at import instead of failing opaquely at query time. Unqualified and self-qualified refs are still validated at field-overlay time. Tests added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
slayer/osi/converter.py (1)
347-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse keyword arguments for multi-parameter calls.
These new helper calls pass multiple positional arguments. Please switch them to keyword arguments to match the project rule.
As per coding guidelines, "
slayer/**/*.py: ... use keyword arguments for functions with more than one parameter".Proposed fix
- missing = _missing_expr_columns(sql, introspected, ds.name) + missing = _missing_expr_columns(sql=sql, available=introspected, self_name=ds.name) ... - bad = self._unresolvable_cross_model_refs(model, col.sql) + bad = self._unresolvable_cross_model_refs(model=model, sql=col.sql) ... - target = self._walk_join_alias(model, col.table) + target = self._walk_join_alias(host=model, alias=col.table)Also applies to: 440-440, 461-461
🤖 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/osi/converter.py` at line 347, The new helper invocations use multiple positional arguments, which violates the project rule for functions with more than one parameter. Update the calls to the affected helpers, including _missing_expr_columns and the other similar calls at the referenced spots, so each argument is passed by keyword rather than position. Keep the same values and order of intent, but make the call sites explicit with named parameters to match the slayer/**/*.py guideline.Source: Coding guidelines
🤖 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/osi/converter.py`:
- Around line 429-447: The cross-model validation pass in
_validate_cross_model_field_refs can remove a derived column that was previously
accepted by the join graph, leaving a stale ModelJoin that still points at a
missing column. After dropping invalid columns, revalidate or prune any affected
joins so the join structures stay consistent and unsupported structural
mismatches fail cleanly. Use the existing _validate_cross_model_field_refs flow
and the ModelJoin/join-graph handling in the converter to locate and fix the
stale references.
---
Nitpick comments:
In `@slayer/osi/converter.py`:
- Line 347: The new helper invocations use multiple positional arguments, which
violates the project rule for functions with more than one parameter. Update the
calls to the affected helpers, including _missing_expr_columns and the other
similar calls at the referenced spots, so each argument is passed by keyword
rather than position. Keep the same values and order of intent, but make the
call sites explicit with named parameters to match the slayer/**/*.py guideline.
🪄 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: a9df87d2-6e1f-4ef4-9e4b-c24d25c55677
📒 Files selected for processing (3)
slayer/osi/converter.pyslayer/osi/expression.pytests/test_osi_converter.py
🚧 Files skipped from review as they are similar to previous changes (1)
- slayer/osi/expression.py
- converter: cross-model field validation now runs to a FIXED POINT — dropping a column can invalidate another column that referenced it, so re-run until a pass drops nothing (transitive chains resolve correctly) - converter: the validator is now scope-aware (reuses column_expansion's _root_scope_column_ids) so nested subquery/CTE aliases are not mistaken for join refs, and catalog/db-qualified physical refs are skipped — no false drops - converter: a second relationship from a model to the same target is reported and skipped (SLayer joins key only on target_model and can't disambiguate multiple joins to one model) - tests for all three Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_resolve_expression returned the requested --dialect even when it was non-SQL (MDX/MAQL/TABLEAU), feeding non-SQL syntax into the SQL conversion path. Now the requested dialect is used only when it is in SQL_DIALECTS; a non-SQL request falls back to an available SQL dialect (or clean-fails if none). Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… queries _looks_like_query ran the SELECT regex over the whole source string, so a valid quoted identifier segment like "My Select" routed the source to sql-mode. Run the SELECT check on the text outside double-quoted spans (the space check already respects quotes). Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Only COUNT(DISTINCT) maps to a SLayer aggregation (count_distinct). For the other simple aggregates, sqlglot makes the operand an exp.Distinct, which the materialize path would turn into a hidden column with invalid SQL (`DISTINCT amount`) that fails at query time. Detect exp.Distinct in the simple-agg branch and clean-fail. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A field expression that is a single double-quoted identifier (e.g. "legalEntityType", common for case-sensitive columns) was misclassified as a derived expression and rebuilt as a DOUBLE column, corrupting the introspected type. _as_bare_column now parses the expression and recognizes a single unqualified column reference (bare or quoted), so it overlays the introspected column instead. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- converter: when an OSI dataset sets primary_key it now fully REPLACES the introspected primary key (clearing physical PK flags OSI omits), matching the "authoritative" contract; when unset, the introspected PK is kept - converter: fold join-pruning into the cross-model fixed-point loop so a join whose key column is dropped during validation is itself dropped+reported, and the column<->join cascade converges (no join referencing a removed column) - tests for both Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- converter: an aliased quoted bare column keeps its ORIGINAL quoted SQL (the unquoted name is used only for lookup/type) so it doesn't break on case-folding dialects like Snowflake - converter: an OSI primary_key with an unknown column no longer silently clears the physical PK — it reports and keeps the introspected PK - tests for both Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
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 `@slayer/osi/converter.py`:
- Around line 492-506: When _drop_invalid_cross_model_columns() removes a column
from a model, also update that model’s default time dimension if the removed
column matches model.default_time_dimension. Add a check in the drop path around
the existing model.columns.remove(col) logic so the default is reset to another
valid remaining time column or set to None when no suitable time column remains,
keeping the model state consistent after the drop pass.
🪄 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: 62a4c6c6-4d1f-4974-b5fb-79324a6d43bc
📒 Files selected for processing (6)
slayer/osi/converter.pyslayer/osi/expression.pyslayer/osi/source.pytests/test_osi_converter.pytests/test_osi_expression.pytests/test_osi_source.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_osi_source.py
- slayer/osi/source.py
- tests/test_osi_expression.py
- slayer/osi/expression.py
- cli: import-osi catches FileNotFoundError from parse_osi_path and exits with a clean message instead of a raw traceback (CodeRabbit) - converter: the cross-model column/join drop passes now collect-then-remove instead of iterating over list(...) snapshots, clearing 4 Sonar S7504 issues while keeping mutation-safe iteration - test: import-osi on a nonexistent path exits cleanly Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A derived field that redefines an existing physical column (e.g. status: LOWER(status)) was hard-typed DOUBLE, clobbering the introspected TEXT type. It now inherits the shadowed column's known type; a genuinely new derived column still defaults to DOUBLE (is_time still wins as TIMESTAMP). Test tightened. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… column A derived field replacing an introspected column produced a fresh Column with primary_key=False, silently dropping the physical PK when OSI didn't restate primary_key. The replace path now carries the shadowed column's primary_key across the redefinition (OSI's explicit primary_key still overrides later). Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a derived OSI overlay shadowed a physical column and was then dropped by cross-model validation (bad cross-model ref), the physical column was deleted entirely, losing a real column + PK and cascading into dropped joins. The converter now records the shadowed introspected column and, when the overlay is invalidated, reverts to that physical column instead of removing it. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- sql/client: add public get_column_types_sync wrapper; the OSI converter uses it instead of importing the private _get_column_types_sync (CodeRabbit) - converter: extract _invalid_cross_model_columns and _revert_or_drop_column from _drop_invalid_cross_model_columns to bring its cognitive complexity back under the limit (Sonar S3776) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A genuinely new derived field (no matching physical column) was hard-typed DOUBLE, so a non-numeric expression (UPPER(x), concat, ...) would be emitted as CAST(<text> AS DOUBLE) and fail at query time. The converter now live-probes the expression's type against the model's physical source (LIMIT-0 cursor metadata, same mechanism as sql-mode column typing), falling back to DOUBLE only when the probe is unavailable. Shadowing overlays still inherit the shadowed type; is_time still wins as TIMESTAMP. Test added. (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…column A brand-new derived field that is exactly a cross-model column ref (customers.segment) couldn't be probed locally (the join doesn't exist at overlay time) and fell back to DOUBLE, which would mis-cast a joined text/temporal column at query time. A post-join pass now resolves a single cross-model ref through the join graph and sets the column's type from the target column. Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- converter: dropping a column that is the model's default_time_dimension now resets it to another remaining time column or None (CodeRabbit) - parser: a single-file input honors the .yaml/.yml/.json suffix policy, same as directory scanning (CodeRabbit nitpick) - converter: multi-parameter internal calls use keyword arguments per the project convention (CodeRabbit nitpick) - tests for the time-default reset and the single-file suffix filter Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A model name becomes a filename in YAML storage (<dir>/<name>.yaml), and neither _legal_model_name nor SlayerModel rejected path separators, so an OSI dataset named e.g. "/tmp/owned" could write outside the storage tree. _legal_model_name now rejects empty/whitespace names and forbidden characters including '/', '\', and NUL (in addition to '__', '.', ':'). Test added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The path-separator rejection test used "/tmp/owned" as the fake malicious name; Sonar flags the /tmp literal as a publicly-writable-directory security issue. Use "evil/name" instead — still exercises the '/' rejection without the literal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The importer advertises SNOWFLAKE/DATABRICKS as SQL-compatible, but all downstream sqlglot parsing uses the default dialect, so dialect-specific syntax (e.g. Databricks backticks `col`) would fail to parse and incorrectly clean-fail otherwise-valid fields/metrics. _resolve_expression now normalizes the resolved expression via sqlglot transpile (read=source dialect -> default SQL); ANSI_SQL is returned verbatim and an unparseable expression falls through to a clear downstream clean-fail. Test added. 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.
Adds
slayer import-osi <path> --datasource X [--dialect ANSI_SQL]— convert OSI (Open Semantic Interchange) semantic-model configs (YAML/JSON, file or directory) into SLayer models. CLI-only, mirroringimport-dbt(parse → convert →save_modelper model → printed conversion report).OSI maps directly to SLayer, not through the dbt intermediate: SLayer's dbt converter clean-fails measure-less metrics and infers joins from shared entity names, both structural mismatches with OSI's raw-SQL metrics and explicit relationships.
What it does
slayer/osi/(mirrorsslayer/dbt/):models.py(Pydantic port of the OSI schema — versions 1.0/0.1.0/0.1.1/0.2.0.dev0 accepted, all structurally identical; unknown → warn+parse),source.py([cat.]db.schema.tableparsing + stubbedresolve_datasourceper-database routing hook),expression.py(sqlglot-AST → SLayer colon formula),converter.py.introspect_table_to_model, query sources via the_get_column_types_syncLIMIT-0 probe). OSI overlays labels/descriptions/is_time/ai_context→description+meta/custom_extensions+unique_keys→meta.ModelJoin(composite-key safe; length-mismatch/unknown-target clean-fail).ModelJoingains optionaldescription/meta(additive — noSlayerModelversion bump).ModelMeasurecolon formulas: agg leaves, arithmetic+constants,SCALAR_PASSTHROUGHfuncs, non-bare operands materialized as hidden derived columns, percentile→col:percentile(p=…)/median; validated viaparse_formula; everything else clean-failed. Anchored on the model reaching every referenced dataset via the sharedjoin_graph.min_hops_root(now also used byrecommend_root_model); cross-dataset refs emit anchor-relative dotted paths. OrphanCOUNT(*)(no unique fact table) errors rather than guessing.ConversionResult/ConversionWarningextracted to neutralslayer/ingest_report.py, re-exported fromslayer/dbt/converter.py(same class objects).Tests
92 new tests (parser, expression transform, converter with live SQLite introspection, source parsing, ModelJoin round-trip, report re-export, CLI e2e). Full non-integration suite: 6542 passed, 0 failures; ruff clean.
Docs:
docs/osi/osi_import.md(linked inzensical.toml), CLI reference docs, CLAUDE.md.🤖 Generated with Claude Code
Summary by CodeRabbit
slayer import-osicommand to import OSI configurations into SLayer models, with dialect handling and per-model conversion reporting.