From 37def5c657406a6c9825f60ed8a0c8dbcc7bca9c Mon Sep 17 00:00:00 2001 From: Rohan Dsouza Date: Tue, 1 Sep 2026 16:02:47 +0530 Subject: [PATCH 1/3] feat: widen the provisioning tools, and mark the ones that can destroy data The tool signatures were narrower than the framework calls beneath them, so capabilities that already existed were unreachable from an agent. `hotdata_create_managed_database` gains `keys` and `expires_at`. A key can only be declared at creation, so a table made through the tool was keyless for its whole life and every key-matched load mode was rejected against it. `hotdata_load_managed_table` gains `mode` and `key`. The load hardcoded `replace`. A keyed mode called without a key now raises before the upload rather than being rejected by the engine after it. `DESTRUCTIVE_TOOL_NAMES` is exported and the tools it names carry `metadata={"destructive": True}`, so `interrupt_on` can be wired from the package rather than from a guess about naming. Only the load tool is in it. `partition_by` and `sorted_by` are not included: they arrive on the framework client at 0.12.0 and this package declares `>=0.10.0`. `format` and `result_id` are on no released framework version. Measured across 0.10.0, 0.11.0, 0.12.0, 0.12.1 and 0.13.0. Closes #91 --- CHANGELOG.md | 32 ++++++++++ README.md | 49 +++++++++++++-- hotdata_langchain/__init__.py | 4 ++ hotdata_langchain/databases.py | 50 +++++++++++++-- hotdata_langchain/tools.py | 47 ++++++++++++-- tests/test_database_ids.py | 9 ++- tests/test_tools.py | 112 +++++++++++++++++++++++++++++++++ 7 files changed, 289 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0c88f6..1591a97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,38 @@ 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. + +### Notes + +- Two parameters named in the issue this closes are not here, and neither is an oversight. + `partition_by` and `sorted_by` arrive on the framework client at **0.12.0**, while this + package declares `hotdata-framework>=0.10.0`; wrapping them needs that floor raised, which is + a separate decision. `format` and `result_id` exist on the `LoadManagedTableRequest` model but + on **no** released version of the framework client, so they need an upstream change rather + than a wider signature here. + ## [0.14.0] - 2026-08-31 ### Added diff --git a/README.md b/README.md index 2811619..5d04766 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,43 @@ 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. + +### 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 +244,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 +691,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..ebd9d59 100644 --- a/hotdata_langchain/__init__.py +++ b/hotdata_langchain/__init__.py @@ -11,6 +11,7 @@ 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 +76,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 +96,7 @@ "DEFAULT_SEARCH_TOOL_NAME", "DEFAULT_SEMANTIC_TOOL_NAME", "DEFAULT_SQL_TOOL_NAME", + "DESTRUCTIVE_TOOL_NAMES", "DISTANCE_COLUMN", "DISTANCE_FUNCTIONS", "RRF_K", @@ -106,6 +109,7 @@ "HotdataClient", "HotdataToolError", "HotdataVectorStore", + "LoadMode", "ManagedDatabase", "QueryResult", "SearchIndex", diff --git a/hotdata_langchain/databases.py b/hotdata_langchain/databases.py index b0bc9f0..e6eb1f6 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -8,7 +8,7 @@ import socket import tempfile 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 @@ -29,6 +29,14 @@ "WHERE table_schema <> 'information_schema'" ) +#: How a load treats the rows a table already holds. ``replace`` discards them; +#: ``append`` keeps them; the remaining three match incoming rows against existing ones +#: and so are legal only on a table that was declared with a key. +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 +164,29 @@ 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, ) -> 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. """ - 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, + ) def is_url(file: str) -> bool: @@ -325,6 +350,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 +366,27 @@ 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. ``upsert``, ``update`` and + ``delete`` match incoming rows against existing ones, so they need ``key``, and they + are rejected unless the table was declared with one — which can only happen at + creation. 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 +394,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..eb4f567 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__) @@ -574,6 +586,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,6 +596,12 @@ 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()] db = create_managed_database( @@ -589,6 +609,8 @@ def hotdata_create_managed_database( 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 +619,23 @@ 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, + 'append' keeps them, and 'upsert', 'update' and 'delete' match incoming + rows against existing ones by key. + 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 +643,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 +740,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,7 +753,11 @@ 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, and 'upsert', 'update' and " + "'delete' match incoming rows against existing ones, so they need 'key' " + "and work only on a table that was declared with one. '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 " @@ -728,6 +766,7 @@ def hotdata_load_managed_table( "target would destroy data. Only parquet is accepted, not CSV or JSON." ), parse_docstring=True, + metadata={"destructive": True}, ), ] ) 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..6517b44 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -28,6 +28,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 +74,8 @@ def test_create_managed_database_delegates(mock_client: MagicMock) -> None: description="sales", schema="public", tables=["orders"], + keys=None, + expires_at=None, ) assert db.description == "sales" @@ -113,6 +116,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 +645,110 @@ 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.""" + tools = {tool.name: tool for tool in make_hotdata_tools(mock_client)} + schema = json.dumps(tools[DEFAULT_LOAD_TABLE_TOOL_NAME].args["mode"]) + for mode in ("replace", "append", "upsert", "update", "delete"): + assert mode in schema From c987d76a56f719885348b1f0cd8824a59bd46f94 Mon Sep 17 00:00:00 2001 From: Rohan Dsouza Date: Tue, 1 Sep 2026 17:56:01 +0530 Subject: [PATCH 2/3] feat: raise the framework floor to 0.13.0, and set table layout from Python MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.13.0 removed the exclusion that kept an `append` load from being retried. Before it, an append that hit `409 RESOURCE_LOCKED` — which the destination returns because it serialises writes per table and refuses rather than queues — failed whatever `max_retries` was configured. The previous commit makes `append` reachable from a tool, so the floor moves with it. The suite passes on 0.10.0, 0.11.0, 0.12.0, 0.12.1 and 0.13.0, so this is about the failure modes the new load modes can hit rather than a broken build. Raising the floor also brings `partition_by` and `sorted_by` into range. Both reach `hl.create_managed_database` and neither is offered to a model. The API has no ALTER path, so undoing a layout choice means deleting the table and reloading it, which burns the table name in that database. The load tool's description now says not to repeat a failed `append`. Re-sending one 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 and the rows land twice. No framework version changes that. --- CHANGELOG.md | 31 +++++++++++++++++++++----- README.md | 22 +++++++++++++++++++ hotdata_langchain/databases.py | 13 +++++++++++ hotdata_langchain/tools.py | 16 ++++++++++++-- pyproject.toml | 3 ++- tests/test_tools.py | 40 +++++++++++++++++++++++++++++++++- uv.lock | 8 +++---- 7 files changed, 119 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1591a97..511fe14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,14 +30,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 names, so a set built with `tool_name_suffix` should be filtered on the metadata instead — the README shows both. +- **`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. + +### Changed + +- **`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 -- Two parameters named in the issue this closes are not here, and neither is an oversight. - `partition_by` and `sorted_by` arrive on the framework client at **0.12.0**, while this - package declares `hotdata-framework>=0.10.0`; wrapping them needs that floor raised, which is - a separate decision. `format` and `result_id` exist on the `LoadManagedTableRequest` model but - on **no** released version of the framework client, so they need an upstream change rather - than a wider signature here. +- `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. + +- 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 diff --git a/README.md b/README.md index 5d04766..feedd89 100644 --- a/README.md +++ b/README.md @@ -91,6 +91,28 @@ 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. +**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 +from hotdata_framework import TableSortKey + +db = hl.create_managed_database( + client, name="events", tables=["spans"], + keys={"spans": ["span_id"]}, + sorted_by={"spans": [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 diff --git a/hotdata_langchain/databases.py b/hotdata_langchain/databases.py index e6eb1f6..4307635 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -7,6 +7,7 @@ import logging import socket import tempfile +from collections.abc import Sequence from pathlib import Path from typing import Any, Literal from urllib.parse import urlsplit @@ -19,6 +20,8 @@ HotdataClient, LoadManagedTableResult, ManagedDatabase, + TablePartitionKey, + TableSortKey, ) from hotdata_framework.databases import api_error_message, managed_database_from_detail @@ -166,6 +169,8 @@ def create_managed_database( 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``. @@ -179,6 +184,12 @@ def create_managed_database( ``"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, @@ -186,6 +197,8 @@ def create_managed_database( tables=tables, keys=keys, expires_at=expires_at, + partition_by=partition_by, + sorted_by=sorted_by, ) diff --git a/hotdata_langchain/tools.py b/hotdata_langchain/tools.py index eb4f567..0d90588 100644 --- a/hotdata_langchain/tools.py +++ b/hotdata_langchain/tools.py @@ -489,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 @@ -763,7 +771,11 @@ def hotdata_load_managed_table( 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_tools.py b/tests/test_tools.py index 6517b44..9a8c0a4 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -10,7 +10,12 @@ from urllib.request import Request import pytest -from hotdata_framework import LoadManagedTableResult, ManagedDatabase, QueryResult +from hotdata_framework import ( + LoadManagedTableResult, + ManagedDatabase, + QueryResult, + TableSortKey, +) from hotdata_langchain.databases import ( _ValidatingRedirectHandler, @@ -76,6 +81,8 @@ def test_create_managed_database_delegates(mock_client: MagicMock) -> None: tables=["orders"], keys=None, expires_at=None, + partition_by=None, + sorted_by=None, ) assert db.description == "sales" @@ -752,3 +759,34 @@ def test_the_load_tool_offers_every_mode_to_the_model(mock_client: MagicMock) -> schema = json.dumps(tools[DEFAULT_LOAD_TABLE_TOOL_NAME].args["mode"]) for mode in ("replace", "append", "upsert", "update", "delete"): assert mode in schema + + +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 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" }, From a56abc8b1a41360814eeca8773d9ff5c21b7810c Mon Sep 17 00:00:00 2001 From: Rohan Dsouza Date: Tue, 1 Sep 2026 18:09:03 +0530 Subject: [PATCH 3/3] fix: say what each keyed load mode does, and test at the declared floor Review found the model-facing text naming `upsert`, `update` and `delete` together with only the shared clause that each matches incoming rows against existing ones. A model choosing between them had no statement of any one's effect, and `delete` reads as delete-then-insert by key. It removes matched rows and inserts nothing, so that misreading destroys rows and reports success. The API's own wording is on `LoadManagedTableRequest.key`: the key columns decide which existing row an incoming row "removes, updates, or replaces". Each mode now states its effect on a matched row and on one that matches nothing, in `LoadMode`, in the tool docstring and in the tool description. Writing that surfaced a break at the `langchain-core>=1.0` floor: 1.0.0's docstring parser treats a colon in a wrapped `Args:` continuation as a new argument name, so every `make_hotdata_tools` call raised. Newer versions parse it, which is why the locked build was green. CI now runs the suite with `--resolution lowest-direct` so the advertised floor is exercised rather than only the resolved ceiling. Also from review: a `keys` entry naming an undeclared table is refused rather than silently creating a keyless table; `TablePartitionKey` and `TableSortKey` are re-exported now that both appear in a public signature; the mode-schema test reads the enum rather than the prose description that satisfied it either way; and one test binds the provisioning calls against the real client signature, which a `MagicMock` cannot do. --- .github/workflows/ci.yml | 18 +++++++++ CHANGELOG.md | 25 ++++++++++++ README.md | 18 +++++++-- hotdata_langchain/__init__.py | 11 +++++- hotdata_langchain/databases.py | 19 ++++++---- hotdata_langchain/tools.py | 26 +++++++++---- tests/test_tools.py | 69 +++++++++++++++++++++++++++++++--- 7 files changed, 161 insertions(+), 25 deletions(-) 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 511fe14..5ef5a1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,14 +30,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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` @@ -53,6 +72,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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 diff --git a/README.md b/README.md index feedd89..ad685e1 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,20 @@ window like `"24h"`; without it the database lives until something deletes it, w 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. +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 @@ -104,12 +117,11 @@ and reloading it, which burns the table name in that database — so it is set b the tools, not chosen per call by a model: ```python -from hotdata_framework import TableSortKey db = hl.create_managed_database( client, name="events", tables=["spans"], keys={"spans": ["span_id"]}, - sorted_by={"spans": [TableSortKey(column="start_time")]}, + sorted_by={"spans": [hl.TableSortKey(column="start_time")]}, ) ``` diff --git a/hotdata_langchain/__init__.py b/hotdata_langchain/__init__.py index ebd9d59..1a47d7f 100644 --- a/hotdata_langchain/__init__.py +++ b/hotdata_langchain/__init__.py @@ -7,7 +7,14 @@ 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 ( @@ -116,6 +123,8 @@ "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 4307635..550dab9 100644 --- a/hotdata_langchain/databases.py +++ b/hotdata_langchain/databases.py @@ -32,9 +32,11 @@ "WHERE table_schema <> 'information_schema'" ) -#: How a load treats the rows a table already holds. ``replace`` discards them; -#: ``append`` keeps them; the remaining three match incoming rows against existing ones -#: and so are legal only on a table that was declared with a key. +#: 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. @@ -382,11 +384,12 @@ def load_managed_table( target. The default load replaces the table's contents, which is why addressing it unambiguously matters. - ``mode`` chooses what happens to rows already there. ``upsert``, ``update`` and - ``delete`` match incoming rows against existing ones, so they need ``key``, and they - are rejected unless the table was declared with one — which can only happen at - creation. Raises ``ValueError`` for a keyed mode called without ``key``, rather than - letting the engine reject it after the file has been uploaded. + ``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( diff --git a/hotdata_langchain/tools.py b/hotdata_langchain/tools.py index 0d90588..e1667ef 100644 --- a/hotdata_langchain/tools.py +++ b/hotdata_langchain/tools.py @@ -612,6 +612,13 @@ def hotdata_create_managed_database( 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, @@ -639,9 +646,11 @@ def hotdata_load_managed_table( 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, - 'append' keeps them, and 'upsert', 'update' and 'delete' match incoming - rows against existing ones by key. + 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. """ @@ -762,10 +771,13 @@ def hotdata_load_managed_table( description=( "Load a parquet file into a table that was declared on an instant " "database. By default this replaces whatever the table held; pass 'mode' " - "to keep it. 'append' adds rows blindly, and 'upsert', 'update' and " - "'delete' match incoming rows against existing ones, so they need 'key' " - "and work only on a table that was declared with one. 'file' is either " - "a path on " + "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 " diff --git a/tests/test_tools.py b/tests/test_tools.py index 9a8c0a4..d289a8b 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -1,16 +1,18 @@ 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 ( + HotdataClient, LoadManagedTableResult, ManagedDatabase, QueryResult, @@ -18,6 +20,7 @@ ) from hotdata_langchain.databases import ( + LoadMode, _ValidatingRedirectHandler, create_managed_database, fetch_parquet, @@ -754,11 +757,23 @@ def test_the_destructive_marking_survives_a_tool_name_suffix(mock_client: MagicM 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.""" + """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)} - schema = json.dumps(tools[DEFAULT_LOAD_TABLE_TOOL_NAME].args["mode"]) - for mode in ("replace", "append", "upsert", "update", "delete"): - assert mode in schema + 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( @@ -790,3 +805,45 @@ def test_the_load_tool_warns_against_repeating_a_failed_append(mock_client: Magi 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 + )