From 8fb4043b0f4f659a1771ec06d44b6150edac48ad Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 2 Sep 2026 17:55:15 -0600 Subject: [PATCH 1/4] feat(py): the definitions registry, token expansion and the prompt index The registry surface commons owns whatever data-dict eventually provides: {{name}} and {{table::name}} expansion, the kind index for the prompt, grain metadata for call_metrics' mixed-grain guard, and the check that a definition's table is one the source exposes. Everything consumes ExportRecord and nothing reaches into an expression parser or a typed IR. That rule is what makes replacing this with a shared data-dict interface a deletion rather than a rewrite, and it is why the registry can be built and tested before the compiler exists. The dictionary's first-touch entry and its retrieval chunks now render a table's definitions, which the reader left as a marked seam. Both show compiled SQL rather than the authored expression, since the expression is in data-dict's language and the model writes SQL. Tests run the registry against tests/shared/definitions.json as well as against hand-built records: every definition data-dict produced expands to its own compiled SQL, and each kind is one the index groups. This is the first Python code to consume that contract. --- pkg-py/src/commons/_data_dictionary.py | 18 +- pkg-py/src/commons/_definitions/__init__.py | 37 ++ pkg-py/src/commons/_definitions/_registry.py | 297 ++++++++++++++ pkg-py/tests/test_data_dictionary.py | 59 +++ pkg-py/tests/test_definitions_registry.py | 410 +++++++++++++++++++ tests/shared/README.md | 9 +- 6 files changed, 822 insertions(+), 8 deletions(-) create mode 100644 pkg-py/src/commons/_definitions/__init__.py create mode 100644 pkg-py/src/commons/_definitions/_registry.py create mode 100644 pkg-py/tests/test_definitions_registry.py diff --git a/pkg-py/src/commons/_data_dictionary.py b/pkg-py/src/commons/_data_dictionary.py index 974476d0..796a3306 100644 --- a/pkg-py/src/commons/_data_dictionary.py +++ b/pkg-py/src/commons/_data_dictionary.py @@ -99,6 +99,9 @@ class Table(_Permissive): details: str | None = None columns: dict[str, Column] = {} definitions: dict[str, Definition] = {} + # Attached by _definitions at data-source construction; empty until then, + # so the registry can be exercised without the compiler. + compiled_definitions: list[Any] = [] @model_validator(mode="before") @classmethod @@ -202,16 +205,17 @@ def entry_parts( entry = self.tables.get(table) if entry is None: return [] - # Governed definitions belong between the columns and the - # relationships, and are added once the compiler can supply them. - # They render as compiled SQL, never as the authored expression, so - # there is nothing correct to show before compilation happens. + # Imported here because _definitions does not import this module and + # this keeps it that way. + from ._definitions import entry_text as definitions_entry_text + parts = [ part for part in ( entry.description, entry.details, columns_text, + definitions_entry_text(entry.compiled_definitions), self._relationships_text(table), ) if part @@ -288,8 +292,10 @@ def context_chunks(self) -> list[str]: if prose: chunks.append(f"Table `{name}`: {prose}") chunks.extend(f"{term}: {body}" for term, body in self.glossary.items()) - # One chunk per governed definition joins these once the compiler can - # supply them, for the same reason as the first-touch entry. + from ._definitions import context_chunks as definitions_context_chunks + + for entry in self.tables.values(): + chunks.extend(definitions_context_chunks(entry.compiled_definitions)) return [chunk for chunk in chunks if chunk] diff --git a/pkg-py/src/commons/_definitions/__init__.py b/pkg-py/src/commons/_definitions/__init__.py new file mode 100644 index 00000000..58602fe4 --- /dev/null +++ b/pkg-py/src/commons/_definitions/__init__.py @@ -0,0 +1,37 @@ +"""Governed definitions: the registry, and the compiler that feeds it. + +Definitions are authored in data-dict's expression language, not in the SQL +dialect of the attached source, so they are type-checked against the +dictionary and lowered to the source's dialect before anything runs. That work +sits behind one export-record seam, pinned to data-dict commit +d950c5ac90d0ab939d330600f3a5ee1bfde0f604, so it can be replaced by a shared +data-dict interface later. + +Runtime code consumes `ExportRecord` and never an expression parser or a typed +IR. Cross-implementation conformance against the data-dict binary is the +authority, not this code. +""" + +from ._registry import ( + ExportRecord, + Registry, + applied_text, + build_registry, + context_chunks, + entry_text, + expand_tokens, + index_overflows, + index_text, +) + +__all__ = [ + "ExportRecord", + "Registry", + "applied_text", + "build_registry", + "context_chunks", + "entry_text", + "expand_tokens", + "index_overflows", + "index_text", +] diff --git a/pkg-py/src/commons/_definitions/_registry.py b/pkg-py/src/commons/_definitions/_registry.py new file mode 100644 index 00000000..5b7dde18 --- /dev/null +++ b/pkg-py/src/commons/_definitions/_registry.py @@ -0,0 +1,297 @@ +"""The registry surface commons owns, whatever data-dict eventually provides. + +Everything here consumes export records. Nothing here may reach into an +expression parser or a typed IR: that rule is what keeps the seam real, and +what makes an eventual swap to a shared data-dict interface a deletion rather +than a rewrite. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, field, replace +from typing import Any + +__all__ = [ + "ExportRecord", + "Registry", + "applied_text", + "build_registry", + "context_chunks", + "entry_text", + "expand_tokens", + "index_overflows", + "index_text", +] + +INDEX_CAP_CHARS = 4000 + +_TOKEN = re.compile(r"\{\{\s*([^{}]+?)\s*\}\}") +_LEGACY_DOTTED = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)+$") +_GROUPS = {"filter": "filters", "derived": "derived", "metric": "metrics"} + + +@dataclass(frozen=True) +class ExportRecord: + """One governed definition, compiled for a source and ready to expand. + + `mixed_grain` is the one field data-dict's export does not carry. It is + derived from the typed IR, and `call_metrics` needs it to refuse a mix of + row and aggregate definitions in one call. + """ + + name: str + table: str + source: str + kind: str + type: str | None + expression: str + label: str | None + description: str | None + details: str | None + columns: list[str] + definitions: list[str] + sql: str + target: str + notes: list[str] + mixed_grain: bool + + +@dataclass +class Registry: + """Every source's definitions in one place.""" + + records: list[ExportRecord] = field(default_factory=list) + + def for_source(self, name: str | None = None) -> list[ExportRecord]: + if name is None: + return list(self.records) + return [record for record in self.records if record.source == name] + + +def build_registry(sources: dict[str, Any]) -> Registry: + """Collect every source's compiled definitions into one registry. + + Takes `DataSource` values, typed loosely because `_data_source` imports + this module and the reverse import would close the cycle. + + A definition on a table the source does not expose is a construction + error: its token would resolve at conversation time against nothing. + """ + records: list[ExportRecord] = [] + for label, source in sources.items(): + dictionary = getattr(source, "dictionary", None) + if dictionary is None: + continue + exposed = set(getattr(source, "tables", [])) + for table, entry in dictionary.tables.items(): + compiled = getattr(entry, "compiled_definitions", None) or [] + if not compiled: + continue + if table not in exposed: + raise ValueError( + f"The data dictionary declares definitions on table {table!r}, " + "which the data source does not expose. Exposed tables: " + f"{', '.join(sorted(exposed))}." + ) + for record in compiled: + records.append(replace(record, table=table, source=label)) + return Registry(records) + + +# ---- token expansion ----------------------------------------------------- + + +def expand_tokens( + sql: str, records: list[ExportRecord] +) -> tuple[str, list[ExportRecord]]: + """Replace `{{name}}` and `{{table::name}}` with compiled SQL. + + Runs before the read-only guard, so the guard sees the SQL that will + actually execute. Failures are tool errors the model can recover from + in-conversation, so they name the alternatives. + """ + tokens: list[str] = [] + for match in _TOKEN.finditer(sql): + token = match.group(1) + if token not in tokens: + tokens.append(token) + + applied: list[ExportRecord] = [] + for token in tokens: + record = _resolve_token(token, sql, records) + pattern = re.compile(r"\{\{\s*" + re.escape(token) + r"\s*\}\}") + # A lambda, not a replacement string: the SQL is literal and a + # backslash in it would otherwise be read as an escape. + sql = pattern.sub(lambda _match, found=record: f"({found.sql})", sql) + applied.append(record) + return sql, applied + + +def _word_pattern(word: str) -> re.Pattern[str]: + return re.compile(rf"(? bool: + # Tokens are stripped first so a token's own text cannot bring its table + # into scope. A word match rather than a parse, which is the same + # heuristic that picks entries for first touch. + return bool(_word_pattern(table).search(_TOKEN.sub("", sql))) + + +def _resolve_token(token: str, sql: str, records: list[ExportRecord]) -> ExportRecord: + if "::" in token: + table, _, name = token.partition("::") + return _resolve_qualified(table, name, token, sql, records) + + named = [record for record in records if record.name == token] + if not named: + if not _LEGACY_DOTTED.match(token): + _abort_unknown(token, records) + table, _, name = token.rpartition(".") + return _resolve_qualified(table, name, token, sql, records) + + in_scope = [record for record in named if _table_in_query(record.table, sql)] + if len(in_scope) == 1: + return in_scope[0] + if not in_scope: + tables = ", ".join(sorted({record.table for record in named})) + raise ValueError( + f"{{{{{token}}}}} is defined on table {tables}, which does not " + "appear in this query." + ) + qualified = " or ".join(f"{{{{{record.table}::{token}}}}}" for record in in_scope) + raise ValueError( + f"{{{{{token}}}}} is ambiguous here: it is defined on several tables in " + f"this query. Qualify the token: {qualified}." + ) + + +def _resolve_qualified( + table: str, name: str, token: str, sql: str, records: list[ExportRecord] +) -> ExportRecord: + hits = [ + record for record in records if record.table == table and record.name == name + ] + if not hits: + _abort_unknown(token, records) + if not _table_in_query(table, sql): + raise ValueError( + f"{{{{{token}}}}} is defined on table {table}, which does not appear " + "in this query." + ) + return hits[0] + + +def _abort_unknown(token: str, records: list[ExportRecord]) -> None: + if not records: + detail = "This source has no governed definitions." + else: + available = ", ".join(f"{{{{{r.name}}}}} ({r.table})" for r in records) + detail = f"Available definitions: {available}." + raise ValueError(f"No governed definition matches {{{{{token}}}}}. {detail}") + + +# ---- rendering ----------------------------------------------------------- + + +def _flatten_inline(text: str) -> str: + return re.sub(r"\s+", " ", text).strip() + + +def _index_lines(registry: Registry) -> list[str]: + records = registry.records + if not records: + return [] + multi_source = len({record.source for record in records}) > 1 + + def scope(record: ExportRecord) -> str: + return f"{record.source}.{record.table}" if multi_source else record.table + + lines: list[str] = [] + for one in dict.fromkeys(scope(record) for record in records): + here = [record for record in records if scope(record) == one] + parts = [] + for kind, plural in _GROUPS.items(): + items = [ + f"`{{{{{record.name}}}}}` ({_flatten_inline(record.label)})" + if record.label + else f"`{{{{{record.name}}}}}`" + for record in here + if record.kind == kind + ] + if items: + parts.append(f"{plural} {', '.join(items)}") + lines.append(f"- {one}: {'; '.join(parts)}") + return lines + + +def index_text(registry: Registry, cap_chars: int = INDEX_CAP_CHARS) -> str: + """The kind index for the system prompt, truncated to the cap.""" + kept: list[str] = [] + used = 0 + for line in _index_lines(registry): + used += len(line) + if used > cap_chars: + break + kept.append(line) + return "\n".join(kept) + + +def index_overflows(registry: Registry, cap_chars: int = INDEX_CAP_CHARS) -> bool: + """Whether the index did not fit, so the model is told to search instead.""" + return sum(len(line) for line in _index_lines(registry)) > cap_chars + + +def _gist(record: ExportRecord) -> str: + detail = _flatten_inline( + " ".join(part for part in (record.description, record.details) if part) + ) + parts = [f"({record.kind}, {record.type})"] + if detail: + parts.append(detail) + parts.append(f"Selected {record.target}: `({_flatten_inline(record.sql)})`.") + if record.notes: + parts.append(f"Translation notes: {' '.join(record.notes)}") + return " ".join(parts) + + +def entry_text(records: list[ExportRecord]) -> str | None: + """A table's governed definitions, delivered at first touch. + + Shows the compiled SQL, never the authored expression: the expression is + in data-dict's language, and the model writes SQL. + """ + if not records: + return None + header = ( + "Governed definitions (write as `{{name}}` tokens in SQL; use " + "`{{table::name}}` to qualify):\n\n" + ) + lines = [f"- `{{{{{record.name}}}}}` {_gist(record)}" for record in records] + return header + "\n".join(lines) + + +def context_chunks(records: list[ExportRecord]) -> list[str]: + """One retrievable chunk per definition.""" + return [ + f"Governed definition `{{{{{record.name}}}}}` on table `{record.table}` " + f"({record.kind}, {record.type})" + for record in records + ] + + +def applied_text(applied: list[ExportRecord]) -> str | None: + """What each expanded token became, reported alongside a query's results.""" + if not applied: + return None + lines = [] + for record in applied: + line = ( + f"- {{{{{record.name}}}}} ({record.table}): {record.target} " + f"`({_flatten_inline(record.sql)})`" + ) + if record.notes: + line += f"\n Translation notes: {' '.join(record.notes)}" + lines.append(line) + return "Applied governed definitions:\n\n" + "\n".join(lines) diff --git a/pkg-py/tests/test_data_dictionary.py b/pkg-py/tests/test_data_dictionary.py index 5abf86ce..9c7ad067 100644 --- a/pkg-py/tests/test_data_dictionary.py +++ b/pkg-py/tests/test_data_dictionary.py @@ -347,3 +347,62 @@ def test_a_frame_named_dictionary_fails_loudly_rather_than_silently() -> None: with pytest.raises(TypeError, match="DataFrame"): data_source(dictionary=pd.DataFrame({"revenue": [1.0]})) + + +def test_a_tables_entry_lists_its_governed_definitions(retail: DataDictionary) -> None: + from commons._definitions import ExportRecord + + retail.tables["sales"].compiled_definitions = [ + ExportRecord( + name="net_revenue", + table="sales", + source="", + kind="metric", + type="number", + expression="SUM(revenue)", + label=None, + description=None, + details=None, + columns=["revenue"], + definitions=[], + sql="sum(revenue)", + target="SQL(duckdb)", + notes=[], + mixed_grain=False, + ) + ] + + text = retail.entry_text("sales") + + assert text is not None + assert "Governed definitions" in text + assert text.index("Documented columns:") < text.index("Governed definitions") + assert text.index("Governed definitions") < text.index("Relationships:") + + +def test_context_chunks_include_governed_definitions(retail: DataDictionary) -> None: + from commons._definitions import ExportRecord + + retail.tables["sales"].compiled_definitions = [ + ExportRecord( + name="net_revenue", + table="sales", + source="", + kind="metric", + type="number", + expression="SUM(revenue)", + label=None, + description=None, + details=None, + columns=["revenue"], + definitions=[], + sql="sum(revenue)", + target="SQL(duckdb)", + notes=[], + mixed_grain=False, + ) + ] + + chunks = retail.context_chunks() + + assert any(chunk.startswith("Governed definition `{{net_revenue}}`") for chunk in chunks) diff --git a/pkg-py/tests/test_definitions_registry.py b/pkg-py/tests/test_definitions_registry.py new file mode 100644 index 00000000..02a709eb --- /dev/null +++ b/pkg-py/tests/test_definitions_registry.py @@ -0,0 +1,410 @@ +"""Token expansion, the kind index, and grain metadata. + +The registry consumes export records. These tests build records directly +rather than compiling them, which is the seam working as intended: the +registry is usable before the compiler exists and keeps working after it is +replaced by whatever data-dict eventually provides. +""" + +import pytest + +from commons._definitions import ( + ExportRecord, + Registry, + applied_text, + context_chunks, + entry_text, + expand_tokens, + index_overflows, + index_text, +) +from commons._definitions._registry import _GROUPS as _GROUPS_FOR_TEST + + +def record( + name: str, + table: str = "sales", + *, + source: str = "sales_db", + kind: str = "metric", + sql: str = "sum(revenue)", + label: str | None = None, + description: str | None = None, + mixed_grain: bool = False, + definitions: list[str] | None = None, + notes: list[str] | None = None, +) -> ExportRecord: + return ExportRecord( + name=name, + table=table, + source=source, + kind=kind, + type="number", + expression="SUM(revenue)", + label=label, + description=description, + details=None, + columns=["revenue"], + definitions=definitions or [], + sql=sql, + target="SQL(duckdb)", + notes=notes or [], + mixed_grain=mixed_grain, + ) + + +# ---- token expansion ----------------------------------------------------- + + +def test_a_bare_token_expands_to_parenthesized_sql() -> None: + sql, applied = expand_tokens( + "SELECT {{net_revenue}} FROM sales", [record("net_revenue")] + ) + + assert sql == "SELECT (sum(revenue)) FROM sales" + assert [item.name for item in applied] == ["net_revenue"] + + +def test_whitespace_inside_a_token_is_tolerated() -> None: + sql, _ = expand_tokens( + "SELECT {{ net_revenue }} FROM sales", [record("net_revenue")] + ) + + assert sql == "SELECT (sum(revenue)) FROM sales" + + +def test_sql_without_tokens_passes_through_untouched() -> None: + sql, applied = expand_tokens("SELECT 1 FROM sales", [record("net_revenue")]) + + assert sql == "SELECT 1 FROM sales" + assert applied == [] + + +def test_a_qualified_token_selects_the_named_table() -> None: + records = [ + record("total", table="sales", sql="sum(a)"), + record("total", table="returns", sql="sum(b)"), + ] + + sql, _ = expand_tokens("SELECT {{sales::total}} FROM sales JOIN returns", records) + + assert sql == "SELECT (sum(a)) FROM sales JOIN returns" + + +def test_a_bare_token_scopes_to_the_table_the_query_names() -> None: + records = [ + record("total", table="sales", sql="sum(a)"), + record("total", table="returns", sql="sum(b)"), + ] + + sql, _ = expand_tokens("SELECT {{total}} FROM returns", records) + + assert sql == "SELECT (sum(b)) FROM returns" + + +def test_an_ambiguous_bare_token_says_how_to_qualify_it() -> None: + records = [record("total", table="sales"), record("total", table="returns")] + + with pytest.raises(ValueError) as caught: + expand_tokens("SELECT {{total}} FROM sales JOIN returns", records) + + assert "{{sales::total}}" in str(caught.value) + assert "{{returns::total}}" in str(caught.value) + + +def test_a_token_whose_table_is_absent_from_the_query_errors() -> None: + with pytest.raises(ValueError, match="does not appear in this query"): + expand_tokens("SELECT {{net_revenue}} FROM other", [record("net_revenue")]) + + +def test_an_unknown_token_lists_what_is_available() -> None: + with pytest.raises(ValueError) as caught: + expand_tokens("SELECT {{nope}} FROM sales", [record("net_revenue")]) + + assert "{{net_revenue}} (sales)" in str(caught.value) + + +def test_an_unknown_token_against_no_definitions_says_so() -> None: + with pytest.raises(ValueError, match="no governed definitions"): + expand_tokens("SELECT {{nope}} FROM sales", []) + + +def test_a_dotted_token_is_read_as_table_qualified() -> None: + sql, _ = expand_tokens( + "SELECT {{sales.net_revenue}} FROM sales", [record("net_revenue")] + ) + + assert sql == "SELECT (sum(revenue)) FROM sales" + + +def test_a_definition_name_may_contain_spaces() -> None: + sql, _ = expand_tokens( + "SELECT {{net revenue}} FROM sales", [record("net revenue", sql="sum(x)")] + ) + + assert sql == "SELECT (sum(x)) FROM sales" + + +def test_backslashes_in_expanded_sql_survive() -> None: + escaped = record("pattern", sql=r"regexp_matches(x, '\d+')") + + sql, _ = expand_tokens("SELECT {{pattern}} FROM sales", [escaped]) + + assert sql == r"SELECT (regexp_matches(x, '\d+')) FROM sales" + + +def test_the_same_token_twice_expands_both_occurrences() -> None: + sql, applied = expand_tokens( + "SELECT {{total}}, {{total}} FROM sales", [record("total", sql="sum(a)")] + ) + + assert sql == "SELECT (sum(a)), (sum(a)) FROM sales" + assert len(applied) == 1 + + +# ---- the prompt index ---------------------------------------------------- + + +def test_the_index_groups_definitions_by_table_and_kind() -> None: + registry = Registry( + [ + record("net_revenue", kind="metric"), + record("is_emea", kind="filter"), + record("list_price", kind="derived"), + ] + ) + + assert index_text(registry) == ( + "- sales: filters `{{is_emea}}`; derived `{{list_price}}`; " + "metrics `{{net_revenue}}`" + ) + + +def test_a_label_is_the_index_hint() -> None: + registry = Registry([record("is_emea", kind="filter", label="EMEA segment")]) + + assert index_text(registry) == "- sales: filters `{{is_emea}}` (EMEA segment)" + + +def test_the_index_scopes_by_source_when_there_are_several() -> None: + registry = Registry( + [record("a", source="one"), record("b", source="two", table="other")] + ) + + text = index_text(registry) + + assert "- one.sales:" in text + assert "- two.other:" in text + + +def test_the_index_is_capped_and_reports_its_overflow() -> None: + registry = Registry([record(f"filter_{i:03d}", kind="filter") for i in range(400)]) + + assert index_overflows(registry) + # One line per table, so a roster this size is a single long line: the cap + # drops it entirely rather than truncating mid-token. + assert index_text(registry) == "" + assert not index_overflows(registry, cap_chars=100_000) + assert "filter_399" in index_text(registry, cap_chars=100_000) + + +def test_an_empty_registry_has_an_empty_index() -> None: + assert index_text(Registry([])) == "" + assert not index_overflows(Registry([])) + + +# ---- rendering ----------------------------------------------------------- + + +def test_first_touch_shows_compiled_sql_and_not_the_expression() -> None: + text = entry_text([record("net_revenue", sql="sum(revenue)")]) + + assert text is not None + assert "Governed definitions" in text + assert "`{{net_revenue}}`" in text + assert "Selected SQL(duckdb)" in text + assert "SUM(revenue)" not in text + + +def test_first_touch_carries_translation_notes() -> None: + text = entry_text([record("remainder", notes=["integer modulus by zero differs"])]) + + assert text is not None + assert "Translation notes: integer modulus by zero differs" in text + + +def test_first_touch_is_none_without_definitions() -> None: + assert entry_text([]) is None + + +def test_definitions_are_indexed_as_context_chunks() -> None: + assert context_chunks([record("net_revenue")]) == [ + "Governed definition `{{net_revenue}}` on table `sales` (metric, number)" + ] + + +def test_applied_text_reports_the_sql_each_token_became() -> None: + text = applied_text([record("net_revenue", sql="sum(revenue)")]) + + assert text is not None + assert "- {{net_revenue}} (sales): SQL(duckdb) `(sum(revenue))`" in text + + +def test_applied_text_is_none_when_nothing_expanded() -> None: + assert applied_text([]) is None + + +# ---- grain and construction ---------------------------------------------- + + +def test_grain_metadata_rides_on_the_record() -> None: + # call_metrics' mixed-grain guard reads this and nothing else; the shape + # that produced it stays behind the seam. + assert record("net_revenue", mixed_grain=True).mixed_grain is True + + +def test_definitions_on_an_unexposed_table_fail_construction() -> None: + from commons import _duckdb + from commons._backends import DuckDBBackend + from commons._data_dictionary import DataDictionary, Table + from commons._data_source import DataSource + from commons._definitions import build_registry + + dictionary = DataDictionary(tables={"ghost": Table()}) + dictionary.tables["ghost"].compiled_definitions = [record("total", table="ghost")] + source = DataSource( + backend=DuckDBBackend(_duckdb.connect()), + tables=["sales"], + dictionary=dictionary, + ) + + with pytest.raises(ValueError, match="ghost"): + build_registry({"sales_db": source}) + + +def test_the_registry_labels_records_with_their_source() -> None: + from commons import _duckdb + from commons._backends import DuckDBBackend + from commons._data_dictionary import DataDictionary, Table + from commons._data_source import DataSource + from commons._definitions import build_registry + + dictionary = DataDictionary(tables={"sales": Table()}) + dictionary.tables["sales"].compiled_definitions = [record("total", source="")] + source = DataSource( + backend=DuckDBBackend(_duckdb.connect()), + tables=["sales"], + dictionary=dictionary, + ) + + registry = build_registry({"sales_db": source}) + + assert [item.source for item in registry.records] == ["sales_db"] + assert registry.for_source("nope") == [] + + +def test_an_empty_source_set_builds_an_empty_registry() -> None: + from commons._definitions import build_registry + + assert build_registry({}).records == [] + + +# ---- against the shared contract ----------------------------------------- +# +# The first Python code to consume the definitions contract. The compiler does +# not exist yet, so these read records data-dict itself produced, which is +# what the registry will consume either way. + + +def fixture_records() -> list[ExportRecord]: + from ._shared import load_shared_fixture + + spec = load_shared_fixture("definitions") + records = [] + for name, cases in spec["export_records"].items(): + for key, payload in cases.items(): + table, _, definition = key.partition("::") + translation = payload["translation"] + records.append( + ExportRecord( + name=definition, + table=table, + source=name, + kind=payload["kind"], + type=payload["type"], + expression=payload["expression"], + label=None, + description=None, + details=None, + columns=list(payload["columns"]), + definitions=list(payload["definitions"]), + sql=translation["code"], + target=translation["target"], + notes=list(translation["notes"]), + mixed_grain=spec["mixed_grain"][name][key], + ) + ) + return records + + +def test_the_fixture_yields_records() -> None: + assert len(fixture_records()) > 0 + + +def test_every_fixture_definition_expands_to_its_compiled_sql() -> None: + for item in fixture_records(): + sql, applied = expand_tokens( + f"SELECT {{{{{item.table}::{item.name}}}}} FROM {item.table}", [item] + ) + + assert sql == f"SELECT ({item.sql}) FROM {item.table}", item.name + assert applied == [item] + + +def test_every_fixture_kind_is_one_the_index_groups() -> None: + # An unrecognised kind would silently vanish from the prompt index. + for item in fixture_records(): + assert item.kind in _GROUPS_FOR_TEST, item.name + + +def test_the_index_renders_every_fixture_definition() -> None: + registry = Registry(fixture_records()) + + text = index_text(registry, cap_chars=1_000_000) + + for item in registry.records: + assert f"`{{{{{item.name}}}}}`" in text, item.name + + +def test_first_touch_renders_every_fixture_definition() -> None: + records = fixture_records() + text = entry_text(records) + + assert text is not None + for item in records: + assert f"`{{{{{item.name}}}}}`" in text, item.name + + +def test_a_typeless_definition_renders_its_missing_type_as_a_placeholder() -> None: + # data-dict omits `type` when no single one is inferred, so the record + # carries None and it reaches the prompt as the string "None". pkg-r puts + # "NA" in the same slot from its own missing value. Pinned as it stands + # rather than improved here, because this text is a shared contract and + # changing one implementation alone is the drift the fixtures exist to + # prevent. + typeless = [item for item in fixture_records() if item.type is None] + assert len(typeless) == 1 + + assert context_chunks(typeless) == [ + ( + "Governed definition `{{mixed temporal case}}` on table `survey` " + "(derived, None)" + ) + ] + + +def test_grain_metadata_reaches_the_records() -> None: + mixed = [item for item in fixture_records() if item.mixed_grain] + + assert mixed + assert not all(item.mixed_grain for item in fixture_records()) diff --git a/tests/shared/README.md b/tests/shared/README.md index f54deb26..e4d51e24 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -20,12 +20,17 @@ Most Posit R/Python pairs (ellmer/chatlas, ragnar/raghilda, shiny/py-shiny) shar The Python suite reads this directory directly. The R suite cannot. `testthat` needs its fixtures inside the package, and an installed R package cannot reach files outside its own directory. The R suite reads a copy synced into `pkg-r/tests/testthat/fixtures/shared/`. That copy is committed, `scripts/sync-shared-fixtures.sh` generates it, and a CI job re-runs the script and fails when the copy is stale. -## What belongs here +## What sort of fixtures are here - **Span names and attributes.** `commons_conversation_turn`, `commons_agent_create`, `commons_data_source_create`, and friends. Also `gen_ai.conversation.id`, `commons.provenance.tag`, and the exact JSON shape of `commons.citation.candidates`. This contract lets the R trajectory reviewer read Python traces. Write it so that it survives the conversation-id ownership moving upstream to shinychat. - **The provenance and citation behavior.** The `derive_provenance_tag()` truth table, `normalize_citation()` input/output pairs, `match_citation()` verdicts including both guards (10-character minimum, only-the-quote-verifies), `parse_commons_citation()` well-formed and malformed bodies. Also the chunk-invariance cases of the streaming scanner. The scanner is a pure chunks-in/string-out function, so it is ideal fixture material. - **The citation dialect and display copy.** The `` grammar and the `PROVENANCE_DISPLAY` strings, so that both UIs say the same words. -- **The definitions interface.** `definitions.json` pins the export-record contract both packages consume, the grain metadata `call_metrics` needs for its mixed-grain guard, and the data-dict problem code each invalid fixture must produce. `definition-export/` is the conformance corpus itself, read by both suites rather than copied into either. `export_records` is generated from the data-dict binary at the pinned commit by `scripts/generate-definitions-fixture.sh`, which refuses to run against a binary built from anything else; `mixed_grain` and `invalid` are hand-maintained and the generator preserves them. This fixture does not replace the conformance harness: the harness compares against a real binary, while this pins what both packages agree to consume. +- **The definitions interface.** `definition-export/` holds the shared fixtures: 14 data dictionaries in data-dict's YAML format, each declaring table-level `definitions` whose expressions use data-dict's expression language — 3 valid files (42 definitions) and 11 invalid ones — read by both commons implementations. `definitions.json` pins what both packages agree to produce from them, in three sections: + - `export_records` — the expected export for each valid definition: its SQL translation, its inferred kind and type, and the columns and definitions it references. Generated from the data-dict binary at the pinned commit by `scripts/generate-definitions-fixture.sh`, which refuses to run against a binary built from anything else. Never hand-edit. + - `mixed_grain` — a per-definition boolean for `call_metrics`' mixed-grain guard: true when a definition's exported shape is `row` but its expression contains an aggregate, directly or through a definition it references. Absent from data-dict's export (it exists only in the compiler's internal parse tree), so it is hand-maintained and the generator preserves it. + - `invalid` — the data-dict problem code each invalid fixture must produce (e.g. `cycle.yaml` must fail with the cycle error, not a generic parse failure). Hand-maintained; the generator preserves it. + + This fixture does not replace the conformance harness: the harness compares against a real binary, while this pins what both packages agree to consume. - **Trace file naming.** `trace(-[0-9]+)?\.jsonl`, one OTLP envelope per line. ## Conventions From 7980ead4faec1cec81faf823a80304d346e54fc4 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Wed, 2 Sep 2026 17:59:08 -0600 Subject: [PATCH 2/4] fix(py): resolve tokens against the query as written, and complete the gist Four defects, each reproduced first. Token resolution ran against SQL that earlier expansions had already rewritten, so a definition whose compiled SQL named another table brought that table into scope for a later bare token. Every token now resolves against the query as written, and substitution happens afterwards. Retrieval chunks carried only the kind and type, so a retrieved definition named a token without saying what it expands to. They carry the same gist as the first-touch entry now, which is compiled SQL and notes, never the authored expression. An absent type reached the prompt as the word "None". data-dict omits the type when no single one is inferred, so it is left out rather than printed. pkg-r loses the entire gist in this case, filed as #259. The index cap counted line lengths but not the newlines joining them, so the result could exceed the cap. It measures the joined text. --- pkg-py/src/commons/_definitions/_registry.py | 25 +++++--- pkg-py/tests/test_definitions_registry.py | 65 +++++++++++++++----- 2 files changed, 67 insertions(+), 23 deletions(-) diff --git a/pkg-py/src/commons/_definitions/_registry.py b/pkg-py/src/commons/_definitions/_registry.py index 5b7dde18..83c17b09 100644 --- a/pkg-py/src/commons/_definitions/_registry.py +++ b/pkg-py/src/commons/_definitions/_registry.py @@ -117,9 +117,14 @@ def expand_tokens( if token not in tokens: tokens.append(token) + # Every token resolves against the query as written. Compiled SQL can + # name a table the query does not, and resolving against already-expanded + # text would let that count as the table being present, which is the check + # that keeps a token bound to its own table. + resolved = [(token, _resolve_token(token, sql, records)) for token in tokens] + applied: list[ExportRecord] = [] - for token in tokens: - record = _resolve_token(token, sql, records) + for token, record in resolved: pattern = re.compile(r"\{\{\s*" + re.escape(token) + r"\s*\}\}") # A lambda, not a replacement string: the SQL is literal and a # backslash in it would otherwise be read as an escape. @@ -229,10 +234,10 @@ def scope(record: ExportRecord) -> str: def index_text(registry: Registry, cap_chars: int = INDEX_CAP_CHARS) -> str: """The kind index for the system prompt, truncated to the cap.""" kept: list[str] = [] - used = 0 for line in _index_lines(registry): - used += len(line) - if used > cap_chars: + # Measured as joined, so the newlines between lines count against the + # cap rather than pushing the result past it. + if len("\n".join([*kept, line])) > cap_chars: break kept.append(line) return "\n".join(kept) @@ -240,14 +245,18 @@ def index_text(registry: Registry, cap_chars: int = INDEX_CAP_CHARS) -> str: def index_overflows(registry: Registry, cap_chars: int = INDEX_CAP_CHARS) -> bool: """Whether the index did not fit, so the model is told to search instead.""" - return sum(len(line) for line in _index_lines(registry)) > cap_chars + return len("\n".join(_index_lines(registry))) > cap_chars def _gist(record: ExportRecord) -> str: detail = _flatten_inline( " ".join(part for part in (record.description, record.details) if part) ) - parts = [f"({record.kind}, {record.type})"] + # An absent type is left out rather than printed: data-dict omits it when + # no single type is inferred, and the language's null spelling would + # otherwise reach the model as a word. + scope = f"{record.kind}, {record.type}" if record.type else record.kind + parts = [f"({scope})"] if detail: parts.append(detail) parts.append(f"Selected {record.target}: `({_flatten_inline(record.sql)})`.") @@ -276,7 +285,7 @@ def context_chunks(records: list[ExportRecord]) -> list[str]: """One retrievable chunk per definition.""" return [ f"Governed definition `{{{{{record.name}}}}}` on table `{record.table}` " - f"({record.kind}, {record.type})" + f"{_gist(record)}" for record in records ] diff --git a/pkg-py/tests/test_definitions_registry.py b/pkg-py/tests/test_definitions_registry.py index 02a709eb..158d891a 100644 --- a/pkg-py/tests/test_definitions_registry.py +++ b/pkg-py/tests/test_definitions_registry.py @@ -238,9 +238,27 @@ def test_first_touch_is_none_without_definitions() -> None: def test_definitions_are_indexed_as_context_chunks() -> None: - assert context_chunks([record("net_revenue")]) == [ + # A retrieved chunk has to say what the token expands to, or retrieval + # surfaces a name the model cannot use. + chunks = context_chunks([record("net_revenue", sql="sum(revenue)")]) + + assert len(chunks) == 1 + assert chunks[0].startswith( "Governed definition `{{net_revenue}}` on table `sales` (metric, number)" - ] + ) + assert "Selected SQL(duckdb): `(sum(revenue))`." in chunks[0] + + +def test_a_context_chunk_shows_compiled_sql_and_not_the_expression() -> None: + chunks = context_chunks([record("net_revenue", sql="sum(revenue)")]) + + assert "SUM(revenue)" not in chunks[0] + + +def test_a_context_chunk_carries_translation_notes() -> None: + chunks = context_chunks([record("remainder", notes=["modulus by zero differs"])]) + + assert "Translation notes: modulus by zero differs" in chunks[0] def test_applied_text_reports_the_sql_each_token_became() -> None: @@ -385,22 +403,18 @@ def test_first_touch_renders_every_fixture_definition() -> None: assert f"`{{{{{item.name}}}}}`" in text, item.name -def test_a_typeless_definition_renders_its_missing_type_as_a_placeholder() -> None: - # data-dict omits `type` when no single one is inferred, so the record - # carries None and it reaches the prompt as the string "None". pkg-r puts - # "NA" in the same slot from its own missing value. Pinned as it stands - # rather than improved here, because this text is a shared contract and - # changing one implementation alone is the drift the fixtures exist to - # prevent. +def test_a_typeless_definition_omits_the_type_rather_than_naming_it() -> None: + # data-dict omits `type` when no single one is inferred. Printing the + # language's null spelling would put "None" in the prompt, so the type is + # left out and the rest of the gist still reaches the model. typeless = [item for item in fixture_records() if item.type is None] assert len(typeless) == 1 - assert context_chunks(typeless) == [ - ( - "Governed definition `{{mixed temporal case}}` on table `survey` " - "(derived, None)" - ) - ] + chunk = context_chunks(typeless)[0] + + assert "None" not in chunk + assert "(derived)" in chunk + assert "Selected SQL(duckdb)" in chunk def test_grain_metadata_reaches_the_records() -> None: @@ -408,3 +422,24 @@ def test_grain_metadata_reaches_the_records() -> None: assert mixed assert not all(item.mixed_grain for item in fixture_records()) + + +def test_an_expansion_cannot_widen_scope_for_a_later_token() -> None: + # Compiled SQL can name a table the query does not. Resolving later tokens + # against already-expanded text would let that count as the table being + # present, which is the check that keeps a token bound to its own table. + records = [ + record("total", table="sales", sql="sum(returns.x)"), + record("other", table="returns", sql="count(*)"), + ] + + with pytest.raises(ValueError, match="does not appear in this query"): + expand_tokens("SELECT {{sales::total}}, {{other}} FROM sales", records) + + +def test_the_index_cap_counts_the_lines_it_joins() -> None: + registry = Registry( + [record(f"d{i}", table=f"t{i}", kind="metric") for i in range(30)] + ) + + assert len(index_text(registry, cap_chars=300)) <= 300 From 13b22a0f38833c15cd1c92a08cdb1d70709bbadc Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Thu, 3 Sep 2026 13:27:10 -0600 Subject: [PATCH 3/4] test(py): run the shared definition-rendering cases against the registry The fixture landed in #265 with only the R suite executing it, and tests/shared/README.md asks for both runners to land with a fixture. The registry exists here, so Python runs the same 13 cases from the same file: which queries expand and to what, the gist, and the index under a cap. Replaces the integrity-only test that stood in for this. Refusals assert the reason the fixture names, through a map to this package's message wording, which is what the R runner does with its own. Every case passes without touching the registry. Checked by perturbing one expansion, one gist and one overflow flag in the fixture and confirming the three matching cases fail. --- pkg-py/tests/test_definition_rendering.py | 76 +++++++++++++++++++ .../test_definition_rendering_fixture.py | 63 --------------- .../fixtures/shared/definition-rendering.json | 2 +- tests/shared/README.md | 1 + tests/shared/definition-rendering.json | 2 +- 5 files changed, 79 insertions(+), 65 deletions(-) create mode 100644 pkg-py/tests/test_definition_rendering.py delete mode 100644 pkg-py/tests/test_definition_rendering_fixture.py diff --git a/pkg-py/tests/test_definition_rendering.py b/pkg-py/tests/test_definition_rendering.py new file mode 100644 index 00000000..c25b8f04 --- /dev/null +++ b/pkg-py/tests/test_definition_rendering.py @@ -0,0 +1,76 @@ +"""The definition-rendering contract both packages consume. + +The R suite runs the same cases from the same file. See tests/shared/README.md. +""" + +import pytest + +from commons._definitions import ( + ExportRecord, + Registry, + expand_tokens, + index_overflows, + index_text, +) +from commons._definitions._registry import _gist + +from ._shared import load_shared_fixture + +SPEC = load_shared_fixture("definition-rendering") +RECORDS = SPEC["records"]["values"] + +# The fixture names why a query must be refused; the wording is this package's. +REFUSAL = { + "table_not_in_query": "does not appear in this query", + "unknown_token": "No governed definition matches", + "ambiguous_token": "is ambiguous here", +} + + +def record(key: str) -> ExportRecord: + return ExportRecord(**RECORDS[key]) + + +def cases(section: str) -> list[dict]: + # An empty section would collect zero cases and the runner would pass. + found = SPEC[section]["cases"] + assert found, section + return found + + +@pytest.mark.parametrize("case", cases("expand_tokens"), ids=lambda c: c["name"]) +def test_expansion_matches_the_shared_contract(case: dict) -> None: + records = [record(key) for key in case["records"]] + + if case["expanded"] is None: + with pytest.raises(ValueError, match=REFUSAL[case["reason"]]): + expand_tokens(case["sql"], records) + return + + sql, applied = expand_tokens(case["sql"], records) + + assert sql == case["expanded"] + assert [found.name for found in applied] == case["applied"] + + +@pytest.mark.parametrize("case", cases("gist"), ids=lambda c: c["name"]) +def test_the_gist_matches_the_shared_contract(case: dict) -> None: + assert _gist(record(case["record"])) == case["expected"] + + +def test_every_record_in_the_bank_is_used() -> None: + used: set[str] = set() + for section in ("expand_tokens", "index"): + for case in cases(section): + used.update(case["records"]) + used.update(case["record"] for case in cases("gist")) + + assert used == set(RECORDS) + + +@pytest.mark.parametrize("case", cases("index"), ids=lambda c: c["name"]) +def test_the_index_matches_the_shared_contract(case: dict) -> None: + registry = Registry([record(key) for key in case["records"]]) + + assert index_text(registry, case["cap_chars"]) == case["text"] + assert index_overflows(registry, case["cap_chars"]) is case["overflows"] diff --git a/pkg-py/tests/test_definition_rendering_fixture.py b/pkg-py/tests/test_definition_rendering_fixture.py deleted file mode 100644 index 9c1ad1ef..00000000 --- a/pkg-py/tests/test_definition_rendering_fixture.py +++ /dev/null @@ -1,63 +0,0 @@ -"""The definition-rendering contract both packages consume. - -This checks the fixture's own integrity: that every section has cases, that -each case names records the bank defines, and that a refusal says why. Running -the cases against an implementation comes with the registry. -""" - -from typing import Any - -from ._shared import load_shared_fixture - -SPEC = load_shared_fixture("definition-rendering") -RECORDS: dict[str, dict[str, Any]] = SPEC["records"]["values"] -SECTIONS = ("expand_tokens", "gist", "index") - - -def test_every_section_has_cases() -> None: - # An empty section would collect zero cases and every runner would pass. - for section in SECTIONS: - assert SPEC[section]["cases"], section - - -def test_every_record_is_used() -> None: - used: set[str] = set() - for section in ("expand_tokens", "index"): - for case in SPEC[section]["cases"]: - used.update(case["records"]) - used.update(case["record"] for case in SPEC["gist"]["cases"]) - - assert used == set(RECORDS) - - -def test_every_named_record_resolves() -> None: - for section in ("expand_tokens", "index"): - for case in SPEC[section]["cases"]: - for key in case["records"]: - assert key in RECORDS, f"{section}: {case['name']}: {key}" - - -def test_refusals_say_why_and_expansions_do_not() -> None: - for case in SPEC["expand_tokens"]["cases"]: - if case["expanded"] is None: - assert case["reason"], case["name"] - assert case["applied"] == [], case["name"] - else: - assert "reason" not in case, case["name"] - - -def test_the_index_cases_pin_both_sides_of_the_cap() -> None: - # A fixture where nothing overflows would pass against an implementation - # that never reports overflow. - overflows = {case["overflows"] for case in SPEC["index"]["cases"]} - - assert overflows == {True, False} - - -def test_the_gist_cases_cover_a_typeless_definition() -> None: - # The defect this section exists for: an absent type must not take the - # rest of the gist with it. - typeless = [key for key, record in RECORDS.items() if record["type"] is None] - - assert typeless - assert any(case["record"] in typeless for case in SPEC["gist"]["cases"]) diff --git a/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json b/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json index 7df1d35a..f65bc5aa 100644 --- a/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json +++ b/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json @@ -1,5 +1,5 @@ { - "description": "How both packages expand governed-definition tokens and render definitions into the prompt. The source is tests/shared/definition-rendering.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. The R suite runs these cases against the implementation; the Python suite checks the fixture's integrity until its registry lands, then runs them too.", + "description": "How both packages expand governed-definition tokens and render definitions into the prompt. The source is tests/shared/definition-rendering.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. Both suites run these cases against their own implementation.", "records": { "description": "Export records keyed by a fixture-local id, in the shape both packages hydrate: pkg-py builds ExportRecord, pkg-r builds a row of the registry data frame. A null scalar is a field data-dict omitted.", "values": { diff --git a/tests/shared/README.md b/tests/shared/README.md index e4d51e24..44b1d522 100644 --- a/tests/shared/README.md +++ b/tests/shared/README.md @@ -31,6 +31,7 @@ The Python suite reads this directory directly. The R suite cannot. `testthat` n - `invalid` — the data-dict problem code each invalid fixture must produce (e.g. `cycle.yaml` must fail with the cycle error, not a generic parse failure). Hand-maintained; the generator preserves it. This fixture does not replace the conformance harness: the harness compares against a real binary, while this pins what both packages agree to consume. +- **Definition expansion and rendering.** `definition-rendering.json` pins what happens to a governed definition after the compiler is done with it: which `{{token}}` queries expand and to what, the one-line gist shown at first touch and in retrieval, and the kind index under a character cap. It carries a bank of export records that each package hydrates into its own shape. A refused query pins the refusal and a reason slug rather than the message, because the wording belongs to each language. - **Trace file naming.** `trace(-[0-9]+)?\.jsonl`, one OTLP envelope per line. ## Conventions diff --git a/tests/shared/definition-rendering.json b/tests/shared/definition-rendering.json index 7df1d35a..f65bc5aa 100644 --- a/tests/shared/definition-rendering.json +++ b/tests/shared/definition-rendering.json @@ -1,5 +1,5 @@ { - "description": "How both packages expand governed-definition tokens and render definitions into the prompt. The source is tests/shared/definition-rendering.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. The R suite runs these cases against the implementation; the Python suite checks the fixture's integrity until its registry lands, then runs them too.", + "description": "How both packages expand governed-definition tokens and render definitions into the prompt. The source is tests/shared/definition-rendering.json; the copy under pkg-r/tests/testthat/fixtures/shared/ is generated by scripts/sync-shared-fixtures.sh. Edit the source and re-run that script. Both suites run these cases against their own implementation.", "records": { "description": "Export records keyed by a fixture-local id, in the shape both packages hydrate: pkg-py builds ExportRecord, pkg-r builds a row of the registry data frame. A null scalar is a field data-dict omitted.", "values": { From 501a7290af15b16cf2b4faefc8db844e97f77e4e Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Thu, 3 Sep 2026 21:40:17 -0600 Subject: [PATCH 4/4] fix(py): flatten prose around newlines only, per the shared contract Self-review found the gist collapsing every whitespace run while the shared contract keeps authored whitespace; the fixture now pins the rule with a double-space case. Also from that pass: - pin the qualified-token refusals (table absent, unknown name) in tests/shared/definition-rendering.json, synced copy included - restore the fixture-integrity guards dropped with test_definition_rendering_fixture.py: both sides of the index cap and a typeless gist case - cover the qualified and legacy-dotted refusal paths and multi-source build_registry/for_source - build_registry: tolerate tables = None instead of crashing past the getattr default - correct stale docstrings: the compiler is not present yet, and the registry now runs the definitions.json records --- pkg-py/src/commons/_data_dictionary.py | 3 +- pkg-py/src/commons/_definitions/__init__.py | 2 +- pkg-py/src/commons/_definitions/_registry.py | 6 ++- pkg-py/tests/test_definition_rendering.py | 16 ++++++ pkg-py/tests/test_definitions_fixture.py | 5 +- pkg-py/tests/test_definitions_registry.py | 52 +++++++++++++++++++ .../fixtures/shared/definition-rendering.json | 47 ++++++++++++++++- tests/shared/definition-rendering.json | 47 ++++++++++++++++- 8 files changed, 170 insertions(+), 8 deletions(-) diff --git a/pkg-py/src/commons/_data_dictionary.py b/pkg-py/src/commons/_data_dictionary.py index 796a3306..de1402a4 100644 --- a/pkg-py/src/commons/_data_dictionary.py +++ b/pkg-py/src/commons/_data_dictionary.py @@ -100,7 +100,8 @@ class Table(_Permissive): columns: dict[str, Column] = {} definitions: dict[str, Definition] = {} # Attached by _definitions at data-source construction; empty until then, - # so the registry can be exercised without the compiler. + # so the registry can be exercised without the compiler. Elements are + # ExportRecord, typed Any so this model need not import _definitions. compiled_definitions: list[Any] = [] @model_validator(mode="before") diff --git a/pkg-py/src/commons/_definitions/__init__.py b/pkg-py/src/commons/_definitions/__init__.py index 58602fe4..b30e236e 100644 --- a/pkg-py/src/commons/_definitions/__init__.py +++ b/pkg-py/src/commons/_definitions/__init__.py @@ -1,4 +1,4 @@ -"""Governed definitions: the registry, and the compiler that feeds it. +"""Governed definitions: the registry, and the compiler that will feed it. Definitions are authored in data-dict's expression language, not in the SQL dialect of the attached source, so they are type-checked against the diff --git a/pkg-py/src/commons/_definitions/_registry.py b/pkg-py/src/commons/_definitions/_registry.py index 83c17b09..1434803f 100644 --- a/pkg-py/src/commons/_definitions/_registry.py +++ b/pkg-py/src/commons/_definitions/_registry.py @@ -83,7 +83,7 @@ def build_registry(sources: dict[str, Any]) -> Registry: dictionary = getattr(source, "dictionary", None) if dictionary is None: continue - exposed = set(getattr(source, "tables", [])) + exposed = set(getattr(source, "tables", None) or []) for table, entry in dictionary.tables.items(): compiled = getattr(entry, "compiled_definitions", None) or [] if not compiled: @@ -201,7 +201,9 @@ def _abort_unknown(token: str, records: list[ExportRecord]) -> None: def _flatten_inline(text: str) -> str: - return re.sub(r"\s+", " ", text).strip() + # Collapse each newline and the whitespace around it; other whitespace + # stays as authored. tests/shared/definition-rendering.json pins this. + return re.sub(r"\s*\n\s*", " ", text).strip() def _index_lines(registry: Registry) -> list[str]: diff --git a/pkg-py/tests/test_definition_rendering.py b/pkg-py/tests/test_definition_rendering.py index c25b8f04..35f89fd8 100644 --- a/pkg-py/tests/test_definition_rendering.py +++ b/pkg-py/tests/test_definition_rendering.py @@ -68,6 +68,22 @@ def test_every_record_in_the_bank_is_used() -> None: assert used == set(RECORDS) +def test_the_index_cases_pin_both_sides_of_the_cap() -> None: + # A fixture where nothing overflows would pass against an implementation + # that never reports overflow. + overflows = {case["overflows"] for case in cases("index")} + + assert overflows == {True, False} + + +def test_the_gist_cases_cover_a_typeless_definition() -> None: + # The defect this section exists for: an absent type must not take the + # rest of the gist with it. + typeless = [key for key, found in RECORDS.items() if found["type"] is None] + + assert any(case["record"] in typeless for case in cases("gist")) + + @pytest.mark.parametrize("case", cases("index"), ids=lambda c: c["name"]) def test_the_index_matches_the_shared_contract(case: dict) -> None: registry = Registry([record(key) for key in case["records"]]) diff --git a/pkg-py/tests/test_definitions_fixture.py b/pkg-py/tests/test_definitions_fixture.py index 5da0b1ee..42cc0ce9 100644 --- a/pkg-py/tests/test_definitions_fixture.py +++ b/pkg-py/tests/test_definitions_fixture.py @@ -1,8 +1,9 @@ """The definitions contract both packages consume. This checks the fixture's own integrity: that it covers the corpus, that its -sections agree with each other, and that it is not empty. Running the cases -against an implementation comes with the registry and the compiler. +sections agree with each other, and that it is not empty. +test_definitions_registry.py runs the export records against the registry; +running them against the compiler's own output comes with the compiler. """ from typing import Any diff --git a/pkg-py/tests/test_definitions_registry.py b/pkg-py/tests/test_definitions_registry.py index 158d891a..ab02a87e 100644 --- a/pkg-py/tests/test_definitions_registry.py +++ b/pkg-py/tests/test_definitions_registry.py @@ -137,6 +137,30 @@ def test_a_dotted_token_is_read_as_table_qualified() -> None: assert sql == "SELECT (sum(revenue)) FROM sales" +def test_a_qualified_token_whose_table_is_absent_from_the_query_errors() -> None: + with pytest.raises(ValueError, match="does not appear in this query"): + expand_tokens( + "SELECT {{returns::total}} FROM sales", [record("total", table="returns")] + ) + + +def test_an_unknown_qualified_token_lists_what_is_available() -> None: + with pytest.raises(ValueError, match="No governed definition matches"): + expand_tokens("SELECT {{sales::nope}} FROM sales", [record("total")]) + + +def test_an_unknown_dotted_token_lists_what_is_available() -> None: + with pytest.raises(ValueError, match="No governed definition matches"): + expand_tokens("SELECT {{sales.nope}} FROM sales", [record("total")]) + + +def test_a_dotted_token_whose_table_is_absent_from_the_query_errors() -> None: + with pytest.raises(ValueError, match="does not appear in this query"): + expand_tokens( + "SELECT {{other.total}} FROM sales", [record("total", table="other")] + ) + + def test_a_definition_name_may_contain_spaces() -> None: sql, _ = expand_tokens( "SELECT {{net revenue}} FROM sales", [record("net revenue", sql="sum(x)")] @@ -327,6 +351,34 @@ def test_an_empty_source_set_builds_an_empty_registry() -> None: assert build_registry({}).records == [] +def test_the_registry_merges_sources_and_for_source_filters_by_label() -> None: + from commons import _duckdb + from commons._backends import DuckDBBackend + from commons._data_dictionary import DataDictionary, Table + from commons._data_source import DataSource + from commons._definitions import build_registry + + def source_with(definition_name: str) -> DataSource: + dictionary = DataDictionary(tables={"sales": Table()}) + dictionary.tables["sales"].compiled_definitions = [ + record(definition_name, source="") + ] + return DataSource( + backend=DuckDBBackend(_duckdb.connect()), + tables=["sales"], + dictionary=dictionary, + ) + + registry = build_registry( + {"one": source_with("total"), "two": source_with("count_all")} + ) + + assert sorted(item.source for item in registry.records) == ["one", "two"] + assert [item.name for item in registry.for_source("one")] == ["total"] + assert [item.name for item in registry.for_source("two")] == ["count_all"] + assert len(registry.for_source()) == 2 + + # ---- against the shared contract ----------------------------------------- # # The first Python code to consume the definitions contract. The compiler does diff --git a/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json b/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json index f65bc5aa..2d5fe68d 100644 --- a/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json +++ b/pkg-r/tests/testthat/fixtures/shared/definition-rendering.json @@ -116,6 +116,26 @@ "target": "SQL(duckdb)", "notes": [], "mixed_grain": false + }, + "sales_prose": { + "name": "region_list", + "table": "sales", + "source": "warehouse", + "kind": "derived", + "type": "string", + "expression": "CONCAT(region, country)", + "label": null, + "description": "Region and country.\n Kept verbatim.", + "details": null, + "columns": [ + "region", + "country" + ], + "definitions": [], + "sql": "concat(\"region\", country)", + "target": "SQL(duckdb)", + "notes": [], + "mixed_grain": false } } }, @@ -178,6 +198,26 @@ "applied": [], "reason": "unknown_token" }, + { + "name": "a qualified token whose table is absent is refused", + "records": [ + "returns_other" + ], + "sql": "SELECT {{returns::other}} FROM sales", + "expanded": null, + "applied": [], + "reason": "table_not_in_query" + }, + { + "name": "a qualified token no definition matches is refused", + "records": [ + "sales_total" + ], + "sql": "SELECT {{sales::nope}} FROM sales", + "expanded": null, + "applied": [], + "reason": "unknown_token" + }, { "name": "a bare token defined on two tables in the query is refused", "records": [ @@ -205,7 +245,7 @@ }, "gist": { "description": "The one-line summary rendered for a definition, used in the first-touch entry and in every retrieval chunk. Shows the compiled SQL, never the authored expression.", - "rule": "`(kind, type)` when data-dict inferred a type, `(kind)` alone when it did not. The rest of the gist is unaffected by an absent type.", + "rule": "`(kind, type)` when data-dict inferred a type, `(kind)` alone when it did not. The rest of the gist is unaffected by an absent type. Multi-line prose flattens onto one line by collapsing each newline and the whitespace around it; other whitespace stays as authored.", "cases": [ { "name": "kind and type, with a description", @@ -221,6 +261,11 @@ "name": "an absent type is left out and the rest of the gist survives", "record": "survey_mixed_temporal", "expected": "(derived) When the response was captured. Selected SQL(duckdb): `(CASE WHEN \"done\" THEN \"taken_on\" ELSE \"taken_at\" END)`." + }, + { + "name": "newlines flatten but authored whitespace survives", + "record": "sales_prose", + "expected": "(derived, string) Region and country. Kept verbatim. Selected SQL(duckdb): `(concat(\"region\", country))`." } ] }, diff --git a/tests/shared/definition-rendering.json b/tests/shared/definition-rendering.json index f65bc5aa..2d5fe68d 100644 --- a/tests/shared/definition-rendering.json +++ b/tests/shared/definition-rendering.json @@ -116,6 +116,26 @@ "target": "SQL(duckdb)", "notes": [], "mixed_grain": false + }, + "sales_prose": { + "name": "region_list", + "table": "sales", + "source": "warehouse", + "kind": "derived", + "type": "string", + "expression": "CONCAT(region, country)", + "label": null, + "description": "Region and country.\n Kept verbatim.", + "details": null, + "columns": [ + "region", + "country" + ], + "definitions": [], + "sql": "concat(\"region\", country)", + "target": "SQL(duckdb)", + "notes": [], + "mixed_grain": false } } }, @@ -178,6 +198,26 @@ "applied": [], "reason": "unknown_token" }, + { + "name": "a qualified token whose table is absent is refused", + "records": [ + "returns_other" + ], + "sql": "SELECT {{returns::other}} FROM sales", + "expanded": null, + "applied": [], + "reason": "table_not_in_query" + }, + { + "name": "a qualified token no definition matches is refused", + "records": [ + "sales_total" + ], + "sql": "SELECT {{sales::nope}} FROM sales", + "expanded": null, + "applied": [], + "reason": "unknown_token" + }, { "name": "a bare token defined on two tables in the query is refused", "records": [ @@ -205,7 +245,7 @@ }, "gist": { "description": "The one-line summary rendered for a definition, used in the first-touch entry and in every retrieval chunk. Shows the compiled SQL, never the authored expression.", - "rule": "`(kind, type)` when data-dict inferred a type, `(kind)` alone when it did not. The rest of the gist is unaffected by an absent type.", + "rule": "`(kind, type)` when data-dict inferred a type, `(kind)` alone when it did not. The rest of the gist is unaffected by an absent type. Multi-line prose flattens onto one line by collapsing each newline and the whitespace around it; other whitespace stays as authored.", "cases": [ { "name": "kind and type, with a description", @@ -221,6 +261,11 @@ "name": "an absent type is left out and the rest of the gist survives", "record": "survey_mixed_temporal", "expected": "(derived) When the response was captured. Selected SQL(duckdb): `(CASE WHEN \"done\" THEN \"taken_on\" ELSE \"taken_at\" END)`." + }, + { + "name": "newlines flatten but authored whitespace survives", + "record": "sales_prose", + "expected": "(derived, string) Region and country. Kept verbatim. Selected SQL(duckdb): `(concat(\"region\", country))`." } ] },