diff --git a/BUG-reserved-keyword-identifiers.md b/BUG-reserved-keyword-identifiers.md new file mode 100644 index 00000000..198d21b8 --- /dev/null +++ b/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) +: 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. diff --git a/CLAUDE.md b/CLAUDE.md index 071950ea..73d4f225 100644 --- a/CLAUDE.md +++ b/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() <= 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/.yaml` flat files into `models//.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). diff --git a/slayer/engine/column_expansion.py b/slayer/engine/column_expansion.py index 3653e4ef..d99aca6e 100644 --- a/slayer/engine/column_expansion.py +++ b/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 diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index fdaa9132..f7b3130a 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -49,10 +49,12 @@ EnrichedTransform, ) from slayer.sql.sql_predicate import parse_sql_predicate +from slayer.sql.generator import quote_ident_for from slayer.sql.window_detect import WINDOW_IN_FILTER_ERROR, has_window_function _SELF_JOIN_TRANSFORMS = {"time_shift"} -_TABLE_COL_RE = re.compile(r"\b([a-zA-Z_]\w*)\.([a-zA-Z_]\w*)\b") +# ``.`` for join-path discovery; ``"?`` tolerates quoted reserved-keyword aliases. +_TABLE_COL_RE = re.compile(r'"?\b([a-zA-Z_]\w*)\b"?\."?\b([a-zA-Z_]\w*)\b"?') def _strip_string_literal(value: str) -> str: """Strip one layer of single/double quotes from a query parameter value.""" if len(value) >= 2 and value[0] == value[-1] and value[0] in ("'", '"'): @@ -2457,7 +2459,10 @@ async def _resolve_joins( # Build join condition join_conds = [] for src_col, tgt_col in join.join_pairs: - join_conds.append(f"{current_alias}.{src_col} = {hop_alias}.{tgt_col}") + # Dialect-quoted so reserved-keyword aliases survive the generator's re-parse. + src_q = quote_ident_for(current_alias, dialect) + tgt_q = quote_ident_for(hop_alias, dialect) + join_conds.append(f"{src_q}.{src_col} = {tgt_q}.{tgt_col}") resolved_joins[hop_alias] = (target_table, hop_alias, " AND ".join(join_conds), str(join.join_type)) @@ -2701,6 +2706,8 @@ async def _expanded_sql_expr(*, sql_expr: str, owning_model: SlayerModel, sql_expr = dim.sql or col_name if sql_expr.isidentifier(): qualified = f"{model_name}.{sql_expr}" + # Quote the alias for the emitted SQL; resolved_columns stays bare. + repl_sql = f"{quote_ident_for(model_name, dialect)}.{sql_expr}" else: qualified = await _expanded_sql_expr( sql_expr=sql_expr, @@ -2708,9 +2715,10 @@ async def _expanded_sql_expr(*, sql_expr: str, owning_model: SlayerModel, alias_path=model_name, is_root=True, ) + repl_sql = qualified resolved_sql = _re.sub( rf"(? exp.Exp return expr return exp.Cast(this=expr, to=exp.DataType(this=target)) + +def _model_identifier(name: str) -> exp.Identifier: + """Quoted identifier for a model name / FROM alias / column qualifier. + + Always quoted: sqlglot doesn't quote reserved keywords, so a model named + ``grant``/``order``/``select`` would otherwise emit bare and break. One + helper keeps alias defs and references quoted identically (matters on + case-folding dialects like Snowflake). + """ + return exp.to_identifier(name, quoted=True) + + +def quote_ident_for(name: str, dialect: Optional[str]) -> str: + """Quoted-identifier text for ``name`` in ``dialect`` (double quotes / MySQL + backticks / T-SQL brackets). For raw-string sites re-parsed with the same + dialect — a hardcoded ``"`` is a string literal on backtick dialects. + """ + return exp.to_identifier(name, quoted=True).sql(dialect=dialect or "postgres") + + +def _qualify_in_string(alias: str, col: str, dialect: Optional[str]) -> str: + """Dialect-quoted ``alias.col`` for embedding in a re-parsed SQL string. + + Quoting the alias dodges sqlglot's Command-statement fallback for + reserved-keyword model names (bare ``grant.col`` parses as ``GRANT``). + """ + return f"{quote_ident_for(alias, dialect)}.{col}" + + logger = logging.getLogger(__name__) # Maps aggregation name (string) → SQL function name. @@ -666,24 +695,24 @@ def _build_combined(self, enriched: EnrichedQuery, if cm.source_sql: source_from = exp.Subquery( this=self._parse(cm.source_sql), - alias=exp.to_identifier(cm.source_model_name), + alias=_model_identifier(cm.source_model_name), ) else: - source_from = exp.to_table(cm.source_sql_table, alias=cm.source_model_name) + source_from = exp.to_table(cm.source_sql_table, alias=_model_identifier(cm.source_model_name)) select = select.from_(source_from) # JOIN target model if cm.target_model_sql: target_join = exp.Subquery( this=self._parse(cm.target_model_sql), - alias=exp.to_identifier(cm.target_model_name), + alias=_model_identifier(cm.target_model_name), ) else: - target_join = exp.to_table(cm.target_model_sql_table, alias=cm.target_model_name) + target_join = exp.to_table(cm.target_model_sql_table, alias=_model_identifier(cm.target_model_name)) join_on = exp.and_(*( exp.EQ( - this=exp.Column(this=exp.to_identifier(src), table=exp.to_identifier(cm.source_model_name)), - expression=exp.Column(this=exp.to_identifier(tgt), table=exp.to_identifier(cm.target_model_name)), + this=exp.Column(this=exp.to_identifier(src), table=_model_identifier(cm.source_model_name)), + expression=exp.Column(this=exp.to_identifier(tgt), table=_model_identifier(cm.target_model_name)), ) for src, tgt in cm.join_pairs )) @@ -800,10 +829,10 @@ def _build_combined(self, enriched: EnrichedQuery, if target_table.startswith("("): join_target = exp.Subquery( this=self._parse(target_table), - alias=exp.to_identifier(target_alias), + alias=_model_identifier(target_alias), ) else: - join_target = exp.to_table(target_table, alias=target_alias) + join_target = exp.to_table(target_table, alias=_model_identifier(target_alias)) join_on = self._parse(join_cond) select = select.join(join_target, on=join_on, join_type=jtype.upper()) @@ -1207,10 +1236,10 @@ def _build_window_source_select( if target_table.startswith("("): join_target = exp.Subquery( this=self._parse(target_table), - alias=exp.to_identifier(target_alias), + alias=_model_identifier(target_alias), ) else: - join_target = exp.to_table(target_table, alias=target_alias) + join_target = exp.to_table(target_table, alias=_model_identifier(target_alias)) join_on = self._parse(join_cond) select = select.join(join_target, on=join_on, join_type=jtype.upper()) @@ -1425,10 +1454,10 @@ def _generate_base(self, enriched: EnrichedQuery, # Inline-SQL target: parse as subquery parsed_target = self._parse(target_table) join_target = exp.Subquery( - this=parsed_target, alias=exp.to_identifier(target_alias), + this=parsed_target, alias=_model_identifier(target_alias), ) else: - join_target = exp.to_table(target_table, alias=target_alias) + join_target = exp.to_table(target_table, alias=_model_identifier(target_alias)) join_on = self._parse(join_cond) select = select.join(join_target, on=join_on, join_type=jtype.upper()) @@ -1919,10 +1948,10 @@ def _resolve_order_column(col, enriched: EnrichedQuery) -> str: def _build_from_clause(self, enriched: EnrichedQuery) -> exp.Expression: if enriched.sql_table: - return exp.to_table(enriched.sql_table, alias=enriched.model_name) + return exp.to_table(enriched.sql_table, alias=_model_identifier(enriched.model_name)) elif enriched.sql: parsed = self._parse(enriched.sql) - return exp.Subquery(this=parsed, alias=exp.to_identifier(enriched.model_name)) + return exp.Subquery(this=parsed, alias=_model_identifier(enriched.model_name)) else: raise ValueError(f"Model '{enriched.model_name}' has neither sql_table nor sql defined") @@ -1947,8 +1976,8 @@ def _build_last_ranked_from( model = enriched.model_name default_time_col = enriched.last_agg_time_column - # Build SELECT * plus ROW_NUMBER - parts = [f"{model}.*"] + # Quoted so a reserved-keyword model survives the _parse(ranked_sql) round-trip. + parts = [f"{quote_ident_for(model, self.dialect)}.*"] # Add pre-computed time dimension expressions (DATE_TRUNC) for td in enriched.time_dimensions: @@ -2050,7 +2079,8 @@ def _build_last_ranked_from( # `FROM
AS ` and would miss this subquery wrapper. if enriched.resolved_joins: join_sql_parts = [ - f"{jtype.upper()} JOIN {target_table} AS {target_alias} ON {join_cond}" + f"{jtype.upper()} JOIN {target_table} AS " + f"{quote_ident_for(target_alias, self.dialect)} ON {join_cond}" for target_table, target_alias, join_cond, jtype in enriched.resolved_joins ] ranked_sql += " " + " ".join(join_sql_parts) @@ -2062,7 +2092,7 @@ def _build_last_ranked_from( parsed = self._parse(ranked_sql) return ( - exp.Subquery(this=parsed, alias=exp.to_identifier(model)), + exp.Subquery(this=parsed, alias=_model_identifier(model)), rn_suffix_map, filtered_rn_map, filtered_match_map, @@ -2114,11 +2144,11 @@ def _resolve_sql( ``type``. """ if sql is None: - return exp.Column(this=exp.to_identifier(name), table=exp.to_identifier(model_name)) + return exp.Column(this=exp.to_identifier(name), table=_model_identifier(model_name)) # Bare column name → qualify with model name # Use isidentifier() to distinguish column names from literals (e.g. "1") if sql.isidentifier(): - return exp.Column(this=exp.to_identifier(sql), table=exp.to_identifier(model_name)) + return exp.Column(this=exp.to_identifier(sql), table=_model_identifier(model_name)) return _wrap_cast_for_type(self._parse(sql), type) def _resolve_value_sql(self, measure: "EnrichedMeasure") -> str: @@ -2191,7 +2221,7 @@ def _build_agg( ), False return exp.Column( this=exp.to_identifier(measure.name), - table=exp.to_identifier(measure.model_name), + table=_model_identifier(measure.model_name), ), False # --- first/last: MAX(CASE WHEN _rn = 1 THEN col END) --- @@ -2269,7 +2299,7 @@ def _build_agg( else: inner = exp.Column( this=exp.to_identifier(measure.name), - table=exp.to_identifier(measure.model_name), + table=_model_identifier(measure.model_name), ) # --- Apply measure-level filter as CASE WHEN wrapper --- @@ -2563,7 +2593,7 @@ def _build_where_and_having( elif col_name.isidentifier(): qualified_sql = re.sub( rf'(? None: + """Group-by on a reserved-keyword model executes (was a syntax error).""" + query = SlayerQuery(source_model="grant", dimensions=[ColumnRef(name="status")]) + result = await reserved_kw_duckdb_env.execute(query=query) + statuses = {r["grant.status"] for r in result.data} + assert statuses == {"completed", "pending"} + + async def test_measure_with_dimension_and_filter( + self, reserved_kw_duckdb_env: SlayerQueryEngine + ) -> None: + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="status")], + filters=["status = 'completed'"], + ) + result = await reserved_kw_duckdb_env.execute(query=query) + assert len(result.data) == 1 + assert result.data[0]["grant.status"] == "completed" + assert float(result.data[0]["grant.amount_sum"]) == pytest.approx(450.0) + + async def test_group_by_mixed_case_column( + self, reserved_kw_duckdb_env: SlayerQueryEngine + ) -> None: + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "*:count"}], + dimensions=[ColumnRef(name="idempotencyKey")], + ) + result = await reserved_kw_duckdb_env.execute(query=query) + assert result.row_count == 4 + + async def test_join_from_reserved_keyword_model( + self, reserved_kw_duckdb_env: SlayerQueryEngine + ) -> None: + """Joined dimension off a reserved-keyword source: quoted JOIN alias + ON.""" + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="regions.name")], + ) + result = await reserved_kw_duckdb_env.execute(query=query) + by_region = {r["grant.regions.name"]: float(r["grant.amount_sum"]) for r in result.data} + assert by_region == {"US": pytest.approx(300.0), "EU": pytest.approx(200.0)} + + async def test_cross_model_measure_from_reserved_keyword_model( + self, reserved_kw_duckdb_env: SlayerQueryEngine + ) -> None: + """Cross-model measure (regions.population:sum): the cross-model CTE path.""" + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "regions.population:sum"}], + dimensions=[ColumnRef(name="status")], + ) + result = await reserved_kw_duckdb_env.execute(query=query) + assert result.row_count >= 1 diff --git a/tests/integration/test_integration_postgres.py b/tests/integration/test_integration_postgres.py index 50944c54..8043f6ae 100644 --- a/tests/integration/test_integration_postgres.py +++ b/tests/integration/test_integration_postgres.py @@ -11,7 +11,7 @@ from pytest_postgresql import factories from slayer.core.enums import DataType, TimeGranularity -from slayer.core.models import Column, DatasourceConfig, ModelMeasure, SlayerModel +from slayer.core.models import Column, DatasourceConfig, ModelJoin, ModelMeasure, SlayerModel from slayer.core.query import ColumnRef, OrderItem, SlayerQuery, TimeDimension from slayer.engine.ingestion import ingest_datasource from slayer.engine.query_engine import SlayerQueryEngine @@ -1182,3 +1182,111 @@ async def test_integration_postgres_cross_model_derived_columnsql( assert response.row_count == 2 assert float(response.data[0]["a_tbl.ratio_using_derived"]) == pytest.approx(2.0) assert float(response.data[1]["a_tbl.ratio_using_derived"]) == pytest.approx(2.0) + + +# Reserved-keyword model names (BUG-reserved-keyword-identifiers): a model named +# after a reserved SQL word (table ``Grant`` → model ``grant``) must be emitted +# quoted, else Postgres rejects the statement with a syntax error. +@pytest.fixture(scope="module") +def _pg_reserved_kw_storage(postgresql_proc, tmp_path_factory): + conn, db_name = _create_module_db(postgresql_proc) + try: + cur = conn.cursor() + cur.execute( + 'CREATE TABLE "grant" (' + " id INTEGER PRIMARY KEY," + ' "idempotencyKey" TEXT,' + " status TEXT NOT NULL," + " amount NUMERIC(10,2) NOT NULL," + " region_id INTEGER" + ")" + ) + cur.execute("CREATE TABLE regions (id INTEGER PRIMARY KEY, name TEXT NOT NULL)") + cur.executemany( + "INSERT INTO regions VALUES (%s, %s)", [(1, "US"), (2, "EU")] + ) + cur.executemany( + 'INSERT INTO "grant" VALUES (%s, %s, %s, %s, %s)', + [ + (1, "key-a", "completed", 100, 1), + (2, "key-b", "completed", 200, 1), + (3, "key-c", "pending", 50, 2), + (4, "key-d", "completed", 150, 2), + ], + ) + conn.commit() + + tmpdir = str(tmp_path_factory.mktemp("pg_reserved_kw")) + storage = YAMLStorage(base_dir=tmpdir) + info = postgresql_proc + run_sync(storage.save_datasource(DatasourceConfig( + name="reservedpg", type="postgres", host=info.host, port=info.port, + database=db_name, username=info.user, password="", + ))) + grant_model = SlayerModel( + name="grant", + sql_table='"grant"', + data_source="reservedpg", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="idempotencyKey", sql="idempotencyKey", type=DataType.TEXT), + Column(name="status", sql="status", type=DataType.TEXT), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column(name="region_id", sql="region_id", type=DataType.DOUBLE), + ], + joins=[ModelJoin(target_model="regions", join_pairs=[["region_id", "id"]])], + ) + regions_model = SlayerModel( + name="regions", + sql_table="regions", + data_source="reservedpg", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + ], + ) + run_sync(storage.save_model(grant_model)) + run_sync(storage.save_model(regions_model)) + yield storage + finally: + conn.close() + _drop_module_db(postgresql_proc, db_name) + + +@pytest.fixture +def pg_reserved_kw_env(_pg_reserved_kw_storage): + return SlayerQueryEngine(storage=_pg_reserved_kw_storage) + + +@pytest.mark.integration +class TestPostgresReservedKeywordModelName: + async def test_dimension_only_query(self, pg_reserved_kw_env: SlayerQueryEngine) -> None: + """Group-by on a reserved-keyword model executes (was a syntax error).""" + query = SlayerQuery(source_model="grant", dimensions=[ColumnRef(name="status")]) + result = await pg_reserved_kw_env.execute(query=query) + assert {r["grant.status"] for r in result.data} == {"completed", "pending"} + + async def test_measure_with_dimension_and_filter( + self, pg_reserved_kw_env: SlayerQueryEngine + ) -> None: + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="status")], + filters=["status = 'completed'"], + ) + result = await pg_reserved_kw_env.execute(query=query) + assert len(result.data) == 1 + assert float(result.data[0]["grant.amount_sum"]) == pytest.approx(450.0) + + async def test_join_from_reserved_keyword_model( + self, pg_reserved_kw_env: SlayerQueryEngine + ) -> None: + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="regions.name")], + ) + result = await pg_reserved_kw_env.execute(query=query) + by_region = {r["grant.regions.name"]: float(r["grant.amount_sum"]) for r in result.data} + assert by_region == {"US": pytest.approx(300.0), "EU": pytest.approx(200.0)} diff --git a/tests/test_aggregation_gating.py b/tests/test_aggregation_gating.py index 391d2aaa..495370b1 100644 --- a/tests/test_aggregation_gating.py +++ b/tests/test_aggregation_gating.py @@ -218,7 +218,7 @@ async def test_numeric_column_accepts_stat_agg( # the qualified value column. The earlier "( in sql" check passed # for any SELECT and didn't prove the aggregate survived enrichment # (Codex #6 / CodeRabbit nitpick on PR #82). - assert f"{fn}(orders.amount)" in sql + assert f'{fn}("orders".amount)' in sql @pytest.mark.parametrize( "agg,sql_fn", @@ -243,7 +243,7 @@ async def test_numeric_two_arg_stat_with_other_kwarg_accepted( ) # Both legs must be qualified and appear in the function call's # two-arg slot in canonical Postgres-style order. - assert f"{sql_fn}(orders.amount, orders.quantity)" in sql + assert f'{sql_fn}("orders".amount, "orders".quantity)' in sql @pytest.mark.parametrize( "agg", diff --git a/tests/test_cross_model_derived_columns.py b/tests/test_cross_model_derived_columns.py index 62124bf4..9f9709df 100644 --- a/tests/test_cross_model_derived_columns.py +++ b/tests/test_cross_model_derived_columns.py @@ -101,7 +101,7 @@ async def test_cross_model_dim_derived_column_via_query(tmp_path) -> None: dimensions=[ColumnRef(name="foo_normalized", model="B")], ) sql = await _gen_sql(engine, query, model_a) - assert "B.foo_raw / 100.0" in _norm(sql), f"Expected qualified B.foo_raw, got:\n{sql}" + assert '"B".foo_raw / 100.0' in _norm(sql), f"Expected qualified B.foo_raw, got:\n{sql}" # --------------------------------------------------------------------------- @@ -129,9 +129,9 @@ async def test_cross_model_columnsql_references_derived_column(tmp_path) -> None f"Generated SQL still references B.foo_normalized literally:\n{sql}" ) # The expansion must inline the derived expression - assert "B.foo_raw / 100.0" in norm, f"Expected inlined expansion, got:\n{sql}" + assert '"B".foo_raw / 100.0' in norm, f"Expected inlined expansion, got:\n{sql}" # The base reference A.bar passes through unchanged - assert "A.bar" in norm + assert '"A".bar' in norm async def test_cross_model_base_column_still_works(tmp_path) -> None: @@ -142,7 +142,7 @@ async def test_cross_model_base_column_still_works(tmp_path) -> None: ]) query = SlayerQuery(source_model="A", dimensions=[ColumnRef(name="ratio_using_base")]) sql = await _gen_sql(engine, query, model_a) - assert "A.bar / B.foo_raw" in _norm(sql), f"Base column ref broken:\n{sql}" + assert '"A".bar / "B".foo_raw' in _norm(sql), f"Base column ref broken:\n{sql}" # --------------------------------------------------------------------------- @@ -170,7 +170,7 @@ async def test_local_columnsql_references_local_derived(tmp_path) -> None: f"Local derived column A.c1 leaked into SQL:\n{sql}" ) # Inlined: (A.raw_a + 1) * 2 - assert "A.raw_a + 1" in norm + assert '"A".raw_a + 1' in norm assert "* 2" in norm @@ -199,7 +199,7 @@ async def test_chain_of_three_derived_columns(tmp_path) -> None: assert _no_bare_derived_ref(norm, "A", "c1") assert _no_bare_derived_ref(norm, "A", "c2") assert _no_bare_derived_ref(norm, "A", "c3") - assert "A.raw_a + 1" in norm + assert '"A".raw_a + 1' in norm assert "+ 10" in norm assert "+ 100" in norm @@ -249,11 +249,11 @@ async def test_joined_model_derived_referencing_further_joined(tmp_path) -> None sql = await _gen_sql(engine, query, model_a) norm = _norm(sql) # Must qualify under the canonical multi-hop alias B__C, not bare C. - assert "B__C.name" in norm, ( + assert '"B__C".name' in norm, ( f"Expected canonical B__C alias, got:\n{sql}" ) # And the C join must actually be present in the FROM. - assert "JOIN C AS B__C" in norm or "JOIN \"C\" AS \"B__C\"" in norm or "JOIN C B__C" in norm, ( + assert "JOIN C AS \"B__C\"" in norm or "JOIN \"C\" AS \"B__C\"" in norm or "JOIN C B__C" in norm, ( f"C join missing from FROM clause:\n{sql}" ) @@ -313,7 +313,7 @@ async def test_multihop_derived_via_join_path(tmp_path) -> None: f"Multi-hop derived ref leaked into SQL:\n{sql}" ) # Inlined: (B__C.raw_c * 2) - assert "B__C.raw_c * 2" in norm + assert '"B__C".raw_c * 2' in norm # --------------------------------------------------------------------------- @@ -382,8 +382,8 @@ async def test_diamond_join_derived(tmp_path) -> None: norm = _norm(sql) assert _no_bare_derived_ref(norm, "customers__regions", "name_upper") assert _no_bare_derived_ref(norm, "warehouses__regions", "name_upper") - assert "UPPER(customers__regions.name_raw)" in norm - assert "UPPER(warehouses__regions.name_raw)" in norm + assert 'UPPER("customers__regions".name_raw)' in norm + assert 'UPPER("warehouses__regions".name_raw)' in norm # --------------------------------------------------------------------------- @@ -434,7 +434,7 @@ async def test_self_reference_terminates(tmp_path) -> None: ) query = SlayerQuery(source_model="A", dimensions=[ColumnRef(name="bar")]) sql = await _gen_sql(engine, query, model_a) - assert "A.bar" in _norm(sql) + assert '"A".bar' in _norm(sql) # --------------------------------------------------------------------------- @@ -454,9 +454,9 @@ async def test_mixed_base_and_derived_refs_in_one_columnsql(tmp_path) -> None: query = SlayerQuery(source_model="A", dimensions=[ColumnRef(name="mixed")]) sql = await _gen_sql(engine, query, model_a) norm = _norm(sql) - assert "B.foo_raw / 100.0" in norm # derived expanded + assert '"B".foo_raw / 100.0' in norm # derived expanded assert _no_bare_derived_ref(norm, "B", "foo_normalized") - assert "A.bar / B.foo_raw" in norm # base still there as base + assert '"A".bar / "B".foo_raw' in norm # base still there as base # --------------------------------------------------------------------------- @@ -478,7 +478,7 @@ async def test_measure_aggregation_over_cross_model_derived(tmp_path) -> None: # in CAST when its type is set, e.g. ``SUM(CAST(B.foo_raw / 100.0 AS …))``. # Either form is acceptable — the assertion only pins the inlining # behavior, not exact CAST-vs-no-CAST shape. - assert "B.foo_raw / 100.0" in norm or "B.foo_raw/100.0" in norm + assert '"B".foo_raw / 100.0' in norm or '"B".foo_raw/100.0' in norm assert "SUM(" in norm @@ -505,7 +505,7 @@ async def test_measure_aggregation_via_local_columnsql_referencing_derived(tmp_p norm = _norm(sql) assert _no_bare_derived_ref(norm, "B", "foo_normalized") assert _no_bare_derived_ref(norm, "A", "ratio_using_derived") - assert "B.foo_raw / 100.0" in norm + assert '"B".foo_raw / 100.0' in norm assert "SUM(" in norm @@ -527,7 +527,7 @@ async def test_filter_referencing_derived_column(tmp_path) -> None: assert _no_bare_derived_ref(norm, "B", "foo_normalized"), ( f"Filter still references B.foo_normalized literally:\n{sql}" ) - assert "B.foo_raw / 100.0" in norm + assert '"B".foo_raw / 100.0' in norm # --------------------------------------------------------------------------- @@ -587,7 +587,7 @@ async def test_disambiguation_when_both_models_have_same_column_name(tmp_path) - sql = await _gen_sql(engine, query, model_a) norm = _norm(sql) # Must qualify to B explicitly so it's not ambiguous with A.foo_raw. - assert "B.foo_raw / 100.0" in norm, ( + assert '"B".foo_raw / 100.0' in norm, ( f"Expansion did not qualify foo_raw under B:\n{sql}" ) @@ -656,7 +656,7 @@ async def test_multihop_query_dim_to_derived_target_column(tmp_path) -> None: ) assert _no_bare_derived_ref(norm, "C", "x_derived") # Inner base ref must be qualified to the canonical multi-hop alias. - assert "B__C.raw_c * 2" in norm, ( + assert '"B__C".raw_c * 2' in norm, ( f"Expected B__C.raw_c * 2, got:\n{sql}" ) # And the C join must be present under the canonical alias. @@ -712,7 +712,7 @@ async def test_multihop_filter_on_derived_target_column(tmp_path) -> None: assert _no_bare_derived_ref(norm, "B__C", "x_derived"), ( f"Filter still references B__C.x_derived literally:\n{sql}" ) - assert "B__C.raw_c * 2" in norm, ( + assert '"B__C".raw_c * 2' in norm, ( f"Filter expansion did not qualify under B__C:\n{sql}" ) # The > 0 comparison must survive past expansion. @@ -742,7 +742,7 @@ async def test_multihop_time_dim_to_derived_target_column(tmp_path) -> None: sql = await _gen_sql(engine, query, model_a) norm = _norm(sql) # The derived sql ``raw_ts + 0`` should be qualified to B__C. - assert "B__C.raw_ts" in norm, ( + assert '"B__C".raw_ts' in norm, ( f"Multi-hop time-dim derived col not qualified to B__C:\n{sql}" ) @@ -790,7 +790,7 @@ async def test_dev_1339_solar_panels_repro(tmp_path) -> None: arrays, ) norm = _norm(dim_sql) - assert "solar_panels.energy_out / solar_panels.energy_in" in norm, ( + assert '"solar_panels".energy_out / "solar_panels".energy_in' in norm, ( f"Bare inner refs in derived col leaked into SQL — DEV-1339 regressed:\n{dim_sql}" ) # Measure-side: the issue specifically called out measure formulas. @@ -801,7 +801,7 @@ async def test_dev_1339_solar_panels_repro(tmp_path) -> None: arrays, ) norm = _norm(measure_sql) - assert "solar_panels.energy_out / solar_panels.energy_in" in norm, ( + assert '"solar_panels".energy_out / "solar_panels".energy_in' in norm, ( f"Cross-model measure agg over derived col left bare refs unqualified:\n{measure_sql}" ) @@ -1306,7 +1306,7 @@ async def test_dev1410_local_bare_ref_to_local_derived(tmp_path) -> None: f"Bare-ref c1 leaked into SQL as A.c1:\n{sql}" ) # Inlined: (A.raw_a + 1) * 2 — the inner raw_a still gets qualified. - assert "A.raw_a + 1" in norm, f"raw_a not qualified:\n{sql}" + assert '"A".raw_a + 1' in norm, f"raw_a not qualified:\n{sql}" assert "* 2" in norm, f"outer arithmetic missing:\n{sql}" @@ -1331,7 +1331,7 @@ async def test_dev1410_local_bare_ref_three_deep(tmp_path) -> None: assert _no_bare_derived_ref(norm, "A", "c1") assert _no_bare_derived_ref(norm, "A", "c2") assert _no_bare_derived_ref(norm, "A", "c3") - assert "A.raw_a + 1" in norm + assert '"A".raw_a + 1' in norm assert "+ 10" in norm assert "+ 100" in norm @@ -1358,8 +1358,8 @@ async def test_dev1410_local_bare_ref_mixed_with_base(tmp_path) -> None: f"Bare derived_b leaked into SQL:\n{sql}" ) # Base raw_a qualifies; derived_b inlines. - assert "A.raw_a +" in norm - assert "A.raw_a * 2" in norm # the inlined body + assert '"A".raw_a +' in norm + assert '"A".raw_a * 2' in norm # the inlined body async def test_dev1410_local_bare_ref_inside_case_coalesce_nullif_cast(tmp_path) -> None: @@ -1403,7 +1403,7 @@ async def test_dev1410_local_bare_ref_inside_case_coalesce_nullif_cast(tmp_path) assert _no_bare_derived_ref(norm, "A", "score"), ( f"score leaked as A.score in {col}:\n{sql}" ) - assert "A.x * 10" in norm, f"score body not inlined in {col}:\n{sql}" + assert '"A".x * 10' in norm, f"score body not inlined in {col}:\n{sql}" async def test_dev1410_local_bare_ref_to_derived_inside_subquery_left_alone( @@ -1439,7 +1439,7 @@ async def test_dev1410_local_bare_ref_to_derived_inside_subquery_left_alone( assert "MAX(score)" in norm, ( f"Subquery-scope bare ``score`` was rewritten or removed:\n{sql}" ) - assert "A.raw_a * 10" in norm, ( + assert '"A".raw_a * 10' in norm, ( f"Root-scope ``score`` was not inlined:\n{sql}" ) @@ -1507,7 +1507,7 @@ async def test_dev1410_local_bare_ref_to_derived_inside_window_over_partition_in assert _no_bare_derived_ref(norm, "A", "bucket"), ( f"OVER-clause bare derived ref not inlined:\n{sql}" ) - assert "A.raw_a / 10" in norm, f"bucket body not inlined:\n{sql}" + assert '"A".raw_a / 10' in norm, f"bucket body not inlined:\n{sql}" async def test_dev1410_local_bare_ref_to_derived_inside_union_left_alone( @@ -1539,7 +1539,7 @@ async def test_dev1410_local_bare_ref_to_derived_inside_union_left_alone( f"UNION-branch bare ref was rewritten:\n{sql}" ) # Root-scope ``score`` (rightmost) is inlined. - assert "A.raw_a + 1" in norm, f"Root-scope score body not inlined:\n{sql}" + assert '"A".raw_a + 1' in norm, f"Root-scope score body not inlined:\n{sql}" async def test_dev1410_local_bare_ref_to_derived_inside_values_left_alone( @@ -1572,7 +1572,7 @@ async def test_dev1410_local_bare_ref_to_derived_inside_values_left_alone( sql = await _gen_sql(engine, query, model_a) norm = _norm(sql) # Root-scope ``score`` is inlined. - assert "A.raw_a + 1" in norm, ( + assert '"A".raw_a + 1' in norm, ( f"Root-scope score body not inlined:\n{sql}" ) # Subquery's ``score`` left bare (refers to the VALUES alias). @@ -1611,7 +1611,7 @@ async def test_dev1410_local_bare_ref_to_string_literal_lookalike_left_alone( ) # The CAST(score AS TEXT) actually inlines: CAST((A.raw_a + 1) AS TEXT) # — verify the body shows up at least once. - assert "A.raw_a + 1" in norm, f"score body not inlined:\n{sql}" + assert '"A".raw_a + 1' in norm, f"score body not inlined:\n{sql}" async def test_dev1410_local_bare_ref_with_double_underscore_name(tmp_path) -> None: @@ -1655,7 +1655,7 @@ async def test_dev1410_local_bare_ref_with_double_underscore_name(tmp_path) -> N f"Double-underscore-named derived col leaked into SQL:\n{sql}" ) # Inlined body with inner ref qualified to host alias. - assert "infrastructure.dwelling_specs * 1.0" in norm, ( + assert '"infrastructure".dwelling_specs * 1.0' in norm, ( f"Body not inlined with qualified inner ref:\n{sql}" ) @@ -1747,8 +1747,8 @@ async def test_dev1410_exact_repro(tmp_path) -> None: f"Derived column ``{derived}`` leaked as physical column ref:\n{sql}" ) # CASE bodies inlined with inner refs qualified to host alias. - assert "infrastructure.wateraccess" in norm - assert "infrastructure.roadsurface" in norm - assert "infrastructure.parkavail" in norm + assert '"infrastructure".wateraccess' in norm + assert '"infrastructure".roadsurface' in norm + assert '"infrastructure".parkavail' in norm # Parse the result as SQLite to confirm it's syntactically valid. sqlglot.parse_one(sql, dialect="sqlite") diff --git a/tests/test_reserved_keyword_quoting.py b/tests/test_reserved_keyword_quoting.py new file mode 100644 index 00000000..887f61d0 --- /dev/null +++ b/tests/test_reserved_keyword_quoting.py @@ -0,0 +1,183 @@ +"""Regression tests for BUG-reserved-keyword-identifiers. + +A model named after a reserved SQL keyword (``grant``/``order``/``user``/ +``select``) must be emitted QUOTED, else (A) the DB rejects the bare keyword +and (B) a re-parsed ``grant."col"`` string falls back to a ``Command`` parse +(logged on the ``sqlglot`` logger). These tests assert the quoted form, a +clean parse, and no ``Command`` fallback warning. +""" + +import logging + +import pytest +import sqlglot + +from slayer.core.enums import DataType, TimeGranularity +from slayer.core.models import Column, ModelJoin, SlayerModel +from slayer.core.query import ColumnRef, SlayerQuery, TimeDimension +from slayer.engine.enrichment import enrich_query +from slayer.sql.generator import SQLGenerator + +# A representative spread: SQLite (STRFTIME branch), Postgres (the reported +# dialect), DuckDB, Snowflake (case-folds unquoted → UPPER), MySQL, ClickHouse. +DIALECTS = ["postgres", "sqlite", "duckdb", "snowflake", "mysql", "clickhouse"] +RESERVED_NAMES = ["grant", "order", "user", "select"] + +_COMMAND_FALLBACK = "Falling back to parsing as a 'Command'" + + +async def _noop(**kw): + return None + + +def _q(name: str, dialect: str) -> str: + """Quoted identifier form for ``name`` in ``dialect`` (tracks its quote char).""" + return sqlglot.exp.to_identifier(name, quoted=True).sql(dialect=dialect) + + +def _grant_model(name: str = "grant") -> SlayerModel: + """Reserved-keyword model with a mixed-case column (mirrors the reported schema).""" + return SlayerModel( + name=name, + sql_table=f'"{name}"', + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="idempotencyKey", sql="idempotencyKey", type=DataType.TEXT), + Column(name="status", sql="status", type=DataType.TEXT), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP), + Column(name="region_id", sql="region_id", type=DataType.DOUBLE), + ], + ) + + +async def _enrich(query: SlayerQuery, model: SlayerModel, dialect: str, **resolvers): + """Enrich with a real ``resolve_model`` (exercises column-expansion, surface + B) and the generator's ``dialect`` (mirrors the engine's one-dialect flow).""" + async def resolve_model(model_name=None, named_queries=None, **kw): + return resolvers.get("models", {}).get(model_name) + + return await enrich_query( + query=query, + model=model, + resolve_dimension_via_joins=_noop, + resolve_cross_model_measure=_noop, + resolve_join_target=resolvers.get("resolve_join_target", _noop), + resolve_model=resolvers.get("resolve_model", resolve_model), + dialect=dialect, + ) + + +@pytest.mark.parametrize("dialect", DIALECTS) +@pytest.mark.parametrize("name", RESERVED_NAMES) +async def test_reserved_keyword_model_name_is_quoted(dialect, name, caplog): + model = _grant_model(name) + query = SlayerQuery( + source_model=name, + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="status")], + ) + enriched = await _enrich(query, model, dialect, models={name: model}) + with caplog.at_level(logging.WARNING, logger="sqlglot"): + sql = SQLGenerator(dialect=dialect).generate(enriched=enriched) + + # Surface A: quoted identifier present + a clean parse (no bare keyword). + assert _q(name, dialect) in sql + assert sqlglot.parse(sql, dialect=dialect) + # Surface B: no Command-fallback warning. + assert not any(_COMMAND_FALLBACK in r.getMessage() for r in caplog.records), [ + r.getMessage() for r in caplog.records + ] + + +@pytest.mark.parametrize("dialect", DIALECTS) +async def test_surface_b_mixed_case_qualifier_no_command_fallback(dialect, caplog): + """Reported reproducer: ``grant`` model + mixed-case ``idempotencyKey`` dim — + quoted qualifier, no ``grant .`` (space-dot) Command artifact.""" + model = _grant_model() + query = SlayerQuery(source_model="grant", dimensions=[ColumnRef(name="idempotencyKey")]) + enriched = await _enrich(query, model, dialect, models={"grant": model}) + with caplog.at_level(logging.WARNING, logger="sqlglot"): + sql = SQLGenerator(dialect=dialect).generate(enriched=enriched) + + assert _q("grant", dialect) in sql + assert "grant ." not in sql # Command-misparse spacing artifact + assert sqlglot.parse(sql, dialect=dialect) + assert not any(_COMMAND_FALLBACK in r.getMessage() for r in caplog.records) + + +@pytest.mark.parametrize("dialect", DIALECTS) +async def test_filter_on_reserved_keyword_model(dialect, caplog): + model = _grant_model() + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="status")], + filters=["status = 'completed'"], + ) + enriched = await _enrich(query, model, dialect, models={"grant": model}) + with caplog.at_level(logging.WARNING, logger="sqlglot"): + sql = SQLGenerator(dialect=dialect).generate(enriched=enriched) + + q = _q("grant", dialect) + assert f"{q}.status" in sql or f"{q}.{_q('status', dialect)}" in sql + assert sqlglot.parse(sql, dialect=dialect) + assert not any(_COMMAND_FALLBACK in r.getMessage() for r in caplog.records) + + +@pytest.mark.parametrize("dialect", DIALECTS) +async def test_join_from_reserved_keyword_model(dialect, caplog): + """Joined dimension off a reserved-keyword source: quoted JOIN alias + ON.""" + regions = SlayerModel( + name="regions", + sql_table="regions", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + Column(name="population", sql="population", type=DataType.DOUBLE), + ], + ) + grant = _grant_model() + grant.joins = [ModelJoin(target_model="regions", join_pairs=[["region_id", "id"]])] + + async def resolve_join_target(target_model_name=None, named_queries=None, **kw): + return (regions.sql_table, regions) if target_model_name == "regions" else None + + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:sum"}], + dimensions=[ColumnRef(name="regions.name")], + ) + enriched = await _enrich( + query, grant, dialect, models={"grant": grant, "regions": regions}, + resolve_join_target=resolve_join_target, + ) + with caplog.at_level(logging.WARNING, logger="sqlglot"): + sql = SQLGenerator(dialect=dialect).generate(enriched=enriched) + + assert _q("grant", dialect) in sql + assert sqlglot.parse(sql, dialect=dialect) + assert not any(_COMMAND_FALLBACK in r.getMessage() for r in caplog.records) + + +@pytest.mark.parametrize("dialect", DIALECTS) +async def test_first_last_ranked_subquery_on_reserved_keyword_model(dialect, caplog): + """Ranked subquery emits ``SELECT "grant".*`` — guards star-projection quoting.""" + model = _grant_model() + query = SlayerQuery( + source_model="grant", + measures=[{"formula": "amount:last"}], + dimensions=[ColumnRef(name="status")], + time_dimensions=[ + TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.DAY) + ], + ) + enriched = await _enrich(query, model, dialect, models={"grant": model}) + with caplog.at_level(logging.WARNING, logger="sqlglot"): + sql = SQLGenerator(dialect=dialect).generate(enriched=enriched) + + assert f'{_q("grant", dialect)}.*' in sql + assert sqlglot.parse(sql, dialect=dialect) + assert not any(_COMMAND_FALLBACK in r.getMessage() for r in caplog.records) diff --git a/tests/test_sql_generator.py b/tests/test_sql_generator.py index 4b6317df..553ac521 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -79,6 +79,10 @@ async def _generate( resolve_dimension_via_joins=_noop_async, resolve_cross_model_measure=_noop_async, resolve_join_target=_noop_async, + # Mirror the engine: enrichment and generation share one dialect, so + # the dialect-aware quoting of re-parsed strings (join conditions, + # WHERE qualifiers) matches the generator's quote character. + dialect=generator.dialect, ) sql = generator.generate(enriched=enriched) _assert_valid_sql(sql, dialect=generator.dialect) @@ -1809,6 +1813,17 @@ def test_dialect_for_type(self, ds_type: str, expected: str) -> None: assert SlayerQueryEngine._dialect_for_type(ds_type) == expected +# Per-dialect identifier quote characters (open, close). Table qualifiers +# in generated SQL are now emitted quoted with the dialect's quote chars. +_IDENT_QUOTES = { + "mysql": ("`", "`"), + "bigquery": ("`", "`"), + "databricks": ("`", "`"), + "spark": ("`", "`"), + "tsql": ("[", "]"), +} + + class TestMultiDialectGeneration: """Test SQL generation across all supported dialects.""" @@ -2002,8 +2017,11 @@ async def test_one_arg_stat_agg_generation( assert "SELECT" in upper # The aggregate must wrap the resolved value column (orders.amount, # since the `revenue` Column has sql="amount") in its single-arg slot. - assert "(ORDERS.AMOUNT)" in upper, ( - f"expected single-arg call (ORDERS.AMOUNT) in SQL for {formula!r} on {dialect}:\n{sql}" + # The table qualifier is now emitted quoted with the dialect's + # identifier quote character. + oq, cq = _IDENT_QUOTES.get(dialect, ('"', '"')) + assert f"({oq}ORDERS{cq}.AMOUNT)" in upper, ( + f"expected single-arg call ({oq}ORDERS{cq}.AMOUNT) in SQL for {formula!r} on {dialect}:\n{sql}" ) # corr / covar_samp / covar_pop are not supported on MySQL — the generator @@ -2038,8 +2056,9 @@ async def test_two_arg_stat_agg_generation( # asymmetry vs the 1-arg test is what distinguishes the test bodies # for Sonar python:S4144 and pins the new `_resolve_agg_param` + # `_resolve_value_sql` qualification path for 2-arg stats. - assert "(ORDERS.AMOUNT, ORDERS.QUANTITY)" in upper, ( - f"expected two-arg call (ORDERS.AMOUNT, ORDERS.QUANTITY) in SQL for {formula!r} " + oq, cq = _IDENT_QUOTES.get(dialect, ('"', '"')) + assert f"({oq}ORDERS{cq}.AMOUNT, {oq}ORDERS{cq}.QUANTITY)" in upper, ( + f"expected two-arg call ({oq}ORDERS{cq}.AMOUNT, {oq}ORDERS{cq}.QUANTITY) in SQL for {formula!r} " f"on {dialect}:\n{sql}" ) @@ -2228,27 +2247,27 @@ def test_build_percentile_postgres(self) -> None: gen = SQLGenerator(dialect="postgres") m = self._measure(agg="percentile", agg_kwargs={"p": "0.95"}) sql = gen._build_percentile(m).sql(dialect="postgres") - assert sql == "PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY orders.amount)" + assert sql == 'PERCENTILE_CONT(0.95) WITHIN GROUP (ORDER BY "orders".amount)' def test_build_percentile_sqlite(self) -> None: gen = SQLGenerator(dialect="sqlite") m = self._measure(agg="percentile", agg_kwargs={"p": "0.5"}) sql = gen._build_percentile(m).sql(dialect="sqlite") - assert sql == "PERCENTILE_CONT(orders.amount, 0.5)" + assert sql == 'PERCENTILE_CONT("orders".amount, 0.5)' def test_build_percentile_clickhouse_emits_quantile(self) -> None: gen = SQLGenerator(dialect="clickhouse") m = self._measure(agg="percentile", agg_kwargs={"p": "0.75"}) sql = gen._build_percentile(m).sql(dialect="clickhouse") # ClickHouse parametric aggregate syntax. - assert sql == "quantile(0.75)(orders.amount)" + assert sql == 'quantile(0.75)("orders".amount)' @pytest.mark.parametrize("p", ["0.05", "0.25", "0.5", "0.95"]) def test_build_percentile_clickhouse_param_substitution(self, p: str) -> None: gen = SQLGenerator(dialect="clickhouse") m = self._measure(agg="percentile", agg_kwargs={"p": p}) sql = gen._build_percentile(m).sql(dialect="clickhouse") - assert sql == f"quantile({p})(orders.amount)" + assert sql == f'quantile({p})("orders".amount)' def test_build_percentile_duckdb(self) -> None: gen = SQLGenerator(dialect="duckdb") @@ -2257,7 +2276,7 @@ def test_build_percentile_duckdb(self) -> None: # sqlglot rewrites the WITHIN GROUP form to DuckDB's QUANTILE_CONT. assert "QUANTILE_CONT" in sql # Qualified column. - assert "orders.amount" in sql + assert '"orders".amount' in sql def test_build_percentile_mysql_raises(self) -> None: gen = SQLGenerator(dialect="mysql") @@ -2294,7 +2313,7 @@ def test_build_percentile_uses_model_level_default_p(self) -> None: aggregation_def=agg_def, ) sql = gen._build_percentile(m).sql(dialect="postgres") - assert sql == "PERCENTILE_CONT(0.9) WITHIN GROUP (ORDER BY orders.amount)" + assert sql == 'PERCENTILE_CONT(0.9) WITHIN GROUP (ORDER BY "orders".amount)' def test_build_percentile_query_kwarg_overrides_model_default(self) -> None: """Query-time agg_kwargs win over the model-level default.""" @@ -2313,7 +2332,7 @@ def test_build_percentile_query_kwarg_overrides_model_default(self) -> None: aggregation_def=agg_def, ) sql = gen._build_percentile(m).sql(dialect="postgres") - assert sql == "PERCENTILE_CONT(0.25) WITHIN GROUP (ORDER BY orders.amount)" + assert sql == 'PERCENTILE_CONT(0.25) WITHIN GROUP (ORDER BY "orders".amount)' # --- A2: percentile p must be a numeric literal in [0, 1] ----------- @@ -2412,10 +2431,10 @@ def _measure( @pytest.mark.parametrize( "dialect,expected", [ - ("postgres", "STDDEV_SAMP(orders.amount)"), - ("duckdb", "STDDEV_SAMP(orders.amount)"), - ("mysql", "STDDEV_SAMP(orders.amount)"), - ("sqlite", "STDDEV_SAMP(orders.amount)"), + ("postgres", 'STDDEV_SAMP("orders".amount)'), + ("duckdb", 'STDDEV_SAMP("orders".amount)'), + ("mysql", "STDDEV_SAMP(`orders`.amount)"), + ("sqlite", 'STDDEV_SAMP("orders".amount)'), ], ) def test_build_stddev_samp(self, dialect: str, expected: str) -> None: @@ -2429,10 +2448,10 @@ def test_build_stddev_samp(self, dialect: str, expected: str) -> None: @pytest.mark.parametrize( "dialect,expected", [ - ("postgres", "STDDEV_POP(orders.amount)"), - ("duckdb", "STDDEV_POP(orders.amount)"), - ("mysql", "STDDEV_POP(orders.amount)"), - ("sqlite", "STDDEV_POP(orders.amount)"), + ("postgres", 'STDDEV_POP("orders".amount)'), + ("duckdb", 'STDDEV_POP("orders".amount)'), + ("mysql", "STDDEV_POP(`orders`.amount)"), + ("sqlite", 'STDDEV_POP("orders".amount)'), ], ) def test_build_stddev_pop(self, dialect: str, expected: str) -> None: @@ -2446,7 +2465,7 @@ def test_build_stddev_pop(self, dialect: str, expected: str) -> None: @pytest.mark.parametrize( "dialect,expected", [ - ("postgres", "VAR_SAMP(orders.amount)"), + ("postgres", 'VAR_SAMP("orders".amount)'), # sqlglot rewrites VAR_SAMP → VARIANCE on SQLite/DuckDB; the # SQLite UDF is therefore registered under the alias `variance` # so generator output still resolves at runtime. MySQL is the @@ -2455,9 +2474,9 @@ def test_build_stddev_pop(self, dialect: str, expected: str) -> None: # variance), so the rewritten SQL would silently return the # wrong value. The generator emits ``VAR_SAMP`` directly on # MySQL via ``exp.Anonymous`` to bypass the transpile. - ("duckdb", "VARIANCE(orders.amount)"), - ("mysql", "VAR_SAMP(orders.amount)"), - ("sqlite", "VARIANCE(orders.amount)"), + ("duckdb", 'VARIANCE("orders".amount)'), + ("mysql", "VAR_SAMP(`orders`.amount)"), + ("sqlite", 'VARIANCE("orders".amount)'), ], ) def test_build_var_samp(self, dialect: str, expected: str) -> None: @@ -2471,14 +2490,14 @@ def test_build_var_samp(self, dialect: str, expected: str) -> None: @pytest.mark.parametrize( "dialect,expected", [ - ("postgres", "VAR_POP(orders.amount)"), - ("duckdb", "VAR_POP(orders.amount)"), + ("postgres", 'VAR_POP("orders".amount)'), + ("duckdb", 'VAR_POP("orders".amount)'), # sqlglot rewrites VAR_POP → VARIANCE_POP on SQLite (handled by # a registered UDF alias). MySQL gets the same buggy rewrite, # but ``VARIANCE_POP`` is not a real MySQL function — the # generator emits ``VAR_POP`` directly via ``exp.Anonymous``. - ("mysql", "VAR_POP(orders.amount)"), - ("sqlite", "VARIANCE_POP(orders.amount)"), + ("mysql", "VAR_POP(`orders`.amount)"), + ("sqlite", 'VARIANCE_POP("orders".amount)'), ], ) def test_build_var_pop(self, dialect: str, expected: str) -> None: @@ -2508,7 +2527,7 @@ def test_build_two_arg_stat_emits_two_arg_call( sql = gen._build_agg(measure=m)[0].sql(dialect=dialect) # Both legs go through _resolve_sql, so a bare `quantity` kwarg # qualifies under the LHS measure's model_name. - assert sql == f"{sql_fn}(orders.amount, orders.quantity)" + assert sql == f'{sql_fn}("orders".amount, "orders".quantity)' @pytest.mark.parametrize("agg", ["corr", "covar_samp", "covar_pop"]) def test_build_two_arg_stat_clickhouse(self, agg: str) -> None: @@ -2516,7 +2535,7 @@ def test_build_two_arg_stat_clickhouse(self, agg: str) -> None: m = self._measure(agg=agg, agg_kwargs={"other": "quantity"}) sql = gen._build_agg(measure=m)[0].sql(dialect="clickhouse") # ClickHouse casing is its own thing; assert the call shape only. - assert sql.lower() == f"{agg.lower()}(orders.amount, orders.quantity)" + assert sql.lower() == f'{agg.lower()}("orders".amount, "orders".quantity)' @pytest.mark.parametrize("agg", ["corr", "covar_samp", "covar_pop"]) def test_build_two_arg_stat_mysql_raises(self, agg: str) -> None: @@ -2573,7 +2592,7 @@ def test_build_stddev_samp_with_filter_wraps_value(self) -> None: ) sql = gen._build_agg(measure=m)[0].sql(dialect="postgres") # Filter wraps the qualified column reference. - assert "CASE WHEN status = 'completed' THEN orders.amount END" in sql + assert "CASE WHEN status = 'completed' THEN \"orders\".amount END" in sql assert "STDDEV_SAMP" in sql def test_build_corr_with_filter_wraps_both_columns(self) -> None: @@ -2593,8 +2612,8 @@ def test_build_corr_with_filter_wraps_both_columns(self) -> None: assert sql.count("CASE WHEN status = 'completed'") == 2 assert "CORR(" in sql # Both legs are also qualified. - assert "orders.amount" in sql - assert "orders.quantity" in sql + assert '"orders".amount' in sql + assert '"orders".quantity' in sql class TestStatAggsViaQueryEnrichment: @@ -2641,7 +2660,7 @@ async def test_stat_agg_via_colon_syntax( # by the qualified column ref in its single-arg slot. `sales.latency` # being inside the call is what proves enrichment+generation reached # the value column, not just that the alias contains the fragment. - assert f"{expected_fragment}(sales.latency)" in sql + assert f'{expected_fragment}("sales".latency)' in sql async def test_corr_via_colon_syntax_with_other_kwarg( self, sales_model: SlayerModel, @@ -2654,7 +2673,7 @@ async def test_corr_via_colon_syntax_with_other_kwarg( sql = await _generate(generator=gen, query=query, model=sales_model) # Both legs flow through _resolve_sql and qualify under the LHS # measure's model_name. - assert "CORR(sales.price, sales.quantity)" in sql + assert 'CORR("sales".price, "sales".quantity)' in sql class TestPathAliasJoinInference: @@ -3373,7 +3392,7 @@ async def test_filtered_measure_uses_source_alias_not_model_name( m for m in enriched.measures if m.source_measure_name == "active_revenue" ) assert measure.filter_sql is not None - assert "orders_alias.status" in measure.filter_sql + assert '"orders_alias".status' in measure.filter_sql assert "orders_underlying" not in measure.filter_sql async def test_filtered_measure_source_alias_propagates_to_generated_sql( @@ -4054,7 +4073,7 @@ async def resolve_join_target(*, target_model_name, named_queries): ) sql = generator.generate(enriched=enriched) # Normal dimension should be qualified with the table alias - assert "customers.status" in sql + assert '"customers".status' in sql class TestDimensionAggregation: @@ -6017,11 +6036,11 @@ async def capture_sql(sql): # Expression measure must NOT be corrupted: "orders.COALESCE(amount, 0)" is invalid assert "orders.COALESCE" not in sql, f"Expression measure corrupted:\n{sql}" # Bare measure should be qualified - assert "orders.amount" in sql + assert '"orders".amount' in sql # Bare ``amount`` inside the expression now qualifies to the model's # alias (``orders.amount``) — derived-ref expansion (DEV-1333) makes # every base-column reference unambiguous. - assert "COALESCE(orders.amount, 0)" in sql + assert 'COALESCE("orders".amount, 0)' in sql async def test_cross_model_measures_probed_via_engine(self) -> None: """Cross-model measures should be probed via the engine's enrich+generate pipeline.""" @@ -6738,11 +6757,11 @@ class TestStringHygieneDialectTranslation: @pytest.mark.parametrize( "dialect,expected", [ - ("sqlite", "LOWER(orders.status) = 'active'"), - ("postgres", "LOWER(orders.status) = 'active'"), - ("mysql", "LOWER(orders.status) = 'active'"), - ("duckdb", "LOWER(orders.status) = 'active'"), - ("clickhouse", "lower(orders.status) = 'active'"), + ("sqlite", 'LOWER("orders".status) = \'active\''), + ("postgres", 'LOWER("orders".status) = \'active\''), + ("mysql", "LOWER(`orders`.status) = 'active'"), + ("duckdb", 'LOWER("orders".status) = \'active\''), + ("clickhouse", 'lower("orders".status) = \'active\''), ], ) async def test_lower(self, orders_model: SlayerModel, dialect: str, expected: str) -> None: @@ -6761,11 +6780,11 @@ async def test_lower(self, orders_model: SlayerModel, dialect: str, expected: st @pytest.mark.parametrize( "dialect,expected", [ - ("sqlite", "INSTR(orders.status, ',')"), - ("postgres", "POSITION(',' IN orders.status)"), - ("mysql", "LOCATE(',', orders.status)"), - ("duckdb", "STRPOS(orders.status, ',')"), - ("clickhouse", "POSITION(orders.status, ',')"), + ("sqlite", 'INSTR("orders".status, \',\')'), + ("postgres", 'POSITION(\',\' IN "orders".status)'), + ("mysql", "LOCATE(',', `orders`.status)"), + ("duckdb", 'STRPOS("orders".status, \',\')'), + ("clickhouse", 'POSITION("orders".status, \',\')'), ], ) async def test_instr_translates_per_dialect( @@ -6786,11 +6805,11 @@ async def test_instr_translates_per_dialect( @pytest.mark.parametrize( "dialect,expected", [ - ("sqlite", "SUBSTRING(orders.status, 1, 5)"), - ("postgres", "SUBSTRING(orders.status FROM 1 FOR 5)"), - ("mysql", "SUBSTRING(orders.status, 1, 5)"), - ("duckdb", "SUBSTRING(orders.status, 1, 5)"), - ("clickhouse", "substr(orders.status, 1, 5)"), + ("sqlite", 'SUBSTRING("orders".status, 1, 5)'), + ("postgres", 'SUBSTRING("orders".status FROM 1 FOR 5)'), + ("mysql", "SUBSTRING(`orders`.status, 1, 5)"), + ("duckdb", 'SUBSTRING("orders".status, 1, 5)'), + ("clickhouse", 'substr("orders".status, 1, 5)'), ], ) async def test_substr_translates_per_dialect( @@ -6812,11 +6831,11 @@ async def test_substr_translates_per_dialect( "dialect,expected_substring", [ # SQLite normalises CONCAT(...) → a || b at emit time. - ("sqlite", "orders.status || orders.status"), - ("postgres", "CONCAT(orders.status, orders.status)"), - ("mysql", "CONCAT(orders.status, orders.status)"), - ("duckdb", "CONCAT(orders.status, orders.status)"), - ("clickhouse", "CONCAT(orders.status, orders.status)"), + ("sqlite", '"orders".status || "orders".status'), + ("postgres", 'CONCAT("orders".status, "orders".status)'), + ("mysql", "CONCAT(`orders`.status, `orders`.status)"), + ("duckdb", 'CONCAT("orders".status, "orders".status)'), + ("clickhouse", 'CONCAT("orders".status, "orders".status)'), ], ) async def test_concat_via_pipe_pipe_translates_per_dialect(