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

enhance: retrieve output fields after local reduce #32346

Merged
merged 19 commits into from
Apr 25, 2024
Merged

enhance: retrieve output fields after local reduce #32346

merged 19 commits into from
Apr 25, 2024

Conversation

longjiquan
Copy link
Contributor

issue: #31822

@longjiquan
api: retrieve by offsets
f82a164 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
reduce io
4e30f80 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
fix retrive by offsets
58d348d 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
fix memory leak
175052e 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
concurrent retrieval
cd0209d 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
api: plan.ShouldIgnoreNonPk
5bacc65 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
fix conflicts
88eb602 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
format code
ffe2b3d 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@sre-ci-robot sre-ci-robot added the size/XL Denotes a PR that changes 500-999 lines. label Apr 17, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement labels Apr 17, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 17, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 17, 2024

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

@longjiquan
fix ids not returned in fields data
68d01e9 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 17, 2024

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

@longjiquan
fix code checker
5c20850 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 17, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 18, 2024

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

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 18, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@longjiquan
fix ut
368db0b 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
Merge branch 'master' of github.com:milvus-io/milvus into opt-local-r…
e2b2dc1 
…educe-pr
@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 19, 2024

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

bigsheeper
bigsheeper reviewed Apr 19, 2024
View reviewed changes
pkg/util/typeutil/schema.go Outdated
@@ -568,6 +568,121 @@ func PrepareResultFieldData(sample []*schemapb.FieldData, topK int64) []*schemap
return result
}

func ReserveSize(dst, src *schemapb.FieldData, size int64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

no usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll delete this function.

@codecov Codecov
Copy link

codecov bot commented Apr 22, 2024 •
edited

Codecov Report

Attention: Patch coverage is 43.88889% with 101 lines in your changes are missing coverage. Please review.

Project coverage is 81.73%. Comparing base (72b054e) to head (9aec090).
Report is 19 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #32346      +/-   ##
==========================================
- Coverage   81.80%   81.73%   -0.08%     
==========================================
  Files         999      990       -9     
  Lines      124240   124558     +318     
==========================================
+ Hits       101631   101803     +172     
- Misses      18734    18883     +149     
+ Partials     3875     3872       -3
Files Coverage Δ
internal/core/src/common/Tracer.h 100.00% <100.00%> (ø)
internal/core/src/segcore/SegmentInterface.h 100.00% <ø> (ø)
internal/querynodev2/segments/count_reducer.go 100.00% <100.00%> (ø)
...rnal/querynodev2/segments/default_limit_reducer.go 100.00% <100.00%> (ø)
internal/querynodev2/segments/reducer.go 100.00% <ø> (ø)
internal/querynodev2/tasks/query_task.go 93.18% <100.00%> (ø)
internal/querynodev2/segments/retrieve.go 84.87% <93.33%> (+0.94%) ⬆️
internal/querynodev2/segments/plan.go 75.53% <0.00%> (-1.65%) ⬇️
internal/querynodev2/segments/segment_l0.go 68.57% <0.00%> (-2.02%) ⬇️
internal/core/src/segcore/SegmentInterface.cpp 88.57% <77.27%> (-3.26%) ⬇️
... and 4 more

... and 218 files with indirect coverage changes

czs007
czs007 reviewed Apr 23, 2024
View reviewed changes
internal/querynodev2/segments/result.go Outdated
}

// limit retrieve result to avoid oom
if retSize > maxOutputSize {
Copy link
Contributor

Choose a reason for hiding this comment

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

revert this change

czs007
czs007 reviewed Apr 23, 2024
View reviewed changes
internal/querynodev2/segments/result.go Outdated
continue
}
idx, theOffsets := i, offsets
eg.Go(func() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is advisable to avoid creating many goroutines here.

czs007
czs007 reviewed Apr 23, 2024
View reviewed changes
internal/core/src/segcore/segment_c.cpp Outdated
segment->Retrieve(plan, timestamp, limit_size, ignore_non_pk);

auto size = retrieve_result->ByteSizeLong();
void* buffer = malloc(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

make use of RAII to ensure exception safety. you can modify this in another pr.

@longjiquan
fix comments
5e50e7e 
Signed-off-by: longjiquan <jiquan.long@zilliz.com>
@longjiquan
Merge branch 'master' of github.com:milvus-io/milvus into opt-local-r…
9aec090 
…educe-pr
@sre-ci-robot sre-ci-robot removed the lgtm label Apr 23, 2024
@longjiquan
Copy link
Contributor Author

comments fixed at 5e50e7e, cc @czs007

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 23, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 23, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 23, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 23, 2024

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

@czs007
Copy link
Contributor

czs007 commented Apr 23, 2024

/approve
/lgtm

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: czs007, longjiquan

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 24, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 24, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 24, 2024

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

@longjiquan
Copy link
Contributor Author

/run-cpu-e2e

@sre-ci-robot sre-ci-robot merged commit c002745 into milvus-io:master Apr 25, 2024
14 of 15 checks passed
@longjiquan longjiquan deleted the opt-local-reduce-pr branch April 25, 2024 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bigsheeper bigsheeper bigsheeper left review comments

@czs007 czs007 czs007 left review comments

@congqixia congqixia Awaiting requested review from congqixia

Assignees

@bigsheeper bigsheeper

@czs007 czs007

Labels
approved ci-passed dco-passed DCO check passed. kind/enhancement Issues or changes related to enhancement lgtm size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@longjiquan @bigsheeper @czs007 @sre-ci-robot