Select documents to filter by id, not by displayed name
A title repeats within a corpus and across databases, so a substring match on the displayed name widened the filter to documents the user did not pick. The label names the database.
This commit is contained in:
parent
16e0add64b
commit
307e250f29
7 changed files with 119 additions and 17 deletions
|
|
@ -11,6 +11,7 @@
|
|||
|
||||
### Fixed
|
||||
|
||||
- The chat TUI's document filter selects documents by id and names each document's database, instead of matching the displayed title or URI as a substring across every database.
|
||||
- `doctor`'s docling-serve probe sends `X-Api-Key`, so an instance requiring a key is reported reachable rather than unreachable.
|
||||
- The picture-description request to the public OpenAI endpoint sends `OPENAI_API_KEY`; it carried no authorization header.
|
||||
- `haiku-rag` prints the message and exits when the configured embedder does not match the database, instead of raising a traceback.
|
||||
|
|
|
|||
|
|
@ -416,11 +416,11 @@ class ChatApp(App):
|
|||
|
||||
def on_document_filter_modal_filter_changed(self, event: Any) -> None:
|
||||
"""Handle document filter changes from modal."""
|
||||
from haiku.rag.tools.filters import build_multi_document_filter
|
||||
from haiku.rag.tools.filters import build_document_id_filter
|
||||
|
||||
self._document_filter = event.selected
|
||||
|
||||
doc_filter = build_multi_document_filter(self._document_filter)
|
||||
doc_filter = build_document_id_filter(self._document_filter)
|
||||
for namespace, state_type in (
|
||||
(RAG_STATE_NAMESPACE, RAGState),
|
||||
(ANALYSIS_STATE_NAMESPACE, AnalysisState),
|
||||
|
|
|
|||
|
|
@ -148,19 +148,37 @@ class DocumentFilterModal(ModalScreen):
|
|||
await filter_list.remove_children()
|
||||
|
||||
# The page is picked to represent every database; sorting is so it reads
|
||||
# like a list rather than in whatever order the tables returned.
|
||||
names = sorted(doc.title or doc.uri or str(doc.id) for doc in docs)
|
||||
# like a list rather than in whatever order the tables returned. The label
|
||||
# names the database, since a title alone does not say which one it is in.
|
||||
labelled = sorted(
|
||||
(
|
||||
(
|
||||
f"{doc.title or doc.uri or doc.id}"
|
||||
+ (f" ({doc.source})" if doc.source else ""),
|
||||
doc.id,
|
||||
)
|
||||
for doc in docs
|
||||
if doc.id is not None
|
||||
),
|
||||
key=lambda pair: pair[0],
|
||||
)
|
||||
|
||||
boxes = []
|
||||
for position, display_name in enumerate(names):
|
||||
for position, (label, doc_id) in enumerate(labelled):
|
||||
checkbox = Checkbox(
|
||||
display_name,
|
||||
value=display_name in self._selected,
|
||||
# Positional, because titles repeat and a repeated id is an error.
|
||||
label,
|
||||
value=doc_id in self._selected,
|
||||
# Positional, because a label is not unique and a repeated
|
||||
# widget id is an error.
|
||||
id=f"doc-{position}",
|
||||
classes="doc-checkbox",
|
||||
)
|
||||
checkbox._doc_id = display_name # type: ignore[attr-defined] # ty: ignore[unresolved-attribute]
|
||||
# The selection is the document id: a title repeats within a corpus
|
||||
# and across databases, so selecting by name widens to documents the
|
||||
# user did not pick.
|
||||
checkbox._doc_id = doc_id # type: ignore[attr-defined] # ty: ignore[unresolved-attribute]
|
||||
# Not `_label`: Textual's ToggleButton owns that name.
|
||||
checkbox._search_text = label # type: ignore[attr-defined] # ty: ignore[unresolved-attribute]
|
||||
boxes.append(checkbox)
|
||||
if boxes:
|
||||
await filter_list.mount_all(boxes)
|
||||
|
|
@ -207,8 +225,8 @@ class DocumentFilterModal(ModalScreen):
|
|||
filter_list = self.query_one("#filter-list", VerticalScroll)
|
||||
|
||||
for checkbox in filter_list.query(Checkbox):
|
||||
doc_id = getattr(checkbox, "_doc_id", "")
|
||||
if search_term == "" or search_term in doc_id.lower():
|
||||
label = getattr(checkbox, "_search_text", "")
|
||||
if search_term == "" or search_term in label.lower():
|
||||
checkbox.display = True
|
||||
else:
|
||||
checkbox.display = False
|
||||
|
|
|
|||
|
|
@ -1,10 +1,14 @@
|
|||
from haiku.rag.tools.context import RAGDeps
|
||||
from haiku.rag.tools.document import create_document_toolset
|
||||
from haiku.rag.tools.filters import build_multi_document_filter
|
||||
from haiku.rag.tools.filters import (
|
||||
build_document_id_filter,
|
||||
build_multi_document_filter,
|
||||
)
|
||||
from haiku.rag.tools.search import create_search_toolset
|
||||
|
||||
__all__ = [
|
||||
"RAGDeps",
|
||||
"build_document_id_filter",
|
||||
"build_multi_document_filter",
|
||||
"create_document_toolset",
|
||||
"create_search_toolset",
|
||||
|
|
|
|||
|
|
@ -1,3 +1,6 @@
|
|||
from haiku.rag.utils import escape_sql_string
|
||||
|
||||
|
||||
def _build_document_filter(document_name: str) -> str:
|
||||
"""Build SQL filter for document name matching.
|
||||
|
||||
|
|
@ -12,6 +15,19 @@ def _build_document_filter(document_name: str) -> str:
|
|||
)
|
||||
|
||||
|
||||
def build_document_id_filter(document_ids: list[str]) -> str | None:
|
||||
"""SQL filter matching exactly these documents, or None for an empty list.
|
||||
|
||||
Unlike name matching, an id identifies one document: names repeat within a
|
||||
corpus and across databases, so a name filter can widen to documents the
|
||||
caller did not pick.
|
||||
"""
|
||||
if not document_ids:
|
||||
return None
|
||||
ids = ", ".join(f"'{escape_sql_string(i)}'" for i in document_ids)
|
||||
return f"id IN ({ids})"
|
||||
|
||||
|
||||
def build_multi_document_filter(document_names: list[str]) -> str | None:
|
||||
"""Build SQL filter for multiple document names (OR combined).
|
||||
|
||||
|
|
|
|||
|
|
@ -386,22 +386,27 @@ async def test_document_filter_updates_rag_state(temp_db_path: Path):
|
|||
"""Test that selecting document filters updates RAGState.document_filter."""
|
||||
from haiku.rag.chat.app import RAG_STATE_NAMESPACE
|
||||
from haiku.rag.chat.widgets.document_filter_modal import DocumentFilterModal
|
||||
from haiku.rag.tools.filters import build_multi_document_filter
|
||||
from haiku.rag.tools.filters import build_document_id_filter
|
||||
|
||||
app, mock_client = _make_app_with_state(temp_db_path)
|
||||
|
||||
with patch("haiku.rag.chat.app.HaikuRAG", return_value=mock_client):
|
||||
async with app.run_test():
|
||||
# Simulate the FilterChanged message
|
||||
selected = ["AI Overview", "ML Basics"]
|
||||
# The selection is document ids, so a repeated title cannot widen it.
|
||||
selected = [
|
||||
"6f1c2d4e-0000-4000-8000-000000000001",
|
||||
"6f1c2d4e-0000-4000-8000-000000000002",
|
||||
]
|
||||
app.on_document_filter_modal_filter_changed(
|
||||
DocumentFilterModal.FilterChanged(selected)
|
||||
)
|
||||
|
||||
# RAGState.document_filter should be set
|
||||
rag_state = RAGState.model_validate(app._state[RAG_STATE_NAMESPACE])
|
||||
expected_filter = build_multi_document_filter(selected)
|
||||
expected_filter = build_document_id_filter(selected)
|
||||
assert rag_state.document_filter == expected_filter
|
||||
assert rag_state.document_filter is not None
|
||||
assert "LIKE" not in rag_state.document_filter
|
||||
|
||||
# The state snapshot should also reflect the change
|
||||
assert app._state["rag"]["document_filter"] == expected_filter
|
||||
|
|
@ -541,6 +546,44 @@ async def test_visual_grounding_uses_the_database_holding_the_citation(tmp_path)
|
|||
assert push.await_args.args[0].client is owner
|
||||
|
||||
|
||||
class TestDocumentSelectionIdentity:
|
||||
"""Two documents can share a title, within a corpus and across databases, so
|
||||
the selection is by id and the label says which database."""
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_a_repeated_title_selects_one_document(self, temp_db_path: Path):
|
||||
from textual.widgets import Checkbox
|
||||
|
||||
from haiku.rag.chat.widgets.document_filter_modal import DocumentFilterModal
|
||||
from haiku.rag.store.models.document import Document
|
||||
|
||||
client = AsyncMock()
|
||||
client._federated = {"arxiv": "a", "wiki": "b"}
|
||||
client.list_documents.return_value = [
|
||||
Document(id="id-one", content="", title="Capital region", source="arxiv"),
|
||||
Document(id="id-two", content="", title="Capital region", source="wiki"),
|
||||
]
|
||||
client.count_documents.return_value = 2
|
||||
|
||||
modal = DocumentFilterModal(client=client)
|
||||
app, _ = _make_app(temp_db_path, client)
|
||||
with patch("haiku.rag.chat.app.HaikuRAG", return_value=client):
|
||||
async with app.run_test() as pilot:
|
||||
await app.push_screen(modal)
|
||||
await pilot.pause()
|
||||
|
||||
boxes = list(modal.query(Checkbox))
|
||||
selected_ids = [getattr(box, "_doc_id", None) for box in boxes]
|
||||
assert selected_ids == ["id-one", "id-two"]
|
||||
labels = [str(b.label) for b in boxes]
|
||||
assert "Capital region (arxiv)" in labels
|
||||
assert "Capital region (wiki)" in labels
|
||||
|
||||
boxes[0].value = True
|
||||
await pilot.pause()
|
||||
assert modal._selected == {"id-one"}
|
||||
|
||||
|
||||
class TestDocumentSearchFilter:
|
||||
"""The filter modal shows one page and asks the database for the rest, so the
|
||||
typed term reaches SQL."""
|
||||
|
|
|
|||
|
|
@ -1,4 +1,8 @@
|
|||
from haiku.rag.tools.filters import _build_document_filter, build_multi_document_filter
|
||||
from haiku.rag.tools.filters import (
|
||||
_build_document_filter,
|
||||
build_document_id_filter,
|
||||
build_multi_document_filter,
|
||||
)
|
||||
|
||||
|
||||
def test_build_document_filter_simple():
|
||||
|
|
@ -45,3 +49,19 @@ def test_build_multi_document_filter_multiple():
|
|||
assert "doc1" in result
|
||||
assert "doc2" in result
|
||||
assert " OR (" in result
|
||||
|
||||
|
||||
def test_build_document_id_filter_empty():
|
||||
"""No selection means no filter."""
|
||||
assert build_document_id_filter([]) is None
|
||||
|
||||
|
||||
def test_build_document_id_filter_matches_exactly():
|
||||
"""An id filter never widens: names repeat, ids do not."""
|
||||
result = build_document_id_filter(["id-one", "id-two"])
|
||||
|
||||
assert result == "id IN ('id-one', 'id-two')"
|
||||
|
||||
|
||||
def test_build_document_id_filter_escapes_quotes():
|
||||
assert build_document_id_filter(["O'Reilly"]) == "id IN ('O''Reilly')"
|
||||
|
|
|
|||
Loading…
Reference in a new issue