DEV-1716: Route typed-generator emission through slayer/sql/dialects/ (DEV-1542 parity) - #257
Merged
ZmeiGorynych merged 8 commits intoJul 27, 2026
Conversation
Port main's DEV-1542 dialect-strategy delegation into the typed pipeline so every dialect-specific SQL decision routes through slayer/sql/dialects/, flipping the 127 strict-xfail parity pins DEV-1716 owns. Generator: - __init__(dialect: str | SqlDialect) + read-only `dialect` property - _parse/_parse_predicate apply rewrite_parsed_ast + rewrite_target_ast - delegate _build_date_trunc / _build_time_offset_expr / _duration_interval_exprs / _add_intervals_expr / _build_median / _build_percentile / _build_stat_agg / _build_outer_wrap; _rewrite_log_aliases -> should_use_native_log - _build_agg(measure=EnrichedMeasure) compat (keeps DEV-1452 AggRenderSpec) - terminal rewrite_emitted_sql on generate() + generate_planned_stages - _quote_ident replaces raw ANSI quoting at time-shift/self-join CTE, transform, projection and ORDER BY sites; _ordered suppresses T-SQL NULLS-emulation Engine: decode_result_keys on the data path; _dialect_for_type -> dialect_for_ds_type().sqlglot_name (drop duplicate map); _build_explain_sql -> build_explain_sql hook. response_meta: decode SQL-derived expected_columns so BigQuery/T-SQL attributes survive. Tests: un-pin the 127 DEV-1716 parity entries; add engine EXPLAIN-dispatch, multi-stage mangling, attribute-survival, decode-scoping and interval/parse delegation coverage; drop stale MySQL-raises / T-SQL duplicate tests superseded by tests/dialects/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…erge-originmain-into-the-typed-pipeline' into egor/dev-1716-dev-1703-route-typed-generator-emission-through # Conflicts: # tests/parity_xfails.py
…-model / consecutive / projection / order)
Complete the §3d identifier-quoting port on the PLANNED path (generate_from_planned
and its helpers) that the engine uses — the prior commit only covered the enriched
generate() path that the unit tests exercise. The planned path built CTE / projection
/ ORDER BY / join-ON SQL with raw ANSI double-quoted identifiers, which MySQL reads as
string literals (1064 syntax error) and BigQuery/T-SQL can't mangle.
Route every raw `f'"{alias}"'` site through `_quote_ident` (backticks on MySQL/BigQuery,
brackets on T-SQL, ANSI on Postgres/SQLite/DuckDB — identity there, so no change for
ANSI dialects): planned time-shift shifted/self-join CTEs, cross-model combined SELECT +
join-ON, transform-chain step/outer projection, consecutive-periods reset/value CTEs,
window-transform measure/time/partition refs, planned ORDER BY entries, and the
post-filter qualified-alias wrap. A couple of enriched leftovers folded in for consistency.
Verified: all 9 formerly-pinned MySQL integration tests pass (live testcontainers);
full non-integration suite green (7700); SQLite/DuckDB/Postgres integration green.
Un-pin the 6 remaining DEV-1716 MySQL integration parity entries -> zero DEV-1716 pins.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change centralizes SQL dialect behavior in shared strategies. Query execution and metadata decode dialect-mangled keys, while ChangesDialect strategy integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant QueryPlanner
participant SQLGenerator
participant SqlDialect
participant Database
QueryPlanner->>SQLGenerator: generate planned SQL
SQLGenerator->>SqlDialect: rewrite AST and render dialect SQL
SqlDialect-->>SQLGenerator: dialect-specific SQL
SQLGenerator->>Database: execute terminal SQL
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 2
🧹 Nitpick comments (2)
tests/test_generator2_multistage.py (1)
365-365: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
reimport to module scope.Line 365 violates the repository rule to keep imports at the top of files.
As per coding guidelines, “keep imports at the top of files.”
🤖 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/test_generator2_multistage.py` at line 365, Move the aliased `re` import from its current local position to the module-level import section at the top of tests/test_generator2_multistage.py, preserving the `_re` alias and removing the duplicate local import.Source: Coding guidelines
slayer/sql/generator.py (1)
2419-2440: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDual
spec=/measure=entry points on_build_aggare a compatibility wart.Two mutually exclusive inputs with a runtime
Noneguard is easy to misuse. Consider keeping_build_agg(spec)as the only signature and having the delegation tests adapt via_agg_render_spec_from_enrichedthemselves; then drop themeasure=branch once those tests move.🤖 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/generator.py` around lines 2419 - 2440, The _build_agg method exposes an unnecessary compatibility-only measure entry point alongside spec. Remove the measure keyword parameter, its _agg_render_spec_from_enriched adaptation branch, and the related None guard; keep _build_agg accepting only the required AggRenderSpec and update callers or delegation tests to adapt EnrichedMeasure through _agg_render_spec_from_enriched before invoking it.
🤖 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/sql/generator.py`:
- Around line 764-769: Update _query_as_model to use the shared flat-rename
wrapper from stage_wrapper.py when emitting aliased columns, rather than
constructing f'"{alias}" AS {short}' directly. Ensure the wrapper receives the
dialect-mangled alias names so wrapped BigQuery/T-SQL inner references stay
consistent and quoting remains dialect-aware, including MySQL.
In `@tests/test_generator2_multistage.py`:
- Around line 384-386: Strengthen the assertion in the multi-stage T-SQL
regression test to reject bracketed dotted identifiers such as
[stage1.amount_sum] and [orders.*], not only ANSI double-quoted forms. Preserve
the existing SQL diagnostic and continue validating that dotted aliases are
mangled.
---
Nitpick comments:
In `@slayer/sql/generator.py`:
- Around line 2419-2440: The _build_agg method exposes an unnecessary
compatibility-only measure entry point alongside spec. Remove the measure
keyword parameter, its _agg_render_spec_from_enriched adaptation branch, and the
related None guard; keep _build_agg accepting only the required AggRenderSpec
and update callers or delegation tests to adapt EnrichedMeasure through
_agg_render_spec_from_enriched before invoking it.
In `@tests/test_generator2_multistage.py`:
- Line 365: Move the aliased `re` import from its current local position to the
module-level import section at the top of tests/test_generator2_multistage.py,
preserving the `_re` alias and removing the duplicate local import.
🪄 Autofix (Beta)
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: e3edb127-4124-4234-87ee-1ea81c0c2ac4
📒 Files selected for processing (10)
slayer/engine/query_engine.pyslayer/engine/response_meta.pyslayer/sql/generator.pytests/dialects/test_bigquery.pytests/dialects/test_engine_explain_dispatch.pytests/dialects/test_generator_dispatch.pytests/dialects/test_tsql.pytests/parity_xfails.pytests/test_generator2_multistage.pytests/test_sql_generator.py
💤 Files with no reviewable changes (1)
- tests/parity_xfails.py
…CodeRabbit nits - Codex (major): _query_as_model built its outer rename wrapper with raw ANSI "orders.status" refs, but generate(render_mode="wrapped") now dialect-quotes AND (BigQuery/T-SQL) alias-mangles the inner projection — so the outer ref pointed at a column the mangled inner no longer exposes (and was a string literal on MySQL/BigQuery/T-SQL). Dialect-quote + rewrite_emitted_sql each inner ref (identity for Postgres/SQLite/DuckDB). + regression test. - Sonar S5778: hoist BigqueryDialect() construction out of the pytest.raises block in test_engine_explain_dispatch. - Sonar S3776: NOSONAR on build_response_metadata (pre-existing per-slot metadata-loop complexity; matches the codebase's existing S3776 pattern). - CodeRabbit: move the `re` import to module scope in test_generator2_multistage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 2386-2398: Replace the raw f-string projection assembly around
`_inner_ref` and `rename_parts` with a sqlglot AST helper that builds each
wrapper projection from the alias map and renders it through the existing
dialect pipeline. Preserve the current alias rewriting, short-name mapping, and
generated `AS` semantics while eliminating SQL string concatenation in this
wrapper construction.
🪄 Autofix (Beta)
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: 987e765d-3d07-41c7-8730-155a127e43fa
📒 Files selected for processing (5)
slayer/engine/query_engine.pyslayer/engine/response_meta.pytests/dialects/test_bigquery.pytests/dialects/test_engine_explain_dispatch.pytests/test_generator2_multistage.py
🚧 Files skipped from review as they are similar to previous changes (4)
- slayer/engine/response_meta.py
- tests/test_generator2_multistage.py
- tests/dialects/test_bigquery.py
- tests/dialects/test_engine_explain_dispatch.py
…entifiers CodeRabbit: the multi-stage T-SQL regression test only rejected ANSI-quoted dotted identifiers; add a check that no bracketed identifier ([stage1.amount_sum]) contains a dot either, so a missed T-SQL alias-mangle is caught. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…set_expr Codex review: delegating _build_time_offset_expr exposed a T-SQL gap — a time_shift over a WEEK_SUNDAY time dimension passed granularity="week_sunday" to TsqlDialect.build_time_offset_expr, whose unit_map lacked the key and fell through to DATEADD(WEEK_SUNDAY, ...), invalid T-SQL. The base _granularity_to_unit already normalizes it (a one-period Sunday-week shift is one week, DEV-1572); add the same to the T-SQL unit map + a coverage test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…pper Codex review: generate_planned_stages now returns BigQuery/T-SQL alias-mangled terminal SQL, but _expand_query_backed_model feeds that into build_flat_rename_wrapper, which strips the "<source_relation>." prefix expecting canonical dotted aliases. After mangling it saw orders___status, couldn't strip "orders.", and the produced/expected assertion raised for BigQuery/T-SQL query-backed models. Decode each rendered output name to canonical dotted form (via the dialect's decode_result_keys, identity for non-mangling dialects and a no-op on already-dotted names) for the prefix-strip + flat-name computation, while still referencing the ACTUAL mangled name as the inner-column source. + tests. (Keeping wrapped-mode mangling — it's a main-parity pin; the consumer decodes, matching how the legacy pipeline's consumers handle mangled aliases.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_stage_wrapper_helper.py`:
- Line 95: Move the build_flat_rename_wrapper import out of the affected test
functions and add it once alongside the module-level imports in
tests/test_stage_wrapper_helper.py. Remove the duplicate local imports while
leaving the test behavior unchanged.
🪄 Autofix (Beta)
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: fcc8fefb-e6fc-408e-9365-b617cdcaf96b
📒 Files selected for processing (2)
slayer/sql/stage_wrapper.pytests/test_stage_wrapper_helper.py
| the expected flat schema, while still referencing the ACTUAL mangled inner | ||
| column. Without the decode the prefix-strip misses and the produced/expected | ||
| assertion raises for BigQuery/T-SQL query-backed models.""" | ||
| from slayer.sql.stage_wrapper import build_flat_rename_wrapper |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the new imports to module scope.
The added build_flat_rename_wrapper imports are inside test functions, contrary to the **/*.py rule requiring imports at the top of the file. Import it once with the module-level imports.
Also applies to: 120-120
🤖 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/test_stage_wrapper_helper.py` at line 95, Move the
build_flat_rename_wrapper import out of the affected test functions and add it
once alongside the module-level imports in tests/test_stage_wrapper_helper.py.
Remove the duplicate local imports while leaving the test behavior unchanged.
Source: Coding guidelines
… nit
- Codex: get_column_types runs a LIMIT-0 probe whose SQL is alias-mangled on
BigQuery/T-SQL (it must be, to execute), so the cursor returns mangled keys
(orders___amount_max). The qualified-alias map-back looked up canonical dotted
keys and silently returned {} for those dialects. Decode raw_types keys via the
dialect (identity elsewhere) before the map-back. + end-to-end test.
- CodeRabbit: hoist the build_flat_rename_wrapper import to module scope in
test_stage_wrapper_helper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
ZmeiGorynych
merged commit Jul 27, 2026
d685377
into
egor/dev-1704-dev-1703-stage-0-merge-originmain-into-the-typed-pipeline
6 checks passed
This was referenced Aug 1, 2026
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.
What
Ports main's DEV-1542 dialect-strategy delegation into the typed pipeline so every dialect-specific SQL decision routes through
slayer/sql/dialects/, flipping all DEV-1716-owned strict-xfail parity pins (127 unit + 9 MySQL integration → zero DEV-1716 pins remain).Generator (
slayer/sql/generator.py)__init__(dialect: str | SqlDialect)+ read-onlydialectproperty (self._dialect.sqlglot_name)_parse/_parse_predicateapplyrewrite_parsed_ast(parse-dialect) +rewrite_target_ast(target-dialect — Postgres 2-arg ROUND cast)_build_date_trunc/_build_time_offset_expr/_duration_interval_exprs/_add_intervals_expr/_build_median/_build_percentile/_build_stat_agg/_build_outer_wrap;_rewrite_log_aliases→should_use_native_log_build_agg(measure=EnrichedMeasure)compat param (preserves the DEV-1452AggRenderSpecsplit)rewrite_emitted_sqlongenerate()+generate_planned_stages(DB-bound terminals, not per-stage)_quote_identreplaces raw ANSI"..."identifiers across both the enriched and planned paths (time-shift/self-join/step CTEs, cross-model combined, consecutive-periods, projections, ORDER BY, join-ON) so MySQL/BigQuery/T-SQL get correct quoting (identity for Postgres/SQLite/DuckDB);_orderedsuppresses T-SQL NULLS-emulationCASE WHENEngine + metadata
decode_result_keyson the data path;_dialect_for_type→dialect_for_ds_type().sqlglot_name(drop duplicate map);_build_explain_sql→build_explain_sqlhookresponse_meta: decode SQL-derivedexpected_columnsso BigQuery/T-SQL attributes surviveTests
tests/dialects/Verification
Also merges the latest DEV-1704 (Stage-0) state into this branch.
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
EXPLAINSQL construction via the active dialect strategy (legacy output preserved where supported).Bug Fixes
dry_run/EXPLAINexecution paths.Tests
week_sundaytime-shift coverage; pruned obsolete parity xfails.