Repository navigation
Conversation
Sequence numbers used to restart whenever a page number reappeared later in the element list, so two elements on the same page that share their text received the same id and their parent_id mappings were merged silently. Count per page number instead, matching the sibling _assign_hash_ids(). Signed-off-by: Eapp1e <163811520+Eapp1e@users.noreply.github.com>
cragwolfe
left a comment
There was a problem hiding this comment.
Reviewed 9ad24de65b7340776dd31c3f7a8ccd19b8f680d8 against 9f600504519ac7ab80a49169b4b6487a7c79efc8 with GPT Pro. The per-page counter correction is technically clean: no introduced P1/P2 defect was verified. Public identity compatibility still needs maintainer disposition, so this is a COMMENT rather than approval.
The counter now retains each page’s ordinal across intervening pages, matching _assign_hash_ids(). With distinct incoming IDs and noncolliding outputs, the existing complete old-to-new mapping preserves parent references, including forward references, while unknown parents remain unchanged. Repeating the call with unchanged inputs/order is deterministic. Inputs where each page occupies one contiguous block retain their IDs; ascending page order is sufficient but not necessary.
HUMAN REVIEW REQUIRED — accept the public ID change and choose release/migration treatment. Every later run of a revisited page gets offset ordinals, including elements with distinct text whose previous IDs never collided. Element.to_dict() exposes the changed element_id; corresponding parent_id values can change too. A destination keyed by those IDs may see new records after reprocessing, and references persisted elsewhere are not remapped by this list-local function. The affected public paths include direct/decorated hashing and JSON rehydration with chunking and hash IDs enabled. Unchunked JSON rehydration preserves stored IDs. Ordinary all-missing-page input remains unchanged, but revisited None runs can change.
Please explicitly decide whether to accept this scope and what release communication, reindexing guidance or identity-version treatment it requires. The source does not establish a mandatory particular version bump. Correcting the counter does not itself settle that policy decision.
Important existing limits remain outside this correction:
- Already duplicated incoming IDs still make the old-to-new map ambiguous; its last mapping wins. This cannot recover parent identity already lost in stored or independently hashed fragments.
- Counters are page-keyed, not document-isolated. Combined-document processing does not guarantee composition-independent IDs.
- The unchanged undelimited hash input has structural ambiguities, for example page
1/ordinal10versus page11/ordinal0with the same filename/text. This is not a new counter defect or a cryptographic hash collision. - Repeated references to the same object still share the final mutated ID.
These limits prevent a general claim of collision-free identity or recovery of arbitrary combined parent graphs. No unrelated source repair is requested as a condition of this narrow technical result. Useful regression coverage would assert exact noncontiguous per-page ordinals, distinct parent/child targets with forward/unknown references, second-pass stability, and the JSON rehashing path; the new uniqueness-only test does not establish those properties.
The complete actual-base diff, final element definitions, metadata helper, JSON caller/rehydration path, serializers and relevant tests were reviewed without truncation. uv.lock and CHANGELOG.md were excluded; release files match current main. No local tests or reproductions ran; author counts remain unverified. All 50 reported exact-head checks succeeded at observation time. Technical source correctness, compatibility disposition and integration readiness remain separate.
(authored by codex)
|
On the identity-compatibility note: the change only affects the ordinal each element receives on its page, and the mapping is still computed inside a single call from the same |
Summary
Fixes #4620
assign_and_map_hash_ids()counts the "sequence number on page" per contiguous run of equal page numbers, so an element's id depends on which elements precede it. When the elements of two partitions of the same document are combined, page numbers can come back later in the list, and two distinct elements on the same page that share their text then receive the same id —old_to_new_mappingsilently merges theirparent_ids.This counts per page number instead, matching the comment on the block,
Element.id_to_hash()("sequence number on page"), and the sibling_assign_hash_ids()inunstructured/partition/common/metadata.py.Testing
3elements,2unique ids (both "Section" elements hash alike)3unique idstest_unstructured/documents/test_elements.pywith the new test only: 3 failed / 79 passed. With the change: 2 failed / 80 passed. The two remaining failures are pre-existing fixture gaps in this checkout (example-docs/test_evaluate_files/unstructured_output/form.jsonis absent) and fail identically without the change.Ids for ascending page numbers are unchanged —
test_hash_ids_are_deterministic(which pins exact ids) and the other hash-id tests still pass.Notes
groupbyis no longer used anywhere in the module, so the import goes away with it.