feat(graph): engine overhaul — freeze-boundary correctness + P1-P3 optimization waves#231
Merged
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThis PR adds CSR v5 property/embedding storage, cross-tier freeze-boundary reads and writes, lazy per-segment indexing with plan-cache normalization, slot-based Cypher execution with timeout guards, HNSW-backed hybrid search, traversal fast paths, and updated tests, fuzzing, and changelog entries. ChangesGraph engine correctness and performance overhaul
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQuery
participant PlanCache
participant Executor
participant MergedNodeView
participant MemGraph
participant CsrStorage
Client->>GraphQuery: GRAPH.QUERY(cypher)
GraphQuery->>GraphQuery: normalize_cypher / parameterize
GraphQuery->>PlanCache: lookup normalized hash
alt cache hit
PlanCache-->>GraphQuery: cached PhysicalPlan
else cache miss
GraphQuery->>GraphQuery: parse_effective + compile plan
GraphQuery->>PlanCache: insert plan
end
GraphQuery->>Executor: run_read_query(plan, params)
Executor->>MergedNodeView: is_visible / property / labels
MergedNodeView->>MemGraph: check mutable tier
MergedNodeView->>CsrStorage: check frozen segments
Executor-->>GraphQuery: result rows
GraphQuery-->>Client: response
sequenceDiagram
participant HybridEngine
participant MergedNodeView
participant GraphHnsw
participant SegmentMergeReader
participant CsrStorage
HybridEngine->>MergedNodeView: validate start node / embedding
HybridEngine->>SegmentMergeReader: bfs_collect across tiers
SegmentMergeReader-->>HybridEngine: candidate nodes
HybridEngine->>CsrStorage: hnsw_bridge() lazily built
HybridEngine->>GraphHnsw: search(query, k, allow)
GraphHnsw-->>HybridEngine: approximate scored rows
HybridEngine->>MergedNodeView: exact-score residual candidates
HybridEngine-->>HybridEngine: merge + rank top-k results
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/graph/memgraph.rs (1)
34-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider
FxHashMapfor ghost adjacency.
ghost_out/ghost_inare keyed byNodeKey(a slotmap key) — exactly the "graph-internal keys" profilefasthash.rs(added in this same PR) documentsFxHashMap/FxHashSetfor. Usingstd::collections::HashMaphere means SipHash overhead on every cross-tier edge insert/freeze rebuild for a key type that doesn't need DoS resistance.♻️ Suggested change
- ghost_out: std::collections::HashMap<NodeKey, SmallVec<[EdgeKey; 4]>>, - ghost_in: std::collections::HashMap<NodeKey, SmallVec<[EdgeKey; 4]>>, + ghost_out: crate::graph::fasthash::FxHashMap<NodeKey, SmallVec<[EdgeKey; 4]>>, + ghost_in: crate::graph::fasthash::FxHashMap<NodeKey, SmallVec<[EdgeKey; 4]>>,- ghost_out: std::collections::HashMap::new(), - ghost_in: std::collections::HashMap::new(), + ghost_out: crate::graph::fasthash::FxHashMap::default(), + ghost_in: crate::graph::fasthash::FxHashMap::default(),Also applies to: 55-56
🤖 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 `@src/graph/memgraph.rs` around lines 34 - 39, The ghost adjacency maps in MemGraph should use the faster internal-key hash map type instead of std::collections::HashMap. Update the ghost_out and ghost_in fields in MemGraph to use FxHashMap (matching the guidance in fasthash.rs for graph-internal keys like NodeKey), and make the corresponding adjustment wherever these maps are rebuilt or initialized so the same type is used consistently.src/graph/fasthash.rs (1)
1-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider the
rustc-hashcrate instead of a hand-rolled FxHash.The reimplementation here faithfully matches the legacy FxHash algorithm, but upstream
rustc-hash(2.x) replaced this exact algorithm with a newer polynomial hash + bit-rotation finisher (by Orson Peters) specifically to fix known weaknesses in the old design (poor string/slice hashing, high-bit information loss). The doc comment's justification ("to avoid a new dependency") is reasonable, butrustc-hashis a tiny, zero-dependency, widely-vetted crate with the sameFxHashMap/FxHashSet/FxBuildHasherAPI surface, so swapping would get a better-tested and improved hash for free with a near drop-in 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 `@src/graph/fasthash.rs` around lines 1 - 105, Replace the hand-rolled hashing in FxHasher/FxBuildHasher with the upstream rustc-hash crate, since it already provides the same FxHashMap/FxHashSet/FxBuildHasher API and a better-tested newer hash implementation. Update the graph/fasthash module to re-export or alias the crate types instead of maintaining the custom Hasher/write_* logic, and keep the tests focused on the public map/set behavior rather than the internal algorithm.
🤖 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 `@src/graph/csr/props.rs`:
- Around line 81-119: The record offset stored by encode_node_record and
encode_edge_record is being truncated to u32, which can wrap once the
props/embedding blob grows past 4 GiB. Update these writers (and the related
copy_record path) to either use a larger offset type or explicitly validate pos
before casting, and make freeze/compaction fail early if the blob would overflow
u32. Keep the behavior consistent across encode_node_record, encode_edge_record,
and any code that persists property_offset so decoders never see a wrapped
offset.
In `@src/graph/cypher/executor/mod.rs`:
- Around line 194-201: Row::iter currently returns every plan-bound name in slot
order, which causes later unbound variables to be projected as Null in
Expr::Star cases like RETURN * and WITH *. Update Row::iter in Row to skip
unbound slots by filtering against the row’s bound state, or make the iterator
sparse so only in-scope bindings are yielded; then ensure Expr::Star uses that
bounded iteration path when expanding projections.
In `@src/graph/row_bfs.rs`:
- Around line 288-293: The worker-join handling in row_bfs::expand_push and the
matching pattern in expand_pull is swallowing panics by only collecting Ok
results, which can return an incomplete BfsResult as if traversal succeeded.
Update the join handling to propagate any JoinHandle panic immediately instead
of skipping it, so a panicking expand_chunk worker causes the overall BFS to
fail loudly rather than silently losing discoveries.
---
Nitpick comments:
In `@src/graph/fasthash.rs`:
- Around line 1-105: Replace the hand-rolled hashing in FxHasher/FxBuildHasher
with the upstream rustc-hash crate, since it already provides the same
FxHashMap/FxHashSet/FxBuildHasher API and a better-tested newer hash
implementation. Update the graph/fasthash module to re-export or alias the crate
types instead of maintaining the custom Hasher/write_* logic, and keep the tests
focused on the public map/set behavior rather than the internal algorithm.
In `@src/graph/memgraph.rs`:
- Around line 34-39: The ghost adjacency maps in MemGraph should use the faster
internal-key hash map type instead of std::collections::HashMap. Update the
ghost_out and ghost_in fields in MemGraph to use FxHashMap (matching the
guidance in fasthash.rs for graph-internal keys like NodeKey), and make the
corresponding adjustment wherever these maps are rebuilt or initialized so the
same type is used consistently.
🪄 Autofix (Beta)
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
Run ID: 81f09bef-cbed-4ac9-a9e9-e073fbe93b41
📒 Files selected for processing (35)
.github/workflows/fuzz.yml.gitignoreCHANGELOG.mdfuzz/Cargo.tomlsrc/command/graph/graph_read.rssrc/command/graph/graph_write.rssrc/command/graph/mod.rssrc/graph/compaction.rssrc/graph/csr/mmap.rssrc/graph/csr/mod.rssrc/graph/csr/props.rssrc/graph/csr/storage.rssrc/graph/cypher/executor/eval.rssrc/graph/cypher/executor/mod.rssrc/graph/cypher/executor/read.rssrc/graph/cypher/executor/write.rssrc/graph/cypher/mod.rssrc/graph/cypher/parameterize.rssrc/graph/cypher/planner.rssrc/graph/fasthash.rssrc/graph/hnsw_bridge.rssrc/graph/hybrid.rssrc/graph/index.rssrc/graph/memgraph.rssrc/graph/mod.rssrc/graph/replay.rssrc/graph/row_bfs.rssrc/graph/store.rssrc/graph/traversal.rssrc/graph/traversal_guard.rssrc/graph/types.rssrc/graph/view.rssrc/graph/visibility.rssrc/graph/wal.rstests/graph_freeze_boundary.rs
Comment on lines
+81
to
+119
| pub fn encode_node_record( | ||
| blob: &mut Vec<u8>, | ||
| props: &[(u16, PropertyValue)], | ||
| embedding: Option<&[f32]>, | ||
| ) -> u32 { | ||
| if props.is_empty() && embedding.is_none() { | ||
| return 0; | ||
| } | ||
| let pos = blob.len(); | ||
| encode_props(blob, props); | ||
| match embedding { | ||
| Some(emb) => { | ||
| blob.extend_from_slice(&(emb.len() as u32).to_le_bytes()); | ||
| for f in emb { | ||
| blob.extend_from_slice(&f.to_le_bytes()); | ||
| } | ||
| } | ||
| None => blob.extend_from_slice(&0u32.to_le_bytes()), | ||
| } | ||
| (pos as u32) + 1 | ||
| } | ||
|
|
||
| /// Append an edge record (weight + optional properties) to `blob`. | ||
| /// | ||
| /// Returns the stored offset (`byte_pos + 1`), or `0` when the edge carries | ||
| /// the default weight and no properties (nothing is written). | ||
| pub fn encode_edge_record(blob: &mut Vec<u8>, weight: f64, props: Option<&PropertyMap>) -> u32 { | ||
| let has_props = props.is_some_and(|p| !p.is_empty()); | ||
| if weight == DEFAULT_EDGE_WEIGHT && !has_props { | ||
| return 0; | ||
| } | ||
| let pos = blob.len(); | ||
| blob.extend_from_slice(&weight.to_le_bytes()); | ||
| match props { | ||
| Some(p) => encode_props(blob, p), | ||
| None => blob.extend_from_slice(&0u16.to_le_bytes()), | ||
| } | ||
| (pos as u32) + 1 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any segment/blob size cap that would bound node_props/edge_props length.
rg -nP 'max_segment|segment.*cap|property_offset|u32::MAX' src/graph --type=rust -C2Repository: pilotspace/moon
Length of output: 23902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the codec and compaction paths around property offsets and blob sizing.
sed -n '1,260p' src/graph/csr/props.rs
printf '\n---\n'
sed -n '1,260p' src/graph/csr/mod.rs
printf '\n---\n'
sed -n '340,500p' src/graph/compaction.rs
printf '\n---\n'
rg -n "u32::MAX|max_segment_edges|max_segments|property_offset|blob.len\(|as u32\)\s*\+\s*1|copy_record\(" src/graph/csr src/graph/compaction.rsRepository: pilotspace/moon
Length of output: 32586
property_offset must not be truncated to u32. encode_node_record, encode_edge_record, and copy_record all store (pos as u32) + 1, and there’s no blob-size cap in the CSR/compaction path. Once a node_props or edge_props blob exceeds 4 GiB, the offset wraps and decoders read the wrong record, silently corrupting properties/embeddings. Guard the cast or fail freeze/compaction before the blob can overflow u32.
🤖 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 `@src/graph/csr/props.rs` around lines 81 - 119, The record offset stored by
encode_node_record and encode_edge_record is being truncated to u32, which can
wrap once the props/embedding blob grows past 4 GiB. Update these writers (and
the related copy_record path) to either use a larger offset type or explicitly
validate pos before casting, and make freeze/compaction fail early if the blob
would overflow u32. Keep the behavior consistent across encode_node_record,
encode_edge_record, and any code that persists property_offset so decoders never
see a wrapped offset.
Comment on lines
+194
to
+201
| /// Iterate (name, value) pairs in slot order. | ||
| pub fn iter(&self) -> impl Iterator<Item = (&str, &Value)> { | ||
| self.table | ||
| .names | ||
| .iter() | ||
| .map(String::as_str) | ||
| .zip(self.slots.iter()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all Star / RETURN * / WITH * consumers of Row::iter to gauge column impact.
rg -nP -C3 'Expr::Star|row\.iter\(\)|\.iter\(\)\.map\(\|\(k, v\)\|' src/graph/cypherRepository: pilotspace/moon
Length of output: 5926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the row/slot-table implementation and the star projection consumers.
sed -n '1,260p' src/graph/cypher/executor/mod.rs
printf '\n==== eval.rs slice ====\n'
sed -n '390,430p' src/graph/cypher/executor/eval.rs
printf '\n==== read.rs slice 1 ====\n'
sed -n '390,430p' src/graph/cypher/executor/read.rs
printf '\n==== read.rs slice 2 ====\n'
sed -n '930,970p' src/graph/cypher/executor/read.rs
printf '\n==== write.rs slice ====\n'
sed -n '170,195p' src/graph/cypher/executor/write.rs
printf '\n==== tests around star/projection ====\n'
rg -n -C 3 'RETURN \*|WITH \*|Expr::Star|row\.iter\(\)' src/graph/cypher -g '!target'Repository: pilotspace/moon
Length of output: 21316
Row::iter should skip unbound slots row.iter() now walks every plan-bound name, so RETURN * / WITH * can project later variables as Null before they’re in scope. Filter Expr::Star to bound bindings or make the iterator sparse.
🤖 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 `@src/graph/cypher/executor/mod.rs` around lines 194 - 201, Row::iter currently
returns every plan-bound name in slot order, which causes later unbound
variables to be projected as Null in Expr::Star cases like RETURN * and WITH *.
Update Row::iter in Row to skip unbound slots by filtering against the row’s
bound state, or make the iterator sparse so only in-scope bindings are yielded;
then ensure Expr::Star uses that bounded iteration path when expanding
projections.
Comment on lines
+288
to
+293
| for h in handles { | ||
| // A worker panicking is a bug in expand_chunk itself; surface it. | ||
| if let Ok(out) = h.join() { | ||
| outs.push(out); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Propagate worker panics instead of swallowing them. The comment says a panicking worker should be surfaced, but if let Ok(out) = h.join() silently discards that chunk's discoveries — a panicked worker would produce an incomplete BfsResult returned as Ok, i.e. silently wrong traversal results rather than a crash. expand_pull (Lines 375-379) has the identical pattern.
🛡️ Re-raise the worker panic
for h in handles {
- // A worker panicking is a bug in expand_chunk itself; surface it.
- if let Ok(out) = h.join() {
- outs.push(out);
- }
+ // A worker panicking is a bug in expand_chunk itself; surface it.
+ match h.join() {
+ Ok(out) => outs.push(out),
+ Err(e) => std::panic::resume_unwind(e),
+ }
}📝 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.
Suggested change
| for h in handles { | |
| // A worker panicking is a bug in expand_chunk itself; surface it. | |
| if let Ok(out) = h.join() { | |
| outs.push(out); | |
| } | |
| } | |
| for h in handles { | |
| // A worker panicking is a bug in expand_chunk itself; surface it. | |
| match h.join() { | |
| Ok(out) => outs.push(out), | |
| Err(e) => std::panic::resume_unwind(e), | |
| } | |
| } |
🤖 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 `@src/graph/row_bfs.rs` around lines 288 - 293, The worker-join handling in
row_bfs::expand_push and the matching pattern in expand_pull is swallowing
panics by only collecting Ok results, which can return an incomplete BfsResult
as if traversal succeeded. Update the join handling to propagate any JoinHandle
panic immediately instead of skipping it, so a panicking expand_chunk worker
causes the overall BFS to fail loudly rather than silently losing discoveries.
…hase 0)
Adds tests/graph_freeze_boundary.rs driving the real GRAPH.* command
handlers with edge_threshold=8 so MemGraph::freeze_and_compact fires
after 8 inserts. Documents the P0 found in the 2026-07 graph deep
review (tmp/GRAPH-DEEP-REVIEW-2026-07.md): freeze() DRAINS the write
buffer and CSR segments carry no properties, so compacted data is
lost to queries.
Red (9 failing / 1 passing mechanism-sanity):
- MATCH label scan / property filter / RETURN prop lose frozen nodes
- 1-hop expand from a frozen point query returns 0 rows
- mixed-tier scan sees only the mutable tail
- GRAPH.NEIGHBORS rejects frozen nodes ("node not found")
- GRAPH.ADDEDGE between frozen endpoints fails (append-once flaw)
- GRAPH.PROFILE returns 0 rows post-freeze
- GRAPH.HYBRID loses compacted candidates
These go green across Phase 0 (property blob v5, MergedNodeView,
NEIGHBORS/PROFILE/HYBRID rewiring).
author: Tin Dang
…ross freeze Fixes the freeze-boundary data-loss P0 (storage half): CsrSegment::from_frozen wrote property_offset: 0 unconditionally, silently DISCARDING node properties, embeddings, edge weights, and edge properties when a graph crossed edge_threshold (64K edges). - src/graph/csr/props.rs: self-contained record codec (tagged prop entries + embedding dim + edge weight), offset+1 scheme so 0 keeps meaning "absent". Bounds-checked decoders never panic; fuzz target graph_props_record added to fuzz/ and both CI fuzz matrices. - Format v5: two trailing [len:u64][bytes] blob sections after label_overflow; header layout unchanged (positional parsing like v4). v1-v4 files load with empty blobs (graceful degradation). New unsafe: two mmap pointer-slice accessors following the module's existing validated-pattern (SAFETY comments; flagged for unsafe-policy review). - from_frozen + compact_segments now encode/carry records; the merge copies each winner's record verbatim from its winning segment. - Bonus fix: compact_segments left node_id_to_row + MPH EMPTY, so a freshly merged in-memory segment could not resolve any NodeKey (lookup_node → None) until persisted and reloaded. Rebuilt from external_id like from_bytes does. - parse_label_overflow now returns its end offset (v5 sections follow it). Tests: props codec unit suite (incl. truncation sweep), v5 roundtrip (from_frozen / bytes / mmap), v4-compat parse, merged-segment carry-over + NodeKey lookup, downgrade helpers fixed for v5 trailer. graph lib 424/424. Read-side wiring (NodeScan/eval/NEIGHBORS/HYBRID) lands next — the freeze-boundary integration suite stays red until then by design. author: Tin Dang
…y collision fix Phase 0 task 3 of the graph-engine overhaul: post-freeze read correctness for Cypher MATCH / property eval, plus a newly discovered NodeKey aliasing bug in the freeze path. MergedNodeView (src/graph/view.rs): one seam over both tiers (mutable MemGraph write buffer + immutable CSR segments) — segment_row / contains / is_visible / property / properties / embedding / labels / for_each_visible_node. Frozen rows resolve properties through the v5 property blob, labels through label_bitmap + overflow, visibility through the new visibility::is_meta_visible (NodeMeta twin of is_node_visible, txn_id = 0 since frozen segments hold only committed data). Wired through: - Cypher NodeScan (execute + execute_profile): scans both tiers via for_each_visible_node — frozen nodes were invisible to MATCH before (freeze DRAINS the memgraph; the read side assumed a copy). - Expand target visibility: frozen targets now get the CSR NodeMeta MVCC/valid-time check instead of a silent free pass. - eval_expr PropertyAccess + labels(): merged-tier lookup, so RETURN n.prop / WHERE n.prop = x / labels(n) work on compacted nodes. NodeKey collision fix (P0, pre-existing): freeze_and_compact replaced write_buf with a FRESH MemGraph. A fresh SlotMap restarts key allocation at the same (index, generation) pairs, so the first node added after a freeze received the same NodeKey ffi as the first frozen node — aliasing new nodes onto frozen CSR rows in every merged lookup (SegmentMergeReader included). Fixed with MemGraph::thaw(): re-arm the SAME drained slot maps (drain bumps each slot's generation, guaranteeing fresh keys) and reset live counters; freeze_and_compact now thaws in place on both the success and CSR-build-failure paths. Freeze-boundary integration suite: 6/10 green (was 1/10) — label scan, property filter, property return, one-hop expand, mixed-tier scan. Remaining 4 red are the next tasks: NEIGHBORS/ADDEDGE existence + PROFILE Expand (task 4), GRAPH.HYBRID (task 5). Tests: graph lib suite 429/429; new red/green coverage: - store: test_freeze_and_compact_preserves_key_uniqueness (red on the fresh-MemGraph design, green with thaw) - view: two-tier fixture tests (contains / property+embedding / labels / merged scan with label filter) Also: /target-fast/ gitignored (release-fast iteration dir, matches the target-* convention). author: Tin Dang
… see frozen tier Phase 0 task 4: the native command surface and WAL replay now work across the freeze boundary, built on a new delta-edge mechanism in MemGraph. Delta edges (memgraph.rs): an edge whose endpoint was frozen into a CSR segment has no MutableNode to carry adjacency, and CsrSegment::from_frozen silently drops edges without in-segment endpoint rows. New design: - ghost_out / ghost_in maps hold adjacency keyed by frozen NodeKey; neighbors() serves them for non-resident nodes, so cross-tier edges are traversable from BOTH ends (SegmentMergeReader picks them up for free). - add_edge_across_tiers(): validates resident endpoints alive; trusts the caller (MergedNodeView::is_visible) for frozen ones. Plain add_edge keeps rejecting unknown endpoints. - freeze() now PARTITIONS edges: live + both endpoints resident → CSR; live + any earlier-frozen endpoint → retained in the mutable tier with its EdgeKey intact and ghost adjacency rebuilt; dead → dropped. freeze_and_compact skips empty freezes (all-delta buffer would otherwise churn empty segments) and thaw() recounts retained edges. Fixed call sites (each silently lost or rejected data post-freeze): - GRAPH.ADDEDGE: verifies frozen endpoints via MergedNodeView (alive in a segment), then inserts a delta edge — previously "ERR source or destination node not found". - GRAPH.NEIGHBORS: existence check via the merged view (comment claimed "freeze copies, doesn't move" — it drains); new DIRECTION IN|OUT|BOTH argument is parsed and honored (was hardcoded Both). - GRAPH.PROFILE Expand: now uses SegmentMergeReader + merged-view target visibility (parity with GRAPH.QUERY) — profiled expansions returned 0 rows on frozen graphs. - WAL replay AddEdge: node_map already seeded CSR-resident endpoints, but add_edge refused them, silently dropping every cross-segment edge during recovery; now inserts delta edges (node_map membership is the existence proof). Freeze-boundary suite: 10/11 green (was 6/10; +new DIRECTION test). Last red is GRAPH.HYBRID (next task). Graph lib 431/431, command::graph 25/25. author: Tin Dang
…complete Phase 0 task 5 (last freeze-boundary correctness gap): the hybrid graph+vector executors (HYB-01 FILTER, HYB-02 EXPAND, HYB-03 WALK, HYB-04 RERANK) read only the mutable write buffer — post-freeze they rejected the start node (NodeNotFound) or silently scored an empty candidate set. - All four executors now take the immutable CSR segments alongside the MemGraph: traversal goes through SegmentMergeReader (frozen + delta edges), node existence and embeddings resolve through MergedNodeView (embeddings decode from the CSR v5 blob for frozen rows). bfs_collect / collect_context are segment-aware; RERANK's all-nodes scan enumerates both tiers with MVCC visibility. Pass `&[]` for a bare MemGraph. - GRAPH.VSEARCH and GRAPH.HYBRID handlers load the segment snapshot and thread it through. - extract_f32_vector: binary little-endian f32 fallback after the text form — the SAME blob format GRAPH.ADDNODE's VECTOR argument accepts, so vectors round-trip between ADDNODE and VSEARCH/HYBRID unchanged (previously HYBRID only accepted whitespace-separated text and answered "ERR invalid vector" to its own storage format). Freeze-boundary suite: 11/11 GREEN (was 1/10 at review time) — Phase 0 exit criterion met. Graph lib 431/431. author: Tin Dang
…elete dead insert-time index Phase 1 task 6 — the storage half of the property-index lever identified by the 2026-07 deep review (point query = 99% unindexed label scan; FalkorDB wins reads on exactly this). SegmentPropertyIndexes (index.rs): per-CSR-segment indexes over row ids, built lazily on first use from the v5 node-property blob (OnceLock, same pattern as the incoming-edge index — one code path covers from_frozen, compact, from_bytes, and mmap reload). Numeric values (Int/Float/Bool) share a per-property B-tree (equality + range via the existing PropertyIndex); String/Bytes index by xxh64 hash (equality). Hash collisions and Bool/Int aliasing only ever produce SUPERSET candidate sets — the planner keeps its residual Filter, so collisions cost a re-check, never a wrong row. The build is exhaustive over segment rows, so lookups return empty bitmaps for absent values (no fall-back-to-scan signal needed; pre-v5 segments genuinely hold no properties). Accessor: CsrStorage::property_index() (heap + mmap variants). Deleted the dead insert-time maintenance: NamedGraph.property_indexes was populated on every ADDNODE with `ext_id as u32` — TRUNCATED external ids, a row space nothing could resolve — and no query path ever read it. Field, initializer, and the per-ADDNODE update loop are gone (the _key registration that lived in the same loop is preserved). Next (task 7): PhysicalOp::IndexScan consumes these via label-bitmap ∧ property-bitmap per segment. Tests: graph lib 432/432; new test_segment_property_index_eq_and_range covers Int/Float equality on one B-tree, string hash equality, range, and absent-property emptiness. author: Tin Dang
…operty indexes
Phase 1 task 7, the read-side half of the property-index lever (probe
showed Cypher point queries spend ~99% of their time in the unindexed
label scan; this is FalkorDB's entire read advantage).
Planner: a pattern node whose inline properties are all literals or
parameters (`(n:L {k:3})`, `(n:L {k:$v})`) now plans as
PhysicalOp::IndexScan { variable, label, prop_eq } instead of NodeScan —
emitted for the scanned first node and shortestPath endpoints (shared
push_node_scan helper). The full residual Filter is ALWAYS kept
immediately downstream: index lookups have superset semantics (string
hashes can collide, Bool/Int alias numerically), so the index only
prunes, never decides.
Executor (index_scan_keys, both execute and execute_profile):
- prop_eq values resolve per run via eval_expr (parameters index-hit under
the plan cache); an unresolvable value degrades to the merged label scan.
- Frozen tier: per segment, property bitmaps intersect (short-circuit on
empty) ∧ label bitmap, then MVCC/valid-time visibility per surviving row.
- Mutable tail: linear scan with a superset-consistent loose-equality
check (numeric coercion mirrors the index) — bounded by edge_threshold.
- Write-path executor treats IndexScan as its NodeScan (mutable-tier
MATCH semantics unchanged; residual Filter keeps it exact).
- GRAPH.PROFILE reports the op as "IndexScan".
Planner contract tests updated: literal inline props now pin
IndexScan+Filter (the v3-2 "Filter immediately follows the binding op"
contract is preserved, the binding op changed); new emission tests cover
literal / parameter / multi-prop / no-prop cases.
Known scope cut: WHERE-clause equality (`WHERE n.k = 3`) still plans as
NodeScan+Filter — extracting index-eligible conjuncts from WHERE is a
follow-up.
Tests: graph lib 433/433; freeze-boundary 11/11 (point-query tests now
exercise IndexScan end-to-end); tokio cypher suites green (inline-filter,
shortest-path, txn-write-rollback).
author: Tin Dang
…spatch
Phase 2 task 8. The plan cache was keyed on the raw query bytes, so the
dominant graph workload — point queries differing only in literal values —
NEVER hit it (every GRAPH.QUERY re-parsed and re-compiled). Eviction was
also arbitrary (HashMap keys().next()), and the server routing path parsed
every Cypher query TWICE (is_cypher_write_query full parse + handler parse).
Auto-parameterization (new src/graph/cypher/parameterize.rs): value
literals (int/float/string) are rewritten into synthetic parameters
($__p0, $__p1, …) at the TOKEN level, with values extracted into the
params map — so `{id: 3}` and `{id: 4}` share one cached plan and still
index-hit (PhysicalOp::IndexScan resolves Expr::Parameter per run).
Structural literals stay literal because they are baked into the plan,
not evaluated: variable-length hop bounds (prev token `*`/`..` or next
`..`), LIMIT/SKIP counts (conservative pin), and leading-minus literals
(logos maximal munch lexes `x-1` as Ident+Integer(-1); rewriting would
swallow a binary minus). Fail-open by construction: lex error, non-UTF-8
string, unparsable number, or a pre-existing `$__p` name returns None and
the caller falls back to raw-text raw-hash caching; if normalized text
ever failed to parse, parse_effective re-parses the raw text so user
errors reference their own query. Wrong results are impossible — only a
missed cache share.
Plan cache: proper LRU (monotonic access tick, evict min on overflow;
O(capacity) scan only when a full cache takes a new query text) and a new
invariant — only READ-ONLY plans are inserted (graph_query previously
cached write plans compiled for its error path).
Single-parse dispatch, enabled by that invariant:
- Cache HIT now runs with ZERO parse/compile work (previously: parse +
hash + lookup). The hit path routes read-only directly since only read
plans are cached.
- is_cypher_write_query: full parse → token scan for CREATE/DELETE/SET/
MERGE. May false-positive (property named `set`) which only routes via
the write-capable handler; graph_query_or_write AST-classifies and runs
the read path correctly. Never false-negatives for a parsable write.
- GRAPH.RO_QUERY: parsed twice before (own classify parse + delegated
graph_query parse); now shares graph_query_readonly (single parse, and
zero on cache hit — a hit is read-only by the invariant).
- Write path merges auto-params before execute_mut (CREATE literals are
normalized too; without the merge they would store Nulls).
Behavior notes: RO_QUERY of a write query against a MISSING graph now
reports "graph not found" instead of the write-clause error (graph lookup
precedes parse in the shared core); GRAPH.EXPLAIN/PROFILE stay raw-parse
(stable output, no cache).
Tests (red→green): plan-cache sharing across int and string literal
variants with per-query correct results, write-plan non-caching, LRU
eviction order, var-length hop bounds pinned to distinct plans (wrongly
parameterizing hops would return wrong row counts — pinned 5 vs 6 rows),
11 parameterize unit tests incl. normalized-text-reparses.
Suites: lib 3800 green, graph_freeze_boundary 11/11, tokio cypher suites
(inline-filter, shortest-path, txn-write-rollback) green, clippy both
feature sets, fmt.
author: Tin Dang
…tion Phase 2 task 9. Every executor row was a HashMap<String, Value>: each scan/expand/unwind emission allocated a fresh hasher table AND cloned the variable name String per binding; row clones re-hashed everything. For multi-row pipelines (the normal case) this dominated per-row cost. Row is now a slot-indexed SmallVec<[Value; 4]> plus a reference to a per-execution SlotTable (all variables the plan can bind, collected once by walking the binding operators: scans, expands, unwind, shortestPath, create/merge patterns). Name→slot resolution is a linear scan over a handful of short names — cheaper than hashing at this cardinality. The public row API is shape-compatible (get/insert/iter), so eval_expr's signature is unchanged; insert now takes &str, deleting the per-binding String clone. Semantics: unbound slots hold Value::Null, which every consumer already treats identically to the old HashMap miss — expression eval defaulted a missing variable to Null, and operator sources pattern-match a concrete variant (Some(Value::Node(_))), which Null fails exactly like None did. Binding is uniform per operator (each binding op binds its variable on every row it emits), so the no-Project column set (SlotTable names, sorted) and RETURN * maps match the old bound-keys view. Referenced-but-never-bound names resolve to no slot (None), matching a HashMap miss; a bound name missing from the table would be a plan/table desync and is debug_assert'ed. Tests: SlotTable collection (match + create/merge pattern vars), Row get/insert/iter/clone semantics incl. unbound-vs-unknown distinction. Suites: lib 3802 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets. author: Tin Dang
Phase 2 task 10 — the deep review's allocation-discipline findings (NEIGHBORS profile: ~25% of time dropping Frames, ~20% in the ALLOCATING SegmentMergeReader::neighbors; ADDNODE 2× WAL-encode). - ADDNODE double WAL serialize deleted: a full record (incl. embedding bytes) was built with a placeholder id, dropped unread, and rebuilt from the stored node. Now a single serialization from the stored node's refs. - wal.rs: 15 `itoa::Buffer::format().to_owned()` sites drop the String — the formatted &str is passed straight to write_bulk (statement-temporary pattern already used by write_bulk itself). format_f64's format!() is replaced by ryu (already a dep) for the edge weight and Float property sites; replay parses f64 so "1.0"-vs-"1" style differences are inert. - New src/graph/fasthash.rs: FxHash multiply-fold (rustc's internal hasher, inlined — no new dependency). NodeKey/EdgeKey are slotmap PODs in maps we control (visited sets, dist maps), where SipHash's DoS resistance buys nothing. Module docs pin the provenance contract: never key by attacker-controlled bytes. - traversal.rs: all non-test NodeKey sets/maps → FxHashSet/FxHashMap; Dijkstra's dist/prev/depth map TRIPLE merged into one FxHashMap<NodeKey, DijkstraNodeState> — one hash lookup per relaxation where three (×SipHash) used to run. - Cypher Expand (both executors): the allocating `neighbors()` (fresh HashSet+Vec per source node — worst in variable-length BFS, per frontier node) replaced with `neighbors_into` on scratch hoisted per operator; BFS visited sets → FxHashSet. Deferred (follow-up, noted in review doc): Value::String(String)→Bytes to stop value_to_frame cloning string cells — ripples through all of eval's string ops, separate change. Tests: fasthash unit tests (map/set behavior, 10k-distinct-hash sanity); suites: lib 3804 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets. author: Tin Dang
…n-optimizing pull Phase 3 task 11. BFS on a fully-frozen graph paid NodeKey hashing on every edge probe (visited set), MPH lookups per hop, and — in the old "ParallelBfs" — fake parallelism: the reader borrows !Send MemGraph, so neighbor expansion ran sequentially (with a per-node nb_buf.clone()) and worker threads only did DashSet merging. New src/graph/row_bfs.rs, gated per query (mutable tier absent-or-empty ∧ exactly one CSR segment ∧ segment visible at the snapshot — every case the reader path handles generically is a reason to fall back, never a behavior fork; BoundedBfs/ParallelBfs::execute try the gate first): - Row-space expansion: rows are dense u32s — visited set is a bitmap, adjacency is row_offsets/col_indices slice walks, NodeKey materialization happens once per RESULT row instead of per edge probe. - TRUE parallel levels: CsrStorage is Send+Sync, so frontier chunks expand on worker threads (thread::scope) against a shared AtomicU64 visited bitmap (fetch_or test-and-set); level triggers at frontier ≥ 256, ≤ 8 workers. - Direction-optimizing BFS (Beamer α=14/β=24): when the frontier's out-edge count dwarfs the unexplored remainder, the level switches to bottom-up pull over the v3-2 IncomingIndex (each unvisited row scans its in-edges for a frontier parent; rows partitioned per worker). Outgoing direction only; the lazy incoming index is warmed outside the worker scope. Semantics parity with the reader path: same edge-validity bitmap, same edge-type filter, same deleted_lsn node visibility, same MergedNeighbor materialization for CSR edges (placeholder edge key, weight 1.0, segment-LSN timestamp). Within a parallel level, discovery ORDER is unspecified (visited set + depths deterministic). Tests (7): push parity vs a hand-rolled reader-path oracle across all 3 directions × filters; pull-vs-push equality (incl. edge-type filter); parallel-level parity on a 2000-node star; gate fallback (mutable tier non-empty / two segments / future segment); deleted-node exclusion parity; depth limit + frontier cap; synthetic-key NodeNotFound (a key from a fresh MemGraph would ALIAS frozen rows — the task-3 collision class — so the test uses an out-of-range synthetic key). Suites: lib 3811 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets. author: Tin Dang
…wPreFilter Phase 3 task 12 — the deep review's last two items. TraversalGuard threading (bounded epoch hold, wall-clock): - ExecutionContext gains `guard: Option<TraversalGuard>` (None default — Default/unit-test behavior unchanged) and ExecErrorKind gains Timeout(TraversalTimeout). - Both read executors (execute + execute_profile) check the guard once per hop in variable-length Expand BFS and once per row before each ShortestPath run; GRAPH.QUERY/RO_QUERY/PROFILE create the guard with the default 30s budget. The write executor is untouched: it has no ExecutionContext and walks the mutable tier only (hop-capped), so the epoch-hold risk the guard bounds — long traversals pinning old CSR snapshots — doesn't apply. Hybrid HnswPreFilter (was a silent stub — select_strategy picked it at >= 10K candidates, execute brute-forced anyway): - New src/graph/hnsw_bridge.rs: GraphHnsw builds the vector engine's HnswGraph (HnswBuilder, m=16/efC=128, deterministic seed) over a CSR segment's v5 embeddings with raw-f32 cosine distances — no TurboQuant. Embeddings copied once, unit-normalized (scores identical to simd::cosine_similarity). Search = greedy upper descent + L0 ef-beam (ef = max(64, 4k)), allow-filter applied to the collected beam. - Lazy per-segment build via CsrStorage::hnsw_bridge (OnceLock, same pattern as incoming/property indexes), gated at >= 4096 embeddings; the negative answer is cached. In-memory only, never persisted. - GraphFilteredSearch::execute now honors the strategy: CSR-resident candidates route through their segment's bridge (group top-k with exact cosines), everything else — mutable tier, bridge-less rows, dim mismatch, and any group whose approximate beam under-fills k — falls back to exact scoring. Never silently truncates. Tests: guard timeout red/green for var-length Expand + shortestPath (both executors) with generous-guard parity; bridge build/recall (>= 8/10 vs brute force, exact-score parity), allow-filter, min-vectors gate, degenerate queries, unembedded-row skip; hybrid HnswPreFilter vs brute-force overlap + bit-exact fallback without a bridge + tier partition contract. Suites: lib 3821 green, graph_freeze_boundary 11/11, tokio cypher suites green, clippy both feature sets, fmt. author: Tin Dang
CHANGELOG entry covering the P0-P3 graph deep-review implementation (freeze-boundary correctness, point-query indexes, plan-cache + executor cost, row-space BFS + TraversalGuard + HNSW bridge), per the CI Lint changelog gate. author: Tin Dang
`slotmap::SlotMap` key allocation is deterministic: a fresh `MemGraph` always mints SlotMap index 0 / generation 1 first. CSR segments persist `NodeMeta::external_id` as the pre-crash process's raw NodeKey FFI bits. `recover_graph_store` previously re-seeded the post-recovery mutable tier with another fresh, unoffset `MemGraph`, and `replay.rs`'s AddNode replay loop called `mg.add_node()` unconditionally for every replayed record. The first brand-new node replayed after a restart could therefore mint the exact same key as a loaded CSR segment's first frozen node -- `MergedNodeView` checks the mutable tier first, so the frozen node's properties would be silently and permanently shadowed by the replayed node's properties. Fix is two-part: - `MemGraph::with_id_offset` translates every public NodeKey by a fixed offset at the mutable/internal boundary. `recover_graph_store` seeds the post-recovery mutable tier with a watermark one past the largest persisted `external_id` across all loaded segments, so freshly-minted keys can never numerically alias a loaded segment's rows. Zero extra memory (no dummy insert/remove cycle, which would both leak an ever-growing SlotMap backing Vec and remain unsound against arbitrarily bumped pre-crash generations). - `replay.rs`'s AddNode loop now seeds its `node_map` from every loaded CSR segment's node_meta before replaying, and skips `mg.add_node()` for any `node_id` already resident there -- handling full-history replay (AOF fold / WAL truncation left a pre-freeze record in place) without double-enumerating the same logical node. Also updates `MergedNodeView`'s doc comment (src/graph/view.rs) to describe how the "NodeKey lives in exactly one tier" invariant is actively enforced across restart, not merely automatic the way it is for in-process freeze/thaw. Added tests/graph_restart_id_aliasing.rs (red/green, confirmed failing against the pre-fix code via git stash before this commit): asserts no replayed key aliases a loaded segment's external_id, that a frozen node's properties are never shadowed by a replayed node, and that replaying an already-frozen node's record doesn't double-enumerate it. Known follow-up (not in scope here, documented for a separate fix): `NamedGraph.write_buf` -- not `NamedGraph.segments.mutable` -- is the tier every live command dispatch/read path actually consults. Nothing currently reconciles `segments.mutable` (which recovery/replay populate) back into `write_buf`, so today a restart does not expose replayed post-freeze WAL mutations to live queries at all. This fix protects the `segments`-based tier exactly as specified and matches replay.rs's own pre-existing test contract; the write_buf/segments reconciliation gap is a distinct, likely more severe issue requiring its own design pass. author: Tin Dang
`normalize_cypher()` -> `parameterize()` ran unconditionally before the plan-cache lookup on every GRAPH.QUERY -- a full lexer pass plus allocations even for an exact-repeat query -- and `SlotTable::from_plan` was rebuilt from scratch on every execution regardless of cache hit. `PlanCache` now supports dual-keyed inserts: a raw query-bytes hash (exact-repeat fast path, checked first, skipping `parameterize()` entirely on hit and replaying that text's own auto-extracted params) and the pre-existing literal-normalized hash (shared across every query that differs only in literal values), both pointing at one shared `Arc<PhysicalPlan>` + `Arc<SlotTable>` so a hit never rebuilds the slot table. `graph_query_readonly`/`graph_query_or_write` check the raw hash first, fall back to the normalized hash on raw miss, and only compile + `insert_both()` on a full miss. Cache storage switches from `std::HashMap` to the in-repo `FxHashMap` (keyed by an already-xxhashed u64 digest -- never raw attacker bytes -- and bounded by `max_entries` LRU eviction, so the documented "not DoS-resistant" caveat on `FxHashMap` does not apply here). `PlanCache::len()` now reports raw hash-key-slot count (what `max_entries`/LRU actually bounds); added `distinct_plan_count()` for "how many distinct compiled plans are cached" -- the metric the existing literal-sharing correctness tests in src/command/graph/mod.rs actually want, now that a single distinct plan can occupy two hash-key slots. New unit tests: exact-repeat raw-hash hit, string/int literal-variant sharing via the normalized hash, same-hash dedup when a query has no literals to extract, and LRU eviction correctness under dual keys. author: Tin Dang
…ytes `CsrStorage::resident_bytes()` fed the shard's elastic memory budget but omitted three real allocations: the v5 `node_props`/`edge_props` blobs, the lazily-built `SegmentPropertyIndexes`, and the lazily-built `GraphHnsw` bridge -- all invisible to eviction accounting. It also double-counted the mmap-backed `row_offsets`/`col_indices`/`edge_meta`/ `node_meta` arrays for `CsrStorage::Mmap` segments as if they were heap allocations, even though the doc comment already claimed otherwise. Rewritten to follow the `RawF16Store` precedent (PR #232): mmap-backed sections are kernel page cache (reclaimable under memory pressure) and count as 0 for the `Mmap` variant; the same sections count in full for the `Heap` variant. `props_index`/`hnsw_bridge` are ALWAYS heap-owned once built (derived structures over the source data, never themselves mapped) and count in full for both variants -- read via `OnceLock::get` only, so `resident_bytes()` stays a cheap read that never triggers a lazy build. New tests in src/graph/csr/storage.rs: resident_bytes grows once after `property_index()` is built and again after the HNSW bridge is built (fixture carries real per-node properties and embeddings so both indexes have non-trivial content, not an accidental 0 -> 0 pass); a second test pins the Heap-variant invariant that the core CSR arrays count in full. author: Tin Dang
Adds the three review-gated fixes (restart NodeKey aliasing P0, plan- cache raw-hash fast path, CSR resident_bytes accounting) to the graph engine overhaul entry under [Unreleased]. author: Tin Dang
TinDang97
force-pushed
the
feat/graph-engine-overhaul
branch
from
July 7, 2026 05:38
4fc5190 to
206eaf5
Compare
Contributor
Author
Pre-merge deep review (2 review agents + manual verification)Branch rebased onto main and extended with 3 review-gated fix commits (
Validation: 475 graph tests green (lib + Follow-ups (documented, intentionally not gating this merge)
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graph/replay.rs (1)
539-556: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd edge replay should skip already-applied frozen edges
AddEdgereplay ignoresedge_idand always callsadd_edge_across_tiers, which inserts a fresh edge. If an already-frozen edge is left in WAL history, replay can duplicate it and inflate neighbor/traversal results. Add an edge-id dedup check or make the insert path idempotent for replay.🤖 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 `@src/graph/replay.rs` around lines 539 - 556, The AddEdge replay path in replay.rs always inserts through add_edge_across_tiers and ignores edge_id, which can duplicate already-frozen edges during WAL replay. Update the AddEdge handling in the replay loop to detect and skip edges that have already been applied, using edge_id-based deduplication or by making add_edge_across_tiers idempotent for replay. Keep the change localized around the replay logic that increments replayed after a successful insert.
🤖 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.
Outside diff comments:
In `@src/graph/replay.rs`:
- Around line 539-556: The AddEdge replay path in replay.rs always inserts
through add_edge_across_tiers and ignores edge_id, which can duplicate
already-frozen edges during WAL replay. Update the AddEdge handling in the
replay loop to detect and skip edges that have already been applied, using
edge_id-based deduplication or by making add_edge_across_tiers idempotent for
replay. Keep the change localized around the replay logic that increments
replayed after a successful insert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 083ad10e-b25c-4d4b-84b2-28cccab6dd0d
📒 Files selected for processing (37)
.github/workflows/fuzz.yml.gitignoreCHANGELOG.mdfuzz/Cargo.tomlsrc/command/graph/graph_read.rssrc/command/graph/graph_write.rssrc/command/graph/mod.rssrc/graph/compaction.rssrc/graph/csr/mmap.rssrc/graph/csr/mod.rssrc/graph/csr/props.rssrc/graph/csr/storage.rssrc/graph/cypher/executor/eval.rssrc/graph/cypher/executor/mod.rssrc/graph/cypher/executor/read.rssrc/graph/cypher/executor/write.rssrc/graph/cypher/mod.rssrc/graph/cypher/parameterize.rssrc/graph/cypher/planner.rssrc/graph/fasthash.rssrc/graph/hnsw_bridge.rssrc/graph/hybrid.rssrc/graph/index.rssrc/graph/memgraph.rssrc/graph/mod.rssrc/graph/recovery.rssrc/graph/replay.rssrc/graph/row_bfs.rssrc/graph/store.rssrc/graph/traversal.rssrc/graph/traversal_guard.rssrc/graph/types.rssrc/graph/view.rssrc/graph/visibility.rssrc/graph/wal.rstests/graph_freeze_boundary.rstests/graph_restart_id_aliasing.rs
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/fuzz.yml
- src/graph/cypher/mod.rs
- .gitignore
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (28)
- fuzz/Cargo.toml
- src/graph/mod.rs
- src/graph/traversal_guard.rs
- src/graph/fasthash.rs
- src/graph/types.rs
- src/graph/row_bfs.rs
- src/graph/visibility.rs
- src/graph/hnsw_bridge.rs
- src/graph/wal.rs
- src/graph/index.rs
- src/graph/store.rs
- src/graph/csr/props.rs
- src/graph/cypher/executor/eval.rs
- tests/graph_freeze_boundary.rs
- src/graph/cypher/executor/mod.rs
- src/graph/csr/mmap.rs
- src/graph/cypher/parameterize.rs
- src/command/graph/mod.rs
- src/graph/view.rs
- src/graph/traversal.rs
- src/command/graph/graph_write.rs
- src/graph/memgraph.rs
- src/command/graph/graph_read.rs
- src/graph/cypher/executor/write.rs
- src/graph/compaction.rs
- src/graph/csr/mod.rs
- src/graph/hybrid.rs
- src/graph/cypher/executor/read.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Graph engine overhaul — deep-review fixes + optimization waves (P0–P3)
Implements the full roadmap from
tmp/GRAPH-DEEP-REVIEW-2026-07.md(12 commits, red/green TDD throughout).P0 — freeze-boundary correctness (data loss fixes)
tests/graph_freeze_boundary.rs(11 tests) pins the mutable→frozen lifecycle; every P0 fix turned one red.P1 — point-query indexes
OnceLock, built at first use).PhysicalOp::IndexScanin planner + executor:MATCH (n:L {p: v})narrows instead of label-scanning.P2 — query-path cost
$__pNparams re-bound per run) + LRU eviction (1024/graph). A cache hit runs with ZERO parse/compile. Single-parse dispatch (token-scan write detection).SmallVecslots against a per-planSlotTable.FxHashfor slotmap-keyed sets; Dijkstra 3-maps→1; allocation-freeneighbors_intoin both executors' Expand.P3 — traversal engine
src/graph/row_bfs.rs): on fully-frozen graphs — dense AtomicU64 visited bitmap, TRUE parallel frontier levels (thread::scopeover Send+SyncCsrStorage, ≤8 workers), direction-optimizing Beamer push/pull (α=14/β=24) over the incoming index. Gate: anything the reader path handles generically falls back — never a behavior fork. Parity-tested against a hand-rolled reader-path oracle.ExecErrorKind::Timeout);Noneguard = old behavior.src/graph/hnsw_bridge.rs): was a silent stub (strategy selected, brute force always ran). Now a lazy per-segment HNSW (vector engine'sHnswBuilder, raw-f32 cosine, ≥4096 embeddings gate) with exact-scoring fallback whenever the approximate beam under-fills — never silently truncates.⚠ Requires approval: 4 new
unsafeexpressionssrc/graph/csr/mmap.rs(commit 67e80aa, CSR v5 property blobs): twobase.add(ppos)pointer derivations + twoslice::from_raw_partsaccessors for the node/edge property blobs — the exact pattern of the existing v3 mmap array accessors, bounds validated at load, SAFETY comments present,scripts/audit-unsafe.shgreen.File-size cap note
csr/mod.rs(2104) andtraversal.rs(1815) exceed 1500 raw lines but are under 1000 lines of non-test code (tests stay inmod.rsper convention).graph_read.rs(1789) is a command-group file already read/write-split. All three were over the raw cap on main before this branch; left unsplit to keep the review diff clean.Verification
graph_freeze_boundary11/11; full default + tokio suites green (both runtimes).shard::scatter_aggregate::tests::test_scatter_text_aggregate_single_shard_skips_spscunderruntime-tokio,jemalloc,graph,text-index— "ShardSlice not initialized on this thread". CI's tokio job runs withoutgraph,text-indexso it never sees this; not touched by this branch.-D warningsboth feature sets;cargo fmt --check; unsafe audit green.graph_bench+graph_traversalvs main baseline):graph_addedge_command−23%;bfs_memgraph/bfs_csr2-hop expansions −1.5–2%; everything else statistically unchanged (BFS 10k-depth3 both variants, NEIGHBORS, freeze, cosine kernels).graph_addnode_commandreads +18% (~9 ns on a 60 ns loop) butMemGraph::add_nodeis byte-identical on this branch and the rawgraph_edge_insertbench (same file) is unchanged — thin-LTO code-layout shift, not an algorithmic regression. (A first, contended run of the same pair read −43%; the bench is layout/noise-sensitive.)Summary by CodeRabbit
GRAPH.NEIGHBORSand more reliable timeout enforcement for long-running paths.