Skip to content

Commit 3c29117

Browse files
committed
refactor: address review nits on create_index
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.
1 parent 2e01cd3 commit 3c29117

3 files changed

Lines changed: 75 additions & 25 deletions

File tree

‎hotdata_framework/client.py‎

Lines changed: 18 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
import time
55
from collections.abc import Iterator
66
from dataclasses import asdict, dataclass
7-
from typing import Any, Literal
7+
from typing import Any, Literal, get_args
88

99
from hotdata import ApiClient, Configuration
1010
from hotdata.api.connections_api import ConnectionsApi
@@ -71,8 +71,8 @@
7171
# the function from the index instead, so it cannot mismatch.
7272
VectorMetric = Literal["l2", "cosine", "dot"]
7373

74-
_INDEX_TYPES = frozenset({"sorted", "bm25", "vector"})
75-
_VECTOR_METRICS = frozenset({"l2", "cosine", "dot"})
74+
_INDEX_TYPES = frozenset(get_args(IndexType))
75+
_VECTOR_METRICS = frozenset(get_args(VectorMetric))
7676

7777
_TERMINAL = frozenset({"succeeded", "failed", "cancelled"})
7878
_RESULT_FAILURE = frozenset({"failed", "cancelled"})
@@ -514,7 +514,10 @@ def create_index(
514514
515515
``dimensions`` picks the output width for providers that support several;
516516
it does not apply when indexing an existing vector column, whose width is
517-
read from the data. A vector index takes exactly one column.
517+
read from the data. ``description`` is a user-facing label for the
518+
embedding (e.g. ``"product descriptions"``), stored alongside it. A vector
519+
index takes exactly one column, and every option in this paragraph — plus
520+
``metric`` — is rejected for a non-vector ``index_type``, matching the CLI.
518521
519522
The server builds the index as a background job. This method polls that
520523
job to a terminal state and raises ``RuntimeError`` if it failed, because
@@ -593,7 +596,10 @@ def create_index(
593596
raise RuntimeError(f"Unexpected create_index response type: {type(submitted)!r}")
594597

595598
job_id = submitted.id
596-
if not wait:
599+
600+
def requested_result(status: str) -> CreateIndexResult:
601+
"""Echo the requested values, for the paths where the server hands
602+
back a job rather than the built index."""
597603
return CreateIndexResult(
598604
full_name=full_name,
599605
schema_name=schema,
@@ -603,31 +609,24 @@ def create_index(
603609
columns=list(columns),
604610
metric=metric,
605611
source_column=columns[0] if embedding_provider_id else None,
606-
status="pending",
612+
status=status,
607613
job_id=job_id,
608614
)
609615

616+
if not wait:
617+
return requested_result(enum_value(submitted.status))
618+
610619
job = self._poll_job(job_id, timeout_s=timeout_s, interval_s=poll_interval_s)
611620
status = enum_value(job.status)
612621
if status != "succeeded":
613622
detail = job.error_message or f"Index build {status}"
614623
raise RuntimeError(f"Index {resolved_name!r} on {full_name}: {detail}")
615624

616-
built = job.result.actual_instance if job.result is not None else None
625+
# `result` is a oneOf wrapper today; tolerate the model arriving directly.
626+
built = getattr(job.result, "actual_instance", job.result)
617627
if isinstance(built, IndexInfoResponse):
618628
return self._index_result(built, full_name, schema, table, job_id=job_id)
619-
return CreateIndexResult(
620-
full_name=full_name,
621-
schema_name=schema,
622-
table_name=table,
623-
index_name=resolved_name,
624-
index_type=index_type,
625-
columns=list(columns),
626-
metric=metric,
627-
source_column=columns[0] if embedding_provider_id else None,
628-
status="ready",
629-
job_id=job_id,
630-
)
629+
return requested_result("ready")
631630

632631
@staticmethod
633632
def _index_result(

‎hotdata_framework/databases.py‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,12 +56,16 @@ class CreateIndexResult:
5656
owns checking the job's outcome.
5757
5858
``source_column`` is set only for an embedding-backed vector index, where it
59-
names the *text* column a query passes to ``vector_distance(col, 'text')``;
60-
``columns`` then holds the generated embedding column instead. It is ``None``
61-
for BM25, sorted, and plain (existing-vector-column) indexes.
62-
63-
``index_type``, ``columns``, and ``metric`` echo the requested values when
64-
the server does not return the built index alongside the finished job.
59+
names the *text* column a query passes to ``vector_distance(col, 'text')``.
60+
It is ``None`` for BM25, sorted, and plain (existing-vector-column) indexes.
61+
62+
``index_type``, ``columns``, and ``metric`` echo the requested values when the
63+
server does not return the built index alongside the finished job — that is,
64+
on the ``wait=False`` path and when a finished job carries no index payload.
65+
Only when the server did return it does ``columns`` hold the *generated*
66+
embedding column for an embedding-backed index; on the echoing paths
67+
``columns[0]`` is the source text column, the same value as
68+
``source_column``. Read ``status`` to tell the cases apart.
6569
"""
6670

6771
full_name: str

‎tests/test_indexes.py‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,53 @@ def test_create_index_wait_false_returns_pending_without_polling():
292292
assert result.columns == ["body"]
293293

294294

295+
def test_wait_false_reports_the_server_status_rather_than_assuming_pending():
296+
"""The server states the accepted job's status; don't hardcode it."""
297+
client = _client()
298+
submitted = SubmitJobResponse(id="job_2", status=JobStatus.RUNNING, status_url="/v1/jobs/job_2")
299+
indexes = FakeIndexesApi(submitted)
300+
jobs = FakeJobsApi([_job(JobStatus.SUCCEEDED)])
301+
302+
with (
303+
patch.object(client, "_indexes_api", return_value=indexes),
304+
patch.object(client, "_jobs_api", return_value=jobs),
305+
):
306+
result = client.create_index(
307+
_db(),
308+
"docs",
309+
columns=["body"],
310+
index_type="bm25",
311+
wait=False,
312+
)
313+
314+
assert jobs.calls == 0
315+
assert result.status == "running"
316+
317+
318+
def test_job_result_carrying_the_index_directly_is_still_read():
319+
"""`result` is a oneOf wrapper today; a bare model must not raise."""
320+
client = _client()
321+
indexes = FakeIndexesApi(_submitted())
322+
built = _index_info(index_name="docs_body_bm25", index_type="bm25", metric=None)
323+
# No `.actual_instance` — the model sits directly on `result`.
324+
jobs = FakeJobsApi([_job(JobStatus.SUCCEEDED, result=built)])
325+
326+
with (
327+
patch.object(client, "_indexes_api", return_value=indexes),
328+
patch.object(client, "_jobs_api", return_value=jobs),
329+
):
330+
result = client.create_index(
331+
_db(),
332+
"docs",
333+
columns=["body"],
334+
index_type="bm25",
335+
poll_interval_s=0,
336+
)
337+
338+
assert result.status == "ready"
339+
assert result.index_name == "docs_body_bm25"
340+
341+
295342
def test_create_index_accepts_an_inline_201_response():
296343
"""A build the server finishes inline answers with the index itself, not a job."""
297344
client = _client()

0 commit comments

Comments
 (0)