From d10baaeb2edebcca5627acd00fb374d6707f4290 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 14:51:56 -0500 Subject: [PATCH 01/11] feat!: align data dictionaries with the current data-dict spec (#316) DataDict files now follow the current data-dict spec (https://data-dict.tidyverse.org/spec.html): - tables is a list of tables each with a name field (previously a mapping of name to definition) - column range is a two-element [min, max] list (either end may be null) - values may be a value-to-label map ({M: Male, F: Female}) - constraints may include assertion objects ({assert, language, description}) pkg-py parses the spec natively via pydantic models and rejects legacy mapping-form files with a descriptive migration error. pkg-r stays validation-free, keying the table list by name via dd_tables_by_name() at existing consumption sites. Docs, examples, vignettes, and changelogs updated in both packages. --- pkg-py/CHANGELOG.md | 2 + pkg-py/docs/context.qmd | 10 +- pkg-py/src/querychat/_data_dict.py | 137 +++++++++++---- pkg-py/src/querychat/_system_prompt.py | 4 +- pkg-py/src/querychat/tools.py | 2 +- pkg-py/tests/test_base.py | 14 +- pkg-py/tests/test_data_dict.py | 161 ++++++++++++++---- pkg-py/tests/test_multi_table.py | 22 ++- pkg-py/tests/test_querychat.py | 6 +- pkg-py/tests/test_system_prompt.py | 24 +-- pkg-py/tests/test_tools.py | 16 +- pkg-r/NEWS.md | 2 + pkg-r/R/DBISource.R | 27 ++- pkg-r/R/DataDict.R | 26 ++- pkg-r/R/QueryChatSystemPrompt.R | 10 +- pkg-r/R/querychat_tools.R | 5 +- .../nutrition-data-dict.yaml | 14 +- pkg-r/tests/testthat/test-DataDict.R | 53 ++++-- pkg-r/tests/testthat/test-QueryChat.R | 6 +- pkg-r/tests/testthat/test-querychat_tools.R | 5 +- pkg-r/vignettes/context.Rmd | 10 +- 21 files changed, 401 insertions(+), 155 deletions(-) diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index 59095ab16..6cec56df5 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Bug fixes +* `DataDict` now follows the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` is a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, `values` may be a value-to-label map, and `constraints` may include assertion objects. Files written in the older mapping format fail with a descriptive error explaining how to update. (#316) + * JSON pins now fall back to `pin_read()` when DuckDB's JSON extension cannot be installed or loaded, including on builds that disable extension loading. Native DuckDB JSON reading is retained when the extension is available. * Fixed SQL code blocks (the visualization footer's "Show Query" editor and the data drawer's SQL editor) rendering illegibly in dark mode — light syntax-highlighted text on a forced light background. The code panel background now adapts to the app's color mode. (#329) diff --git a/pkg-py/docs/context.qmd b/pkg-py/docs/context.qmd index 123335dc0..f6d63e07c 100644 --- a/pkg-py/docs/context.qmd +++ b/pkg-py/docs/context.qmd @@ -30,13 +30,13 @@ qc.add_table(customers, "customers") ### Format -A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. +The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. ```{.yaml filename="data-dict.yaml"} -version: "0.2.0" +$version: "0.1.0" tables: - orders: + - name: orders description: One row per customer order. columns: - name: order_id @@ -55,7 +55,7 @@ tables: values: [pending, shipped, delivered, cancelled] description: Current order status. - customers: + - name: customers description: One row per customer. columns: - name: id @@ -81,7 +81,7 @@ glossary: #### Tables -Each entry under `tables` describes one table. The key must match the table name you pass to `QueryChat` or `.add_table()`. +Each entry under `tables` describes one table. Its `name` must match the table name you pass to `QueryChat` or `.add_table()`. - **`description`**: What this table represents (one sentence is usually enough). - **`columns`**: A list of column annotations. Each column can have: diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index 0baa589e4..35fee8b1a 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -1,38 +1,63 @@ from __future__ import annotations from pathlib import Path -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, Literal -from pydantic import BaseModel +from pydantic import BaseModel, Field, field_validator, model_validator if TYPE_CHECKING: from ._datasource import ColumnMeta from ._query_executor import QueryExecutor -class ColumnRange(BaseModel): - """Inclusive numeric range for a column, used instead of live min/max queries.""" +class ConstraintAssertion(BaseModel): + """ + A column constraint expressed as a boolean expression. + + In the data-dict spec, entries in a column's ``constraints`` list are either + barewords (``primary_key``, ``foreign_key``, ``required``, ``unique``) or + assertion objects with an ``assert`` expression. + + Parameters + ---------- + assert_ + The boolean expression (e.g. ``"amount >= 0"``). Populated from the + YAML key ``assert``. + language + The language ``assert`` is written in (``sql``, ``r``, or ``python``). + description + Human-readable explanation of the assertion. - min: Any = None - max: Any = None + """ + + assert_: str = Field(alias="assert") + language: Literal["sql", "r", "python"] | None = None + description: str | None = None + + def __str__(self) -> str: + if self.description: + return f"{self.assert_} ({self.description})" + return self.assert_ class ColumnSpec(BaseModel): """ Per-column metadata entry in a :class:`DataDict`. - All fields are optional. Only ``name`` is required, and is used to match - this spec against columns returned by the data source. + Follows the column format of the `data-dict spec + `_. Only ``name`` is required; + it is used to match this spec against columns returned by the data source. Parameters ---------- name Column name as it appears in the data source. type - Human-readable type override (e.g. ``"date"``, ``"currency"``). When + Type override (e.g. ``"date"``, ``"number(quantity)"``). When supplied, this replaces the inferred SQL type in the LLM schema view. constraints - Free-text constraints conveyed to the LLM (e.g. ``"non-negative"``). + Bareword constraints (``primary_key``, ``required``, ...) and/or + :class:`ConstraintAssertion` objects, conveyed to the LLM. description Short description forwarded verbatim to the LLM's schema view. details @@ -41,12 +66,14 @@ class ColumnSpec(BaseModel): units Unit label (e.g. ``"kg"``, ``"USD"``), included in the schema view. values - Exhaustive list of valid values. Replaces categorical inference for - this column — querychat will not query the data source for distinct - values when this is set. + Valid values for the column, either a list (``[M, F, U]``) or a + value-to-label map (``{M: Male, F: Female}``). Replaces categorical + inference for this column — querychat will not query the data source + for distinct values when this is set. For the map form, the values + (keys) are used. range - Inclusive min/max bounds. Replaces live min/max statistics queries - when set. + Inclusive ``[min, max]`` bounds; either end may be ``None`` for an + open range. Replaces live min/max statistics queries when set. examples Representative sample values shown to the LLM as context. @@ -54,14 +81,23 @@ class ColumnSpec(BaseModel): name: str type: str | None = None - constraints: list[str] = [] + constraints: list[str | ConstraintAssertion] = [] description: str | None = None details: str | None = None units: str | None = None - values: list[Any] | None = None - range: ColumnRange | None = None + values: list[Any] | dict[str, str] | None = None + range: list[Any] | None = None examples: list[Any] | None = None + @field_validator("range") + @classmethod + def _range_is_two_elements(cls, v: list[Any] | None) -> list[Any] | None: + if v is not None and len(v) != 2: # noqa: PLR2004 + raise ValueError( + "range must be a two-element list [min, max]; use null for an open end" + ) + return v + class TableSpec(BaseModel): """ @@ -69,6 +105,8 @@ class TableSpec(BaseModel): Parameters ---------- + name + Table name. Must match a table registered with ``QueryChat``. description Short description of the table, forwarded to the LLM's schema view. details @@ -80,6 +118,7 @@ class TableSpec(BaseModel): """ + name: str description: str | None = None details: str | None = None columns: list[ColumnSpec] = [] @@ -105,13 +144,31 @@ class RelationshipSpec(BaseModel): join: str +_LEGACY_TABLES_MESSAGE = """\ +This data dictionary uses an outdated format: `tables` must be a list of +tables, each with a `name` field — not a mapping of name to definition. + + # before # after + tables: tables: + orders: - name: orders + columns: [...] columns: [...] + +Also changed in the current spec: column `range` is a two-element list +[min, max], and `values` may be a value-to-label map ({M: Male, F: Female}). + +Run `data-dict validate-spec ` to check the file against the spec: +https://data-dict.tidyverse.org/spec.html +""" + + class DataDict(BaseModel): """ A data dictionary providing rich per-table and per-column metadata. Pass a ``DataDict`` to ``QueryChat`` (or load one from YAML via :meth:`from_yaml`) to give the LLM better context about your data without - querying the data source for statistics at startup. + querying the data source for statistics at startup. Files must follow the + current `data-dict spec `_. For columns listed in a ``DataDict``: @@ -132,9 +189,8 @@ class DataDict(BaseModel): One-line summary of the domain, shown alongside ``name`` in the system prompt. tables - Per-table metadata, keyed by table name. Each value is a - :class:`TableSpec` with optional description and column specs. - Table names must match those registered with ``QueryChat``. + Per-table metadata. Each :class:`TableSpec` has a required ``name`` + that must match a table registered with ``QueryChat``. relationships Cross-table relationship declarations. Useful context for multi-table apps where the LLM needs to know how tables join. @@ -149,23 +205,35 @@ class DataDict(BaseModel): ```python from querychat import QueryChat, DataDict - qc = QueryChat(df, "sales", data_dict=DataDict.from_yaml("data_dict.yaml")) + qc = QueryChat(df, "sales", data_dict=DataDict.from_yaml("data-dict.yaml")) ``` Or pass a path directly and let QueryChat load it: ```python - qc = QueryChat(df, "sales", data_dict="data_dict.yaml") + qc = QueryChat(df, "sales", data_dict="data-dict.yaml") ``` """ name: str | None = None description: str | None = None - tables: dict[str, TableSpec] = {} + tables: list[TableSpec] = [] relationships: list[RelationshipSpec] = [] glossary: dict[str, str] = {} + @model_validator(mode="before") + @classmethod + def _reject_legacy_tables(cls, data: Any) -> Any: + if isinstance(data, dict) and isinstance(data.get("tables"), dict): + # ValueError (not TypeError) so pydantic wraps it in ValidationError + raise ValueError(_LEGACY_TABLES_MESSAGE) # noqa: TRY004 + return data + + def get_table(self, table_name: str) -> TableSpec | None: + """Return the :class:`TableSpec` for ``table_name``, or ``None``.""" + return next((t for t in self.tables if t.name == table_name), None) + def to_prompt_dict(self) -> dict[str, Any]: """Return a filtered dict for the system prompt (excludes per-column details).""" result: dict[str, Any] = {} @@ -175,8 +243,8 @@ def to_prompt_dict(self) -> dict[str, Any]: result["description"] = self.description if self.tables: result["tables"] = { - name: ({"description": spec.description} if spec.description else None) - for name, spec in self.tables.items() + t.name: ({"description": t.description} if t.description else None) + for t in self.tables } if self.relationships: result["relationships"] = [ @@ -197,7 +265,7 @@ def get_table_schema( metas: list[ColumnMeta] = executor.get_column_metas(table_name) # Build lookup from data_dict for this table - table_spec = self.tables.get(table_name) + table_spec = self.get_table(table_name) documented: dict[str, ColumnSpec] = ( {col.name: col for col in table_spec.columns} if table_spec else {} ) @@ -209,16 +277,21 @@ def get_table_schema( if spec.type is not None: meta.sql_type = spec.type if spec.range is not None: - meta.min_val = spec.range.min - meta.max_val = spec.range.max + meta.min_val = spec.range[0] + meta.max_val = spec.range[1] if spec.values is not None: - meta.categories = [str(v) for v in spec.values] + vals = ( + list(spec.values) + if isinstance(spec.values, dict) + else spec.values + ) + meta.categories = [str(v) for v in vals] if spec.description is not None: meta.description = spec.description if spec.units is not None: meta.units = spec.units if spec.constraints: - meta.constraints = list(spec.constraints) + meta.constraints = [str(c) for c in spec.constraints] else: undocumented.append(meta) diff --git a/pkg-py/src/querychat/_system_prompt.py b/pkg-py/src/querychat/_system_prompt.py index b53626827..1e9f1b607 100644 --- a/pkg-py/src/querychat/_system_prompt.py +++ b/pkg-py/src/querychat/_system_prompt.py @@ -50,7 +50,7 @@ def __init__( self._data_dicts = [ dd for dd in self._data_dicts - if any(n in resolved for n in dd.tables) + if any(t.name in resolved for t in dd.tables) or (not dd.tables and dd.description) ] update: dict[str, object] = {} @@ -119,7 +119,7 @@ def escape_attr(val: str) -> str: d.pop("name", None) d.pop("description", None) - claimed = {n for n in self._data_sources if n in dd.tables} + claimed = {t.name for t in dd.tables if t.name in self._data_sources} all_claimed.update(claimed) if "tables" in d: d["tables"] = { diff --git a/pkg-py/src/querychat/tools.py b/pkg-py/src/querychat/tools.py index a371e5e8b..8cfc5d878 100644 --- a/pkg-py/src/querychat/tools.py +++ b/pkg-py/src/querychat/tools.py @@ -61,7 +61,7 @@ def get_schema(table_name: str) -> ContentToolResult: error = f"Table '{table_name}' not found. Available: {available}" return ContentToolResult(value=error, error=Exception(error)) - dd = next((d for d in data_dicts if table_name in d.tables), None) + dd = next((d for d in data_dicts if d.get_table(table_name) is not None), None) if dd is not None: columns = dd.get_table_schema(table_name, executor, categorical_threshold) else: diff --git a/pkg-py/tests/test_base.py b/pkg-py/tests/test_base.py index 11925a6f0..96d7f4a3c 100644 --- a/pkg-py/tests/test_base.py +++ b/pkg-py/tests/test_base.py @@ -282,27 +282,27 @@ def test_cleanup(self, sample_df): class TestDataDict: def test_data_dict_path_accepted(self, tmp_path: Path) -> None: f = tmp_path / "spec.yaml" - f.write_text('version: "0.1.0"\ntables:\n t:\n columns: []\n') + f.write_text('$version: "0.1.0"\ntables:\n - name: t\n columns: []\n') df = pl.DataFrame({"x": [1]}) qc = QueryChat(df, table_name="t", data_dict=str(f)) assert len(qc._data_dicts) == 1 - assert "t" in qc._data_dicts[0].tables + assert qc._data_dicts[0].get_table("t") is not None def test_data_dict_instance_accepted(self) -> None: - dd = DataDict(tables={"t": TableSpec(columns=[])}) + dd = DataDict(tables=[TableSpec(name="t")]) df = pl.DataFrame({"x": [1]}) qc = QueryChat(df, table_name="t", data_dict=dd) assert qc._data_dicts[0] is dd def test_data_dict_table_not_in_dict_is_allowed(self) -> None: - dd = DataDict(tables={"other": TableSpec(columns=[])}) + dd = DataDict(tables=[TableSpec(name="other")]) df = pl.DataFrame({"x": [1]}) qc = QueryChat(df, table_name="t", data_dict=dd) assert "t" in qc._data_sources def test_add_table_not_in_data_dict_is_allowed(self) -> None: dd = DataDict( - tables={"t1": TableSpec(columns=[])}, + tables=[TableSpec(name="t1")], ) df = pl.DataFrame({"x": [1]}) qc = QueryChat(df, table_name="t1", data_dict=dd) @@ -575,9 +575,7 @@ def test_remove_after_sessions_started_raises(self, sample_df): qc.remove_table("other") assert qc.table_names() == ["users", "other"] - def test_add_tables_replace_after_sessions_started_raises( - self, multi_table_engine - ): + def test_add_tables_replace_after_sessions_started_raises(self, multi_table_engine): qc = QueryChatBase() qc.add_tables(multi_table_engine) qc._sessions_started = True diff --git a/pkg-py/tests/test_data_dict.py b/pkg-py/tests/test_data_dict.py index 0cf62f007..a9399ea9a 100644 --- a/pkg-py/tests/test_data_dict.py +++ b/pkg-py/tests/test_data_dict.py @@ -2,16 +2,18 @@ import narwhals.stable.v1 as nw import polars as pl -from querychat._data_dict import ColumnRange, ColumnSpec, DataDict, TableSpec +import pytest +from pydantic import ValidationError +from querychat._data_dict import ColumnSpec, DataDict, TableSpec from querychat._datasource import DataFrameSource from querychat._query_executor import DataSourceExecutor def test_from_yaml_full_spec(tmp_path: Path) -> None: yaml_content = """\ -version: "0.1.0" +$version: "0.1.0" tables: - orders: + - name: orders description: One row per order. columns: - name: order_id @@ -21,12 +23,19 @@ def test_from_yaml_full_spec(tmp_path: Path) -> None: examples: [1, 2, 3] - name: amount type: number(quantity) - range: - min: 0 - max: 10000 + range: [0, 10000] - name: status type: enum values: [pending, shipped, delivered] + - name: channel + type: enum + values: {W: Web, R: Retail} + - name: discount + type: number(quantity) + constraints: + - assert: discount >= 0 + language: sql + description: Non-negative discount relationships: - description: Order placed by customer. cardinality: many-to-one @@ -38,23 +47,25 @@ def test_from_yaml_full_spec(tmp_path: Path) -> None: f.write_text(yaml_content) dd = DataDict.from_yaml(f) - assert "orders" in dd.tables - t = dd.tables["orders"] + assert [t.name for t in dd.tables] == ["orders"] + t = dd.tables[0] assert t.description == "One row per order." - assert len(t.columns) == 3 + assert len(t.columns) == 5 col = t.columns[0] assert col.name == "order_id" assert col.description == "Unique order identifier." assert col.constraints == ["primary_key"] - col = t.columns[1] - assert col.range is not None - assert col.range.min == 0 - assert col.range.max == 10000 + assert t.columns[1].range == [0, 10000] + assert t.columns[2].values == ["pending", "shipped", "delivered"] + assert t.columns[3].values == {"W": "Web", "R": "Retail"} - col = t.columns[2] - assert col.values == ["pending", "shipped", "delivered"] + constraint = t.columns[4].constraints[0] + assert not isinstance(constraint, str) + assert constraint.assert_ == "discount >= 0" + assert constraint.language == "sql" + assert constraint.description == "Non-negative discount" assert len(dd.relationships) == 1 assert dd.relationships[0].join == "orders.customer_id = customers.id" @@ -62,20 +73,39 @@ def test_from_yaml_full_spec(tmp_path: Path) -> None: assert dd.glossary["churn"] == "Customer with no orders in 90+ days." +def test_from_yaml_legacy_dict_tables_error(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text("tables:\n orders:\n columns: []\n") + with pytest.raises(ValidationError, match="outdated format"): + DataDict.from_yaml(f) + + +def test_table_requires_name(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text("tables:\n - description: No name here\n columns: []\n") + with pytest.raises(ValidationError): + DataDict.from_yaml(f) + + +def test_range_must_have_two_elements() -> None: + with pytest.raises(ValidationError, match="two-element"): + ColumnSpec(name="x", range=[0, 1, 2]) + + def test_from_yaml_partial_spec(tmp_path: Path) -> None: f = tmp_path / "spec.yaml" - f.write_text('version: "0.1.0"\ntables:\n orders:\n columns: []\n') + f.write_text("tables:\n - name: orders\n columns: []\n") dd = DataDict.from_yaml(f) - assert dd.tables["orders"].description is None + assert dd.tables[0].description is None assert dd.relationships == [] assert dd.glossary == {} def test_from_yaml_str_path(tmp_path: Path) -> None: f = tmp_path / "spec.yaml" - f.write_text('version: "0.1.0"\n') + f.write_text('$version: "0.1.0"\n') dd = DataDict.from_yaml(str(f)) - assert dd.tables == {} + assert dd.tables == [] def test_name_and_description_are_optional() -> None: @@ -86,7 +116,7 @@ def test_name_and_description_are_optional() -> None: def test_from_yaml_derives_name_from_file_stem(tmp_path: Path) -> None: f = tmp_path / "my_schema.yaml" - f.write_text('version: "0.1.0"\n') + f.write_text('$version: "0.1.0"\n') dd = DataDict.from_yaml(f) assert dd.name == "my_schema" @@ -107,7 +137,7 @@ def test_from_yaml_loads_description(tmp_path: Path) -> None: def test_from_yaml_description_defaults_to_none(tmp_path: Path) -> None: f = tmp_path / "spec.yaml" - f.write_text('version: "0.1.0"\n') + f.write_text('$version: "0.1.0"\n') dd = DataDict.from_yaml(f) assert dd.description is None @@ -119,14 +149,15 @@ def _make_executor(df: pl.DataFrame, table_name: str) -> DataSourceExecutor: def test_get_table_schema_all_documented() -> None: dd = DataDict( - tables={ - "orders": TableSpec( + tables=[ + TableSpec( + name="orders", description="Order records.", columns=[ ColumnSpec( name="amount", description="Order total in USD.", - range=ColumnRange(min=0, max=500), + range=[0, 500], ), ColumnSpec( name="status", @@ -134,7 +165,7 @@ def test_get_table_schema_all_documented() -> None: ), ], ) - }, + ], ) df = pl.DataFrame({"amount": [10, 20], "status": ["pending", "shipped"]}) executor = _make_executor(df, "orders") @@ -149,7 +180,7 @@ def test_get_table_schema_all_documented() -> None: def test_get_table_schema_no_documentation() -> None: - dd = DataDict(tables={"orders": TableSpec(columns=[])}) + dd = DataDict(tables=[TableSpec(name="orders", columns=[])]) df = pl.DataFrame({"amount": [10, 20, 30], "status": ["a", "b", "a"]}) executor = _make_executor(df, "orders") cols = dd.get_table_schema("orders", executor, categorical_threshold=10) @@ -161,13 +192,14 @@ def test_get_table_schema_no_documentation() -> None: def test_get_table_schema_mixed_coverage() -> None: dd = DataDict( - tables={ - "orders": TableSpec( + tables=[ + TableSpec( + name="orders", columns=[ - ColumnSpec(name="amount", range=ColumnRange(min=0, max=999)), - ] + ColumnSpec(name="amount", range=[0, 999]), + ], ) - }, + ], ) df = pl.DataFrame({"amount": [10, 20], "status": ["a", "b"]}) executor = _make_executor(df, "orders") @@ -178,28 +210,83 @@ def test_get_table_schema_mixed_coverage() -> None: assert "status" in col_map # from SQL fallback +def test_get_table_schema_open_ended_range() -> None: + dd = DataDict( + tables=[ + TableSpec( + name="orders", + columns=[ColumnSpec(name="amount", range=[0, None])], + ) + ], + ) + df = pl.DataFrame({"amount": [10, 20]}) + executor = _make_executor(df, "orders") + cols = dd.get_table_schema("orders", executor, categorical_threshold=10) + assert cols[0].min_val == 0 + assert cols[0].max_val is None + + +def test_get_table_schema_values_map_uses_keys() -> None: + dd = DataDict( + tables=[ + TableSpec( + name="orders", + columns=[ColumnSpec(name="status", values={"W": "Web", "R": "Retail"})], + ) + ], + ) + df = pl.DataFrame({"status": ["W", "R"]}) + executor = _make_executor(df, "orders") + cols = dd.get_table_schema("orders", executor, categorical_threshold=10) + assert cols[0].categories == ["W", "R"] + + +def test_get_table_schema_assertion_constraints() -> None: + dd = DataDict( + tables=[ + TableSpec( + name="orders", + columns=[ + ColumnSpec( + name="amount", + constraints=[ + {"assert": "amount >= 0", "description": "Non-negative"}, + "required", + ], + ) + ], + ) + ], + ) + df = pl.DataFrame({"amount": [1, 2]}) + executor = _make_executor(df, "orders") + cols = dd.get_table_schema("orders", executor, categorical_threshold=10) + assert cols[0].constraints == ["amount >= 0 (Non-negative)", "required"] + + def test_to_prompt_dict_excludes_column_specs() -> None: dd = DataDict( name="sales", - tables={ - "orders": TableSpec( + tables=[ + TableSpec( + name="orders", description="Order records.", - columns=[ColumnSpec(name="amount", range=ColumnRange(min=0, max=100))], + columns=[ColumnSpec(name="amount", range=[0, 100])], ) - }, + ], ) d = dd.to_prompt_dict() assert "columns" not in (d.get("tables", {}).get("orders") or {}) def test_to_prompt_dict_includes_table_description() -> None: - dd = DataDict(tables={"orders": TableSpec(description="Order records.")}) + dd = DataDict(tables=[TableSpec(name="orders", description="Order records.")]) d = dd.to_prompt_dict() assert d["tables"]["orders"]["description"] == "Order records." def test_to_prompt_dict_table_with_no_description_is_null() -> None: - dd = DataDict(tables={"orders": TableSpec()}) + dd = DataDict(tables=[TableSpec(name="orders")]) d = dd.to_prompt_dict() assert d["tables"]["orders"] is None diff --git a/pkg-py/tests/test_multi_table.py b/pkg-py/tests/test_multi_table.py index 709f65e71..f82eb17ea 100644 --- a/pkg-py/tests/test_multi_table.py +++ b/pkg-py/tests/test_multi_table.py @@ -1122,9 +1122,11 @@ class TestDataDictListInput: def test_accepts_list_of_data_dicts(self, orders_df, customers_df) -> None: from querychat._data_dict import DataDict, TableSpec - dd1 = DataDict(name="sales", tables={"orders": TableSpec(description="Orders")}) + dd1 = DataDict( + name="sales", tables=[TableSpec(name="orders", description="Orders")] + ) dd2 = DataDict( - name="people", tables={"customers": TableSpec(description="Customers")} + name="people", tables=[TableSpec(name="customers", description="Customers")] ) qc = QueryChat(orders_df, "orders", data_dict=[dd1, dd2]) qc.add_table(customers_df, "customers") @@ -1133,8 +1135,10 @@ def test_accepts_list_of_data_dicts(self, orders_df, customers_df) -> None: def test_accepts_list_of_paths(self, orders_df, customers_df, tmp_path) -> None: f1 = tmp_path / "orders_dict.yaml" f2 = tmp_path / "customers_dict.yaml" - f1.write_text('tables:\n orders:\n description: "Order records."\n') - f2.write_text('tables:\n customers:\n description: "Customer records."\n') + f1.write_text('tables:\n - name: orders\n description: "Order records."\n') + f2.write_text( + 'tables:\n - name: customers\n description: "Customer records."\n' + ) qc = QueryChat(orders_df, "orders", data_dict=[f1, f2]) qc.add_table(customers_df, "customers") assert qc is not None @@ -1142,16 +1146,20 @@ def test_accepts_list_of_paths(self, orders_df, customers_df, tmp_path) -> None: def test_single_data_dict_still_works(self, orders_df) -> None: from querychat._data_dict import DataDict, TableSpec - dd = DataDict(name="sales", tables={"orders": TableSpec(description="Orders")}) + dd = DataDict( + name="sales", tables=[TableSpec(name="orders", description="Orders")] + ) qc = QueryChat(orders_df, "orders", data_dict=dd) assert qc is not None def test_list_dicts_appear_in_system_prompt(self, orders_df, customers_df) -> None: from querychat._data_dict import DataDict, TableSpec - dd1 = DataDict(name="sales", tables={"orders": TableSpec(description="Orders")}) + dd1 = DataDict( + name="sales", tables=[TableSpec(name="orders", description="Orders")] + ) dd2 = DataDict( - name="people", tables={"customers": TableSpec(description="Customers")} + name="people", tables=[TableSpec(name="customers", description="Customers")] ) qc = QueryChat(orders_df, "orders", data_dict=[dd1, dd2]) qc.add_table(customers_df, "customers") diff --git a/pkg-py/tests/test_querychat.py b/pkg-py/tests/test_querychat.py index 03b26bd42..fa1331804 100644 --- a/pkg-py/tests/test_querychat.py +++ b/pkg-py/tests/test_querychat.py @@ -325,12 +325,12 @@ def test_greeting_prompt_omits_dicts_for_excluded_tables(sqlite_engine, tmp_path orders_yaml = tmp_path / "orders.yaml" orders_yaml.write_text( "name: orders_dict\ndescription: ORDERS_DICT_DESC\n" - "tables:\n orders:\n description: Orders info\n" + "tables:\n - name: orders\n description: Orders info\n" ) customers_yaml = tmp_path / "customers.yaml" customers_yaml.write_text( "name: customers_dict\ndescription: CUSTOMERS_DICT_DESC\n" - "tables:\n customers:\n description: Customers info\n" + "tables:\n - name: customers\n description: Customers info\n" ) qc = QueryChat(data_dict=[str(orders_yaml), str(customers_yaml)]) @@ -353,7 +353,7 @@ def test_greeting_prompt_keeps_global_dict_desc_drops_global_fields( orders_yaml = tmp_path / "orders.yaml" orders_yaml.write_text( "name: orders_dict\ndescription: ORDERS_DICT_DESC\n" - "tables:\n orders:\n description: Orders info\n" + "tables:\n - name: orders\n description: Orders info\n" "relationships:\n - join: orders.id = customers.id\n description: REL_DESC\n" ) diff --git a/pkg-py/tests/test_system_prompt.py b/pkg-py/tests/test_system_prompt.py index 38f99b819..8b8d91be0 100644 --- a/pkg-py/tests/test_system_prompt.py +++ b/pkg-py/tests/test_system_prompt.py @@ -70,7 +70,7 @@ def test_system_prompt_includes_table_description() -> None: df = nw.from_native(pl.DataFrame({"x": [1]})) source = DataFrameSource(df, "orders") dd = DataDict( - tables={"orders": TableSpec(description="Order records.")}, + tables=[TableSpec(name="orders", description="Order records.")], ) sp = _make_system_prompt({"orders": source}, data_dicts=[dd]) rendered = sp.render({"query"}) @@ -81,7 +81,7 @@ def test_system_prompt_includes_glossary() -> None: df = nw.from_native(pl.DataFrame({"x": [1]})) source = DataFrameSource(df, "orders") dd = DataDict( - tables={"orders": TableSpec(columns=[])}, + tables=[TableSpec(name="orders")], glossary={"churn": "No orders in 90 days."}, ) sp = _make_system_prompt({"orders": source}, data_dicts=[dd]) @@ -94,7 +94,7 @@ def test_system_prompt_includes_relationships() -> None: df = nw.from_native(pl.DataFrame({"x": [1]})) source = DataFrameSource(df, "orders") dd = DataDict( - tables={"orders": TableSpec(columns=[])}, + tables=[TableSpec(name="orders")], relationships=[ RelationshipSpec( join="orders.customer_id = customers.id", @@ -442,7 +442,9 @@ def test_no_dict_renders_flat_tables_block(self) -> None: assert " None: - dd = DataDict(name="sales", tables={"orders": TableSpec(description="Orders.")}) + dd = DataDict( + name="sales", tables=[TableSpec(name="orders", description="Orders.")] + ) sp = _make_system_prompt( {"orders": self._make_source("orders")}, data_dicts=[dd] ) @@ -451,7 +453,7 @@ def test_single_dict_renders_xml_tag_with_name(self) -> None: assert "Orders." in rendered def test_xml_tag_includes_description_attribute(self) -> None: - dd = DataDict(name="sales", description="Sales domain data", tables={}) + dd = DataDict(name="sales", description="Sales domain data", tables=[]) sp = _make_system_prompt( {"orders": self._make_source("orders")}, data_dicts=[dd] ) @@ -459,7 +461,7 @@ def test_xml_tag_includes_description_attribute(self) -> None: assert 'description="Sales domain data"' in rendered def test_xml_tag_omits_description_when_absent(self) -> None: - dd = DataDict(name="sales", tables={}) + dd = DataDict(name="sales", tables=[]) sp = _make_system_prompt( {"orders": self._make_source("orders")}, data_dicts=[dd] ) @@ -467,7 +469,7 @@ def test_xml_tag_omits_description_when_absent(self) -> None: assert "description=" not in rendered def test_yaml_body_excludes_name_and_description(self) -> None: - dd = DataDict(name="sales", description="Sales domain data", tables={}) + dd = DataDict(name="sales", description="Sales domain data", tables=[]) sp = _make_system_prompt( {"orders": self._make_source("orders")}, data_dicts=[dd] ) @@ -479,7 +481,7 @@ def test_yaml_body_excludes_name_and_description(self) -> None: def test_yaml_body_includes_tables_relationships_glossary(self) -> None: dd = DataDict( name="sales", - tables={"orders": TableSpec(description="Order records.")}, + tables=[TableSpec(name="orders", description="Order records.")], relationships=[RelationshipSpec(join="orders.customer_id = customers.id")], glossary={"churn": "No orders in 90 days."}, ) @@ -492,9 +494,11 @@ def test_yaml_body_includes_tables_relationships_glossary(self) -> None: assert "Order records." in rendered def test_multiple_dicts_render_sibling_xml_tags(self) -> None: - dd1 = DataDict(name="sales", tables={"orders": TableSpec(description="Orders")}) + dd1 = DataDict( + name="sales", tables=[TableSpec(name="orders", description="Orders")] + ) dd2 = DataDict( - name="catalog", tables={"products": TableSpec(description="Products")} + name="catalog", tables=[TableSpec(name="products", description="Products")] ) sp = _make_system_prompt( { diff --git a/pkg-py/tests/test_tools.py b/pkg-py/tests/test_tools.py index 0b0c64a4d..66ff68480 100644 --- a/pkg-py/tests/test_tools.py +++ b/pkg-py/tests/test_tools.py @@ -8,7 +8,7 @@ import polars as pl import pytest from htmltools import Tag -from querychat._data_dict import ColumnRange, ColumnSpec, DataDict, TableSpec +from querychat._data_dict import ColumnSpec, DataDict, TableSpec from querychat._datasource import ColumnMeta, DataFrameSource from querychat._query_executor import DataSourceExecutor from querychat._utils import querychat_tool_starts_open @@ -193,23 +193,24 @@ def _make_executor_and_table( def test_get_schema_tool_preserves_metadata_for_model_and_display() -> None: dd = DataDict( - tables={ - "orders": TableSpec( + tables=[ + TableSpec( + name="orders", columns=[ ColumnSpec( name="amount", description="Gross & tax", units="USD", constraints=[">= 0", "required"], - range=ColumnRange(min=0, max=100), + range=[0, 100], ), ColumnSpec( name="status", values=["pending", "shipped & paid"], ), - ] + ], ) - }, + ], ) df = pl.DataFrame( { @@ -248,8 +249,7 @@ def test_get_schema_tool_preserves_metadata_for_model_and_display() -> None: "Constraints", ) header_positions = [ - rendered.index(f'{header}') - for header in expected_headers + rendered.index(f'{header}') for header in expected_headers ] amount_cell_positions = [ rendered.index(content) diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index a9b27b7a4..6f942c2c9 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -2,6 +2,8 @@ ## Bug fixes +* Data dictionaries now follow the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` must be a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, and `values` may be a value-to-label map. Update older dictionary files accordingly; `data-dict validate-spec` (from the data-dict CLI) checks a file against the spec. (#316) + * JSON pins now fall back to `pins::pin_read()` when DuckDB's JSON extension cannot be installed or loaded, including on builds that disable extension loading. Native DuckDB JSON reading is retained when the extension is available. * Fixed SQL code blocks (the visualization footer's "Show Query" editor and the data drawer's SQL editor) rendering illegibly in dark mode — light syntax-highlighted text on a forced light background. The code panel background now adapts to the app's color mode. (#329) diff --git a/pkg-r/R/DBISource.R b/pkg-r/R/DBISource.R index 723434d40..f4bc1320e 100644 --- a/pkg-r/R/DBISource.R +++ b/pkg-r/R/DBISource.R @@ -372,15 +372,29 @@ build_column_details_impl <- function( max_val <- NA_character_ categories <- list() constraints <- if (length(spec[["constraints"]]) > 0) { - as.list(spec[["constraints"]]) + lapply(spec[["constraints"]], function(c) { + if (is.list(c)) { + # Assertion object: {assert, language?, description?} + txt <- c[["assert"]] %||% "" + if (!is.null(c[["description"]])) { + txt <- paste0(txt, " (", c[["description"]], ")") + } + txt + } else { + as.character(c) + } + }) } else { list() } if (col %in% numeric_columns) { if (!is.null(spec[["range"]])) { - min_val <- as.character(spec[["range"]][["min"]] %||% "?") - max_val <- as.character(spec[["range"]][["max"]] %||% "?") + # Spec form: two-element list [min, max]; either end may be null. + # (`[` indexing pads short lists with NULL instead of erroring.) + rng <- unname(spec[["range"]]) + min_val <- as.character(rng[1][[1]] %||% "?") + max_val <- as.character(rng[2][[1]] %||% "?") } else { min_key <- paste0(col, "__min") max_key <- paste0(col, "__max") @@ -397,7 +411,12 @@ build_column_details_impl <- function( } } else if (col %in% text_columns) { if (!is.null(spec[["values"]])) { - categories <- as.list(as.character(spec[["values"]])) + vals <- spec[["values"]] + # Map form (value -> label): the values are the names + if (!is.null(names(vals))) { + vals <- as.list(names(vals)) + } + categories <- as.list(as.character(vals)) } else if (col %in% names(categorical_values)) { vals <- categorical_values[[col]] if (length(vals) > 0) categories <- as.list(as.character(vals)) diff --git a/pkg-r/R/DataDict.R b/pkg-r/R/DataDict.R index 04b59651c..b4ae7333e 100644 --- a/pkg-r/R/DataDict.R +++ b/pkg-r/R/DataDict.R @@ -24,6 +24,27 @@ read_data_dict <- function(path) { dd } +#' Key a data dict's table list by table name. +#' +#' In the data-dict spec, `tables` is a list of table entries each with a +#' `name` field. Returns a named list mapping table name to table entry, for +#' lookup-style access. Entries without a `name` are skipped. +#' +#' @param dd A data dict list (from [read_data_dict()]). +#' @return A named list of table entries. +#' @noRd +dd_tables_by_name <- function(dd) { + tables <- dd[["tables"]] %||% list() + out <- list() + for (tbl in tables) { + nm <- tbl[["name"]] + if (!is.null(nm) && nzchar(nm)) { + out[[nm]] <- tbl + } + } + out +} + #' Convert a data dict list to a filtered list for system prompt rendering. #' #' Keeps table descriptions (for LLM context), strips per-column details. @@ -41,8 +62,9 @@ data_dict_to_prompt_list <- function(dd) { if (!is.null(dd[["description"]])) { result[["description"]] <- dd[["description"]] } - if (length(dd[["tables"]]) > 0) { - result[["tables"]] <- lapply(dd[["tables"]], function(ts) { + tables <- dd_tables_by_name(dd) + if (length(tables) > 0) { + result[["tables"]] <- lapply(tables, function(ts) { if (is.null(ts[["description"]])) { NULL } else { diff --git a/pkg-r/R/QueryChatSystemPrompt.R b/pkg-r/R/QueryChatSystemPrompt.R index 712f521dc..67cd06902 100644 --- a/pkg-r/R/QueryChatSystemPrompt.R +++ b/pkg-r/R/QueryChatSystemPrompt.R @@ -85,8 +85,9 @@ QueryChatSystemPrompt <- R6::R6Class( # (table-less) dict carrying a dict-level description. self$data_dicts <- Filter( function(dd) { - length(intersect(names(dd$tables), resolved)) > 0 || - (length(dd$tables) == 0 && !is.null(dd$description)) + tbl_names <- names(dd_tables_by_name(dd)) + length(intersect(tbl_names, resolved)) > 0 || + (length(tbl_names) == 0 && !is.null(dd$description)) }, self$data_dicts ) @@ -217,7 +218,10 @@ QueryChatSystemPrompt <- R6::R6Class( d$name <- NULL d$description <- NULL - claimed <- intersect(names(self$data_sources), names(dd$tables)) + claimed <- intersect( + names(self$data_sources), + names(dd_tables_by_name(dd)) + ) all_claimed <- c(all_claimed, claimed) if (!is.null(d$tables)) { d$tables <- d$tables[names(d$tables) %in% names(self$data_sources)] diff --git a/pkg-r/R/querychat_tools.R b/pkg-r/R/querychat_tools.R index 6b653a042..b7e0b0754 100644 --- a/pkg-r/R/querychat_tools.R +++ b/pkg-r/R/querychat_tools.R @@ -23,8 +23,9 @@ tool_get_schema <- function( } table_spec <- NULL for (dd in data_dicts) { - if (!is.null(dd[["tables"]][[table_name]])) { - table_spec <- dd[["tables"]][[table_name]] + tbl <- dd_tables_by_name(dd)[[table_name]] + if (!is.null(tbl)) { + table_spec <- tbl break } } diff --git a/pkg-r/inst/examples-shiny/11-multi-table-nutrition/nutrition-data-dict.yaml b/pkg-r/inst/examples-shiny/11-multi-table-nutrition/nutrition-data-dict.yaml index a8455e6fc..6cd8426e4 100644 --- a/pkg-r/inst/examples-shiny/11-multi-table-nutrition/nutrition-data-dict.yaml +++ b/pkg-r/inst/examples-shiny/11-multi-table-nutrition/nutrition-data-dict.yaml @@ -1,7 +1,7 @@ -version: "0.2.0" +$version: "0.1.0" tables: - foods: + - name: foods description: > One row per USDA Foundation Food item. Nutrient values are not stored here — see food_nutrients. All nutrient amounts in food_nutrients are per 100g of the food. @@ -21,7 +21,7 @@ tables: type: date description: Date the food record was published by USDA. - food_categories: + - name: food_categories description: Lookup table of USDA food categories. One row per category. columns: - name: id @@ -35,7 +35,7 @@ tables: type: string description: Human-readable category name (e.g., "Vegetables and Vegetable Products"). - nutrients: + - name: nutrients description: > Registry of the 12 key nutritional compounds tracked in this dataset. One row per nutrient type. See the glossary for the full list of nutrient IDs. @@ -54,7 +54,7 @@ tables: values: [G, KCAL, MG] description: Unit of measurement. G = grams, KCAL = kilocalories, MG = milligrams. - food_nutrients: + - name: food_nutrients description: > Nutrient content per 100g of food. One row per food–nutrient combination. Only the 12 key nutrients are included (see nutrients table and glossary). @@ -76,7 +76,7 @@ tables: nutrients.unit_name. May be null if the nutrient was not measured for this food. - food_portions: + - name: food_portions description: > Common serving sizes for foods. One row per portion definition. A single food may have multiple portions (e.g., "1 cup" and "1 oz"). @@ -107,7 +107,7 @@ tables: Optional preparation note for the portion (e.g., "chopped", "drained", "cooked"). Null when no modifier applies. - measure_units: + - name: measure_units description: > Lookup table of measurement unit names used in food_portions. Only units that appear in food_portions are included (~32 of the 123 total). diff --git a/pkg-r/tests/testthat/test-DataDict.R b/pkg-r/tests/testthat/test-DataDict.R index f10aab403..858680e41 100644 --- a/pkg-r/tests/testthat/test-DataDict.R +++ b/pkg-r/tests/testthat/test-DataDict.R @@ -3,10 +3,11 @@ describe("read_data_dict()", { yaml_file <- withr::local_tempfile(fileext = ".yaml") writeLines( c( + "$version: \"0.1.0\"", "name: test", "description: Test domain", "tables:", - " orders:", + " - name: orders", " description: Orders table", " columns:", " - name: id", @@ -26,10 +27,11 @@ describe("read_data_dict()", { expect_true(is.list(dd)) expect_equal(dd[["name"]], "test") expect_equal(dd[["description"]], "Test domain") - expect_true(is.list(dd[["tables"]][["orders"]])) - expect_equal(dd[["tables"]][["orders"]][["description"]], "Orders table") - expect_length(dd[["tables"]][["orders"]][["columns"]], 1) - expect_equal(dd[["tables"]][["orders"]][["columns"]][[1]][["name"]], "id") + orders <- dd[["tables"]][[1]] + expect_equal(orders[["name"]], "orders") + expect_equal(orders[["description"]], "Orders table") + expect_length(orders[["columns"]], 1) + expect_equal(orders[["columns"]][[1]][["name"]], "id") expect_length(dd[["relationships"]], 1) expect_equal( dd[["relationships"]][[1]][["join"]], @@ -51,12 +53,10 @@ describe("read_data_dict()", { writeLines( c( "tables:", - " products:", + " - name: products", " columns:", " - name: price", - " range:", - " min: 0", - " max: 999", + " range: [0, 999]", " - name: category", " values: [A, B, C]" ), @@ -64,16 +64,40 @@ describe("read_data_dict()", { ) dd <- read_data_dict(yaml_file) - cols <- dd[["tables"]][["products"]][["columns"]] + cols <- dd[["tables"]][[1]][["columns"]] price_col <- cols[[1]] cat_col <- cols[[2]] - expect_equal(price_col[["range"]][["min"]], 0) - expect_equal(price_col[["range"]][["max"]], 999) + expect_equal(price_col[["range"]][[1]], 0) + expect_equal(price_col[["range"]][[2]], 999) expect_equal(cat_col[["values"]], c("A", "B", "C")) }) }) +describe("dd_tables_by_name()", { + it("keys the table list by name", { + dd <- list( + tables = list( + list(name = "orders", description = "Orders"), + list(name = "customers", description = "Customers") + ) + ) + tables <- dd_tables_by_name(dd) + expect_equal(names(tables), c("orders", "customers")) + expect_equal(tables[["orders"]][["description"]], "Orders") + }) + + it("returns an empty list when there are no tables", { + expect_equal(dd_tables_by_name(list()), list()) + expect_equal(dd_tables_by_name(list(tables = list())), list()) + }) + + it("skips table entries without a name", { + dd <- list(tables = list(list(description = "No name"))) + expect_equal(dd_tables_by_name(dd), list()) + }) +}) + describe("data_dict_to_prompt_list()", { it("returns list with name and description", { dd <- list(name = "sales", description = "Sales domain") @@ -92,7 +116,8 @@ describe("data_dict_to_prompt_list()", { it("includes table descriptions but strips column details", { dd <- list( tables = list( - orders = list( + list( + name = "orders", description = "Orders table", details = "Long details that should not appear", columns = list( @@ -146,7 +171,7 @@ describe("data_dict_to_prompt_list()", { }) it("includes table entry as NULL when table has no description", { - dd <- list(tables = list(no_desc = list())) + dd <- list(tables = list(list(name = "no_desc"))) result <- data_dict_to_prompt_list(dd) expect_true("tables" %in% names(result)) expect_null(result[["tables"]][["no_desc"]]) diff --git a/pkg-r/tests/testthat/test-QueryChat.R b/pkg-r/tests/testthat/test-QueryChat.R index c2b1ddb22..604a010e7 100644 --- a/pkg-r/tests/testthat/test-QueryChat.R +++ b/pkg-r/tests/testthat/test-QueryChat.R @@ -1783,7 +1783,7 @@ describe("QueryChatGreeter", { "name: orders_dict", "description: ORDERS_DICT_DESC", "tables:", - " orders:", + " - name: orders", " description: Orders info" ), orders_yaml @@ -1794,7 +1794,7 @@ describe("QueryChatGreeter", { "name: customers_dict", "description: CUSTOMERS_DICT_DESC", "tables:", - " customers:", + " - name: customers", " description: Customers info" ), customers_yaml @@ -1831,7 +1831,7 @@ describe("QueryChatGreeter", { "name: orders_dict", "description: ORDERS_DICT_DESC", "tables:", - " orders:", + " - name: orders", " description: Orders info", "relationships:", " - join: orders.id = customers.id", diff --git a/pkg-r/tests/testthat/test-querychat_tools.R b/pkg-r/tests/testthat/test-querychat_tools.R index 46fbbc0f7..6c2e29b98 100644 --- a/pkg-r/tests/testthat/test-querychat_tools.R +++ b/pkg-r/tests/testthat/test-querychat_tools.R @@ -521,17 +521,18 @@ describe("tool_get_schema()", { df_source <- local_data_frame_source(new_test_df()) executor <- local_executor(df_source) table_spec <- list( + name = "test_table", columns = list( list( name = "id", description = "Primary key", units = "rows", constraints = c("positive", "required"), - range = list(min = 1, max = 5) + range = list(1, 5) ) ) ) - data_dicts <- list(list(tables = list(test_table = table_spec))) + data_dicts <- list(list(tables = list(table_spec))) expected <- executor$get_schema_result( "test_table", 20, diff --git a/pkg-r/vignettes/context.Rmd b/pkg-r/vignettes/context.Rmd index e66413cf5..d2e3cfa11 100644 --- a/pkg-r/vignettes/context.Rmd +++ b/pkg-r/vignettes/context.Rmd @@ -45,14 +45,14 @@ qc$add_table(customers, "customers") ### Format -A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. +The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables` (a list of tables, each with a `name`), `relationships`, and `glossary`. ```yaml # data-dict.yaml -version: "0.2.0" +$version: "0.1.0" tables: - orders: + - name: orders description: One row per customer order. columns: - name: order_id @@ -71,7 +71,7 @@ tables: values: [pending, shipped, delivered, cancelled] description: Current order status. - customers: + - name: customers description: One row per customer. columns: - name: id @@ -97,7 +97,7 @@ glossary: #### Tables -Each entry under `tables` describes one table. The key must match the table name you pass to `QueryChat$new()` or `$add_table()`. +Each entry under `tables` describes one table. Its `name` must match the table name you pass to `QueryChat$new()` or `$add_table()`. - **`description`**: What this table represents (one sentence is usually enough). - **`columns`**: A list of column annotations. Each column can have: From 57775c6ac37003fb5ea8717a59698912f660024f Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 15:24:29 -0500 Subject: [PATCH 02/11] fix(py): migrate nutrition example to current spec; render open-ended range bounds Addresses Copilot review on #330: the bundled Python nutrition app's data dict still used mapping-form tables (now rejected at load), and a one-sided range like [0, null] lost its known bound in both the LLM schema text and the user-facing schema card (now rendered as '0 to ?'). --- .../nutrition-data-dict.yaml | 14 +++++++------- pkg-py/src/querychat/_datasource.py | 11 ++++++----- pkg-py/src/querychat/tools.py | 11 ++++++----- pkg-py/tests/test_datasource.py | 10 ++++++++++ pkg-py/tests/test_tools.py | 12 ++++++++++++ 5 files changed, 41 insertions(+), 17 deletions(-) diff --git a/pkg-py/examples/11-multi-table-nutrition/nutrition-data-dict.yaml b/pkg-py/examples/11-multi-table-nutrition/nutrition-data-dict.yaml index a8455e6fc..6cd8426e4 100644 --- a/pkg-py/examples/11-multi-table-nutrition/nutrition-data-dict.yaml +++ b/pkg-py/examples/11-multi-table-nutrition/nutrition-data-dict.yaml @@ -1,7 +1,7 @@ -version: "0.2.0" +$version: "0.1.0" tables: - foods: + - name: foods description: > One row per USDA Foundation Food item. Nutrient values are not stored here — see food_nutrients. All nutrient amounts in food_nutrients are per 100g of the food. @@ -21,7 +21,7 @@ tables: type: date description: Date the food record was published by USDA. - food_categories: + - name: food_categories description: Lookup table of USDA food categories. One row per category. columns: - name: id @@ -35,7 +35,7 @@ tables: type: string description: Human-readable category name (e.g., "Vegetables and Vegetable Products"). - nutrients: + - name: nutrients description: > Registry of the 12 key nutritional compounds tracked in this dataset. One row per nutrient type. See the glossary for the full list of nutrient IDs. @@ -54,7 +54,7 @@ tables: values: [G, KCAL, MG] description: Unit of measurement. G = grams, KCAL = kilocalories, MG = milligrams. - food_nutrients: + - name: food_nutrients description: > Nutrient content per 100g of food. One row per food–nutrient combination. Only the 12 key nutrients are included (see nutrients table and glossary). @@ -76,7 +76,7 @@ tables: nutrients.unit_name. May be null if the nutrient was not measured for this food. - food_portions: + - name: food_portions description: > Common serving sizes for foods. One row per portion definition. A single food may have multiple portions (e.g., "1 cup" and "1 oz"). @@ -107,7 +107,7 @@ tables: Optional preparation note for the portion (e.g., "chopped", "drained", "cooked"). Null when no modifier applies. - measure_units: + - name: measure_units description: > Lookup table of measurement unit names used in food_portions. Only units that appear in food_portions are included (~32 of the 123 total). diff --git a/pkg-py/src/querychat/_datasource.py b/pkg-py/src/querychat/_datasource.py index 5dee42c6a..05fb716dc 100644 --- a/pkg-py/src/querychat/_datasource.py +++ b/pkg-py/src/querychat/_datasource.py @@ -75,12 +75,13 @@ def format_schema(table_name: str, columns: list[ColumnMeta]) -> str: lines.append(f" Description: {col.description}") if col.constraints: lines.append(f" Constraints: {', '.join(col.constraints)}") - if ( - col.kind in ("numeric", "date") - and col.min_val is not None - and col.max_val is not None + if col.kind in ("numeric", "date") and ( + col.min_val is not None or col.max_val is not None ): - lines.append(f" Range: {col.min_val} to {col.max_val}") + # Open-ended bounds (allowed by the data-dict spec) render as '?' + lo = col.min_val if col.min_val is not None else "?" + hi = col.max_val if col.max_val is not None else "?" + lines.append(f" Range: {lo} to {hi}") elif col.categories: cats = ", ".join(f"'{v}'" for v in col.categories) lines.append(f" Categorical values: {cats}") diff --git a/pkg-py/src/querychat/tools.py b/pkg-py/src/querychat/tools.py index 8cfc5d878..21de714ad 100644 --- a/pkg-py/src/querychat/tools.py +++ b/pkg-py/src/querychat/tools.py @@ -139,12 +139,13 @@ def _schema_table(columns: list[ColumnMeta]) -> Tag: ) range_values = "" - if ( - column.kind in ("numeric", "date") - and column.min_val is not None - and column.max_val is not None + if column.kind in ("numeric", "date") and ( + column.min_val is not None or column.max_val is not None ): - range_values = f"{column.min_val} to {column.max_val}" + # Open-ended bounds (allowed by the data-dict spec) render as '?' + lo = column.min_val if column.min_val is not None else "?" + hi = column.max_val if column.max_val is not None else "?" + range_values = f"{lo} to {hi}" elif column.categories: range_values = ", ".join(f"'{value}'" for value in column.categories) diff --git a/pkg-py/tests/test_datasource.py b/pkg-py/tests/test_datasource.py index 1f17389fb..12b555a73 100644 --- a/pkg-py/tests/test_datasource.py +++ b/pkg-py/tests/test_datasource.py @@ -584,3 +584,13 @@ def test_sqlalchemy_cleanup_does_not_dispose_caller_owned_engine( dispose.assert_not_called() assert len(source.get_data()) > 0 + + +def test_format_schema_open_ended_range() -> None: + cols = [ + ColumnMeta(name="a", sql_type="INTEGER", kind="numeric", min_val=0), + ColumnMeta(name="b", sql_type="INTEGER", kind="numeric", max_val=100), + ] + text = format_schema("t", cols) + assert " Range: 0 to ?" in text + assert " Range: ? to 100" in text diff --git a/pkg-py/tests/test_tools.py b/pkg-py/tests/test_tools.py index 66ff68480..b1305dc21 100644 --- a/pkg-py/tests/test_tools.py +++ b/pkg-py/tests/test_tools.py @@ -329,3 +329,15 @@ def test_get_schema_tool_uses_singular_column_preview() -> None: result = tool.func(table_name="orders") assert result.extra["display"].value_preview == "1 column" + + +def test_schema_card_shows_open_ended_range() -> None: + from querychat.tools import _schema_table + + cols = [ + ColumnMeta(name="a", sql_type="INTEGER", kind="numeric", min_val=0), + ColumnMeta(name="b", sql_type="INTEGER", kind="numeric", max_val=100), + ] + html = str(_schema_table(cols)) + assert "0 to ?" in html + assert "? to 100" in html From b1f49f9ef18b291e33a468ee14c9fcb2b589c1d9 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 15:46:20 -0500 Subject: [PATCH 03/11] fix: address Copilot review on #330 - ConstraintAssertion: enable populate_by_name so assert_= construction and model_validate(model_dump()) round-trips work - ColumnSpec.values: coerce numeric YAML map keys (e.g. {0: Control}) to strings instead of rejecting them - pkg-r: add regression tests asserting rendered output for open-ended ranges, map-form value keys, and assertion constraints --- pkg-py/src/querychat/_data_dict.py | 15 +++++++- pkg-py/tests/test_data_dict.py | 30 +++++++++++++++ pkg-r/tests/testthat/test-querychat_tools.R | 42 +++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index 35fee8b1a..e23f1edb2 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -3,7 +3,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Literal -from pydantic import BaseModel, Field, field_validator, model_validator +from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator if TYPE_CHECKING: from ._datasource import ColumnMeta @@ -30,6 +30,10 @@ class ConstraintAssertion(BaseModel): """ + # populate_by_name allows construction with `assert_=` and round-trips + # through model_dump() (which emits the field name, not the YAML alias) + model_config = ConfigDict(populate_by_name=True) + assert_: str = Field(alias="assert") language: Literal["sql", "r", "python"] | None = None description: str | None = None @@ -89,6 +93,15 @@ class ColumnSpec(BaseModel): range: list[Any] | None = None examples: list[Any] | None = None + @field_validator("values", mode="before") + @classmethod + def _values_map_keys_are_strings(cls, v: Any) -> Any: + # YAML parses numeric map keys (e.g. {0: Control, 1: Treatment}) as + # ints; the spec allows them, so coerce keys to strings. + if isinstance(v, dict): + return {str(k): val for k, val in v.items()} + return v + @field_validator("range") @classmethod def _range_is_two_elements(cls, v: list[Any] | None) -> list[Any] | None: diff --git a/pkg-py/tests/test_data_dict.py b/pkg-py/tests/test_data_dict.py index a9399ea9a..5c7c34087 100644 --- a/pkg-py/tests/test_data_dict.py +++ b/pkg-py/tests/test_data_dict.py @@ -318,3 +318,33 @@ def test_to_prompt_dict_includes_glossary() -> None: dd = DataDict(glossary={"ARR": "Annual Recurring Revenue"}) d = dd.to_prompt_dict() assert d["glossary"]["ARR"] == "Annual Recurring Revenue" + + +def test_constraint_assertion_constructible_by_name_and_round_trips() -> None: + from querychat._data_dict import ConstraintAssertion + + ca = ConstraintAssertion(assert_="amount >= 0", description="Non-negative") + assert ca.assert_ == "amount >= 0" + dd = DataDict( + tables=[ + TableSpec( + name="orders", + columns=[ColumnSpec(name="amount", constraints=[ca])], + ) + ], + ) + dd2 = DataDict.model_validate(dd.model_dump()) + constraint = dd2.tables[0].columns[0].constraints[0] + assert not isinstance(constraint, str) + assert constraint.assert_ == "amount >= 0" + assert constraint.description == "Non-negative" + + +def test_values_map_accepts_numeric_keys(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text( + "tables:\n - name: orders\n columns:\n" + " - name: arm\n values: {0: Control, 1: Treatment}\n" + ) + dd = DataDict.from_yaml(f) + assert dd.tables[0].columns[0].values == {"0": "Control", "1": "Treatment"} diff --git a/pkg-r/tests/testthat/test-querychat_tools.R b/pkg-r/tests/testthat/test-querychat_tools.R index 6c2e29b98..22d852e84 100644 --- a/pkg-r/tests/testthat/test-querychat_tools.R +++ b/pkg-r/tests/testthat/test-querychat_tools.R @@ -517,6 +517,48 @@ describe("tool_update_dashboard_impl()", { describe("tool_get_schema()", { skip_if_no_dataframe_engine() + it("renders spec range, value keys, and assertion constraints", { + df <- data.frame( + id = 1:5, + score = 1:5, + grp = c("A", "B", "A", "B", "A"), + stringsAsFactors = FALSE + ) + df_source <- local_data_frame_source(df) + executor <- local_executor(df_source) + table_spec <- list( + name = "test_table", + columns = list( + list(name = "id", range = list(1, NULL)), + list(name = "score", range = list(NULL, 5)), + list( + name = "grp", + values = list(A = "Alpha", B = "Beta"), + constraints = list( + list(assert = "grp <> ''", description = "Non-empty"), + "required" + ) + ) + ) + ) + schema_result <- executor$get_schema_result( + "test_table", + 20, + table_spec = table_spec + ) + text <- schema_result$text + + # Open-ended ranges keep their known bound + expect_match(text, "Range: 1 to ?", fixed = TRUE) + expect_match(text, "Range: ? to 5", fixed = TRUE) + # Map-form values render the values (keys), not the labels + expect_match(text, "Categorical values: 'A', 'B'", fixed = TRUE) + expect_false(grepl("Alpha", text, fixed = TRUE)) + # Assertion objects render as 'assert (description)' alongside barewords + expect_match(text, "grp <> '' (Non-empty)", fixed = TRUE) + expect_match(text, "required", fixed = TRUE) + }) + it("returns schema text with a native rich display", { df_source <- local_data_frame_source(new_test_df()) executor <- local_executor(df_source) From 443d872e2d0baa94c6b4df1a68104abf8d154184 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 16:01:11 -0500 Subject: [PATCH 04/11] docs: document range, map-form values, and assertion constraints in data dict guide --- pkg-py/docs/context.qmd | 5 +++-- pkg-r/vignettes/context.Rmd | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg-py/docs/context.qmd b/pkg-py/docs/context.qmd index f6d63e07c..f0d08f44e 100644 --- a/pkg-py/docs/context.qmd +++ b/pkg-py/docs/context.qmd @@ -87,9 +87,10 @@ Each entry under `tables` describes one table. Its `name` must match the table n - **`columns`**: A list of column annotations. Each column can have: - `name`: Column name (must match the actual column) - `type`: Semantic type hint — `string`, `number`, `number(id)`, `number(quantity)`, `date`, `enum` - - `constraints`: Optional list — `primary_key`, `foreign_key` + - `constraints`: Optional list — barewords like `primary_key`/`foreign_key`, or assertion objects (`assert` with an optional `language` and `description`) - `description`: What this column means in plain English - - `values`: For `enum` columns, the list of possible values + - `values`: For `enum` columns, the possible values — a list, or a value-to-label map (e.g. `{M: Male, F: Female}`) + - `range`: Optional `[min, max]` bounds (either end may be `null` for an open range). Replaces live min/max queries for the column Columns listed in the data dictionary are excluded from the auto-generated schema (since your description supersedes the auto-detected metadata). Columns not listed are still auto-detected as usual. diff --git a/pkg-r/vignettes/context.Rmd b/pkg-r/vignettes/context.Rmd index d2e3cfa11..9a5fb5f32 100644 --- a/pkg-r/vignettes/context.Rmd +++ b/pkg-r/vignettes/context.Rmd @@ -103,9 +103,10 @@ Each entry under `tables` describes one table. Its `name` must match the table n - **`columns`**: A list of column annotations. Each column can have: - `name`: Column name (must match the actual column) - `type`: Semantic type hint — `string`, `number`, `number(id)`, `number(quantity)`, `date`, `enum` - - `constraints`: Optional list — `primary_key`, `foreign_key` + - `constraints`: Optional list — barewords like `primary_key`/`foreign_key`, or assertion objects (`assert` with an optional `language` and `description`) - `description`: What this column means in plain English - - `values`: For `enum` columns, the list of possible values + - `values`: For `enum` columns, the possible values — a list, or a value-to-label map (e.g. `{M: Male, F: Female}`) + - `range`: Optional `[min, max]` bounds (either end may be `null` for an open range). Replaces live min/max queries for the column Columns listed in the data dictionary are excluded from the auto-generated schema (since your description supersedes the auto-detected metadata). Columns not listed are still auto-detected as usual. From 4861ac0d0b65a55c48bf831d39dea9e0d52f2931 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 16:14:28 -0500 Subject: [PATCH 05/11] feat: accept the spec's -.inf/.inf sentinels for open range bounds Addresses Copilot review on #330: the linked data-dict spec uses -.inf/.inf (not null) for open bounds and asks that open bounds be described in prose. Both packages now normalize infinite range ends to an open bound (rendered as '?', same as null), and the data dict guide documents the sentinels and the prose requirement. --- pkg-py/docs/context.qmd | 2 +- pkg-py/src/querychat/_data_dict.py | 13 ++++++++++++- pkg-py/tests/test_data_dict.py | 12 ++++++++++++ pkg-r/R/DBISource.R | 15 ++++++++++++--- pkg-r/tests/testthat/test-querychat_tools.R | 3 ++- pkg-r/vignettes/context.Rmd | 2 +- 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/pkg-py/docs/context.qmd b/pkg-py/docs/context.qmd index f0d08f44e..5dc18e7c1 100644 --- a/pkg-py/docs/context.qmd +++ b/pkg-py/docs/context.qmd @@ -90,7 +90,7 @@ Each entry under `tables` describes one table. Its `name` must match the table n - `constraints`: Optional list — barewords like `primary_key`/`foreign_key`, or assertion objects (`assert` with an optional `language` and `description`) - `description`: What this column means in plain English - `values`: For `enum` columns, the possible values — a list, or a value-to-label map (e.g. `{M: Male, F: Female}`) - - `range`: Optional `[min, max]` bounds (either end may be `null` for an open range). Replaces live min/max queries for the column + - `range`: Optional `[min, max]` bounds. Either end may be left open with the `-.inf`/`.inf` sentinels (or `null`) — if you leave a bound open, describe the range in prose in the column's `description`. Replaces live min/max queries for the column Columns listed in the data dictionary are excluded from the auto-generated schema (since your description supersedes the auto-detected metadata). Columns not listed are still auto-detected as usual. diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index e23f1edb2..4e81bc2ac 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -1,5 +1,6 @@ from __future__ import annotations +import math from pathlib import Path from typing import TYPE_CHECKING, Any, Literal @@ -102,12 +103,22 @@ def _values_map_keys_are_strings(cls, v: Any) -> Any: return {str(k): val for k, val in v.items()} return v + @field_validator("range", mode="before") + @classmethod + def _range_open_bound_sentinels(cls, v: Any) -> Any: + # The spec's open-bound sentinels (-.inf/.inf) parse as float + # infinities; normalize them to None for an open end. + if isinstance(v, list): + return [None if isinstance(x, float) and math.isinf(x) else x for x in v] + return v + @field_validator("range") @classmethod def _range_is_two_elements(cls, v: list[Any] | None) -> list[Any] | None: if v is not None and len(v) != 2: # noqa: PLR2004 raise ValueError( - "range must be a two-element list [min, max]; use null for an open end" + "range must be a two-element list [min, max]; " + "use null (or the spec's -.inf/.inf sentinels) for an open end" ) return v diff --git a/pkg-py/tests/test_data_dict.py b/pkg-py/tests/test_data_dict.py index 5c7c34087..071587e07 100644 --- a/pkg-py/tests/test_data_dict.py +++ b/pkg-py/tests/test_data_dict.py @@ -348,3 +348,15 @@ def test_values_map_accepts_numeric_keys(tmp_path: Path) -> None: ) dd = DataDict.from_yaml(f) assert dd.tables[0].columns[0].values == {"0": "Control", "1": "Treatment"} + + +def test_range_infinity_sentinels_mean_open(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text( + "tables:\n - name: orders\n columns:\n" + " - name: amount\n range: [0, .inf]\n" + " - name: score\n range: [-.inf, 100]\n" + ) + dd = DataDict.from_yaml(f) + assert dd.tables[0].columns[0].range == [0, None] + assert dd.tables[0].columns[1].range == [None, 100] diff --git a/pkg-r/R/DBISource.R b/pkg-r/R/DBISource.R index f4bc1320e..f78fa5386 100644 --- a/pkg-r/R/DBISource.R +++ b/pkg-r/R/DBISource.R @@ -390,11 +390,20 @@ build_column_details_impl <- function( if (col %in% numeric_columns) { if (!is.null(spec[["range"]])) { - # Spec form: two-element list [min, max]; either end may be null. + # Spec form: two-element list [min, max]. Either end may be open: + # null, or the spec's -.inf/.inf sentinels (which parse as -Inf/Inf). # (`[` indexing pads short lists with NULL instead of erroring.) rng <- unname(spec[["range"]]) - min_val <- as.character(rng[1][[1]] %||% "?") - max_val <- as.character(rng[2][[1]] %||% "?") + lo <- rng[1][[1]] + hi <- rng[2][[1]] + if (is.numeric(lo) && is.infinite(lo)) { + lo <- NULL + } + if (is.numeric(hi) && is.infinite(hi)) { + hi <- NULL + } + min_val <- as.character(lo %||% "?") + max_val <- as.character(hi %||% "?") } else { min_key <- paste0(col, "__min") max_key <- paste0(col, "__max") diff --git a/pkg-r/tests/testthat/test-querychat_tools.R b/pkg-r/tests/testthat/test-querychat_tools.R index 22d852e84..a27ab5107 100644 --- a/pkg-r/tests/testthat/test-querychat_tools.R +++ b/pkg-r/tests/testthat/test-querychat_tools.R @@ -530,7 +530,8 @@ describe("tool_get_schema()", { name = "test_table", columns = list( list(name = "id", range = list(1, NULL)), - list(name = "score", range = list(NULL, 5)), + # The spec's -.inf/.inf sentinels also mean an open bound + list(name = "score", range = list(-Inf, 5)), list( name = "grp", values = list(A = "Alpha", B = "Beta"), diff --git a/pkg-r/vignettes/context.Rmd b/pkg-r/vignettes/context.Rmd index 9a5fb5f32..2ef20b402 100644 --- a/pkg-r/vignettes/context.Rmd +++ b/pkg-r/vignettes/context.Rmd @@ -106,7 +106,7 @@ Each entry under `tables` describes one table. Its `name` must match the table n - `constraints`: Optional list — barewords like `primary_key`/`foreign_key`, or assertion objects (`assert` with an optional `language` and `description`) - `description`: What this column means in plain English - `values`: For `enum` columns, the possible values — a list, or a value-to-label map (e.g. `{M: Male, F: Female}`) - - `range`: Optional `[min, max]` bounds (either end may be `null` for an open range). Replaces live min/max queries for the column + - `range`: Optional `[min, max]` bounds. Either end may be left open with the `-.inf`/`.inf` sentinels (or `null`) — if you leave a bound open, describe the range in prose in the column's `description`. Replaces live min/max queries for the column Columns listed in the data dictionary are excluded from the auto-generated schema (since your description supersedes the auto-detected metadata). Columns not listed are still auto-detected as usual. From 142a402647f6725eec8f99b7fbcc22a21a10bd69 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 16:35:24 -0500 Subject: [PATCH 06/11] fix(py): valid YAML in legacy-format migration message Addresses Copilot review on #330: the after example indented columns farther than name (invalid YAML); before/after are now separate blocks that copy cleanly. --- pkg-py/src/querychat/_data_dict.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index 4e81bc2ac..e210933a1 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -172,10 +172,15 @@ class RelationshipSpec(BaseModel): This data dictionary uses an outdated format: `tables` must be a list of tables, each with a `name` field — not a mapping of name to definition. - # before # after - tables: tables: - orders: - name: orders - columns: [...] columns: [...] + # before + tables: + orders: + columns: [...] + + # after + tables: + - name: orders + columns: [...] Also changed in the current spec: column `range` is a two-element list [min, max], and `values` may be a value-to-label map ({M: Male, F: Female}). From 09c93b73d976c46172e8c91f1cfb4cfdcada2041 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 17:28:43 -0500 Subject: [PATCH 07/11] fix: warn on data-dict spec-compat problems instead of silently misreading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addressing review findings on the spec-alignment PR: - `$version` handling: files declaring a version that is invalid, predates the first spec version, or is newer than the newest spec version querychat understands now warn and are read on a best-effort basis (unknown fields are ignored). This keeps additive spec evolution maintenance-free — the known-version constant is calibration for the warning, not a hard gate, so it only needs bumping opportunistically. The legacy mapping-form `tables` check in pkg-py remains a hard error. - pkg-r: read_data_dict() now warns on those same `$version` problems, on legacy mapping-form `tables`, and on nameless table entries — all of which previously degraded silently (the whole dict was ignored). - pkg-r: dd_tables_by_name() keeps the first entry on duplicate table names, matching pkg-py's DataDict.get_table(). - Move the changelog entries to Breaking changes sections and document the `$version` behavior in the context guides. --- pkg-py/CHANGELOG.md | 6 ++- pkg-py/docs/context.qmd | 2 +- pkg-py/src/querychat/_data_dict.py | 64 ++++++++++++++++++++++ pkg-py/tests/test_data_dict.py | 35 ++++++++++++ pkg-r/NEWS.md | 6 ++- pkg-r/R/DataDict.R | 81 +++++++++++++++++++++++++++- pkg-r/tests/testthat/test-DataDict.R | 50 +++++++++++++++++ pkg-r/vignettes/context.Rmd | 2 +- 8 files changed, 238 insertions(+), 8 deletions(-) diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index 6cec56df5..a5a116bbb 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -7,14 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Unreleased +### Breaking changes + +* `DataDict` now follows the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` is a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, `values` may be a value-to-label map, and `constraints` may include assertion objects. Files written in the older mapping format fail with a descriptive error explaining how to update. Files declaring a `$version` newer than the spec version querychat understands are still read on a best-effort basis (unknown fields are ignored), with a warning. (#316) + ### New features * **Faster iterative visualization, and support for layer-level data sources.** Visualization queries now execute as a whole through `ggsql.CachingReader`: the underlying SQL runs against your data source once and the result is memoized for the chat session, so re-rendering a chart with a different encoding no longer re-queries the source (a win for remote databases especially). Visualization layers can also draw from other relations in the query — e.g. raw points from the final `SELECT` with a summary line from a CTE — via `DRAW ... MAPPING ... FROM `. Caching can be tuned or disabled with the `GGSQL_CACHE_TTL`, `GGSQL_CACHE_MAX_BYTES`, and `GGSQL_CACHE_DISABLED` environment variables. (#321) ### Bug fixes -* `DataDict` now follows the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` is a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, `values` may be a value-to-label map, and `constraints` may include assertion objects. Files written in the older mapping format fail with a descriptive error explaining how to update. (#316) - * JSON pins now fall back to `pin_read()` when DuckDB's JSON extension cannot be installed or loaded, including on builds that disable extension loading. Native DuckDB JSON reading is retained when the extension is available. * Fixed SQL code blocks (the visualization footer's "Show Query" editor and the data drawer's SQL editor) rendering illegibly in dark mode — light syntax-highlighted text on a forced light background. The code panel background now adapts to the app's color mode. (#329) diff --git a/pkg-py/docs/context.qmd b/pkg-py/docs/context.qmd index 5dc18e7c1..ec81d596a 100644 --- a/pkg-py/docs/context.qmd +++ b/pkg-py/docs/context.qmd @@ -30,7 +30,7 @@ qc.add_table(customers, "customers") ### Format -The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. +The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. The optional `$version` field declares the spec version the file conforms to; a file declaring a version newer than querychat understands is still read on a best-effort basis (unknown fields are ignored), with a warning. ```{.yaml filename="data-dict.yaml"} $version: "0.1.0" diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index e210933a1..ed1b3e212 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -1,6 +1,7 @@ from __future__ import annotations import math +import warnings from pathlib import Path from typing import TYPE_CHECKING, Any, Literal @@ -189,6 +190,62 @@ class RelationshipSpec(BaseModel): https://data-dict.tidyverse.org/spec.html """ +# The newest data-dict spec version this version of querychat fully +# understands. Files declaring a newer `$version` are still read, on a +# best-effort basis (unknown fields are ignored), with a warning. Bump this +# opportunistically to keep the warning calibrated — it is not a hard gate. +_KNOWN_SPEC_VERSION = (0, 1, 0) + + +def _parse_spec_version(value: Any) -> tuple[int, int, int] | None: + """ + Parse a `$version` value into a (major, minor, patch) tuple. + + Returns ``None`` when ``value`` is not a recognizable version string. + Short forms are zero-padded, so ``"0.1"`` parses as ``(0, 1, 0)``. + """ + if isinstance(value, bool) or not isinstance(value, (str, int, float)): + return None + parts = str(value).split(".") + if len(parts) > 3 or not all(p.isdigit() for p in parts): # noqa: PLR2004 + return None + nums = [int(p) for p in parts] + # Zero-pad short forms, so "0.1" parses as (0, 1, 0) + nums.extend([0] * (3 - len(nums))) + return (nums[0], nums[1], nums[2]) + + +def _check_spec_version(version: Any) -> None: + parsed = _parse_spec_version(version) + if parsed is None: + warnings.warn( + f"This data dictionary has an invalid `$version`: {version!r}. " + 'Expected a version like "0.1.0"; see ' + "https://data-dict.tidyverse.org/spec.html", + stacklevel=3, + ) + return + if parsed < (0, 1, 0): + warnings.warn( + f'This data dictionary declares `$version: "{version}"`, which ' + "predates the first data-dict spec version (0.1.0). See " + "https://data-dict.tidyverse.org/spec.html", + stacklevel=3, + ) + return + if parsed > _KNOWN_SPEC_VERSION: + known = ".".join(str(p) for p in _KNOWN_SPEC_VERSION) + warnings.warn( + f'This data dictionary declares `$version: "{version}"`, which is ' + "newer than the newest data-dict spec version this version of " + f"querychat fully understands ({known}). The file is read on a " + "best-effort basis — spec fields querychat doesn't know about are " + "ignored. If anything looks off, upgrade querychat, or check the " + "file against the spec with `data-dict validate-spec `: " + "https://data-dict.tidyverse.org/spec.html", + stacklevel=3, + ) + class DataDict(BaseModel): """ @@ -259,6 +316,13 @@ def _reject_legacy_tables(cls, data: Any) -> Any: raise ValueError(_LEGACY_TABLES_MESSAGE) # noqa: TRY004 return data + @model_validator(mode="before") + @classmethod + def _reject_unsupported_spec_version(cls, data: Any) -> Any: + if isinstance(data, dict) and "$version" in data: + _check_spec_version(data["$version"]) + return data + def get_table(self, table_name: str) -> TableSpec | None: """Return the :class:`TableSpec` for ``table_name``, or ``None``.""" return next((t for t in self.tables if t.name == table_name), None) diff --git a/pkg-py/tests/test_data_dict.py b/pkg-py/tests/test_data_dict.py index 071587e07..a63b945b8 100644 --- a/pkg-py/tests/test_data_dict.py +++ b/pkg-py/tests/test_data_dict.py @@ -1,3 +1,4 @@ +import warnings from pathlib import Path import narwhals.stable.v1 as nw @@ -80,6 +81,40 @@ def test_from_yaml_legacy_dict_tables_error(tmp_path: Path) -> None: DataDict.from_yaml(f) +def test_newer_spec_version_warns_but_is_read(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text('$version: "0.2.0"\ntables: []\n') + with pytest.warns(UserWarning, match="best-effort basis"): + dd = DataDict.from_yaml(f) + assert dd.tables == [] + + +def test_spec_version_predating_first_spec_warns() -> None: + with pytest.warns(UserWarning, match="predates the first"): + DataDict.model_validate({"$version": "0.0.9"}) + + +def test_invalid_spec_version_warns(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text('$version: "not-a-version"\n') + with pytest.warns(UserWarning, match=r"invalid `\$version`"): + DataDict.from_yaml(f) + + +def test_known_spec_version_ok(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text('$version: "0.1.0"\ntables: []\n') + with warnings.catch_warnings(): + warnings.simplefilter("error") + assert DataDict.from_yaml(f).tables == [] + + # Short forms are zero-padded ("0.1" == "0.1.0") + f.write_text('$version: "0.1"\ntables: []\n') + with warnings.catch_warnings(): + warnings.simplefilter("error") + assert DataDict.from_yaml(f).tables == [] + + def test_table_requires_name(tmp_path: Path) -> None: f = tmp_path / "spec.yaml" f.write_text("tables:\n - description: No name here\n columns: []\n") diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 6f942c2c9..31787230b 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -1,8 +1,10 @@ # querychat (development version) -## Bug fixes +## Breaking changes -* Data dictionaries now follow the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` must be a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, and `values` may be a value-to-label map. Update older dictionary files accordingly; `data-dict validate-spec` (from the data-dict CLI) checks a file against the spec. (#316) +* Data dictionaries now follow the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` must be a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, and `values` may be a value-to-label map. Update older dictionary files accordingly; `data-dict validate-spec` (from the data-dict CLI) checks a file against the spec. Files in the older mapping format, or declaring a `$version` newer than the spec version querychat understands, warn at load time but are still read on a best-effort basis. (#316) + +## Bug fixes * JSON pins now fall back to `pins::pin_read()` when DuckDB's JSON extension cannot be installed or loaded, including on builds that disable extension loading. Native DuckDB JSON reading is retained when the extension is available. diff --git a/pkg-r/R/DataDict.R b/pkg-r/R/DataDict.R index b4ae7333e..09814d5ed 100644 --- a/pkg-r/R/DataDict.R +++ b/pkg-r/R/DataDict.R @@ -17,6 +17,8 @@ read_data_dict <- function(path) { dd <- yaml::read_yaml(path) %||% list() + dd_check_spec_compat(dd) + if (is.null(dd[["name"]])) { dd[["name"]] <- tools::file_path_sans_ext(basename(path)) } @@ -24,11 +26,86 @@ read_data_dict <- function(path) { dd } +# The newest data-dict spec version this version of querychat fully +# understands. Files declaring a newer `$version` are still read, on a +# best-effort basis (unknown fields are ignored), with a warning. Bump this +# opportunistically to keep the warning calibrated — it is not a hard gate. +.dd_known_spec_version <- numeric_version("0.1.0") + +#' Warn about data-dict spec compatibility problems. +#' +#' pkg-r intentionally does not fully validate data dictionaries, but a few +#' problems are worth surfacing because they otherwise fail silently: a +#' `$version` newer than the spec version querychat understands, `tables` in +#' the legacy mapping format, and table entries without a `name`. +#' +#' @param dd A data dict list (from [yaml::read_yaml()]). +#' @noRd +dd_check_spec_compat <- function(dd) { + version <- dd[["$version"]] + if (!is.null(version)) { + v <- tryCatch( + numeric_version(as.character(version)), + error = function(e) NULL + ) + if (is.null(v)) { + cli::cli_warn(c( + "!" = "Data dictionary has an invalid {.field $version}: {.val {version}}.", + "i" = "Expected a version like {.val 0.1.0}; see {.url https://data-dict.tidyverse.org/spec.html}." + )) + } else if (v > .dd_known_spec_version) { + cli::cli_warn(c( + "!" = "Data dictionary declares {.field $version} {.val {version}}, which is newer than the newest data-dict spec version this version of {.pkg querychat} fully understands ({.val {as.character(.dd_known_spec_version)}}).", + "i" = "The file is read on a best-effort basis — spec fields querychat doesn't know about are ignored.", + "i" = "If anything looks off, consider upgrading {.pkg querychat} or running {.code data-dict validate-spec }." + )) + } else if (v < .dd_known_spec_version) { + cli::cli_warn(c( + "!" = "Data dictionary declares {.field $version} {.val {version}}, which predates the first data-dict spec version (0.1.0).", + "i" = "See {.url https://data-dict.tidyverse.org/spec.html}." + )) + } + } + + tables <- dd[["tables"]] + if (length(tables) == 0) { + return(invisible(dd)) + } + + table_names <- names(tables) + if (!is.null(table_names) && any(nzchar(table_names))) { + cli::cli_warn(c( + "!" = "Data dictionary {.field tables} uses an outdated format and will be ignored.", + "i" = "{.field tables} must be a list of tables, each with a {.field name} field — not a mapping of name to definition.", + "i" = "Update the file to the current spec ({.url https://data-dict.tidyverse.org/spec.html}); {.code data-dict validate-spec } checks a file against the spec." + )) + return(invisible(dd)) + } + + unnamed <- vapply( + tables, + function(tbl) { + nm <- if (is.list(tbl)) tbl[["name"]] else NULL + is.null(nm) || length(nm) != 1 || !nzchar(nm) + }, + logical(1) + ) + if (any(unnamed)) { + cli::cli_warn(c( + "!" = "{sum(unnamed)} table entr{?y/ies} in the data dictionary ha{?s/ve} no {.field name} and will be ignored.", + "i" = "Each entry in {.field tables} needs a {.field name}; see {.url https://data-dict.tidyverse.org/spec.html}." + )) + } + + invisible(dd) +} + #' Key a data dict's table list by table name. #' #' In the data-dict spec, `tables` is a list of table entries each with a #' `name` field. Returns a named list mapping table name to table entry, for -#' lookup-style access. Entries without a `name` are skipped. +#' lookup-style access. Entries without a `name` are skipped. On duplicate +#' names the first entry wins (matching pkg-py's `DataDict.get_table()`). #' #' @param dd A data dict list (from [read_data_dict()]). #' @return A named list of table entries. @@ -38,7 +115,7 @@ dd_tables_by_name <- function(dd) { out <- list() for (tbl in tables) { nm <- tbl[["name"]] - if (!is.null(nm) && nzchar(nm)) { + if (!is.null(nm) && nzchar(nm) && is.null(out[[nm]])) { out[[nm]] <- tbl } } diff --git a/pkg-r/tests/testthat/test-DataDict.R b/pkg-r/tests/testthat/test-DataDict.R index 858680e41..fcb13a81a 100644 --- a/pkg-r/tests/testthat/test-DataDict.R +++ b/pkg-r/tests/testthat/test-DataDict.R @@ -96,6 +96,56 @@ describe("dd_tables_by_name()", { dd <- list(tables = list(list(description = "No name"))) expect_equal(dd_tables_by_name(dd), list()) }) + + it("keeps the first entry on duplicate names", { + dd <- list( + tables = list( + list(name = "orders", description = "First"), + list(name = "orders", description = "Second") + ) + ) + tables <- dd_tables_by_name(dd) + expect_equal(tables[["orders"]][["description"]], "First") + }) +}) + +describe("dd_check_spec_compat()", { + it("accepts the supported $version without warning", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines('$version: "0.1.0"', yaml_file) + expect_no_warning(read_data_dict(yaml_file)) + }) + + it("warns when $version is newer, but still reads the file", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines('$version: "99.0.0"', yaml_file) + expect_warning(dd <- read_data_dict(yaml_file), "best-effort basis") + expect_equal(dd[["$version"]], "99.0.0") + }) + + it("warns when $version predates the first spec version", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines('$version: "0.0.9"', yaml_file) + expect_warning(read_data_dict(yaml_file), "predates the first") + }) + + it("warns when $version is invalid", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines('$version: "not-a-version"', yaml_file) + expect_warning(read_data_dict(yaml_file), "invalid") + }) + + it("warns when tables uses the legacy mapping format", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines(c("tables:", " orders:", " columns: []"), yaml_file) + expect_warning(read_data_dict(yaml_file), "outdated format") + }) + + it("warns when a table entry has no name", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines(c("tables:", " - description: No name"), yaml_file) + expect_warning(read_data_dict(yaml_file), "will be ignored") + }) }) describe("data_dict_to_prompt_list()", { diff --git a/pkg-r/vignettes/context.Rmd b/pkg-r/vignettes/context.Rmd index 2ef20b402..8f9841c8d 100644 --- a/pkg-r/vignettes/context.Rmd +++ b/pkg-r/vignettes/context.Rmd @@ -45,7 +45,7 @@ qc$add_table(customers, "customers") ### Format -The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables` (a list of tables, each with a `name`), `relationships`, and `glossary`. +The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables` (a list of tables, each with a `name`), `relationships`, and `glossary`. The optional `$version` field declares the spec version the file conforms to; a file declaring a version newer than querychat understands warns at load time but is still read on a best-effort basis. ```yaml # data-dict.yaml From 714c4be34e002b3becfd97e8dfd633f2ee063f1f Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 17:35:00 -0500 Subject: [PATCH 08/11] fix(py): treat a null `$version` as absent rather than invalid --- pkg-py/src/querychat/_data_dict.py | 3 +++ pkg-py/tests/test_data_dict.py | 8 ++++++++ pkg-r/tests/testthat/test-DataDict.R | 6 ++++++ 3 files changed, 17 insertions(+) diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index ed1b3e212..bd377b250 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -216,6 +216,9 @@ def _parse_spec_version(value: Any) -> tuple[int, int, int] | None: def _check_spec_version(version: Any) -> None: + if version is None: + # `$version:` with no value means the same as omitting it + return parsed = _parse_spec_version(version) if parsed is None: warnings.warn( diff --git a/pkg-py/tests/test_data_dict.py b/pkg-py/tests/test_data_dict.py index a63b945b8..c2c0cba11 100644 --- a/pkg-py/tests/test_data_dict.py +++ b/pkg-py/tests/test_data_dict.py @@ -101,6 +101,14 @@ def test_invalid_spec_version_warns(tmp_path: Path) -> None: DataDict.from_yaml(f) +def test_null_spec_version_means_absent(tmp_path: Path) -> None: + f = tmp_path / "spec.yaml" + f.write_text("$version:\ntables: []\n") + with warnings.catch_warnings(): + warnings.simplefilter("error") + assert DataDict.from_yaml(f).tables == [] + + def test_known_spec_version_ok(tmp_path: Path) -> None: f = tmp_path / "spec.yaml" f.write_text('$version: "0.1.0"\ntables: []\n') diff --git a/pkg-r/tests/testthat/test-DataDict.R b/pkg-r/tests/testthat/test-DataDict.R index fcb13a81a..4dc24ed2a 100644 --- a/pkg-r/tests/testthat/test-DataDict.R +++ b/pkg-r/tests/testthat/test-DataDict.R @@ -116,6 +116,12 @@ describe("dd_check_spec_compat()", { expect_no_warning(read_data_dict(yaml_file)) }) + it("treats a null $version as absent", { + yaml_file <- withr::local_tempfile(fileext = ".yaml") + writeLines(c("$version:", "tables: []"), yaml_file) + expect_no_warning(read_data_dict(yaml_file)) + }) + it("warns when $version is newer, but still reads the file", { yaml_file <- withr::local_tempfile(fileext = ".yaml") writeLines('$version: "99.0.0"', yaml_file) From 266f5e19456cddbb0ee6ad6eacf2846ab1475689 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 17:38:31 -0500 Subject: [PATCH 09/11] refactor: use packaging.version for spec $version checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the hand-rolled version parser with packaging.version.Version (already in the dependency tree; now declared directly). Also split the floor comparison out into a fixed _FIRST_SPEC_VERSION / .dd_first_spec_version constant — comparing against the known-version constant would have wrongly warned on legitimate 0.1.0 files once the known version bumps. --- pkg-py/src/querychat/_data_dict.py | 42 +++++++++++------------------- pkg-r/R/DataDict.R | 6 ++++- pyproject.toml | 1 + 3 files changed, 21 insertions(+), 28 deletions(-) diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index bd377b250..068811196 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -5,6 +5,7 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Literal +from packaging.version import InvalidVersion, Version from pydantic import BaseModel, ConfigDict, Field, field_validator, model_validator if TYPE_CHECKING: @@ -190,37 +191,24 @@ class RelationshipSpec(BaseModel): https://data-dict.tidyverse.org/spec.html """ +# The first data-dict spec version ever released. A `$version` below this +# is not a real spec version. +_FIRST_SPEC_VERSION = Version("0.1.0") + # The newest data-dict spec version this version of querychat fully # understands. Files declaring a newer `$version` are still read, on a # best-effort basis (unknown fields are ignored), with a warning. Bump this # opportunistically to keep the warning calibrated — it is not a hard gate. -_KNOWN_SPEC_VERSION = (0, 1, 0) - - -def _parse_spec_version(value: Any) -> tuple[int, int, int] | None: - """ - Parse a `$version` value into a (major, minor, patch) tuple. - - Returns ``None`` when ``value`` is not a recognizable version string. - Short forms are zero-padded, so ``"0.1"`` parses as ``(0, 1, 0)``. - """ - if isinstance(value, bool) or not isinstance(value, (str, int, float)): - return None - parts = str(value).split(".") - if len(parts) > 3 or not all(p.isdigit() for p in parts): # noqa: PLR2004 - return None - nums = [int(p) for p in parts] - # Zero-pad short forms, so "0.1" parses as (0, 1, 0) - nums.extend([0] * (3 - len(nums))) - return (nums[0], nums[1], nums[2]) +_KNOWN_SPEC_VERSION = Version("0.1.0") def _check_spec_version(version: Any) -> None: if version is None: # `$version:` with no value means the same as omitting it return - parsed = _parse_spec_version(version) - if parsed is None: + try: + parsed = Version(str(version)) + except InvalidVersion: warnings.warn( f"This data dictionary has an invalid `$version`: {version!r}. " 'Expected a version like "0.1.0"; see ' @@ -228,7 +216,7 @@ def _check_spec_version(version: Any) -> None: stacklevel=3, ) return - if parsed < (0, 1, 0): + if parsed < _FIRST_SPEC_VERSION: warnings.warn( f'This data dictionary declares `$version: "{version}"`, which ' "predates the first data-dict spec version (0.1.0). See " @@ -237,14 +225,14 @@ def _check_spec_version(version: Any) -> None: ) return if parsed > _KNOWN_SPEC_VERSION: - known = ".".join(str(p) for p in _KNOWN_SPEC_VERSION) warnings.warn( f'This data dictionary declares `$version: "{version}"`, which is ' "newer than the newest data-dict spec version this version of " - f"querychat fully understands ({known}). The file is read on a " - "best-effort basis — spec fields querychat doesn't know about are " - "ignored. If anything looks off, upgrade querychat, or check the " - "file against the spec with `data-dict validate-spec `: " + f"querychat fully understands ({_KNOWN_SPEC_VERSION}). The file is " + "read on a best-effort basis — spec fields querychat doesn't know " + "about are ignored. If anything looks off, upgrade querychat, or " + "check the file against the spec with " + "`data-dict validate-spec `: " "https://data-dict.tidyverse.org/spec.html", stacklevel=3, ) diff --git a/pkg-r/R/DataDict.R b/pkg-r/R/DataDict.R index 09814d5ed..5d55e297a 100644 --- a/pkg-r/R/DataDict.R +++ b/pkg-r/R/DataDict.R @@ -26,6 +26,10 @@ read_data_dict <- function(path) { dd } +# The first data-dict spec version ever released. A `$version` below this +# is not a real spec version. +.dd_first_spec_version <- numeric_version("0.1.0") + # The newest data-dict spec version this version of querychat fully # understands. Files declaring a newer `$version` are still read, on a # best-effort basis (unknown fields are ignored), with a warning. Bump this @@ -59,7 +63,7 @@ dd_check_spec_compat <- function(dd) { "i" = "The file is read on a best-effort basis — spec fields querychat doesn't know about are ignored.", "i" = "If anything looks off, consider upgrading {.pkg querychat} or running {.code data-dict validate-spec }." )) - } else if (v < .dd_known_spec_version) { + } else if (v < .dd_first_spec_version) { cli::cli_warn(c( "!" = "Data dictionary declares {.field $version} {.val {version}}, which predates the first data-dict spec version (0.1.0).", "i" = "See {.url https://data-dict.tidyverse.org/spec.html}." diff --git a/pyproject.toml b/pyproject.toml index d8bea98de..4fd04f744 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ dependencies = [ "great-tables>=0.16.0", "pyyaml", "nbformat>=5.10.4", + "packaging", ] classifiers = [ "Programming Language :: Python", From 804e775f9d29e482bfc82454be53087a217e70d7 Mon Sep 17 00:00:00 2001 From: Carson Date: Thu, 15 Oct 2026 17:44:57 -0500 Subject: [PATCH 10/11] fix!: fail hard on old or malformed data-dict files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old/malformed files have a clear migration path, so error instead of warning — a warning at app startup is easy to miss, and reading on would silently produce a degraded dictionary. This also gives pkg-r and pkg-py a single uniform policy: old or malformed files fail with a descriptive error; files declaring a newer spec `$version` warn and are read on a best-effort basis. - Python: invalid and predating `$version` collapse into a single rejection in `_check_spec_version()` (the legacy mapping-form `tables` error is unchanged). - R: `dd_check_spec_compat()` now aborts — instead of warning — on an invalid or predating `$version`, legacy mapping-form `tables` (previously the whole dict was silently ignored), and table entries without a `name` (matching pydantic's required `name`). --- pkg-py/CHANGELOG.md | 2 +- pkg-py/docs/context.qmd | 2 +- pkg-py/src/querychat/_data_dict.py | 26 ++++++++---------- pkg-py/tests/test_data_dict.py | 8 +++--- pkg-r/NEWS.md | 2 +- pkg-r/R/DataDict.R | 41 ++++++++++++++-------------- pkg-r/man/read_data_dict.Rd | 5 ++++ pkg-r/tests/testthat/test-DataDict.R | 16 +++++------ pkg-r/vignettes/context.Rmd | 2 +- 9 files changed, 54 insertions(+), 50 deletions(-) diff --git a/pkg-py/CHANGELOG.md b/pkg-py/CHANGELOG.md index a5a116bbb..914a27b9f 100644 --- a/pkg-py/CHANGELOG.md +++ b/pkg-py/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Breaking changes -* `DataDict` now follows the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` is a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, `values` may be a value-to-label map, and `constraints` may include assertion objects. Files written in the older mapping format fail with a descriptive error explaining how to update. Files declaring a `$version` newer than the spec version querychat understands are still read on a best-effort basis (unknown fields are ignored), with a warning. (#316) +* `DataDict` now follows the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` is a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, `values` may be a value-to-label map, and `constraints` may include assertion objects. Files written in the older mapping format, or with a `$version` that is invalid or predates the first spec version, fail with a descriptive error explaining how to update. Files declaring a `$version` newer than the spec version querychat understands are still read on a best-effort basis (unknown fields are ignored), with a warning. (#316) ### New features diff --git a/pkg-py/docs/context.qmd b/pkg-py/docs/context.qmd index ec81d596a..bd0603d34 100644 --- a/pkg-py/docs/context.qmd +++ b/pkg-py/docs/context.qmd @@ -30,7 +30,7 @@ qc.add_table(customers, "customers") ### Format -The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. The optional `$version` field declares the spec version the file conforms to; a file declaring a version newer than querychat understands is still read on a best-effort basis (unknown fields are ignored), with a warning. +The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables`, `relationships`, and `glossary`. The optional `$version` field declares the spec version the file conforms to. A `$version` that is invalid or predates the first spec version fails with a descriptive error; a file declaring a version newer than querychat understands is still read on a best-effort basis (unknown fields are ignored), with a warning. ```{.yaml filename="data-dict.yaml"} $version: "0.1.0" diff --git a/pkg-py/src/querychat/_data_dict.py b/pkg-py/src/querychat/_data_dict.py index 068811196..90c0bcc48 100644 --- a/pkg-py/src/querychat/_data_dict.py +++ b/pkg-py/src/querychat/_data_dict.py @@ -209,21 +209,19 @@ def _check_spec_version(version: Any) -> None: try: parsed = Version(str(version)) except InvalidVersion: - warnings.warn( - f"This data dictionary has an invalid `$version`: {version!r}. " - 'Expected a version like "0.1.0"; see ' - "https://data-dict.tidyverse.org/spec.html", - stacklevel=3, + parsed = None + # Old or unreadable files fail hard: we can't establish they're readable, + # and there is a clear migration path (unlike newer files, which may only + # use additive features and are read on a best-effort basis below) + if parsed is None or parsed < _FIRST_SPEC_VERSION: + raise ValueError( + f"This data dictionary declares `$version: {version!r}`, which is " + "not a data-dict spec version this version of querychat can read " + f"(the first spec version is {_FIRST_SPEC_VERSION}). Update the " + "file to the current spec — `data-dict validate-spec ` " + "checks a file against the spec: " + "https://data-dict.tidyverse.org/spec.html" ) - return - if parsed < _FIRST_SPEC_VERSION: - warnings.warn( - f'This data dictionary declares `$version: "{version}"`, which ' - "predates the first data-dict spec version (0.1.0). See " - "https://data-dict.tidyverse.org/spec.html", - stacklevel=3, - ) - return if parsed > _KNOWN_SPEC_VERSION: warnings.warn( f'This data dictionary declares `$version: "{version}"`, which is ' diff --git a/pkg-py/tests/test_data_dict.py b/pkg-py/tests/test_data_dict.py index c2c0cba11..e8a827bef 100644 --- a/pkg-py/tests/test_data_dict.py +++ b/pkg-py/tests/test_data_dict.py @@ -89,15 +89,15 @@ def test_newer_spec_version_warns_but_is_read(tmp_path: Path) -> None: assert dd.tables == [] -def test_spec_version_predating_first_spec_warns() -> None: - with pytest.warns(UserWarning, match="predates the first"): +def test_spec_version_predating_first_spec_errors() -> None: + with pytest.raises(ValidationError, match="not a data-dict spec version"): DataDict.model_validate({"$version": "0.0.9"}) -def test_invalid_spec_version_warns(tmp_path: Path) -> None: +def test_invalid_spec_version_errors(tmp_path: Path) -> None: f = tmp_path / "spec.yaml" f.write_text('$version: "not-a-version"\n') - with pytest.warns(UserWarning, match=r"invalid `\$version`"): + with pytest.raises(ValidationError, match="not a data-dict spec version"): DataDict.from_yaml(f) diff --git a/pkg-r/NEWS.md b/pkg-r/NEWS.md index 31787230b..2b3dc65d1 100644 --- a/pkg-r/NEWS.md +++ b/pkg-r/NEWS.md @@ -2,7 +2,7 @@ ## Breaking changes -* Data dictionaries now follow the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` must be a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, and `values` may be a value-to-label map. Update older dictionary files accordingly; `data-dict validate-spec` (from the data-dict CLI) checks a file against the spec. Files in the older mapping format, or declaring a `$version` newer than the spec version querychat understands, warn at load time but are still read on a best-effort basis. (#316) +* Data dictionaries now follow the current [data-dict spec](https://data-dict.tidyverse.org/spec.html): `tables` must be a list of tables each with a `name` field (previously a mapping of name to definition), column `range` is a two-element `[min, max]` list, and `values` may be a value-to-label map. Update older dictionary files accordingly; `data-dict validate-spec` (from the data-dict CLI) checks a file against the spec. Files in the older mapping format, or with a `$version` that is invalid or predates the first spec version, fail with a descriptive error; files declaring a newer `$version` warn at load time but are still read on a best-effort basis. (#316) ## Bug fixes diff --git a/pkg-r/R/DataDict.R b/pkg-r/R/DataDict.R index 5d55e297a..6fe93ecba 100644 --- a/pkg-r/R/DataDict.R +++ b/pkg-r/R/DataDict.R @@ -8,6 +8,11 @@ #' #' If `name` is absent from the YAML file, it defaults to the file stem. #' +#' Files written for older formats (a mapping-form `tables`, table entries +#' without a `name`, or a `$version` that is invalid or predates the first +#' spec version) fail with a descriptive error. Files declaring a newer spec +#' `$version` warn and are read on a best-effort basis. +#' #' @param path Path to the YAML file. #' #' @return A named list with the structure of the YAML file. @@ -36,12 +41,12 @@ read_data_dict <- function(path) { # opportunistically to keep the warning calibrated — it is not a hard gate. .dd_known_spec_version <- numeric_version("0.1.0") -#' Warn about data-dict spec compatibility problems. +#' Check a data dict for spec compatibility problems. #' -#' pkg-r intentionally does not fully validate data dictionaries, but a few -#' problems are worth surfacing because they otherwise fail silently: a -#' `$version` newer than the spec version querychat understands, `tables` in -#' the legacy mapping format, and table entries without a `name`. +#' Old or malformed files fail hard — there is a clear migration path, and +#' reading on would silently produce a degraded dictionary. Files declaring +#' a newer spec `$version` are only warned about: they may use purely +#' additive features, so they are read on a best-effort basis. #' #' @param dd A data dict list (from [yaml::read_yaml()]). #' @noRd @@ -52,22 +57,19 @@ dd_check_spec_compat <- function(dd) { numeric_version(as.character(version)), error = function(e) NULL ) - if (is.null(v)) { - cli::cli_warn(c( - "!" = "Data dictionary has an invalid {.field $version}: {.val {version}}.", - "i" = "Expected a version like {.val 0.1.0}; see {.url https://data-dict.tidyverse.org/spec.html}." + if (is.null(v) || v < .dd_first_spec_version) { + cli::cli_abort(c( + "Data dictionary declares {.field $version} {.val {version}}, which is not a data-dict spec version this version of {.pkg querychat} can read.", + "i" = "The first spec version is {.val {as.character(.dd_first_spec_version)}}; update the file to the current spec ({.url https://data-dict.tidyverse.org/spec.html}).", + "i" = "{.code data-dict validate-spec } checks a file against the spec." )) - } else if (v > .dd_known_spec_version) { + } + if (v > .dd_known_spec_version) { cli::cli_warn(c( "!" = "Data dictionary declares {.field $version} {.val {version}}, which is newer than the newest data-dict spec version this version of {.pkg querychat} fully understands ({.val {as.character(.dd_known_spec_version)}}).", "i" = "The file is read on a best-effort basis — spec fields querychat doesn't know about are ignored.", "i" = "If anything looks off, consider upgrading {.pkg querychat} or running {.code data-dict validate-spec }." )) - } else if (v < .dd_first_spec_version) { - cli::cli_warn(c( - "!" = "Data dictionary declares {.field $version} {.val {version}}, which predates the first data-dict spec version (0.1.0).", - "i" = "See {.url https://data-dict.tidyverse.org/spec.html}." - )) } } @@ -78,12 +80,11 @@ dd_check_spec_compat <- function(dd) { table_names <- names(tables) if (!is.null(table_names) && any(nzchar(table_names))) { - cli::cli_warn(c( - "!" = "Data dictionary {.field tables} uses an outdated format and will be ignored.", + cli::cli_abort(c( + "Data dictionary {.field tables} uses an outdated format.", "i" = "{.field tables} must be a list of tables, each with a {.field name} field — not a mapping of name to definition.", "i" = "Update the file to the current spec ({.url https://data-dict.tidyverse.org/spec.html}); {.code data-dict validate-spec } checks a file against the spec." )) - return(invisible(dd)) } unnamed <- vapply( @@ -95,8 +96,8 @@ dd_check_spec_compat <- function(dd) { logical(1) ) if (any(unnamed)) { - cli::cli_warn(c( - "!" = "{sum(unnamed)} table entr{?y/ies} in the data dictionary ha{?s/ve} no {.field name} and will be ignored.", + cli::cli_abort(c( + "{sum(unnamed)} table entr{?y/ies} in the data dictionary ha{?s/ve} no {.field name}.", "i" = "Each entry in {.field tables} needs a {.field name}; see {.url https://data-dict.tidyverse.org/spec.html}." )) } diff --git a/pkg-r/man/read_data_dict.Rd b/pkg-r/man/read_data_dict.Rd index 25c43271d..4deccba9c 100644 --- a/pkg-r/man/read_data_dict.Rd +++ b/pkg-r/man/read_data_dict.Rd @@ -19,4 +19,9 @@ returned as a plain list and can be passed directly to \link{QueryChat} via the \code{data_dict} argument. If \code{name} is absent from the YAML file, it defaults to the file stem. + +Files written for older formats (a mapping-form \code{tables}, table entries +without a \code{name}, or a \verb{$version} that is invalid or predates the first +spec version) fail with a descriptive error. Files declaring a newer spec +\verb{$version} warn and are read on a best-effort basis. } diff --git a/pkg-r/tests/testthat/test-DataDict.R b/pkg-r/tests/testthat/test-DataDict.R index 4dc24ed2a..5d62162be 100644 --- a/pkg-r/tests/testthat/test-DataDict.R +++ b/pkg-r/tests/testthat/test-DataDict.R @@ -129,28 +129,28 @@ describe("dd_check_spec_compat()", { expect_equal(dd[["$version"]], "99.0.0") }) - it("warns when $version predates the first spec version", { + it("errors when $version predates the first spec version", { yaml_file <- withr::local_tempfile(fileext = ".yaml") writeLines('$version: "0.0.9"', yaml_file) - expect_warning(read_data_dict(yaml_file), "predates the first") + expect_error(read_data_dict(yaml_file), "can read") }) - it("warns when $version is invalid", { + it("errors when $version is invalid", { yaml_file <- withr::local_tempfile(fileext = ".yaml") writeLines('$version: "not-a-version"', yaml_file) - expect_warning(read_data_dict(yaml_file), "invalid") + expect_error(read_data_dict(yaml_file), "can read") }) - it("warns when tables uses the legacy mapping format", { + it("errors when tables uses the legacy mapping format", { yaml_file <- withr::local_tempfile(fileext = ".yaml") writeLines(c("tables:", " orders:", " columns: []"), yaml_file) - expect_warning(read_data_dict(yaml_file), "outdated format") + expect_error(read_data_dict(yaml_file), "outdated format") }) - it("warns when a table entry has no name", { + it("errors when a table entry has no name", { yaml_file <- withr::local_tempfile(fileext = ".yaml") writeLines(c("tables:", " - description: No name"), yaml_file) - expect_warning(read_data_dict(yaml_file), "will be ignored") + expect_error(read_data_dict(yaml_file), "has no") }) }) diff --git a/pkg-r/vignettes/context.Rmd b/pkg-r/vignettes/context.Rmd index 8f9841c8d..1f4237e89 100644 --- a/pkg-r/vignettes/context.Rmd +++ b/pkg-r/vignettes/context.Rmd @@ -45,7 +45,7 @@ qc$add_table(customers, "customers") ### Format -The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables` (a list of tables, each with a `name`), `relationships`, and `glossary`. The optional `$version` field declares the spec version the file conforms to; a file declaring a version newer than querychat understands warns at load time but is still read on a best-effort basis. +The file format is defined by the [data-dict spec](https://data-dict.tidyverse.org/spec.html). A data dictionary has three top-level sections: `tables` (a list of tables, each with a `name`), `relationships`, and `glossary`. The optional `$version` field declares the spec version the file conforms to. A `$version` that is invalid or predates the first spec version fails with a descriptive error; a file declaring a version newer than querychat understands warns at load time but is still read on a best-effort basis. ```yaml # data-dict.yaml From 29b582ff215e891b396b968ea4c54ae811c54d4a Mon Sep 17 00:00:00 2001 From: Carson Date: Mon, 19 Oct 2026 10:18:26 -0500 Subject: [PATCH 11/11] fix(r): replace non-ASCII em-dashes in DataDict.R to fix R CMD check WARNING --- pkg-r/R/DataDict.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg-r/R/DataDict.R b/pkg-r/R/DataDict.R index 6fe93ecba..ea047b5ee 100644 --- a/pkg-r/R/DataDict.R +++ b/pkg-r/R/DataDict.R @@ -38,12 +38,12 @@ read_data_dict <- function(path) { # The newest data-dict spec version this version of querychat fully # understands. Files declaring a newer `$version` are still read, on a # best-effort basis (unknown fields are ignored), with a warning. Bump this -# opportunistically to keep the warning calibrated — it is not a hard gate. +# opportunistically to keep the warning calibrated -- it is not a hard gate. .dd_known_spec_version <- numeric_version("0.1.0") #' Check a data dict for spec compatibility problems. #' -#' Old or malformed files fail hard — there is a clear migration path, and +#' Old or malformed files fail hard -- there is a clear migration path, and #' reading on would silently produce a degraded dictionary. Files declaring #' a newer spec `$version` are only warned about: they may use purely #' additive features, so they are read on a best-effort basis. @@ -67,7 +67,7 @@ dd_check_spec_compat <- function(dd) { if (v > .dd_known_spec_version) { cli::cli_warn(c( "!" = "Data dictionary declares {.field $version} {.val {version}}, which is newer than the newest data-dict spec version this version of {.pkg querychat} fully understands ({.val {as.character(.dd_known_spec_version)}}).", - "i" = "The file is read on a best-effort basis — spec fields querychat doesn't know about are ignored.", + "i" = "The file is read on a best-effort basis \u2014 spec fields querychat doesn't know about are ignored.", "i" = "If anything looks off, consider upgrading {.pkg querychat} or running {.code data-dict validate-spec }." )) } @@ -82,7 +82,7 @@ dd_check_spec_compat <- function(dd) { if (!is.null(table_names) && any(nzchar(table_names))) { cli::cli_abort(c( "Data dictionary {.field tables} uses an outdated format.", - "i" = "{.field tables} must be a list of tables, each with a {.field name} field — not a mapping of name to definition.", + "i" = "{.field tables} must be a list of tables, each with a {.field name} field \u2014 not a mapping of name to definition.", "i" = "Update the file to the current spec ({.url https://data-dict.tidyverse.org/spec.html}); {.code data-dict validate-spec } checks a file against the spec." )) }