Quote model-name identifiers so reserved-keyword model names work

BUG-reserved-keyword-identifiers.md

@@ -0,0 +1,186 @@
+# Bug: model names that are reserved SQL keywords are emitted unquoted, producing invalid SQL
+
+| | |
+|---|---|
+| **Reported** | 2026-05-22 |
+| **Reporter** | artemy@motley.ai |
+| **Component** | `slayer/sql/generator.py` (SQL rendering), `slayer/engine/column_expansion.py` (column qualification) |
+| **Version / commit** | 0.6.9 / `a8b8f2c` (branch `main`) |
+| **Dialect** | postgres (default), affects any dialect with reserved keywords |
+| **Severity** | High — any query against a model whose name is a reserved SQL keyword fails outright |
+| **Found via** | Storyline datasource ingestion in shadow-compare mode (SLayer as shadow); customer schema has a table `Grant` |
+
+## Summary
+
+When a model's name is a **reserved SQL keyword** (e.g. `grant`, `order`, `user`, `select`),
+SLayer emits that name as an **unquoted** identifier in the generated SQL — both as the
+`FROM` table alias and as the column qualifier. The database then rejects the statement
+with a syntax error.
+
+The trigger in the wild: a source table named `Grant` becomes a SLayer model named `grant`.
+The table name itself is correctly quoted (`"Grant"`, because it is mixed-case), but the
+derived alias/qualifier `grant` is not — and `grant` is a reserved keyword.
+
+## Symptoms
+
+Observed during a shadow-compare ingestion run:
+
+```
+'grant."idempotencyKey"' contains unsupported syntax. Falling back to parsing as a 'Command'.
+shadow_compare: load_data shadow backend raised ProgrammingError:
+(sqlalchemy.dialects.postgresql.asyncpg.ProgrammingError)
+<class 'asyncpg.exceptions.PostgresSyntaxError'>: syntax error at or near "grant"
+[SQL: SELECT
+  grant ."idempotencyKey" AS "grant.idempotencyKey"
+FROM "Grant" AS grant
+GROUP BY
+  grant ."idempotencyKey"
+LIMIT 21]
+```
+
+There are **two distinct failure surfaces**, both caused by `grant` being an unquoted
+reserved keyword:
+
+### A. Rendering (the fatal error)
+
+The generated SQL contains `FROM "Grant" AS grant` and `grant."idempotencyKey"`. PostgreSQL
+rejects `grant` as an unquoted table alias / column qualifier because it is a reserved word →
+`PostgresSyntaxError: syntax error at or near "grant"`.
+
+### B. Parsing (the warning + the mangled `grant .` spacing)
+
+The warning comes from `sqlglot.parse_one` being handed a qualified-column **string** of the
+form `grant."idempotencyKey"`. Because `grant` begins a `GRANT` statement, sqlglot cannot
+parse it as a column and falls back to a `Command`. The misparse is what leaks the tell-tale
+`grant .` spacing (space before the dot) into the final SQL.
+
+## Root cause
+
+`grant` is a reserved SQL keyword, but SLayer builds it into an `exp.Identifier` **without
+`quoted=True`**, and sqlglot's default generator only auto-quotes identifiers it deems unsafe
+by *case / special-char* rules — it does **not** quote reserved keywords. A bare lowercase
+`grant` looks "safe" to sqlglot, so it is emitted verbatim.
+
+### Affected code
+
+The FROM-clause builder for the simple single-model path:
+
+```python
+# slayer/sql/generator.py:1920
+def _build_from_clause(self, enriched: EnrichedQuery) -> exp.Expression:
+    if enriched.sql_table:
+        return exp.to_table(enriched.sql_table, alias=enriched.model_name)   # <-- alias unquoted
+    elif enriched.sql:
+        parsed = self._parse(enriched.sql)
+        return exp.Subquery(this=parsed, alias=exp.to_identifier(enriched.model_name))  # <-- unquoted
+    ...
+```
+
+The column qualifier for a bare-column dimension:
+
+```python
+# slayer/sql/generator.py:2116
+if sql is None:
+    return exp.Column(this=exp.to_identifier(name), table=exp.to_identifier(model_name))  # <-- table unquoted
+if sql.isidentifier():
+    return exp.Column(this=exp.to_identifier(sql), table=exp.to_identifier(model_name))   # <-- table unquoted
+```
+
+The column-expansion qualifier rewrite:
+
+```python
+# slayer/engine/column_expansion.py:202
+col.set("table", exp.to_identifier(canonical_alias))   # <-- unquoted
+```
+
+Other sites that build identifiers from model names the same way (cross-model / join / window
+paths) and would fail identically for a reserved-keyword model name:
+
+- `generator.py:669` — source subquery `alias=exp.to_identifier(cm.source_model_name)`
+- `generator.py:672` — `exp.to_table(cm.source_sql_table, alias=cm.source_model_name)`
+- `generator.py:679` — target subquery `alias=exp.to_identifier(cm.target_model_name)`
+- `generator.py:682` — `exp.to_table(cm.target_model_sql_table, alias=cm.target_model_name)`
+- `generator.py:685-686` — join `ON` column qualifiers `table=exp.to_identifier(cm.{source,target}_model_name)`
+- `generator.py:803, 806` — join-target alias (last/ranked path)
+- `generator.py:1210, 1213` — join-target alias (window path)
+
+(The synthetic internal aliases `_src`, `_base`, `_w_time`, etc. are underscore-prefixed and
+never reserved, so they are unaffected.)
+
+## Minimal reproduction
+
+```python
+import sqlglot
+from sqlglot import exp
+
+# Mirrors SQLGenerator._build_from_clause + _resolve_sql for a bare-column dimension.
+frm = exp.to_table('"Grant"', alias="grant", dialect="postgres")
+sel = (
+    exp.Select()
+    .select(
+        exp.Column(
+            this=exp.to_identifier("idempotencyKey", quoted=True),
+            table=exp.to_identifier("grant"),
+        ).as_("grant.idempotencyKey")
+    )
+    .from_(frm)
+)
+print(sel.sql(dialect="postgres", pretty=True))
+# SELECT
+#   grant."idempotencyKey" AS "grant.idempotencyKey"
+# FROM "Grant" AS grant      <-- invalid: `grant` is a reserved keyword
+
+# sqlglot does NOT quote reserved-keyword identifiers by default:
+for name in ["grant", "orders", "select"]:
+    print(name, "->", exp.to_table('"T"', alias=name, dialect="postgres").sql(dialect="postgres"))
+# grant  -> "T" AS grant
+# orders -> "T" AS orders
+# select -> "T" AS select
+
+# And the parse-side fallback (surface B):
+sqlglot.parse_one('grant."idempotencyKey"', dialect="postgres")
+# WARNING: 'grant."idempotencyKey"' contains unsupported syntax. Falling back to parsing as a 'Command'.
+# parsed as Command -> 'grant ."idempotencyKey"'   (note the `grant .` spacing)
+```
+
+(Reproduced with sqlglot 26.33.0; the no-quote-for-reserved-keywords behavior is sqlglot's
+documented default and is stable across versions.)
+
+## Suggested fix
+
+Quote every identifier derived from a **model name** (alias and column qualifier), at every
+site listed above. The lowest-risk approach is a single helper used uniformly:
+
+```python
+def _model_identifier(name: str) -> exp.Identifier:
+    """Identifiers derived from model names may collide with reserved
+    keywords (e.g. a `Grant` table -> model `grant`); always quote them."""
+    return exp.to_identifier(name, quoted=True)
+```
+
+- Replace `alias=enriched.model_name` (string) with `alias=_model_identifier(enriched.model_name)`
+  in `_build_from_clause`, and likewise the cross-model/join/window alias sites.
+- Replace `table=exp.to_identifier(model_name)` with `table=_model_identifier(model_name)` in
+  `_resolve_sql` and `column_expansion.py:202`.
+
+Surface B additionally needs the **parse** sites that round-trip a qualified column **string**
+through `sqlglot.parse_one` (e.g. the `self._parse(sql)` branch in `_resolve_sql`, predicate
+parsing, and column-expansion enrichment) to either build the qualifier as a quoted identifier
+before parsing, or avoid the string round-trip. Otherwise the `Command` fallback can still
+silently corrupt the expression even if the final render is quoted.
+
+Note: rendering the final statement with `.sql(dialect=..., identify=True)` would blanket-quote
+all identifiers and fix surface A, but it does **not** fix surface B (the parse fallback happens
+before rendering) and broadly changes emitted SQL, so the targeted helper is preferred.
+
+## Suggested regression test
+
+Add a model whose name is a reserved keyword (e.g. a `Grant` table → model `grant`, with a
+mixed-case column like `idempotencyKey`) and assert that:
+
+1. A dimension-only query renders valid SQL (`FROM "Grant" AS "grant"`, `"grant"."idempotencyKey"`)
+   and executes against Postgres without a syntax error.
+2. No `Falling back to parsing as a 'Command'` warning is emitted while building the query.
+
+Parametrizing over a few reserved words (`grant`, `order`, `user`, `select`) and dialects
+(postgres, sqlite) would guard the cross-model/join/window paths too.

CLAUDE.md

@@ -48,6 +48,7 @@ poetry run ruff check slayer/ tests/
 - Use keyword arguments for functions with more than 1 parameter
 - Imports at the top of files
 - SQL generation uses sqlglot AST building (not string concatenation)
+- **Reserved-keyword identifier quoting**: every SLayer model name / FROM alias / column qualifier is emitted as a **quoted** identifier (`FROM public.orders AS "orders"`, `"orders"."status"`, `LEFT JOIN customers AS "customers" ON "orders".customer_id = "customers".id`) via the `_model_identifier(name)` helper in `slayer/sql/generator.py` (which wraps `exp.to_identifier(name, quoted=True)`). Quoting is **unconditional** — sqlglot's default generator does NOT quote reserved keywords, so a model named after a reserved SQL word (a source table `Grant` → model `grant`, or `order`/`user`/`select`) would otherwise be emitted bare and rejected by the DB (and, when a model-qualified string like `grant."col"` is re-parsed mid-pipeline, sqlglot falls back to a `Command` statement parse — corrupting the SQL with a tell-tale `grant .` spacing). Routing every model-name identifier through the one helper also guarantees alias definitions and qualifier references quote identically (required on case-folding dialects like Snowflake). NOTE: only the **table/qualifier** side is quoted by the helper — bare column names are left to sqlglot's own rules; `CAST(... identify=True)`-style blanket quoting is intentionally NOT used (it doesn't fix the mid-pipeline re-parse and over-quotes). The handful of sites that build a qualified ref as a raw SQL *string* destined for re-parsing (join conditions in `enrichment.py`, the ranked-subquery JOIN, the WHERE-qualifier builder) quote only the alias token via `f'"{alias}".{col}'` (`_qualify_in_string`); strings re-parsed by the Python-AST DSL parser (`parse_filter` — e.g. the rerooted-filter qualifier in `query_engine.py`) must stay BARE there (a double-quoted `"x"` is a Python string literal) and are quoted later in `resolve_filter_columns`. The join-path-discovery regex (`_TABLE_COL_RE` in `enrichment.py`) is quote-tolerant so it still matches the now-quoted expanded-column refs. See `BUG-reserved-keyword-identifiers.md`.
 - **Two reference modes** (DEV-1369): SLayer has exactly two expression layers and the rules differ by design. **Mode A (SQL)** covers `Column.sql`, `Column.filter`, and `SlayerModel.filters` — sqlglot-parsed free SQL accepting any function call (`json_extract`, `coalesce`, `CASE WHEN`, …), bare names referencing the underlying table, and `__`-delimited join paths (`customers__regions.name` — `__` between hops, single dot before the leaf). **Mode B (DSL)** covers `ModelMeasure.formula`, `SlayerQuery.measures`, `SlayerQuery.filters`, and every other query field — Python-AST DSL accepting only `Column` / `ModelMeasure` references, single-dot dotted paths through joins, aggregation colon syntax (`revenue:sum`, `*:count`), transform calls, and arithmetic/boolean ops. DSL mode rejects raw SQL function calls, `__` in user input, and bare names that don't resolve. The internal carve-out: `Column.name` accepts `__` because `_query_as_model` flattens joined columns into virtual-model columns like `stores__name`. Single source of truth: [docs/concepts/references.md](docs/concepts/references.md). Predicate-promotion (DEV-1336) is removed — a query filter naming a `Column` whose `sql` contains a window function now raises with a suggestion to use a rank-family transform (`rank(<measure>) <= N`, etc.). DEV-1378 closed two implementation gaps: (1) Mode A model filters now route through `parse_sql_predicate` at enrichment time (previously they hit the DSL parser and `Column.filter`/`SlayerModel.filters` containing arbitrary SQL functions raised at runtime); (2) `SlayerQuery.filters` (Mode B) accepts a small lowercase allowlist of string-hygiene scalars — `lower`, `upper`, `trim`, `replace`, `substr`, `instr`, `length`, `concat` — plus the SQL `||` concat operator (rewritten to `concat(...)`).
 - Models, columns, measures (formulas), and aggregations have an optional `meta: Dict[str, Any]` field for arbitrary user-defined JSON metadata. Persisted in storage, editable via MCP (`edit_model`), HTTP API, and CLI. `inspect_model` renders `meta` for any entity that has it set; the column is auto-pruned when no entity in the section uses meta.
 - **Schema versioning**: `SlayerModel`, `SlayerQuery`, and `DatasourceConfig` carry a `version: int` (currently `6` for `SlayerModel`, `3` for `SlayerQuery`, `1` for `DatasourceConfig`). On load, older versions are upgraded via the converter chain in `slayer/storage/migrations.py` before Pydantic validates the dict. Saves always emit the current version. The hook is on the Pydantic class itself (`@model_validator(mode="before")`), so every storage backend — YAML, SQLite, third-party backends, plus MCP/API/dbt entry points — gets migrations automatically without backend changes. The v1→v2 converter (in `slayer/storage/v2_migration.py`) merges v1 `dimensions`+`measures` into v2 `columns`, repurposes `measures` to hold `ModelMeasure` formulas, and renames `SlayerQuery.fields`→`measures`. The v2→v3 converter (in `slayer/storage/v3_migration.py`) drops the legacy `dry_run`/`explain` fields from `SlayerQuery` (they are now engine kwargs only — `engine.execute(query, dry_run=..., explain=...)`) and walks `SlayerModel.source_queries` entries through the SlayerQuery chain. The v3→v4 converter (in `slayer/storage/v4_migration.py`) requires non-empty `data_source` on table-backed SlayerModel dicts (`sql_table` or `sql` mode); query-backed models (`source_queries` set) are exempt because their `data_source` is filled by `engine._validate_and_populate_cache` from the resolved virtual model before save. The v4 converter also ships layout migrators that move legacy `models/<name>.yaml` flat files into `models/<data_source>/<name>.yaml` and rebuild the SQLite `models` table with a composite `(data_source, name)` primary key. The v4→v5 converter (in `slayer/storage/v5_migration.py`, DEV-1361) coarse-renames `Column.type` legacy values to the new sqlglot-aligned vocabulary (`string→TEXT`, `number→DOUBLE`, `time→TIMESTAMP`, etc.) and strips the dead aggregation pseudo-types (`count`/`sum`/...). The v5→v6 converter (in `slayer/storage/v6_migration.py`, DEV-1375) is a no-op forward — v6 introduces a single new optional field, `Column.sampled: Optional[str]`, that caches the per-column sample-value snapshot consumed by [`search`](docs/concepts/search.md) and `inspect_model`. Storage backends additionally introspect each model's datasource on first load and refine `DOUBLE → INT` for base columns whose live SQL type is integer (`slayer/storage/type_refinement.py`); the refined model is written back so subsequent loads skip both steps. `SlayerQuery` v3 sets `extra="forbid"` so unknown fields raise. See [docs/concepts/models.md](docs/concepts/models.md#schema-versioning).

slayer/engine/column_expansion.py

@@ -25,6 +25,7 @@
 
 from slayer.core.errors import ColumnCycleError
 from slayer.core.models import Column, SlayerModel
+from slayer.sql.generator import _model_identifier
 
 ResolveModel = Callable[..., Awaitable[Optional[SlayerModel]]]
 
@@ -197,9 +198,8 @@ async def _process_column_node(
 
     target_col = target_model.get_column(col_name)
     if target_col is None or _is_trivial_base(column=target_col):
-        # Base column or unknown identifier on a known target model:
-        # rewrite the table to the canonical alias and stop.
-        col.set("table", exp.to_identifier(canonical_alias))
+        # Rewrite table to the canonical alias, quoted so reserved-keyword models survive the re-parse.
+        col.set("table", _model_identifier(canonical_alias))
         return
 
     # DEV-1410 scope guard: only inline derived-column bodies when the