Skip to content
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

refactor the code for index parameter parsing #126

Merged
merged 16 commits into from
Nov 28, 2024
Merged

Conversation

inabao
Copy link
Collaborator

@inabao inabao commented Nov 11, 2024

No description provided.

jinjiabao.jjb added 3 commits November 12, 2024 00:00
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
@inabao inabao self-assigned this Nov 11, 2024
@inabao inabao added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Nov 11, 2024
jinjiabao.jjb added 3 commits November 12, 2024 00:35
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
} else {
common_param.allocator_ = DefaultAllocator::Instance().get();
if (allocator == nullptr) {
index_common_params.allocator_ = DefaultAllocator::Instance().get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

the allocator_ can change to shared_ptr ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The allocator_ may be passed in as a raw pointer from outside.

CMakeLists.txt Outdated Show resolved Hide resolved
src/factory/factory.cpp Outdated Show resolved Hide resolved
src/factory/factory.cpp Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
src/index/index_commom_param_test.cpp Show resolved Hide resolved
src/index/hnsw.cpp Show resolved Hide resolved
src/index/hnsw.cpp Outdated Show resolved Hide resolved
src/index/hgraph_zparameters.h Outdated Show resolved Hide resolved
src/index/hgraph_zparameters.h Outdated Show resolved Hide resolved
src/index/diskann_zparameters.h Show resolved Hide resolved
jinjiabao.jjb added 3 commits November 21, 2024 22:43
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
src/index/diskann.cpp Outdated Show resolved Hide resolved
src/index/diskann_zparameters.h Show resolved Hide resolved
@@ -24,10 +24,10 @@
namespace vsag {
class HGraphParameters {
public:
explicit HGraphParameters(const IndexCommonParam& common_param, const std::string& str = "");
explicit HGraphParameters(JsonType& hgraph_param, const IndexCommonParam& common_param);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep a consistent params sequence style:
explicit HGraphParams(const IndexCommonParam& common_param, JsonType& hgraph_param)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have refactored all the construct function of index to the style like HGraphParameters(JsonType& hgraph_param, const IndexCommonParam& common_param); to keep the same style with data_cell

Copy link
Collaborator

Choose a reason for hiding this comment

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

miss commit ?

jinjiabao.jjb added 3 commits November 25, 2024 21:52
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Signed-off-by: jinjiabao.jjb <jinjiabao.jjb@antgroup.com>
Copy link
Collaborator

@jiaweizone jiaweizone left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -31,6 +32,6 @@ class IndexCommonParam {
Allocator* allocator_{nullptr};
Copy link
Collaborator

Choose a reason for hiding this comment

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

public fields do not need to end with _
change in next PR

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

@inabao inabao merged commit 8d96099 into main Nov 28, 2024
2 checks passed
@inabao inabao deleted the refactor_param_parser branch November 28, 2024 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants