diff --git a/slayer/core/formula.py b/slayer/core/formula.py index d41dd5a2..fd01ed45 100644 --- a/slayer/core/formula.py +++ b/slayer/core/formula.py @@ -13,6 +13,7 @@ """ import ast +import difflib import io import re import tokenize @@ -600,13 +601,38 @@ def parse_formula( except SyntaxError as e: raise ValueError(f"Invalid formula syntax: {formula!r} — {e}") - return _parse_node(tree.body, original=formula, agg_refs=agg_refs) + return _parse_node( + tree.body, + original=formula, + agg_refs=agg_refs, + known_measures=frozenset(named_measures or ()), + ) + + +def _bare_name_message(*, name: str, known_measures: frozenset[str]) -> str: + """Explain a bare name that expanded to no saved measure. + + A bare name is only ever a saved measure, and by this point expansion has + already failed, so the name is either misspelled or a column. Suggesting a + real measure first stops the reader from adding a colon suffix that a + measure would reject anyway. + """ + suggestion = difflib.get_close_matches( + word=name, possibilities=sorted(known_measures), n=1 + ) + hint = f" Did you mean '{suggestion[0]}'?" if suggestion else "" + return ( + f"'{name}' is not a saved measure.{hint} Reference a saved measure by its " + f"bare name, or aggregate a column with colon syntax (e.g., '{name}:sum'). " + f"For COUNT(*), use '*:count'." + ) def _parse_node( node: ast.AST, original: str, agg_refs: dict[str, AggregatedMeasureRef] | None = None, + known_measures: frozenset[str] = frozenset(), ) -> FieldSpec: """Recursively parse an AST node into a FieldSpec.""" if agg_refs is None: @@ -616,11 +642,8 @@ def _parse_node( if isinstance(node, ast.Name): if node.id in agg_refs: return agg_refs[node.id] - name = node.id raise ValueError( - f"Bare measure name '{name}' is not valid. " - f"Use colon syntax (e.g., '{name}:sum', '{name}:avg'). " - f"For COUNT(*), use '*:count'." + _bare_name_message(name=node.id, known_measures=known_measures) ) # Dotted name → cross-model measure must include aggregation @@ -643,7 +666,7 @@ def _parse_node( # _parse_mixed_arithmetic so inner aggregated refs and nested # transforms are registered/extracted. _validate_scalar_call(node, original) - return _parse_mixed_arithmetic(node, original, agg_refs) + return _parse_mixed_arithmetic(node, original, agg_refs, known_measures) if category != "transform": raise ValueError( f"Unknown function '{func_name}' in formula {original!r}. " @@ -656,7 +679,7 @@ def _parse_node( raise ValueError(f"Transform '{func_name}' requires at least one argument (the measure)") # First arg is the measure/expression being transformed - inner = _parse_node(node.args[0], original, agg_refs) + inner = _parse_node(node.args[0], original, agg_refs, known_measures) # Remaining positional args are transform parameters (offset, granularity, etc.) # The rank family is keyword-only after the measure; reject extra positionals @@ -682,7 +705,7 @@ def _parse_node( # Binary/unary/comparison/boolean operation → check if it contains transform calls if isinstance(node, (ast.BinOp, ast.UnaryOp, ast.Compare, ast.BoolOp)): if _contains_call(node): - return _parse_mixed_arithmetic(node, original, agg_refs) + return _parse_mixed_arithmetic(node, original, agg_refs, known_measures) measure_names = _collect_names(node) # Reject bare measure names (not from colon syntax preprocessing) for mname in measure_names: @@ -693,9 +716,9 @@ def _parse_node( f"(e.g., '{mname}:sum')." ) raise ValueError( - f"Bare measure name '{mname}' is not valid. " - f"Use colon syntax (e.g., '{mname}:sum', '{mname}:avg'). " - f"For COUNT(*), use '*:count'." + _bare_name_message( + name=mname, known_measures=known_measures + ) ) field_agg_refs = {n: agg_refs[n] for n in measure_names if n in agg_refs} return ArithmeticField( @@ -728,12 +751,13 @@ def _replace_calls_in_arith( counter: list[int], agg_refs: dict[str, AggregatedMeasureRef], original: str, + known_measures: frozenset[str] = frozenset(), ) -> ast.AST: """Walk the AST, replacing transform Call nodes with Name placeholders.""" if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id in ALL_TRANSFORMS: placeholder = f"_t{counter[0]}" counter[0] += 1 - transform = _parse_node(node, original, agg_refs) + transform = _parse_node(node, original, agg_refs, known_measures) sub_transforms.append((placeholder, transform)) return ast.Name(id=placeholder, ctx=ast.Load()) @@ -743,6 +767,7 @@ def _replace_calls_in_arith( "counter": counter, "agg_refs": agg_refs, "original": original, + "known_measures": known_measures, } if isinstance(node, ast.Name): @@ -795,6 +820,7 @@ def _parse_mixed_arithmetic( node: ast.AST, original: str, agg_refs: dict[str, AggregatedMeasureRef] | None = None, + known_measures: frozenset[str] = frozenset(), ) -> MixedArithmeticField: """Parse arithmetic that contains transform calls. @@ -815,6 +841,7 @@ def _parse_mixed_arithmetic( counter=counter, agg_refs=agg_refs, original=original, + known_measures=known_measures, ) modified_sql = ast.unparse(modified) diff --git a/slayer/engine/enrichment.py b/slayer/engine/enrichment.py index bae49204..55196e72 100644 --- a/slayer/engine/enrichment.py +++ b/slayer/engine/enrichment.py @@ -170,6 +170,58 @@ def _public_field_name(qfield: Any) -> str: ) +def _close_name_hint(*, name: str, model: Any) -> str: + """A ' Did you mean ...?' clause drawn from every name the model offers. + + ``ModelMeasure.name`` is optional, so unnamed measures are dropped rather + than sorted against strings. + """ + known = sorted( + {c.name for c in model.columns} + | {m.name for m in model.measures if m.name is not None} + ) + suggestion = difflib.get_close_matches(word=name, possibilities=known, n=1) + return f" Did you mean '{suggestion[0]}'?" if suggestion else "" + + +def _unknown_column_message( + *, model: Any, measure_name: str, aggregation_name: str +) -> str: + """Explain why ``measure_name:aggregation_name`` did not resolve. + + Columns and saved measures share one namespace but take opposite syntax: + a column needs the colon suffix, a measure must not carry one. Naming the + kind that does exist turns a dead end into a one-step correction. + """ + if model.get_measure(measure_name) is not None: + return ( + f"'{measure_name}' is a saved measure on model '{model.name}', not a " + f"column, so it takes no aggregation. Reference it as '{measure_name}' " + f"instead of '{measure_name}:{aggregation_name}'." + ) + hint = _close_name_hint(name=measure_name, model=model) + return f"Column '{measure_name}' not found in model '{model.name}'.{hint}" + + +def _bare_name_in_expression_message(*, model: Any, name: str) -> str: + """Explain a bare name inside an arithmetic expression. + + Saved measures are inlined before this point, so a name that survives is + either a column that forgot its aggregation or nothing at all. + """ + if model.get_column(name) is not None: + return ( + f"'{name}' is a column on model '{model.name}', so it needs an " + f"aggregation inside an expression — write '{name}:sum', or another " + f"aggregation." + ) + hint = _close_name_hint(name=name, model=model) + return ( + f"'{name}' is not a saved measure on model '{model.name}'.{hint} " + f"Aggregate a column with colon syntax (e.g., '{name}:sum')." + ) + + async def enrich_query( query: SlayerQuery, model: SlayerModel, @@ -461,7 +513,11 @@ async def _ensure_aggregated_measure( measure_def = model.get_column(measure_name) if measure_def is None: raise ValueError( - f"Column '{measure_name}' not found in model '{model.name}'" + _unknown_column_message( + model=model, + measure_name=measure_name, + aggregation_name=aggregation_name, + ) ) # DEV-1576 §3: distinguish "unknown aggregation name" from "known # but not allowed for this column type". The name check runs BEFORE @@ -870,7 +926,9 @@ async def _ensure_measure_from_spec(mname: str, agg_refs: dict | None = None): agg_kwargs=ref.agg_kwargs, ) else: - raise ValueError(f"Bare measure name '{mname}' in expression is not valid. Use colon syntax.") + raise ValueError( + _bare_name_in_expression_message(model=model, name=mname) + ) async def _resolve_inner_alias(inner_spec, fallback_name: str) -> str: """Flatten a transform's inner spec to a measure alias. diff --git a/slayer/search/retrievers/embeddings.py b/slayer/search/retrievers/embeddings.py index 44000031..4135e7d1 100644 --- a/slayer/search/retrievers/embeddings.py +++ b/slayer/search/retrievers/embeddings.py @@ -192,10 +192,9 @@ async def retrieve( and entity rankings from a single ``fetch_corpus`` + ``embed_question`` + dim-check (Codex Finding 1). - Skipped (with a warning) when: + Skipped when ``question`` is blank or the ``advanced_search`` extra is + unavailable, and skipped with a warning when: - * ``question`` is blank, - * the ``advanced_search`` extra is not installed, * the active model has no embedding rows in storage, * the query embedding call fails, * dim mismatch between query vec and corpus. @@ -203,11 +202,14 @@ async def retrieve( if corpus is None or not question or not question.strip(): return RetrievalResult() if not embedding_client.is_available(): - return RetrievalResult(warnings=[ + # Deployment configuration, not something the caller can act on: + # log it for the operator rather than returning it as a warning. + _log.warning( "embedding channel skipped: `advanced_search` extra not " "installed or no API key configured for the active " - "embedding model.", - ]) + "embedding model." + ) + return RetrievalResult() rows = await self.fetch_corpus() if datasource is not None: @@ -230,10 +232,9 @@ async def retrieve( ]) # Inline imports: ``numpy`` and ``slayer.embeddings.ranker`` - # require the optional ``advanced_search`` extra. When the extra - # is not installed, we fall through to a soft warning instead of - # raising at module import time so the rest of slayer keeps - # working without the extra. + # require the optional ``advanced_search`` extra. Importing here + # rather than at module scope keeps the rest of slayer working + # without the extra. try: import numpy as np from slayer.embeddings.ranker import ( @@ -242,10 +243,13 @@ async def retrieve( top_k_cosine, ) except ImportError: - return RetrievalResult(warnings=[ + # Same packaging gap as the is_available() check above, reached by + # a different route: an operator concern, not a caller's. + _log.warning( "embedding channel skipped: numpy not installed " - "(reinstall with the `advanced_search` extra).", - ]) + "(reinstall with the `advanced_search` extra)." + ) + return RetrievalResult() query_vec = await self.embed_question(question or "") if query_vec is None: diff --git a/tests/integration/test_integration.py b/tests/integration/test_integration.py index 77163e0d..8e4c47f7 100644 --- a/tests/integration/test_integration.py +++ b/tests/integration/test_integration.py @@ -3844,7 +3844,7 @@ async def test_round_over_bare_measure_raises(integration_env): source_model="orders", measures=[ModelMeasure(formula="round(amount, 2)", name="r")], ) - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="needs an aggregation inside an expression"): await engine.execute(query) @@ -3855,5 +3855,5 @@ async def test_abs_over_bare_measure_raises(integration_env): source_model="orders", measures=[ModelMeasure(formula="abs(amount)", name="a")], ) - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="needs an aggregation inside an expression"): await engine.execute(query) diff --git a/tests/integration/test_mcp_inspect.py b/tests/integration/test_mcp_inspect.py index c69301bd..3aeb001b 100644 --- a/tests/integration/test_mcp_inspect.py +++ b/tests/integration/test_mcp_inspect.py @@ -315,7 +315,7 @@ async def test_full_response(self, env) -> None: # No leaked error artefacts from the old implementation assert "sample_data_error" not in result - assert "Bare measure name" not in result + assert "is not a saved measure" not in result async def test_no_longer_json(self, env) -> None: server = create_mcp_server(storage=env["storage"]) diff --git a/tests/test_formula.py b/tests/test_formula.py index 07e8d434..3aac090e 100644 --- a/tests/test_formula.py +++ b/tests/test_formula.py @@ -20,11 +20,11 @@ class TestFormulaParser: def test_bare_measure_raises(self) -> None: - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula("count") def test_bare_measure_in_arithmetic_raises(self) -> None: - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula("revenue / count") def test_aggregated_measure(self) -> None: @@ -350,7 +350,7 @@ def test_not_substituted_when_called(self) -> None: assert result.inner.measure_name == "revenue" def test_unknown_bare_name_still_raises(self) -> None: - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula( "unknown_thing", named_measures={"aov": "revenue:sum"} ) @@ -360,7 +360,7 @@ def test_no_named_measures_preserves_old_behavior(self) -> None: existing bare-name rejection — no regression for callers that don't opt in. """ - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): parse_formula("aov") diff --git a/tests/test_named_measures.py b/tests/test_named_measures.py index 3193a845..5d8d0758 100644 --- a/tests/test_named_measures.py +++ b/tests/test_named_measures.py @@ -173,7 +173,76 @@ async def test_unknown_bare_name_still_errors(self) -> None: measures=[{"formula": "nonexistent", "name": "result"}], ) - with pytest.raises(ValueError, match="Bare measure name"): + with pytest.raises(ValueError, match="is not a saved measure"): + await _generate(query, model) + + async def test_near_miss_bare_name_suggests_the_saved_measure(self) -> None: + """A misspelled saved measure names the real one.""" + model = _orders_model( + measures=[ModelMeasure(name="aov_net", formula="revenue:sum")] + ) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "aov_nett", "name": "result"}], + ) + + with pytest.raises(ValueError, match="Did you mean 'aov_net'"): + await _generate(query, model) + + async def test_aggregating_a_saved_measure_says_to_drop_the_suffix(self) -> None: + """``measure:agg`` used to report the measure as a missing column.""" + model = _orders_model( + measures=[ModelMeasure(name="aov", formula="revenue:sum")] + ) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "aov:sum", "name": "result"}], + ) + + with pytest.raises( + ValueError, match="is a saved measure.*takes no aggregation" + ): + await _generate(query, model) + + async def test_bare_column_in_expression_asks_for_an_aggregation(self) -> None: + """The mixed-arithmetic path reaches its own error site.""" + model = _orders_model() + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "round(revenue, 2)", "name": "result"}], + ) + + with pytest.raises( + ValueError, match="needs an aggregation inside an expression" + ): + await _generate(query, model) + + async def test_unnamed_measure_does_not_break_the_suggestion(self) -> None: + """``ModelMeasure.name`` is optional; unnamed measures are skipped. + + The model validator rejects unnamed measures at construction, so this + reaches the helper the way a post-construction mutation would. + """ + model = _orders_model( + measures=[ModelMeasure(name="aov", formula="revenue:sum")] + ) + model.measures.append(ModelMeasure(formula="revenue:avg")) + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "revenu:sum", "name": "result"}], + ) + + with pytest.raises(ValueError, match="Did you mean 'revenue'"): + await _generate(query, model) + + async def test_unknown_column_suggests_a_close_column(self) -> None: + model = _orders_model() + query = SlayerQuery( + source_model="orders", + measures=[{"formula": "revenu:sum", "name": "result"}], + ) + + with pytest.raises(ValueError, match="Did you mean 'revenue'"): await _generate(query, model) async def test_duplicate_saved_measure_name_rejected_in_enrichment(self) -> None: diff --git a/tests/test_search_three_channel.py b/tests/test_search_three_channel.py index 79fda6ca..7db57764 100644 --- a/tests/test_search_three_channel.py +++ b/tests/test_search_three_channel.py @@ -9,6 +9,7 @@ from __future__ import annotations +import logging import tempfile from collections.abc import Iterator @@ -69,20 +70,28 @@ async def _seed_basic_corpus(storage: YAMLStorage) -> None: # --------------------------------------------------------------------------- -async def test_question_only_warns_when_extra_missing( - storage: YAMLStorage, monkeypatch: pytest.MonkeyPatch, +async def test_question_only_degrades_quietly_when_extra_missing( + storage: YAMLStorage, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, ) -> None: """If the extra isn't installed (or no API key is configured), the - embedding channel emits a warning and the search still returns - whatever tantivy + BM25 found. The session-wide autouse fixture - already stubs ``is_available`` to ``False`` — this test relies on - that default rather than re-patching it.""" + embedding channel is skipped and the search still returns whatever + tantivy + BM25 found. That is a deployment gap the caller cannot act + on, so it reaches the operator log rather than the response. The + session-wide autouse fixture already stubs ``is_available`` to + ``False`` — this test relies on that default rather than re-patching + it.""" await _seed_basic_corpus(storage) service = SearchService(storage=storage) - response = await service.search(question="how do I look up purchases?") - assert any( + with caplog.at_level( + logging.WARNING, logger="slayer.search.retrievers.embeddings" + ): + response = await service.search(question="how do I look up purchases?") + assert not any( "advanced_search" in w for w in response.warnings ), response.warnings + assert "advanced_search" in caplog.text async def test_question_only_warns_when_no_embeddings_persisted(