expected support error message

[Presburger]

issue: #971

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Presburger

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Presburger]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mergify]

@Presburger Please associate the related issue to the body of your Pull Request. (eg. “issue: #”)

[PwzXxm]

Is it going to be another PR for hooking msg?

[zhengbuqian]

consider creating a temp string for both LOG_KNOWHERE_ERROR_ and assigning to err_msg? may be cleaner if we avoid repeating the formatting code twice.

[Presburger]

good advice, but the code maybe looks weird.

[zhengbuqian]

IMO the below looks slightly better? Unless we have a good reason to have different messages for the logging/exception. Feel free to ignore this comment if you insist.

auto msg = std::string("param ") + it.first + " out of range " + "[ " +
                std::to_string(ptr->range.value().first) + "," +
                std::to_string(ptr->range.value().second) + " ]";
LOG_KNOWHERE_ERROR_ << msg;
if (err_msg) {
    *err_msg = msg;
}

[zhengbuqian]

I feel it would be better if the message is attached to a non success Status instead of an excepted: we may have some error info to be returned even if the current method returns only Status.

But now our Status is an enum class and cannot be extended. Would be a large change if we want to do that.

See absl::Status for reference.

[zhengbuqian]

In your proposed change to pass err_msg to the returned value of Search the code looks like this, which is pretty clumsy:

expected<DataSetPtr> Search() const {
    auto cfg = this->node->CreateConfig();
    std::string err;
    auto status = LoadConfig(cfg.get(), json, knowhere::SEARCH, "Search", &err);
    if (err != Status::success) {
        expected<DataSetPtr> ret(err);
        ret << err;
        return ret;
    }
    ...
}

Status
LoadConfig(BaseConfig* cfg, const Json& json, knowhere::PARAM_TYPE param_type, const std::string& method, std::string* const err = nullptr) const {
    Json json_(json);
    auto res = Config::FormatAndCheck(*cfg, json_, err);
    LOG_KNOWHERE_DEBUG_ << method << " config dump: " << json_.dump();
    RETURN_IF_ERROR(res);
    return Config::Load(*cfg, json_, param_type, err);
}

[Presburger]

change the return status, Leads to huge changes.
err_msg, Compatible with old code.
I think just add err_msg for search api.

[zhengbuqian]

/lgtm

with a minor comment

[liliu-z]

issue: milvus-io/milvus#25079