feat(DEV-1809): import column/table/dataset comments during schema ingestion

.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), importing DB column/table comments into `description`s (fill-if-empty, never overwriting; BigQuery also fills the datasource description from the dataset description; SQLite has no comments). 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.
 - **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

.github/workflows/ci.yml

@@ -141,6 +141,13 @@ jobs:
         timeout-minutes: 5
         run: poetry run python examples/bigquery/verify.py
 
+      - name: Run BigQuery integration tests
+        if: steps.gate.outputs.skip != 'true'
+        timeout-minutes: 10
+        env:
+          GCP_PROJECT_ID: ${{ secrets.GCP_PROJECT_ID }}
+        run: poetry run pytest tests/integration/test_integration_bigquery.py -m integration -v
+
       - name: Dump server logs on failure
         if: failure() && steps.gate.outputs.skip != 'true'
         run: cat /tmp/slayer-bq.log

DECISIONS.md

@@ -69,3 +69,4 @@ implementation detail. Include issue refs when known.
 - 2026-08-03 — Optional blocks + Cube JS/FILTER_PARAMS import (DEV-1730 / #270): a Mode-A-only `{? ... ?}` block renders its content parenthesised when every inner `{var}` is supplied, else collapses to the neutral `(1=1)` — the SLayer form of a Cube `FILTER_PARAMS` optional pushdown. Blocks live in the same `substitute_variables` (escape="sql") scanner as `{var}`/`{{`/`}}`, must contain ≥1 var, do not nest, and are rejected in Mode-B. A block-bearing model runs substitution even on a zero-variable call so its blocks collapse (the `_substitute_model_sql_surfaces` fast-path now checks for `{?` too); a block-free, required-only model with zero variables is still left untouched (the documented DEV-1625 raw-brace-literal boundary). `extract_model_variables(model)` derives required (bare, no default) vs optional (in-block or defaulted) from the four Mode-A surfaces — structural, nothing persisted, surfaced additively in the inspect skeleton `Variables:` line. The Cube importer gains a **JavaScript front-end** (esprima ESTree parser, a new core dep) that parses `cube()`/`view()` into the same `CubeCube`/`CubeView` shapes as YAML (dynamic values → report + skip member). FILTER_PARAMS refs are carried JS→converter as structured `CubeFilterParamRef` on the transient `CubeCube` (sentinels in the surface text; no arrow-body re-parse, sidestepping the `{var}`-vs-`{FILTER_PARAMS…}` brace clash); the converter resolves sentinels AFTER `translate_cube_refs` so the introduced `{var}` are never eaten. Requiredness (bare vs block) is decided in the converter alone via `honor_required_meta` (default on; CLI `--ignore-required-meta`) AND the member's `meta.required`; with the flag off a scalar-position arrow collapses to Cube's own `(1=1)::TIMESTAMP` booby-trap, faithfully. Cross-cube refs, unknown members, and generated-name collisions (`d`→`d_from` clashing member `d_from`) drop the cube (`filter_params_unsupported`); each logical variable is reported once (`filter_params_variable`) and stashed in `meta.cube_variables`. `render_probe_text` (blocks→`(1=1)`, bare vars→`0`) is the single import-time validation renderer, matching runtime collapse.
 - 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-20 — DB comments imported at ingestion (DEV-1809): column comments → `Column.description`, table comments → `SlayerModel.description`, and (BigQuery only) the dataset description → `DatasourceConfig.description` — strictly **fill-if-empty**, preserving the DEV-1356 additive-only doctrine with no provenance tracking (a comment later edited in the DB does not propagate over an existing description; clear the field and re-ingest). The introspection contract moved from positional 5-tuples to the Pydantic `IntrospectedColumn`, and the sync ingest core became `_ingest_datasource_full(...) -> DatasourceIngestOutput` (public `ingest_datasource` stays a models-only wrapper) so the dataset description is fetched inside the live engine session — used by the idempotent path, MCP `create_datasource`, and CLI `datasources create --ingest`. The Inspector path carries comments for Postgres/MySQL/SQL Server/Snowflake/ClickHouse/BigQuery; the `information_schema` fallback gained per-dialect comment SQL for the "cheap five" (MySQL `COLUMN_COMMENT`, Snowflake `COMMENT`, ClickHouse `system.columns`, DuckDB `duckdb_columns()`, Postgres `col_description()`) — DuckDB's comments come ONLY via the fallback since its `Inspector.get_columns` crashes on the pg_catalog emulation; SQL Server/BigQuery fallbacks stay types-only (Inspector already delivers; `sys.extended_properties` / region-qualified `INFORMATION_SCHEMA` disproportionate). SQLite has no comments. Fetching is always best-effort (silent skip, never a failed ingest); schema-level comments have no generic Inspector API, hence BigQuery-only datasource descriptions via the same private `connection.connection._client` handle sqlalchemy-bigquery itself uses (dataset resolution: explicit `--schema` → dialect default dataset → `schema_name`). Report shape: `ModelAddition.described_columns`/`model_described`, `IdempotentIngestResult.datasource_described` (populated for created models too); `save_datasource` failures isolate as `IngestionError(model_name="")`. dbt hidden-model import now lets curated dbt descriptions WIN over DB comments at creation time (OSI already had `or`-precedence); DB comments fill only gaps. Drift detection ignores comment text. BigQuery coverage is two-layer: credential-free driver-contract tests run the real `BigQueryDialect` reflection over a locally built `Table` (only `client.get_table` mocked), and a new live suite `tests/integration/test_integration_bigquery.py` (wired into the CI `bigquery-example` job) creates a temp dataset with descriptions in the billing project, ingests, asserts the full comment/dataset-description/re-ingest semantics, and deletes it — the CI SA therefore needs dataset create/delete rights (`roles/bigquery.user`), not just `jobUser`. The `examples/bigquery/verify.py` script itself is unchanged (it never ingests — the public dataset is not cross-project introspectable).

docs/concepts/ingestion.md

@@ -42,6 +42,7 @@ Tables with no FK references use their plain table name with no joins.
 SLayer introspects each table's column types and generates a model:
 
 - **One `Column`** per non-joined column on the source table — name, `type` inferred from the database (`string` / `number` / `boolean` / `time` / `date`), `primary_key=True` for PKs. Whether each column is used as a group-by dimension or as an aggregation source is decided per query.
+- **Column and table comments** stored in the database become `Column.description` and `SlayerModel.description` (see [Comments and descriptions](#comments-and-descriptions) below).
 - **A column literally named `count`** is renamed to `count_col` to avoid clashing with the always-available `*:count`.
 - **No auto-generated `measures`** — `SlayerModel.measures` is the named-formula library and stays empty after ingestion. You can add named formulas later via the API/MCP if you want bare-name shortcuts (`{"formula": "aov"}`).
 - **`*:count`** is always available without any model definition.
@@ -51,6 +52,36 @@ FK columns from referenced tables are excluded from the source model to avoid re
 
 All models use `sql_table` (the source table) plus `joins` (direct FK joins only, storing source/target column pairs). Multi-hop JOINs are resolved dynamically at query time by walking the join graph.
 
+### Comments and descriptions
+
+Textual metadata stored in the database is imported during ingestion, so
+agents see the same documentation the DBA wrote:
+
+- **Column comments** → `Column.description`
+- **Table comments** → `SlayerModel.description`
+- **BigQuery dataset description** → the datasource's `description`
+  (BigQuery only — no cross-database API exists for schema-level comments)
+
+The import is strictly **fill-if-empty**: a comment lands only where the
+existing `description` is empty. Hand-written descriptions are never
+overwritten, on first ingest or any re-ingest — consistent with the
+additive-only re-ingestion contract. To re-import a comment, clear the
+description and re-run `slayer ingest`. Imported descriptions feed the
+search corpus like any other description, so the next embedding refresh
+picks them up.
+
+Per-database support:
+
+| Database | Column comments | Table comments |
+|---|---|---|
+| BigQuery | ✓ (field descriptions) | ✓ (table description) |
+| Postgres, MySQL, SQL Server, Snowflake, ClickHouse | ✓ | ✓ |
+| DuckDB | ✓ (`COMMENT ON`) | ✓ |
+| SQLite | — (no such feature) | — |
+
+Comment fetching is best-effort: a database or driver that cannot surface
+comments simply yields models without descriptions, never a failed ingest.
+
 ### SQLite affinity probing
 
 SQLite's declared column types are affinity hints, not strict constraints: a column declared `INTEGER` can store `INTEGER`, `REAL`, `TEXT`, or `BLOB` values per row. To prevent silent truncation downstream (a column declared `INTEGER` but actually storing `0.99` would cast to `0` and break `AVG`/`SUM` results), SLayer runs an additional value-level probe on SQLite ingestion for every column the inspector reports as `INTEGER`-affinity.
@@ -273,7 +304,7 @@ Ingest-on-startup: N/M datasources ingested (K failed: name1, name2)
 `slayer ingest` (and the equivalent MCP / REST entry points) is idempotent by default — re-runs are safe. For each in-scope live table:
 
 - **No persisted model with that name** → ingest from scratch via the path above.
-- **Existing `sql_table`-mode model** → append new columns and joins from the live schema. Existing columns and joins are **never** mutated — `description`, `label`, `format`, `meta`, and `allowed_aggregations` are preserved verbatim.
+- **Existing `sql_table`-mode model** → append new columns and joins from the live schema. Existing columns and joins are **never** mutated — `label`, `format`, `meta`, and `allowed_aggregations` are preserved verbatim, and `description` too, with one additive exception: an **empty** description is filled from the live DB comment (column and model level). Filled columns are reported per model (`ModelAddition.described_columns`, `model_described`; the CLI prints `+descriptions: …`).
 - **Existing `sql`-mode or query-backed model with the matching name** → skipped silently; those are user-authored.
 
 With the default YAML storage, two live tables whose quoted names differ only by letter case (`"Orders"` vs `orders`) cannot both be persisted — model names collide as filenames on macOS / Windows, so the save is rejected (`IdCollisionError`). The first table wins; the second surfaces as a per-model entry in `IdempotentIngestResult.errors` (or a per-model message on the CLI / MCP paths) without aborting the rest of the ingest. SQLite storage persists both.