fix(analyzer): add nullptr check and copy query vectors in HGraphAnalyzer #1528
Conversation
Signed-off-by: baoyuan <[email protected]>
misaka0714
requested review from
LHT129,
ShawnShawnYou,
inabao,
jiaweizone and
wxyucs
as code owners
Summary of ChangesHello @misaka0714, 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 enhances the stability and reliability of the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly identifies and fixes a critical issue in HGraphAnalyzer::SetQuery where query vectors were not being copied, and it also adds a necessary nullptr check to prevent potential crashes. The error handling is improved by returning early if the query dataset is invalid. I have one suggestion to further refine the implementation using a C++17 feature for better readability and to avoid a redundant function call. The other change is a minor formatting fix, which is also good.
| if (query->GetFloat32Vectors() != nullptr) { | ||
| std::memcpy(query_sample_datas_.data(), | ||
| query->GetFloat32Vectors(), | ||
| query_sample_size_ * dim_ * sizeof(float)); | ||
| } else { | ||
| logger::error("Query dataset is empty or not float32"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
To improve readability and avoid calling query->GetFloat32Vectors() twice, you can use an if statement with an initializer (a C++17 feature). This makes the code more concise and efficient.
| if (query->GetFloat32Vectors() != nullptr) { | |
| std::memcpy(query_sample_datas_.data(), | |
| query->GetFloat32Vectors(), | |
| query_sample_size_ * dim_ * sizeof(float)); | |
| } else { | |
| logger::error("Query dataset is empty or not float32"); | |
| return false; | |
| } | |
| if (const float* query_vectors = query->GetFloat32Vectors()) { | |
| std::memcpy(query_sample_datas_.data(), | |
| query_vectors, | |
| query_sample_size_ * dim_ * sizeof(float)); | |
| } else { | |
| logger::error("Query dataset is empty or not float32"); | |
| return false; | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1528 +/- ##
==========================================
+ Coverage 91.13% 91.38% +0.24%
==========================================
Files 327 327
Lines 19146 19146
==========================================
+ Hits 17449 17496 +47
+ Misses 1697 1650 -47
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
This pull request improves error handling in the
HGraphAnalyzer::SetQuerymethod by adding a check to ensure the query dataset contains valid float32 vectors before copying data. If the dataset is empty or not of the expected type, an error is logged and the function returnsfalseearly.Error handling improvements:
query->GetFloat32Vectors()is notnullptrbefore copying data; logs an error and returnsfalseif the dataset is empty or not float32.