fix(index): align HNSW construction with reference

[lance-gatekeeper]

What is the bug?

Several HNSW construction paths diverged from Algorithms 1–4 and hnswlib: reciprocal candidates could be rejected before diversity pruning, new level-0 nodes used the reciprocal Mmax0 limit, offline construction forced node 0 to full height, persisted level boundaries omitted the entry point, and SEARCH-LAYER reused a stale furthest bound. Invalid construction parameters also reached undefined algorithm states.

After correcting the new-node limit to M, Algorithm 4 selection could leave the directed graph under-filled and split into components. Current-head CI exposed this as stochastic HNSW-PQ recall between 0.902 and 0.987 when a high-budget query should reach essentially every indexed row. Truncating persisted levels to the sampled height also broke version-1 readers that index the configured level count directly.

How does this fix it?

• Add reciprocal candidates before applying the diversity heuristic, and use M for new nodes while retaining Mmax0 for reciprocal level-0 capacity in both builders.
• Use Algorithm 4 keepPrunedConnections to refill rejected candidates up to the existing M limit, reducing graph fragmentation without restoring the old 2*M new-node degree.
• Randomly assign every node level, use the first globally highest node as the offline entry point, and retain online promotion semantics.
• Keep corrected level offsets aligned with serialized rows while padding empty trailing ranges to the configured level count for version-1 reader compatibility. Current readers derive runtime height from non-empty levels.
• Apply shared construction-parameter validation through OnlineHnswBuilder::try_with_capacity, deprecate the infallible compatibility constructor, and reject M below 4 because smaller values produce severely fragmented parallel builds.
• Refresh the SEARCH-LAYER result bound per neighbor and document the selected Algorithm 4 options and low-setting quality trade-off.
• Add structural, offline/online parity, loaded-index compatibility, parameter-validation, non-entry-point search, minimum-setting reachability, and recall regressions.

Scope

This PR repairs the core algorithm deviations. The refreshed tracker evidence shows a remaining production recall deficit at low ef and large partitions, but it is a scale baseline rather than evidence for another construction deviation or a safe default change. Benchmark completion and the #5183 distance-computation regression remain follow-up work, so #8036 remains open for that work.

Validation

• cargo test -p lance-index --lib (943 passed, 2 ignored)
• cargo test -p lance-index --lib vector::hnsw (39 passed)
• minimum-setting HNSW reachability regression repeated 20 times
• HNSW-PQ null-case recall reproducer repeated 10 times (20 test cases passed)
• cargo test -p lance-index --doc try_with_capacity (1 passed)
• cargo clippy --all --tests --benches -- -D warnings
• cargo fmt --all -- --check
• git diff --check

Refs #8036

[lance-gatekeeper]

❌ Gate recommendation: request changes.

Keep the sampled hierarchy, but preserve the version-1 persisted-index contract: either serialize the configured level-boundary count (empty trailing levels are enough) or version this incompatible shape so older readers ignore it. Apply the same parameter validation at both offline and online construction boundaries.

[lance-gatekeeper]

This truncates version-1 metadata to the sampled height while HnswMetadata.params.max_level remains at the configured value. Prior version-1 readers derive the search height from that field and directly index the loaded per-level vectors, so querying a head-written index panics whenever the sampled height is lower. The HNSW vector-index version is unchanged, so those readers will not ignore the file.

Keep the configured number of serialized offsets (zero-length trailing ranges are sufficient, while current readers can derive the runtime height from nonempty levels), or bump the applicable vector-index format version.

Reproducer run on this head

I temporarily restored the base reader behavior:

fn max_level(&self) -> u16 {
    self.params.max_level
}

Then a unit test built, serialized, loaded, and queried one vector with default max_level = 7:

cargo test -p lance-index --lib gate_reproducer_current_writer_legacy_reader_missing_levels -- --nocapture

Expected one result; observed index out of bounds: the len is 1 but the index is 6 in LoadedHnswGraph::neighbors_at.

[lance-gatekeeper]

Addressed in cbf7075 and verified on current head 01534bb. Version-1 metadata now retains the configured level-boundary shape with empty trailing ranges, while current readers derive runtime height from non-empty levels; the loaded compatibility regression covers this path.

[lance-gatekeeper]

The new validation is only invoked by HNSW::index_vectors; the public OnlineHnswBuilder::with_capacity still accepts the same invalid shared parameters. With m = 0, its prune path selects zero neighbors for both nodes and silently produces a disconnected graph. Make this validation available to the online builder and enforce it through a fallible online construction boundary.

Reproducer run on this head

let params = HnswBuildParams::default().max_level(1).num_edges(0);
let builder = OnlineHnswBuilder::with_capacity(2, params);
builder.insert(0, &storage);
builder.insert(1, &storage);
assert!(
    !builder.nodes[0].bottom_neighbors.load().is_empty(),
    "a two-node graph must not silently discard every connection"
);

cargo test -p lance-index --lib gate_reproducer_online_builder_accepts_zero_m -- --nocapture

The assertion failed because node 0 had no connection.

[lance-gatekeeper]

Addressed in cbf7075 and verified on current head 01534bb. OnlineHnswBuilder::try_with_capacity now enforces the shared validation, and the compatibility constructor delegates through that fallible boundary; online invalid-parameter cases cover the behavior.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[lance-gatekeeper]

⚠️ Gate recommendation: approve with a non-blocking risk.

The follow-up resolves both earlier blockers: version-1 readers retain the configured level-boundary shape, and online/offline builders now share parameter validation. The reference-aligned refill is worthwhile, but its connectivity claim is stronger than the behavior at the lowest accepted construction settings. Treat those settings as an explicit recall-quality trade-off with coverage, or raise the validation floor if high-budget reachability is part of the contract.

[lance-gatekeeper]

Refilling rejected candidates does not prevent directed components for every parameter set that validate() accepts, so this comment overstates the guarantee. With m = 2 and ef_construction = 2, reciprocal pruning can still discard all incoming links to a new node from the entry-reachable component; a full-budget search reached only 121 of 2048 nodes.

If full-budget reachability is required, preserve at least one reciprocal link into every inserted node (or repair components after construction). Otherwise, document the low-M recall trade-off and add a regression floor, or reject parameters below that floor.

Reproducer run on this head

use rand::Rng;

#[test]
fn gate_reproducer_bottom_level_reachability() {
    const DIM: usize = 32;
    const TOTAL: usize = 2048;
    let mut rng = SmallRng::seed_from_u64(0);
    let values = Float32Array::from_iter_values(
        (0..TOTAL * DIM).map(|_| rng.random::<f32>()),
    );
    let vectors = FixedSizeListArray::try_new_from_values(values, DIM as i32).unwrap();
    let store = FlatFloatStorage::new(vectors.clone(), DistanceType::L2);
    let hnsw = HNSW::index_vectors(
        &store,
        HnswBuildParams::default().num_edges(2).ef_construction(2),
    )
    .unwrap();
    let results = hnsw.search_basic(
        vectors.value(0),
        TOTAL,
        &HnswQueryParams {
            ef: TOTAL,
            lower_bound: None,
            upper_bound: None,
            dist_q_c: 0.0,
            use_acorn: false,
        },
        None,
        &store,
    ).unwrap();
    assert_eq!(results.len(), TOTAL);
}

cargo test -p lance-index --lib gate_reproducer_bottom_level_reachability -- --nocapture

Expected 2048 results; observed 121.

[lance-gatekeeper]

Addressed in 01534bb. The Algorithm 4 documentation no longer claims universal connectivity, shared validation rejects m below 4, and a seeded minimum-setting regression requires at least 90% full-budget reachability.

[lance-gatekeeper]

Addressed in 01534bb. The minimum supported M is now 4, the documentation states the remaining low-setting quality trade-off without promising universal reachability, and the seeded minimum-setting regression enforces at least 90% full-budget reachability.

[lance-gatekeeper]

✅ Gate recommendation: approve.

The new quality-floor contract resolves the remaining risk: both construction paths reject m < 4, the selector documentation accurately scopes reachability, and the lowest accepted setting has a direct regression. Central validation is preferable to connectivity repair here because it preserves the reference degree semantics without adding a new graph-repair contract.

Please mark this PR with the breaking-change label.

[u70b3]

As the author of the tracker (#8036): this is a thorough piece of work, and I want to acknowledge that before the notes.

Every classified difference from the tracker is addressed, in both builders: reciprocal candidates now join the complete old-plus-new set before Algorithm 4 runs, the new-node M vs. reciprocal Mmax0 distinction is in place with per-role degree tests, offline construction adopts the online/paper level model (first-globally-highest is provably the fixpoint of sequential strict promotion — the parity tests pinning both builders to the same seeded level sequence are exactly the right coverage), the SEARCH-LAYER bound refresh matches Algorithm 2 at O(1) per neighbor, and validation now guards both construction boundaries with the stale default-value docs corrected. Making the keepPrunedConnections refill atomic with the new-node M correction is the right call — the CI recall evidence (0.902–0.987 without refill) shows the degree correction alone would have fragmented the graph.

The persisted-format handling also deserves credit: padding level_offsets to the configured level count keeps the version-1 reader contract while current readers derive runtime height from non-empty levels, and the legacy misaligned-load regression is preserved. One accuracy note for the changelog: the counting half of #5156 was already fixed on main by #8167; what this PR removes is the root cause (the forced full-height entry point), which is the more important half.

A few gaps against the tracker's completion criteria — none blocking this PR, but they should land somewhere:

1. Please don't auto-close bug: track HNSW deviations from the reference algorithm #8036. This PR says "Fixes bug: track HNSW deviations from the reference algorithm #8036", but the tracker's completion criteria include benchmark evidence — recall, latency, build time, and degree distribution before/after, on SIFT and clustered data — which is not reported here. This matters concretely because the refill heuristic will saturate level-0 reciprocal lists at 2M far more often, so index size and build time should move measurably. I'd rather the tracker stay open (or this become "Refs bug: track HNSW deviations from the reference algorithm #8036") until the benchmark item is resolved; I'm happy to own that follow-up.

2. Potential Bug: HNSW beam_search incorrectly uses a static furthest variable #5183 still lacks its regression test. The bound refresh is correct, but the tracker asked for a distance-computation regression to lock it in, and I don't see one here. The counter from fix(index): report distance comparisons from HNSW search #8142 would make that test nearly free once it lands — flagging so it isn't forgotten when this closes.

3. Breaking-change surface. Rejecting m < 4 and ef_construction < m converts previously accepted (silently degraded) settings into hard errors. The gate review already asked for the breaking-change label — +1 from me, and per repo convention the title could carry !. The m >= 4 floor is stricter than the m > 1 precondition I proposed in the tracker; I think it's the right engineering call given the fragmentation evidence, but it deserves to be visible in release notes.

4. Scope note, not this PR's job: the mem_wal memtable carries a third, independent HNSW construction (rust/lance/src/dataset/mem_wal/hnsw/graph.rs) with the same family of deviations. Filed bug: align mem_wal HNSW construction with the reference algorithm #8237 to track aligning it — I'm taking that one myself.

Nice work. This closes the core of the tracker, and the structural tests are the ones the audit asked for.

[lance-gatekeeper]

No code change was made: the requested breaking-change label is now applied and verified on current head 01534bb.

[Xuanwo]

Please don't auto-close #8036.

@u70b3 Please create a new issue as a follow-up. If you intend to maintain a tracking issue, please don't mark it as a bug.