diff --git a/CHANGELOG.md b/CHANGELOG.md index 665a5eb..307fd87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,41 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **`searchable_columns=` on `make_hotdata_tools`, so the SQL tool can name every indexed + column rather than only the one the search tool ranks.** Takes `(table, column)` pairs written + `catalog.schema.table`, confirms each against the control plane when the tools are built, and + drops with a warning any a ready index does not cover. Every confirmed column gets its own + worked `bm25_search(...)` (or `vector_search(...)`) call in the description. Order carries: + the first is the one a model reaches for most, so lead with the table most questions are + about. `SearchableColumn` and `verify_searchable_columns` are exported for callers assembling + descriptions themselves. + +### Fixed + +- **The SQL tool no longer claims the registered column is the *only* indexed one.** It said + "the BM25-indexed column is `` on ``" on the strength of what the caller had + wired to the search tool, which is a statement about one tool's configuration and not about + the database. On any database indexing more than one column that sentence was false, and it + was measured being followed in preference to what `hotdata_describe_tables` reports — so the + model searched a table the answer was not about and returned a confident, wrong number. It now + says that column *has* an index, which is what the caller actually told us. + + Measured across 84 runs on one model (`gpt-5.1`), one dataset and one question, asking an + aggregate whose numbers lived on a table other than the registered corpus. With the corpus + named alone the model searched the right table in 0 runs of 12; declaring the other column + through `searchable_columns=` took that to 7 of 12 (Fisher exact p = 0.005). Composition rose + 7 of 12 to 10 and the `ILIKE` fallback fell 4 of 12 to 2, but neither is significant at this + sample size (p = 0.37 and p = 0.64) — read them as direction, not as result. + + Three things this does *not* fix. Naming a column without giving it its own worked call moved + almost nothing (2 of 12). The gain is much weaker without a prior `hotdata_describe_tables` + call in the thread, which is the likelier shape of a single-question session (2 of 6 cold + against 5 of 6 warm). And the cohort size a model asks for is still an arbitrary `k`, so the + answers themselves remain wrong for a different reason — 0 of 84 runs across every wording + produced the correct figure ([#62](https://github.com/hotdata-dev/hotdata-langchain/issues/62)). + ## [0.13.0] - 2026-08-26 ### Added diff --git a/README.md b/README.md index c442a9f..9ebbb59 100644 --- a/README.md +++ b/README.md @@ -264,9 +264,12 @@ Rows come back ranked, each with a `score`. The agent supplies only `query` and `k`; the table and column are fixed when you build the tool. That is deliberate: the engine errors outright rather than falling back to a scan when a column has no BM25 index, so a model choosing its own corpus can pick one that cannot answer. `hotdata_describe_tables` now -reports which columns are searchable, which is what a model would need to choose from -something it read rather than guessed — the search tool has not been changed to accept that -choice yet. +reports which columns are searchable, which is what a model would need to choose from something +it read rather than guessed. The search tool has not been changed to accept that choice, and the +annotation on its own was measured not to change what a model searches: over 24 runs it never +moved the table chosen, including runs where the model had already read it and then matched with +`ILIKE` on that same column. What moved the choice was naming the column in a worked call, which +is what `searchable_columns=` below does. ### Text or meaning, decided by the index @@ -359,6 +362,39 @@ same rows, but the engine matches its index lookup on the reference as written, form can quietly forfeit an index. `HotdataVectorStore` and the search tool emit the full form themselves. +### Telling the model about the other indexed columns + +`search_table`/`search_column` describe the corpus the *search tool* ranks over. A database +usually indexes more than one column, and the SQL tool's description is the only place a model +learns the others exist — so name them: + +```python +tools = hl.make_hotdata_tools( + client, + database_id="dbid...", + search_table="default.public.listing_corpus", # what the search tool ranks + search_column="content", + searchable_columns=[ # what SQL can also rank + ("default.public.listings", "description"), + ], +) +``` + +Each pair is confirmed against the control plane when the tools are built, and one no ready +index covers is dropped with a warning rather than named — BM25 has no brute-force fallback, so +a column offered wrongly is a hard error at the point the model has already committed to the +route. Every confirmed column gets its own worked call in the description. + +**Order carries.** A model writes the call it is shown and largely ignores columns it is only +told about: over 84 runs on one model, one dataset and one question, naming a second indexed +column moved the table it searched in 2 runs of 12, while giving that column its own worked call +moved it in 8. So put the table most questions are about first. + +This narrows the failure rather than removing it. The right table depends on the question and a +description is written once, so declaring the fact table took the right-table rate from 0 of 12 +to 7 of 12 (Fisher exact p = 0.005) rather than to 12 of 12 — and most of that gain needs a +`hotdata_describe_tables` call earlier in the thread (5 of 6 with one, 2 of 6 without). + For more than one searchable corpus, build the tools yourself and give each a distinct name and description — the agent then routes on the descriptions: diff --git a/docs/ai-native-layer-roadmap.md b/docs/ai-native-layer-roadmap.md index 7b6e5d2..31dd9c1 100644 --- a/docs/ai-native-layer-roadmap.md +++ b/docs/ai-native-layer-roadmap.md @@ -204,8 +204,17 @@ Grouped by code surface: provider-backed index generated; no engine change was needed. `make_hotdata_tools` takes `tool_name_suffix=` so two tool sets no longer both register `hotdata_execute_sql`, and the database-scoped descriptions name their database. What this unlocks is not itself done: the - search corpus is still pinned at construction, and un-pinning it is now safe because an agent - can read what is searchable rather than guess. + search corpus is still pinned at construction, and un-pinning it is now *expressible* because + an agent can read what is searchable rather than guess. + + Whether an agent would act on that is a separate question, and it has since been measured + answering no. Across 24 runs on one model, `searchable_by` never once moved which table the + agent searched, including runs where the annotation was already in the thread from an earlier + describe call and the agent then wrote `ILIKE` against the very column it said was searchable. + What moved the choice was a worked call in the SQL tool's description naming that column, so + the discovery surface is true information that nothing consumes on its own. Read the finding + before designing anything that assumes an agent will look it up: + [#73](https://github.com/hotdata-dev/hotdata-langchain/issues/73). - ~~**Tool-layer robustness**~~ ([#41](https://github.com/hotdata-dev/hotdata-langchain/issues/41)) — **done.** `with_error_feedback` and `engine_error_message` are package API, `make_hotdata_tools(handle_errors=True)` turns the wrapping on, and both the sync and async callables are wrapped — the async one is what LangChain actually calls in a diff --git a/docs/engine-contract.md b/docs/engine-contract.md index eef9279..29c96a0 100644 --- a/docs/engine-contract.md +++ b/docs/engine-contract.md @@ -111,6 +111,15 @@ Returns the table's columns plus a trailing `score` (Float32). Four properties s default. Correctness is unaffected (explicit-`k` and trailing-`LIMIT` returned identical top-3), and at 7.5k rows the cost was not measurable (40 ms vs 38 ms median), so this is a scan-bound difference rather than an observed slowdown. Passing `k` explicitly is free, so we do. +- **Omitting the limit caps the result at 1,000, silently** (verified 2026-08-29). The fourth + argument is optional, and leaving it off is not "give me every match": on + `default.public.listings.description`, `'quiet garden'` matched 1,426 rows and the unbounded + call returned exactly 1,000. Below that ceiling the two agree — `'garden'` returned 555 either + way, and the 500-row corpus returned its whole 171-row pool — so the truncation shows up only + on the pools large enough to matter, and nothing in the result marks it. There is therefore no + way to ask for a whole match pool without already knowing its size, which is what makes an + aggregate over a relevance-defined cohort a two-step operation rather than one query. Tracked + as [#62](https://github.com/hotdata-dev/hotdata-langchain/issues/62). - **The index is a hard prerequisite.** No brute-force fallback: a column without a BM25 index gives `No BM25 index found on column 'name' for .public.listings`. This differs from vector search, where the *explicit-vector* scalar UDFs (`cosine_distance(col, ARRAY[...])`) diff --git a/hotdata_langchain/__init__.py b/hotdata_langchain/__init__.py index 684daa7..00777d4 100644 --- a/hotdata_langchain/__init__.py +++ b/hotdata_langchain/__init__.py @@ -29,6 +29,7 @@ SEARCH_NOUNS, SEMANTIC, TEXT, + SearchableColumn, SearchIndex, capabilities_by_column, fusable_vector_indexes, @@ -36,6 +37,7 @@ indexes_for_column, list_search_indexes, search_nouns_by_column, + verify_searchable_columns, ) from hotdata_langchain.results import ( CLIENT_WARNING_KEY, @@ -109,6 +111,7 @@ "SearchIndex", "SearchRoute", "SearchStrategy", + "SearchableColumn", "__version__", "bm25_search_json", "bm25_search_sql", @@ -142,5 +145,6 @@ "suffixed_tool_name", "vector_distance_sql", "vector_search_sql", + "verify_searchable_columns", "with_error_feedback", ] diff --git a/hotdata_langchain/indexes.py b/hotdata_langchain/indexes.py index 9d8942f..56e2f7c 100644 --- a/hotdata_langchain/indexes.py +++ b/hotdata_langchain/indexes.py @@ -245,6 +245,92 @@ def generated_vector_columns(indexes: Sequence[SearchIndex]) -> Iterator[str]: yield index.vector_column +@dataclass(frozen=True) +class SearchableColumn: + """One column a ready index covers, carrying the table reference to name it by. + + :class:`SearchIndex` describes an index within a table already known to the caller. + This pairs one with the three-part reference a query has to write, which is what a + tool description needs and what the index record does not carry. + """ + + table: str + index: SearchIndex + + @property + def column(self) -> str: + return self.index.column + + @property + def kind(self) -> SearchKind: + return self.index.kind + + @property + def function(self) -> str: + """Return the table function this column is searched through.""" + return "vector_search" if self.index.kind == SEMANTIC else "bm25_search" + + @property + def composable(self) -> bool: + """Report whether a query against this column can be written in SQL. + + False for a plain vector index, whose query has to arrive as a vector. + """ + return self.index.kind == TEXT or self.index.embeds_query + + +def verify_searchable_columns( + client: HotdataClient, + *, + columns: Sequence[tuple[str, str]], + database: ManagedDatabase | None, +) -> list[SearchableColumn]: + """Return the declared ``(table, column)`` pairs a ready index actually covers. + + Declared rather than discovered, and then confirmed rather than trusted. Naming a + column a model can search is a claim about the database, and this package states one + only after reading it back — the same stance :func:`query_catalogs` takes towards the + catalog name. A pair no index covers is dropped with a warning rather than named, + because BM25 has no brute-force fallback and a search against an unindexed column is + a hard error at the point the model has already committed to the route. + + One control-plane call per distinct table, not per declared column. Order is the + caller's, and it is preserved: a description that names several columns leads with + the first, which is the one a model was measured reaching for most. + + Returns an empty list without ``database``, which is the scope every index listing + needs. Duplicated pairs are named once. + """ + if database is None: + return [] + listed: dict[str, list[SearchIndex]] = {} + found: list[SearchableColumn] = [] + seen: set[tuple[str, str]] = set() + for table, column in columns: + if (table, column) in seen: + continue + seen.add((table, column)) + parts = table.split(".") + if len(parts) != 3: + raise ValueError( + f"a searchable column's table must be written catalog.schema.table, got {table!r}" + ) + if table not in listed: + listed[table] = list_search_indexes( + client, table=parts[2], schema=parts[1], database=database + ) + covering = indexes_for_column(listed[table], column) + if not covering: + logger.warning( + "no ready search index was found covering %r on %s; not naming it as searchable", + column, + table, + ) + continue + found.append(SearchableColumn(table, covering[0])) + return found + + def fusable_vector_indexes(indexes: Sequence[SearchIndex]) -> list[SearchIndex]: """Return the plain vector indexes among ``indexes``, in listing order. diff --git a/hotdata_langchain/tools.py b/hotdata_langchain/tools.py index 5bf7863..5e5d093 100644 --- a/hotdata_langchain/tools.py +++ b/hotdata_langchain/tools.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import logging import re from collections.abc import Sequence from typing import Any @@ -25,6 +26,12 @@ scoped_description, ) from hotdata_langchain.errors import HotdataToolError, engine_error_message, with_error_feedback +from hotdata_langchain.indexes import ( + SEMANTIC, + TEXT, + SearchableColumn, + verify_searchable_columns, +) from hotdata_langchain.results import result_json from hotdata_langchain.schema import ( DEFAULT_DESCRIBE_TOOL_NAME, @@ -51,6 +58,45 @@ DEFAULT_CREATE_DATABASE_TOOL_NAME = "hotdata_create_managed_database" DEFAULT_LOAD_TABLE_TOOL_NAME = "hotdata_load_managed_table" +logger = logging.getLogger(__name__) + + +def _search_examples( + function: str, + *, + singular: str, + generic: str, + search_table: str | None, + search_column: str | None, + also_searchable: Sequence[SearchableColumn] = (), +) -> str: + """Return the worked calls naming every column this function can search. + + A model was measured writing whichever call the description shows it and ignoring + columns it was merely told about: naming a second indexed column moved the table it + searched in 2 runs of 12, while giving that column its own worked call moved it in 8. + So each column gets a call rather than a mention, and the order is the caller's, + because the leading one is what a model reaches for most. + """ + named: list[tuple[str, str]] = [(one.table, one.column) for one in also_searchable] + if search_table and search_column and (search_table, search_column) not in named: + named.append((search_table, search_column)) + + def call(table: str, column: str) -> str: + return f"{function}('{table}', '{column}', '', )" + + if not named: + return generic + if len(named) == 1: + table, column = named[0] + return f"Here '{column}' on {table} {singular}, so the call is {call(table, column)}." + ranked = [f"{call(table, column)} ranks {table}" for table, column in named] + return ( + f"More than one column can be searched here, with a call for each: " + f"{', '.join(ranked[:-1])}, and {ranked[-1]}. Search the one whose table your " + f"answer is about." + ) + def sql_tool_description( search_tool_name: str | None = None, @@ -59,6 +105,7 @@ def sql_tool_description( search_table: str | None = None, search_column: str | None = None, search_route: SearchRoute | None = None, + also_searchable: Sequence[SearchableColumn] | None = None, catalogs: Sequence[str] | None = None, max_rows: int | None = None, ) -> str: @@ -82,7 +129,22 @@ def sql_tool_description( limit and quadratic in prompt size. Naming the function, and preferring it whenever the answer aggregates over the matches, is what makes the composed form reachable. ``search_table``/``search_column`` are woven into the text when known, so the model - is told which column is actually indexed rather than guessing one. + is told which column is actually indexed rather than guessing one. They name the + column the search tool was registered over, which is not necessarily the only indexed + one, so the sentence says that column has an index rather than that it is *the* + indexed column. The stronger claim was measured being followed in preference to what + ``hotdata_describe_tables`` reports, which makes it a wrong answer the model cannot + recover from rather than a wording preference. + + ``also_searchable`` names the other columns a caller has had confirmed, each with its + own worked call. Which one a model picks tracks which call the description shows it, + so the order is the caller's and the first is the one it reaches for most. This + narrows the failure rather than removing it: the table a model chose still followed + the leading example, and a description is written once while the right table depends + on the question. Declaring the table an aggregate question was about moved the + right-table rate from 0 runs of 12 to 7 (p = 0.005); the rest of what was observed — + more composing, less substring matching — did not reach significance at that sample + size and should not be quoted as an effect. ``search_route`` says which function that column is reachable through, and the paragraph is rewritten around it. The two descriptions arrive in one prompt, so a SQL @@ -91,8 +153,10 @@ def sql_tool_description( semantic wording also carries the sort, because ``vector_search`` returns its rows unsorted and a trailing ``LIMIT`` without ``ORDER BY _distance`` was measured returning arbitrary rows rather than the nearest ones. A plain vector index gets no - composable paragraph at all: writing that search needs a query vector, which an agent - writing SQL cannot produce. + composable call of its own: writing that search needs a query vector, which an agent + writing SQL cannot produce. That is a fact about the registered column and not about + the database, so it is stated of that column by name, and any ``also_searchable`` + column that does compose is still offered beside it. Table references are asked for in full. A two-part `schema.table` reference resolves and returns correct rows, but the engine's index-lookup rewrite matches on the @@ -129,29 +193,54 @@ def sql_tool_description( "them — it keeps the whole cohort in the query instead of passing ids back as " "literals." ) - if semantic and search_route is not None and not search_route.composable: - # No composable form: this index needs a query vector, which SQL cannot express. - composable = ( - f"Ranking rows by meaning is not available in SQL here — it needs the query " - f"as a vector, which SQL cannot express — so use the {search_tool_name} tool " - f"for it and aggregate over what it returns." - if search_tool_name - else "Ranking rows by meaning is not available in SQL here: it needs the " - "query as a vector, which SQL cannot express." - ) - elif semantic: - if search_table and search_column: - example = ( - f"Here the column searchable by meaning is '{search_column}' on " - f"{search_table}, so the call is vector_search('{search_table}', " - f"'{search_column}', '', )." + declared = list(also_searchable or ()) + text_columns = [one for one in declared if one.kind == TEXT] + semantic_columns = [one for one in declared if one.kind == SEMANTIC and one.composable] + for one in declared: + if one.kind == SEMANTIC and not one.composable: + logger.debug( + "%r on %s is reached only with a query vector, which SQL cannot express; " + "not naming it as composable", + one.column, + one.table, ) - else: - example = ( + # A route that cannot be composed still leaves the *other* declared columns callable, + # so which function leads is the registered route's and neither is dropped for it. + registered_composes = search_route is None or search_route.composable + + def text_paragraph() -> str: + example = _search_examples( + "bm25_search", + singular="has a BM25 index", + generic=( + "The call is bm25_search('catalog.schema.table', '', " + "'', ), over a column that has a BM25 index." + ), + search_table=None if semantic else search_table, + search_column=None if semantic else search_column, + also_searchable=text_columns, + ) + return ( + f"To rank rows by how well their text matches a phrase, call bm25_search " + f"inside SQL: it is a table-valued function returning the matched rows' " + f"columns plus a `score`, so it joins, groups and nests in subqueries like " + f"any other table. {example} {prefer}" + ) + + def semantic_paragraph() -> str: + named = semantic and registered_composes + example = _search_examples( + "vector_search", + singular="is searchable by meaning", + generic=( "The call is vector_search('catalog.schema.table', '', " "'', ), over a column that is searchable by meaning." - ) - composable = ( + ), + search_table=search_table if named else None, + search_column=search_column if named else None, + also_searchable=semantic_columns, + ) + return ( f"To rank rows by how close their meaning is to a phrase, call vector_search " f"inside SQL: it is a table-valued function returning the matched rows' " f"columns plus a `_distance` where smaller is nearer, so it joins, groups and " @@ -159,24 +248,32 @@ def sql_tool_description( f"unsorted, so add ORDER BY _distance ASC — a trailing LIMIT without that " f"sort returns arbitrary rows rather than the nearest ones. {prefer}" ) - else: - if search_table and search_column: - example = ( - f"Here the BM25-indexed column is '{search_column}' on {search_table}, so " - f"the call is bm25_search('{search_table}', '{search_column}', " - f"'', )." + + parts: list[str] = [] + if semantic: + if registered_composes or semantic_columns: + parts.append(semantic_paragraph()) + if not registered_composes: + # Scoped to the registered column. "not available in SQL here" would be a + # claim about the database made from one tool's registration, which is the + # defect the rest of this function was corrected for. + reach = ( + f"Ranking '{search_column}' on {search_table} by meaning needs the query " + f"as a vector, which SQL cannot express" ) - else: - example = ( - "The call is bm25_search('catalog.schema.table', '', " - "'', ), over a column that has a BM25 index." + parts.append( + f"{reach}, so use the {search_tool_name} tool for it and aggregate over " + f"what it returns." + if search_tool_name + else f"{reach}." ) - composable = ( - f"To rank rows by how well their text matches a phrase, call bm25_search " - f"inside SQL: it is a table-valued function returning the matched rows' " - f"columns plus a `score`, so it joins, groups and nests in subqueries like " - f"any other table. {example} {prefer}" - ) + if text_columns: + parts.append(text_paragraph()) + else: + parts.append(text_paragraph()) + if semantic_columns: + parts.append(semantic_paragraph()) + composable = " ".join(parts) # On a fused route the search tool does *not* do the same ranking: it also ranks by # meaning, and only the text half of that is expressible in SQL. Saying "the same # ranking" there would understate the tool in the one prompt that also carries the @@ -336,6 +433,7 @@ def make_hotdata_tools( search_strategy: SearchStrategy = "auto", search_embedding: Embeddings | None = None, search_semantic_column: str | None = None, + searchable_columns: Sequence[tuple[str, str]] | None = None, describe_tables: bool = True, describe_column_stats: bool = True, describe_search_capabilities: bool = True, @@ -422,6 +520,20 @@ def make_hotdata_tools( column alone. A search over a vector column never returns that column, so there the key carries the hit on its own unless ``search_columns`` names more. + ``searchable_columns`` names other indexed columns as ``(table, column)`` pairs, each + written ``catalog.schema.table``. The search tool ranks one corpus, so ``search_table`` + describes that one and nothing else; a database usually has more, and the SQL tool's + description is the only place a model learns they exist. Each pair is confirmed + against the control plane before it is named, and one no ready index covers is dropped + with a warning rather than offered. Order carries: the first is the one a model + reaches for most, so lead with the table most questions are about. A malformed pair + raises ``ValueError`` here, and without ``database_id`` there is no scope to confirm + against, so the argument is ignored entirely. A declared column of the kind the + registered route does not use is still named, through its own function — a BM25 + column beside a semantic search tool composes perfectly well, and dropping it would + repeat the defect this parameter exists to fix. Only a plain vector column is left + out, because writing that search needs a query vector. + Supplying only one of ``search_table``/``search_column`` raises ``ValueError``. For more than one searchable corpus, call @@ -514,6 +626,13 @@ def hotdata_load_managed_table( else " (a URL must be on the public internet, not an internal address)" ) + # Confirmed before it is named. A column reported searchable that no index covers + # sends the model to a function with no fallback, and the error arrives after it has + # committed to the route. + confirmed = verify_searchable_columns( + client, columns=list(searchable_columns or ()), database=database + ) + has_search = search_table is not None and search_column is not None # Resolved once, before either description is built: the SQL tool and the search tool # both describe this column to the same model in the same prompt, and resolving twice @@ -554,6 +673,7 @@ def hotdata_load_managed_table( search_table=search_table if has_search else None, search_column=search_column if has_search else None, search_route=search_route, + also_searchable=confirmed, catalogs=catalogs, max_rows=max_rows, ), diff --git a/tests/test_descriptions.py b/tests/test_descriptions.py index 54ae114..277692e 100644 --- a/tests/test_descriptions.py +++ b/tests/test_descriptions.py @@ -16,7 +16,7 @@ from hotdata_framework import ManagedDatabase from langchain_core.tools import StructuredTool -from hotdata_langchain.indexes import SEMANTIC, SearchIndex +from hotdata_langchain.indexes import SEMANTIC, SearchableColumn, SearchIndex from hotdata_langchain.search import SearchRoute from hotdata_langchain.tools import make_hotdata_tools, sql_tool_description @@ -309,7 +309,11 @@ def test_sql_description_offers_no_composed_form_it_cannot_write() -> None: search_route=_semantic_route(embeds_query=False), ) assert "vector_search(" not in description - assert "not available in SQL here" in description + # Scoped to the registered column. "not available in SQL here" would be a claim about + # the database inferred from one tool's registration, which is the defect this + # function was corrected for — another table may carry a composable index. + assert f"Ranking '{COLUMN}' on {TABLE} by meaning needs the query as a vector" in description + assert "not available in SQL here" not in description def test_sql_description_keeps_bm25_wording_without_a_route() -> None: @@ -507,3 +511,159 @@ def test_the_database_tools_quote_only_keys_their_payload_carries() -> None: described = set(quoted.findall(tools[name].description or "")) missing = described - set(payload) - set(tools[name].args) assert not missing, f"{name} quotes {sorted(missing)}, but returns {sorted(payload)}" + + +def _searchable(table: str, column: str, kind: str = "text") -> SearchableColumn: + return SearchableColumn( + table, + SearchIndex( + column=column, + kind=kind, # type: ignore[arg-type] + index_type="bm25" if kind == "text" else "vector", + ready=True, + embeds_query=kind == SEMANTIC, + ), + ) + + +def test_sql_description_does_not_claim_the_registered_column_is_the_only_indexed_one() -> None: + """`search_table` is what the search tool ranks over, which is not a statement about + what else the database indexes. The exclusive claim was measured being followed in + preference to what hotdata_describe_tables reports, which makes it a wrong answer the + model cannot recover from.""" + description = descriptions(search_table=TABLE, search_column=COLUMN)["hotdata_execute_sql"] + assert f"'{COLUMN}' on {TABLE} has a BM25 index" in description + assert "the BM25-indexed column is" not in description + + +def test_each_searchable_column_gets_its_own_worked_call() -> None: + """A model was measured writing whichever call it is shown and ignoring columns it is + merely told about: naming a second column moved the table it searched in 2 runs of 12, + giving that column its own call moved it in 8.""" + description = sql_tool_description( + search_table=TABLE, + search_column=COLUMN, + also_searchable=[_searchable("default.public.corpus", "content")], + ) + assert "bm25_search('default.public.corpus', 'content'" in description + assert f"bm25_search('{TABLE}', '{COLUMN}'" in description + + +def test_the_first_declared_column_leads_the_examples() -> None: + """Which column a model picks tracks which call it is shown first, so the caller's + ordering is what decides it and has to survive into the text.""" + description = sql_tool_description( + search_table=TABLE, + search_column=COLUMN, + also_searchable=[ + _searchable("default.public.corpus", "content"), + _searchable("default.public.reviews", "body"), + ], + ) + order = [ + description.index(f"'{t}'") + for t in ("default.public.corpus", "default.public.reviews", TABLE) + ] + assert order == sorted(order) + + +def test_a_column_named_twice_is_worked_once() -> None: + description = sql_tool_description( + search_table=TABLE, search_column=COLUMN, also_searchable=[_searchable(TABLE, COLUMN)] + ) + assert description.count(f"bm25_search('{TABLE}', '{COLUMN}'") == 1 + + +def test_a_semantic_column_is_never_offered_to_bm25_search() -> None: + """The two functions read different indexes, and BM25 has no fallback: offering a + vector column to bm25_search is a hard error at the point the model has committed.""" + description = sql_tool_description( + search_table=TABLE, + search_column=COLUMN, + also_searchable=[_searchable("default.public.corpus", "embedding", SEMANTIC)], + ) + assert "bm25_search('default.public.corpus'" not in description + + +def test_a_plain_vector_column_is_not_offered_as_composable() -> None: + """Composing one needs a query vector, which an agent writing SQL cannot produce.""" + plain = SearchableColumn( + "default.public.corpus", + SearchIndex(column="embedding", kind=SEMANTIC, index_type="vector", ready=True), + ) + description = sql_tool_description( + search_table=TABLE, + search_column="content", + search_route=SearchRoute( + SEMANTIC, + SearchIndex( + column="content", + kind=SEMANTIC, + index_type="vector", + ready=True, + embeds_query=True, + ), + ), + also_searchable=[plain], + ) + assert "vector_search('default.public.corpus'" not in description + + +def test_a_declared_text_column_survives_a_semantic_registered_route() -> None: + """A confirmed column the caller paid a control-plane call for should not vanish + because the search tool happens to be registered over the other kind of index.""" + description = sql_tool_description( + "hotdata_search_semantic", + search_table=TABLE, + search_column=COLUMN, + search_route=_semantic_route(embeds_query=True), + also_searchable=[_searchable("default.public.corpus", "content")], + ) + assert "bm25_search('default.public.corpus', 'content'" in description + assert f"vector_search('{TABLE}', '{COLUMN}'" in description + + +def test_a_plain_vector_route_still_offers_the_columns_that_do_compose() -> None: + """The registered column needing a query vector says nothing about another table's + BM25 index, and dropping every declared column for it repeats the defect this + function was corrected for.""" + description = sql_tool_description( + "hotdata_search_semantic", + search_table=TABLE, + search_column=COLUMN, + search_route=_semantic_route(embeds_query=False), + also_searchable=[_searchable("default.public.corpus", "content")], + ) + assert "bm25_search('default.public.corpus', 'content'" in description + assert f"Ranking '{COLUMN}' on {TABLE} by meaning needs the query as a vector" in description + assert f"vector_search('{TABLE}'" not in description + + +def test_a_declared_plain_vector_column_is_never_offered_as_composable() -> None: + """Writing that search needs a query vector, which an agent writing SQL cannot make.""" + plain = SearchableColumn( + "default.public.corpus", + SearchIndex(column="embedding", kind=SEMANTIC, index_type="vector", ready=True), + ) + description = sql_tool_description( + "hotdata_search_text", search_table=TABLE, search_column=COLUMN, also_searchable=[plain] + ) + assert "vector_search" not in description + assert f"bm25_search('{TABLE}', '{COLUMN}'" in description + + +def test_searchable_columns_reaches_the_sql_description(mock_client: MagicMock) -> None: + """The wiring between the verifier and the description is what a caller touches, and + it is the part that would silently stop threading through.""" + confirmed = [_searchable("default.public.corpus", "content")] + with patch("hotdata_langchain.tools.verify_searchable_columns", return_value=confirmed): + built = { + tool.name: tool.description or "" + for tool in make_hotdata_tools( + mock_client, + search_table=TABLE, + search_column=COLUMN, + searchable_columns=[("default.public.corpus", "content")], + ) + } + assert "bm25_search('default.public.corpus', 'content'" in built["hotdata_execute_sql"] diff --git a/tests/test_indexes.py b/tests/test_indexes.py index 52a0a29..24f25e7 100644 --- a/tests/test_indexes.py +++ b/tests/test_indexes.py @@ -6,14 +6,21 @@ from __future__ import annotations +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest + from hotdata_langchain.indexes import ( CAPABILITY_PHRASES, SEARCH_NOUNS, SEMANTIC, TEXT, + SearchableColumn, SearchIndex, capabilities_by_column, search_nouns_by_column, + verify_searchable_columns, ) @@ -66,3 +73,95 @@ def test_both_kinds_on_one_column_are_both_named() -> None: def test_no_indexes_means_no_columns_rather_than_an_error() -> None: assert capabilities_by_column([]) == {} assert search_nouns_by_column([]) == {} + + +def _listing(**per_table: list[SimpleNamespace]) -> MagicMock: + """Patch the indexes API so each table reports its own listing.""" + api = patch("hotdata_langchain.indexes.IndexesApi").start() + api.return_value.list_indexes.side_effect = lambda _conn, _schema, table: SimpleNamespace( + indexes=per_table.get(table, []) + ) + return api + + +def _bm25(column: str = "description") -> SimpleNamespace: + return SimpleNamespace( + index_name=f"{column}_bm25", + index_type="bm25", + columns=[column], + metric=None, + status="ready", + source_column=None, + ) + + +def test_a_declared_column_is_named_only_when_an_index_covers_it() -> None: + """BM25 has no brute-force fallback, so naming an unindexed column is a hard error + the model reaches only after committing to the route.""" + api = _listing(listings=[_bm25("description")], notes=[]) + try: + found = verify_searchable_columns( + MagicMock(), + columns=[("d.public.listings", "description"), ("d.public.notes", "body")], + database=MagicMock(), + ) + finally: + patch.stopall() + assert [(one.table, one.column) for one in found] == [("d.public.listings", "description")] + assert api.return_value.list_indexes.call_count == 2 + + +def test_declared_order_is_preserved_and_one_table_is_listed_once() -> None: + """Order carries into the description: the leading call is the one a model was + measured reaching for most, so the caller's ordering must survive.""" + api = _listing(listings=[_bm25("description"), _bm25("name")]) + try: + found = verify_searchable_columns( + MagicMock(), + columns=[ + ("d.public.listings", "name"), + ("d.public.listings", "description"), + ("d.public.listings", "name"), + ], + database=MagicMock(), + ) + finally: + patch.stopall() + assert [one.column for one in found] == ["name", "description"] + assert api.return_value.list_indexes.call_count == 1 + + +def test_a_table_reference_that_is_not_three_parts_is_refused() -> None: + """The engine's index-lookup rewrite matches on the reference as written, so a + two-part form silently forfeits the index it was named to reach.""" + with pytest.raises(ValueError, match=r"catalog\.schema\.table"): + verify_searchable_columns( + MagicMock(), columns=[("public.listings", "description")], database=MagicMock() + ) + + +def test_nothing_is_named_without_a_database_to_confirm_against() -> None: + assert verify_searchable_columns(MagicMock(), columns=[("d.p.t", "c")], database=None) == [] + + +def test_only_a_column_the_engine_can_be_asked_in_sql_is_composable() -> None: + """A plain vector index needs a query vector, which an agent writing SQL cannot + produce; a provider-backed one takes text.""" + plain = SearchableColumn("d.p.t", vector("embedding")) + backed = SearchableColumn( + "d.p.t", + SearchIndex( + column="content", + kind=SEMANTIC, + index_type="vector", + ready=True, + embeds_query=True, + ), + ) + text = SearchableColumn("d.p.t", text_index()) + assert (plain.composable, backed.composable, text.composable) == (False, True, True) + assert (plain.function, backed.function, text.function) == ( + "vector_search", + "vector_search", + "bm25_search", + ) diff --git a/tests/test_search.py b/tests/test_search.py index 95ae81b..dbfde7e 100644 --- a/tests/test_search.py +++ b/tests/test_search.py @@ -908,7 +908,7 @@ def test_the_semantic_tool_and_the_sql_tool_agree_about_composing( described = {tool.name: tool.description or "" for tool in tools} sql = described["hotdata_execute_sql"] search = described["hotdata_search_semantic"] - assert "not available in SQL" in sql + assert f"Ranking '{COLUMN}' on {TABLE} by meaning needs the query as a vector" in sql assert "rank inside SQL instead" not in search diff --git a/tests/test_tools.py b/tests/test_tools.py index e18220d..e32f2d1 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -4,6 +4,7 @@ import socket import tempfile from pathlib import Path +from typing import Any from unittest.mock import MagicMock from urllib.request import Request @@ -395,7 +396,12 @@ def test_management_tools_are_on_by_default(mock_client): def test_the_sql_tool_stays_first_whichever_tools_are_included(mock_client): """It is the one every agent needs; the ordering the model sees should not shift.""" - for kwargs in ({}, {"management_tools": False}, {"describe_tables": False}): + kwarg_sets: tuple[dict[str, Any], ...] = ( + {}, + {"management_tools": False}, + {"describe_tables": False}, + ) + for kwargs in kwarg_sets: tools = make_hotdata_tools(mock_client, **kwargs) assert tools[0].name == DEFAULT_SQL_TOOL_NAME