DEV-1726: dialect-aware case folding for CTE name collision detection - #273
Conversation
…oach-expressions-crossing-joins-on-the' into egor/dev-1726-dialect-aware-case-folding-for-cte-name-collision-detection # Conflicts: # DECISIONS.md
AliasAllocator gains folds_case (comparison-only str.lower() folding; allocated names keep original case) and assert_unique_cte_names folds per dialect, so case-differing user aliases that both generate CTEs (shifted_Foo/shifted_foo) dedup instead of colliding after the backend folds. Policy lives in naming.py (CASE_FOLDING_SQLGLOT_DIALECTS + KNOWN_CASE_SENSITIVE_SQLGLOT_DIALECTS + dialect_folds_case); the single SQLGenerator._new_allocator factory threads it to all former construction sites (test-pinned as the only one). Fold set = every registry dialect except ClickHouse; unknown strings stay exact. Issue-text corrections (GoogleSQL docs + sqlglot + empirics): BigQuery FOLDS (CTE names are query aliases, case-insensitive); SQLite/DuckDB fold even quoted names. MySQL/T-SQL fold deliberately despite config-dependence (rename-only-safe). Belt folds regardless of quoting — over-strict by design on allocator-sanitized output. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
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 centralizes SQL naming policies, adds dialect-aware case-folding allocation, validates CTE name collisions, routes generator allocators through one factory, and expands regression coverage for aliases, CTEs, SQL validation, and result-key preservation. ChangesNaming and SQL generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SQLGenerator
participant AliasAllocator
participant CTEValidator
SQLGenerator->>AliasAllocator: create dialect-aware allocator
SQLGenerator->>AliasAllocator: allocate aliases and CTE names
AliasAllocator-->>SQLGenerator: return names with preserved casing
SQLGenerator->>CTEValidator: validate generated WITH scopes
CTEValidator-->>SQLGenerator: return validation result or collision error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
tests/_engine_helpers.py (1)
33-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the three duplicate
_assert_valid_sqlimplementations. All three files define an identical_assert_valid_sqlhelper (parse, statement-count assert,TypeError→AssertionErrorre-raise, nested-WITH check). This PR had to apply the same BigQuery-carve-out removal in all three places by hand, which is exactly the kind of drift risk that duplicated test helpers create.
tests/_engine_helpers.py#L33-L49: keep this as the canonical_assert_valid_sqldefinition (it is already imported as a shared test utility module).tests/dialects/conftest.py#L31-L45: remove the local copy and import_assert_valid_sqlfromtests/_engine_helpers.pyinstead.tests/test_sql_generator.py#L179-L196: remove the local copy and import_assert_valid_sqlfromtests/_engine_helpers.pyinstead.🤖 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/_engine_helpers.py` around lines 33 - 49, Consolidate the duplicate _assert_valid_sql helpers by keeping tests/_engine_helpers.py#L33-L49 as the canonical implementation, including parsing, statement-count validation, nested-WITH checking, and TypeError-to-AssertionError handling. Remove the local definitions from tests/dialects/conftest.py#L31-L45 and tests/test_sql_generator.py#L179-L196, and import and reuse the canonical helper in both files.slayer/engine/stage_planner.py (1)
1084-1110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared saved-measure lookup.
_bare_saved_measure_nameduplicates the guard clauses of_saved_model_measure_type(isinstance(scope, ModelScope)check,bare.isidentifier()check,scope.source_model.get_measure(bare)lookup). Only the returned attribute differs (.namevs.type).Extract a shared helper that returns the matched
ModelMeasure(orNone), and have both callers read the field they need.♻️ Proposed consolidation
+def _bare_saved_measure( + *, scope: Union[ModelScope, StageSchema], formula: str, +) -> Optional["ModelMeasure"]: + """The saved ``ModelMeasure`` when ``formula`` is a bare identifier + matching a ``ModelMeasure.name`` on the source model, else ``None``.""" + if not isinstance(scope, ModelScope) or scope.source_model is None: + return None + bare = formula.strip() + if not bare.isidentifier(): + return None + return scope.source_model.get_measure(bare) + + def _saved_model_measure_type( *, scope: Union[ModelScope, StageSchema], formula: str, ) -> Optional[DataType]: ... - if not isinstance(scope, ModelScope) or scope.source_model is None: - return None - bare = formula.strip() - if not bare.isidentifier(): - return None - saved = scope.source_model.get_measure(bare) - return saved.type if saved is not None else None + saved = _bare_saved_measure(scope=scope, formula=formula) + return saved.type if saved is not None else None def _bare_saved_measure_name( *, scope: Union[ModelScope, StageSchema], formula: str, ) -> Optional[str]: ... - if not isinstance(scope, ModelScope) or scope.source_model is None: - return None - bare = formula.strip() - if not bare.isidentifier(): - return None - saved = scope.source_model.get_measure(bare) - return saved.name if saved is not None else None + saved = _bare_saved_measure(scope=scope, formula=formula) + return saved.name if saved is not None else NoneAlso applies to: 1113-1134
🤖 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/stage_planner.py` around lines 1084 - 1110, Extract the shared saved-measure lookup from _saved_model_measure_type and _bare_saved_measure_name into a helper that validates ModelScope, accepts only bare identifiers, and returns the matched ModelMeasure or None. Update both callers to use this helper, then read .type in _saved_model_measure_type and .name in _bare_saved_measure_name while preserving their existing fallback behavior.tests/test_dev1713_naming.py (2)
63-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the pytest
tmp_pathfixture instead oftempfile.mkdtemp().
tempfile.mkdtemp()creates a directory that is never removed. Theenginefixture is function-scoped, so each test inTestJoinedDimensionDottedKeysandTestMultiStageNamingleaves one temp directory with a SQLite file and a YAML store behind.tmp_pathgives per-test isolation and automatic cleanup.♻️ Proposed change
`@pytest.fixture` -async def engine() -> AsyncIterator[SlayerQueryEngine]: - d = tempfile.mkdtemp() - db_path = os.path.join(d, "t.db") +async def engine(tmp_path) -> AsyncIterator[SlayerQueryEngine]: + d = str(tmp_path) + db_path = os.path.join(d, "t.db")Then drop the now-unused
tempfileimport.🤖 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_dev1713_naming.py` around lines 63 - 66, Update the async engine fixture to accept pytest’s tmp_path fixture and build the database path from it instead of calling tempfile.mkdtemp(). Remove the now-unused tempfile import while preserving the existing per-test database setup.
486-508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the imports to module scope, and prefer a behavioral assertion over source-text inspection.
Two points:
- The
inspectand module imports sit inside the test bodies. The coding guidelines require imports at the top of files.SlayerQueryEngineis already imported at line 42, so the local re-import is redundant.assert "flat_name(" in srccouples the test to source text. A refactor that keeps the delegation but renames the call site or wraps it in a helper breaks the test, while a body that inlines.replace('.', '__')and also mentionsflat_namein a comment still passes. If the goal is single ownership of flattening, patchingflat_nameand asserting that the flattener calls it gives the same guarantee without the text dependency.♻️ Proposed change for the import placement
+import inspect + ... -from slayer.engine.query_engine import SlayerQueryEngine +from slayer.engine.enrichment import enrich_query +from slayer.engine.query_engine import SlayerQueryEnginedef test_query_as_model_delegates_to_flat_name(self) -> None: - import inspect - - from slayer.engine.query_engine import SlayerQueryEngine - src = inspect.getsource(SlayerQueryEngine._query_as_model) assert "flat_name(" in src, "expected _query_as_model to call flat_name()" def test_enrich_query_delegates_to_flat_name(self) -> None: - import inspect - - from slayer.engine.enrichment import enrich_query - src = inspect.getsource(enrich_query) assert "flat_name(" in src, "expected enrich_query to call flat_name()"Note that both flatteners are closures, so a patch-based assertion needs the patch target in the defining module namespace. Confirm the import style at each definition site before you switch approach.
As per coding guidelines: "Use keyword arguments for functions with more than one parameter, and 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_dev1713_naming.py` around lines 486 - 508, Move inspect and module imports to module scope, removing the redundant local SlayerQueryEngine import already available at file scope. Replace source-text checks in TestLegacyFlattenerDelegation with behavioral tests that patch flat_name in each defining module namespace and assert both closure flatteners invoke it, preserving the single-owner flattening guarantee; follow the existing import style and use keyword arguments for multi-parameter calls.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.
Nitpick comments:
In `@slayer/engine/stage_planner.py`:
- Around line 1084-1110: Extract the shared saved-measure lookup from
_saved_model_measure_type and _bare_saved_measure_name into a helper that
validates ModelScope, accepts only bare identifiers, and returns the matched
ModelMeasure or None. Update both callers to use this helper, then read .type in
_saved_model_measure_type and .name in _bare_saved_measure_name while preserving
their existing fallback behavior.
In `@tests/_engine_helpers.py`:
- Around line 33-49: Consolidate the duplicate _assert_valid_sql helpers by
keeping tests/_engine_helpers.py#L33-L49 as the canonical implementation,
including parsing, statement-count validation, nested-WITH checking, and
TypeError-to-AssertionError handling. Remove the local definitions from
tests/dialects/conftest.py#L31-L45 and tests/test_sql_generator.py#L179-L196,
and import and reuse the canonical helper in both files.
In `@tests/test_dev1713_naming.py`:
- Around line 63-66: Update the async engine fixture to accept pytest’s tmp_path
fixture and build the database path from it instead of calling
tempfile.mkdtemp(). Remove the now-unused tempfile import while preserving the
existing per-test database setup.
- Around line 486-508: Move inspect and module imports to module scope, removing
the redundant local SlayerQueryEngine import already available at file scope.
Replace source-text checks in TestLegacyFlattenerDelegation with behavioral
tests that patch flat_name in each defining module namespace and assert both
closure flatteners invoke it, preserving the single-owner flattening guarantee;
follow the existing import style and use keyword arguments for multi-parameter
calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bec582b5-656b-42d1-b03e-4db2a1196e5c
📒 Files selected for processing (23)
DECISIONS.mdslayer/engine/enrichment.pyslayer/engine/query_engine.pyslayer/engine/response_meta.pyslayer/engine/stage_planner.pyslayer/sql/dialects/_alias_mangle.pyslayer/sql/dialects/bigquery.pyslayer/sql/dialects/tsql.pyslayer/sql/generator.pyslayer/sql/naming.pyslayer/sql/scope_check.pyslayer/sql/stage_wrapper.pytests/_engine_helpers.pytests/dialects/conftest.pytests/dialects/test__alias_mangle.pytests/integration/test_notebooks.pytests/parity_xfails.pytests/test_dev1713_naming.pytests/test_dev1726_cte_case_folding.pytests/test_naming.pytests/test_projection_trim.pytests/test_scope_check.pytests/test_sql_generator.py
💤 Files with no reviewable changes (4)
- tests/test_projection_trim.py
- tests/parity_xfails.py
- tests/integration/test_notebooks.py
- slayer/sql/dialects/_alias_mangle.py
- S9073: split the composite belt-message assertion into two asserts. - S7632: the time_shift emitter's suppression comment ended with the literal word NOSONAR, which Sonar parsed as a second malformed suppression tag — reworded to "sibling emitters' suppressions". - S3776: _emit_consecutive_periods_ctes_for_planned gains the same cohesive-emitter suppression its siblings carry (per the documented sibling-emitter rationale), instead of a scattering refactor. - CodeRabbit nitpick: the three identical _assert_valid_sql copies consolidate onto tests/_engine_helpers.py; tests/dialects/conftest.py and tests/test_sql_generator.py now import it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…oach-expressions-crossing-joins-on-the' into egor/dev-1726-dialect-aware-case-folding-for-cte-name-collision-detection # Conflicts: # DECISIONS.md
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 `@DECISIONS.md`:
- Line 80: Update the DECISIONS.md entry so the compound modifier reads
“crossing-derived column” instead of “crossing derived column,” preserving the
surrounding wording and meaning.
🪄 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: 6a089baf-9c63-42d2-b93a-80aacab497e7
📒 Files selected for processing (4)
DECISIONS.mdslayer/engine/stage_planner.pyslayer/sql/generator.pytests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (3)
- slayer/engine/stage_planner.py
- tests/test_sql_generator.py
- slayer/sql/generator.py
…oach-expressions-crossing-joins-on-the' into egor/dev-1726-dialect-aware-case-folding-for-cte-name-collision-detection # Conflicts: # DECISIONS.md # slayer/sql/generator.py # tests/test_dev1713_naming.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slayer/sql/generator.py (1)
5771-5779: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winShare the generation-wide allocator here for consistency.
placeholder_allocator = self._new_allocator()builds a standalone allocator instead of reusingself._gen_allocator. Every other comparable call site in this diff (lines 4215, 5107, 6801, 8596) usesself._gen_allocator or self._new_allocator()to keep_val_<n>materialisation names unique across the whole generation (DEV-1708 Law 2).
_render_with_cross_model_plansis reachable only fromgenerate_from_planned, which always setsself._gen_allocatorbefore this code runs. Today this is not a live bug:_resolve_where_filter_joins_via_scopecallsscope.resolve(...)without aconsumerargument, so_materialize/allocate_valnever fires throughplaceholder_scope. But the standalone allocator breaks the single-allocator invariant this PR establishes. If a future change routes a materializingresolve(..., consumer=...)call through this scope, its_val_<n>counter would restart from_val_0and could collide with names minted by the sharedself._gen_allocatorelsewhere in the same generation.Change this to
self._gen_allocator or self._new_allocator()to match the pattern used at the other four sites.♻️ Proposed fix
- placeholder_allocator = self._new_allocator() + placeholder_allocator = self._gen_allocator or self._new_allocator()🤖 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 5771 - 5779, Update the allocator initialization in _render_with_cross_model_plans to use self._gen_allocator when available, falling back to self._new_allocator() otherwise. Keep the existing placeholder_scope construction unchanged and preserve the shared generation-wide allocator invariant.
🤖 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 `@slayer/sql/generator.py`:
- Around line 5771-5779: Update the allocator initialization in
_render_with_cross_model_plans to use self._gen_allocator when available,
falling back to self._new_allocator() otherwise. Keep the existing
placeholder_scope construction unchanged and preserve the shared generation-wide
allocator invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 19212933-0f31-42ca-a842-bc4f8d4adfce
📒 Files selected for processing (4)
DECISIONS.mdslayer/sql/generator.pytests/test_dev1713_naming.pytests/test_sql_generator.py
💤 Files with no reviewable changes (1)
- tests/test_sql_generator.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_dev1713_naming.py
…der site CodeRabbit: placeholder_allocator now uses the `self._gen_allocator or self._new_allocator()` form like the other four comparable sites, so a future materializing resolve through the placeholder scope cannot restart the _val_<n> counter and collide with names minted elsewhere in the same generation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
385d377
into
egor/dev-1703-comprehensive-approach-expressions-crossing-joins-on-the
1 participant
Follow-up from DEV-1713 Stage 9 (Codex review of PR #269). Closes DEV-1726.
Problem
AliasAllocatorandassert_unique_cte_namescompared CTE names by exact string, but generated CTE names are emitted unquoted — so on case-folding backends, two user measure aliases differing only in case (Foo/foo) that both generate CTEs producedshifted_Foo/shifted_foo, a duplicateWITHname after the backend folds (reproduced on Postgres).Fix
AliasAllocatorgainsfolds_case: every_takencomparison folds withstr.lower()(comparison only — allocated names keep the caller's case), so the second family walks toshifted_foo_2/sjoin_foo_2and every reference follows the returned name.assert_unique_cte_namesfolds per dialect (same signature; policy resolved internally). The belt folds regardless of quoting — deliberately over-strict on SLayer's own allocator-sanitized output, where a fold-collision always signals an allocator-bypass bug.naming.py:CASE_FOLDING_SQLGLOT_DIALECTS+ explicitKNOWN_CASE_SENSITIVE_SQLGLOT_DIALECTS(so a future dialect can't skip the decision — registry-guard-tested) +dialect_folds_case()(inputstrip().lower()-normalized; unknown strings stay exact).SQLGenerator._new_allocator()factory threads the policy to all 8 former construction sites and is test-pinned as the module's onlyAliasAllocatorconstruction site.Fold-set corrections vs the issue text (evidence-backed)
CASE_INSENSITIVE.duplicate WITH table nameon the quoted pair), correcting the "quoted SQLite" parenthetical.Tests
52 new tests in
tests/test_dev1726_cte_case_folding.py: fold-set membership for all 14 registry dialects + registry-fully-classified guard; allocator fold/exact/suffix-walk/lower-not-casefoldsemantics; belt fold/exact/quoted/nested-scope/message-content; e2eFoo/footime_shift vehicle across all 14 dialects (exact renamed families, structural CTE-reference check, user-facing result keys survive byte-identical);cp_reset_*/cp_value_*sibling-family vehicle; single-construction-site guard. Full non-integration suite: 8337 passed.Also includes the merge of
origin/egor/dev-1703-…(Stages 4+6) into this branch, resolving the Stage 4 × Stage 9 semantic conflict perDECISIONS.md(the user-approved derived-shared-grain raise wins; the two Stage-9 Codex-F6 agreement tests split their coverage into separate queries).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation