eliminate the effect of data size on the efsearch threshold#993
Merged
Conversation
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
inabao
added
the
version/0.16
Contributor
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
Reviewer's GuideThis PR refactors the search methods to enforce the ef_search threshold consistently at the start of each KnnSearch call—using max(AMPLIFICATION_FACTOR * k, 1000)—removes duplicated threshold checks in HGraph, and aligns the k validation and lock ordering in HNSW::knn_search to match the new pattern. Sequence diagram for consistent ef_search threshold validation in KnnSearchsequenceDiagram
participant User
participant HGraph
participant HGraphSearchParameters
User->>HGraph: KnnSearch(query, k, parameters, filter)
HGraph->>HGraphSearchParameters: FromJson(parameters)
HGraph->>HGraph: Compute ef_search_threshold = max(AMPLIFICATION_FACTOR * k, 1000)
HGraph->>HGraph: CHECK_ARGUMENT(1 <= params.ef_search <= ef_search_threshold)
HGraph->>HGraph: CHECK_ARGUMENT(k > 0)
HGraph->>HGraph: k = min(k, GetNumElements())
Note right of HGraph: Proceed with search logic
Sequence diagram for reordered validation and locking in HNSW::knn_searchsequenceDiagram
participant User
participant HNSW
participant HnswSearchParameters
User->>HNSW: knn_search(query, k, parameters, ...)
HNSW->>HnswSearchParameters: FromJson(parameters)
HNSW->>HNSW: Compute ef_search_threshold = max(AMPLIFICATION_FACTOR * k, 1000)
HNSW->>HNSW: CHECK_ARGUMENT(1 <= params.ef_search <= ef_search_threshold)
HNSW->>HNSW: CHECK_ARGUMENT(k > 0)
HNSW->>HNSW: k = min(k, GetNumElements())
HNSW->>HNSW: std::shared_lock lock_global(rw_mutex_)
Note right of HNSW: Continue with search logic
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Collaborator
Author
|
@sourcery-ai summary |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @inabao - I've reviewed your changes - here's some feedback:
- The repeated ef_search threshold calculation and validation across multiple overloads should be refactored into a shared helper to reduce duplication and ensure consistency.
- Consider validating and capping k before computing ef_search_threshold so the threshold logic always uses the final k value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated ef_search threshold calculation and validation across multiple overloads should be refactored into a shared helper to reduce duplication and ensure consistency.
- Consider validating and capping k before computing ef_search_threshold so the threshold logic always uses the final k value.
## Individual Comments
### Comment 1
<location> `src/algorithm/hgraph.cpp:303` </location>
<code_context>
+
+ auto params = HGraphSearchParameters::FromJson(parameters);
+
+ auto ef_search_threshold = std::max(AMPLIFICATION_FACTOR * k, 1000L);
+ CHECK_ARGUMENT( // NOLINT
+ (1 <= params.ef_search) and (params.ef_search <= ef_search_threshold),
+ fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold));
</code_context>
<issue_to_address>
The ef_search_threshold calculation uses k before it is clamped to the number of elements.
Since k is clamped after ef_search_threshold is calculated, ef_search_threshold may be set too high if k exceeds the dataset size. Consider basing ef_search_threshold on the clamped k value for more accurate validation.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
auto ef_search_threshold = std::max(AMPLIFICATION_FACTOR * k, 1000L);
CHECK_ARGUMENT( // NOLINT
(1 <= params.ef_search) and (params.ef_search <= ef_search_threshold),
fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold));
// check k
CHECK_ARGUMENT(k > 0, fmt::format("k({}) must be greater than 0", k));
k = std::min(k, GetNumElements());
=======
// check k
CHECK_ARGUMENT(k > 0, fmt::format("k({}) must be greater than 0", k));
k = std::min(k, GetNumElements());
auto ef_search_threshold = std::max(AMPLIFICATION_FACTOR * k, 1000L);
CHECK_ARGUMENT( // NOLINT
(1 <= params.ef_search) and (params.ef_search <= ef_search_threshold),
fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold));
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+303
to
310
| auto ef_search_threshold = std::max(AMPLIFICATION_FACTOR * k, 1000L); | ||
| CHECK_ARGUMENT( // NOLINT | ||
| (1 <= params.ef_search) and (params.ef_search <= ef_search_threshold), | ||
| fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold)); | ||
|
|
||
| // check k | ||
| CHECK_ARGUMENT(k > 0, fmt::format("k({}) must be greater than 0", k)); | ||
| k = std::min(k, GetNumElements()); |
There was a problem hiding this comment.
suggestion (bug_risk): The ef_search_threshold calculation uses k before it is clamped to the number of elements.
Since k is clamped after ef_search_threshold is calculated, ef_search_threshold may be set too high if k exceeds the dataset size. Consider basing ef_search_threshold on the clamped k value for more accurate validation.
Suggested change
| auto ef_search_threshold = std::max(AMPLIFICATION_FACTOR * k, 1000L); | |
| CHECK_ARGUMENT( // NOLINT | |
| (1 <= params.ef_search) and (params.ef_search <= ef_search_threshold), | |
| fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold)); | |
| // check k | |
| CHECK_ARGUMENT(k > 0, fmt::format("k({}) must be greater than 0", k)); | |
| k = std::min(k, GetNumElements()); | |
| // check k | |
| CHECK_ARGUMENT(k > 0, fmt::format("k({}) must be greater than 0", k)); | |
| k = std::min(k, GetNumElements()); | |
| auto ef_search_threshold = std::max(AMPLIFICATION_FACTOR * k, 1000L); | |
| CHECK_ARGUMENT( // NOLINT | |
| (1 <= params.ef_search) and (params.ef_search <= ef_search_threshold), | |
| fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold)); |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #993 +/- ##
==========================================
- Coverage 91.92% 91.69% -0.23%
==========================================
Files 293 293
Lines 17020 17022 +2
==========================================
- Hits 15645 15609 -36
- Misses 1375 1413 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
inabao
added
the
kind/improvement
LHT129
reviewed
Jul 31, 2025
| (1 <= params.ef_search) and (params.ef_search <= ef_search_threshold), | ||
| fmt::format("ef_search({}) must in range[1, {}]", params.ef_search, ef_search_threshold)); | ||
|
|
||
| // check k |
Collaborator
There was a problem hiding this comment.
what will happen, when ef_search larger than num_element?
Collaborator
Author
There was a problem hiding this comment.
The function will return all vectors if the graph is connected; otherwise, it will return a connected component.
ShawnShawnYou
approved these changes
Aug 1, 2025
Contributor
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
LHT129
pushed a commit
to LHT129/vsag
that referenced
this pull request
Apr 16, 2026
…#993) Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
LHT129
pushed a commit
that referenced
this pull request
May 11, 2026
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com> Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
Sia-Sheerland
pushed a commit
to Sia-Sheerland/vsag
that referenced
this pull request
Jun 26, 2026
…#993) Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com> Signed-off-by: Sia Sheerland <x1075956441x@163.com> Signed-off-by: Sia Sheerland <x1075956441x@163.com>
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
4 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.
Summary by Sourcery
Enforce a consistent ef_search threshold across graph and HNSW searches and consolidate parameter parsing to eliminate code duplication and the impact of data size on the threshold.
Enhancements: