refactor(hgraph): centralize regular search routing

[LHT129]

Summary

• Centralize regular HGraph route selection in a private SearchPlan selector.
• Preserve brute-force priority, MCI eligibility, route statistics, and bitset empty-seed fallback behavior.
• Add coverage for brute-force precedence and range search selecting the bottom graph.

Closes #2605

[vsag-bot]

/label status/waiting-for-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao

[vsag-bot]

Automated pull request review completed.

Review effort: high (756 changed lines across 3 files).

Submitted 1 inline comment.
Review: #2606 (review)

[mergify]

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

• label~=^kind/

🟢 Require version label

• label~=^version/

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Centralizes HGraph “regular” search route selection into a single internal selector while preserving existing routing priorities (brute-force vs MCI vs bottom-graph) and adding regression coverage for route selection.

Changes:

• Added an internal SearchPlan selector (select_search_plan) and filter seed-source helpers to consolidate routing logic.
• Refactored HGraph::SearchWithRequest to route via switch on the selected plan (brute-force / MCI / bottom-graph).
• Added tests to confirm brute-force precedence and that range search selects the bottom graph route.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/algorithm/hgraph/hgraph_search.cpp	Introduces centralized plan selection + helper functions and refactors routing in SearchWithRequest.
src/algorithm/hgraph/hgraph_mci_test.cpp	Adds coverage for brute-force precedence and range-search routing outcome.

[LHT129]

Thanks for this refactoring. The extraction of SearchPlan, SearchPlanInput, and the helper functions into a dedicated decision module makes the routing logic much clearer and easier to test.

Summary of changes reviewed:

• src/algorithm/hgraph/hgraph_search.cpp: +108/-16 — extracted SearchPlan enum, SearchPlanInput struct, and helpers (is_mci_available, select_search_plan, has_valid_id_source, has_bitset_source) into an anonymous namespace; replaced the inline if-else routing with a switch on select_search_plan.
• src/algorithm/hgraph/hgraph_mci_test.cpp: +28/-0 — added 3 test cases covering brute-force precedence (KNN), MCI routing, and range search selecting the bottom graph.

What looks good:

• The select_search_plan function now takes mci_available as a parameter, avoiding the redundant is_mci_available call noted earlier.
• The SearchPlanInput construction uses positional aggregate init (no C++20 designated initializers), maintaining C++17 compatibility.
• has_bitset_source includes an explicit nullptr guard, consistent with has_valid_id_source.
• All three routing paths (brute force, MCI, bottom graph) are covered by tests.
• The [[fallthrough]] from kMCI to kBottomGraph correctly handles the case where try_mci_search fails to produce an MCI result.
• The mci_result.route is correctly set to "hgraph" only when MCI was available but the bottom graph was chosen; it stays as "disabled" when MCI was never a candidate — preserving the existing statistics semantics.

One minor note posted as an inline comment about the fragility of positional aggregate initialization with 10 fields.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/algorithm/hgraph/hgraph_search.cpp:88

• has_valid_id_source() / has_bitset_source() (and related MCI eligibility checks) are now duplicated between hgraph_search.cpp and hgraph_mci.cpp (see src/algorithm/hgraph/hgraph_mci.cpp:77-92). This raises drift risk if future MCI eligibility semantics change in one place but not the other, which is exactly what this refactor is trying to avoid. Consider extracting these helpers into a shared internal header (e.g., src/algorithm/hgraph/hgraph_mci_utils.h) and including it from both translation units so there is a single source of truth for seed-source detection.

bool
has_valid_id_source(const FilterPtr& filter) {
    if (filter == nullptr) {
        return false;
    }
    const int64_t* valid_ids = nullptr;
    int64_t valid_count = 0;
    filter->GetValidIds(&valid_ids, valid_count);
    return valid_ids != nullptr and valid_count > 0;
}

bool
has_bitset_source(const FilterPtr& filter) {
    if (filter == nullptr) {
        return false;
    }
    const auto bitset_filter = std::dynamic_pointer_cast<BlackListFilter>(filter);
    return bitset_filter != nullptr and bitset_filter->IsBitsetFilter();
}

[LHT129]

This revision addresses all previously raised comments:

• Switched from C++20 designated initializers to positional aggregate initialization for SearchPlanInput.
• Extracted is_mci_available result into a local variable to avoid redundant calls.
• Added explicit nullptr guard in has_bitset_source.
• Added test coverage for all three routing paths (MCI, brute-force, bottom graph).

The refactoring cleanly centralizes search routing logic into select_search_plan with a clear switch-based dispatch. No new issues found.

[LHT129]

[suggestion] configure_bottom_graph_search dereferences ctx->stats at line 602 (ctx->stats->is_timeout.store(...)) without any null check. While the current sole caller (SearchWithRequest) guarantees ctx->stats is non-null, this function is a public static method and could be called from other contexts in the future.

Consider either:

1. Adding an assertion like assert(ctx->stats != nullptr) before the dereference, or
2. Guarding with if (ctx->stats != nullptr) before the is_timeout.store call.

[LHT129]

[suggestion] search_result is passed by value to pack_search_result at line 971-972, but it is a local variable in SearchWithRequest that is not used after this call. Passing with std::move(search_result) would avoid an unnecessary reference count increment/decrement on the shared pointer.

// Current:
return this->pack_search_result(
    request, k, search_result, ctx, mci_result, stats, reasoning_ctx);

// Suggested:
return this->pack_search_result(
    request, k, std::move(search_result), ctx, mci_result, stats, reasoning_ctx);

[LHT129]

This refactoring cleanly decomposes SearchWithRequest into well-named helper methods, making the search pipeline much easier to follow. The SearchPlan selector centralizes routing logic that was previously scattered.

I left three inline comments covering minor points:

1. A dead-code guard in search_route_graphs that can never trigger in the current call path.
2. A potential null-pointer dereference of ctx->stats in configure_bottom_graph_search when called from future contexts.
3. A missed std::move opportunity when passing search_result to pack_search_result.

None of these are blocking — the logic is correct and the test coverage for brute-force precedence and range search routing is solid.

[LHT129]

[note] The QueryContext* ctx parameter in configure_bottom_graph_search is declared but never used in the function body (see hgraph_search.cpp implementation). If it was added for future extensibility, consider adding a comment explaining its intended purpose. Otherwise, it can be removed to keep the interface minimal.

[LHT129]

Code Review for PR #2606

[suggestion] select_search_plan called twice in SearchWithRequest

In hgraph_search.cpp, select_search_plan is called twice (lines 877-878 and 898). The first call with mci_available=false serves as a pre-check to determine whether brute force should be used regardless of MCI eligibility, so that MCI availability computation can be skipped when brute force is already selected.

This double-call pattern makes the control flow harder to follow. Consider computing MCI availability unconditionally first (the has_bitset_source / has_valid_id_source calls are cheap), then calling select_search_plan once with the real mci_available value. This would eliminate the pre-check and simplify the logic.

// Compute MCI availability unconditionally, then call select_search_plan once
const auto bitset_seed_source = has_bitset_source(request.filter_);
bool valid_id_seed_source = false;
if (params.use_mci and this->mci_parameters_.enabled and search_param.executors.empty()) {
    valid_id_seed_source = has_valid_id_source(request.filter_);
    if (valid_id_seed_source or bitset_seed_source) {
        plan_input.mci_has_clique_index = ...;
    }
}
plan_input.has_valid_id_source = valid_id_seed_source;
plan_input.has_bitset_source = bitset_seed_source;
const bool mci_available = is_mci_available(plan_input);
switch (select_search_plan(plan_input, mci_available)) { ... }

[note] Brute force KNN search now receives request.threshold_

At line 905, the brute force KNN search call now passes request.threshold_ as an additional parameter:

search_result = this->brute_force_search<InnerSearchMode::KNN_SEARCH>(
    raw_query, ft, k, 0.0F, &ctx, request.threshold_);

The original code did not pass a threshold to brute force KNN search. This is a behavioral change that enables threshold-based filtering in the brute force path. Please confirm this is intentional and not an accidental inclusion from the refactoring.

[note] Redundant INVALID_ENTRY_POINT check in search_route_graphs

search_route_graphs (line 549) checks search_param.ep == INVALID_ENTRY_POINT and returns early. However, the caller SearchWithRequest already validates entry_point == INVALID_ENTRY_POINT at line 819 and returns before calling search_route_graphs. This defensive check is harmless but redundant.

Summary

The refactoring is well-structured and significantly improves readability by extracting clear sub-functions (create_query_context, search_route_graphs, configure_bottom_graph_search, pack_search_result, parse_and_validate_search_params, initialize_reasoning_context) from the monolithic SearchWithRequest. The VisitedListGuard RAII wrapper is a nice addition. The test coverage for brute-force precedence and range search routing is good.

The main suggestion is simplifying the double select_search_plan call pattern. The brute force threshold parameter addition should be confirmed as intentional.

[LHT129]

test review body

[LHT129]

Short test

[LHT129]

test

[LHT129]

test

[LHT129]

Code Review for PR #2606

[suggestion] select_search_plan called twice in SearchWithRequest

In hgraph_search.cpp, select_search_plan is called twice (lines 877-878 and 898). The first call with mci_available=false serves as a pre-check to determine whether brute force should be used regardless of MCI eligibility, so that MCI availability computation can be skipped when brute force is already selected.

This double-call pattern makes the control flow harder to follow. Consider computing MCI availability unconditionally first (the has_bitset_source / has_valid_id_source calls are cheap), then calling select_search_plan once with the real mci_available value. This would eliminate the pre-check and simplify the logic.

// Compute MCI availability unconditionally, then call select_search_plan once
const auto bitset_seed_source = has_bitset_source(request.filter_);
bool valid_id_seed_source = false;
if (params.use_mci and this->mci_parameters_.enabled and search_param.executors.empty()) {
    valid_id_seed_source = has_valid_id_source(request.filter_);
    if (valid_id_seed_source or bitset_seed_source) {
        plan_input.mci_has_clique_index = ...;
    }
}
plan_input.has_valid_id_source = valid_id_seed_source;
plan_input.has_bitset_source = bitset_seed_source;
const bool mci_available = is_mci_available(plan_input);
switch (select_search_plan(plan_input, mci_available)) { ... }

[note] Brute force KNN search now receives request.threshold_

At line 905, the brute force KNN search call now passes request.threshold_ as an additional parameter:

search_result = this->brute_force_search<InnerSearchMode::KNN_SEARCH>(
    raw_query, ft, k, 0.0F, &ctx, request.threshold_);

The original code did not pass a threshold to brute force KNN search. This is a behavioral change that enables threshold-based filtering in the brute force path. Please confirm this is intentional and not an accidental inclusion from the refactoring.

[note] Redundant INVALID_ENTRY_POINT check in search_route_graphs

search_route_graphs (line 549) checks search_param.ep == INVALID_ENTRY_POINT and returns early. However, the caller SearchWithRequest already validates entry_point == INVALID_ENTRY_POINT at line 819 and returns before calling search_route_graphs. This defensive check is harmless but redundant.

Summary

The refactoring is well-structured and significantly improves readability by extracting clear sub-functions (create_query_context, search_route_graphs, configure_bottom_graph_search, pack_search_result, parse_and_validate_search_params, initialize_reasoning_context) from the monolithic SearchWithRequest. The VisitedListGuard RAII wrapper is a nice addition. The test coverage for brute-force precedence and range search routing is good.

The main suggestion is simplifying the double select_search_plan call pattern. The brute force threshold parameter addition should be confirmed as intentional.

[LHT129]

[note] search_route_graphs accepts const HGraphSearchParameters& params but never uses it in the function body. The parameter can be removed to simplify the signature.

[LHT129]

[note] configure_bottom_graph_search unconditionally dereferences ctx->stats (ctx->stats->bottom_graph_search_count++), but create_query_context may create a context with a null stats pointer when params->stats is null. If configure_bottom_graph_search is only ever called after a stats-capable path, consider adding a // stats is always non-null here comment, or add a null guard.

[LHT129]

[note] search_route_graphs accepts const HGraphSearchParameters& params but never uses it in the function body. The parameter can be removed to simplify the signature.

[LHT129]

[note] configure_bottom_graph_search unconditionally dereferences ctx->stats (ctx->stats->bottom_graph_search_count++), but create_query_context may create a context with a null stats pointer when params->stats is null. If configure_bottom_graph_search is only ever called after a stats-capable path, consider adding a // stats is always non-null here comment, or add a null guard.

[LHT129]

[suggestion] MCI seed-source detection (has_valid_id_source, has_bitset_source) operates on request.filter_, but the actual search filter and mci_result are constructed from ft (which may be a wrapped/augmented filter via create_search_filter). When params.use_extra_info_filter is true, ft can differ from request.filter_, causing the routing decision to be based on a different filter instance than the one used for MCI seed extraction and bottom-graph search.

Consider either:

1. Running has_valid_id_source / has_bitset_source on ft instead of request.filter_, or
2. Explicitly unwrapping ft to the original filter before seed-source detection, with a comment explaining why the unwrapped filter is correct for seed purposes.

[LHT129]

[note] configure_bottom_graph_search is declared static in the header (hgraph.h:838) and defined as a static member function, but it mutates ctx->stats->is_timeout (line 576 in the .cpp). While technically valid since ctx is passed as a parameter, the static qualifier is semantically misleading — it suggests the method does not depend on or mutate instance state, yet the stats object it modifies is typically owned by the calling SearchWithRequest instance flow.

Consider removing static and making it a regular const member function. This would make the dependency on ctx (which carries instance-derived state) more explicit and consistent with other helper methods like search_route_graphs and create_query_context which are non-static const members.

[LHT129]

test comment

[LHT129]

[suggestion] configure_bottom_graph_search dereferences ctx->stats at line 602 without a null check on ctx. While the single call site in SearchWithRequest always passes a valid pointer (&ctx where ctx is a stack variable), this is a static method that could be called from other contexts in the future. Consider adding a null guard for defensive safety.

if (params.enable_time_record) {
    search_param.time_cost = std::make_shared<Timer>();
    search_param.time_cost->SetThreshold(params.timeout_ms);
    if (ctx != nullptr and ctx->stats != nullptr) {
        ctx->stats->is_timeout.store(false, std::memory_order_relaxed);
    }
}

[LHT129]

[suggestion] configure_bottom_graph_search dereferences ctx->stats without a null check on ctx. While the single call site in SearchWithRequest always passes a valid pointer (&ctx where ctx is a stack variable), this is a static method that could be called from other contexts in the future. Consider adding a null guard for defensive safety.

if (params.enable_time_record) {
    search_param.time_cost = std::make_shared<Timer>();
    search_param.time_cost->SetThreshold(params.timeout_ms);
    if (ctx != nullptr and ctx->stats != nullptr) {
        ctx->stats->is_timeout.store(false, std::memory_order_relaxed);
    }
}

[LHT129]

[suggestion] configure_bottom_graph_search dereferences ctx->stats without a null check on ctx. While the single call site in SearchWithRequest always passes a valid pointer (&ctx where ctx is a stack variable), this is a static method that could be called from other contexts in the future. Consider adding a null guard for defensive safety.

if (params.enable_time_record) {
    search_param.time_cost = std::make_shared<Timer>();
    search_param.time_cost->SetThreshold(params.timeout_ms);
    if (ctx != nullptr and ctx->stats != nullptr) {
        ctx->stats->is_timeout.store(false, std::memory_order_relaxed);
    }
}

[LHT129]

[suggestion] configure_bottom_graph_search dereferences ctx->stats without a null check on ctx. While the single call site in SearchWithRequest always passes a valid pointer (&ctx where ctx is a stack variable), this is a static method that could be called from other contexts in the future. Consider adding a null guard for defensive safety.

if (params.enable_time_record) {
    search_param.time_cost = std::make_shared<Timer>();
    search_param.time_cost->SetThreshold(params.timeout_ms);
    if (ctx != nullptr and ctx->stats != nullptr) {
        ctx->stats->is_timeout.store(false, std::memory_order_relaxed);
    }
}

[LHT129]

[suggestion] Returning std::move(dataset_results) prevents copy elision (NRVO). Since dataset_results is a local variable, the compiler can elide the copy/move entirely if returned without std::move. The explicit std::move forces a move construction, which is strictly worse than the guaranteed copy elision that NRVO provides.

// Current:
return std::move(dataset_results);

// Suggested:
return dataset_results;

[LHT129]

[suggestion] plan_input.mci_valid_ratio_threshold is set before the brute_force_route check (line 876), but mci_has_clique_index, has_valid_id_source, and has_bitset_source are only populated inside the if (not brute_force_route) block (lines 880-895). This means the first select_search_plan(plan_input, false) call on line 878 operates on a partially-populated plan_input where mci_has_clique_index, has_valid_id_source, and has_bitset_source still hold their default false values. While this is intentional (the first call only checks for brute-force routing which doesn't depend on those fields), it creates a subtle coupling between the population order and the select_search_plan logic. Consider either:

1. Populating all fields before the first select_search_plan call, or
2. Adding a comment noting that the first call intentionally uses a partially-populated struct.

[LHT129]

[note] The if (not use_custom_distance) block at line 868 now spans ~70 lines (868-930) containing the entire search plan selection and execution logic. The else branch (lines 931-938) is just a single search_one_graph call. Consider extracting the if block body into a dedicated private method (e.g., execute_search_plan) to keep SearchWithRequest at a consistent abstraction level, matching the extraction pattern already applied to search_route_graphs, configure_bottom_graph_search, etc.

[LHT129]

[suggestion] In initialize_reasoning_context (hgraph_search.cpp:788), the QueryContext* ctx parameter is passed to precise_flatten->Query, but ctx->reasoning_ctx is null during this call (it is set later in create_query_context). While this matches pre-existing behavior, the function signature implies broader QueryContext usage than is actually safe. Consider narrowing the parameter to SearchStatistics* for the distance counting at line 783, and constructing a minimal QueryContext locally for the Query call to make the limited contract explicit.

[LHT129]

Thanks for this clean refactoring of the HGraph search routing logic. The extraction of SearchWithRequest into well-named helper methods (create_query_context, search_route_graphs, configure_bottom_graph_search, pack_search_result, parse_and_validate_search_params, initialize_reasoning_context) significantly improves readability and maintainability.

A few observations from the review:

1. The VisitedListGuard RAII class is a nice improvement over the previous local struct — extracting it to file scope makes it reusable and testable.

2. The select_search_plan / is_mci_available split with the two-phase check (brute force first, then MCI eligibility) is a sensible optimization that avoids computing MCI clique index state when brute force would win anyway.

3. The early vt_guard.Release() before reorder (step 7) is a good performance improvement — returning the visited list to the pool early allows other concurrent searches to reuse it during the reorder phase.

4. The NaN-aware MCI threshold comparison (not(valid_ratio >= threshold)) with the explicit comment is the correct approach for preserving MCI attempt behavior with NaN ratios.

One suggestion (posted as a separate PR comment): the initialize_reasoning_context method signature takes QueryContext* ctx but ctx->reasoning_ctx is null during the call. Consider narrowing the parameter to reflect the actual contract.

The new tests in hgraph_mci_test.cpp provide good coverage for brute-force precedence, NaN ratio handling, and range search bottom-graph fallback. The refactoring preserves all existing behavior while making the search routing logic much easier to follow.

[LHT129]

[note] The search_plan enum and search_plan_input struct use snake_case naming, which differs from the project convention of PascalCase for types (e.g., InnerSearchMode, QueryContext, HGraphSearchParameters). The enum values also mix a K_ prefix with UPPER_SNAKE_CASE (K_BOTTOM_GRAPH), while existing enums like InnerSearchMode use plain UPPER_SNAKE_CASE (KNN_SEARCH).

Consider renaming for consistency:

enum class SearchPlan { kBottomGraph, kBruteForce, kMCI };
struct SearchPlanInput { ... };

or following the existing InnerSearchMode pattern:

enum class SearchPlan { BOTTOM_GRAPH, BRUTE_FORCE, MCI };

[LHT129]

Overall this is a well-executed refactoring that cleanly decomposes SearchWithRequest into focused, testable sub-methods. The routing logic extraction into select_search_plan / is_mci_available is sound, and the VisitedListGuard RAII wrapper is a nice improvement over the previous ad-hoc struct.

Summary of changes:

• Extracted parse_and_validate_search_params, create_query_context, initialize_reasoning_context, search_route_graphs, configure_bottom_graph_search, and pack_search_result from SearchWithRequest.
• Introduced search_plan / search_plan_input / select_search_plan to centralize brute-force/MCI/bottom-graph routing decisions.
• Added VisitedListGuard RAII class for visited list lifecycle management.
• Added test coverage for brute-force precedence, NaN ratio handling, and range search selecting bottom graph.
• Fixed size_t underflow risk in route_graphs_.size() - 1 by casting before subtraction.

Behavior preservation: The refactoring correctly preserves all existing behaviors including brute-force priority, MCI eligibility checks, route statistics annotation, bitset/valid-id seed source detection, and the NaN-ratio MCI fallback path. The two-phase select_search_plan call (first checking brute-force alone, then full routing) is a deliberate optimization to avoid computing MCI availability when brute-force already wins.

One minor style note was left inline regarding type naming conventions.

[LHT129]

[suggestion] The search_plan enum, search_plan_input struct, VisitedListGuard class, and the helper functions (is_mci_available, select_search_plan, has_valid_id_source, has_bitset_source) are defined at namespace vsag scope in this .cpp file, giving them external linkage. If another translation unit also defines symbols with these names in namespace vsag, it would cause an ODR violation.

The previous iteration of this PR used an anonymous namespace for these symbols. Consider either wrapping them back in an anonymous namespace or marking them static to give them internal linkage, since they are only used within this translation unit.

[LHT129]

[note] select_search_plan(plan_input, false) is called before plan_input.mci_has_clique_index, plan_input.has_valid_id_source, and plan_input.has_bitset_source are initialized (they are set later in the if (not brute_force_route) block).

This is currently safe because mci_available=false prevents select_search_plan from reading those fields, but it creates a fragile dependency on the internal implementation of select_search_plan. If someone later modifies select_search_plan to read those fields unconditionally (e.g., for logging or assertions), it would read uninitialized values.

Consider either initializing all plan_input fields before the first select_search_plan call, or adding a comment noting that the first call intentionally uses incomplete input and only checks brute-force eligibility.

[LHT129]

I have completed a thorough review of this PR (commit 0de14d4f). The refactoring cleanly extracts SearchWithRequest into focused, well-named helper methods, and the centralized select_search_plan / search_plan_input routing logic is a clear improvement over the previous inline conditionals.

Summary of findings:

The PR has already received extensive review feedback (30+ comments) covering the key areas: C++20 compatibility, MCI availability computation, NaN handling, filter mismatch between request.filter_ and ft, naming conventions, and test coverage. Most of the actionable issues have been addressed in this revision.

Remaining notes (already flagged in existing comments):

1. The search_plan / search_plan_input naming uses snake_case while the VSAG convention is PascalCase — noted as file-local types, low priority.
2. mci_result.route defaults to "disabled" and is only set to "hgraph" when mci_available is true — the author has confirmed this is intentional to preserve existing callback-only filter regression behavior.
3. MCI seed-source detection uses request.filter_ while mci_result is constructed with ft (the wrapped filter) — the comment on L881 explains this is intentional since MCI seeds use the original external-label filter.

Overall assessment: The refactoring is well-executed, preserves existing behavior, and adds meaningful test coverage for the routing decision paths. No blocking issues found.

[LHT129]

[suggestion] In search_route_graphs (hgraph_search.cpp:393), the params and use_custom_distance parameters are never read in the function body. These appear to be leftovers from the extraction — the original inline routing code did not use params either. Removing these unused parameters would simplify the signature.

Additionally, configure_bottom_graph_search is declared static in the header, which is correct since it only operates on its parameters. However, the function mutates ctx->stats->is_timeout — this is fine but worth noting that the "static" label is slightly misleading since the function has side effects through the ctx pointer.