feat: add create_index to HotdataClient - #53
Conversation
Bring the framework client to parity with `hotdata indexes create`, which
already creates BM25 and vector indexes from the CLI. Previously callers
had to drop to the raw `hotdata.IndexesApi`, leaving a managed database's
data loaded but not searchable.
Covers both vector-index modes: a plain index over an existing vector
column, and a provider-backed index over a source text column that the
server embeds. The returned `source_column` names the column to pass to
`vector_distance` in the latter.
The build runs as a background job whose submit call reports success even
when the build later fails, so the method polls the job to a terminal
state and raises RuntimeError with the job's error_message. `wait=False`
mirrors the CLI's `--async`.
`index_name` derives from `{table}_{columns}_{index_type}` when omitted,
matching the CLI so both surfaces name the same index identically.
`index_type` is required rather than inheriting the API's "sorted"
default. Combinations the server would silently ignore — a multi-column
vector index, or vector-only options on a non-vector index — raise
ValueError, since each otherwise surfaces only as an unaccelerated query.
Release 0.10.0.
| detail = job.error_message or f"Index build {status}" | ||
| raise RuntimeError(f"Index {resolved_name!r} on {full_name}: {detail}") | ||
|
|
||
| built = job.result.actual_instance if job.result is not None else None |
There was a problem hiding this comment.
nit: .actual_instance is accessed unguarded, and every test that exercises this line mocks the job as SimpleNamespace(actual_instance=...) — so the suite gives no evidence about the real shape of JobStatusResponse.result. If that field is ever the model directly rather than a oneOf wrapper (or a dict), a successful build raises AttributeError from here instead of returning a result. Cheap to make shape-agnostic:
| built = job.result.actual_instance if job.result is not None else None | |
| built = getattr(job.result, "actual_instance", job.result) |
(not blocking)
| last = jobs.get_job(job_id) | ||
| except ApiException as e: | ||
| raise RuntimeError(api_error_message(e)) from e | ||
| if last.status in _JOB_TERMINAL: |
There was a problem hiding this comment.
nit: this is an allowlist of terminal states, so any terminal status not in _JOB_TERMINAL — a cancelled state that appears later, or any new state the server adds — is treated as "still running" and burns the whole timeout_s (300s by default) before surfacing as TimeoutError rather than a RuntimeError naming the actual outcome. The comment at line 79 asserts jobs have no cancelled state, which is true of today's server but is exactly the kind of thing that changes without a client release. Checking the non-terminal set instead fails fast and degrades better:
| if last.status in _JOB_TERMINAL: | |
| if enum_value(last.status) not in _JOB_RUNNING: # {"pending", "running"} |
(not blocking)
| if not wait: | ||
| return CreateIndexResult( | ||
| full_name=full_name, | ||
| schema_name=schema, | ||
| table_name=table, | ||
| index_name=resolved_name, | ||
| index_type=index_type, | ||
| columns=list(columns), | ||
| metric=metric, | ||
| source_column=columns[0] if embedding_provider_id else None, | ||
| status="pending", | ||
| job_id=job_id, | ||
| ) |
There was a problem hiding this comment.
nit: two things about this block.
- It is byte-for-byte the same construction as lines 619–630 except for
status. A single local helper (closing overfull_name/schema/table/resolved_name/index_type/columns/metric/embedding_provider_idand takingstatus) would keep the two paths from drifting — e.g. a future field added toCreateIndexResultis easy to add in one place and forget in the other. status="pending"is synthesized even though the server just told you what it is onsubmitted.status. If the server ever answers 202 withrunning, the result misreports.enum_value(submitted.status)is the faithful value and_submitted()in the tests already sets it.
(not blocking)
| _INDEX_TYPES = frozenset({"sorted", "bm25", "vector"}) | ||
| _VECTOR_METRICS = frozenset({"l2", "cosine", "dot"}) |
There was a problem hiding this comment.
super nit: these restate IndexType and VectorMetric (lines 65 and 72), so adding a kind or metric means editing two places and a miss silently rejects a valid value at runtime while type-checking clean. Deriving them removes the second source of truth:
| _INDEX_TYPES = frozenset({"sorted", "bm25", "vector"}) | |
| _VECTOR_METRICS = frozenset({"l2", "cosine", "dot"}) | |
| _INDEX_TYPES = frozenset(get_args(IndexType)) | |
| _VECTOR_METRICS = frozenset(get_args(VectorMetric)) |
(needs get_args added to the typing import) (not blocking)
| "dimensions": dimensions, | ||
| "embedding_provider_id": embedding_provider_id, | ||
| "output_column": output_column, | ||
| "description": description, |
There was a problem hiding this comment.
super nit: description is grouped with the genuinely vector-specific options here, but unlike metric/dimensions/embedding_provider_id/output_column it carries no vector semantics — it reads as generic index metadata. Rejecting it on bm25/sorted is a hard error on a call the server would accept, and if the server does persist it (or starts to) for those kinds, callers have to wait for a client release to use it. Being strict about combinations the server silently ignores is the right instinct; I'd just double-check description is actually one of them rather than stored-and-unused.
Related: description is the one parameter in the signature that the docstring never explains, so its meaning is only discoverable via the rejection message. (not blocking)
| ``source_column`` is set only for an embedding-backed vector index, where it | ||
| names the *text* column a query passes to ``vector_distance(col, 'text')``; | ||
| ``columns`` then holds the generated embedding column instead. It is ``None`` | ||
| for BM25, sorted, and plain (existing-vector-column) indexes. | ||
|
|
||
| ``index_type``, ``columns``, and ``metric`` echo the requested values when | ||
| the server does not return the built index alongside the finished job. |
There was a problem hiding this comment.
super nit: "columns then holds the generated embedding column instead" holds only when the server returned the built index. On the wait=False path and the succeeded-but-no-result fallback, provider-backed mode returns columns == [source_text_col] and source_column == source_text_col — the same value in both fields, not the embedding column. The sentence at the end of the docstring hedges this ("echo the requested values when the server does not return the built index"), but a reader who takes the earlier paragraph literally will use columns[0] as the embedding column name and get the text column. Worth making the exception explicit in the source_column paragraph itself. (not blocking)
There was a problem hiding this comment.
Additive, well-scoped, and consistent with the existing managed-table methods: same database resolution via _as_managed_database, same ApiException -> RuntimeError wrapping, and _poll_job mirrors _poll_query_run. Polling the build job instead of trusting the 202 is the right call, and the isinstance dispatch follows the _execute_sql_once precedent. CONTRACT/CHANGELOG/README and the export list are all updated together, and the tests cover the failure, partially_succeeded, timeout, inline-201, and validation branches.
Six non-blocking comments inline. The one worth a look before merge is the unguarded job.result.actual_instance on the success path — every test that reaches it mocks that shape, so the suite can't tell you whether it matches the real JobStatusResponse.
Note that CI (CI / Test (Python 3.12), Verify changelog matches version bump) had not reported when this review ran, so I have not seen the test results.
The 0.10.0 heading carried the date the entry was drafted, not the release date. The entry also implied a vector index accelerates queries; verification against api.hotdata.dev found it does not currently do so on a managed database, so the entry now says what was observed.
There was a problem hiding this comment.
Docs-only changes since the last review (CHANGELOG release date, README wording) — both accurate, and the vector-planner caveat is a useful thing to have written down in the changelog rather than only in the PR description.
No new findings. The prior threads are all nits/super nits and stay non-blocking; not re-raising them.
Note: I could not confirm that JobStatus is a str-mixin enum (the hotdata dependency isn't installed in the review checkout), which is what makes the raw membership test at client.py:802 work where line 611 uses enum_value. tests/test_indexes.py covers it with the real enum, but CI / Test (Python 3.12) hadn't reported when this review ran — worth a glance at that check before merging.
Derive the index-type and metric frozensets from their Literal aliases so the accepted values have one source of truth. Collapse the two requested-value CreateIndexResult constructions into a local helper, and report the server's own status on the wait=False path instead of assuming "pending". Read the finished job's payload shape-agnostically so a bare model on `result` does not raise on the success path. Document `description`, and spell out in CreateIndexResult that `columns` holds the generated embedding column only when the server returned the built index — on the echoing paths it is the source text column.
| inline. A caller that passed ``wait=False`` always gets ``"pending"`` and | ||
| owns checking the job's outcome. |
There was a problem hiding this comment.
nit: switching wait=False to report enum_value(submitted.status) left the docs behind — three places still promise "pending" unconditionally, and the new test asserts "running", so a reader following the docs writes if result.status == "pending" and misses the accepted-but-already-running case:
- here (
"A caller that passed wait=False always gets \"pending\"") client.py:526(the result then carries status="pending" and a job_id)CONTRACT.md:67andCHANGELOG.md:34
The truthful statement is now "whatever status the server reported when it accepted the job — pending in practice, but read it rather than assume".
| inline. A caller that passed ``wait=False`` always gets ``"pending"`` and | |
| owns checking the job's outcome. | |
| inline. A caller that passed ``wait=False`` gets whatever status the server | |
| reported when it accepted the job (``"pending"`` in practice) and owns | |
| checking the job's outcome. |
(not blocking)
There was a problem hiding this comment.
Prior nits addressed: shape-agnostic job.result read (with a test for the bare-model case), get_args-derived _INDEX_TYPES/_VECTOR_METRICS, the duplicated result construction folded into requested_result() now reporting the server's own status, and the columns/source_column echoing exception spelled out in CreateIndexResult. One leftover doc nit inline; nothing blocking.
Note: CI's test job had not reported when this review ran, so I have no test results to cite.
The entry ran 46 lines for a single added method, against neighbouring entries of 3-10 (0.9.0 = 10, 0.8.0 = 6, 0.7.2 = 3). Trimmed to 6. Keeps what a reader scanning the changelog needs: what the method is, and that it polls the build job because the submit call reports success even when the build later fails. Everything cut is already documented in the create_index docstring, CONTRACT.md, and #53.
The entry ran 46 lines for a single added method, against neighbouring entries of 3-10 (0.9.0 = 10, 0.8.0 = 6, 0.7.2 = 3). Trimmed to 6. Keeps what a reader scanning the changelog needs: what the method is, and that it polls the build job because the submit call reports success even when the build later fails. Everything cut is already documented in the create_index docstring, CONTRACT.md, and #53.
Why
HotdataClienthad no index API at all, so a caller could create a managed database and load data into it, then not make that data searchable — full-text queries error outright without an index, and vector queries run at full-scan speed. The only workaround was dropping to the rawhotdata.IndexesApi, whichhotdata-langchain's BM25 demo does today.This is a parity gap, not a new capability:
hotdata indexes createhas done this from the CLI all along. The CLI's flags are treated as the spec here.Closes item 2 of hotdata-dev/hotdata-langchain#36.
What
create_index(database, table, *, columns, index_type, ...) -> CreateIndexResult, covering all three kinds the API accepts (bm25,vector,sorted) and both vector modes:embedding_provider_idcolumnsiscosine_distance(col, ARRAY[…])vector_distance(col, 'text')The returned
source_columnnames the column to query in provider-backed mode, where the indexed column is the generated embedding column rather than the one you passed.Design decisions worth reviewing
It polls the build job. With
async, the create call returnsSubmitJobResponse(status="pending")and looks successful even for a build that later fails — the failure appears only onJobsApi.get_job(id).error_message. Without polling, callers believe they have an index they don't have.wait=Falseopts out (the CLI's--async).The generated client types
create_indexas returningIndexInfoResponse, but at runtime it returns that on 201 orSubmitJobResponseon 202, so the code dispatches onisinstance— the same shape_execute_sql_oncealready uses forQueryApi.query. (The Rust CLI hit this too and bypasses its typed SDK with a raw POST.)Stricter than the API where the API fails silently. Each of these otherwise surfaces only as a slow query, never an error:
index_typeis required; the API would default it to"sorted".columns[0]and drops the rest.metric/dimensions/embedding_provider_id/output_column/descriptionare rejected on non-vector indexes. The server silently drops most of these.metricis validated againstl2/cosine/dot. Note the wire value isdot, notip—ipis the internal USearchMetricKindname. Confirmed inruntimedb'smetric_to_distance_function.index_nameis optional, deriving{table}_{columns}_{index_type}— byte-identical to the CLI's derivation when--nameis omitted, so both surfaces name the same index the same way.Scope: managed databases only. The CLI's
--catalogalso accepts a plain connection; that's deliberately out of scope here, consistent with every other managed-table method on this client, and stated in the docstring.Verification
Contracts checked statically against
hotdata0.8.0 and theruntimedb/hotdata-clisources: same endpoint path as the CLI, same request body shape,var_asyncreally serializes to the"async"wire alias, and managed-DB resolution todefault_connection_idmatchestry_resolve_connection_id.Then run end to end against
api.hotdata.dev— create database → load 64 rows → build BM25 + cosine vector indexes → confirm vialist_indexes→ delete. BM25 built in 3.1s, vector in 0.7s, bothready, both correctly named.create_indexagainst a nonexistent table raisedRuntimeErrorrather than reporting phantom success. Test databases were cleaned up.123 unit tests (
tests/test_indexes.py, 26 new) cover the job-failure path,partially_succeeded, timeouts, the 201 inline path, enum-vs-wire-string status, read-probe skipping, and every validation branch.Known issue — not introduced here
A plain vector index is created correctly (
status=ready, right metric and column) but the planner does not rewritecosine_distance(…) ORDER BY … LIMIT ktoUSearchExec; it full-scans. Reproduced identically with an index created byhotdata indexes createwith no wrapper involved, so it is not a client bug. Held across 1536 dims/200 rows, a 2+ minute wait, and all three table-reference forms. BM25 on the same table works, so it is vector-rewrite-specific. Managed-database indexes register under the internal__db_<id>.public.docslabel while the plan seesdefault.public.docs— a suspected, unconfirmed registry-key mismatch. Worth a separate engine issue.Separately, the
could not detect dimensionfailure noted during earlier spikes is runtimedb #934 (scan-based detection is unreliable for a DuckLakeListcolumn), already fixed upstream by pinning the width from the catalog.Release
Ships as 0.10.0 (additive;
hotdata_framework/is 291 insertions and zero deletions).hotdata-langchainpins>=0.9.0and will need a bump to consume this.Follow-ups
list_indexes/delete_indexwrappers, so an idempotentensure_indexstill needs the raw API.async_after_msis not exposed; polling makes it redundant.