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.

Sign up for GitHub

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

Jump to bottom

Implement "SearchByPks" path in Search (work in progress) #25882

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Implement "SearchByPks" path in Search (work in progress) #25882

merged 1 commit into from
Oct 16, 2023

Conversation

unfode
Copy link
Contributor

@unfode unfode commented Jul 24, 2023 •
edited
Loading

This fixes issue #23184

@sre-ci-robot
Copy link
Contributor

Welcome @unfode! It looks like this is your first PR to milvus-io/milvus 🎉

@sre-ci-robot sre-ci-robot added area/dependency Pull requests that update a dependency file size/L Denotes a PR that changes 100-499 lines. labels Jul 24, 2023
@mergify mergify bot added the needs-dco DCO is missing in this pull request. label Jul 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 24, 2023

@unfode Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco.

@mergify
Copy link
Contributor

mergify bot commented Jul 24, 2023

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

@mergify
Copy link
Contributor

mergify bot commented Jul 24, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

yah01
yah01 suggested changes Jul 25, 2023
View reviewed changes
internal/proxy/impl.go Outdated Show resolved Hide resolved
internal/proxy/impl.go Outdated Show resolved Hide resolved
internal/proxy/impl.go Outdated Show resolved Hide resolved
@mergify
Copy link
Contributor

mergify bot commented Jul 25, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Jul 25, 2023

@unfode, please be sure the pr should only have one commit, check https://github.com/milvus-io/milvus/blob/master/CODE_REVIEW.md for more details.

@mergify mergify bot added the needs-rebase label Jul 25, 2023
@mergify
Copy link
Contributor

mergify bot commented Jul 25, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Aug 14, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Sep 4, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

yah01
yah01 suggested changes Sep 7, 2023
View reviewed changes
Copy link
Member

@yah01 yah01 left a comment

Choose a reason for hiding this comment

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

good contribution as you are on the right way, some code could be simpilified

internal/proxy/impl.go Show resolved Hide resolved
internal/proxy/impl.go Outdated Show resolved Hide resolved
internal/proxy/impl.go Outdated Show resolved Hide resolved
internal/proxy/impl.go Show resolved Hide resolved
@yah01
Copy link
Member

yah01 commented Sep 7, 2023

Milvus uses merr pkg to handle error, and this provides methods to convert error to Status, and vice versa, like:

err := merr.Error(resp.GetStatus())
status := merr.Status(err)

@unfode unfode force-pushed the issue#23184 branch 2 times, most recently from 53db458 to fe12cf8 Compare September 7, 2023 20:33
@mergify mergify bot added dco-passed DCO check passed. and removed needs-dco DCO is missing in this pull request. needs-rebase labels Sep 7, 2023
yah01
yah01 reviewed Sep 8, 2023
View reviewed changes
Copy link
Member

@yah01 yah01 left a comment

Choose a reason for hiding this comment

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

/lgtm

@sre-ci-robot sre-ci-robot added size/XL Denotes a PR that changes 500-999 lines. and removed lgtm size/L Denotes a PR that changes 100-499 lines. labels Sep 27, 2023
@mergify mergify bot removed the ci-passed label Sep 27, 2023
@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@unfode ut workflow job failed, comment rerun ut can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@unfode ut workflow job failed, comment rerun ut can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

yah01
yah01 suggested changes Sep 27, 2023
View reviewed changes
internal/proxy/impl.go Outdated
}

if len(placeholderGroup.Placeholders) != 1 || len(placeholderGroup.Placeholders[0].Values) != 1 {
return nil, errors.New("PlaceholderGroup is invalid")
Copy link
Member

Choose a reason for hiding this comment

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

return `merr.WrapParameterInvalidMsg("please provide primary key")

internal/proxy/impl.go Outdated

if queryResults.FieldsData == nil ||
len(queryResults.FieldsData) != 2 ||
queryResults.FieldsData[0].FieldName != annsField && queryResults.FieldsData[1].FieldName != annsField {
Copy link
Member

Choose a reason for hiding this comment

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

these checks could be removed, and looking up the vector field could be done by iterating queryResults.GetFieldsData().

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify
Copy link
Contributor

mergify bot commented Sep 27, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@yah01
Copy link
Member

yah01 commented Sep 28, 2023

/run-cpu-e2e

1 similar comment
@yah01
Copy link
Member

yah01 commented Sep 29, 2023

/run-cpu-e2e

@mergify
Copy link
Contributor

mergify bot commented Sep 29, 2023

@unfode E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@mergify mergify bot added the ci-passed label Oct 16, 2023
@xiaofan-luan
Copy link
Collaborator

/approve

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: unfode, xiaofan-luan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@xiaofan-luan
Copy link
Collaborator

pre allocated array may help a little bit on the performance

@xiaofan-luan
Copy link
Collaborator

xiaofan-luan commented Oct 16, 2023 •
edited
Loading

for example
"
func flattenedByteVectorsToByteVectors(flattenedVectors []byte, dimension int) [][]byte {
// Preallocate the slice to the necessary size
numVectors := len(flattenedVectors) / dimension
result := make([][]byte, numVectors)
for i := 0; i < numVectors; i++ {
result[i] = flattenedVectors[i*dimension : (i+1)*dimension]
}
return result
}"

@yah01
Copy link
Member

yah01 commented Oct 16, 2023

/lgtm
/cc @unfode we need a check to guarantee the query expression is "equal to primary key(s)", let's do this in next PR, also the performance improvements in @xiaofan-luan's comment above.

@sre-ci-robot sre-ci-robot merged commit 599012a into milvus-io:master Oct 16, 2023
12 checks passed
@unfode unfode mentioned this pull request Oct 17, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yah01 yah01 yah01 requested changes

@congqixia congqixia Awaiting requested review from congqixia

@xiaocai2333 xiaocai2333 Awaiting requested review from xiaocai2333

@xige-16 xige-16 Awaiting requested review from xige-16

@xiaofan-luan xiaofan-luan Awaiting requested review from xiaofan-luan

Assignees

@yah01 yah01

Labels
approved area/dependency Pull requests that update a dependency file ci-passed dco-passed DCO check passed. lgtm size/XL Denotes a PR that changes 500-999 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@unfode @sre-ci-robot @yah01 @xiaofan-luan