From 704b09fa7a80e75b80c16f2323b91134c0238b63 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 7 Aug 2026 11:41:04 +0200 Subject: [PATCH 1/4] fix: ingest resolves a schema scope and qualifies non-default schemas duckdb_engine's get_table_names(schema=None) returns objects from every schema as bare names -- every other Tier-1 dialect restricts it to the connection's default -- so _build_one_model wrote an unqualified sql_table for a non-default-schema object and the generator emitted `FROM reports`, which fails table-not-found. The same schema-blindness made _get_columns_fallback union two same-named tables' columns together. Ingest now resolves an explicit schema scope: --schema a,b / --all-schemas (schemas / all_schemas on the Python, REST and MCP surfaces), else datasource.schema_name, else the connection default. Multi-schema is opt-in because it changes what sql_table holds; when one schema is scanned and others exist, the run prints which and exits 0. Two different strings, easy to conflate: * the discovery token is carried exactly as get_schema_names() yields it (catalog-qualified on DuckDB), because the qualified form is the safe one -- with a catalog ATTACHed, a bare `main` makes get_table_names and has_table reach into it and makes the column fallback return the cross-catalog union. is_default therefore compares tokens in full. * the emitted qualifier is the bare last segment; the connection's current catalog is already correct. Both INFORMATION_SCHEMA fallbacks now filter on table_catalog as well as table_schema. table_schema alone holds the bare name, so a qualified token matched nothing: silently column-less models from the column fallback, and -- on DuckDB, where the Inspector reports no PK even for a declared PRIMARY KEY, so the fallback is the path that runs -- every primary key dropped. Only non-default schemas are qualified, so widening the scan never rewrites models already on disk; a single explicitly-named schema is written verbatim, preserving --schema public -> public.orders. Re-ingest heals a MISSING qualifier (participating in the short-circuit and the save gate, like source_kind) but never rewrites one, and two schemas' same-named tables are never fused into one model -- including the case a schema comparison alone cannot see, where the persisted model is unqualified because it IS the default schema's table. Collisions resolve in one phase over final model names with a 4-key total order, so the mixed sanitize/cross-schema case is defined and the outcome never depends on inspector listing order. validate-models derives its schema set from the models being validated and keys the live map on the full . identity, with shorter aliases inserted only when unique -- an ambiguous alias is dropped so a lookup misses rather than resolving to another catalog's same-named table. That is a data-loss path: an unresolvable model is a WholeModelDelete that --force-clean acts on. One dotted-name splitter replaces three disagreeing parsers, so hand-written Snowflake db.schema.table and BigQuery project.dataset.table stop losing their catalog. No new model field and no migration -- the schema lives in sql_table, which is where the generator already reads it. Closes DEV-1758 Co-Authored-By: Claude Opus 5 (1M context) --- .claude/skills/slayer-models.md | 9 + .claude/skills/slayer-overview.md | 2 +- DECISIONS.md | 1 + docs/concepts/ingestion.md | 39 +- docs/concepts/models.md | 12 + docs/configuration/datasources.md | 18 + docs/reference/cli.md | 45 +- slayer/api/server.py | 21 +- slayer/cli.py | 64 +- slayer/engine/ingestion.py | 877 +++++++-- slayer/engine/introspect_utils.py | 244 ++- slayer/engine/schema_drift.py | 133 +- slayer/mcp/server.py | 74 +- slayer/storage/type_refinement.py | 22 +- tests/test_ingestion.py | 14 +- tests/test_ingestion_schema_qualification.py | 1810 ++++++++++++++++++ 16 files changed, 3132 insertions(+), 253 deletions(-) create mode 100644 tests/test_ingestion_schema_qualification.py diff --git a/.claude/skills/slayer-models.md b/.claude/skills/slayer-models.md index 2031438d..8172daeb 100644 --- a/.claude/skills/slayer-models.md +++ b/.claude/skills/slayer-models.md @@ -88,6 +88,15 @@ Model names cannot contain `__` (reserved for join-path aliases), but `sql_table` can. Ingestion sanitizes only the name: object `reports__patient__drug` → model `reports_patient_drug`, `sql_table` unchanged. +`sql_table` is emitted verbatim into the generated SQL, so anything outside the +connection's default schema **must** be schema-qualified (`analytics.orders`) +or the query fails with table-not-found. Ingestion qualifies exactly when the +object's schema differs from the default, or when the schema was named +explicitly — default-schema objects stay bare, so both forms coexist in one +datasource by design. The schema is everything before the final dot, so +`project.dataset.table` works. A missing qualifier is repaired by re-ingesting +that schema; an existing one is never rewritten. + ## Query-backed models `create_model_from_query(query, name, variables=None)` saves a query (or list of stages) as a query-backed model. It populates `model.source_queries`, optional `model.query_variables` defaults, and caches `model.columns` + `model.backing_query_sql` from a save-time dry-run (unresolved `{var}` placeholders default to `'0'`). diff --git a/.claude/skills/slayer-overview.md b/.claude/skills/slayer-overview.md index 353955ab..da6db4b4 100644 --- a/.claude/skills/slayer-overview.md +++ b/.claude/skills/slayer-overview.md @@ -12,7 +12,7 @@ SLayer is a lightweight, agent-first semantic layer. Instead of writing raw SQL, - **SQLGenerator** — takes an EnrichedQuery (not SlayerQuery) and converts it to SQL via sqlglot (dialect-aware: postgres, mysql, bigquery, etc.) - **SlayerSQLClient** — executes SQL via SQLAlchemy with retry logic and statement timeouts - **Storage** — YAML or SQLite backends for model and datasource configs -- **Ingestion** — auto-generates models from DB schema with rollup-style FK joins (denormalized LEFT JOINs). It can be triggered manually (`slayer ingest`, `ingest_datasource_models`, `POST /ingest`) or **on every server boot** via `slayer serve --ingest-on-startup` / `slayer mcp --ingest-on-startup` (also `SLAYER_INGEST_ON_STARTUP=1`, or `create_app/create_mcp_server(ingest_on_startup=True)` programmatically). It is idempotent and continues on per-datasource failures. +- **Ingestion** — auto-generates models from DB schema with rollup-style FK joins (denormalized LEFT JOINs). It can be triggered manually (`slayer ingest`, `ingest_datasource_models`, `POST /ingest`) or **on every server boot** via `slayer serve --ingest-on-startup` / `slayer mcp --ingest-on-startup` (also `SLAYER_INGEST_ON_STARTUP=1`, or `create_app/create_mcp_server(ingest_on_startup=True)` programmatically). It is idempotent and continues on per-datasource failures. One pass covers **one schema** — `--schema a,b` / `--all-schemas` (and the `schemas` / `all_schemas` equivalents on MCP, REST and the Python API) opt into more; precedence is explicit flag → `datasource.schema_name` → the connection default. Non-default-schema objects get a schema-qualified `sql_table`. - **Interfaces** — MCP server (stdio via `slayer mcp`, SSE via `slayer serve` at `/mcp/sse`), REST API (FastAPI on port 5143), Python SDK, and two read-only wire-protocol facades for BI tools: Arrow Flight SQL (`slayer flight-serve`, port 5144) and Postgres (`slayer pg-serve`, port 5145; the connection `database` selects the SLayer datasource) ## Key Models diff --git a/DECISIONS.md b/DECISIONS.md index c5b1a4f0..67f149bc 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -70,3 +70,4 @@ implementation detail. Include issue refs when known. - 2026-08-04 — Dialect-aware / complete escaping for Mode-A `{variable}` substitution (DEV-1727), hardening DEV-1625. `substitute_variables(..., escape="sql")` is now **dialect-aware** and **fail-closed**: it gained a required keyword-only `backslash_escapes` signal (`bool | None`, raises if `None` in sql mode) so a caller rendering raw SQL can never silently under-escape. On backslash-escaping dialects (MySQL/ClickHouse/Snowflake/Redshift/BigQuery/Databricks/Spark) it doubles the backslash before escaping the single quote; on standard dialects it keeps the `''` quote-doubling. The double quote is deliberately left untouched — inside a single-quoted literal `\"` is NOT a recognised escape on 6 of the 7 backslash dialects (only MySQL), so escaping it would corrupt the value. The regime is DERIVED from sqlglot's own tokenizer via `SqlDialect.backslash_escapes_strings` (= `"\\" in tokenizer.STRING_ESCAPES`, guarded + 14-dialect pinned) so our escaping can never drift from the parser that reads the substituted SQL. `escape="python"` (Mode-B) additionally encodes the full C0 control range (`\t`/`\n`/`\r` named, rest `\xNN`) so raw newlines/NUL no longer break `ast.parse`. Engine fail-closed: `_substitute_model_sql_surfaces` / `_render_probe_model` require a `dialect`, threaded from the resolved datasource — no bare bool to forget. Assumes MySQL's default `sql_mode` (backslash escapes on); `NO_BACKSLASH_ESCAPES` servers are a sqlglot-layer-wide limitation, documented not fixed. The SQLite backslash end-to-end gap stays a pinned strict-xfail (pre-existing, out of scope). Bound parameters rejected (don't fit substitute-into-raw-SQL). Nested/join/cross-model lineages remain DEV-1678. - 2026-08-04 — Declared list-valued `{variable}` coercion (DEV-1730 follow-up): a scalar supplied for a variable the model declares `list_valued` is wrapped into a one-element list before Mode-A substitution, so an importer-generated `col IN ({var})` renders `IN ('US')` rather than the unquoted `IN (US)`. The generic scalar rule (author writes the quotes, so `{var}` also works in numeric/fragment positions like `amount >= {floor}` and `{d}::TIMESTAMP`) is CORRECT and unchanged — it just presumes an author who can see the SQL position, which a machine-generated fixed template does not have; the caller cannot supply per-element quotes through parentheses the importer wrote. Silent-wrong-answer risk drove the fix over a raise: `region IN (US)` parses as a column reference, so it fails at the database with a confusing message, or resolves against a real column and returns wrong rows. Opt-in is a front-end-NEUTRAL flag: the Cube converter writes `list_valued: ref.kind == "string"` into each `meta.cube_variables` entry (arrow forms splice pre-quoted scalars and stay `False`), and the engine reads only that flag — never Cube's `kind` taxonomy — so a future list-shaped front-end opts in the same way. Coercion lives at the single Mode-A choke point `_substitute_model_sql_surfaces` (execution and the `_render_probe_model` type-probe both route through it, so it cannot be bypassed) via `coerce_declared_list_variables` / `list_valued_variable_names` in `slayer/core/query.py`. Scope is deliberately narrow: only `str`/`int`/`float`/`bool` are wrapped; `list`/`tuple` pass through (the **empty list still raises** — "no filter" belongs to an optional block or a sentinel default); `None`/`dict` are left for `_render_variable_value` to reject with its own naming error; hand-written models declare nothing and are untouched. Follow-on from the same review: `declares_variables(model)` (any non-empty `meta.cube_variables`) now also defeats the DEV-1625 zero-variable fast path, via the shared `_model_needs_substitution_pass` predicate used by both `_substitute_model_sql_surfaces` and `_render_probe_model`. This closes the fast-path hole for a GENERATED model whose pushdowns are all required (no `{? ?}` block to force the pass): such a model used to emit a bare `{var}` into the SQL on a zero-variable call instead of raising the documented missing-variable error. The hole stays open — deliberately — for hand-written models, which declare nothing and keep the raw-brace-literal protection (`'{1,2,3}'`). The `list_valued` flag is matched with `is True`, not truthiness, since `meta` is user-extensible and a stray `1` or the string `"false"` must not switch substitution semantics. The bag is also SELF-IDENTIFYING — an entry counts only with a string `member` (the shape every importer writes) — so a hand-written `meta` that reuses the `cube_variables` key is not mistaken for generated SQL and silently stripped of its brace-literal protection. - 2026-08-05 — Ingestion sees views, survives unmodellable names, and stops being silent (DEV-1741). **Views**: `list_ingestable_objects` replaces the bare `get_table_names()` at every introspection site, adding `get_view_names` + `get_materialized_view_names` behind a `NotImplementedError`/`Exception` guard (the base `Inspector` RAISES for matviews on unsupporting dialects rather than returning `[]`), de-duplicated first-classification-wins because some dialects return views from `get_table_names()`, in a deterministic tables→views→matviews order that the name-collision policy depends on. Ingested by default — dbt materializes staging models as views, so opt-in would have left the reported failure in place for a fresh install — with `--no-views` on both `slayer ingest` and `datasources create --ingest`. The drift side (`_live_schema_for_datasource`) and the MCP listing (`_fetch_tables`) take **no flag and are unconditional**: that map is only ever a lookup target (`validate_datasource` iterates the *persisted* models, `available_in_ds` derives from them), so views there cannot manufacture a model or a drift entry — what they fix is a pre-existing **data-loss** bug where a hand-authored model whose `sql_table` named a view resolved to `live_table=None` → `WholeModelDelete` → deleted by `validate-models --force-clean`. Gating that on `--no-views` would re-arm it for exactly the users who opted out. **Names**: model names can't contain `__` (six modules — generator/enrichment/column_expansion/column_dependency/schema_drift/osi — `split("__")` an alias back into a join path, so a model named `a__b` is read as the alias for `a→b` and yields a silently wrong query, not a crash), but `sql_table` can, so a dlt child table `reports__patient__drug` is modelled as `reports_patient_drug` with `sql_table` verbatim. Sanitizer is `re.sub(r"_{2,}", "_")`, NOT `replace("__","_")` — `str.replace` is non-overlapping, so `a___b`→`a__b` would still fail validation. Collisions reserve every unsanitized name first (a real `a_b` always beats a sanitized `a__b`, order-independently) and **skip** rather than suffix, since suffixes shift as the object set changes and would orphan models and churn drift. A per-object `try/except` backstops everything else (`.`/`:`/`/`/`\` names, bad column names, per-object introspection failures); the FK-collection loop and `_get_fk_relationships` are guarded too because they run BEFORE that isolation and would otherwise still kill the run. **Reporting**: skips travel in a new `skipped` list, deliberately NOT folded into `errors` — `slayer ingest` exits 1 on either (we declined a perfectly valid object; `--exclude` is the documented remedy) but `POST /ingest` keeps 422 for `errors` only, because a permanent 422 aimed at a machine that can't act on the hint buries a successful partial ingest behind an error status. An empty scan prints the available schemas and exits 1 (the reporter listed the missing exit code as part of the defect), gated on `objects` not `additions` so a healthy no-op re-ingest stays quiet; `datasources create --ingest` on an empty DB still exits 0, since creating the datasource is that command's job and it succeeded. `in_scope_table_names` switched from model names to `_bare_table_name(sql_table)` — it is compared against table names in `_scoped_models_for_validation`, so the old keying silently dropped from validation scope any model whose name differs from its table (every sanitized model, and already every dbt/OSI hidden model passing `model_name=`). **`source_kind`** (`table`/`view`/`materialized_view`/`None`=unknown) persists on `SlayerModel`, v7→v8 with a no-op migration (mandatory: `migrate()` raises `RuntimeError` on an unregistered step); `None` for pre-v8, hand-authored and sql/query-backed models is the honest value, not a guess. It is a deliberate **exception to the additive-merge contract** — refreshed, not preserved — because it describes the live object rather than user intent, and the transition it exists to capture (dbt `+materialized: table`) usually changes no columns at all; the refresh therefore has to be made in three places (the early return, the `model_copy(update=...)`, and the save gate in `_process_one_table`), since editing only the update dict computes a corrected model and throws it away. A `None` from a non-classifying path never erases a known value. Docs-only fix for the advertised `motley-slayer[duckdb]` extra, which does not exist: `duckdb`/`duckdb-engine` are unconditional core deps (the Postgres facade imports duckdb at top level on every CLI invocation), and adding them under `[tool.poetry.extras]` would *gate* rather than alias them, breaking bare `pip install motley-slayer` → `datasources create demo`. +- 2026-08-07 — Ingest resolves a schema scope, and non-default schemas are written into `sql_table` (DEV-1758). **The bug**: `duckdb_engine`'s `get_table_names(schema=None)` returns objects from *every* schema as bare names — Postgres/MySQL/MSSQL/Snowflake/BigQuery/ClickHouse/SQLite all restrict it to the connection's default — so `_build_one_model`'s `f"{schema}.{name}" if schema else name` wrote an unqualified `sql_table` for a non-default-schema object and the generator emitted `FROM reports`, which fails table-not-found. The same schema-blindness made `_get_columns_fallback` union two same-named tables' columns. Not literally a #283 regression: that `sql_table` line is byte-identical before and after, and `--schema` always qualified correctly; #283 changed *visibility* by ingesting views by default, and dbt materialises staging models as views, so a dlt+dbt DuckDB file went from a handful of models to dozens, most unqueryable. **Scope**: one pass covers ONE schema — explicit `--schema a,b` / `--all-schemas` (plus `schemas` / `all_schemas` on the Python, REST and MCP surfaces), else `datasource.schema_name`, else the connection default. Multi-schema is opt-in because it changes what `sql_table` holds; when exactly one schema is scanned and others exist, the result carries a hint naming them and the exit code is unchanged (a hint is not a failure). `schema_name` is a **fallback**, never a conflict with an explicit flag; the genuine conflicts (`schema`+`schemas`, `all_schemas`+either) are rejected by one shared helper called from every entry point, so the CLI's mutually-exclusive group is not the only thing holding the line. **Two different strings**: the *discovery token* is carried exactly as `get_schema_names()` yields it — catalog-qualified on DuckDB (`fda.main`), bare elsewhere — and the qualified form is the SAFE one. Measured: with a second catalog `ATTACH`ed, a bare `main` makes `get_table_names` and `has_table` reach into the attached catalog and makes the column fallback return the cross-catalog union, while `att_main.main` is exact. So `is_default` compares the token IN FULL (no last-segment comparison, which is what made `att_main.main` and `other.main` both read as the default), and the INFORMATION_SCHEMA fallbacks filter on `table_catalog` as well as `table_schema` — `table_schema` alone holds the bare name, so a qualified token matched nothing and produced a silently **column-less** model, and retrying bare would resurrect the union. This applies to the PK fallback too, which on DuckDB is the path that actually runs (its Inspector reports no PK even for a declared PRIMARY KEY) — filtering it wrong drops every primary key, and fan-out safety leans on `Column.primary_key`. The *emitted qualifier* is a different string: the bare last segment, since the connection's current catalog is already correct. **What gets qualified** (D-9): only non-default schemas, so widening the scan never rewrites models already on disk and one datasource legitimately mixes both forms; a schema named explicitly as a single value is written verbatim, preserving today's `--schema public` → `public.orders`. A multi-schema list is deliberately NOT verbatim — listing the default alongside another schema would re-qualify every existing model. `--all-schemas` means the **current catalog only**; attached catalogs are dropped loudly with the exact `--schema .` invocation that ingests them, never silently. **Merging**: re-ingest heals a MISSING qualifier (participating in the short-circuit and the save gate, like `source_kind` — a repair usually changes no columns, so a merge that only edits the update dict computes the fix and discards it) but never rewrites an existing one. Two schemas' same-named tables are never fused into one model: a schema mismatch skips, and — the case a schema comparison alone cannot see, because D-9 persists default-schema models *unqualified* — a bare persisted `sql_table` that names a real default-schema object also skips, rather than being repointed at another schema's table by the heal. **Collisions** are resolved in ONE phase over final model names with a 4-key total order (unsanitized beats sanitized, then default schema, then schema name, then object name), not as successive passes, so the mixed case (`s1.a__b` sanitizing onto a real `s2.a_b`) is defined and the outcome never depends on inspector listing order. Losers skip, never suffix. **Validation** derives its schema set from the models being validated rather than gaining a flag, and the live map is keyed on the full `.` identity with shorter aliases inserted only when unique — an ambiguous alias is dropped so a lookup misses instead of resolving to another catalog's same-named table. This is a data-loss path, not a nuisance: an unresolvable model is a `WholeModelDelete` that `validate-models --force-clean` acts on. One dotted-name splitter (`split_sql_table`, everything before the FINAL dot) replaces three disagreeing parsers, so hand-written Snowflake `db.schema.table` and BigQuery `project.dataset.table` stop losing their catalog. No new model field and no v9 migration — the schema lives in `sql_table`, which is where the generator already reads it. diff --git a/docs/concepts/ingestion.md b/docs/concepts/ingestion.md index 09cff708..57521965 100644 --- a/docs/concepts/ingestion.md +++ b/docs/concepts/ingestion.md @@ -68,12 +68,36 @@ Non-SQLite datasources (Postgres, MySQL, DuckDB, ClickHouse, SQL Server) skip th Already-persisted v7 SQLite models with the wrong `INT` type are **not** auto-repaired on `storage.get_model()` load (running a full table scan per column on every load would be too expensive). Re-ingest is the auto-heal path: `slayer ingest` or `slayer serve --ingest-on-startup`. The DEV-1361 DOUBLE → INT narrowing on legacy-dict migration is also gated on the probe on SQLite — it only fires when the probe positively certifies INT. +## Schema scope + +One ingest pass covers one schema unless told otherwise. The scope is, in +order of precedence: the schemas named on the call; the datasource's +`schema_name`; the connection's default schema. Naming several schemas, or +every schema, is opt-in on all four surfaces: + +| Surface | One schema | Several | Every schema | +|---|---|---|---| +| CLI | `--schema public` | `--schema public,analytics` | `--all-schemas` | +| Python | `schemas=["public"]` | `schemas=["public", "analytics"]` | `all_schemas=True` | +| MCP | `schema_name="public"` | `schemas="public,analytics"` | `all_schemas=True` | +| REST | `"schema_name": "public"` | `"schemas": ["public","analytics"]` | `"all_schemas": true` | + +Combining two of them is rejected (a `ValueError`, a 422, or an error string) +rather than silently preferring one. When exactly one schema is scanned and +others exist, the result carries a hint naming them. + +Objects outside the connection's default schema are written with a +schema-qualified `sql_table`; default-schema objects stay unqualified. See +[`slayer ingest`](../reference/cli.md#which-schemas-get-ingested) for the full +rules, including qualifier repair and the cross-schema guard. + ## Usage ### CLI ```bash slayer ingest --datasource my_postgres --schema public --storage ./slayer_data +slayer ingest --datasource my_postgres --all-schemas --storage ./slayer_data ``` ### Python @@ -86,13 +110,15 @@ async def main(): result = await ingest_datasource_idempotent( datasource=ds, storage=storage, - schema="public", + schemas=["public"], # or all_schemas=True include_tables=["orders", "customers"], # Optional filter exclude_tables=["migrations"], # Optional exclusion ) - # result.additions — what was added (new models / columns / joins) - # result.to_delete — pending validate_models drift entries - # result.errors — per-model failures (best-effort, doesn't abort) + # result.additions — what was added (new models / columns / joins) + # result.to_delete — pending validate_models drift entries + # result.errors — per-model failures (best-effort, doesn't abort) + # result.skipped — live objects we declined to model, with reasons + # result.schema_hint — set when other schemas were left out return result asyncio.run(main()) @@ -103,6 +129,7 @@ asyncio.run(main()) ``` create_datasource(name="mydb", type="postgres", ...) ingest_datasource_models(datasource_name="mydb", schema_name="public") +ingest_datasource_models(datasource_name="mydb", all_schemas=True) ``` ### REST API @@ -111,6 +138,10 @@ ingest_datasource_models(datasource_name="mydb", schema_name="public") curl -X POST http://localhost:5143/ingest \ -H "Content-Type: application/json" \ -d '{"datasource": "my_postgres", "schema_name": "public"}' + +curl -X POST http://localhost:5143/ingest \ + -H "Content-Type: application/json" \ + -d '{"datasource": "my_postgres", "all_schemas": true}' ``` ## Querying Rolled-Up Models diff --git a/docs/concepts/models.md b/docs/concepts/models.md index 422b2a3c..09a09e4f 100644 --- a/docs/concepts/models.md +++ b/docs/concepts/models.md @@ -63,6 +63,18 @@ generated SQL, but `sql_table` has no such restriction. Auto-ingestion uses this: an object named `reports__patient__drug` becomes a model named `reports_patient_drug` whose `sql_table` is still `reports__patient__drug`. +`sql_table` may be schema-qualified (`analytics.orders`), and for anything +outside the connection's default schema it has to be — the generated SQL uses +the value verbatim, so an unqualified name resolves through the search path +and a non-default-schema table is simply not found. Auto-ingestion writes the +qualifier whenever the object's schema differs from the connection's default, +or whenever the schema was named explicitly; default-schema objects stay +unqualified. Within one datasource the two forms therefore coexist, which is +intended: it keeps widening the ingest scope from rewriting models that +already exist. Snowflake `db.schema.table` and BigQuery +`project.dataset.table` are accepted too — the schema is everything before the +final dot. + ## Columns A column is the unit of structure on the model. The same column entry can serve as a group-by key in one query and as input to an aggregation in another — the role is decided per query, not declared up front. What the column *carries* is its identity (name), how to compute it from the source (`sql`), what data type to expect, and a handful of policy fields (which aggregations are allowed, whether it's a primary key, whether it's hidden). diff --git a/docs/configuration/datasources.md b/docs/configuration/datasources.md index 4a332c75..293ae784 100644 --- a/docs/configuration/datasources.md +++ b/docs/configuration/datasources.md @@ -170,6 +170,24 @@ Statement-level timeout is enforced via !!! note Both `username` and `user` field names are accepted. The `user` alias is automatically mapped to `username` for compatibility with common database tooling conventions. +### `schema_name` and ingestion + +`schema_name` is the default schema for both `slayer ingest` and `slayer +validate-models`, so the two always look at the same tables. It is a +*fallback*: an explicit `--schema` / `--all-schemas` on the command line wins, +and neither combination is an error. With `schema_name` unset, ingest uses the +connection's default schema. + +`slayer datasources create --schema X --ingest` persists `schema_name: X`. A +comma-separated list or `--all-schemas` persists nothing — there is no single +value to record, and writing the first one would silently narrow every later +bare `slayer ingest`. + +Ingesting more than one schema is opt-in because it changes what `sql_table` +holds: objects outside the connection's default schema are written +schema-qualified (`analytics.orders`), which is what makes them queryable. +See [`slayer ingest`](../reference/cli.md#slayer-ingest). + ## Ingesting at Startup To run idempotent auto-ingestion across every configured datasource each time `slayer serve` or `slayer mcp` boots, pass `--ingest-on-startup` (or set `SLAYER_INGEST_ON_STARTUP=1`). See [Ingesting at Startup](../concepts/ingestion.md#ingesting-at-startup) for the full contract. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index ef77b9e8..961d90ec 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -83,6 +83,8 @@ Auto-generate models from a datasource. ```bash slayer ingest --datasource my_postgres slayer ingest --datasource my_postgres --schema public +slayer ingest --datasource my_postgres --schema public,analytics +slayer ingest --datasource my_postgres --all-schemas slayer ingest --datasource my_postgres --include orders,customers slayer ingest --datasource my_postgres --exclude migrations,django_session slayer ingest --datasource my_postgres --no-views @@ -91,12 +93,50 @@ slayer ingest --datasource my_postgres --no-views | Flag | Required | Description | |------|----------|-------------| | `--datasource` | Yes | Datasource name | -| `--schema` | No | Database schema to inspect | +| `--schema` | No | Comma-separated schemas to inspect | +| `--all-schemas` | No | Inspect every non-system schema in the current database. Mutually exclusive with `--schema` | | `--include` | No | Comma-separated tables to include | | `--exclude` | No | Comma-separated tables to exclude | | `--no-views` | No | Skip views and materialized views (ingested by default) | | `--storage` | No | Storage path | +#### Which schemas get ingested + +With neither flag, ingest covers exactly one schema, resolved in this order: + +1. `--schema` / `--all-schemas`, when given; +2. the datasource's persisted `schema_name` + ([datasource config](../configuration/datasources.md)); +3. the connection's default schema. + +If other schemas exist, ingest names them and exits 0 — a hint, not a failure: + +``` +Note: ingested schema 'main' only. Other schemas in this datasource: openfda_rest. +Re-run with --schema openfda_rest, or --all-schemas, to ingest them. +``` + +Objects outside the connection's default schema get a schema-qualified +`sql_table` (`openfda_rest.reports`), which is what makes them queryable. +Default-schema objects stay unqualified, so widening the scan never rewrites +models that already exist. A schema named explicitly is always written +verbatim, so `--schema public` keeps producing `public.orders`. + +`--all-schemas` covers the **current database only**. Schemas belonging to an +`ATTACH`ed DuckDB catalog are reported as skipped, naming the +`--schema .` invocation that would ingest them. + +A model whose `sql_table` is missing its schema qualifier is repaired on the +next ingest of that schema, and the repair is reported: + +``` +Updated: reports (sql_table: reports → openfda_rest.reports) +``` + +An existing qualifier is never rewritten, and two schemas' same-named tables +are never merged into one model — the second is skipped with a `cross-schema` +reason rather than silently repointing the first. + #### Views Views and materialized views are ingested alongside tables by default — dbt @@ -205,7 +245,8 @@ slayer datasources create demo --ingest # bundled Jaffle Shop demo | `--name` | No | Override the auto-derived name (default for the demo: `jaffle_shop`) | | `--description` | No | Human-readable description | | `--ingest` | No | Run auto-ingestion immediately after creating the datasource | -| `--schema` | No | (with `--ingest`) Schema to ingest from | +| `--schema` | No | (with `--ingest`) Comma-separated schemas to ingest from. A single schema is also persisted as the datasource's `schema_name`, so later bare `slayer ingest` runs use it | +| `--all-schemas` | No | (with `--ingest`) Ingest every non-system schema. Mutually exclusive with `--schema`; persists no `schema_name` | | `--include` | No | (with `--ingest`) Comma-separated tables to include | | `--exclude` | No | (with `--ingest`) Comma-separated tables to exclude | | `--no-views` | No | (with `--ingest`) Skip views and materialized views (ingested by default) | diff --git a/slayer/api/server.py b/slayer/api/server.py index d1e6c0c1..e5caa9a3 100644 --- a/slayer/api/server.py +++ b/slayer/api/server.py @@ -5,7 +5,7 @@ from typing import Any from fastapi import FastAPI, HTTPException -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator from slayer.mcp.server import create_mcp_server from slayer.core.errors import ( @@ -103,7 +103,24 @@ class IngestRequest(BaseModel): datasource: str include_tables: list[str] | None = None exclude_tables: list[str] | None = None + # Kept for backward compatibility; folded into ``schemas=[schema_name]``. schema_name: str | None = None + schemas: list[str] | None = None + all_schemas: bool = False + + @model_validator(mode="after") + def _one_way_to_say_it(self) -> "IngestRequest": + """Reject conflicting scope arguments rather than silently preferring + whichever the handler reads first. The rule lives here so it applies + to every caller of the endpoint, and mirrors the engine's.""" + from slayer.engine.ingestion import _resolve_scope_args + + _resolve_scope_args( + schema=self.schema_name, + schemas=self.schemas, + all_schemas=self.all_schemas, + ) + return self class ValidateModelsRequest(BaseModel): @@ -650,6 +667,8 @@ async def ingest(request: IngestRequest) -> dict[str, Any]: include_tables=request.include_tables, exclude_tables=request.exclude_tables, schema=request.schema_name, + schemas=request.schemas, + all_schemas=request.all_schemas, ) except SQLAlchemyError as exc: # OperationalError / DatabaseError both derive from SQLAlchemyError diff --git a/slayer/cli.py b/slayer/cli.py index d4b09463..ea1a89a5 100644 --- a/slayer/cli.py +++ b/slayer/cli.py @@ -261,13 +261,30 @@ def main(): # NOSONAR(S3776) — linear top-level CLI command dispatch (one eli examples: slayer ingest --datasource my_postgres slayer ingest --datasource my_postgres --schema public + slayer ingest --datasource my_postgres --schema public,analytics + slayer ingest --datasource my_postgres --all-schemas slayer ingest --datasource my_postgres --include orders,customers slayer ingest --datasource my_postgres --exclude migrations,django_session """, formatter_class=argparse.RawDescriptionHelpFormatter, ) ingest_parser.add_argument("--datasource", required=True, help="Name of the datasource to ingest from") - ingest_parser.add_argument("--schema", default=None, help="Database schema to introspect (e.g., public)") + ingest_scope = ingest_parser.add_mutually_exclusive_group() + ingest_scope.add_argument( + "--schema", + default=None, + help=( + "Comma-separated schemas to introspect (e.g. public or " + "public,analytics). Default: the connection's default schema" + ), + ) + ingest_scope.add_argument( + "--all-schemas", + dest="all_schemas", + action="store_true", + default=False, + help="Introspect every non-system schema in the current database", + ) ingest_parser.add_argument( "--include", default=None, @@ -520,8 +537,23 @@ def main(): # NOSONAR(S3776) — linear top-level CLI command dispatch (one eli action="store_true", help="Run auto-ingestion immediately after creating the datasource", ) - datasources_create_parser.add_argument( - "--schema", default=None, help="(with --ingest) Schema to ingest from" + datasources_create_scope = ( + datasources_create_parser.add_mutually_exclusive_group() + ) + datasources_create_scope.add_argument( + "--schema", + default=None, + help=( + "(with --ingest) Comma-separated schemas to ingest from. A single " + "schema is also persisted as the datasource's default" + ), + ) + datasources_create_scope.add_argument( + "--all-schemas", + dest="all_schemas", + action="store_true", + default=False, + help="(with --ingest) Ingest every non-system schema in the database", ) datasources_create_parser.add_argument( "--include", @@ -1391,7 +1423,8 @@ def _run_ingest(args): ingest_datasource_idempotent( datasource=ds, storage=storage, - schema=args.schema, + schemas=_parse_csv_arg(args.schema), + all_schemas=getattr(args, "all_schemas", False), include_tables=_parse_csv_arg(args.include), exclude_tables=_parse_csv_arg(args.exclude), include_views=getattr(args, "include_views", True), @@ -1425,6 +1458,10 @@ def _run_ingest(args): for addition in result.additions: _print_ingest_addition(addition) _print_ingest_drift_and_errors(result) + # Narrowing the default scan to one schema is a behaviour change, so say + # which schemas were left out. Advisory only — the exit code is unchanged. + if getattr(result, "schema_hint", None): + print(f"\n{result.schema_hint}") # A skip means we declined to ingest a perfectly valid object, so it fails # the command — `--exclude ` is the documented way to make it green. if result.errors or result.skipped: @@ -1965,12 +2002,21 @@ def _run_datasources_create(args, storage): sys.exit(1) name = args.name or derived_name + schemas = _parse_csv_arg(args.schema) + all_schemas = getattr(args, "all_schemas", False) + # ``schema_name`` is a single-schema default. A CSV list or --all-schemas + # has no single value to persist, and persisting the first would silently + # narrow every later bare `slayer ingest`. + persisted_schema = ( + schemas[0] if schemas and len(schemas) == 1 and not all_schemas else None + ) ds = DatasourceConfig.model_validate( { "name": name, "type": ds_type, "connection_string": args.connection_string, "description": args.description, + "schema_name": persisted_schema, } ) @@ -1993,13 +2039,17 @@ def _run_datasources_create(args, storage): from slayer.engine.ingestion import ingest_datasource - include = [t for t in (s.strip() for s in args.include.split(",")) if t] if args.include else None - exclude = [t for t in (s.strip() for s in args.exclude.split(",")) if t] if args.exclude else None + include = _parse_csv_arg(args.include) + exclude = _parse_csv_arg(args.exclude) try: + # The parsed list is passed explicitly rather than relying on the + # persisted ``schema_name``, so the two can never be read as a + # conflict — ``schema=`` is deliberately never passed on this path. models = ingest_datasource( datasource=ds, - schema=args.schema, + schemas=schemas, + all_schemas=all_schemas, include_tables=include, exclude_tables=exclude, include_views=getattr(args, "include_views", True), diff --git a/slayer/engine/ingestion.py b/slayer/engine/ingestion.py index 50309d11..c041a14a 100644 --- a/slayer/engine/ingestion.py +++ b/slayer/engine/ingestion.py @@ -10,6 +10,7 @@ import asyncio import logging import sys +import warnings from collections import defaultdict, deque from typing import TYPE_CHECKING, Any, TextIO @@ -32,6 +33,9 @@ _get_columns_fallback, _parse_info_schema_is_float, _safe_get_columns, + qualified_default_schema, + split_schema_token, + split_sql_table, ) from slayer.core.errors import AmbiguousModelError, EntityResolutionError from slayer.memories.models import MEMORY_CANONICAL_PREFIX as _MEMORY_PREFIX @@ -452,19 +456,33 @@ def _get_pk_constraint_fallback( table_name: str, schema: str | None, ) -> dict: - """Get PK constraint via INFORMATION_SCHEMA when Inspector.get_pk_constraint() fails.""" + """Get PK constraint via INFORMATION_SCHEMA when Inspector.get_pk_constraint() fails. + + On DuckDB this is the path that actually runs — its Inspector reports an + empty ``constrained_columns`` even for a declared PRIMARY KEY — so it has + to understand the same catalog-qualified schema token discovery uses. + ``table_schema`` alone holds the bare name, and filtering on it with a + qualified token silently matched nothing, dropping every primary key. + """ if schema: + catalog, bare_schema = split_schema_token(schema) + clauses = [ + "tc.table_name = :table_name", + "tc.constraint_type = 'PRIMARY KEY'", + "tc.table_schema = :schema", + ] + params = {"table_name": table_name, "schema": bare_schema} + if catalog is not None: + clauses.append("tc.table_catalog = :catalog") + params["catalog"] = catalog sql = ( "SELECT kcu.column_name " "FROM information_schema.table_constraints tc " "JOIN information_schema.key_column_usage kcu " " ON tc.constraint_name = kcu.constraint_name " " AND tc.table_schema = kcu.table_schema " - "WHERE tc.table_name = :table_name " - " AND tc.constraint_type = 'PRIMARY KEY' " - " AND tc.table_schema = :schema" + "WHERE " + " AND ".join(clauses) ) - params = {"table_name": table_name, "schema": schema} else: sql = ( "SELECT kcu.column_name " @@ -726,14 +744,11 @@ def _sqlite_probe_integer_columns( def _parse_qualified_sql_table(sql_table: str) -> tuple[str | None, str]: """Split ``"schema.table"`` into ``(schema, table)`` or ``(None, table)``. - Only splits on a single dot — table/schema names containing dots are - out of scope for the auto-ingest path (the dotted form would never have - survived ``Inspector.get_table_names`` either). + Delegates to :func:`split_sql_table` so a three-part + ``catalog.schema.table`` keeps its catalog instead of losing it to a + split on the first dot. """ - if "." in sql_table: - schema, _, table = sql_table.partition(".") - return schema or None, table - return None, sql_table + return split_sql_table(sql_table) def introspect_table_to_model( @@ -784,11 +799,23 @@ def introspect_table_to_model( # --------------------------------------------------------------------------- -class IngestableObject(BaseModel): - """One database object discovered by :func:`list_ingestable_objects`.""" +with warnings.catch_warnings(): + # ``schema`` shadows Pydantic v2's deprecated ``BaseModel.schema()``. + # The name is deliberate — it is the SQLAlchemy ``Inspector`` keyword this + # value is passed as — and the shadowed classmethod is never called here. + warnings.filterwarnings("ignore", message='Field name "schema"') - name: str - kind: ObjectKind + class IngestableObject(BaseModel): + """One database object discovered by :func:`list_ingestable_objects`.""" + + name: str + kind: ObjectKind + # The discovery token the object was found under — catalog-qualified + # on dialects that qualify (DuckDB), bare elsewhere, ``None`` when the + # dialect reports no default schema. This is the string handed back to + # the Inspector; the qualifier written into ``sql_table`` is a + # different string (see :func:`qualify_sql_table`). + schema: str | None = None class SkippedTable(BaseModel): @@ -813,6 +840,221 @@ class IngestionScanReport(BaseModel): # but they were all skipped / already in sync" — the CLI needs that # distinction to decide between the empty-schema hint and silence. objects: list[IngestableObject] = Field(default_factory=list) + # Set when exactly one schema was scanned and the datasource holds + # others: narrowing the default scan is a behaviour change, so it has to + # be visible. Never an error — a hint is not a failure. + schema_hint: str | None = None + # Object names living in the connection's default schema. The additive + # pass needs it to tell "this persisted unqualified model IS the default + # schema's table" from "a same-named table in another schema", which is + # the one case a schema comparison alone cannot decide. + default_schema_objects: list[str] = Field(default_factory=list) + + +# --------------------------------------------------------------------------- +# Schema scope +# --------------------------------------------------------------------------- + + +_SYSTEM_SCHEMA_NAMES = frozenset( + { + "information_schema", + "pg_catalog", + "pg_toast", + "performance_schema", + "mysql", + "sys", + "sys_temp", + } +) +_SYSTEM_SCHEMA_PREFIXES = ("pg_temp_", "pg_toast_temp_") +# DuckDB exposes its own metadata and scratch space as first-segment +# catalogs (``system.main``, ``temp.main``), so those are matched on the +# catalog rather than on the schema name. +_SYSTEM_CATALOGS = frozenset({"system", "temp"}) + + +def _is_system_schema(token: str) -> bool: + """Whether a discovery token names a system schema rather than user data.""" + catalog, schema = split_schema_token(token) + if catalog is not None and catalog.lower() in _SYSTEM_CATALOGS: + return True + bare = schema.lower() + return bare in _SYSTEM_SCHEMA_NAMES or bare.startswith(_SYSTEM_SCHEMA_PREFIXES) + + +class ResolvedSchema(BaseModel): + """One schema in ingest scope. + + ``name`` is the *discovery* token, carried exactly as the dialect + enumerates it. ``explicit`` means the user named this single schema, so + its qualifier is written verbatim; a multi-schema request follows the + automatic rules instead, or listing the default schema alongside another + would re-qualify every model already on disk. + """ + + name: str | None = None + explicit: bool = False + is_default: bool = False + + +class IngestSchemaScope(BaseModel): + """The schemas one ingest pass will scan, plus what it left out.""" + + schemas: list[ResolvedSchema] = Field(default_factory=list) + # Non-system schemas NOT in scope. Hint only — populated when exactly one + # schema is in scope and ``all_schemas`` was not used. + other_schemas: list[str] = Field(default_factory=list) + # Schemas dropped because they belong to an attached catalog. Reported, + # never silently discarded. + skipped: list[SkippedTable] = Field(default_factory=list) + + +def _bare_schema(token: str | None) -> str: + """The last segment of a discovery token, i.e. the schema without catalog.""" + return token.rsplit(".", 1)[-1] if token else "" + + +def _matches_default(token: str, default_token: str | None) -> bool: + """Whether a user-supplied token names the connection's default schema. + + A qualified token must match in full — that is what stops ``aaa.main`` + and ``att_main.main`` both reading as the default. A bare token is + compared against the default's bare segment, so ``--schema main`` on + DuckDB still recognises ``fda.main``. + """ + if not default_token: + return False + if token == default_token: + return True + if "." in token: + return False + return token == _bare_schema(default_token) + + +def _current_catalog_only( + *, inspector: sa.engine.Inspector, tokens: list[str], +) -> tuple[list[str], list[SkippedTable]]: + """Restrict enumerated tokens to the connection's current catalog. + + ``--all-schemas`` means "this database", not "and everything anyone has + attached to the session" — DuckDB's ``get_schema_names()`` lists + ``ATTACH``ed catalogs too. Dropped tokens are reported with the exact + invocation that would ingest them. + """ + if not any("." in t for t in tokens): + return list(tokens), [] + from slayer.engine.introspect_utils import _current_catalog + + catalog = _current_catalog(inspector) + if not catalog: + return list(tokens), [] + kept: list[str] = [] + dropped: list[SkippedTable] = [] + for token in tokens: + token_catalog, _ = split_schema_token(token) + if token_catalog is None or token_catalog == catalog: + kept.append(token) + continue + dropped.append( + SkippedTable( + table_name=token, + reason=( + f"schema '{token}' belongs to attached catalog " + f"'{token_catalog}'; ingest it with --schema '{token}'" + ), + ) + ) + return kept, dropped + + +def _enumerate_schemas(inspector: sa.engine.Inspector) -> list[str]: + """Every non-system schema the connection can see, tokens as enumerated.""" + try: + names = list(inspector.get_schema_names() or []) + except Exception as exc: # noqa: BLE001 — enumeration is best-effort + logger.debug("get_schema_names failed: %s", exc) + return [] + return sorted(n for n in names if isinstance(n, str) and not _is_system_schema(n)) + + +def resolve_ingest_schemas( + *, + inspector: sa.engine.Inspector, + requested: list[str] | None, + all_schemas: bool, + datasource_schema: str | None, +) -> IngestSchemaScope: + """Decide which schemas one ingest pass covers. + + Precedence, first non-empty wins: ``all_schemas``; an explicit + ``requested`` list; the datasource's persisted ``schema_name``; the + connection's default schema. The persisted value is a *fallback*, + consulted only when nothing more specific was given — never a conflict. + """ + default_token = qualified_default_schema(inspector) + enumerated = _enumerate_schemas(inspector) + local, dropped = _current_catalog_only(inspector=inspector, tokens=enumerated) + + if all_schemas: + schemas = [ + ResolvedSchema( + name=token, + explicit=False, + is_default=(token == default_token), + ) + for token in local + ] + elif requested: + # A single named schema is honoured verbatim (``--schema public`` + # keeps producing ``public.orders``). A list is not: qualifying the + # default schema verbatim there would rewrite every model on disk. + single = len(requested) == 1 + schemas = [ + ResolvedSchema( + name=token, + explicit=single, + is_default=_matches_default(token, default_token), + ) + for token in requested + ] + elif datasource_schema: + schemas = [ + ResolvedSchema( + name=datasource_schema, + explicit=True, + is_default=_matches_default(datasource_schema, default_token), + ) + ] + else: + schemas = [ + ResolvedSchema(name=default_token, explicit=False, is_default=True) + ] + + other: list[str] = [] + if len(schemas) == 1 and not all_schemas: + in_scope = _bare_schema(schemas[0].name) + other = [b for b in (_bare_schema(t) for t in local) if b != in_scope] + return IngestSchemaScope( + schemas=schemas, other_schemas=other, skipped=dropped if all_schemas else [], + ) + + +def qualify_sql_table(*, obj: IngestableObject, resolved: ResolvedSchema) -> str: + """The ``sql_table`` value for ``obj``. + + Note this is NOT the discovery token: the catalog segment is dropped + because the connection's current catalog is already the right one, and + re-stating it would only break if the datasource were repointed. + + Default-schema objects stay unqualified so that widening the scan never + rewrites models that already exist. + """ + if resolved.explicit: + return f"{resolved.name}.{obj.name}" + if resolved.is_default or not resolved.name: + return obj.name + return f"{_bare_schema(resolved.name)}.{obj.name}" def _safe_object_names( @@ -849,10 +1091,19 @@ def list_ingestable_objects( ) -> list[IngestableObject]: """Discover every ingestable object in ``schema``, classified by kind. - Order is deterministic (tables, views, matviews) because - :func:`_assign_model_names` resolves collisions first-come. Deduped across + ``schema=None`` resolves to the connection's *default schema token* + rather than being passed through. On DuckDB a bare ``None`` returns + objects from every schema as bare names, which is how a model in a + non-default schema ended up with an unqualified ``sql_table``; and even + the bare default (``main``) still reaches into ``ATTACH``ed catalogs, + so the token has to carry the catalog. + + Order is deterministic (tables, views, matviews). Deduped across accessors — some dialects return views from ``get_table_names()``. """ + resolved_schema = ( + schema if schema is not None else qualified_default_schema(inspector) + ) objects: list[IngestableObject] = [] seen: set[str] = set() @@ -861,13 +1112,17 @@ def _add(names: list[str], kind: ObjectKind) -> None: if name in seen: continue seen.add(name) - objects.append(IngestableObject(name=name, kind=kind)) + objects.append( + IngestableObject(name=name, kind=kind, schema=resolved_schema) + ) - _add(list(inspector.get_table_names(schema=schema) or []), "table") + _add(list(inspector.get_table_names(schema=resolved_schema) or []), "table") if include_views: _add( _safe_object_names( - accessor_name="get_view_names", inspector=inspector, schema=schema + accessor_name="get_view_names", + inspector=inspector, + schema=resolved_schema, ), "view", ) @@ -875,60 +1130,113 @@ def _add(names: list[str], kind: ObjectKind) -> None: _safe_object_names( accessor_name="get_materialized_view_names", inspector=inspector, - schema=schema, + schema=resolved_schema, ), "materialized_view", ) return objects +def list_ingestable_objects_multi( + *, + inspector: sa.engine.Inspector, + scope: IngestSchemaScope, + include_views: bool = True, +) -> list[IngestableObject]: + """Discover objects across every schema in ``scope``, in scope order.""" + objects: list[IngestableObject] = [] + seen: set[tuple[str | None, str]] = set() + for resolved in scope.schemas: + for obj in list_ingestable_objects( + inspector=inspector, + schema=resolved.name, + include_views=include_views, + ): + key = (obj.schema, obj.name) + if key in seen: + continue + seen.add(key) + objects.append(obj) + return objects + + +def _spans_multiple_schemas(objects: list[IngestableObject]) -> bool: + """Whether ``objects`` were discovered in more than one schema.""" + return len({o.schema for o in objects}) > 1 + + +def _object_label(obj: IngestableObject, *, multi_schema: bool) -> str: + """How to name an object in a message — disambiguated only when needed.""" + if multi_schema and obj.schema: + return f"{_bare_schema(obj.schema)}.{obj.name}" + return obj.name + + def _assign_model_names( objects: list[IngestableObject], -) -> tuple[dict[str, str], list[SkippedTable]]: - """Map each object name to its model name, returning ``(mapping, skipped)``. - - Model names may not contain ``__`` (the SQL generator splits it back into a - join path, so ``a__b`` would silently query ``a -> b``); object names may, - so only the model name is sanitized. - - Unsanitized names are reserved first, so a real ``a_b`` beats a sanitized - ``a__b``. Collisions skip rather than suffix — suffixes shift with the - object set, orphaning models and churning drift. - - Both passes are scan-order independent. The sanitized pass walks its - candidates in sorted order, so when two objects collapse to the same name - (``a__b`` and ``a___b`` both yield ``a_b``) the winner is fixed by the name - itself, not by whichever the inspector happened to list first. Otherwise a - dialect changing its listing order would silently repoint the model at a - different physical object. + *, + resolved_by_schema: dict[str | None, ResolvedSchema] | None = None, +) -> tuple[dict[tuple[str | None, str], str], list[SkippedTable]]: + """Map each ``(schema, object name)`` to its model name. + + Returns ``(mapping, skipped)``. Model names may not contain ``__`` (the + SQL generator splits it back into a join path, so ``a__b`` would silently + query ``a -> b``); object names may, so only the model name is sanitized. + + Contention is resolved in ONE phase keyed on the final model name, not as + successive passes: two objects can now compete either because one was + sanitized into the other's name or because they live in different schemas, + and resolving those separately would leave the mixed case (``s1.a__b`` + sanitizing onto a real ``s2.a_b``) dependent on reservation order. + + The winner is fixed by a total order over the contenders, every key of + which is a property of the object itself — so the outcome is independent + of inspector listing order and of ``--schema`` argument order: + + 1. a name needing no sanitization beats one that did; + 2. then the default schema beats a non-default one; + 3. then the lower schema name; 4. then the lower object name. + + Losers are skipped, never suffixed — suffixes shift with the object set, + orphaning models and churning drift. """ - assigned: dict[str, str] = {} - taken: set[str] = {o.name for o in objects if "__" not in o.name} - skipped: list[SkippedTable] = [] + multi_schema = _spans_multiple_schemas(objects) + resolved_by_schema = resolved_by_schema or {} + + def _rank(obj: IngestableObject) -> tuple: + resolved = resolved_by_schema.get(obj.schema) + return ( + 0 if "__" not in obj.name else 1, + 0 if (resolved is not None and resolved.is_default) else 1, + _bare_schema(obj.schema), + obj.name, + ) + contenders: dict[str, list[IngestableObject]] = defaultdict(list) for obj in objects: - if "__" not in obj.name: - assigned[obj.name] = obj.name + model_name = ( + sanitize_model_name(obj.name) if "__" in obj.name else obj.name + ) + contenders[model_name].append(obj) - for obj in sorted( - (o for o in objects if "__" in o.name), key=lambda o: o.name - ): - candidate = sanitize_model_name(obj.name) - if candidate in taken: + assigned: dict[tuple[str | None, str], str] = {} + skipped: list[SkippedTable] = [] + for model_name in sorted(contenders): + winner, *losers = sorted(contenders[model_name], key=_rank) + assigned[(winner.schema, winner.name)] = model_name + winner_label = _object_label(winner, multi_schema=multi_schema) + for loser in losers: skipped.append( SkippedTable( - table_name=obj.name, - kind=obj.kind, + table_name=_object_label(loser, multi_schema=multi_schema), + kind=loser.kind, reason=( - f"name collision: sanitizing '__' yields " - f"'{candidate}', which is already taken" + f"name collision: model name '{model_name}' is also " + f"claimed by '{winner_label}'; ingest the schemas " + f"separately or use --exclude" ), ) ) - continue - taken.add(candidate) - assigned[obj.name] = candidate - return assigned, skipped @@ -943,7 +1251,7 @@ def _build_one_model( inspector: sa.engine.Inspector, obj: IngestableObject, model_name: str, - schema: str | None, + resolved: ResolvedSchema, data_source: str, fk_graph: dict[str, set[str]], has_cycles: bool, @@ -951,11 +1259,17 @@ def _build_one_model( table_set: set[str], ) -> SlayerModel: """Introspect one live object into a model. Raises on failure; the caller - isolates per-object.""" + isolates per-object. + + Introspection is driven by the *discovery* token (``resolved.name``) while + the emitted ``sql_table`` carries the qualifier — two different strings + that must not be conflated. + """ referenced = ( set() if has_cycles else _compute_transitive_closure(fk_graph, obj.name) ) - sql_table = f"{schema}.{obj.name}" if schema else obj.name + schema = resolved.name + sql_table = qualify_sql_table(obj=obj, resolved=resolved) model_joins = None if referenced: @@ -1036,82 +1350,224 @@ def _collect_fk_columns( return out +def _schema_hint(scope: IngestSchemaScope) -> str | None: + """Tell the user which schemas the scan left out, and how to get them. + + Eligibility is "exactly one schema in scope and others exist" — it is + deliberately independent of whether that schema was named explicitly. A + user who set ``schema_name`` months ago still needs to hear that another + schema has appeared since. + """ + if len(scope.schemas) != 1 or not scope.other_schemas: + return None + in_scope = _bare_schema(scope.schemas[0].name) + return ( + f"Note: ingested schema '{in_scope}' only. Other schemas in this " + f"datasource: {', '.join(scope.other_schemas)}.\n" + f"Re-run with --schema {scope.other_schemas[0]}, or --all-schemas, " + f"to ingest them." + ) + + +def _scan_one_schema( + *, + sa_engine: sa.Engine, + inspector: sa.engine.Inspector, + resolved: ResolvedSchema, + objects: list[IngestableObject], + name_by_object: dict[tuple[str | None, str], str], + data_source: str, + multi_schema: bool, +) -> tuple[list[SlayerModel], list[SkippedTable]]: + """Build every model for one schema. The FK graph is per-schema: joins + only ever resolve within the schema the objects were discovered in.""" + table_names = [o.name for o in objects] + table_set = set(table_names) + schema = resolved.name + + fk_graph = _build_fk_graph( + inspector=inspector, table_names=table_names, schema=schema + ) + has_cycles = False + try: + _check_acyclic(fk_graph) + except RollupGraphError as e: + logger.warning(f"FK graph has cycles, skipping rollup: {e}") + has_cycles = True + + fk_columns_by_table = _collect_fk_columns( + inspector=inspector, table_names=table_names, schema=schema + ) + + models: list[SlayerModel] = [] + skipped: list[SkippedTable] = [] + for obj in objects: + model_name = name_by_object.get((obj.schema, obj.name)) + if model_name is None: + continue # already recorded in ``skipped`` by _assign_model_names + try: + models.append( + _build_one_model( + sa_engine=sa_engine, + inspector=inspector, + obj=obj, + model_name=model_name, + resolved=resolved, + data_source=data_source, + fk_graph=fk_graph, + has_cycles=has_cycles, + fk_columns_by_table=fk_columns_by_table, + table_set=table_set, + ) + ) + except Exception as exc: # noqa: BLE001 — per-object isolation + logger.warning( + "Skipping %s %r in datasource %r: %s", + obj.kind, obj.name, data_source, exc, + ) + skipped.append( + SkippedTable( + table_name=_object_label(obj, multi_schema=multi_schema), + kind=obj.kind, + reason=str(exc), + ) + ) + return models, skipped + + +def _resolve_scope_args( + *, + schema: str | None, + schemas: list[str] | None, + all_schemas: bool, +) -> list[str] | None: + """Fold the three scope arguments into one requested list, or raise. + + Shared by every entry point — engine, CLI, REST and MCP — so the CLI's + mutually-exclusive group is not the only thing holding the line. + """ + if schema is not None and schemas is not None: + raise ValueError( + "Cannot set both 'schema' and 'schemas' — pass one or the other." + ) + if all_schemas and (schema is not None or schemas is not None): + raise ValueError( + "Cannot combine 'all_schemas' with an explicit schema — " + "'all_schemas' already covers every schema." + ) + if schemas is not None: + return list(schemas) or None + if schema is not None: + return [schema] + return None + + +def _default_schema_object_names( + *, + inspector: sa.engine.Inspector, + scope: IngestSchemaScope, + objects: list[IngestableObject], + include_views: bool, +) -> list[str]: + """Object names living in the connection's default schema. + + Derived from the objects already discovered when the default schema is in + scope; otherwise listed explicitly, which costs one extra catalog call on + the only path that needs it. + """ + default_token = qualified_default_schema(inspector) + if any(s.name == default_token for s in scope.schemas): + return [o.name for o in objects if o.schema == default_token] + try: + return [ + o.name + for o in list_ingestable_objects( + inspector=inspector, + schema=default_token, + include_views=include_views, + ) + ] + except Exception as exc: # noqa: BLE001 — the guard degrades, never aborts + logger.debug("default-schema listing failed: %s", exc) + return [] + + def ingest_datasource_report( datasource: DatasourceConfig, include_tables: list[str] | None = None, exclude_tables: list[str] | None = None, schema: str | None = None, include_views: bool = True, + schemas: list[str] | None = None, + all_schemas: bool = False, ) -> IngestionScanReport: """Introspect ``datasource``, returning models plus everything skipped. - Discovers views and matviews (``include_views``), and skips an unmodellable - object with a reason rather than aborting the run. + Scope resolution is :func:`resolve_ingest_schemas`; ``schema`` is kept as + a single-value alias for ``schemas``. Discovers views and matviews + (``include_views``), and skips an unmodellable object with a reason rather + than aborting the run. """ + requested = _resolve_scope_args( + schema=schema, schemas=schemas, all_schemas=all_schemas + ) from slayer.sql import engine_factory sa_engine = engine_factory.get_engine(datasource.resolve_env_vars()) try: inspector = sa.inspect(sa_engine) - objects = list_ingestable_objects( - inspector=inspector, schema=schema, include_views=include_views + scope = resolve_ingest_schemas( + inspector=inspector, + requested=requested, + all_schemas=all_schemas, + datasource_schema=datasource.schema_name or None, ) + discovered = list_ingestable_objects_multi( + inspector=inspector, scope=scope, include_views=include_views + ) + objects = discovered if include_tables: objects = [o for o in objects if o.name in include_tables] if exclude_tables: objects = [o for o in objects if o.name not in exclude_tables] - table_names = [o.name for o in objects] - table_set = set(table_names) - - name_by_object, skipped = _assign_model_names(objects) - - # Build FK graph, check for cycles - fk_graph = _build_fk_graph( - inspector=inspector, table_names=table_names, schema=schema + resolved_by_schema: dict[str | None, ResolvedSchema] = { + s.name: s for s in scope.schemas + } + name_by_object, skipped = _assign_model_names( + objects, resolved_by_schema=resolved_by_schema ) - has_cycles = False - try: - _check_acyclic(fk_graph) - except RollupGraphError as e: - logger.warning(f"FK graph has cycles, skipping rollup: {e}") - has_cycles = True + skipped = list(scope.skipped) + skipped + multi_schema = _spans_multiple_schemas(objects) - fk_columns_by_table = _collect_fk_columns( - inspector=inspector, table_names=table_names, schema=schema - ) - - models = [] - for obj in objects: - model_name = name_by_object.get(obj.name) - if model_name is None: - continue # already recorded in ``skipped`` by _assign_model_names - try: - models.append( - _build_one_model( - sa_engine=sa_engine, - inspector=inspector, - obj=obj, - model_name=model_name, - schema=schema, - data_source=datasource.name, - fk_graph=fk_graph, - has_cycles=has_cycles, - fk_columns_by_table=fk_columns_by_table, - table_set=table_set, - ) - ) - except Exception as exc: # noqa: BLE001 — per-object isolation - logger.warning( - "Skipping %s %r in datasource %r: %s", - obj.kind, obj.name, datasource.name, exc, - ) - skipped.append( - SkippedTable(table_name=obj.name, kind=obj.kind, reason=str(exc)) - ) + models: list[SlayerModel] = [] + for resolved in scope.schemas: + in_schema = [o for o in objects if o.schema == resolved.name] + if not in_schema: + continue + schema_models, schema_skips = _scan_one_schema( + sa_engine=sa_engine, + inspector=inspector, + resolved=resolved, + objects=in_schema, + name_by_object=name_by_object, + data_source=datasource.name, + multi_schema=multi_schema, + ) + models.extend(schema_models) + skipped.extend(schema_skips) return IngestionScanReport( - models=models, skipped=skipped, objects=objects + models=models, + skipped=skipped, + objects=objects, + schema_hint=_schema_hint(scope), + default_schema_objects=_default_schema_object_names( + inspector=inspector, + scope=scope, + objects=discovered, + include_views=include_views, + ), ) finally: # One-shot admin operation, not a hot query path. Disposing releases @@ -1130,6 +1586,8 @@ def ingest_datasource( exclude_tables: list[str] | None = None, schema: str | None = None, include_views: bool = True, + schemas: list[str] | None = None, + all_schemas: bool = False, ) -> list[SlayerModel]: """Models only, for callers that don't need the skip report.""" return ingest_datasource_report( @@ -1138,6 +1596,8 @@ def ingest_datasource( exclude_tables=exclude_tables, schema=schema, include_views=include_views, + schemas=schemas, + all_schemas=all_schemas, ).models @@ -1250,6 +1710,9 @@ class AdditiveMergeResult(BaseModel): new_joins: list[str] = Field(default_factory=list) widened_columns: list[str] = Field(default_factory=list) kind_changed: bool = False + # ``"reports → openfda_rest.reports"`` when a missing schema qualifier was + # repaired, else None. + sql_table_change: str | None = None def _additive_merge_existing( @@ -1280,6 +1743,10 @@ def _additive_merge_existing( usually changes no columns at all. A field that never refreshed would confidently lie about precisely the case it was added for. A ``None`` from a path that doesn't classify never erases a known value. + * Carve-out: a MISSING ``sql_table`` schema qualifier is repaired, so a + model ingested before schemas were recorded becomes queryable again on + the next run. An existing qualifier is never rewritten — healing adds + one, it does not repoint a model someone deliberately aimed elsewhere. """ existing_by_name: dict[str, Column] = {c.name: c for c in persisted.columns} fresh_by_name: dict[str, Column] = {c.name: c for c in fresh.columns} @@ -1314,17 +1781,25 @@ def _additive_merge_existing( and fresh.source_kind != persisted.source_kind ) + # A qualifier repair typically changes nothing else either, so it has to + # participate in the short-circuit for the same reason ``kind_changed`` + # does — otherwise the repaired model is computed and then discarded. + sql_table_change = _qualifier_repair(persisted=persisted, fresh=fresh) + if not ( new_column_names or new_join_targets or widened_column_names or kind_changed + or sql_table_change ): return AdditiveMergeResult(merged=persisted) update: dict[str, Any] = {"columns": merged_columns, "joins": new_joins} if kind_changed: update["source_kind"] = fresh.source_kind + if sql_table_change: + update["sql_table"] = fresh.sql_table return AdditiveMergeResult( merged=persisted.model_copy(update=update), @@ -1332,6 +1807,76 @@ def _additive_merge_existing( new_joins=new_join_targets, widened_columns=widened_column_names, kind_changed=kind_changed, + sql_table_change=sql_table_change, + ) + + +def _qualifier_repair( + *, persisted: SlayerModel, fresh: SlayerModel, +) -> str | None: + """``"before → after"`` when ``fresh`` supplies a qualifier ``persisted`` + is missing, else ``None``. + + Keyed on the bare object names matching: ``reports`` and ``s.other`` are + unrelated tables that happen to share a model name, not a repair. + """ + before, after = persisted.sql_table, fresh.sql_table + if not before or not after: + return None + if "." in before or "." not in after: + return None + if _bare_table_name(after) != before: + return None + return f"{before} → {after}" + + +class ProcessTableOutcome(BaseModel): + """What the additive pass did with one freshly-introspected model. + + A skip ("I declined this one") is not an error ("this failed to persist"), + so it travels separately and is reported separately. + """ + + addition: Any | None = None + skipped: SkippedTable | None = None + + +def _cross_schema_conflict( + *, + model_name: str, + persisted: SlayerModel, + fresh: SlayerModel, + default_schema_objects: set[str], +) -> SkippedTable | None: + """Refuse to merge two different schemas' tables into one model. + + Two sequential single-schema ingests would otherwise fuse them, with no + flag involved. The second check covers the case a schema comparison alone + cannot: a default-schema model is persisted *unqualified*, so a fresh + qualified object with the same bare name looks like a repair when it is + actually a different table. + """ + persisted_table = persisted.sql_table or "" + fresh_table = fresh.sql_table or "" + persisted_schema = _schema_of(persisted_table) + fresh_schema = _schema_of(fresh_table) + if not fresh_schema: + return None + + conflicting = persisted_schema is not None and persisted_schema != fresh_schema + shadows_default = ( + persisted_schema is None and persisted_table in default_schema_objects + ) + if not (conflicting or shadows_default): + return None + return SkippedTable( + table_name=fresh_table, + kind=fresh.source_kind, + reason=( + f"cross-schema conflict: model '{model_name}' is bound to " + f"'{persisted_table}', and '{fresh_table}' is a different table; " + f"ingest the schemas separately or use --exclude" + ), ) @@ -1341,62 +1886,89 @@ async def _process_one_table( fresh: SlayerModel, datasource: DatasourceConfig, storage: StorageBackend, -): - """Save / merge one freshly-introspected model, returning the - ``ModelAddition`` to record. Raises on persistence failure — the caller - isolates errors per-model. + default_schema_objects: set[str] | None = None, +) -> ProcessTableOutcome: + """Save / merge one freshly-introspected model. Raises on persistence + failure — the caller isolates errors per-model. """ from slayer.engine.schema_drift import ModelAddition persisted = await storage.get_model(table_name, data_source=datasource.name) if persisted is None: await storage.save_model(fresh) - return ModelAddition( - model_name=table_name, - data_source=datasource.name, - created=True, - new_columns=[c.name for c in fresh.columns], - new_joins=[j.target_model for j in fresh.joins], - source_kind=fresh.source_kind, + return ProcessTableOutcome( + addition=ModelAddition( + model_name=table_name, + data_source=datasource.name, + created=True, + new_columns=[c.name for c in fresh.columns], + new_joins=[j.target_model for j in fresh.joins], + source_kind=fresh.source_kind, + ) ) if persisted.sql or persisted.source_queries: # User-authored sql / query-backed model with the matching name — # leave it alone. - return None + return ProcessTableOutcome() + + conflict = _cross_schema_conflict( + model_name=table_name, + persisted=persisted, + fresh=fresh, + default_schema_objects=default_schema_objects or set(), + ) + if conflict is not None: + return ProcessTableOutcome(skipped=conflict) + outcome = _additive_merge_existing( persisted=persisted, fresh=fresh, sqlite_widen_enabled=(datasource.type or "").lower() == "sqlite", ) - # ``kind_changed`` must gate the save too: a view→table flip - # usually adds no columns and no joins, so without it the refreshed model - # would be computed and then thrown away. + # ``kind_changed`` and ``sql_table_change`` must gate the save too: a + # view→table flip or a qualifier repair usually adds no columns and no + # joins, so without them the refreshed model would be computed and then + # thrown away. if ( outcome.new_columns or outcome.new_joins or outcome.widened_columns or outcome.kind_changed + or outcome.sql_table_change ): await storage.save_model(outcome.merged) kind_change = None if outcome.kind_changed: before = persisted.source_kind or "unknown" kind_change = f"{before} → {fresh.source_kind}" - return ModelAddition( - model_name=table_name, - data_source=datasource.name, - created=False, - new_columns=outcome.new_columns, - new_joins=outcome.new_joins, - widened_columns=outcome.widened_columns, - source_kind=outcome.merged.source_kind, - kind_change=kind_change, + return ProcessTableOutcome( + addition=ModelAddition( + model_name=table_name, + data_source=datasource.name, + created=False, + new_columns=outcome.new_columns, + new_joins=outcome.new_joins, + widened_columns=outcome.widened_columns, + source_kind=outcome.merged.source_kind, + kind_change=kind_change, + sql_table_change=outcome.sql_table_change, + ) ) def _bare_table_name(sql_table: str) -> str: """Strip an optional schema prefix from a ``schema.table`` reference.""" - return sql_table.split(".", 1)[1] if "." in sql_table else sql_table + return split_sql_table(sql_table)[1] + + +def _schema_of(sql_table: str) -> str | None: + """The bare schema segment of a ``sql_table``, or None when unqualified. + + Built on :func:`split_sql_table` so there is one dotted-name parser, not + two that disagree about three-part names. + """ + schema_token, _ = split_sql_table(sql_table) + return _bare_schema(schema_token) if schema_token else None async def _scoped_models_for_validation( @@ -1435,6 +2007,8 @@ async def ingest_datasource_idempotent( exclude_tables: list[str] | None = None, schema: str | None = None, include_views: bool = True, + schemas: list[str] | None = None, + all_schemas: bool = False, ): """Idempotent re-ingestion. @@ -1460,6 +2034,8 @@ async def ingest_datasource_idempotent( additions: list[ModelAddition] = [] errors: list[IngestionError] = [] + # Rejected before the scan so a conflicting call never opens a connection. + _resolve_scope_args(schema=schema, schemas=schemas, all_schemas=all_schemas) # ``ingest_datasource_report`` is sync (it drives SQLAlchemy ``Inspector``). # Offload to a thread so a slow / large datasource doesn't block the @@ -1471,8 +2047,12 @@ async def ingest_datasource_idempotent( exclude_tables=exclude_tables, schema=schema, include_views=include_views, + schemas=schemas, + all_schemas=all_schemas, ) fresh_models = scan.models + skipped = list(scan.skipped) + default_schema_objects = set(scan.default_schema_objects) fresh_by_name = {m.name: m for m in fresh_models} # Keyed on the LIVE OBJECT name, not the model name. ``_scoped_models_for_validation`` # compares this against ``_bare_table_name(m.sql_table)``, so using model @@ -1485,14 +2065,17 @@ async def ingest_datasource_idempotent( for table_name, fresh in fresh_by_name.items(): try: - addition = await _process_one_table( + outcome = await _process_one_table( table_name=table_name, fresh=fresh, datasource=datasource, storage=storage, + default_schema_objects=default_schema_objects, ) - if addition is not None: - additions.append(addition) + if outcome.addition is not None: + additions.append(outcome.addition) + if outcome.skipped is not None: + skipped.append(outcome.skipped) except Exception as exc: # noqa: BLE001 — best-effort per-model isolation errors.append( IngestionError( @@ -1545,8 +2128,9 @@ async def ingest_datasource_idempotent( additions=additions, to_delete=list(to_delete), errors=errors, - skipped=scan.skipped, + skipped=skipped, objects=scan.objects, + schema_hint=scan.schema_hint, ) @@ -1639,7 +2223,16 @@ def _print_ingest_addition( return widened = getattr(addition, "widened_columns", []) or [] kind_change = getattr(addition, "kind_change", None) - if not (addition.new_columns or addition.new_joins or widened or kind_change): + # A qualifier repair adds no columns, so without it in the gate the whole + # line — the point of the re-ingest — would print nothing at all. + sql_table_change = getattr(addition, "sql_table_change", None) + if not ( + addition.new_columns + or addition.new_joins + or widened + or kind_change + or sql_table_change + ): return details = [] if addition.new_columns: @@ -1650,6 +2243,8 @@ def _print_ingest_addition( details.append(f"widened: {', '.join(widened)}") if kind_change: details.append(f"source_kind: {kind_change}") + if sql_table_change: + details.append(f"sql_table: {sql_table_change}") print(f"Updated: {addition.model_name} ({'; '.join(details)})", file=out) diff --git a/slayer/engine/introspect_utils.py b/slayer/engine/introspect_utils.py index 17ea153d..7d450b7a 100644 --- a/slayer/engine/introspect_utils.py +++ b/slayer/engine/introspect_utils.py @@ -13,7 +13,8 @@ from __future__ import annotations -from typing import Dict, List, Optional +from pathlib import Path +from typing import Any, Dict, List, Optional import sqlalchemy as sa @@ -70,52 +71,200 @@ def _parse_info_schema_is_float(data_type_str: str) -> bool: return True # No precision/scale info, default to float +def split_sql_table(sql_table: str) -> tuple[Optional[str], str]: + """Split a ``sql_table`` reference into ``(schema_token, object_name)``. + + The schema token is everything before the FINAL dot, catalog segment + included — ``proj.dataset.tbl`` yields ``("proj.dataset", "tbl")``. + Truncating it would introspect the wrong catalog, or (on DuckDB) match + nothing at all. + """ + schema_token, sep, obj = sql_table.rpartition(".") + if not sep: + return None, sql_table + return (schema_token or None), obj + + +def split_schema_token(token: str) -> tuple[Optional[str], str]: + """Split a discovery token into ``(catalog, schema)``. + + Discovery tokens are at most ``catalog.schema``, so this splits on the + FIRST dot — the mirror image of :func:`split_sql_table`. + """ + catalog, sep, schema = token.partition(".") + if not sep: + return None, token + return (catalog or None), schema + + +_UNSET = object() +_DEFAULT_SCHEMA_ATTR = "_slayer_default_schema_token" + + +def _current_catalog(inspector: sa.engine.Inspector) -> Optional[str]: + """The catalog (database) the connection is currently attached to.""" + try: + with inspector.engine.connect() as conn: + catalog = conn.exec_driver_sql("SELECT current_database()").scalar() + if isinstance(catalog, str) and catalog: + return catalog + except Exception: # noqa: BLE001 — probe only; the URL stem is the fallback + pass + try: + database = inspector.engine.url.database + except Exception: # noqa: BLE001 + return None + return Path(database).stem if database else None + + +def qualified_default_schema( + inspector: sa.engine.Inspector, +) -> Optional[str]: + """The discovery token identifying the connection's default schema. + + DuckDB's ``get_schema_names()`` always returns catalog-qualified tokens + (``fda.main``), and the qualified form is the *safe* one: a bare ``main`` + makes ``get_table_names`` and ``has_table`` reach into ``ATTACH``ed + catalogs. So the token is matched against what the dialect actually + enumerates, and returned in exactly that shape. Dialects that enumerate + bare names get their bare default back unchanged. + + Returns ``None`` when the dialect reports no default schema, which callers + treat as "pass ``None`` to the accessors", i.e. today's behaviour. + """ + cached = getattr(inspector, _DEFAULT_SCHEMA_ATTR, _UNSET) + if cached is not _UNSET: + return cached if isinstance(cached, str) else None + token = _compute_default_schema_token(inspector) + try: + setattr(inspector, _DEFAULT_SCHEMA_ATTR, token) + except Exception: # noqa: BLE001 — caching is an optimisation, not a contract + pass + return token + + +def _compute_default_schema_token( + inspector: sa.engine.Inspector, +) -> Optional[str]: + try: + default = inspector.default_schema_name + except Exception: # noqa: BLE001 — a dialect may not implement it + return None + if not isinstance(default, str) or not default: + return None + try: + names = list(inspector.get_schema_names() or []) + except Exception: # noqa: BLE001 — enumeration is best-effort + return default + if default in names: + return default + catalog = _current_catalog(inspector) + if catalog and f"{catalog}.{default}" in names: + return f"{catalog}.{default}" + # A dialect that qualifies its tokens but whose current catalog we could + # not determine: accept a unique last-segment match, never an ambiguous one. + matches = [n for n in names if n.rsplit(".", 1)[-1] == default] + return matches[0] if len(matches) == 1 else default + + +def _info_schema_column(col_name: str, data_type_str: str) -> Dict: + """Map one ``INFORMATION_SCHEMA.columns`` row to SLayer's column shape.""" + # Strip precision info (e.g. "DECIMAL(10,2)" → "DECIMAL") + base_type = data_type_str.split("(")[0].upper().strip() + sa_type = _INFO_SCHEMA_TYPE_MAP.get(base_type) + is_float = base_type in _FLOAT_LIKE_INFO_SCHEMA_TYPES + # NUMERIC/DECIMAL: check scale to decide float vs integer + if base_type in ("NUMERIC", "DECIMAL") or ( + sa_type is None and ("DECIMAL" in base_type or "NUMERIC" in base_type) + ): + sa_type = sa_type or DataType.DOUBLE + is_float = _parse_info_schema_is_float(data_type_str) + elif sa_type is None and "INT" in base_type: + # DEV-1361: integer-shaped types should narrow to INT, not the + # coarse DOUBLE fallback (e.g. MEDIUMINT, TINYINT variants not + # otherwise mapped). + sa_type = DataType.INT + elif sa_type is None and ("CHAR" in base_type or "TEXT" in base_type): + sa_type = DataType.TEXT + return {"name": col_name, "type": sa_type or DataType.TEXT, "is_float": is_float} + + +def _columns_in_schema( + sa_engine: sa.Engine, table_name: str, schema: str, +) -> List[Dict]: + """Columns of ``schema.table_name``, filtered on the catalog too. + + ``information_schema.columns`` carries ``table_catalog``, and it is + populated for ``ATTACH``ed catalogs — so a catalog-qualified token filters + exactly, instead of matching nothing (which silently produced a + column-less model) or matching everywhere (which unioned two tables' + columns together). + """ + catalog, bare_schema = split_schema_token(schema) + clauses = ["table_name = :table_name", "table_schema = :schema"] + params: Dict[str, Any] = {"table_name": table_name, "schema": bare_schema} + if catalog is not None: + clauses.append("table_catalog = :catalog") + params["catalog"] = catalog + sql = ( + "SELECT column_name, data_type " + "FROM information_schema.columns " + "WHERE " + " AND ".join(clauses) + " " + "ORDER BY ordinal_position" + ) + with sa_engine.connect() as conn: + rows = conn.execute(sa.text(sql), params).fetchall() + return [_info_schema_column(name, type_str) for name, type_str in rows] + + def _get_columns_fallback( sa_engine: sa.Engine, table_name: str, schema: Optional[str], + *, + default_schema: Optional[str] = None, ) -> List[Dict]: - """Get columns via INFORMATION_SCHEMA when Inspector.get_columns() fails.""" + """Get columns via INFORMATION_SCHEMA when Inspector.get_columns() fails. + + On DuckDB this is not a rare backstop — ``Inspector.get_columns`` raises + for every schema — so it is the primary column path for a Tier-1 dialect. + + With no ``schema`` the query cannot be narrowed, so same-named tables in + two schemas both match. Rather than unioning their columns (which produced + models referencing columns their table does not have) the rows are grouped + by catalog+schema: one group is used, ``default_schema`` breaks a tie, and + anything still ambiguous raises. Picking a winner by sort order would + replace union corruption with wrong-table corruption, which is harder to + notice; the caller isolates the raise per object and reports a skip. + """ if schema: - sql = ( - "SELECT column_name, data_type " - "FROM information_schema.columns " - "WHERE table_name = :table_name " - "AND table_schema = :schema " - "ORDER BY ordinal_position" - ) - params = {"table_name": table_name, "schema": schema} - else: - sql = ( - "SELECT column_name, data_type " - "FROM information_schema.columns " - "WHERE table_name = :table_name " - "ORDER BY ordinal_position" - ) - params = {"table_name": table_name} + return _columns_in_schema(sa_engine, table_name, schema) + + sql = ( + "SELECT table_catalog, table_schema, column_name, data_type " + "FROM information_schema.columns " + "WHERE table_name = :table_name " + "ORDER BY table_catalog, table_schema, ordinal_position" + ) with sa_engine.connect() as conn: - rows = conn.execute(sa.text(sql), params).fetchall() - result = [] - for col_name, data_type_str in rows: - # Strip precision info (e.g. "DECIMAL(10,2)" → "DECIMAL") - base_type = data_type_str.split("(")[0].upper().strip() - sa_type = _INFO_SCHEMA_TYPE_MAP.get(base_type) - is_float = base_type in _FLOAT_LIKE_INFO_SCHEMA_TYPES - # NUMERIC/DECIMAL: check scale to decide float vs integer - if base_type in ("NUMERIC", "DECIMAL") or ( - sa_type is None and ("DECIMAL" in base_type or "NUMERIC" in base_type) - ): - sa_type = sa_type or DataType.DOUBLE - is_float = _parse_info_schema_is_float(data_type_str) - elif sa_type is None and "INT" in base_type: - # DEV-1361: integer-shaped types should narrow to INT, not the - # coarse DOUBLE fallback (e.g. MEDIUMINT, TINYINT variants not - # otherwise mapped). - sa_type = DataType.INT - elif sa_type is None and ("CHAR" in base_type or "TEXT" in base_type): - sa_type = DataType.TEXT - result.append({"name": col_name, "type": sa_type or DataType.TEXT, "is_float": is_float}) - return result + rows = conn.execute(sa.text(sql), {"table_name": table_name}).fetchall() + + by_token: Dict[str, List[Dict]] = {} + for catalog, schema_name, col_name, data_type_str in rows: + token = f"{catalog}.{schema_name}" if catalog else schema_name + by_token.setdefault(token, []).append( + _info_schema_column(col_name, data_type_str) + ) + if not by_token: + return [] + if len(by_token) == 1: + return next(iter(by_token.values())) + if default_schema in by_token: + return by_token[default_schema] + raise ValueError( + f"Column lookup for {table_name!r} is ambiguous: it exists in " + f"{', '.join(sorted(by_token))}. Pass an explicit schema." + ) def _safe_get_columns( @@ -124,8 +273,19 @@ def _safe_get_columns( table_name: str, schema: Optional[str], ) -> List[Dict]: - """Get columns, falling back to INFORMATION_SCHEMA on failure.""" + """Get columns, falling back to INFORMATION_SCHEMA on failure. + + Resolves ``schema=None`` to the connection's default schema token before + falling back, so the schema-blind query never runs for a dialect that + reports a default. + """ try: return inspector.get_columns(table_name, schema=schema) except Exception: - return _get_columns_fallback(sa_engine, table_name, schema) + default_token = qualified_default_schema(inspector) + return _get_columns_fallback( + sa_engine, + table_name, + schema if schema is not None else default_token, + default_schema=default_token, + ) diff --git a/slayer/engine/schema_drift.py b/slayer/engine/schema_drift.py index 5e1478cb..f0339077 100644 --- a/slayer/engine/schema_drift.py +++ b/slayer/engine/schema_drift.py @@ -15,6 +15,7 @@ import asyncio import logging +from collections import Counter from typing import ( Annotated, Any, @@ -42,7 +43,7 @@ ) from slayer.core.query import SlayerQuery from slayer.sql.sql_predicate import parse_sql_predicate -from slayer.engine.introspect_utils import _safe_get_columns +from slayer.engine.introspect_utils import _safe_get_columns, split_sql_table from slayer.engine.ingestion import ( _safe_get_pk_constraint, _sa_type_is_float, @@ -117,6 +118,9 @@ class ModelAddition(BaseModel): # Human-readable transition (e.g. "view → table") when a re-ingest found # the live object had changed kind. None when nothing changed. kind_change: str | None = None + # Human-readable transition (e.g. "reports → openfda_rest.reports") when a + # re-ingest repaired a missing schema qualifier. None when nothing changed. + sql_table_change: str | None = None class IngestionError(BaseModel): @@ -143,6 +147,10 @@ class IdempotentIngestResult(BaseModel): # circular import with ``engine.ingestion``; runtime entries are # ``IngestableObject``. objects: list[Any] = Field(default_factory=list) + # Set when exactly one schema was scanned and the datasource holds others. + # Travels in the response body so REST / MCP callers see the same nudge + # the CLI prints. Advisory — never an error. + schema_hint: str | None = None class AppliedEntry(BaseModel): @@ -1655,14 +1663,36 @@ def compute_datasource_drops( # =========================================================================== +def _alias_keys(schema_token: str | None, name: str) -> list[str]: + """Progressively shorter lookup keys for one live object, longest first. + + A model written before the catalog was known says ``schema.table``; one + written before schemas were recorded at all says just ``table``. Both must + keep resolving, so both aliases are offered — but only inserted when they + are unambiguous across everything scanned. + """ + if not schema_token: + return [name] + return [f"{schema_token.rsplit('.', 1)[-1]}.{name}", name] + + def _live_schema_for_datasource( *, datasource: DatasourceConfig, - schema: str | None = None, + schemas: list[str | None] | None = None, ) -> dict[str, LiveTable]: - """Return ``{object_name: LiveTable}`` for every live table AND view in - the DS, using SQLAlchemy ``Inspector`` and the same fallback path as - auto-ingestion (``slayer/engine/ingestion.py``). + """Return ``{object_key: LiveTable}`` for every live table AND view in the + listed schemas, using SQLAlchemy ``Inspector`` and the same fallback path + as auto-ingestion (``slayer/engine/ingestion.py``). + + Keys are the FULL discovery identity ``"."``, + catalog segment included, plus shorter aliases inserted only when unique + across everything scanned. On a clash the ambiguous alias is dropped, so + a lookup misses rather than silently resolving to another catalog's + same-named table. + + ``schemas=None`` means the connection's default schema, matching the + previous single-schema signature. Views are included **unconditionally** — there is deliberately no ``include_views`` parameter here, and adding one would be a bug. @@ -1681,29 +1711,43 @@ def _live_schema_for_datasource( sa_engine = engine_factory.get_engine(datasource.resolve_env_vars()) try: inspector = sa.inspect(sa_engine) - table_names = [ - o.name - for o in list_ingestable_objects( + entries: list[tuple[str | None, str, LiveTable]] = [] + for schema in schemas if schemas is not None else [None]: + for obj in list_ingestable_objects( inspector=inspector, schema=schema, include_views=True - ) - ] + ): + try: + entries.append(( + obj.schema, + obj.name, + _introspect_one_table( + inspector=inspector, + sa_engine=sa_engine, + table_name=obj.name, + schema=obj.schema, + ), + )) + except Exception as exc: + logger.warning( + "validate_models: failed to introspect %r in datasource " + "%r: %s", + obj.name, + datasource.name, + exc, + ) + out: dict[str, LiveTable] = {} - for table_name in table_names: - try: - out[table_name] = _introspect_one_table( - inspector=inspector, - sa_engine=sa_engine, - table_name=table_name, - schema=schema, - ) - except Exception as exc: - logger.warning( - "validate_models: failed to introspect %r in datasource " - "%r: %s", - table_name, - datasource.name, - exc, - ) + for schema_token, name, live in entries: + out[f"{schema_token}.{name}" if schema_token else name] = live + alias_counts: Counter[str] = Counter( + alias + for schema_token, name, _ in entries + for alias in _alias_keys(schema_token, name) + ) + for schema_token, name, live in entries: + for alias in _alias_keys(schema_token, name): + if alias_counts[alias] == 1 and alias not in out: + out[alias] = live return out finally: # Same rationale as ``ingest_datasource``: this is a one-shot @@ -1822,14 +1866,22 @@ def _strip_ident_quotes(ident: str) -> str: def _resolve_live_table( *, sql_table: str, live_tables: dict[str, LiveTable] ) -> LiveTable | None: - """Look up a model's ``sql_table`` in the live introspection map, - falling back to the bare name when the persisted value is schema- - qualified (``schema.table``) and unquoting double-quoted identifiers - (e.g. ``prod."Company"`` for case-sensitive Postgres tables). + """Look up a model's ``sql_table`` in the live introspection map, walking + progressively shorter keys and unquoting double-quoted identifiers (e.g. + ``prod."Company"`` for case-sensitive Postgres tables). + + Order matters: full identity, then the last two segments, then the bare + name. The live map drops ambiguous short keys, so a miss on a shorter + candidate means "this could be either table" — and returning None there + is correct, where returning an arbitrary match would diff a model against + the wrong table. """ candidates = [sql_table] - if "." in sql_table: - candidates.append(sql_table.split(".", 1)[1]) + parts = sql_table.split(".") + if len(parts) > 2: + candidates.append(".".join(parts[-2:])) + if len(parts) > 1: + candidates.append(parts[-1]) # Materialise the snapshot before extending — a bare generator # ``(_strip_ident_quotes(c) for c in candidates)`` would iterate the # list lazily WHILE ``extend`` appends to it, so every appended item @@ -2060,13 +2112,22 @@ async def _collect_sql_table_diffs( """ if not sql_table_models: return {} - # Honour the datasource's configured schema_name so non-default-schema - # datasources diff against the right table set; otherwise SQLAlchemy - # introspects the default and produces false WholeModelDeletes. + # The schema set is derived from the models being validated, plus the + # datasource's configured default. Introspecting only the default schema + # made every non-default-schema model unresolvable, and an unresolvable + # model is a ``WholeModelDelete`` that ``validate-models --force-clean`` + # acts on — so getting this set wrong is a data-loss path, not a + # false-positive nuisance. + schemas: set[str | None] = { + split_sql_table(m.sql_table)[0] for m in sql_table_models if m.sql_table + } + schemas.discard(None) + schemas.add(datasource.schema_name or None) live_tables = await asyncio.to_thread( _live_schema_for_datasource, datasource=datasource, - schema=datasource.schema_name or None, + # ``None`` (the connection default) first, then a stable order. + schemas=sorted(schemas, key=lambda s: (s is not None, s or "")), ) probe_drifts_by_model = await _sqlite_probe_drifts_for_models( datasource=datasource, diff --git a/slayer/mcp/server.py b/slayer/mcp/server.py index 8c45360b..90e570c3 100644 --- a/slayer/mcp/server.py +++ b/slayer/mcp/server.py @@ -115,6 +115,16 @@ def _fetch_tables( return None, str(e) +def _csv_arg(value: str) -> list[str] | None: + """Split a comma-separated tool argument, or None when it is empty. + + MCP tool arguments are flat scalars, so lists travel as CSV — matching + ``include_tables``' existing style rather than adding a second convention. + """ + items = [part.strip() for part in (value or "").split(",")] + return [item for item in items if item] or None + + def _empty_ingest_message(*, schema_name: str, ds: DatasourceConfig) -> str: """Agent-facing wrapper over the shared engine renderer.""" return _shared_empty_ingest_message( @@ -1311,6 +1321,8 @@ async def create_datasource( connection_string: str | None = None, schema_name: str | None = None, auto_ingest: bool = True, + schemas: str = "", + all_schemas: bool = False, ) -> str: """Create a database connection, verify it, and auto-ingest models. Use ${ENV_VAR} syntax in credentials to reference environment variables. @@ -1323,13 +1335,26 @@ async def create_datasource( username: Database username. password: Database password. connection_string: Full connection string as alternative to individual fields. - schema_name: Default schema name. Also used as the schema for auto-ingestion. + schema_name: Default schema name. Persisted, and used as the schema for auto-ingestion. auto_ingest: Automatically ingest models from the database schema (default: true). Set to false to skip. + schemas: Comma-separated schemas to ingest. Alternative to schema_name; do not set both. Not persisted. + all_schemas: Ingest every non-system schema in the database. Do not combine with the other two. Example: create_datasource(name="mydb", type="postgres", host="localhost", port=5432, database="app", username="user", password="pass") """ + from slayer.engine.ingestion import _resolve_scope_args from slayer.engine.ingestion import ingest_datasource as _ingest + schema_list = _csv_arg(schemas) + try: + _resolve_scope_args( + schema=schema_name, + schemas=schema_list, + all_schemas=all_schemas, + ) + except ValueError as exc: + return f"Cannot create datasource: {exc}" + data = _build_dict( name=name, type=type, @@ -1358,9 +1383,15 @@ async def create_datasource( if not auto_ingest: return "\n".join(lines) - # Auto-ingest models + # Auto-ingest models. The scope is passed explicitly rather than left + # to the persisted ``schema_name``, so the two can never be read as a + # conflict. try: - models = _ingest(datasource=ds, schema=schema_name or None) + models = _ingest( + datasource=ds, + schemas=schema_list or ([schema_name] if schema_name else None), + all_schemas=all_schemas, + ) except Exception as e: if isinstance(e, (sa.exc.OperationalError, sa.exc.DatabaseError)): lines.append(f"Auto-ingestion failed: {_friendly_db_error(e)}") @@ -1380,9 +1411,9 @@ async def create_datasource( if not models and not save_errors: lines.append("No tables found to ingest.") - schemas = _get_schemas(ds) - if schemas: - lines.append(f"Available schemas: {', '.join(schemas)}") + available = _get_schemas(ds) + if available: + lines.append(f"Available schemas: {', '.join(available)}") elif models: lines.append(f"Ingested {len(models)} model(s):") for m in models: @@ -1659,7 +1690,13 @@ async def delete_datasource(name: str) -> str: # ----------------------------------------------------------------------- @mcp.tool() - async def ingest_datasource_models(datasource_name: str, include_tables: str = "", schema_name: str = "") -> str: + async def ingest_datasource_models( + datasource_name: str, + include_tables: str = "", + schema_name: str = "", + schemas: str = "", + all_schemas: bool = False, + ) -> str: """Auto-discover tables in a database and create / additively update semantic models from them. Idempotent (DEV-1356): re-runs are additive only. New columns and joins @@ -1670,21 +1707,38 @@ async def ingest_datasource_models(datasource_name: str, include_tables: str = " Args: datasource_name: Name of an existing datasource (from list_datasources). include_tables: Comma-separated list of table names to include. If empty, all tables are ingested. - schema_name: Database schema to inspect (e.g. "public"). If empty, uses the default schema. + schema_name: A single database schema to inspect (e.g. "public"). If empty, uses the default schema. + schemas: Comma-separated schemas to inspect. Alternative to schema_name; do not set both. + all_schemas: Inspect every non-system schema in the current database. Do not combine with the other two. """ - from slayer.engine.ingestion import ingest_datasource_idempotent + from slayer.engine.ingestion import ( + _resolve_scope_args, + ingest_datasource_idempotent, + ) ds = await storage.get_datasource(datasource_name) if ds is None: return f"Datasource '{datasource_name}' not found." + schema_list = _csv_arg(schemas) + try: + _resolve_scope_args( + schema=schema_name or None, + schemas=schema_list, + all_schemas=all_schemas, + ) + except ValueError as exc: + return f"Cannot ingest: {exc}" + try: - include = [t.strip() for t in include_tables.split(",") if t.strip()] or None + include = _csv_arg(include_tables) result = await ingest_datasource_idempotent( datasource=ds, storage=storage, include_tables=include, schema=schema_name or None, + schemas=schema_list, + all_schemas=all_schemas, ) except Exception as e: if isinstance(e, (sa.exc.OperationalError, sa.exc.DatabaseError)): diff --git a/slayer/storage/type_refinement.py b/slayer/storage/type_refinement.py index eae213da..aa15f0d3 100644 --- a/slayer/storage/type_refinement.py +++ b/slayer/storage/type_refinement.py @@ -30,6 +30,7 @@ from slayer.core.enums import DataType from slayer.core.models import DatasourceConfig +from slayer.engine.introspect_utils import split_sql_table logger = logging.getLogger(__name__) @@ -138,12 +139,14 @@ def _parse_sql_table_with_default_schema( """Split ``sql_table`` into ``(schema, table)``, falling back to ``datasource.schema_name`` when the name is unqualified. This honours attached SQLite schemas instead of silently using ``main``. + + The schema is everything before the FINAL dot, so a hand-written + Snowflake ``db.schema.table`` or BigQuery ``project.dataset.table`` keeps + its catalog rather than losing it to a split on the first dot. """ default_schema = getattr(datasource, "schema_name", None) or None - if "." in sql_table: - schema_name, _, table_name = sql_table.partition(".") - return (schema_name or None), table_name - return default_schema, sql_table + schema_name, table_name = split_sql_table(sql_table) + return (schema_name if schema_name is not None else default_schema), table_name def _safe_probe( @@ -319,10 +322,15 @@ def refine_dict_with_live_schema(d: dict, datasource: DatasourceConfig) -> bool: return False # Local import to avoid circular import at module load time. - from slayer.engine.schema_drift import _live_schema_for_datasource + from slayer.engine.schema_drift import ( + _live_schema_for_datasource, + _resolve_live_table, + ) - live = _live_schema_for_datasource(datasource=datasource) - table = live.get(sql_table) + live = _live_schema_for_datasource( + datasource=datasource, schemas=[split_sql_table(sql_table)[0]], + ) + table = _resolve_live_table(sql_table=sql_table, live_tables=live) if table is None: return False live_columns = table.columns diff --git a/tests/test_ingestion.py b/tests/test_ingestion.py index 591807a5..50fda67d 100644 --- a/tests/test_ingestion.py +++ b/tests/test_ingestion.py @@ -52,7 +52,16 @@ class TestGetColumnsFallback: """Tests for _get_columns_fallback parameterized queries.""" def test_without_schema(self): - engine, conn = _setup_mock_engine([("id", "INTEGER"), ("name", "VARCHAR")]) + """With no schema the query cannot be narrowed, so it selects the + catalog and schema alongside the columns and groups the rows by them. + Unioning every match — which is what selecting only the columns did — + produced models referencing columns their table does not have.""" + engine, conn = _setup_mock_engine( + [ + ("db", "public", "id", "INTEGER"), + ("db", "public", "name", "VARCHAR"), + ] + ) result = _get_columns_fallback(sa_engine=engine, table_name="orders", schema=None) assert len(result) == 2 @@ -65,7 +74,8 @@ def test_without_schema(self): assert isinstance(sql_text, sa.TextClause) sql_str = str(sql_text) assert ":table_name" in sql_str - assert "table_schema" not in sql_str + # No schema was supplied, so none may be bound as a filter. + assert ":schema" not in sql_str params = args[1] if len(args) > 1 else kwargs assert params == {"table_name": "orders"} diff --git a/tests/test_ingestion_schema_qualification.py b/tests/test_ingestion_schema_qualification.py new file mode 100644 index 00000000..f84a5a90 --- /dev/null +++ b/tests/test_ingestion_schema_qualification.py @@ -0,0 +1,1810 @@ +"""Ingested models must keep enough schema information to be queryable. + +A bare ``slayer ingest`` against DuckDB swept *every* schema and wrote each +object's bare name into ``sql_table``, so a model in a non-default schema +generated ``FROM reports`` and failed with a table-not-found error. Models in +the connection's default schema resolved via the search path, which is what +made the breakage look partial rather than systemic. + +Three defects are pinned here: + +* **D1** — cross-schema discovery with no schema recorded. +* **D2** — same-named tables in two schemas silently merged their columns, + because the ``INFORMATION_SCHEMA`` column fallback ran unfiltered. +* **D3** — ``datasources create --schema X --ingest`` discarded ``schema_name`` + while ``validate-models`` read it back. + +The token discipline these tests enforce is easy to get backwards, so it is +worth stating: on DuckDB ``get_schema_names()`` returns **catalog-qualified** +tokens, and the qualified token is the *safe* one. Measured, a bare ``main`` +token makes ``get_table_names`` and ``has_table`` reach into ``ATTACH``ed +catalogs and makes the column fallback return the union across catalogs. So the +discovery token stays qualified end to end, and the column fallback filters on +``table_catalog`` as well as ``table_schema``. Separately — and this is a +different string — the qualifier written into ``sql_table`` is the bare last +segment, because the connection's current catalog is already the right one. +""" +from __future__ import annotations + +import io +import sqlite3 +import sys +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +import sqlalchemy as sa +from fastapi.testclient import TestClient +from pydantic import BaseModel +from sqlalchemy.pool import StaticPool + +duckdb = pytest.importorskip("duckdb") + +from slayer.api.server import create_app +from slayer.cli import _run_datasources_create, _run_ingest, main +from slayer.core.enums import DataType +from slayer.core.models import Column, DatasourceConfig, SlayerModel +from slayer.core.query import SlayerQuery +from slayer.engine import ingestion as ingestion_module +from slayer.engine.ingestion import ( + IngestableObject, + IngestSchemaScope, + ProcessTableOutcome, + ResolvedSchema, + SkippedTable, + _additive_merge_existing, + _assign_model_names, + _bare_table_name, + _is_system_schema, + _print_ingest_addition, + _schema_of, + ingest_datasource, + ingest_datasource_idempotent, + ingest_datasource_report, + list_ingestable_objects, + list_ingestable_objects_multi, + qualify_sql_table, + resolve_ingest_schemas, + split_sql_table, +) +from slayer.engine.introspect_utils import _get_columns_fallback, _safe_get_columns +from slayer.engine.query_engine import SlayerQueryEngine +from slayer.engine.schema_drift import ( + LiveTable, + ModelAddition, + WholeModelDelete, + _live_schema_for_datasource, + _resolve_live_table, + validate_datasource, +) +from slayer.mcp.server import create_mcp_server +from slayer.storage.type_refinement import _parse_sql_table_with_default_schema +from slayer.storage.yaml_storage import YAMLStorage + +# --------------------------------------------------------------------------- +# Fixtures — real DuckDB files, unit-scoped (the pattern already used by +# tests/test_cube_js_e2e_duckdb.py). No @pytest.mark.integration. +# --------------------------------------------------------------------------- + + +def _seed_repro(db_path: str) -> None: + """``main.in_default`` + ``openfda_rest.reports`` — the reported shape.""" + con = duckdb.connect(db_path) + con.execute("CREATE TABLE in_default(x INTEGER)") + con.execute("INSERT INTO in_default VALUES (1)") + con.execute("CREATE SCHEMA openfda_rest") + con.execute("CREATE TABLE openfda_rest.reports(id INTEGER, n INTEGER)") + con.execute("INSERT INTO openfda_rest.reports VALUES (1, 10), (2, 20)") + con.close() + + +def _seed_collide(db_path: str) -> None: + """``main.reports(a)`` + ``s2.reports(b, c)`` — one name, two schemas.""" + con = duckdb.connect(db_path) + con.execute("CREATE TABLE reports(a INTEGER)") + con.execute("CREATE SCHEMA s2") + con.execute("CREATE TABLE s2.reports(b INTEGER, c INTEGER)") + con.close() + + +def _seed_three_schemas(db_path: str) -> None: + """``main.only_main`` + ``s2.reports`` + ``s3.reports`` — collision between + two *non-default* schemas, so the default-wins rule cannot decide it.""" + con = duckdb.connect(db_path) + con.execute("CREATE TABLE only_main(z INTEGER)") + con.execute("CREATE SCHEMA s2") + con.execute("CREATE TABLE s2.reports(b INTEGER)") + con.execute("CREATE SCHEMA s3") + con.execute("CREATE TABLE s3.reports(c INTEGER)") + con.close() + + +def _duckdb_ds(db_path: str, *, name: str = "ds", **kw) -> DatasourceConfig: + return DatasourceConfig(name=name, type="duckdb", database=db_path, **kw) + + +def _repro_ds(tmp_path: Path) -> DatasourceConfig: + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + return _duckdb_ds(db_path) + + +def _collide_ds(tmp_path: Path) -> DatasourceConfig: + db_path = str(tmp_path / "collide.duckdb") + _seed_collide(db_path) + return _duckdb_ds(db_path) + + +def _inspector_for(ds: DatasourceConfig) -> tuple[sa.Engine, sa.engine.Inspector]: + eng = sa.create_engine(f"duckdb:///{ds.database}", poolclass=StaticPool) + return eng, sa.inspect(eng) + + +# --- attached-catalog fixture ---------------------------------------------- +# +# The primary file is ``att_main.duckdb`` (so the current catalog is +# ``att_main``) and the attached one is registered as ``aaa`` — deliberately +# sorting BEFORE the default catalog, so a "lowest sorted wins" tie-break would +# pick the wrong one and be caught. + + +def _seed_attached_pair(tmp_path: Path) -> tuple[str, str]: + main_path = str(tmp_path / "att_main.duckdb") + other_path = str(tmp_path / "att_other.duckdb") + + con = duckdb.connect(main_path) + con.execute("CREATE TABLE in_default(x INTEGER)") + con.execute("CREATE TABLE shared(m INTEGER)") + con.execute("CREATE SCHEMA openfda_rest") + con.execute("CREATE TABLE openfda_rest.reports(id INTEGER, n INTEGER)") + con.close() + + con = duckdb.connect(other_path) + con.execute("CREATE TABLE only_in_other(y INTEGER)") + con.execute("CREATE TABLE shared(o INTEGER)") + con.close() + return main_path, other_path + + +def _attached_engine(main_path: str, other_path: str) -> sa.Engine: + """A DuckDB engine on ``main_path`` with ``other_path`` attached as ``aaa``. + + ``StaticPool`` so the single DBAPI connection carrying the ``ATTACH`` + is the one every later ``Inspector`` call reuses. + """ + eng = sa.create_engine(f"duckdb:///{main_path}", poolclass=StaticPool) + with eng.connect() as conn: + conn.exec_driver_sql(f"ATTACH '{other_path}' AS aaa") + conn.commit() + return eng + + +@pytest.fixture +def attached(tmp_path, monkeypatch): + """An attached-catalog datasource whose engine factory always yields a + freshly-attached engine (ingestion disposes the engine it is handed).""" + main_path, other_path = _seed_attached_pair(tmp_path) + ds = _duckdb_ds(main_path) + + def _factory(_ds, *_a, **_kw): + return _attached_engine(main_path, other_path) + + monkeypatch.setattr("slayer.sql.engine_factory.get_engine", _factory) + return SimpleNamespace( + ds=ds, + main_path=main_path, + other_path=other_path, + engine=lambda: _attached_engine(main_path, other_path), + ) + + +def _storage(tmp_path: Path, *, sub: str = "store") -> YAMLStorage: + return YAMLStorage(base_dir=str(tmp_path / sub)) + + +async def _ingest(ds: DatasourceConfig, storage: YAMLStorage, **kw): + await storage.save_datasource(ds) + return await ingest_datasource_idempotent( + datasource=ds, storage=storage, **kw + ) + + +def _by_name(models: list[SlayerModel]) -> dict[str, SlayerModel]: + return {m.name: m for m in models} + + +def _sole_value(resp): + assert resp.row_count == 1, f"expected exactly 1 row: {resp.data}" + return next(iter(resp.data[0].values())) + + +async def _count(engine: SlayerQueryEngine, model_name: str) -> int: + resp = await engine.execute( + SlayerQuery( + source_model=model_name, + measures=[{"formula": "*:count", "name": "cnt"}], + ) + ) + return _sole_value(resp) + + +# --------------------------------------------------------------------------- +# 1-3. Regression / end-to-end +# --------------------------------------------------------------------------- + + +class TestReportedRegression: + async def test_bare_ingest_covers_only_the_default_schema(self, tmp_path): + """Test 1. Bare ingest must resolve to the connection's default schema + only — DuckDB is the one Tier-1 dialect whose ``schema=None`` sweeps + every schema, which is how ``openfda_rest.reports`` got a bare + ``sql_table``.""" + ds = _repro_ds(tmp_path) + models = ingest_datasource(datasource=ds) + + assert _by_name(models).keys() == {"in_default"} + assert _by_name(models)["in_default"].sql_table == "in_default" + + async def test_bare_ingest_reports_the_other_schemas(self, tmp_path): + """Test 1 (cont). Narrowing the scan must not silently lose models — + the user is told which schemas were left out and how to get them.""" + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + scope = resolve_ingest_schemas( + inspector=insp, + requested=None, + all_schemas=False, + datasource_schema=None, + ) + finally: + eng.dispose() + + assert [s.name.rsplit(".", 1)[-1] for s in scope.schemas] == ["main"] + assert scope.schemas[0].is_default is True + assert scope.schemas[0].explicit is False + assert scope.other_schemas == ["openfda_rest"] + + async def test_explicit_schema_qualifies_and_is_queryable(self, tmp_path): + """Test 2. The issue's exact repro: ingest the non-default schema and + run ``*:count`` against it. Before the fix the generated SQL is + ``FROM reports`` and DuckDB raises a catalog error.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + result = await _ingest(ds, storage, schemas=["openfda_rest"]) + assert not result.errors, result.errors + + model = await storage.get_model("reports", data_source="ds") + assert model.sql_table == "openfda_rest.reports" + assert await _count(SlayerQueryEngine(storage=storage), "reports") == 2 + + async def test_all_schemas_qualifies_only_non_default(self, tmp_path): + """Test 3. Default-schema objects stay unqualified so turning the flag + on never rewrites models that already exist; everything else is + qualified. Both must be queryable.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + result = await _ingest(ds, storage, all_schemas=True) + assert not result.errors, result.errors + + in_default = await storage.get_model("in_default", data_source="ds") + reports = await storage.get_model("reports", data_source="ds") + assert in_default.sql_table == "in_default" + assert reports.sql_table == "openfda_rest.reports" + + engine = SlayerQueryEngine(storage=storage) + assert await _count(engine, "in_default") == 1 + assert await _count(engine, "reports") == 2 + + +# --------------------------------------------------------------------------- +# 4-6. D2 — column corruption +# --------------------------------------------------------------------------- + + +class TestColumnCorruption: + async def test_bare_ingest_does_not_union_columns_across_schemas( + self, tmp_path + ): + """Test 4. ``main.reports(a)`` and ``s2.reports(b, c)`` are different + tables. The schema-blind ``INFORMATION_SCHEMA`` query returned all + three columns, producing a model that references columns its table + does not have.""" + ds = _collide_ds(tmp_path) + models = ingest_datasource(datasource=ds) + + reports = _by_name(models)["reports"] + assert [c.name for c in reports.columns] == ["a"] + + def test_safe_get_columns_resolves_none_to_the_default_schema( + self, tmp_path + ): + """Test 5. ``_safe_get_columns`` holds the Inspector, so it can resolve + ``None`` before the fallback ever runs unfiltered.""" + ds = _collide_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + cols = _safe_get_columns(insp, eng, "reports", None) + finally: + eng.dispose() + + assert [c["name"] for c in cols] == ["a"] + + def test_discovery_reports_one_entry_carrying_its_schema(self, tmp_path): + """Test 6. Discovery must return one object tagged with the schema it + was found in, not two indistinguishable bare duplicates.""" + ds = _collide_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + objects = list_ingestable_objects(inspector=insp, schema=None) + finally: + eng.dispose() + + assert [o.name for o in objects] == ["reports"] + assert objects[0].schema.rsplit(".", 1)[-1] == "main" + + +# --------------------------------------------------------------------------- +# 7-9. Collisions +# --------------------------------------------------------------------------- + + +class TestCollisions: + async def test_all_schemas_collision_gives_the_default_schema_the_name( + self, tmp_path + ): + """Test 7. Two schemas claim model name ``reports``. The default + schema wins; the loser is skipped, never suffixed — suffixes shift + with the object set and orphan models.""" + ds = _collide_ds(tmp_path) + report = ingest_datasource_report(datasource=ds, all_schemas=True) + + reports = _by_name(report.models)["reports"] + assert reports.sql_table == "reports" + assert [c.name for c in reports.columns] == ["a"] + + losers = [s for s in report.skipped if "s2" in s.table_name] + assert len(losers) == 1 + assert "collision" in losers[0].reason + + async def test_non_default_collision_is_order_independent(self, tmp_path): + """Test 8. Neither schema is the default, so the winner is fixed by + the schema name itself — not by argument order or listing order.""" + db_path = str(tmp_path / "three.duckdb") + _seed_three_schemas(db_path) + ds = _duckdb_ds(db_path) + + forward = ingest_datasource_report(datasource=ds, schemas=["s2", "s3"]) + reverse = ingest_datasource_report(datasource=ds, schemas=["s3", "s2"]) + + for report in (forward, reverse): + assert _by_name(report.models)["reports"].sql_table == "s2.reports" + assert {s.table_name for s in forward.skipped} == { + s.table_name for s in reverse.skipped + } + + def test_single_schema_sanitization_behaviour_is_unchanged(self, tmp_path): + """Test 9. With one schema in scope the cross-schema tie-break keys + never fire, so ``__`` sanitization stays byte-identical. The real + guard is tests/test_ingestion_name_sanitize.py passing unchanged; + this pins the same rule at the helper.""" + objects = [ + IngestableObject(name="a_b", kind="table", schema="main"), + IngestableObject(name="a__b", kind="table", schema="main"), + ] + assigned, skipped = _assign_model_names(objects) + + assert assigned[("main", "a_b")] == "a_b" + assert ("main", "a__b") not in assigned + assert [s.table_name for s in skipped] == ["a__b"] + + +# --------------------------------------------------------------------------- +# 10-12. Self-heal +# --------------------------------------------------------------------------- + + +def _persisted_reports_model(*, sql_table: str, columns: list[str]) -> SlayerModel: + """A model carrying hand-authored metadata the additive contract must + preserve verbatim through a qualifier repair.""" + return SlayerModel( + name="reports", + data_source="ds", + sql_table=sql_table, + description="hand written", + columns=[ + Column( + name=name, + type=DataType.INT, + description=f"desc for {name}", + label=f"Label {name}", + ) + for name in columns + ], + measures=[{"name": "total_n", "formula": "n:sum"}], + ) + + +class TestSelfHeal: + async def test_missing_qualifier_is_healed_and_metadata_preserved( + self, tmp_path + ): + """Test 10. Re-ingest repairs a *missing* qualifier. It usually changes + no columns and no joins, so the repair has to participate in the + short-circuit or the merged model is computed and discarded.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + await storage.save_model( + _persisted_reports_model(sql_table="reports", columns=["id", "n"]) + ) + + result = await _ingest(ds, storage, schemas=["openfda_rest"]) + + saved = await storage.get_model("reports", data_source="ds") + assert saved.sql_table == "openfda_rest.reports" + assert saved.description == "hand written" + assert [c.description for c in saved.columns] == [ + "desc for id", + "desc for n", + ] + assert [c.label for c in saved.columns] == ["Label id", "Label n"] + assert [m.name for m in saved.measures] == ["total_n"] + + addition = next(a for a in result.additions if a.model_name == "reports") + assert addition.sql_table_change == "reports → openfda_rest.reports" + + async def test_existing_qualifier_is_never_rewritten(self, tmp_path): + """Test 11. Healing adds a qualifier; it never replaces one. A model + deliberately pointed at ``prod.reports`` stays there.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + await storage.save_model( + _persisted_reports_model(sql_table="prod.reports", columns=["id"]) + ) + + await _ingest(ds, storage, schemas=["openfda_rest"]) + + saved = await storage.get_model("reports", data_source="ds") + assert saved.sql_table == "prod.reports" + + async def test_default_schema_model_is_untouched(self, tmp_path): + """Test 12. A default-schema model is persisted unqualified by design, + and re-ingest must not churn it into ``main.in_default``.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + await storage.save_model( + SlayerModel( + name="in_default", + data_source="ds", + sql_table="in_default", + columns=[Column(name="x", type=DataType.INT)], + ) + ) + + result = await _ingest(ds, storage) + + saved = await storage.get_model("in_default", data_source="ds") + assert saved.sql_table == "in_default" + for addition in result.additions: + if addition.model_name == "in_default": + assert addition.sql_table_change is None + assert addition.new_columns == [] + + +# --------------------------------------------------------------------------- +# 13, 27. Cross-schema merge guard +# --------------------------------------------------------------------------- + + +class TestCrossSchemaMergeGuard: + async def test_sequential_single_schema_ingests_do_not_fuse(self, tmp_path): + """Test 13. Two explicit single-schema ingests must not merge two + different physical tables into one model. No new flag is involved — + this fuses today.""" + ds = _collide_ds(tmp_path) + storage = _storage(tmp_path) + await _ingest(ds, storage, schemas=["main"]) + result = await _ingest(ds, storage, schemas=["s2"]) + + saved = await storage.get_model("reports", data_source="ds") + assert [c.name for c in saved.columns] == ["a"] + assert saved.sql_table == "main.reports" + assert any("cross-schema" in s.reason for s in result.skipped), ( + result.skipped + ) + + async def test_bare_persisted_model_is_not_repointed(self, tmp_path): + """Test 27. The hole that qualifying only non-default schemas opens: a + default-schema model is persisted unqualified, so a schema comparison + alone cannot fire. Without the live ``has_table`` probe the self-heal + happily repoints ``reports`` at ``s2.reports``.""" + ds = _collide_ds(tmp_path) + storage = _storage(tmp_path) + await _ingest(ds, storage) # bare -> sql_table: reports, columns [a] + + before = await storage.get_model("reports", data_source="ds") + assert before.sql_table == "reports" + + result = await _ingest(ds, storage, schemas=["s2"]) + + saved = await storage.get_model("reports", data_source="ds") + assert saved.sql_table == "reports" + assert [c.name for c in saved.columns] == ["a"] + assert any("cross-schema" in s.reason for s in result.skipped), ( + result.skipped + ) + + +# --------------------------------------------------------------------------- +# 14-16. validate-models +# --------------------------------------------------------------------------- + + +class TestValidateModels: + async def test_multi_schema_models_are_not_marked_for_deletion( + self, tmp_path + ): + """Test 14. The data-loss guard. A qualified model that the live map + cannot resolve becomes a ``WholeModelDelete``, which + ``validate-models --force-clean`` acts on.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await _ingest(ds, storage, all_schemas=True) + + models = [ + await storage.get_model(n, data_source="ds") + for n in ("in_default", "reports") + ] + to_delete = await validate_datasource(datasource=ds, models=models) + + whole = [e for e in to_delete if isinstance(e, WholeModelDelete)] + assert whole == [], whole + + async def test_qualified_model_diffs_against_its_own_table(self, tmp_path): + """Test 15. With two same-named tables, resolving via the bare-name + fallback can pick the wrong live table and report phantom drift.""" + ds = _collide_ds(tmp_path) + model = SlayerModel( + name="reports_s2", + data_source="ds", + sql_table="s2.reports", + columns=[ + Column(name="b", type=DataType.INT), + Column(name="c", type=DataType.INT), + ], + ) + to_delete = await validate_datasource(datasource=ds, models=[model]) + assert to_delete == [], to_delete + + async def test_view_in_a_non_default_schema_still_resolves(self, tmp_path): + """Test 16. Views are included in the live map unconditionally; the + schema-awareness change must not quietly re-arm the view blindness + that made view-backed models look deleted.""" + db_path = str(tmp_path / "views.duckdb") + con = duckdb.connect(db_path) + con.execute("CREATE SCHEMA analytics") + con.execute("CREATE TABLE analytics.orders(id INTEGER, amount INTEGER)") + con.execute( + "CREATE VIEW analytics.stg_orders AS SELECT id, amount " + "FROM analytics.orders" + ) + con.close() + ds = _duckdb_ds(db_path) + + model = SlayerModel( + name="stg_orders", + data_source="ds", + sql_table="analytics.stg_orders", + columns=[ + Column(name="id", type=DataType.INT), + Column(name="amount", type=DataType.INT), + ], + ) + to_delete = await validate_datasource(datasource=ds, models=[model]) + assert [e for e in to_delete if isinstance(e, WholeModelDelete)] == [] + + +# --------------------------------------------------------------------------- +# 17-19, 33. Parser hardening (dialect-free) +# --------------------------------------------------------------------------- + + +class TestDottedNameParsers: + @pytest.mark.parametrize( + "sql_table,expected", + [("t", "t"), ("s.t", "t"), ("c.s.t", "t")], + ) + def test_bare_table_name_takes_the_last_segment(self, sql_table, expected): + """Test 17. ``split(".", 1)[1]`` returns ``s.t`` for a three-part name, + so every in-scope-table comparison built on it missed.""" + assert _bare_table_name(sql_table) == expected + + @pytest.mark.parametrize( + "sql_table,expected", + [ + ("t", (None, "t")), + ("s.t", ("s", "t")), + ("c.s.t", ("c.s", "t")), + ("proj.dataset.tbl", ("proj.dataset", "tbl")), + ], + ) + def test_split_sql_table_preserves_the_catalog(self, sql_table, expected): + """Tests 18/33. The schema token is everything before the final dot. + Truncating to the last two segments would discard catalog identity — + which on DuckDB matches nothing at all.""" + assert split_sql_table(sql_table) == expected + + def test_parse_with_default_schema_preserves_the_catalog(self): + """Test 33. Snowflake ``db.schema.table`` and BigQuery + ``project.dataset.table`` are hand-writable today, and ``partition`` + split them after the FIRST dot.""" + ds = _duckdb_ds(":memory:", schema_name="fallback") + assert _parse_sql_table_with_default_schema( + "proj.dataset.tbl", ds + ) == ("proj.dataset", "tbl") + assert _parse_sql_table_with_default_schema("s.t", ds) == ("s", "t") + assert _parse_sql_table_with_default_schema("t", ds) == ("fallback", "t") + + def test_schema_of_returns_the_bare_segment(self): + assert _schema_of("t") is None + assert _schema_of("s.t") == "s" + assert _schema_of("c.s.t") == "s" + + def test_resolve_live_table_walks_progressively_shorter_keys(self): + """Test 19. A three-part ``sql_table`` must resolve against a live map + keyed either way — and must return None rather than a wrong match when + the short key is ambiguous.""" + live = LiveTable(columns={"a": DataType.INT}) + assert _resolve_live_table( + sql_table="c.s.t", live_tables={"s.t": live} + ) is live + assert _resolve_live_table( + sql_table="c.s.t", live_tables={"t": live} + ) is live + assert _resolve_live_table( + sql_table="c.s.t", live_tables={"other.t": live} + ) is None + + def test_resolve_live_table_still_unquotes_identifiers(self): + """The existing quoted-identifier path must survive the candidate-list + rewrite (``prod."Company"`` for case-sensitive Postgres).""" + live = LiveTable(columns={"a": DataType.INT}) + assert _resolve_live_table( + sql_table='prod."Company"', live_tables={"Company": live} + ) is live + + +# --------------------------------------------------------------------------- +# 20-22. schema_name persistence +# --------------------------------------------------------------------------- + + +def _create_args(tmp_path: Path, db_path: str, **overrides) -> SimpleNamespace: + base = dict( + connection_string=f"duckdb:///{db_path}", + name="ds", + description=None, + ingest=True, + include=None, + exclude=None, + schema=None, + all_schemas=False, + include_views=True, + yes=True, + storage=str(tmp_path / "store"), + models_dir=None, + ) + base.update(overrides) + return SimpleNamespace(**base) + + +class TestSchemaNamePersistence: + async def test_create_with_schema_persists_it_and_bare_ingest_reuses_it( + self, tmp_path + ): + """Test 20. ``--schema`` was used for the one-shot ingest and thrown + away, while ``validate-models`` read ``schema_name`` back — so the two + commands looked at different schemas. Drives the real CLI entry + points, which is also what pins the persist-then-ingest call order.""" + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + storage = _storage(tmp_path) + + _run_datasources_create( + _create_args(tmp_path, db_path, schema="openfda_rest"), storage + ) + + ds = await storage.get_datasource("ds") + assert ds.schema_name == "openfda_rest" + + _run_ingest( + SimpleNamespace( + datasource="ds", + schema=None, + all_schemas=False, + include=None, + exclude=None, + include_views=True, + storage=str(tmp_path / "store"), + models_dir=None, + ) + ) + model = await storage.get_model("reports", data_source="ds") + assert model.sql_table == "openfda_rest.reports" + + async def test_multi_schema_create_does_not_persist_schema_name( + self, tmp_path + ): + """Test 21. ``schema_name`` is a single-schema default. A CSV list or + ``--all-schemas`` has no single value to persist, and persisting the + first would silently narrow every later bare ingest.""" + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + + storage = _storage(tmp_path, sub="csv") + _run_datasources_create( + _create_args( + tmp_path, db_path, schema="main,openfda_rest", + storage=str(tmp_path / "csv"), + ), + storage, + ) + assert (await storage.get_datasource("ds")).schema_name is None + + storage2 = _storage(tmp_path, sub="all") + _run_datasources_create( + _create_args( + tmp_path, db_path, all_schemas=True, + storage=str(tmp_path / "all"), + ), + storage2, + ) + assert (await storage2.get_datasource("ds")).schema_name is None + + async def test_precedence_explicit_beats_persisted_beats_default( + self, tmp_path + ): + """Test 22. ``datasource.schema_name`` is a fallback consulted only + when nothing more specific was given — never a conflict.""" + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + ds = _duckdb_ds(db_path, schema_name="openfda_rest") + + persisted = ingest_datasource_report(datasource=ds) + assert _by_name(persisted.models).keys() == {"reports"} + + explicit = ingest_datasource_report(datasource=ds, schemas=["main"]) + assert _by_name(explicit.models).keys() == {"in_default"} + + plain = ingest_datasource_report(datasource=_duckdb_ds(db_path)) + assert _by_name(plain.models).keys() == {"in_default"} + + +# --------------------------------------------------------------------------- +# 23-25, 34. Surface parity and the conflict matrix +# --------------------------------------------------------------------------- + + +class TestEngineConflicts: + @pytest.mark.parametrize( + "kwargs", + [ + {"schema": "a", "schemas": ["b"]}, + {"all_schemas": True, "schema": "a"}, + {"all_schemas": True, "schemas": ["b"]}, + ], + ) + def test_report_rejects_conflicting_scope_arguments(self, tmp_path, kwargs): + """Tests 25/34. One shared rule, enforced at every entry point, so the + CLI's mutually-exclusive group is not the only thing holding the line.""" + ds = _repro_ds(tmp_path) + with pytest.raises(ValueError): + ingest_datasource_report(datasource=ds, **kwargs) + + @pytest.mark.parametrize( + "kwargs", + [ + {"schema": "a", "schemas": ["b"]}, + {"all_schemas": True, "schema": "a"}, + {"all_schemas": True, "schemas": ["b"]}, + ], + ) + def test_ingest_datasource_rejects_conflicting_scope_arguments( + self, tmp_path, kwargs + ): + ds = _repro_ds(tmp_path) + with pytest.raises(ValueError): + ingest_datasource(datasource=ds, **kwargs) + + @pytest.mark.parametrize( + "kwargs", + [ + {"schema": "a", "schemas": ["b"]}, + {"all_schemas": True, "schema": "a"}, + {"all_schemas": True, "schemas": ["b"]}, + ], + ) + async def test_idempotent_rejects_conflicting_scope_arguments( + self, tmp_path, kwargs + ): + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + with pytest.raises(ValueError): + await ingest_datasource_idempotent( + datasource=ds, storage=storage, **kwargs + ) + + +class TestRestParity: + def _client(self, tmp_path, ds: DatasourceConfig): + storage = _storage(tmp_path) + client = TestClient(create_app(storage=storage)) + resp = client.post( + "/datasources", + json={"name": ds.name, "type": ds.type, "database": ds.database}, + ) + assert resp.status_code < 300, resp.text + return client, storage + + async def test_schemas_list_is_honoured(self, tmp_path): + """Test 23.""" + ds = _repro_ds(tmp_path) + client, storage = self._client(tmp_path, ds) + resp = client.post( + "/ingest", json={"datasource": "ds", "schemas": ["openfda_rest"]} + ) + assert resp.status_code == 200, resp.text + model = await storage.get_model("reports", data_source="ds") + assert model.sql_table == "openfda_rest.reports" + + async def test_all_schemas_is_honoured(self, tmp_path): + """Test 23 (cont).""" + ds = _repro_ds(tmp_path) + client, storage = self._client(tmp_path, ds) + resp = client.post( + "/ingest", json={"datasource": "ds", "all_schemas": True} + ) + assert resp.status_code == 200, resp.text + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + assert ( + await storage.get_model("in_default", data_source="ds") + ).sql_table == "in_default" + + @pytest.mark.parametrize( + "body", + [ + {"schema_name": "a", "schemas": ["b"]}, + {"all_schemas": True, "schema_name": "a"}, + {"all_schemas": True, "schemas": ["b"]}, + ], + ) + def test_conflicting_scope_arguments_are_422(self, tmp_path, body): + """Tests 23/34. A conflict is a client error, not a silent preference + for whichever argument the handler happens to read first.""" + ds = _repro_ds(tmp_path) + client, _ = self._client(tmp_path, ds) + resp = client.post("/ingest", json={"datasource": "ds", **body}) + assert resp.status_code == 422, resp.text + + +class TestMcpParity: + async def _call(self, storage, **kwargs) -> str: + server = create_mcp_server(storage=storage) + content, _ = await server.call_tool( + name="ingest_datasource_models", + arguments={"datasource_name": "ds", **kwargs}, + ) + return content[0].text + + async def test_schemas_csv_is_honoured(self, tmp_path): + """Test 24. Comma-separated to match ``include_tables``' existing + style rather than introducing a second list convention.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + + await self._call(storage, schemas="main,openfda_rest") + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + assert ( + await storage.get_model("in_default", data_source="ds") + ).sql_table == "in_default" + + async def test_all_schemas_is_honoured(self, tmp_path): + """Test 24 (cont).""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + + await self._call(storage, all_schemas=True) + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + + @pytest.mark.parametrize( + "kwargs", + [ + {"schema_name": "a", "schemas": "b"}, + {"all_schemas": True, "schema_name": "a"}, + {"all_schemas": True, "schemas": "b"}, + ], + ) + async def test_conflicting_scope_arguments_are_reported( + self, tmp_path, kwargs + ): + """Test 34. MCP returns an error string rather than raising — the + agent has to be able to read and correct it.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + + out = await self._call(storage, **kwargs) + assert "schema" in out.lower() + assert any(w in out.lower() for w in ("cannot", "conflict", "both")) + + +# --------------------------------------------------------------------------- +# 26. Non-regression for single-schema dialects +# --------------------------------------------------------------------------- + + +class TestUnqualifiedNonRegression: + def test_sqlite_ingest_stays_unqualified(self, tmp_path): + """Test 26. SQLite reports ``main`` as its default schema. Qualifying + it would rewrite every existing model on disk for no benefit, and the + SQLite fixtures across the suite pin the unqualified form.""" + db_path = str(tmp_path / "live.db") + conn = sqlite3.connect(db_path) + conn.executescript( + "CREATE TABLE orders (id INTEGER PRIMARY KEY, amount REAL);" + "CREATE VIEW v_orders AS SELECT id FROM orders;" + ) + conn.commit() + conn.close() + + ds = DatasourceConfig(name="ds", type="sqlite", database=db_path) + models = _by_name(ingest_datasource(datasource=ds)) + + assert models["orders"].sql_table == "orders" + assert models["v_orders"].sql_table == "v_orders" + + def test_duckdb_default_schema_stays_unqualified(self, tmp_path): + """Test 26 (cont). Same rule on the dialect that exposed the bug.""" + ds = _repro_ds(tmp_path) + models = _by_name(ingest_datasource(datasource=ds, schemas=None)) + assert models["in_default"].sql_table == "in_default" + + +# --------------------------------------------------------------------------- +# 28-30a, 32. Attached catalogs and the column fallback +# --------------------------------------------------------------------------- + + +class TestAttachedCatalogs: + def test_all_schemas_covers_only_the_current_catalog(self, attached): + """Test 28. ``--all-schemas`` means "this database", not "and whatever + anyone attached to the session". The dropped schemas are reported, not + silently discarded.""" + report = ingest_datasource_report( + datasource=attached.ds, all_schemas=True + ) + + names = _by_name(report.models) + assert "only_in_other" not in names + assert {"in_default", "shared", "reports"} <= set(names) + assert names["reports"].sql_table == "openfda_rest.reports" + assert names["in_default"].sql_table == "in_default" + + dropped = [s for s in report.skipped if "attached catalog" in s.reason] + assert dropped, report.skipped + # The reason has to be actionable: it must name the catalog AND the + # exact invocation that would ingest it. + reason = next(s.reason for s in dropped if "aaa" in s.reason) + assert "aaa.main" in reason + assert "--schema" in reason + + def test_bare_ingest_does_not_reach_into_an_attached_catalog( + self, attached + ): + """Test 29. ``get_table_names(schema="main")`` still returns + ``only_in_other`` from the attached catalog — narrowing to the bare + default schema is not enough, the token must carry the catalog.""" + report = ingest_datasource_report(datasource=attached.ds) + + names = _by_name(report.models) + assert "only_in_other" not in names + assert names["shared"].sql_table == "shared" + assert [c.name for c in names["shared"].columns] == ["m"] + + def test_column_fallback_accepts_a_qualified_token(self, attached): + """Test 30. The measured zero-column trap: filtering on + ``table_schema`` alone means a qualified token matches nothing and the + model is created silently empty.""" + eng = attached.engine() + try: + cols = _get_columns_fallback(eng, "reports", "att_main.openfda_rest") + finally: + eng.dispose() + assert [c["name"] for c in cols] == ["id", "n"] + + def test_column_fallback_never_unions_across_catalogs(self, attached): + """Test 30a. ``shared`` exists in both catalogs under schema ``main``. + Normalising the token to its bare last segment — which an earlier draft + of this work proposed — returns ``['m', 'o']``. This is the test that + keeps that rule from coming back.""" + eng = attached.engine() + try: + qualified = _get_columns_fallback(eng, "shared", "att_main.main") + bare = _get_columns_fallback(eng, "shared", "main") + finally: + eng.dispose() + + assert [c["name"] for c in qualified] == ["m"] + # Pins the hazard itself, so the reason for the qualified token is + # visible in the test rather than only in the commit message. Sorted: + # the union spans two catalogs and ``ORDER BY ordinal_position`` says + # nothing about which catalog's rows come first. + assert sorted(c["name"] for c in bare) == ["m", "o"] + + def test_column_fallback_prefers_the_default_over_the_lowest_sorted( + self, attached + ): + """Test 32. The attached catalog is named ``aaa`` so it sorts first. + A lowest-sorted tie-break would swap union corruption for deterministic + wrong-table corruption, which is harder to notice.""" + eng = attached.engine() + try: + cols = _get_columns_fallback( + eng, "shared", None, default_schema="att_main.main" + ) + finally: + eng.dispose() + assert [c["name"] for c in cols] == ["m"] + + def test_column_fallback_raises_when_it_cannot_disambiguate(self, attached): + """Test 32 (cont). With no default to fall back on, refuse rather than + pick. Per-object isolation turns this into a reported skip, so one + ambiguous object never aborts the run.""" + eng = attached.engine() + try: + with pytest.raises(ValueError) as excinfo: + _get_columns_fallback(eng, "shared", None) + finally: + eng.dispose() + message = str(excinfo.value) + assert "aaa.main" in message and "att_main.main" in message + + def test_all_schemas_never_produces_a_columnless_model(self, attached): + """Test 31. The failure mode this whole token discipline exists to + prevent is a model that persists with zero columns and no error.""" + report = ingest_datasource_report( + datasource=attached.ds, all_schemas=True + ) + assert report.models + for model in report.models: + assert model.columns, f"{model.name} has no columns" + + +# --------------------------------------------------------------------------- +# 35-37. Remaining review-driven cases +# --------------------------------------------------------------------------- + + +class TestCollisionWithSanitization: + @pytest.mark.parametrize("reverse", [False, True]) + def test_exact_name_beats_sanitized_regardless_of_schema_order( + self, reverse + ): + """Test 35. ``s1.a__b`` sanitizes to ``a_b`` and collides with a real + ``s2.a_b``. Resolving in one phase over final model names keeps the + "no sanitization beats sanitization" rule ahead of the schema + tie-break, and keeps the outcome independent of listing order.""" + objects = [ + IngestableObject(name="a__b", kind="table", schema="s1"), + IngestableObject(name="a_b", kind="table", schema="s2"), + ] + if reverse: + objects.reverse() + + assigned, skipped = _assign_model_names(objects) + + assert assigned[("s2", "a_b")] == "a_b" + assert ("s1", "a__b") not in assigned + assert [s.table_name for s in skipped] == ["s1.a__b"] + + +class TestLiveSchemaKeying: + def test_ambiguous_short_keys_are_dropped_not_overwritten(self, attached): + """Test 36. Keying the live map on ``schema.table`` alone lets one + catalog's entry overwrite another's. Full keys are always present; + shorter aliases only when unambiguous.""" + live = _live_schema_for_datasource( + datasource=attached.ds, + schemas=["att_main.main", "aaa.main"], + ) + + assert live["att_main.main.shared"].columns.keys() == {"m"} + assert live["aaa.main.shared"].columns.keys() == {"o"} + # ``main.shared`` and ``shared`` are claimed by both, so neither alias + # may resolve to an arbitrary winner. + assert _resolve_live_table( + sql_table="main.shared", live_tables=live + ) is None + assert _resolve_live_table(sql_table="shared", live_tables=live) is None + + def test_unambiguous_aliases_are_still_inserted(self, attached): + """The alias keys are what let an unqualified legacy model keep + resolving — dropping them wholesale would re-arm the data-loss path.""" + live = _live_schema_for_datasource( + datasource=attached.ds, schemas=["att_main.openfda_rest"], + ) + assert _resolve_live_table( + sql_table="reports", live_tables=live + ) is not None + assert _resolve_live_table( + sql_table="openfda_rest.reports", live_tables=live + ) is not None + + +class TestHint: + def test_hint_fires_for_a_persisted_schema_name(self, tmp_path): + """Test 37. Hint eligibility is "one schema in scope and others exist", + independent of whether that schema was named explicitly. A user who set + ``schema_name`` months ago still needs to hear that a schema appeared.""" + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + ds = _duckdb_ds(db_path, schema_name="openfda_rest") + + report = ingest_datasource_report(datasource=ds) + assert report.schema_hint + assert "main" in report.schema_hint + assert "--all-schemas" in report.schema_hint + + async def test_cli_prints_the_hint_and_still_exits_zero( + self, tmp_path, capsys + ): + """Test 1 (cont). Narrowing the default scan is a behaviour change for + DuckDB users, so it has to be visible — but a hint is not a failure.""" + ds = _repro_ds(tmp_path) + storage = _storage(tmp_path) + await storage.save_datasource(ds) + + _run_ingest( + SimpleNamespace( + datasource="ds", + schema=None, + all_schemas=False, + include=None, + exclude=None, + include_views=True, + storage=str(tmp_path / "store"), + models_dir=None, + ) + ) + out = capsys.readouterr().out + assert "openfda_rest" in out + assert "--all-schemas" in out + + +# --------------------------------------------------------------------------- +# Scope resolution unit coverage +# --------------------------------------------------------------------------- + + +class TestScopeResolution: + def test_explicit_schema_is_qualified_verbatim(self): + """An explicitly-named schema is written exactly as given, so + ``--schema public`` keeps producing ``public.orders`` as it does + today — even though ``public`` is Postgres' default.""" + obj = IngestableObject(name="orders", kind="table", schema="public") + resolved = ResolvedSchema(name="public", explicit=True, is_default=True) + assert qualify_sql_table(obj=obj, resolved=resolved) == "public.orders" + + def test_auto_default_schema_is_not_qualified(self): + obj = IngestableObject(name="orders", kind="table", schema="fda.main") + resolved = ResolvedSchema( + name="fda.main", explicit=False, is_default=True + ) + assert qualify_sql_table(obj=obj, resolved=resolved) == "orders" + + def test_auto_non_default_schema_drops_the_catalog(self): + """The emitted qualifier is the bare last segment: the connection's + current catalog is already the right one, so re-stating it would only + break if the datasource is later repointed.""" + obj = IngestableObject(name="reports", kind="table", schema="fda.ofr") + resolved = ResolvedSchema(name="fda.ofr", explicit=False, is_default=False) + assert qualify_sql_table(obj=obj, resolved=resolved) == "ofr.reports" + + def test_all_schemas_excludes_system_schemas(self, tmp_path): + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + scope = resolve_ingest_schemas( + inspector=insp, + requested=None, + all_schemas=True, + datasource_schema=None, + ) + finally: + eng.dispose() + + bare = {s.name.rsplit(".", 1)[-1] for s in scope.schemas} + assert bare == {"main", "openfda_rest"} + assert not any( + s.name.startswith(("system.", "temp.")) for s in scope.schemas + ) + + def test_requested_schemas_are_marked_explicit(self, tmp_path): + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + scope = resolve_ingest_schemas( + inspector=insp, + requested=["openfda_rest"], + all_schemas=False, + datasource_schema="ignored", + ) + finally: + eng.dispose() + + assert [(s.name, s.explicit) for s in scope.schemas] == [ + ("openfda_rest", True) + ] + + def test_multi_schema_scope_reports_no_hint(self, tmp_path): + """The hint is for "you may be missing something". With more than one + schema in scope there is nothing to nudge about.""" + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + scope = resolve_ingest_schemas( + inspector=insp, + requested=["main", "openfda_rest"], + all_schemas=False, + datasource_schema=None, + ) + finally: + eng.dispose() + assert scope.other_schemas == [] + + def test_multi_returns_objects_tagged_with_their_schema(self, tmp_path): + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + scope = resolve_ingest_schemas( + inspector=insp, + requested=["main", "openfda_rest"], + all_schemas=False, + datasource_schema=None, + ) + objects = list_ingestable_objects_multi(inspector=insp, scope=scope) + finally: + eng.dispose() + + assert {(o.name, o.schema) for o in objects} == { + ("in_default", "main"), + ("reports", "openfda_rest"), + } + + def test_scope_is_a_pydantic_model(self): + """No dataclasses anywhere in this codebase.""" + assert issubclass(IngestSchemaScope, BaseModel) + assert issubclass(ResolvedSchema, BaseModel) + + +class TestProcessTableOutcome: + def test_outcome_carries_addition_and_skip_separately(self): + """``_process_one_table`` has to be able to say "I declined this one" + as well as "here is what I did" — a skip is not an error and must not + travel as one.""" + outcome = ProcessTableOutcome( + skipped=SkippedTable(table_name="s2.reports", reason="cross-schema") + ) + assert outcome.addition is None + assert outcome.skipped.table_name == "s2.reports" + + +# --------------------------------------------------------------------------- +# Coverage added after the test-plan review +# --------------------------------------------------------------------------- + + +def _seed_fk(db_path: str) -> None: + """A parent/child FK pair in a NON-default schema, so the FK graph has to + be built with the same schema token discovery used.""" + con = duckdb.connect(db_path) + con.execute("CREATE SCHEMA ofr") + con.execute("CREATE TABLE ofr.parent(id INTEGER PRIMARY KEY, nm TEXT)") + con.execute( + "CREATE TABLE ofr.child(id INTEGER PRIMARY KEY, " + "parent_id INTEGER REFERENCES ofr.parent(id), amt INTEGER)" + ) + con.close() + + +class TestForeignKeysAreSchemaAware: + """The FK graph, the join generator and the FK-column collector all take + the DISCOVERY token, never the emitted qualifier. Nothing else in the + suite exercises a foreign key, so a token mix-up there would be invisible. + """ + + def test_joins_are_generated_for_a_non_default_schema(self, tmp_path): + db_path = str(tmp_path / "fk.duckdb") + _seed_fk(db_path) + models = _by_name( + ingest_datasource(datasource=_duckdb_ds(db_path), all_schemas=True) + ) + + assert models["child"].sql_table == "ofr.child" + assert models["parent"].sql_table == "ofr.parent" + assert [ + (j.target_model, [list(p) for p in j.join_pairs]) + for j in models["child"].joins + ] == [("parent", [["parent_id", "id"]])] + + def test_fk_columns_are_excluded_from_rollup(self, tmp_path): + """``_collect_fk_columns`` is the other consumer of the token; if it + silently returned nothing the FK column would be rolled up.""" + db_path = str(tmp_path / "fk.duckdb") + _seed_fk(db_path) + models = _by_name( + ingest_datasource(datasource=_duckdb_ds(db_path), all_schemas=True) + ) + child = models["child"] + assert {c.name for c in child.columns} >= {"id", "parent_id", "amt"} + + +class TestPrimaryKeysAreSchemaAware: + def test_primary_key_survives_a_qualified_schema_token(self, tmp_path): + """DuckDB's Inspector reports an empty ``constrained_columns`` even + for a declared PRIMARY KEY, so the ``INFORMATION_SCHEMA`` fallback is + the path that actually runs. It filters on ``table_schema``, which + holds the BARE name — a qualified token matches nothing and drops + every primary key silently. Fan-out safety leans on + ``Column.primary_key``, so losing it is not cosmetic. + """ + db_path = str(tmp_path / "fk.duckdb") + _seed_fk(db_path) + models = _by_name( + ingest_datasource(datasource=_duckdb_ds(db_path), all_schemas=True) + ) + + parent = models["parent"] + assert parent.sql_table == "ofr.parent" + assert [c.name for c in parent.columns if c.primary_key] == ["id"] + + def test_primary_key_survives_in_the_default_schema(self, tmp_path): + """The same fallback runs for the default schema, whose token is + qualified too (``fda.main``) even with nothing attached.""" + ds = _repro_ds(tmp_path) + db_path = ds.database + con = duckdb.connect(db_path) + con.execute("CREATE TABLE keyed(id INTEGER PRIMARY KEY, v INTEGER)") + con.close() + + models = _by_name(ingest_datasource(datasource=ds)) + assert [c.name for c in models["keyed"].columns if c.primary_key] == ["id"] + + +class TestPerObjectIsolation: + async def test_a_raising_column_lookup_becomes_a_skip( + self, tmp_path, monkeypatch + ): + """An ambiguous column lookup raises rather than guessing. That raise + must be isolated per object — one unresolvable table cannot abort the + scan — and must surface as a skip, not an error.""" + ds = _repro_ds(tmp_path) + real = ingestion_module._safe_get_columns + + def _raising(inspector, sa_engine, table_name, schema): + if table_name == "reports": + raise ValueError( + "ambiguous schema for 'reports': aaa.main, att_main.main" + ) + return real(inspector, sa_engine, table_name, schema) + + monkeypatch.setattr(ingestion_module, "_safe_get_columns", _raising) + + report = ingest_datasource_report(datasource=ds, all_schemas=True) + + assert "in_default" in _by_name(report.models) + assert "reports" not in _by_name(report.models) + assert any("ambiguous" in s.reason for s in report.skipped), ( + report.skipped + ) + + +class TestAdditiveMergeQualifierRules: + """Pinned directly on ``_additive_merge_existing``. Driving these through + ingestion would let the cross-schema guard skip the model before the merge + ran, so the merge rule itself would go untested.""" + + @staticmethod + def _model(sql_table: str, columns: list[str]) -> SlayerModel: + return SlayerModel( + name="reports", + data_source="ds", + sql_table=sql_table, + columns=[Column(name=c, type=DataType.INT) for c in columns], + ) + + def test_qualified_persisted_table_is_never_rewritten(self): + result = _additive_merge_existing( + persisted=self._model("prod.reports", ["id"]), + fresh=self._model("openfda_rest.reports", ["id", "n"]), + ) + assert result.merged.sql_table == "prod.reports" + assert result.sql_table_change is None + + def test_unqualified_persisted_table_is_healed(self): + result = _additive_merge_existing( + persisted=self._model("reports", ["id"]), + fresh=self._model("openfda_rest.reports", ["id"]), + ) + assert result.merged.sql_table == "openfda_rest.reports" + assert result.sql_table_change == "reports → openfda_rest.reports" + + def test_heal_alone_is_enough_to_trigger_a_save(self): + """A qualifier repair usually changes no columns and no joins, so if it + does not participate in the short-circuit the merged model is computed + and then discarded.""" + result = _additive_merge_existing( + persisted=self._model("reports", ["id"]), + fresh=self._model("openfda_rest.reports", ["id"]), + ) + assert result.new_columns == [] + assert result.new_joins == [] + assert result.merged is not None + assert result.sql_table_change is not None + + def test_a_different_object_name_is_not_a_qualifier_repair(self): + """Healing keys on the bare names matching. ``reports`` and + ``s.other`` are unrelated tables that happen to share a model name.""" + result = _additive_merge_existing( + persisted=self._model("reports", ["id"]), + fresh=self._model("openfda_rest.other", ["id"]), + ) + assert result.merged.sql_table == "reports" + assert result.sql_table_change is None + + +class TestAdditionRendering: + def test_updated_line_names_the_qualifier_repair(self): + """The repair is the whole point of the re-ingest, so it cannot be + silent — and a repair adds no columns, so without this the line prints + nothing at all.""" + buf = io.StringIO() + _print_ingest_addition( + ModelAddition( + model_name="reports", + data_source="ds", + created=False, + sql_table_change="reports → openfda_rest.reports", + ), + file=buf, + ) + out = buf.getvalue() + assert "reports → openfda_rest.reports" in out + assert "Updated" in out + + +class TestSystemSchemaFilter: + @pytest.mark.parametrize( + "token", + [ + "information_schema", + "INFORMATION_SCHEMA", + "pg_catalog", + "Pg_Catalog", + "pg_toast", + "performance_schema", + "mysql", + "sys", + "sys_temp", + "pg_temp_3", + "pg_toast_temp_1", + "system.main", + "system.information_schema", + "temp.main", + "TEMP.main", + ], + ) + def test_system_schemas_are_filtered(self, token): + assert _is_system_schema(token) is True + + @pytest.mark.parametrize( + "token", + [ + "main", + "public", + "openfda_rest", + "fda.main", + "fda.openfda_rest", + "systems", + "temporary", + "my_sys", + ], + ) + def test_user_schemas_are_kept(self, token): + assert _is_system_schema(token) is False + + +class TestCollisionTieBreakOrder: + def test_lower_schema_wins_and_order_does_not_matter(self): + """Rule 3. Both objects are sanitization-free and neither schema is the + default, so only the schema name can decide — and it must decide the + same way whichever order the inspector listed them in.""" + forward = [ + IngestableObject(name="reports", kind="table", schema="s2"), + IngestableObject(name="reports", kind="table", schema="s3"), + ] + reverse = list(reversed(forward)) + + for objects in (forward, reverse): + assigned, skipped = _assign_model_names(objects) + assert assigned[("s2", "reports")] == "reports" + assert ("s3", "reports") not in assigned + assert [s.table_name for s in skipped] == ["s3.reports"] + + def test_default_schema_beats_a_lower_sorted_schema(self): + """Rule 2 outranks rule 3: ``aaa`` sorts first but ``main`` is the + default, so a plain sort would pick the wrong winner.""" + objects = [ + IngestableObject(name="reports", kind="table", schema="aaa"), + IngestableObject(name="reports", kind="table", schema="main"), + ] + resolved = { + "aaa": ResolvedSchema(name="aaa", explicit=False, is_default=False), + "main": ResolvedSchema(name="main", explicit=False, is_default=True), + } + assigned, _ = _assign_model_names(objects, resolved_by_schema=resolved) + assert assigned[("main", "reports")] == "reports" + assert ("aaa", "reports") not in assigned + + +class TestMcpCreateDatasourceParity: + async def _call(self, storage, **kwargs) -> str: + server = create_mcp_server(storage=storage) + content, _ = await server.call_tool( + name="create_datasource", arguments=kwargs + ) + return content[0].text + + async def test_schema_name_is_persisted_and_used(self, tmp_path): + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + storage = _storage(tmp_path) + + await self._call( + storage, + name="ds", + type="duckdb", + database=db_path, + schema_name="openfda_rest", + ) + assert (await storage.get_datasource("ds")).schema_name == "openfda_rest" + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + + async def test_schemas_csv_does_not_persist_schema_name(self, tmp_path): + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + storage = _storage(tmp_path) + + await self._call( + storage, + name="ds", + type="duckdb", + database=db_path, + schemas="main,openfda_rest", + ) + assert (await storage.get_datasource("ds")).schema_name is None + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + assert ( + await storage.get_model("in_default", data_source="ds") + ).sql_table == "in_default" + + async def test_all_schemas_does_not_persist_schema_name(self, tmp_path): + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + storage = _storage(tmp_path) + + await self._call( + storage, + name="ds", + type="duckdb", + database=db_path, + all_schemas=True, + ) + assert (await storage.get_datasource("ds")).schema_name is None + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + + @pytest.mark.parametrize( + "kwargs", + [ + {"schema_name": "a", "schemas": "b"}, + {"all_schemas": True, "schema_name": "a"}, + {"all_schemas": True, "schemas": "b"}, + ], + ) + async def test_conflicting_scope_arguments_are_reported( + self, tmp_path, kwargs + ): + db_path = str(tmp_path / "fda.duckdb") + _seed_repro(db_path) + storage = _storage(tmp_path) + + out = await self._call( + storage, name="ds", type="duckdb", database=db_path, **kwargs + ) + assert "schema" in out.lower() + assert any(w in out.lower() for w in ("cannot", "conflict", "both")) + + +class TestCliArgumentParsing: + """The parsers are built inline in ``main()``, so drive them through + ``main()`` with a stubbed handler that captures the parsed args.""" + + @staticmethod + def _capture(monkeypatch, argv: list[str], handler: str) -> SimpleNamespace: + captured: dict[str, SimpleNamespace] = {} + + def _stub(args, *_rest, **_kwargs): + captured["args"] = args + + monkeypatch.setattr(f"slayer.cli.{handler}", _stub) + monkeypatch.setattr(sys, "argv", ["slayer", *argv]) + + main() + return captured["args"] + + def test_ingest_schema_accepts_a_csv_list(self, monkeypatch): + args = self._capture( + monkeypatch, + ["ingest", "--datasource", "ds", "--schema", "main,openfda_rest"], + "_run_ingest", + ) + assert args.schema == "main,openfda_rest" + assert args.all_schemas is False + + def test_ingest_accepts_all_schemas(self, monkeypatch): + args = self._capture( + monkeypatch, + ["ingest", "--datasource", "ds", "--all-schemas"], + "_run_ingest", + ) + assert args.all_schemas is True + assert args.schema is None + + def test_ingest_defaults_all_schemas_off(self, monkeypatch): + args = self._capture( + monkeypatch, ["ingest", "--datasource", "ds"], "_run_ingest" + ) + assert args.all_schemas is False + + def test_ingest_rejects_schema_with_all_schemas(self, monkeypatch): + monkeypatch.setattr( + sys, + "argv", + [ + "slayer", "ingest", "--datasource", "ds", + "--schema", "main", "--all-schemas", + ], + ) + with pytest.raises(SystemExit) as excinfo: + main() + assert excinfo.value.code == 2 + + def test_datasources_create_accepts_all_schemas(self, monkeypatch): + args = self._capture( + monkeypatch, + ["datasources", "create", "duckdb:///x.duckdb", "--all-schemas"], + "_run_datasources_create", + ) + assert args.all_schemas is True + + def test_datasources_create_rejects_schema_with_all_schemas( + self, monkeypatch + ): + monkeypatch.setattr( + sys, + "argv", + [ + "slayer", "datasources", "create", "duckdb:///x.duckdb", + "--schema", "main", "--all-schemas", + ], + ) + with pytest.raises(SystemExit) as excinfo: + main() + assert excinfo.value.code == 2 + + +class TestRestLegacyCompatibility: + def _client(self, tmp_path, ds: DatasourceConfig): + storage = _storage(tmp_path) + client = TestClient(create_app(storage=storage)) + resp = client.post( + "/datasources", + json={"name": ds.name, "type": ds.type, "database": ds.database}, + ) + assert resp.status_code < 300, resp.text + return client, storage + + async def test_schema_name_is_still_honoured(self, tmp_path): + """The existing field keeps working — it folds to ``schemas=[value]`` + rather than being replaced.""" + ds = _repro_ds(tmp_path) + client, storage = self._client(tmp_path, ds) + resp = client.post( + "/ingest", json={"datasource": "ds", "schema_name": "openfda_rest"} + ) + assert resp.status_code == 200, resp.text + assert ( + await storage.get_model("reports", data_source="ds") + ).sql_table == "openfda_rest.reports" + + async def test_omitting_every_scope_field_uses_the_default_schema( + self, tmp_path + ): + ds = _repro_ds(tmp_path) + client, storage = self._client(tmp_path, ds) + resp = client.post("/ingest", json={"datasource": "ds"}) + assert resp.status_code == 200, resp.text + assert await storage.get_model("reports", data_source="ds") is None + assert ( + await storage.get_model("in_default", data_source="ds") + ).sql_table == "in_default" + + +class TestFallbackSqlShape: + """The catalog predicate must be added only when the token carries a + catalog, so every non-DuckDB dialect keeps emitting exactly today's SQL.""" + + @staticmethod + def _capture_sql(schema): + conn = MagicMock() + conn.execute.return_value.fetchall.return_value = [] + engine = MagicMock() + engine.connect.return_value.__enter__.return_value = conn + + _get_columns_fallback(engine, "orders", schema) + clause, params = conn.execute.call_args[0] + return str(clause), params + + def test_bare_schema_emits_no_catalog_predicate(self): + sql, params = self._capture_sql("public") + assert "table_catalog" not in sql + assert "catalog" not in params + assert params == {"table_name": "orders", "schema": "public"} + + def test_qualified_schema_emits_the_catalog_predicate(self): + sql, params = self._capture_sql("att_main.openfda_rest") + assert "table_catalog" in sql + assert params["catalog"] == "att_main" + assert params["schema"] == "openfda_rest" + + +# Re-exported for callers that patch discovery in place (the existing +# name-sanitization tests do this); asserting it here keeps that seam visible. +def test_module_exports_discovery_helpers(): + for attr in ( + "list_ingestable_objects", + "list_ingestable_objects_multi", + "resolve_ingest_schemas", + "qualify_sql_table", + "split_sql_table", + ): + assert hasattr(ingestion_module, attr), attr From 558c06dce838e3441e66e122238593a0566409b1 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 7 Aug 2026 12:05:26 +0200 Subject: [PATCH 2/4] fix: resolve outside schema names before discovery; keep contested aliases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the Codex review of the PR diff, four of them defects I introduced, all reproduced against DuckDB before fixing. **Bare schema names still swept ATTACHed catalogs.** The token discipline was applied to the schemas we ENUMERATE but not to the ones handed to us: an explicit `--schema main`, a persisted `schema_name`, and the bare qualifier `validate-models` reads back off `sql_table` all went to the Inspector verbatim. Measured, `--schema main` on a database with a second catalog attached ingested that catalog's `only_in_other` and wrote it as `main.only_in_other`, which does not exist in the current catalog -- the exact bug class this branch exists to remove. `resolve_schema_token` upgrades such a name to the enumerated catalog-qualified token, preferring the current catalog when several expose the same schema name, and `ResolvedSchema` now carries `requested_as` so the emitted `sql_table` stays what the user typed. Resolving for discovery must never change the SQL we persist. **Dropping every contested alias was itself a data-loss bug.** The live map dropped a short alias claimed by more than one object, meaning to avoid an arbitrary winner. But default-schema models are persisted UNQUALIFIED by design, so as soon as another schema gained a same-named table, a legacy `sql_table: orders` stopped resolving -- and an unresolvable model is a `WholeModelDelete` that `validate-models --force-clean` deletes. A contested alias now resolves to the DEFAULT schema's entry, which is what the database itself does: `FROM orders` and `FROM main.orders` both land in the current catalog. It is dropped only when the default cannot break the tie. **The cross-schema guard failed open.** `_default_schema_object_names` converted a failed listing into an empty list, which reads as "no such default-schema object" and waved the qualifier repair through -- repointing a model at another schema's table. Unknown is now `None` and distinct from empty, and refuses the merge. Skipping a legal repair costs a re-run. **The PK fallback joined across catalogs.** DuckDB names a PK constraint after its column, so a same-shaped table in an ATTACHed catalog gets the identical auto-generated name; joining `key_column_usage` on constraint name and schema alone matched both and returned `['id', 'id']`. The join now carries the catalog. One finding rejected: emitting a 3-part `sql_table` for an explicitly-named catalog-qualified schema is by design, and verified queryable on DuckDB. Also from SonarQube: `# noqa: CODE — prose` is malformed suppression syntax (python:S7632), so the reasons move to their own line; the alias indexing is extracted into `_index_live_entries`, which also settles the cognitive complexity finding on `_live_schema_for_datasource`; and one composite test assertion is split. Three existing tests updated -- they pinned the pre-fix behaviour: the discovery token is now qualified where it used to be bare, and the contested alias resolves rather than missing. Co-Authored-By: Claude Opus 5 (1M context) --- DECISIONS.md | 2 +- slayer/engine/ingestion.py | 126 ++++++++++---- slayer/engine/introspect_utils.py | 67 ++++++- slayer/engine/schema_drift.py | 74 ++++++-- tests/test_ingestion_schema_qualification.py | 173 +++++++++++++++++-- 5 files changed, 374 insertions(+), 68 deletions(-) diff --git a/DECISIONS.md b/DECISIONS.md index 67f149bc..144390b1 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -70,4 +70,4 @@ implementation detail. Include issue refs when known. - 2026-08-04 — Dialect-aware / complete escaping for Mode-A `{variable}` substitution (DEV-1727), hardening DEV-1625. `substitute_variables(..., escape="sql")` is now **dialect-aware** and **fail-closed**: it gained a required keyword-only `backslash_escapes` signal (`bool | None`, raises if `None` in sql mode) so a caller rendering raw SQL can never silently under-escape. On backslash-escaping dialects (MySQL/ClickHouse/Snowflake/Redshift/BigQuery/Databricks/Spark) it doubles the backslash before escaping the single quote; on standard dialects it keeps the `''` quote-doubling. The double quote is deliberately left untouched — inside a single-quoted literal `\"` is NOT a recognised escape on 6 of the 7 backslash dialects (only MySQL), so escaping it would corrupt the value. The regime is DERIVED from sqlglot's own tokenizer via `SqlDialect.backslash_escapes_strings` (= `"\\" in tokenizer.STRING_ESCAPES`, guarded + 14-dialect pinned) so our escaping can never drift from the parser that reads the substituted SQL. `escape="python"` (Mode-B) additionally encodes the full C0 control range (`\t`/`\n`/`\r` named, rest `\xNN`) so raw newlines/NUL no longer break `ast.parse`. Engine fail-closed: `_substitute_model_sql_surfaces` / `_render_probe_model` require a `dialect`, threaded from the resolved datasource — no bare bool to forget. Assumes MySQL's default `sql_mode` (backslash escapes on); `NO_BACKSLASH_ESCAPES` servers are a sqlglot-layer-wide limitation, documented not fixed. The SQLite backslash end-to-end gap stays a pinned strict-xfail (pre-existing, out of scope). Bound parameters rejected (don't fit substitute-into-raw-SQL). Nested/join/cross-model lineages remain DEV-1678. - 2026-08-04 — Declared list-valued `{variable}` coercion (DEV-1730 follow-up): a scalar supplied for a variable the model declares `list_valued` is wrapped into a one-element list before Mode-A substitution, so an importer-generated `col IN ({var})` renders `IN ('US')` rather than the unquoted `IN (US)`. The generic scalar rule (author writes the quotes, so `{var}` also works in numeric/fragment positions like `amount >= {floor}` and `{d}::TIMESTAMP`) is CORRECT and unchanged — it just presumes an author who can see the SQL position, which a machine-generated fixed template does not have; the caller cannot supply per-element quotes through parentheses the importer wrote. Silent-wrong-answer risk drove the fix over a raise: `region IN (US)` parses as a column reference, so it fails at the database with a confusing message, or resolves against a real column and returns wrong rows. Opt-in is a front-end-NEUTRAL flag: the Cube converter writes `list_valued: ref.kind == "string"` into each `meta.cube_variables` entry (arrow forms splice pre-quoted scalars and stay `False`), and the engine reads only that flag — never Cube's `kind` taxonomy — so a future list-shaped front-end opts in the same way. Coercion lives at the single Mode-A choke point `_substitute_model_sql_surfaces` (execution and the `_render_probe_model` type-probe both route through it, so it cannot be bypassed) via `coerce_declared_list_variables` / `list_valued_variable_names` in `slayer/core/query.py`. Scope is deliberately narrow: only `str`/`int`/`float`/`bool` are wrapped; `list`/`tuple` pass through (the **empty list still raises** — "no filter" belongs to an optional block or a sentinel default); `None`/`dict` are left for `_render_variable_value` to reject with its own naming error; hand-written models declare nothing and are untouched. Follow-on from the same review: `declares_variables(model)` (any non-empty `meta.cube_variables`) now also defeats the DEV-1625 zero-variable fast path, via the shared `_model_needs_substitution_pass` predicate used by both `_substitute_model_sql_surfaces` and `_render_probe_model`. This closes the fast-path hole for a GENERATED model whose pushdowns are all required (no `{? ?}` block to force the pass): such a model used to emit a bare `{var}` into the SQL on a zero-variable call instead of raising the documented missing-variable error. The hole stays open — deliberately — for hand-written models, which declare nothing and keep the raw-brace-literal protection (`'{1,2,3}'`). The `list_valued` flag is matched with `is True`, not truthiness, since `meta` is user-extensible and a stray `1` or the string `"false"` must not switch substitution semantics. The bag is also SELF-IDENTIFYING — an entry counts only with a string `member` (the shape every importer writes) — so a hand-written `meta` that reuses the `cube_variables` key is not mistaken for generated SQL and silently stripped of its brace-literal protection. - 2026-08-05 — Ingestion sees views, survives unmodellable names, and stops being silent (DEV-1741). **Views**: `list_ingestable_objects` replaces the bare `get_table_names()` at every introspection site, adding `get_view_names` + `get_materialized_view_names` behind a `NotImplementedError`/`Exception` guard (the base `Inspector` RAISES for matviews on unsupporting dialects rather than returning `[]`), de-duplicated first-classification-wins because some dialects return views from `get_table_names()`, in a deterministic tables→views→matviews order that the name-collision policy depends on. Ingested by default — dbt materializes staging models as views, so opt-in would have left the reported failure in place for a fresh install — with `--no-views` on both `slayer ingest` and `datasources create --ingest`. The drift side (`_live_schema_for_datasource`) and the MCP listing (`_fetch_tables`) take **no flag and are unconditional**: that map is only ever a lookup target (`validate_datasource` iterates the *persisted* models, `available_in_ds` derives from them), so views there cannot manufacture a model or a drift entry — what they fix is a pre-existing **data-loss** bug where a hand-authored model whose `sql_table` named a view resolved to `live_table=None` → `WholeModelDelete` → deleted by `validate-models --force-clean`. Gating that on `--no-views` would re-arm it for exactly the users who opted out. **Names**: model names can't contain `__` (six modules — generator/enrichment/column_expansion/column_dependency/schema_drift/osi — `split("__")` an alias back into a join path, so a model named `a__b` is read as the alias for `a→b` and yields a silently wrong query, not a crash), but `sql_table` can, so a dlt child table `reports__patient__drug` is modelled as `reports_patient_drug` with `sql_table` verbatim. Sanitizer is `re.sub(r"_{2,}", "_")`, NOT `replace("__","_")` — `str.replace` is non-overlapping, so `a___b`→`a__b` would still fail validation. Collisions reserve every unsanitized name first (a real `a_b` always beats a sanitized `a__b`, order-independently) and **skip** rather than suffix, since suffixes shift as the object set changes and would orphan models and churn drift. A per-object `try/except` backstops everything else (`.`/`:`/`/`/`\` names, bad column names, per-object introspection failures); the FK-collection loop and `_get_fk_relationships` are guarded too because they run BEFORE that isolation and would otherwise still kill the run. **Reporting**: skips travel in a new `skipped` list, deliberately NOT folded into `errors` — `slayer ingest` exits 1 on either (we declined a perfectly valid object; `--exclude` is the documented remedy) but `POST /ingest` keeps 422 for `errors` only, because a permanent 422 aimed at a machine that can't act on the hint buries a successful partial ingest behind an error status. An empty scan prints the available schemas and exits 1 (the reporter listed the missing exit code as part of the defect), gated on `objects` not `additions` so a healthy no-op re-ingest stays quiet; `datasources create --ingest` on an empty DB still exits 0, since creating the datasource is that command's job and it succeeded. `in_scope_table_names` switched from model names to `_bare_table_name(sql_table)` — it is compared against table names in `_scoped_models_for_validation`, so the old keying silently dropped from validation scope any model whose name differs from its table (every sanitized model, and already every dbt/OSI hidden model passing `model_name=`). **`source_kind`** (`table`/`view`/`materialized_view`/`None`=unknown) persists on `SlayerModel`, v7→v8 with a no-op migration (mandatory: `migrate()` raises `RuntimeError` on an unregistered step); `None` for pre-v8, hand-authored and sql/query-backed models is the honest value, not a guess. It is a deliberate **exception to the additive-merge contract** — refreshed, not preserved — because it describes the live object rather than user intent, and the transition it exists to capture (dbt `+materialized: table`) usually changes no columns at all; the refresh therefore has to be made in three places (the early return, the `model_copy(update=...)`, and the save gate in `_process_one_table`), since editing only the update dict computes a corrected model and throws it away. A `None` from a non-classifying path never erases a known value. Docs-only fix for the advertised `motley-slayer[duckdb]` extra, which does not exist: `duckdb`/`duckdb-engine` are unconditional core deps (the Postgres facade imports duckdb at top level on every CLI invocation), and adding them under `[tool.poetry.extras]` would *gate* rather than alias them, breaking bare `pip install motley-slayer` → `datasources create demo`. -- 2026-08-07 — Ingest resolves a schema scope, and non-default schemas are written into `sql_table` (DEV-1758). **The bug**: `duckdb_engine`'s `get_table_names(schema=None)` returns objects from *every* schema as bare names — Postgres/MySQL/MSSQL/Snowflake/BigQuery/ClickHouse/SQLite all restrict it to the connection's default — so `_build_one_model`'s `f"{schema}.{name}" if schema else name` wrote an unqualified `sql_table` for a non-default-schema object and the generator emitted `FROM reports`, which fails table-not-found. The same schema-blindness made `_get_columns_fallback` union two same-named tables' columns. Not literally a #283 regression: that `sql_table` line is byte-identical before and after, and `--schema` always qualified correctly; #283 changed *visibility* by ingesting views by default, and dbt materialises staging models as views, so a dlt+dbt DuckDB file went from a handful of models to dozens, most unqueryable. **Scope**: one pass covers ONE schema — explicit `--schema a,b` / `--all-schemas` (plus `schemas` / `all_schemas` on the Python, REST and MCP surfaces), else `datasource.schema_name`, else the connection default. Multi-schema is opt-in because it changes what `sql_table` holds; when exactly one schema is scanned and others exist, the result carries a hint naming them and the exit code is unchanged (a hint is not a failure). `schema_name` is a **fallback**, never a conflict with an explicit flag; the genuine conflicts (`schema`+`schemas`, `all_schemas`+either) are rejected by one shared helper called from every entry point, so the CLI's mutually-exclusive group is not the only thing holding the line. **Two different strings**: the *discovery token* is carried exactly as `get_schema_names()` yields it — catalog-qualified on DuckDB (`fda.main`), bare elsewhere — and the qualified form is the SAFE one. Measured: with a second catalog `ATTACH`ed, a bare `main` makes `get_table_names` and `has_table` reach into the attached catalog and makes the column fallback return the cross-catalog union, while `att_main.main` is exact. So `is_default` compares the token IN FULL (no last-segment comparison, which is what made `att_main.main` and `other.main` both read as the default), and the INFORMATION_SCHEMA fallbacks filter on `table_catalog` as well as `table_schema` — `table_schema` alone holds the bare name, so a qualified token matched nothing and produced a silently **column-less** model, and retrying bare would resurrect the union. This applies to the PK fallback too, which on DuckDB is the path that actually runs (its Inspector reports no PK even for a declared PRIMARY KEY) — filtering it wrong drops every primary key, and fan-out safety leans on `Column.primary_key`. The *emitted qualifier* is a different string: the bare last segment, since the connection's current catalog is already correct. **What gets qualified** (D-9): only non-default schemas, so widening the scan never rewrites models already on disk and one datasource legitimately mixes both forms; a schema named explicitly as a single value is written verbatim, preserving today's `--schema public` → `public.orders`. A multi-schema list is deliberately NOT verbatim — listing the default alongside another schema would re-qualify every existing model. `--all-schemas` means the **current catalog only**; attached catalogs are dropped loudly with the exact `--schema .` invocation that ingests them, never silently. **Merging**: re-ingest heals a MISSING qualifier (participating in the short-circuit and the save gate, like `source_kind` — a repair usually changes no columns, so a merge that only edits the update dict computes the fix and discards it) but never rewrites an existing one. Two schemas' same-named tables are never fused into one model: a schema mismatch skips, and — the case a schema comparison alone cannot see, because D-9 persists default-schema models *unqualified* — a bare persisted `sql_table` that names a real default-schema object also skips, rather than being repointed at another schema's table by the heal. **Collisions** are resolved in ONE phase over final model names with a 4-key total order (unsanitized beats sanitized, then default schema, then schema name, then object name), not as successive passes, so the mixed case (`s1.a__b` sanitizing onto a real `s2.a_b`) is defined and the outcome never depends on inspector listing order. Losers skip, never suffix. **Validation** derives its schema set from the models being validated rather than gaining a flag, and the live map is keyed on the full `.` identity with shorter aliases inserted only when unique — an ambiguous alias is dropped so a lookup misses instead of resolving to another catalog's same-named table. This is a data-loss path, not a nuisance: an unresolvable model is a `WholeModelDelete` that `validate-models --force-clean` acts on. One dotted-name splitter (`split_sql_table`, everything before the FINAL dot) replaces three disagreeing parsers, so hand-written Snowflake `db.schema.table` and BigQuery `project.dataset.table` stop losing their catalog. No new model field and no v9 migration — the schema lives in `sql_table`, which is where the generator already reads it. +- 2026-08-07 — Ingest resolves a schema scope, and non-default schemas are written into `sql_table` (DEV-1758). **The bug**: `duckdb_engine`'s `get_table_names(schema=None)` returns objects from *every* schema as bare names — Postgres/MySQL/MSSQL/Snowflake/BigQuery/ClickHouse/SQLite all restrict it to the connection's default — so `_build_one_model`'s `f"{schema}.{name}" if schema else name` wrote an unqualified `sql_table` for a non-default-schema object and the generator emitted `FROM reports`, which fails table-not-found. The same schema-blindness made `_get_columns_fallback` union two same-named tables' columns. Not literally a #283 regression: that `sql_table` line is byte-identical before and after, and `--schema` always qualified correctly; #283 changed *visibility* by ingesting views by default, and dbt materialises staging models as views, so a dlt+dbt DuckDB file went from a handful of models to dozens, most unqueryable. **Scope**: one pass covers ONE schema — explicit `--schema a,b` / `--all-schemas` (plus `schemas` / `all_schemas` on the Python, REST and MCP surfaces), else `datasource.schema_name`, else the connection default. Multi-schema is opt-in because it changes what `sql_table` holds; when exactly one schema is scanned and others exist, the result carries a hint naming them and the exit code is unchanged (a hint is not a failure). `schema_name` is a **fallback**, never a conflict with an explicit flag; the genuine conflicts (`schema`+`schemas`, `all_schemas`+either) are rejected by one shared helper called from every entry point, so the CLI's mutually-exclusive group is not the only thing holding the line. **Two different strings**: the *discovery token* is carried exactly as `get_schema_names()` yields it — catalog-qualified on DuckDB (`fda.main`), bare elsewhere — and the qualified form is the SAFE one. Measured: with a second catalog `ATTACH`ed, a bare `main` makes `get_table_names` and `has_table` reach into the attached catalog and makes the column fallback return the cross-catalog union, while `att_main.main` is exact. So `is_default` compares the token IN FULL (no last-segment comparison, which is what made `att_main.main` and `other.main` both read as the default), and the INFORMATION_SCHEMA fallbacks filter on `table_catalog` as well as `table_schema` — `table_schema` alone holds the bare name, so a qualified token matched nothing and produced a silently **column-less** model, and retrying bare would resurrect the union. This applies to the PK fallback too, which on DuckDB is the path that actually runs (its Inspector reports no PK even for a declared PRIMARY KEY) — filtering it wrong drops every primary key, and fan-out safety leans on `Column.primary_key`. The *emitted qualifier* is a different string: the bare last segment, since the connection's current catalog is already correct. **What gets qualified** (D-9): only non-default schemas, so widening the scan never rewrites models already on disk and one datasource legitimately mixes both forms; a schema named explicitly as a single value is written verbatim, preserving today's `--schema public` → `public.orders`. A multi-schema list is deliberately NOT verbatim — listing the default alongside another schema would re-qualify every existing model. `--all-schemas` means the **current catalog only**; attached catalogs are dropped loudly with the exact `--schema .` invocation that ingests them, never silently. **Merging**: re-ingest heals a MISSING qualifier (participating in the short-circuit and the save gate, like `source_kind` — a repair usually changes no columns, so a merge that only edits the update dict computes the fix and discards it) but never rewrites an existing one. Two schemas' same-named tables are never fused into one model: a schema mismatch skips, and — the case a schema comparison alone cannot see, because D-9 persists default-schema models *unqualified* — a bare persisted `sql_table` that names a real default-schema object also skips, rather than being repointed at another schema's table by the heal. **Collisions** are resolved in ONE phase over final model names with a 4-key total order (unsanitized beats sanitized, then default schema, then schema name, then object name), not as successive passes, so the mixed case (`s1.a__b` sanitizing onto a real `s2.a_b`) is defined and the outcome never depends on inspector listing order. Losers skip, never suffix. **Validation** derives its schema set from the models being validated rather than gaining a flag, and the live map is keyed on the full `.` identity plus shorter aliases. A contested alias resolves to the DEFAULT schema's entry, mirroring what the database itself does (`FROM orders` and `FROM main.orders` both land in the current catalog); it is dropped only when the default cannot break the tie. Dropping every contested alias — the first cut, and what the plan review asked for — was itself a data-loss bug, because default-schema models are persisted UNQUALIFIED by design, so the moment another schema gained a same-named table the legacy model stopped resolving. Schema names arriving from outside — a `--schema` argument, a persisted `schema_name`, a bare qualifier read back off `sql_table` — are upgraded to the enumerated catalog-qualified token before they reach an Inspector, since a bare token is exactly what sweeps ATTACHed catalogs; what the user typed is kept separately and is what gets emitted, so resolving for discovery can never change the SQL persisted. This is a data-loss path, not a nuisance: an unresolvable model is a `WholeModelDelete` that `validate-models --force-clean` acts on. One dotted-name splitter (`split_sql_table`, everything before the FINAL dot) replaces three disagreeing parsers, so hand-written Snowflake `db.schema.table` and BigQuery `project.dataset.table` stop losing their catalog. No new model field and no v9 migration — the schema lives in `sql_table`, which is where the generator already reads it. diff --git a/slayer/engine/ingestion.py b/slayer/engine/ingestion.py index c041a14a..6776544c 100644 --- a/slayer/engine/ingestion.py +++ b/slayer/engine/ingestion.py @@ -27,13 +27,16 @@ SlayerModel, sanitize_model_name, ) -from slayer.engine.introspect_utils import ( # noqa: F401 (re-exported for back-compat) +from slayer.engine.introspect_utils import ( # noqa: F401 + # (re-exported for back-compat) _FLOAT_LIKE_INFO_SCHEMA_TYPES, _INFO_SCHEMA_TYPE_MAP, _get_columns_fallback, _parse_info_schema_is_float, _safe_get_columns, + enumerated_schema_names, qualified_default_schema, + resolve_schema_token, split_schema_token, split_sql_table, ) @@ -475,12 +478,17 @@ def _get_pk_constraint_fallback( if catalog is not None: clauses.append("tc.table_catalog = :catalog") params["catalog"] = catalog + # The join carries the catalog too: constraint names are only unique + # within a catalog, and DuckDB generates the same ``t_id_pkey`` for a + # same-shaped table in an ATTACHed one — which joined across catalogs + # and returned the PK column twice. sql = ( "SELECT kcu.column_name " "FROM information_schema.table_constraints tc " "JOIN information_schema.key_column_usage kcu " " ON tc.constraint_name = kcu.constraint_name " " AND tc.table_schema = kcu.table_schema " + " AND tc.table_catalog = kcu.table_catalog " "WHERE " + " AND ".join(clauses) ) else: @@ -847,8 +855,9 @@ class IngestionScanReport(BaseModel): # Object names living in the connection's default schema. The additive # pass needs it to tell "this persisted unqualified model IS the default # schema's table" from "a same-named table in another schema", which is - # the one case a schema comparison alone cannot decide. - default_schema_objects: list[str] = Field(default_factory=list) + # the one case a schema comparison alone cannot decide. ``None`` means the + # listing failed — distinct from empty, because the consumer fails closed. + default_schema_objects: list[str] | None = None # --------------------------------------------------------------------------- @@ -886,14 +895,22 @@ def _is_system_schema(token: str) -> bool: class ResolvedSchema(BaseModel): """One schema in ingest scope. - ``name`` is the *discovery* token, carried exactly as the dialect - enumerates it. ``explicit`` means the user named this single schema, so - its qualifier is written verbatim; a multi-schema request follows the - automatic rules instead, or listing the default schema alongside another - would re-qualify every model already on disk. + ``name`` is the *discovery* token, in the shape the dialect enumerates — + catalog-qualified on DuckDB. A schema the user named bare is upgraded to + that shape, because a bare token reaches into ``ATTACH``ed catalogs. + + ``requested_as`` is what the user actually typed, and is what gets written + into ``sql_table`` on the ``explicit`` path — resolving a token for + discovery must never change the SQL we emit. + + ``explicit`` means the user named this single schema, so its qualifier is + written verbatim; a multi-schema request follows the automatic rules + instead, or listing the default schema alongside another would re-qualify + every model already on disk. """ name: str | None = None + requested_as: str | None = None explicit: bool = False is_default: bool = False @@ -970,12 +987,32 @@ def _current_catalog_only( def _enumerate_schemas(inspector: sa.engine.Inspector) -> list[str]: """Every non-system schema the connection can see, tokens as enumerated.""" - try: - names = list(inspector.get_schema_names() or []) - except Exception as exc: # noqa: BLE001 — enumeration is best-effort - logger.debug("get_schema_names failed: %s", exc) - return [] - return sorted(n for n in names if isinstance(n, str) and not _is_system_schema(n)) + return sorted( + n for n in enumerated_schema_names(inspector) if not _is_system_schema(n) + ) + + +def _resolved_from_request( + *, + token: str, + inspector: sa.engine.Inspector, + enumerated: list[str], + default_token: str | None, + explicit: bool, +) -> ResolvedSchema: + """Build a :class:`ResolvedSchema` for a schema the user named. + + The discovery token is upgraded to the enumerated (catalog-qualified) + shape where that is unambiguous — a bare ``main`` otherwise sweeps + ``ATTACH``ed catalogs — while ``requested_as`` keeps the user's own string + so the emitted ``sql_table`` is unchanged by the upgrade. + """ + return ResolvedSchema( + name=resolve_schema_token(inspector, token, enumerated=enumerated), + requested_as=token, + explicit=explicit, + is_default=_matches_default(token, default_token), + ) def resolve_ingest_schemas( @@ -1011,19 +1048,23 @@ def resolve_ingest_schemas( # default schema verbatim there would rewrite every model on disk. single = len(requested) == 1 schemas = [ - ResolvedSchema( - name=token, + _resolved_from_request( + token=token, + inspector=inspector, + enumerated=enumerated, + default_token=default_token, explicit=single, - is_default=_matches_default(token, default_token), ) for token in requested ] elif datasource_schema: schemas = [ - ResolvedSchema( - name=datasource_schema, + _resolved_from_request( + token=datasource_schema, + inspector=inspector, + enumerated=enumerated, + default_token=default_token, explicit=True, - is_default=_matches_default(datasource_schema, default_token), ) ] else: @@ -1049,9 +1090,13 @@ def qualify_sql_table(*, obj: IngestableObject, resolved: ResolvedSchema) -> str Default-schema objects stay unqualified so that widening the scan never rewrites models that already exist. + + The explicit path emits ``requested_as`` — what the user typed — not the + resolved discovery token, so upgrading ``main`` to ``fda.main`` for safe + introspection cannot leak a catalog into the persisted SQL. """ if resolved.explicit: - return f"{resolved.name}.{obj.name}" + return f"{resolved.requested_as or resolved.name}.{obj.name}" if resolved.is_default or not resolved.name: return obj.name return f"{_bare_schema(resolved.name)}.{obj.name}" @@ -1468,12 +1513,18 @@ def _default_schema_object_names( scope: IngestSchemaScope, objects: list[IngestableObject], include_views: bool, -) -> list[str]: +) -> list[str] | None: """Object names living in the connection's default schema. Derived from the objects already discovered when the default schema is in scope; otherwise listed explicitly, which costs one extra catalog call on the only path that needs it. + + ``None`` means "could not be determined" and is deliberately distinct from + the empty list. The consumer is a guard against repointing a model at a + different physical table, so an unknown answer has to fail CLOSED — an + empty list would read as "no default-schema object of that name exists" + and wave the repoint through. """ default_token = qualified_default_schema(inspector) if any(s.name == default_token for s in scope.schemas): @@ -1487,9 +1538,13 @@ def _default_schema_object_names( include_views=include_views, ) ] - except Exception as exc: # noqa: BLE001 — the guard degrades, never aborts - logger.debug("default-schema listing failed: %s", exc) - return [] + # Unknown, not empty — see the docstring. + except Exception as exc: # noqa: BLE001 + logger.warning( + "could not list the default schema; cross-schema merges will be " + "refused rather than guessed: %s", exc, + ) + return None def ingest_datasource_report( @@ -1846,7 +1901,7 @@ def _cross_schema_conflict( model_name: str, persisted: SlayerModel, fresh: SlayerModel, - default_schema_objects: set[str], + default_schema_objects: set[str] | None, ) -> SkippedTable | None: """Refuse to merge two different schemas' tables into one model. @@ -1855,6 +1910,12 @@ def _cross_schema_conflict( cannot: a default-schema model is persisted *unqualified*, so a fresh qualified object with the same bare name looks like a repair when it is actually a different table. + + ``default_schema_objects=None`` means the default schema could not be + listed. That fails CLOSED: an unqualified persisted model is treated as a + possible default-schema table and the merge is refused. Skipping a legal + repair costs a re-run; guessing wrong repoints a model at another + schema's data. """ persisted_table = persisted.sql_table or "" fresh_table = fresh.sql_table or "" @@ -1864,8 +1925,8 @@ def _cross_schema_conflict( return None conflicting = persisted_schema is not None and persisted_schema != fresh_schema - shadows_default = ( - persisted_schema is None and persisted_table in default_schema_objects + shadows_default = persisted_schema is None and ( + default_schema_objects is None or persisted_table in default_schema_objects ) if not (conflicting or shadows_default): return None @@ -1915,7 +1976,7 @@ async def _process_one_table( model_name=table_name, persisted=persisted, fresh=fresh, - default_schema_objects=default_schema_objects or set(), + default_schema_objects=default_schema_objects, ) if conflict is not None: return ProcessTableOutcome(skipped=conflict) @@ -2052,7 +2113,12 @@ async def ingest_datasource_idempotent( ) fresh_models = scan.models skipped = list(scan.skipped) - default_schema_objects = set(scan.default_schema_objects) + # ``None`` (listing failed) is carried through, not flattened to empty — + # the merge guard fails closed on it. + default_schema_objects = ( + None if scan.default_schema_objects is None + else set(scan.default_schema_objects) + ) fresh_by_name = {m.name: m for m in fresh_models} # Keyed on the LIVE OBJECT name, not the model name. ``_scoped_models_for_validation`` # compares this against ``_bare_table_name(m.sql_table)``, so using model diff --git a/slayer/engine/introspect_utils.py b/slayer/engine/introspect_utils.py index 7d450b7a..90ba2062 100644 --- a/slayer/engine/introspect_utils.py +++ b/slayer/engine/introspect_utils.py @@ -108,7 +108,8 @@ def _current_catalog(inspector: sa.engine.Inspector) -> Optional[str]: catalog = conn.exec_driver_sql("SELECT current_database()").scalar() if isinstance(catalog, str) and catalog: return catalog - except Exception: # noqa: BLE001 — probe only; the URL stem is the fallback + # Probe only — the URL stem is the fallback. + except Exception: # noqa: BLE001 pass try: database = inspector.engine.url.database @@ -117,6 +118,59 @@ def _current_catalog(inspector: sa.engine.Inspector) -> Optional[str]: return Path(database).stem if database else None +def enumerated_schema_names(inspector: sa.engine.Inspector) -> list[str]: + """Every schema token the dialect enumerates, in its own shape.""" + try: + return [n for n in (inspector.get_schema_names() or []) if isinstance(n, str)] + # Enumeration is best-effort; callers fall back to the token as given. + except Exception: # noqa: BLE001 + return [] + + +def resolve_schema_token( + inspector: sa.engine.Inspector, + token: Optional[str], + *, + enumerated: Optional[list[str]] = None, +) -> Optional[str]: + """Resolve a user-supplied schema name to a safe *discovery* token. + + A user types ``--schema main``, and a persisted ``sql_table`` records the + bare ``analytics``. On DuckDB the bare form is the dangerous one — it makes + ``get_table_names`` and ``has_table`` reach into ``ATTACH``ed catalogs — so + a bare token is upgraded to the catalog-qualified token the dialect + enumerates, when exactly one matches. Ambiguous or unknown names are + returned unchanged: guessing between two catalogs would be worse than + letting the accessors report nothing. + + Dialects that enumerate bare names resolve to themselves, so this is a + no-op everywhere except DuckDB. + """ + if not token: + return token + names = enumerated if enumerated is not None else enumerated_schema_names(inspector) + if not names or token in names: + return token + if "." in token: + # Already carries a catalog and is not a known schema — honour it + # verbatim rather than second-guessing the user. + return token + matches = [n for n in names if n.rsplit(".", 1)[-1] == token] + if len(matches) == 1: + return matches[0] + if not matches: + return token + # Several catalogs expose a schema of this name. A bare name means the + # one in the database we are connected to — the same thing an unqualified + # ``FROM main.t`` resolves to — so prefer it over guessing or giving up. + catalog = _current_catalog(inspector) + if catalog: + in_catalog = [n for n in matches if n.split(".", 1)[0] == catalog] + if len(in_catalog) == 1: + return in_catalog[0] + return token + + def qualified_default_schema( inspector: sa.engine.Inspector, ) -> Optional[str]: @@ -138,7 +192,8 @@ def qualified_default_schema( token = _compute_default_schema_token(inspector) try: setattr(inspector, _DEFAULT_SCHEMA_ATTR, token) - except Exception: # noqa: BLE001 — caching is an optimisation, not a contract + # Caching is an optimisation, not a contract. + except Exception: # noqa: BLE001 pass return token @@ -148,13 +203,13 @@ def _compute_default_schema_token( ) -> Optional[str]: try: default = inspector.default_schema_name - except Exception: # noqa: BLE001 — a dialect may not implement it + # A dialect may not implement it. + except Exception: # noqa: BLE001 return None if not isinstance(default, str) or not default: return None - try: - names = list(inspector.get_schema_names() or []) - except Exception: # noqa: BLE001 — enumeration is best-effort + names = enumerated_schema_names(inspector) + if not names: return default if default in names: return default diff --git a/slayer/engine/schema_drift.py b/slayer/engine/schema_drift.py index f0339077..a36de5c3 100644 --- a/slayer/engine/schema_drift.py +++ b/slayer/engine/schema_drift.py @@ -15,7 +15,7 @@ import asyncio import logging -from collections import Counter +from collections import defaultdict from typing import ( Annotated, Any, @@ -1668,14 +1668,57 @@ def _alias_keys(schema_token: str | None, name: str) -> list[str]: A model written before the catalog was known says ``schema.table``; one written before schemas were recorded at all says just ``table``. Both must - keep resolving, so both aliases are offered — but only inserted when they - are unambiguous across everything scanned. + keep resolving, so both aliases are offered — but a contested alias is + only inserted for the entry the database itself would pick (see + :func:`_index_live_entries`). """ if not schema_token: return [name] return [f"{schema_token.rsplit('.', 1)[-1]}.{name}", name] +def _index_live_entries( + entries: list[tuple[str | None, str, LiveTable]], + *, + default_token: str | None, +) -> dict[str, LiveTable]: + """Key the live objects on their full identity, plus shorter aliases. + + Full ``"."`` keys are always present. A shorter + alias is inserted when exactly one object claims it, and — when several + do — for the one in the connection's DEFAULT schema, because that is + precisely how the database resolves the short form: ``FROM orders`` and + ``FROM main.orders`` both land in the current catalog's default schema. + + Dropping a contested alias outright looked safer but is not: an + unqualified ``sql_table`` is how every default-schema model is persisted, + so the moment another schema happened to hold a same-named table the + legacy model stopped resolving — and an unresolvable model is a + ``WholeModelDelete`` that ``validate-models --force-clean`` deletes. The + alias is only dropped when the default schema cannot break the tie, where + a miss really is better than an arbitrary winner. + """ + out: dict[str, LiveTable] = {} + for schema_token, name, live in entries: + out[f"{schema_token}.{name}" if schema_token else name] = live + + claimants: dict[str, list[tuple[str | None, str, LiveTable]]] = defaultdict(list) + for entry in entries: + for alias in _alias_keys(entry[0], entry[1]): + claimants[alias].append(entry) + + for alias, claiming in claimants.items(): + if alias in out: + continue + if len(claiming) == 1: + out[alias] = claiming[0][2] + continue + from_default = [e for e in claiming if e[0] == default_token] + if len(from_default) == 1: + out[alias] = from_default[0][2] + return out + + def _live_schema_for_datasource( *, datasource: DatasourceConfig, @@ -1707,12 +1750,20 @@ def _live_schema_for_datasource( data-loss bug for anyone who opted out of ingesting views. """ from slayer.engine.ingestion import _dispose_quietly, list_ingestable_objects + from slayer.engine.introspect_utils import ( + qualified_default_schema, + resolve_schema_token, + ) from slayer.sql import engine_factory sa_engine = engine_factory.get_engine(datasource.resolve_env_vars()) try: inspector = sa.inspect(sa_engine) entries: list[tuple[str | None, str, LiveTable]] = [] - for schema in schemas if schemas is not None else [None]: + for requested in schemas if schemas is not None else [None]: + # The schema set is derived from persisted ``sql_table`` values, + # which carry the BARE schema — and a bare token reaches into + # ATTACHed catalogs. Upgrade it to the enumerated shape first. + schema = resolve_schema_token(inspector, requested) for obj in list_ingestable_objects( inspector=inspector, schema=schema, include_views=True ): @@ -1735,20 +1786,9 @@ def _live_schema_for_datasource( datasource.name, exc, ) - - out: dict[str, LiveTable] = {} - for schema_token, name, live in entries: - out[f"{schema_token}.{name}" if schema_token else name] = live - alias_counts: Counter[str] = Counter( - alias - for schema_token, name, _ in entries - for alias in _alias_keys(schema_token, name) + return _index_live_entries( + entries, default_token=qualified_default_schema(inspector), ) - for schema_token, name, live in entries: - for alias in _alias_keys(schema_token, name): - if alias_counts[alias] == 1 and alias not in out: - out[alias] = live - return out finally: # Same rationale as ``ingest_datasource``: this is a one-shot # admin path. Disposing releases the underlying connection so diff --git a/tests/test_ingestion_schema_qualification.py b/tests/test_ingestion_schema_qualification.py index f84a5a90..0f5cf0a6 100644 --- a/tests/test_ingestion_schema_qualification.py +++ b/tests/test_ingestion_schema_qualification.py @@ -56,6 +56,8 @@ _additive_merge_existing, _assign_model_names, _bare_table_name, + _cross_schema_conflict, + _get_pk_constraint_fallback, _is_system_schema, _print_ingest_addition, _schema_of, @@ -1080,7 +1082,54 @@ def test_column_fallback_raises_when_it_cannot_disambiguate(self, attached): finally: eng.dispose() message = str(excinfo.value) - assert "aaa.main" in message and "att_main.main" in message + assert "aaa.main" in message + assert "att_main.main" in message + + def test_a_bare_requested_schema_is_upgraded_before_discovery( + self, attached + ): + """A user types ``--schema main``, and a bare token reaches into the + ATTACHed catalog exactly as ``schema=None`` used to. The token is + upgraded to the enumerated ``att_main.main`` for discovery — but the + emitted qualifier stays the ``main`` the user asked for, because + resolving for introspection must not change the SQL we persist.""" + report = ingest_datasource_report(datasource=attached.ds, schemas=["main"]) + + names = _by_name(report.models) + assert "only_in_other" not in names, sorted(names) + assert names["in_default"].sql_table == "main.in_default" + + def test_a_bare_schema_is_upgraded_for_validation_too(self, attached): + """``validate-models`` derives its schema set from persisted + ``sql_table`` values, which carry the BARE schema — so it hits the + same hazard from the other direction.""" + live = _live_schema_for_datasource( + datasource=attached.ds, schemas=["openfda_rest"] + ) + assert _resolve_live_table( + sql_table="openfda_rest.reports", live_tables=live + ) is not None + assert not any(k.startswith("aaa.") for k in live), sorted(live) + + def test_primary_key_is_not_duplicated_across_catalogs(self, tmp_path): + """DuckDB names a PK constraint after its column, so a same-shaped + table in an ATTACHed catalog gets the SAME auto-generated name. The + INFORMATION_SCHEMA join has to carry the catalog or it matches both + and returns the column twice.""" + main_path = str(tmp_path / "att_main.duckdb") + other_path = str(tmp_path / "att_other.duckdb") + for path, extra in ((main_path, "a"), (other_path, "b")): + con = duckdb.connect(path) + con.execute("CREATE SCHEMA ofr") + con.execute(f"CREATE TABLE ofr.t(id INTEGER PRIMARY KEY, {extra} INTEGER)") + con.close() + + eng = _attached_engine(main_path, other_path) + try: + pk = _get_pk_constraint_fallback(eng, "t", "att_main.ofr") + finally: + eng.dispose() + assert pk["constrained_columns"] == ["id"] def test_all_schemas_never_produces_a_columnless_model(self, attached): """Test 31. The failure mode this whole token discipline exists to @@ -1122,10 +1171,21 @@ def test_exact_name_beats_sanitized_regardless_of_schema_order( class TestLiveSchemaKeying: - def test_ambiguous_short_keys_are_dropped_not_overwritten(self, attached): + def test_contested_short_keys_resolve_the_way_the_database_would( + self, attached + ): """Test 36. Keying the live map on ``schema.table`` alone lets one - catalog's entry overwrite another's. Full keys are always present; - shorter aliases only when unambiguous.""" + catalog's entry overwrite another's, so full keys are always present + and shorter aliases are earned. + + A contested alias goes to the DEFAULT schema's entry, because that is + exactly what the database does: ``FROM shared`` and ``FROM + main.shared`` both land in the current catalog. Dropping the alias + instead looked safer and was not — every default-schema model is + persisted UNQUALIFIED, so the moment another catalog held a same-named + table the legacy model stopped resolving, and an unresolvable model is + a ``WholeModelDelete`` that ``--force-clean`` deletes. + """ live = _live_schema_for_datasource( datasource=attached.ds, schemas=["att_main.main", "aaa.main"], @@ -1133,12 +1193,51 @@ def test_ambiguous_short_keys_are_dropped_not_overwritten(self, attached): assert live["att_main.main.shared"].columns.keys() == {"m"} assert live["aaa.main.shared"].columns.keys() == {"o"} - # ``main.shared`` and ``shared`` are claimed by both, so neither alias - # may resolve to an arbitrary winner. - assert _resolve_live_table( - sql_table="main.shared", live_tables=live - ) is None - assert _resolve_live_table(sql_table="shared", live_tables=live) is None + + for short in ("main.shared", "shared"): + resolved = _resolve_live_table(sql_table=short, live_tables=live) + assert resolved is not None, short + assert resolved.columns.keys() == {"m"}, ( + f"{short} must resolve to the current catalog, not {short!r}'s " + f"namesake in the attached one" + ) + + async def test_a_legacy_unqualified_model_survives_a_same_named_table( + self, tmp_path + ): + """The data-loss path the alias rule exists to prevent, end to end: a + model persisted before schemas were recorded (``sql_table: orders``) + must keep resolving once another schema gains its own ``orders``. + Without the default-schema tie-break it becomes a ``WholeModelDelete`` + and ``validate-models --force-clean`` deletes it.""" + db_path = str(tmp_path / "legacy.duckdb") + con = duckdb.connect(db_path) + con.execute("CREATE TABLE orders(id INTEGER, amt INTEGER)") + con.execute("CREATE SCHEMA analytics") + con.execute("CREATE TABLE analytics.orders(x INTEGER, y INTEGER)") + con.close() + ds = _duckdb_ds(db_path) + + legacy = SlayerModel( + name="orders", data_source="ds", sql_table="orders", + columns=[ + Column(name="id", type=DataType.INT), + Column(name="amt", type=DataType.INT), + ], + ) + qualified = SlayerModel( + name="analytics_orders", data_source="ds", + sql_table="analytics.orders", + columns=[ + Column(name="x", type=DataType.INT), + Column(name="y", type=DataType.INT), + ], + ) + to_delete = await validate_datasource( + datasource=ds, models=[legacy, qualified] + ) + assert [e for e in to_delete if isinstance(e, WholeModelDelete)] == [] + assert to_delete == [], to_delete def test_unambiguous_aliases_are_still_inserted(self, attached): """The alias keys are what let an unqualified legacy model keep @@ -1255,9 +1354,15 @@ def test_requested_schemas_are_marked_explicit(self, tmp_path): finally: eng.dispose() - assert [(s.name, s.explicit) for s in scope.schemas] == [ - ("openfda_rest", True) - ] + # ``name`` is the DISCOVERY token, upgraded to the catalog-qualified + # shape the dialect enumerates — a bare token would reach into an + # ATTACHed catalog. ``requested_as`` keeps what the user typed, and is + # what gets emitted, so the upgrade cannot leak into ``sql_table``. + assert [ + (s.name.rsplit(".", 1)[-1], s.requested_as, s.explicit) + for s in scope.schemas + ] == [("openfda_rest", "openfda_rest", True)] + assert scope.schemas[0].name.endswith(".openfda_rest") def test_multi_schema_scope_reports_no_hint(self, tmp_path): """The hint is for "you may be missing something". With more than one @@ -1289,7 +1394,9 @@ def test_multi_returns_objects_tagged_with_their_schema(self, tmp_path): finally: eng.dispose() - assert {(o.name, o.schema) for o in objects} == { + # Objects carry the resolved discovery token, so the bare schema the + # user asked for shows up qualified here. + assert {(o.name, o.schema.rsplit(".", 1)[-1]) for o in objects} == { ("in_default", "main"), ("reports", "openfda_rest"), } @@ -1422,6 +1529,44 @@ def _raising(inspector, sa_engine, table_name, schema): ) +class TestCrossSchemaGuardFailsClosed: + """The guard answers "is this persisted unqualified model the default + schema's table?" from a live listing. When that listing fails the answer + is UNKNOWN, and unknown has to refuse the merge — an empty list would read + as "no such default-schema object" and wave a repoint through.""" + + @staticmethod + def _models() -> tuple[SlayerModel, SlayerModel]: + persisted = SlayerModel( + name="reports", data_source="ds", sql_table="reports", + columns=[Column(name="a", type=DataType.INT)], + ) + fresh = SlayerModel( + name="reports", data_source="ds", sql_table="s2.reports", + columns=[Column(name="b", type=DataType.INT)], + ) + return persisted, fresh + + def test_unknown_default_schema_refuses_the_merge(self): + persisted, fresh = self._models() + conflict = _cross_schema_conflict( + model_name="reports", persisted=persisted, fresh=fresh, + default_schema_objects=None, + ) + assert conflict is not None + assert "cross-schema" in conflict.reason + + def test_a_known_empty_default_schema_still_allows_the_repair(self): + """Fail-closed must not become fail-always: when the listing + succeeded and genuinely holds no such object, the qualifier repair is + the whole point of the re-ingest.""" + persisted, fresh = self._models() + assert _cross_schema_conflict( + model_name="reports", persisted=persisted, fresh=fresh, + default_schema_objects=set(), + ) is None + + class TestAdditiveMergeQualifierRules: """Pinned directly on ``_additive_merge_existing``. Driving these through ingestion would let the cross-schema guard skip the model before the merge From fb4762e1b48b7742db3ed9f202b3bb3babe3f5e8 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 7 Aug 2026 13:16:37 +0200 Subject: [PATCH 3/4] fix: dedupe resolved schemas; gate token upgrade on qualifying dialects Two findings from review round 2, both reproduced first. **Two requests naming the same schema cancelled each other out.** `validate-models` derives its schema set from persisted `sql_table` values, so a datasource holding both `orders` (bare ingest) and `main.customers` (`--schema main`) asks for `None` AND `main` -- which now resolve to the same discovery token. Scanned twice, every object appeared as two rival claimants for its own alias, the default-schema tie-break found no unique winner, and the aliases were dropped from objects that have no rival at all. Measured: BOTH models became WholeModelDeletes, i.e. the fix for the previous round's data-loss bug had opened a wider one. Resolved tokens are now deduplicated before scanning, and `_index_live_entries` collapses duplicate `(schema_token, object)` pairs so the helper is correct whatever it is fed. **The catalog upgrade assumed every dot is a catalog separator.** Postgres allows `CREATE SCHEMA "foo.bar"` and lists schema names bare, so a request for a nonexistent `bar` would have silently resolved to `foo.bar` and ingested a schema the user never asked for. The upgrade is now gated on the dialect actually enumerating catalog-qualified tokens, decided from its own default schema token rather than from "does any name contain a dot". Co-Authored-By: Claude Opus 5 (1M context) --- slayer/engine/introspect_utils.py | 17 +++ slayer/engine/schema_drift.py | 37 ++++-- tests/test_ingestion_schema_qualification.py | 116 ++++++++++++++++++- 3 files changed, 158 insertions(+), 12 deletions(-) diff --git a/slayer/engine/introspect_utils.py b/slayer/engine/introspect_utils.py index 90ba2062..e95bf64d 100644 --- a/slayer/engine/introspect_utils.py +++ b/slayer/engine/introspect_utils.py @@ -127,6 +127,17 @@ def enumerated_schema_names(inspector: sa.engine.Inspector) -> list[str]: return [] +def _enumerates_qualified_tokens(inspector: sa.engine.Inspector) -> bool: + """Whether this dialect's schema tokens carry a catalog segment. + + Decided from the dialect's own default-schema token rather than from "does + any name contain a dot", so a dialect that merely permits a dot inside a + schema name is never mistaken for a catalog-qualifying one. + """ + default_token = qualified_default_schema(inspector) + return bool(default_token) and "." in default_token + + def resolve_schema_token( inspector: sa.engine.Inspector, token: Optional[str], @@ -155,6 +166,12 @@ def resolve_schema_token( # Already carries a catalog and is not a known schema — honour it # verbatim rather than second-guessing the user. return token + if not _enumerates_qualified_tokens(inspector): + # This dialect lists bare schema names, so a dot in one is part of the + # name (Postgres allows `CREATE SCHEMA "foo.bar"`), not a catalog + # separator. Reading it as one would silently ingest a schema the user + # did not ask for; an unknown name should simply find nothing. + return token matches = [n for n in names if n.rsplit(".", 1)[-1] == token] if len(matches) == 1: return matches[0] diff --git a/slayer/engine/schema_drift.py b/slayer/engine/schema_drift.py index a36de5c3..0ef6924a 100644 --- a/slayer/engine/schema_drift.py +++ b/slayer/engine/schema_drift.py @@ -1698,24 +1698,33 @@ def _index_live_entries( alias is only dropped when the default schema cannot break the tie, where a miss really is better than an arbitrary winner. """ - out: dict[str, LiveTable] = {} + # Deduplicate on identity first. Two requested schemas can resolve to the + # same discovery token (``None`` and ``main`` both become ``fda.main``), + # and a repeat would otherwise look like two rival claimants for every + # alias — silently dropping the aliases of objects that have no rival at + # all, which is the very deletion path this indexing exists to prevent. + unique: dict[tuple[str | None, str], LiveTable] = {} for schema_token, name, live in entries: + unique.setdefault((schema_token, name), live) + + out: dict[str, LiveTable] = {} + for (schema_token, name), live in unique.items(): out[f"{schema_token}.{name}" if schema_token else name] = live - claimants: dict[str, list[tuple[str | None, str, LiveTable]]] = defaultdict(list) - for entry in entries: - for alias in _alias_keys(entry[0], entry[1]): - claimants[alias].append(entry) + claimants: dict[str, list[tuple[str | None, str]]] = defaultdict(list) + for key in unique: + for alias in _alias_keys(key[0], key[1]): + claimants[alias].append(key) for alias, claiming in claimants.items(): if alias in out: continue if len(claiming) == 1: - out[alias] = claiming[0][2] + out[alias] = unique[claiming[0]] continue - from_default = [e for e in claiming if e[0] == default_token] + from_default = [k for k in claiming if k[0] == default_token] if len(from_default) == 1: - out[alias] = from_default[0][2] + out[alias] = unique[from_default[0]] return out @@ -1759,11 +1768,17 @@ def _live_schema_for_datasource( try: inspector = sa.inspect(sa_engine) entries: list[tuple[str | None, str, LiveTable]] = [] + # The schema set is derived from persisted ``sql_table`` values, which + # carry the BARE schema — and a bare token reaches into ATTACHed + # catalogs. Upgrade each to the enumerated shape, then dedupe: `None` + # and `main` both resolve to `fda.main`, and scanning it twice would + # double every entry. + scanned: list[str | None] = [] for requested in schemas if schemas is not None else [None]: - # The schema set is derived from persisted ``sql_table`` values, - # which carry the BARE schema — and a bare token reaches into - # ATTACHed catalogs. Upgrade it to the enumerated shape first. schema = resolve_schema_token(inspector, requested) + if schema not in scanned: + scanned.append(schema) + for schema in scanned: for obj in list_ingestable_objects( inspector=inspector, schema=schema, include_views=True ): diff --git a/tests/test_ingestion_schema_qualification.py b/tests/test_ingestion_schema_qualification.py index 0f5cf0a6..e32d3483 100644 --- a/tests/test_ingestion_schema_qualification.py +++ b/tests/test_ingestion_schema_qualification.py @@ -70,7 +70,11 @@ resolve_ingest_schemas, split_sql_table, ) -from slayer.engine.introspect_utils import _get_columns_fallback, _safe_get_columns +from slayer.engine.introspect_utils import ( + _get_columns_fallback, + _safe_get_columns, + resolve_schema_token, +) from slayer.engine.query_engine import SlayerQueryEngine from slayer.engine.schema_drift import ( LiveTable, @@ -1147,6 +1151,74 @@ def test_all_schemas_never_produces_a_columnless_model(self, attached): # --------------------------------------------------------------------------- +class TestSchemaTokenResolution: + def test_a_bare_token_is_upgraded_on_a_qualifying_dialect(self, tmp_path): + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + assert resolve_schema_token(insp, "openfda_rest") == "fda.openfda_rest" + # Already qualified, and already what the dialect enumerates. + assert resolve_schema_token(insp, "fda.main") == "fda.main" + finally: + eng.dispose() + + def test_an_unknown_name_is_left_alone(self, tmp_path): + """An unknown schema should find nothing, not silently become a + different one.""" + ds = _repro_ds(tmp_path) + eng, insp = _inspector_for(ds) + try: + assert resolve_schema_token(insp, "nope") == "nope" + # A catalog-qualified name we do not recognise stays verbatim + # rather than being second-guessed. + assert resolve_schema_token(insp, "other.main") == "other.main" + finally: + eng.dispose() + + def test_a_dot_in_a_bare_dialects_schema_name_is_not_a_catalog(self): + """Postgres allows ``CREATE SCHEMA "foo.bar"`` and lists schema names + BARE, so a dot there belongs to the name. Reading it as a catalog + separator would silently resolve a request for the nonexistent ``bar`` + into ``foo.bar`` and ingest a schema the user never asked for. + + Mocked because no dialect SLayer tests against can hold both + properties at once — which is exactly why it needs pinning. + """ + insp = MagicMock(spec=sa.engine.Inspector) + insp.get_schema_names.return_value = ["public", "foo.bar"] + insp.default_schema_name = "public" + + assert resolve_schema_token(insp, "bar") == "bar" + assert resolve_schema_token(insp, "public") == "public" + + def test_sqlite_tokens_pass_through_unchanged(self, tmp_path): + db_path = str(tmp_path / "plain.db") + conn = sqlite3.connect(db_path) + conn.executescript("CREATE TABLE t (id INTEGER);") + conn.commit() + conn.close() + + eng = sa.create_engine(f"sqlite:///{db_path}", poolclass=StaticPool) + try: + insp = sa.inspect(eng) + assert resolve_schema_token(insp, "main") == "main" + assert resolve_schema_token(insp, "anything") == "anything" + finally: + eng.dispose() + + def test_the_current_catalog_wins_when_several_catalogs_match( + self, attached + ): + """``aaa`` and ``att_main`` both expose ``main``. A bare ``main`` + means the database we are connected to — the same thing the engine + does for an unqualified reference.""" + eng = attached.engine() + try: + assert resolve_schema_token(sa.inspect(eng), "main") == "att_main.main" + finally: + eng.dispose() + + class TestCollisionWithSanitization: @pytest.mark.parametrize("reverse", [False, True]) def test_exact_name_beats_sanitized_regardless_of_schema_order( @@ -1202,6 +1274,48 @@ def test_contested_short_keys_resolve_the_way_the_database_would( f"namesake in the attached one" ) + async def test_two_requests_naming_the_same_schema_do_not_cancel_out( + self, tmp_path + ): + """``validate-models`` derives its schema set from persisted + ``sql_table`` values, so a datasource holding both ``orders`` (bare + ingest) and ``main.customers`` (``--schema main``) asks for ``None`` + AND ``main`` — which resolve to the same discovery token. + + Scanned twice, every object appeared as two rival claimants for its + own alias, so the tie-break saw no unique winner and dropped the + aliases of objects that have no rival at all. Both models then became + ``WholeModelDelete``s. + """ + db_path = str(tmp_path / "dupe.duckdb") + con = duckdb.connect(db_path) + con.execute("CREATE TABLE orders(id INTEGER, amt INTEGER)") + con.execute("CREATE TABLE customers(cid INTEGER)") + con.close() + ds = _duckdb_ds(db_path) + + live = _live_schema_for_datasource(datasource=ds, schemas=[None, "main"]) + for short in ("orders", "main.customers"): + assert _resolve_live_table( + sql_table=short, live_tables=live + ) is not None, f"{short} lost its alias to a duplicate scan" + + models = [ + SlayerModel( + name="orders", data_source="ds", sql_table="orders", + columns=[ + Column(name="id", type=DataType.INT), + Column(name="amt", type=DataType.INT), + ], + ), + SlayerModel( + name="customers", data_source="ds", sql_table="main.customers", + columns=[Column(name="cid", type=DataType.INT)], + ), + ] + to_delete = await validate_datasource(datasource=ds, models=models) + assert [e for e in to_delete if isinstance(e, WholeModelDelete)] == [] + async def test_a_legacy_unqualified_model_survives_a_same_named_table( self, tmp_path ): From 6a3d49ef4ad10de619156d94fe706e5c7b8b5a49 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 7 Aug 2026 13:21:59 +0200 Subject: [PATCH 4/4] fix: decide the qualifying-dialect gate from the enumeration, not the fallback Two findings from review round 3. **The gate could misclassify DuckDB as bare.** It asked `qualified_default_schema()`, which falls back to the BARE default when the current catalog cannot be determined -- and with attached catalogs supplying several `*.main` tokens, that fallback fires. The gate then reported "this dialect lists bare names", refused the upgrade, and re-armed the cross-catalog sweep the upgrade exists to prevent. It now asks where the dialect's own default schema turns up in its own enumeration: listed bare means bare tokens; absent but present as some `.` means the dialect qualifies. That is independent of catalog detection, and still keeps Postgres' `CREATE SCHEMA "foo.bar"` from being read as a catalog. **`None` was not normalised before the scan dedupe.** `None` and an explicit `main` resolved to different values (`None` stays `None`) even though `list_ingestable_objects` resolves both to the same token internally, so the schema was introspected twice and correctness rested entirely on the entry-level dedupe behind it. `None` now normalises to the default token up front. Co-Authored-By: Claude Opus 5 (1M context) --- slayer/engine/introspect_utils.py | 31 +++++++++++++++----- slayer/engine/schema_drift.py | 10 ++++++- tests/test_ingestion_schema_qualification.py | 26 +++++++++++++++- 3 files changed, 58 insertions(+), 9 deletions(-) diff --git a/slayer/engine/introspect_utils.py b/slayer/engine/introspect_utils.py index e95bf64d..967dbb1d 100644 --- a/slayer/engine/introspect_utils.py +++ b/slayer/engine/introspect_utils.py @@ -128,14 +128,31 @@ def enumerated_schema_names(inspector: sa.engine.Inspector) -> list[str]: def _enumerates_qualified_tokens(inspector: sa.engine.Inspector) -> bool: - """Whether this dialect's schema tokens carry a catalog segment. - - Decided from the dialect's own default-schema token rather than from "does - any name contain a dot", so a dialect that merely permits a dot inside a - schema name is never mistaken for a catalog-qualifying one. + """Whether this dialect prefixes its schema tokens with a catalog. + + Decided by asking where the dialect's OWN default schema turns up in its + OWN enumeration: listed bare means bare tokens (SQLite, Postgres, MySQL, + BigQuery); absent but present as some ``.`` means the + dialect qualifies (DuckDB). + + Deliberately not "does any enumerated name contain a dot" — Postgres + allows ``CREATE SCHEMA "foo.bar"``, and reading that as a catalog would + let it capture a request for a nonexistent ``bar``. Deliberately not + ``qualified_default_schema()`` either: that helper falls back to the bare + default when the current catalog cannot be determined, which would + misreport DuckDB as bare and re-arm the cross-catalog sweep. """ - default_token = qualified_default_schema(inspector) - return bool(default_token) and "." in default_token + try: + default = inspector.default_schema_name + # A dialect may not implement it. + except Exception: # noqa: BLE001 + return False + if not isinstance(default, str) or not default: + return False + names = enumerated_schema_names(inspector) + if not names or default in names: + return False + return any(n.rsplit(".", 1)[-1] == default for n in names) def resolve_schema_token( diff --git a/slayer/engine/schema_drift.py b/slayer/engine/schema_drift.py index 0ef6924a..0aafa1ca 100644 --- a/slayer/engine/schema_drift.py +++ b/slayer/engine/schema_drift.py @@ -1774,8 +1774,16 @@ def _live_schema_for_datasource( # and `main` both resolve to `fda.main`, and scanning it twice would # double every entry. scanned: list[str | None] = [] + default_token = qualified_default_schema(inspector) for requested in schemas if schemas is not None else [None]: - schema = resolve_schema_token(inspector, requested) + # ``None`` means the default schema, which is what + # ``list_ingestable_objects`` resolves it to anyway — normalise it + # here so it dedupes against an explicit request for that same + # schema instead of introspecting everything twice. + schema = ( + default_token if requested is None + else resolve_schema_token(inspector, requested) + ) if schema not in scanned: scanned.append(schema) for schema in scanned: diff --git a/tests/test_ingestion_schema_qualification.py b/tests/test_ingestion_schema_qualification.py index e32d3483..c8f86a22 100644 --- a/tests/test_ingestion_schema_qualification.py +++ b/tests/test_ingestion_schema_qualification.py @@ -31,7 +31,7 @@ import sys from pathlib import Path from types import SimpleNamespace -from unittest.mock import MagicMock +from unittest.mock import MagicMock, patch import pytest import sqlalchemy as sa @@ -1206,6 +1206,21 @@ def test_sqlite_tokens_pass_through_unchanged(self, tmp_path): finally: eng.dispose() + def test_a_qualifying_dialect_is_recognised_without_a_current_catalog( + self, + ): + """The gate must not be decided by ``qualified_default_schema``, which + falls back to the BARE default when the current catalog cannot be + determined. On DuckDB with attached catalogs that fallback would + misreport the dialect as bare and re-arm the cross-catalog sweep — so + the gate asks where the dialect's own default turns up in its own + enumeration instead.""" + insp = MagicMock(spec=sa.engine.Inspector) + insp.default_schema_name = "main" + insp.get_schema_names.return_value = ["aaa.main", "att_main.main", "att_main.ofr"] + + assert resolve_schema_token(insp, "ofr") == "att_main.ofr" + def test_the_current_catalog_wins_when_several_catalogs_match( self, attached ): @@ -1300,6 +1315,15 @@ async def test_two_requests_naming_the_same_schema_do_not_cancel_out( sql_table=short, live_tables=live ) is not None, f"{short} lost its alias to a duplicate scan" + # `None` normalises to the same discovery token as `main`, so the + # schema is introspected once, not twice. + with patch( + "slayer.engine.ingestion.list_ingestable_objects", + side_effect=ingestion_module.list_ingestable_objects, + ) as listed: + _live_schema_for_datasource(datasource=ds, schemas=[None, "main"]) + assert listed.call_count == 1, listed.call_args_list + models = [ SlayerModel( name="orders", data_source="ds", sql_table="orders",