Skip to content

Search to add a separate allocator #716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 4, 2025
Merged

Conversation

Carrot-77
Copy link
Collaborator

No description provided.

Copy link

codecov bot commented May 16, 2025

Codecov Report

Attention: Patch coverage is 87.34177% with 10 lines in your changes missing coverage. Please review.

❌ Your project status has failed because the head coverage (88.73%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage.

@@            Coverage Diff             @@
##             main     #716      +/-   ##
==========================================
- Coverage   88.78%   88.73%   -0.05%     
==========================================
  Files         198      199       +1     
  Lines       12650    12693      +43     
==========================================
+ Hits        11231    11263      +32     
- Misses       1419     1430      +11     
Flag Coverage Δ
cpp 88.73% <87.34%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
common 92.53% <ø> (+0.01%) ⬆️
datacell 91.51% <100.00%> (+<0.01%) ⬆️
index 88.65% <83.33%> (-0.09%) ⬇️
simd 100.00% <ø> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69f889d...8baa2d1. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Carrot-77 Carrot-77 force-pushed the search_param branch 2 times, most recently from 1895dcc to c76bab7 Compare May 19, 2025 02:38
Copy link
Collaborator

@wxyucs wxyucs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@wxyucs wxyucs added version/0.15 kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) labels May 28, 2025
@wxyucs wxyucs self-assigned this May 28, 2025
@wxyucs wxyucs added kind/feature New feature or request and removed kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) labels May 28, 2025
Copy link
Collaborator

@LHT129 LHT129 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -234,6 +235,7 @@ HNSW::knn_search(const DatasetPtr& query,
ret->Dim(0)->NumElements(1);
return ret;
}
vsag::Allocator* search_alloctor = allocator == nullptr ? allocator_.get() : allocator;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: search_allocator

@@ -35,7 +35,8 @@ class SparseVectorDataCell : public FlattenInterface {
Query(float* result_dists,
const ComputerInterfacePtr& computer,
const InnerIdType* idx,
InnerIdType id_count) override {
InnerIdType id_count,
Allocator* allocator = nullptr) override {
auto comp = std::static_pointer_cast<Computer<QuantTmpl>>(computer);
this->query(result_dists, comp, idx, id_count);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wxyucs Here need add param allocator to use custom allocator ?

this->query(result_dists, comp, idx, id_count, allocator);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Search allocator is not used inside query

Signed-off-by: zourunxin.zrx <[email protected]>
@wxyucs wxyucs merged commit 6da48a2 into antgroup:main Jun 4, 2025
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants