Fix compiler emitting invalid SQL: unprojected ORDER BY keys + unquoted mixed-case identifiers (DEV-1645)#224
Merged
ZmeiGorynych merged 7 commits intoJul 7, 2026
Conversation
Two valid-on-SQLite / invalid-on-Postgres compiler defects, both surfacing as UndefinedColumn on real execution (SLayer's dry_run compiles without executing, which hid them until submit_query's dry-run gate). Flavor A - ORDER BY on an unprojected/renamed column. The sort key was rendered as one composite-quoted identifier "<model>.<col>", which resolves only when the column is a projected output alias. _resolve_order_column now returns a discriminated _OrderColRef: projected aliases stay whole-quoted; non-projected/renamed keys emit a split table.column reference (two identifiers) that resolves against the FROM-scope column. Applied at all three ORDER BY emission sites. Flavor B - mixed-case identifiers emitted unquoted, so case-folding dialects (Postgres/Redshift to lower; Snowflake/Oracle to upper) fold them to a non-existent name. A context-aware quoting pass quotes real DB identifiers - column-name leaves and physical table-name parts - while leaving SLayer-internal aliases/qualifiers alone (they fold consistently within a query). Wired into both parse paths (_parse and _parse_predicate) and every direct AST-construction site: _resolve_sql, _build_from_clause, cross-model measure tables/join keys, resolved-join targets, and sql=None measures. Universal across dialects. Tests: new tests/test_dev1645_invalid_postgres_sql.py (unit: both flavors, helper-level, multi-dialect); real-Postgres integration tests reproducing the exact UndefinedColumn failures; a Snowflake upper-fold test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds mixed-case identifier quoting to SQL generation and refactors ORDER BY handling to distinguish projected aliases from FROM-scope columns. Adds Postgres and Snowflake integration coverage plus unit tests for SQL emission and dialect quoting. ChangesMixed-case identifier quoting and ORDER BY fix
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 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: 1
🧹 Nitpick comments (1)
tests/test_dev1645_invalid_postgres_sql.py (1)
71-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared model-factory helper to reduce fixture duplication.
Several tests across this file and the integration suites hand-roll near-identical
SlayerModel/Columnconstruction (id + mixed-case column + join). A small factory helper could reduce repetition, though current duplication is localized and readable enough to defer.🤖 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_dev1645_invalid_postgres_sql.py` around lines 71 - 107, Refactor the repeated SlayerModel/Column fixture setup into a shared helper to reduce duplication across orders_model, accounts_model, and similar integration test fixtures. Add a small factory function for common model construction (id plus shared column patterns, including the mixed-case/join cases) and have the existing fixtures call it so the test intent stays the same while the repeated setup lives in one place.
🤖 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 991-998: The ORDER BY fallback in _assemble_combined_sql() is
using _order_split_sql(ref), which can emit the out-of-scope model prefix from
_resolve_order_column() and break combined queries. Update the fallback path in
the ORDER BY assembly to remap non-alias columns to an in-scope reference, using
the available _base or measure CTE aliases instead of the original model prefix.
Keep the existing alias handling for ref.is_alias, but ensure the
non-projected-column case resolves to a valid column name within the combined
query scope.
---
Nitpick comments:
In `@tests/test_dev1645_invalid_postgres_sql.py`:
- Around line 71-107: Refactor the repeated SlayerModel/Column fixture setup
into a shared helper to reduce duplication across orders_model, accounts_model,
and similar integration test fixtures. Add a small factory function for common
model construction (id plus shared column patterns, including the
mixed-case/join cases) and have the existing fixtures call it so the test intent
stays the same while the repeated setup lives in one place.
🪄 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: 4df0a752-bbbd-4e27-8799-8aa4379c50d4
📒 Files selected for processing (5)
slayer/sql/generator.pytests/integration/test_integration_postgres.pytests/integration/test_integration_snowflake.pytests/test_cross_model_derived_columns.pytests/test_dev1645_invalid_postgres_sql.py
ZmeiGorynych
changed the title
…EV-1645) Codex review of PR #224: the first/last aggregation path referenced group-by dimensions by bare name against the ranked subquery's model.* output via unquoted exp.to_identifier(dim.name). A mixed-case dimension folded to lower on Postgres (upper on Snowflake) and failed to match the subquery output column. Route both ranked-path dimension-reference sites through _to_ident so mixed-case names are quoted, matching the rest of the DEV-1645 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(DEV-1645) Codex review of PR #224: a plain-column ORDER BY on an unprojected multi-hop joined path (e.g. order by customers.regions.name) never pulled its join into scope — filters resolve such joins, ORDER BY did not — so the split fallback emitted a reference to a phantom alias, i.e. invalid SQL, defeating the point of the PR. _resolve_order_column now resolves the fallback qualifier to an in-scope table: the base model, or a join already pulled into scope (dimension / measure / filter), mapping a multi-hop dotted qualifier (customers.regions) to its canonical __ alias (customers__regions). When neither is in scope it raises the new UnresolvableOrderColumnError (SlayerError, ValueError) with an actionable message, rather than emitting SQL that fails at the database. Base-column unprojected ORDER BY (the reported flavor-A cases) still emits the split reference; the measure-CTE _base combined path stays a documented limitation (base-model qualifier is treated as in-scope, so it is not rejected). Adds tests: reject-when-unresolvable, resolve-via-__-alias-when-in- scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up Codex review of PR #224: the unresolvable-ORDER-BY reject check consulted enriched.resolved_joins globally, so a join pulled into scope by a filter was treated as ORDER BY-visible even in the CTE-wrapped emission paths (_assemble_combined_sql / _apply_pagination_to_sql), where the outer query orders from _base / _filtered / measure CTEs and the join alias is NOT bound. That accepted a joined order column and emitted an unbound reference — invalid SQL — instead of rejecting. _resolve_order_column now takes joins_in_scope, set by the caller: True only for the base SELECT (_apply_order_limit), where the resolved LEFT JOINs are physically in the FROM; False for the two CTE-wrapped sites, which therefore reject a joined order column. The base-model qualifier remains in-scope either way, so the documented _base base-column limitation is still not rejected. Adds a test covering a windowed-measure combined-CTE query that filters on and orders by a joined column. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-1645) Further Codex review of PR #224: _apply_order_limit resolves the base SELECT's ORDER BY, but when _generate_base wraps the FROM in a first/last ranked subquery the joins live INSIDE that subquery and the outer SELECT only re- exposes model.*. Passing joins_in_scope=True unconditionally there would let a filter-pulled joined column emit an unbound outer reference. Capture the wrap decision (from_wrapped_in_ranked) and pass joins_in_scope=not from_wrapped_in_ranked into _apply_order_limit, so the ranked-wrap case rejects a joined ORDER BY like the other CTE-wrapped paths. Base-column ORDER BY is unaffected (base-model qualifier stays in-scope). Adds a test for the first/last-wrap + filter-pulled-join + order-by-joined-column case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ity (DEV-1645) Codex kept finding new outer-wrapping layers (measure CTEs, pagination, first/last ranked subquery, and now projection trim) that relocate the ORDER BY out of the base SELECT scope where a filter-pulled join alias is bound. Threading joins_in_scope through each layer was whack-a-mole, so per the chosen design, drop it: _resolve_order_column now rejects ANY unprojected joined / cross-model ORDER BY column (qualifier != the query's source_model) with UnresolvableOrderColumnError, and only emits a split reference for a base-model qualifier. This still fixes every reported Postgres failure: all reported failing ORDER BY qualifiers are the query's own source_model stage (ranked / agg / waitlist_longest_report / snap_metrics / tlc / AuditAndCompliance), i.e. base columns emitted as a split reference — none order by a joined column. Removes the joins_in_scope plumbing across the three emission sites and _apply_order_limit. Adds a TestFlavorAJoinedOrderByDeferred class of xfail(strict=True) tests documenting the deferred aspiration (joined ORDER BY should eventually resolve like filters do); they flip to XPASS and fail the suite if the capability is implemented, prompting removal of the reject. The reject-behaviour tests remain as the current contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_dev1645_invalid_postgres_sql.py (2)
282-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated model-construction block across two tests.
The
regions/customers/orders(withdefault_time_dimension/created_at) setup intest_orderby_joined_column_rejected_in_cte_wrapped_scope(Lines 288-317) is duplicated verbatim intest_joined_orderby_in_cte_wrapped_scope_should_resolve(Lines 409-437). Consider extracting a second shared helper (similar to_save_orders_customers_regions) for this CTE-wrapped-scope model shape to avoid drift between the two.Also applies to: 404-447
🤖 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_dev1645_invalid_postgres_sql.py` around lines 282 - 327, The regions/customers/orders setup in this test is duplicated in the paired CTE-wrapped-scope test, which risks drift. Extract the repeated model-building and storage setup into a shared helper (similar to the existing _save_orders_customers_regions pattern) and reuse it from test_orderby_joined_column_rejected_in_cte_wrapped_scope and test_joined_orderby_in_cte_wrapped_scope_should_resolve, keeping the default_time_dimension and created_at details centralized.
248-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary staticmethod aliasing.
Re-binding the module-level
_save_orders_customers_regionsas a class attribute just to call it viaself.adds indirection with no benefit — other tests in this file (e.g. line 380, 395) already call the module function directly.♻️ Suggested simplification
- _save_orders_customers_regions = staticmethod(_save_orders_customers_regions) - async def test_orderby_unresolvable_joined_column_rejected(self, tmp_path) -> None: ... - orders = await self._save_orders_customers_regions(storage) + orders = await _save_orders_customers_regions(storage)🤖 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_dev1645_invalid_postgres_sql.py` at line 248, Remove the unnecessary staticmethod alias on the test class for _save_orders_customers_regions and call the module-level helper directly instead. Update the affected test setup in the class that currently rebinds _save_orders_customers_regions so it no longer exposes it as a class attribute, and adjust any self.-based calls in that test class to use the existing module function reference like the other tests in this file.
🤖 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.
Nitpick comments:
In `@tests/test_dev1645_invalid_postgres_sql.py`:
- Around line 282-327: The regions/customers/orders setup in this test is
duplicated in the paired CTE-wrapped-scope test, which risks drift. Extract the
repeated model-building and storage setup into a shared helper (similar to the
existing _save_orders_customers_regions pattern) and reuse it from
test_orderby_joined_column_rejected_in_cte_wrapped_scope and
test_joined_orderby_in_cte_wrapped_scope_should_resolve, keeping the
default_time_dimension and created_at details centralized.
- Line 248: Remove the unnecessary staticmethod alias on the test class for
_save_orders_customers_regions and call the module-level helper directly
instead. Update the affected test setup in the class that currently rebinds
_save_orders_customers_regions so it no longer exposes it as a class attribute,
and adjust any self.-based calls in that test class to use the existing module
function reference like the other tests in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32db5af9-8bba-4d26-8d63-c8d52d096504
📒 Files selected for processing (3)
slayer/core/errors.pyslayer/sql/generator.pytests/test_dev1645_invalid_postgres_sql.py
🚧 Files skipped from review as they are similar to previous changes (1)
- slayer/sql/generator.py
…te (DEV-1645) The Sonar quality gate failed on new_duplicated_lines_density (7.9% vs 3%): as the review loop added the CTE-wrapped / first-last / xfail joined-ORDER-BY tests, each rebuilt the same regions -> customers -> orders model graph inline (CPD flagged two 37-line + three 19-line blocks in tests/test_dev1645_invalid_postgres_sql.py, 11.7% density). Extend the shared _save_orders_customers_regions helper with a created_at column + default_time_dimension and route the three inline-rebuilding tests through it, collapsing the repeated graph to one definition. No behavioural change; suite still 6485 passed, 5 xfailed. (The integration-file duplication Sonar also lists is pre-existing mirror-of-dialect-tests code, not new to this PR.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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.
Summary
Fixes two SQL-compiler defects that make SLayer emit SQL that is valid on SQLite (case-insensitive, lenient) but invalid on case-sensitive / case-folding backends. Postgres was where the repros surfaced (
livesqlbench-large: 12 failures across 8/10 tasks; 9 affectedattempt-1.jsonfiles in the repro data), but neither fix is Postgres-specific — both are dialect-agnostic and the mixed-case fix is verified on an upper-folding dialect (Snowflake) too. SLayer'sdry_runcompiles without executing, so these stayed invisible until the first real execution.Flavor A — ORDER BY on an unprojected/renamed column (dialect-agnostic)
The sort key was rendered as one composite-quoted identifier
"<model>.<col>", which resolves only when the column is a projected output alias. When ordered by a renamed (columns:transform) or dropped inner-stage dim, that composite token is a nonexistent column — invalid on every dialect, not just Postgres._resolve_order_columnnow returns a discriminated_OrderColRef: projected aliases stay whole-quoted; non-projected/renamed keys emit a splittable.columnreference that resolves against the FROM-scope column (SQL allows ORDER BY on any FROM-scope column even if unprojected). Applied at all three ORDER BY emission sites.Flavor B — mixed-case identifiers not quoted (all case-folding dialects)
Mixed-case identifiers (
accounts.StateFlag, join keycluster_analysis.CLSTR_PIN) were emitted unquoted, so case-folding dialects reach a non-existent name — Postgres/Redshift fold to lowercase, Snowflake/Oracle fold to uppercase. A context-aware quoting pass quotes real DB identifiers — column-name leaves and physical table-name parts — while leaving SLayer-internal aliases/qualifiers alone (they fold consistently within a query). Wired into both parse paths (_parseand_parse_predicate) and every direct AST-construction site. Universal across dialects (quote-if-contains-uppercase preserves the author's true catalog case everywhere).Tests
tests/test_dev1645_invalid_postgres_sql.py— 27 unit tests (both flavors, helper-level, multi-dialect quote-char matrix).UndefinedColumnfailures (fake_account_23,fake_account_15, ORDER-BY repro).Known limitations (documented, out of scope)
UnresolvableOrderColumnErrorat compile time (project the column, or order by a projected field) instead of emitting invalid SQL. The compiler has several outer-wrapping layers (measure CTEs, pagination, first/last ranked subquery, projection trim) that relocate the ORDER BY out of the base scope where a filter-pulled join alias is bound, so resolving joined ORDER BY columns isn't reliably safe — it's a deferred capability, tracked byxfail(strict=True)tests (TestFlavorAJoinedOrderByDeferred) that will flip to XPASS and fail the suite if it's ever implemented. This does not affect the reported cases: every reported failing ORDER BY qualifier is the query's ownsource_modelstage (a base column), which is still emitted as a valid split reference._basecombined / ranked-subquery paths (base-model qualifier treated as in-scope, so not rejected, but the outer FROM there is a CTE). Pre-fix behaviour, not a regression, no reported instance.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
ORDER BYbehavior for non-projected fields, including safer handling when a sort field cannot be resolved.Tests
ORDER BYscenarios across multiple database engines.