diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fd950f9..a9fc1cc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,24 @@ jobs: - name: Test run: uv run pytest -v + floor: + name: Test at the declared dependency floor + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + + - uses: astral-sh/setup-uv@d4b2f3b6ecc6e67c4457f6d3e41ec42d3d0fcb86 # v6 + with: + enable-cache: true + + - name: Set up Python + run: uv python install 3.11 + + # `uv sync --locked` installs the resolved ceiling, so nothing else here exercises + # the versions `pyproject.toml` actually advertises support for. + - name: Test + run: uv run --isolated --resolution lowest-direct --all-groups --python 3.11 pytest -q + checks: name: Lint and types runs-on: ubuntu-latest diff --git a/CHANGELOG.md b/CHANGELOG.md index a0c88f6..5ef5a1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,82 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`keys` and `expires_at` on `hotdata_create_managed_database`.** Both were available on the + framework call underneath and neither was reachable from the tool. `keys` declares a table's + natural key, which can only be set at creation: a table declared without one is keyless for + the rest of its life, so `upsert`, `update` and `delete` are rejected against it and an agent + cannot make a re-run idempotent. `expires_at` takes an RFC 3339 timestamp or a relative window + such as `"24h"`, which makes lifetime a property of the database rather than a cleanup + script's problem. + +- **`mode` and `key` on `hotdata_load_managed_table`.** The load hardcoded `mode="replace"`, so + `append`, `upsert`, `update` and `delete` were unreachable and an agent could not top up a + table it had already loaded. A keyed mode called without `key` now raises before the file is + uploaded rather than being rejected by the engine at the far end of a transfer that had + already happened. + +- **`DESTRUCTIVE_TOOL_NAMES`, and `metadata={"destructive": True}` on the tools it names.** + `HumanInTheLoopMiddleware(interrupt_on=...)` is keyed by tool name, so wiring approval meant + inferring the mutating set from naming. Only the load tool is in it: creating a database makes + something new rather than overwriting something existing. The constant holds the *default* + names, so a set built with `tool_name_suffix` should be filtered on the metadata instead — + the README shows both. + +- **`TablePartitionKey` and `TableSortKey` re-exported**, since both now appear in + `hl.create_managed_database`'s signature and a caller should not need a second import from a + package this one's docs do not name. + +- **`partition_by` and `sorted_by` on `hl.create_managed_database`.** Reachable from Python, + and deliberately not offered to a model: layout is permanent, the API has no ALTER path, and + undoing a choice means deleting the table and reloading it, which burns the table name in + that database. A model has no basis for choosing a partition transform and cannot undo a + wrong one. + +### Fixed + +- **Building the tools raised on `langchain-core` 1.0.0**, the version this package declares as + its floor. Its docstring parser reads a colon in a wrapped `Args:` continuation line as the + start of a new argument, so `Arg one by key in docstring not found in function signature` + aborted every `make_hotdata_tools` call. Newer `langchain-core` parses it without complaint, + which is why the locked build never saw it. Found by running the suite at the floor, not from + a report. + +### Changed + +- **CI now runs the suite at the declared dependency floor** with + `uv --resolution lowest-direct`, alongside the existing matrix. `uv sync --locked` installs + the resolved ceiling, so until now nothing exercised the versions `pyproject.toml` advertises + support for — which is how the `langchain-core` break above reached a release, and how the + framework floor drifted three minor versions behind what the package was developed against. + +- **`hotdata-framework>=0.13.0`** (from `>=0.10.0`). 0.13.0 is where an `append` load became + retryable — before it, `append` was excluded from retries, so a load that hit + `409 RESOURCE_LOCKED` failed whatever `max_retries` was set to. This release makes `append` + reachable from a tool, so the floor moves with it. It also brings terminal-vs-transient 409 + classification and `Retry-After` handling. The suite passes on every version from the old + floor to this one; the bump is about the failure modes the new load modes can hit, not a + broken build. + +### Notes + +- `format` and `result_id` on the load are named in the issue this closes and are not here. + They exist on the `LoadManagedTableRequest` model but on **no** released version of the + framework client, which hardcodes the fields it forwards — so they need an upstream change + rather than a wider signature in this package. + +- The load tool now states what each keyed mode does to a matched row rather than naming the + three together. `delete` removes matched rows and inserts nothing, which the previous wording + left open to reading as delete-then-insert by key — a misreading that destroys rows and + reports success. A `keys` entry naming an undeclared table is now refused rather than + silently creating a keyless table, which cannot be corrected afterwards. + +- Retrying a failed `append` from an agent still duplicates rows, and no version fixes that. + Re-sending the same upload replays the server's receipt, but a tool call has no memory across + turns, so a repeat stages a fresh upload with no receipt to replay. The load tool's + description now says so, and points at `replace` or a keyed `upsert` instead. + ## [0.14.0] - 2026-08-31 ### Added diff --git a/README.md b/README.md index 2811619..ad685e1 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ id of each. | `hotdata_execute_sql` | Run a SQL query and return rows as JSON | | `hotdata_list_managed_databases` | List available instant databases, with the id of each | | `hotdata_create_managed_database` | Create a new instant database and return its id | -| `hotdata_load_managed_table` | Load a parquet file — local path or URL — into a managed table, addressed by database id | +| `hotdata_load_managed_table` | Load a parquet file — local path or URL — into a managed table, addressed by database id, replacing or merging with what it holds | | `hotdata_describe_tables` | List tables, or one table's columns, types and how many rows hold a value | | `hotdata_search_text` | Search an indexed column by text relevance, fused with meaning where the table supports it (opt-in — see below) | | `hotdata_search_semantic` | Search an indexed column by meaning; replaces the above when the column carries a vector index | @@ -79,6 +79,77 @@ tools = hl.make_hotdata_tools(client, database_id="dbid...", describe_tables=Fal listing databases is itself a read, so the set it removes is the instant-database workflow rather than everything that writes. +### Keys and lifetime are set once, at creation + +A table's natural key can only be declared when the database is created. A table made without +one is keyless for the rest of its life, and `upsert`, `update` and `delete` are rejected +against it — so an agent that will load the same table twice needs `keys` on the first call or +it can never make a re-run idempotent. `expires_at` takes an RFC 3339 timestamp or a relative +window like `"24h"`; without it the database lives until something deletes it, which turns +lifetime into a cleanup script rather than a property of the thing created. + +A keyed load with no `key` raises before the file is uploaded rather than after, since the +engine would reject it at the far end of a transfer that had already happened. A `keys` entry +naming a table the same call is not declaring is refused too, since it could never take effect. + +The three keyed modes match an incoming row to an existing one and then differ in what they do +with it, so they are worth stating separately: + +| `mode` | Matched row | Row that matches nothing | +|---|---|---| +| `upsert` | replaced | inserted | +| `update` | replaced | ignored | +| `delete` | **removed** | ignored | + +`delete` inserts nothing. The rows you upload choose which existing rows to remove; they are +not added to the table. + +**Do not repeat a failed `append`.** Re-sending the same upload replays the server's receipt +instead of applying the load twice, but a tool call has no memory across turns: a repeat stages +a fresh upload, which has no receipt to replay, and the rows land a second time. `replace` and +a keyed `upsert` both reach the same state however many times they run, so prefer those for +anything an agent might retry — and `handle_errors=True` makes a retry likely, since the +failure goes back to the model rather than ending the run. + +`partition_by` and `sorted_by` are on `hl.create_managed_database` but not on the create tool. +Layout is permanent — the API has no ALTER path, and undoing a choice means deleting the table +and reloading it, which burns the table name in that database — so it is set by whoever builds +the tools, not chosen per call by a model: + +```python + +db = hl.create_managed_database( + client, name="events", tables=["spans"], + keys={"spans": ["span_id"]}, + sorted_by={"spans": [hl.TableSortKey(column="start_time")]}, +) +``` + +### Wiring approval around the tools that can destroy data + +`HumanInTheLoopMiddleware(interrupt_on=...)` is keyed by tool name, so the mutating set has to +be readable rather than inferred from naming: + +```python +from langchain.agents.middleware import HumanInTheLoopMiddleware + +middleware = HumanInTheLoopMiddleware( + interrupt_on=dict.fromkeys(hl.DESTRUCTIVE_TOOL_NAMES, True), +) +``` + +`DESTRUCTIVE_TOOL_NAMES` holds the **default** names. A set built with `tool_name_suffix` +carries different ones, so read the marking off the built tools instead: + +```python +tools = hl.make_hotdata_tools(client, database_id="dbid...", tool_name_suffix="sales") +names = [t.name for t in tools if (t.metadata or {}).get("destructive")] +``` + +Creating a database is not in the set. It makes something new rather than overwriting +something existing, and gating it would put an approval in front of the one call an agent has +to make before it can do anything at all. + ## Letting the model recover from a failed call The tools raise on failure, which is right in a script and wrong in an agent: an exception out @@ -207,12 +278,16 @@ created = tools["hotdata_create_managed_database"].invoke({ "name": "sales", # a display label, not an identifier "schema_name": "public", "tables": "orders,customers", + "keys": {"orders": ["id"]}, # only settable at creation + "expires_at": "7d", # or an RFC 3339 timestamp; omit to keep it forever }) tools["hotdata_load_managed_table"].invoke({ "database_id": json.loads(created)["id"], "table": "orders", "file": "/path/to/orders.parquet", # or "https://example.com/orders.parquet" + "mode": "upsert", # default is "replace" + "key": ["id"], }) ``` @@ -650,9 +725,9 @@ tools = hl.make_hotdata_tools(client, database_id="dbid...") **Databases are addressed by id, never by name.** A database name is a display label and is not unique, so a name lookup can silently resolve to the wrong database — and the agent's -`hotdata_load_managed_table` overwrites the table it loads into. Passing a name raises -`KeyError` — or, under `handle_errors=True`, returns it to the model rather than resolving -anything. Ids come from `client.list_managed_databases()`, the +`hotdata_load_managed_table` overwrites the table it loads into unless a mode says otherwise. +Passing a name raises `KeyError` — or, under `handle_errors=True`, returns it to the model +rather than resolving anything. Ids come from `client.list_managed_databases()`, the `hotdata_list_managed_databases` tool, or the response of a create. The id is resolved once when the tools are built, so a bad id fails there rather than on the diff --git a/hotdata_langchain/__init__.py b/hotdata_langchain/__init__.py index 00777d4..1a47d7f 100644 --- a/hotdata_langchain/__init__.py +++ b/hotdata_langchain/__init__.py @@ -7,10 +7,18 @@ except PackageNotFoundError: __version__ = "0.0.0+unknown" -from hotdata_framework import HotdataClient, ManagedDatabase, QueryResult, from_env +from hotdata_framework import ( + HotdataClient, + ManagedDatabase, + QueryResult, + TablePartitionKey, + TableSortKey, + from_env, +) from hotdata_langchain._sql import DISTANCE_FUNCTIONS, DistanceMetric from hotdata_langchain.databases import ( + LoadMode, create_managed_database, list_managed_databases_json, load_managed_table, @@ -75,6 +83,7 @@ DEFAULT_LIST_DATABASES_TOOL_NAME, DEFAULT_LOAD_TABLE_TOOL_NAME, DEFAULT_SQL_TOOL_NAME, + DESTRUCTIVE_TOOL_NAMES, execute_sql_json, make_hotdata_tools, result_rows_for_llm, @@ -94,6 +103,7 @@ "DEFAULT_SEARCH_TOOL_NAME", "DEFAULT_SEMANTIC_TOOL_NAME", "DEFAULT_SQL_TOOL_NAME", + "DESTRUCTIVE_TOOL_NAMES", "DISTANCE_COLUMN", "DISTANCE_FUNCTIONS", "RRF_K", @@ -106,12 +116,15 @@ "HotdataClient", "HotdataToolError", "HotdataVectorStore", + "LoadMode", "ManagedDatabase", "QueryResult", "SearchIndex", "SearchRoute", "SearchStrategy", "SearchableColumn", + "TablePartitionKey", + "TableSortKey", "__version__", "bm25_search_json", "bm25_search_sql", diff --git a/hotdata_langchain/databases.py b/hotdata_langchain/databases.py index b0bc9f0..550dab9 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -7,8 +7,9 @@ import logging import socket import tempfile +from collections.abc import Sequence from pathlib import Path -from typing import Any +from typing import Any, Literal from urllib.parse import urlsplit from urllib.request import HTTPRedirectHandler, Request, build_opener @@ -19,6 +20,8 @@ HotdataClient, LoadManagedTableResult, ManagedDatabase, + TablePartitionKey, + TableSortKey, ) from hotdata_framework.databases import api_error_message, managed_database_from_detail @@ -29,6 +32,16 @@ "WHERE table_schema <> 'information_schema'" ) +#: How a load treats the rows a table already holds. ``replace`` discards them and +#: ``append`` keeps them. The other three match an incoming row against an existing one +#: by the table's declared key, so they are legal only on a table that has one: +#: ``upsert`` replaces a matched row and inserts one that matches nothing, ``update`` +#: replaces a matched row only, and ``delete`` removes a matched row and inserts nothing. +LoadMode = Literal["replace", "append", "upsert", "update", "delete"] + +#: Load modes that match rows by key, and so require one. +KEYED_LOAD_MODES: frozenset[str] = frozenset({"upsert", "update", "delete"}) + URL_SCHEMES = ("http://", "https://") PARQUET_MAGIC = b"PAR1" FETCH_TIMEOUT_SECONDS = 30.0 @@ -156,12 +169,39 @@ def create_managed_database( name: str, schema: str = DEFAULT_SCHEMA, tables: list[str] | None = None, + keys: dict[str, list[str]] | None = None, + expires_at: str | None = None, + partition_by: dict[str, Sequence[TablePartitionKey]] | None = None, + sorted_by: dict[str, Sequence[TableSortKey]] | None = None, ) -> ManagedDatabase: """Create an instant database, labelled ``name``. ``name`` is a display label only; address the result by its ``id`` from here on. + + ``keys`` declares each table's natural key, mapping a table name to its key columns. + A key can only be declared here, at creation: a table created without one is keyless + for the rest of its life, and every key-matched load mode is rejected against it. + + ``expires_at`` is an RFC 3339 timestamp or a relative window such as ``"24h"`` or + ``"7d"``, after which the database is reaped. Without it the database lives until + something deletes it, which makes lifetime a cleanup script's problem rather than a + property of the thing created. + + ``partition_by`` and ``sorted_by`` set a table's physical layout, and both are + permanent: the API has no ALTER path, so the only way to change one is to delete the + table and reload it, which burns the table name in that database. They are reachable + here and deliberately not offered to a model — see + :func:`~hotdata_langchain.tools.make_hotdata_tools`. """ - return client.create_managed_database(description=name, schema=schema, tables=tables) + return client.create_managed_database( + description=name, + schema=schema, + tables=tables, + keys=keys, + expires_at=expires_at, + partition_by=partition_by, + sorted_by=sorted_by, + ) def is_url(file: str) -> bool: @@ -325,6 +365,8 @@ def load_managed_table( table: str, file: str, schema: str = DEFAULT_SCHEMA, + mode: LoadMode = "replace", + key: list[str] | None = None, allow_private_hosts: bool = False, ) -> LoadManagedTableResult: """Load a parquet file into a declared table of the database with that id. @@ -339,14 +381,28 @@ def load_managed_table( ``database_id`` is resolved by id (see :func:`resolve_database_by_id`) and the resolved record is what addresses the load, so a display label never selects the - target. This load replaces the table's contents, which is why addressing it + target. The default load replaces the table's contents, which is why addressing it unambiguously matters. + + ``mode`` chooses what happens to rows already there, and :data:`LoadMode` gives each + one's effect. The three keyed modes need ``key`` and are rejected unless the table was + declared with one, which can only happen at creation. Note that ``delete`` inserts + nothing: it uses the uploaded rows to choose which existing rows to remove. Raises + ``ValueError`` for a keyed mode called without ``key``, rather than letting the engine + reject it after the file has been uploaded. """ + if mode in KEYED_LOAD_MODES and not key: + raise ValueError( + f"mode={mode!r} matches rows by key, so 'key' is required. Pass the column " + "names the table was declared with, or use mode='replace' or 'append'." + ) database = resolve_database_by_id(client, database_id) if is_url(file): path = fetch_parquet(file, allow_private_hosts=allow_private_hosts) try: - return client.load_managed_table(database, table, schema=schema, file=path) + return client.load_managed_table( + database, table, schema=schema, file=path, mode=mode, key=key + ) finally: Path(path).unlink(missing_ok=True) if not Path(file).is_file(): @@ -354,7 +410,7 @@ def load_managed_table( f"no file at {file!r}. Pass a path to a local parquet file, or an http:// or " "https:// URL to one — other formats are not accepted." ) - return client.load_managed_table(database, table, schema=schema, file=file) + return client.load_managed_table(database, table, schema=schema, file=file, mode=mode, key=key) def managed_database_summary(db: ManagedDatabase) -> dict[str, str]: diff --git a/hotdata_langchain/tools.py b/hotdata_langchain/tools.py index 5e5d093..e1667ef 100644 --- a/hotdata_langchain/tools.py +++ b/hotdata_langchain/tools.py @@ -14,6 +14,7 @@ from hotdata_langchain._sql import format_pattern_warnings from hotdata_langchain.databases import ( + LoadMode, create_managed_database, database_label, list_managed_databases_json, @@ -58,6 +59,17 @@ DEFAULT_CREATE_DATABASE_TOOL_NAME = "hotdata_create_managed_database" DEFAULT_LOAD_TABLE_TOOL_NAME = "hotdata_load_managed_table" +#: Tools that can destroy data a caller already has, for wiring approval around. +#: Pass straight to ``HumanInTheLoopMiddleware(interrupt_on=...)``, which is keyed by +#: tool name. These are the default names: a tool set built with ``tool_name_suffix`` +#: carries different ones, so read ``tool.metadata["destructive"]`` off the built tools +#: instead of matching against this set. +#: +#: Creating a database is not here. It makes something new rather than overwriting +#: something existing, and gating it would put an approval in front of the one call an +#: agent has to make before it can do anything at all. +DESTRUCTIVE_TOOL_NAMES: frozenset[str] = frozenset({DEFAULT_LOAD_TABLE_TOOL_NAME}) + logger = logging.getLogger(__name__) @@ -477,7 +489,15 @@ def make_hotdata_tools( databases themselves — listing, creating and loading. Turn it off for an agent that reads one fixed database, where they are surface the model can only misuse. The flag is not called ``read_only``: listing databases is itself a read, so the set it removes - is the instant-database workflow rather than everything that writes. + is the instant-database workflow rather than everything that writes. The load tool + carries ``metadata={"destructive": True}``; :data:`DESTRUCTIVE_TOOL_NAMES` holds the + same set under the default names. + + The create tool takes ``keys`` and ``expires_at`` but not ``partition_by`` or + ``sorted_by``, which :func:`~hotdata_langchain.databases.create_managed_database` + accepts. Layout is permanent — the API has no ALTER path, and undoing a choice means + deleting the table and reloading it, which burns the table name in that database — + so it is set by the caller building the tools, not chosen per call by a model. ``handle_errors`` returns each tool's failures as ``{"error": ""}`` instead of raising. An exception out of a tool aborts the whole agent run, so one @@ -574,6 +594,8 @@ def hotdata_create_managed_database( name: str, schema_name: str = DEFAULT_SCHEMA, tables: str = "", + keys: dict[str, list[str]] | None = None, + expires_at: str = "", ) -> str: """Create an instant database and optionally declare tables. @@ -582,13 +604,28 @@ def hotdata_create_managed_database( response carries the id every other tool needs. schema_name: schema the declared tables live in. tables: table names to declare up front, comma- or newline-separated. + keys: each table's natural key, as a table name mapped to its key columns. + A key can only be set here. A table declared without one can never be + loaded with upsert, update or delete. + expires_at: when to reap the database, as an RFC 3339 timestamp or a + relative window such as '24h' or '7d'. Left empty it lives until + something deletes it. """ table_names = [t.strip() for t in tables.replace(",", "\n").splitlines() if t.strip()] + undeclared = sorted(set(keys or {}) - set(table_names)) + if undeclared: + raise ValueError( + f"keys names {undeclared}, which is not among the declared tables " + f"{table_names}. A key can only be set when its table is declared, so a " + "key on a table that is not created here can never take effect." + ) db = create_managed_database( client, name=name, schema=schema_name or DEFAULT_SCHEMA, tables=table_names or None, + keys=keys or None, + expires_at=expires_at or None, ) return json.dumps(managed_database_summary(db), indent=2) @@ -597,17 +634,25 @@ def hotdata_load_managed_table( table: str, file: str, schema_name: str = DEFAULT_SCHEMA, + mode: LoadMode = "replace", + key: list[str] | None = None, ) -> str: """Load a parquet file, local or at a URL, into a declared managed table. Args: database_id: id of the target database, as returned by listing or creating one; a database name is rejected. - table: name of a table already declared on that database. The load replaces - whatever it holds. + table: name of a table already declared on that database. file: a local filesystem path, or an http:// or https:// URL, to a parquet file. Only parquet is accepted. schema_name: schema the table was declared in. + mode: what happens to rows already in the table. 'replace' discards them and + 'append' keeps them. The other three match an incoming row to an existing + one by the table's key. 'upsert' replaces a matched row and inserts one + that matches nothing, 'update' replaces a matched row only, and 'delete' + removes a matched row and inserts nothing. + key: the key columns to match on, required by upsert, update and delete. + They must be the columns the table was declared with. """ loaded = load_managed_table( client, @@ -615,6 +660,8 @@ def hotdata_load_managed_table( table=table, file=file, schema=schema_name or DEFAULT_SCHEMA, + mode=mode, + key=key or None, allow_private_hosts=allow_private_hosts, ) return json.dumps(load_result_summary(loaded), indent=2) @@ -710,7 +757,11 @@ def hotdata_load_managed_table( "label only and is not an identifier; the response carries the 'id', which " "is what every other tool needs — keep it. Declare the tables you intend " "to load up front as a comma- or newline-separated list, so data loads " - "straight into them." + "straight into them. Declare 'keys' at the same time for any table you " + "will load more than once: a key can only be set here, and a table " + "created without one can never be loaded with upsert, update or delete. " + "Set 'expires_at' when the data is temporary, so the database is reaped " + "rather than left behind." ), parse_docstring=True, ), @@ -719,15 +770,27 @@ def hotdata_load_managed_table( name=load_name, description=( "Load a parquet file into a table that was declared on an instant " - "database, replacing whatever the table held. 'file' is either a path on " + "database. By default this replaces whatever the table held; pass 'mode' " + "to keep it. 'append' adds rows blindly. The other three match an " + "incoming row to an existing one by key, so they need 'key' and work " + "only on a table that was declared with one: 'upsert' replaces a matched " + "row and inserts one that matches nothing, 'update' replaces a matched " + "row only, and 'delete' REMOVES a matched row and inserts nothing — the " + "rows you upload choose what is deleted, they are not added. " + "'file' is either a path on " "the local filesystem or an http:// or https:// URL, which is downloaded " f"and uploaded for you{url_rule}. 'database_id' must be a database id " f"returned by {list_name} or {create_name} — call one of those first if " "you do not have an id. A database name is rejected: " "names are not unique, and this load overwrites the table, so the wrong " - "target would destroy data. Only parquet is accepted, not CSV or JSON." + "target would destroy data. Only parquet is accepted, not CSV or JSON. " + "If a load fails without saying whether it landed, do not repeat it with " + "mode='append': the retry adds the rows a second time. Re-run 'replace', " + "or use 'upsert' with a key — both land the same rows however many " + "times they run." ), parse_docstring=True, + metadata={"destructive": True}, ), ] ) diff --git a/pyproject.toml b/pyproject.toml index d6b147f..a242574 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,8 @@ readme = "README.md" requires-python = ">=3.10" license = { text = "MIT" } dependencies = [ - "hotdata-framework>=0.10.0", + # 0.13.0 is where an `append` load became retryable; the tool exposes that mode. + "hotdata-framework>=0.13.0", "hotdata>=0.8.0", "langchain-core>=1.0", "numpy>=1.26", diff --git a/tests/test_database_ids.py b/tests/test_database_ids.py index 0f72237..3a88d5a 100644 --- a/tests/test_database_ids.py +++ b/tests/test_database_ids.py @@ -198,7 +198,14 @@ def load_tool(client: MagicMock) -> object: def test_load_tool_takes_a_database_id_argument(mock_client: MagicMock) -> None: """The argument name is what the model sees in the schema, so it must say 'id'.""" tool = load_tool(mock_client) - assert set(tool.args) == {"database_id", "table", "file", "schema_name"} # type: ignore[attr-defined] + assert set(tool.args) == { # type: ignore[attr-defined] + "database_id", + "table", + "file", + "schema_name", + "mode", + "key", + } def test_load_tool_resolves_the_agent_supplied_id_by_id( diff --git a/tests/test_tools.py b/tests/test_tools.py index 8fdb29d..d289a8b 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -1,18 +1,26 @@ from __future__ import annotations +import inspect import json import socket import tempfile from collections.abc import Callable from pathlib import Path -from typing import Any, NoReturn -from unittest.mock import MagicMock +from typing import Any, NoReturn, get_args +from unittest.mock import MagicMock, create_autospec from urllib.request import Request import pytest -from hotdata_framework import LoadManagedTableResult, ManagedDatabase, QueryResult +from hotdata_framework import ( + HotdataClient, + LoadManagedTableResult, + ManagedDatabase, + QueryResult, + TableSortKey, +) from hotdata_langchain.databases import ( + LoadMode, _ValidatingRedirectHandler, create_managed_database, fetch_parquet, @@ -28,6 +36,7 @@ DEFAULT_LIST_DATABASES_TOOL_NAME, DEFAULT_LOAD_TABLE_TOOL_NAME, DEFAULT_SQL_TOOL_NAME, + DESTRUCTIVE_TOOL_NAMES, execute_sql_json, make_hotdata_tools, result_rows_for_llm, @@ -73,6 +82,10 @@ def test_create_managed_database_delegates(mock_client: MagicMock) -> None: description="sales", schema="public", tables=["orders"], + keys=None, + expires_at=None, + partition_by=None, + sorted_by=None, ) assert db.description == "sales" @@ -113,6 +126,8 @@ def test_load_managed_table_delegates( "orders", schema="public", file=str(parquet_file), + mode="replace", + key=None, ) assert loaded.row_count == 3 @@ -640,3 +655,195 @@ def test_a_database_with_no_name_gets_no_sentence_rather_than_one_naming_its_id( def test_an_unscoped_tool_set_names_no_database(mock_client: MagicMock) -> None: tools = {t.name: t for t in make_hotdata_tools(mock_client)} assert "Works on the" not in (tools["hotdata_execute_sql"].description or "") + + +# --- Provisioning arguments the tools used to drop (#91) ------------------------------ + + +def test_create_passes_keys_and_expiry_through(mock_client: MagicMock) -> None: + """A key can only be declared at creation, so a tool that drops it makes a table + that can never take an upsert.""" + mock_client.create_managed_database.return_value = ManagedDatabase( + id="c1", description="sales", default_connection_id="conn_c1" + ) + create_managed_database( + mock_client, + name="sales", + tables=["orders"], + keys={"orders": ["id"]}, + expires_at="24h", + ) + kwargs = mock_client.create_managed_database.call_args.kwargs + assert kwargs["keys"] == {"orders": ["id"]} + assert kwargs["expires_at"] == "24h" + + +def test_load_passes_mode_and_key_through( + mock_client: MagicMock, managed_db: ManagedDatabase, parquet_file: Path +) -> None: + mock_client.load_managed_table.return_value = LoadManagedTableResult( + connection_id="c1", + schema_name="public", + table_name="orders", + row_count=3, + full_name="sales.public.orders", + ) + load_managed_table( + mock_client, + database_id=managed_db, + table="orders", + file=str(parquet_file), + mode="upsert", + key=["id"], + ) + kwargs = mock_client.load_managed_table.call_args.kwargs + assert kwargs["mode"] == "upsert" + assert kwargs["key"] == ["id"] + + +@pytest.mark.parametrize("mode", ["upsert", "update", "delete"]) +def test_a_keyed_mode_without_a_key_is_refused_before_upload( + mock_client: MagicMock, managed_db: ManagedDatabase, parquet_file: Path, mode: str +) -> None: + """The engine would reject it too, but only after the file had been uploaded.""" + with pytest.raises(ValueError, match="matches rows by key"): + load_managed_table( + mock_client, + database_id=managed_db, + table="orders", + file=str(parquet_file), + mode=mode, # type: ignore[arg-type] + ) + mock_client.load_managed_table.assert_not_called() + + +@pytest.mark.parametrize("mode", ["replace", "append"]) +def test_an_unkeyed_mode_needs_no_key( + mock_client: MagicMock, managed_db: ManagedDatabase, parquet_file: Path, mode: str +) -> None: + mock_client.load_managed_table.return_value = LoadManagedTableResult( + connection_id="c1", + schema_name="public", + table_name="orders", + row_count=3, + full_name="sales.public.orders", + ) + load_managed_table( + mock_client, + database_id=managed_db, + table="orders", + file=str(parquet_file), + mode=mode, # type: ignore[arg-type] + ) + assert mock_client.load_managed_table.call_args.kwargs["mode"] == mode + + +def test_the_load_tool_is_marked_destructive_and_the_create_tool_is_not( + mock_client: MagicMock, +) -> None: + """`interrupt_on` is keyed by tool name, so the mutating set has to be readable.""" + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + assert (tools[DEFAULT_LOAD_TABLE_TOOL_NAME].metadata or {}).get("destructive") is True + assert (tools[DEFAULT_CREATE_DATABASE_TOOL_NAME].metadata or {}).get("destructive") is None + assert {DEFAULT_LOAD_TABLE_TOOL_NAME} == DESTRUCTIVE_TOOL_NAMES + + +def test_the_destructive_marking_survives_a_tool_name_suffix(mock_client: MagicMock) -> None: + """The constant holds default names, so a suffixed set has to be read off metadata.""" + tools = make_hotdata_tools(mock_client, tool_name_suffix="sales") + marked = {t.name for t in tools if (t.metadata or {}).get("destructive")} + assert marked == {f"{DEFAULT_LOAD_TABLE_TOOL_NAME}_sales"} + assert not marked & DESTRUCTIVE_TOOL_NAMES + + +def test_the_load_tool_offers_every_mode_to_the_model(mock_client: MagicMock) -> None: + """A mode absent from the schema is one the model cannot reach. + + Read off `enum` rather than the serialised field: the field's description names every + mode in prose, so a substring check passes even with the enum gone. + """ + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + field = tools[DEFAULT_LOAD_TABLE_TOOL_NAME].args["mode"] + assert set(field["enum"]) == set(get_args(LoadMode)) + + +def test_the_load_tool_says_what_each_keyed_mode_does(mock_client: MagicMock) -> None: + """Naming the three together without an effect reads as delete-then-insert by key.""" + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + description = tools[DEFAULT_LOAD_TABLE_TOOL_NAME].description or "" + assert "inserts one that matches nothing" in description + assert "replaces a matched row only" in description + assert "REMOVES a matched row and inserts nothing" in description + + +def test_layout_reaches_the_helper_but_is_not_offered_to_the_model( + mock_client: MagicMock, +) -> None: + """Layout is permanent and has no ALTER path, so the caller sets it, not the model.""" + mock_client.create_managed_database.return_value = ManagedDatabase( + id="c1", description="sales", default_connection_id="conn_c1" + ) + create_managed_database( + mock_client, + name="sales", + tables=["orders"], + sorted_by={"orders": [TableSortKey(column="id")]}, + ) + assert mock_client.create_managed_database.call_args.kwargs["sorted_by"] == { + "orders": [TableSortKey(column="id")] + } + + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + offered = set(tools[DEFAULT_CREATE_DATABASE_TOOL_NAME].args) + assert {"keys", "expires_at"} <= offered + assert not offered & {"partition_by", "sorted_by"} + + +def test_the_load_tool_warns_against_repeating_a_failed_append(mock_client: MagicMock) -> None: + """`handle_errors=True` hands the failure back to the model, which invites a retry.""" + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + description = tools[DEFAULT_LOAD_TABLE_TOOL_NAME].description or "" + assert "append" in description + assert "second time" in description + + +def test_the_create_tool_refuses_a_key_on_a_table_it_is_not_declaring( + mock_client: MagicMock, +) -> None: + """A key can only be set at creation, so one aimed at no declared table is lost.""" + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + with pytest.raises(ValueError, match="not among the declared tables"): + tools[DEFAULT_CREATE_DATABASE_TOOL_NAME].invoke( + {"name": "sales", "tables": "orders", "keys": {"ordres": ["id"]}} + ) + mock_client.create_managed_database.assert_not_called() + + +def test_every_provisioning_call_binds_to_the_real_client_signature() -> None: + """A MagicMock accepts any keyword, so it cannot catch one the framework rejects. + + This is the failure `format` and `result_id` would have been: names that exist on the + request model but not on the client method that forwards it. + """ + spec = create_autospec(HotdataClient, instance=True) + spec.create_managed_database.return_value = ManagedDatabase( + id="c1", description="sales", default_connection_id="conn_c1" + ) + spec.load_managed_table.return_value = LoadManagedTableResult( + connection_id="c1", + schema_name="public", + table_name="orders", + row_count=1, + full_name="sales.public.orders", + ) + create_managed_database( + spec, + name="sales", + tables=["orders"], + keys={"orders": ["id"]}, + expires_at="24h", + sorted_by={"orders": [TableSortKey(column="id")]}, + ) + inspect.signature(HotdataClient.create_managed_database).bind( + spec, **spec.create_managed_database.call_args.kwargs + ) diff --git a/uv.lock b/uv.lock index 5cfbe58..afcede0 100644 --- a/uv.lock +++ b/uv.lock @@ -259,7 +259,7 @@ wheels = [ [[package]] name = "hotdata-framework" -version = "0.12.1" +version = "0.13.0" source = { registry = "https://pypi.org/simple" } dependencies = [ { name = "hotdata" }, @@ -267,9 +267,9 @@ dependencies = [ { name = "pandas", version = "3.0.3", source = { registry = "https://pypi.org/simple" }, marker = "python_full_version >= '3.11'" }, { name = "pyarrow" }, ] -sdist = { url = "https://files.pythonhosted.org/packages/d7/b1/c44c520b861696cd138a5686e54fa3f0213ab35aede16a4fe0c4f5260915/hotdata_framework-0.12.1.tar.gz", hash = "sha256:86dd453f14ed3793cd2526b48cbb823b0d20d610fe784075debc74a544b99a5e", size = 126907, upload-time = "2026-08-18T13:11:54.76Z" } +sdist = { url = "https://files.pythonhosted.org/packages/35/96/ed80c234ce53d6f85205f2139026911439adec7cf131295b93895dafbc1b/hotdata_framework-0.13.0.tar.gz", hash = "sha256:7c5eddac64b2bcf9b20249b2a5f0821f0166ef4215a0a29f96ba1c29007c8c5a", size = 133203, upload-time = "2026-08-27T07:37:10.819Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/b9/0e/2ab773f9b351b9bfc7753f723d6581e25cb498629d01be98d80acdf2bc7c/hotdata_framework-0.12.1-py3-none-any.whl", hash = "sha256:7fd12ff359570d1e0a23d5334de32f0765d3670c9ce65bb23a5b82cbd4e8895d", size = 24836, upload-time = "2026-08-18T13:11:53.31Z" }, + { url = "https://files.pythonhosted.org/packages/ea/79/4bcc8fb2f1698eac828878a1828b8b9dfa9900843e4c9e64f87041f94afd/hotdata_framework-0.13.0-py3-none-any.whl", hash = "sha256:735656e4d85730370daf1324fc99b20ed6e71261c36eb52034a635281a783908", size = 27273, upload-time = "2026-08-27T07:37:09.542Z" }, ] [[package]] @@ -306,7 +306,7 @@ dev = [ [package.metadata] requires-dist = [ { name = "hotdata", specifier = ">=0.8.0" }, - { name = "hotdata-framework", specifier = ">=0.10.0" }, + { name = "hotdata-framework", specifier = ">=0.13.0" }, { name = "langchain", marker = "extra == 'agents'", specifier = ">=1.0" }, { name = "langchain-core", specifier = ">=1.0" }, { name = "numpy", specifier = ">=1.26" },