-
Notifications
You must be signed in to change notification settings - Fork 7
feat: add vector search support #87
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
base: main
Are you sure you want to change the base?
feat: add vector search support #87
Conversation
Add vector index creation support for Android: - CollectionManagerVector.kt helper for vector indexes - Handle 'vector' case in createIndex Ref: Couchbase-Ecosystem#86
Summary of ChangesHello @Fujio-Turner, 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 integrates vector search functionality into the Highlights
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.
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 adds support for creating vector indexes on Android. The new createVectorIndex function in CollectionManagerVector.kt correctly parses the configuration map to build a VectorIndexConfiguration.
My review includes a few suggestions to improve robustness and error handling:
- Using more specific exceptions (
IllegalArgumentException) for invalid parameters. - Implementing stricter validation for configuration options like
metricandencodingto prevent silent failures from typos. - Fixing a potential runtime crash in Product Quantizer (
PQ) configuration due to an invalid default value.
Overall, the changes are a great addition. Once these suggestions are addressed, the code will be more robust.
| vectorConfig.encoding = when (encodingType) { | ||
| "none" -> VectorEncoding.none() | ||
| "SQ" -> { | ||
| // Scalar Quantizer - determine type from config or default to SQ8 | ||
| val sqType = encodingConfig["sqType"] as? String | ||
| when (sqType) { | ||
| "SQ4" -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ4) | ||
| "SQ6" -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ6) | ||
| "SQ8" -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ8) | ||
| else -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ8) | ||
| } | ||
| } | ||
| "PQ" -> { | ||
| // Product Quantizer | ||
| val subquantizers = (encodingConfig["subquantizers"] as? Number)?.toLong() ?: 0 | ||
| val bits = (encodingConfig["bits"] as? Number)?.toLong() ?: 8 | ||
| VectorEncoding.productQuantizer(subquantizers, bits) | ||
| } | ||
| else -> VectorEncoding.none() | ||
| } |
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 block has a few areas for improvement:
- Strict Validation: The
whenstatements forencodingTypeandsqTypeuse default fallbacks for unknown values, which can hide configuration errors. It's better to throw anIllegalArgumentExceptionfor invalid values. - Invalid
subquantizersdefault: For "PQ" encoding,subquantizersdefaults to0, which is an invalid value and will cause a runtime error. This parameter should be required and validated for "PQ" encoding.
Here is a suggested refactoring that addresses these points.
vectorConfig.encoding = when (encodingType) {
"none", null -> VectorEncoding.none()
"SQ" -> {
// Scalar Quantizer - determine type from config or default to SQ8
val sqType = encodingConfig["sqType"] as? String
when (sqType) {
"SQ4" -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ4)
"SQ6" -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ6)
"SQ8", null -> VectorEncoding.scalarQuantizer(ScalarQuantizerType.SQ8)
else -> throw IllegalArgumentException("Unknown Scalar Quantizer type: $sqType")
}
}
"PQ" -> {
// Product Quantizer
val subquantizers = (encodingConfig["subquantizers"] as? Number)?.toLong()
?: throw IllegalArgumentException("Product Quantizer encoding requires 'subquantizers' parameter")
if (subquantizers <= 0) {
throw IllegalArgumentException("'subquantizers' must be a positive number")
}
val bits = (encodingConfig["bits"] as? Number)?.toLong() ?: 8
VectorEncoding.productQuantizer(subquantizers, bits)
}
else -> throw IllegalArgumentException("Unknown vector encoding type: $encodingType")
}| val expression = indexConfig["expression"] as? String | ||
| ?: throw Exception("Vector index requires 'expression' parameter") | ||
|
|
||
| val dimensions = (indexConfig["dimensions"] as? Number)?.toLong() | ||
| ?: throw Exception("Vector index requires 'dimensions' parameter") | ||
|
|
||
| val centroids = (indexConfig["centroids"] as? Number)?.toLong() | ||
| ?: throw Exception("Vector index requires 'centroids' parameter") |
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 error handling and clarity, it's recommended to throw a more specific exception like IllegalArgumentException instead of the generic Exception when a required parameter is missing. This helps callers to better understand and handle the error. Please apply this to all three required parameter checks (expression, dimensions, centroids) and update the function's KDoc @throws tag accordingly.
| val expression = indexConfig["expression"] as? String | |
| ?: throw Exception("Vector index requires 'expression' parameter") | |
| val dimensions = (indexConfig["dimensions"] as? Number)?.toLong() | |
| ?: throw Exception("Vector index requires 'dimensions' parameter") | |
| val centroids = (indexConfig["centroids"] as? Number)?.toLong() | |
| ?: throw Exception("Vector index requires 'centroids' parameter") | |
| val expression = indexConfig["expression"] as? String | |
| ?: throw IllegalArgumentException("Vector index requires 'expression' parameter") | |
| val dimensions = (indexConfig["dimensions"] as? Number)?.toLong() | |
| ?: throw IllegalArgumentException("Vector index requires 'dimensions' parameter") | |
| val centroids = (indexConfig["centroids"] as? Number)?.toLong() | |
| ?: throw IllegalArgumentException("Vector index requires 'centroids' parameter") |
| vectorConfig.metric = when (metricStr) { | ||
| "cosine" -> DistanceMetric.COSINE | ||
| "euclidean" -> DistanceMetric.EUCLIDEAN | ||
| "euclideanSquared" -> DistanceMetric.EUCLIDEAN_SQUARED | ||
| "dot" -> DistanceMetric.DOT | ||
| else -> DistanceMetric.EUCLIDEAN_SQUARED // Default | ||
| } |
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.
The when statement for metric has a default case that silently uses DistanceMetric.EUCLIDEAN_SQUARED for any unrecognized string. This can hide configuration mistakes (e.g., a typo in the metric name). It would be safer to throw an IllegalArgumentException for unknown values to provide clear feedback to the developer.
| vectorConfig.metric = when (metricStr) { | |
| "cosine" -> DistanceMetric.COSINE | |
| "euclidean" -> DistanceMetric.EUCLIDEAN | |
| "euclideanSquared" -> DistanceMetric.EUCLIDEAN_SQUARED | |
| "dot" -> DistanceMetric.DOT | |
| else -> DistanceMetric.EUCLIDEAN_SQUARED // Default | |
| } | |
| vectorConfig.metric = when (metricStr) { | |
| "cosine" -> DistanceMetric.COSINE | |
| "euclidean" -> DistanceMetric.EUCLIDEAN | |
| "euclideanSquared" -> DistanceMetric.EUCLIDEAN_SQUARED | |
| "dot" -> DistanceMetric.DOT | |
| else -> throw IllegalArgumentException("Unknown distance metric: $metricStr") | |
| } |
Summary
Add vector search support to cbl-reactnative, enabling
APPROX_VECTOR_DISTANCE()queries for similarity search.Changes
Android:
android/src/main/java/com/cblreactnative/CollectionManagerVector.kt- Vector index creation helperFeatures
VectorIndexConfigurationRelated PRs
This PR depends on:
Usage
Closes #86