diff --git a/slayer/core/errors.py b/slayer/core/errors.py index 239745a3..79601be8 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -163,3 +163,31 @@ class DistinctDimensionValuesError(SlayerError, ValueError): Multi-inherits ``ValueError`` so existing ``except ValueError`` call sites continue to work unchanged. """ + + +class UnresolvableOrderColumnError(SlayerError, ValueError): + """Raised when an ``order`` item references a column that cannot be bound + to the query's FROM scope (DEV-1645). + + Fires when the sort key is neither a projected output alias, a base-model + column, nor a column on a join that the query already pulled into scope + (via a dimension, measure, or filter). The common case is ordering by an + *unprojected joined column* — e.g. ``order=[{"column": + "customers.regions.name"}]`` — whose join was never resolved, so there is + no in-scope table to qualify against. Emitting a reference anyway would + produce SQL that fails at the database with UndefinedTable/UndefinedColumn; + rejecting at compile time surfaces an actionable error instead. + + Multi-inherits ``ValueError`` so existing ``except ValueError`` call sites + continue to work unchanged. + """ + + def __init__(self, *, column: str, qualifier: str) -> None: + self.column = column + self.qualifier = qualifier + super().__init__( + f"ORDER BY column '{qualifier}.{column}' cannot be resolved: it is not a " + f"projected field, a base column, or a column on a join that is in scope. " + f"Project it (add to dimensions/measures), reference it in a filter, or " + f"order by a projected field instead." + ) diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index 16ca670e..4f09ce40 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -8,7 +8,7 @@ import copy import logging import re -from typing import Any +from typing import Any, NamedTuple import sqlglot from sqlglot import exp @@ -19,6 +19,7 @@ DataType, TimeGranularity, ) +from slayer.core.errors import UnresolvableOrderColumnError from slayer.engine.enriched import EnrichedMeasure, EnrichedQuery, public_projection_aliases from slayer.sql.dialects import SqlDialect, get_dialect @@ -124,6 +125,18 @@ def _wrap_cast_for_type(expr: exp.Expression, dt: DataType | None) -> exp.Expres # at every join site. _SQL_AND_JOINER = " AND " + +class _OrderColRef(NamedTuple): + """DEV-1645: resolved ORDER BY key. ``is_alias`` distinguishes a projected + output alias (emit whole-quoted, e.g. ``"orders.revenue_sum"``) from a + table.column fallback (emit SPLIT, e.g. ``ranked."time_mark"``), so a sort + on an unprojected/renamed column references the underlying FROM-scope column + instead of a nonexistent composite identifier.""" + text: str # whole resolved string (used for base_cols membership) + is_alias: bool # True => projected output alias; False => table.column fallback + qualifier: str | None # fallback only: FROM-scope alias + column: str | None # fallback only: underlying column short name + # DEV-1444: separator used between pretty-printed SELECT projection columns # (",\n "). Extracted as a constant so Sonar S1192 doesn't flag every # join site that follows the same pattern. @@ -392,6 +405,60 @@ def dialect(self) -> str: strategy object from the string sqlglot consumes.""" return self._dialect.sqlglot_name + @staticmethod + def _maybe_quote_ident(ident: exp.Expression | None) -> None: + """Set ``quoted=True`` in place on ``ident`` when it is an unquoted + ``Identifier`` containing an uppercase letter (DEV-1645). No-op + otherwise (None, already-quoted, all-lowercase, non-Identifier).""" + if ( + isinstance(ident, exp.Identifier) + and not ident.quoted + and any(c.isupper() for c in ident.this) + ): + ident.set("quoted", True) + + @staticmethod + def _quote_mixed_case_identifiers(node: exp.Expression) -> exp.Expression: + """DEV-1645: quote mixed-case DB identifiers so case-folding dialects + (Postgres/Redshift fold to lower; Snowflake/Oracle fold to upper) reach + the right physical object instead of silently folding to a non-existent + name. + + Context-aware: quotes only the **column-name leaf** of a ``Column`` + (``Column.this``) and the **physical-table name parts** of a ``Table`` + (``this``/``db``/``catalog``). It deliberately does NOT quote table + aliases or the qualifier side of a column reference — those are + SLayer-internal aliases that fold consistently within a query and never + need quoting; quoting them would only churn output and (for uppercase + model names) diverge from string-built references. Function names parse + to ``Anonymous``/``Func`` and string literals to ``Literal``, so neither + is touched. Idempotent. Applied at every parse / AST-construction site + (``_parse``, ``_parse_predicate``, ``_to_ident``, ``_to_table``).""" + if isinstance(node, exp.Column): + SQLGenerator._maybe_quote_ident(node.this) + elif isinstance(node, exp.Table): + SQLGenerator._maybe_quote_ident(node.this) + SQLGenerator._maybe_quote_ident(node.args.get("db")) + SQLGenerator._maybe_quote_ident(node.args.get("catalog")) + return node + + def _to_ident(self, name: str) -> exp.Identifier: + """Build a column/table-name identifier, quoting it when mixed-case + (DEV-1645). Use for real DB column/table names — NOT for aliases or + qualifiers (those stay unquoted via plain ``exp.to_identifier``).""" + ident = exp.to_identifier(name) + self._maybe_quote_ident(ident) + return ident + + def _to_table(self, name: str, alias: str | None = None) -> exp.Expression: + """Build a (possibly schema-qualified) table reference with mixed-case + physical-name parts quoted (DEV-1645). The ``alias`` is SLayer-internal + and stays unquoted.""" + table = exp.to_table(name).transform(self._quote_mixed_case_identifiers) + if alias is not None: + table.set("alias", exp.TableAlias(this=exp.to_identifier(alias))) + return table + def _q(self, name: str) -> str: """Dialect-aware identifier quoting (DEV-1571 Bug 3 generalised). @@ -434,6 +501,9 @@ def _parse(self, sql: str, *, dialect: str | None = None) -> exp.Expression: # lives inside ``_rewrite_log_aliases`` so unsupported dialects # (oracle; tsql for log2) keep the canonical 2-arg LOG form. tree = tree.transform(self._rewrite_log_aliases) + # DEV-1645: quote mixed-case identifiers so case-folding dialects reach + # the right physical column/table. + tree = tree.transform(self._quote_mixed_case_identifiers) # DEV-1576: target-keyed AST rewrite. Keyed to the generator's TARGET # dialect (``self._dialect``), NOT the parse dialect — expressions are # canonically parsed as Postgres regardless of target, so this is the @@ -468,6 +538,10 @@ def _parse_predicate(self, sql: str, *, dialect: str | None = None) -> exp.Expre ) tree = active.rewrite_parsed_ast(where.this) tree = tree.transform(self._rewrite_log_aliases) + # DEV-1645: quote mixed-case identifiers (same policy as ``_parse``) — + # this is the separate WHERE/HAVING/``filter_sql`` parser, so it needs + # the transform too or predicate-side mixed-case idents stay unquoted. + tree = tree.transform(self._quote_mixed_case_identifiers) # DEV-1576: same target-keyed rewrite as ``_parse`` — a 2-arg ROUND # over a DOUBLE in a Mode-A SQL filter needs the Postgres numeric cast. return self._dialect.rewrite_target_ast(tree) @@ -703,7 +777,7 @@ def _build_combined(self, enriched: EnrichedQuery, alias=exp.to_identifier(cm.source_model_name), ) else: - source_from = exp.to_table(cm.source_sql_table, alias=cm.source_model_name) + source_from = self._to_table(cm.source_sql_table, alias=cm.source_model_name) select = select.from_(source_from) # JOIN target model @@ -713,11 +787,11 @@ def _build_combined(self, enriched: EnrichedQuery, alias=exp.to_identifier(cm.target_model_name), ) else: - target_join = exp.to_table(cm.target_model_sql_table, alias=cm.target_model_name) + target_join = self._to_table(cm.target_model_sql_table, alias=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=self._to_ident(src), table=exp.to_identifier(cm.source_model_name)), + expression=exp.Column(this=self._to_ident(tgt), table=exp.to_identifier(cm.target_model_name)), ) for src, tgt in cm.join_pairs )) @@ -791,7 +865,7 @@ def _build_combined(self, enriched: EnrichedQuery, group_exprs: list[exp.Expression] = [] # Dimensions are already resolved inside the ranked subquery for dim in enriched.dimensions: - col_expr = exp.Column(this=exp.to_identifier(dim.name)) + col_expr = exp.Column(this=self._to_ident(dim.name)) select = select.select(col_expr.as_(dim.alias)) group_exprs.append(col_expr) for td in enriched.time_dimensions: @@ -837,7 +911,7 @@ def _build_combined(self, enriched: EnrichedQuery, alias=exp.to_identifier(target_alias), ) else: - join_target = exp.to_table(target_table, alias=target_alias) + join_target = self._to_table(target_table, alias=target_alias) join_on = self._parse(join_cond) select = select.join(join_target, on=join_on, join_type=jtype.upper()) @@ -915,12 +989,14 @@ def _assemble_combined_sql(self, enriched: EnrichedQuery, } for order_item in enriched.order: col = order_item.column - col_name = self._resolve_order_column(col=col, enriched=enriched) + ref = self._resolve_order_column(col=col, enriched=enriched) direction = "ASC" if order_item.direction == "asc" else "DESC" - if col_name in base_cols: - order_parts.append(f'_base.{self._q(col_name)} {direction}') + if ref.is_alias and ref.text in base_cols: + order_parts.append(f'_base.{self._q(ref.text)} {direction}') + elif ref.is_alias: + order_parts.append(f'{self._q(ref.text)} {direction}') else: - order_parts.append(f'{self._q(col_name)} {direction}') + order_parts.append(f'{self._order_split_sql(ref)} {direction}') sql += "\nORDER BY " + ", ".join(order_parts) if enriched.limit is not None: sql += f"\nLIMIT {enriched.limit}" @@ -935,9 +1011,12 @@ def _apply_pagination_to_sql(self, enriched: EnrichedQuery, sql: str) -> str: order_parts = [] for order_item in enriched.order: col = order_item.column - col_name = SQLGenerator._resolve_order_column(col=col, enriched=enriched) + ref = SQLGenerator._resolve_order_column(col=col, enriched=enriched) direction = "ASC" if order_item.direction == "asc" else "DESC" - order_parts.append(f'{self._q(col_name)} {direction}') + if ref.is_alias: + order_parts.append(f'{self._q(ref.text)} {direction}') + else: + order_parts.append(f'{self._order_split_sql(ref)} {direction}') sql += "\nORDER BY " + ", ".join(order_parts) if enriched.limit is not None: sql += f"\nLIMIT {enriched.limit}" @@ -1191,7 +1270,7 @@ def _build_window_source_select( alias=exp.to_identifier(target_alias), ) else: - join_target = exp.to_table(target_table, alias=target_alias) + join_target = self._to_table(target_table, alias=target_alias) join_on = self._parse(join_cond) select = select.join(join_target, on=join_on, join_type=jtype.upper()) @@ -1294,8 +1373,10 @@ def _generate_base(self, enriched: EnrichedQuery, for dim in enriched.dimensions: col_expr = self._resolve_sql(sql=dim.sql, name=dim.name, model_name=dim.model_name, type=dim.type) if has_first_or_last: - # In ranked subquery, dimensions are already columns — reference directly - col_expr = exp.Column(this=exp.to_identifier(dim.name)) + # In ranked subquery, dimensions are already columns — reference + # directly (DEV-1645: quote mixed-case names so they match the + # ranked subquery's model.* output column on case-folding dialects) + col_expr = exp.Column(this=self._to_ident(dim.name)) select_columns.append(col_expr.as_(dim.alias)) group_by_columns.append(col_expr) @@ -1416,7 +1497,7 @@ def _generate_base(self, enriched: EnrichedQuery, this=parsed_target, alias=exp.to_identifier(target_alias), ) else: - join_target = exp.to_table(target_table, alias=target_alias) + join_target = self._to_table(target_table, alias=target_alias) join_on = self._parse(join_cond) select = select.join(join_target, on=join_on, join_type=jtype.upper()) @@ -1853,8 +1934,13 @@ def _apply_order_limit(self, select: exp.Select, enriched: EnrichedQuery) -> exp if enriched.order: for order_item in enriched.order: col = order_item.column - col_name = self._resolve_order_column(col=col, enriched=enriched) - order_col = exp.Column(this=exp.to_identifier(col_name, quoted=True)) + ref = self._resolve_order_column(col=col, enriched=enriched) + if ref.is_alias: + order_col = exp.Column(this=exp.to_identifier(ref.text, quoted=True)) + else: + order_col = exp.Column( + this=self._to_ident(ref.column), table=exp.to_identifier(ref.qualifier) + ) ascending = order_item.direction == "asc" ordered_kwargs: dict[str, Any] = {"this": order_col, "desc": not ascending} if suppress_nulls_emulation: @@ -1870,16 +1956,32 @@ def _apply_order_limit(self, select: exp.Select, enriched: EnrichedQuery) -> exp return select + def _order_split_sql(self, ref: _OrderColRef) -> str: + """DEV-1645: emit a non-projected ORDER BY key as a SPLIT + ``qualifier.column`` reference (mixed-case-quoted), not one + composite-quoted token.""" + col = exp.Column(this=self._to_ident(ref.column), table=exp.to_identifier(ref.qualifier)) + return col.sql(dialect=self.dialect) + @staticmethod - def _resolve_order_column(col, enriched: EnrichedQuery) -> str: - """Resolve an order column reference to the correct enriched alias. + def _resolve_order_column(col, enriched: EnrichedQuery) -> _OrderColRef: + """Resolve an order column reference to a discriminated result. Users refer to columns by their short name (e.g., ``count``, ``revenue_sum``). The enriched query stores fully qualified aliases (e.g., ``orders._count``, ``orders.revenue_sum``). This method - matches the user-provided name against all enriched columns and - returns the matching alias. If no match is found, the name is - qualified with the model name as a fallback. + matches the user-provided name against all enriched columns. + + When it matches a projected alias, the result carries ``is_alias=True`` + and the caller emits it whole-quoted (``"orders.revenue_sum"`` — that IS + the real output column name via ``AS "orders.revenue_sum"``). + + When no projected alias matches (DEV-1645: renamed via ``columns:``, or + an inner-stage dim the outer stage dropped), the result carries + ``is_alias=False`` with ``qualifier``/``column`` set, and the caller + emits a SPLIT ``qualifier.column`` reference (two identifiers) that + resolves against the FROM-scope table — instead of the old composite + ``"."`` token that Postgres rejects as UndefinedColumn. For ``*:count`` results, the internal name is ``_count`` but users refer to it as ``count``. A fallback check for ``_name`` handles @@ -1907,22 +2009,35 @@ def _resolve_order_column(col, enriched: EnrichedQuery) -> str: # Direct match on the user-provided name if user_name in alias_lookup: - return alias_lookup[user_name] + return _OrderColRef(alias_lookup[user_name], True, None, None) # Qualified match for cross-model measures: # col.model="customers", col.name="revenue_sum" → "customers.revenue_sum" if col.model: qualified = f"{col.model}.{col.name}" if qualified in alias_lookup: - return alias_lookup[qualified] + return _OrderColRef(alias_lookup[qualified], True, None, None) # Fallback for *:count → _count: user says "count", internal is "_count" prefixed = f"_{user_name}" if prefixed in alias_lookup: - return alias_lookup[prefixed] - - # Fallback: qualify with model prefix - return f"{model_prefix}.{user_name}" + return _OrderColRef(alias_lookup[prefixed], True, None, None) + + # Fallback: a non-projected order key is only safe to emit as a split + # reference against the BASE-model alias. DEV-1645: a joined qualifier + # (anything other than the base model) is rejected. Even when a filter + # pulls the join into the base FROM, the compiler's outer-wrapping + # layers (measure CTEs, pagination, the first/last ranked subquery, and + # projection trimming) relocate the ORDER BY into a scope where the + # joined table is unbound — emitting the reference there would produce + # invalid SQL. Ordering by a joined column therefore requires projecting + # it (add it to dimensions) or ordering by a projected field. The + # base-model qualifier is still emitted as a split reference (the + # documented measure-CTE ``_base`` base-column case is intentionally not + # rejected). + if model_prefix != enriched.model_name: + raise UnresolvableOrderColumnError(column=user_name, qualifier=model_prefix) + return _OrderColRef(f"{model_prefix}.{user_name}", False, model_prefix, user_name) # ------------------------------------------------------------------ # FROM / JOIN building @@ -1930,7 +2045,7 @@ 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 self._to_table(enriched.sql_table, alias=enriched.model_name) elif enriched.sql: parsed = self._parse(enriched.sql) return exp.Subquery(this=parsed, alias=exp.to_identifier(enriched.model_name)) @@ -2125,11 +2240,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=self._to_ident(name), table=exp.to_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=self._to_ident(sql), table=exp.to_identifier(model_name)) return _wrap_cast_for_type(self._parse(sql), type) def _resolve_value_sql(self, measure: "EnrichedMeasure") -> str: @@ -2201,7 +2316,7 @@ def _build_agg( type=measure.column_type, ), False return exp.Column( - this=exp.to_identifier(measure.name), + this=self._to_ident(measure.name), table=exp.to_identifier(measure.model_name), ), False @@ -2284,7 +2399,7 @@ def _build_agg( ) else: inner = exp.Column( - this=exp.to_identifier(measure.name), + this=self._to_ident(measure.name), table=exp.to_identifier(measure.model_name), ) diff --git a/tests/integration/test_integration_postgres.py b/tests/integration/test_integration_postgres.py index 744e2d48..5234d10f 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 @@ -1245,3 +1245,136 @@ async def test_stddev_alias_executes(self, pg_env: SlayerQueryEngine) -> None: assert float(result.data[0]["orders.total_stddev_samp"]) == pytest.approx( 92.76, abs=0.1 ) + + +# --------------------------------------------------------------------------- +# DEV-1645: compiler must emit valid Postgres SQL. +# Flavor A — ORDER BY on an unprojected/renamed column. +# Flavor B — mixed-case identifiers (Column.sql, filters, JOIN ON) must be +# double-quoted so Postgres does not fold them to lowercase. +# The models below deliberately write identifiers UNQUOTED (as an OTF agent +# naturally would); the fix is what makes these queries execute. +# --------------------------------------------------------------------------- + +@pytest.fixture(scope="module") +def _pg_dev1645_storage(postgresql_proc, tmp_path_factory): + conn, db_name = _create_module_db(postgresql_proc) + try: + cur = conn.cursor() + # Mixed-case column + mixed-case PK created via QUOTED DDL so their true + # stored case is genuinely mixed (Postgres would otherwise fold to lower). + cur.execute(""" + CREATE TABLE clusters ( + "CLSTR_PIN" INTEGER PRIMARY KEY, + score NUMERIC(10,2) NOT NULL + ) + """) + cur.execute(""" + CREATE TABLE accounts ( + id INTEGER PRIMARY KEY, + "StateFlag" TEXT NOT NULL, + acct_form TEXT NOT NULL, + clu_ref INTEGER REFERENCES clusters("CLSTR_PIN"), + created_at TIMESTAMP NOT NULL + ) + """) + cur.executemany( + 'INSERT INTO clusters ("CLSTR_PIN", score) VALUES (%s, %s)', + [(10, 1.5), (20, 2.5)], + ) + cur.executemany( + "INSERT INTO accounts VALUES (%s, %s, %s, %s, %s)", + [ + (1, "Dormant", "Bot", 10, "2024-01-15 10:00:00"), + (2, "Active", "Human", 20, "2024-02-20 11:00:00"), + (3, "Dormant", "Human", 10, "2024-03-01 09:00:00"), + ], + ) + conn.commit() + + storage = YAMLStorage(base_dir=str(tmp_path_factory.mktemp("pg_dev1645"))) + info = postgresql_proc + run_sync(storage.save_datasource(DatasourceConfig( + name="testpg", type="postgres", host=info.host, port=info.port, + database=db_name, username=info.user, password="", + ))) + + clusters_model = SlayerModel( + name="clusters", sql_table="clusters", data_source="testpg", + columns=[ + # PK column name is mixed-case and written UNQUOTED in the model. + Column(name="CLSTR_PIN", sql="CLSTR_PIN", type=DataType.INT, primary_key=True), + Column(name="score", sql="score", type=DataType.DOUBLE), + ], + ) + accounts_model = SlayerModel( + name="accounts", sql_table="accounts", data_source="testpg", + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + # Derived boolean referencing mixed-case StateFlag UNQUOTED (the + # fake_account_23 repro). Fix must emit "StateFlag". + Column(name="is_inactive", type=DataType.BOOLEAN, + sql="CASE WHEN StateFlag = 'Dormant' THEN TRUE ELSE FALSE END"), + Column(name="acct_form", sql="acct_form", type=DataType.TEXT), + Column(name="clu_ref", sql="clu_ref", type=DataType.INT), + Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP), + ], + # Join key CLSTR_PIN written UNQUOTED (the fake_account_15 repro). + joins=[ModelJoin(target_model="clusters", join_pairs=[["clu_ref", "CLSTR_PIN"]])], + ) + run_sync(storage.save_model(clusters_model)) + run_sync(storage.save_model(accounts_model)) + yield storage + finally: + conn.close() + _drop_module_db(postgresql_proc, db_name) + + +@pytest.fixture +def pg_dev1645(_pg_dev1645_storage): + return SlayerQueryEngine(storage=_pg_dev1645_storage) + + +@pytest.mark.integration +class TestDev1645ValidPostgres: + async def test_flavor_b_mixed_case_column_sql_executes( + self, pg_dev1645: SlayerQueryEngine, + ) -> None: + """Filter on a derived column whose SQL references mixed-case StateFlag + (the fake_account_23 repro) — must execute, not raise UndefinedColumn.""" + query = SlayerQuery( + source_model="accounts", + measures=[{"formula": "*:count", "name": "cnt"}], + filters=["is_inactive == True"], + ) + result = await pg_dev1645.execute(query=query) + assert result.data[0]["accounts.cnt"] == 2 # two Dormant rows + + async def test_flavor_b_mixed_case_join_key_executes( + self, pg_dev1645: SlayerQueryEngine, + ) -> None: + """Cross-model dimension across a join whose key is mixed-case CLSTR_PIN + (the fake_account_15 repro) — must execute.""" + query = SlayerQuery( + source_model="accounts", + dimensions=[ColumnRef(name="score", model="clusters")], + measures=[{"formula": "*:count", "name": "cnt"}], + ) + result = await pg_dev1645.execute(query=query) + by_score = {float(r["accounts.clusters.score"]): r["accounts.cnt"] for r in result.data} + assert by_score[1.5] == 2 + assert by_score[2.5] == 1 + + async def test_flavor_a_orderby_nonprojected_column_executes( + self, pg_dev1645: SlayerQueryEngine, + ) -> None: + """Raw-row query ordering by a base column that is not in the projection + (the mental_healths_1 / organ_transplant_13 repro) — must execute.""" + query = SlayerQuery( + source_model="accounts", + dimensions=[ColumnRef(name="acct_form")], + distinct_dimension_values=False, + order=[OrderItem(column=ColumnRef(name="created_at"), direction="desc")], + ) + result = await pg_dev1645.execute(query=query) + assert result.row_count == 3 diff --git a/tests/integration/test_integration_snowflake.py b/tests/integration/test_integration_snowflake.py index 43fc6464..8ba5fa91 100644 --- a/tests/integration/test_integration_snowflake.py +++ b/tests/integration/test_integration_snowflake.py @@ -733,3 +733,64 @@ def test_query_result_keys_use_lowercase(sf_storage_with_models) -> None: keys = set(first_row.keys()) # Lowercase form is the canonical key. assert "orders.status" in keys, f"Expected lowercase 'orders.status' in row keys, got: {keys}" + + +# --------------------------------------------------------------------------- +# DEV-1645: mixed-case identifiers must be double-quoted for Snowflake. +# +# Snowflake folds UNQUOTED identifiers to UPPERCASE. A column created via +# QUOTED DDL (``"StateFlag"``) is stored with genuinely mixed case, so an +# UNQUOTED reference (which folds to ``STATEFLAG``) fails to resolve — exactly +# the upper-folding mirror of the Postgres bug. The fix quotes it, so the +# model (which writes ``StateFlag`` unquoted, as an OTF agent would) executes. +# --------------------------------------------------------------------------- + +@pytest.fixture +def sf_dev1645_storage(sf_transient_schema: str, sf_datasource: DatasourceConfig, tmp_path): + conn = snowflake.connector.connect(connection_name=_CONNECTION_NAME) + cur = conn.cursor() + try: + cur.execute(f"USE SCHEMA {sf_transient_schema}") + # QUOTED mixed-case column => genuinely stored as "StateFlag". + cur.execute('CREATE TABLE dev1645_accounts (id INT, "StateFlag" TEXT)') + cur.executemany( + 'INSERT INTO dev1645_accounts (id, "StateFlag") VALUES (%s, %s)', + [(1, "Dormant"), (2, "Active"), (3, "Dormant")], + ) + conn.commit() + + storage = YAMLStorage(base_dir=str(tmp_path)) + run_sync(storage.save_datasource(sf_datasource)) + model = SlayerModel( + name="dev1645_accounts", + sql_table="dev1645_accounts", + data_source="sf_test", + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + # Written UNQUOTED — the fix must emit "StateFlag". + Column(name="is_inactive", type=DataType.BOOLEAN, + sql="CASE WHEN StateFlag = 'Dormant' THEN TRUE ELSE FALSE END"), + ], + ) + run_sync(storage.save_model(model)) + yield storage + finally: + try: + cur.execute("DROP TABLE IF EXISTS dev1645_accounts") + conn.commit() + finally: + cur.close() + conn.close() + + +def test_dev1645_mixed_case_column_executes(sf_dev1645_storage) -> None: + """Filter on a derived column referencing UNQUOTED mixed-case StateFlag — + must execute on Snowflake (upper-folding), proving universal quoting is + correct there too.""" + engine = SlayerQueryEngine(storage=sf_dev1645_storage) + result = run_sync(engine.execute(SlayerQuery( + source_model="dev1645_accounts", + measures=[ModelMeasure(formula="*:count", name="cnt")], + filters=["is_inactive == True"], + ))) + assert result.data[0]["dev1645_accounts.cnt"] == 2 diff --git a/tests/test_cross_model_derived_columns.py b/tests/test_cross_model_derived_columns.py index 9df460d4..0abf7f34 100644 --- a/tests/test_cross_model_derived_columns.py +++ b/tests/test_cross_model_derived_columns.py @@ -276,8 +276,15 @@ async def test_joined_model_derived_referencing_further_joined(tmp_path) -> None 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, ( + # And the C join must actually be present in the FROM. DEV-1645 quotes the + # mixed-case physical table name but not the SLayer-internal alias, so the + # asymmetric `JOIN "C" AS B__C` is the current (correct) emitted form. + assert ( + "JOIN C AS B__C" in norm + or '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}" ) diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py new file mode 100644 index 00000000..1a09b902 --- /dev/null +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -0,0 +1,733 @@ +"""DEV-1645: SLayer compiler must not emit invalid Postgres SQL. + +Two defect classes, both valid-on-SQLite / invalid-on-Postgres: + +* **Flavor A — ORDER BY on an unprojected/renamed column.** The sort key was + rendered as a single composite-quoted identifier ``"."`` (the + projection-alias convention), which resolves only when the column is a + projected output alias. For a column that is not projected (renamed by a + ``columns:`` transform, or an inner-stage grouping dim the outer stage + dropped), that composite token is a nonexistent column -> ``UndefinedColumn``. + Fix: emit the fallback sort key as a real split ``table.column`` reference. + +* **Flavor B — mixed-case identifier not double-quoted.** Free-SQL identifiers + referencing mixed-case columns/tables were emitted unquoted; Postgres folds + them to lowercase and can't find them. Fix: quote any identifier containing + an uppercase letter (universal, all dialects), applied at every construction + site (``_parse``, ``_parse_predicate``, and direct ``exp.Column`` / table + builders). + +These are byte-level emission tests; execution against a real Postgres / +Snowflake lives in the integration suites. +""" + +from __future__ import annotations + +import pytest + +import sqlglot + +from slayer.core.enums import DataType, TimeGranularity +from slayer.core.errors import UnresolvableOrderColumnError +from slayer.core.models import Column, ModelJoin, SlayerModel +from slayer.core.query import ColumnRef, OrderItem, SlayerQuery, TimeDimension +from slayer.engine.enrichment import enrich_query +from slayer.engine.query_engine import SlayerQueryEngine +from slayer.sql.generator import SQLGenerator +from slayer.storage.yaml_storage import YAMLStorage + + +async def _noop_async(**kw): # NOSONAR(S7503) — resolver-callback contract is async + return None + + +def _norm(s: str) -> str: + return " ".join(s.split()) + + +async def _generate(query: SlayerQuery, model: SlayerModel, *, dialect: str = "postgres") -> str: + enriched = await enrich_query( + query=query, + model=model, + resolve_dimension_via_joins=_noop_async, + resolve_cross_model_measure=_noop_async, + resolve_join_target=_noop_async, + ) + return SQLGenerator(dialect=dialect).generate(enriched=enriched) + + +async def _generate_via_engine( + query: SlayerQuery, model: SlayerModel, storage: YAMLStorage, *, dialect: str = "postgres" +) -> str: + """Enrich through a real engine+storage so joins resolve from storage.""" + engine = SlayerQueryEngine(storage=storage) + enriched = await engine._enrich(query=query, model=model) + return SQLGenerator(dialect=dialect).generate(enriched=enriched) + + +async def _save_orders_customers_regions(storage: YAMLStorage) -> SlayerModel: + """orders → customers → regions two-hop join graph. Returns the orders + (root) model. Shared by the joined-ORDER-BY reject + deferred xfail tests.""" + 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), + ], + ) + await storage.save_model(regions) + customers = SlayerModel( + name="customers", sql_table="customers", data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="region_id", sql="region_id", type=DataType.DOUBLE), + ], + joins=[ModelJoin(target_model="regions", join_pairs=[["region_id", "id"]])], + ) + await storage.save_model(customers) + orders = SlayerModel( + name="orders", sql_table="orders", data_source="test", + default_time_dimension="created_at", # lets first/last + cumsum tests reuse this graph + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.DOUBLE), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP), + ], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + await storage.save_model(orders) + return orders + + +# ---------------------------------------------------------------------------- +# Fixtures +# ---------------------------------------------------------------------------- + +@pytest.fixture +def orders_model() -> SlayerModel: + return SlayerModel( + name="orders", + sql_table="public.orders", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="status", sql="status", type=DataType.TEXT), + Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP), + Column(name="revenue", sql="amount", type=DataType.DOUBLE), + ], + ) + + +@pytest.fixture +def accounts_model() -> SlayerModel: + """Mirrors the ``fake_account_23`` repro: a derived boolean column whose SQL + references a mixed-case physical column ``StateFlag``.""" + return SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column( + name="is_inactive", + type=DataType.BOOLEAN, + sql="CASE WHEN StateFlag = 'Dormant' THEN TRUE ELSE FALSE END", + ), + Column( + name="acct_form", + type=DataType.TEXT, + sql="acct_form", # lowercase sibling — must stay unquoted + ), + ], + ) + + +# ============================================================================ +# Flavor A — ORDER BY on an unprojected / renamed column +# ============================================================================ + +class TestFlavorAOrderByUnprojected: + async def test_orderby_nonprojected_column_emits_split_not_composite( + self, orders_model: SlayerModel + ) -> None: + """A raw-row query ordering by a base column that is not projected must + emit a split ``orders.created_at`` reference, NOT the composite + ``"orders.created_at"`` (a nonexistent output column on Postgres).""" + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="status")], + distinct_dimension_values=False, + order=[OrderItem(column=ColumnRef(name="created_at"), direction="desc")], + ) + sql = _norm(await _generate(query, orders_model)) + assert "ORDER BY orders.created_at DESC" in sql + assert '"orders.created_at"' not in sql + + async def test_orderby_projected_alias_stays_composite_quoted( + self, orders_model: SlayerModel + ) -> None: + """Regression guard: ordering by a projected measure alias keeps the + whole-quoted composite ``"orders.rev"`` form (that IS the real output + column name via ``AS "orders.rev"``).""" + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="status")], + measures=[{"formula": "revenue:sum", "name": "rev"}], + order=[OrderItem(column=ColumnRef(name="rev"), direction="desc")], + ) + sql = _norm(await _generate(query, orders_model)) + assert 'ORDER BY "orders.rev" DESC' in sql + + async def test_orderby_nonprojected_mixed_case_column_split_and_quoted( + self, orders_model: SlayerModel + ) -> None: + """Flavor A + B together: a non-projected mixed-case sort column emits a + split reference whose column part is quoted: ``orders."CreatedAt"``.""" + model = orders_model.model_copy(deep=True) + model.columns.append(Column(name="CreatedAt", sql="CreatedAt", type=DataType.TIMESTAMP)) + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="status")], + distinct_dimension_values=False, + order=[OrderItem(column=ColumnRef(name="CreatedAt"), direction="desc")], + ) + sql = _norm(await _generate(query, model)) + assert 'ORDER BY orders."CreatedAt" DESC' in sql + assert '"orders.CreatedAt"' not in sql + + async def test_orderby_split_key_keeps_asc_limit_offset( + self, orders_model: SlayerModel + ) -> None: + """ASC direction plus LIMIT / OFFSET are still applied around the split + fallback sort key.""" + query = SlayerQuery( + source_model="orders", + dimensions=[ColumnRef(name="status")], + distinct_dimension_values=False, + order=[OrderItem(column=ColumnRef(name="created_at"), direction="asc")], + limit=10, + offset=5, + ) + sql = _norm(await _generate(query, orders_model)) + assert "ORDER BY orders.created_at ASC" in sql + assert "LIMIT 10" in sql + assert "OFFSET 5" in sql + assert '"orders.created_at"' not in sql + + async def test_orderby_projected_alias_combined_cte_path_unchanged(self, tmp_path) -> None: + """Regression guard for the combined measure-CTE ORDER BY site + (`_assemble_combined_sql`): ordering by a projected cross-model measure + alias keeps the whole-quoted composite form.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + clusters = SlayerModel( + name="clusters", sql_table="clusters", data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="score", sql="score", type=DataType.DOUBLE), + ], + ) + await storage.save_model(clusters) + accts = SlayerModel( + name="accts", sql_table="accts", data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="clu_ref", sql="clu_ref", type=DataType.DOUBLE), + Column(name="status", sql="status", type=DataType.TEXT), + ], + joins=[ModelJoin(target_model="clusters", join_pairs=[["clu_ref", "id"]])], + ) + await storage.save_model(accts) + query = SlayerQuery( + source_model="accts", + dimensions=[ColumnRef(name="status")], + measures=[{"formula": "clusters.score:sum", "name": "sc"}], + order=[OrderItem(column=ColumnRef(name="sc"), direction="desc")], + ) + sql = _norm(await _generate_via_engine(query, accts, storage)) + assert 'ORDER BY "accts.clusters.sc" DESC' in sql + + _save_orders_customers_regions = staticmethod(_save_orders_customers_regions) + + async def test_orderby_unresolvable_joined_column_rejected(self, tmp_path) -> None: + """Ordering by an unprojected multi-hop joined column whose join was + never pulled into scope cannot bind to any FROM table — reject at + compile time rather than emit invalid SQL (DEV-1645, user decision on + Codex review of PR #224).""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await self._save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "amount:sum", "name": "rev"}], + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + with pytest.raises(UnresolvableOrderColumnError): + await _generate_via_engine(query, orders, storage) + + async def test_orderby_joined_column_rejected_even_when_filter_pulls_join_in(self, tmp_path) -> None: + """An unprojected joined ORDER BY is rejected even when a filter pulls + the join into the base FROM: the compiler's outer-wrapping layers + (measure CTEs, pagination, first/last ranked, projection trim) relocate + the ORDER BY into a scope where the joined table is unbound, so resolving + it is unsafe. Project the column or order by a projected field instead.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await self._save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "amount:sum", "name": "rev"}], + filters=["customers.regions.name == 'US'"], + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + with pytest.raises(UnresolvableOrderColumnError): + await _generate_via_engine(query, orders, storage) + + async def test_orderby_joined_column_rejected_in_cte_wrapped_scope(self, tmp_path) -> None: + """A joined column can be resolved in the base SELECT (joins in FROM), + but NOT in a CTE-wrapped scope (measure CTEs / windowed measures order + from `_base`, where the join alias is unbound). Even when a filter pulls + the join in, ordering by that joined column in the combined-CTE path must + reject rather than emit an unbound reference (Codex review of PR #224).""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await _save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + time_dimensions=[TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH)], + measures=[{"formula": "cumsum(amount:sum)", "name": "cs"}], # windowed -> combined CTE path + filters=["customers.regions.name == 'US'"], # pulls the join into resolved_joins + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + with pytest.raises(UnresolvableOrderColumnError): + await _generate_via_engine(query, orders, storage) + + async def test_orderby_joined_column_rejected_in_first_last_ranked_scope(self, tmp_path) -> None: + """first/last measures wrap the FROM (with its joins) in a ranked + subquery that only re-exposes model.* — so the outer ORDER BY can't see + a joined column even when a filter pulled the join in. Must reject, not + emit an unbound reference (Codex review of PR #224).""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await _save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "amount:last(created_at)", "name": "latest"}], # ranked-subquery wrap + filters=["customers.regions.name == 'US'"], # pulls the join into resolved_joins + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + with pytest.raises(UnresolvableOrderColumnError): + await _generate_via_engine(query, orders, storage) + + +# ============================================================================ +# Flavor A — DEFERRED capability: joined / cross-model ORDER BY resolution +# ============================================================================ + +class TestFlavorAJoinedOrderByDeferred: + """DEV-1645 chose to REJECT any unprojected joined/cross-model ORDER BY + (raising ``UnresolvableOrderColumnError``) rather than resolve it, because + the compiler's several outer-wrapping layers (measure CTEs, pagination, the + first/last ranked subquery, projection trim) relocate the ORDER BY into a + scope where the joined table is unbound — so resolving it in the base SELECT + is not reliably safe. + + These xfail tests document the DEFERRED aspiration: ordering by an (in-scope) + joined column should eventually produce valid SQL the way filters already + resolve joined columns. They currently xfail because the code rejects. + ``strict=True`` flips them to XPASS — failing the suite — if joined ORDER BY + resolution is ever implemented, prompting removal of this xfail and the + reject. The companion reject tests in ``TestFlavorAOrderByUnprojected`` pin + the current contract (reject cleanly, never emit invalid SQL).""" + + _REASON = ( + "DEV-1645: joined/cross-model ORDER BY resolution is deferred — currently " + "rejected with UnresolvableOrderColumnError (project the column or order by " + "a projected field). Remove this xfail when joined ORDER BY resolves." + ) + + @pytest.mark.xfail(strict=True, reason=_REASON) + async def test_joined_orderby_with_filter_in_scope_should_resolve(self, tmp_path) -> None: + """A filter pulls the join into the base FROM; ordering by that joined + column should resolve to the canonical ``__`` alias.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await _save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "amount:sum", "name": "rev"}], + filters=["customers.regions.name == 'US'"], + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + sql = _norm(await _generate_via_engine(query, orders, storage)) + assert "ORDER BY customers__regions.name" in sql + + @pytest.mark.xfail(strict=True, reason=_REASON) + async def test_joined_orderby_without_filter_should_pull_join_and_resolve(self, tmp_path) -> None: + """Ordering by a joined column with no other reference should pull the + join in (like filters do) and resolve, rather than reject.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await _save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "amount:sum", "name": "rev"}], + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + sql = _norm(await _generate_via_engine(query, orders, storage)) + assert "ORDER BY customers__regions.name" in sql + + @pytest.mark.xfail(strict=True, reason=_REASON) + async def test_joined_orderby_in_cte_wrapped_scope_should_resolve(self, tmp_path) -> None: + """A windowed-measure (combined-CTE) query that filters on and orders by + a joined column should eventually resolve it in the outer scope.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + orders = await _save_orders_customers_regions(storage) + query = SlayerQuery( + source_model="orders", + time_dimensions=[TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH)], + measures=[{"formula": "cumsum(amount:sum)", "name": "cs"}], + filters=["customers.regions.name == 'US'"], + order=[OrderItem(column=ColumnRef(name="name", model="customers.regions"), direction="desc")], + ) + sql = _norm(await _generate_via_engine(query, orders, storage)) + assert "customers__regions.name" in sql + + +# ============================================================================ +# Flavor B — mixed-case identifier quoting +# ============================================================================ + +class TestFlavorBMixedCaseQuoting: + async def test_column_sql_mixed_case_quoted(self, accounts_model: SlayerModel) -> None: + """Mixed-case ident inside ``Column.sql`` (CASE WHEN) is quoted; a + lowercase sibling column that IS projected stays unquoted.""" + query = SlayerQuery( + source_model="accounts", + dimensions=[ColumnRef(name="is_inactive"), ColumnRef(name="acct_form")], + distinct_dimension_values=False, + ) + sql = _norm(await _generate(query, accounts_model)) + assert 'CASE WHEN "StateFlag" = \'Dormant\'' in sql + # lowercase sibling is emitted (projected) and stays unquoted + assert "accounts.acct_form" in sql + assert '"acct_form"' not in sql + + async def test_filter_predicate_path_mixed_case_quoted( + self, accounts_model: SlayerModel + ) -> None: + """The headline ``fake_account_23`` repro: a filter referencing a + derived column whose SQL holds a mixed-case ident flows through + ``_parse_predicate`` and must emit the quoted form in the WHERE.""" + query = SlayerQuery( + source_model="accounts", + measures=[{"formula": "*:count", "name": "cnt"}], + filters=["is_inactive == True"], + ) + sql = _norm(await _generate(query, accounts_model)) + assert "WHERE" in sql + assert '"StateFlag"' in sql + + async def test_model_filter_mixed_case_quoted(self) -> None: + """A model-level always-applied filter with a mixed-case ident is quoted.""" + model = SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + filters=["StateFlag IS NOT NULL"], + columns=[Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True)], + ) + query = SlayerQuery(source_model="accounts", measures=[{"formula": "*:count"}]) + sql = _norm(await _generate(query, model)) + assert "WHERE" in sql + assert '"StateFlag"' in sql + + async def test_column_filter_mixed_case_quoted(self) -> None: + """A ``Column.filter`` (CASE-WHEN at aggregation time) with a mixed-case + ident is quoted.""" + model = SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column( + name="amount", + sql="amount", + type=DataType.DOUBLE, + filter="StateFlag = 'Active'", + ), + ], + ) + query = SlayerQuery(source_model="accounts", measures=[{"formula": "amount:sum"}]) + sql = _norm(await _generate(query, model)) + assert '"StateFlag"' in sql + + async def test_resolved_join_key_mixed_case_quoted(self, tmp_path) -> None: + """A join whose join_pairs reference a mixed-case key (``CLSTR_PIN``) + emits the quoted form in the ON clause (the ``fake_account_15`` repro).""" + storage = YAMLStorage(base_dir=str(tmp_path)) + cluster = SlayerModel( + name="cluster_analysis", + sql_table="cluster_analysis", + data_source="test", + columns=[ + Column(name="CLSTR_PIN", sql="CLSTR_PIN", type=DataType.DOUBLE, primary_key=True), + Column(name="score", sql="score", type=DataType.DOUBLE), + ], + ) + await storage.save_model(cluster) + accounts = SlayerModel( + name="account_clusters", + sql_table="account_clusters", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="clu_ref", sql="clu_ref", type=DataType.DOUBLE), + ], + joins=[ModelJoin(target_model="cluster_analysis", join_pairs=[["clu_ref", "CLSTR_PIN"]])], + ) + await storage.save_model(accounts) + # A cross-model dimension forces a visible LEFT JOIN ... ON , + # matching the fake_account_15 shape (`ON ... = cluster_analysis.CLSTR_PIN`). + query = SlayerQuery( + source_model="account_clusters", + dimensions=[ColumnRef(name="score", model="cluster_analysis")], + distinct_dimension_values=False, + ) + sql = await _generate_via_engine(query, accounts, storage) + assert "LEFT JOIN" in sql and " ON " in sql + assert '"CLSTR_PIN"' in _norm(sql) + + async def test_bare_mixed_case_dimension_quoted(self) -> None: + """A dimension referencing a bare mixed-case column (``_resolve_sql`` + direct-construction path) quotes both qualifier-free column and any + model qualifier consistently.""" + model = SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="StateFlag", sql="StateFlag", type=DataType.TEXT), + ], + ) + query = SlayerQuery( + source_model="accounts", + dimensions=[ColumnRef(name="StateFlag")], + distinct_dimension_values=False, + ) + sql = _norm(await _generate(query, model)) + assert '"StateFlag"' in sql + + async def test_first_last_ranked_mixed_case_dimension_quoted(self) -> None: + """The first/last ranked-subquery path references group-by dimensions by + bare name against the model.* subquery output; a mixed-case dimension + must be quoted there (DEV-1645, Codex review of PR #224).""" + model = SlayerModel( + name="events", + sql_table="public.events", + data_source="test", + default_time_dimension="ts", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="RegionCode", sql="RegionCode", type=DataType.TEXT), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column(name="ts", sql="ts", type=DataType.TIMESTAMP), + ], + ) + query = SlayerQuery( + source_model="events", + dimensions=[ColumnRef(name="RegionCode")], + measures=[{"formula": "amount:last(ts)", "name": "latest"}], + ) + sql = _norm(await _generate(query, model)) + # The OUTER group-by / projection reference (line 1376 path) must be + # quoted so it matches the ranked subquery's model.* output column. + assert 'GROUP BY "RegionCode"' in sql + assert 'GROUP BY RegionCode' not in sql + + async def test_standard_agg_inner_mixed_case_column_quoted(self) -> None: + """A standard aggregation over a bare mixed-case column quotes the inner + column (the ``_resolve_sql`` / standard-agg inner construction path): + ``SUM(accounts."MixedAmt")``.""" + model = SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="MixedAmt", sql="MixedAmt", type=DataType.DOUBLE), + ], + ) + query = SlayerQuery(source_model="accounts", measures=[{"formula": "MixedAmt:sum", "name": "s"}]) + sql = _norm(await _generate(query, model)) + assert 'SUM(accounts."MixedAmt")' in sql + + async def test_resolved_join_target_table_mixed_case_quoted(self, tmp_path) -> None: + """A join whose TARGET model has a mixed-case physical ``sql_table`` emits + a quoted table name in the LEFT JOIN.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + regions = SlayerModel( + name="regions", sql_table="MyRegions", data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + ], + ) + await storage.save_model(regions) + cust = SlayerModel( + name="cust", sql_table="cust", data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="region_id", sql="region_id", type=DataType.DOUBLE), + ], + joins=[ModelJoin(target_model="regions", join_pairs=[["region_id", "id"]])], + ) + await storage.save_model(cust) + query = SlayerQuery( + source_model="cust", + dimensions=[ColumnRef(name="name", model="regions")], + distinct_dimension_values=False, + ) + sql = _norm(await _generate_via_engine(query, cust, storage)) + assert 'LEFT JOIN "MyRegions"' in sql + + async def test_physical_table_name_mixed_case_quoted(self) -> None: + """A mixed-case physical ``sql_table`` is quoted in the FROM clause.""" + model = SlayerModel( + name="accounts", + sql_table="public.MyAccounts", + data_source="test", + columns=[Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True)], + ) + query = SlayerQuery(source_model="accounts", measures=[{"formula": "*:count"}]) + sql = _norm(await _generate(query, model)) + assert '"MyAccounts"' in sql + + async def test_lowercase_only_expression_unchanged(self, orders_model: SlayerModel) -> None: + """No spurious quoting: an all-lowercase expression emits unchanged.""" + query = SlayerQuery(source_model="orders", measures=[{"formula": "revenue:sum"}]) + sql = _norm(await _generate(query, orders_model)) + assert "SUM(orders.amount)" in sql + assert '"amount"' not in sql + + async def test_function_name_not_quoted(self) -> None: + """A function call whose name is mixed-case-ish stays a function — only + Identifier nodes are quoted, not Func/Anonymous names.""" + model = SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="j", sql="COALESCE(RawVal, 0)", type=DataType.DOUBLE), + ], + ) + query = SlayerQuery(source_model="accounts", measures=[{"formula": "j:sum"}]) + sql = _norm(await _generate(query, model)) + assert "COALESCE" in sql + assert '"COALESCE"' not in sql + assert '"RawVal"' in sql # the mixed-case *argument* is quoted + + async def test_mixed_case_join_target_alias_consistent(self, tmp_path) -> None: + """Def/ref consistency (Codex finding #4): when a join key is mixed-case, + the ON reference and the projected join column resolve consistently — the + emitted SQL parses and every quoted identifier is balanced.""" + storage = YAMLStorage(base_dir=str(tmp_path)) + regions = SlayerModel( + name="regions", + sql_table="regions", + data_source="test", + columns=[ + Column(name="RegionId", sql="RegionId", type=DataType.DOUBLE, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + ], + ) + await storage.save_model(regions) + customers = SlayerModel( + name="customers", + sql_table="customers", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="RegionRef", sql="RegionRef", type=DataType.DOUBLE), + ], + joins=[ModelJoin(target_model="regions", join_pairs=[["RegionRef", "RegionId"]])], + ) + await storage.save_model(customers) + query = SlayerQuery( + source_model="customers", + dimensions=[ColumnRef(name="name", model="regions")], + distinct_dimension_values=False, + ) + sql = await _generate_via_engine(query, customers, storage) + # both mixed-case join keys quoted, and the SQL parses cleanly + assert '"RegionRef"' in sql + assert '"RegionId"' in sql + sqlglot.parse_one(sql, dialect="postgres") + + +class TestMixedCaseHelperUnit: + """Direct policy tests for the shared quoting helper / constructors.""" + + def test_quote_mixed_case_identifiers_quotes_uppercase_only(self) -> None: + gen = SQLGenerator(dialect="postgres") + tree = sqlglot.parse_one("CASE WHEN accounts.StateFlag = 'x' THEN 1 ELSE 0 END") + out = tree.transform(gen._quote_mixed_case_identifiers).sql(dialect="postgres") + assert '"StateFlag"' in out + assert "accounts" in out and '"accounts"' not in out # lowercase qualifier untouched + + def test_quote_mixed_case_idempotent_and_skips_prequoted(self) -> None: + gen = SQLGenerator(dialect="postgres") + tree = sqlglot.parse_one('accounts."StateFlag" = 1') + once = tree.transform(gen._quote_mixed_case_identifiers) + twice = once.transform(gen._quote_mixed_case_identifiers) + assert twice.sql(dialect="postgres").count('"StateFlag"') == 1 # no double-quoting + + def test_to_ident_quotes_mixed_case(self) -> None: + gen = SQLGenerator(dialect="postgres") + assert gen._to_ident("StateFlag").sql(dialect="postgres") == '"StateFlag"' + assert gen._to_ident("state_flag").sql(dialect="postgres") == "state_flag" + + def test_to_table_quotes_physical_name_not_alias(self) -> None: + """Physical table name is quoted when mixed-case; the SLayer-internal + alias is left unquoted (it folds consistently within the query).""" + gen = SQLGenerator(dialect="postgres") + out = gen._to_table("public.MyTable", alias="MyAlias").sql(dialect="postgres") + assert '"MyTable"' in out + assert '"MyAlias"' not in out + assert "AS MyAlias" in out + + +# ============================================================================ +# Multi-dialect: universal quoting with per-dialect quote characters +# ============================================================================ + +class TestMixedCaseQuotingMultiDialect: + _QUOTE = { + "postgres": ('"', '"'), + "sqlite": ('"', '"'), + "duckdb": ('"', '"'), + "snowflake": ('"', '"'), + "mysql": ("`", "`"), + "tsql": ("[", "]"), + } + + @pytest.mark.parametrize("dialect", list(_QUOTE)) + async def test_mixed_case_column_quoted_per_dialect(self, dialect: str) -> None: + model = SlayerModel( + name="accounts", + sql_table="public.accounts", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column( + name="is_inactive", + type=DataType.BOOLEAN, + sql="CASE WHEN StateFlag = 'Dormant' THEN TRUE ELSE FALSE END", + ), + ], + ) + query = SlayerQuery( + source_model="accounts", + dimensions=[ColumnRef(name="is_inactive")], + distinct_dimension_values=False, + ) + sql = _norm(await _generate(query, model, dialect=dialect)) + lq, rq = self._QUOTE[dialect] + assert f"{lq}StateFlag{rq}" in sql