feat: use tl::unexpected in Index default implementations by LHT129 · Pull Request #2141 · antgroup/vsag

LHT129 · 2026-06-04T06:40:31Z

Summary

Replace throw std::runtime_error with LOG_ERROR_AND_RETURNS in Index class virtual method default implementations
Provides consistent error handling via tl::expected<T, Error>

Fixes #2140

mergify · 2026-06-04T06:41:11Z

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Require kind label

Wonderful, this rule succeeded.

label~=^kind/

🟢 Require version label

Wonderful, this rule succeeded.

label~=^version/

🟢 Require linked issue for feature/bug PRs

Wonderful, this rule succeeded.

body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)

gemini-code-assist

Code Review

This pull request refactors include/vsag/index.h to return expected errors using the LOG_ERROR_AND_RETURNS macro instead of throwing std::runtime_error for unsupported index operations, alongside correcting some grammatical errors in the remaining exception messages. The review feedback highlights a garbled error message in the Tune method and identifies several formatting inconsistencies across multiple methods (such as Pretrain, CalcDistanceById, and GetRawVectorByIds), including cramped single-line function bodies, missing semicolons on macro calls, and redundant trailing semicolons.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-06-04T06:41:50Z

+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support Tune supported by this index")


The error message string contains a garbled phrase: "Index does not support Tune supported by this index". It should be simplified to "Index does not support Tune" for clarity. Also, please append a semicolon ; to the macro call to adhere to standard C++ statement formatting.

LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support Tune");

gemini-code-assist · 2026-06-04T06:41:50Z

+    Pretrain(const std::vector<int64_t>& base_tag_ids, uint32_t k, const std::string& parameters){
+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support pretrain")};


The formatting of this function is inconsistent and cramped. The opening brace { lacks a preceding space, the closing brace } is placed on the same line as the macro call, and there is a redundant trailing semicolon ; after the closing brace.

Please format it consistently with the rest of the class, and append a semicolon to the macro call.

Pretrain(const std::vector<int64_t>& base_tag_ids, uint32_t k, const std::string& parameters) { LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support pretrain"); }

gemini-code-assist · 2026-06-04T06:41:50Z

+    CalcDistanceById(const float* vector, int64_t id, bool calculate_precise_distance = true)
+        const {LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                                     "Index does not support get distance by id")};


The formatting of this function is cramped and inconsistent. The function body is placed on the same line as the opening brace, and there is a redundant trailing semicolon after the closing brace.

Please format it cleanly on multiple lines and append a semicolon to the macro call.

CalcDistanceById(const float* vector, int64_t id, bool calculate_precise_distance = true) const { LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support get distance by id"); }

gemini-code-assist · 2026-06-04T06:41:50Z

+    CalcDistanceById(const DatasetPtr& vector, int64_t id, bool calculate_precise_distance = true)
+        const {LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                                     "Index does not support get distance by id")};


The formatting of this function is cramped and inconsistent. Please format it cleanly on multiple lines and append a semicolon to the macro call.

CalcDistanceById(const DatasetPtr& vector, int64_t id, bool calculate_precise_distance = true) const { LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support get distance by id"); }

gemini-code-assist · 2026-06-04T06:41:50Z

+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support get distance by id")};


The closing brace and redundant trailing semicolon are placed on the same line as the macro call. Please format it cleanly on separate lines and append a semicolon to the macro call.

LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support get distance by id");

gemini-code-assist · 2026-06-04T06:41:50Z

+    GetRawVectorByIds(const int64_t* ids, int64_t count, Allocator* specified_allocator = nullptr)
+        const {LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                                     "Index does not support GetRawVectorByIds")};


The formatting of this function is cramped and inconsistent. Please format it cleanly on multiple lines and append a semicolon to the macro call.

GetRawVectorByIds(const int64_t* ids, int64_t count, Allocator* specified_allocator = nullptr) const { LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support GetRawVectorByIds"); }

Copilot

Pull request overview

This PR updates the vsag::Index base class’s default virtual method implementations to avoid throwing exceptions for APIs that already return tl::expected<T, Error>, instead returning tl::unexpected(Error) via LOG_ERROR_AND_RETURNS to standardize unsupported-operation handling (Fixes #2140).

Changes:

Replaced throw std::runtime_error(...) with LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION, ...) in many tl::expected<..., Error> default implementations.
Standardized several unsupported-operation messages to the “Index does not support …” form.
Adjusted behavior of at least one non-tl::expected method default (CheckFeature) in the base class.

    Tune(const std::string& parameters, bool disable_future_tuning = false) {
-        throw std::runtime_error("Tune is not supported by this index");
+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support Tune supported by this index")
    }


    virtual tl::expected<void, Error>
    Train(const DatasetPtr& data) {
-        throw std::runtime_error("Index not support Train");
+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support Train")
    }


+    Pretrain(const std::vector<int64_t>& base_tag_ids, uint32_t k, const std::string& parameters){
+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support pretrain")};


+             int64_t global_optimum_tag_id = std::numeric_limits<int64_t>::max()){
+        LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                              "Index does not support feedback")};


    [[nodiscard]] virtual tl::expected<float, Error>
-    CalcDistanceById(const float* vector,
-                     int64_t id,
-                     bool calculate_precise_distance = true) const {
-        throw std::runtime_error("Index doesn't support get distance by id");
-    };
+    CalcDistanceById(const float* vector, int64_t id, bool calculate_precise_distance = true)
+        const {LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                                     "Index does not support get distance by id")};


@@ -656,8 +666,8 @@ class Index {
     */
    [[nodiscard]] virtual tl::expected<std::vector<IndexDetailInfo>, Error>
    GetIndexDetailInfos() const {


@@ -672,8 +682,8 @@ class Index {
     */
    virtual tl::expected<DetailDataPtr, Error>
    GetDetailDataByName(const std::string& name, IndexDetailInfo& info) const {


@@ -692,8 +702,8 @@ class Index {
     */
    [[nodiscard]] virtual tl::expected<DatasetPtr, Error>


    [[nodiscard]] virtual bool
    CheckFeature(IndexFeature feature) const {
-        throw std::runtime_error("Index doesn't support check feature");
+        return false;
    }


    [[nodiscard]] virtual std::string
    GetStats() const {
-        throw std::runtime_error("Index not support GetStats");
+        throw std::runtime_error("Index does not support GetStats");
    }


Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

    virtual tl::expected<bool, Error>
    Tune(const std::string& parameters, bool disable_future_tuning = false) {
-        throw std::runtime_error("Tune is not supported by this index");
+        return tl::unexpected(
+            Error(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support Tune"));
    }


    [[nodiscard]] virtual tl::expected<DatasetPtr, Error>
    KnnSearch(const DatasetPtr& query,
              int64_t k,
              const std::string& parameters,
              const FilterPtr& filter) const {
-        throw std::runtime_error("Index doesn't support new filter");
+        return tl::unexpected(
+            Error(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support new filter"));
    }


    virtual tl::expected<void, Error>
    GetExtraInfoByIds(const int64_t* ids, int64_t count, char* extra_infos) const {
-        throw std::runtime_error("Index doesn't support GetExtraInfoByIds");
+        return tl::unexpected(Error(ErrorType::UNSUPPORTED_INDEX_OPERATION,
+                                    "Index does not support GetExtraInfoByIds"));
    };


    [[nodiscard]] virtual bool
    CheckFeature(IndexFeature feature) const {
-        throw std::runtime_error("Index doesn't support check feature");
+        return false;
    }


LHT129 · 2026-06-04T08:23:26Z

Thank you for the review! I have addressed the formatting issues by removing the trailing semicolons after function bodies and updated the documentation comments to reflect the new tl::unexpected-based error handling.

Regarding the LOG_ERROR_AND_RETURNS macro suggestion: The macro depends on Options::Instance().logger(), which causes linking errors in header-only default implementations. After consulting with the maintainers, we decided to use return tl::unexpected(Error(...)) directly to avoid these linking issues while still providing the expected-based error handling.

For CheckFeature: The task description indicates that methods returning bool should keep throwing exceptions, but CheckFeature is specifically a feature capability check method where returning false (indicating the feature is not supported) is more semantically correct than throwing an exception. This aligns with the common pattern in feature detection APIs.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.

    REQUIRE_THROWS(index->Add(dataset->base_));
    REQUIRE_THROWS(index->Remove(0));
-    REQUIRE_THROWS(index->CheckFeature(IndexFeature::SUPPORT_ESTIMATE_MEMORY));
+    REQUIRE_FALSE(index->CheckFeature(IndexFeature::SUPPORT_ESTIMATE_MEMORY));
    REQUIRE_THROWS(index->EstimateMemory(1000));


+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index implementation does not support this operation
     * (default behavior for base class).


+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index implementation does not support this operation
     *            (default behavior for base class).


+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index implementation does not support this operation
     *            (default behavior for base class).


+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index implementation does not support this operation
     *            (default behavior for base class).


     * 
     * @return IndexPtr A pointer to the exported model index.
-     * @throws std::runtime_error If the index does not support exporting the model.
+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index does not support exporting the model.


     * 
     * @return DatasetPtr A pointer to the exported IDs dataset.
-     * @throws std::runtime_error If the index does not support exporting the IDs.
+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index does not support exporting the IDs.


     * After setting this state, no further modifications are supported, such as no additions or deletions 
     *
-     * @throws std::runtime_error If the index does not support to set immutable
+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index does not support to set immutable


    [[nodiscard]] virtual bool
    CheckFeature(IndexFeature feature) const {
-        throw std::runtime_error("Index doesn't support check feature");
+        return false;


    virtual tl::expected<bool, Error>
    Tune(const std::string& parameters, bool disable_future_tuning = false) {
-        throw std::runtime_error("Tune is not supported by this index");
+        return tl::unexpected(
+            Error(ErrorType::UNSUPPORTED_INDEX_OPERATION, "Index does not support Tune"));
    }


Replace throw std::runtime_error with tl::unexpected(Error) in Index virtual method default implementations that return tl::expected<T, Error>. This ensures error handling is consistent with the API contract. - Use ErrorType::UNSUPPORTED_INDEX_OPERATION for all unsupported operations - Standardize error messages to "Index does not support <operation>" - Change CheckFeature default to return false instead of throwing - Update tests to use REQUIRE_FALSE(has_value()) for expected-returning methods - Remove is_old_index parameter from TestBatchCalcDistanceById Methods returning non-expected types (GetStats, GetMemoryUsageDetail, etc.) retain throw behavior as they cannot use tl::unexpected. Closes antgroup#2140 Signed-off-by: LHT129 <tianlan.lht@antgroup.com>

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 8 comments.

 const char* const HGRAPH_EXTRA_INFO_SIZE = "extra_info_size";
 const char* const HGRAPH_SUPPORT_DUPLICATE = "support_duplicate";
 const char* const HGRAPH_DUPLICATE_DISTANCE_THRESHOLD = "duplicate_distance_threshold";
+const char* const HGRAPH_SUPPORT_TOMBSTONE = "support_tomb_stone";


+    inline bool
+    RecoverRemove(LabelType label) {
+        // 1. check is removed
+        if (not use_reverse_map_) {
+            return false;
+        }
+        InnerIdType inner_id = INVALID_ID;
+        if (not label_remap_.Find(label, inner_id) or inner_id != INVALID_ID) {
+            return false;
+        }
+
+        // 2. find inner_id
+        inner_id = GetIdByLabel(label, true);
+
+        // 3. recover
+        deleted_ids_.erase(inner_id);
+        label_remap_.InsertOrAssign(label, inner_id);
+        return true;
+    }


+    inline bool
+    IsTombstoneLabel(LabelType label) {
+        if (not use_reverse_map_) {
+            return false;
+        }
+        InnerIdType inner_id = INVALID_ID;
+        if (not label_remap_.Find(label, inner_id)) {
+            return false;
+        }
+        return inner_id == INVALID_ID;
+    }


+void
+HGraph::recover_remove(int64_t id) {
+    // note:
+    // 1. this function doesn't recover entry_point and route_graphs caused by Remove()
+    // 2. use this function only when is_tombstone is checked
+
+    std::shared_lock label_lock(this->label_lookup_mutex_);
+    auto inner_id = this->label_table_->GetIdByLabel(id, true);
+    this->bottom_graph_->RecoverDeleteNeighborsById(inner_id);
+    this->label_table_->RecoverRemove(id);
+    delete_count_--;
+}


+            // recover failed: roll back
+            Remove({label});
+        } catch (std::runtime_error& e) {
+            // recover failed: roll back
+            Remove({label});
+        }


+     * [case 2] fail to recovery -> add process
+     * exist + delete + not recovery: is_label_valid = false, is_tombstone = ture, is_recovered = false
+     *
+     * [case 3] tombstone recovery -> continue
+     * exist + delete + recovery: is_label_valid = false, is_tombstone = ture, is_recovered = true


     * @return tl::expected<DatasetPtr, Error>
     *         - On success: A DatasetPtr containing the raw vector data
     *           (format depends on implementation, but typically includes vector arrays).
     *         - On failure: An error object (e.g., invalid ID, out of memory).
-     * @throws std::runtime_error If the index implementation does not support this operation
+     * @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index implementation does not support this operation
     *            (default behavior for base class).
     *


+        // try recover tombstone
+        if (this->data_type_ != DataTypes::DATA_TYPE_SPARSE) {
+            auto one_base = get_single_dataset(data, j);
+            bool is_process_finished = try_recover_tombstone(one_base, failed_ids);
+            if (is_process_finished) {


LHT129 requested review from inabao and wxyucs as code owners June 4, 2026 06:40

Copilot AI review requested due to automatic review settings June 4, 2026 06:40

LHT129 requested a review from jiaweizone as a code owner June 4, 2026 06:40

pull-request-size Bot added the size/L label Jun 4, 2026

Copilot started reviewing on behalf of LHT129 June 4, 2026 06:40 View session

LHT129 added kind/feature New feature version/1.0 labels Jun 4, 2026

mergify Bot added the module/api label Jun 4, 2026

gemini-code-assist Bot reviewed Jun 4, 2026

View reviewed changes

LHT129 self-assigned this Jun 4, 2026

LHT129 force-pushed the opencode/improve-index-default-impl branch from f3d42fb to 8efabed Compare June 4, 2026 06:45

Copilot AI reviewed Jun 4, 2026

View reviewed changes

LHT129 force-pushed the opencode/improve-index-default-impl branch 2 times, most recently from 52f7cc3 to f943474 Compare June 4, 2026 06:50

Copilot AI review requested due to automatic review settings June 4, 2026 08:03

LHT129 force-pushed the opencode/improve-index-default-impl branch from f943474 to 830d2ef Compare June 4, 2026 08:03

Copilot started reviewing on behalf of LHT129 June 4, 2026 08:03 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

LHT129 force-pushed the opencode/improve-index-default-impl branch from 830d2ef to 80b8766 Compare June 4, 2026 08:17

Copilot AI review requested due to automatic review settings June 4, 2026 09:11

Copilot started reviewing on behalf of LHT129 June 4, 2026 09:11 View session

mergify Bot added the module/testing label Jun 4, 2026

Copilot AI reviewed Jun 4, 2026

View reviewed changes

LHT129 force-pushed the opencode/improve-index-default-impl branch 2 times, most recently from af903b7 to 94d4646 Compare June 5, 2026 04:32

Copilot AI review requested due to automatic review settings June 5, 2026 06:17

LHT129 force-pushed the opencode/improve-index-default-impl branch from 94d4646 to 7c55664 Compare June 5, 2026 06:17

Copilot started reviewing on behalf of LHT129 June 5, 2026 06:17 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

		LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
		"Index does not support Tune supported by this index")

		LOG_ERROR_AND_RETURNS(ErrorType::UNSUPPORTED_INDEX_OPERATION,
		"Index does not support get distance by id")};

		@@ -692,8 +702,8 @@ class Index {
		*/
		[[nodiscard]] virtual tl::expected<DatasetPtr, Error>

		* @return tl::unexpected(ErrorType::UNSUPPORTED_INDEX_OPERATION) If the index implementation does not support this operation
		* (default behavior for base class).

Conversation

LHT129 commented Jun 4, 2026

Summary

Uh oh!

mergify Bot commented Jun 4, 2026

Merge Protections

🟢 Require kind label

🟢 Require version label

🟢 Require linked issue for feature/bug PRs

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

LHT129 commented Jun 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants