test(vector): unit-test page-aware routing; clarify fallback comment
Address claude-review round 2 on PR #868: - Extract the use_page_aware branching into a pure `should_use_page_aware` helper and cover the (doc_type, page_boundaries, page_aware_setting) matrix in tests/unit/test_processor_routing.py (file+boundaries+enabled, empty list, None, non-file doc types, disabled setting). - Clarify the PageAwareChunker.chunk_text no-boundaries comment: the processor pre-filters via should_use_page_aware, so that branch is a direct-call safety net, not a production indexing path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
8d20339b3a
commit
bb4ef809c2
@@ -167,8 +167,11 @@ class PageAwareChunker:
|
|||||||
if not content:
|
if not content:
|
||||||
return [ChunkWithPosition(text="", start_offset=0, end_offset=0)]
|
return [ChunkWithPosition(text="", start_offset=0, end_offset=0)]
|
||||||
|
|
||||||
# No page info (e.g. a non-PDF that reached this path) — degrade to the
|
# No page info — degrade to char-based behaviour so the class is safe to
|
||||||
# char-based behaviour so the caller still gets sensible chunks.
|
# call directly. The vector-sync processor pre-filters this case
|
||||||
|
# (``should_use_page_aware`` requires a truthy boundary list), so in
|
||||||
|
# production this branch is only reached by direct callers/tests, not
|
||||||
|
# the indexing path.
|
||||||
if not page_boundaries:
|
if not page_boundaries:
|
||||||
docs = await anyio.to_thread.run_sync( # type: ignore[attr-defined]
|
docs = await anyio.to_thread.run_sync( # type: ignore[attr-defined]
|
||||||
self.splitter.create_documents,
|
self.splitter.create_documents,
|
||||||
|
|||||||
@@ -92,6 +92,25 @@ def assign_page_numbers(chunks, page_boundaries):
|
|||||||
chunk.page_number = assigned_page
|
chunk.page_number = assigned_page
|
||||||
|
|
||||||
|
|
||||||
|
def should_use_page_aware(
|
||||||
|
*, page_aware_enabled: bool, doc_type: str, page_boundaries: Any
|
||||||
|
) -> bool:
|
||||||
|
"""Decide whether the page-aware chunker applies to this document.
|
||||||
|
|
||||||
|
Page-aware chunking applies only to paginated files (PDFs) that actually
|
||||||
|
carry page boundaries. ``page_boundaries`` is tested for truthiness, not
|
||||||
|
just ``is not None``: an empty list carries no pages, so it routes through
|
||||||
|
the char-based path rather than the page-aware chunker's no-boundaries
|
||||||
|
fallback.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
page_aware_enabled: ``settings.document_chunk_page_aware``.
|
||||||
|
doc_type: The document type (only ``"file"`` is paginated).
|
||||||
|
page_boundaries: The extractor's page-boundary list (or ``None``).
|
||||||
|
"""
|
||||||
|
return page_aware_enabled and doc_type == "file" and bool(page_boundaries)
|
||||||
|
|
||||||
|
|
||||||
async def processor_task(
|
async def processor_task(
|
||||||
worker_id: int,
|
worker_id: int,
|
||||||
receive_stream: MemoryObjectReceiveStream[DocumentTask],
|
receive_stream: MemoryObjectReceiveStream[DocumentTask],
|
||||||
@@ -690,13 +709,10 @@ async def _index_document(
|
|||||||
# which assigns page numbers inline; everything else uses the char-based
|
# which assigns page numbers inline; everything else uses the char-based
|
||||||
# chunker followed by post-hoc page assignment.
|
# chunker followed by post-hoc page assignment.
|
||||||
page_boundaries = file_metadata.get("page_boundaries")
|
page_boundaries = file_metadata.get("page_boundaries")
|
||||||
use_page_aware = (
|
use_page_aware = should_use_page_aware(
|
||||||
settings.document_chunk_page_aware
|
page_aware_enabled=settings.document_chunk_page_aware,
|
||||||
and doc_task.doc_type == "file"
|
doc_type=doc_task.doc_type,
|
||||||
# Truthy (not just "is not None"): an empty list carries no pages, so
|
page_boundaries=page_boundaries,
|
||||||
# route it through the char-based path rather than the page-aware
|
|
||||||
# chunker's no-boundaries fallback.
|
|
||||||
and bool(page_boundaries)
|
|
||||||
)
|
)
|
||||||
with trace_operation(
|
with trace_operation(
|
||||||
"vector_sync.chunk_text",
|
"vector_sync.chunk_text",
|
||||||
|
|||||||
@@ -0,0 +1,62 @@
|
|||||||
|
"""Unit tests for the page-aware chunker routing decision (processor.py)."""
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
from nextcloud_mcp_server.vector.processor import should_use_page_aware
|
||||||
|
|
||||||
|
pytestmark = pytest.mark.unit
|
||||||
|
|
||||||
|
_BOUNDARIES = [{"page": 1, "start_offset": 0, "end_offset": 10}]
|
||||||
|
|
||||||
|
|
||||||
|
class TestShouldUsePageAware:
|
||||||
|
"""Cover the (doc_type, page_boundaries, page_aware_setting) matrix."""
|
||||||
|
|
||||||
|
def test_pdf_with_boundaries_and_enabled_uses_page_aware(self):
|
||||||
|
assert (
|
||||||
|
should_use_page_aware(
|
||||||
|
page_aware_enabled=True,
|
||||||
|
doc_type="file",
|
||||||
|
page_boundaries=_BOUNDARIES,
|
||||||
|
)
|
||||||
|
is True
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_empty_boundaries_falls_back_to_char_based(self):
|
||||||
|
"""Empty list carries no pages -> char-based path."""
|
||||||
|
assert (
|
||||||
|
should_use_page_aware(
|
||||||
|
page_aware_enabled=True, doc_type="file", page_boundaries=[]
|
||||||
|
)
|
||||||
|
is False
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_none_boundaries_falls_back_to_char_based(self):
|
||||||
|
assert (
|
||||||
|
should_use_page_aware(
|
||||||
|
page_aware_enabled=True, doc_type="file", page_boundaries=None
|
||||||
|
)
|
||||||
|
is False
|
||||||
|
)
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("doc_type", ["note", "deck_card", "news_item"])
|
||||||
|
def test_non_file_doc_types_never_page_aware(self, doc_type):
|
||||||
|
"""Only paginated files are page-aware, even with boundaries present."""
|
||||||
|
assert (
|
||||||
|
should_use_page_aware(
|
||||||
|
page_aware_enabled=True,
|
||||||
|
doc_type=doc_type,
|
||||||
|
page_boundaries=_BOUNDARIES,
|
||||||
|
)
|
||||||
|
is False
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_disabled_setting_forces_char_based(self):
|
||||||
|
assert (
|
||||||
|
should_use_page_aware(
|
||||||
|
page_aware_enabled=False,
|
||||||
|
doc_type="file",
|
||||||
|
page_boundaries=_BOUNDARIES,
|
||||||
|
)
|
||||||
|
is False
|
||||||
|
)
|
||||||
Reference in New Issue
Block a user