graph: calcify an imported ontology into LanceDB as 64k tables - #906
graph: calcify an imported ontology into LanceDB as 64k tables#906AdaWorldAPI wants to merge 2 commits into
Conversation
An import that stops at an in-memory artifact leaves every consumer to slice the same blob its own way. This module ends the import on disk: baked node rows go into Lance datasets, and from then on the filled database is what gets addressed. Sibling of `graph::hydrate`, following its source -> RecordBatch -> `Dataset::write` shape. The seam is bytes, not a type. `&[u8]` with a 512-byte stride, so lance-graph takes no dependency on whichever harvest produced the rows and the seam cannot drift from the node layout, because it is the node layout. An OBO harvest, a relational transcode, and a hand-built fixture all arrive the same way. 64k tables are not a chosen chunk size. `identity` is a u16, so a (classid, family) pair addresses exactly 65_536 slots -- one table is a closed address space, 64 Ki x 512 B = 32 MiB at full extent. `partition` is a positional scan that cuts where the prefix changes; it never sorts, groups, or moves a row, and it refuses unsorted input rather than merging it (the alternative is one table silently written as two datasets). One column, not three. `node: FixedSizeBinary(512)` over the caller's own allocation via `Buffer::from_custom_allocation`, with the caller's `Arc` keeping it alive. Splitting into key/edges/value columns would read better and would cost a strided gather over every row -- the copy this module exists to avoid. That carving is instead a read-side projection over byte positions (KEY_RANGE / EDGES_RANGE / VALUE_RANGE). `write_database` is the import's last step; `NodeTable::open` is the addressing that follows. It holds the dataset, not a decoded copy, which is why `count_rows` is async and fallible -- a row count is a read against the data, never a field this struct would have to keep in step with it. Verification: `cargo test -p lance-graph --lib graph::ontology_hydrate` -- 7 passed, 0 failed; `cargo clippy -p lance-graph --lib` reports nothing in this module. The zero-copy claim rests on `the_batch_borrows_the_callers_allocation`, which asserts pointer identity between the bytes Arrow exposes and the buffer handed in, so it is measured rather than asserted in prose.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2b69325b-d7cb-464a-ab26-3c7281719b5c) |
|
Warning Review limit reached
Next review available in: 37 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe graph module now exposes ontology hydration APIs. The implementation validates and partitions sorted 512-byte node rows, builds zero-copy Arrow batches, writes per-table Lance datasets, imports databases, and reads table metadata. ChangesOntology hydration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant InputBuffer
participant partition
participant node_rows_batch
participant write_table
participant LanceDataset
participant NodeTable
InputBuffer->>partition: validate and split sorted node rows
partition->>node_rows_batch: pass each TableRun byte range
node_rows_batch->>write_table: return zero-copy RecordBatch
write_table->>LanceDataset: create per-table dataset
NodeTable->>LanceDataset: open dataset by TableAddr
LanceDataset-->>NodeTable: provide row count and dataset handle
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb617bd9a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // holds that `Arc` for the buffer's whole life, so the memory outlives every | ||
| // read. The buffer is never written through: `Buffer` is immutable, and the | ||
| // `cast_mut` above only satisfies `NonNull<u8>`'s signature. | ||
| let buffer = unsafe { Buffer::from_custom_allocation(ptr, bytes.len(), owner) }; |
There was a problem hiding this comment.
Because this safe public function accepts bytes and owner independently, a caller can pass a stack/local byte slice together with Arc<()>; the returned RecordBatch then outlives the slice and Arrow dereferences a dangling pointer, causing undefined behavior. The function must derive the pointer from the owning Arc (and an offset), copy the data, or expose the ownership relationship as an explicit unsafe precondition rather than assuming it in this unsafe call.
Useful? React with 👍 / 👎.
| if addr == current { | ||
| continue; |
There was a problem hiding this comment.
For same-table rows, this branch skips all validation of identity, so inputs such as identities [7, 3] or [7, 7] are accepted and persisted even though each identity is supposed to name one slot in the table. The later row-count check does not detect either case, allowing an import with malformed upstream ordering or duplicate keys to produce an ambiguously addressed dataset instead of being refused.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/lance-graph/src/graph/ontology_hydrate.rs`:
- Around line 351-356: Update the NodeTable documentation to replace the
nonexistent Self::row reference with the available Self::dataset reference,
while preserving the surrounding description of how rows are read.
- Around line 192-201: Update the row-order validation loop around table_addr_of
so it compares each row’s complete (classid, family, identity) key with the
previous row and rejects any non-increasing key, including descending or
duplicate identities; do not continue when addr == current. Add tests covering
descending and duplicate identities within a table.
- Around line 342-346: Update the import flow around partition and write_table
so database writes target a staging root rather than the final database root,
and publish a completion marker only after every run succeeds. Ensure failed
imports do not leave partial datasets visible and retries can start cleanly;
alternatively, add deterministic cleanup or resume handling before exposing the
database.
- Around line 253-271: Update node_rows_batch and the corresponding
write_table/write_database paths so buffer ownership is explicit: either mark
the API unsafe with documentation requiring owner to retain the allocation
backing bytes, or redesign the safe API to derive bytes from the Arc-owned
allocation. Ensure every Buffer::from_custom_allocation call preserves this
ownership invariant and cannot accept an unrelated Arc owner.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 08807898-4823-4652-b252-5a74f73cb51f
📒 Files selected for processing (2)
crates/lance-graph/src/graph/mod.rscrates/lance-graph/src/graph/ontology_hydrate.rs
| for i in 1..n { | ||
| let addr = table_addr_of(bytes, i); | ||
| if addr == current { | ||
| continue; | ||
| } | ||
| if addr < current { | ||
| return Err(format!( | ||
| "rows are not sorted by (classid, family): row {i} is {addr:?} \ | ||
| after {current:?}" | ||
| )); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject unsorted and duplicate identities within a table.
Line 194 accepts every identity sequence when addr == current. Rows with identities 9, 2, or duplicate identity 9, pass validation. The resulting dataset can contain two rows for one addressable slot.
Compare the full (classid, family, identity) key. Reject a non-increasing key. Add tests for descending and duplicate identities in one table.
Based on PR objectives, partitioning must reject unsorted node rows.
🤖 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 `@crates/lance-graph/src/graph/ontology_hydrate.rs` around lines 192 - 201,
Update the row-order validation loop around table_addr_of so it compares each
row’s complete (classid, family, identity) key with the previous row and rejects
any non-increasing key, including descending or duplicate identities; do not
continue when addr == current. Add tests covering descending and duplicate
identities within a table.
| pub fn node_rows_batch<O>(bytes: &[u8], owner: Arc<O>) -> Result<RecordBatch, String> | ||
| where | ||
| O: std::panic::RefUnwindSafe + Send + Sync + 'static, | ||
| { | ||
| if !bytes.len().is_multiple_of(NODE_ROW_STRIDE) { | ||
| return Err(format!( | ||
| "row buffer of {} bytes is not a multiple of the {NODE_ROW_STRIDE}-byte stride", | ||
| bytes.len() | ||
| )); | ||
| } | ||
| let ptr = std::ptr::NonNull::new(bytes.as_ptr().cast_mut()) | ||
| .ok_or_else(|| "row buffer pointer is null".to_string())?; | ||
|
|
||
| // SAFETY: `ptr`/`bytes.len()` describe exactly the caller's slice, and | ||
| // `owner` is an `Arc` over the allocation that slice points into. Arrow | ||
| // holds that `Arc` for the buffer's whole life, so the memory outlives every | ||
| // read. The buffer is never written through: `Buffer` is immutable, and the | ||
| // `cast_mut` above only satisfies `NonNull<u8>`'s signature. | ||
| let buffer = unsafe { Buffer::from_custom_allocation(ptr, bytes.len(), owner) }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect every public caller. Each byte slice must originate from the same Arc
# passed as the Arrow allocation owner.
rg -n -C 4 --glob '*.rs' \
'\b(node_rows_batch|write_table|write_database)\s*\(' cratesRepository: AdaWorldAPI/lance-graph
Length of output: 3315
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline crates/lance-graph/src/graph/ontology_hydrate.rs --view expanded | sed -n '1,220p' || true
echo "== relevant source =="
sed -n '1,380p' crates/lance-graph/src/graph/ontology_hydrate.rs | cat -n
echo "== public exports / module path =="
rg -n --glob '*.rs' 'mod ontology_hydrate|ontology_hydrate::|node_rows_batch|write_table|write_database' crates/lance-graph/srcRepository: AdaWorldAPI/lance-graph
Length of output: 19603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== arrow dependency versions =="
rg -n 'arrow\s*=|package ="arrow"' Cargo.toml crates -g 'Cargo.toml' || true
cat Cargo.lock 2>/dev/null | sed -n '/^\[\[package\]\]$/,/^\]$/{ /name = "arrow"/,/^\]\$/p }' | head -80
echo "== usages beyond crates tree =="
rg -n -C 3 --glob '*.rs' '\bnode_rows_batch\b|\bwrite_table\b|\bwrite_database\b' . || trueRepository: AdaWorldAPI/lance-graph
Length of output: 2500
Make custom-buffer ownership explicit or derive the slice from the allocation owner.
node_rows_batch accepts any Arc<O> while deriving bytes.as_ptr() from the argument slice. If a caller passes a view plus an unrelated owner, the returned RecordBatch can outlive the allocation backing the view. Make this unsafe with a clear requirement that the Arc owns the buffer allocation, or restrict the safe API to a slice derived from the Arc’s allocation and apply the same model through write_table and write_database.
🤖 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 `@crates/lance-graph/src/graph/ontology_hydrate.rs` around lines 253 - 271,
Update node_rows_batch and the corresponding write_table/write_database paths so
buffer ownership is explicit: either mark the API unsafe with documentation
requiring owner to retain the allocation backing bytes, or redesign the safe API
to derive bytes from the Arc-owned allocation. Ensure every
Buffer::from_custom_allocation call preserves this ownership invariant and
cannot accept an unrelated Arc owner.
| let runs = partition(bytes)?; | ||
| let mut written = Vec::with_capacity(runs.len()); | ||
| for run in runs { | ||
| write_table(bytes, Arc::clone(&owner), run, db_dir).await?; | ||
| written.push(run.addr); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make failed database imports recoverable.
Line 345 writes directly to the final database root. If a later table write fails, earlier datasets remain visible. A retry then fails on the first existing dataset because WriteMode::Create is used.
Write to a staging root and publish a completion marker only after all writes succeed. Alternatively, implement deterministic cleanup or resume semantics.
🤖 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 `@crates/lance-graph/src/graph/ontology_hydrate.rs` around lines 342 - 346,
Update the import flow around partition and write_table so database writes
target a staging root rather than the final database root, and publish a
completion marker only after every run succeeds. Ensure failed imports do not
leave partial datasets visible and retries can start cleanly; alternatively, add
deterministic cleanup or resume handling before exposing the database.
| /// A 64k table opened from disk — the addressable form. | ||
| /// | ||
| /// Holds the Lance dataset, not a decoded copy of it. Reads go through | ||
| /// [`Self::row`], which returns the row's 512 bytes and leaves every reading of | ||
| /// them (key / edges / value, and whatever the classid says those mean) to the | ||
| /// caller. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the nonexistent Self::row reference.
NodeTable has no row method. The documentation directs callers to an unavailable API.
Reference Self::dataset instead, or add the documented method.
🤖 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 `@crates/lance-graph/src/graph/ontology_hydrate.rs` around lines 351 - 356,
Update the NodeTable documentation to replace the nonexistent Self::row
reference with the available Self::dataset reference, while preserving the
surrounding description of how rows are read.
States the one-way ratchet -- source -> ContextBundle -> node rows -> LanceDB tables -> reads -- and the rule it exists for: nothing ever hydrates upward. A consumer addresses the calcified form; it does not parse, does not bake, and does not reach for a source artifact. The moment one consumer parses at runtime, the source becomes a live dependency of every deploy and the ratchet is gone. Records why the CALCIFIED form is what sits in object storage rather than the source: the expensive stages are paid once, every reader gets the same bytes, addressing survives compression (the key is never compressed), and a deploy needs an object-store client instead of a format library per vocabulary. Names two seams honestly, because both are load-bearing: - `lance-graph-ontology::hydrators` produces a `ContextBundle` and stops. Nothing carries a bundle into node rows, so the Pattern-D path ends in memory -- it parses, and the result has nowhere to calcify to. The bake path reaches storage; the hydrator path does not. A session adding a `hydrate_*` today is extending the half with no floor. - The bake emits an edge table beside the rows and only the rows are calcified, although the engine already answers reachability over relation matrices (`TypedGraph::traverse`, `blasgraph::ops::hdr_bfs`). Until the edges are carried there, every consumer needing an ancestor walk writes its own over whatever slice it can get. Separates what is verified from what is not. Verified: the partition cut, the mis-stride refusal, the pointer-identity assertion behind the zero-copy write, the read-side range tiling. NOT verified: no object-store URI has been exercised (every test runs on in-memory buffers); zero-copy is measured up to the `RecordBatch` and NOT through Lance's writer; the multi-table `write_database` -> `NodeTable::open` composition has no test. Each is written as an invitation for the first session that measures it to say so there. Mechanism only -- no corpus, source, or terms appear, per the public-repo rule. Credentials reach Lance through the environment or an explicit `storage_options` map, never a committed path.
1 participant
Ends the ontology import on disk. Baked node rows are written into LanceDB
datasets; from then on the filled database is what gets addressed, not the
in-memory artifact and not the source document.
Sibling of
graph::hydrate, following itssource → RecordBatch → Dataset::writeshape.The seam is bytes, not a type
&[u8]with a 512-byte stride. lance-graph takes no dependency on whicheverharvest produced the rows, and the seam cannot drift from the node layout
because it is the node layout (
key(16) | edges(16) | value(480)). An OBOharvest, a relational transcode, and a hand-built fixture all arrive the same
way.
64k tables
Rows are partitioned into one dataset per
(classid, family). That is not achosen chunk size:
identityis au16, so the pair addresses exactly 65 536slots — a table is a closed address space, 64 Ki × 512 B = 32 MiB at full
extent.
partitionis a positional scan that cuts where the prefix changes. It neversorts, groups, or moves a row, and it refuses unsorted input rather than
merging it — the alternative is one table silently written as two datasets.
One column, not three
node: FixedSizeBinary(512)over the caller's own allocation viaBuffer::from_custom_allocation, kept alive by the caller'sArc.Splitting into
key/edges/valuecolumns would read better and would cost astrided gather over every row — the copy this module exists to avoid. That
carving is instead a read-side projection over byte positions (
KEY_RANGE/EDGES_RANGE/VALUE_RANGE).Read side
write_databaseis the import's last step;NodeTable::openis the addressingthat follows. It holds the dataset rather than a decoded copy, which is why
count_rowsis async and fallible — a row count is a read against the data,never a field the struct would have to keep in step with it.
Verification
cargo test -p lance-graph --lib graph::ontology_hydrate— 7 passed, 0 failedcargo clippy -p lance-graph --lib— no findings in this moduleThe zero-copy claim rests on
the_batch_borrows_the_callers_allocation, whichasserts pointer identity between the bytes Arrow exposes and the buffer handed
in — measured, not asserted in prose.
Note for anyone building this tree fresh:
lance-encoding's build scriptrequires
protoc(apt-get install protobuf-compiler), and a fulltarget/debugfor this workspace runs to several GB.Summary by CodeRabbit