DEV-1369: pin two-mode reference semantics; drop predicate-promotion escape hatch#107
Merged
Merged
Conversation
The previous ranker `match_count = |wanted ∩ memory.entities|` trivially favoured memories with large entity sets — a memory tagged with 50 entities would out-overlap a precisely-tagged one of 2 regardless of relevance. `recall_memories` now ranks via BM25 (`rank_bm25.BM25Plus`, implemented in `slayer/memories/ranker.py`); IDF / avgdl are computed over the full memory corpus, an explicit set-intersection pre-filter enforces "must overlap on ≥1 entity," and BM25 is used purely to rank the eligible set. `RecallHit.match_count: int` becomes `RecallHit.score: float` across MCP, REST, CLI, and Python client. `BM25Plus` is used rather than `BM25Okapi` because at small corpus sizes the latter's IDF goes negative for terms that appear in even a moderate fraction of documents, and BM25's length normalisation inverts under negative IDF — broad memories rank above narrow ones, the exact bug DEV-1365 is trying to fix. `BM25Plus` uses `log((N+1)/df)` and stays positive. The empty-`about` recency fallback is unchanged. `inspect_model`'s Learnings section is unchanged (per-model browsing view, not a retrieval). `Memory` persisted shape is unchanged — no version bump. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three threads + one nitpick on the BM25 memory ranker landed; Sonar clean. All four are valid; fixes are confined to test wording, the DEV-1357 historical-note block, and a pyproject inline comment — no runtime code changes. - tests/test_memories_ranker.py: convert every _mem() / bm25_rank() call to keyword arguments per the global "kwargs for >1-param functions" rule, and rewrite the test_term_in_every_doc_still_returned comment to reflect that the implementation uses BM25Plus (not BM25Okapi — the latter is what BM25Plus is chosen to avoid). - specs/DEV-1357-memories.md: broaden the historical-note block so it enumerates explicitly that both the recall ranking algorithm AND the RecallHit response shape are superseded by DEV-1365. Spec body stays as a record of the v2 surface as shipped. - pyproject.toml: add an inline comment next to rank-bm25 noting the upstream is unmaintained at 0.2.2 with bm25s as the active alternative; the >= constraint stays. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…escape hatch
SLayer has two distinct expression layers — model-side (free SQL) and
query-side (pure DSL) — but the implementation had drifted: model-level
filters went through the same Python-AST DSL parser as query filters
(rejecting json_extract / coalesce / CASE WHEN), and query-side filters
silently passed unknown bare names through to underlying-table SQL.
This change formalises and enforces the two modes:
* **Mode A — SQL.** Column.sql / Column.filter / SlayerModel.filters
parse via sqlglot. Arbitrary SQL expressions are accepted (function
calls, CASE WHEN, IN, IS NULL, dialect operators). Aggregation colon
syntax, transform calls, and raw OVER (...) are rejected with an
actionable error pointing at the DSL.
* **Mode B — DSL.** ModelMeasure.formula / SlayerQuery.{measures,
filters, dimensions, time_dimensions, order, main_time_dimension}
parse via Python AST. Bare names must resolve to a Column / saved
ModelMeasure / custom Aggregation / canonical agg alias / query-level
alias on the source model — strict resolution at enrichment time
catches typos that previously produced silent SQL errors. Raw SQL
function calls are rejected; raw OVER (...) is rejected at construction.
The DEV-1336 escape hatch — auto-promoting a query filter against a
``Column`` whose ``sql`` contains a window function to a post-aggregation
outer WHERE — is removed. The rank-family transforms (DEV-1353) cover
top-N filtering in pure DSL, so the escape hatch is redundant. A query
filter naming a windowed Column now raises with a suggestion to use a
rank transform or a multi-stage source_queries model.
Opportunistic consolidation:
* New ``slayer/core/refs.py`` is the single source of truth for
identifier-shape regexes, agg-suffix parsing, and the user-input
dunder helper. Four prior duplicates (formula.py, dbt/converter.py,
engine/enrichment.py, memories/resolver.py) re-export from here.
* ``_walk_join_chain`` collapses the two near-duplicate join walkers
in query_engine.py (``_resolve_dimension_with_terminal`` and
``_resolve_cross_model_measure``).
A sibling Linear issue DEV-1370 tracks unifying the DSL formula parser
and filter parser behind one FieldSpec-producing entry point — deferred
to keep this PR's surface tight.
Spec: docs/concepts/references.md (new) is the single source of truth.
CLAUDE.md, docs/concepts/{models,queries}.md updated to point at it.
Tests: tests/test_reference_semantics.py covers Mode A acceptance /
rejection, Mode B acceptance / rejection, strict resolution at
enrichment, predicate-promotion removal, and the internal
``__``-in-Column-name carve-out used by ``_query_as_model`` for
virtual-model column flattening. 2208 unit tests + 168 Tier 1
integration tests (SQLite / Postgres / DuckDB) green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds BM25-based memory ranking (RecallHit.score, bm25_rank, rank-bm25 dep), centralizes identifier/aggregation refs, splits filter parsing into SQL vs DSL modes (parse_filter mode, parse_sql_predicate), enforces construction- and enrichment-time validation (reject raw OVER(...) and filters on windowed Column.sql), refactors join-walks, removes windowed-column materialization in SQL generation, and updates docs and tests. ChangesMemory Ranking (DEV-1365)
Reference Semantics & Filter Validation (DEV-1369)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Three groups of issues flagged on PR #107: **Group A — Drop unused `async`** on three SQL-generator tests (`test_drop_table_rejected`, `test_union_select_rejected`, `test_block_comment_passes_through_safely`). DEV-1369 moved SQL-mode filter validation up to ``Column.__init__`` so the ``await _generate(...)`` calls in their bodies became bare ``Column(...)`` constructions; the ``async`` keyword was leftover. (Sonar S7503 ×3.) **Group B — Mechanical regex / shorthand cleanups**: * ``slayer/core/refs.py:24`` — ``IDENTIFIER_RE`` switches the second character class from ``[a-zA-Z0-9_]`` to ``\w`` for consistency with ``IDENT_OR_PATH_RE`` and the rest of the module's regexes. (Sonar S6353.) * ``slayer/sql/sql_predicate.py:81`` — the ``\!=`` → ``<>`` rewrite in ``_normalize_operators`` is a literal pattern, not a regex; switch to ``str.replace``. The neighboring ``==`` → ``=`` rewrite keeps ``re.sub`` because it uses lookbehind/lookahead. (Sonar S5361.) **Group C — Drop cognitive complexity of ``_filter_node_to_sql``** from 53 to ~10 by extracting one handler per AST node kind (``_compare_to_sql``, ``_boolop_to_sql``, ``_unaryop_to_sql``, ``_attribute_to_sql``, ``_name_to_sql``, ``_constant_to_sql``, ``_seq_to_sql``, ``_binop_to_sql``, ``_call_to_sql``). The top-level function is now a thin dispatcher that closes over ``recur`` for the per-handler recursion threading; behaviour is unchanged. (Sonar S3776.) All 2208 unit tests + 168 Tier 1 integration tests still green; ruff clean. Co-Authored-By: Claude Opus 4.7 (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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
slayer/engine/enrichment.py (1)
337-350:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModel-side filters are still reparsed with the DSL defaults.
Column.filterandSlayerModel.filtersare SQL-mode surfaces now, but these enrichment call sites still invokeparse_filter(...)withoutmode="sql". A model that validates withlower(status) = 'active'orjson_extract(...) = 1will still break duringenrich_query()because it gets pushed back through the DSL parser here.Also applies to: 820-821, 902-912
🤖 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/enrichment.py` around lines 337 - 350, The model-side filters (Column.filter and SlayerModel.filters) are being reparsed with DSL defaults because parse_filter is called without mode="sql"; update the parse_filter calls used before resolve_filter_columns (including the call around parse_filter(...) -> resolved = await resolve_filter_columns(...) and the other occurrences you noted at the other call sites) to pass mode="sql" so existing SQL-mode filter expressions are preserved; keep the rest of the call arguments unchanged and run enrich_query-related paths (e.g., resolve_filter_columns, enrich_query) to ensure no other callers assume DSL defaults.slayer/core/formula.py (1)
919-968:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
parse_filterwithmode="sql"is dead code and its docstring is misleading.The docstring claims
mode="sql"is for model-side filters (Column.filter, SlayerModel.filters) and documents SQL grammar support (CASE WHEN, json_extract, etc.). However:
- All actual model-side filters use
parse_sql_predicate()directly, notparse_filter(mode="sql")parse_filter()is never called withmode="sql"anywhere in the codebase- The
mode="sql"branch still uses Python'sast.parse(), so it cannot parse SQL-only syntax likeCASE WHEN ... ENDThe working SQL parser is
parse_sql_predicate()inslayer/sql/sql_predicate.py, which uses sqlglot. Themode="sql"parameter inparse_filter()should either be removed as unused code or properly implemented with sqlglot instead of Python AST.🤖 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/core/formula.py` around lines 919 - 968, parse_filter's mode="sql" branch is dead/misleading: it never gets called, still uses ast.parse (so cannot parse SQL) and duplicates behavior of parse_sql_predicate; either remove the SQL mode entirely or implement it by delegating to the SQL parser. Fix by updating the function parse_filter (and its docstring) to drop support for mode="sql" and remove related code paths (_reject_dsl_constructs_in_sql and the else branch that treats processed as SQL), or alternatively replace the mode=="sql" branch to call parse_sql_predicate from slayer.sql.sql_predicate (and avoid ast.parse there), ensure _preprocess_* and has_window_function checks are applied appropriately, and update tests and docstring to reflect the chosen approach.
🧹 Nitpick comments (1)
tests/test_sql_generator.py (1)
3443-3460: ⚡ Quick winBlock-comment test doesn’t validate the advertised round-trip behavior
test_block_comment_passes_through_safelycurrently only checksColumn(...)construction. That won’t catch regressions in enrichment/generation round-tripping, which the docstring explicitly claims to cover.Proposed patch
- async def test_block_comment_passes_through_safely(self, orders_model: SlayerModel) -> None: + async def test_block_comment_passes_through_safely( + self, generator: SQLGenerator, orders_model: SlayerModel + ) -> None: @@ - col = Column( - name="benign", - sql="amount", - filter="status = 'a' /* x */ OR 1=1", - type=DataType.DOUBLE, - ) - # The filter survives construction and round-trips cleanly. - assert col.filter is not None + orders_model.columns.append( + Column( + name="benign", + sql="amount", + filter="status = 'a' /* x */ OR 1=1", + type=DataType.DOUBLE, + ) + ) + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="benign:sum")], + ) + sql = await _generate(generator=generator, query=query, model=orders_model) + self._assert_round_trips_cleanly(sql=sql, dialect=generator.dialect)🤖 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_sql_generator.py` around lines 3443 - 3460, The test only checks Column(...) construction but should also assert the documented round-trip through sqlglot; parse the filter string with sqlglot (e.g., via sqlglot.parse_one or equivalent), re-emit it (parsed.sql()) and assert the result preserves the block comment and logical structure (compare parsed.sql() to the expected SQL or that it contains "/* x */"), ensuring test_block_comment_passes_through_safely validates both construction (Column) and the parse->re-emit round-trip behavior described in the docstring.
🤖 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 @.claude/skills/slayer-models.md:
- Line 72: Update the guidance to remove the incorrect statement that SLayer
auto-promotes filters on windowed Column.sql values to an outer WHERE (this
behavior was removed in DEV-1369); instead state that raw OVER(...) in
ModelMeasure.formula is rejected at construction and that windowed Column.sql
cannot be filtered via auto-promotion—recommend using the inline rank(<measure>)
<= N transform for top‑N or explicitly materializing the windowed expression and
filtering post-aggregation via a dedicated transform step; reference Column.sql,
ModelMeasure.formula, and the filters guidance so reviewers can find and update
the line.
In @.claude/skills/slayer-overview.md:
- Around line 28-32: Update the condensed MCP tools list to include the missing
capability families for memory and schema-drift: add an “Also available” line
(or restore sections) mentioning memory tools (save_memory, recall_memories,
forget_memory) and schema-drift/drift validation/apply flows (documented engine
capabilities), while keeping the existing discovery, querying, model editing,
datasources, and ingestion entries (list_datasources, models_summary,
inspect_model, query, create_model, edit_model, delete_model, create_datasource,
describe_datasource, edit_datasource, delete_datasource,
ingest_datasource_models); ensure the overview text is concise and
non-misleading about agent capabilities.
In @.claude/skills/slayer-query.md:
- Line 69: Update the Top-N guidance to remove the sentence claiming that
filtering on a windowed Column.sql is auto-promoted to an outer WHERE (the
removed escape hatch) and instead state that windowed-column filters now raise
an error; recommend using rank(...), first(), last(), lag(), lead() in filters
or performing multi-stage queries (source_queries) as the supported
alternatives, and update any mention of ModelMeasure.formula to avoid suggesting
raw OVER(...) SQL or Column.sql windowing as a workaround; keep references to
SLayer behavior and the supported functions (rank, first, last, lag, lead) for
clarity.
In `@CLAUDE.md`:
- Line 51: The doc contradicts itself about predicate-promotion: update the
paragraph that still claims auto-promotion for windowed Column.sql filters to
state that predicate-promotion (DEV-1336) has been removed and that a query
filter referencing a Column whose Column.sql contains a window function now
raises an error with a suggested alternative (use a rank-family transform like
rank(<measure>) <= N or other transform calls). Replace the old "auto-promotion"
wording with the explicit error-and-suggestion behavior and mention the affected
symbols predicate-promotion, Column.sql, and the rank() transform so readers
know what to use instead.
In `@docs/concepts/references.md`:
- Around line 29-35: Update the docs to clarify that reject_user_dunder only
blocks user-authored input (e.g., at SlayerQuery or ModelMeasure construction)
but does not prevent internal virtual-model column names that include `__`
created by the internal factory `_query_as_model`; explicitly state that
`Column._validate_name` permits `__` for those synthetic columns (e.g.,
stores__name, customers__regions__name) and that such `__`-containing filters
remain constructible but may later fail during enrichment if they cannot be
resolved; apply the same clarification in the related paragraph covering lines
76-83.
- Around line 55-97: Replace the Python constructor-style examples (Column(...),
SlayerQuery(...), ModelMeasure(...)) with JSON/dict-style representations
throughout the shown accepted/rejected blocks so the page is language-agnostic;
keep the same field names and values (e.g., name, sql, filter for Column;
source_model, filters, dimensions, variables for SlayerQuery; name, formula for
ModelMeasure), preserve which examples are accepted vs rejected and the points
about where they are rejected (construction vs enrichment), and ensure DSL-mode
context remains explicit for those examples.
In `@slayer/engine/enrichment.py`:
- Around line 1629-1657: The regex-only check for is_canonical_agg is too
permissive — replace it with a membership test against the actual set of
synthesized canonical aggregation aliases built from the model's real
columns/measures and BUILTIN_AGGREGATIONS, not a pattern match. Concretely, in
the strict block (the code computing is_measure, is_custom_agg,
is_canonical_agg, is_query_alias) remove the _re.match logic and instead
generate the canonical alias set by iterating the model's concrete
column/measure names (use whatever accessor the model exposes, e.g.,
model.columns or model.get_column_names()) and combining each base name with
each entry in BUILTIN_AGGREGATIONS (including the optional leading
underscore/prefix logic your code uses), then set is_canonical_agg = col_name in
synthesized_alias_set; keep the other checks (model.get_measure,
model.get_aggregation, query_aliases) unchanged.
In `@slayer/sql/sql_predicate.py`:
- Around line 111-129: parse_sql_predicate() currently parses using sqlglot's
generic grammar and thus rejects valid backend-specific operators; thread a
sqlglot dialect through validation or defer dialect-specific checks to query
time: either (A) modify the call sites (e.g. where _fix_multidot_filters()
constructs/validates models) to pass a resolved dialect from the datasource
registry into parse_sql_predicate(formula, read=<dialect>) and propagate that
dialect through the validator/Model constructor, or (B) remove dialect-dependent
parsing from parse_sql_predicate and perform dialect-specific parsing/validation
inside SQLGenerator at query time; update parse_sql_predicate signature and all
callers (including _fix_multidot_filters) or move validation logic so model save
uses only generic checks and SQLGenerator enforces dialect rules.
In `@tests/test_reference_semantics.py`:
- Around line 264-266: The class docstring on TestDslModeRejection is
inconsistent with the test behavior: update the docstring to state that
double-underscore (`__`) in user input is rejected at enrichment time (not at
SlayerQuery/ModelMeasure construction), so it matches the asserted behavior in
test_query_filter_rejects_unknown_double_underscore_at_enrichment; mention the
lifecycle explicitly and keep references to SlayerQuery and ModelMeasure to
clarify that other constructs (raw SQL, OVER(...)) are rejected at construction
time while `__` is validated during enrichment.
---
Outside diff comments:
In `@slayer/core/formula.py`:
- Around line 919-968: parse_filter's mode="sql" branch is dead/misleading: it
never gets called, still uses ast.parse (so cannot parse SQL) and duplicates
behavior of parse_sql_predicate; either remove the SQL mode entirely or
implement it by delegating to the SQL parser. Fix by updating the function
parse_filter (and its docstring) to drop support for mode="sql" and remove
related code paths (_reject_dsl_constructs_in_sql and the else branch that
treats processed as SQL), or alternatively replace the mode=="sql" branch to
call parse_sql_predicate from slayer.sql.sql_predicate (and avoid ast.parse
there), ensure _preprocess_* and has_window_function checks are applied
appropriately, and update tests and docstring to reflect the chosen approach.
In `@slayer/engine/enrichment.py`:
- Around line 337-350: The model-side filters (Column.filter and
SlayerModel.filters) are being reparsed with DSL defaults because parse_filter
is called without mode="sql"; update the parse_filter calls used before
resolve_filter_columns (including the call around parse_filter(...) -> resolved
= await resolve_filter_columns(...) and the other occurrences you noted at the
other call sites) to pass mode="sql" so existing SQL-mode filter expressions are
preserved; keep the rest of the call arguments unchanged and run
enrich_query-related paths (e.g., resolve_filter_columns, enrich_query) to
ensure no other callers assume DSL defaults.
---
Nitpick comments:
In `@tests/test_sql_generator.py`:
- Around line 3443-3460: The test only checks Column(...) construction but
should also assert the documented round-trip through sqlglot; parse the filter
string with sqlglot (e.g., via sqlglot.parse_one or equivalent), re-emit it
(parsed.sql()) and assert the result preserves the block comment and logical
structure (compare parsed.sql() to the expected SQL or that it contains "/* x
*/"), ensuring test_block_comment_passes_through_safely validates both
construction (Column) and the parse->re-emit round-trip behavior described in
the docstring.
🪄 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: 641d7f9a-f0cf-484b-acee-103adf49326c
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.claude/skills/slayer-models.md.claude/skills/slayer-overview.md.claude/skills/slayer-query.mdCLAUDE.mddocs/concepts/memories.mddocs/concepts/models.mddocs/concepts/queries.mddocs/concepts/references.mdpyproject.tomlslayer/cli.pyslayer/core/formula.pyslayer/core/models.pyslayer/core/query.pyslayer/core/refs.pyslayer/dbt/converter.pyslayer/engine/enriched.pyslayer/engine/enrichment.pyslayer/engine/query_engine.pyslayer/mcp/server.pyslayer/memories/models.pyslayer/memories/ranker.pyslayer/memories/resolver.pyslayer/memories/service.pyslayer/sql/generator.pyslayer/sql/sql_predicate.pyspecs/DEV-1357-memories.mdtests/integration/test_integration.pytests/integration/test_integration_duckdb.pytests/integration/test_integration_postgres.pytests/test_memories_cli.pytests/test_memories_client.pytests/test_memories_mcp.pytests/test_memories_ranker.pytests/test_memories_rest.pytests/test_reference_semantics.pytests/test_sql_generator.py
💤 Files with no reviewable changes (2)
- slayer/engine/enriched.py
- slayer/sql/generator.py
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
slayer/sql/sql_predicate.py (1)
111-126:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPass dialect into SQL-mode predicate parsing to avoid backend-valid syntax being rejected.
Line 125 parses with sqlglot’s generic grammar only. That can reject dialect-specific operators/functions during model validation, which conflicts with the SQL-mode “free SQL” contract.
Possible fix direction
-def parse_sql_predicate(formula: str) -> ParsedFilter: +def parse_sql_predicate(*, formula: str, dialect: str | None = None) -> ParsedFilter: @@ - parsed = sqlglot.parse_one(normalized, into=exp.Condition) + parsed = sqlglot.parse_one( + normalized, + into=exp.Condition, + read=dialect, + )Then thread
dialectfrom the caller where datasource context is available.#!/bin/bash set -euo pipefail # 1) Verify current call sites (to confirm whether dialect context is propagated) rg -nP '\bparse_sql_predicate\s*\(' --type=py # 2) Reproduce generic-vs-dialect parse behavior in sqlglot poetry run python - <<'PY' import sqlglot from sqlglot import exp samples = [ ("postgres", "payload @> '{\"k\":1}'"), ("postgres", "name ILIKE ANY (ARRAY['a%', 'b%'])"), ("mysql", "a <=> b"), ] for dialect, expr in samples: print(f"\n{dialect}: {expr}") for label, kwargs in [("generic", {}), (dialect, {"read": dialect})]: try: sqlglot.parse_one(expr, into=exp.Condition, **kwargs) print(f" {label}: OK") except Exception as exc: print(f" {label}: FAIL {type(exc).__name__}: {exc}") PY🤖 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/sql_predicate.py` around lines 111 - 126, parse_sql_predicate currently parses with sqlglot's generic grammar which rejects dialect-specific syntax; update parse_sql_predicate to accept an optional dialect parameter (e.g., dialect: Optional[str] = None) and pass it through to sqlglot.parse_one using the read= dialect (or appropriate sqlglot arg) so dialect-specific operators/functions are accepted; then thread this dialect argument from each caller site where datasource/context is known so callers supply the correct dialect when validating SQL-mode predicates (identify parse_sql_predicate and its callers to propagate the new parameter).
🧹 Nitpick comments (1)
slayer/sql/sql_predicate.py (1)
38-39: ⚡ Quick winUse keyword arguments for multi-parameter calls to match repo rule.
These calls currently pass multiple positional arguments. Convert them to keyword arguments for consistency with project standards.
Small compliance diff
def _strip_string_literals(formula: str) -> str: @@ - return _STRING_LITERAL_RE.sub("''", formula) + return _STRING_LITERAL_RE.sub(repl="''", string=formula) @@ - part = re.sub(r"(?<![<>=!])==(?!=)", "=", part) + part = re.sub(pattern=r"(?<![<>=!])==(?!=)", repl="=", string=part)As per coding guidelines:
**/*.py: Use keyword arguments for functions with more than 1 parameter.Also applies to: 79-80
🤖 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/sql_predicate.py` around lines 38 - 39, Replace positional arguments with keyword arguments for the regex substitution calls: change uses of _STRING_LITERAL_RE.sub("''", formula) (and the similar calls around lines 79-80) to _STRING_LITERAL_RE.sub(repl="''", string=formula) so the .sub method uses named parameters; update each occurrence of _STRING_LITERAL_RE.sub(...) accordingly.
🤖 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.
Duplicate comments:
In `@slayer/sql/sql_predicate.py`:
- Around line 111-126: parse_sql_predicate currently parses with sqlglot's
generic grammar which rejects dialect-specific syntax; update
parse_sql_predicate to accept an optional dialect parameter (e.g., dialect:
Optional[str] = None) and pass it through to sqlglot.parse_one using the read=
dialect (or appropriate sqlglot arg) so dialect-specific operators/functions are
accepted; then thread this dialect argument from each caller site where
datasource/context is known so callers supply the correct dialect when
validating SQL-mode predicates (identify parse_sql_predicate and its callers to
propagate the new parameter).
---
Nitpick comments:
In `@slayer/sql/sql_predicate.py`:
- Around line 38-39: Replace positional arguments with keyword arguments for the
regex substitution calls: change uses of _STRING_LITERAL_RE.sub("''", formula)
(and the similar calls around lines 79-80) to _STRING_LITERAL_RE.sub(repl="''",
string=formula) so the .sub method uses named parameters; update each occurrence
of _STRING_LITERAL_RE.sub(...) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ddfb28f9-d8ef-45ca-9421-9cc0c79c5d0c
📒 Files selected for processing (4)
slayer/core/formula.pyslayer/core/refs.pyslayer/sql/sql_predicate.pytests/test_sql_generator.py
✅ Files skipped from review due to trivial changes (2)
- slayer/core/refs.py
- slayer/core/formula.py
Three groups of issues flagged on round 1:
**Group 1 — Docs alignment with the at-enrichment lifecycle.**
The DEV-1369 round-1 fix moved several rejection paths from construction
time to enrichment time (notably ``__`` in user input, since virtual-model
columns produced by ``_query_as_model`` legitimately contain ``__``).
Several docs / skill files / test docstrings were left describing the
old construction-time behavior. Sync everything to the at-enrichment
shape:
- ``.claude/skills/slayer-models.md:72`` — drop the auto-promotion
guidance for filters on windowed ``Column.sql``; recommend
rank-family transforms / multi-stage source_queries instead.
- ``.claude/skills/slayer-overview.md:32`` — add the Memory and
Schema-drift MCP tool families to the discoverability list.
- ``.claude/skills/slayer-query.md:69`` — drop the "auto-wraps" claim;
state plainly that filtering on a windowed ``Column.sql`` is
rejected (use rank() or a multi-stage model).
- ``CLAUDE.md:76`` — same; align with the line-51 statement about
predicate-promotion removal.
- ``docs/concepts/references.md:33-35`` — explain the at-enrichment
carve-out for ``__``: virtual-model column names need to be
representable, so we don't reject ``__`` at construction; strict
resolution at enrichment surfaces typos.
- ``docs/concepts/references.md:97`` — switch the accepted/rejected
examples from Python constructors to JSON/dict per the project's
``docs/CLAUDE.md`` style rule.
- ``tests/test_reference_semantics.py:266`` — align the
``TestDslModeRejection`` class docstring with the at-enrichment
behavior.
**Group 2 — Plug the strict-resolution regex bypass.**
``resolve_filter_columns`` used a permissive regex
``^_?[a-zA-Z_]\w*?_(sum|count|...)`` to recognise canonical agg
aliases the parser had introduced from colon syntax. The shape-only
match let typos like ``made_up_sum`` or ``typo_count_distinct``
through, reintroducing the silent raw-SQL leak DEV-1369 was meant to
close. Replaced with explicit per-filter membership: ``parse_filter``
now records the exact set of canonical aliases it synthesised on the
returned ``ParsedFilter.synthesized_aliases``, and the strict-
resolution check verifies bare names against that set instead of a
shape regex. Added a regression test covering three typo shapes.
**Group 3 — Stop sqlglot-parsing in the SQL-mode validator.**
``parse_sql_predicate`` previously invoked ``sqlglot.parse_one(...,
into=exp.Condition)`` with no ``read=`` argument, so generic-grammar
parsing would reject Postgres ``payload @> '{"k":1}'`` (verified
empirically) at ``Column``/``SlayerModel`` construction even though
the operator is valid for that backend. The validator now does
regex-only DSL-construct rejection (aggregation colon, transform
calls, raw OVER) and ``ParsedFilter.sql`` is the original formula.
Dialect-aware sqlglot parsing already happens at SQL-generation time
via the existing formula resolver, where the datasource's dialect is
known.
Test consequence: the two SQL-injection rejection tests
(``test_drop_table_rejected``, ``test_union_select_rejected``) now
assert at SQL generation time instead of at Column construction —
the formula parser still refuses the multi-statement / UNION
payloads when the column is actually used in a query. The
``test_block_comment_passes_through_safely`` test stays at
construction-only assertion (round-trip through enrichment depends on
the deeper at-enrichment-with-dialect refactor that's a separate
follow-up). Applied the related CodeRabbit nitpick by extending the
docstring to reflect this contract.
All 2209 unit tests + 168 Tier 1 integration tests (SQLite +
DuckDB + Postgres) green; ruff clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Member
Author
|
@coderabbitai responses to the two summary nitpicks on this round:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
5 tasks
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
SLayer has two distinct expression layers — models (the boundary that lifts raw SQL tables into the SLayer DSL) and queries (pure-DSL descriptions of what data to retrieve) — but the implementation had drifted. This PR pins the intended semantics and enforces them.
Column.sql,Column.filter,SlayerModel.filters): sqlglot-parsed free SQL. Accepts arbitrary function calls (json_extract,coalesce,nullif,CASE WHEN, …), dialect operators, and__-delimited join paths (customers__regions.name). Rejects DSL constructs (aggregation colon syntax, transform calls, rawOVER (...)in filters).ModelMeasure.formula,SlayerQuery.{measures, filters, dimensions, time_dimensions, order, main_time_dimension}): Python-AST DSL. Bare names strict-resolve at enrichment time against the model's definedColumn/ModelMeasure/ custom aggregation / query-level alias set. Raw SQL function calls and unknown names are rejected with actionable messages.Columnwhosesqlcontained a window function to a post-aggregation outerWHERE— is removed. The rank-family transforms (DEV-1353) cover top-N filtering in pure DSL. A query filter naming a windowedColumnnow raises with a suggestion to use a rank transform or a multi-stagesource_queriesmodel.Opportunistic consolidation:
slayer/core/refs.pyis the single source of truth for identifier-shape regexes (AGG_REF_RE,IDENT_OR_PATH_RE,DOTTED_IDENT_REF_RE,IDENTIFIER_RE), the agg-suffix parser (canonical_agg_name,strip_agg_suffix), and the user-input dunder helper. Four prior duplicates (formula.py,dbt/converter.py,engine/enrichment.py,memories/resolver.py) re-export from here._walk_join_chaincollapses the two near-duplicate join walkers inquery_engine.py(_resolve_dimension_with_terminaland_resolve_cross_model_measure).Sibling DEV-1370 tracks unifying the DSL formula parser and filter parser behind one
FieldSpec-producing entry point — deferred to keep this PR's surface tight.Spec
docs/concepts/references.mdis the new single source of truth.CLAUDE.md,docs/concepts/queries.md, anddocs/concepts/models.mdupdated to point at it.Migration notes for users
Column.filterorSlayerModel.filtersentry containing aggregation colon syntax (revenue:sum > 100) or a SLayer transform call (cumsum(...)) will now fail at load. Move the predicate to a query-level filter or aModelMeasure.formula.SlayerQueryobjects that filter on a column whosesqlcontains a window function will now raise. Switch tofilters=["rank(<measure>) <= N"]or factor the window column into a multi-stagesource_queriesmodel.SlayerQueryobjects that filter on bare names not declared asColumnentries on the model (silent pass-through to underlying-table columns) will now raise at enrichment. Define the column on the model first.Test plan
poetry run pytest -m \"not integration\"— 2208 passedpoetry run pytest tests/integration/test_integration.py -m integration(SQLite) — greenpoetry run pytest tests/integration/test_integration_duckdb.py -m integration— greenpoetry run pytest tests/integration/test_integration_postgres.py -m integration— greenpoetry run ruff check slayer/ tests/— cleantests/test_reference_semantics.pycovers Mode A acceptance/rejection, Mode B acceptance/rejection, strict resolution at enrichment, predicate-promotion removal, and the internal__-in-Column.namecarve-out used by_query_as_modelfor virtual-model column flattening.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Documentation