[FEATURE] Stop a build when two different documents claim one source id - #559
noel-improv wants to merge 2 commits into
Conversation
…one source id MERGE on the source id attaches a second document to the first one's node and nothing reports it, so a collision reads back as one document holding both sets of chunks. Widening the id makes this rare; it does not make it detectable. A document is identified by the hash llama_index computes over its text and metadata, which every chunk already carries on its SOURCE relationship, so no node gains a field. The source write records that hash on the __Source__ node on create and keeps the first one on match. Before the build pipeline, a guard raises when one source id is seen with two hashes: inside one document (a storage prefix that merged two documents), across documents in one run, or against the hash recorded in the graph. The error names both documents. The graph lookup runs once per build batch of documents, bound as a parameter. Re-ingesting a document is unchanged. A source written before the hash was recorded carries none and is accepted; the next write records it, so a graph written at the legacy width reports its existing collisions the first time both documents are ingested again. Refs awslabs#533
The source write's coalesce on match and the guard's id lookup through node_id() need a store to prove them. Runs against Neo4j or Neptune Analytics and skips unless NEO4J_TEST_URI or NEPTUNE_GRAPH_TEST_ID is set. Each run uses its own tenant and deletes what it wrote.
| document_hash = hashes.pop() | ||
| metadata = sources[0][2] | ||
| earlier = self._seen.get(source_id) | ||
| if earlier and earlier[0] != document_hash: |
There was a problem hiding this comment.
The document hash discriminates more finely than the source id, so this raises on documents the id generator considers identical. TextNode.hash is sha256(str(text) + str(metadata)) — insertion-order sensitive — while create_source_id hashes _get_properties_str, which sorts. Re-ingesting one document with its metadata keys built in a different order ({'a':'1','b':'2'} vs {'b':'2','a':'1'}, same text) yields one source id and two hashes, and the build stops with "Two different documents share source id …". Same divergence hits _check_graph, so run 2 is rejected against run 1's graph. Derive the identity from the same normalized metadata string the source id uses rather than RelatedNodeInfo.hash
|
|
||
|
|
||
| def _name(metadata:Optional[Dict]) -> str: | ||
| return str((metadata or {}).get('file_path') or 'a document') |
There was a problem hiding this comment.
_name reads only file_path, so for a Document built in code, or a reader that uses url/file_name/s3_key, the error reads "a document (hash …) and a document (hash …)" — naming neither, which is the thing the PR is for. Fall back through a few metadata keys and include the ref node's doc_id
| # key of the same name can't override the merge key. | ||
| properties = {**clean_metadata, 'sourceId': source_id} | ||
| if document_hash: | ||
| properties[DOCUMENT_HASH_PROPERTY] = document_hash |
There was a problem hiding this comment.
A metadata key literally named documentHash defeats the first-writer-wins guarantee: on_match already holds source.\documentHash = params.documentHashfromclean_metadata, SET clauses run left to right, so by the time the appended coalesceevaluates,source.documentHashis already the incoming hash. ThesourceIdcase two lines below has a comment guarding exactly this; this one has no equivalent. ExcludedocumentHashfromclean_metadata, or put the coalesce first.
|
|
||
| def __init__(self, graph_store:GraphStore, tenant_id:TenantId, lookup_batch_size:Optional[int]=None): | ||
| self.graph_store = graph_store | ||
| self.tenant_id = tenant_id |
There was a problem hiding this comment.
tenant_id is stored and never read — the lookup is already tenant-scoped by MultiTenantGraphStore._rewrite_query matching the backticked Source label. SourceIdWidthGuard needs it for record_graph_source_id_width; this guard doesn't. Drop the parameter.
| hash already recorded on the graph's __Source__ node. A source written before | ||
| the hash was recorded carries none and is accepted; the build records it. | ||
|
|
||
| Graph lookups run once per ``lookup_batch_size`` documents, so documents are |
There was a problem hiding this comment.
"documents are yielded in groups of that size" only holds for a generator input. build() and extract_and_build() hand the width guard a list, which comes back a list, so call materialises everything and every graph lookup runs before the first document reaches the build pipeline — lookup_batch_size bounds the IN $sourceIds list, nothing else. Worth saying so in the docstring.
2 participants
Description
Stop a build when two different documents claim one source id. Widening the id in #541 makes a collision rare, but a graph still merges the second document into the first one's
__Source__node when one happens, and a graph written at the legacy width keeps the collisions it already has. This makes a collision an error that names both documents instead of a silent merge.A document is identified by the hash llama_index computes over its text and metadata. Every chunk already carries it on its SOURCE relationship, and it survives the S3 and file round trip, so no node gains a new field.
Changes
SourceNodeBuildercopies the document hash from the SOURCE relationship onto the source node.SourceGraphBuilderrecords it as__Source__.documentHashon create and keeps the first one on match (coalesce), so the first document to claim an id owns it.indexing/source_id_collision.py:SourceIdCollisionGuardraisesSourceIdCollisionErrorwhen one source id is seen with two hashes, whether inside one document (a storage prefix that merged two documents), across documents in one run, or against the hash already recorded in the graph.LexicalGraphIndex.build()andextract_and_build()run the guard after the width guard, before the build pipeline.Problem
MERGE on the source id attaches a second document to the first one's node, and the S3 prefix reads back as one document holding both sets of chunks. Nothing logs or raises, so a collision looks like a document with more chunks.
Related issue (if any): #533
Testing
pytest)Unit tests use the colliding pair from
test_source_id_collision.pyand cover: two documents on one id raise naming both, re-ingesting the same document passes, chunks in one merged prefix that disagree raise, a chunk with no hash is not checked, a different document already in the graph raises, a source written before the hash existed is accepted, the graph lookup is batched and binds ids as a parameter, the source write keeps the first hash, andbuild()refuses a colliding document. Full unit suite: 2,306 passed.Each detection branch and the
coalescewere reverted one at a time; each change failed a test.tests/integration/indexing/test_source_id_collision_live.pyruns against a real graph and skips unlessNEO4J_TEST_URIorNEPTUNE_GRAPH_TEST_IDis set. Both stores pass, which covers what a mocked store cannot: thecoalesceon match, and the guard's lookup throughnode_id(), which is a property on Neo4j and~idon Neptune.Each run uses its own tenant, so every node it writes carries a tenant-suffixed label and is deleted afterwards.
Checklist
Detection is on by default and stops the build. Two behaviour notes for the release:
Not in this PR
extract()on its own cannot detect a collision against the graph, since it does not write to one. It is caught when the extracted documents are built.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.