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

move common params check to class IndexCommonParam #109

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

LHT129
Copy link
Collaborator

@LHT129 LHT129 commented Nov 1, 2024

  • common params(dim,metric_type,data_type...) should be checked for all index
  • add param value's datatype check for hnsw and common params

@LHT129 LHT129 added the kind/improvement Code improvements (variable/function renaming, refactoring, etc. ) label Nov 1, 2024
@LHT129 LHT129 requested review from wxyucs and inabao November 1, 2024 04:01
@LHT129 LHT129 self-assigned this Nov 1, 2024
@LHT129 LHT129 requested a review from jiaweizone as a code owner November 1, 2024 04:01
@LHT129 LHT129 force-pushed the param_check branch 2 times, most recently from 0e12c80 to 9536ab5 Compare November 1, 2024 04:38
- common params(dim,metric_type,data_type...) should be checked for all
  index
- add param value's datatype check for hnsw and common params

Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
CHECK_ARGUMENT(metric_obj.is_string(),
fmt::format("parameters[{}] must string type", PARAMETER_METRIC_TYPE));
std::string metric = metric_obj;
if (metric == METRIC_L2) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps we don't need to require strict string equality here; we can ignore case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can discuss the strict equality on another pull request.

Copy link
Collaborator

@inabao inabao left a comment

Choose a reason for hiding this comment

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

LGTM

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

@LHT129 LHT129 merged commit a6bf150 into antgroup:main Nov 4, 2024
2 checks passed
@LHT129 LHT129 deleted the param_check branch January 17, 2025 06:35
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/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants