feat:support columnar search result to better performance#3214
feat:support columnar search result to better performance#3214jac0626 wants to merge 3 commits intomilvus-io:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jac0626 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello @jac0626, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant performance enhancement for search results by implementing a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces ColumnarSearchResult as a performance-optimized, drop-in replacement for SearchResult, focusing on lazy data access and reduced object creation. The implementation is well-done and includes an extensive and thorough test suite, which is excellent for ensuring compatibility and correctness. My review includes a few suggestions for improvement, mainly concerning a potential Liskov Substitution Principle violation in an accessor class, an opportunity to enhance performance when handling dynamic fields, and a note on the use of contextlib.suppress which could mask underlying issues.
552ab0b to
1fe4346
Compare
76c71ab to
da00c8a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3214 +/- ##
==========================================
+ Coverage 76.06% 76.27% +0.21%
==========================================
Files 62 63 +1
Lines 13018 13559 +541
==========================================
+ Hits 9902 10342 +440
- Misses 3116 3217 +101 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
More work is needed in the future to improve the code's readability, maintainability, extensibility, and performance @jac0626
|
|
#3208 made some changes in search_result, might need extra attention. But lets not hurry into this new feature, I believe we can do some small changes that we missed on the perf test, mainly engineering improvements. Let's not skip the engineering improvements, that's more like to be released quickly and harmlessly. And for a new feature like this one, we need a design not a decision. In the design, I'd like some of these questions answered:
Anyway, let's discuss designs based on the issue #3213, and then implement based on the final design. For compatibility test(if we're choosing this way): the new code should pass OLD ut, new ut doesn't prove anything. |
I will try to do engineering improvements firstly, then I would upload a design doc soon. |
32e8221 to
1a3c4a2
Compare
Signed-off-by: silas.jiang <[email protected]>
1a3c4a2 to
86573ae
Compare
Signed-off-by: silas.jiang <[email protected]>
Signed-off-by: silas.jiang <[email protected]>
e7db13d to
ad267f8
Compare
@XuanYang-cn Thanks for the feedback! Regarding #3208: Already addressed — the columnar implementation has been updated to accommodate those changes. On engineering improvements: Done.see #3240, I have investigating some other improvements, but just make little sense. On design: I've prepared a design doc: To answer your specific questions:
On testing: Let me know if you'd like me to update anything in the design doc! |
see #3213