From 32d375b857081e813de953bc4c17c1f4a901b043 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Fri, 19 Jun 2026 22:17:25 +0200 Subject: [PATCH 1/5] DEV-1576: aggregation-name alias healing, round()/abs() formula functions, clearer unknown-aggregation error Three SlayerQuery ergonomics fixes: 1. Aggregation-name alias / casing normalization. New normalize_aggregation_name (slayer/core/enums.py) lowercases and maps countd/countDistinct/countdistinct -> count_distinct, stddev -> stddev_samp, var/variance -> var_samp; only adopted when the result is a builtin, else returned unchanged. Wired at the colon-syntax chokepoint (_preprocess_agg_refs) so it heals both formulas and filters. Unknown names pass through. 2. round(expr[, ndigits]) and abs(expr) accepted as top-level formula functions (SCALAR_FUNCTIONS), recognised case-insensitively, routed through the arithmetic-passthrough path with parse-time arity validation. Postgres lacks round(double precision, integer), so the new target-keyed PostgresDialect.rewrite_target_ast wraps a 2-arg ROUND's first arg in a numeric CAST; SQLite/DuckDB round DOUBLE natively. 3. enrichment now splits "Unknown aggregation ''. Did you mean ''? Known: [...]" (checked before the PK/whitelist/type gates) from the existing "not applicable to column" wording for known-but-disallowed aggregations. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/slayer-query.md | 4 +- docs/concepts/formulas.md | 12 + slayer/core/enums.py | 35 +++ slayer/core/formula.py | 79 +++++- slayer/engine/enrichment.py | 19 ++ slayer/sql/dialects/base.py | 17 ++ slayer/sql/dialects/postgres.py | 33 +++ slayer/sql/generator.py | 8 +- tests/dialects/test_base.py | 23 ++ tests/dialects/test_postgres.py | 55 ++++ tests/integration/test_integration.py | 84 ++++++ .../integration/test_integration_postgres.py | 63 +++++ tests/test_aggregation_gating.py | 256 ++++++++++++++++- tests/test_dev1576_heals.py | 263 ++++++++++++++++++ 14 files changed, 944 insertions(+), 7 deletions(-) create mode 100644 tests/test_dev1576_heals.py diff --git a/.claude/skills/slayer-query.md b/.claude/skills/slayer-query.md index 511a6e08..e42cbb70 100644 --- a/.claude/skills/slayer-query.md +++ b/.claude/skills/slayer-query.md @@ -40,7 +40,9 @@ Each entry in `measures` is either a bare formula string or a `{"formula": ..., "last(revenue:sum)", "time_shift(revenue:sum, -1, 'year')", "lag(revenue:sum, 1)", - "rank(revenue:sum)" + "rank(revenue:sum)", + "round(revenue:sum, 2)", + "abs(revenue:sum - cost:sum)" ] ``` diff --git a/docs/concepts/formulas.md b/docs/concepts/formulas.md index ea5cc84f..40e7c626 100644 --- a/docs/concepts/formulas.md +++ b/docs/concepts/formulas.md @@ -277,6 +277,18 @@ Inside `Column.sql`, `ModelMeasure.formula`, or any `Aggregation.formula`, you c | `exp(x)` | 1 | `e^x` | | `sqrt(x)` | 1 | Square root | | `pow(x, n)` / `power(x, n)` | 2 | `x^n`. Both spellings are accepted (sqlglot may emit either depending on origin dialect). | +| `round(x[, ndigits])` | 1–2 | Round to `ndigits` decimal places (default 0). `ndigits` must be an integer literal. | +| `abs(x)` | 1 | Absolute value. | + +Unlike the other scalar functions above — which pass through only when embedded in a larger `Column.sql` or arithmetic expression — `round` and `abs` are also valid as the **top-level** form of a query measure or `ModelMeasure.formula`: + +```python +{"formula": "round(revenue:sum, 2)"} # round an aggregate +{"formula": "abs(revenue:sum - cost:sum)"} # absolute difference +{"formula": "round(revenue:sum / *:count, 2)"} +``` + +On Postgres, 2-argument `round` over a floating-point value is automatically cast to `numeric` so it executes (Postgres has no `round(double precision, integer)` overload). SQLite and DuckDB round `DOUBLE` natively. These are native on Postgres / DuckDB / MySQL / ClickHouse. SQLite doesn't have most of them in the standard build, so SLayer registers Python implementations on every connection (see `slayer/sql/dialects/sqlite.py`). NULL inputs always return NULL. Math-domain errors (`ln(0)`, `sqrt(-1)`, `pow(0, -1)`) propagate as `sqlite3.OperationalError` — matching Postgres's strict semantics rather than SQLite ≥3.35's silent-NULL built-in `log()`. diff --git a/slayer/core/enums.py b/slayer/core/enums.py index 87f50110..d6ee8652 100644 --- a/slayer/core/enums.py +++ b/slayer/core/enums.py @@ -149,6 +149,41 @@ class JoinType(StrEnum): "corr", "covar_samp", "covar_pop", }) +# DEV-1576: unambiguous aggregation-name aliases that LLM agents routinely +# emit. ``normalize_aggregation_name`` lowercases the incoming token and maps +# it through this table; the result is only adopted when it lands in +# ``BUILTIN_AGGREGATIONS``. ``stddev``/``var``/``variance`` map to the *sample* +# variants, matching Postgres' bare ``stddev``/``variance`` defaults. +AGGREGATION_ALIASES: dict[str, str] = { + "countd": "count_distinct", + "countdistinct": "count_distinct", # also matches "countDistinct" once lowercased + "stddev": "stddev_samp", + "var": "var_samp", + "variance": "var_samp", +} + + +def normalize_aggregation_name(name: str) -> str: + """Coerce an aggregation token to its canonical SLayer spelling. + + Lowercases the token and applies :data:`AGGREGATION_ALIASES`. The + normalized form is only adopted when it is a real built-in aggregation; + otherwise the **original** string is returned unchanged so genuinely + unknown names still raise downstream (and custom aggregation names keep + their exact casing). Examples:: + + "countd" -> "count_distinct" + "countDistinct" -> "count_distinct" + "stddev" -> "stddev_samp" + "SUM" -> "sum" + "myCustomAgg" -> "myCustomAgg" (unchanged — not a builtin) + "bogus" -> "bogus" (unchanged — still raises later) + """ + lowered = name.lower() + candidate = AGGREGATION_ALIASES.get(lowered, lowered) + return candidate if candidate in BUILTIN_AGGREGATIONS else name + + # Built-in aggregation SQL formulas (for aggregations that use a template). # {value} = measure's SQL expression; {param_name} = parameter values. # Note: percentile is dialect-dependent (no single template works on diff --git a/slayer/core/formula.py b/slayer/core/formula.py index 7c36fc32..4dc73279 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -21,7 +21,7 @@ from pydantic import BaseModel, Field -from slayer.core.enums import BUILTIN_AGGREGATIONS +from slayer.core.enums import BUILTIN_AGGREGATIONS, normalize_aggregation_name from slayer.core.refs import ( AGG_REF_RE as _AGG_REF_RE, IDENT_OR_PATH_RE as _IDENT_OR_PATH_RE, @@ -70,7 +70,15 @@ "concat", }) -CallCategory = Literal["transform", "hygiene", "like_internal", "unknown"] +# DEV-1576: scalar post-aggregation functions accepted as top-level formula +# calls. They are recognised case-insensitively (agents overwhelmingly emit +# ``ROUND``) and pass through to the emitted SQL via the arithmetic-passthrough +# machinery (``_parse_mixed_arithmetic``). The list is intentionally tiny — the +# existing inside-arithmetic passthrough already accepts arbitrary functions, +# but top-level calls stay gated so genuinely unknown functions keep raising. +SCALAR_FUNCTIONS = frozenset({"round", "abs"}) + +CallCategory = Literal["transform", "scalar", "hygiene", "like_internal", "unknown"] _LIKE_INTERNAL_NAMES = frozenset({"__like__", "__notlike__"}) @@ -84,6 +92,8 @@ def _classify_call_name(name: str) -> CallCategory: """ if name in ALL_TRANSFORMS: return "transform" + if name.lower() in SCALAR_FUNCTIONS: + return "scalar" if name in STRING_HYGIENE_OPS: return "hygiene" if name in _LIKE_INTERNAL_NAMES: @@ -91,6 +101,56 @@ def _classify_call_name(name: str) -> CallCategory: return "unknown" +def _validate_scalar_call(node: ast.Call, original: str) -> None: + """Validate arity for a top-level ``round`` / ``abs`` call (DEV-1576). + + ``abs`` takes exactly one argument; ``round`` takes one or two + (``expression[, ndigits]``) where ``ndigits`` must be an integer literal + (negative allowed). Neither accepts keyword arguments. + """ + name = node.func.id.lower() + if node.keywords: + raise ValueError( + f"'{name}' does not accept keyword arguments. Formula: {original!r}" + ) + if name == "abs": + if len(node.args) != 1: + raise ValueError( + f"'abs' requires exactly one argument (the expression). " + f"Formula: {original!r}" + ) + return + # round + if len(node.args) not in (1, 2): + raise ValueError( + f"'round' accepts 1 or 2 arguments (expression[, ndigits]). " + f"Formula: {original!r}" + ) + if len(node.args) == 2 and not _is_integer_literal(node.args[1]): + raise ValueError( + f"'round' ndigits (2nd argument) must be an integer literal. " + f"Formula: {original!r}" + ) + + +def _is_integer_literal(node: ast.AST) -> bool: + """True for an integer constant or a negated integer constant (``-2``). + + Booleans are rejected — ``True``/``False`` are ``int`` subclasses but are + not meaningful ndigits values. + """ + if isinstance(node, ast.Constant): + return isinstance(node.value, int) and not isinstance(node.value, bool) + if ( + isinstance(node, ast.UnaryOp) + and isinstance(node.op, ast.USub) + and isinstance(node.operand, ast.Constant) + ): + v = node.operand.value + return isinstance(v, int) and not isinstance(v, bool) + return False + + class AggregatedMeasureRef(BaseModel): """A measure reference with explicit aggregation (new colon syntax). @@ -354,7 +414,10 @@ def _preprocess_agg_refs(formula: str) -> tuple[str, Dict[str, AggregatedMeasure def _replace(match: re.Match) -> str: measure_name = match.group(1) - agg_name = match.group(2) + # DEV-1576: heal aggregation-name aliases / casing at the single colon- + # syntax chokepoint (shared by parse_formula and parse_filter). Unknown + # names pass through unchanged so the §3 enrichment error still fires. + agg_name = normalize_aggregation_name(match.group(2)) args_str = match.group(3) agg_args: list[str] = [] @@ -554,7 +617,15 @@ def _parse_node( raise ValueError(f"Unsupported function call in formula: {original!r}") func_name = node.func.id - if _classify_call_name(func_name) != "transform": + category = _classify_call_name(func_name) + if category == "scalar": + # DEV-1576: round()/abs() are passthrough scalar functions. Route + # through the arithmetic-passthrough path, which preserves the call + # in the emitted SQL and registers any inner aggregated refs (and + # extracts any nested transform as a sub-transform). + _validate_scalar_call(node, original) + return _parse_mixed_arithmetic(node, original, agg_refs) + if category != "transform": raise ValueError( f"Unknown transform function '{func_name}'. " f"Supported: {', '.join(sorted(ALL_TRANSFORMS))}" diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index f098deb2..673bc92c 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -8,6 +8,7 @@ transformation step in the query pipeline. """ +import difflib import re from typing import Any, Dict, List, Mapping, Optional, Set, Tuple @@ -409,6 +410,24 @@ async def _ensure_aggregated_measure( raise ValueError( f"Column '{measure_name}' not found in model '{model.name}'" ) + # DEV-1576 §3: distinguish "unknown aggregation name" from "known + # but not allowed for this column type". The name check runs BEFORE + # the PK / whitelist / type gates so an unknown name never gets + # mislabelled as a type restriction (a perfectly aggregatable DOUBLE + # column with a misspelled agg should say "Unknown aggregation", + # not "not applicable to DOUBLE column"). + known_aggregations = BUILTIN_AGGREGATIONS | { + a.name for a in model.aggregations + } + if aggregation_name not in known_aggregations: + suggestion = difflib.get_close_matches( + aggregation_name, sorted(known_aggregations), n=1 + ) + hint = f" Did you mean '{suggestion[0]}'?" if suggestion else "" + raise ValueError( + f"Unknown aggregation '{aggregation_name}'.{hint} " + f"Known: {sorted(known_aggregations)}." + ) # Apply aggregation eligibility gates per the v2 contract: # 1. Primary-key columns are always restricted to count/count_distinct # (regardless of type or any explicit whitelist). diff --git a/slayer/sql/dialects/base.py b/slayer/sql/dialects/base.py index 1abcb14a..596ec5be 100644 --- a/slayer/sql/dialects/base.py +++ b/slayer/sql/dialects/base.py @@ -345,6 +345,23 @@ def rewrite_parsed_ast(self, tree: exp.Expression) -> exp.Expression: the function-call form (DEV-1331).""" return tree + def rewrite_target_ast(self, tree: exp.Expression) -> exp.Expression: + """Default: identity. Target-keyed AST rewrite (DEV-1576). + + Applied in ``SQLGenerator._parse`` using the generator's **target** + dialect (``self._dialect``), independent of the parse dialect. This is + the place for output-shaping a dialect needs that the input-side + ``rewrite_parsed_ast`` cannot do: formula/measure expressions are + canonically parsed as Postgres regardless of target, so a + ``rewrite_parsed_ast`` override would fire for every backend. + + ``PostgresDialect`` overrides this to wrap the first argument of a + 2-arg ``ROUND`` in a numeric ``CAST`` (Postgres has no + ``round(double precision, integer)`` — only ``round(numeric, int)``). + SQLite / DuckDB round ``DOUBLE`` natively, so they keep the identity. + """ + return tree + def emit_outer_wrap( self, *, diff --git a/slayer/sql/dialects/postgres.py b/slayer/sql/dialects/postgres.py index eeb68bee..bf851afd 100644 --- a/slayer/sql/dialects/postgres.py +++ b/slayer/sql/dialects/postgres.py @@ -8,9 +8,37 @@ from typing import Optional +from sqlglot import exp + from slayer.sql.dialects.base import SqlDialect +def _cast_round_arg_to_numeric(node: exp.Expression) -> exp.Expression: + """Wrap the first arg of a 2-arg ``ROUND`` in ``CAST(... AS DECIMAL)``. + + Postgres has no ``round(double precision, integer)`` overload — only + ``round(numeric, integer)`` — so a 2-arg round over a DOUBLE expression + fails without an explicit numeric cast. 1-arg round (``round(double)``) + is fine and left alone. Idempotent: skips when the arg is already cast to + a numeric/decimal type. + """ + if not isinstance(node, exp.Round): + return node + decimals = node.args.get("decimals") + if decimals is None: # 1-arg round — no overload problem. + return node + inner = node.this + if isinstance(inner, exp.Cast): + cast_to = inner.to + if isinstance(cast_to, exp.DataType) and cast_to.this in ( + exp.DataType.Type.DECIMAL, + exp.DataType.Type.BIGDECIMAL, + ): + return node # already numeric-cast — idempotent. + node.set("this", exp.cast(inner.copy(), "DECIMAL")) + return node + + class PostgresDialect(SqlDialect): sqlglot_name: str = "postgres" ds_type_aliases: frozenset[str] = frozenset({"postgres", "postgresql"}) @@ -18,3 +46,8 @@ class PostgresDialect(SqlDialect): explain_postfix: str = "" log10_native: bool = True log2_native: bool = True + + def rewrite_target_ast(self, tree: exp.Expression) -> exp.Expression: + """DEV-1576: numeric-cast the first arg of every 2-arg ROUND so + ``round(double precision, int)`` becomes ``round(numeric, int)``.""" + return tree.transform(_cast_round_arg_to_numeric) diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index ad0e1a89..12d73c88 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -433,7 +433,13 @@ def _parse(self, sql: str, *, dialect: Optional[str] = None) -> exp.Expression: # Log-alias rewrite is multi-dialect; the per-base allowlist check # lives inside ``_rewrite_log_aliases`` so unsupported dialects # (oracle; tsql for log2) keep the canonical 2-arg LOG form. - return tree.transform(self._rewrite_log_aliases) + tree = tree.transform(self._rewrite_log_aliases) + # DEV-1576: target-keyed AST rewrite. Keyed to the generator's TARGET + # dialect (``self._dialect``), NOT the parse dialect — expressions are + # canonically parsed as Postgres regardless of target, so this is the + # only place a Postgres-only ROUND cast can fire without corrupting + # SQLite / DuckDB output. + return self._dialect.rewrite_target_ast(tree) def _parse_predicate(self, sql: str, *, dialect: Optional[str] = None) -> exp.Expression: """Parse a bare WHERE/HAVING predicate expression (DEV-1378). diff --git a/tests/dialects/test_base.py b/tests/dialects/test_base.py index 345bc75a..e8035c61 100644 --- a/tests/dialects/test_base.py +++ b/tests/dialects/test_base.py @@ -435,3 +435,26 @@ def test_default_emit_outer_wrap_uses_sqlglot_name_not_dialect_attr() -> None: limit=None, offset_arg=None, ) + + +# --------------------------------------------------------------------------- +# DEV-1576: rewrite_target_ast default is identity (only Postgres overrides). +# --------------------------------------------------------------------------- + + +def test_base_rewrite_target_ast_is_identity_for_round() -> None: + from slayer.sql.dialects.base import SqlDialect + d = SqlDialect() + tree = sqlglot.parse_one("ROUND(x, 2)", dialect="postgres") + before = tree.sql(dialect="postgres") + after = d.rewrite_target_ast(tree).sql(dialect="postgres") + assert before == after + + +def test_duckdb_and_sqlite_rewrite_target_ast_leave_round_uncast() -> None: + from slayer.sql.dialects.duckdb import DuckdbDialect + from slayer.sql.dialects.sqlite import SqliteDialect + for d in (DuckdbDialect(), SqliteDialect()): + tree = sqlglot.parse_one("ROUND(x, 2)", dialect="postgres") + out = d.rewrite_target_ast(tree).sql(dialect=d.sqlglot_name).upper() + assert "CAST(" not in out diff --git a/tests/dialects/test_postgres.py b/tests/dialects/test_postgres.py index 94f8405b..fd9e9755 100644 --- a/tests/dialects/test_postgres.py +++ b/tests/dialects/test_postgres.py @@ -194,3 +194,58 @@ def test_postgres_register_udfs_is_noop() -> None: def test_postgres_build_explain_sql() -> None: assert PostgresDialect().build_explain_sql("SELECT 1") == "EXPLAIN ANALYZE SELECT 1" + + +# --------------------------------------------------------------------------- +# DEV-1576: rewrite_target_ast — numeric cast for 2-arg ROUND +# +# Postgres has no ``round(double precision, integer)`` — only +# ``round(numeric, integer)``. The target-keyed hook wraps the first arg of a +# 2-arg ROUND in a numeric CAST so 2-arg round over a DOUBLE measure executes. +# 1-arg round and abs are untouched. +# --------------------------------------------------------------------------- + + +def test_postgres_rewrite_target_ast_casts_two_arg_round() -> None: + d = PostgresDialect() + tree = sqlglot.parse_one("ROUND(x, 2)", dialect="postgres") + out = d.rewrite_target_ast(tree).sql(dialect="postgres").upper() + assert "ROUND(CAST(" in out + assert "AS DECIMAL" in out or "AS NUMERIC" in out + + +def test_postgres_rewrite_target_ast_one_arg_round_unchanged() -> None: + d = PostgresDialect() + tree = sqlglot.parse_one("ROUND(x)", dialect="postgres") + out = d.rewrite_target_ast(tree).sql(dialect="postgres").upper() + assert "CAST(" not in out + + +def test_postgres_rewrite_target_ast_abs_unchanged() -> None: + d = PostgresDialect() + tree = sqlglot.parse_one("ABS(x)", dialect="postgres") + out = d.rewrite_target_ast(tree).sql(dialect="postgres").upper() + assert "CAST(" not in out + + +def test_postgres_rewrite_target_ast_idempotent() -> None: + d = PostgresDialect() + once = d.rewrite_target_ast(sqlglot.parse_one("ROUND(x, 2)", dialect="postgres")) + twice = d.rewrite_target_ast(once) + assert once.sql(dialect="postgres") == twice.sql(dialect="postgres") + + +def test_postgres_rewrite_target_ast_casts_round_over_expression() -> None: + d = PostgresDialect() + tree = sqlglot.parse_one("ROUND(SUM(amount) / 7, 2)", dialect="postgres") + out = d.rewrite_target_ast(tree).sql(dialect="postgres").upper() + assert "ROUND(CAST(" in out + + +def test_postgres_rewrite_target_ast_preserves_json_extract() -> None: + # The new hook must only touch ROUND — leave everything else alone. + d = PostgresDialect() + tree = sqlglot.parse_one("SELECT json_extract(j, '$.k') FROM t", dialect="postgres") + before = tree.sql(dialect="postgres") + after = d.rewrite_target_ast(tree).sql(dialect="postgres") + assert before == after diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index b911dcbd..80f47d3d 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -3704,3 +3704,87 @@ async def test_dev1539_having_multiterm_measure_emits_outer_parens(composite_sco assert "SUM(" in having.upper() and "/" in having and "NULLIF" in having.upper(), ( f"Expected HAVING body to combine SUM/NULLIF via `/`; got:\n{having}" ) + + +# --------------------------------------------------------------------------- +# DEV-1576 — round()/abs() execution + aggregation-alias healing (SQLite) +# --------------------------------------------------------------------------- + + +async def test_round_two_args_executes(integration_env): + """round(expr, ndigits) compiles and rounds the value.""" + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="round(total_amount:sum / 7.0, 2)", name="r")], + ) + response = await engine.execute(query) + assert response.data[0]["orders.r"] == pytest.approx(107.14) + + +async def test_round_zero_ndigits_executes(integration_env): + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="round(total_amount:sum / 7.0, 0)", name="r")], + ) + response = await engine.execute(query) + assert response.data[0]["orders.r"] == pytest.approx(107.0) + + +async def test_abs_executes(integration_env): + """abs(expr) compiles and returns the magnitude.""" + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="abs(total_amount:sum - 1000.0)", name="a")], + ) + response = await engine.execute(query) + assert response.data[0]["orders.a"] == pytest.approx(250.0) + + +async def test_count_distinct_alias_executes(integration_env): + """``status:countd`` heals to count_distinct and executes.""" + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="status:countd")], + ) + response = await engine.execute(query) + # completed / pending / cancelled → 3 distinct statuses. + assert response.data[0]["orders.status_count_distinct"] == 3 + + +async def test_stddev_alias_maps_to_sample_variant(integration_env): + """``amount:stddev`` heals to stddev_samp (sample, not population).""" + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="amount:stddev")], + ) + response = await engine.execute(query) + # Sample stddev of [100,200,50,75,300,25] = sqrt(55000/5) ≈ 104.88 + # (population variant would be sqrt(55000/6) ≈ 95.74). + assert response.data[0]["orders.amount_stddev_samp"] == pytest.approx(104.88, abs=0.1) + + +async def test_round_over_bare_measure_raises(integration_env): + """A bare (non-aggregated) measure inside round is rejected, not leaked.""" + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="round(amount, 2)", name="r")], + ) + with pytest.raises(ValueError, match="Bare measure name"): + await engine.execute(query) + + +async def test_abs_over_bare_measure_raises(integration_env): + """Same guard for abs — a bare measure inside abs is rejected.""" + engine = integration_env + query = SlayerQuery( + source_model="orders", + measures=[ModelMeasure(formula="abs(amount)", name="a")], + ) + with pytest.raises(ValueError, match="Bare measure name"): + await engine.execute(query) diff --git a/tests/integration/test_integration_postgres.py b/tests/integration/test_integration_postgres.py index 50944c54..744e2d48 100644 --- a/tests/integration/test_integration_postgres.py +++ b/tests/integration/test_integration_postgres.py @@ -129,6 +129,13 @@ def _pg_env_storage(postgresql_proc, tmp_path_factory): Column(name="total", sql="amount", type=DataType.DOUBLE), Column(name="avg_amount", sql="amount", type=DataType.DOUBLE), + # DEV-1576: a genuine DOUBLE PRECISION expression column. + # ``amount`` is NUMERIC, so ``round(amount:sum, 2)`` works on + # Postgres natively; the non-bare DOUBLE-typed sql gets wrapped + # in CAST(... AS DOUBLE PRECISION), so SUM yields double and + # ``round(double precision, int)`` would fail without the + # PostgresDialect numeric-cast rewrite. + Column(name="amount_d", sql="amount * 1.0", type=DataType.DOUBLE), ], ) customers_model = SlayerModel( @@ -1182,3 +1189,59 @@ async def test_integration_postgres_cross_model_derived_columnsql( assert response.row_count == 2 assert float(response.data[0]["a_tbl.ratio_using_derived"]) == pytest.approx(2.0) assert float(response.data[1]["a_tbl.ratio_using_derived"]) == pytest.approx(2.0) + + +@pytest.mark.integration +class TestPostgresDev1576Heals: + """DEV-1576 — round()/abs() execution (incl. the double-precision cast + path) and aggregation-alias healing against a real Postgres.""" + + async def test_round_two_args_over_double_executes( + self, pg_env: SlayerQueryEngine, + ) -> None: + # SUM(amount_d) = 875.0 (double precision); /8.0 = 109.375 → round 2. + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "round(amount_d:sum / 8.0, 2)", "name": "r"}], + ) + result = await pg_env.execute(query=query) + assert float(result.data[0]["orders.r"]) == pytest.approx(109.38) + + async def test_round_one_arg_over_double_executes( + self, pg_env: SlayerQueryEngine, + ) -> None: + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "round(amount_d:sum / 8.0)", "name": "r"}], + ) + result = await pg_env.execute(query=query) + assert float(result.data[0]["orders.r"]) == pytest.approx(109.0) + + async def test_abs_over_double_executes(self, pg_env: SlayerQueryEngine) -> None: + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "abs(amount_d:sum - 1000.0)", "name": "a"}], + ) + result = await pg_env.execute(query=query) + assert float(result.data[0]["orders.a"]) == pytest.approx(125.0) + + async def test_count_distinct_alias_executes( + self, pg_env: SlayerQueryEngine, + ) -> None: + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "status:countd"}], + ) + result = await pg_env.execute(query=query) + assert result.data[0]["orders.status_count_distinct"] == 3 + + async def test_stddev_alias_executes(self, pg_env: SlayerQueryEngine) -> None: + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "total:stddev"}], + ) + result = await pg_env.execute(query=query) + # Sample stddev of [100,200,50,150,75,300] (mean 145.83) ≈ 92.76. + assert float(result.data[0]["orders.total_stddev_samp"]) == pytest.approx( + 92.76, abs=0.1 + ) diff --git a/tests/test_aggregation_gating.py b/tests/test_aggregation_gating.py index 391d2aaa..507e5e22 100644 --- a/tests/test_aggregation_gating.py +++ b/tests/test_aggregation_gating.py @@ -53,6 +53,7 @@ async def _generate_sql( orders: SlayerModel, customers: SlayerModel, measures: list, + dialect: str = "postgres", ) -> str: """Run a real engine + SQL generator and return the SQL string.""" with tempfile.TemporaryDirectory() as tmp: @@ -62,7 +63,25 @@ async def _generate_sql( engine = SlayerQueryEngine(storage=storage) query = SlayerQuery(source_model="orders", measures=measures) enriched = await engine._enrich(query=query, model=orders, named_queries={}) - return SQLGenerator(dialect="postgres").generate(enriched=enriched) + return SQLGenerator(dialect=dialect).generate(enriched=enriched) + + +def _orders_with_status() -> SlayerModel: + """Orders model carrying a local TEXT column (``status``) and a numeric + ``rating`` column — for the DEV-1576 §3 error-split tests.""" + return SlayerModel( + name="orders", + sql_table="public.orders", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.DOUBLE), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column(name="status", sql="status", type=DataType.TEXT), + Column(name="rating", sql="rating", type=DataType.DOUBLE), + ], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) class TestCrossModelGating: @@ -348,3 +367,238 @@ async def test_cross_model_column_filter_applied(self) -> None: ) assert "CASE" in sql.upper() assert "completed" in sql.lower() + + +class TestDev1576UnknownVsDisallowed: + """DEV-1576 §3 — split 'unknown aggregation name' from 'not allowed for + this column type'. Local (non-cross-model) path only. + """ + + async def test_unknown_name_raises_unknown_aggregation(self) -> None: + with pytest.raises(ValueError, match=r"Unknown aggregation 'bogus'"): + await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "amount:bogus", "name": "result"}], + ) + + async def test_unknown_name_lists_known_aggregations(self) -> None: + with pytest.raises(ValueError, match=r"Known:"): + await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "amount:bogus", "name": "result"}], + ) + + async def test_unknown_name_suggests_close_match(self) -> None: + # ``stdev`` is a near-miss for stddev_samp/stddev_pop. It is NOT an + # alias (so §1 leaves it), and enrichment should offer a suggestion. + with pytest.raises(ValueError, match=r"Did you mean 'stddev_(samp|pop)'"): + await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "amount:stdev", "name": "result"}], + ) + + async def test_type_disallowed_keeps_not_applicable_wording(self) -> None: + # ``sum`` is a KNOWN aggregation but not eligible for a TEXT column — + # keep the existing 'not applicable to column' message. + with pytest.raises(ValueError, match=r"not applicable to TEXT column"): + await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "status:sum", "name": "result"}], + ) + + async def test_type_disallowed_is_not_reported_as_unknown(self) -> None: + with pytest.raises(ValueError) as exc: + await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "status:sum", "name": "result"}], + ) + assert "Unknown aggregation" not in str(exc.value) + + async def test_star_unknown_keeps_star_message(self) -> None: + # ``*`` only supports count — keep the dedicated star message rather + # than the generic 'Unknown aggregation' wording. + with pytest.raises(ValueError, match=r"\*:count"): + await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "*:bogus", "name": "result"}], + ) + + async def test_custom_agg_known_but_disallowed_keeps_not_allowed(self) -> None: + # A custom aggregation known model-wide but absent from a column's + # allowed_aggregations whitelist → 'not allowed for column', NOT + # 'Unknown aggregation'. + orders = SlayerModel( + name="orders", + sql_table="public.orders", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.DOUBLE), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column( + name="rating", + sql="rating", + type=DataType.DOUBLE, + allowed_aggregations=["avg"], + ), + ], + aggregations=[Aggregation(name="myagg", formula="SUM({value}) * 2")], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + with pytest.raises(ValueError) as exc: + await _generate_sql( + orders=orders, + customers=_customers_model(), + measures=[{"formula": "rating:myagg", "name": "result"}], + ) + assert "Unknown aggregation" not in str(exc.value) + assert "not allowed" in str(exc.value) + + async def test_custom_agg_allowed_when_no_whitelist(self) -> None: + # Guard: the unknown-name check must not break a legitimate custom + # aggregation on a column with no whitelist. + orders = SlayerModel( + name="orders", + sql_table="public.orders", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.DOUBLE), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + ], + aggregations=[Aggregation(name="myagg", formula="SUM({value}) * 2")], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + sql = await _generate_sql( + orders=orders, + customers=_customers_model(), + measures=[{"formula": "amount:myagg", "name": "result"}], + ) + assert "SUM" in sql.upper() + + async def test_healed_alias_does_not_trigger_unknown(self) -> None: + # ``countd`` heals to count_distinct in §1 — it must reach SQL, not + # the §3 'Unknown aggregation' error. + sql = await _generate_sql( + orders=_orders_with_status(), + customers=_customers_model(), + measures=[{"formula": "amount:countd", "name": "result"}], + ) + assert "COUNT(DISTINCT" in sql.upper() + + +class TestDev1576RoundAbsGeneration: + """DEV-1576 §2 — round()/abs() compile in a formula; Postgres needs a + numeric CAST for 2-arg round (round(double precision, int) doesn't + exist), while SQLite/DuckDB round natively over DOUBLE. + """ + + async def test_round_two_args_postgres_casts_to_numeric(self) -> None: + sql = await _generate_sql( + orders=_orders_model(), + customers=_customers_model(), + measures=[{"formula": "round(amount:sum, 2)", "name": "r"}], + dialect="postgres", + ) + up = sql.upper() + assert "ROUND(CAST(" in up + assert "AS DECIMAL" in up or "AS NUMERIC" in up + + @pytest.mark.parametrize("dialect", ["sqlite", "duckdb"]) + async def test_round_two_args_non_postgres_no_cast(self, dialect: str) -> None: + sql = await _generate_sql( + orders=_orders_model(), + customers=_customers_model(), + measures=[{"formula": "round(amount:sum, 2)", "name": "r"}], + dialect=dialect, + ) + up = sql.upper() + assert "ROUND(" in up + # No numeric cast injected — these backends round DOUBLE natively. + assert "ROUND(CAST(" not in up + + async def test_round_one_arg_postgres_no_cast(self) -> None: + # 1-arg round(double precision) exists on Postgres — no cast needed. + sql = await _generate_sql( + orders=_orders_model(), + customers=_customers_model(), + measures=[{"formula": "round(amount:sum)", "name": "r"}], + dialect="postgres", + ) + up = sql.upper() + assert "ROUND(" in up + assert "ROUND(CAST(" not in up + + @pytest.mark.parametrize("dialect", ["postgres", "sqlite", "duckdb"]) + async def test_abs_generates_unchanged(self, dialect: str) -> None: + sql = await _generate_sql( + orders=_orders_model(), + customers=_customers_model(), + measures=[{"formula": "abs(amount:sum)", "name": "a"}], + dialect=dialect, + ) + assert "ABS(" in sql.upper() + + +class TestDev1576UnknownNamePrecedence: + """DEV-1576 §3 — the unknown-name guard fires BEFORE the PK / whitelist / + type gates, and the suggestion list reflects model-wide custom aggs.""" + + def _orders_pk_whitelist_custom(self) -> SlayerModel: + return SlayerModel( + name="orders", + sql_table="public.orders", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.DOUBLE), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + Column(name="status", sql="status", type=DataType.TEXT), + Column( + name="rating", + sql="rating", + type=DataType.DOUBLE, + allowed_aggregations=["avg"], + ), + ], + aggregations=[Aggregation(name="myagg", formula="SUM({value}) * 2")], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + + @pytest.mark.parametrize("column", ["rating", "id", "status"]) + async def test_unknown_name_beats_pk_whitelist_and_type_gates( + self, column: str, + ) -> None: + # rating has a whitelist, id is PK, status is TEXT — for an unknown + # aggregation name all three must surface 'Unknown aggregation', not + # the whitelist / PK / type-applicability message. + with pytest.raises(ValueError, match=r"Unknown aggregation 'bogus'"): + await _generate_sql( + orders=self._orders_pk_whitelist_custom(), + customers=_customers_model(), + measures=[{"formula": f"{column}:bogus", "name": "result"}], + ) + + async def test_known_list_includes_custom_aggregations(self) -> None: + with pytest.raises(ValueError) as exc: + await _generate_sql( + orders=self._orders_pk_whitelist_custom(), + customers=_customers_model(), + measures=[{"formula": "amount:bogus", "name": "result"}], + ) + assert "myagg" in str(exc.value) + + async def test_no_did_you_mean_for_poor_match(self) -> None: + with pytest.raises(ValueError) as exc: + await _generate_sql( + orders=self._orders_pk_whitelist_custom(), + customers=_customers_model(), + measures=[{"formula": "amount:zzzzz", "name": "result"}], + ) + assert "Did you mean" not in str(exc.value) diff --git a/tests/test_dev1576_heals.py b/tests/test_dev1576_heals.py new file mode 100644 index 00000000..4d7c9b5a --- /dev/null +++ b/tests/test_dev1576_heals.py @@ -0,0 +1,263 @@ +"""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``). +2. ``round()`` / ``abs()`` as top-level formula functions (parse into a + ``MixedArithmeticField`` passthrough; arity validation). + +The §3 error-message split and end-to-end execution live in +``tests/test_aggregation_gating.py`` and the integration suites respectively. +""" + +import pytest + +from slayer.core.enums import ( + AGGREGATION_ALIASES, + BUILTIN_AGGREGATIONS, + normalize_aggregation_name, +) +from slayer.core.formula import ( + AggregatedMeasureRef, + MixedArithmeticField, + parse_filter, + parse_formula, +) + + +# --------------------------------------------------------------------------- +# §1 — normalize_aggregation_name +# --------------------------------------------------------------------------- + + +class TestNormalizeAggregationName: + @pytest.mark.parametrize( + "raw,expected", + [ + ("countd", "count_distinct"), + ("countdistinct", "count_distinct"), + ("countDistinct", "count_distinct"), + ("COUNTD", "count_distinct"), + ("stddev", "stddev_samp"), + ("STDDEV", "stddev_samp"), + ("var", "var_samp"), + ("variance", "var_samp"), + ("VARIANCE", "var_samp"), + ], + ) + def test_known_aliases_heal(self, raw: str, expected: str) -> None: + assert normalize_aggregation_name(raw) == expected + + @pytest.mark.parametrize( + "raw,expected", + [ + ("SUM", "sum"), + ("Count", "count"), + ("Count_Distinct", "count_distinct"), + ("WEIGHTED_AVG", "weighted_avg"), + ], + ) + def test_casing_of_canonical_names_heals(self, raw: str, expected: str) -> None: + assert normalize_aggregation_name(raw) == expected + + def test_canonical_names_pass_through_unchanged(self) -> None: + for agg in BUILTIN_AGGREGATIONS: + assert normalize_aggregation_name(agg) == agg + + @pytest.mark.parametrize("raw", ["bogus", "stdev", "sum_over", "count_if", "group_concat"]) + def test_unknown_names_returned_unchanged(self, raw: str) -> None: + # An unknown token must be returned verbatim so the §3 enrichment + # error still fires (and so a genuinely unknown name is never + # silently swallowed). + assert normalize_aggregation_name(raw) == raw + + def test_custom_agg_name_preserved_with_original_casing(self) -> None: + # A custom aggregation name (not a builtin / not an alias) keeps its + # exact spelling — normalization never lowercases names it cannot + # resolve to the builtin vocabulary. + assert normalize_aggregation_name("myCustomAgg") == "myCustomAgg" + + def test_unknown_uppercase_name_preserved(self) -> None: + # An unresolvable name keeps its original casing (no silent lowercasing + # of names that don't map to the vocabulary). + assert normalize_aggregation_name("BOGUS") == "BOGUS" + + def test_alias_table_only_targets_builtins(self) -> None: + # Every alias must resolve to a real builtin aggregation. + for target in AGGREGATION_ALIASES.values(): + assert target in BUILTIN_AGGREGATIONS + + +# --------------------------------------------------------------------------- +# §1 — colon-syntax healing through parse_formula / parse_filter +# --------------------------------------------------------------------------- + + +class TestColonSyntaxAliasHealing: + @pytest.mark.parametrize( + "raw_agg,expected", + [ + ("countd", "count_distinct"), + ("countDistinct", "count_distinct"), + ("countdistinct", "count_distinct"), + ("stddev", "stddev_samp"), + ("var", "var_samp"), + ("variance", "var_samp"), + ("SUM", "sum"), + ], + ) + def test_formula_colon_alias_heals(self, raw_agg: str, expected: str) -> None: + result = parse_formula(f"revenue:{raw_agg}") + assert isinstance(result, AggregatedMeasureRef) + assert result.aggregation_name == expected + assert result.measure_name == "revenue" + + def test_star_count_alias_unaffected(self) -> None: + result = parse_formula("*:count") + assert isinstance(result, AggregatedMeasureRef) + assert result.aggregation_name == "count" + + def test_unknown_agg_in_formula_left_for_enrichment(self) -> None: + # parse_formula does not validate the vocabulary — an unknown agg + # parses through (heal leaves it unchanged); enrichment raises later. + result = parse_formula("revenue:bogus") + 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) + + +# --------------------------------------------------------------------------- +# §2 — round() / abs() as top-level formula functions +# --------------------------------------------------------------------------- + + +def _agg_names(field: MixedArithmeticField) -> set[str]: + return {ref.aggregation_name for ref in field.agg_refs.values()} + + +class TestScalarFunctionsParse: + def test_round_two_args(self) -> None: + result = parse_formula("round(revenue:sum, 2)") + assert isinstance(result, MixedArithmeticField) + assert "round" in result.sql.lower() + assert "2" in result.sql + assert _agg_names(result) == {"sum"} + assert result.sub_transforms == [] + + def test_round_one_arg(self) -> None: + result = parse_formula("round(revenue:sum)") + assert isinstance(result, MixedArithmeticField) + assert "round" in result.sql.lower() + assert _agg_names(result) == {"sum"} + + def test_abs_one_arg(self) -> None: + result = parse_formula("abs(revenue:sum)") + assert isinstance(result, MixedArithmeticField) + assert "abs" in result.sql.lower() + assert _agg_names(result) == {"sum"} + + def test_abs_over_arithmetic(self) -> None: + result = parse_formula("abs(revenue:sum - cost:sum)") + assert isinstance(result, MixedArithmeticField) + assert "abs" in result.sql.lower() + assert _agg_names(result) == {"sum"} + + def test_round_over_arithmetic(self) -> None: + result = parse_formula("round(revenue:sum / *:count, 2)") + assert isinstance(result, MixedArithmeticField) + assert "round" in result.sql.lower() + assert _agg_names(result) == {"sum", "count"} + + def test_round_negative_ndigits(self) -> None: + # round(x, -2) is valid SQL (round to hundreds); negative integer + # literals must be accepted. + result = parse_formula("round(revenue:sum, -2)") + assert isinstance(result, MixedArithmeticField) + assert "round" in result.sql.lower() + + def test_round_wraps_nested_transform(self) -> None: + # A transform nested inside round is still extracted as a sub-transform. + result = parse_formula("round(cumsum(revenue:sum), 2)") + assert isinstance(result, MixedArithmeticField) + assert result.sub_transforms, "nested cumsum should become a sub-transform" + assert result.sub_transforms[0][1].transform == "cumsum" + + @pytest.mark.parametrize("raw", ["ROUND(revenue:sum, 2)", "Round(revenue:sum, 2)"]) + def test_round_case_insensitive(self, raw: str) -> None: + result = parse_formula(raw) + assert isinstance(result, MixedArithmeticField) + assert _agg_names(result) == {"sum"} + + @pytest.mark.parametrize("raw", ["ABS(revenue:sum)", "Abs(revenue:sum)"]) + def test_abs_case_insensitive(self, raw: str) -> None: + result = parse_formula(raw) + assert isinstance(result, MixedArithmeticField) + assert _agg_names(result) == {"sum"} + + +class TestScalarFunctionArity: + def test_round_no_args_raises(self) -> None: + with pytest.raises(ValueError, match="round"): + parse_formula("round()") + + def test_round_three_args_raises(self) -> None: + with pytest.raises(ValueError, match="round"): + parse_formula("round(revenue:sum, 2, 3)") + + def test_round_non_integer_ndigits_raises(self) -> None: + with pytest.raises(ValueError, match="round"): + parse_formula("round(revenue:sum, 2.5)") + + def test_round_non_literal_ndigits_raises(self) -> None: + # ndigits must be a literal, not another measure / expression. + with pytest.raises(ValueError, match="round"): + parse_formula("round(revenue:sum, x)") + + def test_round_kwarg_raises(self) -> None: + with pytest.raises(ValueError, match="round"): + parse_formula("round(revenue:sum, ndigits=2)") + + def test_abs_two_args_raises(self) -> None: + with pytest.raises(ValueError, match="abs"): + parse_formula("abs(revenue:sum, cost:sum)") + + def test_abs_no_args_raises(self) -> None: + with pytest.raises(ValueError, match="abs"): + parse_formula("abs()") + + def test_abs_kwarg_raises(self) -> None: + with pytest.raises(ValueError, match="abs"): + parse_formula("abs(revenue:sum, foo=1)") + + +class TestScalarAllowlistIsExclusive: + @pytest.mark.parametrize("fn", ["ceil", "floor", "sqrt", "ln", "nullif", "coalesce"]) + def test_other_functions_still_reject_at_top_level(self, fn: str) -> None: + # Only round/abs are promoted to top-level scalar functions. Every + # other function call at the top level keeps raising "Unknown + # transform" (the inside-arithmetic passthrough is unchanged and + # tested separately). + with pytest.raises(ValueError, match="Unknown transform"): + parse_formula(f"{fn}(revenue:sum)") + + def test_inside_arithmetic_passthrough_unchanged(self) -> None: + # Pre-existing behaviour: a non-transform call inside arithmetic + # passes through and registers the inner agg ref. + result = parse_formula("*:count / nullif(revenue:max, 0)") + assert isinstance(result, MixedArithmeticField) + assert "nullif" in result.sql.lower() + + @pytest.mark.parametrize("raw", ["countd(revenue)", "countDistinct(revenue)"]) + def test_funcstyle_aggregation_alias_out_of_scope(self, raw: str) -> None: + # §1 heals colon syntax only. Function-style alias calls are NOT + # rewritten — they fall through to the parser as unknown calls. + with pytest.raises(ValueError, match="Unknown transform"): + parse_formula(raw) From 5c20893ee76de13cfad9e921366cf6cc98952775 Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Sat, 20 Jun 2026 11:55:23 +0200 Subject: [PATCH 2/5] DEV-1576 PR #198 review round 1: predicate-path round cast, kwargs, test dedup - generator._parse_predicate now also applies rewrite_target_ast so a 2-arg ROUND over a DOUBLE in a Mode-A SQL filter gets the Postgres numeric cast, matching _parse (CodeRabbit). - enrichment: difflib.get_close_matches called with keyword args per the repo convention (CodeRabbit nitpick). - tests: merge the two scalar-function case-insensitivity tests into one parametrized test (Sonar python:S4144 duplicate body); add a predicate-path round-cast test. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/engine/enrichment.py | 4 +++- slayer/sql/generator.py | 5 ++++- tests/dialects/test_postgres.py | 9 +++++++++ tests/test_dev1576_heals.py | 18 ++++++++++-------- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index 673bc92c..cc0e1759 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -421,7 +421,9 @@ async def _ensure_aggregated_measure( } if aggregation_name not in known_aggregations: suggestion = difflib.get_close_matches( - aggregation_name, sorted(known_aggregations), n=1 + word=aggregation_name, + possibilities=sorted(known_aggregations), + n=1, ) hint = f" Did you mean '{suggestion[0]}'?" if suggestion else "" raise ValueError( diff --git a/slayer/sql/generator.py b/slayer/sql/generator.py index 12d73c88..c3d058ca 100644 --- a/slayer/sql/generator.py +++ b/slayer/sql/generator.py @@ -467,7 +467,10 @@ def _parse_predicate(self, sql: str, *, dialect: Optional[str] = None) -> exp.Ex f"Could not extract WHERE predicate from {sql!r} (dialect={d!r})" ) tree = active.rewrite_parsed_ast(where.this) - return tree.transform(self._rewrite_log_aliases) + tree = tree.transform(self._rewrite_log_aliases) + # DEV-1576: same target-keyed rewrite as ``_parse`` — a 2-arg ROUND + # over a DOUBLE in a Mode-A SQL filter needs the Postgres numeric cast. + return self._dialect.rewrite_target_ast(tree) def generate( self, diff --git a/tests/dialects/test_postgres.py b/tests/dialects/test_postgres.py index fd9e9755..ba2136f3 100644 --- a/tests/dialects/test_postgres.py +++ b/tests/dialects/test_postgres.py @@ -242,6 +242,15 @@ def test_postgres_rewrite_target_ast_casts_round_over_expression() -> None: assert "ROUND(CAST(" in out +def test_postgres_parse_predicate_casts_two_arg_round() -> None: + # DEV-1576: a 2-arg ROUND in a Mode-A SQL filter (parsed via + # _parse_predicate) must get the same numeric cast as projections. + from slayer.sql.generator import SQLGenerator + gen = SQLGenerator(dialect="postgres") + out = gen._parse_predicate("round(amount, 2) > 5").sql(dialect="postgres").upper() + assert "ROUND(CAST(" in out + + def test_postgres_rewrite_target_ast_preserves_json_extract() -> None: # The new hook must only touch ROUND — leave everything else alone. d = PostgresDialect() diff --git a/tests/test_dev1576_heals.py b/tests/test_dev1576_heals.py index 4d7c9b5a..97656dc5 100644 --- a/tests/test_dev1576_heals.py +++ b/tests/test_dev1576_heals.py @@ -190,14 +190,16 @@ def test_round_wraps_nested_transform(self) -> None: assert result.sub_transforms, "nested cumsum should become a sub-transform" assert result.sub_transforms[0][1].transform == "cumsum" - @pytest.mark.parametrize("raw", ["ROUND(revenue:sum, 2)", "Round(revenue:sum, 2)"]) - def test_round_case_insensitive(self, raw: str) -> None: - result = parse_formula(raw) - assert isinstance(result, MixedArithmeticField) - assert _agg_names(result) == {"sum"} - - @pytest.mark.parametrize("raw", ["ABS(revenue:sum)", "Abs(revenue:sum)"]) - def test_abs_case_insensitive(self, raw: str) -> None: + @pytest.mark.parametrize( + "raw", + [ + "ROUND(revenue:sum, 2)", + "Round(revenue:sum, 2)", + "ABS(revenue:sum)", + "Abs(revenue:sum)", + ], + ) + def test_scalar_functions_case_insensitive(self, raw: str) -> None: result = parse_formula(raw) assert isinstance(result, MixedArithmeticField) assert _agg_names(result) == {"sum"} From 83e9db2f4b91c6ec13b734c54ac760950f16cdef Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Sat, 20 Jun 2026 12:31:19 +0200 Subject: [PATCH 3/5] DEV-1576 PR #198 review round 2: custom aggregation takes precedence over alias healing (Codex) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit normalize_aggregation_name ran at parse time with no model context, so a model custom aggregation named like an alias key (countd/countdistinct/stddev/ var/variance) or a builtin case-variant was silently rewritten to the builtin before enrichment looked it up — shadowing the custom aggregation. _preprocess_agg_refs now takes the host model's custom-aggregation names and skips healing for an exact custom-name match, so a custom aggregation always resolves. Threaded extra_agg_names through the three call sites (parse_formula, parse_filter, enrichment filter preprocessing). Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/core/formula.py | 22 +++++++++++++---- slayer/engine/enrichment.py | 4 +++- tests/test_aggregation_gating.py | 41 ++++++++++++++++++++++++++++++++ tests/test_dev1576_heals.py | 17 +++++++++++++ 4 files changed, 79 insertions(+), 5 deletions(-) diff --git a/slayer/core/formula.py b/slayer/core/formula.py index 4dc73279..cbbaac89 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -404,10 +404,17 @@ def _split_args(s: str) -> list[str]: # --------------------------------------------------------------------------- -def _preprocess_agg_refs(formula: str) -> tuple[str, Dict[str, AggregatedMeasureRef]]: +def _preprocess_agg_refs( + formula: str, + custom_agg_names: frozenset[str] = frozenset(), +) -> tuple[str, Dict[str, AggregatedMeasureRef]]: """Replace colon-syntax aggregated measure refs with placeholder identifiers. Returns (preprocessed_formula, {placeholder: AggregatedMeasureRef}). + + ``custom_agg_names`` are the host model's custom-aggregation names; a colon + token matching one of them exactly is left un-normalized so a custom + aggregation always takes precedence over alias/casing healing (DEV-1576). """ refs: Dict[str, AggregatedMeasureRef] = {} counter = [0] @@ -417,7 +424,14 @@ def _replace(match: re.Match) -> str: # DEV-1576: heal aggregation-name aliases / casing at the single colon- # syntax chokepoint (shared by parse_formula and parse_filter). Unknown # names pass through unchanged so the §3 enrichment error still fires. - agg_name = normalize_aggregation_name(match.group(2)) + # A model-level custom aggregation named like an alias key / builtin + # casing wins — skip healing for an exact custom-name match so it still + # resolves at enrichment. + raw_agg = match.group(2) + agg_name = ( + raw_agg if raw_agg in custom_agg_names + else normalize_aggregation_name(raw_agg) + ) args_str = match.group(3) agg_args: list[str] = [] @@ -573,7 +587,7 @@ def parse_formula( # Rewrite function-style aggregations (e.g., sum(revenue) → revenue:sum) formula = _rewrite_funcstyle_aggregations(formula, extra_agg_names) # Preprocess colon syntax into ast-parseable placeholders - processed, agg_refs = _preprocess_agg_refs(formula) + processed, agg_refs = _preprocess_agg_refs(formula, extra_agg_names or frozenset()) try: tree = ast.parse(processed, mode="eval") @@ -1106,7 +1120,7 @@ def parse_filter( # 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(processed) + processed, agg_refs = _preprocess_agg_refs(processed, extra_agg_names or frozenset()) agg_canonical = { ph: canonical_agg_name( measure_name=ref.measure_name, diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index cc0e1759..4043f8a0 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -2793,7 +2793,9 @@ def extract_filter_transforms( preprocessed = _preprocess_concat(preprocessed) preprocessed = _preprocess_like(preprocessed) # Preprocess colon syntax (e.g., "order_total:sum") into ast-safe placeholders - preprocessed, agg_refs = _preprocess_agg_refs(preprocessed) + preprocessed, agg_refs = _preprocess_agg_refs( + preprocessed, extra_agg_names or frozenset() + ) # Build reverse map: placeholder → original colon form _agg_reverse = { ph: ( diff --git a/tests/test_aggregation_gating.py b/tests/test_aggregation_gating.py index 507e5e22..a0d09c04 100644 --- a/tests/test_aggregation_gating.py +++ b/tests/test_aggregation_gating.py @@ -602,3 +602,44 @@ async def test_no_did_you_mean_for_poor_match(self) -> None: measures=[{"formula": "amount:zzzzz", "name": "result"}], ) assert "Did you mean" not in str(exc.value) + + +class TestDev1576CustomAggPrecedence: + """DEV-1576 (Codex): a model custom aggregation named like an alias key + (countd/stddev/var/...) or a builtin casing must take precedence over + alias healing — the heal must not silently shadow it.""" + + async def test_custom_agg_named_like_alias_takes_precedence(self) -> None: + orders = SlayerModel( + name="orders", + sql_table="public.orders", + data_source="test", + columns=[ + Column(name="id", sql="id", type=DataType.DOUBLE, primary_key=True), + Column(name="customer_id", sql="customer_id", type=DataType.DOUBLE), + Column(name="amount", sql="amount", type=DataType.DOUBLE), + ], + aggregations=[ + Aggregation(name="countd", formula="COUNT(DISTINCT {value}) + 1"), + ], + joins=[ModelJoin(target_model="customers", join_pairs=[["customer_id", "id"]])], + ) + sql = await _generate_sql( + orders=orders, + customers=_customers_model(), + measures=[{"formula": "amount:countd", "name": "result"}], + ) + # The custom formula (… + 1) must win, not the healed builtin + # count_distinct (plain COUNT(DISTINCT …)). + assert "+ 1" in sql + + async def test_alias_heals_when_no_colliding_custom_agg(self) -> None: + # Same query on a model WITHOUT a colliding custom agg still heals to + # the builtin count_distinct. + sql = await _generate_sql( + orders=_orders_model(), + customers=_customers_model(), + measures=[{"formula": "amount:countd", "name": "result"}], + ) + assert "COUNT(DISTINCT" in sql.upper() + assert "+ 1" not in sql diff --git a/tests/test_dev1576_heals.py b/tests/test_dev1576_heals.py index 97656dc5..6a23fa89 100644 --- a/tests/test_dev1576_heals.py +++ b/tests/test_dev1576_heals.py @@ -133,6 +133,23 @@ 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. + result = parse_formula("revenue:countd", extra_agg_names=frozenset({"countd"})) + assert isinstance(result, AggregatedMeasureRef) + assert result.aggregation_name == "countd" + + def test_alias_still_heals_when_custom_name_differs(self) -> None: + result = parse_formula( + "revenue:countd", extra_agg_names=frozenset({"some_other_agg"}) + ) + 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 From 04cb062fa30f3fad2f70ef9b4489b6c543ea1eae Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Sat, 20 Jun 2026 13:54:51 +0200 Subject: [PATCH 4/5] DEV-1576 PR #198 review round 3: reduce _preprocess_agg_refs complexity, kwargs, document scope - Sonar python:S3776: extract the colon-aggregation arglist parsing into a module-level _split_agg_arglist helper, dropping _preprocess_agg_refs cognitive complexity back under the threshold. - CodeRabbit: pass custom_agg_names= as a keyword at all three _preprocess_agg_refs call sites (parse_formula, parse_filter, enrichment). - Document (Codex follow-on) that the custom-aggregation precedence check is parse-time model-set-scoped, not per-ref target-scoped: the residual only ever yields an explicit "Unknown aggregation" error, never a silent wrong aggregation; per-ref scoping would desync parse-time filter/ORDER-BY aliases. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/core/formula.py | 60 +++++++++++++++++++++++++------------ slayer/engine/enrichment.py | 2 +- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/slayer/core/formula.py b/slayer/core/formula.py index cbbaac89..d04dd507 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -404,6 +404,30 @@ def _split_args(s: str) -> list[str]: # --------------------------------------------------------------------------- +def _split_agg_arglist(args_str: Optional[str]) -> tuple[list[str], dict[str, str]]: + """Parse a colon-aggregation ``(...)`` arglist into (positional, keyword). + + ``args_str`` is the raw ``(...)`` capture (with parens) or ``None``. + ``price:weighted_avg(weight=quantity)`` → ``([], {"weight": "quantity"})``; + ``revenue:last(ordered_at)`` → ``(["ordered_at"], {})``. + """ + agg_args: list[str] = [] + agg_kwargs: dict[str, str] = {} + if not args_str: + return agg_args, agg_kwargs + inner = args_str[1:-1].strip() + if not inner: + return agg_args, agg_kwargs + for part in inner.split(","): + part = part.strip() + if "=" in part: + key, val = part.split("=", 1) + agg_kwargs[key.strip()] = val.strip() + else: + agg_args.append(part) + return agg_args, agg_kwargs + + def _preprocess_agg_refs( formula: str, custom_agg_names: frozenset[str] = frozenset(), @@ -412,9 +436,16 @@ def _preprocess_agg_refs( Returns (preprocessed_formula, {placeholder: AggregatedMeasureRef}). - ``custom_agg_names`` are the host model's custom-aggregation names; a colon - token matching one of them exactly is left un-normalized so a custom - aggregation always takes precedence over alias/casing healing (DEV-1576). + ``custom_agg_names`` are the reachable custom-aggregation names (the source + model plus its join graph); a colon token matching one exactly is left + un-normalized so a custom aggregation takes precedence over alias/casing + healing (DEV-1576). This is parse-time and model-set-scoped, not per-ref + target-scoped: in a mixed-model graph a custom name on one model suppresses + healing of that same token on a different model. The failure mode is benign + — the un-healed token simply resolves (custom agg) or raises an explicit + "Unknown aggregation" at enrichment; it never silently picks the wrong + aggregation. Per-ref scoping would require deferring the heal past parse, + which would desync the canonical filter/ORDER-BY aliases built here. """ refs: Dict[str, AggregatedMeasureRef] = {} counter = [0] @@ -432,20 +463,7 @@ def _replace(match: re.Match) -> str: raw_agg if raw_agg in custom_agg_names else normalize_aggregation_name(raw_agg) ) - args_str = match.group(3) - - agg_args: list[str] = [] - agg_kwargs: dict[str, str] = {} - if args_str: - inner = args_str[1:-1].strip() - if inner: - for part in inner.split(","): - part = part.strip() - if "=" in part: - key, val = part.split("=", 1) - agg_kwargs[key.strip()] = val.strip() - else: - agg_args.append(part) + agg_args, agg_kwargs = _split_agg_arglist(match.group(3)) placeholder = f"__agg{counter[0]}__" counter[0] += 1 @@ -587,7 +605,9 @@ def parse_formula( # Rewrite function-style aggregations (e.g., sum(revenue) → revenue:sum) formula = _rewrite_funcstyle_aggregations(formula, extra_agg_names) # Preprocess colon syntax into ast-parseable placeholders - processed, agg_refs = _preprocess_agg_refs(formula, extra_agg_names or frozenset()) + processed, agg_refs = _preprocess_agg_refs( + formula, custom_agg_names=extra_agg_names or frozenset() + ) try: tree = ast.parse(processed, mode="eval") @@ -1120,7 +1140,9 @@ def parse_filter( # 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(processed, extra_agg_names or frozenset()) + processed, agg_refs = _preprocess_agg_refs( + processed, custom_agg_names=extra_agg_names or frozenset() + ) agg_canonical = { ph: canonical_agg_name( measure_name=ref.measure_name, diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index 4043f8a0..614a6462 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -2794,7 +2794,7 @@ def extract_filter_transforms( preprocessed = _preprocess_like(preprocessed) # Preprocess colon syntax (e.g., "order_total:sum") into ast-safe placeholders preprocessed, agg_refs = _preprocess_agg_refs( - preprocessed, extra_agg_names or frozenset() + preprocessed, custom_agg_names=extra_agg_names or frozenset() ) # Build reverse map: placeholder → original colon form _agg_reverse = { From 8f1830d9c029cf4bc7c8e3ef6c37ace1f60a964c Mon Sep 17 00:00:00 2001 From: Egor Kraev Date: Sat, 20 Jun 2026 17:44:18 +0200 Subject: [PATCH 5/5] DEV-1576 PR #198 review round 4: name the formula= arg at _preprocess_agg_refs call sites Fully satisfy the repo kwargs-for-multi-param guideline (CodeRabbit) by naming the first positional argument too at all three _preprocess_agg_refs calls. Co-Authored-By: Claude Opus 4.8 (1M context) --- slayer/core/formula.py | 4 ++-- slayer/engine/enrichment.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/slayer/core/formula.py b/slayer/core/formula.py index d04dd507..0e8f9fd4 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -606,7 +606,7 @@ def parse_formula( formula = _rewrite_funcstyle_aggregations(formula, extra_agg_names) # Preprocess colon syntax into ast-parseable placeholders processed, agg_refs = _preprocess_agg_refs( - formula, custom_agg_names=extra_agg_names or frozenset() + formula=formula, custom_agg_names=extra_agg_names or frozenset() ) try: @@ -1141,7 +1141,7 @@ def parse_filter( # ``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( - processed, custom_agg_names=extra_agg_names or frozenset() + formula=processed, custom_agg_names=extra_agg_names or frozenset() ) agg_canonical = { ph: canonical_agg_name( diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index 614a6462..effbf23b 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -2794,7 +2794,7 @@ def extract_filter_transforms( preprocessed = _preprocess_like(preprocessed) # Preprocess colon syntax (e.g., "order_total:sum") into ast-safe placeholders preprocessed, agg_refs = _preprocess_agg_refs( - preprocessed, custom_agg_names=extra_agg_names or frozenset() + formula=preprocessed, custom_agg_names=extra_agg_names or frozenset() ) # Build reverse map: placeholder → original colon form _agg_reverse = {