From b4a7315908877a11eb03bb66dd21a3d55a90b445 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 15:35:37 +0200 Subject: [PATCH 1/7] Fix SLayer compiler emitting invalid Postgres SQL (DEV-1645) Two valid-on-SQLite / invalid-on-Postgres compiler defects, both surfacing as UndefinedColumn on real execution (SLayer's dry_run compiles without executing, which hid them until submit_query's dry-run gate). Flavor A - ORDER BY on an unprojected/renamed column. The sort key was rendered as one composite-quoted identifier ".", which resolves only when the column is a projected output alias. _resolve_order_column now returns a discriminated _OrderColRef: projected aliases stay whole-quoted; non-projected/renamed keys emit a split table.column reference (two identifiers) that resolves against the FROM-scope column. Applied at all three ORDER BY emission sites. Flavor B - mixed-case identifiers emitted unquoted, so case-folding dialects (Postgres/Redshift to lower; Snowflake/Oracle to upper) fold them to a non-existent name. A context-aware quoting pass quotes real DB identifiers - column-name leaves and physical table-name parts - while leaving SLayer-internal aliases/qualifiers alone (they fold consistently within a query). Wired into both parse paths (_parse and _parse_predicate) and every direct AST-construction site: _resolve_sql, _build_from_clause, cross-model measure tables/join keys, resolved-join targets, and sql=None measures. Universal across dialects. Tests: new tests/test_dev1645_invalid_postgres_sql.py (unit: both flavors, helper-level, multi-dialect); real-Postgres integration tests reproducing the exact UndefinedColumn failures; a Snowflake upper-fold test. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/sql/generator.py | 161 +++++- .../integration/test_integration_postgres.py | 135 ++++- .../integration/test_integration_snowflake.py | 61 ++ tests/test_cross_model_derived_columns.py | 11 +- tests/test_dev1645_invalid_postgres_sql.py | 530 ++++++++++++++++++ 5 files changed, 864 insertions(+), 34 deletions(-) create mode 100644 tests/test_dev1645_invalid_postgres_sql.py diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index 16ca670e..bc42d098 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 @@ -124,6 +124,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 +404,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 +500,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 +537,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 +776,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 +786,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 )) @@ -837,7 +910,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 +988,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 +1010,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 +1269,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()) @@ -1416,7 +1494,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 +1931,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 +1953,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 +2006,22 @@ 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] + return _OrderColRef(alias_lookup[prefixed], True, None, None) - # Fallback: qualify with model prefix - return f"{model_prefix}.{user_name}" + # Fallback: split table.column reference against the FROM-scope alias. + return _OrderColRef(f"{model_prefix}.{user_name}", False, model_prefix, user_name) # ------------------------------------------------------------------ # FROM / JOIN building @@ -1930,7 +2029,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 +2224,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 +2300,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 +2383,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..5caffcd4 --- /dev/null +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -0,0 +1,530 @@ +"""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 +from slayer.core.models import Column, ModelJoin, SlayerModel +from slayer.core.query import ColumnRef, OrderItem, SlayerQuery +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) + + +# ---------------------------------------------------------------------------- +# 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 + + +# ============================================================================ +# 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_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 From 3160939637c450f3378384c8002b4a37f323fefb Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 15:57:14 +0200 Subject: [PATCH 2/7] Quote mixed-case dimensions in the first/last ranked-subquery path (DEV-1645) Codex review of PR #224: the first/last aggregation path referenced group-by dimensions by bare name against the ranked subquery's model.* output via unquoted exp.to_identifier(dim.name). A mixed-case dimension folded to lower on Postgres (upper on Snowflake) and failed to match the subquery output column. Route both ranked-path dimension-reference sites through _to_ident so mixed-case names are quoted, matching the rest of the DEV-1645 pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/sql/generator.py | 8 ++++--- tests/test_dev1645_invalid_postgres_sql.py | 27 ++++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index bc42d098..055405f8 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -864,7 +864,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: @@ -1372,8 +1372,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) diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py index 5caffcd4..a8b73ce8 100644 --- a/tests/test_dev1645_invalid_postgres_sql.py +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -338,6 +338,33 @@ async def test_bare_mixed_case_dimension_quoted(self) -> None: 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): From 0e52d3a95ae3f5bb0650e3ed2197916edadd5ef6 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 16:59:56 +0200 Subject: [PATCH 3/7] Reject unresolvable ORDER BY columns instead of emitting invalid SQL (DEV-1645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of PR #224: a plain-column ORDER BY on an unprojected multi-hop joined path (e.g. order by customers.regions.name) never pulled its join into scope — filters resolve such joins, ORDER BY did not — so the split fallback emitted a reference to a phantom alias, i.e. invalid SQL, defeating the point of the PR. _resolve_order_column now resolves the fallback qualifier to an in-scope table: the base model, or a join already pulled into scope (dimension / measure / filter), mapping a multi-hop dotted qualifier (customers.regions) to its canonical __ alias (customers__regions). When neither is in scope it raises the new UnresolvableOrderColumnError (SlayerError, ValueError) with an actionable message, rather than emitting SQL that fails at the database. Base-column unprojected ORDER BY (the reported flavor-A cases) still emits the split reference; the measure-CTE _base combined path stays a documented limitation (base-model qualifier is treated as in-scope, so it is not rejected). Adds tests: reject-when-unresolvable, resolve-via-__-alias-when-in- scope. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/core/errors.py | 28 ++++++++++ slayer/sql/generator.py | 17 +++++- tests/test_dev1645_invalid_postgres_sql.py | 63 ++++++++++++++++++++++ 3 files changed, 107 insertions(+), 1 deletion(-) 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 055405f8..aef7b475 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -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 @@ -2023,7 +2024,21 @@ def _resolve_order_column(col, enriched: EnrichedQuery) -> _OrderColRef: return _OrderColRef(alias_lookup[prefixed], True, None, None) # Fallback: split table.column reference against the FROM-scope alias. - return _OrderColRef(f"{model_prefix}.{user_name}", False, model_prefix, user_name) + # DEV-1645: resolve the qualifier to an in-scope table — the base model + # or a join already pulled into scope. A multi-hop dotted qualifier + # (``customers.regions``) maps to its ``__`` join alias + # (``customers__regions``). If neither is in scope (ordering by an + # unprojected joined column whose join was never resolved), reject + # rather than emit SQL that fails at the database. + in_scope = {enriched.model_name} | {a for _, a, _, _ in enriched.resolved_joins} + join_alias = model_prefix.replace(".", "__") + if model_prefix in in_scope: + qualifier = model_prefix + elif join_alias in in_scope: + qualifier = join_alias + else: + raise UnresolvableOrderColumnError(column=user_name, qualifier=model_prefix) + return _OrderColRef(f"{qualifier}.{user_name}", False, qualifier, user_name) # ------------------------------------------------------------------ # FROM / JOIN building diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py index a8b73ce8..4ede42d5 100644 --- a/tests/test_dev1645_invalid_postgres_sql.py +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -28,6 +28,7 @@ import sqlglot from slayer.core.enums import DataType +from slayer.core.errors import UnresolvableOrderColumnError from slayer.core.models import Column, ModelJoin, SlayerModel from slayer.core.query import ColumnRef, OrderItem, SlayerQuery from slayer.engine.enrichment import enrich_query @@ -211,6 +212,68 @@ async def test_orderby_projected_alias_combined_cte_path_unchanged(self, tmp_pat sql = _norm(await _generate_via_engine(query, accts, storage)) assert 'ORDER BY "accts.clusters.sc" DESC' in sql + @staticmethod + async def _save_orders_customers_regions(storage: YAMLStorage) -> SlayerModel: + 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", + 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), + ], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + await storage.save_model(orders) + return orders + + 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_in_scope_uses_join_alias(self, tmp_path) -> None: + """When the join IS in scope (pulled in by a filter), ordering by the + joined column resolves to the canonical ``__`` join alias, not a bogus + dotted composite.""" + 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")], + ) + sql = _norm(await _generate_via_engine(query, orders, storage)) + assert "ORDER BY customers__regions.name" in sql + assert '"customers.regions"' not in sql + # ============================================================================ # Flavor B — mixed-case identifier quoting From f6b32aa32fa5d9290cba30ac6971e2884f2c34b3 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 17:11:02 +0200 Subject: [PATCH 4/7] Make ORDER BY reject scope-aware per emission site (DEV-1645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up Codex review of PR #224: the unresolvable-ORDER-BY reject check consulted enriched.resolved_joins globally, so a join pulled into scope by a filter was treated as ORDER BY-visible even in the CTE-wrapped emission paths (_assemble_combined_sql / _apply_pagination_to_sql), where the outer query orders from _base / _filtered / measure CTEs and the join alias is NOT bound. That accepted a joined order column and emitted an unbound reference — invalid SQL — instead of rejecting. _resolve_order_column now takes joins_in_scope, set by the caller: True only for the base SELECT (_apply_order_limit), where the resolved LEFT JOINs are physically in the FROM; False for the two CTE-wrapped sites, which therefore reject a joined order column. The base-model qualifier remains in-scope either way, so the documented _base base-column limitation is still not rejected. Adds a test covering a windowed-measure combined-CTE query that filters on and orders by a joined column. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/sql/generator.py | 23 +++++++--- tests/test_dev1645_invalid_postgres_sql.py | 50 +++++++++++++++++++++- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index aef7b475..d24cd49f 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -989,7 +989,7 @@ def _assemble_combined_sql(self, enriched: EnrichedQuery, } for order_item in enriched.order: col = order_item.column - ref = self._resolve_order_column(col=col, enriched=enriched) + ref = self._resolve_order_column(col=col, enriched=enriched, joins_in_scope=False) direction = "ASC" if order_item.direction == "asc" else "DESC" if ref.is_alias and ref.text in base_cols: order_parts.append(f'_base.{self._q(ref.text)} {direction}') @@ -1011,7 +1011,7 @@ def _apply_pagination_to_sql(self, enriched: EnrichedQuery, sql: str) -> str: order_parts = [] for order_item in enriched.order: col = order_item.column - ref = SQLGenerator._resolve_order_column(col=col, enriched=enriched) + ref = SQLGenerator._resolve_order_column(col=col, enriched=enriched, joins_in_scope=False) direction = "ASC" if order_item.direction == "asc" else "DESC" if ref.is_alias: order_parts.append(f'{self._q(ref.text)} {direction}') @@ -1934,7 +1934,7 @@ def _apply_order_limit(self, select: exp.Select, enriched: EnrichedQuery) -> exp if enriched.order: for order_item in enriched.order: col = order_item.column - ref = self._resolve_order_column(col=col, enriched=enriched) + ref = self._resolve_order_column(col=col, enriched=enriched, joins_in_scope=True) if ref.is_alias: order_col = exp.Column(this=exp.to_identifier(ref.text, quoted=True)) else: @@ -1964,7 +1964,7 @@ def _order_split_sql(self, ref: _OrderColRef) -> str: return col.sql(dialect=self.dialect) @staticmethod - def _resolve_order_column(col, enriched: EnrichedQuery) -> _OrderColRef: + def _resolve_order_column(col, enriched: EnrichedQuery, *, joins_in_scope: bool) -> _OrderColRef: """Resolve an order column reference to a discriminated result. Users refer to columns by their short name (e.g., ``count``, @@ -2030,7 +2030,20 @@ def _resolve_order_column(col, enriched: EnrichedQuery) -> _OrderColRef: # (``customers__regions``). If neither is in scope (ordering by an # unprojected joined column whose join was never resolved), reject # rather than emit SQL that fails at the database. - in_scope = {enriched.model_name} | {a for _, a, _, _ in enriched.resolved_joins} + # + # ``joins_in_scope`` is set by the caller per emission site: True only + # for the base SELECT (``_apply_order_limit``), where the resolved + # LEFT JOINs are physically in the FROM. The CTE-wrapped sites + # (``_assemble_combined_sql`` / ``_apply_pagination_to_sql``) order from + # ``_base`` / ``_filtered`` / measure CTEs, so a filter-pulled join + # alias is NOT bound there — treating it as in-scope would emit an + # unbound reference, so those sites exclude joins and reject a joined + # order column. The base-model qualifier stays in-scope either way (the + # documented ``_base`` base-column limitation is intentionally not + # rejected). + in_scope = {enriched.model_name} + if joins_in_scope: + in_scope |= {a for _, a, _, _ in enriched.resolved_joins} join_alias = model_prefix.replace(".", "__") if model_prefix in in_scope: qualifier = model_prefix diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py index 4ede42d5..c7f045f9 100644 --- a/tests/test_dev1645_invalid_postgres_sql.py +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -27,10 +27,10 @@ import sqlglot -from slayer.core.enums import DataType +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 +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 @@ -274,6 +274,52 @@ async def test_orderby_joined_column_in_scope_uses_join_alias(self, tmp_path) -> assert "ORDER BY customers__regions.name" in sql assert '"customers.regions"' not in sql + 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)) + 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", + 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) + 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) + # ============================================================================ # Flavor B — mixed-case identifier quoting From 6ede315c6fb0aefa0266809f09f921e6f37867c1 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Mon, 6 Jul 2026 17:19:46 +0200 Subject: [PATCH 5/7] Treat first/last ranked wrap as out-of-scope for joined ORDER BY (DEV-1645) Further Codex review of PR #224: _apply_order_limit resolves the base SELECT's ORDER BY, but when _generate_base wraps the FROM in a first/last ranked subquery the joins live INSIDE that subquery and the outer SELECT only re- exposes model.*. Passing joins_in_scope=True unconditionally there would let a filter-pulled joined column emit an unbound outer reference. Capture the wrap decision (from_wrapped_in_ranked) and pass joins_in_scope=not from_wrapped_in_ranked into _apply_order_limit, so the ranked-wrap case rejects a joined ORDER BY like the other CTE-wrapped paths. Base-column ORDER BY is unaffected (base-model qualifier stays in-scope). Adds a test for the first/last-wrap + filter-pulled-join + order-by-joined-column case. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/sql/generator.py | 19 +++++++++++++++---- tests/test_dev1645_invalid_postgres_sql.py | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index d24cd49f..ddfbddd2 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -1357,7 +1357,11 @@ def _generate_base(self, enriched: EnrichedQuery, rn_suffix_map: dict[str, str] = {} filtered_rn_map: dict[str, str] = {} filtered_match_map: dict[str, str] = {} - if has_first_or_last and enriched.last_agg_time_column: + # DEV-1645: when the FROM is wrapped in a first/last ranked subquery, the + # joins live INSIDE that subquery (the outer SELECT only sees model.*), + # so joined columns are NOT in the outer ORDER BY scope. + from_wrapped_in_ranked = bool(has_first_or_last and enriched.last_agg_time_column) + if from_wrapped_in_ranked: ( from_clause, rn_suffix_map, @@ -1467,7 +1471,10 @@ def _generate_base(self, enriched: EnrichedQuery, and not skip_isolated and not has_post_filters ): - select = self._apply_order_limit(select=select, enriched=enriched) + select = self._apply_order_limit( + select=select, enriched=enriched, + joins_in_scope=not from_wrapped_in_ranked, + ) # Append LEFT JOINs from resolved joins via sqlglot AST (works for both # sql_table and inline-SQL models). @@ -1914,7 +1921,9 @@ def _build_self_join_column(self, transform: str, right_table: str, return prev raise ValueError(f"Unknown self-join transform: {transform}") - def _apply_order_limit(self, select: exp.Select, enriched: EnrichedQuery) -> exp.Select: + def _apply_order_limit( + self, select: exp.Select, enriched: EnrichedQuery, *, joins_in_scope: bool = True + ) -> exp.Select: """Apply ORDER BY, LIMIT, OFFSET to a select expression. DEV-1571 Bug 2 follow-up: on MySQL / T-SQL, sqlglot emits a @@ -1934,7 +1943,9 @@ def _apply_order_limit(self, select: exp.Select, enriched: EnrichedQuery) -> exp if enriched.order: for order_item in enriched.order: col = order_item.column - ref = self._resolve_order_column(col=col, enriched=enriched, joins_in_scope=True) + ref = self._resolve_order_column( + col=col, enriched=enriched, joins_in_scope=joins_in_scope + ) if ref.is_alias: order_col = exp.Column(this=exp.to_identifier(ref.text, quoted=True)) else: diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py index c7f045f9..de9a1934 100644 --- a/tests/test_dev1645_invalid_postgres_sql.py +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -320,6 +320,26 @@ async def test_orderby_joined_column_rejected_in_cte_wrapped_scope(self, tmp_pat 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 self._save_orders_customers_regions(storage) + # add a time column so first/last has a time context + orders.columns.append(Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP)) + orders.default_time_dimension = "created_at" + await storage.save_model(orders) + 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 B — mixed-case identifier quoting From 616f4a9657b3ab3c0e89cec126db87e6239ba6f6 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 7 Jul 2026 09:05:54 +0200 Subject: [PATCH 6/7] Always reject unprojected joined ORDER BY; xfail the deferred capability (DEV-1645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex kept finding new outer-wrapping layers (measure CTEs, pagination, first/last ranked subquery, and now projection trim) that relocate the ORDER BY out of the base SELECT scope where a filter-pulled join alias is bound. Threading joins_in_scope through each layer was whack-a-mole, so per the chosen design, drop it: _resolve_order_column now rejects ANY unprojected joined / cross-model ORDER BY column (qualifier != the query's source_model) with UnresolvableOrderColumnError, and only emits a split reference for a base-model qualifier. This still fixes every reported Postgres failure: all reported failing ORDER BY qualifiers are the query's own source_model stage (ranked / agg / waitlist_longest_report / snap_metrics / tlc / AuditAndCompliance), i.e. base columns emitted as a split reference — none order by a joined column. Removes the joins_in_scope plumbing across the three emission sites and _apply_order_limit. Adds a TestFlavorAJoinedOrderByDeferred class of xfail(strict=True) tests documenting the deferred aspiration (joined ORDER BY should eventually resolve like filters do); they flip to XPASS and fail the suite if the capability is implemented, prompting removal of the reject. The reject-behaviour tests remain as the current contract. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/sql/generator.py | 65 +++----- tests/test_dev1645_invalid_postgres_sql.py | 179 ++++++++++++++++----- 2 files changed, 162 insertions(+), 82 deletions(-) diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index ddfbddd2..4f09ce40 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -989,7 +989,7 @@ def _assemble_combined_sql(self, enriched: EnrichedQuery, } for order_item in enriched.order: col = order_item.column - ref = self._resolve_order_column(col=col, enriched=enriched, joins_in_scope=False) + ref = self._resolve_order_column(col=col, enriched=enriched) direction = "ASC" if order_item.direction == "asc" else "DESC" if ref.is_alias and ref.text in base_cols: order_parts.append(f'_base.{self._q(ref.text)} {direction}') @@ -1011,7 +1011,7 @@ def _apply_pagination_to_sql(self, enriched: EnrichedQuery, sql: str) -> str: order_parts = [] for order_item in enriched.order: col = order_item.column - ref = SQLGenerator._resolve_order_column(col=col, enriched=enriched, joins_in_scope=False) + ref = SQLGenerator._resolve_order_column(col=col, enriched=enriched) direction = "ASC" if order_item.direction == "asc" else "DESC" if ref.is_alias: order_parts.append(f'{self._q(ref.text)} {direction}') @@ -1357,11 +1357,7 @@ def _generate_base(self, enriched: EnrichedQuery, rn_suffix_map: dict[str, str] = {} filtered_rn_map: dict[str, str] = {} filtered_match_map: dict[str, str] = {} - # DEV-1645: when the FROM is wrapped in a first/last ranked subquery, the - # joins live INSIDE that subquery (the outer SELECT only sees model.*), - # so joined columns are NOT in the outer ORDER BY scope. - from_wrapped_in_ranked = bool(has_first_or_last and enriched.last_agg_time_column) - if from_wrapped_in_ranked: + if has_first_or_last and enriched.last_agg_time_column: ( from_clause, rn_suffix_map, @@ -1471,10 +1467,7 @@ def _generate_base(self, enriched: EnrichedQuery, and not skip_isolated and not has_post_filters ): - select = self._apply_order_limit( - select=select, enriched=enriched, - joins_in_scope=not from_wrapped_in_ranked, - ) + select = self._apply_order_limit(select=select, enriched=enriched) # Append LEFT JOINs from resolved joins via sqlglot AST (works for both # sql_table and inline-SQL models). @@ -1921,9 +1914,7 @@ def _build_self_join_column(self, transform: str, right_table: str, return prev raise ValueError(f"Unknown self-join transform: {transform}") - def _apply_order_limit( - self, select: exp.Select, enriched: EnrichedQuery, *, joins_in_scope: bool = True - ) -> exp.Select: + def _apply_order_limit(self, select: exp.Select, enriched: EnrichedQuery) -> exp.Select: """Apply ORDER BY, LIMIT, OFFSET to a select expression. DEV-1571 Bug 2 follow-up: on MySQL / T-SQL, sqlglot emits a @@ -1943,9 +1934,7 @@ def _apply_order_limit( if enriched.order: for order_item in enriched.order: col = order_item.column - ref = self._resolve_order_column( - col=col, enriched=enriched, joins_in_scope=joins_in_scope - ) + 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: @@ -1975,7 +1964,7 @@ def _order_split_sql(self, ref: _OrderColRef) -> str: return col.sql(dialect=self.dialect) @staticmethod - def _resolve_order_column(col, enriched: EnrichedQuery, *, joins_in_scope: bool) -> _OrderColRef: + 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``, @@ -2034,35 +2023,21 @@ def _resolve_order_column(col, enriched: EnrichedQuery, *, joins_in_scope: bool) if prefixed in alias_lookup: return _OrderColRef(alias_lookup[prefixed], True, None, None) - # Fallback: split table.column reference against the FROM-scope alias. - # DEV-1645: resolve the qualifier to an in-scope table — the base model - # or a join already pulled into scope. A multi-hop dotted qualifier - # (``customers.regions``) maps to its ``__`` join alias - # (``customers__regions``). If neither is in scope (ordering by an - # unprojected joined column whose join was never resolved), reject - # rather than emit SQL that fails at the database. - # - # ``joins_in_scope`` is set by the caller per emission site: True only - # for the base SELECT (``_apply_order_limit``), where the resolved - # LEFT JOINs are physically in the FROM. The CTE-wrapped sites - # (``_assemble_combined_sql`` / ``_apply_pagination_to_sql``) order from - # ``_base`` / ``_filtered`` / measure CTEs, so a filter-pulled join - # alias is NOT bound there — treating it as in-scope would emit an - # unbound reference, so those sites exclude joins and reject a joined - # order column. The base-model qualifier stays in-scope either way (the - # documented ``_base`` base-column limitation is intentionally not + # 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). - in_scope = {enriched.model_name} - if joins_in_scope: - in_scope |= {a for _, a, _, _ in enriched.resolved_joins} - join_alias = model_prefix.replace(".", "__") - if model_prefix in in_scope: - qualifier = model_prefix - elif join_alias in in_scope: - qualifier = join_alias - else: + if model_prefix != enriched.model_name: raise UnresolvableOrderColumnError(column=user_name, qualifier=model_prefix) - return _OrderColRef(f"{qualifier}.{user_name}", False, qualifier, user_name) + return _OrderColRef(f"{model_prefix}.{user_name}", False, model_prefix, user_name) # ------------------------------------------------------------------ # FROM / JOIN building diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py index de9a1934..87e2495e 100644 --- a/tests/test_dev1645_invalid_postgres_sql.py +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -65,6 +65,39 @@ async def _generate_via_engine( 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", + 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), + ], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + await storage.save_model(orders) + return orders + + # ---------------------------------------------------------------------------- # Fixtures # ---------------------------------------------------------------------------- @@ -212,36 +245,7 @@ async def test_orderby_projected_alias_combined_cte_path_unchanged(self, tmp_pat sql = _norm(await _generate_via_engine(query, accts, storage)) assert 'ORDER BY "accts.clusters.sc" DESC' in sql - @staticmethod - async def _save_orders_customers_regions(storage: YAMLStorage) -> SlayerModel: - 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", - 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), - ], - joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], - ) - await storage.save_model(orders) - return orders + _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 @@ -258,10 +262,12 @@ async def test_orderby_unresolvable_joined_column_rejected(self, tmp_path) -> No with pytest.raises(UnresolvableOrderColumnError): await _generate_via_engine(query, orders, storage) - async def test_orderby_joined_column_in_scope_uses_join_alias(self, tmp_path) -> None: - """When the join IS in scope (pulled in by a filter), ordering by the - joined column resolves to the canonical ``__`` join alias, not a bogus - dotted composite.""" + 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( @@ -270,9 +276,8 @@ async def test_orderby_joined_column_in_scope_uses_join_alias(self, tmp_path) -> 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 - assert '"customers.regions"' not in sql + 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), @@ -341,6 +346,106 @@ async def test_orderby_joined_column_rejected_in_first_last_ranked_scope(self, t 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)) + 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", + 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) + 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 # ============================================================================ From 8c7bbf2304149315da282c311cd40c0660edff53 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Tue, 7 Jul 2026 09:20:03 +0200 Subject: [PATCH 7/7] Deduplicate joined-ORDER-BY test setups to clear Sonar duplication gate (DEV-1645) The Sonar quality gate failed on new_duplicated_lines_density (7.9% vs 3%): as the review loop added the CTE-wrapped / first-last / xfail joined-ORDER-BY tests, each rebuilt the same regions -> customers -> orders model graph inline (CPD flagged two 37-line + three 19-line blocks in tests/test_dev1645_invalid_postgres_sql.py, 11.7% density). Extend the shared _save_orders_customers_regions helper with a created_at column + default_time_dimension and route the three inline-rebuilding tests through it, collapsing the repeated graph to one definition. No behavioural change; suite still 6485 passed, 5 xfailed. (The integration-file duplication Sonar also lists is pre-existing mirror-of-dialect-tests code, not new to this PR.) Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_dev1645_invalid_postgres_sql.py | 68 ++-------------------- 1 file changed, 5 insertions(+), 63 deletions(-) diff --git a/tests/test_dev1645_invalid_postgres_sql.py b/tests/test_dev1645_invalid_postgres_sql.py index 87e2495e..1a09b902 100644 --- a/tests/test_dev1645_invalid_postgres_sql.py +++ b/tests/test_dev1645_invalid_postgres_sql.py @@ -87,10 +87,12 @@ async def _save_orders_customers_regions(storage: YAMLStorage) -> SlayerModel: 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"]])], ) @@ -286,35 +288,7 @@ async def test_orderby_joined_column_rejected_in_cte_wrapped_scope(self, tmp_pat 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)) - 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", - 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) + orders = await _save_orders_customers_regions(storage) query = SlayerQuery( source_model="orders", time_dimensions=[TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH)], @@ -331,11 +305,7 @@ async def test_orderby_joined_column_rejected_in_first_last_ranked_scope(self, t 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 self._save_orders_customers_regions(storage) - # add a time column so first/last has a time context - orders.columns.append(Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP)) - orders.default_time_dimension = "created_at" - await storage.save_model(orders) + orders = await _save_orders_customers_regions(storage) query = SlayerQuery( source_model="orders", measures=[{"formula": "amount:last(created_at)", "name": "latest"}], # ranked-subquery wrap @@ -406,35 +376,7 @@ async def test_joined_orderby_in_cte_wrapped_scope_should_resolve(self, tmp_path """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)) - 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", - 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) + orders = await _save_orders_customers_regions(storage) query = SlayerQuery( source_model="orders", time_dimensions=[TimeDimension(dimension=ColumnRef(name="created_at"), granularity=TimeGranularity.MONTH)],