Repository navigation
Conversation
…AI#1648) Document.id is uuid5(source). PDF page/paragraph loaders and SQL rows reused one source, so delete-before-write kept only the last document. Namespace sources like the CSV/HF loaders, warn on duplicate ids in one ingest, and add regressions.
| # --------------------------------------------------------------------------- | ||
|
|
||
|
|
||
| async def test_ingest_keeps_all_pages_when_sources_are_unique(): |
There was a problem hiding this comment.
#1648's acceptance criteria ask for a real multi-page PDF going through RetrievalRuntime: every page kept, and all of them skipped on the second run. This test builds the Documents by hand with the #page= sources already set, so it would still pass if PyPDFLoader went back to the shared source. Could you add one that runs PyPDFLoader (with PdfReader patched the way test_pdf_loader.py does it) through the runtime? PyPDFOCRLoader's new source format doesn't have a test either, and the criteria cover the OCR loader too.
| source = str(row[source_col]) | ||
| row_key = str(row[source_col]) | ||
| elif row_index is not None: | ||
| row_key = str(row_index) |
There was a problem hiding this comment.
This fallback shares a namespace with real column values. If the id column is NULL for some rows, the row at index 1 gets documents#1, which is also the source of a row whose id is 1, so one overwrites the other. A distinct form like documents#row=1 would keep them apart.
| else: | ||
| source = self._table_or_query | ||
| row_key = "0" | ||
| source = f"{self._table_or_query}#{row_key}" |
There was a problem hiding this comment.
Question: for a raw query, _table_or_query is the whole SQL text, so every row's source includes the query. Editing the query (adding a column, reformatting it) gives every row a new id, and the old copies stay in the store until #1667 adds cleanup. #1648 did say to namespace by table/query, so this may be intended. Just checking that's what we want for raw queries.
| row_key = "0" | ||
| source = f"{self._table_or_query}#{row_key}" | ||
| if row_index is not None: | ||
| meta.setdefault("row_index", row_index) |
There was a problem hiding this comment.
Can we drop row_index from metadata? #1648 doesn't ask for it, and it's added even when the caller passes an explicit metadata_columns list, which is meant to be the full set (#1668 is also moving toward less metadata by default). Since astream always passes row_index, it could also become a required argument, which removes the row_key = "0" branch above. That branch would give every row table#0 if anything ever called this without an index.
| # stays in metadata["path"] for filtering/display. | ||
| source=f"{source}#page={page_number}", | ||
| metadata={ | ||
| "path": source, |
There was a problem hiding this comment.
path is set in the page and paragraph modes but not in the document mode (here or in PyPDFOCRLoader), so there's no single key that filters every chunk from one file. Could it go on all three, or none?
Also, the class docstrings for PyPDFLoader and PyPDFOCRLoader list the metadata keys and don't mention path. And the LangChainLoaderAdapter docstring still says an explicit source= is used as-is, but it now gets #page=N appended. The docs pages can wait for #1664, but the docstrings in these files should match the code.
| source = str(raw_source) if raw_source is not None else None | ||
| # LangChain PDF loaders reuse the file path as source for every page. | ||
| # Append page (when present) so Document.id stays unique per page. | ||
| page = metadata.get("page") |
There was a problem hiding this comment.
Not a blocker for this PR, but page is only one of the keys LangChain uses this way. Its CSV and JSON loaders and text-splitter output use row, seq_num and start_index and still reuse one source, so they collapse to one document the same way. #1648 only asked for the page case, so a follow-up issue seems right. A general fix might be to add a counter suffix whenever a source repeats within one stream.
| ]: | ||
| seen_document_ids: set[UUID] = set() | ||
| async for doc in loader.astream(): | ||
| if doc.id in seen_document_ids: |
There was a problem hiding this comment.
This warns once for every repeated document, so a loader that reuses one source for 10k documents logs 10k warnings. Warning once per duplicated id, or one summary line with a count at the end of the run, would give the same signal.
|
|
||
| async def test_ingest_shared_source_still_overwrites_and_warns(caplog): | ||
| """Same source → same Document.id → delete-before-write keeps only the last page.""" | ||
| import logging |
There was a problem hiding this comment.
Nit: logging and IdentityChunker can be imported at the top of the file with the others. There's no optional dependency to defer here.
|
@ningyixi Thanks for your contribution! This PR has had open change requests for over a week without an update. Are you still planning to continue? If we don't hear back in a few days, we'll take it over (keeping your commits and credit) so #1648 can land. Some context on why it matters now: #1691 (PDFs in S3/GCS/Azure) depends on this PR. It moves Looking forward to it! ٩(◕‿◕)۶ |
Summary
Fixes #1648.
Document.idisuuid5(NAMESPACE_URL, source). PDF page/paragraph (and OCR page) loaders setsourceto the plain file path for every page, and SQL rows without a distinct namespaced key reused the table name (or bare id).RetrievalRuntimedeletes bydocument_idbefore writing, so a multi-page PDF embedded all pages but the store kept only the last one. Re-ingest also never skipped pages.This matches the existing CSV / HuggingFace pattern: give each emitted document a unique stable
source.Changes
path#page=N/path#page=N¶graph=M; plain path inmetadata["path"]path#page=Nin page mode#page=when metadata haspagetable_or_query#row_key(id/source column, else row index)Document.idTest plan
source="report.pdf"→ store count 1; unique#page=sources → store count 3 and re-run skips 3uv run pytest packages/railtracks/tests/unit_tests/retrieval/loaders/test_pdf_loader.py packages/railtracks/tests/unit_tests/retrieval/loaders/test_langchain_loader.py packages/railtracks/tests/unit_tests/retrieval/loaders/cloud/test_sql.py packages/railtracks/tests/unit_tests/retrieval/test_runtime.py— 98 passed