From 42e887f5586919e688fe3779b4fc855599506efe Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 13:07:55 +0200 Subject: [PATCH 1/7] DEV-1833: harden Mode-B keyword lexing (CASE/LIKE) for Unicode identifiers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tokenize complete Unicode/dotted identifiers in the CASE rewriter, gate CASE lowering on a depth-0 WHEN, and make keyword recognition ASCII-exact so a name named after, containing, or qualified by a SQL keyword (case, customers.case, écase, customers.end in a THEN value) is never captured. Rework the LIKE rewriter with ASCII keyword classes (drops IGNORECASE, which folded spoofs like lıke), an escape-aware pattern, and a string-literal-span skip; same ASCII treatment for OVER and the SQL operator-keyword rewrites. Adds the regression suite and the OpenSpec change (new expression-keywords capability). --- docs/concepts/formulas.md | 3 + .../.openspec.yaml | 2 + .../design.md | 86 ++++ .../proposal.md | 63 +++ .../specs/queries/expression-keywords/spec.md | 131 ++++++ .../tasks.md | 73 +++ slayer/engine/syntax.py | 439 +++++++----------- tests/test_dev1833_keyword_lexing.py | 284 +++++++++++ 8 files changed, 809 insertions(+), 272 deletions(-) create mode 100644 openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml create mode 100644 openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md create mode 100644 openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md create mode 100644 openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md create mode 100644 openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md create mode 100644 tests/test_dev1833_keyword_lexing.py diff --git a/docs/concepts/formulas.md b/docs/concepts/formulas.md index 2565ea29..1bb5a1b2 100644 --- a/docs/concepts/formulas.md +++ b/docs/concepts/formulas.md @@ -526,6 +526,9 @@ Any formula, filter, or field expression can branch with SQL `CASE`: - **Searched** (`CASE WHEN c1 THEN v1 [WHEN c2 THEN v2 …] [ELSE d] END`) and **simple** (`CASE x WHEN v1 THEN r1 … END`, lowered to `x = v1`) forms are both accepted; keywords are case-insensitive and CASE nests anywhere. +- Identifiers named after, containing, or qualified by SQL keywords (`case`, + `customers.end`, `écase`) always parse as ordinary references — `CASE` starts + a conditional only when a `WHEN` follows it. - A missing `ELSE` yields `NULL`. `iif(cond, then, otherwise)` is an equivalent spelling — an allowlisted scalar function taking exactly three arguments. Everything renders to a portable SQL `CASE` on every Tier-1 dialect. diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml new file mode 100644 index 00000000..1d9aeef9 --- /dev/null +++ b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-04 diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md new file mode 100644 index 00000000..e0006af2 --- /dev/null +++ b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md @@ -0,0 +1,86 @@ +# Design + +## Context + +See proposal.md — Why. The rewriter layer at stake runs on the raw Mode-B string before +`ast.parse`: `_rewrite_case_when` (all expressions, `slayer/engine/syntax.py:420`) and +`_rewrite_sql_like` + `_normalize_sql_filter_operators` (filters only). Verified current +failures: bare `case` / `customers.case` / `écase` raise "Malformed CASE"; `customers.end` +in a THEN value yields `iif(a, customers., None) ELSE 0 END` (silent corruption); +`_rewrite_sql_like` rewrites inside string literals and truncates escaped-quote patterns. +The legacy `formula.parse_filter` has one production caller +(`schema_drift._filter_refs_dsl`, already `try/except → []`); its `.sql` output is +production-dead since DEV-1450. DEV-1826 makes both aggregation spellings parse natively +in the typed parser (verified: `sum(revenue) > 100`, `revenue:countd > 5`, `*:count > 3` +all yield `AggCall`), so the migration needs no funcstyle pre-rewrite. + +## Goals / Non-Goals + +Goals: keyword-safe lexing per the spec; exactly one Mode-B filter parser and one LIKE +rewriter in the codebase. Non-goals: Unicode support in `_COLON_AGG_RE` / +`_SCAN_TOKEN_RE` (ASCII-start colon-agg identifiers — separate issue if wanted); +double-quoted LIKE patterns; migrating converters off `parse_formula`. + +## Decisions + +1. **Tokenizer** (`_CASE_TOKEN_RE` id alternative): + `[^\W\d]\w*(?:\s*\.\s*[^\W\d]\w*)*` — Unicode-aware start; dotted paths (whitespace + tolerated around dots, matching Python attribute syntax) lex as ONE token, so a + qualified keyword can never equal a keyword. Alternative rejected: `tokenize` module + (the text is not valid Python at this stage — SQL keywords, colon aggs). +2. **Keyword guard** (helper applied at every comparison site): a token is a keyword iff + it `isascii()` and uppercases to the keyword AND neither adjacent raw-text character + is identifier material beyond `\w` — i.e. not `isalnum()`/`_`/`isidentifier()` and not + Unicode category `M*` (combining marks). Covers `caſe`, decomposed `écase`, `℘case`, + trailing-mark spoofs without a full Unicode-identifier lexer (Codex finding, folded). +3. **WHEN-lookahead gate** (`_case_has_when`): scan after CASE at relative depth 0 + (parens + brackets); FIRST structural keyword decides — `WHEN` → conditional, any of + `THEN`/`ELSE`/`END`/`CASE` → identifier; unmatched `)` or end → identifier. + Alternative rejected (interview Q1, upheld against a Codex objection): backtracking + parse that would also accept a bare keyword-named simple-CASE operand + (`CASE case WHEN 1 …`) — real grammar ambiguity, disproportionate complexity; the + parenthesized operand `CASE (case) WHEN …` works under the gate and is documented. +4. **ASCII keyword classes instead of `re.IGNORECASE`** for `_SQL_LIKE_RE` (`like`, + `not`), `_OVER_RE`, and the keyword rewrites in `_normalize_sql_filter_operators`: + IGNORECASE folds `lıke` (dotless ı) and `liKe` (KELVIN) into `like` (verified). + `[lL][iI][kK][eE]`-style classes keep every ASCII casing and nothing else. +5. **LIKE literal-span guard**: precompute string-literal spans with + `_PY_STRING_LITERAL_RE`; a match starting inside a span is returned unchanged. RHS + group becomes the escape-aware `'(?:\\.|[^'\\])*'`. Single-quote-only by decision + (interview Q3): pg-facade SQL double quotes denote identifiers. +6. **`parse_filter` retirement**: `_filter_refs_dsl` reimplemented as + `parse_filter_expr` + the node-walk already used by `_measure_formula_refs` + (shared helper extracted; DEV-1826 expression-source descent included). Reference + order changes from measures-first to expression order — authorized: the sole caller + (`_filter_refs_on_base`) aggregates into a set. Then delete `parse_filter` and every + private helper reaching zero references, iterating to fixpoint: + `_preprocess_like`, `_LIKE_RE`, `_preprocess_sql_operators`, `_preprocess_concat`, + `_filter_node_to_sql`, `_call_to_sql`, `_compare_to_sql`, `_binop_to_sql`, + `_flatten_lshift_chain`, `_LIKE_INTERNAL_NAMES` + the `__like__` branch of + `_classify_call_name`; audit `AggRef` / `_SUBQUERY_IN_FILTER_RE` for other users + before deleting. Retained: `parse_formula` (converters), `_preprocess_agg_refs`, + `_rewrite_funcstyle_aggregations`, `has_window_function`, shared constants. +7. **`ParsedFilter` moves to `slayer/sql/sql_predicate.py`** trimmed to `sql` + + `columns` — its only remaining producer is `parse_sql_predicate`; `agg_refs` / + `synthesized_aliases` / `is_having` / `is_post_filter` have zero remaining readers. +8. **Stale-comment scrub**: syntax.py's "module DEV-1452 deletes" claim (DEV-1452 + completed and deliberately retained formula.py) and the `_preprocess_like` mirror + claims; `parse_filter` mentions in docs/architecture. + +## Risks / Trade-offs + +- [Keyword-named bare simple-CASE operand and nested-CASE-as-operand mislex] → loud + parse/plan error, never silent corruption; parenthesize escape hatch documented in + code comment; spec scenario pins the behavior. +- [Legacy test semantics lost in migration] → DEV-1452 Stage C pattern: audit each of + the ~52 call sites; delete only what typed-parser tests already cover; migrate the + rest onto `parse_filter_expr`. +- [Drift-refs parity regression] → dedicated parity suite (colon/funcstyle/custom aggs, + dotted paths, expression sources, `*` exclusion, failure → `[]`) written before the + migration lands. + +## Migration Plan + +Single PR, sequenced commits: (1) hardening + new regression tests, (2) +`_filter_refs_dsl` migration + parity tests, (3) deletions + test migration + comment +scrub. Rollback = revert; no storage or API surface touched. diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md new file mode 100644 index 00000000..429dfbdb --- /dev/null +++ b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md @@ -0,0 +1,63 @@ +# Proposal: Harden Mode-B keyword lexing (CASE / LIKE) for Unicode identifiers + +## Why + +The Mode-B pre-`ast.parse` textual preprocessors in `slayer/engine/syntax.py` key off SQL +keywords with ASCII-oriented regex patterns, so a legal identifier can be misread as a +keyword: a column named `case`, a qualified `customers.case`, or a Unicode-prefixed +`écase` all fail to parse today, and `customers.end` inside a THEN value silently +corrupts the rewritten expression. The sibling LIKE rewriter corrupts expressions whose +string literals contain ` like ` and patterns with escaped quotes. Same bug class as the +`__slayer_` boundary fix in DEV-1743 / PR #334. + +Additionally (interview-approved scope extension): the legacy `formula.parse_filter` is a +second, near-dead Mode-B filter parser — its only production caller is +`schema_drift._filter_refs_dsl`, and its `__like__`/`__notlike__` machinery exists solely +to feed itself. Retiring it leaves exactly one Mode-B filter parser and one LIKE rewriter. + +## What Changes + +- `_CASE_TOKEN_RE` lexes complete identifiers: Unicode-aware start, dotted paths (with + optional whitespace around dots) as single tokens. +- Keyword recognition requires ASCII tokens (blocks `'caſe'.upper() == 'CASE'` spoofs) + and rejects tokens adjacent to identifier-material characters the regex `\w` class + misses (combining marks, `Other_ID_Start` symbols like `℘`). +- `_rw_case` is entered only when a depth-0 `WHEN` follows the `CASE` token; otherwise + `case` flows through as an ordinary identifier. +- `_SQL_LIKE_RE`: keyword matched as explicit ASCII character classes (drops + `re.IGNORECASE`, which folds `lıke`/`liKe` into `like`); escape-aware pattern + literal; matches starting inside string literals are skipped. Same ASCII-classes + treatment for `_OVER_RE` and the keyword rewrites in `_normalize_sql_filter_operators`. +- **BREAKING (internal only)**: legacy `formula.parse_filter` and its private subtree + (`_preprocess_like`, `_preprocess_sql_operators`, the `__like__`/`__notlike__` helpers, + the Mode-B filter→SQL lowering functions) are deleted; `schema_drift._filter_refs_dsl` + moves onto the typed `parse_filter_expr`; trimmed `ParsedFilter` (`sql` + `columns`) + moves to `slayer/sql/sql_predicate.py`. No public API changes. +- Error-surface change: a bare `CASE`-named reference no longer raises "Malformed CASE"; + a `CASE` with no `WHEN` at all degrades to the generic invalid-expression error. + +## Capabilities + +### New Capabilities + +- `queries/expression-keywords`: SQL keyword affordances inside Mode-B expressions — + CASE WHEN lowering to `iif`, LIKE/NOT LIKE rewriting to the `like()` scalar — and the + identifier-safety rules guaranteeing that identifiers named after, containing, or + qualified by SQL keywords are never captured by that recognition. + +### Modified Capabilities + +(none — the `parse_filter` retirement preserves the drift-refs contract; no existing +spec's requirements change) + +## Impact + +- `slayer/engine/syntax.py` — tokenizer, CASE gate, LIKE/OVER/operator keyword regexes. +- `slayer/core/formula.py` — `parse_filter` subtree deleted (~several hundred LOC); + `parse_formula`, `_preprocess_agg_refs`, `_rewrite_funcstyle_aggregations`, constants retained. +- `slayer/engine/schema_drift.py` — `_filter_refs_dsl` on the typed parser; reference + order becomes expression order (verified: sole caller aggregates into a set). +- `slayer/sql/sql_predicate.py` — receives the trimmed `ParsedFilter`. +- Tests: new regression file; ~52 legacy `parse_filter` call sites across 4 files + migrated or deleted (DEV-1452 Stage C pattern, user-consented). +- Docs: one sentence in `docs/concepts/formulas.md`. diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md new file mode 100644 index 00000000..ce53071a --- /dev/null +++ b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md @@ -0,0 +1,131 @@ +## Purpose + +SQL keyword affordances inside Mode-B expressions — `CASE WHEN` lowering to `iif` and +`LIKE`/`NOT LIKE` rewriting to the `like()` scalar — and the identifier-safety rules +guaranteeing that identifiers named after, containing, or qualified by SQL keywords are +never captured by that recognition. + +## ADDED Requirements + +### Requirement: CASE WHEN lowering + +A Mode-B expression SHALL accept SQL `CASE … END` conditionals — searched +(`CASE WHEN cond THEN val … [ELSE val] END`) and simple +(`CASE operand WHEN val THEN val … [ELSE val] END`) — lowering them to nested +`iif(cond, then, otherwise)` calls with `None` as the default otherwise. WHEN +conditions SHALL accept SQL operator spellings (`=`, `<>`, `AND`/`OR`/`NOT`, +`IS [NOT] NULL`, `[NOT] IN`, `[NOT] LIKE`) in every expression position, including +measures. A `CASE` that is recognized as a conditional (a `WHEN` follows it) but is +malformed MUST raise a specific malformed-CASE error naming the defect. + +#### Scenario: searched CASE lowers to iif + +- WHEN `CASE WHEN amount > 100 THEN 'big' ELSE 'small' END` is parsed as a Mode-B expression +- THEN it parses as `iif(amount > 100, 'big', 'small')` — a scalar call, usable wherever a scalar expression is legal + +#### Scenario: simple CASE compares the operand per branch + +- WHEN `CASE status WHEN 'a' THEN 1 WHEN 'b' THEN 2 END` is parsed +- THEN it parses as `iif(status == 'a', 1, iif(status == 'b', 2, None))` + +#### Scenario: nested CASE in THEN and ELSE values + +- WHEN a THEN or ELSE value itself contains a complete `CASE … END` +- THEN the nested conditional is lowered recursively and the enclosing branches are unaffected + +#### Scenario: SQL operator spellings inside WHEN conditions + +- WHEN `CASE WHEN region = 'EU' AND amount IS NOT NULL THEN 1 ELSE 0 END` appears in a measure formula +- THEN the WHEN condition is normalized (`==`, `and`, `is not None`) and the expression parses + +#### Scenario: recognized-but-malformed CASE still errors specifically + +- WHEN `CASE WHEN a THEN 1` (missing END) or `CASE WHEN a 1 END` (missing THEN) is parsed +- THEN a malformed-CASE error is raised naming the missing keyword, not a generic syntax error + +### Requirement: keyword-named identifiers are never captured + +An identifier that is merely named after, prefixed by, containing, or qualified by a SQL +keyword SHALL parse as an ordinary reference in every Mode-B expression position. `CASE` +SHALL be treated as a conditional only when a `WHEN` token follows it at parenthesis +depth 0 before any other structural keyword (`THEN`/`ELSE`/`END`/`CASE`), an unmatched +closing parenthesis, or end of input. Keyword recognition MUST be ASCII-exact: tokens +whose uppercase form only coincides with a keyword via Unicode case folding, and tokens +adjacent to identifier-forming characters outside the regex word class (combining marks, +`Other_ID_Start` symbols), are ordinary identifiers. Dotted references qualify their +leaf regardless of whitespace around the dots. + +#### Scenario: bare keyword-named column + +- WHEN `case` (or `case + 1`, or `iif(case, 1, 2)`) is parsed as a Mode-B expression +- THEN `case` resolves as an ordinary column reference and no CASE lowering occurs + +#### Scenario: qualified keyword-named column + +- WHEN `customers.case` is parsed, with or without whitespace around the dot (`customers . case`) +- THEN it parses as a dotted reference to the `case` column of `customers` + +#### Scenario: Unicode identifiers containing keywords + +- WHEN `écase`, `变量`, decomposed `écase`, or `℘case` is parsed +- THEN each parses as a single ordinary identifier; no fragment of it is read as a keyword + +#### Scenario: Unicode case-fold spoofs are not keywords + +- WHEN an identifier like `caſe` (uppercases to `CASE`) appears in an expression +- THEN it is an ordinary identifier, not a CASE keyword + +#### Scenario: keyword-named identifier alongside a real CASE + +- WHEN `case + CASE WHEN x THEN 1 END` or `CASE WHEN case THEN 1 WHEN other THEN 2 END` is parsed +- THEN the bare `case` references stay identifiers while the real `CASE WHEN … END` lowers to `iif` + +#### Scenario: keyword-named dotted reference inside CASE branch values + +- WHEN `CASE WHEN a THEN customers.end ELSE 0 END` is parsed +- THEN the THEN value is the complete `customers.end` reference and the conditional lowers correctly + +#### Scenario: bare CASE with no WHEN is not a conditional + +- WHEN `CASE` appears with no depth-0 `WHEN` following (e.g. the whole expression is `case` or `case_total * 2`) +- THEN no CASE lowering is attempted; the text parses (or fails) exactly as if `case` were any other identifier + +#### Scenario: keyword-named simple-CASE operand requires parentheses + +- WHEN `CASE case WHEN 1 THEN 2 END` is parsed +- THEN the ambiguous bare keyword-named operand raises an error (never silent corruption), and the parenthesized form `CASE (case) WHEN 1 THEN 2 END` parses correctly with `case` as the operand reference + +### Requirement: LIKE operator rewriting + +A Mode-B filter SHALL accept `lhs [NOT] LIKE 'pattern'` — LHS a bare or dotted +identifier or single scalar call, pattern a single-quoted string literal with +backslash-escape support — rewriting it to the `like(lhs, pattern)` scalar (negated: +`not like(...)`). The keyword match SHALL be ASCII-exact (any ASCII casing; never via +Unicode case folding) and SHALL never apply inside a string literal. A double-quoted +pattern is NOT rewritten (in SQL sources double quotes denote identifiers), so it fails +loudly rather than silently changing meaning. + +#### Scenario: basic LIKE and NOT LIKE + +- WHEN `name LIKE 'a%'` / `name NOT LIKE 'a%'` / `lower(customers.email) like '%@x.io'` appear in a filter +- THEN each rewrites to the corresponding `like(...)` / `not like(...)` scalar call + +#### Scenario: escaped quote inside the pattern + +- WHEN `col LIKE 'It\'s%'` appears in a filter +- THEN the full pattern including the escaped quote is preserved as the second argument + +#### Scenario: LIKE inside a string literal is untouched + +- WHEN a filter contains ` like ` only inside a string literal, e.g. `note == "we like 'cats'"` +- THEN the literal is preserved byte-for-byte and no rewrite occurs + +#### Scenario: case-fold keyword spoofs are not LIKE + +- WHEN a filter contains `x lıke 'p%'` or `x liKe 'p%'` (dotless ı / KELVIN SIGN fold to `like`) +- THEN no rewrite occurs (the token is an ordinary identifier), while ASCII `x LiKe 'p%'` still rewrites + +#### Scenario: double-quoted pattern is rejected loudly + +- WHEN `col like "p%"` appears in a filter +- THEN parsing fails with an invalid-expression error rather than rewriting to a string match diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md new file mode 100644 index 00000000..6e63f6f5 --- /dev/null +++ b/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md @@ -0,0 +1,73 @@ +# Tasks + +## 1. Regression tests first (spec-tests stage) + +- [x] 1.1 Create `tests/test_dev1833_keyword_lexing.py` with the CASE identifier-safety + matrix from the spec scenarios: bare `case` (also `case + 1`, `iif(case, 1, 2)`), + `customers.case`, `customers . case`, `é_case`, `écase`, `变量`, decomposed `écase`, + `℘case`, `caſe`, `case + CASE WHEN x THEN 1 END`, + `CASE WHEN case THEN 1 WHEN other THEN 2 END`, `customers.end` in a THEN value, + `CASE (case) WHEN 1 THEN 2 END`, and the `CASE case WHEN 1 THEN 2 END` loud-error + pin. Verify: every regression test FAILS on the pre-fix code. +- [x] 1.2 Add CASE still-works coverage: searched/simple/nested lowering, SQL operator + normalization in WHEN conditions, malformed-with-WHEN errors (missing THEN / END) + preserved. Verify: these pass before AND after the fix. +- [x] 1.3 Add the LIKE matrix: escaped-quote pattern, ` like ` inside a string literal + untouched, `x lıke 'p%'` / `x liKe 'p%'` not rewritten, `x LiKe 'p%'` rewritten, + double-quoted RHS still errors, LIKE/NOT LIKE/dotted/scalar-call/Unicode LHS still + rewrite. Verify: regression cases fail pre-fix; still-works cases pass pre-fix. +- [x] 1.4 Add the `_filter_refs_dsl` parity suite (in the schema-drift test module): + colon aggs, funcstyle aggs, custom agg names, dotted paths, expression agg sources, + `*:count` exclusion of `*`, LIKE-containing filter, unparseable → `[]`; assert set + equality (order authorized to change). Verify: suite passes against the LEGACY + implementation before migration (parity baseline). + +## 2. CASE hardening (slayer/engine/syntax.py) + +- [x] 2.1 Change `_CASE_TOKEN_RE` id alternative to + `[^\W\d]\w*(?:\s*\.\s*[^\W\d]\w*)*`. Verify: task 1.1 Unicode + dotted cases pass. +- [x] 2.2 Add the keyword-guard helper (ASCII-exact + raw-text adjacency check per + design decision 2) and use it at every keyword comparison in `_rewrite_case_when` / + `_rw_value` / `_rw_case`. Verify: `caſe`, decomposed, `℘case` cases pass. +- [x] 2.3 Add `_case_has_when` lookahead gate and require it before entering `_rw_case` + from `_rw_value`; document the two accepted mislex edges + parenthesize escape hatch + in a concise comment. Verify: bare-`case` family and coexistence cases pass; 1.2 + still green. + +## 3. LIKE + operator-keyword hardening (slayer/engine/syntax.py) + +- [x] 3.1 Rework `_SQL_LIKE_RE`: ASCII character-class keywords (drop IGNORECASE), + escape-aware RHS group. Verify: 1.3 spoof + escaped-quote cases pass. +- [x] 3.2 Add literal-span skip to `_rewrite_sql_like`. Verify: inside-literal case passes. +- [x] 3.3 Apply ASCII keyword classes to `_OVER_RE` and the keyword rewrites in + `_normalize_sql_filter_operators`. Verify: existing operator-normalization and OVER + rejection tests stay green. + +## 4. parse_filter retirement + +- [x] 4.1 Extract the shared ref-walk helper from `_measure_formula_refs` and + reimplement `_filter_refs_dsl` on `parse_filter_expr`. Verify: 1.4 parity suite green + on the new implementation; schema-drift tests green. +- [x] 4.2 Migrate/delete the ~52 legacy `parse_filter` test call sites + (43 `tests/test_formula.py`, 5 `tests/test_sql_generator.py`, + 3 `tests/test_dev1576_heals.py`, 1 `tests/facade/test_translator.py`) per the + DEV-1452 Stage C pattern. Verify: no `parse_filter` reference remains under `tests/`; + suite green. +- [x] 4.3 Delete `parse_filter` and iterate orphan deletion to fixpoint (candidate list + in design decision 6; audit `AggRef` / `_SUBQUERY_IN_FILTER_RE` first). Verify: + `grep -rn "parse_filter\b\|__like__\|__notlike__\|_preprocess_like" slayer/ tests/` + returns nothing; suite green. +- [x] 4.4 Move trimmed `ParsedFilter` (`sql`, `columns`) to + `slayer/sql/sql_predicate.py`; update imports. Verify: suite + ruff green. +- [x] 4.5 Stale-comment scrub per design decision 8 (syntax.py DEV-1452/mirror claims, + formula.py docstrings, docs/architecture `parse_filter` mentions). Verify: + `grep -rn "parse_filter\|_preprocess_like" slayer/ docs/` shows no stale claims. + +## 5. Docs + gates + +- [x] 5.1 Add one sentence to `docs/concepts/formulas.md` (CASE section): keyword-named + identifiers are safe; CASE only lowers when WHEN follows. Verify: sentence present, + page already in `zensical.toml` nav. +- [x] 5.2 Full gates: `poetry run pytest -m "not integration"` green, + `poetry run ruff check slayer/ tests/` clean, + `openspec validate dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode --strict` green. diff --git a/slayer/engine/syntax.py b/slayer/engine/syntax.py index 6e2bf40f..3605081d 100644 --- a/slayer/engine/syntax.py +++ b/slayer/engine/syntax.py @@ -1,53 +1,12 @@ -"""Stage 7a.3 (DEV-1450) — Mode-B Python-AST parser. - -Public entry point: ``parse_expr(text: str) -> ParsedExpr``. - -The parser consumes a Mode-B expression string (the SLayer DSL used in -``ModelMeasure.formula``, ``SlayerQuery.measures``, -``SlayerQuery.filters``, …) and emits a typed ``ParsedExpr`` tree. It -is PURE syntax — no scope resolution, no named-measure expansion -(those are the binder's concerns). - -Pipeline order (per query / model save): - - raw → parse_expr → bind → plan → SQL - -Mode-B grammar: - -* bare identifier (``revenue``) -* dotted path (``customers.regions.name``) -* aggregation, colon or functional spelling — both collapse to the SAME - ``AggCall`` (DEV-1826): ``revenue:sum`` / ``sum(revenue)``, - ``*:count`` / ``count(*)``, ``price:weighted_avg(weight=qty)`` / - ``weighted_avg(price, weight=qty)``, ``revenue:last(ordered_at)`` / - ``last(revenue, ordered_at)``. An unknown call name whose first - argument is aggregatable defers to binding as an ``AggCall`` - candidate, exactly like ``x:whatever``. A functional aggregation may - take a same-model scalar EXPRESSION source (``sum(amount - cost)``). -* transform call (``cumsum``, ``lag``, ``rank``, ``time_shift``, …); - ``first`` / ``last`` dispatch by first-arg shape — an aggregated - input makes them transforms, anything else the aggregation -* scalar function (closed allowlist from ``SCALAR_FUNCTIONS``) -* arithmetic / comparison / boolean / unary -* parenthesised grouping - -Rejections (per DEV-1450 spec): - -* Function calls not in SCALAR_FUNCTIONS / transforms / aggregations → - ``UnknownFunctionError``. -* Raw ``OVER(...)`` clauses → ``IllegalWindowInFilterError``. -* ``__`` in any user-supplied identifier → ``ValueError`` (reserved for - internal join-path aliases on the SQL side). -* Chained comparisons (``1 < x < 10``) → ``ValueError``; the user - splits as ``1 < x and x < 10``. - -ParsedExpr family: ``Ref`` / ``DottedRef`` / ``StarSource`` / -``Literal`` / ``AggCall`` / ``TransformCall`` / ``ScalarCall`` / -``Arith`` / ``UnaryOp`` / ``Cmp`` / ``BoolOp``. All are frozen -Pydantic models with value-based equality so tests assert via ``==``. - -Dormant in stage 7a — no engine code calls ``parse_expr`` yet. The -binder (stage 7a.5) is the first consumer. +"""Mode-B Python-AST parser (DEV-1450). + +``parse_expr(text) -> ParsedExpr`` lowers a Mode-B DSL string +(``ModelMeasure.formula``, ``SlayerQuery.measures`` / ``.filters``) to a typed +tree — pure syntax, no scope resolution or named-measure expansion (binder's +job). Grammar: bare/dotted refs; colon or functional aggregations, which +collapse to one ``AggCall`` (DEV-1826); transform calls; a closed scalar +allowlist; arithmetic / comparison / boolean / unary; grouping. Rejects +non-allowlisted calls, raw ``OVER(...)``, and chained comparisons. """ from __future__ import annotations @@ -92,22 +51,14 @@ class Literal(_BaseNode): class TupleLit(_BaseNode): - """A literal-only tuple/list RHS for ``IN`` / ``NOT IN`` filters (DEV-1475). - - Only emitted on the right-hand side of a ``Cmp`` whose op is ``in`` or - ``not in``. Every ``elements`` entry is a ``Literal`` — references and - expressions on the RHS are rejected at parse time so the binder can - fold the predicate into a single ``InKey`` with a tuple of - ``LiteralKey``. Empty tuples are rejected too (an empty IN is a SQL - quirk that varies by dialect; reject early with a clear message). - """ + """Literal-only tuple/list RHS for ``IN`` / ``NOT IN`` (DEV-1475); non-literal + elements and empty tuples are rejected at parse time.""" elements: Tuple[Literal, ...] class AggCall(_BaseNode): - # Beyond column/star sources, an aggregation may take an aggregation-free - # same-model scalar EXPRESSION source (``sum(amount - cost)``, DEV-1826). + # source may also be an aggregation-free scalar expression (``sum(a - b)``). source: Union[ Ref, DottedRef, StarSource, Literal, "ScalarCall", "Arith", "UnaryOp", ] @@ -166,51 +117,40 @@ class BoolOp(_BaseNode): # --------------------------------------------------------------------------- -# The internal namespace SLayer mints for aggregation placeholders. The whole -# ``__slayer_`` prefix is reserved from user input (P3), so a literal spoof of -# the placeholder cannot slip past ``_convert``'s ``_PLACEHOLDER_RE`` match. +# The ``__slayer_`` prefix is reserved from user input (P3). Matched only at an +# identifier boundary so a legal embedded name (``foo__slayer_bar``) stays +# referenceable; ``\w`` is Unicode-aware, covering ``é__slayer_bar`` too. _RESERVED_EXPR_PREFIX = "__slayer_" -# Match the reserved prefix only at an identifier boundary: name validation -# reserves it as a *prefix* (a name must START with it), so ``foo__slayer_bar`` -# is a legal saved name and must stay referenceable — a raw substring check -# would reject it. The lookbehind rejects a token that opens the identifier -# (start-of-string or after an operator/dot) while allowing an embedded run. -# ``\w`` is Unicode-aware (str patterns), so a Unicode-identifier name like -# ``é__slayer_bar`` — legal at save — stays referenceable too (Codex). _RESERVED_EXPR_PREFIX_RE = re.compile(r"(? str: """``col LIKE 'p%'`` → ``like(col, 'p%')``; ``col NOT LIKE 'p%'`` → - ``not like(col, 'p%')`` — outside/inside handling matches - ``formula._preprocess_like``.""" + ``not like(col, 'p%')``. Matches starting inside string literals are + left untouched.""" + spans = [(m.start(), m.end()) for m in _PY_STRING_LITERAL_RE.finditer(text)] def _sub(m: "re.Match[str]") -> str: + if any(s <= m.start() < e for s, e in spans): + return m.group(0) lhs, neg, pat = m.group(1), m.group(2), m.group(3) call = f"like({lhs}, {pat})" return f"not {call}" if neg else call @@ -224,15 +164,67 @@ def _sub(m: "re.Match[str]") -> str: # (so ``CASE WHEN a = 5 …`` works in measures too); THEN/ELSE values are sliced # verbatim and only recursed for nested CASE, so a value like ``'a AND b'`` is # never rewritten. String literals are single tokens, so keywords inside them are -# invisible to the keyword scan. +# invisible to the keyword scan. Identifiers lex complete (Unicode-aware start, +# dotted paths as ONE token), so a name containing or qualified by a keyword +# (``customers.case``, ``écase``) can never equal one. _CASE_TOKEN_RE = re.compile( r"(?P'(?:\\.|[^'\\])*'|\"(?:\\.|[^\"\\])*\")" - r"|(?P[A-Za-z_]\w*)" + r"|(?P[^\W\d]\w*(?:\s*\.\s*[^\W\d]\w*)*)" r"|(?P[(\[])" r"|(?P[)\]])", re.DOTALL, ) _CASE_STOPS_OPERAND = frozenset({"WHEN", "THEN", "ELSE", "END"}) +_CASE_KEYWORDS = _CASE_STOPS_OPERAND | {"CASE"} + + +def _is_ident_adjacent(text: str, pos: int) -> bool: + """Whether ``text[pos]`` is identifier material the ``\\w`` token class + misses (combining marks, ``Other_ID_Start`` symbols like ``℘``).""" + return 0 <= pos < len(text) and ("a" + text[pos]).isidentifier() + + +def _case_keyword(text: str, tok: Tuple[Optional[str], str, int, int]) -> Optional[str]: + """The CASE-grammar keyword a token spells, or None for an identifier. + + ASCII-exact (``caſe`` uppercases to ``CASE`` only via Unicode folding) and + rejected when adjacent raw text continues an identifier around the token. + """ + kind, val, start, end = tok + if kind != "id" or not val.isascii(): + return None + up = val.upper() + if up not in _CASE_KEYWORDS: + return None + if _is_ident_adjacent(text, start - 1) or _is_ident_adjacent(text, end): + return None + return up + + +def _case_has_when( + text: str, toks: List[Tuple[Optional[str], str, int, int]], i: int, +) -> bool: + """Whether the CASE at ``toks[i]`` opens a conditional: a depth-0 ``WHEN`` + follows before any other structural keyword, an unmatched ``)``, or end. + + Accepted mislex edge: a BARE keyword-named simple-CASE operand + (``CASE case WHEN 1 …``) is genuinely ambiguous and errors loudly — + parenthesize the operand (``CASE (case) WHEN 1 …``) to disambiguate. + """ + depth = 0 + for j in range(i + 1, len(toks)): + kind = toks[j][0] + if kind == "lp": + depth += 1 + elif kind == "rp": + if depth == 0: + return False + depth -= 1 + elif kind == "id" and depth == 0: + kw = _case_keyword(text, toks[j]) + if kw is not None: + return kw == "WHEN" + return False def _rewrite_case_when(text: str) -> str: @@ -242,27 +234,25 @@ def _rewrite_case_when(text: str) -> str: (m.lastgroup, m.group(), m.start(), m.end()) for m in _CASE_TOKEN_RE.finditer(text) ] - if not any(k == "id" and v.upper() == "CASE" for k, v, _, _ in toks): + if not any(_case_keyword(text, t) == "CASE" for t in toks): return text result, _ = _rw_value(text, toks, 0, frozenset(), 0) return result def _rw_value(text, toks, i, stop_kws, start_char): # NOSONAR(S3776) — one cohesive token-scan over a value/condition span: paren-depth tracking, the depth-0 stop-keyword break, and nested-CASE recursion are each one decision, and splitting them scatters the slice-and-recurse contract that preserves the original text. - """Rewrite one value/condition span, returning ``(rewritten, next_index)``. - - ``start_char`` is where the span begins in ``text`` (the end of the keyword - just consumed) — the lexer only tokenises strings / idents / parens, so bare - values like ``1`` have no token and must be recovered by slicing. Slices the - ORIGINAL text for non-CASE content (preserving colons, dots, spacing) and - recurses into nested CASE. Stops at a depth-0 stop keyword or an unmatched - closing paren. + """Rewrite one value/condition span → ``(rewritten, next_index)``. + + ``start_char`` is the span start (end of the keyword just consumed): bare + values (``1``) have no token, so non-CASE content is recovered by slicing the + original text (preserving colons/dots/spacing). Stops at a depth-0 stop + keyword or an unmatched ``)``. """ parts: List[str] = [] depth = 0 last = start_char while i < len(toks): - kind, val, start, _ = toks[i] + kind, _, start, _ = toks[i] if kind == "lp": depth += 1 elif kind == "rp": @@ -270,10 +260,10 @@ def _rw_value(text, toks, i, stop_kws, start_char): # NOSONAR(S3776) — one co break depth -= 1 elif kind == "id": - up = val.upper() - if depth == 0 and up in stop_kws: + kw = _case_keyword(text, toks[i]) + if depth == 0 and kw in stop_kws: break - if up == "CASE": + if kw == "CASE" and _case_has_when(text, toks, i): parts.append(text[last:start]) nested, i = _rw_case(text, toks, i) parts.append(nested) @@ -291,10 +281,10 @@ def _rw_case(text, toks, i): operand, i = _rw_value(text, toks, i, _CASE_STOPS_OPERAND, toks[i - 1][3]) is_simple = bool(operand) branches: List[Tuple[str, str]] = [] - while i < len(toks) and toks[i][0] == "id" and toks[i][1].upper() == "WHEN": + while i < len(toks) and _case_keyword(text, toks[i]) == "WHEN": i += 1 cond, i = _rw_value(text, toks, i, frozenset({"THEN"}), toks[i - 1][3]) - if not (i < len(toks) and toks[i][0] == "id" and toks[i][1].upper() == "THEN"): + if not (i < len(toks) and _case_keyword(text, toks[i]) == "THEN"): raise ValueError( f"Malformed CASE expression in {text!r}: a WHEN branch is " f"missing its THEN." @@ -305,11 +295,11 @@ def _rw_case(text, toks, i): ) branches.append((cond, then_val)) else_val = "None" - if i < len(toks) and toks[i][0] == "id" and toks[i][1].upper() == "ELSE": + if i < len(toks) and _case_keyword(text, toks[i]) == "ELSE": i += 1 else_val, i = _rw_value(text, toks, i, frozenset({"END"}), toks[i - 1][3]) else_val = else_val or "None" - if not (i < len(toks) and toks[i][0] == "id" and toks[i][1].upper() == "END"): + if not (i < len(toks) and _case_keyword(text, toks[i]) == "END"): raise ValueError( f"Malformed CASE expression in {text!r}: missing END." ) @@ -362,19 +352,11 @@ def _rw_case(text, toks, i): ast.Eq: "==", ast.NotEq: "!=", ast.Lt: "<", ast.LtE: "<=", ast.Gt: ">", ast.GtE: ">=", - # ``IS`` / ``IS NOT`` (Codex review): the filter normalizer lowers SQL - # ``IS NULL`` / ``IS NOT NULL`` to Python ``is None`` / ``is not None``; - # without these entries the AST converter raised on ``ast.Is`` / - # ``ast.IsNot`` and any DSL filter using the SQL-style spelling failed - # to plan. The downstream SQL generator renders ``is`` / ``is not`` - # against a ``None`` literal as ``IS NULL`` / ``IS NOT NULL``. + # SQL ``IS [NOT] NULL`` lowers to ``is [not] None``; rendered back as + # ``IS [NOT] NULL`` by the SQL generator. ast.Is: "is", ast.IsNot: "is not", - # DEV-1475: SQL-style ``IN`` / ``NOT IN`` with a literal-tuple RHS. - # ``_normalize_sql_filter_operators`` already lowercases ``IN`` / - # ``NOT IN`` to the Python keywords; the AST then carries ``ast.In`` - # / ``ast.NotIn`` here. The ``ast.Compare`` branch enforces the - # tuple/list-only RHS shape and validates that every element is a - # literal. + # DEV-1475: ``IN`` / ``NOT IN`` with a literal-tuple RHS (shape enforced in + # the ``ast.Compare`` branch). ast.In: "in", ast.NotIn: "not in", } @@ -387,26 +369,18 @@ def _rw_case(text, toks, i): def parse_expr(text: str) -> ParsedExpr: """Parse a Mode-B expression string into a ``ParsedExpr``. - ``__`` in identifiers is legal (DEV-1743): binding legality is the binder's - concern, not the parser's. The only reserved token is the ``__slayer_`` - prefix, scanned on the RAW input below (P3). - - Raises: - ValueError: empty input, syntax error, unsupported AST node, - chained comparison, or the reserved ``__slayer_`` prefix. - UnknownFunctionError: function call not in - ``SCALAR_FUNCTIONS`` / ``ALL_TRANSFORMS``. - IllegalWindowInFilterError: raw ``OVER(...)`` clause anywhere - in ``text``. + ``__`` in identifiers is legal (DEV-1743); only the ``__slayer_`` prefix is + reserved. Raises ``ValueError`` (empty/syntax/unsupported node/chained + comparison/reserved prefix), ``UnknownFunctionError``, or + ``IllegalWindowInFilterError`` (raw ``OVER(...)``). """ if not text or not text.strip(): raise ValueError("Empty Mode-B expression.") _reject_reserved_expr_token(text) - # Scan for a raw window clause AFTER blanking string literals (Python - # syntax, so escapes count), so a value like ``status == 'OVER('`` or - # ``status == "x \" OVER("`` isn't mistaken for window usage (CR / Codex). + # Scan for raw ``OVER(`` after blanking string literals so a quoted value + # (``status == 'OVER('``) isn't mistaken for window usage. if _OVER_RE.search(_PY_STRING_LITERAL_RE.sub("", text)): raise IllegalWindowInFilterError( filter_expr=text, @@ -441,49 +415,47 @@ def parse_filter_expr(text: str) -> ParsedExpr: alongside the Python spellings. This wrapper normalizes those to their Python equivalents (string-literal-aware, so quoted contents are untouched) and then delegates to :func:`parse_expr`. Measures / order use - ``parse_expr`` directly — only filters get the SQL-operator leniency, - matching the legacy ``parse_filter`` contract. + ``parse_expr`` directly — only filters get the SQL-operator leniency. """ return parse_expr(_normalize_sql_filter_operators(text)) +# SQL keyword rewrites as explicit ASCII character classes — ``re.IGNORECASE`` +# folds Unicode spoofs (``ıs`` → ``is``) into keywords. +_SQL_NULL_RE = re.compile(r"\b[nN][uU][lL][lL]\b") +_SQL_KEYWORD_RES: Tuple[Tuple[re.Pattern, str], ...] = tuple( + (re.compile(r"\b" + "".join(f"[{c}{c.upper()}]" for c in kw) + r"\b"), kw) + for kw in ("is", "not", "and", "or", "in") +) + + def _normalize_sql_filter_operators(text: str) -> str: """Rewrite SQL operator spellings to Python ones outside string literals. ``NULL`` → ``None``; ``IS`` / ``NOT`` / ``AND`` / ``OR`` / ``IN`` → lowercase; standalone ``=`` → ``==``; ``<>`` → ``!=``; ``col [NOT] LIKE - 'p%'`` → ``[not ]like(col, 'p%')``. Replicated from the legacy - ``slayer.core.formula._preprocess_sql_operators`` / ``_preprocess_like`` so - the typed pipeline doesn't depend on the module DEV-1452 deletes. + 'p%'`` → ``[not ]like(col, 'p%')``. """ - # LIKE runs first, on the whole string: its pattern is a quoted literal, so - # it can't be rewritten per-non-literal-part like the other operators. + # LIKE first, on the whole string (its pattern is a quoted literal). The + # rest run per non-literal part (escape-aware split) so a keyword inside a + # quoted value isn't rewritten. text = _rewrite_sql_like(text) - # CR review: use the escape-aware Python-string matcher so backslash- - # escaped quotes don't leak ``IS`` / ``IN`` / ``AND`` rewrites into - # the string body (``"x \" IN ("``). parts = _PY_STRING_LITERAL_RE.split(text) literals = _PY_STRING_LITERAL_RE.findall(text) result: List[str] = [] for i, part in enumerate(parts): - part = re.sub(r"\bNULL\b", "None", part, flags=re.IGNORECASE) - for kw in ("IS", "NOT", "AND", "OR", "IN"): - part = re.sub(rf"\b{kw}\b", kw.lower(), part, flags=re.IGNORECASE) + part = _SQL_NULL_RE.sub("None", part) + for kw_re, kw in _SQL_KEYWORD_RES: + part = kw_re.sub(kw, part) part = part.replace("<>", "!=") - # SQL ``||`` concat → Python ``|`` (BitOr), reinterpreted as a - # ``concat(...)`` ScalarCall in ``_convert``. ``|`` binds tighter - # than comparisons in Python just as ``||`` does in SQL, so - # ``a || b = 'x'`` and ``a | b == 'x'`` group identically. + # SQL ``||`` → Python ``|`` (BitOr), desugared to ``concat`` in + # ``_convert``; same precedence relative to comparisons. part = part.replace("||", "|") result.append(part) if i < len(literals): result.append(literals[i]) - # DEV-1492: the `=` → `==` rewrite runs on the rejoined string with a - # call-paren-aware scanner that leaves keyword-argument `=` alone - # inside non-scalar calls (transforms / parametric aggregations). Run - # last so the scanner sees lowercased keywords and the post-`<>` / - # post-`||` text — only its own pass can touch literal-spanning - # paren context correctly. + # ``=`` → ``==`` runs last, on the rejoined string, via a paren-aware scanner + # that leaves kwarg ``=`` inside non-scalar calls alone (DEV-1492). return _rewrite_comparison_equals("".join(result)) @@ -492,21 +464,10 @@ def _classify_paren( ) -> Tuple[bool, Optional[str]]: """Classify an open ``(`` as a CALL or GROUPING paren. - A ``(`` is a call paren when the previous significant token is a - bare identifier (callable name) or a callable-suffix token (``)`` - / ``]``). Lowercase keywords (``and`` / ``or`` / ``not`` / ``in`` - / ``is``) do NOT make the next ``(`` a call paren — that's why - ``not(...)`` and ``x in (...)`` carry grouping parens. - - DEV-1492 iteration 3: a colon-aggregation context - (``revenue:first(...)``) makes the call a parametric aggregation - regardless of the callee name. ``first`` and ``last`` sit in both - :data:`ALL_TRANSFORMS` and the built-in aggregation set - (``_AMBIGUOUS_AGG_TRANSFORMS`` in ``slayer/core/formula.py``); - after a ``:`` they are always aggregations, never transforms. - Drop the callee to ``None`` so :func:`_is_kwarg_equals` takes the - aggregation/unknown branch (kwargs preserved after ``(`` or - ``,``). + A call paren follows a bare identifier or a ``)`` / ``]``; lowercase keywords + (``and`` / ``not`` / ``in`` …) do not open one. After a ``:`` + (``revenue:first(...)``) the callee is dropped to ``None`` so + :func:`_is_kwarg_equals` treats it as an aggregation, not a transform. """ prev_kind = hist[-1][0] if hist else None prev_text = hist[-1][1] if hist else "" @@ -521,26 +482,17 @@ def _is_kwarg_equals( stack: List[Tuple[bool, Optional[str]]], hist: List[Tuple[str, str]], ) -> bool: - """Whether a lone ``=`` is a Python keyword-argument separator. - - Three callee classes get different treatment (DEV-1492 iteration 2): - - * **Scalar** (``callee in SCALAR_FUNCTIONS``) — never a kwarg; - scalars reject keyword args by design. - * **Transform** (``callee in ALL_TRANSFORMS``) — the first - positional is always the value to transform, so a kwarg can - only appear AFTER a ``,``. This preserves the documented - predicate-input form (``consecutive_periods(status = 'paid')`` - where the SQL ``=`` is part of the predicate, not a kwarg). - * **Aggregation or unknown** — kwarg can be the first arg - (``weighted_avg(weight=qty)``, ``percentile(p=0.5)``), so the - ``=`` may follow either ``(`` or ``,``. + """Whether a lone ``=`` is a Python keyword-argument separator (DEV-1492). + + Scalars never take kwargs. Transforms take the value first, so a kwarg only + follows a ``,`` (keeping ``consecutive_periods(status = 'paid')`` a + predicate). Aggregations/unknowns may take a kwarg first, so ``=`` follows + ``(`` or ``,``. """ top = stack[-1] if stack else None if top is None or not top[0]: return False callee = top[1] - # Case-insensitive, matching the scalar-call parse branch below. if callee is not None and callee.lower() in SCALAR_FUNCTIONS: return False prev_kind = hist[-1][0] if hist else None @@ -654,44 +606,18 @@ def _handle_op_eq( def _rewrite_comparison_equals(text: str) -> str: - """Rewrite SQL-style ``=`` to Python ``==`` except where Python would - treat the ``=`` as a keyword argument inside a non-scalar call. - - Per the architecture (``docs/architecture/parsing.md`` — scalars - reject kwargs, only ``AggCall`` / ``TransformCall`` carry kwargs), - a lone ``=`` is preserved (kwarg) iff: - - 1. the innermost open paren is a CALL paren (see - :func:`_classify_paren`), - 2. the callee of that innermost call is NOT in - :data:`SCALAR_FUNCTIONS` — scalars never accept kwargs, so a - ``=`` inside them is the user's SQL comparison - (``coalesce(status = 'paid', False)``), - 3. the ``=`` is immediately preceded (skipping whitespace) by an - identifier preceded (skipping whitespace) by the call's ``(`` - or by a ``,`` at that paren depth — Python's keyword-argument - grammar (see :func:`_is_kwarg_equals`). - - Compound operators (``==``, ``<=``, ``>=``, ``!=``) are emitted - verbatim by the tokenizer so their ``=`` is never touched. String - literals are tokenized as a unit (Python single/double-quoted with - backslash escapes) and pass through without perturbing the paren - stack or the previous-significant-token history. - - Token-class dispatch goes through :data:`_HANDLERS` keyed by the - regex's ``lastgroup`` (each token-class group is named and the - arms are mutually exclusive, so ``lastgroup`` is exactly the - matched arm). + """Rewrite SQL ``=`` to Python ``==`` except where it is a kwarg separator. + + A lone ``=`` is kept (kwarg) inside a non-scalar CALL paren when it follows + ``ident`` after ``(`` or ``,`` — Python's kwarg grammar (see + :func:`_is_kwarg_equals`); inside scalars a ``=`` is always the user's + comparison. Compound ops and string literals pass through untouched. + Token dispatch is keyed by the scan regex's ``lastgroup``. """ out: List[str] = [] - # Each frame: (is_call, callee). ``callee`` is the identifier text - # preceding the call's ``(``, or ``None`` (e.g. a callable expression - # like ``f()(x=1)`` where the prior token is ``)``). + # Each frame: (is_call, callee) — callee is the ident before ``(``, else None. stack: List[Tuple[bool, Optional[str]]] = [] - # Trailing window of the last 2 significant tokens (oldest-first). - # Categories: NAME (bare identifier), KW (lowercase Python keyword), - # LPAREN, COMMA, OTHER (everything else; string literals don't enter - # the history). + # Last 2 significant tokens (NAME / KW / LPAREN / COMMA / OTHER; no strings). hist: List[Tuple[str, str]] = [] for m in _SCAN_TOKEN_RE.finditer(text): _HANDLERS[m.lastgroup](m, out, stack, hist) @@ -706,27 +632,13 @@ def _rewrite_comparison_equals(text: str) -> str: def walk_parsed_refs( parsed: ParsedExpr, ) -> Iterator[Union[Ref, DottedRef, AggCall]]: - """Yield every reference-bearing leaf node in a ``ParsedExpr`` tree. - - Yields ``Ref`` (bare identifier), ``DottedRef`` (dotted join path), and - ``AggCall`` (colon-syntax aggregation) nodes — the leaves a formula - actually references. This is the scope-free counterpart to the binder's - ``walk_value_keys``: callers that only need the *names* a formula touches - (schema-drift cascade attribution, memory entity tagging) walk the parse - tree directly instead of binding it against a scope. - - Descent rules (chosen to match the legacy ``parse_formula`` / - ``FieldSpec`` walk exactly): - - * ``AggCall`` is yielded as a unit — the aggregation's source / args / - kwargs are NOT descended (``weighted_avg(weight=quantity)`` surfaces - ``price``, never ``quantity``). - * ``TransformCall`` descends ``input`` only; positional args, kwargs, and - ``partition_by`` columns are opaque. - * ``ScalarCall`` descends every positional arg (``coalesce`` / ``nullif`` - wrapping aggregated or bare refs). - * ``Arith`` / ``UnaryOp`` / ``Cmp`` / ``BoolOp`` descend their operands. - * ``Literal`` and ``StarSource`` yield nothing. + """Yield the reference-bearing leaves (``Ref`` / ``DottedRef`` / ``AggCall``) + of a tree — scope-free name extraction for schema-drift / memory tagging. + + Descent (matches the binder's walk): ``AggCall`` yielded whole (args/kwargs + opaque, so ``weighted_avg(weight=quantity)`` surfaces ``price`` not + ``quantity``); ``TransformCall`` descends ``input`` only; ``ScalarCall`` / + arithmetic / comparison / boolean descend operands; literals yield nothing. """ if isinstance(parsed, (Ref, DottedRef, AggCall)): yield parsed @@ -753,10 +665,8 @@ def walk_parsed_refs( for op in parsed.operands: yield from walk_parsed_refs(op) return - # Literal / StarSource / TupleLit → no references. - # ``TupleLit`` carries only ``Literal`` elements by construction - # (see the ``ast.Compare`` branch of ``_convert``) so the walk stops - # here without descending — same shape as ``Literal``. + # Literal / StarSource / TupleLit → no references (TupleLit holds only + # Literals by construction). # --------------------------------------------------------------------------- @@ -912,9 +822,7 @@ def _convert(node: ast.AST, *, agg_map: Dict, original: str) -> ParsedExpr: # N if isinstance(node, ast.BinOp): op_type = type(node.op) if op_type is ast.BitOr: - # SQL ``||`` concat operator (``parse_filter_expr`` normalizes - # ``||`` → ``|``). Desugar to the existing ``concat`` scalar - # call so binding + per-dialect SQL emission are fully reused. + # SQL ``||`` (normalized to ``|``) desugars to the ``concat`` scalar. return ScalarCall( name="concat", args=( @@ -967,13 +875,8 @@ def _convert(node: ast.AST, *, agg_map: Dict, original: str) -> ParsedExpr: # N f"Invalid Mode-B expression {original!r}: unsupported " f"comparison operator {op_type.__name__}." ) - # DEV-1475: ``IN`` / ``NOT IN`` carry a literal-only tuple RHS - # (``status in ('completed', 'pending')``). Reject scalar RHS, - # empty RHS, and any non-literal element so the binder can fold - # the predicate into a single ``InKey`` with confidence. Signed - # numerics (``amount in (-1, -2)``) are admitted by collapsing - # ``UnaryOp(USub/UAdd, Constant(int|float))`` to a signed - # ``Literal`` at validation time (Codex review). + # DEV-1475: ``IN`` / ``NOT IN`` carry a literal-only tuple RHS; scalar, + # empty, and non-literal RHS are rejected (signed numerics admitted). if op_type in (ast.In, ast.NotIn): rhs_node = node.comparators[0] if not isinstance(rhs_node, (ast.Tuple, ast.List)): @@ -1243,13 +1146,8 @@ def _convert_call( # NOSONAR(S3776) — the one call-dispatch ladder (colon pla kwargs=kwargs, ) - # Scalar function? Matched case-INSENSITIVELY: SQL function names are - # case-insensitive and users write ``COALESCE(x, 0)`` as readily as - # ``coalesce(x, 0)``. The legacy parser lowercased before the allowlist - # lookup; matching exactly here rejected every SQL-cased formula. The - # name is normalised to lower case on the way into ``ScalarCall`` so the - # two spellings intern to ONE key rather than two slots computing the - # same value. + # Scalar function? Case-insensitive match; normalised to lower case into + # ``ScalarCall`` so ``COALESCE`` / ``coalesce`` intern to one key. if func_name.lower() in SCALAR_FUNCTIONS: if kwargs: raise ValueError( @@ -1260,14 +1158,11 @@ def _convert_call( # NOSONAR(S3776) — the one call-dispatch ladder (colon pla _reject_bare_star_args(args, kwargs, func_name=func_name, original=original) return ScalarCall(name=func_name.lower(), args=args) - # Unknown name with an aggregatable first argument → AggCall candidate, - # validated at binding — parity with ``x:whatever`` (custom aggregations - # need no parser plumbing; genuinely unknown names get the standard - # unknown-aggregation error from the binder). + # Unknown name with an aggregatable first arg → AggCall candidate (parity + # with ``x:whatever``), validated at binding. if args and isinstance(args[0], _AGG_SOURCE_KINDS) and not _contains_agg_or_transform(args[0]): return AggCall(source=args[0], agg=func_name, args=args[1:], kwargs=kwargs) - # Otherwise — unknown. raise UnknownFunctionError( name=func_name, location=original, diff --git a/tests/test_dev1833_keyword_lexing.py b/tests/test_dev1833_keyword_lexing.py new file mode 100644 index 00000000..22a4f130 --- /dev/null +++ b/tests/test_dev1833_keyword_lexing.py @@ -0,0 +1,284 @@ +"""DEV-1833 — Mode-B keyword lexing hardened for Unicode identifiers. + +Identifiers named after, containing, or qualified by SQL keywords must parse +as ordinary references; keyword recognition is ASCII-exact, and CASE lowers +only when a depth-0 WHEN follows. Spec: +openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode. +""" + +from __future__ import annotations + +from decimal import Decimal + +import pytest + +from slayer.engine.syntax import ( + Arith, + BoolOp, + Cmp, + DottedRef, + Literal, + Ref, + ScalarCall, + UnaryOp, + _rewrite_sql_like, + parse_expr, + parse_filter_expr, +) + + +def _iif(node) -> ScalarCall: + """Assert ``node`` is the iif ScalarCall and return it.""" + assert isinstance(node, ScalarCall) + assert node.name == "iif" + return node + + +# --------------------------------------------------------------------------- # +# Bare keyword-named identifiers +# --------------------------------------------------------------------------- # +class TestBareKeywordIdentifiers: + def test_bare_case_is_a_ref(self) -> None: + assert parse_expr("case") == Ref(name="case") + + def test_case_in_arithmetic(self) -> None: + assert parse_expr("case + 1") == Arith( + op="+", left=Ref(name="case"), right=Literal(value=Decimal(1)), + ) + + def test_case_as_call_argument(self) -> None: + node = _iif(parse_expr("iif(case, 1, 2)")) + assert node.args[0] == Ref(name="case") + + def test_case_in_scalar_call(self) -> None: + assert parse_expr("upper(case)") == ScalarCall( + name="upper", args=(Ref(name="case"),), + ) + + def test_case_in_filter_predicate(self) -> None: + assert parse_filter_expr("case = 1") == Cmp( + op="==", left=Ref(name="case"), right=Literal(value=Decimal(1)), + ) + + def test_case_prefixed_name_untouched(self) -> None: + assert parse_expr("case_total * 2") == Arith( + op="*", left=Ref(name="case_total"), right=Literal(value=Decimal(2)), + ) + + +# --------------------------------------------------------------------------- # +# Keyword-named leaves of dotted references +# --------------------------------------------------------------------------- # +class TestQualifiedKeywordIdentifiers: + def test_dotted_case(self) -> None: + assert parse_expr("customers.case") == DottedRef(parts=("customers", "case")) + + def test_dotted_case_spaced_dot(self) -> None: + assert parse_expr("customers . case") == DottedRef( + parts=("customers", "case"), + ) + + def test_dotted_end_in_then_value(self) -> None: + node = _iif(parse_expr("CASE WHEN a THEN customers.end ELSE 0 END")) + assert node.args == ( + Ref(name="a"), + DottedRef(parts=("customers", "end")), + Literal(value=Decimal(0)), + ) + + +# --------------------------------------------------------------------------- # +# Unicode identifiers containing / adjacent to keyword spellings +# --------------------------------------------------------------------------- # +class TestUnicodeIdentifiers: + def test_unicode_prefixed_case(self) -> None: + assert parse_expr("écase") == Ref(name="écase") + + def test_decomposed_prefix(self) -> None: + # e + COMBINING ACUTE; Python NFKC-normalizes identifiers to composed é. + assert parse_expr("écase") == Ref(name="écase") + + def test_other_id_start_prefix(self) -> None: + assert parse_expr("℘case") == Ref(name="℘case") + + def test_unicode_name_with_underscore(self) -> None: + assert parse_expr("é_case") == Ref(name="é_case") + + def test_cjk_name(self) -> None: + assert parse_expr("变量") == Ref(name="变量") + + def test_trailing_combining_mark(self) -> None: + # A combining mark directly AFTER the keyword spelling; NFKC composes. + assert parse_expr("casé") == Ref(name="casé") + + +# --------------------------------------------------------------------------- # +# Unicode case-fold spoofs are never keywords +# --------------------------------------------------------------------------- # +class TestCaseFoldSpoofs: + def test_spoof_alone_is_identifier(self) -> None: + # LATIN SMALL LETTER LONG S uppercases to S; NFKC folds it to "case". + assert parse_expr("caſe") == Ref(name="case") + + def test_spoof_beside_real_case(self) -> None: + node = parse_expr("caſe + CASE WHEN x THEN 1 END") + assert isinstance(node, Arith) + assert node.left == Ref(name="case") + _iif(node.right) + + +# --------------------------------------------------------------------------- # +# Keyword-named identifiers coexisting with a real CASE +# --------------------------------------------------------------------------- # +class TestKeywordAlongsideRealCase: + def test_identifier_plus_case_when(self) -> None: + node = parse_expr("case + CASE WHEN x THEN 1 END") + assert isinstance(node, Arith) + assert node.left == Ref(name="case") + right = _iif(node.right) + assert right.args[0] == Ref(name="x") + + def test_identifier_in_when_condition(self) -> None: + node = _iif(parse_expr("CASE WHEN case THEN 1 WHEN other THEN 2 END")) + assert node.args[0] == Ref(name="case") + inner = _iif(node.args[2]) + assert inner.args[0] == Ref(name="other") + + +# --------------------------------------------------------------------------- # +# The WHEN-lookahead gate +# --------------------------------------------------------------------------- # +class TestWhenLookaheadGate: + def test_parenthesized_keyword_operand(self) -> None: + node = _iif(parse_expr("CASE (case) WHEN 1 THEN 2 END")) + assert node.args == ( + Cmp(op="==", left=Ref(name="case"), right=Literal(value=Decimal(1))), + Literal(value=Decimal(2)), + Literal(value=None), + ) + + def test_bare_keyword_operand_errors_loudly(self) -> None: + # Genuinely ambiguous — must error (any message), never silently corrupt. + with pytest.raises(ValueError): + parse_expr("CASE case WHEN 1 THEN 2 END") + + def test_case_with_no_when_degrades_to_generic_error(self) -> None: + with pytest.raises(ValueError, match=r"Invalid Mode-B expression"): + parse_expr("CASE case_total") + + def test_else_stops_the_gate(self) -> None: + node = _iif(parse_expr("CASE WHEN a THEN case ELSE 1 END")) + assert node.args[1] == Ref(name="case") + + def test_end_stops_the_gate(self) -> None: + node = _iif(parse_expr("CASE WHEN a THEN case END")) + assert node.args == ( + Ref(name="a"), Ref(name="case"), Literal(value=None), + ) + + +# --------------------------------------------------------------------------- # +# Real CASE still lowers (unchanged before/after the hardening) +# --------------------------------------------------------------------------- # +class TestCaseStillLowers: + def test_searched(self) -> None: + _iif(parse_expr("CASE WHEN amount > 5 THEN 1 ELSE 0 END")) + + def test_simple(self) -> None: + node = _iif(parse_expr("CASE region WHEN 'EU' THEN 1 ELSE 0 END")) + assert node.args[0] == Cmp( + op="==", left=Ref(name="region"), right=Literal(value="EU"), + ) + + def test_nested_in_then(self) -> None: + node = _iif(parse_expr( + "CASE WHEN a > 2 THEN CASE WHEN b > 1 THEN 1 ELSE 2 END ELSE 0 END" + )) + _iif(node.args[1]) + + def test_sql_operators_in_when(self) -> None: + node = _iif(parse_expr( + "CASE WHEN region = 'EU' AND amount IS NOT NULL THEN 1 ELSE 0 END" + )) + cond = node.args[0] + assert isinstance(cond, BoolOp) + assert cond.op == "and" + + def test_missing_end_still_specific(self) -> None: + with pytest.raises(ValueError, match=r"missing END"): + parse_expr("CASE WHEN a THEN 1") + + def test_missing_then_still_specific(self) -> None: + with pytest.raises(ValueError, match=r"missing its THEN"): + parse_expr("CASE WHEN a 1 END") + + +# --------------------------------------------------------------------------- # +# LIKE hardening +# --------------------------------------------------------------------------- # +class TestLikeHardening: + def test_escaped_quote_pattern(self) -> None: + assert parse_filter_expr(r"col LIKE 'It\'s%'") == ScalarCall( + name="like", args=(Ref(name="col"), Literal(value="It's%")), + ) + + def test_like_inside_string_literal_untouched(self) -> None: + node = parse_filter_expr("note == \"we like 'cats'\"") + assert node == Cmp( + op="==", left=Ref(name="note"), right=Literal(value="we like 'cats'"), + ) + + def test_dotless_i_is_not_like(self) -> None: + text = "x lıke 'p%'" + assert _rewrite_sql_like(text) == text + + def test_kelvin_sign_is_not_like(self) -> None: + text = "x liKe 'p%'" + assert _rewrite_sql_like(text) == text + + def test_spoofed_like_fails_loudly(self) -> None: + with pytest.raises(ValueError, match=r"Invalid Mode-B expression"): + parse_filter_expr("x lıke 'p%'") + + def test_spoofed_is_keyword_not_normalized(self) -> None: + with pytest.raises(ValueError, match=r"Invalid Mode-B expression"): + parse_filter_expr("x ıs None") + + +# --------------------------------------------------------------------------- # +# LIKE still rewrites (unchanged before/after the hardening) +# --------------------------------------------------------------------------- # +class TestLikeStillWorks: + def test_basic_like(self) -> None: + assert parse_filter_expr("name LIKE 'a%'") == ScalarCall( + name="like", args=(Ref(name="name"), Literal(value="a%")), + ) + + def test_not_like(self) -> None: + node = parse_filter_expr("name NOT LIKE 'a%'") + assert isinstance(node, UnaryOp) + assert node.op == "not" + assert isinstance(node.operand, ScalarCall) + assert node.operand.name == "like" + + def test_scalar_call_lhs(self) -> None: + node = parse_filter_expr("lower(customers.email) like '%@x.io'") + assert isinstance(node, ScalarCall) + assert node.name == "like" + assert node.args[0] == ScalarCall( + name="lower", args=(DottedRef(parts=("customers", "email")),), + ) + + def test_unicode_lhs_still_rewrites(self) -> None: + assert parse_filter_expr("é_col LIKE 'p%'") == ScalarCall( + name="like", args=(Ref(name="é_col"), Literal(value="p%")), + ) + + def test_ascii_mixed_case_still_rewrites(self) -> None: + node = parse_filter_expr("x LiKe 'p%'") + assert isinstance(node, ScalarCall) + assert node.name == "like" + + def test_double_quoted_pattern_rejected(self) -> None: + with pytest.raises(ValueError, match=r"Invalid Mode-B expression"): + parse_filter_expr('col like "p%"') From ecf48f01a71b36ddbfa8731b87e4bc14011e7018 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 13:08:10 +0200 Subject: [PATCH 2/7] DEV-1833: migrate _filter_refs_dsl onto the typed parser Reimplement _filter_refs_dsl on parse_filter_expr via a shared _walk_ref_names helper extracted from _measure_formula_refs (DEV-1826 expression sources included); reference order becomes expression order (sole caller aggregates into a set). Adds the parity suite and the migration-gains cases (funcstyle custom aggs, expression agg sources). --- slayer/engine/schema_drift.py | 45 +++++++++++----------- tests/test_schema_drift_typed.py | 65 +++++++++++++++++++++++++++++++- 2 files changed, 88 insertions(+), 22 deletions(-) diff --git a/slayer/engine/schema_drift.py b/slayer/engine/schema_drift.py index 24fada01..535126e1 100644 --- a/slayer/engine/schema_drift.py +++ b/slayer/engine/schema_drift.py @@ -20,7 +20,6 @@ from sqlglot.optimizer.scope import Scope, traverse_scope from slayer.core.enums import DataType -from slayer.core.formula import parse_filter from slayer.core.models import ( Column, DatasourceConfig, @@ -49,9 +48,11 @@ from slayer.engine.syntax import ( AggCall, DottedRef, + ParsedExpr, Ref, StarSource, parse_expr, + parse_filter_expr, walk_parsed_refs, ) from slayer.sql import engine_factory, sqlite_introspect @@ -492,30 +493,34 @@ def _parsed_ref_name(node: Union[Ref, DottedRef, AggCall]) -> Optional[str]: return ".".join(node.parts) -def _measure_formula_refs(formula: str) -> Set[str]: - """Column/measure names in a Mode-B formula (dotted for cross-model); - textual only. Both aggregation spellings parse natively (DEV-1826), and an - unknown functional name defers as an ``AggCall`` candidate, so custom - aggregations — joined-model ones included — need no registry walk.""" - try: - parsed = parse_expr(formula) - except Exception: - return set() - out: Set[str] = set() +def _walk_ref_names(parsed: ParsedExpr): + """Yield the name of each reference in a parsed Mode-B tree; an ``AggCall`` + collapses to its source name, a DEV-1826 expression source attributes each + operand ref, ``*`` sources yield nothing.""" for node in walk_parsed_refs(parsed): if isinstance(node, AggCall) and not isinstance( node.source, (Ref, DottedRef, StarSource) ): - # DEV-1826 expression source: attribute each operand ref. for inner in walk_parsed_refs(node.source): inner_name = _parsed_ref_name(inner) if inner_name is not None: - out.add(inner_name) + yield inner_name continue name = _parsed_ref_name(node) if name is not None: - out.add(name) - return out + yield name + + +def _measure_formula_refs(formula: str) -> Set[str]: + """Column/measure names in a Mode-B formula (dotted for cross-model); + textual only. Both aggregation spellings parse natively (DEV-1826), and an + unknown functional name defers as an ``AggCall`` candidate, so custom + aggregations — joined-model ones included — need no registry walk.""" + try: + parsed = parse_expr(formula) + except Exception: + return set() + return set(_walk_ref_names(parsed)) def _filter_refs(filter_str: str) -> list[str]: @@ -528,15 +533,13 @@ def _filter_refs(filter_str: str) -> list[str]: def _filter_refs_dsl(filter_str: str) -> list[str]: - """Column/measure references in a DSL (Mode B) filter; recovers base measures from ``agg_refs`` and strips synthesized colon aliases (``*`` excluded).""" + """Column/measure references in a DSL (Mode B) filter, in expression order + (deduplicated); ``[]`` on parse failure.""" try: - pf = parse_filter(filter_str) + parsed = parse_filter_expr(filter_str) except Exception: return [] - measure_names = [ref.measure_name for ref in pf.agg_refs if ref.measure_name != "*"] - canonical_aliases = set(pf.synthesized_aliases) - raw_columns = [c for c in pf.columns if c not in canonical_aliases] - return list(dict.fromkeys(measure_names + raw_columns)) + return list(dict.fromkeys(_walk_ref_names(parsed))) def _walk_alias_to_target_model( diff --git a/tests/test_schema_drift_typed.py b/tests/test_schema_drift_typed.py index 5228b309..64ed0553 100644 --- a/tests/test_schema_drift_typed.py +++ b/tests/test_schema_drift_typed.py @@ -22,7 +22,7 @@ from __future__ import annotations -from slayer.engine.schema_drift import _measure_formula_refs +from slayer.engine.schema_drift import _filter_refs_dsl, _measure_formula_refs from slayer.engine.syntax import ( AggCall, DottedRef, @@ -229,3 +229,66 @@ def test_colon_agg_dunder_source_is_extracted(self) -> None: assert _measure_formula_refs("robot__details:sum") == { "robot__details" } + + +class TestFilterRefsDslParity: + """DEV-1833 parity suite pinned while migrating ``_filter_refs_dsl`` onto + ``parse_filter_expr`` (verified green on the legacy implementation first). + Set equality only: reference order is expression order.""" + + def test_colon_agg(self) -> None: + assert set(_filter_refs_dsl("revenue:sum > 100")) == {"revenue"} + + def test_funcstyle_builtin_agg(self) -> None: + assert set(_filter_refs_dsl("sum(revenue) > 100")) == {"revenue"} + + def test_colon_agg_alias(self) -> None: + assert set(_filter_refs_dsl("revenue:countd > 5")) == {"revenue"} + + def test_colon_custom_agg(self) -> None: + assert set(_filter_refs_dsl("revenue:my_custom_agg > 5")) == {"revenue"} + + def test_dotted_path(self) -> None: + assert set(_filter_refs_dsl("customers.regions.name == 'EU'")) == { + "customers.regions.name" + } + + def test_dotted_colon_agg(self) -> None: + assert set(_filter_refs_dsl("customers.revenue:sum > 10")) == { + "customers.revenue" + } + + def test_star_count_excluded(self) -> None: + assert set(_filter_refs_dsl("*:count > 3")) == set() + + def test_like_filter(self) -> None: + assert set(_filter_refs_dsl("name LIKE 'a%'")) == {"name"} + + def test_plain_columns(self) -> None: + assert set(_filter_refs_dsl("status == 'x' and amount > 5")) == { + "status", + "amount", + } + + def test_agg_and_column_mixed(self) -> None: + assert set(_filter_refs_dsl("amount:sum > 100 and region == 'EU'")) == { + "amount", + "region", + } + + def test_unparseable_returns_empty(self) -> None: + assert _filter_refs_dsl("this is (( not parseable") == [] + + +class TestFilterRefsDslMigrationGains: + """Refs only the migrated ``parse_filter_expr`` implementation surfaces + (DEV-1833); red until the migration lands.""" + + def test_funcstyle_custom_agg(self) -> None: + assert set(_filter_refs_dsl("my_custom_agg(revenue) > 5")) == {"revenue"} + + def test_expression_agg_source(self) -> None: + assert set(_filter_refs_dsl("sum(amount - cost) > 0")) == { + "amount", + "cost", + } From 80f47a7b3ae0e4ffe4c86a68ee75e92b7fe0f41d Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 13:08:39 +0200 Subject: [PATCH 3/7] DEV-1833: retire legacy parse_filter; move ParsedFilter to sql_predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delete parse_filter and its whole private subtree (_preprocess_like/_sql_operators/ _concat, the _filter_node_to_sql emitter, __like__/__notlike__ machinery, _SUBQUERY_IN_FILTER_RE) — the typed pipeline is now the only Mode-B filter parser. Move the trimmed ParsedFilter (sql + columns) to sql_predicate.py, its sole remaining producer. Migrate the legacy parse_filter test call sites onto parse_filter_expr or delete those already covered by the typed suites, and scrub stale parse_filter references from code and docs. --- docs/architecture/parsing.md | 3 +- slayer/core/formula.py | 587 +------------------------------ slayer/sql/sql_predicate.py | 11 +- tests/facade/test_translator.py | 18 +- tests/test_dev1576_heals.py | 20 +- tests/test_dev1744_value_expr.py | 6 +- tests/test_formula.py | 367 +++---------------- tests/test_sql_generator.py | 69 +--- tests/test_syntax.py | 2 +- 9 files changed, 88 insertions(+), 995 deletions(-) diff --git a/docs/architecture/parsing.md b/docs/architecture/parsing.md index 1adab9da..440b6541 100644 --- a/docs/architecture/parsing.md +++ b/docs/architecture/parsing.md @@ -121,8 +121,7 @@ Filters historically accepted SQL operator spellings (`=`, `<>`, `NULL`, keyword `AND`/`OR`/`NOT`/`IS`/`IN`) alongside Python ones. `parse_filter_expr` normalizes those to Python equivalents (string-literal-aware) via `_normalize_sql_filter_operators`, then delegates to `parse_expr`. Measures and -order parse with `parse_expr` directly; only filters get the leniency — matching -the legacy `parse_filter` contract. +order parse with `parse_expr` directly; only filters get the leniency. ### `walk_parsed_refs` — scope-free reference extraction diff --git a/slayer/core/formula.py b/slayer/core/formula.py index 4d19458b..34573435 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -27,9 +27,7 @@ from slayer.core.refs import ( AGG_REF_RE as _AGG_REF_RE, IDENT_OR_PATH_RE as _IDENT_OR_PATH_RE, - canonical_agg_name, ) -from slayer.sql.window_detect import WINDOW_IN_FILTER_ERROR, has_window_function # Transforms that require a time dimension for ORDER BY TIME_TRANSFORMS = { @@ -75,20 +73,15 @@ "instr", "length", "concat", }) -CallCategory = Literal["transform", "scalar", "like_internal", "unknown"] - -_LIKE_INTERNAL_NAMES = frozenset({"__like__", "__notlike__"}) +CallCategory = Literal["transform", "scalar", "unknown"] def _classify_call_name(name: str) -> CallCategory: - """Categorize a Mode B function-call identifier — shared by the formula - and filter walkers.""" + """Categorize a Mode B function-call identifier.""" if name in ALL_TRANSFORMS: return "transform" if name.lower() in SCALAR_PASSTHROUGH: return "scalar" - if name in _LIKE_INTERNAL_NAMES: - return "like_internal" return "unknown" @@ -215,6 +208,8 @@ class MixedArithmeticField(BaseModel): # Aggregation names that are also transform names — ambiguous, need special handling _AMBIGUOUS_AGG_TRANSFORMS = BUILTIN_AGGREGATIONS & ALL_TRANSFORMS # {"first", "last"} +_STRING_LITERAL_RE = re.compile(r"'(?:[^'\\]|\\.)*'") + def _find_balanced_close(s: str, start: int) -> int: """Find the index of the balanced closing paren starting after the open paren at `start`.""" @@ -440,7 +435,7 @@ def _preprocess_agg_refs( def _replace(match: re.Match) -> str: measure_name = match.group(1) # DEV-1576: heal aggregation-name aliases / casing at the single colon- - # syntax chokepoint (shared by parse_formula and parse_filter). Unknown + # syntax chokepoint (parse_formula). Unknown # names pass through unchanged so the §3 enrichment error still fires. # A model-level custom aggregation named like an alias key / builtin # casing wins — skip healing for an exact custom-name match so it still @@ -954,578 +949,6 @@ def _parse_transform_kwargs( # NOSONAR S3776 — straight-line whitelist + per- return parsed -# --------------------------------------------------------------------------- -# Filter parsing -# --------------------------------------------------------------------------- - -# Internal filter functions (used after pre-processing operators like `like`) - - -class ParsedFilter(BaseModel): - """A parsed filter condition ready for SQL generation. - - The sql field contains a SQL-ready WHERE condition with column names - as-is (they get qualified with the model name during SQL generation). - """ - sql: str = Field(description="SQL WHERE condition, e.g. \"status = 'completed'\"") - columns: list[str] = Field(description="Column names referenced in the filter") - is_having: bool = Field(default=False, description="True if this is a HAVING filter (aggregate condition)") - is_post_filter: bool = Field(default=False, description="True if this references a computed column (transform/expression)") - synthesized_aliases: list[str] = Field( - default_factory=list, - description=( - "Canonical aggregation aliases this filter introduced from " - "colon syntax (e.g. ``revenue:sum`` → ``revenue_sum``, " - "``*:count`` → ``_count``). DEV-1369: strict-resolution uses " - "this exact set to validate bare names instead of a permissive " - "regex that would let typos like ``made_up_sum`` through." - ), - ) - agg_refs: list["AggregatedMeasureRef"] = Field( - default_factory=list, - description=( - "Aggregated measure references extracted from colon syntax in " - "the source filter (one per ``:`` occurrence). " - "Empty for Mode A SQL filters parsed by ``parse_sql_predicate``. " - "Schema drift uses these to recover the underlying measure name " - "(e.g. ``revenue`` from ``revenue:sum > 100``) — ``columns`` " - "alone only carries the canonical alias." - ), - ) - - -# LHS of a LIKE / NOT LIKE may be a bare/dotted identifier OR a single -# scalar call (e.g. ``lower(name)``, ``trim(customers.email)``). The call -# alternative is matched first so ``lower(name) like 'a%'`` resolves to -# the call form. ``[^()]*`` keeps this to one level of parens — nested -# scalar calls (``concat(lower(a), b) like '%'``) still fall through. -_LIKE_RE = re.compile( - r"\b(\w+\([^()]*\)|(?:\w+\.)*\w+)\s+(not\s+)?like\s+('[^']*')", - flags=re.IGNORECASE, -) - -_SUBQUERY_IN_FILTER_RE = re.compile( - r"\b(?:not\s+in|in|exists)\s*\(\s*select\b", - flags=re.IGNORECASE, -) - - -def _preprocess_like(formula: str) -> str: - """Convert SQL ``LIKE`` / ``NOT LIKE`` operators to internal function calls. - - Examples:: - - "name like '%acme%'" → "__like__(name, '%acme%')" - "name not like '%acme%'" → "__notlike__(name, '%acme%')" - "customers.email like '%@x.io'" → "__like__(customers.email, '%@x.io')" - - The LHS may be a bare identifier or a dotted path through joined - models — the same shape the rest of the DSL accepts in - ``dimensions`` / ``measures`` references. - """ - if "__like__" in formula or "__notlike__" in formula: - return formula - - def _sub(m: "re.Match[str]") -> str: - lhs, neg, pat = m.group(1), m.group(2), m.group(3) - fn = "__notlike__" if neg else "__like__" - return f"{fn}({lhs}, {pat})" - - return _LIKE_RE.sub(_sub, formula) - - -_STRING_LITERAL_RE = re.compile(r"'(?:[^'\\]|\\.)*'") - - -def _preprocess_concat(formula: str) -> str: - """Rewrite the SQL ``||`` concat operator to Python's ``<<`` so AST parsing - accepts it (DEV-1378). - - Python doesn't have ``||``; we substitute the bitwise-LShift token, - which has the right precedence relative to the operators a SLayer DSL - filter can carry — higher than comparison and boolean operators - (``==``, ``!=``, ``<``, ``>``, ``<=``, ``>=``, ``and``, ``or``, - ``not``), lower than additive/multiplicative arithmetic. The - ``BinOp(LShift, ...)`` AST node is then re-emitted by - :func:`_binop_to_sql` as a flat ``concat(, , ...)`` call, - folding chains at the same nesting level. - - String literals are preserved untouched: ``note = 'a||b'`` is left - alone. - """ - if "||" not in formula: - return formula - parts = _STRING_LITERAL_RE.split(formula) - literals = _STRING_LITERAL_RE.findall(formula) - rewritten_parts = [p.replace("||", "<<") for p in parts] - out: list[str] = [] - for i, p in enumerate(rewritten_parts): - out.append(p) - if i < len(literals): - out.append(literals[i]) - return "".join(out) - - -def _preprocess_sql_operators(formula: str) -> str: - """Normalize SQL operators to Python equivalents for AST parsing. - - Converts (outside string literals): - - ``NULL`` → ``None`` (case-insensitive, so ``IS NULL`` parses as ``is None``) - - ``IS``, ``NOT``, ``AND``, ``OR`` → lowercase (Python requires lowercase keywords) - - standalone ``=`` → ``==`` (so ``x = 1`` parses as ``x == 1``) - - ``<>`` → ``!=`` (so ``x <> 1`` parses as ``x != 1``) - """ - # Split into literal / non-literal segments to avoid mangling string contents - parts = _STRING_LITERAL_RE.split(formula) - literals = _STRING_LITERAL_RE.findall(formula) - - result = [] - for i, part in enumerate(parts): - part = re.sub(r'\bNULL\b', 'None', part, flags=re.IGNORECASE) - # Lowercase SQL keywords that are also Python keywords - for kw in ("IS", "NOT", "AND", "OR", "IN"): - part = re.sub(rf'\b{kw}\b', kw.lower(), part, flags=re.IGNORECASE) - part = re.sub(r'(?=!])=(?!=)', '==', part) - part = re.sub(r'<>', '!=', part) - result.append(part) - if i < len(literals): - result.append(literals[i]) - return "".join(result) - - -def parse_filter( - formula: str, - extra_agg_names: frozenset[str] | None = None, - named_measures: Mapping[str, str] | None = None, -) -> ParsedFilter: - """Parse a Mode B (DSL) filter formula into a ParsedFilter. - - Used by query-side filters (``SlayerQuery.filters``) and - ``ModelMeasure.formula`` predicates. Accepts: - - - aggregation colon syntax (``revenue:sum > 100``) → canonical names - - function-style aggregations (``sum(revenue) > 100``) → rewritten - - transform calls inside predicates (``change(revenue:sum) > 0``) - - Python or SQL operator spellings (``=``/``==``, ``<>``/``!=``) - - ``LIKE`` / ``NOT LIKE``, ``IS NULL`` / ``IS NOT NULL`` - - Rejects unknown function calls (the Python AST walker raises on any - call outside ``SCALAR_PASSTHROUGH`` and the internal ``__like__`` / - ``__notlike__`` helpers). - - Pre-rejects raw ``OVER (...)`` window-function syntax via - :func:`has_window_function` (the rank-family transforms cover the - ergonomic top-N case). - - For Mode A (SQL) filters — ``Column.filter`` and - ``SlayerModel.filters`` — see - :func:`slayer.sql.sql_predicate.parse_sql_predicate`. - - Args: - formula: The filter string to parse. - extra_agg_names: Additional aggregation names for function-style - rewriting. - named_measures: Mapping of saved-measure name → its formula. Bare - references to these names in filter expressions (e.g. - ``cumsum(aov) > 0``) are inline-expanded before parsing. - """ - if named_measures: - formula = _expand_named_measures(formula, named_measures) - # DEV-1336: reject raw window-function syntax (`OVER (...)`) before AST parsing. - # Python's ast.parse() rejects `over` as a keyword and surfaces a misleading - # "invalid syntax. Perhaps you forgot a comma?" error; the actionable error - # below points at SLayer's transforms / Column.sql / multi-stage models. - if has_window_function(formula): - raise ValueError(f"Filter '{formula}' {WINDOW_IN_FILTER_ERROR}") - # DEV-1376: detect SQL-style subqueries before ast.parse, so the agent - # gets a pointer at `source_queries` / `Column.sql` / joins instead of - # Python's "Perhaps you forgot a comma?" advice (which sends agents - # off on a nonsense recovery path). Strip string literals first so the - # sniff doesn't false-positive on subquery-shaped text inside a - # comparison literal like ``note = 'in (select …)'``. - if _SUBQUERY_IN_FILTER_RE.search(_STRING_LITERAL_RE.sub("''", formula)): - raise ValueError( - f"Subqueries are not allowed in DSL filters: {formula!r}. " - "Express the inner relation via `source_queries`, a derived " - "`Column.sql`, or by adding the related table to " - "`source_model.joins`." - ) - - # Rewrite function-style aggregations (e.g., sum(revenue) > 100 → revenue:sum > 100) - processed = _rewrite_funcstyle_aggregations(formula, extra_agg_names) - # Normalize SQL operator spellings to Python equivalents for AST parsing - # (=/<>/NULL → ==/!=/None) and rewrite LIKE / NOT LIKE / concat into helpers. - processed = _preprocess_sql_operators(processed) - processed = _preprocess_concat(processed) - processed = _preprocess_like(processed) - - # Pre-process colon syntax (e.g., "total_amount:sum") into canonical names. - # Include agg args/kwargs in the canonical name so e.g. - # ``revenue:sum(window='90d') > 100`` matches the windowed measure's alias - # ``orders.revenue_sum_window_90d`` and not the bare ``orders.revenue_sum``. - processed, agg_refs = _preprocess_agg_refs( - formula=processed, custom_agg_names=extra_agg_names or frozenset() - ) - agg_canonical = { - ph: canonical_agg_name( - measure_name=ref.measure_name, - aggregation_name=ref.aggregation_name, - agg_args=ref.agg_args, - agg_kwargs=ref.agg_kwargs, - ) - for ph, ref in agg_refs.items() - } - for ph, canonical in agg_canonical.items(): - processed = processed.replace(ph, canonical) - synthesized_aliases = list(dict.fromkeys(agg_canonical.values())) - - try: - tree = ast.parse(processed, mode="eval") - except SyntaxError as e: - raise ValueError(f"Invalid filter syntax: {formula!r} — {e}") - - columns: list[str] = [] - sql = _filter_node_to_sql(tree.body, formula, columns) - return ParsedFilter( - sql=sql, - columns=columns, - synthesized_aliases=synthesized_aliases, - agg_refs=list(agg_refs.values()), - ) - - -_BINOP_OP_MAP: dict[type, str] = { - ast.Add: "+", ast.Sub: "-", ast.Mult: "*", - ast.Div: "/", ast.Mod: "%", ast.Pow: "**", -} - - -# DEV-1539: SQL-precedence tier for each supported ``ast.BinOp`` op. -# Higher = tighter binding. Used by ``_binop_to_sql`` to decide whether -# a child BinOp's operands need parenthesising so the tree-encoded -# precedence survives serialisation. Only left-associative arithmetic -# ops live here: ``ast.Pow`` is intentionally absent because it is -# right-associative — its equal-precedence rule is the mirror of the -# others (wrap on LEFT, not right) and the simplest way to stay correct -# is to fall through to the ``parent_prec is None`` fallback in -# ``_emit_binop_operand`` (wrap every BinOp child unconditionally). -# That adds at most one harmless paren on mixed-precedence Pow -# expressions and guarantees ``(a ** b) ** c`` doesn't silently -# re-associate to ``a ** (b ** c)``. -_BINOP_PRECEDENCE: dict[type, int] = { - ast.Mult: 2, ast.Div: 2, ast.Mod: 2, - ast.Add: 1, ast.Sub: 1, -} - - -def _resolve_dotted_attribute(node: ast.expr) -> str: - """Render an ``ast.Attribute`` (or ``ast.Name`` leaf) as a dotted string.""" - if isinstance(node, ast.Name): - return node.id - if isinstance(node, ast.Attribute): - return f"{_resolve_dotted_attribute(node.value)}.{node.attr}" - raise ValueError(f"Unsupported node in dotted reference: {ast.dump(node)}") - - -def _compare_to_sql(node: ast.Compare, recur) -> str: - # DEV-1539: chained comparisons (``a < b < c``) have different - # semantics in Python (``(a < b) AND (b < c)``) and SQL (left-to-right - # ``(a < b) < c``, a boolean re-compared with ``c``). Reject up front - # with a pointer at the ``AND`` rewrite rather than emit silently - # wrong SQL. - if len(node.ops) > 1: - raise ValueError( - "Chained comparisons (e.g. `a < b < c`) are not supported in " - "DSL filters because their Python semantics differ from SQL. " - "Rewrite using AND: `a < b AND b < c`." - ) - # DEV-1539: wrap a Compare LHS/RHS that is ``ast.BinOp`` in outer - # parens so the comparator's precedence is explicit in the emitted - # SQL. ``ast.BoolOp`` is intentionally NOT included — ``_boolop_to_sql`` - # already self-wraps multi-operand outputs in ``(...)`` and a second - # layer would add noise. ``ast.LShift`` is also excluded — it is a - # marker for the SQL ``||`` concat operator (pre-processed by - # ``_preprocess_concat``) and ``_binop_to_sql`` rewrites the chain - # into a single ``concat(...)`` function call, which doesn't need - # an outer paren. - def _needs_wrap(n: ast.AST) -> bool: - return isinstance(n, ast.BinOp) and not isinstance(n.op, ast.LShift) - - left_sql = recur(node.left) - if _needs_wrap(node.left): - left_sql = f"({left_sql})" - parts = [left_sql] - for op, comparator in zip(node.ops, node.comparators): - # ``is None`` / ``is not None`` map to ``IS NULL`` / ``IS NOT NULL`` - # — ``_compare_op_to_sql`` already returns the complete operator - # string and there is no RHS to render. Every other ``is`` / - # ``is not`` (e.g. ``flag is True``) falls through to the - # standard ``IS `` / ``IS NOT `` emission; without - # this fall-through ``flag is True`` previously serialised as - # the broken ``flag IS`` (no RHS). - is_null_check = ( - isinstance(op, (ast.Is, ast.IsNot)) - and isinstance(comparator, ast.Constant) - and comparator.value is None - ) - sql_op = _compare_op_to_sql(op, comparator) - if is_null_check: - parts.append(sql_op) - continue - right_sql = recur(comparator) - if _needs_wrap(comparator): - right_sql = f"({right_sql})" - # Regular comparisons, IN / NOT IN, and IS / IS NOT with - # non-None RHS all take " "; for IN/NotIn the right - # is already "(val1, val2, ...)". - parts.append(f"{sql_op} {right_sql}") - return " ".join(parts) - - -def _boolop_to_sql(node: ast.BoolOp, recur) -> str: - op_str = "AND" if isinstance(node.op, ast.And) else "OR" - parts = [recur(v) for v in node.values] - joined = f" {op_str} ".join(parts) - return f"({joined})" if len(parts) > 1 else joined - - -def _unaryop_to_sql(node: ast.UnaryOp, recur) -> str: - if isinstance(node.op, ast.Not): - return f"NOT ({recur(node.operand)})" - if isinstance(node.op, ast.USub) and isinstance(node.operand, ast.Constant): - return str(-node.operand.value) - raise ValueError(f"Unsupported unary operator: {ast.dump(node)}") - - -def _attribute_to_sql(node: ast.Attribute, columns: list[str]) -> str: - dotted = f"{_resolve_dotted_attribute(node.value)}.{node.attr}" - columns.append(dotted) - return dotted - - -#: SQL spells its boolean literals in lower case, but Python's ``ast`` only -#: recognises ``True`` / ``False`` as constants — every other casing arrives -#: here as a name and would otherwise be resolved as a column. -_SQL_BOOLEAN_LITERALS = {"true": "TRUE", "false": "FALSE"} - - -def _name_to_sql(node: ast.Name, columns: list[str]) -> str: - literal = _SQL_BOOLEAN_LITERALS.get(node.id.lower()) - if literal is not None: - # Deliberately not appended to ``columns``: it is a value, not a - # reference, so strict name resolution must not see it. - return literal - if node.id != "None": - columns.append(node.id) - return node.id - - -def _constant_to_sql(node: ast.Constant) -> str: - if node.value is None: - return "NULL" - if isinstance(node.value, str): - return _escape_sql_string(node.value) - return str(node.value) - - -def _seq_to_sql(node, recur) -> str: - elts = [recur(e) for e in node.elts] - return f"({', '.join(elts)})" - - -def _flatten_lshift_chain(node: ast.AST, recur) -> list[str]: - """Flatten a chain of ``ast.BinOp(LShift)`` nodes into a flat list of - SQL strings. Used by ``_binop_to_sql`` to fold a ``a || b || c`` chain - (which arrives as left-associative LShift after `_preprocess_concat`) - into a single n-ary ``concat(...)`` call. - """ - if isinstance(node, ast.BinOp) and isinstance(node.op, ast.LShift): - return _flatten_lshift_chain(node.left, recur) + _flatten_lshift_chain(node.right, recur) - return [recur(node)] - - -def _binop_to_sql(node: ast.BinOp, original: str, recur) -> str: - if isinstance(node.op, ast.LShift): - # DEV-1378: SQL `||` was rewritten to `<<` by `_preprocess_concat`. - # Fold chained `<<` into a flat n-ary `concat(...)` SQL call. - operands = _flatten_lshift_chain(node, recur) - return f"concat({', '.join(operands)})" - op_str = _BINOP_OP_MAP.get(type(node.op)) - if op_str is None: - raise ValueError(f"Unsupported arithmetic operator in filter: {original!r}") - # DEV-1539: precedence-aware operand wrapping. Without this, - # ``(a + b) * c`` and ``a + b * c`` both serialise to - # ``a + b * c`` — the Python AST's parens are tracked only by tree - # shape, not by an explicit node, so re-emission via plain - # ``left op right`` silently drops grouping. Wrap a child BinOp - # whose operator has *strictly lower* precedence than this op - # (preserves grouping like ``(a + b) * c``); for the right operand - # also wrap on *equal* precedence so left-to-right associativity is - # preserved (``a - (b - c)`` vs ``a - b - c``). LShift / concat - # children carry their own grouping via ``concat(...)`` and are - # already self-contained. - parent_prec = _BINOP_PRECEDENCE.get(type(node.op)) - return f"{_emit_binop_operand(node.left, parent_prec, is_right=False, recur=recur)} {op_str} {_emit_binop_operand(node.right, parent_prec, is_right=True, recur=recur)}" - - -def _emit_binop_operand( - child: ast.AST, - parent_prec: int | None, - *, - is_right: bool, - recur, -) -> str: - """Render a ``BinOp`` operand, wrapping in ``(...)`` when the child's - precedence-tier rule says it would otherwise be misread on re-parse. - - The wrap rule (left-associative ops): - - - Left operand: wrap iff child has *strictly lower* precedence than - parent (``(a + b) * c`` — parent ``*``, left child ``+``). - - Right operand: wrap iff child has *lower or equal* precedence - (``a / (b * c)`` — parent ``/``, right child ``*``, equal — must - wrap to keep left-associative grouping intact). - - Conservative fallbacks (return wrap=True): the child is a BinOp - using a non-arithmetic op we haven't registered in - ``_BINOP_PRECEDENCE``, or ``parent_prec`` is missing. - """ - sql = recur(child) - if not isinstance(child, ast.BinOp): - return sql - if isinstance(child.op, ast.LShift): - # ``concat(...)`` is self-grouped — no extra wrap needed. - return sql - child_prec = _BINOP_PRECEDENCE.get(type(child.op)) - if parent_prec is None or child_prec is None: - return f"({sql})" - if child_prec < parent_prec: - return f"({sql})" - if is_right and child_prec == parent_prec: - return f"({sql})" - return sql - - -def _call_to_sql(node: ast.Call, original: str, recur) -> str: - if not isinstance(node.func, ast.Name): - raise ValueError(f"Unsupported call expression: {ast.dump(node)}") - func_name = node.func.id - category = _classify_call_name(func_name) - if category == "like_internal" and len(node.args) >= 2: - sql_op = "LIKE" if func_name == "__like__" else "NOT LIKE" - return f"{recur(node.args[0])} {sql_op} '{_get_string_arg(node.args[1], original)}'" - if category == "scalar": - # SCALAR_PASSTHROUGH: pass through with the user-written casing - # preserved; sqlglot re-spells per dialect at SQL-generation time. - if node.keywords: - raise ValueError( - f"Filter scalar function {func_name!r} does not accept " - f"keyword arguments: {original!r}" - ) - arg_sqls = [recur(a) for a in node.args] - return f"{func_name}({', '.join(arg_sqls)})" - raise ValueError(f"Unknown filter function '{func_name}' in: {original!r}") - - -def _filter_node_to_sql( - node: ast.AST, - original: str, - columns: list[str], -) -> str: - """Recursively convert a Mode B (DSL) filter AST node to a SQL fragment. - - Only the internal ``__like__`` / ``__notlike__`` helpers are accepted - as function calls; every other call name raises. Mode A (SQL) filters - are handled separately by - :func:`slayer.sql.sql_predicate.parse_sql_predicate`. - """ - - def recur(child: ast.AST) -> str: - return _filter_node_to_sql(child, original, columns) - - if isinstance(node, ast.Compare): - return _compare_to_sql(node, recur) - if isinstance(node, ast.BoolOp): - return _boolop_to_sql(node, recur) - if isinstance(node, ast.UnaryOp): - return _unaryop_to_sql(node, recur) - if isinstance(node, ast.Attribute): - return _attribute_to_sql(node, columns) - if isinstance(node, ast.Name): - return _name_to_sql(node, columns) - if isinstance(node, ast.Constant): - return _constant_to_sql(node) - if isinstance(node, (ast.Tuple, ast.List)): - return _seq_to_sql(node, recur) - if isinstance(node, ast.BinOp): - return _binop_to_sql(node, original, recur) - if isinstance(node, ast.Call): - return _call_to_sql(node, original, recur) - raise ValueError(f"Unsupported filter syntax: {original!r}") - - -def _compare_op_to_sql(op: ast.AST, comparator: ast.AST) -> str: - """Convert an ast comparison operator to SQL.""" - if isinstance(op, ast.Eq): - return "=" - elif isinstance(op, ast.NotEq): - return "!=" - elif isinstance(op, ast.Gt): - return ">" - elif isinstance(op, ast.GtE): - return ">=" - elif isinstance(op, ast.Lt): - return "<" - elif isinstance(op, ast.LtE): - return "<=" - elif isinstance(op, ast.In): - return "IN" - elif isinstance(op, ast.NotIn): - return "NOT IN" - elif isinstance(op, ast.Is): - if isinstance(comparator, ast.Constant) and comparator.value is None: - return "IS NULL" - return "IS" - elif isinstance(op, ast.IsNot): - if isinstance(comparator, ast.Constant) and comparator.value is None: - return "IS NOT NULL" - return "IS NOT" - raise ValueError(f"Unsupported comparison operator: {type(op).__name__}") - - -def _escape_sql_string(value: str) -> str: - """Render a Python string as a safely-quoted SQL string literal. - - Escapes both ``\\`` and ``'`` so the emitted literal is safe under every - supported dialect — including MySQL and ClickHouse, whose default string - parsing treats backslash as an escape character (so an unescaped trailing - ``\\`` would break out of the quoted literal). Backslashes are escaped - **before** single quotes so the newly-inserted ``''`` pair isn't itself - re-escaped into ``\\''``. - - Note: for strict-ANSI dialects (Postgres with ``standard_conforming_strings`` - on, SQLite, DuckDB) a literal backslash in the input is now rendered as - ``\\\\`` in the SQL, which those dialects treat as two backslashes. Since - measure filters almost never contain backslashes this trade-off is - preferred over a dialect-specific emission that could silently mis-escape - on MySQL. - """ - escaped = value.replace("\\", "\\\\").replace("'", "''") - return f"'{escaped}'" - - -def _get_string_arg(node: ast.AST, original: str) -> str: - """Extract a string value from an AST node (for LIKE patterns). - - Returns the content with single quotes doubled and backslashes escaped, - ready for interpolation between ``'...'`` in the emitted SQL. See - :func:`_escape_sql_string` for rationale. - """ - if isinstance(node, ast.Constant) and isinstance(node.value, str): - return node.value.replace("\\", "\\\\").replace("'", "''") - raise ValueError(f"Expected a string argument in filter: {original!r}") - - def _collect_names(node: ast.AST) -> list[str]: """Collect all Name and dotted Attribute references from an AST subtree.""" names = [] diff --git a/slayer/sql/sql_predicate.py b/slayer/sql/sql_predicate.py index 8591bd05..d3eca357 100644 --- a/slayer/sql/sql_predicate.py +++ b/slayer/sql/sql_predicate.py @@ -20,10 +20,19 @@ import re -from slayer.core.formula import ALL_TRANSFORMS, ParsedFilter +from pydantic import BaseModel, Field + +from slayer.core.formula import ALL_TRANSFORMS from slayer.core.refs import AGG_REF_RE from slayer.sql.window_detect import WINDOW_IN_FILTER_ERROR, has_window_function + +class ParsedFilter(BaseModel): + """A validated SQL-mode predicate ready for SQL generation.""" + sql: str = Field(description="SQL WHERE condition, e.g. \"status = 'completed'\"") + columns: list[str] = Field(description="Column names referenced in the filter") + + _STRING_LITERAL_RE = re.compile(r"'(?:[^'\\]|\\.)*'") _DSL_TRANSFORM_CALL_RE = re.compile( diff --git a/tests/facade/test_translator.py b/tests/facade/test_translator.py index 9dd5480b..8ed425a6 100644 --- a/tests/facade/test_translator.py +++ b/tests/facade/test_translator.py @@ -9,12 +9,15 @@ from __future__ import annotations import logging +import time import pytest +from pydantic import BaseModel from slayer.core.enums import DataType, JoinType, TimeGranularity from slayer.core.models import Column, ModelJoin, ModelMeasure, SlayerModel from slayer.core.query import ModelExtension +from slayer.engine.syntax import Cmp, Ref, parse_filter_expr from slayer.facade.catalog import FacadeCatalog, build_catalog from slayer.facade.rows import FacadeColumn, RowBatch from slayer.facade.translator import ( @@ -28,6 +31,7 @@ ResetSettingOp, SetSettingOp, TranslationError, + _classify_transaction_open, translate, ) @@ -141,8 +145,6 @@ def test_show_statement_is_noop_with_tag(dialect) -> None: def test_transaction_open_shim_does_not_over_match() -> None: - from slayer.facade.translator import _classify_transaction_open - # Not transaction-opens: a word merely starting with "begin", and real SQL. assert _classify_transaction_open("BEGINNER") is None assert _classify_transaction_open("SELECT * FROM begin_events") is None @@ -158,10 +160,6 @@ def test_transaction_open_regex_is_linear_on_pathological_input() -> None: client string could stall the asyncio loop. The possessive quantifier makes matching linear; assert a large pathological input classifies fast (well under a timeout the old regex would have blown).""" - import time - - from slayer.facade.translator import _classify_transaction_open - evil = "BEGIN" + " " * 200_000 + ";" + "x" * 5 start = time.perf_counter() result = _classify_transaction_open(evil) @@ -514,7 +512,6 @@ def test_classify_begin_commit_rollback_have_no_setting_capture(dialect) -> None def _row_batch(name: str, value: str) -> RowBatch: - from slayer.core.enums import DataType return RowBatch( columns=[FacadeColumn(name=name, type=DataType.TEXT)], rows=[{name: value}], @@ -574,14 +571,12 @@ def test_probe_result_settings_mutation_defaults_none(dialect) -> None: def test_set_setting_op_is_pydantic_model() -> None: """SetSettingOp must be a Pydantic BaseModel (frozen-ish; equal by value). Per project convention — never dataclasses.""" - from pydantic import BaseModel assert issubclass(SetSettingOp, BaseModel) assert SetSettingOp(name="x", value="y") == SetSettingOp(name="x", value="y") assert SetSettingOp(name="x", value="y") != SetSettingOp(name="x", value="z") def test_reset_setting_op_is_pydantic_model() -> None: - from pydantic import BaseModel assert issubclass(ResetSettingOp, BaseModel) assert ResetSettingOp(reset_all=True) == ResetSettingOp(reset_all=True) assert ( @@ -1021,8 +1016,9 @@ def test_double_quoted_column_in_where_becomes_column_not_string_literal(dialect # Emitted as a bare column, NOT a double-quoted string-literal lookalike. assert filters[0] == "status = 'paid'" # The Mode B DSL must read ``status`` as a column, not a literal. - from slayer.core.formula import parse_filter - assert parse_filter(filters[0]).columns == ["status"] + parsed = parse_filter_expr(filters[0]) + assert isinstance(parsed, Cmp) + assert parsed.left == Ref(name="status") def test_double_quoted_qualified_column_in_where_unquotes(dialect) -> None: diff --git a/tests/test_dev1576_heals.py b/tests/test_dev1576_heals.py index 7986cd7e..d6ddd141 100644 --- a/tests/test_dev1576_heals.py +++ b/tests/test_dev1576_heals.py @@ -1,7 +1,8 @@ """DEV-1576 — parse-level coverage for the three SlayerQuery heals. 1. Aggregation-name alias / casing normalization (``normalize_aggregation_name`` - + colon-syntax healing in ``parse_formula`` / ``parse_filter``). + + colon-syntax healing in ``parse_formula``; the typed pipeline heals at + binding — see ``tests/test_aggregation_gating.py``). 2. ``round()`` / ``abs()`` as top-level formula functions (parse into a ``MixedArithmeticField`` passthrough; arity validation). @@ -19,7 +20,6 @@ from slayer.core.formula import ( AggregatedMeasureRef, MixedArithmeticField, - parse_filter, parse_formula, ) @@ -88,7 +88,7 @@ def test_alias_table_only_targets_builtins(self) -> None: # --------------------------------------------------------------------------- -# §1 — colon-syntax healing through parse_formula / parse_filter +# §1 — colon-syntax healing through parse_formula # --------------------------------------------------------------------------- @@ -123,16 +123,6 @@ def test_unknown_agg_in_formula_left_for_enrichment(self) -> None: assert isinstance(result, AggregatedMeasureRef) assert result.aggregation_name == "bogus" - def test_filter_colon_alias_heals(self) -> None: - pf = parse_filter("revenue:countd > 5") - assert any(ref.aggregation_name == "count_distinct" for ref in pf.agg_refs) - # The canonical alias used downstream reflects the healed name. - assert any("count_distinct" in a for a in pf.synthesized_aliases) - - def test_filter_stddev_alias_heals(self) -> None: - pf = parse_filter("amount:stddev > 1") - assert any(ref.aggregation_name == "stddev_samp" for ref in pf.agg_refs) - def test_custom_agg_named_like_alias_not_healed(self) -> None: # A model custom aggregation named like an alias key takes precedence — # an exact custom-name match is NOT rewritten to the builtin. @@ -146,10 +136,6 @@ def test_alias_still_heals_when_custom_name_differs(self) -> None: ) assert result.aggregation_name == "count_distinct" - def test_custom_agg_named_like_alias_not_healed_in_filter(self) -> None: - pf = parse_filter("revenue:countd > 5", extra_agg_names=frozenset({"countd"})) - assert any(ref.aggregation_name == "countd" for ref in pf.agg_refs) - # --------------------------------------------------------------------------- # §2 — round() / abs() as top-level formula functions diff --git a/tests/test_dev1744_value_expr.py b/tests/test_dev1744_value_expr.py index 27334f1e..68980970 100644 --- a/tests/test_dev1744_value_expr.py +++ b/tests/test_dev1744_value_expr.py @@ -1917,9 +1917,9 @@ def test_no_parser_only_names_remain(self) -> None: assert SCALAR_PASSTHROUGH - SCALAR_FUNCTIONS == set() def test_binder_only_names_are_like_and_iif(self) -> None: - """``like`` is an operator (the parser handles it through its internal - ``__like__`` form) and ``iif`` is the CASE-rewrite target (DEV-1740); - neither is a legacy-formula pass-through name.""" + """``like`` is the LIKE-operator rewrite target and ``iif`` the + CASE-rewrite target (DEV-1740); neither is a legacy-formula + pass-through name.""" assert SCALAR_FUNCTIONS - SCALAR_PASSTHROUGH == {"like", "iif"} diff --git a/tests/test_formula.py b/tests/test_formula.py index 90d79a2b..56ad71d8 100644 --- a/tests/test_formula.py +++ b/tests/test_formula.py @@ -1,12 +1,13 @@ -"""Tests for the retained formula helpers — ``parse_filter`` (filter -injection + string-hygiene scalars) and ``_rewrite_funcstyle_aggregations`` -(function-style -> colon rewrite + ORDER BY normalization). - -The legacy free-function formula parser and its AST node types were removed -when the typed pipeline took over; their coverage now lives in -``test_syntax.py`` (parse shape), ``test_binding.py`` / -``test_transforms_planner.py`` (bind-time validation), -``test_transform_lowerer.py`` (change/change_pct desugar), +"""Tests for the retained formula helpers — ``_rewrite_funcstyle_aggregations`` +(function-style -> colon rewrite + ORDER BY normalization) — plus the typed +filter parser's injection hardening. + +The legacy free-function formula parser, its AST node types, and the legacy +``parse_filter`` were removed when the typed pipeline took over; their +coverage now lives in ``test_syntax.py`` (parse shape, LIKE / ``||`` / scalar +calls in filters), ``test_sql_boolean_literal_filters.py`` (SQL-cased boolean +literals), ``test_binding.py`` / ``test_transforms_planner.py`` (bind-time +validation), ``test_transform_lowerer.py`` (change/change_pct desugar), ``test_named_measures.py`` (named-measure expansion, end-to-end), ``test_model_measure_expansion.py`` (bind-time expansion, cycles, scoping), ``test_measure_expansion.py`` (expansion eligibility), and @@ -18,235 +19,52 @@ import pytest -from slayer.core.formula import ( - _rewrite_funcstyle_aggregations, - parse_filter, -) +from slayer.core.formula import _rewrite_funcstyle_aggregations from slayer.core.models import Aggregation from slayer.core.query import _FUNCSTYLE_PENDING, OrderItem +from slayer.engine.syntax import BoolOp, parse_filter_expr -class TestParseFilterBooleanLiterals: - """SQL's lowercase ``true`` / ``false`` arrive as ``ast`` names, not - constants, so without special handling they'd be read as column refs.""" +class TestFilterInjection: + """SQL-injection hardening for ``parse_filter_expr`` — the choke-point for + all user-supplied Mode-B filter expressions. Payloads must be rejected at + parse time; literal SQL emission is dialect-owned (sqlglot) and covered by + the SQL-generator round-trip suites.""" @pytest.mark.parametrize( - ("expression", "expected"), + "payload", [ - ("is_active = true", "is_active = TRUE"), - ("is_active = false", "is_active = FALSE"), - ("is_active = TRUE", "is_active = TRUE"), - ("is_active <> false", "is_active != FALSE"), + # Classic "break out of string, run DROP, comment rest". + "status = 'a'; DROP TABLE orders; --'", + # SQL block comment. + "status = 'a' /* foo */ OR 1=1", + # Stacked UNION SELECT. + "status = 'a' UNION SELECT * FROM users --'", + # Stacked statement via semicolon. + "status = 'a'; SELECT 1", + # DROP smuggled where an identifier is expected. + "status; DROP TABLE users; --", ], ) - def test_boolean_literal_renders_as_sql( - self, expression: str, expected: str, - ) -> None: - parsed = parse_filter(expression) - assert parsed.sql == expected - # The literal is a value, not a reference — it must not land in columns. - assert parsed.columns == ["is_active"] - - def test_python_cased_literals_keep_working(self) -> None: - # ``True`` / ``False`` are ``ast`` constants, so they skip the name path. - assert parse_filter("is_active = True").sql == "is_active = True" - assert parse_filter("is_active = False").sql == "is_active = False" - - def test_bare_literal_is_not_a_column_reference(self) -> None: - # ``true`` wins over a same-named column — moot, it's a reserved word. - assert parse_filter("true").columns == [] - - -class TestParseFilterInjection: - """SQL-injection hardening for ``parse_filter``. - - ``parse_filter`` is the single choke-point for all user-supplied filter - expressions (measure-level ``filter``, model-level ``filters``, and - query-level filters). These tests assert each injection payload is either - rejected at parse time (``ValueError``) or neutralised — i.e. the payload - appears in the output SQL only as a properly-quoted string literal, never - as executable SQL tokens. - """ - - # --- Payloads rejected outright by ast.parse --------------------------- - - def test_rejects_statement_terminator_dropout(self) -> None: - """Classic "break out of string, run DROP, comment rest" payload. - - Trailing ``--`` terminates with a single-quoted ``D`` followed by an - unclosed apostrophe, which cannot parse as a Python expression. - """ - with pytest.raises(ValueError, match="Invalid filter syntax"): - parse_filter("status = 'a'; DROP TABLE orders; --'") - - def test_rejects_block_comment(self) -> None: - """SQL block-comment tokens must not survive — ``/`` without a RHS - operand yields a Python SyntaxError.""" - with pytest.raises(ValueError, match="Invalid filter syntax"): - parse_filter("status = 'a' /* foo */ OR 1=1") - - def test_rejects_union_select(self) -> None: - """Stacked UNION SELECT payload — ``SELECT`` is not a Python operand.""" - with pytest.raises(ValueError, match="Invalid filter syntax"): - parse_filter("status = 'a' UNION SELECT * FROM users --'") - - def test_rejects_stacked_semicolon(self) -> None: - """A bare semicolon separates Python statements; ``eval`` mode rejects.""" - with pytest.raises(ValueError, match="Invalid filter syntax"): - parse_filter("status = 'a'; SELECT 1") - - def test_rejects_unknown_function_call(self) -> None: - """Only the internal ``__like__`` / ``__notlike__`` helpers are allowed.""" - with pytest.raises(ValueError, match="Unknown filter function"): - parse_filter("pg_sleep(10)") - - # --- Payloads that are legitimate expressions --------------------------- + def test_payload_rejected_at_parse(self, payload: str) -> None: + with pytest.raises(ValueError, match="Invalid Mode-B expression"): + parse_filter_expr(payload) def test_allows_tautology_with_literal(self) -> None: - """``1 = 1`` is a legal, user-authored tautology — not injection per se. - - A measure filter written by the model author is by design trusted to - express arbitrary boolean logic; this test pins the intended semantics - so we don't accidentally over-restrict the grammar. - """ - result = parse_filter("status = 'a' or 1 = 1") - assert "OR" in result.sql - assert "1 = 1" in result.sql - - # --- Payloads that must be neutralised in the emitted SQL -------------- - - def test_embedded_quote_is_doubled(self) -> None: - """Single quote inside a string literal must emit as ``''`` (SQL standard).""" - # The runtime filter value here contains an embedded apostrophe. - result = parse_filter("name = 'O\\'Brien'") - # Emitted literal must have a doubled quote, never a bare ``'``. - assert "'O''Brien'" in result.sql - - def test_backslash_in_string_literal_is_escaped(self) -> None: - """A backslash inside a string literal must not be able to escape the - closing quote in MySQL-family dialects. - - Before the fix: ``parse_filter`` emits ``'a\\'`` (single backslash - inside single quotes). In MySQL default mode, ``\\'`` is a literal - apostrophe and the string remains open, letting trailing tokens be - read as string content. After the fix: the backslash is doubled so - the emitted literal is ``'a\\\\'`` (two backslashes = one literal - backslash in MySQL's escape-aware string parsing). - """ - # Runtime filter string is: name = 'a\' (six chars) - # Python source: "name = 'a\\\\'" (escape both backslashes) - result = parse_filter("name = 'a\\\\'") - # The emitted SQL must not contain an unescaped trailing ``\'`` that - # MySQL would read as a literal quote. - assert "'a\\\\'" in result.sql, ( - f"Expected backslash-escaped literal, got {result.sql!r}" - ) - - def test_backslash_mid_string_is_escaped(self) -> None: - """Backslash anywhere inside a string literal must be doubled so that - subsequent characters can't be (mis)interpreted as escape sequences. - """ - # Runtime string: name = 'a\b' and x = 1 - result = parse_filter("name = 'a\\\\b' and x = 1") - assert "'a\\\\b'" in result.sql - # Sanity: the surrounding AND clause is preserved intact. - assert "x = 1" in result.sql - - def test_backslash_in_like_pattern_is_escaped(self) -> None: - """The ``LIKE`` pattern path runs through ``_get_string_arg`` — make - sure it applies the same backslash protection as ``_filter_node_to_sql``. - """ - # Runtime string: name like 'a\' - result = parse_filter("name like 'a\\\\'") - assert "LIKE" in result.sql - assert "'a\\\\'" in result.sql - - def test_identifier_cannot_inject_sql(self) -> None: - """Bare column names are constrained to valid Python identifiers. - - A name containing a space / punctuation can't even reach the AST as - an ``ast.Name``, so there's no way to sneak ``DROP`` in via a name. - """ - with pytest.raises(ValueError, match="Invalid filter syntax"): - parse_filter("status; DROP TABLE users; --") + # ``1 = 1`` is a legal, user-authored tautology — not injection per se. + result = parse_filter_expr("status = 'a' or 1 = 1") + assert isinstance(result, BoolOp) + assert result.op == "or" def test_deeply_nested_boolean_does_not_crash(self) -> None: - """A very deep boolean expression must either parse bounded or raise - cleanly — never crash the interpreter / exhaust the stack.""" + # 200 chained ORs must parse bounded or raise cleanly — never crash. payload = " or ".join(["x = 1"] * 200) - # Either accepted (returns SQL containing many ORs) or rejected with - # a normal ValueError; both are acceptable outcomes. try: - result = parse_filter(payload) + result = parse_filter_expr(payload) except ValueError: return - assert result.sql.count("OR") >= 100 - - # --- DEV-1376: path-qualified LIKE / NOT LIKE --------------------------- - - def test_like_path_qualified_simple_literal(self) -> None: - """``. like '...'`` must parse — agents reach for - this shape because dotted refs work in dimensions/measures.""" - result = parse_filter("infrastructure.wateraccess like '%yes%'") - assert "infrastructure.wateraccess LIKE '%yes%'" in result.sql - - def test_like_path_qualified_messy_literal(self) -> None: - """Literal content (commas, spaces, mixed case) must not affect - whether path-qualified LIKE parses. Reproduces the original - benchmark failure (households_14).""" - result = parse_filter( - "infrastructure.wateraccess like '%Yes, available at least in one room%'" - ) - assert ( - "infrastructure.wateraccess LIKE " - "'%Yes, available at least in one room%'" - ) in result.sql - - def test_not_like_path_qualified(self) -> None: - """NOT LIKE on a dotted path mirrors the LIKE fix.""" - result = parse_filter("customers.email not like '%spam.com'") - assert "customers.email NOT LIKE '%spam.com'" in result.sql - - # --- Scalar-call LHS for LIKE / NOT LIKE -------------------------------- - - def test_like_scalar_call_lhs(self) -> None: - """``lower(name) like 'a%'`` and friends — LIKE preprocessor must - match scalar calls on the LHS, not just bare/dotted identifiers.""" - result = parse_filter("lower(name) like 'a%'") - assert "lower(name) LIKE 'a%'" in result.sql - - def test_not_like_scalar_call_lhs(self) -> None: - result = parse_filter("trim(email) not like '%@test.com'") - assert "trim(email) NOT LIKE '%@test.com'" in result.sql - - def test_like_scalar_call_dotted_arg(self) -> None: - """The scalar call's argument can be a dotted ref.""" - result = parse_filter("lower(customers.email) like '%@motley.ai'") - assert "lower(customers.email) LIKE '%@motley.ai'" in result.sql - - # --- DEV-1376: subquery-in-filter helpful error ------------------------- - - def test_filter_subquery_in_clause_raises(self) -> None: - """``IN (SELECT ...)`` should surface the targeted error instead of - Python's misleading "Perhaps you forgot a comma" advice.""" - with pytest.raises(ValueError, match="Subqueries are not allowed"): - parse_filter("housenum in (select houselink from properties)") - - def test_filter_subquery_not_in_clause_raises(self) -> None: - """``NOT IN (SELECT ...)`` is also a subquery shape.""" - with pytest.raises(ValueError, match="Subqueries are not allowed"): - parse_filter("id not in (select id from t)") - - def test_filter_exists_subquery_raises(self) -> None: - """``EXISTS (SELECT ...)`` is also a subquery shape.""" - with pytest.raises(ValueError, match="Subqueries are not allowed"): - parse_filter("exists (select 1 from t)") - - def test_filter_subquery_shape_inside_string_literal_does_not_raise(self) -> None: - """The subquery sniff must ignore SQL-shaped text that lives inside a - string-literal RHS of a comparison — it's data, not syntax.""" - result = parse_filter("note = 'in (select 1 from t)'") - assert "note = 'in (select 1 from t)'" in result.sql + assert isinstance(result, BoolOp) + assert len(result.operands) >= 100 # --------------------------------------------------------------------------- @@ -494,102 +312,31 @@ def test_weighted_avg_args_stripped(self) -> None: class TestStringHygieneFilters: - """DEV-1378: lowercase string-hygiene scalar functions accepted inline - in Mode B (DSL) filters: ``lower``, ``upper``, ``trim``, ``replace``, - ``substr``, ``instr``, ``length``, ``concat``. The SQL ``||`` - operator is rewritten to ``concat(...)`` by ``_preprocess_concat``. - """ - - @pytest.mark.parametrize("op", ["lower", "upper", "trim", "length"]) - def test_unary_op_round_trips(self, op: str) -> None: - pf = parse_filter(f"{op}(name) = 'eu'") - assert pf.sql == f"{op}(name) = 'eu'" - assert "name" in pf.columns - - def test_replace_three_arg(self) -> None: - pf = parse_filter("replace(x, ',', '') = 'foo'") - assert pf.sql == "replace(x, ',', '') = 'foo'" - assert "x" in pf.columns - - def test_substr_three_arg(self) -> None: - pf = parse_filter("substr(s, 1, 5) = 'abcde'") - assert pf.sql == "substr(s, 1, 5) = 'abcde'" - assert "s" in pf.columns - - def test_substr_two_arg(self) -> None: - pf = parse_filter("substr(s, 3) = 'abc'") - assert pf.sql == "substr(s, 3) = 'abc'" - - def test_instr_with_string_literal(self) -> None: - pf = parse_filter("instr(s, ',') > 0") - assert pf.sql == "instr(s, ',') > 0" - assert "s" in pf.columns - - def test_concat_explicit_call(self) -> None: - pf = parse_filter("concat(a, b, c) = 'abc'") - assert pf.sql == "concat(a, b, c) = 'abc'" - assert {"a", "b", "c"}.issubset(set(pf.columns)) - - def test_nested_length_replace(self) -> None: - pf = parse_filter("length(replace(x, ',', '')) > 0") - assert pf.sql == "length(replace(x, ',', '')) > 0" - assert "x" in pf.columns - - def test_substr_instr_pairing(self) -> None: - # Canonical "first delimited token" pattern from the issue. - pf = parse_filter("substr(s, 1, instr(s, ',') - 1) = 'first'") - assert pf.sql == "substr(s, 1, instr(s, ',') - 1) = 'first'" - - def test_pipe_pipe_two_operands(self) -> None: - pf = parse_filter("a || b = 'foo'") - assert pf.sql == "concat(a, b) = 'foo'" - assert {"a", "b"}.issubset(set(pf.columns)) + """DEV-1378 string-hygiene shapes on the typed filter parser; single + scalar calls and the two-operand ``||`` are covered by + ``test_syntax.py::TestScalarFunctions`` / + ``TestFilterOperatorNormalization``.""" def test_pipe_pipe_chain_three_operands(self) -> None: - # Chained `||` folds into a flat n-ary concat. - pf = parse_filter("a || b || c = 'foo'") - assert pf.sql == "concat(a, b, c) = 'foo'" + # Chained `||` desugars left-associatively to nested concat calls. + result = parse_filter_expr("a || b || c = 'foo'") + outer = result.left + assert outer.name == "concat" + assert outer.args[0].name == "concat" def test_pipe_pipe_no_spaces(self) -> None: - pf = parse_filter("a||b = 'foo'") - assert pf.sql == "concat(a, b) = 'foo'" + result = parse_filter_expr("a||b = 'foo'") + assert result.left.name == "concat" def test_pipe_pipe_with_function_call_operands(self) -> None: - pf = parse_filter("lower(name) || ' ' || trim(addr) = 'eu london'") - assert pf.sql == "concat(lower(name), ' ', trim(addr)) = 'eu london'" + result = parse_filter_expr("lower(name) || ' ' || trim(addr) = 'eu london'") + assert result.left.name == "concat" - def test_pipe_pipe_preserves_string_literal(self) -> None: + def test_pipe_pipe_preserved_in_string_literal(self) -> None: # `||` inside a string literal must NOT be rewritten. - pf = parse_filter("note = 'a||b'") - assert pf.sql == "note = 'a||b'" + result = parse_filter_expr("note = 'a||b'") + assert result.right.value == "a||b" def test_function_name_preserved_in_string_literal(self) -> None: - pf = parse_filter("note = 'lower(x)'") - assert pf.sql == "note = 'lower(x)'" - - def test_uppercase_function_name_accepted(self) -> None: - # SCALAR_PASSTHROUGH lookup is case-insensitive, matching the - # formula-side policy. The user-written casing is preserved on - # emission so sqlglot can re-spell per dialect. - pf = parse_filter("LOWER(name) = 'eu'") - assert "LOWER(name)" in pf.sql - - def test_substring_synonym_accepted(self) -> None: - # Both ``substr`` (SQLite) and ``substring`` (Postgres/ANSI) are - # in the unified SCALAR_PASSTHROUGH set. - pf = parse_filter("substring(s, 1, 5) = 'abcde'") - assert "substring(s, 1, 5)" in pf.sql - - -class TestUnifiedScalarPassthrough: - """The canonical SCALAR_PASSTHROUGH set drives the retained filter - surface. The formula-side half of this class was deleted with the legacy - parser; the typed parser's allowlist is covered by - ``test_syntax.py::TestScalarFunctions`` and - ``test_keys.py::TestScalarFunctionsAllowlist``.""" - - def test_filter_uses_same_set(self) -> None: - # The filter walker consults SCALAR_PASSTHROUGH so things like - # coalesce / greatest are accepted in filters too. - pf = parse_filter("coalesce(name, 'unknown') = 'foo'") - assert "coalesce" in pf.sql.lower() + result = parse_filter_expr("note = 'lower(x)'") + assert result.right.value == "lower(x)" diff --git a/tests/test_sql_generator.py b/tests/test_sql_generator.py index 203d2943..4aabc65c 100644 --- a/tests/test_sql_generator.py +++ b/tests/test_sql_generator.py @@ -11,7 +11,6 @@ from pydantic import ValidationError as PydanticValidationError from slayer.core.enums import DataType, TimeGranularity -from slayer.core.formula import parse_filter from slayer.core.models import Aggregation, AggregationParam, Column, DatasourceConfig, ModelJoin, ModelMeasure, SlayerModel from slayer.core.query import ColumnRef, OrderItem, SlayerQuery, TimeDimension from slayer.engine.query_engine import SlayerQueryEngine @@ -4879,7 +4878,7 @@ async def test_backslash_mid_string_is_neutralised( async def test_like_pattern_backslash_is_neutralised( self, orders_model: SlayerModel, dialect: str, ) -> None: - """The ``LIKE`` path in ``_filter_node_to_sql`` goes through a separate helper (``_get_string_arg``); its backslash handling must match.""" + """The LIKE-pattern literal path must apply the same backslash protection as plain string literals.""" orders_model.columns.append( Column( name="evil", @@ -9798,39 +9797,6 @@ async def test_dsl_compare_call_lhs_not_wrapped( f"Spurious parens around LOWER(...) call; got:\n{norm}" ) - @pytest.mark.parametrize( - ["formula", "expected_sql"], - [ - # IS NULL / IS NOT NULL stay as-is — `_compare_op_to_sql` returns the complete operator string when the RHS is None. - ("flag is None", "flag IS NULL"), - ("flag is not None", "flag IS NOT NULL"), - # Non-None IS / IS NOT: previously the `continue` in the IS/IsNot branch dropped the RHS and emitted broken SQL like `flag IS` / `flag IS NOT`. Fall-through must render `IS ` / `IS NOT `. - ("flag is True", "flag IS True"), - ("flag is not False", "flag IS NOT False"), - # IS-non-None composed with another predicate still flows through `_boolop_to_sql`'s outer wrap. - ("flag is True and value > 0", "(flag IS True AND value > 0)"), - ], - ) - def test_dsl_compare_is_isnot_with_non_none_rhs( - self, formula: str, expected_sql: str, - ) -> None: - """``is`` / ``is not`` against a non-None RHS used to drop the RHS entirely and emit the broken ``IS`` / ``IS NOT`` operator string. Fix: only the ``is None`` / ``is not None`` paths short-circuit to the complete operator; everything else falls through to the standard `` `` emission.""" - pf = parse_filter(formula) - assert pf.sql == expected_sql, ( - f"parse_filter({formula!r}).sql == {pf.sql!r}, expected " - f"{expected_sql!r}" - ) - - def test_dsl_chained_compare_rejected(self) -> None: - """Chained comparisons (``a < b < c``) have different semantics between Python and SQL. Python: ``(a < b) AND (b < c)``. SQL: ``(a < b) < c`` (a boolean re-compared to c). The DSL parser must reject chained comparisons with a clear, actionable error rather than silently emit subtly wrong SQL.""" - with pytest.raises(ValueError, match=r"[Cc]hained comparison") as excinfo: - parse_filter("a < b < c") - # The error must point at the actionable alternative. - assert "AND" in str(excinfo.value) or "and" in str(excinfo.value), ( - f"Chained-compare rejection should point at the `AND` rewrite; " - f"got: {excinfo.value!r}" - ) - async def test_dsl_compare_lhs_boolop_wrapped( self, generator: SQLGenerator, orders_model: SlayerModel, ) -> None: @@ -10003,39 +9969,6 @@ async def test_filter_inline_preserves_backslash_in_column_sql( f"preserved in emitted SQL; got:\n{sql}" ) - @pytest.mark.parametrize( - ["formula", "expected_sql"], - [ - # Inner low-prec child under high-prec parent — left operand. - ("(a + b) * c > 10", "((a + b) * c) > 10"), - # Same shape — RHS of comparator. - ("a > (b + c) * d", "a > ((b + c) * d)"), - # Equal-precedence right child of /, must stay wrapped. - ("a / (b * c) > 0", "(a / (b * c)) > 0"), - # Equal-precedence right child of -, must stay wrapped to preserve right-grouping semantics. - ("a - (b - c) > 0", "(a - (b - c)) > 0"), - # Left-assoc, no source parens: no inner wrap needed. - ("a - b - c > 0", "(a - b - c) > 0"), - # Higher-precedence child under lower-precedence parent: no wrap needed — `a + b * c` reads correctly bare. - ("a + b * c > 10", "(a + b * c) > 10"), - # User-supplied parens around left equal-precedence are semantically a no-op (`(a + b) + c` == `a + b + c`) so we don't emit a stray inner wrap. - ("(a + b) + c > 0", "(a + b + c) > 0"), - # Pow is right-associative: (a ** b) ** c must keep its inner parens, else it re-parses as a ** (b ** c) — a different result. - ("(a ** b) ** c > 0", "((a ** b) ** c) > 0"), - # `a ** b ** c` parses RIGHT-assoc; emission must preserve the grouping via explicit parens on the right operand. - ("a ** b ** c > 0", "(a ** (b ** c)) > 0"), - ], - ) - def test_dsl_compare_preserves_nested_arithmetic_precedence( - self, formula: str, expected_sql: str, - ) -> None: - """DEV-1539: ``_binop_to_sql`` must wrap nested child operands so the AST-encoded operator precedence survives serialisation. Without this, ``(a + b) * c > 10`` and ``a + b * c > 10`` would both emit as ``(a + b * c) > 10`` — semantically distinct inputs collapse to the same output, silently changing results.""" - pf = parse_filter(formula) - assert pf.sql == expected_sql, ( - f"parse_filter({formula!r}).sql == {pf.sql!r}, expected " - f"{expected_sql!r}" - ) - @pytest.mark.parametrize( ["body_sql", "connector"], [ diff --git a/tests/test_syntax.py b/tests/test_syntax.py index 55e5da38..7b9c17ad 100644 --- a/tests/test_syntax.py +++ b/tests/test_syntax.py @@ -647,7 +647,7 @@ def test_sql_concat_pipe_pipe_normalised(self): def test_sql_like_operator_normalised(self): # DEV-1704: a SQL ``LIKE`` operator (e.g. from the pg-facade WHERE # translation) normalises to the ``like(col, pattern)`` scalar the DSL - # already emits as SQL LIKE — matching formula._preprocess_like. + # already emits as SQL LIKE. result = parse_filter_expr("name LIKE 'do%'") assert isinstance(result, ScalarCall) assert result.name == "like" From 440230fb6bb413cf095f75b18aafef961ef85950 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 14:53:45 +0200 Subject: [PATCH 4/7] DEV-1833: extend the Unicode-identifier keyword guard to the CASE siblings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CASE rewriter was hardened for Other_ID_Start / combining-mark identifiers, but the sibling keyword rewrites still keyed off \b/\w and corrupted a reference fused to a keyword spelling: ℘NULL→℘None, éname LIKE→élike(name,…), and ℘OVER( misread as a raw window. Route the NULL/operator subs through the new _sub_keyword_isolated and add leading-edge identifier guards to the LIKE/OVER scans, mirroring _case_keyword's _is_ident_adjacent check. Also adds a filter-path alias-healing regression (CodeRabbit nitpick): countd / stddev in a HAVING predicate heal after the typed-parser filter migration. --- slayer/engine/syntax.py | 33 +++++++++++++--- tests/test_aggregation_gating.py | 23 +++++++++++ tests/test_dev1833_keyword_lexing.py | 57 ++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 5 deletions(-) diff --git a/slayer/engine/syntax.py b/slayer/engine/syntax.py index 3605081d..dbc0c8a9 100644 --- a/slayer/engine/syntax.py +++ b/slayer/engine/syntax.py @@ -149,7 +149,12 @@ def _rewrite_sql_like(text: str) -> str: spans = [(m.start(), m.end()) for m in _PY_STRING_LITERAL_RE.finditer(text)] def _sub(m: "re.Match[str]") -> str: - if any(s <= m.start() < e for s, e in spans): + # Skip a match inside a string literal, or one whose LHS is fused into a + # Unicode identifier the ``\b``/``\w`` LHS token splits (``℘name``, + # decomposed ``éname``) — that name is a reference, not a LIKE operand. + if any(s <= m.start() < e for s, e in spans) or _is_ident_adjacent( + text, m.start() - 1 + ): return m.group(0) lhs, neg, pat = m.group(1), m.group(2), m.group(3) call = f"like({lhs}, {pat})" @@ -184,6 +189,19 @@ def _is_ident_adjacent(text: str, pos: int) -> bool: return 0 <= pos < len(text) and ("a" + text[pos]).isidentifier() +def _sub_keyword_isolated(pattern: "re.Pattern[str]", repl: str, text: str) -> str: + """``pattern.sub(repl, text)`` but leaving a keyword match fused into a + Unicode identifier untouched — combining marks or ``Other_ID_Start`` symbols + (``℘``) on either side that ``\\b``/``\\w`` cannot see would otherwise + rewrite ``℘NULL`` → ``℘None`` or ``℘AND`` → ``℘and``, silently rebinding the + reference (mirrors ``_case_keyword``'s adjacency guard).""" + def _guard(m: "re.Match[str]") -> str: + if _is_ident_adjacent(text, m.start() - 1) or _is_ident_adjacent(text, m.end()): + return m.group(0) + return repl + return pattern.sub(_guard, text) + + def _case_keyword(text: str, tok: Tuple[Optional[str], str, int, int]) -> Optional[str]: """The CASE-grammar keyword a token spells, or None for an identifier. @@ -380,8 +398,13 @@ def parse_expr(text: str) -> ParsedExpr: _reject_reserved_expr_token(text) # Scan for raw ``OVER(`` after blanking string literals so a quoted value - # (``status == 'OVER('``) isn't mistaken for window usage. - if _OVER_RE.search(_PY_STRING_LITERAL_RE.sub("", text)): + # (``status == 'OVER('``) isn't mistaken for window usage. Skip an ``OVER`` + # fused into a Unicode identifier (``℘OVER(x)``) the leading ``\b`` splits. + blanked = _PY_STRING_LITERAL_RE.sub("", text) + if any( + not _is_ident_adjacent(blanked, m.start() - 1) + for m in _OVER_RE.finditer(blanked) + ): raise IllegalWindowInFilterError( filter_expr=text, source="raw OVER(...) is not allowed in Mode-B DSL", @@ -444,9 +467,9 @@ def _normalize_sql_filter_operators(text: str) -> str: literals = _PY_STRING_LITERAL_RE.findall(text) result: List[str] = [] for i, part in enumerate(parts): - part = _SQL_NULL_RE.sub("None", part) + part = _sub_keyword_isolated(_SQL_NULL_RE, "None", part) for kw_re, kw in _SQL_KEYWORD_RES: - part = kw_re.sub(kw, part) + part = _sub_keyword_isolated(kw_re, kw, part) part = part.replace("<>", "!=") # SQL ``||`` → Python ``|`` (BitOr), desugared to ``concat`` in # ``_convert``; same precedence relative to comparisons. diff --git a/tests/test_aggregation_gating.py b/tests/test_aggregation_gating.py index 4f9188f4..f7f3d369 100644 --- a/tests/test_aggregation_gating.py +++ b/tests/test_aggregation_gating.py @@ -492,6 +492,29 @@ async def test_healed_alias_does_not_trigger_unknown(self) -> None: ) assert "COUNT(DISTINCT" in sql.upper() + @pytest.mark.parametrize( + "raw,sql_fn", [("countd", "COUNT(DISTINCT"), ("stddev", "STDDEV")], + ) + async def test_healed_alias_in_filter_reaches_having( + self, raw: str, sql_fn: str, + ) -> None: + # Binding-time healing must also fire on the filter path (DEV-1833 + # migrated filters onto the typed parser): an aliased aggregation in a + # HAVING predicate heals just like a measure. + query = SlayerQuery( + source_model="orders", + dimensions=["status"], + measures=[{"formula": "amount:sum", "name": "total"}], + filters=[f"amount:{raw} > 5"], + ) + sql = await _engine_generate( + query=query, model=_orders_with_status(), + extra_models=[_customers_model()], dialect="postgres", + ) + up = sql.upper() + assert "HAVING" in up + assert sql_fn in up + class TestDev1576RoundAbsGeneration: """DEV-1576 §2 — round()/abs() compile in a formula; Postgres needs a diff --git a/tests/test_dev1833_keyword_lexing.py b/tests/test_dev1833_keyword_lexing.py index 22a4f130..dd652103 100644 --- a/tests/test_dev1833_keyword_lexing.py +++ b/tests/test_dev1833_keyword_lexing.py @@ -12,7 +12,9 @@ import pytest +from slayer.core.errors import IllegalWindowInFilterError from slayer.engine.syntax import ( + AggCall, Arith, BoolOp, Cmp, @@ -282,3 +284,58 @@ def test_ascii_mixed_case_still_rewrites(self) -> None: def test_double_quoted_pattern_rejected(self) -> None: with pytest.raises(ValueError, match=r"Invalid Mode-B expression"): parse_filter_expr('col like "p%"') + + +# --------------------------------------------------------------------------- # +# Sibling keyword rewriters (NULL / IS / OVER / LIKE) hardened alongside CASE +# --------------------------------------------------------------------------- # +class TestSiblingRewriterUnicode: + """The NULL / IS-NOT-AND-OR-IN / OVER / LIKE rewrites share CASE's + identifier-adjacency guard, so a keyword spelling fused into a Unicode + identifier the ``\\b``/``\\w`` token class splits — ``Other_ID_Start`` ``℘`` + or a combining mark — is never rewritten out from under the reference. + """ + + def test_null_keyword_fused_into_other_id_start(self) -> None: + # ``℘NULL`` is one identifier; without the guard NULL → None rebinds it. + assert parse_filter_expr("℘NULL is None") == Cmp( + op="is", left=Ref(name="℘NULL"), right=Literal(value=None), + ) + + def test_null_keyword_fused_after_combining_mark(self) -> None: + # e + COMBINING ACUTE then NULL; NFKC composes the ref back to ``éNULL``. + assert parse_filter_expr("éNULL is None") == Cmp( + op="is", left=Ref(name="éNULL"), right=Literal(value=None), + ) + + def test_operator_keyword_fused_not_lowercased(self) -> None: + # ``℘IS`` is a name; case-normalisation must not fold it to ``℘is``. + assert parse_filter_expr("℘IS") == Ref(name="℘IS") + + def test_null_still_normalizes_when_standalone(self) -> None: + assert parse_filter_expr("amount is NULL") == Cmp( + op="is", left=Ref(name="amount"), right=Literal(value=None), + ) + + def test_over_fused_into_identifier_is_not_window(self) -> None: + # ``℘OVER(x)`` calls a keyword-named function, not raw OVER(); it defers + # to binding as a custom aggregation instead of being rejected outright. + node = parse_expr("℘OVER(x)") + assert isinstance(node, AggCall) + assert node.agg == "℘OVER" + assert node.source == Ref(name="x") + + def test_real_over_still_rejected(self) -> None: + with pytest.raises(IllegalWindowInFilterError): + parse_expr("amount OVER (partition_by)") + + def test_like_lhs_fused_never_corrupts(self) -> None: + # A fused LHS can't lex as a LIKE operand; rather than the old + # ``℘like(name, …)`` corruption it now errors cleanly. + for expr in ("℘name LIKE 'x%'", "éname LIKE 'x%'"): + with pytest.raises(ValueError): + parse_filter_expr(expr) + + def test_like_unit_leaves_fused_lhs_untouched(self) -> None: + for text in ("℘name LIKE 'x%'", "éname LIKE 'x%'"): + assert _rewrite_sql_like(text) == text From df96f5af4964d602f278431db7bc91b6350ad939 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 15:14:51 +0200 Subject: [PATCH 5/7] DEV-1833: guard dotted keyword components in the sibling rewrites too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex follow-up: the leading identifier guard checked only the char before the match, so a dotted path whose root the \w LHS can't lex (℘.name LIKE) still sliced the leaf into ℘.like(name, …), and a keyword-named dotted component (a.NULL → a.None, a.AND.b → a.and.b) was rewritten out from under the ref. Generalise the guard to _continues_ref — a `.` join separator OR Unicode identifier material — shared by _sub_keyword_isolated and the LIKE/OVER scans. --- slayer/engine/syntax.py | 34 ++++++++++++++++++---------- tests/test_dev1833_keyword_lexing.py | 14 ++++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/slayer/engine/syntax.py b/slayer/engine/syntax.py index dbc0c8a9..a413fbdf 100644 --- a/slayer/engine/syntax.py +++ b/slayer/engine/syntax.py @@ -149,10 +149,11 @@ def _rewrite_sql_like(text: str) -> str: spans = [(m.start(), m.end()) for m in _PY_STRING_LITERAL_RE.finditer(text)] def _sub(m: "re.Match[str]") -> str: - # Skip a match inside a string literal, or one whose LHS is fused into a - # Unicode identifier the ``\b``/``\w`` LHS token splits (``℘name``, - # decomposed ``éname``) — that name is a reference, not a LIKE operand. - if any(s <= m.start() < e for s, e in spans) or _is_ident_adjacent( + # Skip a match inside a string literal, or one whose LHS is only part of + # a larger reference the ``\b``/``\w`` LHS token can't lex whole — a + # ``.``-rooted dotted path (``℘.name``) or a Unicode-fused prefix + # (``℘name``, decomposed ``éname``); that name is not a LIKE operand. + if any(s <= m.start() < e for s, e in spans) or _continues_ref( text, m.start() - 1 ): return m.group(0) @@ -189,14 +190,22 @@ def _is_ident_adjacent(text: str, pos: int) -> bool: return 0 <= pos < len(text) and ("a" + text[pos]).isidentifier() +def _continues_ref(text: str, pos: int) -> bool: + """Whether ``text[pos]`` extends a reference token past a keyword span — a + ``.`` join separator, or identifier material the ``\\w`` class misses. A + keyword touching such a char is a name component (``a.NULL``, ``℘case``), + not a keyword.""" + return _is_ident_adjacent(text, pos) or (0 <= pos < len(text) and text[pos] == ".") + + def _sub_keyword_isolated(pattern: "re.Pattern[str]", repl: str, text: str) -> str: - """``pattern.sub(repl, text)`` but leaving a keyword match fused into a - Unicode identifier untouched — combining marks or ``Other_ID_Start`` symbols - (``℘``) on either side that ``\\b``/``\\w`` cannot see would otherwise - rewrite ``℘NULL`` → ``℘None`` or ``℘AND`` → ``℘and``, silently rebinding the - reference (mirrors ``_case_keyword``'s adjacency guard).""" + """``pattern.sub(repl, text)`` but leaving a keyword match that is really a + reference component untouched — a dotted leaf/root (``a.NULL`` → keeps + ``NULL``) or one fused to ``Other_ID_Start`` / combining marks the ``\\b`` + boundary splits (``℘NULL`` → ``℘None``, ``℘AND`` → ``℘and``). Both would + silently rebind the reference (mirrors ``_case_keyword``'s guard).""" def _guard(m: "re.Match[str]") -> str: - if _is_ident_adjacent(text, m.start() - 1) or _is_ident_adjacent(text, m.end()): + if _continues_ref(text, m.start() - 1) or _continues_ref(text, m.end()): return m.group(0) return repl return pattern.sub(_guard, text) @@ -399,10 +408,11 @@ def parse_expr(text: str) -> ParsedExpr: # Scan for raw ``OVER(`` after blanking string literals so a quoted value # (``status == 'OVER('``) isn't mistaken for window usage. Skip an ``OVER`` - # fused into a Unicode identifier (``℘OVER(x)``) the leading ``\b`` splits. + # that only continues a reference — a Unicode-fused (``℘OVER(``) or dotted + # (``a.OVER(``) name the leading ``\b`` splits. blanked = _PY_STRING_LITERAL_RE.sub("", text) if any( - not _is_ident_adjacent(blanked, m.start() - 1) + not _continues_ref(blanked, m.start() - 1) for m in _OVER_RE.finditer(blanked) ): raise IllegalWindowInFilterError( diff --git a/tests/test_dev1833_keyword_lexing.py b/tests/test_dev1833_keyword_lexing.py index dd652103..c3c7a364 100644 --- a/tests/test_dev1833_keyword_lexing.py +++ b/tests/test_dev1833_keyword_lexing.py @@ -339,3 +339,17 @@ def test_like_lhs_fused_never_corrupts(self) -> None: def test_like_unit_leaves_fused_lhs_untouched(self) -> None: for text in ("℘name LIKE 'x%'", "éname LIKE 'x%'"): assert _rewrite_sql_like(text) == text + + def test_keyword_named_dotted_leaf_preserved(self) -> None: + # ``a.NULL`` is a dotted ref; the NULL sub must not rewrite the leaf to + # ``a.None`` (parity with the CASE lexer keeping ``customers.end``). + assert parse_filter_expr("a.NULL is None") == Cmp( + op="is", left=DottedRef(parts=("a", "NULL")), right=Literal(value=None), + ) + + def test_dotted_root_other_id_start_not_a_like_operand(self) -> None: + # ``\u2118.name`` roots at an Other_ID_Start component the ``\w`` LHS can't + # lex whole; LIKE must not slice the leaf off into ``\u2118.like(name, \u2026)``. + assert _rewrite_sql_like("\u2118.name LIKE 'x%'") == "\u2118.name LIKE 'x%'" + with pytest.raises(ValueError): + parse_filter_expr("\u2118.name LIKE 'x%'") From eb0e7e176f446d422298b927fa5d40b9e40df6a4 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 15:59:52 +0200 Subject: [PATCH 6/7] DEV-1833: pass _continues_ref args by keyword (CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Project convention — keyword arguments for calls with more than one parameter. --- slayer/engine/syntax.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/slayer/engine/syntax.py b/slayer/engine/syntax.py index a413fbdf..b9caf4dc 100644 --- a/slayer/engine/syntax.py +++ b/slayer/engine/syntax.py @@ -154,7 +154,7 @@ def _sub(m: "re.Match[str]") -> str: # ``.``-rooted dotted path (``℘.name``) or a Unicode-fused prefix # (``℘name``, decomposed ``éname``); that name is not a LIKE operand. if any(s <= m.start() < e for s, e in spans) or _continues_ref( - text, m.start() - 1 + text=text, pos=m.start() - 1 ): return m.group(0) lhs, neg, pat = m.group(1), m.group(2), m.group(3) @@ -195,7 +195,9 @@ def _continues_ref(text: str, pos: int) -> bool: ``.`` join separator, or identifier material the ``\\w`` class misses. A keyword touching such a char is a name component (``a.NULL``, ``℘case``), not a keyword.""" - return _is_ident_adjacent(text, pos) or (0 <= pos < len(text) and text[pos] == ".") + return _is_ident_adjacent(text=text, pos=pos) or ( + 0 <= pos < len(text) and text[pos] == "." + ) def _sub_keyword_isolated(pattern: "re.Pattern[str]", repl: str, text: str) -> str: @@ -205,7 +207,9 @@ def _sub_keyword_isolated(pattern: "re.Pattern[str]", repl: str, text: str) -> s boundary splits (``℘NULL`` → ``℘None``, ``℘AND`` → ``℘and``). Both would silently rebind the reference (mirrors ``_case_keyword``'s guard).""" def _guard(m: "re.Match[str]") -> str: - if _continues_ref(text, m.start() - 1) or _continues_ref(text, m.end()): + if _continues_ref(text=text, pos=m.start() - 1) or _continues_ref( + text=text, pos=m.end() + ): return m.group(0) return repl return pattern.sub(_guard, text) @@ -412,7 +416,7 @@ def parse_expr(text: str) -> ParsedExpr: # (``a.OVER(``) name the leading ``\b`` splits. blanked = _PY_STRING_LITERAL_RE.sub("", text) if any( - not _continues_ref(blanked, m.start() - 1) + not _continues_ref(text=blanked, pos=m.start() - 1) for m in _OVER_RE.finditer(blanked) ): raise IllegalWindowInFilterError( From de88b802e7d5e6f349b24526ac907026db506c09 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 4 Sep 2026 16:51:15 +0200 Subject: [PATCH 7/7] =?UTF-8?q?DEV-1833:=20archive=20OpenSpec=20change=20?= =?UTF-8?q?=E2=80=94=20merge=20expression-keywords=20delta=20into=20specs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../.openspec.yaml | 0 .../design.md | 0 .../proposal.md | 0 .../specs/queries/expression-keywords/spec.md | 0 .../tasks.md | 0 .../specs/queries/expression-keywords/spec.md | 132 ++++++++++++++++++ 6 files changed, 132 insertions(+) rename openspec/changes/{dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode => archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode}/.openspec.yaml (100%) rename openspec/changes/{dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode => archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode}/design.md (100%) rename openspec/changes/{dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode => archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode}/proposal.md (100%) rename openspec/changes/{dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode => archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode}/specs/queries/expression-keywords/spec.md (100%) rename openspec/changes/{dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode => archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode}/tasks.md (100%) create mode 100644 openspec/specs/queries/expression-keywords/spec.md diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml b/openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml similarity index 100% rename from openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml rename to openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/.openspec.yaml diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md b/openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md similarity index 100% rename from openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md rename to openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/design.md diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md b/openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md similarity index 100% rename from openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md rename to openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/proposal.md diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md b/openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md similarity index 100% rename from openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md rename to openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/specs/queries/expression-keywords/spec.md diff --git a/openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md b/openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md similarity index 100% rename from openspec/changes/dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md rename to openspec/changes/archive/2026-09-04-dev-1833-harden-mode-b-keyword-lexing-case-like-for-unicode/tasks.md diff --git a/openspec/specs/queries/expression-keywords/spec.md b/openspec/specs/queries/expression-keywords/spec.md new file mode 100644 index 00000000..6bbf96d1 --- /dev/null +++ b/openspec/specs/queries/expression-keywords/spec.md @@ -0,0 +1,132 @@ +# queries/expression-keywords Specification + +## Purpose +SQL keyword affordances inside Mode-B expressions — `CASE WHEN` lowering to `iif` and +`LIKE`/`NOT LIKE` rewriting to the `like()` scalar — and the identifier-safety rules +guaranteeing that identifiers named after, containing, or qualified by SQL keywords are +never captured by that recognition. + +## Requirements + +### Requirement: CASE WHEN lowering + +A Mode-B expression SHALL accept SQL `CASE … END` conditionals — searched +(`CASE WHEN cond THEN val … [ELSE val] END`) and simple +(`CASE operand WHEN val THEN val … [ELSE val] END`) — lowering them to nested +`iif(cond, then, otherwise)` calls with `None` as the default otherwise. WHEN +conditions SHALL accept SQL operator spellings (`=`, `<>`, `AND`/`OR`/`NOT`, +`IS [NOT] NULL`, `[NOT] IN`, `[NOT] LIKE`) in every expression position, including +measures. A `CASE` that is recognized as a conditional (a `WHEN` follows it) but is +malformed MUST raise a specific malformed-CASE error naming the defect. + +#### Scenario: searched CASE lowers to iif + +- WHEN `CASE WHEN amount > 100 THEN 'big' ELSE 'small' END` is parsed as a Mode-B expression +- THEN it parses as `iif(amount > 100, 'big', 'small')` — a scalar call, usable wherever a scalar expression is legal + +#### Scenario: simple CASE compares the operand per branch + +- WHEN `CASE status WHEN 'a' THEN 1 WHEN 'b' THEN 2 END` is parsed +- THEN it parses as `iif(status == 'a', 1, iif(status == 'b', 2, None))` + +#### Scenario: nested CASE in THEN and ELSE values + +- WHEN a THEN or ELSE value itself contains a complete `CASE … END` +- THEN the nested conditional is lowered recursively and the enclosing branches are unaffected + +#### Scenario: SQL operator spellings inside WHEN conditions + +- WHEN `CASE WHEN region = 'EU' AND amount IS NOT NULL THEN 1 ELSE 0 END` appears in a measure formula +- THEN the WHEN condition is normalized (`==`, `and`, `is not None`) and the expression parses + +#### Scenario: recognized-but-malformed CASE still errors specifically + +- WHEN `CASE WHEN a THEN 1` (missing END) or `CASE WHEN a 1 END` (missing THEN) is parsed +- THEN a malformed-CASE error is raised naming the missing keyword, not a generic syntax error + +### Requirement: keyword-named identifiers are never captured + +An identifier that is merely named after, prefixed by, containing, or qualified by a SQL +keyword SHALL parse as an ordinary reference in every Mode-B expression position. `CASE` +SHALL be treated as a conditional only when a `WHEN` token follows it at parenthesis +depth 0 before any other structural keyword (`THEN`/`ELSE`/`END`/`CASE`), an unmatched +closing parenthesis, or end of input. Keyword recognition MUST be ASCII-exact: tokens +whose uppercase form only coincides with a keyword via Unicode case folding, and tokens +adjacent to identifier-forming characters outside the regex word class (combining marks, +`Other_ID_Start` symbols), are ordinary identifiers. Dotted references qualify their +leaf regardless of whitespace around the dots. + +#### Scenario: bare keyword-named column + +- WHEN `case` (or `case + 1`, or `iif(case, 1, 2)`) is parsed as a Mode-B expression +- THEN `case` resolves as an ordinary column reference and no CASE lowering occurs + +#### Scenario: qualified keyword-named column + +- WHEN `customers.case` is parsed, with or without whitespace around the dot (`customers . case`) +- THEN it parses as a dotted reference to the `case` column of `customers` + +#### Scenario: Unicode identifiers containing keywords + +- WHEN `écase`, `变量`, decomposed `écase`, or `℘case` is parsed +- THEN each parses as a single ordinary identifier; no fragment of it is read as a keyword + +#### Scenario: Unicode case-fold spoofs are not keywords + +- WHEN an identifier like `caſe` (uppercases to `CASE`) appears in an expression +- THEN it is an ordinary identifier, not a CASE keyword + +#### Scenario: keyword-named identifier alongside a real CASE + +- WHEN `case + CASE WHEN x THEN 1 END` or `CASE WHEN case THEN 1 WHEN other THEN 2 END` is parsed +- THEN the bare `case` references stay identifiers while the real `CASE WHEN … END` lowers to `iif` + +#### Scenario: keyword-named dotted reference inside CASE branch values + +- WHEN `CASE WHEN a THEN customers.end ELSE 0 END` is parsed +- THEN the THEN value is the complete `customers.end` reference and the conditional lowers correctly + +#### Scenario: bare CASE with no WHEN is not a conditional + +- WHEN `CASE` appears with no depth-0 `WHEN` following (e.g. the whole expression is `case` or `case_total * 2`) +- THEN no CASE lowering is attempted; the text parses (or fails) exactly as if `case` were any other identifier + +#### Scenario: keyword-named simple-CASE operand requires parentheses + +- WHEN `CASE case WHEN 1 THEN 2 END` is parsed +- THEN the ambiguous bare keyword-named operand raises an error (never silent corruption), and the parenthesized form `CASE (case) WHEN 1 THEN 2 END` parses correctly with `case` as the operand reference + +### Requirement: LIKE operator rewriting + +A Mode-B filter SHALL accept `lhs [NOT] LIKE 'pattern'` — LHS a bare or dotted +identifier or single scalar call, pattern a single-quoted string literal with +backslash-escape support — rewriting it to the `like(lhs, pattern)` scalar (negated: +`not like(...)`). The keyword match SHALL be ASCII-exact (any ASCII casing; never via +Unicode case folding) and SHALL never apply inside a string literal. A double-quoted +pattern is NOT rewritten (in SQL sources double quotes denote identifiers), so it fails +loudly rather than silently changing meaning. + +#### Scenario: basic LIKE and NOT LIKE + +- WHEN `name LIKE 'a%'` / `name NOT LIKE 'a%'` / `lower(customers.email) like '%@x.io'` appear in a filter +- THEN each rewrites to the corresponding `like(...)` / `not like(...)` scalar call + +#### Scenario: escaped quote inside the pattern + +- WHEN `col LIKE 'It\'s%'` appears in a filter +- THEN the full pattern including the escaped quote is preserved as the second argument + +#### Scenario: LIKE inside a string literal is untouched + +- WHEN a filter contains ` like ` only inside a string literal, e.g. `note == "we like 'cats'"` +- THEN the literal is preserved byte-for-byte and no rewrite occurs + +#### Scenario: case-fold keyword spoofs are not LIKE + +- WHEN a filter contains `x lıke 'p%'` or `x liKe 'p%'` (dotless ı / KELVIN SIGN fold to `like`) +- THEN no rewrite occurs (the token is an ordinary identifier), while ASCII `x LiKe 'p%'` still rewrites + +#### Scenario: double-quoted pattern is rejected loudly + +- WHEN `col like "p%"` appears in a filter +- THEN parsing fails with an invalid-expression error rather than rewriting to a string match