From 95af686495ea4c624f65981ec0f06401b5e29898 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 12:58:08 +0200 Subject: [PATCH 01/25] DEV-1643: import OSI (Open Semantic Interchange) configs into SLayer 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) --- CLAUDE.md | 3 +- docs/interfaces/cli.md | 15 + docs/osi/osi_import.md | 50 +++ docs/reference/cli.md | 15 + slayer/cli.py | 70 +++ slayer/core/models.py | 5 + slayer/dbt/converter.py | 64 +-- slayer/engine/join_graph.py | 24 ++ slayer/engine/query_engine.py | 14 +- slayer/ingest_report.py | 73 ++++ slayer/osi/__init__.py | 1 + slayer/osi/converter.py | 523 +++++++++++++++++++++++ slayer/osi/expression.py | 293 +++++++++++++ slayer/osi/models.py | 165 +++++++ slayer/osi/parser.py | 93 ++++ slayer/osi/source.py | 93 ++++ tests/fixtures/osi/aicontext_string.yaml | 11 + tests/fixtures/osi/malformed.yaml | 2 + tests/fixtures/osi/shop.json | 396 +++++++++++++++++ tests/fixtures/osi/shop.yaml | 138 ++++++ tests/fixtures/osi/unknown_version.yaml | 9 + tests/test_cli_import_osi.py | 82 ++++ tests/test_ingest_report_reexport.py | 41 ++ tests/test_osi_converter.py | 519 ++++++++++++++++++++++ tests/test_osi_expression.py | 238 +++++++++++ tests/test_osi_modeljoin_fields.py | 91 ++++ tests/test_osi_parser.py | 121 ++++++ tests/test_osi_source.py | 57 +++ zensical.toml | 3 + 29 files changed, 3139 insertions(+), 70 deletions(-) create mode 100644 docs/osi/osi_import.md create mode 100644 slayer/ingest_report.py create mode 100644 slayer/osi/__init__.py create mode 100644 slayer/osi/converter.py create mode 100644 slayer/osi/expression.py create mode 100644 slayer/osi/models.py create mode 100644 slayer/osi/parser.py create mode 100644 slayer/osi/source.py create mode 100644 tests/fixtures/osi/aicontext_string.yaml create mode 100644 tests/fixtures/osi/malformed.yaml create mode 100644 tests/fixtures/osi/shop.json create mode 100644 tests/fixtures/osi/shop.yaml create mode 100644 tests/fixtures/osi/unknown_version.yaml create mode 100644 tests/test_cli_import_osi.py create mode 100644 tests/test_ingest_report_reexport.py create mode 100644 tests/test_osi_converter.py create mode 100644 tests/test_osi_expression.py create mode 100644 tests/test_osi_modeljoin_fields.py create mode 100644 tests/test_osi_parser.py create mode 100644 tests/test_osi_source.py diff --git a/CLAUDE.md b/CLAUDE.md index 2403209a..b6182e91 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,7 +77,7 @@ poetry run ruff check slayer/ tests/ - **Window functions in filters**: filter strings and `ModelMeasure.formula` cannot contain raw `OVER (...)` SQL — SLayer's formula parser is Python-AST-based and rejects with an actionable error pointing at the `rank()` / `first()` / `last()` / `lag()` / `lead()` transforms. Filtering on a `Column` whose `sql` contains a window function is also rejected (DEV-1369; the prior auto-promotion escape hatch from DEV-1336 is removed). For top-N use the inline `rank() <= N` transform (or `dense_rank` / `percent_rank` / `ntile(n=)`); for non-standard window expressions, factor them into an earlier stage of a multi-stage `source_queries` model. - **Bare same-model derived refs in `Column.sql`** (DEV-1410): A bare identifier inside a `Column.sql` that names a sibling **derived** column on the host model is inlined parenthesised, identical to the qualified form (`A.foo` and bare `foo` produce the same SQL). Inlining is scope-guarded — refs inside a nested scope (sub-query, set-op branch, CTE, `VALUES`) are left alone because they belong to the inner rowset. Cycles in the derived-ref graph (`c1.sql = "c2 + 1"`, `c2.sql = "c1 - 1"`) raise `slayer.core.errors.ColumnCycleError` at `storage.save_model` time, before the model reaches a query. `ColumnCycleError` subclasses both `SlayerError` and `ValueError` so existing `except ValueError` call sites keep working. `StorageBackend.save_model` is a template method that runs the cycle validator before delegating to the backend's `_save_model_impl`; the migration write-back at `_migrate_and_refine_on_load` passes `_validate=False` so legacy cyclic data remains loadable. Save-time validation stays within the model's `data_source`; unresolved join targets are silently skipped (best-effort) — the compile-time guard in `slayer/engine/column_expansion.py` remains authoritative. - Filters support `{variable}` placeholders substituted from `query.variables: Dict[str, Any]`. Values must be str/number, inserted as-is. `{{`/`}}` for literal braces. Undefined variables raise errors. -- Models can have explicit `joins` to other models (LEFT JOINs). Cross-model measures use dotted syntax with colon aggregation (`customers.revenue:sum`) and multi-hop (`customers.regions.name`). Joins are auto-resolved by walking the join graph. Transforms work on cross-model measures (`cumsum(customers.revenue:sum)`) +- Models can have explicit `joins` to other models (LEFT JOINs). Cross-model measures use dotted syntax with colon aggregation (`customers.revenue:sum`) and multi-hop (`customers.regions.name`). Joins are auto-resolved by walking the join graph. Transforms work on cross-model measures (`cumsum(customers.revenue:sum)`). `ModelJoin` also carries optional `description` and `meta` fields (DEV-1643, used to carry OSI relationship `ai_context` on import); they are purely additive/optional, so **no** `SlayerModel` version bump — old v7 join data validates unchanged. - **Path-based table aliases**: Joined tables use `__`-delimited path aliases in SQL to disambiguate diamond joins. In queries, dots denote paths (`customers.regions.name`); in model SQL definitions, `__` denotes the table alias (`customers__regions.name`). For diamond joins (same table reached via different paths, e.g., `orders → customers → regions` AND `orders → warehouses → regions`), each path gets a unique alias (`customers__regions` vs `warehouses__regions`). Auto-ingestion creates only direct joins (one per FK on the source table); multi-hop paths are resolved at query time by walking each intermediate model's own joins - `SlayerQuery.source_model` accepts a model name, inline `SlayerModel`, or `ModelExtension` (extends a model with extra `columns`/`measures` formulas/`joins`). `create_model_from_query()` saves a query as a permanent model - Models can have `filters` (always-applied WHERE conditions, e.g., `"deleted_at IS NULL"`) @@ -181,6 +181,7 @@ poetry run ruff check slayer/ tests/ - `slayer serve --ingest-on-startup` and `slayer mcp --ingest-on-startup` (DEV-1392) — opt-in boot-time idempotent auto-ingestion across every configured datasource, sync-before-listen (uvicorn/mcp.run don't start until ingest finishes). Continue-on-failure: per-datasource errors are friendly-formatted to stderr and never abort startup; `storage.list_datasources()` raising is the only thing that prevents the server from starting. `to_delete` drift entries are printed but **never auto-applied** — destructive cleanup stays gated behind `slayer validate-models --force-clean [--yes]`. Composes freely with `--demo` (demo first, then the ingest pass over every datasource including the freshly-created demo). Also exposed via `SLAYER_INGEST_ON_STARTUP=1` env var (flag wins when both set) and the `ingest_on_startup=True` kwarg on `create_app` / `create_mcp_server`. All output goes to stderr — `slayer mcp` stdio JSON-RPC remains protocol-safe. Orchestrator: `slayer/engine/ingestion.py::ingest_all_datasources_idempotent`. **Memory embeddings** (DEV-1416): each per-datasource pass also re-embeds every memory whose canonical entities are rooted at the datasource, so a stale `embeddings.db` is repaired by the next `--ingest-on-startup` without extra steps. See [docs/concepts/ingestion.md](docs/concepts/ingestion.md#ingesting-at-startup). - `slayer validate-models [--datasource X] [--force-clean] [--yes]` (DEV-1356) — read-only diff against live schemas; with `--force-clean`, prompts to apply each delete via `engine.apply_drift_deletes`. See [docs/concepts/schema-drift.md](docs/concepts/schema-drift.md). - `slayer recommend-root-model ITEM... [--data-source X] [--root-hint M] [--format json|text]` (DEV-1626) — given `model.column` / `model.metric` items, introspect the join graph and recommend the query `source_model` (root) plus each item's join-qualified path from it. Core is `engine.recommend_root_model(items, *, data_source=None, root_hint=None) -> RootModelRecommendation` (+ `_sync`), delegating to the pure `slayer/engine/join_graph.py::JoinGraph` primitive (LEFT = directed edge, INNER = undirected via storage symmetry; `reachable_from` + lexicographic-tie-broken `shortest_path`) and reusing `resolve_entity` for input normalization + `_all_models_in_datasource` for candidate loading. A valid root reaches every mentioned owning model; selection minimizes total hops over **distinct** owning models, then prefers a mentioned model, then the lexicographically smallest name. Items must be columns or metrics (aggregations/bare-model/datasource refs raise `ValueError`; unresolvable → `EntityResolutionError`; cross-datasource mix → `ValueError`). Aggregation suffixes are preserved verbatim (the engine resolves agg kwargs relative to the aggregated column's owning model, invariant under root choice). **`root_hint`** (optional, `_resolve_root_hint`) names the intended root — a bare model name or `.` within the resolved datasource; resolved **after** the datasource is fixed from items, so it can't pick the datasource. When the hint is a **feasible** root (reaches every item) it is honored outright, overriding the min-hops pick — this lets a caller force a **bridge** model that owns none of the items but matches the intended grain (the descriptions-first workflow). When **infeasible** the auto-pick is used and a warning names the rejected hint (echoed verbatim as typed), the unreachable owning model(s), and the fallback root; honored-vs-fallback is conveyed via `message`/`warnings` only (**no structured result field**). A non-existent / wrong-kind (bare column) / 3-part / cross-datasource hint raises `ValueError`; an empty/whitespace hint is a no-op. When no single model reaches everything, returns a structured `reachable=False` result whose `coverage` is the **Pareto frontier** of partial roots (dominance: strictly-larger reach set, or equal reach set with no longer and ≥1 shorter path), **plus the `root_hint`'s own row when given** (force-included even if dominated / zero-reach, never duplicated) — `_build_recommend_coverage(..., force_include=...)`. `_expand_join_graph` (schema-drift error path) delegates its directed reachability to `JoinGraph.reachable_from` (one reachability impl). Result models in `slayer/core/recommend.py` (`RootModelRecommendation`/`ItemPath`/`CandidateCoverage`). Surfaced on MCP `recommend_root_model`, REST `POST /recommend-root-model` (400 on `ValueError`/`SlayerError`), CLI, and `SlayerClient.recommend_root_model(_sync)`. See [docs/concepts/queries.md](docs/concepts/queries.md#choosing-a-root-model). +- `slayer import-osi --datasource X [--dialect ANSI_SQL] [--storage ...]` (DEV-1643) — import OSI (Open Semantic Interchange) configs (YAML/JSON, file or dir) into SLayer models, mirroring `import-dbt`'s CLI-only shape (parse → convert → `storage.save_model` per model → printed report). OSI maps DIRECTLY to SLayer (not via the dbt intermediate — SLayer's dbt converter clean-fails measure-less metrics and infers joins from entity names, both structural mismatches). Package `slayer/osi/` mirrors `slayer/dbt/`: `models.py` (Pydantic port of the OSI schema; versions 1.0/0.1.0/0.1.1/0.2.0.dev0 are structurally identical, accepted; unknown → warn+parse), `source.py` (`parse_source` splits `[cat.]db.schema.table`; `resolve_datasource(database, default)` is the stubbed per-database routing hook, drops db for now), `expression.py` (`convert_expression` — sqlglot-AST → SLayer colon formula: agg leaves, arithmetic+constants, `SCALAR_PASSTHROUGH` funcs, non-bare operands materialized as hidden derived columns, percentile → `col:percentile(p=…)`/`median`; validates via `parse_formula`; everything else clean-fails), `converter.py` (`OsiToSlayerConverter`). Live introspection (`introspect_table_to_model`) provides real column types/PK; OSI overlays label/description/is_time/`ai_context`→description+`meta['osi_ai_context']`/`custom_extensions`→`meta`/`unique_keys`→`meta`. Relationships → LEFT `ModelJoin` (composite-safe, length-mismatch clean-fails). Metrics → `ModelMeasure` on an anchor model chosen via the shared `slayer/engine/join_graph.py::min_hops_root` (also now used by `recommend_root_model`); cross-dataset refs emit anchor-relative dotted paths; a column-less `COUNT(*)` anchors on the semantic model's **unique fact table** (the one dataset never used as a join target) — 0-or-many candidates → clean-failed as an orphan (ambiguous grain), never guessed. The conversion-report types (`ConversionResult`/`ConversionWarning`) were extracted to the neutral `slayer/ingest_report.py` and re-exported from `slayer/dbt/converter.py`. Query-source (`sql`-mode) datasets are live-introspected too via `slayer/sql/client.py::_get_column_types_sync` (the same LIMIT-0 / cursor-metadata probe used elsewhere), driven by the datasource type. CLI-only — no MCP/REST. See [docs/osi/osi_import.md](docs/osi/osi_import.md). - `slayer storage migrate-types [--data-source X] [--dry-run]` (DEV-1361) — refine `DOUBLE → INT` on base columns whose live SQL type is integer for every persisted model, then write the refined v5 dict back. Hard-fails if a datasource is unreachable. The same refinement runs transparently inside `storage.get_model` on first load; this CLI is a batch / inspectable alternative. - `slayer search [--entity ENT ...] [--query JSON_OR_@FILE] [--question TEXT] [--datasource DS] [--cypher-filter CYPHER] [--max-results N] [--format json|text] [--verbose]` (DEV-1375 / DEV-1386 / DEV-1409 / DEV-1464 / DEV-1532 / DEV-1549) — up to three-channel semantic search over memories + canonical entities (BM25 over memory entity tags + tantivy full-text + optional dense embedding similarity). Returns a single flat ranked `results` list. `--datasource` scopes the corpus to one datasource. `--cypher-filter` pre-filters all channels: full openCypher when `advanced_search` is installed; simple `MATCH (n:Label) RETURN n.id AS id` kind-filter without it. **DEV-1549**: compact-by-default. Each `SearchHit` carries a `description` field; under the default `compact=True` (everywhere — MCP / REST `compact: bool = True` / CLI / `SlayerClient`) memory hits surface `Memory.description` (or the first non-empty paragraph of `learning`, capped at 500 chars) and entity hits surface `entity.description`, with `hit.text == ""`. CLI flips compact off via `--verbose`; REST/MCP/SlayerClient via `compact=False`. Drill-in pattern: `search(entities=["memory:"], max_results=1, compact=False)`. See [docs/concepts/search.md](docs/concepts/search.md). - `slayer search refresh-samples [--data-source X] [--model M ...]` (DEV-1375) — re-profile and persist `Column.sampled` for table-backed models. Best-effort: per-column failures are reported but don't abort. diff --git a/docs/interfaces/cli.md b/docs/interfaces/cli.md index a83bfd1d..d19cffd2 100644 --- a/docs/interfaces/cli.md +++ b/docs/interfaces/cli.md @@ -124,6 +124,21 @@ slayer import-dbt ./my_dbt_project --datasource my_postgres --include-hidden-mod | `dbt_project_path` | Yes | Path to the dbt project root (or a models directory) | | `--datasource` | Yes | SLayer datasource name for the imported models | | `--include-hidden-models` | No | Also import regular dbt models (those not wrapped by a `semantic_model`) as hidden SLayer models via SQL introspection. Requires the `dbt` extra. | + +### `slayer import-osi` + +Import OSI (Open Semantic Interchange) configs into SLayer. See [Importing OSI configs](../osi/osi_import.md). + +```bash +slayer import-osi ./osi_configs --datasource my_postgres +slayer import-osi ./model.yaml --datasource my_postgres --dialect SNOWFLAKE +``` + +| Flag | Required | Description | +|------|----------|-------------| +| `osi_path` | Yes | Path to an OSI file or directory (`.yaml`/`.yml`/`.json`) | +| `--datasource` | Yes | SLayer datasource name (must be reachable — types come from live introspection) | +| `--dialect` | No | OSI expression dialect to read (default `ANSI_SQL`); falls back to another SQL dialect when absent | | `--storage` | No | Storage path | ### `slayer models` diff --git a/docs/osi/osi_import.md b/docs/osi/osi_import.md new file mode 100644 index 00000000..ae5e666b --- /dev/null +++ b/docs/osi/osi_import.md @@ -0,0 +1,50 @@ +# Importing OSI (Open Semantic Interchange) Configs + +SLayer can import [Open Semantic Interchange](https://github.com/open-semantic-interchange/OSI) (OSI) semantic-model configs and convert them into SLayer models. OSI is a vendor-neutral YAML/JSON standard for semantic models, datasets, relationships, and metrics. + +## Quick Start + +```bash +slayer import-osi ./osi_configs --datasource my_postgres --storage ./slayer_data +``` + +This reads every `.yaml`/`.yml`/`.json` file in the path (a single file or a directory), converts each OSI dataset into a SLayer model, and saves the models to storage. A **reachable datasource is required** — column data types come from live table introspection (OSI carries no column types), so the importer connects to `--datasource` and overlays OSI's semantic metadata on top. + +Spec versions `1.0`, `0.1.0`, `0.1.1`, and `0.2.0.dev0` are all accepted (they are structurally identical); an unknown version is warned about but still parsed. + +## What Gets Converted + +| OSI construct | SLayer target | +|---|---| +| `semantic_model[].datasets[]` | one `SlayerModel` each (name = dataset name) | +| dataset `source` (`db.schema.table`) | `sql_table` + `schema`; a query source → `sql` mode | +| dataset `fields[]` | `Column`s (real types from introspection) | +| field `expression` (bare) | overlaid onto the introspected column | +| field `expression` (derived, e.g. `UPPER(x)`) | a derived `Column` with `sql` set | +| field `dimension.is_time` | column typed temporal; sets `default_time_dimension` | +| dataset `primary_key` | `Column.primary_key = true` | +| `relationships[]` (`from` → `to`) | a LEFT `ModelJoin` on the `from` model | +| `metrics[]` (raw SQL aggregation) | a `ModelMeasure` formula | +| `ai_context` (instructions + synonyms) | entity `description` + `meta["osi_ai_context"]` | +| `unique_keys` / `custom_extensions` | model/column `meta` | + +### Metrics + +An OSI metric holds a raw SQL aggregation expression. SLayer parses it into colon-syntax formulas: + +- `SUM(amount)` → `amount:sum`, `COUNT(*)` → `*:count`, `COUNT(DISTINCT id)` → `id:count_distinct` +- arithmetic + constants + scalar functions pass through: `SUM(a) / NULLIF(COUNT(*), 0)` → `a:sum / nullif(*:count, 0)` +- a non-bare aggregate operand is materialized as a hidden derived column: `SUM(quantity * amount)` → a hidden column `quantity * amount` plus `:sum` +- `PERCENTILE_CONT(0.9) WITHIN GROUP (ORDER BY x)` → `x:percentile(p=0.9)` (`0.5` → `x:median`) + +A metric that references columns from more than one dataset is attached to an **anchor** model — the model that reaches every referenced dataset over the relationship-derived joins (chosen via the same logic as [`recommend_root_model`](../concepts/queries.md#choosing-a-root-model)). Cross-dataset columns are emitted as join-qualified dotted refs (`customers.regions.population:sum`). + +## Dialect Selection + +OSI expressions are multi-dialect. `--dialect` (default `ANSI_SQL`) picks which one to read; when it is absent, the importer falls back to another SQL-compatible dialect (`SNOWFLAKE`, `DATABRICKS`). An expression available only in a non-SQL dialect (`MDX`, `MAQL`, `TABLEAU`) is clean-failed to the report. + +## Clean-Fail Report + +Anything that cannot be expressed exactly is reported (never silently dropped) and the raw construct is preserved in `meta`. Examples: a metric with a `CASE`/window expression, a relationship with mismatched key lengths or an unknown target, a dataset whose table cannot be introspected, an illegal name (containing `.`/`:`), a metric whose referenced datasets are not connected by any join path, or an orphan `COUNT(*)` (a column-less metric in a semantic model with no unique fact table, so its grain is ambiguous). The CLI prints a grouped report and a `models / unconverted / dropped` tally at the end. + +A query source is introspected live (a `LIMIT 0` / cursor-metadata probe) for real column types, the same way table sources are — no connection-less heuristics. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index c4398b59..50b94cba 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -109,6 +109,21 @@ slayer import-dbt ./my_dbt_project --datasource my_postgres --include-hidden-mod | `dbt_project_path` | Yes | Path to the dbt project root (or a models directory) | | `--datasource` | Yes | SLayer datasource name for the imported models | | `--include-hidden-models` | No | Also import regular dbt models (those not wrapped by a `semantic_model`) as hidden SLayer models via SQL introspection. Requires the `dbt` extra (`pip install 'motley-slayer[dbt]'`). See [dbt Import](../dbt/dbt_import.md#regular-dbt-models-hidden-import). | + +### `slayer import-osi` + +Import OSI (Open Semantic Interchange) configs into SLayer. See [Importing OSI configs](../osi/osi_import.md). + +```bash +slayer import-osi ./osi_configs --datasource my_postgres +slayer import-osi ./model.yaml --datasource my_postgres --dialect SNOWFLAKE +``` + +| Flag | Required | Description | +|------|----------|-------------| +| `osi_path` | Yes | Path to an OSI file or directory (`.yaml`/`.yml`/`.json`) | +| `--datasource` | Yes | SLayer datasource name (must be reachable — column types come from live introspection) | +| `--dialect` | No | OSI expression dialect to read (default `ANSI_SQL`); falls back to another SQL dialect when the requested one is absent | | `--storage` | No | Storage path | ### `slayer models` diff --git a/slayer/cli.py b/slayer/cli.py index b85ec24b..227add15 100644 --- a/slayer/cli.py +++ b/slayer/cli.py @@ -372,6 +372,26 @@ def main(): # NOSONAR(S3776) — linear top-level CLI command dispatch (one eli ) _add_storage_arg(import_dbt_parser) + # ── import-osi ──────────────────────────────────────────────────── + import_osi_parser = subparsers.add_parser( + "import-osi", + help="Import OSI (Open Semantic Interchange) configs into SLayer models", + epilog="""\ +examples: + slayer import-osi ./osi_configs --datasource my_postgres + slayer import-osi ./model.yaml --datasource my_pg --dialect SNOWFLAKE +""", + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + import_osi_parser.add_argument("osi_path", help="Path to an OSI file or directory (YAML/JSON)") + import_osi_parser.add_argument("--datasource", required=True, help="SLayer datasource name for the imported models") + import_osi_parser.add_argument( + "--dialect", default="ANSI_SQL", + help="OSI expression dialect to read (default: ANSI_SQL). Falls back to " + "another SQL dialect when the requested one is absent.", + ) + _add_storage_arg(import_osi_parser) + # ── models ──────────────────────────────────────────────────────── models_parser = subparsers.add_parser( "models", @@ -810,6 +830,8 @@ def main(): # NOSONAR(S3776) — linear top-level CLI command dispatch (one eli _run_recommend_root_model(args) elif args.command == "import-dbt": _run_import_dbt(args) + elif args.command == "import-osi": + _run_import_osi(args) elif args.command == "models": _run_models(args) elif args.command == "datasources": @@ -1520,6 +1542,54 @@ def _run_import_dbt(args): ) +def _run_import_osi(args): + from slayer.osi.converter import OsiToSlayerConverter + from slayer.osi.parser import parse_osi_path + from slayer.sql import engine_factory + + storage = _resolve_storage(args) + documents = parse_osi_path(args.osi_path) + if not documents: + print(f"No OSI documents found in {args.osi_path}") + sys.exit(1) + + ds = run_sync(storage.get_datasource(args.datasource)) + if ds is None: + storage_path = args.storage or args.models_dir or _STORAGE_DEFAULT + print( + f"Datasource '{args.datasource}' not found in {storage_path}; " + "a reachable datasource is required (types come from live introspection)." + ) + sys.exit(1) + + sa_engine = engine_factory.get_engine(ds.resolve_env_vars()) + converter = OsiToSlayerConverter( + documents=documents, + data_source=args.datasource, + sa_engine=sa_engine, + dialect=args.dialect, + target_dialect=ds.type, + ) + result = converter.convert() + + for model in result.models: + run_sync(storage.save_model(model)) + print( + f"Imported model: {model.name} " + f"({len(model.columns)} columns, {len(model.measures)} measures)" + ) + + if result.unconverted_metrics or result.warnings: + print("\nConversion report:") + print(result.render_report()) + + unconverted, dropped = result.tally() + print( + f"\nDone: {len(result.models)} models, " + f"{unconverted} unconverted, {dropped} dropped" + ) + + def _run_models(args): import yaml diff --git a/slayer/core/models.py b/slayer/core/models.py index ab39a91b..f2b21e2c 100644 --- a/slayer/core/models.py +++ b/slayer/core/models.py @@ -418,6 +418,11 @@ class ModelJoin(BaseModel): target_model: str # Name of the joined model join_pairs: list[list[str]] = Field(...) # [["source_dim", "target_dim"], ...] join_type: JoinType = JoinType.LEFT # LEFT (default) or INNER + # DEV-1643: optional human/agent metadata (e.g. carrying OSI relationship + # ai_context on import). Purely additive/optional — old data omits them and + # validates unchanged, so no SlayerModel schema-version bump is needed. + description: str | None = None + meta: dict[str, Any] | None = None @field_validator("join_pairs") @classmethod diff --git a/slayer/dbt/converter.py b/slayer/dbt/converter.py index cbf6c77e..ca3ad7ae 100644 --- a/slayer/dbt/converter.py +++ b/slayer/dbt/converter.py @@ -21,7 +21,6 @@ from typing import Any, Literal import sqlalchemy as sa -from pydantic import BaseModel, Field from slayer.core.enums import DataType, JoinType from slayer.core.format import NumberFormat, NumberFormatType @@ -45,6 +44,11 @@ ) from slayer.dbt.sql_resolver import resolve_refs from slayer.engine.ingestion import introspect_table_to_model +# DEV-1643: the conversion-report types are shared with the OSI importer. They +# live in the neutral ``slayer.ingest_report`` module and are re-exported here so +# existing ``from slayer.dbt.converter import ConversionResult`` imports keep +# working against the same class objects. +from slayer.ingest_report import ConversionResult, ConversionWarning logger = logging.getLogger(__name__) @@ -92,64 +96,6 @@ class DbtConversionError(Exception): """ -class ConversionWarning(BaseModel): - """A structured entry in the conversion report (DEV-1595). - - ``category`` groups entries in ``render_report``; ``severity`` is one of - ``"unconverted"`` (tried to convert, couldn't), ``"dropped"`` (intentional - clean-fail of an inexpressible construct), or ``"info"`` (a caveat — the - construct imports but has a runtime limitation). ``suggestion`` carries the - documented workaround. - """ - model_name: str | None = None - metric_name: str | None = None - message: str - category: str = "general" - severity: Literal["unconverted", "dropped", "info"] = "unconverted" - suggestion: str | None = None - - -class ConversionResult(BaseModel): - """Result of converting a DbtProject to SLayer representations.""" - models: list[SlayerModel] = Field(default_factory=list) - unconverted_metrics: list[ConversionWarning] = Field(default_factory=list) - warnings: list[ConversionWarning] = Field(default_factory=list) - - def _all_entries(self) -> list[ConversionWarning]: - return list(self.unconverted_metrics) + list(self.warnings) - - def render_report(self) -> str: - """Render the conversion report grouped by category (DEV-1595). - - Each category becomes a heading with a count; each entry lists its - entity, severity, reason, and (when present) the documented workaround. - """ - entries = self._all_entries() - if not entries: - return "No conversion issues." - by_cat: dict[str, list[ConversionWarning]] = defaultdict(list) - for e in entries: - by_cat[e.category or "general"].append(e) - lines: list[str] = [] - for cat in sorted(by_cat): - items = by_cat[cat] - lines.append(f"## {cat} ({len(items)})") - for e in items: - entity = e.metric_name or e.model_name or "general" - lines.append(f" - [{e.severity}] {entity}: {e.message}") - if e.suggestion: - lines.append(f" workaround: {e.suggestion}") - lines.append("") - return "\n".join(lines).rstrip() - - def tally(self) -> tuple[int, int]: - """``(unconverted, dropped)`` counts by severity for the CLI summary.""" - entries = self._all_entries() - unconverted = sum(1 for e in entries if e.severity == "unconverted") - dropped = sum(1 for e in entries if e.severity == "dropped") - return unconverted, dropped - - def _map_agg(dbt_agg: str) -> str: """Map a dbt aggregation name to a SLayer aggregation name.""" mapped = _AGG_MAP.get(dbt_agg.lower()) diff --git a/slayer/engine/join_graph.py b/slayer/engine/join_graph.py index 29513c79..8f628962 100644 --- a/slayer/engine/join_graph.py +++ b/slayer/engine/join_graph.py @@ -103,3 +103,27 @@ def shortest_path(self, root: str, target: str) -> list[str] | None: ] best[v] = min(best[u] for u in preds) + [v] return best[target] + + +def min_hops_root( + graph: "JoinGraph", candidates: list[str], mentioned: set[str] +) -> str | None: + """Pick the root that reaches every ``mentioned`` model over ``graph``. + + Shared selection core (DEV-1626 / DEV-1643): among ``candidates`` that reach + all mentioned models, minimize total hops summed over the mentioned set, + prefer a mentioned candidate on ties, then the lexicographically smallest + name. Returns ``None`` when no candidate reaches every mentioned model. An + empty ``mentioned`` set makes every candidate trivially valid (0 hops), so + the lexicographically smallest candidate is returned. + """ + def total_hops(root: str) -> int: + return sum(len(graph.shortest_path(root, m) or []) for m in mentioned) + + def reaches_all(root: str) -> bool: + return all(graph.shortest_path(root, m) is not None for m in mentioned) + + valid = [c for c in candidates if reaches_all(c)] + if not valid: + return None + return min(valid, key=lambda n: (total_hops(n), 0 if n in mentioned else 1, n)) diff --git a/slayer/engine/query_engine.py b/slayer/engine/query_engine.py index 6d5c9cb7..33988f2a 100644 --- a/slayer/engine/query_engine.py +++ b/slayer/engine/query_engine.py @@ -36,7 +36,7 @@ RootModelRecommendation, ) from slayer.core.refs import split_agg_suffix -from slayer.engine.join_graph import JoinGraph +from slayer.engine.join_graph import JoinGraph, min_hops_root from slayer.engine.enriched import ( CrossModelMeasure, EnrichedMeasure, @@ -1453,21 +1453,15 @@ async def recommend_root_model( hint_model = hint[0] if hint is not None else None hint_display = hint[1] if hint is not None else None - def total_hops(root: str) -> int: - return sum(len(graph.shortest_path(root, m) or []) for m in mentioned) - def reaches_all(root: str) -> bool: return all(graph.shortest_path(root, m) is not None for m in mentioned) def missing_models(root: str) -> list[str]: return sorted(m for m in mentioned if graph.shortest_path(root, m) is None) - valid_roots = [name for name in all_names if reaches_all(name)] - if valid_roots: - auto = min( - valid_roots, - key=lambda n: (total_hops(n), 0 if n in mentioned else 1, n), - ) + # Shared selection core (also used by the OSI importer's anchor pick). + auto = min_hops_root(graph, all_names, mentioned) + if auto is not None: if hint_model is not None and reaches_all(hint_model): root = hint_model message = ( diff --git a/slayer/ingest_report.py b/slayer/ingest_report.py new file mode 100644 index 00000000..28cc1fc0 --- /dev/null +++ b/slayer/ingest_report.py @@ -0,0 +1,73 @@ +"""Shared conversion-report types for semantic-layer importers (DEV-1643). + +``ConversionWarning`` / ``ConversionResult`` were originally defined in +``slayer.dbt.converter``; they are extracted here so both the dbt importer and +the OSI importer (``slayer.osi.converter``) can reuse them without importing +each other. ``slayer.dbt.converter`` re-exports them for back-compat, so the +class objects are shared (identity-equal) across both import paths. +""" + +from collections import defaultdict +from typing import Literal + +from pydantic import BaseModel, Field + +from slayer.core.models import SlayerModel + + +class ConversionWarning(BaseModel): + """A structured entry in a conversion report. + + ``category`` groups entries in ``render_report``; ``severity`` is one of + ``"unconverted"`` (tried to convert, couldn't), ``"dropped"`` (intentional + clean-fail of an inexpressible construct), or ``"info"`` (a caveat — the + construct imports but has a runtime limitation). ``suggestion`` carries the + documented workaround. + """ + model_name: str | None = None + metric_name: str | None = None + message: str + category: str = "general" + severity: Literal["unconverted", "dropped", "info"] = "unconverted" + suggestion: str | None = None + + +class ConversionResult(BaseModel): + """Result of converting a semantic-layer project into SLayer models.""" + models: list[SlayerModel] = Field(default_factory=list) + unconverted_metrics: list[ConversionWarning] = Field(default_factory=list) + warnings: list[ConversionWarning] = Field(default_factory=list) + + def _all_entries(self) -> list[ConversionWarning]: + return list(self.unconverted_metrics) + list(self.warnings) + + def render_report(self) -> str: + """Render the conversion report grouped by category. + + Each category becomes a heading with a count; each entry lists its + entity, severity, reason, and (when present) the documented workaround. + """ + entries = self._all_entries() + if not entries: + return "No conversion issues." + by_cat: dict[str, list[ConversionWarning]] = defaultdict(list) + for e in entries: + by_cat[e.category or "general"].append(e) + lines: list[str] = [] + for cat in sorted(by_cat): + items = by_cat[cat] + lines.append(f"## {cat} ({len(items)})") + for e in items: + entity = e.metric_name or e.model_name or "general" + lines.append(f" - [{e.severity}] {entity}: {e.message}") + if e.suggestion: + lines.append(f" workaround: {e.suggestion}") + lines.append("") + return "\n".join(lines).rstrip() + + def tally(self) -> tuple[int, int]: + """``(unconverted, dropped)`` counts by severity for the CLI summary.""" + entries = self._all_entries() + unconverted = sum(1 for e in entries if e.severity == "unconverted") + dropped = sum(1 for e in entries if e.severity == "dropped") + return unconverted, dropped diff --git a/slayer/osi/__init__.py b/slayer/osi/__init__.py new file mode 100644 index 00000000..2746bbe5 --- /dev/null +++ b/slayer/osi/__init__.py @@ -0,0 +1 @@ +"""OSI (Open Semantic Interchange) importer for SLayer (DEV-1643).""" diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py new file mode 100644 index 00000000..c4b0c0a4 --- /dev/null +++ b/slayer/osi/converter.py @@ -0,0 +1,523 @@ +"""Convert parsed OSI documents into SLayer models (DEV-1643). + +Each OSI dataset becomes one ``SlayerModel``: its physical table is introspected +live (real column types + PK) and OSI semantic metadata (labels, descriptions, +is_time, ai_context, primary keys) is overlaid on top. OSI relationships become +``ModelJoin`` entries; OSI metrics become ``ModelMeasure`` formulas, anchored on +the model that reaches every dataset the metric references (via the shared +``recommend_root_model`` selection core). Constructs that cannot be expressed +exactly are clean-failed to a ``ConversionResult`` report, never silently lost. +""" + +from __future__ import annotations + +import logging +import re +from typing import Any, Optional + +import sqlalchemy as sa +import sqlglot +import sqlglot.expressions as exp + +from slayer.core.enums import DataType +from slayer.core.formula import parse_formula +from slayer.core.models import Column, ModelJoin, ModelMeasure, SlayerModel +from slayer.engine.ingestion import introspect_table_to_model +from slayer.engine.join_graph import JoinGraph, min_hops_root +from slayer.ingest_report import ConversionResult, ConversionWarning +from slayer.sql.client import _get_column_types_sync +from slayer.osi.expression import SQL_DIALECTS, convert_expression +from slayer.osi.models import ( + OSIAIContext, + OSICustomExtension, + OSIDataset, + OSIDocument, + OSIExpression, + OSIField, + OSIMetric, + OSIRelationship, + OSISemanticModel, + ai_context_to_dict, +) +from slayer.osi.source import parse_source, resolve_datasource + +logger = logging.getLogger(__name__) + +_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +# Dialects lacking a GROUP BY percentile/median aggregate (mirrors the dbt +# converter's caveat set). +_NO_PERCENTILE_DIALECTS = frozenset({"mysql", "tsql", "mssql", "sqlserver"}) + + +class OsiConversionError(Exception): + """Raised when an OSI import set cannot be converted (e.g. duplicate names).""" + + +def _legal_model_name(name: str) -> bool: + return "__" not in name and "." not in name and ":" not in name + + +def _legal_column_name(name: str) -> bool: + return "." not in name and ":" not in name + + +def _legal_measure_name(name: str) -> bool: + return bool(_IDENTIFIER_RE.match(name)) + + +def _is_bare_identifier(sql: str) -> bool: + return bool(_IDENTIFIER_RE.match(sql.strip())) + + +def _render_description(explicit: Optional[str], ctx: Optional[OSIAIContext]) -> Optional[str]: + """Description = explicit OSI description (lead) + ai_context instructions + + synonyms.""" + parts: list[str] = [] + if explicit: + parts.append(explicit) + ctx_dict = ai_context_to_dict(ctx) + if ctx_dict: + instructions = ctx_dict.get("instructions") + if instructions and instructions != explicit: + parts.append(instructions) + synonyms = ctx_dict.get("synonyms") + if synonyms: + parts.append("Synonyms: " + ", ".join(synonyms)) + return "\n".join(parts) or None + + +def _build_meta( + ctx: Optional[OSIAIContext], + custom_extensions: Optional[list[OSICustomExtension]], + extra: Optional[dict[str, Any]] = None, +) -> Optional[dict[str, Any]]: + meta: dict[str, Any] = dict(extra or {}) + ctx_dict = ai_context_to_dict(ctx) + if ctx_dict: + meta["osi_ai_context"] = ctx_dict + if custom_extensions: + meta["osi_custom_extensions"] = [e.model_dump() for e in custom_extensions] + return meta or None + + +class OsiToSlayerConverter: + """Convert OSI documents into SLayer models.""" + + def __init__( + self, + documents: list[OSIDocument], + data_source: str, + sa_engine: sa.Engine, + *, + dialect: str = "ANSI_SQL", + target_dialect: str | None = None, + ) -> None: + self.documents = documents + self.data_source = data_source + self.sa_engine = sa_engine + self.dialect = dialect + self.target_dialect = target_dialect + self._models: dict[str, SlayerModel] = {} + self._warnings: list[ConversionWarning] = [] + self._unconverted: list[ConversionWarning] = [] + + # ---- report helpers ---- + + def _warn(self, message: str, *, model_name: str | None = None, + metric_name: str | None = None, category: str = "general", + severity: str = "dropped", suggestion: str | None = None) -> None: + self._warnings.append(ConversionWarning( + model_name=model_name, metric_name=metric_name, message=message, + category=category, severity=severity, suggestion=suggestion, + )) + + def _unconv(self, message: str, *, metric_name: str, category: str = "metric", + suggestion: str | None = None) -> None: + self._unconverted.append(ConversionWarning( + metric_name=metric_name, message=message, category=category, + severity="unconverted", suggestion=suggestion, + )) + + # ---- top-level ---- + + def convert(self) -> ConversionResult: + inspector = sa.inspect(self.sa_engine) + + semantic_models = [sm for doc in self.documents for sm in doc.semantic_model] + self._check_duplicate_dataset_names(semantic_models) + + # dataset name -> its semantic model (for relationships/metrics scope) + sm_of_dataset: dict[str, OSISemanticModel] = {} + for sm in semantic_models: + for ds in sm.datasets: + if _legal_model_name(ds.name): + sm_of_dataset[ds.name] = sm + + for sm in semantic_models: + for ds in sm.datasets: + self._build_model(ds, sm, inspector) + + for sm in semantic_models: + for rel in sm.relationships or []: + self._build_join(rel) + + graph = JoinGraph.build_from_models(list(self._models.values())) + for sm in semantic_models: + sm_model_names = [d.name for d in sm.datasets if d.name in self._models] + for metric in sm.metrics or []: + self._build_measure(metric, sm_model_names, graph) + + return ConversionResult( + models=list(self._models.values()), + unconverted_metrics=self._unconverted, + warnings=self._warnings, + ) + + def _check_duplicate_dataset_names(self, sms: list[OSISemanticModel]) -> None: + seen: set[str] = set() + dupes: set[str] = set() + for sm in sms: + for ds in sm.datasets: + if ds.name in seen: + dupes.add(ds.name) + seen.add(ds.name) + if dupes: + raise OsiConversionError( + f"Duplicate dataset names across the OSI import set: " + f"{sorted(dupes)}. Dataset names map to SLayer model names and " + f"must be unique within a datasource." + ) + + # ---- datasets -> models ---- + + def _build_model(self, ds: OSIDataset, sm: OSISemanticModel, + inspector: sa.engine.Inspector) -> None: + if not _legal_model_name(ds.name): + self._warn( + f"Dataset name {ds.name!r} contains characters SLayer forbids in " + f"model names ('__', '.', ':'); skipping.", + model_name=ds.name, category="illegal_name", + ) + return + + parsed = parse_source(ds.source) + resolve_datasource(parsed.database, self.data_source) # stubbed routing + + try: + if parsed.is_query: + base = self._build_sql_mode_model(ds, parsed.query) + else: + base = introspect_table_to_model( + sa_engine=self.sa_engine, inspector=inspector, + table_name=parsed.table, schema=parsed.schema_name, + data_source=self.data_source, model_name=ds.name, + ) + except Exception as exc: # noqa: BLE001 — per-dataset isolation + self._warn( + f"Failed to introspect dataset {ds.name!r} (source {ds.source!r}): " + f"{exc}; skipping.", + model_name=ds.name, category="introspection", + ) + return + + self._overlay_fields(base, ds) + self._apply_dataset_metadata(base, ds, sm) + self._models[ds.name] = base + + def _build_sql_mode_model(self, ds: OSIDataset, query: str) -> SlayerModel: + # Query source: introspect the query's output columns live (LIMIT-0 / + # cursor-metadata probe), exactly as table sources are introspected. + # ``target_dialect`` is the datasource type, used for the dialect-correct + # probe (LIMIT 0 vs SELECT TOP vs SQLite's LIMIT-1 fallback). + types = _get_column_types_sync( + query, connection_string="", db_type=self.target_dialect, + engine=self.sa_engine, + ) + columns = [Column(name=name, type=category) for name, category in types.items()] + return SlayerModel(name=ds.name, sql=query, data_source=self.data_source, + columns=columns) + + def _overlay_fields(self, model: SlayerModel, ds: OSIDataset) -> None: + by_name = {c.name: c for c in model.columns} + introspected = set(by_name) + first_time_dim: str | None = None + + for field in ds.fields or []: + if not _legal_column_name(field.name): + self._warn( + f"Field name {field.name!r} on dataset {ds.name!r} contains " + f"'.'/':'; skipping the field.", + model_name=ds.name, category="illegal_name", + ) + continue + + sql = self._resolve_expression(field.expression) + if sql is None: + self._warn( + f"Field {field.name!r} on {ds.name!r} has no SQL-dialect " + f"expression; skipping.", + model_name=ds.name, category="dialect", + ) + continue + + is_time = bool(field.dimension and field.dimension.is_time) + col = self._resolve_field_column(field, sql, by_name, introspected, ds) + if col is None: + continue + + col.label = field.label or col.label + col.description = _render_description(field.description, field.ai_context) \ + or col.description + meta = _build_meta(field.ai_context, field.custom_extensions) + if meta: + col.meta = {**(col.meta or {}), **meta} + if is_time: + if col.type not in (DataType.DATE, DataType.TIMESTAMP): + col.type = DataType.TIMESTAMP + if first_time_dim is None: + first_time_dim = col.name + + if col.name not in by_name: + model.columns.append(col) + by_name[col.name] = col + + # OSI primary_key is authoritative. + for pk in ds.primary_key or []: + if pk in by_name: + by_name[pk].primary_key = True + + if first_time_dim and not model.default_time_dimension: + model.default_time_dimension = first_time_dim + + def _resolve_field_column(self, field: OSIField, sql: str, + by_name: dict[str, Column], introspected: set[str], + ds: OSIDataset) -> Column | None: + """Return the Column (existing or new) this field maps to, or None on a + clean-fail (already reported).""" + if _is_bare_identifier(sql): + if sql == field.name and field.name in by_name: + return by_name[field.name] # overlay existing column + if sql in introspected: + # aliased/renamed reference to a real column -> derived column + return Column(name=field.name, sql=sql, type=by_name[sql].type) + self._warn( + f"Field {field.name!r} on {ds.name!r} references column {sql!r} " + f"which is not present in the table; skipping.", + model_name=ds.name, category="missing_column", + ) + return None + # derived expression + is_time = bool(field.dimension and field.dimension.is_time) + return Column( + name=field.name, sql=sql, + type=DataType.TIMESTAMP if is_time else DataType.DOUBLE, + ) + + def _apply_dataset_metadata(self, model: SlayerModel, ds: OSIDataset, + sm: OSISemanticModel) -> None: + model.description = _render_description(ds.description, ds.ai_context) \ + or model.description + extra: dict[str, Any] = {} + if ds.unique_keys: + extra["osi_unique_keys"] = ds.unique_keys + sm_ctx = ai_context_to_dict(sm.ai_context) + if sm_ctx or sm.description: + extra["osi_semantic_model"] = { + "name": sm.name, + "description": sm.description, + "ai_context": sm_ctx, + } + meta = _build_meta(ds.ai_context, ds.custom_extensions, extra=extra) + if meta: + model.meta = {**(model.meta or {}), **meta} + + # ---- relationships -> joins ---- + + def _build_join(self, rel: OSIRelationship) -> None: + src = rel.from_dataset + if src not in self._models: + self._warn( + f"Relationship {rel.name!r} references unknown source dataset " + f"{src!r}; skipping.", + category="relationship", + ) + return + if rel.to not in self._models: + self._warn( + f"Relationship {rel.name!r} targets unknown dataset {rel.to!r}; " + f"skipping.", + model_name=src, category="relationship", + ) + return + if len(rel.from_columns) != len(rel.to_columns): + self._warn( + f"Relationship {rel.name!r} has mismatched key lengths " + f"({len(rel.from_columns)} vs {len(rel.to_columns)}); skipping.", + model_name=src, category="relationship", + ) + return + + pairs = [[f, t] for f, t in zip(rel.from_columns, rel.to_columns)] + self._models[src].joins.append(ModelJoin( + target_model=rel.to, + join_pairs=pairs, + description=_render_description(None, rel.ai_context), + meta=_build_meta(rel.ai_context, rel.custom_extensions), + )) + + # ---- metrics -> measures ---- + + def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], + graph: JoinGraph) -> None: + if not _legal_measure_name(metric.name): + self._unconv( + f"Metric name {metric.name!r} is not a valid SLayer identifier.", + metric_name=metric.name, + ) + return + + expr = self._resolve_expression(metric.expression) + if expr is None: + self._unconv( + f"Metric {metric.name!r} has no SQL-dialect expression.", + metric_name=metric.name, category="dialect", + ) + return + + owner_of = self._make_owner_of(sm_model_names) + anchor = self._select_anchor(metric.name, expr, sm_model_names, owner_of, graph) + if anchor is None: + return # already reported + + ref_of = self._make_ref_of(graph, anchor) + percentile_unsupported = ( + self.target_dialect is not None + and self.target_dialect.lower() in _NO_PERCENTILE_DIALECTS + ) + result = convert_expression( + expr, entity_name=metric.name, owner_of=owner_of, ref_of=ref_of, + percentile_unsupported=percentile_unsupported, + ) + if not result.ok: + self._unconv( + f"Metric {metric.name!r}: {result.reason}", + metric_name=metric.name, + ) + return + + try: + parse_formula(result.formula) + except Exception as exc: # noqa: BLE001 — reject un-parseable emission + self._unconv( + f"Metric {metric.name!r}: emitted formula {result.formula!r} is " + f"not a valid SLayer formula ({exc}).", + metric_name=metric.name, + ) + return + + self._materialize_columns(result.materialized) + self._models[anchor].measures.append(ModelMeasure( + formula=result.formula, + name=metric.name, + description=_render_description(metric.description, metric.ai_context), + meta=_build_meta(metric.ai_context, metric.custom_extensions), + )) + for w in result.warnings: + self._warn(f"Metric {metric.name!r}: {w}", metric_name=metric.name, + category="dialect", severity="info") + + def _select_anchor(self, metric_name: str, expr: str, sm_model_names: list[str], + owner_of, graph: JoinGraph) -> str | None: + owners = self._referenced_owners(expr, owner_of) + if owners: + anchor = min_hops_root(graph, sm_model_names, owners) + if anchor is None: + self._unconv( + f"Metric {metric_name!r} references models {sorted(owners)} " + f"with no single model reaching all of them via joins.", + metric_name=metric_name, category="no_join_path", + suggestion="Add the required relationship(s), or split the " + "metric across a multi-stage query.", + ) + return anchor + # No column references (e.g. COUNT(*)). Attribute it to the semantic + # model's unique fact table (the one dataset that is never a join + # target). When there is no unique fact table, the metric is an orphan + # with no determinable grain -> error rather than guess. + anchor = self._fact_root(sm_model_names) + if anchor is None: + self._unconv( + f"Metric {metric_name!r} has no column references and the " + f"semantic model has no unique fact table to attribute it to; " + f"its grain is ambiguous.", + metric_name=metric_name, category="orphan_metric", + suggestion="Aggregate over an explicit column, or ensure the " + "semantic model has a single fact dataset.", + ) + return anchor + + def _referenced_owners(self, expr: str, owner_of) -> set[str]: + try: + tree = sqlglot.parse_one(expr) + except sqlglot.errors.ParseError: + return set() + owners: set[str] = set() + for col in tree.find_all(exp.Column): + owner = owner_of(col.table or None, col.name) + if owner: + owners.add(owner) + return owners + + def _fact_root(self, sm_model_names: list[str]) -> str | None: + """The unique dataset that is never a join target (the fact table), or + ``None`` when there is no unique fact table (0 or >1 candidates).""" + if not sm_model_names: + return None + targets: set[str] = set() + for name in sm_model_names: + for j in self._models[name].joins: + targets.add(j.target_model) + non_targets = [n for n in sm_model_names if n not in targets] + return non_targets[0] if len(non_targets) == 1 else None + + def _make_owner_of(self, sm_model_names: list[str]): + def owner_of(qualifier: Optional[str], column: str) -> Optional[str]: + if qualifier is not None: + return qualifier if qualifier in self._models else None + for name in sm_model_names: + if any(c.name == column for c in self._models[name].columns): + return name + return None + return owner_of + + def _make_ref_of(self, graph: JoinGraph, anchor: str): + def ref_of(model: str, column: str) -> Optional[str]: + path = graph.shortest_path(anchor, model) + if path is None: + return None + return ".".join([*path, column]) + return ref_of + + def _materialize_columns(self, materialized) -> None: + for mc in materialized: + model = self._models.get(mc.owning_model) + if model is None: + continue + if any(c.name == mc.name for c in model.columns): + continue + model.columns.append(Column( + name=mc.name, sql=mc.sql, type=DataType.DOUBLE, hidden=True, + )) + + # ---- dialect selection ---- + + def _resolve_expression(self, osi_expr: OSIExpression) -> str | None: + """Pick the expression for the requested dialect, else fall back among + SQL-compatible dialects. Non-SQL-only expressions return None.""" + by_dialect = {de.dialect.value: de.expression for de in osi_expr.dialects} + if self.dialect in by_dialect: + return by_dialect[self.dialect] + for name, expression in by_dialect.items(): + if name in SQL_DIALECTS: + return expression + return None diff --git a/slayer/osi/expression.py b/slayer/osi/expression.py new file mode 100644 index 00000000..87a96135 --- /dev/null +++ b/slayer/osi/expression.py @@ -0,0 +1,293 @@ +"""OSI metric/field SQL expression -> SLayer formula transform (DEV-1643). + +An OSI metric carries a raw SQL aggregation expression (e.g. ``SUM(amount)``, +``(SUM(a)) / (COUNT(*))``, ``SUM(quantity * amount)``). SLayer measures use +colon syntax (``amount:sum``, ``*:count``) with arithmetic over aggregated refs. + +``convert_expression`` walks the sqlglot AST, replaces each *outermost* +aggregate subtree with a sentinel, lets sqlglot render the surrounding +arithmetic / scalar-function structure, then substitutes the SLayer ref for each +sentinel. Non-bare aggregate operands (arithmetic / scalar / CASE) are +materialized as hidden derived Columns on the operand's owning model. Anything +inexpressible is clean-failed (``ok=False``) with a reason. + +Model-awareness is injected via two callbacks so the transform is unit-testable +in isolation: +- ``owner_of(qualifier, column) -> model | None`` — which dataset owns a column. +- ``ref_of(model, column) -> anchor-relative dotted ref | None`` — the ref to + emit for a column on ``model`` (``None`` = unreachable from the anchor). +""" + +from __future__ import annotations + +import re +from typing import Callable, Optional + +import sqlglot +import sqlglot.expressions as exp +from pydantic import BaseModel + +from slayer.core.formula import SCALAR_PASSTHROUGH + +# Dialects whose expressions are SQL and can be fed to sqlglot / Column.sql. +SQL_DIALECTS = frozenset({"ANSI_SQL", "SNOWFLAKE", "DATABRICKS"}) + +_SENTINEL_PREFIX = "SLAYERTOKEN" +_SIMPLE_AGG = {exp.Sum: "sum", exp.Avg: "avg", exp.Min: "min", exp.Max: "max"} + +OwnerOf = Callable[[Optional[str], str], Optional[str]] +RefOf = Callable[[str, str], Optional[str]] + + +class MaterializedColumn(BaseModel): + """A hidden derived Column the converter must create on ``owning_model``.""" + + owning_model: str + name: str + sql: str + + +class ExprResult(BaseModel): + """Outcome of converting one OSI expression to a SLayer formula.""" + + ok: bool + formula: str | None = None + reason: str | None = None + materialized: list[MaterializedColumn] = [] + warnings: list[str] = [] + + +class _Unconvertible(Exception): + """Internal control-flow signal carrying a clean-fail reason.""" + + +class _Converter: + def __init__(self, entity_name: str, owner_of: OwnerOf, ref_of: RefOf, + percentile_unsupported: bool) -> None: + self.entity_name = entity_name + self.owner_of = owner_of + self.ref_of = ref_of + self.percentile_unsupported = percentile_unsupported + self.materialized: list[MaterializedColumn] = [] + self.warnings: list[str] = [] + self._dedup: dict[tuple[str, str], str] = {} + self._counter = 0 + + # ---- ref building for a single aggregate ---- + + def _column_ref(self, qualifier: Optional[str], column: str) -> str: + owner = self.owner_of(qualifier or None, column) + if owner is None: + raise _Unconvertible(f"cannot resolve owning dataset for column {column!r}") + ref = self.ref_of(owner, column) + if ref is None: + raise _Unconvertible( + f"column {column!r} on model {owner!r} is not reachable from the anchor" + ) + return ref + + def _materialize(self, operand: exp.Expression) -> str: + """Create/reuse a hidden derived column for a non-bare operand; return + its anchor-relative ref.""" + if operand.find(exp.AggFunc, exp.Window, exp.WithinGroup): + raise _Unconvertible("nested aggregate in aggregate operand") + columns = list(operand.find_all(exp.Column)) + owners = {self.owner_of(c.table or None, c.name) for c in columns} + owners.discard(None) + if len(owners) != 1: + raise _Unconvertible( + "aggregate operand spans multiple datasets (or resolves to none)" + ) + owner = owners.pop() + operand_sql = operand.sql() + key = (owner, operand_sql) + name = self._dedup.get(key) + if name is None: + name = f"_{self.entity_name}_{self._counter}" + self._counter += 1 + self._dedup[key] = name + self.materialized.append( + MaterializedColumn(owning_model=owner, name=name, sql=operand_sql) + ) + ref = self.ref_of(owner, name) + if ref is None: + raise _Unconvertible("materialized operand is not reachable from the anchor") + return ref + + def _operand_ref(self, operand: exp.Expression) -> str: + if isinstance(operand, exp.Column): + return self._column_ref(operand.table or None, operand.name) + return self._materialize(operand) + + def _agg_ref(self, node: exp.Expression) -> str: + """Return the ``ref:agg`` token for one outermost aggregate node.""" + # PERCENTILE_CONT/DISC(...) WITHIN GROUP (ORDER BY col) + if isinstance(node, exp.WithinGroup): + return self._percentile_ref(node) + + if type(node) in _SIMPLE_AGG: + agg = _SIMPLE_AGG[type(node)] + return f"{self._operand_ref(node.this)}:{agg}" + + if isinstance(node, exp.Count): + inner = node.this + if inner is None or isinstance(inner, exp.Star): + return "*:count" + if isinstance(inner, exp.Distinct): + exprs = inner.expressions + if len(exprs) != 1: + raise _Unconvertible("COUNT(DISTINCT ...) with multiple columns") + return f"{self._operand_ref(exprs[0])}:count_distinct" + return f"{self._operand_ref(inner)}:count" + + raise _Unconvertible(f"unsupported aggregation {node.sql_name()!r}") + + def _percentile_ref(self, node: exp.WithinGroup) -> str: + inner = node.this + if not isinstance(inner, (exp.PercentileCont, exp.PercentileDisc)): + raise _Unconvertible("unsupported WITHIN GROUP aggregate") + p_node = inner.this + if not (isinstance(p_node, exp.Literal) and p_node.is_number): + raise _Unconvertible("percentile fraction must be a numeric literal") + try: + p_val = float(p_node.name) + except ValueError as exc: # pragma: no cover - defensive + raise _Unconvertible("percentile fraction is not numeric") from exc + if not 0.0 <= p_val <= 1.0: + raise _Unconvertible("percentile fraction must be between 0 and 1") + + order = node.args.get("expression") + if not (isinstance(order, exp.Order) and order.expressions): + raise _Unconvertible("percentile WITHIN GROUP missing ORDER BY") + ordered = order.expressions[0] + col_node = ordered.this if isinstance(ordered, exp.Ordered) else ordered + if not isinstance(col_node, exp.Column): + raise _Unconvertible("percentile ORDER BY must be a bare column") + ref = self._column_ref(col_node.table or None, col_node.name) + + if self.percentile_unsupported: + self.warnings.append( + "percentile/median has no GROUP BY aggregate on the target dialect; " + "the measure imports but fails at query time there." + ) + if isinstance(inner, exp.PercentileCont) and p_val == 0.5: + return f"{ref}:median" + return f"{ref}:percentile(p={p_node.name})" + + # ---- residual validation ---- + + @staticmethod + def _is_sentinel(node: exp.Expression) -> bool: + return isinstance(node, exp.Column) and node.name.startswith(_SENTINEL_PREFIX) + + def _validate_residual(self, root: exp.Expression) -> None: + for node in root.walk(): + if isinstance(node, (exp.AggFunc, exp.Window, exp.WithinGroup)): + raise _Unconvertible("window/aggregate function not expressible") + if isinstance(node, exp.Case): + raise _Unconvertible("CASE outside an aggregate is not expressible") + if isinstance(node, exp.Column) and not self._is_sentinel(node): + raise _Unconvertible( + f"bare column {node.name!r} must appear inside an aggregation" + ) + if isinstance(node, exp.Literal) and node.is_string: + raise _Unconvertible("string literal is not expressible in a measure") + if isinstance(node, exp.Func) and not isinstance(node, exp.Count): + name = node.sql_name().lower() + if name not in SCALAR_PASSTHROUGH: + raise _Unconvertible(f"function {node.sql_name()!r} is not allowed") + + # ---- top-level ---- + + def convert(self, expr: str) -> ExprResult: + try: + tree = sqlglot.parse_one(expr) + except sqlglot.errors.ParseError as exc: + return self._fail(f"could not parse expression: {exc}") + + if tree.find(exp.Window): + return self._fail("window functions are not expressible; use a transform") + + try: + outermost = self._find_outermost_aggregates(tree) + replacements: list[tuple[exp.Expression, exp.Column]] = [] + for i, node in enumerate(outermost): + ref = self._agg_ref(node) + sentinel = exp.column(f"{_SENTINEL_PREFIX}{i}") + sentinel.meta["slayer_ref"] = ref + replacements.append((node, sentinel)) + + new_root = tree + ref_by_token: dict[str, str] = {} + for node, sentinel in replacements: + ref_by_token[sentinel.name] = sentinel.meta["slayer_ref"] + if node is new_root: + new_root = sentinel + else: + node.replace(sentinel) + + new_root = self._strip_redundant_parens(new_root) + self._validate_residual(new_root) + rendered = new_root.sql(normalize_functions="lower") + formula = self._substitute(rendered, ref_by_token) + except _Unconvertible as exc: + return self._fail(str(exc)) + + return ExprResult( + ok=True, formula=formula, + materialized=self.materialized, warnings=self.warnings, + ) + + @staticmethod + def _find_outermost_aggregates(tree: exp.Expression) -> list[exp.Expression]: + agg_types = (exp.AggFunc, exp.WithinGroup) + result: list[exp.Expression] = [] + for node in tree.find_all(*agg_types): + ancestor = node.parent + nested = False + while ancestor is not None: + if isinstance(ancestor, agg_types): + nested = True + break + ancestor = ancestor.parent + if not nested: + result.append(node) + return result + + @staticmethod + def _strip_redundant_parens(root: exp.Expression) -> exp.Expression: + """Drop parentheses that wrap a single atom (a sentinel ref or literal), + so ``(SUM(a)) / (COUNT(*))`` renders as ``a:sum / *:count``.""" + for paren in list(root.find_all(exp.Paren)): + if isinstance(paren.this, (exp.Column, exp.Literal)): + if paren is root: + root = paren.this + else: + paren.replace(paren.this) + return root + + @staticmethod + def _substitute(rendered: str, ref_by_token: dict[str, str]) -> str: + out = rendered + for token, ref in ref_by_token.items(): + out = re.sub(rf"\b{re.escape(token)}\b", ref, out) + return out + + def _fail(self, reason: str) -> ExprResult: + return ExprResult(ok=False, formula=None, reason=reason, + materialized=[], warnings=self.warnings) + + +def convert_expression( + expr: str, + *, + entity_name: str, + owner_of: OwnerOf, + ref_of: RefOf, + percentile_unsupported: bool = False, +) -> ExprResult: + """Convert an OSI SQL aggregation expression into a SLayer formula.""" + return _Converter( + entity_name=entity_name, owner_of=owner_of, ref_of=ref_of, + percentile_unsupported=percentile_unsupported, + ).convert(expr) diff --git a/slayer/osi/models.py b/slayer/osi/models.py new file mode 100644 index 00000000..3c161246 --- /dev/null +++ b/slayer/osi/models.py @@ -0,0 +1,165 @@ +"""Pydantic v2 models for OSI (Open Semantic Interchange) documents. + +Ported from the OSI reference package (``open-semantic-interchange/OSI``, +``python/src/osi/models.py``, Apache-2.0). The schema is stable across OSI spec +versions 1.0 / 0.1.0 / 0.1.1 / 0.2.0.dev0 — the only differences are the version +string and two optional document-level enum arrays, both absorbed by +``extra="ignore"``. + +Deviations from the reference package: +- ``extra="ignore"`` everywhere (forward/back-compat across spec versions). +- ``vendor_name`` is a free string (0.2.0 widened it from an enum). +- Models are not frozen (the importer does not mutate them, but leaving them + mutable avoids friction with Pydantic ``model_validate`` round-trips in tests). +""" + +from enum import Enum +from typing import Any, Optional, Union + +from pydantic import BaseModel, ConfigDict, Field + + +class OSIDialect(str, Enum): + """Supported SQL and expression language dialects.""" + + ANSI_SQL = "ANSI_SQL" + SNOWFLAKE = "SNOWFLAKE" + MDX = "MDX" + MAQL = "MAQL" + TABLEAU = "TABLEAU" + DATABRICKS = "DATABRICKS" + + +class OSIAIContextObject(BaseModel): + """Structured AI context with instructions, synonyms, and examples.""" + + model_config = ConfigDict(extra="allow") + + instructions: Optional[str] = None + synonyms: Optional[list[str]] = None + examples: Optional[list[str]] = None + + +# ai_context is either a plain string or the structured object above. +OSIAIContext = Union[str, OSIAIContextObject] + + +class OSICustomExtension(BaseModel): + """Vendor-specific metadata as a serialized JSON string.""" + + model_config = ConfigDict(extra="ignore") + + vendor_name: str + data: str + + +class OSIDialectExpression(BaseModel): + """Expression in a specific dialect.""" + + model_config = ConfigDict(extra="ignore") + + dialect: OSIDialect + expression: str + + +class OSIExpression(BaseModel): + """Expression definition with multi-dialect support.""" + + model_config = ConfigDict(extra="ignore") + + dialects: list[OSIDialectExpression] + + +class OSIDimension(BaseModel): + """Dimension metadata on a field.""" + + model_config = ConfigDict(extra="ignore") + + is_time: Optional[bool] = None + + +class OSIField(BaseModel): + """Row-level attribute for grouping, filtering, and metric expressions.""" + + model_config = ConfigDict(extra="ignore") + + name: str + expression: OSIExpression + dimension: Optional[OSIDimension] = None + label: Optional[str] = None + description: Optional[str] = None + ai_context: Optional[OSIAIContext] = None + custom_extensions: Optional[list[OSICustomExtension]] = None + + +class OSIDataset(BaseModel): + """Logical dataset representing a business entity (fact or dimension table).""" + + model_config = ConfigDict(extra="ignore") + + name: str + source: str + primary_key: Optional[list[str]] = None + unique_keys: Optional[list[list[str]]] = None + description: Optional[str] = None + ai_context: Optional[OSIAIContext] = None + fields: Optional[list[OSIField]] = None + custom_extensions: Optional[list[OSICustomExtension]] = None + + +class OSIRelationship(BaseModel): + """Foreign key relationship between datasets (``from`` = many, ``to`` = one).""" + + model_config = ConfigDict(extra="ignore", populate_by_name=True) + + name: str + from_dataset: str = Field(..., alias="from") + to: str + from_columns: list[str] + to_columns: list[str] + ai_context: Optional[OSIAIContext] = None + custom_extensions: Optional[list[OSICustomExtension]] = None + + +class OSIMetric(BaseModel): + """Quantitative measure defined on business data (raw SQL aggregation).""" + + model_config = ConfigDict(extra="ignore") + + name: str + expression: OSIExpression + description: Optional[str] = None + ai_context: Optional[OSIAIContext] = None + custom_extensions: Optional[list[OSICustomExtension]] = None + + +class OSISemanticModel(BaseModel): + """Top-level container representing a complete semantic model.""" + + model_config = ConfigDict(extra="ignore") + + name: str + description: Optional[str] = None + ai_context: Optional[OSIAIContext] = None + datasets: list[OSIDataset] + relationships: Optional[list[OSIRelationship]] = None + metrics: Optional[list[OSIMetric]] = None + custom_extensions: Optional[list[OSICustomExtension]] = None + + +class OSIDocument(BaseModel): + """Root OSI document.""" + + model_config = ConfigDict(extra="ignore") + + version: str = "0.2.0.dev0" + semantic_model: list[OSISemanticModel] + + +def ai_context_to_dict(ctx: Optional[OSIAIContext]) -> Optional[dict[str, Any]]: + """Normalize an ai_context (string or object) into a plain dict, or None.""" + if ctx is None: + return None + if isinstance(ctx, str): + return {"instructions": ctx} + return ctx.model_dump(exclude_none=True) diff --git a/slayer/osi/parser.py b/slayer/osi/parser.py new file mode 100644 index 00000000..0fcdf542 --- /dev/null +++ b/slayer/osi/parser.py @@ -0,0 +1,93 @@ +"""Parse OSI config files (YAML or JSON) into ``OSIDocument`` objects. + +``parse_osi_path`` accepts a single file or a directory (walked recursively). +Per-file parse/validation failures are logged and skipped (mirroring the dbt +parser's leniency). Known OSI spec versions parse silently; unknown versions +warn but are still attempted (the schema is stable across versions). +""" + +import json +import logging +import os +from pathlib import Path + +import yaml + +from slayer.osi.models import OSIDocument + +logger = logging.getLogger(__name__) + +# All OSI spec versions are structurally identical (verified via git diff of +# core-spec/osi-schema.json); only the version const and two optional top-level +# enum arrays differ. So every known version parses through the same models. +KNOWN_OSI_VERSIONS = frozenset({"1.0", "0.1.0", "0.1.1", "0.2.0.dev0"}) + +_SUFFIXES = (".yaml", ".yml", ".json") + + +def _collect_files(path: Path) -> list[Path]: + if path.is_file(): + return [path] + files: list[Path] = [] + for root, dirs, names in os.walk(path): + dirs[:] = [d for d in dirs if not d.startswith(".")] + for name in sorted(names): + if name.startswith("."): + continue + if name.endswith(_SUFFIXES): + files.append(Path(root) / name) + return files + + +def parse_osi_file(path: Path) -> OSIDocument | None: + """Parse a single OSI file into an ``OSIDocument`` (or ``None`` on failure).""" + try: + text = path.read_text(encoding="utf-8") + except OSError as exc: + logger.warning("Failed to read OSI file %s: %s", path, exc) + return None + + try: + if path.suffix == ".json": + data = json.loads(text) + else: + data = yaml.safe_load(text) + except (yaml.YAMLError, json.JSONDecodeError) as exc: + logger.warning("Failed to parse OSI file %s: %s", path, exc) + return None + + if not isinstance(data, dict): + logger.warning("OSI file %s is not a mapping; skipping", path) + return None + + version = data.get("version") + if version is not None and version not in KNOWN_OSI_VERSIONS: + logger.warning( + "OSI file %s declares unknown spec version %r (known: %s); " + "attempting to parse anyway.", + path, version, ", ".join(sorted(KNOWN_OSI_VERSIONS)), + ) + + try: + return OSIDocument.model_validate(data) + except Exception as exc: # noqa: BLE001 — any validation error -> skip + logger.warning("Failed to validate OSI document in %s: %s", path, exc) + return None + + +def parse_osi_path(path: str | Path) -> list[OSIDocument]: + """Parse an OSI file or directory into a list of ``OSIDocument`` objects.""" + root = Path(path) + if not root.exists(): + raise FileNotFoundError(f"OSI path does not exist: {root}") + + files = _collect_files(root) + if not files: + logger.warning("No OSI files (.yaml/.yml/.json) found in %s", root) + + docs: list[OSIDocument] = [] + for f in files: + doc = parse_osi_file(f) + if doc is not None: + docs.append(doc) + return docs diff --git a/slayer/osi/source.py b/slayer/osi/source.py new file mode 100644 index 00000000..e344d661 --- /dev/null +++ b/slayer/osi/source.py @@ -0,0 +1,93 @@ +"""Parse an OSI ``Dataset.source`` into its physical components (DEV-1643). + +``source`` is either a dotted physical identifier (``[catalog.]db.schema.table``, +optionally double-quoted per segment) or a raw SQL query. The identifier form is +split table-last / schema-second-last / database-the-rest; the query form is +carried through to SLayer sql-mode. + +The parsed ``database`` is currently dropped — every dataset binds to the +importer's ``--datasource``. ``resolve_datasource`` is the stubbed extension +point for future per-database routing. +""" + +from __future__ import annotations + +import re + +from pydantic import BaseModel + +_SELECT_RE = re.compile(r"\bselect\b", re.IGNORECASE) + + +class ParsedSource(BaseModel): + """A parsed OSI dataset source.""" + + database: str | None = None + schema_name: str | None = None + table: str | None = None + query: str | None = None + is_query: bool = False + + +def _has_top_level_space(s: str) -> bool: + """True if ``s`` contains whitespace outside of double-quoted spans.""" + in_quote = False + for ch in s: + if ch == '"': + in_quote = not in_quote + elif ch.isspace() and not in_quote: + return True + return False + + +def _looks_like_query(s: str) -> bool: + stripped = s.strip() + return ( + stripped.startswith("(") + or _has_top_level_space(stripped) + or bool(_SELECT_RE.search(stripped)) + ) + + +def _split_identifier(s: str) -> list[str]: + """Split a dotted identifier on unquoted dots, stripping double-quotes.""" + parts: list[str] = [] + cur: list[str] = [] + in_quote = False + for ch in s: + if ch == '"': + in_quote = not in_quote + continue + if ch == "." and not in_quote: + parts.append("".join(cur)) + cur = [] + else: + cur.append(ch) + parts.append("".join(cur)) + return parts + + +def parse_source(source: str) -> ParsedSource: + """Parse an OSI ``Dataset.source`` string.""" + if _looks_like_query(source): + return ParsedSource(is_query=True, query=source.strip()) + + parts = [p for p in _split_identifier(source.strip()) if p != ""] + if not parts: + return ParsedSource(is_query=True, query=source.strip()) + + table = parts[-1] + schema_name = parts[-2] if len(parts) >= 2 else None + database = ".".join(parts[:-2]) if len(parts) >= 3 else None + return ParsedSource(database=database, schema_name=schema_name, table=table) + + +def resolve_datasource(database: str | None, default: str) -> str: + """Map an OSI dataset's ``database`` to a SLayer datasource name. + + Stubbed extension point (DEV-1643): for now the parsed database is dropped + and every dataset binds to ``default`` (the importer's ``--datasource``). A + future version can route different OSI databases to different SLayer + datasources here without reworking the converter. + """ + return default diff --git a/tests/fixtures/osi/aicontext_string.yaml b/tests/fixtures/osi/aicontext_string.yaml new file mode 100644 index 00000000..e82029f1 --- /dev/null +++ b/tests/fixtures/osi/aicontext_string.yaml @@ -0,0 +1,11 @@ +version: "0.1.1" +semantic_model: + - name: s + ai_context: "plain string context" + datasets: + - name: t + source: t + ai_context: "dataset string ctx" + fields: + - name: c + expression: {dialects: [{dialect: ANSI_SQL, expression: c}]} diff --git a/tests/fixtures/osi/malformed.yaml b/tests/fixtures/osi/malformed.yaml new file mode 100644 index 00000000..45fd2871 --- /dev/null +++ b/tests/fixtures/osi/malformed.yaml @@ -0,0 +1,2 @@ +version: "0.2.0.dev0" +semantic_model: [ this is : not valid yaml diff --git a/tests/fixtures/osi/shop.json b/tests/fixtures/osi/shop.json new file mode 100644 index 00000000..e11041bf --- /dev/null +++ b/tests/fixtures/osi/shop.json @@ -0,0 +1,396 @@ +{ + "version": "0.2.0.dev0", + "semantic_model": [ + { + "name": "shop", + "description": "Retail shop semantic model", + "ai_context": { + "instructions": "Use for order, customer, and product analytics.", + "synonyms": [ + "store", + "retail" + ] + }, + "datasets": [ + { + "name": "orders", + "source": "orders", + "primary_key": [ + "order_id" + ], + "description": "Order line items", + "ai_context": { + "instructions": "One row per order.", + "synonyms": [ + "sales", + "purchases" + ] + }, + "fields": [ + { + "name": "order_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "order_id" + } + ] + }, + "description": "Order id" + }, + { + "name": "customer_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "customer_id" + } + ] + } + }, + { + "name": "product_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "product_id" + } + ] + } + }, + { + "name": "amount", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "amount" + } + ] + }, + "label": "Order amount", + "ai_context": { + "instructions": "Gross order value in USD.", + "synonyms": [ + "revenue", + "gross" + ] + }, + "custom_extensions": [ + { + "vendor_name": "SNOWFLAKE", + "data": "{\"unit\": \"usd\"}" + } + ] + }, + { + "name": "quantity", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "quantity" + } + ] + } + }, + { + "name": "ordered_at", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "ordered_at" + } + ] + }, + "dimension": { + "is_time": true + } + }, + { + "name": "status", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "status" + } + ] + } + } + ] + }, + { + "name": "customers", + "source": "customers", + "primary_key": [ + "customer_id" + ], + "fields": [ + { + "name": "customer_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "customer_id" + } + ] + } + }, + { + "name": "region_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "region_id" + } + ] + } + }, + { + "name": "name", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "name" + } + ] + } + }, + { + "name": "segment", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "segment" + } + ] + } + } + ] + }, + { + "name": "products", + "source": "products", + "primary_key": [ + "product_id" + ], + "fields": [ + { + "name": "product_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "product_id" + } + ] + } + }, + { + "name": "category", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "category" + } + ] + } + }, + { + "name": "price", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "price" + } + ] + } + } + ] + }, + { + "name": "regions", + "source": "regions", + "primary_key": [ + "region_id" + ], + "fields": [ + { + "name": "region_id", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "region_id" + } + ] + } + }, + { + "name": "name", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "name" + } + ] + } + }, + { + "name": "population", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "population" + } + ] + } + } + ] + } + ], + "relationships": [ + { + "name": "orders_to_customers", + "from": "orders", + "to": "customers", + "from_columns": [ + "customer_id" + ], + "to_columns": [ + "customer_id" + ], + "ai_context": { + "instructions": "Each order has one customer." + } + }, + { + "name": "orders_to_products", + "from": "orders", + "to": "products", + "from_columns": [ + "product_id" + ], + "to_columns": [ + "product_id" + ] + }, + { + "name": "customers_to_regions", + "from": "customers", + "to": "regions", + "from_columns": [ + "region_id" + ], + "to_columns": [ + "region_id" + ] + } + ], + "metrics": [ + { + "name": "total_amount", + "description": "Total order amount", + "ai_context": { + "instructions": "Sum of gross order value.", + "synonyms": [ + "gmv" + ] + }, + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "SUM(amount)" + } + ] + } + }, + { + "name": "order_count", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "COUNT(*)" + } + ] + } + }, + { + "name": "aov", + "description": "Average order value", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "(SUM(amount)) / (COUNT(*))" + } + ] + } + }, + { + "name": "revenue_line", + "description": "Quantity-weighted revenue (materialized operand)", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "SUM(quantity * amount)" + } + ] + } + }, + { + "name": "cust_reach", + "description": "Amount per distinct customer (cross-dataset, anchor = orders)", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "SUM(amount) / COUNT(DISTINCT customers.customer_id)" + } + ] + } + }, + { + "name": "rev_plus_pop", + "description": "Amount plus region population (multi-hop orders->customers->regions)", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "SUM(amount) + SUM(regions.population)" + } + ] + } + }, + { + "name": "bridge_metric", + "description": "References products + customers only; must anchor on orders (bridge)", + "expression": { + "dialects": [ + { + "dialect": "ANSI_SQL", + "expression": "SUM(products.price) + COUNT(DISTINCT customers.customer_id)" + } + ] + } + } + ] + } + ] +} diff --git a/tests/fixtures/osi/shop.yaml b/tests/fixtures/osi/shop.yaml new file mode 100644 index 00000000..1c360651 --- /dev/null +++ b/tests/fixtures/osi/shop.yaml @@ -0,0 +1,138 @@ +# Crafted OSI fixture for SLayer import-osi tests. +# Sources are BARE table names so SQLite introspection (no schemas) resolves them. +version: "0.2.0.dev0" + +semantic_model: + - name: shop + description: Retail shop semantic model + ai_context: + instructions: "Use for order, customer, and product analytics." + synonyms: + - "store" + - "retail" + + datasets: + - name: orders + source: orders + primary_key: [order_id] + description: Order line items + ai_context: + instructions: "One row per order." + synonyms: + - "sales" + - "purchases" + fields: + - name: order_id + expression: {dialects: [{dialect: ANSI_SQL, expression: order_id}]} + description: Order id + - name: customer_id + expression: {dialects: [{dialect: ANSI_SQL, expression: customer_id}]} + - name: product_id + expression: {dialects: [{dialect: ANSI_SQL, expression: product_id}]} + - name: amount + expression: {dialects: [{dialect: ANSI_SQL, expression: amount}]} + label: Order amount + ai_context: + instructions: "Gross order value in USD." + synonyms: + - "revenue" + - "gross" + custom_extensions: + - vendor_name: SNOWFLAKE + data: '{"unit": "usd"}' + - name: quantity + expression: {dialects: [{dialect: ANSI_SQL, expression: quantity}]} + - name: ordered_at + expression: {dialects: [{dialect: ANSI_SQL, expression: ordered_at}]} + dimension: {is_time: true} + - name: status + expression: {dialects: [{dialect: ANSI_SQL, expression: status}]} + + - name: customers + source: customers + primary_key: [customer_id] + fields: + - name: customer_id + expression: {dialects: [{dialect: ANSI_SQL, expression: customer_id}]} + - name: region_id + expression: {dialects: [{dialect: ANSI_SQL, expression: region_id}]} + - name: name + expression: {dialects: [{dialect: ANSI_SQL, expression: name}]} + - name: segment + expression: {dialects: [{dialect: ANSI_SQL, expression: segment}]} + + - name: products + source: products + primary_key: [product_id] + fields: + - name: product_id + expression: {dialects: [{dialect: ANSI_SQL, expression: product_id}]} + - name: category + expression: {dialects: [{dialect: ANSI_SQL, expression: category}]} + - name: price + expression: {dialects: [{dialect: ANSI_SQL, expression: price}]} + + - name: regions + source: regions + primary_key: [region_id] + fields: + - name: region_id + expression: {dialects: [{dialect: ANSI_SQL, expression: region_id}]} + - name: name + expression: {dialects: [{dialect: ANSI_SQL, expression: name}]} + - name: population + expression: {dialects: [{dialect: ANSI_SQL, expression: population}]} + + relationships: + - name: orders_to_customers + from: orders + to: customers + from_columns: [customer_id] + to_columns: [customer_id] + ai_context: + instructions: "Each order has one customer." + - name: orders_to_products + from: orders + to: products + from_columns: [product_id] + to_columns: [product_id] + - name: customers_to_regions + from: customers + to: regions + from_columns: [region_id] + to_columns: [region_id] + + metrics: + - name: total_amount + description: Total order amount + ai_context: + instructions: "Sum of gross order value." + synonyms: + - "gmv" + expression: {dialects: [{dialect: ANSI_SQL, expression: SUM(amount)}]} + - name: order_count + expression: {dialects: [{dialect: ANSI_SQL, expression: COUNT(*)}]} + - name: aov + description: Average order value + expression: {dialects: [{dialect: ANSI_SQL, expression: (SUM(amount)) / (COUNT(*))}]} + - name: revenue_line + description: Quantity-weighted revenue (materialized operand) + expression: {dialects: [{dialect: ANSI_SQL, expression: SUM(quantity * amount)}]} + - name: cust_reach + description: Amount per distinct customer (cross-dataset, anchor = orders) + expression: + dialects: + - dialect: ANSI_SQL + expression: SUM(amount) / COUNT(DISTINCT customers.customer_id) + - name: rev_plus_pop + description: Amount plus region population (multi-hop orders->customers->regions) + expression: + dialects: + - dialect: ANSI_SQL + expression: SUM(amount) + SUM(regions.population) + - name: bridge_metric + description: References products + customers only; must anchor on orders (bridge) + expression: + dialects: + - dialect: ANSI_SQL + expression: SUM(products.price) + COUNT(DISTINCT customers.customer_id) diff --git a/tests/fixtures/osi/unknown_version.yaml b/tests/fixtures/osi/unknown_version.yaml new file mode 100644 index 00000000..c2016759 --- /dev/null +++ b/tests/fixtures/osi/unknown_version.yaml @@ -0,0 +1,9 @@ +version: "9.9.9" +semantic_model: + - name: v + datasets: + - name: t + source: t + fields: + - name: c + expression: {dialects: [{dialect: ANSI_SQL, expression: c}]} diff --git a/tests/test_cli_import_osi.py b/tests/test_cli_import_osi.py new file mode 100644 index 00000000..99526410 --- /dev/null +++ b/tests/test_cli_import_osi.py @@ -0,0 +1,82 @@ +"""End-to-end CLI test for `slayer import-osi`. + +Registers a file-backed SQLite datasource, runs the importer over the crafted +shop.yaml fixture, and asserts models (with overlaid measures/joins) are saved +and the conversion report is printed. +""" + +from pathlib import Path +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa + +from slayer.async_utils import run_sync +from slayer.cli import _run_import_osi +from slayer.core.models import DatasourceConfig +from slayer.storage.yaml_storage import YAMLStorage + +FIXTURES = Path(__file__).parent / "fixtures" / "osi" + +_SCHEMA = [ + "CREATE TABLE orders (order_id INTEGER PRIMARY KEY, customer_id INTEGER, " + "product_id INTEGER, amount REAL, quantity INTEGER, ordered_at DATE, status TEXT)", + "CREATE TABLE customers (customer_id INTEGER PRIMARY KEY, region_id INTEGER, " + "name TEXT, segment TEXT)", + "CREATE TABLE products (product_id INTEGER PRIMARY KEY, category TEXT, price REAL)", + "CREATE TABLE regions (region_id INTEGER PRIMARY KEY, name TEXT, population INTEGER)", +] + + +@pytest.fixture +def shop_setup(tmp_path: Path): + db = tmp_path / "shop.db" + engine = sa.create_engine(f"sqlite:///{db}") + with engine.connect() as conn: + for ddl in _SCHEMA: + conn.execute(sa.text(ddl)) + conn.commit() + engine.dispose() + + store = tmp_path / "store" + storage = YAMLStorage(base_dir=str(store)) + run_sync(storage.save_datasource( + DatasourceConfig(name="testds", type="sqlite", database=str(db)) + )) + return store, db + + +def _args(store: Path, path: Path) -> SimpleNamespace: + return SimpleNamespace( + osi_path=str(path), + datasource="testds", + dialect="ANSI_SQL", + storage=str(store), + models_dir=None, + ) + + +def test_import_osi_saves_models_and_prints_report(shop_setup, capsys) -> None: + store, _ = shop_setup + _run_import_osi(_args(store, FIXTURES / "shop.yaml")) + + out = capsys.readouterr().out + assert "orders" in out # per-model summary printed + + storage = YAMLStorage(base_dir=str(store)) + names = run_sync(storage.list_models()) + assert {"orders", "customers", "products", "regions"}.issubset(set(names)) + + orders = run_sync(storage.get_model("orders", data_source="testds")) + assert orders is not None + measure_names = {m.name for m in orders.measures} + assert {"total_amount", "order_count", "aov"}.issubset(measure_names) + # relationship -> join persisted + assert any(j.target_model == "customers" for j in orders.joins) + + +def test_import_osi_missing_datasource_exits(tmp_path: Path) -> None: + store = tmp_path / "empty_store" + YAMLStorage(base_dir=str(store)) # no datasource registered + with pytest.raises(SystemExit): + _run_import_osi(_args(store, FIXTURES / "shop.yaml")) diff --git a/tests/test_ingest_report_reexport.py b/tests/test_ingest_report_reexport.py new file mode 100644 index 00000000..0b6decbe --- /dev/null +++ b/tests/test_ingest_report_reexport.py @@ -0,0 +1,41 @@ +"""The conversion-report classes move to a neutral module (DEV-1643). + +``ConversionResult`` / ``ConversionWarning`` are extracted from +``slayer.dbt.converter`` into ``slayer.ingest_report`` so the OSI importer can +reuse them without importing the dbt package. The dbt module re-exports them, +so existing imports keep working and it's the *same* class object. +""" + +from slayer.dbt.converter import ConversionResult as DbtConversionResult +from slayer.dbt.converter import ConversionWarning as DbtConversionWarning +from slayer.ingest_report import ConversionResult, ConversionWarning + + +def test_reexport_is_same_class() -> None: + assert DbtConversionResult is ConversionResult + assert DbtConversionWarning is ConversionWarning + + +def test_render_report_and_tally() -> None: + result = ConversionResult( + models=[], + unconverted_metrics=[ + ConversionWarning( + metric_name="m1", + message="cannot convert", + category="expr", + severity="unconverted", + ) + ], + warnings=[ + ConversionWarning( + model_name="mdl", + message="dropped a thing", + category="shape", + severity="dropped", + ) + ], + ) + report = result.render_report() + assert "m1" in report and "cannot convert" in report + assert result.tally() == (1, 1) # (unconverted, dropped) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py new file mode 100644 index 00000000..77ff5000 --- /dev/null +++ b/tests/test_osi_converter.py @@ -0,0 +1,519 @@ +"""OSI -> SLayer conversion (slayer/osi/converter.py), with live introspection. + +A file-backed SQLite DB provides real column types / PKs; the converter overlays +OSI semantic metadata (labels, descriptions, is_time, relationships->joins, +metrics->measures) on top. +""" + +from pathlib import Path + +import pytest +import sqlalchemy as sa + +from slayer.core.enums import DataType, JoinType +from slayer.osi.converter import OsiConversionError, OsiToSlayerConverter +from slayer.osi.models import ( + OSIDataset, + OSIDialectExpression, + OSIDocument, + OSIExpression, + OSIField, + OSIMetric, + OSIRelationship, + OSISemanticModel, +) +from slayer.osi.parser import parse_osi_path + +FIXTURES = Path(__file__).parent / "fixtures" / "osi" + +_SCHEMA = [ + "CREATE TABLE orders (order_id INTEGER PRIMARY KEY, customer_id INTEGER, " + "product_id INTEGER, amount REAL, quantity INTEGER, ordered_at DATE, status TEXT)", + "CREATE TABLE customers (customer_id INTEGER PRIMARY KEY, region_id INTEGER, " + "name TEXT, segment TEXT)", + "CREATE TABLE products (product_id INTEGER PRIMARY KEY, category TEXT, price REAL)", + "CREATE TABLE regions (region_id INTEGER PRIMARY KEY, name TEXT, population INTEGER)", + "CREATE TABLE ckey_parent (k1 INTEGER, k2 INTEGER, label TEXT, PRIMARY KEY (k1, k2))", + "CREATE TABLE ckey_child (k1 INTEGER, k2 INTEGER, v REAL)", +] + + +@pytest.fixture +def shop_engine(tmp_path: Path) -> sa.Engine: + engine = sa.create_engine(f"sqlite:///{tmp_path}/shop.db") + with engine.connect() as conn: + for ddl in _SCHEMA: + conn.execute(sa.text(ddl)) + conn.commit() + return engine + + +def _convert(engine: sa.Engine, doc: OSIDocument, **kw): + return OsiToSlayerConverter( + documents=[doc], data_source="testds", sa_engine=engine, **kw + ).convert() + + +def _shop_result(engine: sa.Engine): + doc = parse_osi_path(FIXTURES / "shop.yaml")[0] + return _convert(engine, doc) + + +def _by_name(result): + return {m.name: m for m in result.models} + + +def _reported(result) -> bool: + """True if the conversion report has any entry (public surface).""" + return bool(result.warnings or result.unconverted_metrics) + + +def _expr(sql: str, dialect: str = "ANSI_SQL") -> OSIExpression: + return OSIExpression(dialects=[OSIDialectExpression(dialect=dialect, expression=sql)]) + + +# ─────────────────────────── datasets -> models ──────────────────────────── + +def test_one_model_per_dataset(shop_engine): + models = _by_name(_shop_result(shop_engine)) + assert set(models) == {"orders", "customers", "products", "regions"} + + +def test_introspected_types_and_pk(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + cols = {c.name: c for c in orders.columns} + assert cols["order_id"].primary_key is True + assert cols["amount"].type == DataType.DOUBLE # REAL + assert cols["quantity"].type == DataType.INT # INTEGER + assert orders.sql_table == "orders" + + +def test_is_time_field_typed_temporal_and_default_time_dim(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + ordered_at = {c.name: c for c in orders.columns}["ordered_at"] + assert ordered_at.type in (DataType.DATE, DataType.TIMESTAMP) + assert orders.default_time_dimension == "ordered_at" + + +# ─────────────────────────── ai_context overlay ──────────────────────────── + +def test_field_ai_context_into_description_and_meta(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + amount = {c.name: c for c in orders.columns}["amount"] + assert amount.label == "Order amount" + # instructions AND synonyms both go into description. + assert "Gross order value in USD." in amount.description + assert "revenue" in amount.description and "gross" in amount.description + # full blob preserved in meta. + assert amount.meta["osi_ai_context"]["instructions"] == "Gross order value in USD." + assert amount.meta["osi_ai_context"]["synonyms"] == ["revenue", "gross"] + + +def test_model_and_semantic_model_ai_context(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + assert "One row per order." in orders.description + assert "sales" in orders.description + assert orders.meta["osi_ai_context"]["instructions"] == "One row per order." + # semantic-model-level ai_context lands on every derived model's meta. + assert "osi_semantic_model" in orders.meta + + +# ─────────────────────────── relationships -> joins ───────────────────────── + +def test_joins_from_relationships(shop_engine): + models = _by_name(_shop_result(shop_engine)) + ojoins = {j.target_model: j for j in models["orders"].joins} + assert set(ojoins) == {"customers", "products"} + assert ojoins["customers"].join_type == JoinType.LEFT + assert ojoins["customers"].join_pairs == [["customer_id", "customer_id"]] + cjoins = {j.target_model: j for j in models["customers"].joins} + assert "regions" in cjoins + + +def test_join_carries_relationship_ai_context(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + cj = {j.target_model: j for j in orders.joins}["customers"] + assert "Each order has one customer." in (cj.description or "") + + +# ─────────────────────────── metrics -> measures ──────────────────────────── + +def test_simple_and_ratio_measures(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + m = {meas.name: meas for meas in orders.measures} + assert m["total_amount"].formula == "amount:sum" + assert m["order_count"].formula == "*:count" + assert m["aov"].formula == "amount:sum / *:count" + + +def test_materialized_derived_column_metric(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + measure = {meas.name: meas for meas in orders.measures}["revenue_line"] + # A hidden derived column was created for quantity*amount. + hidden = [c for c in orders.columns if c.hidden and c.sql] + assert any( + {"quantity", "amount"}.issubset(c.sql.replace("*", " ").split()) for c in hidden + ) + derived_name = measure.formula.split(":")[0] + assert any(c.name == derived_name and c.hidden for c in orders.columns) + assert measure.formula.endswith(":sum") + + +def test_cross_dataset_metric_anchors_and_dotted_ref(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + m = {meas.name: meas for meas in orders.measures} + assert m["cust_reach"].formula == "amount:sum / customers.customer_id:count_distinct" + + +def test_multihop_metric_anchor_relative_path(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + m = {meas.name: meas for meas in orders.measures} + assert m["rev_plus_pop"].formula == "amount:sum + customers.regions.population:sum" + + +# ─────────────────────────── edge / failure cases ─────────────────────────── + +def _mini_doc(datasets, relationships=None, metrics=None, name="s"): + return OSIDocument( + version="0.2.0.dev0", + semantic_model=[ + OSISemanticModel( + name=name, + datasets=datasets, + relationships=relationships, + metrics=metrics, + ) + ], + ) + + +def test_duplicate_dataset_names_raise(shop_engine): + doc = OSIDocument( + version="0.2.0.dev0", + semantic_model=[ + OSISemanticModel( + name="a", + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + ), + OSISemanticModel( + name="b", + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + ), + ], + ) + with pytest.raises(OsiConversionError): + _convert(shop_engine, doc) + + +def test_illegal_dataset_name_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + OSIDataset(name="orders.bad", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + ] + ) + result = _convert(shop_engine, doc) + names = {m.name for m in result.models} + assert "orders" in names and "orders.bad" not in names + assert _reported(result) # a report entry exists + + +def test_illegal_field_name_clean_fails_field_not_model(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[ + OSIField(name="amount", expression=_expr("amount")), + OSIField(name="bad:name", expression=_expr("status")), + ], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + colnames = {c.name for c in orders.columns} + assert "amount" in colnames and "bad:name" not in colnames + assert _reported(result) + + +def test_composite_key_join(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="ckey_child", source="ckey_child", primary_key=["k1", "k2"], + fields=[OSIField(name="v", expression=_expr("v"))]), + OSIDataset(name="ckey_parent", source="ckey_parent", primary_key=["k1", "k2"], + fields=[OSIField(name="label", expression=_expr("label"))]), + ], + relationships=[OSIRelationship( + name="c2p", **{"from": "ckey_child"}, to="ckey_parent", + from_columns=["k1", "k2"], to_columns=["k1", "k2"], + )], + ) + result = _convert(shop_engine, doc) + child = {m.name: m for m in result.models}["ckey_child"] + join = {j.target_model: j for j in child.joins}["ckey_parent"] + assert join.join_pairs == [["k1", "k1"], ["k2", "k2"]] + + +def test_composite_key_length_mismatch_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="ckey_child", source="ckey_child", + fields=[OSIField(name="v", expression=_expr("v"))]), + OSIDataset(name="ckey_parent", source="ckey_parent", + fields=[OSIField(name="label", expression=_expr("label"))]), + ], + relationships=[OSIRelationship( + name="bad", **{"from": "ckey_child"}, to="ckey_parent", + from_columns=["k1", "k2"], to_columns=["k1"], + )], + ) + result = _convert(shop_engine, doc) + child = {m.name: m for m in result.models}["ckey_child"] + assert child.joins == [] # mismatched relationship dropped + assert _reported(result) + + +def test_missing_column_field_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[ + OSIField(name="amount", expression=_expr("amount")), + OSIField(name="ghost", expression=_expr("no_such_col")), + ], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "ghost" not in {c.name for c in orders.columns} + assert _reported(result) + + +def test_aliased_field_expression_pointing_at_real_column(shop_engine): + # field name != a table column, but expression names a real column -> derived col added. + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="amt", expression=_expr("amount"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + amt = {c.name: c for c in orders.columns}.get("amt") + assert amt is not None and amt.sql == "amount" + + +def test_non_sql_dialect_metric_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="mdx_metric", + expression=OSIExpression(dialects=[ + OSIDialectExpression(dialect="MDX", expression="[Measures].[x]")]))], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "mdx_metric" not in {meas.name for meas in orders.measures} + assert _reported(result) + + +def test_per_dataset_failure_isolation(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + OSIDataset(name="phantom", source="no_such_table", + fields=[OSIField(name="x", expression=_expr("x"))]), + ] + ) + result = _convert(shop_engine, doc) + names = {m.name for m in result.models} + assert "orders" in names and "phantom" not in names + assert _reported(result) + + +def test_unique_keys_into_meta(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", unique_keys=[["order_id"]], + fields=[OSIField(name="amount", expression=_expr("amount"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert orders.meta["osi_unique_keys"] == [["order_id"]] + + +# ─────────────── anchoring: bridge model + COUNT(*) fact-root ──────────────── + +def test_bridge_anchor_metric(shop_engine): + # bridge_metric references products + customers only; orders owns neither + # column but is the only model reaching both -> anchor on orders. + orders = _by_name(_shop_result(shop_engine))["orders"] + m = {meas.name: meas for meas in orders.measures} + assert "bridge_metric" in m + assert m["bridge_metric"].formula == ( + "products.price:sum + customers.customer_id:count_distinct" + ) + + +def test_count_star_anchors_on_fact_root(shop_engine): + # A column-less COUNT(*) metric anchors on the fact root — the dataset that + # is never the target of a relationship (ckey_child here). + doc = _mini_doc( + datasets=[ + OSIDataset(name="ckey_child", source="ckey_child", + fields=[OSIField(name="v", expression=_expr("v"))]), + OSIDataset(name="ckey_parent", source="ckey_parent", + fields=[OSIField(name="label", expression=_expr("label"))]), + ], + relationships=[OSIRelationship( + name="c2p", **{"from": "ckey_child"}, to="ckey_parent", + from_columns=["k1"], to_columns=["k1"], + )], + metrics=[OSIMetric(name="row_count", expression=_expr("COUNT(*)"))], + ) + result = _convert(shop_engine, doc) + models = {m.name: m for m in result.models} + assert "row_count" in {meas.name for meas in models["ckey_child"].measures} + assert "row_count" not in {meas.name for meas in models["ckey_parent"].measures} + + +def test_orphan_count_star_errors(shop_engine): + # Two datasets, no relationship -> no unique fact table -> COUNT(*) is an + # orphan (ambiguous grain) and is clean-failed, not guessed onto a model. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + OSIDataset(name="products", source="products", + fields=[OSIField(name="price", expression=_expr("price"))]), + ], + metrics=[OSIMetric(name="orphan_ct", expression=_expr("COUNT(*)"))], + ) + result = _convert(shop_engine, doc) + all_measures = {meas.name for m in result.models for meas in m.measures} + assert "orphan_ct" not in all_measures + assert _reported(result) + + +def test_sql_mode_source_is_live_introspected(shop_engine): + # A query source is introspected live (not heuristically typed): a numeric + # column comes back DOUBLE, a text column TEXT, from the actual query. + with shop_engine.connect() as conn: + conn.execute(sa.text( + "INSERT INTO orders (order_id, amount, status) VALUES (1, 9.5, 'paid')" + )) + conn.commit() + doc = _mini_doc( + datasets=[OSIDataset( + name="order_summary", + source="SELECT order_id, amount, status FROM orders", + fields=[ + OSIField(name="amount", expression=_expr("amount")), + OSIField(name="status", expression=_expr("status")), + ], + )] + ) + result = _convert(shop_engine, doc, target_dialect="sqlite") + m = {mm.name: mm for mm in result.models}["order_summary"] + assert m.sql == "SELECT order_id, amount, status FROM orders" + cols = {c.name: c for c in m.columns} + assert cols["amount"].type == DataType.DOUBLE + assert cols["status"].type == DataType.TEXT + + +# ─────────────────────── ai_context / meta on all kinds ───────────────────── + +def test_metric_ai_context_into_description_and_meta(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + meas = {m.name: m for m in orders.measures}["total_amount"] + assert "Sum of gross order value." in meas.description + assert "gmv" in meas.description + assert meas.meta["osi_ai_context"]["instructions"] == "Sum of gross order value." + + +def test_field_custom_extensions_into_meta(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + amount = {c.name: c for c in orders.columns}["amount"] + exts = amount.meta["osi_custom_extensions"] + assert exts[0]["vendor_name"] == "SNOWFLAKE" + assert exts[0]["data"] == '{"unit": "usd"}' + + +def test_join_meta_carries_ai_context(shop_engine): + orders = _by_name(_shop_result(shop_engine))["orders"] + cj = {j.target_model: j for j in orders.joins}["customers"] + assert cj.meta["osi_ai_context"]["instructions"] == "Each order has one customer." + + +# ─────────────────────── relationship / metric failures ───────────────────── + +def test_relationship_unknown_target_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))])], + relationships=[OSIRelationship( + name="dangling", **{"from": "orders"}, to="nonexistent", + from_columns=["customer_id"], to_columns=["customer_id"], + )], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert orders.joins == [] + assert _reported(result) + + +def test_metric_no_join_path_clean_fails(shop_engine): + # orders + products with NO relationship; a metric spanning both cannot be + # anchored anywhere -> clean-fail. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + OSIDataset(name="products", source="products", + fields=[OSIField(name="price", expression=_expr("price"))]), + ], + metrics=[OSIMetric( + name="cross", + expression=_expr("SUM(orders.amount) + SUM(products.price)"), + )], + ) + result = _convert(shop_engine, doc) + all_measures = {meas.name for m in result.models for meas in m.measures} + assert "cross" not in all_measures + assert _reported(result) + + +# ─────────────────────────── dialect selection ───────────────────────────── + +def test_dialect_fallback_among_sql_dialects(shop_engine): + # Requested ANSI_SQL absent; SNOWFLAKE (SQL-compatible) present -> use it. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="tot", expression=_expr("SUM(amount)", dialect="SNOWFLAKE"))], + ) + result = _convert(shop_engine, doc, dialect="ANSI_SQL") + orders = {m.name: m for m in result.models}["orders"] + assert {"tot"} <= {meas.name for meas in orders.measures} + + +def test_target_dialect_percentile_caveat(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric( + name="p90", + expression=_expr("PERCENTILE_CONT(0.9) WITHIN GROUP (ORDER BY amount)"), + )], + ) + result = _convert(shop_engine, doc, target_dialect="mysql") + orders = {m.name: m for m in result.models}["orders"] + # measure still created, but an info caveat is reported. + assert "p90" in {meas.name for meas in orders.measures} + assert _reported(result) diff --git a/tests/test_osi_expression.py b/tests/test_osi_expression.py new file mode 100644 index 00000000..4c20960a --- /dev/null +++ b/tests/test_osi_expression.py @@ -0,0 +1,238 @@ +"""OSI metric/field expression -> SLayer formula transform (slayer/osi/expression.py). + +``convert_expression`` walks a SQL aggregation expression with sqlglot and emits a +SLayer colon-syntax formula, materializing hidden derived Columns for non-bare +aggregate operands, and clean-failing anything inexpressible. + +Callbacks (so the transform is model-agnostic and unit-testable in isolation): +- ``owner_of(qualifier, column) -> model | None`` which dataset owns a column +- ``ref_of(model, column) -> anchor-relative dotted ref | None`` (None = unreachable) +""" + +import sqlglot + +from slayer.osi.expression import convert_expression + +# Single-model context: everything is owned by "orders"; refs are local (bare). +OWNER = lambda q, c: "orders" # noqa: E731 +REF = lambda m, c: c # noqa: E731 + + +def _run(expr: str, *, entity_name: str = "m", owner_of=OWNER, ref_of=REF, **kw): + return convert_expression( + expr, entity_name=entity_name, owner_of=owner_of, ref_of=ref_of, **kw + ) + + +def _norm(sql: str) -> str: + return sqlglot.parse_one(sql).sql() + + +# ─────────────────────────── simple aggregations ─────────────────────────── + +def test_sum_bare(): + r = _run("SUM(amount)") + assert r.ok and r.formula == "amount:sum" and r.materialized == [] + + +def test_count_star(): + assert _run("COUNT(*)").formula == "*:count" + + +def test_count_col(): + assert _run("COUNT(customer_id)").formula == "customer_id:count" + + +def test_count_distinct(): + assert _run("COUNT(DISTINCT customer_id)").formula == "customer_id:count_distinct" + + +def test_avg_min_max(): + assert _run("AVG(amount)").formula == "amount:avg" + assert _run("MIN(amount)").formula == "amount:min" + assert _run("MAX(amount)").formula == "amount:max" + + +# ─────────────────────── arithmetic + constants + scalar ──────────────────── + +def test_difference_of_aggs(): + assert _run("SUM(amount) - SUM(quantity)").formula == "amount:sum - quantity:sum" + + +def test_divide_by_constant(): + assert _run("SUM(amount) / 100").formula == "amount:sum / 100" + + +def test_ratio_is_plain_arithmetic(): + assert _run("(SUM(amount)) / (COUNT(*))").formula == "amount:sum / *:count" + + +def test_scalar_passthrough_nullif(): + r = _run("SUM(amount) / NULLIF(COUNT(*), 0)") + assert r.ok and r.formula == "amount:sum / nullif(*:count, 0)" + + +def test_constant_times_agg(): + assert _run("0.9 * SUM(amount)").formula == "0.9 * amount:sum" + + +# ───────────────────── derived-column materialization ─────────────────────── + +def test_materialize_arithmetic_operand(): + r = _run("SUM(quantity * amount)", entity_name="revenue_line") + assert r.ok + assert len(r.materialized) == 1 + mc = r.materialized[0] + assert mc.owning_model == "orders" + assert mc.name.startswith("_revenue_line") + assert _norm(mc.sql) == _norm("quantity * amount") + assert r.formula == f"{mc.name}:sum" + + +def test_materialize_scalar_operand(): + r = _run("SUM(COALESCE(amount, 0))", entity_name="safe_rev") + assert r.ok and len(r.materialized) == 1 + assert _norm(r.materialized[0].sql) == _norm("COALESCE(amount, 0)") + assert r.formula == f"{r.materialized[0].name}:sum" + + +def test_materialize_case_filtered_count(): + r = _run("COUNT(CASE WHEN status = 'paid' THEN 1 END)", entity_name="paid_ct") + assert r.ok and len(r.materialized) == 1 + assert "CASE" in r.materialized[0].sql.upper() + assert r.formula == f"{r.materialized[0].name}:count" + + +def test_materialize_dedups_identical_operand(): + r = _run("SUM(quantity * amount) - MIN(quantity * amount)", entity_name="x") + assert r.ok + # The identical operand is materialized once and reused. + assert len(r.materialized) == 1 + nm = r.materialized[0].name + assert r.formula == f"{nm}:sum - {nm}:min" + + +def test_cross_dataset_operand_clean_fails(): + # quantity belongs to orders, price to products -> operand spans datasets. + owner = lambda q, c: "orders" if c == "quantity" else "products" # noqa: E731 + r = _run("SUM(quantity * price)", owner_of=owner) + assert not r.ok and r.formula is None and r.reason + + +# ───────────────────────────── percentile ────────────────────────────────── + +def test_percentile_cont(): + assert _run("PERCENTILE_CONT(0.9) WITHIN GROUP (ORDER BY amount)").formula == ( + "amount:percentile(p=0.9)" + ) + + +def test_percentile_cont_half_is_median(): + assert _run("PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY amount)").formula == ( + "amount:median" + ) + + +def test_percentile_disc(): + assert _run("PERCENTILE_DISC(0.9) WITHIN GROUP (ORDER BY amount)").formula == ( + "amount:percentile(p=0.9)" + ) + + +def test_percentile_out_of_range_clean_fails(): + r = _run("PERCENTILE_CONT(1.5) WITHIN GROUP (ORDER BY amount)") + assert not r.ok and r.reason + + +def test_percentile_nonliteral_clean_fails(): + r = _run("PERCENTILE_CONT(amount) WITHIN GROUP (ORDER BY amount)") + assert not r.ok + + +def test_percentile_unsupported_dialect_warns_but_emits(): + r = _run( + "PERCENTILE_CONT(0.9) WITHIN GROUP (ORDER BY amount)", + percentile_unsupported=True, + ) + assert r.ok and r.formula == "amount:percentile(p=0.9)" + assert r.warnings + + +# ───────────────────────────── clean-fails ───────────────────────────────── + +def test_top_level_case_clean_fails(): + r = _run("CASE WHEN status = 'x' THEN 1 ELSE 0 END") + assert not r.ok and r.formula is None and r.reason + + +def test_bare_unaggregated_column_clean_fails(): + r = _run("amount") + assert not r.ok + + +def test_window_function_clean_fails(): + r = _run("SUM(amount) OVER (PARTITION BY customer_id)") + assert not r.ok + + +def test_nested_aggregate_clean_fails(): + r = _run("SUM(SUM(amount))") + assert not r.ok + + +def test_string_literal_clean_fails(): + r = _run("'hello'") + assert not r.ok + + +def test_non_passthrough_function_clean_fails(): + r = _run("WEIRDFUNC(amount)") + assert not r.ok + + +# ───────────────────────── cross-model refs ──────────────────────────────── + +def test_qualified_column_emits_dotted_ref(): + owner = lambda q, c: "customers" # noqa: E731 + ref = lambda m, c: f"customers.{c}" # noqa: E731 + r = _run("COUNT(DISTINCT customers.customer_id)", owner_of=owner, ref_of=ref) + assert r.ok and r.formula == "customers.customer_id:count_distinct" + + +def test_unreachable_ref_clean_fails(): + owner = lambda q, c: "regions" # noqa: E731 + ref = lambda m, c: None # noqa: E731 (no join path from anchor) + r = _run("SUM(regions.population)", owner_of=owner, ref_of=ref) + assert not r.ok and r.reason + + +def test_multihop_ref_backed_by_real_join_graph(): + # Prove the expression layer builds the SAME dotted path the converter would, + # driven by JoinGraph.shortest_path (orders -> customers -> regions). + from slayer.core.enums import DataType + from slayer.core.models import Column, ModelJoin, SlayerModel + from slayer.engine.join_graph import JoinGraph + + models = [ + SlayerModel(name="orders", sql_table="orders", data_source="d", + columns=[Column(name="customer_id", type=DataType.INT)], + joins=[ModelJoin(target_model="customers", + join_pairs=[["customer_id", "customer_id"]])]), + SlayerModel(name="customers", sql_table="customers", data_source="d", + columns=[Column(name="region_id", type=DataType.INT)], + joins=[ModelJoin(target_model="regions", + join_pairs=[["region_id", "region_id"]])]), + SlayerModel(name="regions", sql_table="regions", data_source="d", + columns=[Column(name="population", type=DataType.INT)]), + ] + graph = JoinGraph.build_from_models(models) + anchor = "orders" + + def ref_of(model: str, column: str): + path = graph.shortest_path(anchor, model) + if path is None: + return None + return ".".join([*path, column]) + + r = _run("SUM(regions.population)", owner_of=lambda q, c: "regions", ref_of=ref_of) + assert r.ok and r.formula == "customers.regions.population:sum" diff --git a/tests/test_osi_modeljoin_fields.py b/tests/test_osi_modeljoin_fields.py new file mode 100644 index 00000000..26763ec7 --- /dev/null +++ b/tests/test_osi_modeljoin_fields.py @@ -0,0 +1,91 @@ +"""ModelJoin gains optional ``description`` + ``meta`` (DEV-1643). + +These carry OSI relationship ai_context. The fields are purely additive/optional +(no SlayerModel version bump), so old v7 join data validates unchanged, and both +storage backends persist the new fields. +""" + +import tempfile + + +from slayer.core.enums import DataType, JoinType +from slayer.core.models import Column, ModelJoin, SlayerModel +from slayer.storage.sqlite_storage import SQLiteStorage +from slayer.storage.yaml_storage import YAMLStorage + + +def _model_with_join() -> SlayerModel: + return SlayerModel( + name="orders", + sql_table="orders", + data_source="testds", + columns=[Column(name="customer_id", type=DataType.INT)], + joins=[ + ModelJoin( + target_model="customers", + join_pairs=[["customer_id", "customer_id"]], + join_type=JoinType.LEFT, + description="Each order has one customer.", + meta={"osi_ai_context": {"instructions": "Each order has one customer."}}, + ) + ], + ) + + +def test_modeljoin_has_optional_description_and_meta_defaulting_none() -> None: + j = ModelJoin(target_model="c", join_pairs=[["a", "b"]]) + assert j.description is None + assert j.meta is None + + +def test_modeljoin_accepts_description_and_meta() -> None: + j = ModelJoin( + target_model="c", + join_pairs=[["a", "b"]], + description="desc", + meta={"k": "v"}, + ) + assert j.description == "desc" + assert j.meta == {"k": "v"} + + +def test_old_v7_join_without_new_fields_validates() -> None: + # Data written before the fields existed omits them entirely. + j = ModelJoin.model_validate({"target_model": "c", "join_pairs": [["a", "b"]]}) + assert j.description is None and j.meta is None + # And a whole v7 model whose join lacks the fields still loads. + m = SlayerModel.model_validate( + { + "version": 7, + "name": "orders", + "sql_table": "orders", + "data_source": "testds", + "columns": [{"name": "customer_id", "type": "INT"}], + "joins": [{"target_model": "customers", "join_pairs": [["customer_id", "customer_id"]]}], + } + ) + assert m.joins[0].description is None and m.joins[0].meta is None + + +async def test_yaml_roundtrip_preserves_join_fields() -> None: + with tempfile.TemporaryDirectory() as tmp: + storage = YAMLStorage(base_dir=tmp) + await storage.save_model(_model_with_join()) + loaded = await storage.get_model("orders", data_source="testds") + assert loaded is not None + assert loaded.joins[0].description == "Each order has one customer." + assert loaded.joins[0].meta == { + "osi_ai_context": {"instructions": "Each order has one customer."} + } + + +async def test_sqlite_roundtrip_preserves_join_fields() -> None: + with tempfile.TemporaryDirectory() as tmp: + storage = SQLiteStorage(db_path=f"{tmp}/s.db") + await storage.save_model(_model_with_join()) + loaded = await storage.get_model("orders", data_source="testds") + assert loaded is not None + assert loaded.joins[0].description == "Each order has one customer." + assert loaded.joins[0].meta["osi_ai_context"]["instructions"] == ( + "Each order has one customer." + ) diff --git a/tests/test_osi_parser.py b/tests/test_osi_parser.py new file mode 100644 index 00000000..d7b0a9de --- /dev/null +++ b/tests/test_osi_parser.py @@ -0,0 +1,121 @@ +"""Tests for the OSI config parser (slayer/osi/parser.py). + +Covers: YAML + JSON, single file + directory, the ``from`` alias, version +acceptance (all four known versions) + unknown-version warning, malformed-file +skip, and ai_context in both string and object form. +""" + +import logging +import shutil +from pathlib import Path + +import pytest + +from slayer.osi.models import OSIAIContextObject, OSIDocument +from slayer.osi.parser import KNOWN_OSI_VERSIONS, parse_osi_path + +FIXTURES = Path(__file__).parent / "fixtures" / "osi" + + +def test_parse_yaml_file_structure() -> None: + docs = parse_osi_path(FIXTURES / "shop.yaml") + assert len(docs) == 1 + doc = docs[0] + assert isinstance(doc, OSIDocument) + assert doc.version == "0.2.0.dev0" + assert len(doc.semantic_model) == 1 + sm = doc.semantic_model[0] + assert sm.name == "shop" + assert {d.name for d in sm.datasets} == {"orders", "customers", "products", "regions"} + orders = next(d for d in sm.datasets if d.name == "orders") + assert orders.source == "orders" + assert orders.primary_key == ["order_id"] + assert {f.name for f in orders.fields} == { + "order_id", "customer_id", "product_id", "amount", + "quantity", "ordered_at", "status", + } + ordered_at = next(f for f in orders.fields if f.name == "ordered_at") + assert ordered_at.dimension is not None and ordered_at.dimension.is_time is True + amount = next(f for f in orders.fields if f.name == "amount") + assert amount.expression.dialects[0].dialect.value == "ANSI_SQL" + assert amount.expression.dialects[0].expression == "amount" + + +def test_relationship_from_alias() -> None: + doc = parse_osi_path(FIXTURES / "shop.yaml")[0] + sm = doc.semantic_model[0] + rels = {r.name: r for r in sm.relationships} + r = rels["orders_to_customers"] + # `from` is a reserved word; it must be exposed as `from_dataset`. + assert r.from_dataset == "orders" + assert r.to == "customers" + assert r.from_columns == ["customer_id"] + assert r.to_columns == ["customer_id"] + + +def test_metrics_parsed() -> None: + doc = parse_osi_path(FIXTURES / "shop.yaml")[0] + metrics = {m.name: m for m in doc.semantic_model[0].metrics} + assert set(metrics) == { + "total_amount", "order_count", "aov", + "revenue_line", "cust_reach", "rev_plus_pop", "bridge_metric", + } + assert metrics["total_amount"].expression.dialects[0].expression == "SUM(amount)" + + +def test_yaml_and_json_parse_identically() -> None: + y = parse_osi_path(FIXTURES / "shop.yaml")[0] + j = parse_osi_path(FIXTURES / "shop.json")[0] + assert y.model_dump() == j.model_dump() + + +def test_parse_directory_collects_and_skips_malformed(tmp_path: Path) -> None: + shutil.copy(FIXTURES / "shop.yaml", tmp_path / "shop.yaml") + shutil.copy(FIXTURES / "malformed.yaml", tmp_path / "malformed.yaml") + docs = parse_osi_path(tmp_path) + # malformed.yaml is skipped; only the valid doc is returned. + assert len(docs) == 1 + assert docs[0].semantic_model[0].name == "shop" + + +def test_known_versions_constant() -> None: + assert KNOWN_OSI_VERSIONS == frozenset({"1.0", "0.1.0", "0.1.1", "0.2.0.dev0"}) + + +@pytest.mark.parametrize("version", sorted(KNOWN_OSI_VERSIONS)) +def test_known_version_no_warning(tmp_path: Path, caplog: pytest.LogCaptureFixture, version: str) -> None: + src = (FIXTURES / "shop.yaml").read_text().replace( + 'version: "0.2.0.dev0"', f'version: "{version}"' + ) + f = tmp_path / "v.yaml" + f.write_text(src) + with caplog.at_level(logging.WARNING): + docs = parse_osi_path(f) + assert len(docs) == 1 and docs[0].version == version + assert not any("version" in r.message.lower() for r in caplog.records) + + +def test_unknown_version_warns_but_parses(caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + docs = parse_osi_path(FIXTURES / "unknown_version.yaml") + assert len(docs) == 1 and docs[0].version == "9.9.9" + assert any("version" in r.message.lower() for r in caplog.records) + + +def test_malformed_file_warns_and_returns_empty(caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + docs = parse_osi_path(FIXTURES / "malformed.yaml") + assert docs == [] + assert caplog.records # a warning was emitted + + +def test_ai_context_string_and_object_forms() -> None: + # Object form on shop.yaml. + shop = parse_osi_path(FIXTURES / "shop.yaml")[0].semantic_model[0] + assert isinstance(shop.ai_context, OSIAIContextObject) + assert shop.ai_context.instructions == "Use for order, customer, and product analytics." + assert shop.ai_context.synonyms == ["store", "retail"] + # String form. + strdoc = parse_osi_path(FIXTURES / "aicontext_string.yaml")[0].semantic_model[0] + assert strdoc.ai_context == "plain string context" + assert strdoc.datasets[0].ai_context == "dataset string ctx" diff --git a/tests/test_osi_source.py b/tests/test_osi_source.py new file mode 100644 index 00000000..ba1abb56 --- /dev/null +++ b/tests/test_osi_source.py @@ -0,0 +1,57 @@ +"""OSI Dataset.source parsing (slayer/osi/source.py). + +`source` is a dotted physical identifier (`[catalog.]db.schema.table`) or a raw +query. The parser splits it (table=last, schema=second-last, database=the rest) +and detects query sources. The database is dropped for now via a stubbed +`resolve_datasource` hook. +""" + + +from slayer.osi.source import ParsedSource, parse_source, resolve_datasource + + +def test_bare_table(): + p = parse_source("orders") + assert isinstance(p, ParsedSource) + assert p.is_query is False + assert p.table == "orders" and p.schema_name is None and p.database is None + + +def test_schema_table(): + p = parse_source("public.orders") + assert p.table == "orders" and p.schema_name == "public" and p.database is None + + +def test_db_schema_table(): + p = parse_source("shopdb.public.orders") + assert p.table == "orders" and p.schema_name == "public" and p.database == "shopdb" + + +def test_four_part_catalog_db_schema_table(): + p = parse_source("cat.shopdb.public.orders") + assert p.table == "orders" and p.schema_name == "public" + # everything before schema is the database part. + assert p.database == "cat.shopdb" + + +def test_quoted_identifiers_unwrapped(): + p = parse_source('"My Schema"."My Table"') + assert p.table == "My Table" and p.schema_name == "My Schema" + + +def test_query_source_detected(): + p = parse_source("SELECT id, amount FROM orders WHERE amount > 0") + assert p.is_query is True + assert p.query == "SELECT id, amount FROM orders WHERE amount > 0" + assert p.table is None + + +def test_parenthesized_query_source_detected(): + p = parse_source("(SELECT 1 AS x)") + assert p.is_query is True + + +def test_resolve_datasource_drops_database_for_now(): + # Stubbed hook: every OSI database maps to the default datasource. + assert resolve_datasource("shopdb", "my_slayer_ds") == "my_slayer_ds" + assert resolve_datasource(None, "my_slayer_ds") == "my_slayer_ds" diff --git a/zensical.toml b/zensical.toml index 0e227bac..bf1e750e 100644 --- a/zensical.toml +++ b/zensical.toml @@ -51,6 +51,9 @@ nav = [ { "SLayer vs dbt" = "dbt/slayer_vs_dbt.md" }, { "Importing dbt definitions" = "dbt/dbt_import.md" }, ]}, + { "OSI" = [ + { "Importing OSI configs" = "osi/osi_import.md" }, + ]}, { "Tutorials" = [ { "Make it dynamic" = "examples/01_dynamic/dynamic.md" }, { "SQL vs DSL" = [ From b29cec1f3bac01dd78ccf2421914b7abe0af4b06 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 13:23:16 +0200 Subject: [PATCH 02/25] DEV-1643: address review feedback (Codex + SonarCloud) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- slayer/osi/converter.py | 114 +++++++++++++++++++---------------- slayer/osi/expression.py | 52 +++++++++------- slayer/osi/parser.py | 4 +- tests/test_cli_import_osi.py | 3 +- tests/test_osi_converter.py | 14 +++++ 5 files changed, 111 insertions(+), 76 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index c4b0c0a4..15eb12fe 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -12,7 +12,6 @@ from __future__ import annotations import logging -import re from typing import Any, Optional import sqlalchemy as sa @@ -22,6 +21,7 @@ from slayer.core.enums import DataType from slayer.core.formula import parse_formula from slayer.core.models import Column, ModelJoin, ModelMeasure, SlayerModel +from slayer.core.refs import IDENTIFIER_RE as _IDENTIFIER_RE from slayer.engine.ingestion import introspect_table_to_model from slayer.engine.join_graph import JoinGraph, min_hops_root from slayer.ingest_report import ConversionResult, ConversionWarning @@ -43,7 +43,6 @@ logger = logging.getLogger(__name__) -_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") # Dialects lacking a GROUP BY percentile/median aggregate (mirrors the dbt # converter's caveat set). _NO_PERCENTILE_DIALECTS = frozenset({"mysql", "tsql", "mssql", "sqlserver"}) @@ -142,30 +141,19 @@ def _unconv(self, message: str, *, metric_name: str, category: str = "metric", def convert(self) -> ConversionResult: inspector = sa.inspect(self.sa_engine) - semantic_models = [sm for doc in self.documents for sm in doc.semantic_model] self._check_duplicate_dataset_names(semantic_models) - # dataset name -> its semantic model (for relationships/metrics scope) - sm_of_dataset: dict[str, OSISemanticModel] = {} - for sm in semantic_models: - for ds in sm.datasets: - if _legal_model_name(ds.name): - sm_of_dataset[ds.name] = sm - for sm in semantic_models: for ds in sm.datasets: self._build_model(ds, sm, inspector) - for sm in semantic_models: for rel in sm.relationships or []: self._build_join(rel) graph = JoinGraph.build_from_models(list(self._models.values())) for sm in semantic_models: - sm_model_names = [d.name for d in sm.datasets if d.name in self._models] - for metric in sm.metrics or []: - self._build_measure(metric, sm_model_names, graph) + self._build_measures_for(sm, graph) return ConversionResult( models=list(self._models.values()), @@ -173,6 +161,11 @@ def convert(self) -> ConversionResult: warnings=self._warnings, ) + def _build_measures_for(self, sm: OSISemanticModel, graph: JoinGraph) -> None: + sm_model_names = [d.name for d in sm.datasets if d.name in self._models] + for metric in sm.metrics or []: + self._build_measure(metric, sm_model_names, graph) + def _check_duplicate_dataset_names(self, sms: list[OSISemanticModel]) -> None: seen: set[str] = set() dupes: set[str] = set() @@ -243,43 +236,9 @@ def _overlay_fields(self, model: SlayerModel, ds: OSIDataset) -> None: first_time_dim: str | None = None for field in ds.fields or []: - if not _legal_column_name(field.name): - self._warn( - f"Field name {field.name!r} on dataset {ds.name!r} contains " - f"'.'/':'; skipping the field.", - model_name=ds.name, category="illegal_name", - ) - continue - - sql = self._resolve_expression(field.expression) - if sql is None: - self._warn( - f"Field {field.name!r} on {ds.name!r} has no SQL-dialect " - f"expression; skipping.", - model_name=ds.name, category="dialect", - ) - continue - - is_time = bool(field.dimension and field.dimension.is_time) - col = self._resolve_field_column(field, sql, by_name, introspected, ds) - if col is None: - continue - - col.label = field.label or col.label - col.description = _render_description(field.description, field.ai_context) \ - or col.description - meta = _build_meta(field.ai_context, field.custom_extensions) - if meta: - col.meta = {**(col.meta or {}), **meta} - if is_time: - if col.type not in (DataType.DATE, DataType.TIMESTAMP): - col.type = DataType.TIMESTAMP - if first_time_dim is None: - first_time_dim = col.name - - if col.name not in by_name: - model.columns.append(col) - by_name[col.name] = col + time_col = self._overlay_one_field(field, model, by_name, introspected, ds) + if time_col and first_time_dim is None: + first_time_dim = time_col # OSI primary_key is authoritative. for pk in ds.primary_key or []: @@ -289,6 +248,48 @@ def _overlay_fields(self, model: SlayerModel, ds: OSIDataset) -> None: if first_time_dim and not model.default_time_dimension: model.default_time_dimension = first_time_dim + def _overlay_one_field(self, field: OSIField, model: SlayerModel, + by_name: dict[str, Column], introspected: set[str], + ds: OSIDataset) -> str | None: + """Overlay one OSI field onto the model. Returns the column name if the + field is a time dimension, else None (clean-fails are reported).""" + if not _legal_column_name(field.name): + self._warn( + f"Field name {field.name!r} on dataset {ds.name!r} contains " + f"'.'/':'; skipping the field.", + model_name=ds.name, category="illegal_name", + ) + return None + + sql = self._resolve_expression(field.expression) + if sql is None: + self._warn( + f"Field {field.name!r} on {ds.name!r} has no SQL-dialect " + f"expression; skipping.", + model_name=ds.name, category="dialect", + ) + return None + + col = self._resolve_field_column(field, sql, by_name, introspected, ds) + if col is None: + return None + + col.label = field.label or col.label + col.description = _render_description(field.description, field.ai_context) \ + or col.description + meta = _build_meta(field.ai_context, field.custom_extensions) + if meta: + col.meta = {**(col.meta or {}), **meta} + + is_time = bool(field.dimension and field.dimension.is_time) + if is_time and col.type not in (DataType.DATE, DataType.TIMESTAMP): + col.type = DataType.TIMESTAMP + + if col.name not in by_name: + model.columns.append(col) + by_name[col.name] = col + return col.name if is_time else None + def _resolve_field_column(self, field: OSIField, sql: str, by_name: dict[str, Column], introspected: set[str], ds: OSIDataset) -> Column | None: @@ -481,11 +482,18 @@ def _fact_root(self, sm_model_names: list[str]) -> str | None: return non_targets[0] if len(non_targets) == 1 else None def _make_owner_of(self, sm_model_names: list[str]): + def has_column(model_name: str, column: str) -> bool: + model = self._models.get(model_name) + return model is not None and any(c.name == column for c in model.columns) + def owner_of(qualifier: Optional[str], column: str) -> Optional[str]: if qualifier is not None: - return qualifier if qualifier in self._models else None + # Verify the column actually exists on the qualified model — + # otherwise a metric like SUM(orders.no_such_col) would import + # as a measure that fails at query time. + return qualifier if has_column(qualifier, column) else None for name in sm_model_names: - if any(c.name == column for c in self._models[name].columns): + if has_column(name, column): return name return None return owner_of diff --git a/slayer/osi/expression.py b/slayer/osi/expression.py index 87a96135..6b4f4d49 100644 --- a/slayer/osi/expression.py +++ b/slayer/osi/expression.py @@ -20,6 +20,7 @@ from __future__ import annotations +import math import re from typing import Callable, Optional @@ -170,7 +171,7 @@ def _percentile_ref(self, node: exp.WithinGroup) -> str: "percentile/median has no GROUP BY aggregate on the target dialect; " "the measure imports but fails at query time there." ) - if isinstance(inner, exp.PercentileCont) and p_val == 0.5: + if isinstance(inner, exp.PercentileCont) and math.isclose(p_val, 0.5): return f"{ref}:median" return f"{ref}:percentile(p={p_node.name})" @@ -182,20 +183,24 @@ def _is_sentinel(node: exp.Expression) -> bool: def _validate_residual(self, root: exp.Expression) -> None: for node in root.walk(): - if isinstance(node, (exp.AggFunc, exp.Window, exp.WithinGroup)): - raise _Unconvertible("window/aggregate function not expressible") - if isinstance(node, exp.Case): - raise _Unconvertible("CASE outside an aggregate is not expressible") - if isinstance(node, exp.Column) and not self._is_sentinel(node): - raise _Unconvertible( - f"bare column {node.name!r} must appear inside an aggregation" - ) - if isinstance(node, exp.Literal) and node.is_string: - raise _Unconvertible("string literal is not expressible in a measure") - if isinstance(node, exp.Func) and not isinstance(node, exp.Count): - name = node.sql_name().lower() - if name not in SCALAR_PASSTHROUGH: - raise _Unconvertible(f"function {node.sql_name()!r} is not allowed") + reason = self._residual_violation(node) + if reason: + raise _Unconvertible(reason) + + def _residual_violation(self, node: exp.Expression) -> str | None: + """Return the clean-fail reason for a single residual node, or None.""" + if isinstance(node, (exp.AggFunc, exp.Window, exp.WithinGroup)): + return "window/aggregate function not expressible" + if isinstance(node, exp.Case): + return "CASE outside an aggregate is not expressible" + if isinstance(node, exp.Column) and not self._is_sentinel(node): + return f"bare column {node.name!r} must appear inside an aggregation" + if isinstance(node, exp.Literal) and node.is_string: + return "string literal is not expressible in a measure" + if isinstance(node, exp.Func) and not isinstance(node, exp.Count): + if node.sql_name().lower() not in SCALAR_PASSTHROUGH: + return f"function {node.sql_name()!r} is not allowed" + return None # ---- top-level ---- @@ -258,12 +263,17 @@ def _find_outermost_aggregates(tree: exp.Expression) -> list[exp.Expression]: def _strip_redundant_parens(root: exp.Expression) -> exp.Expression: """Drop parentheses that wrap a single atom (a sentinel ref or literal), so ``(SUM(a)) / (COUNT(*))`` renders as ``a:sum / *:count``.""" - for paren in list(root.find_all(exp.Paren)): - if isinstance(paren.this, (exp.Column, exp.Literal)): - if paren is root: - root = paren.this - else: - paren.replace(paren.this) + # Materialize the matches first (a list comprehension, not list(gen)) — + # the tree is mutated in place below, so we can't iterate it lazily. + atom_parens = [ + p for p in root.find_all(exp.Paren) + if isinstance(p.this, (exp.Column, exp.Literal)) + ] + for paren in atom_parens: + if paren is root: + root = paren.this + else: + paren.replace(paren.this) return root @staticmethod diff --git a/slayer/osi/parser.py b/slayer/osi/parser.py index 0fcdf542..f2a05661 100644 --- a/slayer/osi/parser.py +++ b/slayer/osi/parser.py @@ -77,7 +77,9 @@ def parse_osi_file(path: Path) -> OSIDocument | None: def parse_osi_path(path: str | Path) -> list[OSIDocument]: """Parse an OSI file or directory into a list of ``OSIDocument`` objects.""" - root = Path(path) + # Canonicalize the caller-supplied path before touching the filesystem so + # every downstream read works off a resolved, symlink-free base. + root = Path(path).resolve() if not root.exists(): raise FileNotFoundError(f"OSI path does not exist: {root}") diff --git a/tests/test_cli_import_osi.py b/tests/test_cli_import_osi.py index 99526410..9e0884a7 100644 --- a/tests/test_cli_import_osi.py +++ b/tests/test_cli_import_osi.py @@ -78,5 +78,6 @@ def test_import_osi_saves_models_and_prints_report(shop_setup, capsys) -> None: def test_import_osi_missing_datasource_exits(tmp_path: Path) -> None: store = tmp_path / "empty_store" YAMLStorage(base_dir=str(store)) # no datasource registered + args = _args(store, FIXTURES / "shop.yaml") with pytest.raises(SystemExit): - _run_import_osi(_args(store, FIXTURES / "shop.yaml")) + _run_import_osi(args) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 77ff5000..ed4935a6 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -468,6 +468,20 @@ def test_relationship_unknown_target_clean_fails(shop_engine): assert _reported(result) +def test_qualified_metric_ref_to_nonexistent_column_clean_fails(shop_engine): + # A qualified ref whose column does not exist on the qualified model must + # clean-fail, not import a measure that fails at query time. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="bad", expression=_expr("SUM(orders.no_such_col)"))], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "bad" not in {meas.name for meas in orders.measures} + assert _reported(result) + + def test_metric_no_join_path_clean_fails(shop_engine): # orders + products with NO relationship; a metric spanning both cannot be # anchored anywhere -> clean-fail. From 76a5356ebedb29d8832c1a3576e66f48186e188e Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 14:13:35 +0200 Subject: [PATCH 03/25] DEV-1643: address second-round Codex findings (robustness) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- slayer/osi/converter.py | 50 +++++++++++++++++++++- slayer/osi/expression.py | 27 +++++++++--- tests/test_osi_converter.py | 83 +++++++++++++++++++++++++++++++++++++ 3 files changed, 154 insertions(+), 6 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 15eb12fe..d937be28 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -307,8 +307,14 @@ def _resolve_field_column(self, field: OSIField, sql: str, model_name=ds.name, category="missing_column", ) return None - # derived expression + # derived expression. When it redefines an existing (introspected) + # column, overlay the expression onto that column so it isn't silently + # dropped by the append-only path; otherwise create a new derived column. is_time = bool(field.dimension and field.dimension.is_time) + existing = by_name.get(field.name) + if existing is not None: + existing.sql = sql + return existing return Column( name=field.name, sql=sql, type=DataType.TIMESTAMP if is_time else DataType.DOUBLE, @@ -358,6 +364,15 @@ def _build_join(self, rel: OSIRelationship) -> None: ) return + missing = self._missing_join_columns(rel) + if missing: + self._warn( + f"Relationship {rel.name!r} references join columns not present " + f"on their models: {missing}; skipping.", + model_name=src, category="relationship", + ) + return + pairs = [[f, t] for f, t in zip(rel.from_columns, rel.to_columns)] self._models[src].joins.append(ModelJoin( target_model=rel.to, @@ -366,6 +381,19 @@ def _build_join(self, rel: OSIRelationship) -> None: meta=_build_meta(rel.ai_context, rel.custom_extensions), )) + def _model_has_column(self, model_name: str, column: str) -> bool: + model = self._models.get(model_name) + return model is not None and any(c.name == column for c in model.columns) + + def _missing_join_columns(self, rel: OSIRelationship) -> list[str]: + """Qualified names of relationship join columns absent from their + model, so a typo clean-fails instead of emitting a broken join.""" + missing = [f"{rel.from_dataset}.{c}" for c in rel.from_columns + if not self._model_has_column(rel.from_dataset, c)] + missing += [f"{rel.to}.{c}" for c in rel.to_columns + if not self._model_has_column(rel.to, c)] + return missing + # ---- metrics -> measures ---- def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], @@ -390,6 +418,25 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], if anchor is None: return # already reported + # Enforce SLayer's namespace invariants before the post-construction + # append (which bypasses SlayerModel's validators): a measure name must + # be unique and must not collide with a column on the same model. + anchor_model = self._models[anchor] + if any(m.name == metric.name for m in anchor_model.measures): + self._unconv( + f"Metric {metric.name!r} duplicates an existing measure on " + f"model {anchor!r}.", + metric_name=metric.name, category="duplicate_measure", + ) + return + if self._model_has_column(anchor, metric.name): + self._unconv( + f"Metric {metric.name!r} collides with a column name on model " + f"{anchor!r}.", + metric_name=metric.name, category="name_collision", + ) + return + ref_of = self._make_ref_of(graph, anchor) percentile_unsupported = ( self.target_dialect is not None @@ -398,6 +445,7 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], result = convert_expression( expr, entity_name=metric.name, owner_of=owner_of, ref_of=ref_of, percentile_unsupported=percentile_unsupported, + name_taken=self._model_has_column, ) if not result.ok: self._unconv( diff --git a/slayer/osi/expression.py b/slayer/osi/expression.py index 6b4f4d49..4f7e67db 100644 --- a/slayer/osi/expression.py +++ b/slayer/osi/expression.py @@ -38,6 +38,7 @@ OwnerOf = Callable[[Optional[str], str], Optional[str]] RefOf = Callable[[str, str], Optional[str]] +NameTaken = Callable[[str, str], bool] # (owning_model, name) -> already exists? class MaterializedColumn(BaseModel): @@ -64,11 +65,12 @@ class _Unconvertible(Exception): class _Converter: def __init__(self, entity_name: str, owner_of: OwnerOf, ref_of: RefOf, - percentile_unsupported: bool) -> None: + percentile_unsupported: bool, name_taken: NameTaken) -> None: self.entity_name = entity_name self.owner_of = owner_of self.ref_of = ref_of self.percentile_unsupported = percentile_unsupported + self.name_taken = name_taken self.materialized: list[MaterializedColumn] = [] self.warnings: list[str] = [] self._dedup: dict[tuple[str, str], str] = {} @@ -104,8 +106,7 @@ def _materialize(self, operand: exp.Expression) -> str: key = (owner, operand_sql) name = self._dedup.get(key) if name is None: - name = f"_{self.entity_name}_{self._counter}" - self._counter += 1 + name = self._fresh_column_name(owner) self._dedup[key] = name self.materialized.append( MaterializedColumn(owning_model=owner, name=name, sql=operand_sql) @@ -115,6 +116,16 @@ def _materialize(self, operand: exp.Expression) -> str: raise _Unconvertible("materialized operand is not reachable from the anchor") return ref + def _fresh_column_name(self, owner: str) -> str: + """A hidden-column name that collides with no existing column on + ``owner`` (the formula references this name verbatim, so a collision + would silently aggregate the wrong column).""" + while True: + name = f"_{self.entity_name}_{self._counter}" + self._counter += 1 + if not self.name_taken(owner, name): + return name + def _operand_ref(self, operand: exp.Expression) -> str: if isinstance(operand, exp.Column): return self._column_ref(operand.table or None, operand.name) @@ -295,9 +306,15 @@ def convert_expression( owner_of: OwnerOf, ref_of: RefOf, percentile_unsupported: bool = False, + name_taken: NameTaken = lambda model, name: False, ) -> ExprResult: - """Convert an OSI SQL aggregation expression into a SLayer formula.""" + """Convert an OSI SQL aggregation expression into a SLayer formula. + + ``name_taken(owning_model, name)`` lets the caller reserve hidden + derived-column names against existing columns so a materialized operand + never collides with a real column. + """ return _Converter( entity_name=entity_name, owner_of=owner_of, ref_of=ref_of, - percentile_unsupported=percentile_unsupported, + percentile_unsupported=percentile_unsupported, name_taken=name_taken, ).convert(expr) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index ed4935a6..d4e85650 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -468,6 +468,89 @@ def test_relationship_unknown_target_clean_fails(shop_engine): assert _reported(result) +def test_derived_field_shadowing_physical_column_overlays(shop_engine): + # A derived OSI field whose name matches a physical column must overlay its + # expression onto that column, not be silently dropped. + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="status", expression=_expr("LOWER(status)"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + status_cols = [c for c in orders.columns if c.name == "status"] + assert len(status_cols) == 1 + assert status_cols[0].sql == "LOWER(status)" + + +def test_materialized_name_avoids_existing_column(shop_engine): + # A materialized operand name must not collide with an existing column, + # else the metric would aggregate the wrong column. + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[ + OSIField(name="amount", expression=_expr("amount")), + OSIField(name="quantity", expression=_expr("quantity")), + OSIField(name="_rev_0", expression=_expr("amount")), # occupies name + ], + )], + metrics=[OSIMetric(name="rev", expression=_expr("SUM(quantity * amount)"))], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + derived = {meas.name: meas for meas in orders.measures}["rev"].formula.split(":")[0] + assert derived != "_rev_0" + assert any(c.name == derived and c.hidden for c in orders.columns) + + +def test_duplicate_metric_name_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[ + OSIMetric(name="tot", expression=_expr("SUM(amount)")), + OSIMetric(name="tot", expression=_expr("MAX(amount)")), + ], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert len([m for m in orders.measures if m.name == "tot"]) == 1 + assert _reported(result) + + +def test_metric_named_as_column_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="amount", expression=_expr("SUM(amount)"))], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "amount" not in {m.name for m in orders.measures} + assert _reported(result) + + +def test_relationship_nonexistent_join_column_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))]), + ], + relationships=[OSIRelationship( + name="bad", **{"from": "orders"}, to="customers", + from_columns=["no_such_fk"], to_columns=["customer_id"], + )], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert orders.joins == [] + assert _reported(result) + + def test_qualified_metric_ref_to_nonexistent_column_clean_fails(shop_engine): # A qualified ref whose column does not exist on the qualified model must # clean-fail, not import a measure that fails at query time. From 729e4235a29d74497f1644811e27e6b99e271c6e Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 14:23:34 +0200 Subject: [PATCH 04/25] DEV-1643: address third-round Codex findings (completeness) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- slayer/osi/converter.py | 65 ++++++++++++++++++++++++----------- tests/test_osi_converter.py | 68 +++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 19 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index d937be28..926312f8 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -285,9 +285,16 @@ def _overlay_one_field(self, field: OSIField, model: SlayerModel, if is_time and col.type not in (DataType.DATE, DataType.TIMESTAMP): col.type = DataType.TIMESTAMP - if col.name not in by_name: + # Bare overlays return the existing column object (no-op here); an + # aliased/derived field that shadows an existing column REPLACES it so + # its expression isn't silently dropped by an append-only path. + existing = by_name.get(col.name) + if existing is None: model.columns.append(col) by_name[col.name] = col + elif existing is not col: + model.columns[model.columns.index(existing)] = col + by_name[col.name] = col return col.name if is_time else None def _resolve_field_column(self, field: OSIField, sql: str, @@ -307,14 +314,9 @@ def _resolve_field_column(self, field: OSIField, sql: str, model_name=ds.name, category="missing_column", ) return None - # derived expression. When it redefines an existing (introspected) - # column, overlay the expression onto that column so it isn't silently - # dropped by the append-only path; otherwise create a new derived column. + # derived expression (collision with an existing column is handled by + # the replace-or-append logic in _overlay_one_field). is_time = bool(field.dimension and field.dimension.is_time) - existing = by_name.get(field.name) - if existing is not None: - existing.sql = sql - return existing return Column( name=field.name, sql=sql, type=DataType.TIMESTAMP if is_time else DataType.DOUBLE, @@ -385,6 +387,16 @@ def _model_has_column(self, model_name: str, column: str) -> bool: model = self._models.get(model_name) return model is not None and any(c.name == column for c in model.columns) + def _model_has_name(self, model_name: str, name: str) -> bool: + """Whether ``name`` is taken by a column OR measure on the model — + SLayer requires the two to share one namespace, so a materialized + hidden-column name must avoid both.""" + model = self._models.get(model_name) + if model is None: + return False + return (any(c.name == name for c in model.columns) + or any(m.name == name for m in model.measures)) + def _missing_join_columns(self, rel: OSIRelationship) -> list[str]: """Qualified names of relationship join columns absent from their model, so a typo clean-fails instead of emitting a broken join.""" @@ -445,7 +457,7 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], result = convert_expression( expr, entity_name=metric.name, owner_of=owner_of, ref_of=ref_of, percentile_unsupported=percentile_unsupported, - name_taken=self._model_has_column, + name_taken=self._model_has_name, ) if not result.ok: self._unconv( @@ -464,13 +476,27 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], ) return + # Build the measure first so a construction failure (e.g. a metric named + # after a reserved transform like ``cumsum``) clean-fails instead of + # crashing the import — and before materializing columns, so a rejected + # metric leaves no orphan hidden columns. + try: + measure = ModelMeasure( + formula=result.formula, + name=metric.name, + description=_render_description(metric.description, metric.ai_context), + meta=_build_meta(metric.ai_context, metric.custom_extensions), + ) + except Exception as exc: # noqa: BLE001 — any validation error -> report + self._unconv( + f"Metric {metric.name!r} cannot be expressed as a SLayer measure " + f"({exc}).", + metric_name=metric.name, + ) + return + self._materialize_columns(result.materialized) - self._models[anchor].measures.append(ModelMeasure( - formula=result.formula, - name=metric.name, - description=_render_description(metric.description, metric.ai_context), - meta=_build_meta(metric.ai_context, metric.custom_extensions), - )) + self._models[anchor].measures.append(measure) for w in result.warnings: self._warn(f"Metric {metric.name!r}: {w}", metric_name=metric.name, category="dialect", severity="info") @@ -540,10 +566,11 @@ def owner_of(qualifier: Optional[str], column: str) -> Optional[str]: # otherwise a metric like SUM(orders.no_such_col) would import # as a measure that fails at query time. return qualifier if has_column(qualifier, column) else None - for name in sm_model_names: - if has_column(name, column): - return name - return None + # Unqualified: resolve only when exactly one dataset owns the column. + # Ambiguity (the same column name on multiple datasets) returns None + # so the metric clean-fails instead of binding by dataset order. + matches = [name for name in sm_model_names if has_column(name, column)] + return matches[0] if len(matches) == 1 else None return owner_of def _make_ref_of(self, graph: JoinGraph, anchor: str): diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index d4e85650..c2ef0918 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -484,6 +484,74 @@ def test_derived_field_shadowing_physical_column_overlays(shop_engine): assert status_cols[0].sql == "LOWER(status)" +def test_bare_alias_field_shadowing_physical_column_replaces(shop_engine): + # A bare-identifier field aliasing one physical column onto the name of + # another must replace the shadowed column, not be silently dropped. + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="status", expression=_expr("order_id"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + status_cols = [c for c in orders.columns if c.name == "status"] + assert len(status_cols) == 1 and status_cols[0].sql == "order_id" + + +def test_materialized_name_avoids_existing_measure(shop_engine): + # A materialized operand name must avoid an existing MEASURE name too + # (SLayer columns and measures share one namespace). + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount")), + OSIField(name="quantity", expression=_expr("quantity"))], + )], + metrics=[ + OSIMetric(name="_rev_0", expression=_expr("SUM(amount)")), + OSIMetric(name="rev", expression=_expr("SUM(quantity * amount)")), + ], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "_rev_0" in {m.name for m in orders.measures} + derived = {m.name: m for m in orders.measures}["rev"].formula.split(":")[0] + assert derived != "_rev_0" + + +def test_metric_named_after_transform_clean_fails(shop_engine): + # A metric named after a reserved transform (cumsum) must clean-fail, not + # crash the whole import at ModelMeasure construction. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="cumsum", expression=_expr("SUM(amount)"))], + ) + result = _convert(shop_engine, doc) # must not raise + orders = {m.name: m for m in result.models}["orders"] + assert "cumsum" not in {m.name for m in orders.measures} + assert _reported(result) + + +def test_ambiguous_unqualified_column_clean_fails(shop_engine): + # customer_id exists on both orders and customers; an unqualified reference + # is ambiguous and must clean-fail rather than bind by dataset order. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="segment", expression=_expr("segment"))]), + ], + metrics=[OSIMetric(name="cid", expression=_expr("COUNT(customer_id)"))], + ) + result = _convert(shop_engine, doc) + all_measures = {meas.name for m in result.models for meas in m.measures} + assert "cid" not in all_measures + assert _reported(result) + + def test_materialized_name_avoids_existing_column(shop_engine): # A materialized operand name must not collide with an existing column, # else the metric would aggregate the wrong column. From 74d18d4ff8f015bd16fbd4656dc94c9cdfdd3841 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 14:32:09 +0200 Subject: [PATCH 05/25] DEV-1643: address fourth-round Codex findings (expression column existence) - 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) --- slayer/osi/converter.py | 31 +++++++++++++++++++++++++++++-- slayer/osi/expression.py | 13 +++++++++---- tests/test_osi_converter.py | 29 +++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 6 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 926312f8..0e697e55 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -68,6 +68,23 @@ def _is_bare_identifier(sql: str) -> bool: return bool(_IDENTIFIER_RE.match(sql.strip())) +def _missing_expr_columns(sql: str, available: set[str]) -> list[str]: + """Bare (unqualified) column names in ``sql`` that are not in ``available``. + + Qualified references (``table.col``) are left alone — they may point at a + joined model and are not validated against the local column set here. + """ + try: + tree = sqlglot.parse_one(sql) + except sqlglot.errors.ParseError: + return [] + missing = [] + for col in tree.find_all(exp.Column): + if not col.table and col.name not in available: + missing.append(col.name) + return missing + + def _render_description(explicit: Optional[str], ctx: Optional[OSIAIContext]) -> Optional[str]: """Description = explicit OSI description (lead) + ai_context instructions + synonyms.""" @@ -314,8 +331,18 @@ def _resolve_field_column(self, field: OSIField, sql: str, model_name=ds.name, category="missing_column", ) return None - # derived expression (collision with an existing column is handled by - # the replace-or-append logic in _overlay_one_field). + # derived expression. Validate its column references exist on the table + # (consistent with the bare-field / metric / relationship checks); a + # collision with an existing column is handled by the replace-or-append + # logic in _overlay_one_field. + missing = _missing_expr_columns(sql, introspected) + if missing: + self._warn( + f"Field {field.name!r} on {ds.name!r} references unknown " + f"column(s) {missing}; skipping.", + model_name=ds.name, category="missing_column", + ) + return None is_time = bool(field.dimension and field.dimension.is_time) return Column( name=field.name, sql=sql, diff --git a/slayer/osi/expression.py b/slayer/osi/expression.py index 4f7e67db..b62fdf9f 100644 --- a/slayer/osi/expression.py +++ b/slayer/osi/expression.py @@ -95,12 +95,17 @@ def _materialize(self, operand: exp.Expression) -> str: if operand.find(exp.AggFunc, exp.Window, exp.WithinGroup): raise _Unconvertible("nested aggregate in aggregate operand") columns = list(operand.find_all(exp.Column)) - owners = {self.owner_of(c.table or None, c.name) for c in columns} - owners.discard(None) - if len(owners) != 1: + resolved = [self.owner_of(c.table or None, c.name) for c in columns] + # An unresolved column (unknown, or ambiguous) must fail the whole + # operand — discarding it would materialize SQL referencing a column + # that does not exist. + if not columns or any(owner is None for owner in resolved): raise _Unconvertible( - "aggregate operand spans multiple datasets (or resolves to none)" + "aggregate operand references a column that cannot be resolved" ) + owners = set(resolved) + if len(owners) != 1: + raise _Unconvertible("aggregate operand spans multiple datasets") owner = owners.pop() operand_sql = operand.sql() key = (owner, operand_sql) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index c2ef0918..6a59e35c 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -484,6 +484,35 @@ def test_derived_field_shadowing_physical_column_overlays(shop_engine): assert status_cols[0].sql == "LOWER(status)" +def test_derived_field_expression_unknown_column_clean_fails(shop_engine): + # A derived field expression referencing a nonexistent column must clean-fail + # rather than import a column that fails at query time. + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="gross", expression=_expr("amount + no_such_col"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "gross" not in {c.name for c in orders.columns} + assert _reported(result) + + +def test_metric_operand_unknown_column_clean_fails(shop_engine): + # A materialized aggregate operand where one column is unknown must clean- + # fail, not materialize SQL that still references the missing column. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="bad", expression=_expr("SUM(amount + no_such_col)"))], + ) + result = _convert(shop_engine, doc) + all_measures = {meas.name for m in result.models for meas in m.measures} + assert "bad" not in all_measures + assert _reported(result) + + def test_bare_alias_field_shadowing_physical_column_replaces(shop_engine): # A bare-identifier field aliasing one physical column onto the name of # another must replace the shadowed column, not be silently dropped. From 65fc3d83006585a874b8f1fa71d2dd826e577f69 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 14:40:12 +0200 Subject: [PATCH 06/25] DEV-1643: address fifth-round Codex findings (derived-field expr validation) - 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 (.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) --- slayer/osi/converter.py | 32 +++++++++++++++++++++++--------- tests/test_osi_converter.py | 26 ++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 0e697e55..599aeecc 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -68,20 +68,27 @@ def _is_bare_identifier(sql: str) -> bool: return bool(_IDENTIFIER_RE.match(sql.strip())) -def _missing_expr_columns(sql: str, available: set[str]) -> list[str]: - """Bare (unqualified) column names in ``sql`` that are not in ``available``. - - Qualified references (``table.col``) are left alone — they may point at a - joined model and are not validated against the local column set here. +def _missing_expr_columns( + sql: str, available: set[str], self_name: str +) -> list[str] | None: + """Unqualified and self-qualified column names in ``sql`` absent from + ``available``. Returns ``None`` when ``sql`` cannot be parsed. + + Self-qualified references (``.col``) are validated too; genuinely + cross-model references (a different qualifier) are left to query-time join + resolution, matching how ``Column.sql`` expansion treats join aliases. """ try: tree = sqlglot.parse_one(sql) except sqlglot.errors.ParseError: - return [] + return None missing = [] for col in tree.find_all(exp.Column): - if not col.table and col.name not in available: - missing.append(col.name) + if not col.table: + if col.name not in available: + missing.append(col.name) + elif col.table == self_name and col.name not in available: + missing.append(f"{col.table}.{col.name}") return missing @@ -335,7 +342,14 @@ def _resolve_field_column(self, field: OSIField, sql: str, # (consistent with the bare-field / metric / relationship checks); a # collision with an existing column is handled by the replace-or-append # logic in _overlay_one_field. - missing = _missing_expr_columns(sql, introspected) + missing = _missing_expr_columns(sql, introspected, ds.name) + if missing is None: + self._warn( + f"Field {field.name!r} on {ds.name!r} has an unparseable " + f"expression {sql!r}; skipping.", + model_name=ds.name, category="expression", + ) + return None if missing: self._warn( f"Field {field.name!r} on {ds.name!r} references unknown " diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 6a59e35c..4a09966e 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -499,6 +499,32 @@ def test_derived_field_expression_unknown_column_clean_fails(shop_engine): assert _reported(result) +def test_derived_field_unparseable_expression_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="gross", expression=_expr("amount +"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "gross" not in {c.name for c in orders.columns} + assert _reported(result) + + +def test_derived_field_self_qualified_unknown_column_clean_fails(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="gross", expression=_expr("orders.amount + orders.no_such_col"))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "gross" not in {c.name for c in orders.columns} + assert _reported(result) + + def test_metric_operand_unknown_column_clean_fails(shop_engine): # A materialized aggregate operand where one column is unknown must clean- # fail, not materialize SQL that still references the missing column. From f429fbbdc91cc9d5785cf7b488576335fde423a3 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 15:30:24 +0200 Subject: [PATCH 07/25] DEV-1643: ingest-time validation of cross-model derived-field refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A derived field expression may reference a joined model via . / __.. 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) --- slayer/osi/converter.py | 53 +++++++++++++++++++++++++++++++++++ tests/test_osi_converter.py | 55 +++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 599aeecc..e30332f5 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -175,6 +175,8 @@ def convert(self) -> ConversionResult: for rel in sm.relationships or []: self._build_join(rel) + self._validate_cross_model_field_refs() + graph = JoinGraph.build_from_models(list(self._models.values())) for sm in semantic_models: self._build_measures_for(sm, graph) @@ -424,6 +426,57 @@ def _build_join(self, rel: OSIRelationship) -> None: meta=_build_meta(rel.ai_context, rel.custom_extensions), )) + def _validate_cross_model_field_refs(self) -> None: + """Post-join pass: derived columns may reference joined models via + ``.`` / ``__.``. Resolve each such ref through the + join graph and drop (with a report) any column whose cross-model ref + names a model with no join path or a nonexistent target column — so a + typo clean-fails at import instead of erroring at query time. + """ + for model in list(self._models.values()): + for col in list(model.columns): + if not col.sql: + continue + bad = self._unresolvable_cross_model_refs(model, col.sql) + if bad: + model.columns.remove(col) + self._warn( + f"Column {col.name!r} on {model.name!r} references " + f"unresolvable cross-model column(s) {bad}; dropping.", + model_name=model.name, category="missing_column", + ) + + def _unresolvable_cross_model_refs(self, model: SlayerModel, sql: str) -> list[str]: + """Cross-model (non-self, qualified) column refs in ``sql`` that don't + resolve to a joined model + existing column. Self / unqualified refs were + validated at field-overlay time.""" + try: + tree = sqlglot.parse_one(sql) + except sqlglot.errors.ParseError: + return [] + bad = [] + for col in tree.find_all(exp.Column): + if not col.table or col.table == model.name: + continue + target = self._walk_join_alias(model, col.table) + if target is None or not any(c.name == col.name for c in target.columns): + bad.append(f"{col.table}.{col.name}") + return bad + + def _walk_join_alias(self, host: SlayerModel, alias: str) -> SlayerModel | None: + """Resolve a ``__``-delimited join alias (e.g. ``customers__regions``) + to the terminal joined model by walking ``host``'s join chain, or None + if any hop is not a declared join.""" + current = host + for hop in (alias.split("__") if "__" in alias else [alias]): + join = next((j for j in current.joins if j.target_model == hop), None) + if join is None: + return None + current = self._models.get(hop) + if current is None: + return None + return current + def _model_has_column(self, model_name: str, column: str) -> bool: model = self._models.get(model_name) return model is not None and any(c.name == column for c in model.columns) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 4a09966e..1c0e0ef6 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -499,6 +499,61 @@ def test_derived_field_expression_unknown_column_clean_fails(shop_engine): assert _reported(result) +def test_valid_cross_model_derived_field_kept(shop_engine): + # A derived field with a valid cross-model ref (join exists, column exists) + # survives the post-join validation pass. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id")), + OSIField(name="cust_seg", expression=_expr("customers.segment"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))]), + ], + relationships=[OSIRelationship( + name="o2c", **{"from": "orders"}, to="customers", + from_columns=["customer_id"], to_columns=["customer_id"])], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "cust_seg" in {c.name for c in orders.columns} + + +def test_cross_model_derived_field_unknown_column_dropped(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id")), + OSIField(name="bad", expression=_expr("customers.no_such_col"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))]), + ], + relationships=[OSIRelationship( + name="o2c", **{"from": "orders"}, to="customers", + from_columns=["customer_id"], to_columns=["customer_id"])], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "bad" not in {c.name for c in orders.columns} + assert _reported(result) + + +def test_cross_model_derived_field_no_join_path_dropped(shop_engine): + # products.category referenced from orders with NO relationship -> dropped. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="cat", expression=_expr("products.category"))]), + OSIDataset(name="products", source="products", + fields=[OSIField(name="category", expression=_expr("category"))]), + ], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "cat" not in {c.name for c in orders.columns} + assert _reported(result) + + def test_derived_field_unparseable_expression_clean_fails(shop_engine): doc = _mini_doc( datasets=[OSIDataset( From b4e6225de41af7bcd3b645fda477c4815cbc36ea Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 15:40:47 +0200 Subject: [PATCH 08/25] DEV-1643: harden cross-model field validation + duplicate joins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- slayer/osi/converter.py | 55 +++++++++++++++++++++++--------- tests/test_osi_converter.py | 62 +++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 14 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index e30332f5..ccede6cd 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -22,6 +22,7 @@ from slayer.core.formula import parse_formula from slayer.core.models import Column, ModelJoin, ModelMeasure, SlayerModel from slayer.core.refs import IDENTIFIER_RE as _IDENTIFIER_RE +from slayer.engine.column_expansion import _root_scope_column_ids from slayer.engine.ingestion import introspect_table_to_model from slayer.engine.join_graph import JoinGraph, min_hops_root from slayer.ingest_report import ConversionResult, ConversionWarning @@ -418,6 +419,19 @@ def _build_join(self, rel: OSIRelationship) -> None: ) return + # SLayer's ModelJoin keys only on target_model and runtime join-walking + # picks the first match, so a second relationship to the same target + # (e.g. a distinct role) would be unreachable and could bind refs to the + # wrong join. Keep the first; report and skip duplicates. + if any(j.target_model == rel.to for j in self._models[src].joins): + self._warn( + f"Relationship {rel.name!r} is a second join from {src!r} to " + f"{rel.to!r}; SLayer cannot disambiguate multiple joins to one " + f"model (no join aliases). Keeping the first; skipping this one.", + model_name=src, category="relationship", + ) + return + pairs = [[f, t] for f, t in zip(rel.from_columns, rel.to_columns)] self._models[src].joins.append(ModelJoin( target_model=rel.to, @@ -433,29 +447,42 @@ def _validate_cross_model_field_refs(self) -> None: names a model with no join path or a nonexistent target column — so a typo clean-fails at import instead of erroring at query time. """ - for model in list(self._models.values()): - for col in list(model.columns): - if not col.sql: - continue - bad = self._unresolvable_cross_model_refs(model, col.sql) - if bad: - model.columns.remove(col) - self._warn( - f"Column {col.name!r} on {model.name!r} references " - f"unresolvable cross-model column(s) {bad}; dropping.", - model_name=model.name, category="missing_column", - ) + # Fixed-point: dropping a column can invalidate another column that + # referenced it, so re-run until a pass drops nothing. + changed = True + while changed: + changed = False + for model in list(self._models.values()): + for col in list(model.columns): + if not col.sql: + continue + bad = self._unresolvable_cross_model_refs(model, col.sql) + if bad: + model.columns.remove(col) + changed = True + self._warn( + f"Column {col.name!r} on {model.name!r} references " + f"unresolvable cross-model column(s) {bad}; dropping.", + model_name=model.name, category="missing_column", + ) def _unresolvable_cross_model_refs(self, model: SlayerModel, sql: str) -> list[str]: """Cross-model (non-self, qualified) column refs in ``sql`` that don't - resolve to a joined model + existing column. Self / unqualified refs were - validated at field-overlay time.""" + resolve to a joined model + existing column. Mirrors runtime scope rules: + only root-scope refs are checked (nested subquery/CTE aliases are left + alone), and catalog/db-qualified physical refs are skipped. Self / + unqualified refs were validated at field-overlay time.""" try: tree = sqlglot.parse_one(sql) except sqlglot.errors.ParseError: return [] + root_ids = _root_scope_column_ids(parsed=tree) bad = [] for col in tree.find_all(exp.Column): + if id(col) not in root_ids: + continue # nested-scope alias (CTE / sub-query) — not a join ref + if col.args.get("db") or col.args.get("catalog"): + continue # catalog/db-qualified physical ref — outside SLayer's contract if not col.table or col.table == model.name: continue target = self._walk_join_alias(model, col.table) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 1c0e0ef6..634f791f 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -499,6 +499,68 @@ def test_derived_field_expression_unknown_column_clean_fails(shop_engine): assert _reported(result) +def test_cross_model_validation_fixed_point_transitive_drop(shop_engine): + # orders.cust_bad -> customers.bad_region -> regions.no_such_col. + # bad_region drops (regions.no_such_col missing); cust_bad must drop too. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id")), + OSIField(name="cust_bad", expression=_expr("customers.bad_region"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="customer_id", expression=_expr("customer_id")), + OSIField(name="region_id", expression=_expr("region_id")), + OSIField(name="bad_region", expression=_expr("regions.no_such_col"))]), + OSIDataset(name="regions", source="regions", + fields=[OSIField(name="region_id", expression=_expr("region_id"))]), + ], + relationships=[ + OSIRelationship(name="o2c", **{"from": "orders"}, to="customers", + from_columns=["customer_id"], to_columns=["customer_id"]), + OSIRelationship(name="c2r", **{"from": "customers"}, to="regions", + from_columns=["region_id"], to_columns=["region_id"]), + ], + ) + result = _convert(shop_engine, doc) + models = {m.name: m for m in result.models} + assert "bad_region" not in {c.name for c in models["customers"].columns} + assert "cust_bad" not in {c.name for c in models["orders"].columns} + + +def test_nested_scope_alias_in_derived_field_not_flagged(shop_engine): + # x is a subquery alias, not a join; scope-aware validation must keep it. + doc = _mini_doc(datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="sub", + expression=_expr("(SELECT x.amount FROM orders AS x LIMIT 1)"))], + )]) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "sub" in {c.name for c in orders.columns} + + +def test_duplicate_relationship_to_same_target_skipped(shop_engine): + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id")), + OSIField(name="product_id", expression=_expr("product_id"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))]), + ], + relationships=[ + OSIRelationship(name="o2c_bill", **{"from": "orders"}, to="customers", + from_columns=["customer_id"], to_columns=["customer_id"]), + OSIRelationship(name="o2c_ship", **{"from": "orders"}, to="customers", + from_columns=["product_id"], to_columns=["customer_id"]), + ], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert len([j for j in orders.joins if j.target_model == "customers"]) == 1 + assert _reported(result) + + def test_valid_cross_model_derived_field_kept(shop_engine): # A derived field with a valid cross-model ref (join exists, column exists) # survives the post-join validation pass. From 87ad02c44dd9b081b80963f4c2a826ee943430e1 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 15:48:33 +0200 Subject: [PATCH 09/25] DEV-1643: honor requested dialect only when it is SQL-compatible _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) --- slayer/osi/converter.py | 9 +++++++-- tests/test_osi_converter.py | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index ccede6cd..c5ec8321 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -717,9 +717,14 @@ def _materialize_columns(self, materialized) -> None: def _resolve_expression(self, osi_expr: OSIExpression) -> str | None: """Pick the expression for the requested dialect, else fall back among - SQL-compatible dialects. Non-SQL-only expressions return None.""" + SQL-compatible dialects. Non-SQL-only expressions return None. + + The requested dialect is honored only when it is itself SQL-compatible — + a non-SQL request (e.g. ``--dialect MDX``) must not feed non-SQL syntax + into the SQL conversion path; it falls back to an available SQL dialect. + """ by_dialect = {de.dialect.value: de.expression for de in osi_expr.dialects} - if self.dialect in by_dialect: + if self.dialect in by_dialect and self.dialect in SQL_DIALECTS: return by_dialect[self.dialect] for name, expression in by_dialect.items(): if name in SQL_DIALECTS: diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 634f791f..96ae9e39 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -840,6 +840,22 @@ def test_dialect_fallback_among_sql_dialects(shop_engine): assert {"tot"} <= {meas.name for meas in orders.measures} +def test_requested_non_sql_dialect_falls_back_to_sql(shop_engine): + # --dialect MDX must NOT feed the MDX expression into the SQL path; it falls + # back to the available SQL (ANSI_SQL) expression. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="tot", expression=OSIExpression(dialects=[ + OSIDialectExpression(dialect="MDX", expression="[Measures].[x]"), + OSIDialectExpression(dialect="ANSI_SQL", expression="SUM(amount)"), + ]))], + ) + result = _convert(shop_engine, doc, dialect="MDX") + orders = {m.name: m for m in result.models}["orders"] + assert {m.name: m for m in orders.measures}["tot"].formula == "amount:sum" + + def test_target_dialect_percentile_caveat(shop_engine): doc = _mini_doc( datasets=[OSIDataset(name="orders", source="orders", From 4b95384079c1146952ce2e1e570f62fa5dfe034c Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 15:55:56 +0200 Subject: [PATCH 10/25] DEV-1643: don't misclassify quoted identifiers containing 'select' as 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) --- slayer/osi/source.py | 5 ++++- tests/test_osi_source.py | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/slayer/osi/source.py b/slayer/osi/source.py index e344d661..d925c4c6 100644 --- a/slayer/osi/source.py +++ b/slayer/osi/source.py @@ -42,10 +42,13 @@ def _has_top_level_space(s: str) -> bool: def _looks_like_query(s: str) -> bool: stripped = s.strip() + # Run the SELECT check on the text OUTSIDE double-quoted spans so a valid + # quoted identifier segment like "My Select" isn't mistaken for a query. + unquoted = re.sub(r'"[^"]*"', "", stripped) return ( stripped.startswith("(") or _has_top_level_space(stripped) - or bool(_SELECT_RE.search(stripped)) + or bool(_SELECT_RE.search(unquoted)) ) diff --git a/tests/test_osi_source.py b/tests/test_osi_source.py index ba1abb56..19d4d1de 100644 --- a/tests/test_osi_source.py +++ b/tests/test_osi_source.py @@ -39,6 +39,16 @@ def test_quoted_identifiers_unwrapped(): assert p.table == "My Table" and p.schema_name == "My Schema" +def test_quoted_identifier_containing_select_is_not_a_query(): + # 'select' inside a quoted segment must not be treated as a SQL query. + p = parse_source('"My Select"."Orders"') + assert p.is_query is False + assert p.table == "Orders" and p.schema_name == "My Select" + p2 = parse_source('"select"."orders"') + assert p2.is_query is False + assert p2.table == "orders" and p2.schema_name == "select" + + def test_query_source_detected(): p = parse_source("SELECT id, amount FROM orders WHERE amount > 0") assert p.is_query is True From d4ccf96a470d3e7e38351ca142482c2805636e48 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:05:10 +0200 Subject: [PATCH 11/25] DEV-1643: clean-fail SUM/AVG/MIN/MAX(DISTINCT ...) 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) --- slayer/osi/expression.py | 4 ++++ tests/test_osi_expression.py | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/slayer/osi/expression.py b/slayer/osi/expression.py index b62fdf9f..101f4954 100644 --- a/slayer/osi/expression.py +++ b/slayer/osi/expression.py @@ -144,6 +144,10 @@ def _agg_ref(self, node: exp.Expression) -> str: if type(node) in _SIMPLE_AGG: agg = _SIMPLE_AGG[type(node)] + # SLayer supports DISTINCT only for COUNT (handled below); SUM/AVG/ + # MIN/MAX over DISTINCT have no colon-syntax representation. + if isinstance(node.this, exp.Distinct): + raise _Unconvertible(f"{agg.upper()}(DISTINCT ...) is not supported") return f"{self._operand_ref(node.this)}:{agg}" if isinstance(node, exp.Count): diff --git a/tests/test_osi_expression.py b/tests/test_osi_expression.py index 4c20960a..1e482141 100644 --- a/tests/test_osi_expression.py +++ b/tests/test_osi_expression.py @@ -190,6 +190,12 @@ def test_non_passthrough_function_clean_fails(): assert not r.ok +def test_sum_distinct_clean_fails(): + # Only COUNT(DISTINCT) is supported; SUM/AVG/MIN/MAX DISTINCT clean-fail. + assert not _run("SUM(DISTINCT amount)").ok + assert not _run("AVG(DISTINCT amount)").ok + + # ───────────────────────── cross-model refs ──────────────────────────────── def test_qualified_column_emits_dotted_ref(): From 56a916c91fccf966e5944c7ebc1c84d5a81bc79b Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:14:01 +0200 Subject: [PATCH 12/25] DEV-1643: treat quoted bare field expressions as base-column references 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) --- slayer/osi/converter.py | 33 +++++++++++++++++++++++++-------- tests/test_osi_converter.py | 16 ++++++++++++++++ 2 files changed, 41 insertions(+), 8 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index c5ec8321..92887d11 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -65,8 +65,24 @@ def _legal_measure_name(name: str) -> bool: return bool(_IDENTIFIER_RE.match(name)) -def _is_bare_identifier(sql: str) -> bool: - return bool(_IDENTIFIER_RE.match(sql.strip())) +def _as_bare_column(sql: str) -> str | None: + """The unquoted column name if ``sql`` is a single unqualified column + reference (bare or double-quoted), else ``None``. + + Lets a case-sensitive quoted identifier (e.g. ``"legalEntityType"``) be + treated as a base-column reference rather than a derived expression. + """ + stripped = sql.strip() + if _IDENTIFIER_RE.match(stripped): + return stripped + try: + tree = sqlglot.parse_one(stripped) + except sqlglot.errors.ParseError: + return None + if (isinstance(tree, exp.Column) and not tree.table + and not tree.args.get("db") and not tree.args.get("catalog")): + return tree.name + return None def _missing_expr_columns( @@ -329,14 +345,15 @@ def _resolve_field_column(self, field: OSIField, sql: str, ds: OSIDataset) -> Column | None: """Return the Column (existing or new) this field maps to, or None on a clean-fail (already reported).""" - if _is_bare_identifier(sql): - if sql == field.name and field.name in by_name: + bare = _as_bare_column(sql) + if bare is not None: + if bare == field.name and field.name in by_name: return by_name[field.name] # overlay existing column - if sql in introspected: - # aliased/renamed reference to a real column -> derived column - return Column(name=field.name, sql=sql, type=by_name[sql].type) + if bare in introspected: + # aliased/renamed reference to a real column + return Column(name=field.name, sql=bare, type=by_name[bare].type) self._warn( - f"Field {field.name!r} on {ds.name!r} references column {sql!r} " + f"Field {field.name!r} on {ds.name!r} references column {bare!r} " f"which is not present in the table; skipping.", model_name=ds.name, category="missing_column", ) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 96ae9e39..7653f33a 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -468,6 +468,22 @@ def test_relationship_unknown_target_clean_fails(shop_engine): assert _reported(result) +def test_quoted_bare_field_expression_overlays_not_derived(shop_engine): + # A double-quoted identifier expression is a base-column reference, not a + # derived expression — it must overlay the introspected column (keep its + # TEXT type), not rebuild it as DOUBLE. + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="status", expression=_expr('"status"'))], + )] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + status = {c.name: c for c in orders.columns}["status"] + assert status.type == DataType.TEXT + + def test_derived_field_shadowing_physical_column_overlays(shop_engine): # A derived OSI field whose name matches a physical column must overlay its # expression onto that column, not be silently dropped. From 9b0b67250ae6158b275ae391f239f044a08c6335 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:22:58 +0200 Subject: [PATCH 13/25] DEV-1643: OSI primary_key overrides physical PK; prune stale joins - 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) --- slayer/osi/converter.py | 77 +++++++++++++++++++++++++++---------- tests/test_osi_converter.py | 36 +++++++++++++++++ 2 files changed, 92 insertions(+), 21 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 92887d11..dfcad921 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -283,10 +283,13 @@ def _overlay_fields(self, model: SlayerModel, ds: OSIDataset) -> None: if time_col and first_time_dim is None: first_time_dim = time_col - # OSI primary_key is authoritative. - for pk in ds.primary_key or []: - if pk in by_name: - by_name[pk].primary_key = True + # OSI primary_key is authoritative: when set, it fully REPLACES the + # introspected primary key (clearing physical PK flags that OSI omits); + # when unset, the introspected PK is kept. + if ds.primary_key: + osi_pk = set(ds.primary_key) + for col in model.columns: + col.primary_key = col.name in osi_pk if first_time_dim and not model.default_time_dimension: model.default_time_dimension = first_time_dim @@ -465,23 +468,55 @@ def _validate_cross_model_field_refs(self) -> None: typo clean-fails at import instead of erroring at query time. """ # Fixed-point: dropping a column can invalidate another column that - # referenced it, so re-run until a pass drops nothing. - changed = True - while changed: - changed = False - for model in list(self._models.values()): - for col in list(model.columns): - if not col.sql: - continue - bad = self._unresolvable_cross_model_refs(model, col.sql) - if bad: - model.columns.remove(col) - changed = True - self._warn( - f"Column {col.name!r} on {model.name!r} references " - f"unresolvable cross-model column(s) {bad}; dropping.", - model_name=model.name, category="missing_column", - ) + # referenced it, and dropping a column used as a join key invalidates + # that join (which can in turn invalidate more columns). Re-run column + # and join pruning until a pass changes nothing. + while True: + if not (self._drop_invalid_cross_model_columns() + or self._drop_stale_joins()): + break + + def _drop_invalid_cross_model_columns(self) -> bool: + dropped = False + for model in list(self._models.values()): + for col in list(model.columns): + if not col.sql: + continue + bad = self._unresolvable_cross_model_refs(model, col.sql) + if bad: + model.columns.remove(col) + dropped = True + self._warn( + f"Column {col.name!r} on {model.name!r} references " + f"unresolvable cross-model column(s) {bad}; dropping.", + model_name=model.name, category="missing_column", + ) + return dropped + + def _drop_stale_joins(self) -> bool: + """Drop joins whose key columns were removed by column validation, so a + join never references a column that no longer exists.""" + dropped = False + for model in list(self._models.values()): + for join in list(model.joins): + if self._join_columns_missing(model, join): + model.joins.remove(join) + dropped = True + self._warn( + f"Join from {model.name!r} to {join.target_model!r} " + f"references a column removed during validation; dropping.", + model_name=model.name, category="relationship", + ) + return dropped + + def _join_columns_missing(self, model: SlayerModel, join: ModelJoin) -> bool: + target = self._models.get(join.target_model) + for src_col, tgt_col in join.join_pairs: + if not self._model_has_column(model.name, src_col): + return True + if target is None or not any(c.name == tgt_col for c in target.columns): + return True + return False def _unresolvable_cross_model_refs(self, model: SlayerModel, sql: str) -> list[str]: """Cross-model (non-self, qualified) column refs in ``sql`` that don't diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 7653f33a..35d921a0 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -336,6 +336,42 @@ def test_per_dataset_failure_isolation(shop_engine): assert _reported(result) +def test_osi_primary_key_overrides_introspected(shop_engine): + # OSI primary_key is authoritative: it replaces the physical PK (order_id). + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", primary_key=["customer_id"], + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))], + )] + ) + result = _convert(shop_engine, doc) + cols = {c.name: c for c in {m.name: m for m in result.models}["orders"].columns} + assert cols["customer_id"].primary_key is True + assert cols["order_id"].primary_key is False + + +def test_join_using_dropped_derived_column_is_pruned(shop_engine): + # A join key that is a derived field dropped by cross-model validation must + # not leave a stale join referencing the removed column. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="product_id", expression=_expr("product_id")), + OSIField(name="bad_key", expression=_expr("customers.no_such_col"))]), + OSIDataset(name="products", source="products", + fields=[OSIField(name="product_id", expression=_expr("product_id"))]), + ], + relationships=[OSIRelationship( + name="o2p", **{"from": "orders"}, to="products", + from_columns=["bad_key"], to_columns=["product_id"])], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "bad_key" not in {c.name for c in orders.columns} + assert orders.joins == [] + assert _reported(result) + + def test_unique_keys_into_meta(shop_engine): doc = _mini_doc( datasets=[OSIDataset( From 4969947ef513b5089fff3a1064aa6b591b75730a Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:31:22 +0200 Subject: [PATCH 14/25] DEV-1643: preserve alias quoting; validate OSI primary_key entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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) --- slayer/osi/converter.py | 25 +++++++++++++++++++------ tests/test_osi_converter.py | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index dfcad921..de800649 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -285,11 +285,22 @@ def _overlay_fields(self, model: SlayerModel, ds: OSIDataset) -> None: # OSI primary_key is authoritative: when set, it fully REPLACES the # introspected primary key (clearing physical PK flags that OSI omits); - # when unset, the introspected PK is kept. + # when unset, the introspected PK is kept. If any listed key column is + # missing (typo), the override is unsafe (it would leave the model with + # no PK) — report and keep the introspected PK instead. if ds.primary_key: - osi_pk = set(ds.primary_key) - for col in model.columns: - col.primary_key = col.name in osi_pk + present = {c.name for c in model.columns} + missing_pk = [k for k in ds.primary_key if k not in present] + if missing_pk: + self._warn( + f"Dataset {ds.name!r} primary_key references unknown " + f"column(s) {missing_pk}; keeping the introspected primary key.", + model_name=ds.name, category="primary_key", + ) + else: + osi_pk = set(ds.primary_key) + for col in model.columns: + col.primary_key = col.name in osi_pk if first_time_dim and not model.default_time_dimension: model.default_time_dimension = first_time_dim @@ -353,8 +364,10 @@ def _resolve_field_column(self, field: OSIField, sql: str, if bare == field.name and field.name in by_name: return by_name[field.name] # overlay existing column if bare in introspected: - # aliased/renamed reference to a real column - return Column(name=field.name, sql=bare, type=by_name[bare].type) + # aliased/renamed reference to a real column. Keep the ORIGINAL + # expression (preserving any identifier quoting, which matters on + # case-folding dialects); use the unquoted name only for lookup. + return Column(name=field.name, sql=sql, type=by_name[bare].type) self._warn( f"Field {field.name!r} on {ds.name!r} references column {bare!r} " f"which is not present in the table; skipping.", diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 35d921a0..1d157944 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -336,6 +336,29 @@ def test_per_dataset_failure_isolation(shop_engine): assert _reported(result) +def test_quoted_alias_field_preserves_quoting(shop_engine): + # An aliased quoted column keeps its quoting in Column.sql (matters on + # case-folding dialects); the unquoted name is used only for lookup. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="legal_type", expression=_expr('"status"'))])] + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert {c.name: c for c in orders.columns}["legal_type"].sql == '"status"' + + +def test_invalid_osi_primary_key_keeps_introspected(shop_engine): + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", primary_key=["no_such_pk"], + fields=[OSIField(name="amount", expression=_expr("amount"))])] + ) + result = _convert(shop_engine, doc) + cols = {c.name: c for c in {m.name: m for m in result.models}["orders"].columns} + assert cols["order_id"].primary_key is True # introspected PK preserved + assert _reported(result) + + def test_osi_primary_key_overrides_introspected(shop_engine): # OSI primary_key is authoritative: it replaces the physical PK (order_id). doc = _mini_doc( From fe3fdba11b29705444306247dcd6c1387bed955e Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:51:23 +0200 Subject: [PATCH 15/25] DEV-1643: friendly bad-path handling; drop unnecessary list() calls - 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) --- slayer/cli.py | 6 ++++- slayer/osi/converter.py | 48 +++++++++++++++++++----------------- tests/test_cli_import_osi.py | 8 ++++++ 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/slayer/cli.py b/slayer/cli.py index 227add15..a0f84f79 100644 --- a/slayer/cli.py +++ b/slayer/cli.py @@ -1548,7 +1548,11 @@ def _run_import_osi(args): from slayer.sql import engine_factory storage = _resolve_storage(args) - documents = parse_osi_path(args.osi_path) + try: + documents = parse_osi_path(args.osi_path) + except FileNotFoundError as exc: + print(str(exc)) + sys.exit(1) if not documents: print(f"No OSI documents found in {args.osi_path}") sys.exit(1) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index de800649..fef747d6 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -491,35 +491,37 @@ def _validate_cross_model_field_refs(self) -> None: def _drop_invalid_cross_model_columns(self) -> bool: dropped = False - for model in list(self._models.values()): - for col in list(model.columns): - if not col.sql: - continue - bad = self._unresolvable_cross_model_refs(model, col.sql) - if bad: - model.columns.remove(col) - dropped = True - self._warn( - f"Column {col.name!r} on {model.name!r} references " - f"unresolvable cross-model column(s) {bad}; dropping.", - model_name=model.name, category="missing_column", - ) + for model in self._models.values(): + bad_cols = [] + for col in model.columns: + if col.sql: + bad = self._unresolvable_cross_model_refs(model, col.sql) + if bad: + bad_cols.append((col, bad)) + for col, bad in bad_cols: + model.columns.remove(col) + dropped = True + self._warn( + f"Column {col.name!r} on {model.name!r} references " + f"unresolvable cross-model column(s) {bad}; dropping.", + model_name=model.name, category="missing_column", + ) return dropped def _drop_stale_joins(self) -> bool: """Drop joins whose key columns were removed by column validation, so a join never references a column that no longer exists.""" dropped = False - for model in list(self._models.values()): - for join in list(model.joins): - if self._join_columns_missing(model, join): - model.joins.remove(join) - dropped = True - self._warn( - f"Join from {model.name!r} to {join.target_model!r} " - f"references a column removed during validation; dropping.", - model_name=model.name, category="relationship", - ) + for model in self._models.values(): + stale = [j for j in model.joins if self._join_columns_missing(model, j)] + for join in stale: + model.joins.remove(join) + dropped = True + self._warn( + f"Join from {model.name!r} to {join.target_model!r} " + f"references a column removed during validation; dropping.", + model_name=model.name, category="relationship", + ) return dropped def _join_columns_missing(self, model: SlayerModel, join: ModelJoin) -> bool: diff --git a/tests/test_cli_import_osi.py b/tests/test_cli_import_osi.py index 9e0884a7..764a5d41 100644 --- a/tests/test_cli_import_osi.py +++ b/tests/test_cli_import_osi.py @@ -75,6 +75,14 @@ def test_import_osi_saves_models_and_prints_report(shop_setup, capsys) -> None: assert any(j.target_model == "customers" for j in orders.joins) +def test_import_osi_nonexistent_path_exits(tmp_path: Path) -> None: + store = tmp_path / "store" + YAMLStorage(base_dir=str(store)) + args = _args(store, tmp_path / "does_not_exist.yaml") + with pytest.raises(SystemExit): + _run_import_osi(args) + + def test_import_osi_missing_datasource_exits(tmp_path: Path) -> None: store = tmp_path / "empty_store" YAMLStorage(base_dir=str(store)) # no datasource registered From 4e216c3b33a76d10b4edcd8872a3e5bfbddc78f9 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:59:56 +0200 Subject: [PATCH 16/25] DEV-1643: derived field replacing a column inherits its known type 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) --- slayer/osi/converter.py | 14 ++++++++++---- tests/test_osi_converter.py | 2 ++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index fef747d6..7dd5566b 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -394,10 +394,16 @@ def _resolve_field_column(self, field: OSIField, sql: str, ) return None is_time = bool(field.dimension and field.dimension.is_time) - return Column( - name=field.name, sql=sql, - type=DataType.TIMESTAMP if is_time else DataType.DOUBLE, - ) + if is_time: + dtype = DataType.TIMESTAMP + elif field.name in by_name: + # A derived field that redefines an existing column inherits that + # column's known type (e.g. LOWER(status) stays TEXT) rather than + # defaulting a genuinely new derived column to DOUBLE. + dtype = by_name[field.name].type + else: + dtype = DataType.DOUBLE + return Column(name=field.name, sql=sql, type=dtype) def _apply_dataset_metadata(self, model: SlayerModel, ds: OSIDataset, sm: OSISemanticModel) -> None: diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 1d157944..6c161c8a 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -557,6 +557,8 @@ def test_derived_field_shadowing_physical_column_overlays(shop_engine): status_cols = [c for c in orders.columns if c.name == "status"] assert len(status_cols) == 1 assert status_cols[0].sql == "LOWER(status)" + # the derived field inherits the shadowed column's known TEXT type, not DOUBLE + assert status_cols[0].type == DataType.TEXT def test_derived_field_expression_unknown_column_clean_fails(shop_engine): From 9f986f1292ecf65d5a593081020e27fbd8564f7f Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 17:08:45 +0200 Subject: [PATCH 17/25] DEV-1643: preserve primary-key flag when a derived overlay replaces a 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) --- slayer/osi/converter.py | 4 ++++ tests/test_osi_converter.py | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 7dd5566b..32dea797 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -350,6 +350,10 @@ def _overlay_one_field(self, field: OSIField, model: SlayerModel, model.columns.append(col) by_name[col.name] = col elif existing is not col: + # Preserve the shadowed column's primary-key flag across the + # redefinition — an introspected PK must survive a derived overlay + # unless OSI's primary_key explicitly overrides it later. + col.primary_key = existing.primary_key model.columns[model.columns.index(existing)] = col by_name[col.name] = col return col.name if is_time else None diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 6c161c8a..ce1471cd 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -359,6 +359,19 @@ def test_invalid_osi_primary_key_keeps_introspected(shop_engine): assert _reported(result) +def test_derived_overlay_preserves_primary_key(shop_engine): + # A derived overlay on the PK column (OSI not restating primary_key) keeps + # the introspected PK flag. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="order_id", expression=_expr("order_id + 0"))])] + ) + result = _convert(shop_engine, doc) + order_id = {c.name: c for c in {m.name: m for m in result.models}["orders"].columns}["order_id"] + assert order_id.primary_key is True + assert order_id.sql == "order_id + 0" + + def test_osi_primary_key_overrides_introspected(shop_engine): # OSI primary_key is authoritative: it replaces the physical PK (order_id). doc = _mini_doc( From eeadd6d1dfbc4ac046ff7b2fec22d561ae7e6c10 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 17:17:46 +0200 Subject: [PATCH 18/25] DEV-1643: revert to the physical column when an overlay is invalidated 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) --- slayer/osi/converter.py | 33 ++++++++++++++++++++++++++------- tests/test_osi_converter.py | 16 ++++++++++++++++ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 32dea797..0ce4131f 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -160,6 +160,10 @@ def __init__( self._models: dict[str, SlayerModel] = {} self._warnings: list[ConversionWarning] = [] self._unconverted: list[ConversionWarning] = [] + # (model, column) -> the introspected Column a derived overlay replaced, + # so an invalidated overlay can be reverted to the physical column + # instead of deleting a real column. + self._shadowed: dict[tuple[str, str], Column] = {} # ---- report helpers ---- @@ -352,8 +356,11 @@ def _overlay_one_field(self, field: OSIField, model: SlayerModel, elif existing is not col: # Preserve the shadowed column's primary-key flag across the # redefinition — an introspected PK must survive a derived overlay - # unless OSI's primary_key explicitly overrides it later. + # unless OSI's primary_key explicitly overrides it later. Remember + # the original so an overlay later invalidated by cross-model + # validation can revert to the physical column (not delete it). col.primary_key = existing.primary_key + self._shadowed.setdefault((model.name, col.name), existing) model.columns[model.columns.index(existing)] = col by_name[col.name] = col return col.name if is_time else None @@ -509,13 +516,25 @@ def _drop_invalid_cross_model_columns(self) -> bool: if bad: bad_cols.append((col, bad)) for col, bad in bad_cols: - model.columns.remove(col) dropped = True - self._warn( - f"Column {col.name!r} on {model.name!r} references " - f"unresolvable cross-model column(s) {bad}; dropping.", - model_name=model.name, category="missing_column", - ) + original = self._shadowed.pop((model.name, col.name), None) + if original is not None: + # The invalid column was a derived overlay of a physical + # column — revert to the physical column, don't delete it. + model.columns[model.columns.index(col)] = original + self._warn( + f"Derived overlay for {col.name!r} on {model.name!r} " + f"references unresolvable cross-model column(s) {bad}; " + f"reverting to the physical column.", + model_name=model.name, category="missing_column", + ) + else: + model.columns.remove(col) + self._warn( + f"Column {col.name!r} on {model.name!r} references " + f"unresolvable cross-model column(s) {bad}; dropping.", + model_name=model.name, category="missing_column", + ) return dropped def _drop_stale_joins(self) -> bool: diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index ce1471cd..a78b5221 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -359,6 +359,22 @@ def test_invalid_osi_primary_key_keeps_introspected(shop_engine): assert _reported(result) +def test_invalid_derived_overlay_reverts_to_physical_column(shop_engine): + # An overlay on a physical column (order_id) with an unresolvable cross-model + # ref must revert to the physical column (keep it + PK), not delete it. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="order_id", + expression=_expr("customers.no_such_col"))])] + ) + result = _convert(shop_engine, doc) + cols = {c.name: c for c in {m.name: m for m in result.models}["orders"].columns} + assert "order_id" in cols + assert cols["order_id"].primary_key is True + assert cols["order_id"].sql != "customers.no_such_col" + assert _reported(result) + + def test_derived_overlay_preserves_primary_key(shop_engine): # A derived overlay on the PK column (OSI not restating primary_key) keeps # the introspected PK flag. From aceb1a07aacab96a92b481942f623abd653d9267 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 17:37:04 +0200 Subject: [PATCH 19/25] DEV-1643: public column-type API; reduce converter cognitive complexity - 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) --- slayer/osi/converter.py | 67 +++++++++++++++++++++++------------------ slayer/sql/client.py | 11 +++++++ 2 files changed, 49 insertions(+), 29 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 0ce4131f..01993a5f 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -26,7 +26,7 @@ from slayer.engine.ingestion import introspect_table_to_model from slayer.engine.join_graph import JoinGraph, min_hops_root from slayer.ingest_report import ConversionResult, ConversionWarning -from slayer.sql.client import _get_column_types_sync +from slayer.sql.client import get_column_types_sync from slayer.osi.expression import SQL_DIALECTS, convert_expression from slayer.osi.models import ( OSIAIContext, @@ -269,9 +269,8 @@ def _build_sql_mode_model(self, ds: OSIDataset, query: str) -> SlayerModel: # cursor-metadata probe), exactly as table sources are introspected. # ``target_dialect`` is the datasource type, used for the dialect-correct # probe (LIMIT 0 vs SELECT TOP vs SQLite's LIMIT-1 fallback). - types = _get_column_types_sync( - query, connection_string="", db_type=self.target_dialect, - engine=self.sa_engine, + types = get_column_types_sync( + query, engine=self.sa_engine, db_type=self.target_dialect, ) columns = [Column(name=name, type=category) for name, category in types.items()] return SlayerModel(name=ds.name, sql=query, data_source=self.data_source, @@ -509,34 +508,44 @@ def _validate_cross_model_field_refs(self) -> None: def _drop_invalid_cross_model_columns(self) -> bool: dropped = False for model in self._models.values(): - bad_cols = [] - for col in model.columns: - if col.sql: - bad = self._unresolvable_cross_model_refs(model, col.sql) - if bad: - bad_cols.append((col, bad)) - for col, bad in bad_cols: + for col, bad in self._invalid_cross_model_columns(model): + self._revert_or_drop_column(model, col, bad) dropped = True - original = self._shadowed.pop((model.name, col.name), None) - if original is not None: - # The invalid column was a derived overlay of a physical - # column — revert to the physical column, don't delete it. - model.columns[model.columns.index(col)] = original - self._warn( - f"Derived overlay for {col.name!r} on {model.name!r} " - f"references unresolvable cross-model column(s) {bad}; " - f"reverting to the physical column.", - model_name=model.name, category="missing_column", - ) - else: - model.columns.remove(col) - self._warn( - f"Column {col.name!r} on {model.name!r} references " - f"unresolvable cross-model column(s) {bad}; dropping.", - model_name=model.name, category="missing_column", - ) return dropped + def _invalid_cross_model_columns( + self, model: SlayerModel + ) -> list[tuple[Column, list[str]]]: + result = [] + for col in model.columns: + if col.sql: + bad = self._unresolvable_cross_model_refs(model, col.sql) + if bad: + result.append((col, bad)) + return result + + def _revert_or_drop_column( + self, model: SlayerModel, col: Column, bad: list[str] + ) -> None: + original = self._shadowed.pop((model.name, col.name), None) + if original is not None: + # The invalid column was a derived overlay of a physical column — + # revert to the physical column, don't delete it. + model.columns[model.columns.index(col)] = original + self._warn( + f"Derived overlay for {col.name!r} on {model.name!r} references " + f"unresolvable cross-model column(s) {bad}; reverting to the " + f"physical column.", + model_name=model.name, category="missing_column", + ) + else: + model.columns.remove(col) + self._warn( + f"Column {col.name!r} on {model.name!r} references unresolvable " + f"cross-model column(s) {bad}; dropping.", + model_name=model.name, category="missing_column", + ) + def _drop_stale_joins(self) -> bool: """Drop joins whose key columns were removed by column validation, so a join never references a column that no longer exists.""" diff --git a/slayer/sql/client.py b/slayer/sql/client.py index 40e5ddb1..4c795c66 100644 --- a/slayer/sql/client.py +++ b/slayer/sql/client.py @@ -435,6 +435,17 @@ def _get_column_types_sync( return _extract_types_from_cursor(result, db_type=db_type) +def get_column_types_sync( + sql: str, *, engine: sa.Engine, db_type: str | None = None +) -> dict[str, str]: + """Public sync column-type inference for a query over an existing engine. + + Stable entry point (e.g. for the OSI importer) around + ``_get_column_types_sync``; returns ``{column_name: type_category}``. + """ + return _get_column_types_sync(sql, connection_string="", db_type=db_type, engine=engine) + + async def _get_column_types_async( sql: str, engine, From 7bea588d995d2acf7343055de3b156ef097a7ace Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 17:58:27 +0200 Subject: [PATCH 20/25] DEV-1643: probe the real type of brand-new derived fields 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( 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) --- slayer/osi/converter.py | 36 +++++++++++++++++++++++++++++++----- tests/test_osi_converter.py | 22 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 01993a5f..29081711 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -330,7 +330,7 @@ def _overlay_one_field(self, field: OSIField, model: SlayerModel, ) return None - col = self._resolve_field_column(field, sql, by_name, introspected, ds) + col = self._resolve_field_column(field, sql, by_name, introspected, ds, model) if col is None: return None @@ -366,7 +366,7 @@ def _overlay_one_field(self, field: OSIField, model: SlayerModel, def _resolve_field_column(self, field: OSIField, sql: str, by_name: dict[str, Column], introspected: set[str], - ds: OSIDataset) -> Column | None: + ds: OSIDataset, model: SlayerModel) -> Column | None: """Return the Column (existing or new) this field maps to, or None on a clean-fail (already reported).""" bare = _as_bare_column(sql) @@ -408,13 +408,39 @@ def _resolve_field_column(self, field: OSIField, sql: str, dtype = DataType.TIMESTAMP elif field.name in by_name: # A derived field that redefines an existing column inherits that - # column's known type (e.g. LOWER(status) stays TEXT) rather than - # defaulting a genuinely new derived column to DOUBLE. + # column's known type (e.g. LOWER(status) stays TEXT). dtype = by_name[field.name].type else: - dtype = DataType.DOUBLE + # A genuinely new derived column: live-probe the expression's real + # type so a non-numeric expression (UPPER(x), concat, ...) isn't + # mis-cast as DOUBLE by the SQL generator. Fall back to DOUBLE if the + # probe is unavailable. + dtype = self._probe_expression_type(model, sql) or DataType.DOUBLE return Column(name=field.name, sql=sql, type=dtype) + def _probe_expression_type(self, model: SlayerModel, expr: str) -> DataType | None: + """Infer a derived expression's SLayer type by probing it against the + model's physical source (LIMIT-0 cursor metadata). Returns None if the + model has no probeable source or the probe fails.""" + if model.sql_table: + from_clause = model.sql_table + elif model.sql: + from_clause = f"({model.sql}) AS _osi_sub" + else: + return None + probe = f"SELECT ({expr}) AS _osi_probe FROM {from_clause}" + try: + types = get_column_types_sync( + probe, engine=self.sa_engine, db_type=self.target_dialect, + ) + except Exception: # noqa: BLE001 — probe is best-effort; fall back + return None + category = next(iter(types.values()), None) + if category is None: + return None + # Reuse Column's category->DataType coercion (as _build_sql_mode_model does). + return Column(name="_osi_probe", type=category).type + def _apply_dataset_metadata(self, model: SlayerModel, ds: OSIDataset, sm: OSISemanticModel) -> None: model.description = _render_description(ds.description, ds.ai_context) \ diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index a78b5221..195d1b47 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -556,6 +556,28 @@ def test_relationship_unknown_target_clean_fails(shop_engine): assert _reported(result) +def test_brand_new_derived_field_type_is_probed(shop_engine): + # A brand-new derived field (no matching physical column) gets its type from + # a live probe, so a text expression stays TEXT instead of being cast DOUBLE. + with shop_engine.connect() as conn: + conn.execute(sa.text( + "INSERT INTO orders (order_id, amount, quantity, status) " + "VALUES (1, 9.5, 3, 'paid')" + )) + conn.commit() + doc = _mini_doc( + datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="status_up", expression=_expr("UPPER(status)")), + OSIField(name="line", expression=_expr("quantity * amount"))], + )] + ) + result = _convert(shop_engine, doc, target_dialect="sqlite") + cols = {c.name: c for c in {m.name: m for m in result.models}["orders"].columns} + assert cols["status_up"].type == DataType.TEXT + assert cols["line"].type in (DataType.DOUBLE, DataType.INT) + + def test_quoted_bare_field_expression_overlays_not_derived(shop_engine): # A double-quoted identifier expression is a base-column reference, not a # derived expression — it must overlay the introspected column (keep its From 30175c5cccb1973189e2e59ea3a8d8e963023afb Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 18:06:20 +0200 Subject: [PATCH 21/25] DEV-1643: type single cross-model derived field refs from the target 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) --- slayer/osi/converter.py | 28 ++++++++++++++++++++++++++++ tests/test_osi_converter.py | 20 ++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 29081711..0b927af0 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -197,6 +197,7 @@ def convert(self) -> ConversionResult: self._build_join(rel) self._validate_cross_model_field_refs() + self._type_cross_model_columns() graph = JoinGraph.build_from_models(list(self._models.values())) for sm in semantic_models: @@ -588,6 +589,33 @@ def _drop_stale_joins(self) -> bool: ) return dropped + def _type_cross_model_columns(self) -> None: + """After joins exist, fix the type of a derived column that is exactly a + single cross-model column ref (e.g. ``customers.segment``) — the local + probe couldn't resolve it, so it was left as the DOUBLE fallback and + would mis-cast a joined text/temporal column at query time.""" + for model in self._models.values(): + for col in model.columns: + if col.sql: + target_type = self._single_cross_model_ref_type(model, col.sql) + if target_type is not None: + col.type = target_type + + def _single_cross_model_ref_type( + self, model: SlayerModel, sql: str + ) -> DataType | None: + try: + tree = sqlglot.parse_one(sql) + except sqlglot.errors.ParseError: + return None + if not isinstance(tree, exp.Column) or not tree.table or tree.table == model.name: + return None + target = self._walk_join_alias(model, tree.table) + if target is None: + return None + tcol = next((c for c in target.columns if c.name == tree.name), None) + return tcol.type if tcol is not None else None + def _join_columns_missing(self, model: SlayerModel, join: ModelJoin) -> bool: target = self._models.get(join.target_model) for src_col, tgt_col in join.join_pairs: diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 195d1b47..4bccdbcc 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -709,6 +709,26 @@ def test_valid_cross_model_derived_field_kept(shop_engine): assert "cust_seg" in {c.name for c in orders.columns} +def test_cross_model_derived_field_inherits_target_type(shop_engine): + # A derived field that is a single cross-model ref (customers.segment) takes + # the resolved target column's TEXT type, not the DOUBLE local-probe fallback. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="customer_id", expression=_expr("customer_id")), + OSIField(name="seg", expression=_expr("customers.segment"))]), + OSIDataset(name="customers", source="customers", + fields=[OSIField(name="customer_id", expression=_expr("customer_id"))]), + ], + relationships=[OSIRelationship( + name="o2c", **{"from": "orders"}, to="customers", + from_columns=["customer_id"], to_columns=["customer_id"])], + ) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert {c.name: c for c in orders.columns}["seg"].type == DataType.TEXT + + def test_cross_model_derived_field_unknown_column_dropped(shop_engine): doc = _mini_doc( datasets=[ From f7a5b8035066dd3183dc75beea799e5f84486a4f Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 18:27:16 +0200 Subject: [PATCH 22/25] DEV-1643: reset dropped time default; single-file suffix filter; kwargs - 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) --- slayer/osi/converter.py | 65 ++++++++++++++++++++----------------- slayer/osi/parser.py | 3 +- tests/test_osi_converter.py | 14 ++++++++ tests/test_osi_parser.py | 7 ++++ 4 files changed, 59 insertions(+), 30 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 0b927af0..1f9e87fc 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -191,7 +191,7 @@ def convert(self) -> ConversionResult: for sm in semantic_models: for ds in sm.datasets: - self._build_model(ds, sm, inspector) + self._build_model(ds=ds, sm=sm, inspector=inspector) for sm in semantic_models: for rel in sm.relationships or []: self._build_join(rel) @@ -212,7 +212,7 @@ def convert(self) -> ConversionResult: def _build_measures_for(self, sm: OSISemanticModel, graph: JoinGraph) -> None: sm_model_names = [d.name for d in sm.datasets if d.name in self._models] for metric in sm.metrics or []: - self._build_measure(metric, sm_model_names, graph) + self._build_measure(metric=metric, sm_model_names=sm_model_names, graph=graph) def _check_duplicate_dataset_names(self, sms: list[OSISemanticModel]) -> None: seen: set[str] = set() @@ -271,7 +271,7 @@ def _build_sql_mode_model(self, ds: OSIDataset, query: str) -> SlayerModel: # ``target_dialect`` is the datasource type, used for the dialect-correct # probe (LIMIT 0 vs SELECT TOP vs SQLite's LIMIT-1 fallback). types = get_column_types_sync( - query, engine=self.sa_engine, db_type=self.target_dialect, + sql=query, engine=self.sa_engine, db_type=self.target_dialect, ) columns = [Column(name=name, type=category) for name, category in types.items()] return SlayerModel(name=ds.name, sql=query, data_source=self.data_source, @@ -283,7 +283,7 @@ def _overlay_fields(self, model: SlayerModel, ds: OSIDataset) -> None: first_time_dim: str | None = None for field in ds.fields or []: - time_col = self._overlay_one_field(field, model, by_name, introspected, ds) + time_col = self._overlay_one_field(field=field, model=model, by_name=by_name, introspected=introspected, ds=ds) if time_col and first_time_dim is None: first_time_dim = time_col @@ -331,14 +331,14 @@ def _overlay_one_field(self, field: OSIField, model: SlayerModel, ) return None - col = self._resolve_field_column(field, sql, by_name, introspected, ds, model) + col = self._resolve_field_column(field=field, sql=sql, by_name=by_name, introspected=introspected, ds=ds, model=model) if col is None: return None col.label = field.label or col.label - col.description = _render_description(field.description, field.ai_context) \ + col.description = _render_description(explicit=field.description, ctx=field.ai_context) \ or col.description - meta = _build_meta(field.ai_context, field.custom_extensions) + meta = _build_meta(ctx=field.ai_context, custom_extensions=field.custom_extensions) if meta: col.meta = {**(col.meta or {}), **meta} @@ -389,7 +389,7 @@ def _resolve_field_column(self, field: OSIField, sql: str, # (consistent with the bare-field / metric / relationship checks); a # collision with an existing column is handled by the replace-or-append # logic in _overlay_one_field. - missing = _missing_expr_columns(sql, introspected, ds.name) + missing = _missing_expr_columns(sql=sql, available=introspected, self_name=ds.name) if missing is None: self._warn( f"Field {field.name!r} on {ds.name!r} has an unparseable " @@ -416,7 +416,7 @@ def _resolve_field_column(self, field: OSIField, sql: str, # type so a non-numeric expression (UPPER(x), concat, ...) isn't # mis-cast as DOUBLE by the SQL generator. Fall back to DOUBLE if the # probe is unavailable. - dtype = self._probe_expression_type(model, sql) or DataType.DOUBLE + dtype = self._probe_expression_type(model=model, expr=sql) or DataType.DOUBLE return Column(name=field.name, sql=sql, type=dtype) def _probe_expression_type(self, model: SlayerModel, expr: str) -> DataType | None: @@ -432,7 +432,7 @@ def _probe_expression_type(self, model: SlayerModel, expr: str) -> DataType | No probe = f"SELECT ({expr}) AS _osi_probe FROM {from_clause}" try: types = get_column_types_sync( - probe, engine=self.sa_engine, db_type=self.target_dialect, + sql=probe, engine=self.sa_engine, db_type=self.target_dialect, ) except Exception: # noqa: BLE001 — probe is best-effort; fall back return None @@ -444,7 +444,7 @@ def _probe_expression_type(self, model: SlayerModel, expr: str) -> DataType | No def _apply_dataset_metadata(self, model: SlayerModel, ds: OSIDataset, sm: OSISemanticModel) -> None: - model.description = _render_description(ds.description, ds.ai_context) \ + model.description = _render_description(explicit=ds.description, ctx=ds.ai_context) \ or model.description extra: dict[str, Any] = {} if ds.unique_keys: @@ -456,7 +456,7 @@ def _apply_dataset_metadata(self, model: SlayerModel, ds: OSIDataset, "description": sm.description, "ai_context": sm_ctx, } - meta = _build_meta(ds.ai_context, ds.custom_extensions, extra=extra) + meta = _build_meta(ctx=ds.ai_context, custom_extensions=ds.custom_extensions, extra=extra) if meta: model.meta = {**(model.meta or {}), **meta} @@ -512,8 +512,8 @@ def _build_join(self, rel: OSIRelationship) -> None: self._models[src].joins.append(ModelJoin( target_model=rel.to, join_pairs=pairs, - description=_render_description(None, rel.ai_context), - meta=_build_meta(rel.ai_context, rel.custom_extensions), + description=_render_description(explicit=None, ctx=rel.ai_context), + meta=_build_meta(ctx=rel.ai_context, custom_extensions=rel.custom_extensions), )) def _validate_cross_model_field_refs(self) -> None: @@ -536,7 +536,7 @@ def _drop_invalid_cross_model_columns(self) -> bool: dropped = False for model in self._models.values(): for col, bad in self._invalid_cross_model_columns(model): - self._revert_or_drop_column(model, col, bad) + self._revert_or_drop_column(model=model, col=col, bad=bad) dropped = True return dropped @@ -546,7 +546,7 @@ def _invalid_cross_model_columns( result = [] for col in model.columns: if col.sql: - bad = self._unresolvable_cross_model_refs(model, col.sql) + bad = self._unresolvable_cross_model_refs(model=model, sql=col.sql) if bad: result.append((col, bad)) return result @@ -567,6 +567,13 @@ def _revert_or_drop_column( ) else: model.columns.remove(col) + if model.default_time_dimension == col.name: + # Don't leave the default pointing at a dropped column. + model.default_time_dimension = next( + (c.name for c in model.columns + if c.type in (DataType.DATE, DataType.TIMESTAMP)), + None, + ) self._warn( f"Column {col.name!r} on {model.name!r} references unresolvable " f"cross-model column(s) {bad}; dropping.", @@ -578,7 +585,7 @@ def _drop_stale_joins(self) -> bool: join never references a column that no longer exists.""" dropped = False for model in self._models.values(): - stale = [j for j in model.joins if self._join_columns_missing(model, j)] + stale = [j for j in model.joins if self._join_columns_missing(model=model, join=j)] for join in stale: model.joins.remove(join) dropped = True @@ -597,7 +604,7 @@ def _type_cross_model_columns(self) -> None: for model in self._models.values(): for col in model.columns: if col.sql: - target_type = self._single_cross_model_ref_type(model, col.sql) + target_type = self._single_cross_model_ref_type(model=model, sql=col.sql) if target_type is not None: col.type = target_type @@ -610,7 +617,7 @@ def _single_cross_model_ref_type( return None if not isinstance(tree, exp.Column) or not tree.table or tree.table == model.name: return None - target = self._walk_join_alias(model, tree.table) + target = self._walk_join_alias(host=model, alias=tree.table) if target is None: return None tcol = next((c for c in target.columns if c.name == tree.name), None) @@ -619,7 +626,7 @@ def _single_cross_model_ref_type( def _join_columns_missing(self, model: SlayerModel, join: ModelJoin) -> bool: target = self._models.get(join.target_model) for src_col, tgt_col in join.join_pairs: - if not self._model_has_column(model.name, src_col): + if not self._model_has_column(model_name=model.name, column=src_col): return True if target is None or not any(c.name == tgt_col for c in target.columns): return True @@ -644,7 +651,7 @@ def _unresolvable_cross_model_refs(self, model: SlayerModel, sql: str) -> list[s continue # catalog/db-qualified physical ref — outside SLayer's contract if not col.table or col.table == model.name: continue - target = self._walk_join_alias(model, col.table) + target = self._walk_join_alias(host=model, alias=col.table) if target is None or not any(c.name == col.name for c in target.columns): bad.append(f"{col.table}.{col.name}") return bad @@ -681,9 +688,9 @@ def _missing_join_columns(self, rel: OSIRelationship) -> list[str]: """Qualified names of relationship join columns absent from their model, so a typo clean-fails instead of emitting a broken join.""" missing = [f"{rel.from_dataset}.{c}" for c in rel.from_columns - if not self._model_has_column(rel.from_dataset, c)] + if not self._model_has_column(model_name=rel.from_dataset, column=c)] missing += [f"{rel.to}.{c}" for c in rel.to_columns - if not self._model_has_column(rel.to, c)] + if not self._model_has_column(model_name=rel.to, column=c)] return missing # ---- metrics -> measures ---- @@ -706,7 +713,7 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], return owner_of = self._make_owner_of(sm_model_names) - anchor = self._select_anchor(metric.name, expr, sm_model_names, owner_of, graph) + anchor = self._select_anchor(metric_name=metric.name, expr=expr, sm_model_names=sm_model_names, owner_of=owner_of, graph=graph) if anchor is None: return # already reported @@ -721,7 +728,7 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], metric_name=metric.name, category="duplicate_measure", ) return - if self._model_has_column(anchor, metric.name): + if self._model_has_column(model_name=anchor, column=metric.name): self._unconv( f"Metric {metric.name!r} collides with a column name on model " f"{anchor!r}.", @@ -729,7 +736,7 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], ) return - ref_of = self._make_ref_of(graph, anchor) + ref_of = self._make_ref_of(graph=graph, anchor=anchor) percentile_unsupported = ( self.target_dialect is not None and self.target_dialect.lower() in _NO_PERCENTILE_DIALECTS @@ -764,8 +771,8 @@ def _build_measure(self, metric: OSIMetric, sm_model_names: list[str], measure = ModelMeasure( formula=result.formula, name=metric.name, - description=_render_description(metric.description, metric.ai_context), - meta=_build_meta(metric.ai_context, metric.custom_extensions), + description=_render_description(explicit=metric.description, ctx=metric.ai_context), + meta=_build_meta(ctx=metric.ai_context, custom_extensions=metric.custom_extensions), ) except Exception as exc: # noqa: BLE001 — any validation error -> report self._unconv( @@ -785,7 +792,7 @@ def _select_anchor(self, metric_name: str, expr: str, sm_model_names: list[str], owner_of, graph: JoinGraph) -> str | None: owners = self._referenced_owners(expr, owner_of) if owners: - anchor = min_hops_root(graph, sm_model_names, owners) + anchor = min_hops_root(graph=graph, candidates=sm_model_names, mentioned=owners) if anchor is None: self._unconv( f"Metric {metric_name!r} references models {sorted(owners)} " diff --git a/slayer/osi/parser.py b/slayer/osi/parser.py index f2a05661..75f1a448 100644 --- a/slayer/osi/parser.py +++ b/slayer/osi/parser.py @@ -27,7 +27,8 @@ def _collect_files(path: Path) -> list[Path]: if path.is_file(): - return [path] + # Apply the same suffix policy as directory scanning. + return [path] if path.name.endswith(_SUFFIXES) else [] files: list[Path] = [] for root, dirs, names in os.walk(path): dirs[:] = [d for d in dirs if not d.startswith(".")] diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 4bccdbcc..29c24242 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -748,6 +748,20 @@ def test_cross_model_derived_field_unknown_column_dropped(shop_engine): assert _reported(result) +def test_dropped_time_default_dimension_is_reset(shop_engine): + # If the model's default_time_dimension is a cross-model time field that gets + # dropped, the default must not keep pointing at the removed column. + doc = _mini_doc(datasets=[OSIDataset( + name="orders", source="orders", + fields=[OSIField(name="ts", expression=_expr("customers.no_such_ts"), + dimension={"is_time": True})], + )]) + result = _convert(shop_engine, doc) + orders = {m.name: m for m in result.models}["orders"] + assert "ts" not in {c.name for c in orders.columns} + assert orders.default_time_dimension != "ts" + + def test_cross_model_derived_field_no_join_path_dropped(shop_engine): # products.category referenced from orders with NO relationship -> dropped. doc = _mini_doc( diff --git a/tests/test_osi_parser.py b/tests/test_osi_parser.py index d7b0a9de..c8b5476f 100644 --- a/tests/test_osi_parser.py +++ b/tests/test_osi_parser.py @@ -102,6 +102,13 @@ def test_unknown_version_warns_but_parses(caplog: pytest.LogCaptureFixture) -> N assert any("version" in r.message.lower() for r in caplog.records) +def test_single_non_osi_file_is_skipped(tmp_path: Path) -> None: + # A single file with an unsupported suffix is skipped, same as in a directory. + f = tmp_path / "notes.txt" + f.write_text("hello: world") + assert parse_osi_path(f) == [] + + def test_malformed_file_warns_and_returns_empty(caplog: pytest.LogCaptureFixture) -> None: with caplog.at_level(logging.WARNING): docs = parse_osi_path(FIXTURES / "malformed.yaml") From c26d112bc8a96d6d920bc5a63a6c281f3490d3e7 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 18:35:34 +0200 Subject: [PATCH 23/25] DEV-1643: reject unsafe OSI dataset names (path-traversal hardening) A model name becomes a filename in YAML storage (/.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) --- slayer/osi/converter.py | 15 ++++++++++++--- tests/test_osi_converter.py | 17 +++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 1f9e87fc..66739cbe 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -53,8 +53,16 @@ class OsiConversionError(Exception): """Raised when an OSI import set cannot be converted (e.g. duplicate names).""" +# Characters/shapes that are unsafe in a SLayer model name — notably path +# separators and NUL, since a model name becomes a filename in YAML storage +# (``/.yaml``); an absolute/traversal name would escape the tree. +_UNSAFE_MODEL_NAME_CHARS = ("__", ".", ":", "/", "\\", "\x00") + + def _legal_model_name(name: str) -> bool: - return "__" not in name and "." not in name and ":" not in name + if not name or name.strip() != name or not name.strip(): + return False + return not any(ch in name for ch in _UNSAFE_MODEL_NAME_CHARS) def _legal_column_name(name: str) -> bool: @@ -235,8 +243,9 @@ def _build_model(self, ds: OSIDataset, sm: OSISemanticModel, inspector: sa.engine.Inspector) -> None: if not _legal_model_name(ds.name): self._warn( - f"Dataset name {ds.name!r} contains characters SLayer forbids in " - f"model names ('__', '.', ':'); skipping.", + f"Dataset name {ds.name!r} is not a safe SLayer model name " + f"(empty, surrounding whitespace, or a forbidden character such " + f"as '__', '.', ':', '/', '\\\\', NUL); skipping.", model_name=ds.name, category="illegal_name", ) return diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index 29c24242..a4e218db 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -222,6 +222,23 @@ def test_illegal_dataset_name_clean_fails(shop_engine): assert _reported(result) # a report entry exists +def test_dataset_name_with_path_separator_clean_fails(shop_engine): + # A model name becomes a filename in YAML storage; path separators must be + # rejected so an OSI config can't write files outside the storage tree. + doc = _mini_doc( + datasets=[ + OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + OSIDataset(name="/tmp/owned", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))]), + ] + ) + result = _convert(shop_engine, doc) + names = {m.name for m in result.models} + assert "orders" in names and "/tmp/owned" not in names + assert _reported(result) + + def test_illegal_field_name_clean_fails_field_not_model(shop_engine): doc = _mini_doc( datasets=[OSIDataset( From 56619f77878b881d0a92dc311b219ce0c9973edb Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 18:51:09 +0200 Subject: [PATCH 24/25] DEV-1643: avoid /tmp literal in path-safety test (Sonar S5443) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- tests/test_osi_converter.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index a4e218db..deeae092 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -229,13 +229,13 @@ def test_dataset_name_with_path_separator_clean_fails(shop_engine): datasets=[ OSIDataset(name="orders", source="orders", fields=[OSIField(name="amount", expression=_expr("amount"))]), - OSIDataset(name="/tmp/owned", source="orders", + OSIDataset(name="evil/name", source="orders", fields=[OSIField(name="amount", expression=_expr("amount"))]), ] ) result = _convert(shop_engine, doc) names = {m.name for m in result.models} - assert "orders" in names and "/tmp/owned" not in names + assert "orders" in names and "evil/name" not in names assert _reported(result) From c95f6dd9fddd937b755f6e972202ab6a50bdc868 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 19:00:27 +0200 Subject: [PATCH 25/25] DEV-1643: normalize Snowflake/Databricks expressions to default SQL 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) --- slayer/osi/converter.py | 32 +++++++++++++++++++++++++++----- tests/test_osi_converter.py | 14 ++++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/slayer/osi/converter.py b/slayer/osi/converter.py index 66739cbe..a22f5940 100644 --- a/slayer/osi/converter.py +++ b/slayer/osi/converter.py @@ -48,6 +48,10 @@ # converter's caveat set). _NO_PERCENTILE_DIALECTS = frozenset({"mysql", "tsql", "mssql", "sqlserver"}) +# OSI SQL dialect -> sqlglot read dialect for normalizing an expression to +# default SQL. ANSI_SQL maps to None (already default-compatible). +_SQLGLOT_DIALECT = {"SNOWFLAKE": "snowflake", "DATABRICKS": "databricks"} + class OsiConversionError(Exception): """Raised when an OSI import set cannot be converted (e.g. duplicate names).""" @@ -899,9 +903,27 @@ def _resolve_expression(self, osi_expr: OSIExpression) -> str | None: into the SQL conversion path; it falls back to an available SQL dialect. """ by_dialect = {de.dialect.value: de.expression for de in osi_expr.dialects} + resolved = None if self.dialect in by_dialect and self.dialect in SQL_DIALECTS: - return by_dialect[self.dialect] - for name, expression in by_dialect.items(): - if name in SQL_DIALECTS: - return expression - return None + resolved = self.dialect + else: + resolved = next((n for n in by_dialect if n in SQL_DIALECTS), None) + if resolved is None: + return None + return self._normalize_sql(raw=by_dialect[resolved], dialect_name=resolved) + + @staticmethod + def _normalize_sql(raw: str, dialect_name: str) -> str: + """Normalize a dialect-specific expression to default-dialect SQL so all + downstream sqlglot parsing (which uses the default dialect) succeeds — a + Databricks/Snowflake expression like ``SUM(`amount`)`` would otherwise + fail to parse as default SQL. ANSI_SQL is already default-compatible and + is returned verbatim; an unparseable expression is returned as-is so the + downstream parse clean-fails with a clear reason.""" + source = _SQLGLOT_DIALECT.get(dialect_name) + if source is None: + return raw + try: + return sqlglot.parse_one(raw, read=source).sql() + except sqlglot.errors.ParseError: + return raw diff --git a/tests/test_osi_converter.py b/tests/test_osi_converter.py index deeae092..2610463f 100644 --- a/tests/test_osi_converter.py +++ b/tests/test_osi_converter.py @@ -1035,6 +1035,20 @@ def test_requested_non_sql_dialect_falls_back_to_sql(shop_engine): assert {m.name: m for m in orders.measures}["tot"].formula == "amount:sum" +def test_databricks_dialect_expression_normalized(shop_engine): + # A Databricks expression with backtick identifiers must be normalized to + # default SQL so it parses and converts, not clean-fail. + doc = _mini_doc( + datasets=[OSIDataset(name="orders", source="orders", + fields=[OSIField(name="amount", expression=_expr("amount"))])], + metrics=[OSIMetric(name="tot", expression=OSIExpression(dialects=[ + OSIDialectExpression(dialect="DATABRICKS", expression="SUM(`amount`)")]))], + ) + result = _convert(shop_engine, doc, dialect="DATABRICKS") + orders = {m.name: m for m in result.models}["orders"] + assert {m.name: m for m in orders.measures}["tot"].formula == "amount:sum" + + def test_target_dialect_percentile_caveat(shop_engine): doc = _mini_doc( datasets=[OSIDataset(name="orders", source="orders",