fix(vector): address PR review round 13 — index offset fields + tighten test
- Add chunk_start_offset / chunk_end_offset to _PAYLOAD_INDEX_FIELDS so the legacy offset-based fallback in search/context.py works on Qdrant Cloud strict mode (pre-#75 clients have no chunk_index payload). - Cover chunk_index / chunk_start_offset / chunk_end_offset in the payload-index summary test; refresh the stale field-list comment. - Flag the is_valid_nextcloud_doc_id gate at both chunk-context handler sites with a TODO for future non-numeric doc_types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
f9ad7dc52e
commit
ae23bbe8b8
@@ -505,6 +505,9 @@ async def get_chunk_context(request: Request) -> JSONResponse:
|
|||||||
# ints from MySQL auto_increment; doc_id stays a str downstream
|
# ints from MySQL auto_increment; doc_id stays a str downstream
|
||||||
# (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id
|
# (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id
|
||||||
# rejects "0", leading zeros, and Unicode digits that pass isdigit().
|
# rejects "0", leading zeros, and Unicode digits that pass isdigit().
|
||||||
|
# TODO: when chunk-context support extends to non-numeric doc_types
|
||||||
|
# (calendar VEVENT UIDs, CardDAV hrefs, …), relax this gate or make
|
||||||
|
# it doc_type-aware. Today every indexed doc_type is numeric.
|
||||||
if not is_valid_nextcloud_doc_id(doc_id):
|
if not is_valid_nextcloud_doc_id(doc_id):
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -567,6 +567,9 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse:
|
|||||||
# ints from MySQL auto_increment; doc_id stays a str downstream
|
# ints from MySQL auto_increment; doc_id stays a str downstream
|
||||||
# (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id
|
# (Qdrant payload index is keyword-typed). is_valid_nextcloud_doc_id
|
||||||
# rejects "0", leading zeros, and Unicode digits that pass isdigit().
|
# rejects "0", leading zeros, and Unicode digits that pass isdigit().
|
||||||
|
# TODO: when chunk-context support extends to non-numeric doc_types
|
||||||
|
# (calendar VEVENT UIDs, CardDAV hrefs, …), relax this gate or make
|
||||||
|
# it doc_type-aware. Today every indexed doc_type is numeric.
|
||||||
if not is_valid_nextcloud_doc_id(doc_id):
|
if not is_valid_nextcloud_doc_id(doc_id):
|
||||||
return JSONResponse(
|
return JSONResponse(
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -31,13 +31,18 @@ logger = logging.getLogger(__name__)
|
|||||||
# ``_get_chunk_by_index_from_qdrant`` and ``get_chunk_bbox_and_page_from_qdrant``
|
# ``_get_chunk_by_index_from_qdrant`` and ``get_chunk_bbox_and_page_from_qdrant``
|
||||||
# (see search/context.py) — the always-indexed fast path that the offset-based
|
# (see search/context.py) — the always-indexed fast path that the offset-based
|
||||||
# fallback exists to avoid; it has to actually be indexed for that promise to
|
# fallback exists to avoid; it has to actually be indexed for that promise to
|
||||||
# hold on Qdrant Cloud strict mode.
|
# hold on Qdrant Cloud strict mode. chunk_start_offset / chunk_end_offset are
|
||||||
|
# the ints used by the legacy offset fallback in the same module — pre-#75
|
||||||
|
# clients have no chunk_index payload, so the offset path still has to work
|
||||||
|
# (or 400 silently and return None on Qdrant Cloud strict mode).
|
||||||
_PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType] = {
|
_PAYLOAD_INDEX_FIELDS: dict[str, PayloadSchemaType] = {
|
||||||
"doc_id": PayloadSchemaType.KEYWORD,
|
"doc_id": PayloadSchemaType.KEYWORD,
|
||||||
"user_id": PayloadSchemaType.KEYWORD,
|
"user_id": PayloadSchemaType.KEYWORD,
|
||||||
"doc_type": PayloadSchemaType.KEYWORD,
|
"doc_type": PayloadSchemaType.KEYWORD,
|
||||||
"is_placeholder": PayloadSchemaType.BOOL,
|
"is_placeholder": PayloadSchemaType.BOOL,
|
||||||
"chunk_index": PayloadSchemaType.INTEGER,
|
"chunk_index": PayloadSchemaType.INTEGER,
|
||||||
|
"chunk_start_offset": PayloadSchemaType.INTEGER,
|
||||||
|
"chunk_end_offset": PayloadSchemaType.INTEGER,
|
||||||
}
|
}
|
||||||
|
|
||||||
# Sentinel point that records "this collection has been backfilled to str
|
# Sentinel point that records "this collection has been backfilled to str
|
||||||
|
|||||||
@@ -763,9 +763,10 @@ async def test_ensure_payload_indexes_summarises_failed_fields(mocker, caplog):
|
|||||||
"""
|
"""
|
||||||
client = mocker.AsyncMock()
|
client = mocker.AsyncMock()
|
||||||
client.get_collection.return_value = SimpleNamespace(payload_schema={})
|
client.get_collection.return_value = SimpleNamespace(payload_schema={})
|
||||||
# All but the second field fail with 5xx. _PAYLOAD_INDEX_FIELDS has
|
# _PAYLOAD_INDEX_FIELDS preserves insertion order; user_id is the second
|
||||||
# insertion-ordered keys (doc_id, user_id, doc_type, is_placeholder),
|
# entry, so call #2 is the success case and every other field fails. Don't
|
||||||
# so call #2 (user_id) is the success case.
|
# hard-code the full field list here — it grows as new fields move into
|
||||||
|
# the index dict, and the assertions below are what enforce coverage.
|
||||||
call_count = {"n": 0}
|
call_count = {"n": 0}
|
||||||
|
|
||||||
async def _create_index(*args, **kwargs):
|
async def _create_index(*args, **kwargs):
|
||||||
@@ -787,9 +788,14 @@ async def test_ensure_payload_indexes_summarises_failed_fields(mocker, caplog):
|
|||||||
if "Payload index creation incomplete" in r.getMessage()
|
if "Payload index creation incomplete" in r.getMessage()
|
||||||
]
|
]
|
||||||
assert len(summary) == 1
|
assert len(summary) == 1
|
||||||
# All fields except user_id should appear in the summary.
|
# Every field that failed must appear in the summary — operators rely on
|
||||||
|
# this single log line to spot the degraded state, so any missing entry
|
||||||
|
# is a silent gap.
|
||||||
assert "doc_id" in summary[0]
|
assert "doc_id" in summary[0]
|
||||||
assert "doc_type" in summary[0]
|
assert "doc_type" in summary[0]
|
||||||
assert "is_placeholder" in summary[0]
|
assert "is_placeholder" in summary[0]
|
||||||
|
assert "chunk_index" in summary[0]
|
||||||
|
assert "chunk_start_offset" in summary[0]
|
||||||
|
assert "chunk_end_offset" in summary[0]
|
||||||
assert "user_id" not in summary[0] # The one that succeeded.
|
assert "user_id" not in summary[0] # The one that succeeded.
|
||||||
assert "test-collection" in summary[0]
|
assert "test-collection" in summary[0]
|
||||||
|
|||||||
Reference in New Issue
Block a user