feat: unify distance-by-ID API naming with legacy compatibility by wxyucs · Pull Request #2532 · antgroup/vsag

wxyucs · 2026-07-24T08:01:33Z

Summary

Unify the canonical batch Calculate Distances by IDs spelling as CalcDistancesById for raw-pointer and DatasetPtr queries.
Keep the historical CalDistanceById spelling as a deprecated, source-compatible alias through the 1.1 migration window.
Apply the naming consistently across the public API, built-in indexes, bindings, tests, examples, and bilingual documentation.

Compatibility

The public canonical Index::CalcDistancesById overloads default-forward to the legacy virtual Index::CalDistanceById overloads. This preserves existing third-party implementations that override only the historical spelling: calls through the new API reach their legacy overrides.

Built-in concrete indexes use CalcDistancesById as the canonical implementation entry point, while their CalDistanceById wrappers forward to CalcDistancesById. The public/index and internal bridges therefore route old and new entry points through the appropriate shared implementation without recursive dispatch: legacy-only third-party overrides use the new-to-old bridge, and built-in implementations use the old-to-new wrapper.

Verification

Final TSAN investigation before the follow-up passed Build with TSAN and Run TSAN Tests on the prior commit.
Post-push rerun for commit 3ec15a1b is live: PR CI run 30087602121 and TSAN run 30087602050 are still in progress; Detect Changes and Format Check have passed.
Post-push PR Issue Link Check run 30087622023, DCO, Mergify protections, and CircleCI path filtering have passed. Compatibility workflow run 30087601337 concluded failure without a job log exposed by GitHub at verification time.
Existing local verification: debug build passed; focused distance and legacy-only dispatch tests passed; direct CMake Python binding build passed; targeted clang-tidy-15 passed; the full test build passed before interruption in unrelated long-running cases; Python wheel build was blocked by missing python3.10-venv; Node binding build was unavailable because node-addon-api is not installed.

Fixes: #2068

vsag-bot · 2026-07-24T08:01:37Z

/label S-waiting-on-review
/waiting-on reviewer
/request-review @jiaweizone
/request-review @inabao

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

vsag-bot · 2026-07-24T08:01:45Z

Automated pull request review completed.

Review effort: high (442 changed lines across 34 files).

Submitted 2 inline comments.
Review: #2532 (review)

mergify · 2026-07-24T08:02:12Z

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Require kind label

label~=^kind/

🟢 Require version label

label~=^version/

LHT129

Review Summary

Reviewed 33 changed files across the CalcDistancesById batch API addition and CalDistanceById deprecation.

Design: The backward-compatibility strategy is well-designed. The base Index class CalcDistancesById defaults to forwarding to the deprecated CalDistanceById virtual, so third-party implementations that only override the legacy API continue to work. Concrete implementations (HGraph, IVF, Pyramid, SINDI, DiskANN, HNSW) override CalcDistancesById with real logic and make CalDistanceById forward to it, avoiding recursion. IndexImpl bridges the two correctly.

Correctness: No critical issues found. The virtual dispatch chains are correct — no infinite recursion, no missed overrides. The [[deprecated]] attribute on CalDistanceById in the public Index API properly warns callers while the internal forwarding uses pragma guards.

Test coverage: Good — new LegacyBatchIndex test in test_simple_index.cpp verifies dispatch to a legacy-only override; test_index_calc.cpp adds corrected/legacy equivalence checks with pragma guards; Python tests verify calc_distances_by_id vs cal_distance_by_id equivalence.

Bindings: Python (calc_distances_by_id + cal_distance_by_id alias), Node (calcDistancesById + calDistanceById alias), and C API all updated correctly.

Documentation: English and Chinese docs updated consistently with clear migration notes.

Issues found: 1 note-level (see inline comment on include/vsag/index.h).

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

vsag-bot

Automated inline review completed.

Review effort: high (442 changed lines across 34 files).
Submitted 1 inline comment.

Reviewed commit 81dc2a2.

vsag-bot · 2026-07-24T08:52:32Z

+     * third-party Index implementations remain source-compatible.
+     */
+    [[nodiscard]] virtual tl::expected<DatasetPtr, Error>
+    CalcDistancesById(const DatasetPtr& query,


[P1] Append the new virtual methods after existing vtable slots

These two virtual overloads are inserted before GetMinAndMaxId, shifting that method and every subsequent Index vtable slot. An existing application or plugin compiled against the previous v1.x header can therefore call a later virtual method on an object built by this version and dispatch to a CalcDistancesById slot with an incompatible signature, causing undefined behavior or a crash. Place both new virtual declarations after all pre-existing virtual methods (including the destructor), leaving every legacy slot at its original offset, or explicitly introduce a new ABI version.

Judged independently: this ABI concern remains valid in the final diff. The new public virtual overloads are still declared before existing virtual slots, so I’m leaving the thread unresolved rather than claiming the ABI layout issue is fixed.

Correction to my previous reply: this ABI concern remains valid because the new public virtual overloads are still declared before existing Index virtual slots. I am leaving the thread unresolved rather than claiming the ABI layout issue is fixed.

LHT129

Code review for PR #2532: feat: add CalcDistancesById batch API

Overall this is a well-structured rename with good backward-compatibility design. The new CalcDistancesById virtual methods are added alongside deprecated CalDistanceById wrappers, and the default implementations cross-dispatch so existing third-party overrides remain compatible.

Two issues noted below.

LHT129 · 2026-07-24T09:17:50Z

+        tl::expected<DatasetPtr, vsag::Error> legacy_result;
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"


[note] The deprecation warning suppression in this test file only covers GCC/Clang. On MSVC, calling the deprecated CalDistanceById will emit C4996. Consider adding the MSVC counterpart for consistency with the pattern used in include/vsag/index.h.

This remains a valid portability note: the current test fixture still only suppresses GCC/Clang deprecation diagnostics. I have not changed that fixture in this follow-up, so I’m leaving this thread unresolved rather than claiming it is handled.

Correction to my previous reply: this remains a valid portability note because the current test fixture still only suppresses GCC and Clang deprecation diagnostics. I have not changed that fixture, so I am leaving this thread unresolved.

Unify the canonical batch distance-by-ID API spelling while preserving the historical source-compatible entry point during the migration window. Signed-off-by: Xiangyu Wang <wxy407827@antgroup.com> Assisted-by: OpenAI Codex:GPT-5

vsag-bot

Automated inline review completed.

Review effort: high (442 changed lines across 34 files).
Submitted 2 inline comments.

Reviewed commit 3ec15a1.

vsag-bot · 2026-07-24T11:00:12Z

+     * third-party Index implementations remain source-compatible.
+     */
+    [[nodiscard]] virtual tl::expected<DatasetPtr, Error>
+    CalcDistancesById(const DatasetPtr& query,


[P1] Preserve the existing Index vtable layout

These new virtual overloads are inserted before GetMinAndMaxId and all subsequent virtual methods. An application or plugin compiled against the existing v1 header but loading this library will therefore dispatch later calls to slots with incompatible signatures, causing undefined behavior or crashes. Append new slots only after every pre-existing virtual entry, including the destructor, or use a nonvirtual forwarding API/bump the ABI version.

Judged independently: this is valid and duplicates the existing unresolved ABI concern. The new public virtual overloads still precede existing Index virtual entries, so I am leaving this thread unresolved.

vsag-bot · 2026-07-24T11:00:12Z

                               InstanceMethod("getMemoryUsage", &VsagIndex::GetMemoryUsage),
                               InstanceMethod("checkIdExist", &VsagIndex::CheckIdExist),
                               InstanceMethod("getMinMaxId", &VsagIndex::GetMinMaxId),
+                               InstanceMethod("calcDistancesById", &VsagIndex::CalcDistancesById),


[P2] Expose the corrected API through the TypeScript package

The npm package exports the wrapper generated from typescript/src/index.ts, but that Index class still exposes only calDistanceById; it has no calcDistancesById forwarding method or declaration. Consequently normal TypeScript consumers cannot call the newly registered native method. Add the corrected wrapper method while retaining the legacy alias.

Judged independently: this is valid. The native Node binding exposes calcDistancesById, but typescript/src/index.ts still declares only calDistanceById, so the corrected API is not available to normal TypeScript consumers. I am leaving this thread unresolved.

wxyucs requested a review from LHT129 as a code owner July 24, 2026 08:01

Copilot AI review requested due to automatic review settings July 24, 2026 08:01

wxyucs requested review from inabao and jiaweizone as code owners July 24, 2026 08:01

pull-request-size Bot added the size/L label Jul 24, 2026

vsag-bot assigned wxyucs Jul 24, 2026

Copilot AI reviewed Jul 24, 2026

vsag-bot self-requested a review July 24, 2026 08:01

mergify Bot added module/docs module/example module/api module/index module/testing labels Jul 24, 2026

wxyucs added kind/improvement Optimizations, UX polish, or minor improvements 性能优化、体验打磨或细节改良 version/1.1 labels Jul 24, 2026

LHT129 reviewed Jul 24, 2026

View reviewed changes

Comment thread include/vsag/index.h

wxyucs force-pushed the codex/issue-2068-calc-distances-api branch from 24bc16c to 574c6e9 Compare July 24, 2026 08:26

Copilot AI review requested due to automatic review settings July 24, 2026 08:27

wxyucs force-pushed the codex/issue-2068-calc-distances-api branch from 574c6e9 to 81dc2a2 Compare July 24, 2026 08:27

Copilot AI reviewed Jul 24, 2026

vsag-bot reviewed Jul 24, 2026

View reviewed changes

LHT129 reviewed Jul 24, 2026

View reviewed changes

Comment thread include/vsag/index.h

Comment thread src/index/hnsw.h

LHT129 reviewed Jul 24, 2026

View reviewed changes

wxyucs force-pushed the codex/issue-2068-calc-distances-api branch from 81dc2a2 to 3ec15a1 Compare July 24, 2026 10:51

vsag-bot self-requested a review July 24, 2026 10:51

wxyucs changed the title ~~feat: add CalcDistancesById batch API~~ feat: unify distance-by-ID API naming with legacy compatibility Jul 24, 2026

vsag-bot reviewed Jul 24, 2026

View reviewed changes

Uh oh!

Conversation

wxyucs commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Compatibility

Verification

Uh oh!

vsag-bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

vsag-bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Protections

🟢 Require kind label

🟢 Require version label

Uh oh!

LHT129 left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

vsag-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHT129 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsag-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wxyucs commented Jul 24, 2026 •

edited

Loading

vsag-bot commented Jul 24, 2026 •

edited

Loading

vsag-bot commented Jul 24, 2026 •

edited

Loading

mergify Bot commented Jul 24, 2026 •

edited

Loading