HNSW Index: Add graph dump tool

[villagedeb]

Add -g/--graph to svector_page_dump: render the HNSW graph from a level-0 root page as ASCII tree or adjacency-list art, walking every level via NID links without needing a root page number per level.

[tomas-villagesql]

My Claude review:

VillageSQL Self-Review — PR #65 "HNSW Index: Add graph dump tool"

Scope note (topology): The PR metadata says base deb/index/5 (#56's head), but the commit graph shows its single commit 5e4cea5 actually branches from deb/index/4 (#55's head) — it does not contain #56. A two-dot deb/index/5..deb/index/6 diff therefore spuriously shows a graph.cc (+24/−14) change that is really "#56's fix being reverted," not part of this PR. I reviewed the actual commit 5e4cea5^..5e4cea5, which touches only the 7 tool files (no graph.cc). Two things worth flagging to you:

1. Merging HNSW Index: Add graph dump tool #65 as-is onto deb/index/5 would revert HNSW Index: Replace neighbours #56's commit-ordering fix. Since I assessed HNSW Index: Replace neighbours #56 as a likely durability regression, dropping it may be intentional/fine — but confirm whether HNSW Index: Replace neighbours #56 is meant to survive in the stack, and rebase HNSW Index: Add graph dump tool #65 onto the intended base so the diff is clean.
2. The PR is co-authored by Claude Sonnet 5 (per the trailer) — noted, no bearing on the review.

This is diagnostic-tool code (svector_page_dump), not the runtime .so, so crash/UB matters less than parse correctness, but I applied the same rigor.

Deletion audit — clean

Nearly pure-additive. The two edited lines in svector_page_dump.cc are both necessary: extending the constructor init list, and hoisting HnswIndexMetadata index_meta out of the if (is_index_) block so --graph can use it. No logic dropped.

Correctness — verified

• hnsw_neighbour_column_size is the exact algebraic inverse of hnsw_max_neighbours (fixed + max_n*NODE_SIZE), matching the real NeighbourEntry::storage_size I verified in HNSW Index: Update tool for index page #55.
• Per-level degree handled correctly. The genuine risk here was the level-0 (2·M) vs upper-level (M) degree difference. fetch_record uses level_max_neighbours(level, M) = level==0 ? 2*M : M to compute each level's column_size — correct. M is derived from level-0's column_size (max_n0/2) with a defensive max_n0 % 2 != 0 → refuse guard. (The header comment "column_size determined by M alone" is slightly loose — it's M and level — but the code is right; minor comment-precision nit.)
• Cycle-safe. Both Tree (visited.count → "...") and List (visited.insert().second gates enqueue) prevent infinite loops on HNSW's cyclic/bidirectional graph. visited is correctly per-level (a node legitimately recurs across levels).
• Bounded. budget (= max_nodes) is threaded across all levels and checked in both walkers, capping total page reads — important for a possibly-corrupt on-disk graph.
• Bounds-safe fetch. fetch_record checks slot_index >= records.size() and is_free before indexing, and relies on the HNSW Index: Update tool for index page #55-verified bounds-safe DataPageParser::parse. A bad NID prints an error line and returns false — no crash.
• Descent follows the entry node's lower_level_nid (incoming bit masked off), captured only from each level's root/first node — mirrors search_knn. Only forward (non-incoming) edges are drawn, correctly excluding incoming slots and the overflow chain.
• Wiring: --graph correctly requires -i + decodable level-0 metadata (guarded, errors cleanly), renders before -d/-a, and leaves existing flags intact. Arg parsing for -g/--max-nodes/--graph-style has proper missing-arg checks.

Findings

None blocking. Two minor notes:

1. --max-nodes uses bare std::stoul without try/catch (optional consistency nit). --max-nodes foo throws an uncaught exception (tool aborts) rather than printing a clean error, whereas -r's parsing wraps std::stoul in try/catch. However, -d and the positional root_page_num_ already use bare std::stoul pre-PR, so --max-nodes follows the file's dominant existing pattern and -r is the outlier. Not introduced-worse by this PR; a file-wide "wrap all std::stoul" cleanup would be nice but is out of scope.
2. Header comment altitude (trivial). hnsw_graph.h says each level's column_size is "determined by M alone" — it's determined by M and level (2·M at level 0). The code handles it correctly; just tighten the sentence.

Otherwise clean

Duplicated-constant discipline continues from #55 (all layout constants still match the real sources, and the new hnsw_neighbour_column_size inverts the verified formula). Includes are correct and minimal. README is thorough with realistic sample output. No line-refs / Phase / TODO / section-separator anti-patterns. uint16_t cast in hnsw_neighbour_column_size can't overflow for real (round-tripped) M.

Verdict: Approve on substance — a correct, cycle-safe, well-bounded, well-documented graph visualizer. Before merge, sort out the base/topology (#1 in the scope note): decide whether #56 stays in the stack and rebase #65 onto the intended base so it doesn't silently revert #56. The two code notes are optional polish.