feat: Support Int8Vector in go #38990
Conversation
sre-ci-robot
added
size/XXL
|
Invalid PR Title Format Detected Your PR submission does not adhere to our required standards. To ensure clarity and consistency, please meet the following criteria:
Required Title Structure: Where Example: Please review and update your PR to comply with these guidelines. |
mergify
bot
added
kind/feature
|
@cydrain Please associate the related issue to the body of your Pull Request. (eg. “issue: #”) |
|
@cydrain E2e jenkins job failed, comment |
|
@cydrain go-sdk check failed, comment |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #38990 +/- ##
===========================================
+ Coverage 69.53% 81.07% +11.53%
===========================================
Files 296 1395 +1099
Lines 26604 197978 +171374
===========================================
+ Hits 18499 160507 +142008
- Misses 8105 31828 +23723
- Partials 0 5643 +5643
|
cydrain
force-pushed
the
caiyd_38666_support_int8_vector
branch
2 times, most recently
from
e411dfc to
f617d0a
Compare
|
@cydrain go-sdk check failed, comment |
cydrain
force-pushed
the
caiyd_38666_support_int8_vector
branch
from
f617d0a to
41fcd1b
Compare
|
@cydrain go-sdk check failed, comment |
cydrain
force-pushed
the
caiyd_38666_support_int8_vector
branch
from
41fcd1b to
58a24f8
Compare
|
@cydrain cpp-unit-test check failed, comment |
|
@cydrain go-sdk check failed, comment |
|
@cydrain E2e jenkins job failed, comment |
cydrain
force-pushed
the
caiyd_38666_support_int8_vector
branch
from
58a24f8 to
84c8e46
Compare
| } | ||
|
|
||
| inline bool | ||
| IsIntVectorMetricType(const MetricType& metric_type) { |
There was a problem hiding this comment.
It is a little bit weird to have this function.
Let's change the metrics check to vector and binary vector
There was a problem hiding this comment.
I cannot get your point.
FloatVector and Int8Vector support different metric types, so I use different APIs to handle.
| @@ -450,6 +450,29 @@ BuildSparseFloatVecIndex(CIndex index, | |||
| return status; | |||
| } | |||
|
|
|||
| CStatus | |||
| BuildInt8VecIndex(CIndex index, int64_t int8_value_num, const int8_t* vectors) { | |||
There was a problem hiding this comment.
I know we just follow the pattern. But it will be great if we can merge these Build functions, since they only have a little discrepancy.
This is a non-blocking comment
There was a problem hiding this comment.
It's because here is C code, we cannot use template here which is only supported in C++
internal/proxy/task_index.go
Outdated
| } else { | ||
| // override float/int vector index params by autoindex | ||
| for k, v := range Params.AutoIndexConfig.IndexParams.GetAsJSONMap() { | ||
| indexParamsMap[k] = v | ||
| } |
There was a problem hiding this comment.
Suggest to explicitly show the condition here to match styles.
Also report error when falls to else block.
| VectorDefaultMetricType = metric.COSINE | ||
| SparseFloatVectorDefaultMetricType = metric.IP |
There was a problem hiding this comment.
I know you are trying to find out a common type for int/float. I guess DenseNumericVector is a not bad one. You can also apply it to all other places
There was a problem hiding this comment.
Use FloatVectorDefaultMetricType and IntVectorDefaultMetricType seperately
Signed-off-by: Cai Yudong <[email protected]>
cydrain
force-pushed
the
caiyd_38666_support_int8_vector
branch
from
84c8e46 to
99e7d7f
Compare
|
@cydrain E2e jenkins job failed, comment |
|
/run-cpu-e2e |
|
@cydrain E2e jenkins job failed, comment |
|
/run-cpu-e2e |
1 similar comment
|
/run-cpu-e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cydrain, liliu-z 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:
Approvers can indicate their approval by writing |
Issue: milvus-io#38666 Signed-off-by: Cai Yudong <[email protected]>
Issue: #38666