docs: structured data ingestion design proposal - #290
Conversation
Design for ingesting CSV/JSON/XLSX/Parquet, tables lifted out of PDFs, and existing graphs into the same graph the unstructured path builds, without an LLM call per row. Three load-bearing ideas: - Reduce every structured source to a stream of flat records, so a PDF table is a record stream carrying the PDF's DocumentInfo and an existing graph is a node stream plus an edge stream. - Make the mapping an ontology fragment, so it validates against an existing ontology or bootstraps one when there is none. - Declare entity identity once on the ontology entity type rather than per source, so differently-shaped sources converge on the same nodes at write time instead of via a fuzzy merge pass. Records are persisted as Chunk nodes in the normal lexical graph, which is what lets update()/delete_document() orphan cleanup and all four retrieval paths work on structured data unchanged. Includes a design review against the storage layer that pins three required write properties (name, fact, source_chunk_ids), flags that record chunk uids must key on the run's effective document uid or the update() cutover deletes the chunks it is about to promote, and records the get_document_entity_candidates() scaling ceiling for large sources. Refs: FalkorDB/research#82, FalkorDB/research#65 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThe PR adds a structured-ingestion design proposal and five spike validations. It defines record loading, mappings, ontology and identity handling, record chunks, shared pipeline stages, ingestion routing, supported formats, examples, open questions, and implementation phases. Documentation navigation is updated. ChangesStructured ingestion design
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a new design proposal documenting a planned structured-data ingestion path, and wires it into the published docs navigation so readers can discover it from the existing ingestion documentation.
Changes:
- Add a new structured ingestion design proposal page under
docs/design/. - Add a pointer from the existing ingestion docs to the structured-ingestion proposal.
- Add a new “Design Proposals” section to the MkDocs nav.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mkdocs.yml |
Adds a “Design Proposals” nav section pointing to the new proposal page. |
docs/ingestion.md |
Adds an info callout linking readers to the structured-ingestion proposal. |
docs/design/structured-ingestion.md |
Introduces the structured data ingestion design proposal document. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| !!! info "Structured sources (CSV, JSON, tables, existing graphs)" | ||
| This page describes the **unstructured** path — prose in, LLM extraction, graph out. | ||
| Structured inputs skip LLM extraction entirely and are covered by a separate proposal: | ||
| [Design: Structured Data Ingestion](design/structured-ingestion.md). |
|
|
||
| ```python | ||
| # One row = one entity. | ||
| await rag.ingest("orgs.csv", mapping=Table(node="Organization", key="org_name")) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
docs/design/structured-ingestion.md (1)
116-123: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftDefine whether record streaming is synchronous or asynchronous.
RecordLoaderStrategy.load_recordsis asynchronous, butRecordBatch.recordsis a synchronousIterable. A blocking reader can block the event loop while the pipeline consumes it, and the contract does not express backpressure or cancellation. UseAsyncIterable, or specify that synchronous readers run in a worker with bounded batch handoff. The existing async ingestion path ingraphrag_sdk/src/graphrag_sdk/api/main.py:1469-1566makes this contract important.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/structured-ingestion.md` around lines 116 - 123, Clarify the streaming contract between RecordLoaderStrategy.load_records and RecordBatch.records: change records to AsyncIterable with async consumption, or explicitly require synchronous readers to run in a worker with bounded batch handoff. Document the chosen backpressure and cancellation behavior so the async ingestion path does not block the event loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/structured-ingestion.md`:
- Around line 403-406: Define a deterministic ordering for the last_write_wins
option in the structured ingestion conflict policy. Update the
Organization.employee_count policy to use a stable source version and
tie-breaker, or explicitly constrain it to serialized writes and document that
limitation; ensure the winning source recorded in sources is determined by the
same ordering.
- Line 15: Add language identifiers to the four fenced code blocks in
structured-ingestion.md at the referenced locations, using text, mermaid, or the
most appropriate language for each block so all fences satisfy markdownlint
MD040.
- Around line 219-220: Expand the Record key section for NodeMapping.key to
define validation for missing, null, and duplicate keys, including the chosen
behavior and its effect on the one-Chunk-per-record contract. Specify
deterministic handling that remains safe for streamed batches and partial
writes, and state whether invalid sources are rejected, duplicates merged, or a
tie-breaker applied.
- Around line 300-301: Update the structured update no-op semantics in the
content-hash short-circuit so unchanged source bytes do not alone qualify as a
no-op; include canonical mapping, ontology identity, type-policy, and
conflict-policy fingerprints in the key, or explicitly require forced
re-ingestion when any changes. Ensure mapping changes for identical source files
reach the update path and refresh graph data.
- Line 362: Update the in-memory rag.ingest example to require a stable document
identity, such as source, document_id, or explicit DocumentInfo, before records
and mapping are processed. Document how that identity is resolved and bound to
RecordBatch, and define its lifecycle plus idempotency semantics so
deterministic Chunk UIDs, provenance, updates, and deletes remain supported.
- Around line 458-460: Update the structured-ingestion example so the
Organization foreign-key reference uses an explicitly resolvable identity:
either declare org_id as the Organization identity and align the orgs.csv
mapping, or define the target-key lookup/alias contract needed to resolve org_id
to the existing name identity. Ensure the example’s identity algorithm and
single-node claim remain consistent.
- Around line 267-269: Update the structured-ingestion design around the Chunk
record representation and its zero-loss acceptance criteria: either define
storage of a canonical raw record payload with explicit size and privacy limits,
including how nested values, arrays, scalar formatting, and omitted values are
preserved, or narrow the “Zero-Loss Data” claim and corresponding acceptance
test to match the human-readable rendering and default skip_value behavior.
- Around line 270-271: Update the deterministic chunk UID specification to hash
a canonical, unambiguous representation of the effective document UID and
record_key, such as a versioned length-prefixed or canonical tuple format,
instead of raw concatenation. Preserve sha256 and ensure distinct field
boundaries cannot produce the same Chunk UID.
- Line 346: Align the documentation and implementation for Entity.name fallback
behavior: update the contract around backfill_entity_embeddings and the node
field table to state that missing names use e.id, or remove that fallback from
backfill_entity_embeddings so the documented unreachable-node behavior remains
accurate.
---
Nitpick comments:
In `@docs/design/structured-ingestion.md`:
- Around line 116-123: Clarify the streaming contract between
RecordLoaderStrategy.load_records and RecordBatch.records: change records to
AsyncIterable with async consumption, or explicitly require synchronous readers
to run in a worker with bounded batch handoff. Document the chosen backpressure
and cancellation behavior so the async ingestion path does not block the event
loop.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 754fda3e-636c-4fca-8df0-38063b69e250
📒 Files selected for processing (3)
docs/design/structured-ingestion.mddocs/ingestion.mdmkdocs.yml
|
|
||
| `rag.ingest(...)` assumes one shape of input: a blob of prose. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to fenced code blocks.
The four fences at Lines 15, 240, 440, and 471 omit language identifiers and trigger markdownlint MD040. Use text, mermaid, or the appropriate language identifier.
Also applies to: 240-240, 440-440, 471-471
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` at line 15, Add language identifiers to
the four fenced code blocks in structured-ingestion.md at the referenced
locations, using text, mermaid, or the most appropriate language for each block
so all fences satisfy markdownlint MD040.
Source: Linters/SAST tools
| - **Record key** (`NodeMapping.key`) — what makes *re-ingesting this source* idempotent. | ||
| Source-local. Governs the record's chunk id and is stored as an indexed property. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define and validate record-key uniqueness.
NodeMapping.key controls chunk identity and re-ingestion idempotency, but the proposal does not define missing, null, or duplicate key behavior. Duplicate keys can make multiple records share one Chunk and violate the “one Chunk per record” contract. Define whether the source is rejected, duplicates are merged, or a deterministic tie-breaker is used. Make the behavior safe for streamed batches and partial writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` around lines 219 - 220, Expand the
Record key section for NodeMapping.key to define validation for missing, null,
and duplicate keys, including the chosen behavior and its effect on the
one-Chunk-per-record contract. Specify deterministic handling that remains safe
for streamed batches and partial writes, and state whether invalid sources are
rejected, duplicates merged, or a tie-breaker applied.
| - one `Chunk` node per record, with `kind="record"`, the record key as a property, and text that | ||
| is a human-readable rendering of the record | ||
| (`"Alice Smith · age 34 · Engineer at Acme Corp"`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the “Zero-Loss Data” claim reversible.
A human-readable rendering is not sufficient to reconstruct nested objects, arrays, scalar formatting, or omitted values. The default on_type_error="skip_value" in Line 351 can also discard data. Store a canonical raw record payload on the Chunk, with explicit size and privacy limits, or narrow the zero-loss claim and acceptance test.
Also applies to: 295-295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` around lines 267 - 269, Update the
structured-ingestion design around the Chunk record representation and its
zero-loss acceptance criteria: either define storage of a canonical raw record
payload with explicit size and privacy limits, including how nested values,
arrays, scalar formatting, and omitted values are preserved, or narrow the
“Zero-Loss Data” claim and corresponding acceptance test to match the
human-readable rendering and default skip_value behavior.
| - a **deterministic** chunk uid — `sha256(<effective document uid> + record_key)` instead of | ||
| today's `uuid4()`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching structured-ingestion.md:"
fd -a 'structured-ingestion\.md$' . || true
file="$(fd 'structured-ingestion\.md$' . | head -n1)"
if [ -n "${file:-}" ]; then
echo "Using: $file"
wc -l "$file"
echo "--- lines 240-290 ---"
sed -n '240,290p' "$file" | cat -n
else
echo "File not found"
fi
echo "--- Search for chunk uid/chunk_uid/record_key/effective document uid in docs code ---"
rg -n "chunk uid|chunk_uid|Chunk UID|effective document uid|record_key|uuid4|sha256" .Repository: FalkorDB/GraphRAG-SDK
Length of output: 7942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- docs/design/structured-ingestion.md lines 288-304 ---"
sed -n '288,304p' docs/design/structured-ingestion.md | cat -n
echo "--- implementation references to effective document uid / chunk keys / structured records ---"
rg -n "effective document|document uid|chunk uid|chunk_uid|record key|record_key|record chunks|structured\.records|structured_ingestion|StructuredIngestion" .
echo "--- deterministic concat collisions (sha256 UTF-8 bytes) ---"
python3 - <<'PY'
import hashlib
examples = [
("ab", "c"),
("a", "bc"),
("", "ab"),
("a", "b"),
]
for doc_uid, record_key in examples:
data = doc_uid + record_key
h = hashlib.sha256(data.encode("utf-8")).hexdigest()
print(f"{repr(doc_uid)!r} + {repr(record_key)!r} = {repr(data)!r} -> {h}")
PYRepository: FalkorDB/GraphRAG-SDK
Length of output: 5621
Hash canonical fields instead of raw concatenation.
sha256(<effective document uid> + record_key) is ambiguous: (uid="ab", key="c") and (uid="a", key="bc") produce the same hash. Use a versioned, length-prefixed, or canonical tuple format before hashing so distinct records cannot share a Chunk UID.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` around lines 270 - 271, Update the
deterministic chunk UID specification to hash a canonical, unambiguous
representation of the effective document UID and record_key, such as a versioned
length-prefixed or canonical tuple format, instead of raw concatenation.
Preserve sha256 and ensure distinct field boundaries cannot produce the same
Chunk UID.
| semantic no-op. Under `update()` the no-op guarantee comes from the existing **content-hash | ||
| short-circuit** instead — an unchanged file never reaches the pending-cutover path at all. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Include mapping semantics in the structured update no-op key.
The proposal skips the pending update path when the source content hash is unchanged. Structured output also depends on mapping, ontology identity definitions, type policies, and conflict policies. Changing a mapping while keeping employees.csv byte-identical would leave stale graph data. Include a canonical mapping and ontology fingerprint, or explicitly require a forced re-ingest when these inputs change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` around lines 300 - 301, Update the
structured update no-op semantics in the content-hash short-circuit so unchanged
source bytes do not alone qualify as a no-op; include canonical mapping,
ontology identity, type-policy, and conflict-policy fingerprints in the key, or
explicitly require forced re-ingestion when any changes. Ensure mapping changes
for identical source files reach the update path and refresh graph data.
|
|
||
| | Property | On | Consequence if omitted | | ||
| | --- | --- | --- | | ||
| | `name` | node | `backfill_entity_embeddings` falls back to the raw id → entity vector search degrades | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^docs/design/structured-ingestion\.md$|backfill_entity_embeddings|Entity|entity_embeddings)' || true
echo "== structured ingestion relevant sections =="
if [ -f docs/design/structured-ingestion.md ]; then
sed -n '1,90p' docs/design/structured-ingestion.md | cat -n
echo "..."
sed -n '320,360p' docs/design/structured-ingestion.md | cat -n
fi
echo "== search for symbols =="
rg -n "backfill_entity_embeddings|Entity\.name|entity_embeddings|unreachable by vector search|raw id" . --glob '!**/.git/**' || trueRepository: FalkorDB/GraphRAG-SDK
Length of output: 12284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== vector_store outline =="
ast-grep outline graphrag_sdk/src/graphrag_sdk/storage/vector_store.py --view compact || true
echo "== backfill_entity_embeddings implementation =="
sed -n '620,690p' graphrag_sdk/src/graphrag_sdk/storage/vector_store.py | cat -n
echo "== entity embedding related implementations =="
rg -n "backfill_entity_embeddings|embed_entity|embeddings|Entity|e\.name|id|unwind|MATCH.*Entity" graphrag_sdk/src/graphrag_sdk/storage/graphrag_sdk/src/graphrag_sdk/storage/vector_store.py graphrag_sdk/src/graphrag_sdk/storage -g '*.py' || true
echo "== relevant tests =="
sed -n '220,275p' graphrag_sdk/tests/test_vector_store.py | cat -nRepository: FalkorDB/GraphRAG-SDK
Length of output: 50377
Align the Entity.name fallback behavior.
backfill_entity_embeddings actually uses e.name when present and falls back to e.id when absent. Lines 60-63 currently state that nodes without name are unreachable by vector search; either change that contract or remove the fallback to match the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` at line 346, Align the documentation and
implementation for Entity.name fallback behavior: update the contract around
backfill_entity_embeddings and the node field table to state that missing names
use e.id, or remove that fallback from backfill_entity_embeddings so the
documented unreachable-node behavior remains accurate.
| ```python | ||
| await rag.ingest("report.pdf") # unstructured — unchanged | ||
| await rag.ingest("employees.csv", mapping=mapping) # structured | ||
| await rag.ingest(records=[{...}], mapping=mapping) # in-memory |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Require a stable document identity for in-memory records.
RecordBatch requires document_info, and deterministic Chunk UIDs use the effective DocumentInfo.uid. This entry point supplies only records and mapping. Without source, document_id, or explicit DocumentInfo, the pipeline cannot provide deterministic IDs, provenance, update, or delete behavior. Add a required identity parameter and define its lifecycle and idempotency semantics. The current ingestion path in graphrag_sdk/src/graphrag_sdk/api/main.py:1469-1566 resolves and binds this identity before running the pipeline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` at line 362, Update the in-memory
rag.ingest example to require a stable document identity, such as source,
document_id, or explicit DocumentInfo, before records and mapping are processed.
Document how that identity is resolved and bound to RecordBatch, and define its
lifecycle plus idempotency semantics so deterministic Chunk UIDs, provenance,
updates, and deletes remain supported.
| Two sources will disagree about `Organization.employee_count`. POC policy: | ||
| `on_conflict="last_write_wins" | "keep_existing" | "record_both"`, defaulting to last-write-wins | ||
| with the winning source recorded in a `sources: LIST` property. Full per-property provenance is | ||
| deferred — it doubles write cost and the POC does not need it. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define a deterministic order for last_write_wins.
last_write_wins depends on ingestion order. Mixed batches, retries, or concurrent sources can produce different final properties and different sources contents for the same entity. Use a stable precedence key, such as source version plus a tie-breaker, or scope this policy to serialized writes and document that limitation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/structured-ingestion.md` around lines 403 - 406, Define a
deterministic ordering for the last_write_wins option in the structured
ingestion conflict policy. Update the Organization.employee_count policy to use
a stable source version and tie-breaker, or explicitly constrain it to
serialized writes and document that limitation; ensure the winning source
recorded in sources is determined by the same ordering.
… design Five throwaway spikes under poc/structured-ingestion/, each answering one open question from docs/design/structured-ingestion.md against the real GraphStore, IngestionPipeline and a live FalkorDB. No LLM, no API keys. run_all.py passes. Four of the five falsified something in the design: - s1 RecordBatch: pydantic keeps Iterable[dict] lazy but one-shot, and the pipeline iterates records twice (step 3 chunks, step 4 mapping). Measured "step 3 saw 10 records, step 4 saw 0" with no error -> a silent zero-row ingest. RecordBatch becomes a stream factory. - s2 mapping DSL: label-addressed edges cannot express a record holding two nodes of the same label; transactions.csv produced a silent self-loop. Nodes now carry an alias. Adds two to_ontology() guards (reserved attribute names, reference-only labels). - s3 identity: identity=["name"] was the design's default and loses. A normalised FK carries the target's key and not its name, so the mapping cannot compute the identity it points at: 2 Acme nodes, 0 people reachable. Key + alias_ids is the only policy that yields one connected graph, making proposal #4 critical-path rather than optional. - s4 record-as-chunk: confirms the predicted update() data-loss trap empirically. Canonical-keyed chunk uids go 3 chunks -> 0 through rollforward_cutover() with no exception; effective-uid keying survives. The three cleanup primitives behave exactly as claimed. - s5 pipeline seam: the steps are reusable verbatim, but IngestionPipeline's __init__ demands a chunker and an LLM extractor the structured path lacks. Share a LexicalGraphWriter base instead of subclassing, and suppress NEXT_CHUNK for record chunks. poc/ is outside the wheel, pytest testpaths and CI (which runs with working-directory: graphrag_sdk and lints only src/), so it ships nothing. No src changes. mkdocs build --strict is unchanged (one pre-existing warning). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Spike round: five POCs, four corrections to the designFollowing the suggestion to POC each proposal before building, I added
Four of the five falsified something in the design. One inverted a headline decision.
The one that matters most:
|
| Policy | Acme nodes | #82 traversal |
|---|---|---|
| name-first (the design's default) | 2 | 0 people reachable |
| key-only | 2 | 0 people reachable |
key + alias_ids |
1 | 2 people reachable |
employees.csv is an ordinary normalised table: it references its org by org_id=ORG-42 and has no org_name column. Under name-first identity the mapping cannot compute the identity of the entity it points at, so #2's rule "every mapping supplies the type's identity attributes" is unsatisfiable for any foreign key — the most common structured shape there is. The failure is silent: a stub node collects all the WORKS_AT edges while the real Acme node holds the prose.
So structured writes are now key-identified, and AliasMatchResolution (#4) moves from optional nicety to critical path. Measured caveat: with no source carrying key and name together the bridge has nothing to build from — ingest order doesn't matter, but presence does, and unbridged stubs should be reported rather than left silent (new §8.9).
The predicted trap is real
The review predicted from reading code that deterministic chunk uids keyed on the canonical document id would destroy data. Run against FalkorDB through the real GraphStore:
| chunk uid keyed on | before update() |
shared with pending | after cutover |
|---|---|---|---|
| canonical doc id | 3 | 3 | 0 |
| effective (pending) uid | 3 | 0 | 3 |
The pending run MERGEs onto the live document's chunks, rollforward_cutover() deletes them, and an empty document is promoted — no exception. Today's uuid4() uids are accidentally immune, which is precisely why making them deterministic is the risky part of #5. Everything else in #5 held: get_document_entity_candidates(), delete_stale_relationships() and delete_orphan_entities() all behaved exactly as designed over record chunks.
What didn't change
Record-as-chunk, RELATES + rel_type, mapping-as-ontology-fragment, deterministic no-LLM mapping, and "no retrieval strategy is touched" all survived contact with the database. Every correction is to a signature or a default, not to the shape of the design — which is the outcome a spike round should produce.
Details: FINDINGS.md and each spike's NOTES.md. Design doc updated throughout, with a new §10 summarising the results.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def note(self, text: str) -> None: | ||
| self.lines.append(f" {text}") | ||
| print(f" {text}") |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
|
||
| # Q3 — the dangerous one. Does incidental inspection eat the stream? | ||
| for label, poke in ( | ||
| ("repr()", lambda b: repr(b)), |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@poc/structured-ingestion/FINDINGS.md`:
- Around line 25-27: Add the appropriate language identifiers to each Markdown
output fence: mark the output blocks as text in
poc/structured-ingestion/FINDINGS.md (25-27),
poc/structured-ingestion/s1_record_stream/NOTES.md (27-29), and
poc/structured-ingestion/s2_mapping_dsl/NOTES.md (22-25); mark the CSV example
as csv in poc/structured-ingestion/s3_identity/NOTES.md (21-24).
In `@poc/structured-ingestion/s1_record_stream/spike.py`:
- Around line 67-75: Update the construction validation around
consumed_at_construction so it requires all 10 source records to remain after
model initialization, rather than accepting zero; keep the existing lazy
records-field check and diagnostic messages aligned with this stricter outcome.
In `@poc/structured-ingestion/s2_mapping_dsl/spike.py`:
- Around line 141-151: Update the entity identity construction in the generated
ontology to always use each node’s structured key rather than selecting “name”
when NodeB.name is set. Adjust the surrounding identity mapping and spike
assertions to verify the emitted Entity.identity value follows the key-based
contract.
In `@poc/structured-ingestion/s3_identity/spike.py`:
- Around line 84-86: Update id_name_first so missing name_value is rejected as
an invalid name-first identity instead of falling back to key_value; propagate
or record this as a validation failure so P1 measures only the declared
name-first policy.
- Around line 171-180: Update the alias-merge queries in the duplicate
relationship cleanup flow to preserve existing RELATES provenance when MERGE
matches an existing edge. Union r.source_chunk_ids with n.source_chunk_ids
without duplicates, and define an explicit policy for conflicting fact and other
relationship properties instead of overwriting them; apply the same behavior to
both relationship directions.
In `@poc/structured-ingestion/s4_record_as_chunk/spike.py`:
- Around line 142-145: Update the post-cutover verification around
rollforward_cutover and count_chunks to query the resulting chunk IDs, compare
them with the expected v2 IDs derived from pending_id, and assert the edited
“Staff Engineer” record is present. Apply the same validation to the
corresponding verification block around lines 203–211 instead of relying only on
chunk counts.
In `@poc/structured-ingestion/s5_pipeline_seam/NOTES.md`:
- Around line 10-13: Add the text language identifier to both fenced code blocks
in NOTES.md, including the block containing _build_lexical_graph, so each
opening fence uses ```text and MD040 warnings are resolved.
In `@poc/structured-ingestion/s5_pipeline_seam/spike.py`:
- Around line 176-215: Use the same effective DocumentInfo.uid for both pipeline
runs before asserting identical graphs, since records_to_chunks() embeds it in
chunk IDs. Update the A/B comparison to validate nodes, edges, and properties
rather than only counts, or revise the assertion wording to check structural
equivalence if distinct UIDs must remain.
- Around line 177-206: Initialize mentions_a before the subclass pipeline try
block with a failure-safe value, and guard the factoring-B comparison in the
MixinStructuredPipeline section so it only compares counts when A completed
successfully. Keep B’s independent success assertion active regardless of A’s
outcome, allowing Report.verdict() to run and report recorded failures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 698ac2c9-f32b-4744-be3f-c41199b34c2f
⛔ Files ignored due to path filters (3)
poc/structured-ingestion/_harness/fixtures/employees.csvis excluded by!**/*.csvpoc/structured-ingestion/_harness/fixtures/orgs.csvis excluded by!**/*.csvpoc/structured-ingestion/_harness/fixtures/transactions.csvis excluded by!**/*.csv
📒 Files selected for processing (18)
docs/design/structured-ingestion.mdpoc/structured-ingestion/FINDINGS.mdpoc/structured-ingestion/README.mdpoc/structured-ingestion/_harness/__init__.pypoc/structured-ingestion/_harness/env.pypoc/structured-ingestion/_harness/fixtures/acme_report.txtpoc/structured-ingestion/_harness/fixtures/catalog.jsonpoc/structured-ingestion/run_all.pypoc/structured-ingestion/s1_record_stream/NOTES.mdpoc/structured-ingestion/s1_record_stream/spike.pypoc/structured-ingestion/s2_mapping_dsl/NOTES.mdpoc/structured-ingestion/s2_mapping_dsl/spike.pypoc/structured-ingestion/s3_identity/NOTES.mdpoc/structured-ingestion/s3_identity/spike.pypoc/structured-ingestion/s4_record_as_chunk/NOTES.mdpoc/structured-ingestion/s4_record_as_chunk/spike.pypoc/structured-ingestion/s5_pipeline_seam/NOTES.mdpoc/structured-ingestion/s5_pipeline_seam/spike.py
| ``` | ||
| step 3 saw 10 records, step 4 saw 0 — no error raised | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to output fences.
poc/structured-ingestion/FINDINGS.md#L25-L27: Mark the output block astext.poc/structured-ingestion/s1_record_stream/NOTES.md#L27-L29: Mark the output block astext.poc/structured-ingestion/s2_mapping_dsl/NOTES.md#L22-L25: Mark the output block astext.poc/structured-ingestion/s3_identity/NOTES.md#L21-L24: Mark the CSV example ascsv.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
📍 Affects 4 files
poc/structured-ingestion/FINDINGS.md#L25-L27(this comment)poc/structured-ingestion/s1_record_stream/NOTES.md#L27-L29poc/structured-ingestion/s2_mapping_dsl/NOTES.md#L22-L25poc/structured-ingestion/s3_identity/NOTES.md#L21-L24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/FINDINGS.md` around lines 25 - 27, Add the
appropriate language identifiers to each Markdown output fence: mark the output
blocks as text in poc/structured-ingestion/FINDINGS.md (25-27),
poc/structured-ingestion/s1_record_stream/NOTES.md (27-29), and
poc/structured-ingestion/s2_mapping_dsl/NOTES.md (22-25); mark the CSV example
as csv in poc/structured-ingestion/s3_identity/NOTES.md (21-24).
Source: Linters/SAST tools
| consumed_at_construction = sum(1 for _ in gen) | ||
| r.check( | ||
| consumed_at_construction in (0, 10), | ||
| "construction does not silently drop records", | ||
| f"generator still yields {consumed_at_construction} after model init", | ||
| ) | ||
| r.note(f"field type after validation: {type(batch.records).__name__}") | ||
| lazy = consumed_at_construction == 10 or type(batch.records).__name__ != "list" | ||
| r.check(lazy, "records field stays lazy (not materialised to a list)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject a consumed source generator.
The check accepts consumed_at_construction == 0. That result means construction drained gen. The spike can then report lazy behavior for an eagerly consumed source.
Require all ten source records to remain after construction.
Proposed fix
- consumed_at_construction in (0, 10),
+ consumed_at_construction == 10,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| consumed_at_construction = sum(1 for _ in gen) | |
| r.check( | |
| consumed_at_construction in (0, 10), | |
| "construction does not silently drop records", | |
| f"generator still yields {consumed_at_construction} after model init", | |
| ) | |
| r.note(f"field type after validation: {type(batch.records).__name__}") | |
| lazy = consumed_at_construction == 10 or type(batch.records).__name__ != "list" | |
| r.check(lazy, "records field stays lazy (not materialised to a list)") | |
| consumed_at_construction = sum(1 for _ in gen) | |
| r.check( | |
| consumed_at_construction == 10, | |
| "construction does not silently drop records", | |
| f"generator still yields {consumed_at_construction} after model init", | |
| ) | |
| r.note(f"field type after validation: {type(batch.records).__name__}") | |
| lazy = consumed_at_construction == 10 or type(batch.records).__name__ != "list" | |
| r.check(lazy, "records field stays lazy (not materialised to a list)") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s1_record_stream/spike.py` around lines 67 - 75,
Update the construction validation around consumed_at_construction so it
requires all 10 source records to remain after model initialization, rather than
accepting zero; keep the existing lazy records-field check and diagnostic
messages aligned with this stricter outcome.
| props = label_props.setdefault(n.label, {}) | ||
| props.update(dict.fromkeys(n.properties, "STRING")) | ||
| if n.name: | ||
| props["name"] = "STRING" | ||
| identity[n.label] = ["name"] if n.name else [n.key] | ||
| entities = [ | ||
| Entity( | ||
| label=lbl, | ||
| properties=[Attribute(name=p, type=t) for p, t in props.items()], | ||
| identity=identity[lbl], # proposal #3 — does extra="allow" carry it? | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use key-based structured identity in the generated ontology.
When NodeB.name is set, this code emits identity=["name"]. That recreates the name-first policy rejected by s3. The current checks only verify serialization, so they do not detect the wrong identity contract.
Use the structured key identity here and assert the emitted value in the spike.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s2_mapping_dsl/spike.py` around lines 141 - 151,
Update the entity identity construction in the generated ontology to always use
each node’s structured key rather than selecting “name” when NodeB.name is set.
Adjust the surrounding identity mapping and spike assertions to verify the
emitted Entity.identity value follows the key-based contract.
| def id_name_first(label: str, key_value: str, name_value: str | None) -> str: | ||
| # identity=["name"]; falls back to the key when the record has no name column. | ||
| return compute_entity_id(name_value or key_value, label) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not model name-first identity with a key fallback.
A name-first mapping without name_value is invalid under the stated identity contract. This fallback instead creates a key-identified stub. The P1 result therefore measures a hybrid policy, not name-first identity.
Reject the missing identity attribute and record P1 as a validation failure, or rename and document the fallback policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s3_identity/spike.py` around lines 84 - 86, Update
id_name_first so missing name_value is rejected as an invalid name-first
identity instead of falling back to key_value; propagate or record this as a
validation failure so P1 measures only the declared name-first policy.
| "MATCH (d:__Entity__ {id:$dup})-[r:RELATES]->(o) MATCH (k:__Entity__ {id:$keep}) " | ||
| "MERGE (k)-[n:RELATES {rel_type: r.rel_type}]->(o) " | ||
| "SET n.fact = r.fact, n.source_chunk_ids = r.source_chunk_ids DELETE r", | ||
| {"dup": dup, "keep": keep}, | ||
| ) | ||
| await store.query_raw( | ||
| "MATCH (o)-[r:RELATES]->(d:__Entity__ {id:$dup}) MATCH (k:__Entity__ {id:$keep}) " | ||
| "MERGE (o)-[n:RELATES {rel_type: r.rel_type}]->(k) " | ||
| "SET n.fact = r.fact, n.source_chunk_ids = r.source_chunk_ids DELETE r", | ||
| {"dup": dup, "keep": keep}, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve RELATES provenance during alias merges.
If (keep)-[:RELATES {rel_type}]->(other) already exists, SET n.source_chunk_ids = r.source_chunk_ids overwrites its existing provenance. A later stale-edge cleanup can then delete a relationship that another record still supports. The same overwrite loses the existing fact without a defined conflict policy.
Merge source_chunk_ids as a unique union. Apply an explicit conflict policy for fact and other relationship properties.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s3_identity/spike.py` around lines 171 - 180, Update
the alias-merge queries in the duplicate relationship cleanup flow to preserve
existing RELATES provenance when MERGE matches an existing edge. Union
r.source_chunk_ids with n.source_chunk_ids without duplicates, and define an
explicit policy for conflicting fact and other relationship properties instead
of overwriting them; apply the same behavior to both relationship directions.
| await store.rollforward_cutover(pending_id, DOC_ID, DOC_ID, "hash-v2") | ||
| after = await count_chunks(store, DOC_ID) | ||
| await conn.close() | ||
| return {"before": before, "shared_with_pending": shared_chunks, "after_cutover": after} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify the promoted v2 chunks, not only their count.
The effective-keying check can pass if cutover retains v1 chunks and deletes the pending chunks. Query the post-cutover chunk IDs and compare them with the expected IDs derived from pending_id. Also verify that the edited Staff Engineer record is present.
Also applies to: 203-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s4_record_as_chunk/spike.py` around lines 142 - 145,
Update the post-cutover verification around rollforward_cutover and count_chunks
to query the resulting chunk IDs, compare them with the expected v2 IDs derived
from pending_id, and assert the edited “Staff Engineer” record is present. Apply
the same validation to the corresponding verification block around lines 203–211
instead of relying only on chunk counts.
| ``` | ||
| _build_lexical_graph(self, doc_info: DocumentInfo, chunks: TextChunks, ctx: Context, *, | ||
| content_hash: str | None = None) -> None | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add language identifiers to these fenced code blocks.
Use text for both blocks. This removes the reported MD040 warnings.
Also applies to: 26-29
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s5_pipeline_seam/NOTES.md` around lines 10 - 13, Add
the text language identifier to both fenced code blocks in NOTES.md, including
the block containing _build_lexical_graph, so each opening fence uses ```text
and MD040 warnings are resolved.
Source: Linters/SAST tools
| doc = DocumentInfo(path="employees.csv", uid="doc-employees-A") | ||
| try: | ||
| pipe_a = SubclassStructuredPipeline( | ||
| loader=None, # type: ignore[arg-type] | ||
| chunker=None, # type: ignore[arg-type] | ||
| extractor=None, # type: ignore[arg-type] | ||
| resolver=None, # type: ignore[arg-type] | ||
| graph_store=store, | ||
| vector_store=None, | ||
| ontology=ONTOLOGY, | ||
| ) | ||
| mentions_a = await pipe_a.run_structured(rows(), doc, ctx) | ||
| r.check( | ||
| mentions_a > 0, | ||
| "A: subclassing works at runtime — the reused steps never touch the unused strategies", | ||
| f"{mentions_a} MENTIONED_IN edges written", | ||
| ) | ||
| except Exception as exc: # noqa: BLE001 | ||
| r.check(False, "A: subclassing works at runtime", f"{type(exc).__name__}: {exc}") | ||
| a_stats = await store.get_statistics() | ||
| await conn.close() | ||
|
|
||
| # B — mixin. | ||
| conn = connection("poc_s5_mixin") | ||
| store = GraphStore(conn) | ||
| await reset_graph(conn) | ||
| doc = DocumentInfo(path="employees.csv", uid="doc-employees-B") | ||
| pipe_b = MixinStructuredPipeline(store, ONTOLOGY) | ||
| mentions_b = await pipe_b.run_structured(rows(), doc, ctx) | ||
| r.check( | ||
| mentions_b == mentions_a, | ||
| "B: the mixin factoring produces an identical graph with no dead dependencies", | ||
| f"{mentions_b} MENTIONED_IN edges", | ||
| ) | ||
| b_stats = await store.get_statistics() | ||
| r.check( | ||
| a_stats.get("node_count") == b_stats.get("node_count"), | ||
| "A and B agree on the resulting graph", | ||
| f"A={a_stats.get('node_count')} nodes · B={b_stats.get('node_count')} nodes", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the same effective UID before claiming identical graphs.
records_to_chunks() includes doc_uid in every chunk ID. doc-employees-A and doc-employees-B therefore produce different chunk IDs and graph identities. The current checks compare counts only. Use one DocumentInfo.uid in the isolated graphs and compare nodes, edges, and properties, or change the claim to structural equivalence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s5_pipeline_seam/spike.py` around lines 176 - 215,
Use the same effective DocumentInfo.uid for both pipeline runs before asserting
identical graphs, since records_to_chunks() embeds it in chunk IDs. Update the
A/B comparison to validate nodes, edges, and properties rather than only counts,
or revise the assertion wording to check structural equivalence if distinct UIDs
must remain.
| try: | ||
| pipe_a = SubclassStructuredPipeline( | ||
| loader=None, # type: ignore[arg-type] | ||
| chunker=None, # type: ignore[arg-type] | ||
| extractor=None, # type: ignore[arg-type] | ||
| resolver=None, # type: ignore[arg-type] | ||
| graph_store=store, | ||
| vector_store=None, | ||
| ontology=ONTOLOGY, | ||
| ) | ||
| mentions_a = await pipe_a.run_structured(rows(), doc, ctx) | ||
| r.check( | ||
| mentions_a > 0, | ||
| "A: subclassing works at runtime — the reused steps never touch the unused strategies", | ||
| f"{mentions_a} MENTIONED_IN edges written", | ||
| ) | ||
| except Exception as exc: # noqa: BLE001 | ||
| r.check(False, "A: subclassing works at runtime", f"{type(exc).__name__}: {exc}") | ||
| a_stats = await store.get_statistics() | ||
| await conn.close() | ||
|
|
||
| # B — mixin. | ||
| conn = connection("poc_s5_mixin") | ||
| store = GraphStore(conn) | ||
| await reset_graph(conn) | ||
| doc = DocumentInfo(path="employees.csv", uid="doc-employees-B") | ||
| pipe_b = MixinStructuredPipeline(store, ONTOLOGY) | ||
| mentions_b = await pipe_b.run_structured(rows(), doc, ctx) | ||
| r.check( | ||
| mentions_b == mentions_a, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle a failed subclass run before comparing mention counts.
If factoring A raises, Line 206 reads an uninitialized mentions_a value. The spike then aborts before Report.verdict() can report the recorded failure. Initialize mentions_a and guard the comparison, while still assert that factoring B succeeds independently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@poc/structured-ingestion/s5_pipeline_seam/spike.py` around lines 177 - 206,
Initialize mentions_a before the subclass pipeline try block with a failure-safe
value, and guard the factoring-B comparison in the MixinStructuredPipeline
section so it only compares counts when A completed successfully. Keep B’s
independent success assertion active regardless of A’s outcome, allowing
Report.verdict() to run and report recorded failures.
|
Closing — this was opened prematurely on my side. Keeping the structured-ingestion design and the spike round local for now; will re-open a PR when it's actually ready to review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
poc/structured-ingestion/README.md:10
- The README claims this folder is “outside CI (every job sets working-directory: graphrag_sdk and lints only src/)”. That’s not strictly true because the docs workflow (
.github/workflows/docs.yml) runs from the repo root (noworking-directory), even though it doesn’t lint/test Python. Consider narrowing the statement to the Python CI (ci.yml) to avoid misleading future contributors.
**This folder is disposable.** Nothing here ships. It is outside the wheel
(`[tool.hatch.build.targets.wheel] packages = ["src/graphrag_sdk"]`), outside pytest
(`testpaths = ["tests"]`), and outside CI (every job sets `working-directory: graphrag_sdk`
and lints only `src/`). Delete the whole directory once the findings are folded into the design
and the real implementation lands.
docs/design/structured-ingestion.md:398
- This bullet says the PDF table and prose become “adjacent chunks”, but later (§6) the design explicitly sets
link_sequential=Falsefor record chunks to suppressNEXT_CHUNK. WithoutNEXT_CHUNKedges, “adjacent” is misleading; the guaranteed connection is that both chunk sets share the sameDocumentviaPART_OF(and can still be ordered byindexif needed).
- **Chunk retrieval finds rows.** A CSV of product descriptions is genuinely useful text; a
question answered by "the row itself" works with no new retrieval path.
- **The PDF table connects to the PDF prose automatically** — same `Document`, adjacent chunks.
- **Zero-Loss Data holds** — the original record is recoverable from the graph.
Design proposal for structured data ingestion. Docs only — no SDK code.
Refs: FalkorDB/research#82 (POC) · design from research#65 · supersedes #74
The problem
rag.ingest(...)assumes prose. Structured inputs break three of its assumptions at once: the schema is already known, row identity is explicit, and typed columns shouldn't be re-discovered by an LLM. Today a mixed corpus needs a bespoke loader per format or pre-flattening to text, which throws away the structure that made the source valuable.The design has to serve differently-shaped CSVs, nested JSON, a table lifted out of a PDF, and an existing graph that may or may not have its own ontology — all landing in one connected, traversable graph.
The design
Reduce every structured source to a stream of flat records, and let one declarative mapping turn records into typed nodes and
RELATESedges. Three ideas carry it:RecordBatch). A PDF table becomes a record stream carrying the PDF'sDocumentInfo, so its rows are chunks of the sameDocumentas the prose. An existing graph becomes two streams — nodes and edges — because a graph is a node table plus an edge table. Neither is a special case any more.mapping.to_ontology()→ validate / merge / bootstrap / reject. That single fact is the whole answer to "the existing graph may or may not have an ontology."Entity(identity=["name"])). A PDF mention ofAcme Corpand a CSV row withorg_name="Acme Corp"compute the same id and MERGE onto the same node — connected at write time, no similarity threshold, no LLM. A deterministicAliasMatchResolutionbridges types keyed by a real business key (sku), since extraction can only ever produce a name.Supporting decision with the most leverage: a record is persisted as a
Chunkin the normal lexical graph. That inheritsupdate()/delete_document()orphan cleanup, chunk retrieval over rows, and Zero-Loss provenance for free.The result: not one retrieval strategy changes. Entity search, edge-fact search, chunk search, neighbour expansion and text-to-Cypher already see structured data. The doc includes a table justifying that claim path by path.
Design review findings
The two central claims were walked against the code before publishing. Both hold, and the walk surfaced three requirements and one gap — all folded into the document:
nameon nodes andfactandsource_chunk_idsonRELATESedges. Omitting any one silently breaks entity embedding, edge embedding, or stale-edge GC respectively.update()runs againstpending_id = f"{resolved_id}__pending__{uuid4().hex[:8]}", androllforward_cutover()deletes the live document's chunks before promoting the pending — so canonical-keyed chunks would MERGE onto the same nodes and the cutover would delete the chunks it is about to promote.get_document_entity_candidates()has noLIMITand explicitly scopes out documents with millions of entities. A large CSV is exactly one such document, soupdate()on it hits a documented scaling ceiling. Recorded as an open question.Contents
Proposals #1–#7 (the POC) and #8–#12 (follow-ups) in build order, a table mapping them to #82's acceptance criteria, a worked acceptance-scenario example with the traversal that answers it, a rejected-alternatives table with reasons, 8 open questions, and a P1–P5 phasing appendix.
Verification
mkdocs build --strictproduces no new warnings — the one remaining warning (incremental-updates.md) reproduces identically on a clean tree.Summary by CodeRabbit