refactor(graph): drop the HNSW reader from the vector/search route

[jfrench9]

What

Removes the HNSW branch of POST /databases/{g}/tables/{t}/vector/search. HNSW indexes are searched in Cypher via CALL QUERY_VECTOR_INDEX; that route now returns 400 for backend="hnsw" with the query to send instead. The Lance branch is untouched, and so is /vector/build, which is the live half of the module.

Why

The HNSW reader read its rows through result.get_as_list() — a method ladybug 0.18.1's QueryResult does not have (it exposes get_all, rows_as_dict, get_next/has_next). The hasattr guard around it had no else, so the function fell through with an empty list:

result = conn.execute(query)          # ANN runs, rows come back
results = []
if hasattr(result, "get_as_list"):    # False
    ...
return {"results": results, "total": 0}   # 200 OK, zero rows, no error

Every HNSW search returned 200 {"results": [], "total": 0} with a plausible latency and nothing logged. The sibling call sites in the same module (lines 263, 386, 401) and tables/materialize.py:153 all carry the else list(result) fallback — only the reader lacked it.

Nothing consumes it. GraphClient.vector_search() is the route's only client and never sends backend, so it defaults to lance and cannot reach the branch; its sole caller is a unit test. /vector/build with backend="hnsw" is live (chunked_materialization.py:169) and is unaffected. The module docstring already said HNSW is "searched in Cypher via CALL QUERY_VECTOR_INDEX, not the /search route" — the code contradicted the docs shipping beside it.

Repairing a reader with no consumers would leave two ways to search one index, one of them undocumented. Removing it leaves the way that works, and the 400 hands the caller the working query rather than an empty list.

Tests

The existing tests could not have caught this — they set get_as_list on a MagicMock, so hasattr was true under test and false against the engine. That's the same shape of blind spot either way, so the replacements are route-level and go through the real request path: the 400, the pointer's contents, an explicit assertion that a 200-with-no-rows can no longer happen, and that backend="lance" is still served.

Verification

Beyond the suite, on the local stack against a subgraph with a FLOAT[384] embedding column and 8 nodes of real fastembed vectors:

• backend="hnsw" → 400 naming CALL QUERY_VECTOR_INDEX('Document', 'document_vec_index', ...) and POST /databases/{g}/query
• column (removed from the request model) → 422, per extra = "forbid"
• /vector/build backend="hnsw" → still indexes, row_count: 8
• CALL QUERY_VECTOR_INDEX through /v1/graphs/{g}/query/cypher → correct nearest neighbour on all three probes (goodwill → Goodwill impairment 0.316; coffee roasting → Espresso roasting 0.255; buyback → Share repurchase 0.351)

Follow-up, not in this PR

The hasattr(result, "get_as_list") pattern remains at three sites in this module plus materialize.py:153. All are correct today because they have the fallback, but all four are guarding against a method the engine no longer has, so all four silently take the fallback path. Worth collapsing to a single reader helper separately.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HHvoJGP23TsTZZ8njeRTWj

[github-actions]

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 4m 44s —— View job

---

Reviewing PR #1368

• Read changed files (client.py, vector_search.py, test_vector_hnsw.py)
• Check diff against origin/main
• Review correctness, edge cases, test coverage
• Post review feedback

Review

This is a clean, well-scoped cleanup. Verified the core claim: _search_hnsw_index (old vector_search.py:277-329ish) read rows via result.get_as_list() behind a bare hasattr guard with no else, so on the real ladybug 0.18.1 QueryResult (which only exposes get_all/rows_as_dict/get_next/has_next) it silently fell through to {"results": [], "total": 0} with a 200. That's a real, nasty bug class — success response, no rows, no log line.

Confirmed the removal is safe:

• GraphClient.vector_search() (client.py:898) never sends backend, so it always defaulted to lance — the HNSW branch had no live caller in the codebase (grep for _search_hnsw_index / backend.*hnsw turns up only this router, the client docstring, the test file, and the unrelated build_vector_index in client.py:1675 which is the /vector/build path and is untouched).
• /vector/build with backend="hnsw" and _build_hnsw_index are unmodified — chunked_materialization.py:169's live call path is unaffected.
• Removed imports (asyncio, time, logger) are all still used elsewhere in the file — no dead imports left behind.
• The column field removal on VectorSearchRequest is safe given extra = "forbid" was already in place and no code path used it outside the deleted reader.
• Index-name derivation in the new 400 detail (table_name.lower()}_vec_index) matches the convention used by _build_hnsw_index and DROP_VECTOR_INDEX elsewhere in the file (vector_search.py:300,375), so the pointer query is correct.

Test coverage: the new TestSearchRejectsHnsw class spins up the full app via create_app() + TestClient, matching the existing pattern in tests/graph_api/routers/databases/test_vector_search.py, and covers exactly the regression described — 400 for hnsw, the pointer query's contents, an explicit "never 200 with empty results" assertion, and that lance is untouched. This is the right fix for the blind spot called out in the PR body: the old tests mocked get_as_list onto a MagicMock, so hasattr was true under test and false against the real engine — same shape of gap either way, so route-level tests against real (un-mocked) response shape are the correct fix.

One very minor, non-blocking note: removing the column field is a breaking change for any external caller who was passing it explicitly (now 422 instead of being silently ignored). Given the PR body's verification and the fact that nothing internal ever sent it, this seems like the right call, just flagging for completeness.

I wasn't able to execute just test / uv run pytest in this sandbox (Bash tool required interactive approval that wasn't available here) — the test logic was reviewed by inspection only, not run. If this session's --allowedTools should permit uv run pytest tests/graph_api/..., running it directly would give higher confidence than the manual walkthrough above.

No blocking issues found. This looks good to merge.