Skip to content

feat: impl StructArray -- support diskann index #45223

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: impl StructArray -- support diskann index #45223

wants to merge 3 commits into from
+274 −21

Conversation

@SpadeA-Tang
Copy link
Contributor

issue: #42148
@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: SpadeA-Tang
To complete the pull request process, please assign czs007 after the PR has been reviewed.
You can assign the PR to them by writing /assign @czs007 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Nov 3, 2025
@mergify mergify bot added dco-passed DCO check passed. kind/feature Issues related to feature request from users labels Nov 3, 2025
@sre-ci-robot
Copy link
Contributor

[ci-v2-notice]
Notice: We are gradually rolling out the new ci-v2 system.

  • Legacy CI jobs remain unaffected, you can just ignore ci-v2 if you don't want to run it.
  • Additional "ci-v2/*" checkers will run for this PR to ensure the new ci-v2 system is working as expected.
  • For tests that exist in both v1 and v2, passing in either system is considered PASS.

To rerun ci-v2 checks, comment with:

  • /ci-rerun-code-check // for ci-v2/code-check
  • /ci-rerun-build // for ci-v2/build
  • /ci-rerun-ut-integration // for ci-v2/ut-integration
  • /ci-rerun-ut-go // for ci-v2/ut-go
  • /ci-rerun-ut-cpp // for ci-v2/ut-cpp
  • /ci-rerun-ut // for all ci-v2/ut-integration, ci-v2/ut-go, ci-v2/ut-cpp
  • /ci-rerun-e2e-arm // for ci-v2/e2e-arm

If you have any questions or requests, please contact @zhikunyao.
@codecov
Copy link

codecov bot commented Nov 3, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 7.76699% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.82%. Comparing base (b045efc) to head (d7d2c05).
⚠️ Report is 25 commits behind head on master.

Files with missing lines Patch % Lines
internal/core/src/storage/DiskFileManagerImpl.cpp 0.00% 44 Missing ⚠️
internal/core/src/index/IndexFactory.cpp 18.75% 26 Missing ⚠️
internal/core/src/index/VectorDiskIndex.cpp 7.40% 25 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #45223      +/-   ##
==========================================
- Coverage   76.91%   76.82%   -0.09%     
==========================================
  Files        1844     1844              
  Lines      287704   287797      +93     
==========================================
- Hits       221280   221096     -184     
- Misses      59216    59475     +259     
- Partials     7208     7226      +18
Components Coverage Δ
Client 78.03% <ø> (ø)
Core 83.65% <7.76%> (-0.10%) ⬇️
Go 74.95% <ø> (-0.10%) ⬇️
Files with missing lines Coverage Δ
internal/core/src/index/VectorDiskIndex.h 20.00% <ø> (ø)
internal/core/src/storage/DiskFileManagerImpl.h 66.66% <ø> (ø)
...l/core/unittest/test_storage_v2_index_raw_data.cpp 100.00% <ø> (ø)
internal/core/src/index/VectorDiskIndex.cpp 66.98% <7.40%> (-8.56%) ⬇️
internal/core/src/index/IndexFactory.cpp 83.33% <18.75%> (-5.23%) ⬇️
internal/core/src/storage/DiskFileManagerImpl.cpp 61.87% <0.00%> (-5.11%) ⬇️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
zhengbuqian
zhengbuqian reviewed Nov 3, 2025
View reviewed changes
internal/core/src/storage/DiskFileManagerImpl.cpp Outdated
local_data_path, write_offset, &dim, sizeof(dim));

// Write offsets file for VECTOR_ARRAY
if (is_vector_array && offsets.size() > 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we handle the error case of is_vector_array && offsets.size() == 1? is it possible for this to happen?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

offsets size equals to num_rows + 1, so it should not happen
internal/core/src/index/VectorDiskIndex.cpp

auto segment_id = file_manager_->GetFieldDataMeta().segment_id;
auto local_data_path = file_manager_->CacheRawDataToDisk<T>(config);
auto field_id = file_manager_->GetFieldDataMeta().field_id;
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add some unit tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opensource knowhere is not supported for this. I have tested end-to-end with cardinal, and can run normally.
@SpadeA-Tang
impl diskann index
f64e435 
Signed-off-by: SpadeA-Tang <tangchenjie1210@gmail.com>
@SpadeA-Tang SpadeA-Tang mentioned this pull request Nov 3, 2025
Open
@SpadeA-Tang
polish
9378a7f 
Signed-off-by: SpadeA <tangchenjie1210@gmail.com>
@SpadeA-Tang
fix test
d7d2c05 
Signed-off-by: SpadeA-Tang <tangchenjie1210@gmail.com>
@mergify mergify bot added the ci-passed label Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/compilation ci-passed dco-passed DCO check passed. kind/feature Issues related to feature request from users size/L Denotes a PR that changes 100-499 lines.

3 participants

@SpadeA-Tang @sre-ci-robot @zhengbuqian