fix(DEV-1756): bound generated identifiers to the dialect's length limit - #289
Merged
ZmeiGorynych merged 12 commits intoAug 11, 2026
Conversation
Postgres caps identifiers at 63 bytes and truncates past it SILENTLY (a NOTICE, never an error). SLayer's alias convention `<root_model>.<join.path>.<column>` crosses that on a 3-hop join, so two sibling aliases collapse onto one output name: SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.name 73 B SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxConsumer.email 74 B Under the DEV-1444 outer wrap that raises AmbiguousColumnError; with no sibling to collide with the query succeeds and the column silently disappears, which is the worse case. Every dialect now declares a conservative `max_identifier_bytes` budget, and an over-limit identifier is fitted at emission to `<head>_<hash8>_<tail>` (new `slayer/sql/dialects/_identifier_fit.py`). Fitting is a pure function of the full original name, so the read side rebuilds the emitted->canonical map by re-running it — nothing is threaded through generation — and `decode_result_keys` restores the canonical dotted alias. Consumers never see the shortened form; only `response.sql` does, because that is the SQL that ran. Shortened only when over the limit, so under-limit output stays byte-identical (pinned by pre-change goldens, not merely by idempotence). Both ends of the alias survive because the reported colliding pair differs only in its final segment. The write pass is an exact-match replacement over the query's own alias set (`all_projection_aliases`, unfiltered — hidden ORDER-BY hoists 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 cannot be corrupted. Substitution is two-phase as defence-in-depth. BigQuery/T-SQL run their existing dot-mangle regex after the length pass: an under-limit alias makes that pass a no-op so their output is unchanged, and an over-limit one arrives still-dotted and is mangled by the same regex — no double-encoding — with the budget sized against `encode_alias`. Covers the three output-name surfaces: projection aliases, CTE names (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, which already owns the `__` path-alias allocator. 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. Also fixes a pre-existing bug in the same code path, reproduced on a live server: `_query_as_model` emitted its short alias bare, so Postgres case-folded it while the outer stage referenced it quoted, making any query-backed model with a mixed-case join path unqueryable (UndefinedColumnError). It is now always dialect-quoted, which additionally takes these names out of the case-folding namespace. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds dialect-aware fitting for projection aliases, CTE names, and query-backed model aliases. It detects collisions, rewrites generated SQL, restores canonical result keys, and documents database-specific identifier limits. ChangesIdentifier fitting and alias preservation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SqlGenerator
participant SqlDialect
participant PostgreSQL
participant QueryEngine
SqlGenerator->>SqlDialect: Rewrite SQL with projection aliases
SqlDialect-->>SqlGenerator: Emit fitted aliases
SqlGenerator->>PostgreSQL: Execute generated SQL
PostgreSQL-->>QueryEngine: Return rows with emitted keys
QueryEngine->>SqlDialect: Decode result keys with aliases
SqlDialect-->>QueryEngine: Restore canonical dotted keys
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Review findings from Codex on PR #289, all in the same family: a collision check that could not observe the collision it was meant to catch. 1. `_query_as_model` keyed its short-name allocation by the short itself (`prior != short`), so two DISTINCT aliases landing on the IDENTICAL short compared equal and passed. Key by the owning inner alias instead, which catches exact duplicates as well as the case-only and fitted-form variants it already caught. Enrichment has a related guard, but it does not cover this: its `_occupied_shorts` map is populated from dimensions and checked only against MEASURES, so dimension-vs-dimension slips through, and its local flattening helper applies no length fitting, so two shorts that differ before fitting and collide after it pass there too. A root column named `SandboxSubscription__SandboxCustomer__SandboxConsumer__name` alongside the deep dimension that flattens to the same string reaches the emission boundary undetected — column names permit `__`, so this is reachable. 2/3. BigQuery and T-SQL `decode_result_keys` pre-decoded row keys in a dict comprehension and only then passed the result to `_rekey_row`. Two keys normalizing to the same name collapsed in that intermediate dict, with one value silently dropped, before the duplicate check ever ran — the exact failure mode this PR exists to prevent. `_rekey_row` now takes an optional `fallback` decoder so mapping lookup, bijection fallback, and duplicate detection all happen in one pass. Also fixes 12 SonarQube findings in the new tests (3 of them bug-class `python:S5863`, which failed the quality gate on `new_reliability_rating`): tautological `f(x) == f(x)` assertions replaced with pinned expected values, composite `assert a and b` split, and `get_dialect(...)` hoisted out of `pytest.raises` blocks so only one call inside can throw. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to the previous commit: moving `decode_alias_map(aliases)` into the list comprehension rebuilt it for every result row, turning decoding from O(aliases + rows x columns) into O(rows x aliases + rows x columns). The map is a pure function of `aliases`, so hoist it back out of the loop. Codex review finding on PR #289. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-exceed-postgres-63-byte Brings in DEV-1741 (ingestion sees views, survives unmodellable names) and the v8 storage migration. One conflict, in DECISIONS.md: both branches appended a final entry to the append-only log. Kept both in chronological order — DEV-1741 (2026-08-05) then DEV-1756 (2026-08-06). Verified on the merged tree: 7601 unit tests pass (up from 7455 — main's 146 new tests came along clean), 464 integration tests pass, ruff clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three review findings on the identifier-length work, plus two doc fixes. 1. `_query_as_model` quoted its short alias UNCONDITIONALLY, which fixed the Postgres mixed-case bug by creating the mirror-image bug on upper-folding backends: the wrapper defined a case-sensitive `"status"` while the downstream `Column(sql="status")` reference stayed bare (only `_quote_mixed_case_identifiers` quotes it, and only when it contains an uppercase letter) and resolved as `STATUS` on Snowflake/Oracle. The contract is AGREEMENT between the two sides, not quoting, so `_short_sql` now mirrors the reference-side policy: quote iff mixed-case or reserved. Three tests bracket it — the agreement test fails against both the pre-PR version and the always-quote version. 2. Caller-supplied virtual-model shorts bypassed the length fitting. `m.name` / `t.name` / `e.name` and a user-declared cross-model `name` went into `column_map` verbatim, so two over-limit names sharing a 63-byte prefix were emitted as `AS "<64+ bytes>"` and truncated onto one column by Postgres — invisible to the `short_owner` check, which compares Python strings. New `_fit_short` covers all four sites plus the DEV-1449 `agg_shorts` breadcrumb, so it cannot drift from `column_map`. 3. `substitute_quoted`'s "cannot reach into a string literal" was an overclaim. Being keyed to an exact alias set bounds the exposure to a literal holding the exact quoted spelling of one of the same query's over-limit aliases; it does not eliminate it. Docstrings corrected — no code change, since the BigQuery/T-SQL dot-mangle regexes already run over this SQL with strictly wider exposure. 4. The `docs/database-support.md` worked example was fabricated: a 53-byte input, under the limit, paired with output `fit_identifier` never produces. Replaced with real 68 -> 63 output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dcoded set Follow-up review found the previous predicate (`short.lower() in SLAYER_RESERVED_KEYWORDS or any(c.isupper() ...)`) wrong on both of its axes for some dialects: * It misses words reserved only NATIVELY. `install_reserved_keywords` unions SLayer's set INTO each sqlglot generator's own, so only the generator knows the full set. On MySQL `index` / `int` / `rows` (and `rows` on BigQuery) are quoted by the downstream reference while the wrapper emitted them bare — and a bare `AS index` is a syntax error there, not merely a mismatch. * It ignores shape. `Column.name` forbids only `.` and `:`, so a short can be `1abc` or `foo bar`, which needs quoting on form alone regardless of case or reserved-ness. Rather than grow the predicate, `_short_sql` now runs the short through the same two mechanisms the downstream `Column(sql=short)` reference goes through: `SQLGenerator._maybe_quote_ident` (DEV-1645's uppercase rule) followed by `Identifier.sql(dialect=...)`, which lets sqlglot apply the target dialect's reserved-word and identifier-safety rules. The two spellings are then equal by construction rather than by a rule that has to be kept in sync. Pinned by a 12-dialect x 7-name test asserting the wrapper spelling and the reference spelling are byte-identical; it fails on 4 dialect/name pairs against the previous predicate. The remaining wrapper/reference disagreements are all names `_parse` cannot read as a column at all (`select`, `foo-bar`, `1abc`) — pre-existing, and unreferenceable however the wrapper spells them. Also splits a composite assertion flagged by Sonar (python:S9073). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
slayer/sql/dialects/bigquery.py (1)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse keyword arguments in new multi-parameter calls.
slayer/sql/dialects/bigquery.py#L144-L146: callfit_identifier(name=name, ...).slayer/sql/dialects/bigquery.py#L172-L175: callrewrite_emitted_sql(sql=sql, ...).slayer/sql/dialects/bigquery.py#L194-L197: call_rekey_row(row=row, mapping=mapping, ...).slayer/sql/generator.py#L285-L287: callfit_identifier(name=prefix + sanitized, ...).slayer/sql/generator.py#L434-L436: call_cte_name_from_alias(prefix=prefix, alias=alias, ...).slayer/sql/generator.py#L681-L683: callrewrite_emitted_sql(sql=sql, ...).slayer/sql/generator.py#L805-L805: call_cte_name(prefix="_cm_", alias=cm.alias).slayer/sql/generator.py#L890-L898: call_cte_name(prefix=..., alias=measure.alias).slayer/sql/generator.py#L1824-L1826: call_cte_name(prefix=..., alias=transform.alias).As per coding guidelines, “Use keyword arguments for functions with more than one parameter.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/sql/dialects/bigquery.py` around lines 144 - 146, Update all listed multi-parameter calls to use keyword arguments: in slayer/sql/dialects/bigquery.py lines 144-146, 172-175, and 194-197, name the fit_identifier, rewrite_emitted_sql, and _rekey_row arguments explicitly; in slayer/sql/generator.py lines 285-287, 434-436, 681-683, 805, 890-898, and 1824-1826, likewise use explicit keywords for fit_identifier, _cte_name_from_alias, rewrite_emitted_sql, and _cte_name arguments. Preserve each call’s current values and behavior.Source: Coding guidelines
docs/database-support.md (1)
54-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
markdownlint reports MD040 for this block. Use
text, since the content is illustrative output rather than runnable code.📝 Proposed fix
-``` +```text orders.customers.regions.districts.neighbourhoods.neighbourhood_name (68 bytes) -> orders.customers.regions.di_e6932600_urhoods.neighbourhood_name (63 bytes)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/database-support.mdaround lines 54 - 57, Add thetextlanguage
identifier to the fenced code block containing the illustrative schema output,
preserving its contents unchanged.</details> <!-- cr-comment:v1:1660843745c1a86d2596db7d --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>tests/test_dev1756_identifier_length.py (1)</summary><blockquote> `669-711`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _💤 Low value_ **Hoist the repeated function-local imports.** `_cte_name_from_alias` is imported separately in seven test bodies, and `re` is imported again at line 706 although the module does not import it at top level. The coding guidelines require imports at the top of the file. Move `from slayer.sql.generator import SQLGenerator, _cte_name_from_alias` and `import re` into the module header at lines 30-43. As per coding guidelines: "Keep imports at the top of files." <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@tests/test_dev1756_identifier_length.pyaround lines 669 - 711, Move the
repeated local imports to the module header: add _cte_name_from_alias alongside
SQLGenerator and add re with the other top-level imports. Remove the
function-body imports from the affected CTE-name tests while leaving their
assertions unchanged.</details> <!-- cr-comment:v1:be031695ea49e5565552f97a --> _Source: Coding guidelines_ </blockquote></details> <details> <summary>tests/integration/test_dev1756_identifier_length_pg.py (1)</summary><blockquote> `144-147`: _🩺 Stability & Availability_ | _🔵 Trivial_ | _⚡ Quick win_ **Dispose the engine after each test.** `chain_env` creates a new `SlayerQueryEngine` for each of the nine tests, but the tests call `execute(...)` directly. Use an async-yield fixture and call `await engine.aclose()` in `finally` to release each async SQLAlchemy connection pool. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@tests/integration/test_dev1756_identifier_length_pg.pyaround lines 144 -
147, Update the chain_env fixture to be async and yield the SlayerQueryEngine
instance, ensuring await engine.aclose() runs in a finally block after each
test; preserve the existing _chain_storage setup and direct execute(...) usage.</details> <!-- cr-comment:v1:9ee1bf06e0e801f6d36e7a2d --> </blockquote></details> <details> <summary>slayer/engine/query_engine.py (1)</summary><blockquote> `3232-3259`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Use a keyword for `fit_identifier`'s `name` argument.** `fit_identifier` has more than one parameter. Use a keyword for `name`. <details> <summary>Proposed fix</summary> ```diff return fit_identifier( - name, limit=get_dialect(dialect).max_identifier_bytes, + name=name, limit=get_dialect(dialect).max_identifier_bytes, )As per coding guidelines,
**/*.py: “Use keyword arguments for functions with more than one parameter.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/engine/query_engine.py` around lines 3232 - 3259, Update the fit_identifier call in _fit_short to pass the name argument by keyword, while preserving the existing dialect limit keyword and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/concepts/queries.md`:
- Around line 134-141: Update the identifier-length sentence in the result-key
description to avoid claiming that every engine imposes a cap; state that
aliases are rewritten only when the target database requires it, while
preserving the existing BigQuery and SQL Server behavior and the distinction
that only sql exposes the rewritten form.
In `@slayer/engine/query_engine.py`:
- Around line 1391-1402: Update get_column_types to decode fitted aliases in
raw_types before the metadata comparison loop, using the same
get_dialect(...).decode_result_keys call and
all_projection_aliases(prepared.enriched) mapping used in _run_and_build. Keep
the existing type-building loop unchanged so canonical EnrichedMeasure.alias
values are matched correctly.
- Around line 3323-3353: Update the virtual-model column construction to store
the rendered _short_sql(short) value in Column.sql, ensuring it matches the
emitted alias. Adjust _is_trivial_base to recognize dialect-specific quoted
self-identities, including double quotes, backticks, and brackets, so unsafe
aliases remain base columns and do not trigger false ColumnCycleError failures.
In `@slayer/sql/generator.py`:
- Around line 1824-1826: Update _generate_with_computed to allocate the shifted
and sjoin CTE names through _cte_name(), then reuse those allocated names
consistently in both CTE definitions and all references. Remove direct
shifted_{t.name} and sjoin_{t.name} construction so long transform aliases are
bounded like the consecutive-period names.
- Around line 438-444: Update the CTE allocation logic around _cte_names and
setdefault to key unquoted names using the dialect-aware normalization required
for case-insensitive identifier resolution, while retaining the original emitted
name for collision reporting. Ensure differently cased names collide on
PostgreSQL without changing IdentifierCollisionError’s first, second, or emitted
values.
In `@tests/test_dev1756_identifier_length.py`:
- Around line 661-667: Update test_cte_allocator_resets_per_statement to
exercise reset through generate() rather than clearing _cte_names directly:
create two enriched statements with different CTE owners, configure them so
their emitted names collide, and generate both using the same SQLGenerator
instance. Add assertions verifying each statement receives the expected
allocation and that per-statement state is reset; do not rely on generating the
same statement twice because _cte_name() is idempotent for one owner.
---
Nitpick comments:
In `@docs/database-support.md`:
- Around line 54-57: Add the `text` language identifier to the fenced code block
containing the illustrative schema output, preserving its contents unchanged.
In `@slayer/engine/query_engine.py`:
- Around line 3232-3259: Update the fit_identifier call in _fit_short to pass
the name argument by keyword, while preserving the existing dialect limit
keyword and behavior.
In `@slayer/sql/dialects/bigquery.py`:
- Around line 144-146: Update all listed multi-parameter calls to use keyword
arguments: in slayer/sql/dialects/bigquery.py lines 144-146, 172-175, and
194-197, name the fit_identifier, rewrite_emitted_sql, and _rekey_row arguments
explicitly; in slayer/sql/generator.py lines 285-287, 434-436, 681-683, 805,
890-898, and 1824-1826, likewise use explicit keywords for fit_identifier,
_cte_name_from_alias, rewrite_emitted_sql, and _cte_name arguments. Preserve
each call’s current values and behavior.
In `@tests/integration/test_dev1756_identifier_length_pg.py`:
- Around line 144-147: Update the chain_env fixture to be async and yield the
SlayerQueryEngine instance, ensuring await engine.aclose() runs in a finally
block after each test; preserve the existing _chain_storage setup and direct
execute(...) usage.
In `@tests/test_dev1756_identifier_length.py`:
- Around line 669-711: Move the repeated local imports to the module header: add
_cte_name_from_alias alongside SQLGenerator and add re with the other top-level
imports. Remove the function-body imports from the affected CTE-name tests while
leaving their assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c75cc9a7-f1e6-46fa-9ecd-d0569d619b7e
📒 Files selected for processing (24)
DECISIONS.mddocs/concepts/queries.mddocs/database-support.mdslayer/core/errors.pyslayer/engine/enriched.pyslayer/engine/query_engine.pyslayer/sql/dialects/_identifier_fit.pyslayer/sql/dialects/_tier2.pyslayer/sql/dialects/base.pyslayer/sql/dialects/bigquery.pyslayer/sql/dialects/clickhouse.pyslayer/sql/dialects/duckdb.pyslayer/sql/dialects/mysql.pyslayer/sql/dialects/postgres.pyslayer/sql/dialects/snowflake.pyslayer/sql/dialects/sqlite.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pytests/dialects/test_bigquery.pytests/dialects/test_identifier_fit.pytests/integration/test_dev1756_identifier_length_pg.pytests/test_dev1756_identifier_length.pytests/test_query_backed_models.pytests/test_sql_generator.py
CodeRabbit round on f573c35. Three real gaps, each with a test that fails without its fix: * `get_column_types` probed with generated SQL — which now carries FITTED aliases — but looked the results up by the canonical `EnrichedMeasure.alias`. An over-limit measure silently vanished from the type map (verified: the map came back empty). Decoded with the same hook and alias set `_run_and_build` uses. * `shifted_<name>` / `sjoin_<name>` self-join CTE names were f-string-built from a USER-supplied transform name, bypassing both the length fitting and the collision check. A 78-char transform name produced 86-byte CTE identifiers on Postgres while the projection alias beside them was correctly fitted. Routed through `_cte_name()`, so definition and every reference share one allocation. * The `_cte_name` allocator keyed on the literal emitted name, but CTE names are emitted UNQUOTED — so `_wm_Foo` and `_wm_foo` are one identifier on Postgres and both were accepted, silently pointing a reference at the wrong CTE. Keyed casefolded. Unlike the projection-alias namespace there is no quoted variant to exempt, so here casefolding is exact rather than merely conservative. Also: * `test_cte_allocator_resets_per_statement` asserted nothing and cleared `_cte_names` by hand. It now seeds an owner the statement does not use and drives the reset through `generate()` — regenerating the same statement proves nothing, since `_cte_name` is idempotent for one owner. * Corrected "every engine caps identifier length" in docs/concepts/queries.md; four dialects are unbounded, as the table two files over already says. * Keyword arguments at the multi-parameter call sites this PR added, per the repo convention. Not taken: storing `_short_sql(short)` in `Column.sql`. Unsafe-shaped shorts are unreachable — `ColumnRef.name`, `ModelMeasure.name` and `Aggregation.name` all enforce the full identifier pattern, so a query can neither reference nor name one. Pre-quoting `Column.sql` would instead double-quote it relative to the downstream reference and desync the pair f573c35 just aligned. Rationale posted on the thread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… over S3776 The decode added in 2d887d5 came with an `if raw_types:` guard, which pushed the function's cognitive complexity from 15 to 16 (Sonar python:S3776). The guard was never needed: `decode_result_keys` returns its input untouched when the alias map is empty, and re-keying an empty dict yields an empty dict. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both remaining multi-parameter calls this PR added were still passing `sql` positionally. Completes the convention sweep from 2d887d5. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Trim the identifier-fitting comments/docstrings that read like design essays down to the essential why. Verified comment/docstring-only: code AST is byte-identical, full non-integration suite still green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/dialects/test_identifier_fit.py (1)
99-105: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the child interpreter through Poetry.
Line 101 invokes
sys.executabledirectly. Replace it withpoetry run pythonso this subprocess uses the required project command path.Proposed fix
- sys.executable, "-c", + "poetry", "run", "python", "-c",As per coding guidelines, “Use
poetry runfor all Python commands.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/dialects/test_identifier_fit.py` around lines 99 - 105, Update the subprocess command in the identifier-fit test around subprocess.run to invoke Python through Poetry, replacing the direct sys.executable argument with the required poetry run python command while preserving the existing script and options.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@slayer/engine/query_engine.py`:
- Around line 1391-1395: Use keyword arguments for all supported parameters in
the three calls: in slayer/engine/query_engine.py lines 1391-1395, pass rows by
keyword to decode_result_keys; in lines 2049-2053, pass the type-probe row list
by keyword to decode_result_keys; and in lines 3231-3239, pass name by keyword
to fit_identifier.
---
Outside diff comments:
In `@tests/dialects/test_identifier_fit.py`:
- Around line 99-105: Update the subprocess command in the identifier-fit test
around subprocess.run to invoke Python through Poetry, replacing the direct
sys.executable argument with the required poetry run python command while
preserving the existing script and options.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22213994-f91b-4f9d-9b00-dd4a615db7a9
📒 Files selected for processing (20)
docs/concepts/queries.mdslayer/core/errors.pyslayer/engine/enriched.pyslayer/engine/query_engine.pyslayer/sql/dialects/_identifier_fit.pyslayer/sql/dialects/_tier2.pyslayer/sql/dialects/base.pyslayer/sql/dialects/bigquery.pyslayer/sql/dialects/clickhouse.pyslayer/sql/dialects/duckdb.pyslayer/sql/dialects/mysql.pyslayer/sql/dialects/postgres.pyslayer/sql/dialects/snowflake.pyslayer/sql/dialects/sqlite.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pytests/dialects/test_identifier_fit.pytests/integration/test_dev1756_identifier_length_pg.pytests/test_dev1756_identifier_length.pytests/test_query_backed_models.py
💤 Files with no reviewable changes (1)
- slayer/sql/dialects/snowflake.py
🚧 Files skipped from review as they are similar to previous changes (16)
- slayer/sql/dialects/clickhouse.py
- docs/concepts/queries.md
- slayer/engine/enriched.py
- tests/test_query_backed_models.py
- slayer/sql/dialects/postgres.py
- slayer/sql/dialects/_identifier_fit.py
- slayer/sql/dialects/tsql.py
- slayer/sql/dialects/duckdb.py
- slayer/sql/dialects/_tier2.py
- slayer/core/errors.py
- slayer/sql/dialects/mysql.py
- slayer/sql/dialects/bigquery.py
- slayer/sql/dialects/sqlite.py
- slayer/sql/dialects/base.py
- tests/test_dev1756_identifier_length.py
- tests/integration/test_dev1756_identifier_length_pg.py
Comment on lines
+1391
to
+1395
| # 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), | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use keyword arguments for the decode and fitting calls.
Use keyword arguments for each supported parameter in these calls.
slayer/engine/query_engine.py#L1391-L1395: passrowstodecode_result_keysby keyword.slayer/engine/query_engine.py#L2049-L2053: pass the type-probe row list todecode_result_keysby keyword.slayer/engine/query_engine.py#L3231-L3239: passnametofit_identifierby keyword.
Proposed fix
- rows, aliases=all_projection_aliases(prepared.enriched),
+ rows=rows, aliases=all_projection_aliases(prepared.enriched),
- [raw_types], aliases=all_projection_aliases(enriched),
+ rows=[raw_types], aliases=all_projection_aliases(enriched),
- name, limit=get_dialect(dialect).max_identifier_bytes,
+ name=name, limit=get_dialect(dialect).max_identifier_bytes,As per coding guidelines, “Use keyword arguments for functions with more than one parameter.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # 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), | |
| ) | |
| # 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=rows, aliases=all_projection_aliases(prepared.enriched), | |
| ) | |
| raw_types = get_dialect(dialect).decode_result_keys( | |
| rows=[raw_types], aliases=all_projection_aliases(enriched), | |
| )[0] | |
| 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=name, limit=get_dialect(dialect).max_identifier_bytes, | |
| ) |
📍 Affects 1 file
slayer/engine/query_engine.py#L1391-L1395(this comment)slayer/engine/query_engine.py#L2049-L2053slayer/engine/query_engine.py#L3231-L3239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@slayer/engine/query_engine.py` around lines 1391 - 1395, Use keyword
arguments for all supported parameters in the three calls: in
slayer/engine/query_engine.py lines 1391-1395, pass rows by keyword to
decode_result_keys; in lines 2049-2053, pass the type-probe row list by keyword
to decode_result_keys; and in lines 3231-3239, pass name by keyword to
fit_identifier.
Source: Coding guidelines
…erated-projection-aliases-exceed-postgres-63-byte # Conflicts: # DECISIONS.md # slayer/sql/dialects/bigquery.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
slayer/sql/dialects/base.py (1)
534-567: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDetect collisions on the final emitted alias.
alias_rewrite_map()usesfit_alias()as its collision key. BigQuery applies dot mangling later inemit_alias(). Therefore,a.banda___bpass this check but both emit asa___b.The generated query can contain duplicate result aliases.
decode_alias_map()then overwrites one canonical mapping. This can return data under the wrong result key.Use
emit_alias(alias)for allocation ownership. Keepfit_alias(alias)only as the canonical-to-write-replacement value. Validate the same allocation before building the decode map.Proposed fix
- if self.max_identifier_bytes is None: - return {} allocation: dict[str, str] = {} - owner: dict[str, str] = {} + emitted_owner: dict[str, str] = {} for alias in aliases: if alias in allocation: continue fitted = self.fit_alias(alias) - prior = owner.get(fitted) + emitted = self.emit_alias(alias) + prior = emitted_owner.get(emitted) if prior is not None and prior != alias: raise IdentifierCollisionError( - first=prior, second=alias, emitted=fitted, + first=prior, second=alias, emitted=emitted, dialect=self.sqlglot_name, limit=self.max_identifier_bytes, namespace="projection alias", ) - owner[fitted] = alias + emitted_owner[emitted] = alias allocation[alias] = fitteddef decode_alias_map(self, aliases: Sequence[str]) -> dict[str, str]: + self.alias_rewrite_map(aliases) out: dict[str, str] = {}Also applies to: 600-631
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@slayer/sql/dialects/base.py` around lines 534 - 567, Update alias_rewrite_map() to use emit_alias(alias) as the ownership and collision key while retaining fit_alias(alias) as the canonical write replacement. Add equivalent collision validation in decode_alias_map() before populating its inverse mapping, raising IdentifierCollisionError for distinct canonical aliases with the same emitted alias so mappings cannot be silently overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@slayer/sql/dialects/base.py`:
- Around line 534-567: Update alias_rewrite_map() to use emit_alias(alias) as
the ownership and collision key while retaining fit_alias(alias) as the
canonical write replacement. Add equivalent collision validation in
decode_alias_map() before populating its inverse mapping, raising
IdentifierCollisionError for distinct canonical aliases with the same emitted
alias so mappings cannot be silently overwritten.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c20481c-fc77-4692-82e4-a6de00397874
📒 Files selected for processing (6)
DECISIONS.mdslayer/engine/query_engine.pyslayer/sql/dialects/base.pyslayer/sql/dialects/bigquery.pytests/dialects/test_bigquery.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (2)
- slayer/engine/query_engine.py
- tests/test_sql_generator.py
Reduce the database-support.md section from ~60 lines to a 7-line note: some DBs cap identifier length (Postgres 63 bytes, silent), SLayer trims over-limit aliases, output column names are unaffected, only the introspectable SQL shows the trimmed form. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
ZmeiGorynych
added a commit
that referenced
this pull request
Aug 17, 2026
…length fitting) Ports DEV-1756 to the DEV-1450 structural pipeline for the projection-alias AND CTE-name surfaces (main's enrichment-based wiring does not apply here). Projection aliases: - Kept main's design intact: pure fit_identifier/substitute_quoted module, the aliases-taking SqlDialect.rewrite_emitted_sql/decode_result_keys + fit_alias/emit_alias/alias_rewrite_map/decode_alias_map/_rekey_row, and the per-dialect max_identifier_bytes budgets. - Write and read sides share ONE plan-derived source of canonical projection keys: new response_meta.projection_result_keys(root_planned). The write side passes it into generate_planned_stages(projection_aliases=...) (NOT parsed off the SQL — sqlglot cannot parse a pre-mangle BigQuery dotted alias); the read side threads the same set into build_response_metadata, _run_data_query (via _Prepared.expected_columns) and the get_column_types probe. CTE names: - naming.cte_name_from_alias now length-fits prefix+sanitized via fit_identifier before AliasAllocator.allocate_cte, and raises IdentifierCollisionError if the allocator's _2 suffix pushes a fitted name back over the limit. dialect+limit threaded from the _cm_/_wm_/ranked and the time-shift shifted_/sjoin_ CTE call sites. Closes the deep-cross-model Postgres truncation-collision gap; limit=None is byte-identical to before. Conflict resolutions: git rm enriched.py; take-HEAD for generator.py / query_engine.py (old-SQLGenerator hunks), re-adding the fitting hooks; union imports in base/bigquery/tsql (branch naming + fit_identifier); DECISIONS.md date-interleave. Tests: test_identifier_fit.py + the projection/CTE scenarios of test_dev1756_identifier_length.py rewritten to drive the real engine and pass. Virtual-model short fitting (no _query_as_model on this branch) stays skipped as a documented follow-up. Two no-churn goldens assert intent (no _<hash>_ marker) since the branch emits CAST(SUM(...)) where main emitted SUM(...).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes DEV-1756 (projection-alias surface). Join-path table aliases are explicitly out of scope — see below.
The bug
Postgres caps identifiers at 63 bytes and truncates past it silently (a
NOTICE, never an error). SLayer's alias convention<root_model>.<join.path>.<column>crosses that on a 3-hop join:Two failure modes, and the quiet one is worse:
AmbiguousColumnError: column reference "SandboxInvoiceV2.SandboxSubscription.SandboxCustomer.SandboxCon" is ambiguous— the reported errorThe repro is executed against a real Postgres in
tests/integration/test_dev1756_identifier_length_pg.py, not just asserted on emitted bytes — the whole point is that byte-level tests pass while the server misbehaves.The fix
Each dialect declares a conservative
SqlDialect.max_identifier_bytes; an over-limit identifier is fitted at emission to<head>_<hash8>_<tail>(newslayer/sql/dialects/_identifier_fit.py):decode_result_keysrestores the canonical dotted alias, sodata/columns/attributesare unchanged on every backend; onlyresponse.sqlshows the fitted form, because that is what ran.dry_runoutput.Write side
Exact-match replacement driven by the query's own alias set — never a length regex over arbitrary SQL, so a long quoted-looking span inside a string literal (
WHERE note LIKE '%\"...\"%') cannot be corrupted. The set isall_projection_aliases, deliberately unfiltered: hidden ORDER-BY hoists and_inner_*/_ft*/_ts*entries are projected in the inner SELECT and truncate identically, so a filtered list would leave their references pointing at an unfitted name. Substitution is two-phase as defence-in-depth (today's key set and value set are provably disjoint, so it cannot cascade — that invariant is pinned by its own test).BigQuery / T-SQL
Their existing dot-mangle regex runs after the length pass. An under-limit alias makes the length pass a genuine no-op, so their output is byte-identical to today; an over-limit one arrives still-dotted and is mangled by the same regex, so there is no double-encoding. The budget is sized against
encode_alias, since mangling lengthens the identifier after fitting.Scope
Three output-name surfaces: projection aliases, CTE names (allocated through a per-statement collision-checked
SQLGenerator._cte_name, budget counted including the_cm_/cp_value_12_prefix), and_query_as_modelvirtual-model shorts.Join-path table aliases (
customers__regions) are deferred to DEV-1743, which already owns the__path-alias allocator and plans exactly this collision check. Their failure mode (silent wrong joins) is worse, but fixing it means decouplingEnrichedDimension.model_namefrom the emitted qualifier across ~8 sites that split it back on__— a different, riskier change that would swamp review of the reported fix. A detailed handoff is on DEV-1743.Bonus: a pre-existing bug in the same code path
_query_as_modelemitted its short alias bare, so Postgres case-folded it while the outer stage referenced it quoted (DEV-1645 quotes mixed-caseColumn.sqlleaves). Any query-backed model with a mixed-case join path was unqueryable:Reproduced on a live server and fixed by always dialect-quoting the short, which also takes these names out of the case-folding namespace entirely. This is why two
TestMultiStageMeasureRenameassertions move fromAS revtoAS \"rev\".Collisions
Two SLayer-generated names colliding after fitting raises
IdentifierCollisionErrorrather than emitting ambiguous SQL. The check covers identity entries too — an already-short alias equal to another's fitted form is a duplicate that no hash width can prevent. Validated per namespace: projection aliases, CTE names, and virtual-model shorts (the last case-insensitively).Verification
7450 passednon-integration;464 passedintegration (Postgres / DuckDB / SQLite / …)AmbiguousColumnErrorfit_identifierfails 70 of the 209 new tests — they exercise the behaviour, not just the importsruff check slayer/ tests/cleanTwo Codex review rounds (plan, then tests) folded in. One finding rejected with reasoning: rewriting aliases at AST-construction time would touch ~20 emission sites and still miss the string-concatenated CTE-assembly paths.
Docs
docs/database-support.mdgains an identifier-length section with the per-dialect table and the stated approximations (bytes not chars; MySQL's identifier limit rather than its column-alias limit; Oracle 12.2+).docs/concepts/queries.mdnotes that result keys are always canonical.DECISIONS.mdentry appended. No new pages, sozensical.tomlnav is unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation