-
-
Notifications
You must be signed in to change notification settings - Fork 87
fix: support List<Float> in vector index #3090
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.
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
fix: support List<Float> in vector index #3090
Conversation
Summary of ChangesHello @szekelyszabi, 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 resolves an issue where vectors inserted into the database via SQL using 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
|
🧪 CI InsightsHere's what we observed from your CI run for 821bcdc. 🟢 All jobs passed!But CI Insights is watching 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds support for List<Float> as a vector type, which is a necessary fix for vectors inserted via SQL. The changes in ArcadePageVectorValues and LSMVectorIndex are appropriate to handle this new type.
My main feedback is regarding code duplication. The logic to convert a List to a float[] has been added to both files. This should be extracted into a single, reusable utility method to improve maintainability and ensure consistency.
I've also pointed out a minor style issue regarding missing braces in for loops, which could be improved for better code clarity and safety.
| for (int i = 0; i < list.size(); i++) | ||
| vector[i] = ((Number) list.get(i)).floatValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better readability and to prevent potential issues when adding more statements to the loop, it's good practice to always use braces {} for loop bodies.
Additionally, this logic for converting a List to a float[] is duplicated in LSMVectorIndex.java. It would be best to extract this into a shared static helper method to improve maintainability.
| for (int i = 0; i < list.size(); i++) | |
| vector[i] = ((Number) list.get(i)).floatValue(); | |
| for (int i = 0; i < list.size(); i++) { | |
| vector[i] = ((Number) list.get(i)).floatValue(); | |
| } |
| for (int i = 0; i < list.size(); i++) | ||
| vector[i] = ((Number) list.get(i)).floatValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same duplicated logic as in ArcadePageVectorValues.java. As mentioned in the other comment, this should be extracted to a shared helper method.
For consistency and to improve readability, please add braces {} to the for loop body.
for (int i = 0; i < list.size(); i++) {
vector[i] = ((Number) list.get(i)).floatValue();
}|
It makes sense, thanks! We should explore also using float[] natively in SQL to reduce conversion from/to List and the heavy weight of List instead of float[] in RAM |
When vectors are inserted via SQL using [0.1, 0.2, ...] syntax, they are stored as ArrayList in documents. The graph building code only accepted float[] type, causing vectorNeighbors() to return empty results.
This fix adds List<?> handling in:
Fixes the issue where vectorNeighbors() returns empty array for SQL-inserted vectors while vectorDistance() works correctly.