diff --git a/DECISIONS.md b/DECISIONS.md index ede699b4..d91ff5ff 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -70,4 +70,5 @@ implementation detail. Include issue refs when known. - 2026-08-04 — Dialect-aware / complete escaping for Mode-A `{variable}` substitution (DEV-1727), hardening DEV-1625. `substitute_variables(..., escape="sql")` is now **dialect-aware** and **fail-closed**: it gained a required keyword-only `backslash_escapes` signal (`bool | None`, raises if `None` in sql mode) so a caller rendering raw SQL can never silently under-escape. On backslash-escaping dialects (MySQL/ClickHouse/Snowflake/Redshift/BigQuery/Databricks/Spark) it doubles the backslash before escaping the single quote; on standard dialects it keeps the `''` quote-doubling. The double quote is deliberately left untouched — inside a single-quoted literal `\"` is NOT a recognised escape on 6 of the 7 backslash dialects (only MySQL), so escaping it would corrupt the value. The regime is DERIVED from sqlglot's own tokenizer via `SqlDialect.backslash_escapes_strings` (= `"\\" in tokenizer.STRING_ESCAPES`, guarded + 14-dialect pinned) so our escaping can never drift from the parser that reads the substituted SQL. `escape="python"` (Mode-B) additionally encodes the full C0 control range (`\t`/`\n`/`\r` named, rest `\xNN`) so raw newlines/NUL no longer break `ast.parse`. Engine fail-closed: `_substitute_model_sql_surfaces` / `_render_probe_model` require a `dialect`, threaded from the resolved datasource — no bare bool to forget. Assumes MySQL's default `sql_mode` (backslash escapes on); `NO_BACKSLASH_ESCAPES` servers are a sqlglot-layer-wide limitation, documented not fixed. The SQLite backslash end-to-end gap stays a pinned strict-xfail (pre-existing, out of scope). Bound parameters rejected (don't fit substitute-into-raw-SQL). Nested/join/cross-model lineages remain DEV-1678. - 2026-08-04 — Declared list-valued `{variable}` coercion (DEV-1730 follow-up): a scalar supplied for a variable the model declares `list_valued` is wrapped into a one-element list before Mode-A substitution, so an importer-generated `col IN ({var})` renders `IN ('US')` rather than the unquoted `IN (US)`. The generic scalar rule (author writes the quotes, so `{var}` also works in numeric/fragment positions like `amount >= {floor}` and `{d}::TIMESTAMP`) is CORRECT and unchanged — it just presumes an author who can see the SQL position, which a machine-generated fixed template does not have; the caller cannot supply per-element quotes through parentheses the importer wrote. Silent-wrong-answer risk drove the fix over a raise: `region IN (US)` parses as a column reference, so it fails at the database with a confusing message, or resolves against a real column and returns wrong rows. Opt-in is a front-end-NEUTRAL flag: the Cube converter writes `list_valued: ref.kind == "string"` into each `meta.cube_variables` entry (arrow forms splice pre-quoted scalars and stay `False`), and the engine reads only that flag — never Cube's `kind` taxonomy — so a future list-shaped front-end opts in the same way. Coercion lives at the single Mode-A choke point `_substitute_model_sql_surfaces` (execution and the `_render_probe_model` type-probe both route through it, so it cannot be bypassed) via `coerce_declared_list_variables` / `list_valued_variable_names` in `slayer/core/query.py`. Scope is deliberately narrow: only `str`/`int`/`float`/`bool` are wrapped; `list`/`tuple` pass through (the **empty list still raises** — "no filter" belongs to an optional block or a sentinel default); `None`/`dict` are left for `_render_variable_value` to reject with its own naming error; hand-written models declare nothing and are untouched. Follow-on from the same review: `declares_variables(model)` (any non-empty `meta.cube_variables`) now also defeats the DEV-1625 zero-variable fast path, via the shared `_model_needs_substitution_pass` predicate used by both `_substitute_model_sql_surfaces` and `_render_probe_model`. This closes the fast-path hole for a GENERATED model whose pushdowns are all required (no `{? ?}` block to force the pass): such a model used to emit a bare `{var}` into the SQL on a zero-variable call instead of raising the documented missing-variable error. The hole stays open — deliberately — for hand-written models, which declare nothing and keep the raw-brace-literal protection (`'{1,2,3}'`). The `list_valued` flag is matched with `is True`, not truthiness, since `meta` is user-extensible and a stray `1` or the string `"false"` must not switch substitution semantics. The bag is also SELF-IDENTIFYING — an entry counts only with a string `member` (the shape every importer writes) — so a hand-written `meta` that reuses the `cube_variables` key is not mistaken for generated SQL and silently stripped of its brace-literal protection. - 2026-08-05 — Ingestion sees views, survives unmodellable names, and stops being silent (DEV-1741). **Views**: `list_ingestable_objects` replaces the bare `get_table_names()` at every introspection site, adding `get_view_names` + `get_materialized_view_names` behind a `NotImplementedError`/`Exception` guard (the base `Inspector` RAISES for matviews on unsupporting dialects rather than returning `[]`), de-duplicated first-classification-wins because some dialects return views from `get_table_names()`, in a deterministic tables→views→matviews order that the name-collision policy depends on. Ingested by default — dbt materializes staging models as views, so opt-in would have left the reported failure in place for a fresh install — with `--no-views` on both `slayer ingest` and `datasources create --ingest`. The drift side (`_live_schema_for_datasource`) and the MCP listing (`_fetch_tables`) take **no flag and are unconditional**: that map is only ever a lookup target (`validate_datasource` iterates the *persisted* models, `available_in_ds` derives from them), so views there cannot manufacture a model or a drift entry — what they fix is a pre-existing **data-loss** bug where a hand-authored model whose `sql_table` named a view resolved to `live_table=None` → `WholeModelDelete` → deleted by `validate-models --force-clean`. Gating that on `--no-views` would re-arm it for exactly the users who opted out. **Names**: model names can't contain `__` (six modules — generator/enrichment/column_expansion/column_dependency/schema_drift/osi — `split("__")` an alias back into a join path, so a model named `a__b` is read as the alias for `a→b` and yields a silently wrong query, not a crash), but `sql_table` can, so a dlt child table `reports__patient__drug` is modelled as `reports_patient_drug` with `sql_table` verbatim. Sanitizer is `re.sub(r"_{2,}", "_")`, NOT `replace("__","_")` — `str.replace` is non-overlapping, so `a___b`→`a__b` would still fail validation. Collisions reserve every unsanitized name first (a real `a_b` always beats a sanitized `a__b`, order-independently) and **skip** rather than suffix, since suffixes shift as the object set changes and would orphan models and churn drift. A per-object `try/except` backstops everything else (`.`/`:`/`/`/`\` names, bad column names, per-object introspection failures); the FK-collection loop and `_get_fk_relationships` are guarded too because they run BEFORE that isolation and would otherwise still kill the run. **Reporting**: skips travel in a new `skipped` list, deliberately NOT folded into `errors` — `slayer ingest` exits 1 on either (we declined a perfectly valid object; `--exclude` is the documented remedy) but `POST /ingest` keeps 422 for `errors` only, because a permanent 422 aimed at a machine that can't act on the hint buries a successful partial ingest behind an error status. An empty scan prints the available schemas and exits 1 (the reporter listed the missing exit code as part of the defect), gated on `objects` not `additions` so a healthy no-op re-ingest stays quiet; `datasources create --ingest` on an empty DB still exits 0, since creating the datasource is that command's job and it succeeded. `in_scope_table_names` switched from model names to `_bare_table_name(sql_table)` — it is compared against table names in `_scoped_models_for_validation`, so the old keying silently dropped from validation scope any model whose name differs from its table (every sanitized model, and already every dbt/OSI hidden model passing `model_name=`). **`source_kind`** (`table`/`view`/`materialized_view`/`None`=unknown) persists on `SlayerModel`, v7→v8 with a no-op migration (mandatory: `migrate()` raises `RuntimeError` on an unregistered step); `None` for pre-v8, hand-authored and sql/query-backed models is the honest value, not a guess. It is a deliberate **exception to the additive-merge contract** — refreshed, not preserved — because it describes the live object rather than user intent, and the transition it exists to capture (dbt `+materialized: table`) usually changes no columns at all; the refresh therefore has to be made in three places (the early return, the `model_copy(update=...)`, and the save gate in `_process_one_table`), since editing only the update dict computes a corrected model and throws it away. A `None` from a non-classifying path never erases a known value. Docs-only fix for the advertised `motley-slayer[duckdb]` extra, which does not exist: `duckdb`/`duckdb-engine` are unconditional core deps (the Postgres facade imports duckdb at top level on every CLI invocation), and adding them under `[tool.poetry.extras]` would *gate* rather than alias them, breaking bare `pip install motley-slayer` → `datasources create demo`. +- 2026-08-06 — Dialect-aware identifier-length fitting (DEV-1756): every dialect declares a conservative universal budget as `SqlDialect.max_identifier_bytes` (postgres 63, mysql 64, redshift 127, oracle/tsql 128, snowflake 255, duckdb 256, bigquery 300, `None` = unbounded for sqlite/clickhouse/trino/presto/databricks/spark), and an over-limit identifier is shortened at emission to `__` via `slayer/sql/dialects/_identifier_fit.py`. Postgres is the binding case and the reason this is a correctness fix rather than cosmetics: it truncates over-length identifiers **silently** (a NOTICE, never an error), so two 3-hop aliases sharing a 63-byte prefix either blow up as `AmbiguousColumnError` under the DEV-1444 outer wrap or — with no sibling to collide with — quietly return the column under a name the engine never looks up. Shorten **only when over the limit**, never uniformly: `decode_result_keys` restores the canonical dotted alias, so the dialect-dependence the issue worried about is invisible to consumers and the 99% case keeps byte-identical SQL (pinned by pre-change goldens, not merely by idempotence). Both ends of the alias are kept because the reported colliding pair differs ONLY in its final segment — a head-only truncation would render the two indistinguishable in `dry_run` output. The digest is sha256 of the FULL original, which is what makes `fit_identifier` a pure function of the name and lets the read side rebuild the emitted→canonical map by re-running it, with no map threaded through generation (the alternative the issue sketched). Write side is an EXACT-match replacement over the query's own alias set (`all_projection_aliases`, unfiltered — hidden ORDER-BY hoists and `_inner_*`/`_ft*`/`_ts*` entries are projected in the inner SELECT and truncate identically), never a length regex over arbitrary SQL, so a long quoted-looking span inside a string literal can never be corrupted; substitution is two-phase (canonical→sentinel→final) as defence-in-depth, though today's key set (over-limit) and value set (within-limit) are provably disjoint. BigQuery/T-SQL compose by running their existing dot-mangle regex AFTER the base length pass: an under-limit alias makes the length pass a genuine no-op so their output is unchanged, and an over-limit one arrives still-dotted and gets mangled by the same regex — no double-encoding — with the budget sized against `encode_alias` so the post-mangle form still fits. Scope is the three OUTPUT-name surfaces: projection aliases, CTE names (`_cte_name_from_alias` fits the whole result, prefix included, allocated through a per-statement collision-checked `SQLGenerator._cte_name`), and `_query_as_model` virtual-model shorts. Join-path TABLE aliases are deferred to DEV-1743, whose plan already owns the `__` path-alias allocator; their failure mode (silent wrong joins) is worse but the fix requires decoupling `EnrichedDimension.model_name` from the emitted qualifier across ~8 sites that split it back on `__`. Collisions raise `IdentifierCollisionError` rather than emitting ambiguous SQL — the check covers identity entries too, since an already-short alias equal to another's fitted form is a duplicate no hash width can prevent. Fixed alongside, in the same code path: `_query_as_model` no longer decides its short alias's quoting with a hand-written predicate. The contract is AGREEMENT between the emitted `AS ` and the downstream `Column(sql=short)` reference — they must resolve to the same column — so the short is now run through the same two mechanisms the reference side uses: `SQLGenerator._maybe_quote_ident` (DEV-1645: quote iff the name contains an uppercase letter) followed by `Identifier.sql(dialect=...)`, which lets sqlglot add quotes for words reserved IN THE TARGET DIALECT and for names that are not safe bare identifiers. Emitted bare, a mixed-case short was case-folded by Postgres while the outer stage referenced it quoted, making any query-backed model with a mixed-case join path unqueryable (`UndefinedColumnError`, reproduced on a live server). Two wrong answers were tried and rejected on the way: quoting UNCONDITIONALLY defines a case-sensitive `"status"` on upper-folding backends (Snowflake, Oracle) while the bare reference still resolves as `STATUS`, trading a Postgres bug for a Snowflake one; and quoting on `SLAYER_RESERVED_KEYWORDS` alone misses words reserved only NATIVELY (`index`, `int`, `rows` on MySQL; `rows` on BigQuery), where the reference side quotes but a bare `AS index` is a syntax error — `install_reserved_keywords` unions our set INTO each generator's, so only the generator knows the full set. Shape matters too: `Column.name` forbids only `.` and `:`, so `1abc` or `foo bar` needs quoting on form alone. Both axes must therefore be dialect-driven, and the rule is pinned by a 12-dialect × 7-name test asserting the two spellings are byte-identical. Because an all-lowercase short stays bare and still shares a case-folded namespace, the short-uniqueness check remains case-insensitive. - 2026-08-06 — `mcp` capped at `>=1.0,<2` (DEV-1757). mcp 2.0.0 renamed `mcp.server.fastmcp` → `mcp.server.mcpserver` (`FastMCP` → `MCPServer`), so the unbounded `mcp = ">=1.0"` let every lockfile-free install (`pip install motley-slayer`, `uv tool install`) resolve a major `slayer/mcp/server.py` cannot import — a broken MCP server, SLayer's primary agent-facing interface, on every fresh install. `poetry.lock` pinned a 1.x, so CI was green throughout and only users saw it; the guard added here (`tests/test_mcp_dependency_pin.py`) is therefore **declaration-level** — it parses the pyproject constraint and asserts 2.0.0 falls outside it, since no in-repo test can execute an mcp major the environment does not have. Two consequences accepted deliberately: users needing mcp≥2 for another package in the same environment now hit a resolver conflict instead of a runtime crash, and other core deps were left unbounded — capping the rest was considered and rejected (caps rot and produce unresolvable trees downstream), which is the scope of this change and NOT a standing no-caps policy. The import failure now diagnoses itself: absent package, wrong major, and a 1.x that failed to import for some other reason are three distinct messages, all offering `pip install 'mcp>=1.0,<2'` first and "upgrade SLayer" second — the old "Reinstall SLayer: pip install motley-slayer" text told users to do the one thing that reproduced the failure, and leading with the upgrade hint would recreate that for anyone already on the latest release. Separately, `serverInfo.version` now reports SLayer's own version: FastMCP 1.x exposes no `version` kwarg and never forwards one to the lowlevel `Server`, which falls back to `pkg_version("mcp")`, so SLayer was announcing the SDK's version as its own. The stamp writes the private `_mcp_server.version` (the only route in 1.x; the file already sets `_slayer_engine` on the same object per DEV-1656) and tolerates both a missing attribute and a read-only one, so a future SDK cannot abort server construction over a cosmetic field. Migrating to the 2.x `MCPServer` API — which would retire that private write via its public `version=` kwarg, and also pulls in `Context` injection, worker-thread sync handlers, snake_case `mcp.types`, and an httpx→httpx2 / pydantic≥2.12 / opentelemetry dependency shift — is deferred, as is removing the deprecated `inspect_model` tool. diff --git a/docs/concepts/queries.md b/docs/concepts/queries.md index 055ea85c..c6b5e1e7 100644 --- a/docs/concepts/queries.md +++ b/docs/concepts/queries.md @@ -131,6 +131,14 @@ Query results are returned as a `SlayerResponse`: } ``` +Result keys are always this canonical dotted form, on every backend. Where a +database's own rules force a different spelling in the emitted SQL — BigQuery +and SQL Server reject dotted column aliases, and most engines cap identifier +length — the alias is rewritten on the way out and restored on the way back, so +`data` and `columns` do not vary by dialect. Only `sql` shows the rewritten +form, since that is what actually ran. See +[Database support](../database-support.md#identifier-length-limits). + --- ## Filters diff --git a/docs/database-support.md b/docs/database-support.md index fa652472..f97b711c 100644 --- a/docs/database-support.md +++ b/docs/database-support.md @@ -35,6 +35,16 @@ Unit tests for SQL generation; no live-instance verification. Redshift, Trino/Presto (Athena uses the Presto dialect), Databricks/Spark, Oracle. +## Identifier length limits + +Some databases cap identifier length (Postgres is the tightest at 63 bytes, and +truncates over-limit names *silently*). SLayer's join-path aliases like +`orders.customers.regions.name` can exceed that on a deep chain, so any +over-limit alias is trimmed at emission to `__`. This does +**not** affect output column names — `response.data` / `response.columns` keep +the canonical dotted alias — but `response.sql` (and `dry_run` / `EXPLAIN`) +shows the trimmed form, so account for it if you introspect the SQL. + ## Aggregation support Most aggregations (`sum`, `avg`, `min`, `max`, `count`, `count_distinct`, diff --git a/slayer/core/errors.py b/slayer/core/errors.py index c02f38c1..26f63fef 100644 --- a/slayer/core/errors.py +++ b/slayer/core/errors.py @@ -151,6 +151,38 @@ def __init__( ) +class IdentifierCollisionError(SlayerError, ValueError): + """Two distinct SLayer-generated names collapse onto one identifier after + the dialect's length fitting (DEV-1756). + + Raised loudly rather than left to corrupt a result set. Also covers an + already-short name equal to another name's fitted form. + """ + + def __init__( + self, + *, + first: str, + second: str, + emitted: str, + dialect: str, + limit: int | None, + namespace: str = "identifier", + ) -> None: + self.first = first + self.second = second + self.emitted = emitted + self.dialect = dialect + self.limit = limit + self.namespace = namespace + super().__init__( + f"{namespace} collision on dialect '{dialect}' " + f"(max_identifier_bytes={limit}): {first!r} and {second!r} both " + f"emit as {emitted!r}. Rename one of the underlying models or " + f"columns to break the tie." + ) + + class ForcedFilterError(SlayerError): """Raised when the session policy's ruleset cannot be safely applied to a query. diff --git a/slayer/engine/enriched.py b/slayer/engine/enriched.py index a6ef17c1..16a1c3d7 100644 --- a/slayer/engine/enriched.py +++ b/slayer/engine/enriched.py @@ -274,6 +274,23 @@ class CrossModelMeasure(BaseModel): CrossModelMeasure.model_rebuild() +def all_projection_aliases(enriched: EnrichedQuery) -> list[str]: + """Every alias ``enriched`` can put into an emitted SELECT, in bucket order. + + Superset of :func:`public_projection_aliases` WITHOUT internal-prefix + filtering: hidden entries (``_inner_*``/``_ft*``/``_ts*`` hoists, ORDER-BY + aggregates) are still projected and must be length-fitted with the same map. + Deduplicated, first-seen order, so the derived rewrite map is deterministic. + """ + out: list[str] = [d.alias for d in enriched.dimensions] + out.extend(td.alias for td in enriched.time_dimensions) + out.extend(m.alias for m in enriched.measures) + out.extend(e.alias for e in enriched.expressions) + out.extend(t.alias for t in enriched.transforms) + out.extend(cm.alias for cm in enriched.cross_model_measures) + return list(dict.fromkeys(out)) + + def public_projection_aliases(enriched: EnrichedQuery) -> list[str]: """Return the ordered list of public-projection aliases for ``enriched``. diff --git a/slayer/engine/query_engine.py b/slayer/engine/query_engine.py index e22d7c17..9c5866e4 100644 --- a/slayer/engine/query_engine.py +++ b/slayer/engine/query_engine.py @@ -20,7 +20,11 @@ from sqlglot import exp from slayer.core.enums import DEFAULT_AGGREGATIONS_BY_TYPE, DataType -from slayer.core.errors import AmbiguousModelError, ForcedFilterError +from slayer.core.errors import ( + AmbiguousModelError, + ForcedFilterError, + IdentifierCollisionError, +) from slayer.core.policy import JoinFilterRuleset, SessionPolicy from slayer.core.format import NumberFormat, NumberFormatType, format_number from slayer.core.models import ( @@ -61,6 +65,7 @@ CrossModelMeasure, EnrichedMeasure, EnrichedQuery, + all_projection_aliases, public_projection_aliases, ) from slayer.engine.enrichment import enrich_query @@ -72,11 +77,11 @@ ) from slayer.sql.client import SlayerSQLClient from slayer.sql.dialects import SqlDialect, dialect_for_ds_type, get_dialect +from slayer.sql.dialects._identifier_fit import fit_identifier from slayer.sql import engine_factory from slayer.sql.engine_factory import EngineCacheKey from slayer.sql.engine_factory import _cache_key as _engine_cache_key from slayer.sql.generator import SQLGenerator -from slayer.sql.reserved_keywords import SLAYER_RESERVED_KEYWORDS from slayer.sql.session_policy import ScopedTable, apply_session_policy from slayer.storage.base import StorageBackend @@ -1386,10 +1391,11 @@ async def _run_and_build( ) raise timing.record("execute", _t) - # Dialect-driven read-side decode: BigQuery reverses its alias - # mangling here so the response keys match SLayer's universal - # dotted shape. Default hook is identity for every other dialect. - rows = get_dialect(prepared.dialect).decode_result_keys(rows) + # Reverse dialect alias fitting/mangling so response keys are canonical. + # Pass the unfiltered alias set: a row may carry a hidden ORDER-BY hoist. + rows = get_dialect(prepared.dialect).decode_result_keys( + rows, aliases=all_projection_aliases(prepared.enriched), + ) columns = prepared.expected_columns if not rows else [] # [] auto-derives return SlayerResponse( data=rows, @@ -2043,6 +2049,12 @@ async def get_column_types( logger.warning("get_column_types probe failed for model '%s'", model_name) return {} + # The probe's metadata keys are the emitted (fitted) names; decode them + # to canonical or an over-limit measure drops out of the type map. + raw_types = get_dialect(dialect).decode_result_keys( + [raw_types], aliases=all_projection_aliases(enriched), + )[0] + # Map qualified aliases (e.g., "orders.revenue_max") back to bare measure names result: dict[str, str] = {} for em in enriched.measures: @@ -3211,19 +3223,23 @@ async def _query_as_model( # NOSONAR S3776 — variable-precedence + enrich + S # from the alias by stripping the source model prefix and replacing # dots with underscores. def _alias_to_short(alias: str) -> str: - """Convert result alias to a flat column name for the virtual model. + """Flatten a result alias to the virtual model's column name, then fit it. - The query result is a self-contained table without the joins the - source model may have had, so dot syntax (join paths) is not - applicable. We use ``__`` to preserve the path information: - - 'orders.customers.regions.name' → 'customers__regions__name' - 'orders.count' → 'count' + The result is a self-contained table without joins, so join-path + dots become ``__``: 'orders.customers.regions.name' → 'customers__regions__name'. """ - # Strip source model prefix stripped = alias.split(".", 1)[-1] if "." in alias else alias - # Replace remaining dots with __ to encode the original join path - return stripped.replace(".", "__") + return _fit_short(stripped.replace(".", "__")) + + def _fit_short(name: str) -> str: + """Fit a virtual-model short to the dialect's budget; identity under the limit. + + Every short is both an output alias and a downstream ``Column.name``, + and caller-supplied names can be arbitrarily long. + """ + return fit_identifier( + name, limit=get_dialect(dialect).max_identifier_bytes, + ) # (inner_alias, short_name, data_type, label, description, format) column_map = [] @@ -3246,14 +3262,14 @@ def _alias_to_short(alias: str) -> str: measure_name=src_name, aggregation=m.aggregation, ) - column_map.append((m.alias, m.name, DataType.DOUBLE, label, desc, fmt)) + column_map.append((m.alias, _fit_short(m.name), DataType.DOUBLE, label, desc, fmt)) for t in enriched.transforms: column_map.append( - (t.alias, t.name, DataType.DOUBLE, t.label, None, NumberFormat(type=NumberFormatType.FLOAT)) + (t.alias, _fit_short(t.name), DataType.DOUBLE, t.label, None, NumberFormat(type=NumberFormatType.FLOAT)) ) for e in enriched.expressions: column_map.append( - (e.alias, e.name, DataType.DOUBLE, e.label, None, NumberFormat(type=NumberFormatType.FLOAT)) + (e.alias, _fit_short(e.name), DataType.DOUBLE, e.label, None, NumberFormat(type=NumberFormatType.FLOAT)) ) for cm in enriched.cross_model_measures: # DEV-1448: when the user supplied an explicit ``name``, cm.name is @@ -3272,37 +3288,51 @@ def _alias_to_short(alias: str) -> str: # leak into the virtual model's column set. Only user-declared # renames qualify for the bare-name short. if cm.user_declared and cm.name and "." not in cm.name: - short = cm.name + short = _fit_short(cm.name) # still fitted: deterministic + collision-checked else: short = _alias_to_short(cm.alias) column_map.append((cm.alias, short, DataType.DOUBLE, cm.label, None, cm.format)) - # Wrap inner SQL: SELECT AS , ... FROM (inner) AS _inner - # DEV-1571 Bug 3 follow-up: identifier quoting must match the - # dialect ``inner_sql`` was generated for. On MySQL the inner CTEs - # use backticks; on T-SQL the inner CTEs use brackets AND have - # their dotted aliases mangled. Hardcoded ANSI double quotes - # would either fail to parse (MySQL) or reference an alias the - # mangled inner subquery doesn't expose (T-SQL). - # DEV-1686: the inner ``alias`` is always dialect-quoted; the ``short`` - # output alias must also be quoted when it is a reserved word (a - # user-declared cross-model rename like ``order``, or an - # ``_alias_to_short`` that yields one), else ``AS order`` is bare and - # the wrapped SQL fails to parse/execute. + # Wrap inner SQL: SELECT AS , ... FROM (inner) AS _inner. + # The ``short`` output alias must be spelled EXACTLY as the downstream + # reference will be, so both resolve to the same column. Run it through the + # two mechanisms the reference side uses: ``_maybe_quote_ident`` (DEV-1645: + # quote iff mixed-case) and ``Identifier.sql`` (dialect-reserved words / + # unsafe shapes). Both dialect-driven — unconditional quoting breaks bare + # refs on upper-folding backends; a fixed keyword set misses native words + # like MySQL's ``index``. def _short_sql(short: str) -> str: - if short.lower() in SLAYER_RESERVED_KEYWORDS: - return exp.Identifier(this=short, quoted=True).sql(dialect=dialect) - return short + ident = exp.Identifier(this=short, quoted=False) + SQLGenerator._maybe_quote_ident(ident) + return ident.sql(dialect=dialect) + + # Validate the whole short allocation: two shorts colliding on one output + # name silently drop a column. Keyed CASEFOLDED — conservative, since a + # mixed-case short is quoted (case-preserved) and would not really collide + # with its folded twin, but SLayer carries no per-dialect fold direction, so + # a loud error beats a silent duplicate on the backends this protects. + short_owner: dict[str, str] = {} + for alias, short, _, _, _, _ in column_map: + prior_alias = short_owner.setdefault(short.casefold(), alias) + if prior_alias != alias: + raise IdentifierCollisionError( + first=prior_alias, second=alias, emitted=short, + dialect=dialect, + limit=get_dialect(dialect).max_identifier_bytes, + namespace="query-backed model column", + ) rename_parts = [ f'{exp.Identifier(this=alias, quoted=True).sql(dialect=dialect)} AS {_short_sql(short)}' for alias, short, _, _, _, _ in column_map ] wrapped_sql = f"SELECT {', '.join(rename_parts)} FROM ({inner_sql}) AS _inner" - # DEV-1571 Bug 2: apply the dialect's emitted-SQL rewrite (e.g. - # T-SQL bracket-mangling) so the rename clause's inner-alias - # references match what the inner subquery actually projects. - wrapped_sql = get_dialect(dialect).rewrite_emitted_sql(wrapped_sql) + # Apply the dialect's emitted-SQL rewrite (T-SQL bracket-mangling, DEV-1756 + # length-fitting) so the wrapper's inner-alias references match the fitted + # names the inner subquery projects. Same alias set the inner SQL used. + wrapped_sql = get_dialect(dialect).rewrite_emitted_sql( + sql=wrapped_sql, aliases=all_projection_aliases(enriched), + ) # One Column per result column — each is potentially both a dimension # (group-by) or measure (with colon-aggregation) at query time. @@ -3329,7 +3359,9 @@ def _short_sql(short: str) -> str: agg_shorts.add(_alias_to_short(cm.alias)) for m in enriched.measures: if m.from_cross_model_intercept: - agg_shorts.add(m.name) + # Same fitting the column_map entry got, else the breadcrumb + # names a column the virtual model never has. + agg_shorts.add(_fit_short(m.name)) # DEV-1449: record the lineage breadcrumb so outer-stage dotted-ref # lookup can strip the right ancestor prefix and find the flat diff --git a/slayer/sql/dialects/_identifier_fit.py b/slayer/sql/dialects/_identifier_fit.py new file mode 100644 index 00000000..af8d0dd9 --- /dev/null +++ b/slayer/sql/dialects/_identifier_fit.py @@ -0,0 +1,114 @@ +"""DEV-1756: shared identifier-length fitting + the write-side substitution. + +Postgres SILENTLY truncates identifiers past 63 bytes (a NOTICE, never an +error), so SLayer's ``..`` aliases can collapse two +siblings onto one output name on a deep join. + +:func:`fit_identifier` shortens an over-limit name to ``__``. +It is a PURE function of ``name`` (the digest covers the full original), so the +read side rebuilds the emitted->canonical map by re-running it — no map threaded +through generation. :func:`substitute_quoted` applies that map to emitted SQL. +BigQuery/T-SQL size the budget against their post-mangle form via ``expand``. +""" + +from __future__ import annotations + +import hashlib +from collections.abc import Callable, Mapping + + +HASH_LEN = 8 # digest hex chars; collisions are caught per-namespace, not by width +MIN_LIMIT = 16 # floor so a mis-configured limit fails loudly, not silently +_MARKER_LEN = HASH_LEN + 2 # ``_`` + digest + ``_`` +_TRIM = "._" # trimmed off head/tail so the marker never abuts a separator +#: NUL bytes never appear in generated SQL, so this sentinel can't clash. +_SENTINEL = "\x00\x01{}\x01\x00" + + +def _digest(name: str) -> str: + """Stable digest of the full original name. + + ``sha256``, not the builtin ``hash`` (salted by ``PYTHONHASHSEED`` and the + read side recomputes in another process). Patched by tests to force collisions. + """ + return hashlib.sha256(name.encode("utf-8")).hexdigest()[:HASH_LEN] + + +def _head_bytes(name: str, n: int) -> str: + """Leading ``n`` bytes of ``name``, cut on a UTF-8 codepoint boundary.""" + if n <= 0: + return "" + return name.encode("utf-8")[:n].decode("utf-8", "ignore") + + +def _tail_bytes(name: str, n: int) -> str: + """Trailing ``n`` bytes of ``name``, cut on a UTF-8 codepoint boundary.""" + if n <= 0: + return "" + return name.encode("utf-8")[-n:].decode("utf-8", "ignore") + + +def fit_identifier( + name: str, + *, + limit: int | None, + expand: Callable[[str], str] | None = None, +) -> str: + """Shorten ``name`` to at most ``limit`` **bytes** as ``__``. + + Identity when ``limit`` is ``None`` or the name already fits, so common-case + SQL is byte-identical. Both ends are kept: head names the root model, tail + the column, and the repro's colliding aliases differ only in their tail. + + ``expand`` sizes the budget against a post-fit transform (BigQuery/T-SQL + ``encode_alias`` adds 2 bytes per dot); the return value itself is NOT + expanded. Residual collisions are caught by the caller's allocation check. + """ + grow = expand or (lambda s: s) + if limit is None or len(grow(name).encode("utf-8")) <= limit: + return name + if limit < MIN_LIMIT: + raise ValueError( + f"identifier limit must be at least MIN_LIMIT ({MIN_LIMIT}) bytes to " + f"leave room for the {_MARKER_LEN}-byte hash marker plus context; got {limit}" + ) + marker = f"_{_digest(name)}_" + # Shrink the budget until the (expanded) candidate fits. The last iteration + # leaves head/tail empty -> bare ``__``, legal even unquoted. + for budget in range(limit, _MARKER_LEN - 1, -1): + avail = budget - _MARKER_LEN + tail_n = avail // 2 + head_n = avail - tail_n + head = _head_bytes(name, head_n).rstrip(_TRIM) + tail = _tail_bytes(name, tail_n).lstrip(_TRIM) + candidate = f"{head}{marker}{tail}" + if len(grow(candidate).encode("utf-8")) <= limit: + return candidate + raise ValueError( + f"cannot fit {name!r} into {limit} bytes: the supplied `expand` grows " + f"even the bare {_MARKER_LEN}-byte marker beyond the limit" + ) + + +def substitute_quoted( + sql: str, + mapping: Mapping[str, str], + *, + quote: Callable[[str], str], +) -> str: + """Replace each quoted ``canonical`` identifier token with its ``emitted`` form. + + Two-phase (canonical -> sentinel -> emitted) so one substitution can't be + re-read by a later one. Only QUOTED occurrences move; a bare occurrence is a + different identifier (e.g. a table alias) and is left alone. String pass, not + literal-aware, but keyed to an exact alias set — no wider exposure than the + dot-mangling regexes already run over this SQL. + """ + if not mapping: + return sql + items = sorted(mapping.items()) + for index, (canonical, _) in enumerate(items): + sql = sql.replace(quote(canonical), _SENTINEL.format(index)) + for index, (_, emitted) in enumerate(items): + sql = sql.replace(_SENTINEL.format(index), quote(emitted)) + return sql diff --git a/slayer/sql/dialects/_tier2.py b/slayer/sql/dialects/_tier2.py index c5014e69..a90b5c54 100644 --- a/slayer/sql/dialects/_tier2.py +++ b/slayer/sql/dialects/_tier2.py @@ -34,6 +34,7 @@ class RedshiftDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = False + max_identifier_bytes: int | None = 127 def build_approx_count_distinct( self, @@ -52,6 +53,7 @@ class TrinoDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = None # unbounded def build_approx_count_distinct( self, @@ -71,6 +73,7 @@ class PrestoDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = None # unbounded def build_approx_count_distinct( self, @@ -89,6 +92,7 @@ class DatabricksDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = None # unbounded def build_approx_count_distinct( self, @@ -107,6 +111,7 @@ class SparkDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = None # unbounded def build_approx_count_distinct( self, @@ -127,6 +132,7 @@ class OracleDialect(SqlDialect): # the canonical 2-arg LOG(base, x) form. log10_native: bool = False log2_native: bool = False + max_identifier_bytes: int | None = 128 # 12.2+; pre-12.2 (30) not modelled def build_approx_count_distinct( self, diff --git a/slayer/sql/dialects/base.py b/slayer/sql/dialects/base.py index 9cda160a..985440ba 100644 --- a/slayer/sql/dialects/base.py +++ b/slayer/sql/dialects/base.py @@ -15,13 +15,15 @@ import hashlib from functools import lru_cache from typing import TYPE_CHECKING, Any -from collections.abc import Callable +from collections.abc import Callable, Sequence from pydantic import BaseModel, ConfigDict from sqlglot import exp from sqlglot.dialects.dialect import Dialect as _SqlglotDialect from slayer.core.enums import TimeGranularity +from slayer.core.errors import IdentifierCollisionError +from slayer.sql.dialects._identifier_fit import fit_identifier, substitute_quoted if TYPE_CHECKING: import sqlalchemy as sa @@ -193,6 +195,11 @@ class SqlDialect(BaseModel): log10_native: bool = True log2_native: bool = True + # Conservative universal identifier budget in BYTES; ``None`` = unbounded + # (fitting hooks become no-ops). Default is the tightest Tier-1 value + # (Postgres), so a new dialect over-shortens rather than silently truncating. + max_identifier_bytes: int | None = 63 + @property def backslash_escapes_strings(self) -> bool: """Whether this dialect's string literals treat a backslash as an escape @@ -501,41 +508,127 @@ def emit_outer_wrap( out += "\n" + offset_arg.sql(dialect=self.sqlglot_name, pretty=True) return out - def rewrite_emitted_sql(self, sql: str) -> str: - """Default: identity. Post-pass string-level rewrite of the final - generator output. + # DEV-1756 identifier-length fitting. Aliases stay canonical inside SLayer, + # fitted only on emission and restored on the result keys. + + def quote_identifier(self, name: str) -> str: + """``name`` wrapped in this dialect's identifier quotes.""" + return exp.Identifier(this=name, quoted=True).sql(dialect=self.sqlglot_name) + + def fit_alias(self, name: str) -> str: + """Length-only fitting; identity when ``name`` already fits. + + Drives the write pass (not ``emit_alias``), so an under-limit alias + produces byte-identical SQL even on dialects that mangle dots. + """ + return fit_identifier(name=name, limit=self.max_identifier_bytes) + + def emit_alias(self, alias: str) -> str: + """The final identifier a canonical alias reaches the SQL as. + + Equals ``fit_alias`` here; BigQuery/T-SQL compose dot-mangling on top. + Used to build the read-side map, so must match the emitted token exactly. + """ + return self.fit_alias(alias) + + def alias_rewrite_map(self, aliases: Sequence[str]) -> dict[str, str]: + """``{canonical: fitted}`` for the write pass, only where they differ. - Symmetric companion to ``rewrite_parsed_ast`` (the input-side - hook): write-side, applied at the end of - ``SQLGenerator.generate()`` AFTER ``_apply_outer_projection_trim``. + The collision check covers every alias including identities: a short + alias equal to another's fitted form is just as much a duplicate. + """ + if self.max_identifier_bytes is None: + return {} + allocation: dict[str, str] = {} + owner: dict[str, str] = {} + for alias in aliases: + if alias in allocation: + continue + fitted = self.fit_alias(alias) + prior = owner.get(fitted) + if prior is not None and prior != alias: + raise IdentifierCollisionError( + first=prior, second=alias, emitted=fitted, + dialect=self.sqlglot_name, limit=self.max_identifier_bytes, + namespace="projection alias", + ) + owner[fitted] = alias + allocation[alias] = fitted + return {k: v for k, v in allocation.items() if k != v} + + def decode_alias_map(self, aliases: Sequence[str]) -> dict[str, str]: + """``{emitted: canonical}`` — read-side inverse, rebuilt by re-running + the pure fitting rather than threading a map through generation.""" + out: dict[str, str] = {} + for alias in aliases: + emitted = self.emit_alias(alias) + if emitted != alias: + out[emitted] = alias + return out - Contract: preserve query semantics. Suitable for alias renames, - identifier mangling/escape, dialect-quoting fixes. Do NOT change - query shape — use the typed ``build_*`` methods on this class for - that. + def _rekey_row( + self, + row: dict[str, Any], + mapping: dict[str, str], + *, + fallback: Callable[[str], str] | None = None, + ) -> dict[str, Any]: + """Apply ``mapping`` to a row's keys, erroring if two keys collapse onto + one (silent column loss). + + ``fallback`` decodes keys absent from ``mapping`` (BigQuery/T-SQL + ``___`` -> ``.``). Applied here, not upstream, so the collapse check + sees every key. + """ + out: dict[str, Any] = {} + for key, value in row.items(): + if key in mapping: + decoded = mapping[key] + elif fallback is not None: + decoded = fallback(key) + else: + decoded = key + if decoded in out: + raise IdentifierCollisionError( + first=key, second=decoded, emitted=decoded, + dialect=self.sqlglot_name, limit=self.max_identifier_bytes, + namespace="result key", + ) + out[decoded] = value + return out - Overrides today: ``BigqueryDialect`` mangles dotted aliases that - would otherwise be rejected by BigQuery's output column-name - grammar. + def rewrite_emitted_sql( + self, sql: str, *, aliases: Sequence[str] = (), + ) -> str: + """Post-pass string rewrite of the final SQL; write-side companion to + ``rewrite_parsed_ast``, applied at the end of ``generate()``. + + Base impl fits over-limit aliases (DEV-1756), replacing each canonical + token everywhere it occurs. Driven by the query's own alias set, not a + length regex, which bounds what it can touch (see :func:`substitute_quoted`). + Empty ``aliases`` is a no-op. Must preserve query semantics, not shape. + BigQuery/T-SQL compose dot-mangling after this pass. """ - return sql + mapping = self.alias_rewrite_map(aliases) + if not mapping: + return sql + return substitute_quoted(sql=sql, mapping=mapping, quote=self.quote_identifier) def decode_result_keys( self, rows: list[dict[str, Any]], + *, + aliases: Sequence[str] = (), ) -> list[dict[str, Any]]: - """Default: identity. Reverse-pass on result-row keys to undo any - write-side mangling applied by ``rewrite_emitted_sql``. - - Called at the end of ``SlayerQueryEngine.execute()`` so consumers - always see SLayer's universal alias shape (``orders._count``, - ``orders.products.category``) regardless of which dialect a - query ran on. + """Reverse the write-side rewrite on result-row keys, so consumers always + see SLayer's canonical alias shape regardless of dialect or shortening. - Overrides today: ``BigqueryDialect`` decodes the ``___`` mangling - back to dots. + BigQuery/T-SQL additionally decode the ``___`` mangling back to dots. """ - return rows + mapping = self.decode_alias_map(aliases) + if not mapping: + return rows + return [self._rekey_row(row=row, mapping=mapping) for row in rows] def register_udfs(self, dbapi_connection) -> None: """Default: no-op. SQLite overrides to register Python aggregate diff --git a/slayer/sql/dialects/bigquery.py b/slayer/sql/dialects/bigquery.py index fd895c6a..7c9ecd24 100644 --- a/slayer/sql/dialects/bigquery.py +++ b/slayer/sql/dialects/bigquery.py @@ -28,13 +28,14 @@ import json import re from typing import TYPE_CHECKING, Any -from collections.abc import Callable +from collections.abc import Callable, Sequence import sqlalchemy as sa from sqlglot import exp from slayer.core.enums import TimeGranularity from slayer.sql.dialects._alias_mangle import decode_alias, encode_alias +from slayer.sql.dialects._identifier_fit import fit_identifier from slayer.sql.dialects.base import SqlDialect, _digest if TYPE_CHECKING: @@ -140,6 +141,7 @@ class BigqueryDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = 300 # column-name limit def build_approx_count_distinct( self, @@ -182,15 +184,28 @@ def build_date_trunc( this="DATE_TRUNC", expressions=[col_expr, week_sunday], ) - def rewrite_emitted_sql(self, sql: str) -> str: - """Replace ``.`` with ``___`` inside backtick-quoted identifiers. + def fit_alias(self, name: str) -> str: + """Size the budget against the post-mangle form (``.`` -> ``___`` adds 2 + bytes per dot); return value stays dotted for the regex below.""" + return fit_identifier( + name=name, limit=self.max_identifier_bytes, expand=encode_alias, + ) + + def emit_alias(self, alias: str) -> str: + """The final identifier: length-fitted, then dot-mangled.""" + return encode_alias(self.fit_alias(alias)) + + def rewrite_emitted_sql( + self, sql: str, *, aliases: Sequence[str] = (), + ) -> str: + """Replace ``.`` with ``___`` inside backtick-quoted identifiers, so + emitted aliases and their references satisfy BigQuery's grammar. - Applied as a post-pass on the BigQuery dialect's final SQL so - emitted column aliases (``SELECT ... AS \\`orders._count\\``) and - references to those aliases - (``ORDER BY \\`orders._count\\``) comply with BigQuery's column-name - grammar. + The base LENGTH pass runs first: it no-ops on under-limit aliases (SQL + stays byte-identical) and rewrites over-limit ones to a still-dotted + form that this regex then mangles — no double-encoding. """ + sql = super().rewrite_emitted_sql(sql=sql, aliases=aliases) return _DOTTED_ALIAS_RE.sub( lambda m: f"`{encode_alias(m.group(1))}`", sql ) @@ -198,11 +213,20 @@ def rewrite_emitted_sql(self, sql: str) -> str: def decode_result_keys( self, rows: list[dict[str, Any]], + *, + aliases: Sequence[str] = (), ) -> list[dict[str, Any]]: - """Reverse the BigQuery alias mangling on result-row keys so - consumers see SLayer's universal dotted alias shape regardless of - whether the query ran against BigQuery or another dialect.""" - return [{decode_alias(k): v for k, v in row.items()} for row in rows] + """Reverse the BigQuery alias mangling on result-row keys so consumers + see SLayer's universal dotted shape whatever dialect ran the query. + + Fitted keys aren't recoverable alone, so the ``emitted -> canonical`` + map is consulted first, falling back to the ``___`` -> ``.`` bijection. + """ + mapping = self.decode_alias_map(aliases) + return [ + self._rekey_row(row=row, mapping=mapping, fallback=decode_alias) + for row in rows + ] def build_engine( self, diff --git a/slayer/sql/dialects/clickhouse.py b/slayer/sql/dialects/clickhouse.py index 51265822..4ae4cf0a 100644 --- a/slayer/sql/dialects/clickhouse.py +++ b/slayer/sql/dialects/clickhouse.py @@ -21,6 +21,7 @@ class ClickhouseDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = None # unbounded def build_median( self, diff --git a/slayer/sql/dialects/duckdb.py b/slayer/sql/dialects/duckdb.py index c8a9e7da..576690fc 100644 --- a/slayer/sql/dialects/duckdb.py +++ b/slayer/sql/dialects/duckdb.py @@ -21,6 +21,7 @@ class DuckdbDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = 256 # safe documented ceiling def build_approx_count_distinct( self, diff --git a/slayer/sql/dialects/mysql.py b/slayer/sql/dialects/mysql.py index c9fd6974..04dec814 100644 --- a/slayer/sql/dialects/mysql.py +++ b/slayer/sql/dialects/mysql.py @@ -25,6 +25,8 @@ class MysqlDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + # Conservative: MySQL allows 256 for column aliases but errors (not truncates). + max_identifier_bytes: int | None = 64 def build_median( self, diff --git a/slayer/sql/dialects/postgres.py b/slayer/sql/dialects/postgres.py index 65909149..3562df9e 100644 --- a/slayer/sql/dialects/postgres.py +++ b/slayer/sql/dialects/postgres.py @@ -45,6 +45,8 @@ class PostgresDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + # NAMEDATALEN: 63 usable bytes; over-length names are SILENTLY truncated. + max_identifier_bytes: int | None = 63 def rewrite_target_ast(self, tree: exp.Expression) -> exp.Expression: """DEV-1576: numeric-cast the first arg of every 2-arg ROUND so diff --git a/slayer/sql/dialects/snowflake.py b/slayer/sql/dialects/snowflake.py index 2fe7a87a..08590557 100644 --- a/slayer/sql/dialects/snowflake.py +++ b/slayer/sql/dialects/snowflake.py @@ -193,6 +193,7 @@ class so ``engine_factory`` / ``client`` stay dialect-agnostic. log10_native: bool = True # No native LOG2 — falls through to canonical ``LOG(2, x)`` form. log2_native: bool = False + max_identifier_bytes: int | None = 255 def build_approx_count_distinct( self, diff --git a/slayer/sql/dialects/sqlite.py b/slayer/sql/dialects/sqlite.py index 13f2994d..4569279a 100644 --- a/slayer/sql/dialects/sqlite.py +++ b/slayer/sql/dialects/sqlite.py @@ -406,6 +406,7 @@ class SqliteDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + max_identifier_bytes: int | None = None # unbounded def build_date_trunc( self, diff --git a/slayer/sql/dialects/tsql.py b/slayer/sql/dialects/tsql.py index 59193fcb..a435bed7 100644 --- a/slayer/sql/dialects/tsql.py +++ b/slayer/sql/dialects/tsql.py @@ -28,13 +28,14 @@ import re from typing import Any -from collections.abc import Callable +from collections.abc import Callable, Sequence import sqlglot from sqlglot import exp from slayer.core.enums import TimeGranularity from slayer.sql.dialects._alias_mangle import decode_alias, encode_alias +from slayer.sql.dialects._identifier_fit import fit_identifier from slayer.sql.dialects.base import SqlDialect, _build_covar_decomposition @@ -70,6 +71,7 @@ class TsqlDialect(SqlDialect): explain_postfix: str = "; SET SHOWPLAN_ALL OFF" log10_native: bool = True log2_native: bool = False + max_identifier_bytes: int | None = 128 # sysname is nvarchar(128) def build_approx_count_distinct( self, @@ -334,21 +336,30 @@ def emit_outer_wrap( # DEV-1571 Bug 2: bracketed dotted-alias mangling # ------------------------------------------------------------------ - def rewrite_emitted_sql(self, sql: str) -> str: + def fit_alias(self, name: str) -> str: + """Size the budget against the post-mangle form (``.`` -> ``___`` adds 2 + bytes per dot); return value stays dotted for the regex below.""" + return fit_identifier( + name=name, limit=self.max_identifier_bytes, expand=encode_alias, + ) + + def emit_alias(self, alias: str) -> str: + """The final identifier: length-fitted, then dot-mangled.""" + return encode_alias(self.fit_alias(alias)) + + def rewrite_emitted_sql( + self, sql: str, *, aliases: Sequence[str] = (), + ) -> str: """Replace ``.`` with ``___`` inside bracket-quoted identifiers. - T-SQL's ``ORDER BY`` resolver does not treat ``[a.b]`` as a - SELECT alias — it tries to resolve it as a column-name lookup - against the FROM scope and fails with ``Invalid column name``. - Mangling on emit gives the parser a single dotless identifier - and the alias resolves cleanly. ``decode_result_keys`` reverses - the mangling on result rows so consumers see SLayer's universal - dotted alias shape. - - Uses the same bijection as ``BigqueryDialect`` (shared encode in - ``slayer.sql.dialects._alias_mangle``); only the regex anchor - differs. + T-SQL's ``ORDER BY`` resolver treats ``[a.b]`` as a column lookup, not a + SELECT alias, and fails; a dotless identifier resolves cleanly. Same + bijection as ``BigqueryDialect``, only the regex anchor differs. + + The base LENGTH pass runs first: no-op on under-limit aliases, and + over-limit ones arrive still-dotted for this pass — no double-encoding. """ + sql = super().rewrite_emitted_sql(sql=sql, aliases=aliases) return _TSQL_DOTTED_ALIAS_RE.sub( lambda m: f"[{encode_alias(m.group(1))}]", sql ) @@ -356,9 +367,17 @@ def rewrite_emitted_sql(self, sql: str) -> str: def decode_result_keys( self, rows: list[dict[str, Any]], + *, + aliases: Sequence[str] = (), ) -> list[dict[str, Any]]: - """Reverse the T-SQL alias mangling on result-row keys so - consumers see SLayer's universal dotted alias shape regardless - of whether the query ran against T-SQL or another dialect. + """Reverse the T-SQL alias mangling on result-row keys so consumers see + SLayer's universal dotted shape whatever dialect ran the query. + + Fitted keys aren't recoverable alone, so the ``emitted -> canonical`` + map is consulted first, falling back to the ``___`` -> ``.`` bijection. """ - return [{decode_alias(k): v for k, v in row.items()} for row in rows] + mapping = self.decode_alias_map(aliases) + return [ + self._rekey_row(row=row, mapping=mapping, fallback=decode_alias) + for row in rows + ] diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index 82dab6b4..8489e099 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -19,9 +19,15 @@ DataType, TimeGranularity, ) -from slayer.core.errors import UnresolvableOrderColumnError -from slayer.engine.enriched import EnrichedMeasure, EnrichedQuery, public_projection_aliases +from slayer.core.errors import IdentifierCollisionError, UnresolvableOrderColumnError +from slayer.engine.enriched import ( + EnrichedMeasure, + EnrichedQuery, + all_projection_aliases, + public_projection_aliases, +) from slayer.sql.dialects import SqlDialect, get_dialect +from slayer.sql.dialects._identifier_fit import fit_identifier from slayer.sql.reserved_keywords import ( SLAYER_RESERVED_KEYWORDS, prequote_reserved_identifiers, @@ -259,17 +265,19 @@ def _parse_window_duration(value: str) -> list[tuple[int, str]]: return parts -def _cte_name_from_alias(prefix: str, alias: str) -> str: +def _cte_name_from_alias(prefix: str, alias: str, *, limit: int | None = None) -> str: """Build a unique CTE name from a measure alias. - Dots are replaced with ``__`` (double underscore) to avoid collision - with aliases that already contain underscores. E.g.: - - ``orders.revenue_sum`` -> ``_fm_orders__revenue_sum`` - - ``orders_v2.revenue_sum`` -> ``_fm_orders_v2__revenue_sum`` + Dots become ``__`` to avoid colliding with aliases that already contain + underscores: ``orders.revenue_sum`` -> ``_fm_orders__revenue_sum``. + + The whole result is fitted to ``limit`` (the dialect's max), prefix + included. Pure function of ``(prefix, alias, limit)``, so a CTE's definition + and every reference derive the same name. """ sanitized = alias.replace(".", "__") sanitized = re.sub(r"[^a-zA-Z0-9_]", "_", sanitized) - return prefix + sanitized + return fit_identifier(name=prefix + sanitized, limit=limit) def _alias_prefixes(model_name: str) -> list: @@ -403,6 +411,29 @@ def __init__(self, dialect: str | SqlDialect = "postgres"): self._dialect: SqlDialect = dialect else: self._dialect = get_dialect(dialect) + # Per-statement CTE-name allocation, ``emitted -> (prefix, alias)``. + self._cte_names: dict[str, tuple[str, str]] = {} + + def _cte_name(self, prefix: str, alias: str) -> str: + """Allocate a length-fitted CTE name, refusing a collision. + + CTE names are emitted UNQUOTED, so two that fit to the same string + silently reference the wrong CTE. Keyed CASEFOLDED, since the server + folds unquoted names. Repeat calls with the same ``(prefix, alias)`` + return the same name, not a collision. + """ + name = _cte_name_from_alias( + prefix=prefix, alias=alias, limit=self._dialect.max_identifier_bytes, + ) + owner = (prefix, alias) + prior = self._cte_names.setdefault(name.casefold(), owner) + if prior != owner: + raise IdentifierCollisionError( + first=f"{prior[0]}{prior[1]}", second=f"{prefix}{alias}", + emitted=name, dialect=self.dialect, + limit=self._dialect.max_identifier_bytes, namespace="CTE name", + ) + return name @property def dialect(self) -> str: @@ -596,6 +627,7 @@ def generate( raise ValueError( f"render_mode must be 'outer' or 'wrapped', got {render_mode!r}" ) + self._cte_names = {} # reset per-statement CTE-name allocation has_isolated = any(_has_cross_model_filter(m) for m in enriched.measures) has_windowed = any(_is_windowed_measure(m) for m in enriched.measures) has_cross_model = bool(enriched.cross_model_measures) @@ -626,12 +658,12 @@ def generate( if render_mode == "outer": sql = self._apply_outer_projection_trim(sql=sql, enriched=enriched) - # Dialect-driven post-pass: BigQuery mangles dotted aliases here. - # Default hook is identity for every other dialect (Postgres-shaped - # SqlDialect base). Fires for BOTH render modes — inner CTE column - # names are subject to the same dialect alias rules as the outer - # projection. - sql = self._dialect.rewrite_emitted_sql(sql) + # Dialect post-pass: fit over-limit aliases (DEV-1756), plus BigQuery/T-SQL + # dot-mangling. Fires for both render modes. Unfiltered alias set: hidden + # hoists are projected too and must be fitted with the same map. + sql = self._dialect.rewrite_emitted_sql( + sql=sql, aliases=all_projection_aliases(enriched), + ) return sql def _apply_outer_projection_trim( @@ -753,7 +785,7 @@ def _build_combined(self, enriched: EnrichedQuery, # --- Cross-model measure CTEs --- seen_cm_ctes: set = set() for cm in enriched.cross_model_measures: - cte_name = _cte_name_from_alias("_cm_", cm.alias) + cte_name = self._cte_name(prefix="_cm_", alias=cm.alias) if cte_name in seen_cm_ctes: measure_cte_refs.append((cte_name, cm.alias, None)) continue @@ -838,7 +870,7 @@ def _build_combined(self, enriched: EnrichedQuery, for measure in enriched.measures: if not _is_windowed_measure(measure): continue - cte_name = _cte_name_from_alias("_wm_", measure.alias) + cte_name = self._cte_name(prefix="_wm_", alias=measure.alias) ctes.append((cte_name, self._generate_window_measure_cte(enriched=enriched, measure=measure))) measure_cte_refs.append((cte_name, measure.alias, None)) @@ -846,7 +878,7 @@ def _build_combined(self, enriched: EnrichedQuery, for measure in enriched.measures: if not _has_cross_model_filter(measure): continue - cte_name = _cte_name_from_alias("_fm_", measure.alias) + cte_name = self._cte_name(prefix="_fm_", alias=measure.alias) # Measure aggregation without CASE WHEN (the join IS the filter) unfiltered = copy.copy(measure) @@ -1632,7 +1664,9 @@ def _generate_with_computed(self, enriched: EnrichedQuery, for t in deferred_self_joins: src_cte = ctes[-1][0] - shift_name = f"shifted_{t.name}" + # Built from a user-supplied transform name; fit + collision-check + # like every CTE. Allocated once, reused for def and references. + shift_name = self._cte_name(prefix="shifted_", alias=t.name) shifted_sql = self._generate_shifted_base( enriched=enriched, transform=t, ) @@ -1654,7 +1688,7 @@ def _generate_with_computed(self, enriched: EnrichedQuery, join_cols = ", ".join( f'{src_cte}.{self._q(a)}' for a in sorted(available_aliases) ) - join_layer = f"sjoin_{t.name}" + join_layer = self._cte_name(prefix="sjoin_", alias=t.name) join_sql = ( f"SELECT {join_cols}, {col_sql} AS {self._q(t.alias)}\n" f"FROM {src_cte}\n" @@ -1772,9 +1806,9 @@ def _build_consecutive_periods_ctes( layer_num: int, ) -> tuple[list[tuple[str, str]], list[tuple[str, str]]]: partition_aliases = getattr(transform, "partition_aliases", []) or [] - reset_alias = _cte_name_from_alias("_cp_reset_", transform.alias) - reset_cte = _cte_name_from_alias(f"cp_reset_{layer_num}_", transform.alias) - value_cte = _cte_name_from_alias(f"cp_value_{layer_num}_", transform.alias) + reset_alias = self._cte_name(prefix="_cp_reset_", alias=transform.alias) + reset_cte = self._cte_name(prefix=f"cp_reset_{layer_num}_", alias=transform.alias) + value_cte = self._cte_name(prefix=f"cp_value_{layer_num}_", alias=transform.alias) def _quoted_col(name: str) -> exp.Column: return exp.Column(this=exp.to_identifier(name, quoted=True)) diff --git a/tests/dialects/test_bigquery.py b/tests/dialects/test_bigquery.py index 4307bdd9..5ffb6ec7 100644 --- a/tests/dialects/test_bigquery.py +++ b/tests/dialects/test_bigquery.py @@ -439,7 +439,7 @@ async def test_generator_dispatches_through_rewrite_emitted_sql_hook() -> None: type(gen._dialect), "rewrite_emitted_sql", autospec=True, - side_effect=lambda self, sql: sql, + side_effect=lambda self, sql, **kw: sql, ) as spy: gen.generate(enriched=enriched) assert spy.called, ( @@ -481,7 +481,7 @@ async def test_engine_dispatches_through_decode_result_keys_hook() -> None: PostgresDialect, "decode_result_keys", autospec=True, - side_effect=lambda self, rows: rows, + side_effect=lambda self, rows, **kw: rows, ) as spy: await engine.execute(SlayerQuery( source_model="orders", diff --git a/tests/dialects/test_identifier_fit.py b/tests/dialects/test_identifier_fit.py new file mode 100644 index 00000000..5785d4b0 --- /dev/null +++ b/tests/dialects/test_identifier_fit.py @@ -0,0 +1,436 @@ +"""DEV-1756: the identifier-length primitive and its dialect wiring. + +Postgres caps identifiers at 63 bytes and silently truncates longer ones, so sibling +aliases can collapse. ``fit_identifier`` shortens to a pure ``__``; +``substitute_quoted`` is the two-phase write side. Emission: ``test_dev1756_identifier_length.py``. +""" + +from __future__ import annotations + +import hashlib +import os +import re +import subprocess +import sys + +import pytest + +from slayer.core.errors import IdentifierCollisionError +from slayer.sql.dialects import _ALL_DIALECTS, get_dialect +from slayer.sql.dialects._alias_mangle import encode_alias +from slayer.sql.dialects._identifier_fit import ( + HASH_LEN, + MIN_LIMIT, + fit_identifier, + substitute_quoted, +) + + +# Repro pair: 73 and 74 bytes, sharing a 63-byte prefix. +LONG_NAME = "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.name" +LONG_EMAIL = "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.email" + +# Differ only in the middle, so head and tail survive fitting identically (forces a digest collision). +TWIN_A = "SandboxAlpha." * 3 + "111" + ".SandboxOmega" * 3 +TWIN_B = "SandboxAlpha." * 3 + "222" + ".SandboxOmega" * 3 + +ALL_LIMITS = (63, 64, 127, 128, 255, 256, 300) + +_UNQUOTED_IDENT_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +_MARKER_RE = re.compile(rf"_([0-9a-f]{{{HASH_LEN}}})_") + + +def _nbytes(s: str) -> int: + return len(s.encode("utf-8")) + + +def _dq(name: str) -> str: + """ANSI double-quote, the shape ``substitute_quoted`` is handed.""" + return f'"{name}"' + + +class TestFitIdentifierCore: + def test_repro_pair_is_actually_over_the_postgres_limit(self) -> None: + """Guard the premise: without this the rest of the file is vacuous.""" + assert _nbytes(LONG_NAME) == 73 + assert _nbytes(LONG_EMAIL) == 74 + assert LONG_NAME.encode()[:63] == LONG_EMAIL.encode()[:63] + + def test_under_limit_returned_unchanged(self) -> None: + assert fit_identifier("orders.revenue_sum", limit=63) == "orders.revenue_sum" + + def test_exactly_at_limit_returned_unchanged(self) -> None: + name = "a" * 63 + assert fit_identifier(name, limit=63) == name + + def test_one_byte_over_limit_is_shortened(self) -> None: + name = "a" * 64 + assert fit_identifier(name, limit=63) != name + + def test_none_limit_is_a_no_op(self) -> None: + assert fit_identifier(LONG_EMAIL, limit=None) == LONG_EMAIL + + @pytest.mark.parametrize("limit", ALL_LIMITS) + def test_output_within_limit_bytes(self, limit: int) -> None: + name = "Sandbox" * 80 # 560 bytes — over every configured limit + assert _nbytes(fit_identifier(name, limit=limit)) <= limit + + def test_output_is_pinned_exactly(self) -> None: + """Pin the whole result: 27 head bytes, 10-byte marker, 26 tail bytes.""" + expected = ( + "SandboxInvoiceV2.SandboxSub" + f"_{hashlib.sha256(LONG_EMAIL.encode()).hexdigest()[:HASH_LEN]}_" + "omer.SandboxConsumer.email" + ) + assert fit_identifier(LONG_EMAIL, limit=63) == expected + assert _nbytes(expected) == 63 + + def test_marker_is_exactly_sha256_of_the_full_original(self) -> None: + got = fit_identifier(LONG_EMAIL, limit=63) + match = _MARKER_RE.search(got) + assert match, got + expected = hashlib.sha256(LONG_EMAIL.encode()).hexdigest()[:HASH_LEN] + assert match.group(1) == expected + + def test_digest_is_not_process_dependent(self) -> None: + """The read side recomputes in another process, so the digest must not depend on PYTHONHASHSEED.""" + env = os.environ.copy() + env["PYTHONHASHSEED"] = "12345" + out = subprocess.run( + [ + sys.executable, "-c", + "from slayer.sql.dialects._identifier_fit import fit_identifier;" + f"print(fit_identifier({LONG_EMAIL!r}, limit=63))", + ], + capture_output=True, text=True, check=True, env=env, + ) + assert out.stdout.strip() == fit_identifier(LONG_EMAIL, limit=63) + + def test_head_and_tail_both_preserved(self) -> None: + got = fit_identifier(LONG_EMAIL, limit=63) + assert got.startswith("SandboxInvoiceV2") + assert got.endswith("email") + + def test_repro_siblings_differ_outside_the_hash(self) -> None: + """Colliding aliases stay distinguishable by eye, not only by digest.""" + a = fit_identifier(LONG_NAME, limit=63) + b = fit_identifier(LONG_EMAIL, limit=63) + assert a != b + assert a.endswith("name") + assert b.endswith("email") + + def test_distinct_inputs_sharing_63_byte_prefix_produce_distinct_outputs(self) -> None: + a = fit_identifier(LONG_NAME, limit=63) + b = fit_identifier(LONG_EMAIL, limit=63) + assert a.encode()[:63] != b.encode()[:63] + + def test_shape_is_head_underscore_hash_underscore_tail(self) -> None: + got = fit_identifier(LONG_EMAIL, limit=63) + assert _MARKER_RE.search(got), got + + def test_separators_are_trimmed_next_to_the_marker(self) -> None: + """Head/tail are stripped of ``._`` so a cut on a separator won't yield ``foo._a1b2c3d4_.bar``.""" + name = "a." * 60 # every even offset lands on a '.' + got = fit_identifier(name, limit=63) + match = _MARKER_RE.search(got) + assert match, got + head, tail = got[: match.start()], got[match.end():] + assert not head.endswith((".", "_")), got + assert not tail.startswith((".", "_")), got + + def test_minimum_budget_form_is_legal(self) -> None: + """At the tightest budget the result is the bare ``__`` marker, still a legal identifier.""" + got = fit_identifier(LONG_EMAIL, limit=MIN_LIMIT) + assert _nbytes(got) <= MIN_LIMIT + assert _UNQUOTED_IDENT_RE.match(got.replace(".", "_")), got + + +class TestFitIdentifierEdges: + def test_multibyte_never_splits_a_codepoint(self) -> None: + name = "é" * 60 # 120 bytes of 2-byte codepoints + got = fit_identifier(name, limit=63) + got.encode("utf-8").decode("utf-8") # raises if a codepoint was split + assert _nbytes(got) <= 63 + + def test_multibyte_bound_is_bytes_not_characters(self) -> None: + name = "é" * 60 + got = fit_identifier(name, limit=63) + assert _nbytes(got) <= 63 < len(name) * 2 + + def test_unquoted_legality_preserved_for_flat_input(self) -> None: + """CTE names and virtual-model shorts are emitted unquoted, so a fitted flat name stays legal.""" + flat = "_cm_" + "SandboxSubscription__SandboxCustomer__SandboxConsumer__" * 2 + got = fit_identifier(flat, limit=63) + assert _UNQUOTED_IDENT_RE.match(got), got + + @pytest.mark.parametrize("limit", range(MIN_LIMIT, 40)) + def test_never_starts_with_a_digit(self, limit: int) -> None: + """A bare hex digest can begin with a digit, illegal unquoted on several dialects.""" + assert not fit_identifier(LONG_EMAIL, limit=limit)[0].isdigit() + + @pytest.mark.parametrize("limit", range(MIN_LIMIT, 40)) + def test_tiny_budget_still_within_limit(self, limit: int) -> None: + assert _nbytes(fit_identifier(LONG_EMAIL, limit=limit)) <= limit + + def test_head_trimmed_to_empty_still_legal(self) -> None: + name = "." * 40 + "abcdefghij" * 5 + got = fit_identifier(name, limit=MIN_LIMIT) + assert _nbytes(got) <= MIN_LIMIT + assert not got[0].isdigit() + + def test_punctuation_heavy_name(self) -> None: + got = fit_identifier("a._." * 40, limit=63) + assert _nbytes(got) <= 63 + + def test_limit_below_minimum_raises(self) -> None: + with pytest.raises(ValueError, match="MIN_LIMIT|too small|at least"): + fit_identifier(LONG_EMAIL, limit=MIN_LIMIT - 1) + + +# fit_identifier — the `expand` hook (BigQuery / T-SQL dot-mangling) + + +class TestFitIdentifierExpand: + def test_post_mangle_length_within_limit(self) -> None: + """Budget is computed against the expanded form since BigQuery/T-SQL mangle ``.`` -> ``___`` after fitting.""" + name = ".".join(["Sandbox" * 4] * 6) # many dots, well over 128 + got = fit_identifier(name, limit=128, expand=encode_alias) + assert _nbytes(encode_alias(got)) <= 128 + + def test_expand_is_not_applied_to_the_return_value(self) -> None: + """``fit_identifier`` only sizes against the expansion; the dialect does the mangling.""" + name = ".".join(["Sandbox" * 4] * 6) + got = fit_identifier(name, limit=128, expand=encode_alias) + assert "___" not in got + + def test_expand_under_limit_is_identity(self) -> None: + assert fit_identifier("a.b", limit=128, expand=encode_alias) == "a.b" + + def test_aggressively_expanding_transform_still_fits(self) -> None: + """The budget loop shrinks until the expanded form fits, however aggressive the expansion.""" + def explode(s: str) -> str: + return s + "#" * (40 * s.count(".")) + + name = "a.b.c" * 20 + got = fit_identifier(name, limit=63, expand=explode) + assert _nbytes(explode(got)) <= 63 + + +# substitute_quoted — the write-side primitive (two-phase) + + +class TestSubstituteQuoted: + def test_empty_mapping_is_identity(self) -> None: + sql = 'SELECT 1 AS "a.b"' + assert substitute_quoted(sql, {}, quote=_dq) == sql + + def test_replaces_every_occurrence(self) -> None: + sql = 'SELECT "a.b" FROM (SELECT x AS "a.b") AS _o ORDER BY "a.b"' + got = substitute_quoted(sql, {"a.b": "z"}, quote=_dq) + assert got.count('"z"') == 3 + assert '"a.b"' not in got + + def test_chained_mapping_does_not_cascade(self) -> None: + """Two-phase pass: ``A->B`` and ``B->C`` must not cascade A into C.""" + sql = 'SELECT "A", "B"' + got = substitute_quoted(sql, {"A": "B", "B": "C"}, quote=_dq) + assert got == 'SELECT "B", "C"' + + def test_chained_mapping_is_order_independent(self) -> None: + sql = 'SELECT "A", "B"' + forward = substitute_quoted(sql, {"A": "B", "B": "C"}, quote=_dq) + reverse = substitute_quoted(sql, {"B": "C", "A": "B"}, quote=_dq) + assert forward == reverse == 'SELECT "B", "C"' + + def test_swap_is_not_a_cascade(self) -> None: + """Simultaneous ``A->B`` and ``B->A`` — only a two-phase pass gets this right.""" + got = substitute_quoted('SELECT "A", "B"', {"A": "B", "B": "A"}, quote=_dq) + assert got == 'SELECT "B", "A"' + + def test_only_quoted_occurrences_are_replaced(self) -> None: + """A bare occurrence of the same text (a table alias, say) is left alone.""" + sql = 'SELECT tbl.col AS "tbl.col" FROM t' + got = substitute_quoted(sql, {"tbl.col": "z"}, quote=_dq) + assert got == 'SELECT tbl.col AS "z" FROM t' + + def test_does_not_reach_into_string_literals(self) -> None: + """Keyed on exact quoted tokens, not a length regex, so quoted text inside a literal is untouched.""" + literal = "x" * 90 + sql = f'SELECT 1 AS "a.b" WHERE note LIKE \'%"{literal}"%\'' + got = substitute_quoted(sql, {"a.b": "z"}, quote=_dq) + assert f'"{literal}"' in got + + def test_sentinel_cannot_leak_into_the_output(self) -> None: + """The two-phase sentinel must not survive, even if the SQL contains sentinel-looking text.""" + sql = 'SELECT "a.b", \'\\x00 0 \\x00\' AS lit' + got = substitute_quoted(sql, {"a.b": "z"}, quote=_dq) + assert "\x00" not in got.replace("\\x00", "") + + +# Conservative universal byte budgets, not exact per-identifier-class rules: MySQL uses its +# 64-char identifier limit (not the 256-char column-alias one) and Oracle assumes 12.2+ (128). +EXPECTED_LIMITS = { + "postgres": 63, + "mysql": 64, + "redshift": 127, + "oracle": 128, + "tsql": 128, + "snowflake": 255, + "duckdb": 256, + "bigquery": 300, + "sqlite": None, + "clickhouse": None, + "trino": None, + "presto": None, + "databricks": None, + "spark": None, +} + + +class TestDialectLimits: + def test_every_registered_dialect_is_covered(self) -> None: + assert {d.sqlglot_name for d in _ALL_DIALECTS} == set(EXPECTED_LIMITS) + + @pytest.mark.parametrize("name,expected", sorted(EXPECTED_LIMITS.items(), key=lambda kv: kv[0])) + def test_configured_limit(self, name: str, expected: int | None) -> None: + assert get_dialect(name).max_identifier_bytes == expected + + def test_base_default_is_conservative(self) -> None: + """A dialect that forgets to set the field inherits the tightest limit, not unbounded.""" + from slayer.sql.dialects.base import SqlDialect + + assert SqlDialect().max_identifier_bytes == 63 + + @pytest.mark.parametrize("name", sorted(EXPECTED_LIMITS)) + def test_emit_alias_identity_under_limit(self, name: str) -> None: + """Only BigQuery/T-SQL dot-mangle a short alias; every other dialect leaves it byte-identical.""" + got = get_dialect(name).emit_alias("orders.revenue_sum") + if name in ("bigquery", "tsql"): + assert got == encode_alias("orders.revenue_sum") + else: + assert got == "orders.revenue_sum" + + @pytest.mark.parametrize("name", sorted(EXPECTED_LIMITS)) + def test_fit_alias_identity_under_limit(self, name: str) -> None: + """``fit_alias`` is length-only, so a short alias is identity on every dialect.""" + assert get_dialect(name).fit_alias("orders.revenue_sum") == "orders.revenue_sum" + + @pytest.mark.parametrize("name", ["sqlite", "clickhouse", "trino", "presto", "databricks", "spark"]) + def test_unbounded_dialects_never_shorten(self, name: str) -> None: + assert get_dialect(name).emit_alias(LONG_EMAIL) == LONG_EMAIL + + def test_postgres_shortens_over_limit(self) -> None: + got = get_dialect("postgres").emit_alias(LONG_EMAIL) + assert got != LONG_EMAIL + assert _nbytes(got) <= 63 + + def test_bigquery_emit_alias_is_mangled_and_fitted(self) -> None: + """``emit_alias`` is the final identifier: length-fitted first, then dot-mangled.""" + long_dotted = ".".join(["Sandbox" * 6] * 8) # way over 300 + bq = get_dialect("bigquery") + got = bq.emit_alias(long_dotted) + assert "." not in got + assert _nbytes(got) <= 300 + assert got == encode_alias(bq.fit_alias(long_dotted)) + + def test_tsql_emit_alias_is_mangled_and_fitted(self) -> None: + long_dotted = ".".join(["Sandbox" * 4] * 8) # over 128 + tsql = get_dialect("tsql") + got = tsql.emit_alias(long_dotted) + assert "." not in got + assert _nbytes(got) <= 128 + assert got == encode_alias(tsql.fit_alias(long_dotted)) + + +# alias_rewrite_map — the collision guard + + +class TestAliasRewriteMap: + def test_returns_only_differing_entries(self) -> None: + pg = get_dialect("postgres") + got = pg.alias_rewrite_map(["orders.status", LONG_EMAIL]) + assert "orders.status" not in got + assert got[LONG_EMAIL] == pg.fit_alias(LONG_EMAIL) + + def test_empty_when_nothing_over_limit(self) -> None: + assert get_dialect("postgres").alias_rewrite_map(["a.b", "c.d"]) == {} + + def test_empty_alias_list(self) -> None: + assert get_dialect("postgres").alias_rewrite_map([]) == {} + + def test_unbounded_dialect_map_is_empty(self) -> None: + assert get_dialect("sqlite").alias_rewrite_map([LONG_NAME, LONG_EMAIL]) == {} + + def test_duplicate_canonical_aliases_are_not_a_collision(self) -> None: + pg = get_dialect("postgres") + assert pg.alias_rewrite_map([LONG_EMAIL, LONG_EMAIL]) == pg.alias_rewrite_map([LONG_EMAIL]) + + def test_keys_and_values_are_disjoint(self) -> None: + """Every key is over the limit and every value within it, so no substitution can produce a key.""" + pg = get_dialect("postgres") + mapping = pg.alias_rewrite_map([LONG_NAME, LONG_EMAIL, "orders.status"]) + assert mapping + assert not (set(mapping) & set(mapping.values())) + for key, value in mapping.items(): + assert _nbytes(key) > 63 >= _nbytes(value) + + def test_digest_collision_raises(self, monkeypatch: pytest.MonkeyPatch) -> None: + """A constant digest forces two mid-differing over-limit aliases onto one emitted name.""" + import slayer.sql.dialects._identifier_fit as fitmod + + monkeypatch.setattr(fitmod, "_digest", lambda name: "deadbeef") + pg = get_dialect("postgres") + with pytest.raises(IdentifierCollisionError) as exc: + pg.alias_rewrite_map([TWIN_A, TWIN_B]) + assert TWIN_A in str(exc.value) + assert TWIN_B in str(exc.value) + + def test_shortened_form_equal_to_an_existing_short_alias_raises(self) -> None: + """The guard covers identity entries: a short alias equal to another's fitted form is a duplicate.""" + pg = get_dialect("postgres") + collider = pg.fit_alias(LONG_EMAIL) # within 63 bytes, so an identity entry + assert pg.fit_alias(collider) == collider + with pytest.raises(IdentifierCollisionError): + pg.alias_rewrite_map([LONG_EMAIL, collider]) + + def test_error_names_the_dialect_and_limit(self, monkeypatch: pytest.MonkeyPatch) -> None: + import slayer.sql.dialects._identifier_fit as fitmod + + monkeypatch.setattr(fitmod, "_digest", lambda name: "deadbeef") + pg = get_dialect("postgres") + with pytest.raises(IdentifierCollisionError) as exc: + pg.alias_rewrite_map([TWIN_A, TWIN_B]) + assert "postgres" in str(exc.value) + assert "63" in str(exc.value) + + def test_is_a_slayer_error_and_value_error(self) -> None: + from slayer.core.errors import SlayerError + + assert issubclass(IdentifierCollisionError, SlayerError) + assert issubclass(IdentifierCollisionError, ValueError) + + +# decode_result_keys — many-to-one must not silently overwrite + + +class TestDecodeCollision: + def test_two_keys_decoding_to_one_canonical_raises(self) -> None: + """A row carrying both an alias's fitted form and its canonical spelling must not lose a value.""" + pg = get_dialect("postgres") + rows = [{pg.fit_alias(LONG_EMAIL): 1, LONG_EMAIL: 2}] + with pytest.raises(IdentifierCollisionError): + pg.decode_result_keys(rows, aliases=[LONG_EMAIL]) + + @pytest.mark.parametrize("dialect", ["bigquery", "tsql"]) + def test_mangle_fallback_collision_raises(self, dialect: str) -> None: + """The ``decode_alias`` fallback runs inside the duplicate check so ``orders___status`` and ``orders.status`` collide.""" + d = get_dialect(dialect) + rows = [{"orders___status": 1, "orders.status": 2}] + with pytest.raises(IdentifierCollisionError): + d.decode_result_keys(rows, aliases=[]) + + @pytest.mark.parametrize("dialect", ["bigquery", "tsql"]) + def test_mangle_fallback_preserves_every_value(self, dialect: str) -> None: + d = get_dialect(dialect) + got = d.decode_result_keys([{"a___b": 1, "c___d": 2}], aliases=[]) + assert got == [{"a.b": 1, "c.d": 2}] diff --git a/tests/integration/test_dev1756_identifier_length_pg.py b/tests/integration/test_dev1756_identifier_length_pg.py new file mode 100644 index 00000000..dcd3fe94 --- /dev/null +++ b/tests/integration/test_dev1756_identifier_length_pg.py @@ -0,0 +1,276 @@ +"""DEV-1756 against a live Postgres, which truncates identifiers past 63 bytes with +only a NOTICE, so emission tests pass while the server mis-answers. Covers two modes: +sibling aliases collapsing at a shared 63-byte prefix, and a lone over-limit alias +vanishing with no error (the more dangerous case). +""" + +import re +import uuid + +import pytest + +pytest.importorskip("pytest_postgresql") + +import psycopg +from pytest_postgresql import factories + +from slayer.async_utils import run_sync +from slayer.core.enums import DataType +from slayer.core.models import Column, DatasourceConfig, ModelJoin, SlayerModel +from slayer.core.query import ColumnRef, OrderItem, SlayerQuery +from slayer.engine.query_engine import SlayerQueryEngine +from slayer.storage.yaml_storage import YAMLStorage + +postgresql_proc = factories.postgresql_proc(port=None) + +DS = "testpg" + +LONG_NAME = "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.name" +LONG_EMAIL = "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.email" + + +def _create_db(postgresql_proc): + info = postgresql_proc + db_name = f"test_{uuid.uuid4().hex[:12]}" + admin = psycopg.connect(host=info.host, port=info.port, user=info.user, dbname="postgres") + admin.autocommit = True + with admin.cursor() as cur: + cur.execute(f'CREATE DATABASE "{db_name}"') + admin.close() + conn = psycopg.connect(host=info.host, port=info.port, user=info.user, dbname=db_name) + return conn, db_name + + +def _drop_db(postgresql_proc, db_name): + info = postgresql_proc + admin = psycopg.connect(host=info.host, port=info.port, user=info.user, dbname="postgres") + admin.autocommit = True + with admin.cursor() as cur: + cur.execute(f'DROP DATABASE IF EXISTS "{db_name}" WITH (FORCE)') + admin.close() + + +@pytest.fixture(scope="module") +def _chain_storage(postgresql_proc, tmp_path_factory): + """3-hop join chain whose projection aliases cross Postgres' 63-byte limit.""" + conn, db_name = _create_db(postgresql_proc) + try: + cur = conn.cursor() + cur.execute( + "CREATE TABLE consumers (id INTEGER PRIMARY KEY, name TEXT, email TEXT, " + "lifetime_value NUMERIC(10,2))" + ) + cur.execute("CREATE TABLE customers (id INTEGER PRIMARY KEY, consumer_id INTEGER)") + cur.execute("CREATE TABLE subscriptions (id INTEGER PRIMARY KEY, customer_id INTEGER)") + cur.execute( + "CREATE TABLE invoices (id INTEGER PRIMARY KEY, subscription_id INTEGER, " + "status TEXT, total_amount NUMERIC(10,2))" + ) + cur.executemany( + "INSERT INTO consumers VALUES (%s, %s, %s, %s)", + [(1, "Ann", "ann@example.io", 10), (2, "Bob", "bob@example.io", 20)], + ) + cur.executemany("INSERT INTO customers VALUES (%s, %s)", [(1, 1), (2, 2)]) + cur.executemany("INSERT INTO subscriptions VALUES (%s, %s)", [(1, 1), (2, 2)]) + cur.executemany( + "INSERT INTO invoices VALUES (%s, %s, %s, %s)", + [(1, 1, "paid", 100), (2, 1, "paid", 50), (3, 2, "paid", 200)], + ) + conn.commit() + + storage = YAMLStorage(base_dir=str(tmp_path_factory.mktemp("dev1756_pg"))) + info = postgresql_proc + run_sync(storage.save_datasource(DatasourceConfig( + name=DS, type="postgres", host=info.host, port=info.port, + database=db_name, username=info.user, password="", + ))) + + run_sync(storage.save_model(SlayerModel( + name="SandboxConsumer", sql_table="consumers", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + Column(name="email", sql="email", type=DataType.TEXT), + Column(name="lifetimeValue", sql="lifetime_value", type=DataType.DOUBLE), + ], + ))) + run_sync(storage.save_model(SlayerModel( + name="SandboxCustomer", sql_table="customers", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="consumer_id", sql="consumer_id", type=DataType.INT), + ], + joins=[ModelJoin(target_model="SandboxConsumer", join_pairs=[["consumer_id", "id"]])], + ))) + run_sync(storage.save_model(SlayerModel( + name="SandboxSubscription", sql_table="subscriptions", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.INT), + ], + joins=[ModelJoin(target_model="SandboxCustomer", join_pairs=[["customer_id", "id"]])], + ))) + run_sync(storage.save_model(SlayerModel( + name="SandboxInvoiceV2", sql_table="invoices", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="status", sql="status", type=DataType.TEXT), + Column(name="totalAmount", sql="total_amount", type=DataType.DOUBLE), + Column(name="subscription_id", sql="subscription_id", type=DataType.INT), + ], + joins=[ModelJoin( + target_model="SandboxSubscription", join_pairs=[["subscription_id", "id"]], + )], + ))) + yield storage + finally: + conn.close() + _drop_db(postgresql_proc, db_name) + + +@pytest.fixture +def chain_env(_chain_storage): + """Per-test engine — the async SQLAlchemy engine binds to the event loop.""" + return SlayerQueryEngine(storage=_chain_storage) + + +DEEP = "SandboxSubscription.SandboxCustomer.SandboxConsumer" + + +@pytest.mark.integration +class TestPostgresIdentifierLength: + async def test_server_truncates_at_63_bytes(self, chain_env) -> None: + """Pin the 63-byte truncation premise against the live server.""" + client = chain_env._get_client( + await chain_env._resolve_datasource( + model=await chain_env.storage.get_model("SandboxInvoiceV2", data_source=DS), + ), + ("probe", "probe"), + ) + rows = await client.execute(sql=f'SELECT 1 AS "{LONG_EMAIL}"') + assert list(rows[0])[0] != LONG_EMAIL, "server did not truncate; premise broken" + assert len(list(rows[0])[0].encode()) == 63 + + async def test_collapse_with_outer_wrap(self, chain_env) -> None: + """Two over-limit siblings + ORDER BY outer wrap; raised AmbiguousColumnError before the fix.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ + ColumnRef(name=f"{DEEP}.name"), + ColumnRef(name=f"{DEEP}.email"), + ColumnRef(name="status"), + ], + measures=[{"formula": "totalAmount:sum"}, {"formula": "*:count"}], + order=[OrderItem(column="totalAmount:avg", direction="desc")], + limit=10, + ) + result = await chain_env.execute(query=query) + # Pin the exact pairing; a wrong-column alias would still be "present with distinct values". + got = { + (row[LONG_NAME], row[LONG_EMAIL], row["SandboxInvoiceV2.status"]): + (float(row["SandboxInvoiceV2.totalAmount_sum"]), row["SandboxInvoiceV2._count"]) + for row in result.data + } + assert got == { + ("Ann", "ann@example.io", "paid"): (150.0, 2), + ("Bob", "bob@example.io", "paid"): (200.0, 1), + } + + async def test_collapse_without_outer_wrap(self, chain_env) -> None: + """No outer wrap: the two siblings would silently collapse into one result key.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ + ColumnRef(name=f"{DEEP}.name"), + ColumnRef(name=f"{DEEP}.email"), + ], + measures=[{"formula": "*:count"}], + ) + result = await chain_env.execute(query=query) + assert result.data + names = {row[LONG_NAME] for row in result.data} + emails = {row[LONG_EMAIL] for row in result.data} + assert names == {"Ann", "Bob"} + assert emails == {"ann@example.io", "bob@example.io"} + + async def test_silent_loss_single_over_limit_alias(self, chain_env) -> None: + """One over-limit alias, no sibling: Postgres keys the row by the truncated name and it vanishes.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.email")], + measures=[{"formula": "totalAmount:sum"}], + ) + result = await chain_env.execute(query=query) + assert result.data + for row in result.data: + assert LONG_EMAIL in row, ( + f"over-limit alias missing from the result row: {sorted(row)}" + ) + assert row[LONG_EMAIL] in {"ann@example.io", "bob@example.io"} + + async def test_values_are_correct_not_merely_present(self, chain_env) -> None: + """Pin the aggregate; a wrong-column fitted alias would still be present.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.email")], + measures=[{"formula": "totalAmount:sum"}], + ) + result = await chain_env.execute(query=query) + totals = {row[LONG_EMAIL]: float(row["SandboxInvoiceV2.totalAmount_sum"]) for row in result.data} + assert totals == {"ann@example.io": 150.0, "bob@example.io": 200.0} + + async def test_response_columns_are_canonical(self, chain_env) -> None: + """Response exposes canonical keys while the emitted SQL carries the fitted form.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.name"), ColumnRef(name=f"{DEEP}.email")], + measures=[{"formula": "*:count"}], + ) + result = await chain_env.execute(query=query) + assert set(result.data[0]) >= {LONG_NAME, LONG_EMAIL} + assert LONG_EMAIL not in result.sql + + async def test_cached_execution_round_trip(self, chain_env) -> None: + """A cache hit returns canonical keys too, proving decode runs before storage.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.email")], + measures=[{"formula": "*:count"}], + ) + first = await chain_env.execute(query=query, cache=True) + second = await chain_env.execute(query=query, cache=True) + assert LONG_EMAIL in first.data[0] + assert LONG_EMAIL in second.data[0] + assert first.data == second.data + + async def test_deep_cross_model_measure_executes(self, chain_env) -> None: + """Surface 3: the `_cm_` cross-model CTE name also crosses 63 bytes on this chain.""" + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name="status")], + measures=[{"formula": f"{DEEP}.lifetimeValue:sum"}], + ) + result = await chain_env.execute(query=query) + assert "_cm_" in result.sql, "no cross-model CTE generated; test is vacuous" + for name in re.findall(r"\b_cm_\w+", result.sql): + assert len(name.encode()) <= 63, f"{name!r} exceeds the Postgres limit" + alias = f"SandboxInvoiceV2.{DEEP}.lifetimeValue_sum" + assert len(result.data) == 1 + assert float(result.data[0][alias]) == 30.0 + + async def test_nested_query_backed_model_executes(self, chain_env) -> None: + """Surface 4: virtual-model short names as output aliases; mixed-case exercises case-folding.""" + stage1 = SlayerQuery( + name="stage1", + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.email")], + measures=[{"formula": "totalAmount:sum"}], + ) + stage2 = SlayerQuery( + source_model="stage1", + dimensions=[ColumnRef(name="SandboxSubscription__SandboxCustomer__SandboxConsumer__email")], + measures=[{"formula": "totalAmount_sum:sum"}], + ) + result = await chain_env.execute(query=[stage1, stage2]) + assert result.data + assert len(result.data) == 2 diff --git a/tests/test_dev1756_identifier_length.py b/tests/test_dev1756_identifier_length.py new file mode 100644 index 00000000..7625be33 --- /dev/null +++ b/tests/test_dev1756_identifier_length.py @@ -0,0 +1,985 @@ +"""SLayer must bound generated identifiers to the dialect's limit. + +Postgres caps identifiers at 63 bytes and silently truncates past it, so long +projection aliases collide. Fixed here: projection aliases (quoted), CTE names +(unquoted), virtual-model shorts. Table aliases are surface 2, deferred to DEV-1743. +""" + +from __future__ import annotations + +import pytest +import sqlglot +from sqlglot import exp + +from slayer.core.enums import DataType +from slayer.core.errors import IdentifierCollisionError +from slayer.core.models import Column, DatasourceConfig, ModelJoin, ModelMeasure, SlayerModel +from slayer.core.query import ColumnRef, OrderItem, SlayerQuery +from slayer.engine.enriched import all_projection_aliases, public_projection_aliases +from slayer.engine.query_engine import SlayerQueryEngine +from slayer.sql.dialects import get_dialect +from slayer.sql.dialects._alias_mangle import encode_alias +from slayer.sql.generator import SQLGenerator +from slayer.storage.yaml_storage import YAMLStorage + +DS = "sandbox" +DEEP = "SandboxSubscription.SandboxCustomer.SandboxConsumer" + +LONG_NAME = "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.name" +LONG_EMAIL = "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.email" + +# Two over-limit names differing only in the middle: forces a digest collision. +TWIN_A = "SandboxAlpha." * 3 + "111" + ".SandboxOmega" * 3 +TWIN_B = "SandboxAlpha." * 3 + "222" + ".SandboxOmega" * 3 + + +# Pre-change golden SQL — pins "no churn for the common case". + +GOLDEN_SHORT_QUERY = { + "postgres": ( + 'SELECT\n SandboxInvoiceV2.status AS "SandboxInvoiceV2.status",\n' + ' SUM(SandboxInvoiceV2.total_amount) AS "SandboxInvoiceV2.totalAmount_sum"\n' + "FROM invoices AS SandboxInvoiceV2\nGROUP BY\n SandboxInvoiceV2.status" + ), + "bigquery": ( + "SELECT\n SandboxInvoiceV2.status AS `SandboxInvoiceV2___status`,\n" + " SUM(SandboxInvoiceV2.total_amount) AS `SandboxInvoiceV2___totalAmount_sum`\n" + "FROM invoices AS SandboxInvoiceV2\nGROUP BY\n SandboxInvoiceV2.status" + ), + "tsql": ( + "SELECT\n SandboxInvoiceV2.status AS [SandboxInvoiceV2___status],\n" + " SUM(SandboxInvoiceV2.total_amount) AS [SandboxInvoiceV2___totalAmount_sum]\n" + "FROM invoices AS SandboxInvoiceV2\nGROUP BY\n SandboxInvoiceV2.status" + ), + "mysql": ( + "SELECT\n SandboxInvoiceV2.status AS `SandboxInvoiceV2.status`,\n" + " SUM(SandboxInvoiceV2.total_amount) AS `SandboxInvoiceV2.totalAmount_sum`\n" + "FROM invoices AS SandboxInvoiceV2\nGROUP BY\n SandboxInvoiceV2.status" + ), + "sqlite": ( + 'SELECT\n SandboxInvoiceV2.status AS "SandboxInvoiceV2.status",\n' + ' SUM(SandboxInvoiceV2.total_amount) AS "SandboxInvoiceV2.totalAmount_sum"\n' + "FROM invoices AS SandboxInvoiceV2\nGROUP BY\n SandboxInvoiceV2.status" + ), +} + +# Full repro on an unbounded dialect: nothing may change, byte for byte. +GOLDEN_SQLITE_REPRO_ORDER = ( + 'SELECT\n "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.name",\n' + ' "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.email",\n' + ' "SandboxInvoiceV2.status",\n "SandboxInvoiceV2.totalAmount_sum",\n' + ' "SandboxInvoiceV2._count"\nFROM (\nSELECT\n' + " SandboxSubscription__SandboxCustomer__SandboxConsumer.name AS " + '"SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.name",\n' + " SandboxSubscription__SandboxCustomer__SandboxConsumer.email AS " + '"SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.email",\n' + ' SandboxInvoiceV2.status AS "SandboxInvoiceV2.status",\n' + ' SUM(SandboxInvoiceV2.total_amount) AS "SandboxInvoiceV2.totalAmount_sum",\n' + ' COUNT(*) AS "SandboxInvoiceV2._count",\n' + ' AVG(SandboxInvoiceV2.total_amount) AS "SandboxInvoiceV2.totalAmount_avg"\n' + "FROM invoices AS SandboxInvoiceV2\nLEFT JOIN subscriptions AS SandboxSubscription\n" + " ON SandboxInvoiceV2.subscription_id = SandboxSubscription.id\n" + "LEFT JOIN customers AS SandboxSubscription__SandboxCustomer\n" + " ON SandboxSubscription.customer_id = SandboxSubscription__SandboxCustomer.id\n" + "LEFT JOIN consumers AS SandboxSubscription__SandboxCustomer__SandboxConsumer\n" + " ON SandboxSubscription__SandboxCustomer.consumer_id = " + "SandboxSubscription__SandboxCustomer__SandboxConsumer.id\nGROUP BY\n" + " SandboxSubscription__SandboxCustomer__SandboxConsumer.name,\n" + " SandboxSubscription__SandboxCustomer__SandboxConsumer.email,\n" + " SandboxInvoiceV2.status\n) AS _outer\nORDER BY\n" + ' "SandboxInvoiceV2.totalAmount_avg" DESC\nLIMIT 10' +) + + +# Fixtures — the reported 3-hop chain + + +def _chain_models( + *, + case_colliding_columns: bool = False, + decoy_root_column: str | None = None, +) -> list[SlayerModel]: + consumer_columns = [ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="name", sql="name", type=DataType.TEXT), + Column(name="email", sql="email", type=DataType.TEXT), + Column(name="lifetimeValue", sql="lifetime_value", type=DataType.DOUBLE), + ] + if case_colliding_columns: + # Differs from ``email`` only by case; Postgres case-folds the namespace. + consumer_columns.append(Column(name="Email", sql="email", type=DataType.TEXT)) + return [ + SlayerModel( + name="SandboxInvoiceV2", sql_table="invoices", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="status", sql="status", type=DataType.TEXT), + Column(name="totalAmount", sql="total_amount", type=DataType.DOUBLE), + Column(name="subscription_id", sql="subscription_id", type=DataType.INT), + *([ + # Root column named what a deep join path flattens to; names permit ``__``. + Column(name=decoy_root_column, sql="decoy", type=DataType.TEXT), + ] if decoy_root_column else []), + ], + joins=[ModelJoin( + target_model="SandboxSubscription", join_pairs=[["subscription_id", "id"]], + )], + ), + SlayerModel( + name="SandboxSubscription", sql_table="subscriptions", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.INT), + ], + joins=[ModelJoin(target_model="SandboxCustomer", join_pairs=[["customer_id", "id"]])], + ), + SlayerModel( + name="SandboxCustomer", sql_table="customers", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="consumer_id", sql="consumer_id", type=DataType.INT), + ], + joins=[ModelJoin(target_model="SandboxConsumer", join_pairs=[["consumer_id", "id"]])], + ), + SlayerModel( + name="SandboxConsumer", sql_table="consumers", data_source=DS, + columns=consumer_columns, + ), + ] + + +async def _build_engine(tmp_path, **kw) -> tuple[SlayerQueryEngine, SlayerModel]: + storage = YAMLStorage(base_dir=str(tmp_path)) + await storage.save_datasource(DatasourceConfig( + name=DS, type="postgres", host="localhost", port=5432, + database="x", username="u", password="p", + )) + models = _chain_models(**kw) + for m in models: + await storage.save_model(m) + return SlayerQueryEngine(storage=storage), models[0] + + +@pytest.fixture +async def chain(tmp_path): + """Engine + root model for the 3-hop chain, with a postgres datasource.""" + return await _build_engine(tmp_path) + + +def _repro_query(*, with_order: bool = False) -> SlayerQuery: + """The exact query from the DEV-1756 report.""" + kwargs = {} + if with_order: + kwargs["order"] = [OrderItem(column="totalAmount:avg", direction="desc")] + kwargs["limit"] = 10 + return SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ + ColumnRef(name=f"{DEEP}.name"), + ColumnRef(name=f"{DEEP}.email"), + ColumnRef(name="status"), + ], + measures=[{"formula": "totalAmount:sum"}, {"formula": "*:count"}], + **kwargs, + ) + + +SHORT_QUERY = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name="status")], + measures=[{"formula": "totalAmount:sum"}], +) + + +# Identifier-inspection helpers — only the namespaces this issue owns (aliases, +# ORDER BY refs, CTE names). Join-path table aliases are surface 2 (DEV-1743). + + +def _nbytes(s: str) -> int: + return len(s.encode("utf-8")) + + +def _pg_effective(name: str, *, quoted: bool) -> str: + """What Postgres resolves an identifier to: truncate to 63 bytes, case-fold if unquoted.""" + clipped = name.encode("utf-8")[:63].decode("utf-8", "ignore") + return clipped if quoted else clipped.lower() + + +def _projection_aliases(select: exp.Select) -> list[tuple[str, bool]]: + """(name, quoted) for each output column of one SELECT scope.""" + names: list[tuple[str, bool]] = [] + for proj in select.expressions: + if isinstance(proj, exp.Alias) and isinstance(proj.args.get("alias"), exp.Identifier): + ident = proj.args["alias"] + names.append((ident.this, bool(ident.quoted))) + elif isinstance(proj, exp.Column) and isinstance(proj.this, exp.Identifier): + names.append((proj.this.this, bool(proj.this.quoted))) + return names + + +def _order_by_refs(select: exp.Select) -> list[tuple[str, bool]]: + """(name, quoted) for each column referenced in one SELECT's ORDER BY.""" + order = select.args.get("order") + if order is None: + return [] + return [ + (col.this.this, bool(col.this.quoted)) + for col in order.find_all(exp.Column) + if isinstance(col.this, exp.Identifier) + ] + + +def _cte_names(tree: exp.Expression) -> list[tuple[str, bool]]: + return [ + (c.alias_or_name, bool(getattr(c.args.get("alias"), "quoted", False))) + for c in tree.find_all(exp.CTE) + ] + + +def _cte_table_refs(tree: exp.Expression) -> set[str]: + """Names of every table reference, to match a CTE reference to its definition exactly.""" + return { + t.this.this + for t in tree.find_all(exp.Table) + if isinstance(t.this, exp.Identifier) + } + + +def _inscope_identifiers(sql: str, dialect: str = "postgres") -> list[tuple[str, bool]]: + """Every identifier in a namespace this issue owns (see scope note).""" + tree = sqlglot.parse_one(sql, dialect=dialect) + out: list[tuple[str, bool]] = [] + for select in tree.find_all(exp.Select): + out.extend(_projection_aliases(select)) + out.extend(_order_by_refs(select)) + out.extend(_cte_names(tree)) + return out + + +def _assert_within_limit(sql: str, limit: int, dialect: str = "postgres") -> None: + for name, _ in _inscope_identifiers(sql, dialect): + assert _nbytes(name) <= limit, f"{name!r} is {_nbytes(name)} bytes\n{sql}" + + +def _assert_no_namespace_collision(sql: str, dialect: str = "postgres") -> None: + """Identifiers in each namespace stay distinct after the backend normalizes them.""" + tree = sqlglot.parse_one(sql, dialect=dialect) + namespaces: list[tuple[str, list[tuple[str, bool]]]] = [ + (f"select#{i}", _projection_aliases(select)) + for i, select in enumerate(tree.find_all(exp.Select)) + ] + namespaces.append(("cte", _cte_names(tree))) + for ns, names in namespaces: + seen: dict[str, str] = {} + for name, quoted in names: + eff = _pg_effective(name, quoted=quoted) + prior = seen.get(eff) + assert prior is None or prior == name, ( + f"{ns}: {prior!r} and {name!r} both normalize to {eff!r}\n{sql}" + ) + seen[eff] = name + + +def _assert_order_by_refs_resolve(sql: str, dialect: str = "postgres") -> None: + """Every quoted ORDER BY reference must name a projection alias that exists.""" + tree = sqlglot.parse_one(sql, dialect=dialect) + projected = {n for select in tree.find_all(exp.Select) for n, _ in _projection_aliases(select)} + for select in tree.find_all(exp.Select): + for name, quoted in _order_by_refs(select): + if quoted: + assert name in projected, ( + f"ORDER BY references {name!r}, which no SELECT projects\n{sql}" + ) + + +async def _sql(engine, model, query, *, dialect: str = "postgres", mode: str = "outer") -> str: + enriched = await engine._enrich(query=query, model=model) + return SQLGenerator(dialect=dialect).generate(enriched=enriched, render_mode=mode) + + +# The premise: without the fix these aliases really do collide + + +class TestPremise: + async def test_canonical_aliases_are_over_the_limit(self, chain) -> None: + engine, model = chain + enriched = await engine._enrich(query=_repro_query(), model=model) + aliases = public_projection_aliases(enriched) + assert LONG_NAME in aliases + assert LONG_EMAIL in aliases + assert _nbytes(LONG_NAME) == 73 + assert _nbytes(LONG_EMAIL) == 74 + + def test_the_two_aliases_share_a_63_byte_prefix(self) -> None: + assert LONG_NAME.encode()[:63] == LONG_EMAIL.encode()[:63] + + +class TestProjectionAliases: + async def test_repro_emits_no_over_limit_identifier(self, chain) -> None: + engine, model = chain + _assert_within_limit(await _sql(engine, model, _repro_query()), 63) + + async def test_repro_has_no_namespace_collision(self, chain) -> None: + engine, model = chain + _assert_no_namespace_collision(await _sql(engine, model, _repro_query())) + + async def test_repro_still_parses(self, chain) -> None: + engine, model = chain + sql = await _sql(engine, model, _repro_query()) + assert len(sqlglot.parse(sql, dialect="postgres")) == 1 + + async def test_outer_wrap_uses_the_same_token_everywhere(self, chain) -> None: + """Inner ``AS``, outer-wrap projection and ORDER BY carry the identical token.""" + engine, model = chain + sql = await _sql(engine, model, _repro_query(with_order=True)) + assert ") AS _outer" in sql, "outer wrap did not fire; test is vacuous" + tree = sqlglot.parse_one(sql, dialect="postgres") + selects = list(tree.find_all(exp.Select)) + outer_names = {n for n, _ in _projection_aliases(selects[0])} + inner_names = {n for n, _ in _projection_aliases(selects[1])} + fitted = get_dialect("postgres").fit_alias(LONG_EMAIL) + assert fitted in outer_names + assert fitted in inner_names + _assert_within_limit(sql, 63) + _assert_no_namespace_collision(sql) + _assert_order_by_refs_resolve(sql) + + async def test_canonical_alias_does_not_survive_anywhere(self, chain) -> None: + """Pairing check: no canonical alias survives — stronger than a max-length assertion.""" + engine, model = chain + sql = await _sql(engine, model, _repro_query(with_order=True)) + assert LONG_NAME not in sql + assert LONG_EMAIL not in sql + + @pytest.mark.parametrize("dialect", sorted(GOLDEN_SHORT_QUERY)) + async def test_under_limit_query_is_byte_identical(self, chain, dialect: str) -> None: + """No churn for the common case, pinned against pre-feature SQL.""" + engine, model = chain + assert await _sql(engine, model, SHORT_QUERY, dialect=dialect) == GOLDEN_SHORT_QUERY[dialect] + + async def test_unbounded_dialect_output_is_byte_identical(self, chain) -> None: + """SQLite has no limit, so even the full repro is untouched byte for byte.""" + engine, model = chain + sql = await _sql(engine, model, _repro_query(with_order=True), dialect="sqlite") + assert sql == GOLDEN_SQLITE_REPRO_ORDER + + @pytest.mark.parametrize("dialect", ["clickhouse", "trino", "databricks"]) + async def test_other_unbounded_dialects_keep_the_long_alias(self, chain, dialect: str) -> None: + engine, model = chain + sql = await _sql(engine, model, _repro_query(with_order=True), dialect=dialect) + assert sql.count(LONG_EMAIL) == 2 # inner AS + outer projection, both untouched + + async def test_wrapped_render_mode_also_fitted(self, chain) -> None: + """``render_mode='wrapped'`` inner aliases are subject to truncation too.""" + engine, model = chain + _assert_within_limit(await _sql(engine, model, _repro_query(), mode="wrapped"), 63) + + +class TestManglingDialects: + """BigQuery / T-SQL mangle dotted aliases; length-fitting must compose with that.""" + + @pytest.mark.parametrize("dialect,limit", [("bigquery", 300), ("tsql", 128)]) + async def test_long_alias_is_mangled_and_fitted(self, chain, dialect: str, limit: int) -> None: + engine, model = chain + sql = await _sql(engine, model, _repro_query(), dialect=dialect) + _assert_within_limit(sql, limit, dialect=dialect) + assert LONG_EMAIL not in sql + + async def test_emit_alias_matches_what_is_in_the_sql(self, chain) -> None: + """``emit_alias`` builds the decode map, so it must be the token the SQL carries.""" + engine, model = chain + for dialect in ("postgres", "bigquery", "tsql", "mysql"): + sql = await _sql(engine, model, _repro_query(), dialect=dialect) + names = {n for n, _ in _inscope_identifiers(sql, dialect)} + assert get_dialect(dialect).emit_alias(LONG_EMAIL) in names, dialect + + async def test_mangling_is_not_applied_twice(self, chain) -> None: + """The length pass runs before the dot-mangle regex; it must not double-encode.""" + engine, model = chain + sql = await _sql(engine, model, _repro_query(), dialect="bigquery") + fitted = get_dialect("bigquery").fit_alias(LONG_EMAIL) + assert encode_alias(fitted) in sql + assert encode_alias(encode_alias(fitted)) not in sql + + +class TestDecodeResultKeys: + def test_shortened_keys_restored_to_canonical(self) -> None: + pg = get_dialect("postgres") + rows = [{pg.emit_alias(LONG_EMAIL): "a@b.io", "SandboxInvoiceV2.status": "paid"}] + got = pg.decode_result_keys(rows, aliases=[LONG_EMAIL, "SandboxInvoiceV2.status"]) + assert got == [{LONG_EMAIL: "a@b.io", "SandboxInvoiceV2.status": "paid"}] + + def test_identity_when_nothing_shortened(self) -> None: + rows = [{"orders.status": "paid"}] + assert get_dialect("postgres").decode_result_keys(rows, aliases=["orders.status"]) == rows + + def test_identity_without_aliases(self) -> None: + rows = [{"whatever": 1}] + assert get_dialect("postgres").decode_result_keys(rows) == rows + + def test_unknown_key_passes_through(self) -> None: + rows = [{"surprise": 1}] + assert get_dialect("postgres").decode_result_keys(rows, aliases=[LONG_EMAIL]) == rows + + def test_empty_rows(self) -> None: + assert get_dialect("postgres").decode_result_keys([], aliases=[LONG_EMAIL]) == [] + + def test_hidden_alias_is_decoded_too(self) -> None: + """A hidden ORDER-BY hoist can appear in a row and must decode like any other.""" + pg = get_dialect("postgres") + hidden = LONG_EMAIL.replace(".email", ".totalAmount_avg") + rows = [{pg.emit_alias(hidden): 1.0}] + assert pg.decode_result_keys(rows, aliases=[hidden]) == [{hidden: 1.0}] + + def test_bigquery_reverses_both_manglings(self) -> None: + bq = get_dialect("bigquery") + long_dotted = ".".join(["Sandbox" * 6] * 8) + rows = [{bq.emit_alias(long_dotted): 1}] + assert bq.decode_result_keys(rows, aliases=[long_dotted]) == [{long_dotted: 1}] + + def test_bigquery_falls_back_to_dot_decode_outside_the_map(self) -> None: + bq = get_dialect("bigquery") + assert bq.decode_result_keys([{"orders___status": 1}], aliases=[]) == [{"orders.status": 1}] + + +class TestDecodeWiring: + """Decode must receive the full alias set, hidden entries included.""" + + async def test_run_and_build_passes_all_projection_aliases( + self, chain, monkeypatch: pytest.MonkeyPatch, + ) -> None: + from slayer.sql.dialects.postgres import PostgresDialect + + engine, _ = chain + prepared = await engine._prepare_pipeline( + query=_repro_query(with_order=True), named_queries={}, runtime_kwarg={}, + ) + expected = all_projection_aliases(prepared.enriched) + hidden = [a for a in expected if a not in public_projection_aliases(prepared.enriched)] + assert hidden, "fixture must produce a hidden alias or this is vacuous" + + seen: dict[str, object] = {} + + def _spy(self, rows, *, aliases=()): + seen["aliases"] = list(aliases) + return rows + + monkeypatch.setattr(PostgresDialect, "decode_result_keys", _spy) + + class _FakeClient: + async def execute(self, sql): + return [] + + await engine._run_and_build(prepared=prepared, client=_FakeClient()) + assert seen["aliases"] == expected + + +class TestAllProjectionAliases: + async def test_includes_public_aliases(self, chain) -> None: + engine, model = chain + enriched = await engine._enrich(query=_repro_query(), model=model) + every = all_projection_aliases(enriched) + for a in public_projection_aliases(enriched): + assert a in every + + async def test_includes_hidden_order_by_hoist(self, chain) -> None: + """``totalAmount:avg`` is projected only for ORDER BY; the pass must still see it.""" + engine, model = chain + enriched = await engine._enrich(query=_repro_query(with_order=True), model=model) + public = public_projection_aliases(enriched) + hidden = [a for a in all_projection_aliases(enriched) if a not in public] + assert any("totalAmount_avg" in a for a in hidden), hidden + + async def test_is_stable_across_calls(self, chain) -> None: + """Order must be stable — the rewrite map is derived from this list.""" + engine, model = chain + enriched = await engine._enrich(query=_repro_query(), model=model) + first = all_projection_aliases(enriched) + assert first == [ + LONG_NAME, LONG_EMAIL, "SandboxInvoiceV2.status", + "SandboxInvoiceV2.totalAmount_sum", "SandboxInvoiceV2._count", + ] + assert all_projection_aliases(enriched) == first + + +# CTE names (unquoted namespace) + + +def _deep_cross_model_query(*, two_measures: bool = False) -> SlayerQuery: + measures = [ModelMeasure(formula=f"{DEEP}.lifetimeValue:sum")] + if two_measures: + measures.append(ModelMeasure(formula=f"{DEEP}.lifetimeValue:avg")) + return SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name="status")], + measures=measures, + ) + + +class TestCteNames: + async def test_cross_model_cte_name_within_limit(self, chain) -> None: + engine, model = chain + sql = await _sql(engine, model, _deep_cross_model_query()) + assert "_cm_" in sql, "no cross-model CTE was generated; test is vacuous" + _assert_within_limit(sql, 63) + + async def test_cte_definition_and_references_agree(self, chain) -> None: + """A CTE name is unquoted; a truncated definition and untruncated reference must agree.""" + engine, model = chain + sql = await _sql(engine, model, _deep_cross_model_query()) + tree = sqlglot.parse_one(sql, dialect="postgres") + defined = [n for n, _ in _cte_names(tree)] + assert defined, "no CTE generated; test is vacuous" + referenced = _cte_table_refs(tree) + for name in defined: + assert _nbytes(name) <= 63 + assert name in referenced, f"CTE {name!r} defined but never referenced\n{sql}" + + async def test_two_deep_cross_model_ctes_stay_distinct(self, chain) -> None: + """Two over-limit cross-model CTE names must fit and stay distinct after truncation.""" + engine, model = chain + sql = await _sql(engine, model, _deep_cross_model_query(two_measures=True)) + tree = sqlglot.parse_one(sql, dialect="postgres") + names = [n for n, _ in _cte_names(tree)] + assert len(names) >= 2, f"expected two cross-model CTEs\n{sql}" + effective = [_pg_effective(n, quoted=False) for n in names] + assert len(set(effective)) == len(effective), effective + _assert_within_limit(sql, 63) + + def test_cte_namespace_collision_raises(self, monkeypatch: pytest.MonkeyPatch) -> None: + """A forced digest collision in the CTE namespace must raise, not emit duplicate names.""" + import slayer.sql.dialects._identifier_fit as fitmod + + gen = SQLGenerator(dialect="postgres") + monkeypatch.setattr(fitmod, "_digest", lambda name: "deadbeef") + first = gen._cte_name("_cm_", TWIN_A) + assert _nbytes(first) <= 63 + with pytest.raises(IdentifierCollisionError) as exc: + gen._cte_name("_cm_", TWIN_B) + assert "CTE name" in str(exc.value) + + def test_cte_allocator_is_idempotent_for_the_same_owner(self) -> None: + """Re-deriving a CTE's name for the same owner must not read as a collision.""" + gen = SQLGenerator(dialect="postgres") + first = gen._cte_name("_cm_", TWIN_A) + second = gen._cte_name("_cm_", TWIN_A) # hits the memo, must not raise + assert second == first + + async def test_cte_allocator_resets_per_statement(self, chain) -> None: + """CTE allocation is per-statement; ``generate()`` clears an owner allocated before it.""" + engine, _ = chain + prepared = await engine._prepare_pipeline( + query=_repro_query(), named_queries={}, runtime_kwarg={}, + ) + gen = SQLGenerator(dialect="postgres") + stale = gen._cte_name(prefix="_cm_", alias=TWIN_A) + assert stale.casefold() in gen._cte_names + + gen.generate(enriched=prepared.enriched) + assert stale.casefold() not in gen._cte_names, ( + "CTE allocation leaked across statements" + ) + + def test_cte_allocator_detects_case_folded_collision(self) -> None: + """CTE names are unquoted and case-folded, so ``_wm_Foo`` and ``_wm_foo`` collide.""" + gen = SQLGenerator(dialect="postgres") + first = gen._cte_name(prefix="_wm_", alias="Foo") + with pytest.raises(IdentifierCollisionError) as exc: + gen._cte_name(prefix="_wm_", alias="foo") + assert "CTE name" in str(exc.value) + assert first == "_wm_Foo" + + def test_cte_name_helper_is_pure_and_bounded(self) -> None: + from slayer.sql.generator import _cte_name_from_alias + + long_alias = LONG_EMAIL + ".lifetimeValue_sum" + a = _cte_name_from_alias("_cm_", long_alias, limit=63) + b = _cte_name_from_alias("_cm_", long_alias, limit=63) + assert a == b + assert _nbytes(a) <= 63 + + def test_cte_name_helper_counts_the_prefix(self) -> None: + from slayer.sql.generator import _cte_name_from_alias + + assert _nbytes(_cte_name_from_alias("cp_value_12_", "a" * 60, limit=63)) <= 63 + + def test_cte_name_helper_unbounded(self) -> None: + """``limit=None`` keeps today's behaviour exactly.""" + from slayer.sql.generator import _cte_name_from_alias + + assert _cte_name_from_alias("_cm_", "a" * 200, limit=None) == "_cm_" + "a" * 200 + + def test_cte_name_helper_still_sanitizes(self) -> None: + from slayer.sql.generator import _cte_name_from_alias + + assert _cte_name_from_alias("_cm_", "a.b", limit=None) == "_cm_a__b" + + def test_distinct_long_aliases_get_distinct_cte_names(self) -> None: + from slayer.sql.generator import _cte_name_from_alias + + a = _cte_name_from_alias("_cm_", LONG_NAME + ".v_sum", limit=63) + b = _cte_name_from_alias("_cm_", LONG_EMAIL + ".v_sum", limit=63) + assert a != b + + def test_cte_name_is_a_legal_unquoted_identifier(self) -> None: + import re + + from slayer.sql.generator import _cte_name_from_alias + + got = _cte_name_from_alias("_cm_", LONG_EMAIL + ".lifetimeValue_sum", limit=63) + assert re.match(r"^[A-Za-z_][A-Za-z0-9_]*$", got), got + + async def test_self_join_transform_cte_names_are_fitted(self, tmp_path) -> None: + """``shifted_``/``sjoin_`` CTE names come from a user transform name and must fit too.""" + long_transform_name = "revenue_" + "x" * 70 # 78 chars, way over 63 + storage = YAMLStorage(base_dir=str(tmp_path)) + await storage.save_datasource(DatasourceConfig( + name=DS, type="postgres", host="localhost", port=5432, + database="x", username="u", password="p", + )) + await storage.save_model(SlayerModel( + name="ShiftOrders", sql_table="orders", data_source=DS, + default_time_dimension="created_at", + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name="created_at", sql="created_at", type=DataType.TIMESTAMP), + Column(name="revenue", sql="revenue", type=DataType.DOUBLE), + ], + )) + engine = SlayerQueryEngine(storage=storage) + prepared = await engine._prepare_pipeline( + query=SlayerQuery( + source_model="ShiftOrders", + time_dimensions=[{"dimension": {"name": "created_at"}, "granularity": "month"}], + measures=[ + {"formula": "revenue:sum"}, + {"formula": "time_shift(revenue:sum, -1)", "name": long_transform_name}, + ], + ), + named_queries={}, runtime_kwarg={}, + ) + sql = prepared.sql + assert "shifted_" in sql, f"fixture must emit a self-join CTE\n{sql}" + for name, _ in _cte_names(sqlglot.parse_one(sql, dialect="postgres")): + assert _nbytes(name) <= 63, f"CTE {name!r} is {_nbytes(name)} bytes\n{sql}" + # The unfitted forms must not survive — definition or reference. + assert f"shifted_{long_transform_name}" not in sql + assert f"sjoin_{long_transform_name}" not in sql + + +def _wrapper_select(vm_sql: str) -> exp.Select: + """The outermost SELECT of a virtual model's SQL — the rename wrapper.""" + return next(iter(sqlglot.parse_one(vm_sql, dialect="postgres").find_all(exp.Select))) + + +class TestVirtualModelShorts: + async def test_short_within_limit_and_matches_column_name(self, chain) -> None: + engine, _ = chain + vm = await engine._query_as_model(inner_query=_repro_query()) + emitted = {n for n, _ in _projection_aliases(_wrapper_select(vm.sql))} + for col in vm.columns: + assert _nbytes(col.name) <= 63, col.name + assert col.name in emitted, ( + f"virtual-model column {col.name!r} is not an alias the wrapper " + f"emits\n{vm.sql}" + ) + + async def test_long_siblings_get_distinct_shorts(self, chain) -> None: + engine, _ = chain + vm = await engine._query_as_model(inner_query=_repro_query()) + names = [c.name for c in vm.columns] + assert len(names) == len(set(names)) + + async def test_short_alias_quoting_matches_the_downstream_reference( + self, chain, + ) -> None: + """The wrapper's ``AS `` is quoted exactly when the downstream reference is.""" + engine, _ = chain + vm = await engine._query_as_model(inner_query=_repro_query()) + gen = SQLGenerator(dialect="postgres") + select = _wrapper_select(vm.sql) + assert select.expressions, vm.sql + by_short = {c.name: c for c in vm.columns} + for proj in select.expressions: + assert isinstance(proj, exp.Alias), f"{proj.sql()} is not an aliased projection" + ident = proj.args.get("alias") + assert isinstance(ident, exp.Identifier), f"{ident} is not an Identifier" + short = ident.this + assert short in by_short, f"{short!r} is not a virtual-model column\n{vm.sql}" + # How the generator will emit a downstream reference to this column. + ref = gen._parse(by_short[short].sql).find(exp.Column) + assert ref is not None + assert ident.quoted == ref.this.quoted, ( + f"short {short!r} is emitted {'quoted' if ident.quoted else 'bare'} " + f"in the wrapper but referenced " + f"{'quoted' if ref.this.quoted else 'bare'} downstream — a " + f"case-folding backend resolves those to different columns\n{vm.sql}" + ) + + async def test_mixed_case_short_is_quoted(self, chain) -> None: + """The original DEV-1756 defect: a mixed-case short emitted bare.""" + engine, _ = chain + vm = await engine._query_as_model(inner_query=_repro_query()) + mixed = [ + proj for proj in _wrapper_select(vm.sql).expressions + if isinstance(proj, exp.Alias) + and any(c.isupper() for c in proj.args["alias"].this) + ] + assert mixed, f"fixture should produce mixed-case shorts\n{vm.sql}" + for proj in mixed: + assert proj.args["alias"].quoted, ( + f"mixed-case short {proj.args['alias'].this!r} must be quoted " + f"or Postgres folds it out from under the reference\n{vm.sql}" + ) + + @pytest.mark.parametrize("dialect", [ + "postgres", "mysql", "snowflake", "tsql", "bigquery", "duckdb", + "sqlite", "clickhouse", "redshift", "oracle", "trino", "spark", + ]) + @pytest.mark.parametrize("short", [ + "status", # plain lowercase -> bare on both sides + "SandboxConsumer__name", # mixed case -> quoted on both sides + "order", # reserved in SLayer's common set + "index", # reserved NATIVELY in mysql/tsql, not in ours + "int", # ditto + "rows", # ditto (also bigquery) + "_fit_a1b2c3d4_tail", # the shape fit_identifier emits + ]) + def test_short_spelling_matches_the_reference_on_every_dialect( + self, dialect: str, short: str, + ) -> None: + """``AS `` spelling must match a downstream reference on every dialect (case + reserved).""" + gen = SQLGenerator(dialect=dialect) + ident = exp.Identifier(this=short, quoted=False) + SQLGenerator._maybe_quote_ident(ident) + wrapper = ident.sql(dialect=gen.dialect) + reference = gen._parse(short).sql(dialect=gen.dialect) + assert wrapper == reference, ( + f"[{dialect}] wrapper emits {wrapper!r} but a downstream reference " + f"to the same column emits {reference!r}" + ) + + async def test_lowercase_short_stays_bare(self, chain) -> None: + """Mirror image: an all-lowercase short must stay bare on upper-folding backends.""" + engine, _ = chain + vm = await engine._query_as_model(inner_query=_repro_query()) + lower = [ + proj for proj in _wrapper_select(vm.sql).expressions + if isinstance(proj, exp.Alias) + and not any(c.isupper() for c in proj.args["alias"].this) + ] + assert lower, f"fixture should produce lowercase shorts\n{vm.sql}" + for proj in lower: + assert not proj.args["alias"].quoted, ( + f"lowercase short {proj.args['alias'].this!r} must stay bare\n{vm.sql}" + ) + + async def test_inner_and_wrapper_agree_on_the_fitted_alias(self, chain) -> None: + engine, _ = chain + vm = await engine._query_as_model(inner_query=_repro_query()) + fitted = get_dialect("postgres").fit_alias(LONG_EMAIL) + select = _wrapper_select(vm.sql) + sources = { + proj.this.this.this + for proj in select.expressions + if isinstance(proj, exp.Alias) + and isinstance(proj.this, exp.Column) + and isinstance(proj.this.this, exp.Identifier) + } + assert fitted in sources, f"wrapper does not reference the fitted alias\n{vm.sql}" + assert LONG_EMAIL not in vm.sql + + async def test_case_colliding_shorts_raise(self, tmp_path) -> None: + """``email`` and ``Email`` yield shorts differing only by case; must be caught.""" + engine, _ = await _build_engine(tmp_path, case_colliding_columns=True) + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ + ColumnRef(name=f"{DEEP}.email"), + ColumnRef(name=f"{DEEP}.Email"), + ], + measures=[{"formula": "*:count"}], + ) + with pytest.raises(IdentifierCollisionError): + await engine._query_as_model(inner_query=query) + + async def test_two_dimensions_landing_on_one_short_raise(self, tmp_path) -> None: + """Two dimensions whose shorts are exactly equal must be caught at the emission boundary.""" + flat = "SandboxSubscription__SandboxCustomer__SandboxConsumer__name" + engine, _ = await _build_engine(tmp_path, decoy_root_column=flat) + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.name"), ColumnRef(name=flat)], + measures=[{"formula": "*:count"}], + ) + with pytest.raises(IdentifierCollisionError) as exc: + await engine._query_as_model(inner_query=query) + assert "query-backed model column" in str(exc.value) + assert flat in str(exc.value) + + async def test_caller_supplied_measure_names_are_fitted(self, chain) -> None: + """A caller-supplied measure ``name`` bypasses ``_alias_to_short`` and needs its own fitting.""" + engine, _ = chain + long_a = "z" * 63 + "b" # 64 bytes + long_b = "z" * 63 + "c" # 64 bytes, identical first 63 + assert long_a[:63] == long_b[:63] + query = SlayerQuery( + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name="status")], + measures=[ + {"formula": "totalAmount:sum", "name": long_a}, + {"formula": "totalAmount:avg", "name": long_b}, + ], + ) + vm = await engine._query_as_model(inner_query=query) + names = [c.name for c in vm.columns] + for name in names: + assert _nbytes(name) <= 63, f"{name!r} is {_nbytes(name)} bytes" + # The real defect: distinct after the server's 63-byte truncation. + truncated = [n.encode()[:63] for n in names] + assert len(set(truncated)) == len(truncated), ( + f"two shorts collapse onto one 63-byte name: {names}" + ) + assert long_a not in vm.sql, vm.sql + assert long_b not in vm.sql, vm.sql + + async def test_nested_dag_two_levels_agree(self, chain) -> None: + """Two stages: stage 2 references stage 1's virtual-model columns.""" + engine, _ = chain + stage1 = _repro_query().model_copy(update={"name": "stage1"}) + vm = await engine._query_as_model(inner_query=stage1, override_name="stage1") + short = next(c.name for c in vm.columns if "email" in c.name.lower()) + stage2 = SlayerQuery( + source_model="stage1", + dimensions=[ColumnRef(name=short)], + measures=[{"formula": "totalAmount_sum:sum"}], + ) + resp = await engine.execute(query=[stage1, stage2], dry_run=True) + _assert_within_limit(resp.sql, 63) + _assert_no_namespace_collision(resp.sql) + _assert_order_by_refs_resolve(resp.sql) + + +# Engine-level contract: consumers never see the shortened form + + +class TestEngineContract: + async def test_dry_run_columns_stay_canonical(self, chain) -> None: + engine, _ = chain + resp = await engine.execute(query=_repro_query(), dry_run=True) + assert LONG_NAME in resp.columns + assert LONG_EMAIL in resp.columns + + async def test_dry_run_sql_carries_the_shortened_form(self, chain) -> None: + engine, _ = chain + resp = await engine.execute(query=_repro_query(), dry_run=True) + assert LONG_EMAIL not in resp.sql + assert get_dialect("postgres").fit_alias(LONG_EMAIL) in resp.sql + + async def test_attribute_keys_are_result_keys(self, chain) -> None: + engine, _ = chain + resp = await engine.execute(query=_repro_query(), dry_run=True) + for key in resp.attributes.dimensions: + assert key in resp.columns + + async def test_get_column_types_decodes_fitted_aliases( + self, chain, monkeypatch: pytest.MonkeyPatch, + ) -> None: + """``get_column_types`` probe keys are fitted aliases and must decode back to canonical.""" + engine, _ = chain + over_limit = "totalAmount_" + "x" * 60 # 72-char measure name + + captured: dict[str, str] = {} + + class _FakeClient: + async def get_column_types(self, sql: str) -> dict[str, str]: + # Echo back server keys exactly as emitted. + for name, _ in _projection_aliases( + next(iter(sqlglot.parse_one(sql, dialect="postgres").find_all(exp.Select))) + ): + captured[name] = "double precision" + return dict(captured) + + async def aclose(self) -> None: # pragma: no cover — not reached + pass + + model = SlayerModel( + name="TypeProbe", sql_table="invoices", data_source=DS, + columns=[ + Column(name="id", sql="id", type=DataType.INT, primary_key=True), + Column(name=over_limit, sql="total_amount", type=DataType.DOUBLE), + ], + ) + await engine.storage.save_model(model) + monkeypatch.setattr( + engine, "_sql_clients", {k: _FakeClient() for k in ("x",)}, + ) + monkeypatch.setattr( + "slayer.engine.query_engine._sql_client_cache_key", lambda ds: "x", + ) + + types = await engine.get_column_types(model_name="TypeProbe") + assert captured, "probe must have emitted at least one alias" + emitted = [k for k in captured if _nbytes(k) > 63] + assert not emitted, f"probe SQL should carry fitted aliases, got {emitted}" + assert over_limit in types, ( + f"over-limit measure dropped from the type map; probe keys were " + f"{sorted(captured)}" + ) + + +# Sweep: every generator shape, not just the reported one + + +class TestSweep: + """Assert pairing (no canonical over-limit alias survives) across every SQL shape.""" + + @pytest.fixture + def queries(self) -> list[SlayerQuery]: + return [ + _repro_query(), # plain dims + measures + _repro_query(with_order=True), # ORDER BY hoist + outer wrap + _deep_cross_model_query(), # cross-model CTE (_cm_ + WITH) + SlayerQuery( # arithmetic expression + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.email")], + measures=[ + {"formula": "totalAmount:sum"}, + {"formula": "totalAmount:sum / *:count", "name": "avg_ticket"}, + ], + ), + SlayerQuery( # filter on a long dotted dim + source_model="SandboxInvoiceV2", + dimensions=[ColumnRef(name=f"{DEEP}.name")], + measures=[{"formula": "*:count"}], + filters=[f"{DEEP}.email IS NOT NULL"], + ), + ] + + async def test_no_over_limit_identifier_anywhere(self, chain, queries) -> None: + engine, model = chain + for q in queries: + _assert_within_limit(await _sql(engine, model, q), 63) + + async def test_no_namespace_collision_anywhere(self, chain, queries) -> None: + engine, model = chain + for q in queries: + _assert_no_namespace_collision(await _sql(engine, model, q)) + + async def test_order_by_refs_resolve_anywhere(self, chain, queries) -> None: + engine, model = chain + for q in queries: + _assert_order_by_refs_resolve(await _sql(engine, model, q)) + + async def test_no_canonical_over_limit_alias_survives(self, chain, queries) -> None: + engine, model = chain + for q in queries: + enriched = await engine._enrich(query=q, model=model) + sql = SQLGenerator(dialect="postgres").generate(enriched=enriched) + over = [a for a in all_projection_aliases(enriched) if _nbytes(a) > 63] + assert over, "sweep entry has no over-limit alias; it proves nothing" + for alias in over: + assert alias not in sql, ( + f"canonical alias {alias!r} still present — some emission " + f"site was not rewritten\n{sql}" + ) diff --git a/tests/test_query_backed_models.py b/tests/test_query_backed_models.py index 105fcb57..33f0e593 100644 --- a/tests/test_query_backed_models.py +++ b/tests/test_query_backed_models.py @@ -1418,15 +1418,15 @@ async def test_inner_stage_aggregated_measure_honors_user_name(self) -> None: f"expected 'rev_sum' in cached columns, got: {col_names}" ) sql = loaded.backing_query_sql or "" - # Inner-stage wrap renames `"orders.rev" AS rev`; loose match on - # the alias keyword + name (newline-tolerant). + # Inner-stage wrap renames `"orders.rev" AS rev`; an all-lowercase + # short stays bare so it folds like the bare downstream reference. import re assert re.search(r"\bAS\s+rev\b", sql), ( f"expected inner-stage 'AS rev' rename in SQL:\n{sql}" ) # The canonical name must not leak into the wrapped subquery's # exposed alias. - assert not re.search(r"\bAS\s+amount_sum\b", sql), ( + assert not re.search(r'\bAS\s+"?amount_sum"?', sql), ( f"canonical 'amount_sum' must not be the surfaced inner alias:\n{sql}" ) finally: @@ -1491,6 +1491,7 @@ async def test_query_as_model_emits_user_alias_unit(self) -> None: f"'name', got: {col_names}" ) import re + # DEV-1756: an all-lowercase short stays bare (see above). assert re.search(r"\bAS\s+rev\b", virtual.sql), ( f"wrapped SQL must rename to user alias 'rev':\n{virtual.sql}" ) diff --git a/tests/test_sql_generator.py b/tests/test_sql_generator.py index 1498ea98..5572946a 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -7017,9 +7017,9 @@ def trim_spy(self, *, sql, enriched): call_order.append("trim") return real_trim(self, sql=sql, enriched=enriched) - def rewrite_spy(self, sql): + def rewrite_spy(self, sql, **kwargs): call_order.append("rewrite") - return real_rewrite(self, sql) + return real_rewrite(self, sql, **kwargs) with patch.object(SQLGenerator, "_apply_outer_projection_trim", trim_spy), \ patch.object(BigqueryDialect, "rewrite_emitted_sql", rewrite_spy):